| Message ID | 1487011784-24966-1-git-send-email-festevam@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
Hi Fabio, All, On Mon, Feb 13, 2017 at 7:49 PM, Fabio Estevam <festevam@gmail.com> wrote: > > Add support for kmscube application, which is helpful for testing > kms/drm drivers. > > Signed-off-by: Fabio Estevam <festevam@gmail.com> Reviewed-by: Gary Bisson <gary.bisson@boundarydevices.com> Tested-by: Gary Bisson <gary.bisson@boundarydevices.com> Tested on a i.MX6Q Nitrogen6x platform (drm-imx, etnaviv). Regards, Gary
Hello, On Mon, 13 Feb 2017 16:49:44 -0200, Fabio Estevam wrote: > Add support for kmscube application, which is helpful for testing > kms/drm drivers. > > Signed-off-by: Fabio Estevam <festevam@gmail.com> Thanks for this patch. I have a few comments, see below. > diff --git a/package/kmscube/Config.in b/package/kmscube/Config.in > new file mode 100644 > index 0000000..b3c3c8d > --- /dev/null > +++ b/package/kmscube/Config.in > @@ -0,0 +1,5 @@ > +config BR2_PACKAGE_KMSCUBE > + bool "kmscube" > + depends on BR2_PACKAGE_MESA3D && BR2_PACKAGE_LIBDRM kmscube apparently wants EGL and OpenGL ES: PKG_CHECK_MODULES(EGL, egl) PKG_CHECK_MODULES(GLES2, glesv2) so probably this needs to be taken into account here by using the BR2_PACKAGE_MESA3D_OPENGL_EGL and BR2_PACKAGE_MESA3D_OPENGL_ES symbols. > + help > + kmscube is an application to test kms/drm drivers. Trailing space at the end of the line + missing upstream project URL. > diff --git a/package/kmscube/kmscube.mk b/package/kmscube/kmscube.mk > new file mode 100644 > index 0000000..267fffa > --- /dev/null > +++ b/package/kmscube/kmscube.mk > @@ -0,0 +1,14 @@ Missing usual comment header. > +KMSCUBE_VERSION = 8c6a20901f95e1b465bbca127f9d47fcfb8762e6 > +KMSCUBE_SITE = $(call github,robclark,kmscube,$(KMSCUBE_VERSION)) > +KMSCUBE_DEPENDENCIES = host-pkgconf libdrm mesa3d > +KMSCUBE_INSTALL_TARGET = YES This is useless, as it is the default. > +KMSCUBE_AUTORECONF = YES > +KMSCUBE_INSTALL_STAGING = Y This line doesn't do anything because 'Y' is not a valid value. And anyway, there's most likely no point in installing kmscube to staging. License information is also missing, and you forgot to add yourself to the DEVELOPERS file (should be done in the same patch). Could you address those issues and submit an updated version? Thanks a lot! Thomas
Hi Thomas, Thanks for your review. On Mon, Feb 13, 2017 at 7:31 PM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: >> diff --git a/package/kmscube/Config.in b/package/kmscube/Config.in >> new file mode 100644 >> index 0000000..b3c3c8d >> --- /dev/null >> +++ b/package/kmscube/Config.in >> @@ -0,0 +1,5 @@ >> +config BR2_PACKAGE_KMSCUBE >> + bool "kmscube" >> + depends on BR2_PACKAGE_MESA3D && BR2_PACKAGE_LIBDRM > > kmscube apparently wants EGL and OpenGL ES: > > PKG_CHECK_MODULES(EGL, egl) > PKG_CHECK_MODULES(GLES2, glesv2) > > so probably this needs to be taken into account here by using the > BR2_PACKAGE_MESA3D_OPENGL_EGL and BR2_PACKAGE_MESA3D_OPENGL_ES symbols. When building kmscube via Buildroot I see: checking for DRM... yes checking for GBM... yes checking for EGL... yes checking for GLES2... yes BR2_PACKAGE_MESA3D_OPENGL_EGL is selected by the Etnaviv mesa driver: BR2_PACKAGE_MESA3D_GALLIUM_DRIVER_ETNAVIV, which comes from a patch I sent today. I don't have BR2_PACKAGE_MESA3D_OPENGL_ES option selected, but I do get "checking for GLES2... yes", so I am bit confused. Is your suggestion to do something like this? config BR2_PACKAGE_KMSCUBE bool "kmscube" depends on BR2_PACKAGE_MESA3D && BR2_PACKAGE_LIBDRM depends on BR2_PACKAGE_MESA3D_OPENGL_EGL select BR2_PACKAGE_MESA3D_OPENGL_ES > License information is also missing, and you forgot to add yourself to > the DEVELOPERS file (should be done in the same patch). It is not clear by looking at the header files which license this project uses. How should I do in a case like this? Thanks
On Mon, Feb 13, 2017 at 8:03 PM, Fabio Estevam <festevam@gmail.com> wrote: > It is not clear by looking at the header files which license this > project uses. How should I do in a case like this? It is MIT license. Now my only question is what should I do with regards to BR2_PACKAGE_MESA3D_OPENGL_EGL and BR2_PACKAGE_MESA3D_OPENGL_ES symbols. Thanks
Hello, On Mon, 13 Feb 2017 20:03:29 -0200, Fabio Estevam wrote: > > PKG_CHECK_MODULES(EGL, egl) > > PKG_CHECK_MODULES(GLES2, glesv2) This code means that kmscube will only build fine if the pkg-config files egl.pc and glesv2.pc are available. > checking for DRM... yes > checking for GBM... yes > checking for EGL... yes > checking for GLES2... yes > > BR2_PACKAGE_MESA3D_OPENGL_EGL is selected by the Etnaviv mesa driver: > BR2_PACKAGE_MESA3D_GALLIUM_DRIVER_ETNAVIV, which comes from a patch I > sent today. > > I don't have BR2_PACKAGE_MESA3D_OPENGL_ES option selected, but I do > get "checking for GLES2... yes", so I am bit confused. > > Is your suggestion to do something like this? > > config BR2_PACKAGE_KMSCUBE > bool "kmscube" > depends on BR2_PACKAGE_MESA3D && BR2_PACKAGE_LIBDRM > depends on BR2_PACKAGE_MESA3D_OPENGL_EGL > select BR2_PACKAGE_MESA3D_OPENGL_ES I would do just: depends on BR2_PACKAGE_MESA3D_OPENGL_EGL depends on BR2_PACKAGE_MESA3D_OPENGL_ES that is sufficient, and it's what glmark2/Config.in is doing (except glmark2 is a bit more complicated since it also supports full OpenGL, not only OpenGL ES). The "depends on BR2_PACKAGE_MESA3D && BR2_PACKAGE_LIBDRM" are useless, because if BR2_PACKAGE_MESA3D_OPENGL_EGL and BR2_PACKAGE_MESA3D_OPENGL_ES are enabled, then surely mesa3d is enabled. And mesa3d selects libdrm, so we're good. Best regards, Thomas
Hi, On Tue, Feb 14, 2017 at 09:18:03AM +0100, Thomas Petazzoni wrote: > Hello, > > On Mon, 13 Feb 2017 20:03:29 -0200, Fabio Estevam wrote: > > > > PKG_CHECK_MODULES(EGL, egl) > > > PKG_CHECK_MODULES(GLES2, glesv2) > > This code means that kmscube will only build fine if the pkg-config > files egl.pc and glesv2.pc are available. > > > checking for DRM... yes > > checking for GBM... yes > > checking for EGL... yes > > checking for GLES2... yes > > > > BR2_PACKAGE_MESA3D_OPENGL_EGL is selected by the Etnaviv mesa driver: > > BR2_PACKAGE_MESA3D_GALLIUM_DRIVER_ETNAVIV, which comes from a patch I > > sent today. > > > > I don't have BR2_PACKAGE_MESA3D_OPENGL_ES option selected, but I do > > get "checking for GLES2... yes", so I am bit confused. > > > > Is your suggestion to do something like this? > > > > config BR2_PACKAGE_KMSCUBE > > bool "kmscube" > > depends on BR2_PACKAGE_MESA3D && BR2_PACKAGE_LIBDRM > > depends on BR2_PACKAGE_MESA3D_OPENGL_EGL > > select BR2_PACKAGE_MESA3D_OPENGL_ES > > I would do just: > > depends on BR2_PACKAGE_MESA3D_OPENGL_EGL > depends on BR2_PACKAGE_MESA3D_OPENGL_ES Actually, it doesn't depend on a specific GLES / EGL implementation, but it does depend on Mesa for GBM. There are alternative implementation of GBM as well, so having something like: depends on BR2_PACKAGE_HAS_LIBEGL depends on BR2_PACKAGE_HAS_LIBGLES depends on BR2_PACKAGE_MESA3D # GBM implementation Seems more natural Maxime
Hello, On Tue, 14 Feb 2017 10:24:00 +0100, Maxime Ripard wrote: > Actually, it doesn't depend on a specific GLES / EGL implementation, > but it does depend on Mesa for GBM. There are alternative > implementation of GBM as well, so having something like: > > depends on BR2_PACKAGE_HAS_LIBEGL > depends on BR2_PACKAGE_HAS_LIBGLES > depends on BR2_PACKAGE_MESA3D # GBM implementation > > Seems more natural In principle, I would agree, but we pass --enable-gbm only when BR2_PACKAGE_MESA3D_OPENGL_EGL=y. So with your proposal, we could end up in a situation where libgbm isn't. Try a build with just BR2_PACKAGE_MESA3D=y, libgm isn't built/installed. So if we want to do this, perhaps we need a separate BR2_PACKAGE_MESA3D_LIBGBM option? Thomas
Hi Thomas, On Tue, Feb 14, 2017 at 6:18 AM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > I would do just: > > depends on BR2_PACKAGE_MESA3D_OPENGL_EGL > depends on BR2_PACKAGE_MESA3D_OPENGL_ES Got it. I can do like this. What about KMSCUBE_DEPENDENCIES? Currently I have: KMSCUBE_DEPENDENCIES = host-pkgconf libdrm mesa3d Should this be adjusted to something different? Thanks
On Tue, Feb 14, 2017 at 11:23:49AM +0100, Thomas Petazzoni wrote: > Hello, > > On Tue, 14 Feb 2017 10:24:00 +0100, Maxime Ripard wrote: > > > Actually, it doesn't depend on a specific GLES / EGL implementation, > > but it does depend on Mesa for GBM. There are alternative > > implementation of GBM as well, so having something like: > > > > depends on BR2_PACKAGE_HAS_LIBEGL > > depends on BR2_PACKAGE_HAS_LIBGLES > > depends on BR2_PACKAGE_MESA3D # GBM implementation > > > > Seems more natural > > In principle, I would agree, but we pass --enable-gbm only when > BR2_PACKAGE_MESA3D_OPENGL_EGL=y. So with your proposal, we could end up > in a situation where libgbm isn't. He, I thought this was a bad choice and why I said like :) Obviously, this isn't a good candidate. > Try a build with just BR2_PACKAGE_MESA3D=y, libgm isn't > built/installed. So if we want to do this, perhaps we need a separate > BR2_PACKAGE_MESA3D_LIBGBM option? Maybe, we could also just depend on BR2_PACKAGE_MESA3D_OPENGL_EGL for now (in addition to the virtual packages). Moving gbm to a virtual package is quite far from now, but still having something to grep for when we move to that would be great. Maxime
Hello, On Wed, 15 Feb 2017 09:55:51 +0100, Maxime Ripard wrote: > Moving gbm to a virtual package is quite far from now, but still > having something to grep for when we move to that would be great. To solve that, I think a simple comment is quite good, like: package/efl/Config.in: depends on BR2_PACKAGE_MESA3D_OPENGL_EGL # require libgbm from mesa3d package/qt5/qt5base/qt5base.mk:# Uses libgbm from mesa3d package/weston/Config.in:# Uses libgbm from mesa3d package/x11r7/xdriver_xf86-video-amdgpu/Config.in: depends on BR2_PACKAGE_MESA3D_OPENGL_EGL # gbm Thomas
diff --git a/package/Config.in b/package/Config.in index deff0fe..af52e1a 100644 --- a/package/Config.in +++ b/package/Config.in @@ -239,6 +239,7 @@ comment "Graphic applications" source "package/glmark2/Config.in" source "package/gnuplot/Config.in" source "package/jhead/Config.in" + source "package/kmscube/Config.in" source "package/mesa3d-demos/Config.in" source "package/qt5cinex/Config.in" source "package/rrdtool/Config.in" diff --git a/package/kmscube/0001-addimxdrm.patch b/package/kmscube/0001-addimxdrm.patch new file mode 100644 index 0000000..85e9a2a --- /dev/null +++ b/package/kmscube/0001-addimxdrm.patch @@ -0,0 +1,28 @@ +From 23186149100dd9c871f65132edce67db0b1ce3c3 Mon Sep 17 00:00:00 2001 +From: XoD <xoddark@gmail.com> +Date: Mon, 13 Feb 2017 16:43:35 -0200 +Subject: [PATCH] Add imx-drm management + +Add imx-drm in the list of kms modules. + +Signed-off-by: XoD <xoddark@gmail.com> +Signed-off-by: Fabio Estevam <festevam@gmail.com> +--- + kmscube.c | 1 + + 1 file changed, 1 insertion(+) + +diff --git a/kmscube.c b/kmscube.c +index ca2c266..95bd77f 100644 +--- a/kmscube.c ++++ b/kmscube.c +@@ -122,6 +122,7 @@ static int init_drm(void) + static const char *modules[] = { + "exynos", + "i915", ++ "imx-drm", + "msm", + "nouveau", + "omapdrm", +-- +2.7.4 + diff --git a/package/kmscube/Config.in b/package/kmscube/Config.in new file mode 100644 index 0000000..b3c3c8d --- /dev/null +++ b/package/kmscube/Config.in @@ -0,0 +1,5 @@ +config BR2_PACKAGE_KMSCUBE + bool "kmscube" + depends on BR2_PACKAGE_MESA3D && BR2_PACKAGE_LIBDRM + help + kmscube is an application to test kms/drm drivers. diff --git a/package/kmscube/kmscube.hash b/package/kmscube/kmscube.hash new file mode 100644 index 0000000..d3e9858 --- /dev/null +++ b/package/kmscube/kmscube.hash @@ -0,0 +1,2 @@ +# Locally computed +sha256 73cf923f915f9f7ee46f9f03bd24254fc04b5f166270c0d37c6f2bc3881186ec kmscube-8c6a20901f95e1b465bbca127f9d47fcfb8762e6.tar.gz diff --git a/package/kmscube/kmscube.mk b/package/kmscube/kmscube.mk new file mode 100644 index 0000000..267fffa --- /dev/null +++ b/package/kmscube/kmscube.mk @@ -0,0 +1,14 @@ +KMSCUBE_VERSION = 8c6a20901f95e1b465bbca127f9d47fcfb8762e6 +KMSCUBE_SITE = $(call github,robclark,kmscube,$(KMSCUBE_VERSION)) +KMSCUBE_DEPENDENCIES = host-pkgconf libdrm mesa3d +KMSCUBE_INSTALL_TARGET = YES +KMSCUBE_AUTORECONF = YES +KMSCUBE_INSTALL_STAGING = Y + +# Autoreconf requires an existing m4 directory +define KMSCUBE_PATCH_M4 + mkdir -p $(@D)/m4 +endef +KMSCUBE_POST_PATCH_HOOKS += KMSCUBE_PATCH_M4 + +$(eval $(autotools-package))
Add support for kmscube application, which is helpful for testing kms/drm drivers. Signed-off-by: Fabio Estevam <festevam@gmail.com> --- Changes since v1: - Use Rob Clark's tree instead (Gary). package/Config.in | 1 + package/kmscube/0001-addimxdrm.patch | 28 ++++++++++++++++++++++++++++ package/kmscube/Config.in | 5 +++++ package/kmscube/kmscube.hash | 2 ++ package/kmscube/kmscube.mk | 14 ++++++++++++++ 5 files changed, 50 insertions(+) create mode 100644 package/kmscube/0001-addimxdrm.patch create mode 100644 package/kmscube/Config.in create mode 100644 package/kmscube/kmscube.hash create mode 100644 package/kmscube/kmscube.mk