| Message ID | 20210928223754.3398867-1-kamel.bouhara@bootlin.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2,1/7] package/opengl/libgbm: new virtual package | expand |
Hello, On Wed, 29 Sep 2021 00:37:48 +0200 Kamel Bouhara <kamel.bouhara@bootlin.com> wrote: > From: Bernd Kuhls <bernd.kuhls@t-online.de> > > Kodi 18.0-Leia will implement stand-alone gbm support alongside x11 & > wayland. To enable building libgbm in mesa3d without x11 & wayland we > need to create a virtual package for libgbm. > > Also other packages besides mesa3d may provide libgbm.so, see > http://patchwork.ozlabs.org/patch/647235/ > http://patchwork.ozlabs.org/patch/939703/ > > We also introduce two feature that shall help user choosing the version > implemented by a libgbm provider. This foresightly avoid building package > without having the required libgbm version (e.g. kmscube, qt5, sdl2 > etc.) I would rephrase this as such: """ It turns out that libgbm has seen several additions in its API over time, and therefore not all libgbm implementations provide support for all features. In order to account for this, this commit adds two hidden boolean options that allow libgbm providers to indicate which optional features they support: BR2_PACKAGE_LIBGBM_HAS_FEATURE_FORMAT_MODIFIER_PLANE_COUNT and BR2_PACKAGE_LIBGBM_HAS_FEATURE_DMA_BUF. These booleans must be selected by the packages providing libgbm implementations, and depended on by packages using libgbm. """ Question: do we have in the tree some libgbm implementation that support neither BR2_PACKAGE_LIBGBM_HAS_FEATURE_FORMAT_MODIFIER_PLANE_COUNT nor BR2_PACKAGE_LIBGBM_HAS_FEATURE_DMA_BUF ? > diff --git a/package/opengl/libgbm/Config.in b/package/opengl/libgbm/Config.in > new file mode 100644 > index 0000000000..7aa3efb97a > --- /dev/null > +++ b/package/opengl/libgbm/Config.in > @@ -0,0 +1,24 @@ > +config BR2_PACKAGE_HAS_LIBGBM > + bool > + > +config BR2_PACKAGE_PROVIDES_LIBGBM > + string > + depends on BR2_PACKAGE_HAS_LIBGBM > + > +config BR2_PACKAGE_LIBGBM_HAS_FEATURE_FORMAT_MODIFIER_PLANE_COUNT > + bool > + depends on BR2_PACKAGE_HAS_LIBGBM > + > +# gbm implementations should select this option if they provide the > +# format modifier plane count feature. This API was initially introduced > +# in mesa3d version 17. A gbm implementation provides this feature if it > +# is implement function gbm_device_get_format_modifier_plane_count. The comment should be before the option. > +config BR2_PACKAGE_LIBGBM_HAS_FEATURE_DMA_BUF > + bool > + depends on BR2_PACKAGE_HAS_LIBGBM > + > +# gbm implementations should select this option if they provide the > +# dma buffer feature. This API was initially introduced in mesa3d > +# version 10. A gbm implementation provides this feature if it > +# is implement function gbm_bo_get_fd. Ditto. Note: no need to resend to address those comments, they can be fixed up when applying. Best regards, Thomas
Kamel, All, Your series has had a few comments. Most are minor, but one or two need a bit more insights and can't be fixed while applying. As such, I've marked it as Changes Requested in Patchwork. Also, could you write a cover-letter that exlpains the goal behind that series and how it is organised? A cover letter usually helps reviewers udnerstand the bigger picture of a series, while the individual patches describe more localised changes. Regards, Yann E. MORIN. On 2021-09-29 00:37 +0200, Kamel Bouhara spake thusly: > From: Bernd Kuhls <bernd.kuhls@t-online.de> > > Kodi 18.0-Leia will implement stand-alone gbm support alongside x11 & > wayland. To enable building libgbm in mesa3d without x11 & wayland we > need to create a virtual package for libgbm. > > Also other packages besides mesa3d may provide libgbm.so, see > http://patchwork.ozlabs.org/patch/647235/ > http://patchwork.ozlabs.org/patch/939703/ > > We also introduce two feature that shall help user choosing the version > implemented by a libgbm provider. This foresightly avoid building package > without having the required libgbm version (e.g. kmscube, qt5, sdl2 > etc.) > > Signed-off-by: Bernd Kuhls <bernd.kuhls@t-online.de> > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com> > Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com> > [ Kamel : introduce gbm api features ] > --- > Changes v1 -> v2: > - Squashed patch 1/2 > - Added more relevant comment for each features added > > package/opengl/Config.in | 1 + > package/opengl/libgbm/Config.in | 24 ++++++++++++++++++++++++ > package/opengl/libgbm/libgbm.mk | 9 +++++++++ > 3 files changed, 34 insertions(+) > create mode 100644 package/opengl/libgbm/Config.in > create mode 100644 package/opengl/libgbm/libgbm.mk > > diff --git a/package/opengl/Config.in b/package/opengl/Config.in > index cbc001427d..cfa51def45 100644 > --- a/package/opengl/Config.in > +++ b/package/opengl/Config.in > @@ -1,5 +1,6 @@ > source "package/opengl/libgl/Config.in" > source "package/opengl/libegl/Config.in" > +source "package/opengl/libgbm/Config.in" > source "package/opengl/libgles/Config.in" > source "package/opengl/libopencl/Config.in" > source "package/opengl/libopenvg/Config.in" > diff --git a/package/opengl/libgbm/Config.in b/package/opengl/libgbm/Config.in > new file mode 100644 > index 0000000000..7aa3efb97a > --- /dev/null > +++ b/package/opengl/libgbm/Config.in > @@ -0,0 +1,24 @@ > +config BR2_PACKAGE_HAS_LIBGBM > + bool > + > +config BR2_PACKAGE_PROVIDES_LIBGBM > + string > + depends on BR2_PACKAGE_HAS_LIBGBM > + > +config BR2_PACKAGE_LIBGBM_HAS_FEATURE_FORMAT_MODIFIER_PLANE_COUNT > + bool > + depends on BR2_PACKAGE_HAS_LIBGBM > + > +# gbm implementations should select this option if they provide the > +# format modifier plane count feature. This API was initially introduced > +# in mesa3d version 17. A gbm implementation provides this feature if it > +# is implement function gbm_device_get_format_modifier_plane_count. > + > +config BR2_PACKAGE_LIBGBM_HAS_FEATURE_DMA_BUF > + bool > + depends on BR2_PACKAGE_HAS_LIBGBM > + > +# gbm implementations should select this option if they provide the > +# dma buffer feature. This API was initially introduced in mesa3d > +# version 10. A gbm implementation provides this feature if it > +# is implement function gbm_bo_get_fd. > diff --git a/package/opengl/libgbm/libgbm.mk b/package/opengl/libgbm/libgbm.mk > new file mode 100644 > index 0000000000..ecab234720 > --- /dev/null > +++ b/package/opengl/libgbm/libgbm.mk > @@ -0,0 +1,9 @@ > +################################################################################ > +# > +# libgbm > +# > +################################################################################ > + > +# This package requires to install a gbm.pc which needs > +# to be provided by GBM providers. > +$(eval $(virtual-package)) > -- > 2.30.2 > > _______________________________________________ > buildroot mailing list > buildroot@buildroot.org > https://lists.buildroot.org/mailman/listinfo/buildroot
On Fri, Oct 01, 2021 at 04:13:54PM +0200, Thomas Petazzoni wrote: > Hello, > Hello Thomas, > On Wed, 29 Sep 2021 00:37:48 +0200 > Kamel Bouhara <kamel.bouhara@bootlin.com> wrote: > > > From: Bernd Kuhls <bernd.kuhls@t-online.de> > > > > Kodi 18.0-Leia will implement stand-alone gbm support alongside x11 & > > wayland. To enable building libgbm in mesa3d without x11 & wayland we > > need to create a virtual package for libgbm. > > > > Also other packages besides mesa3d may provide libgbm.so, see > > http://patchwork.ozlabs.org/patch/647235/ > > http://patchwork.ozlabs.org/patch/939703/ > > > > We also introduce two feature that shall help user choosing the version > > implemented by a libgbm provider. This foresightly avoid building package > > without having the required libgbm version (e.g. kmscube, qt5, sdl2 > > etc.) > > I would rephrase this as such: > > """ > It turns out that libgbm has seen several additions in its API over > time, and therefore not all libgbm implementations provide support for > all features. In order to account for this, this commit adds two hidden > boolean options that allow libgbm providers to indicate which optional > features they support: > BR2_PACKAGE_LIBGBM_HAS_FEATURE_FORMAT_MODIFIER_PLANE_COUNT and > BR2_PACKAGE_LIBGBM_HAS_FEATURE_DMA_BUF. These booleans must be selected > by the packages providing libgbm implementations, and depended on by > packages using libgbm. > """ > OK. > Question: do we have in the tree some libgbm implementation that > support neither > BR2_PACKAGE_LIBGBM_HAS_FEATURE_FORMAT_MODIFIER_PLANE_COUNT nor > BR2_PACKAGE_LIBGBM_HAS_FEATURE_DMA_BUF ? So far, the dma buf feature (mesa gbm v10) is provided by every implementation I compared (gcnano, ti, sunxi, imx-gpu). Thanks for the review, sending a v3 soon. Kamel > > > diff --git a/package/opengl/libgbm/Config.in b/package/opengl/libgbm/Config.in > > new file mode 100644 > > index 0000000000..7aa3efb97a > > --- /dev/null > > +++ b/package/opengl/libgbm/Config.in > > @@ -0,0 +1,24 @@ > > +config BR2_PACKAGE_HAS_LIBGBM > > + bool > > + > > +config BR2_PACKAGE_PROVIDES_LIBGBM > > + string > > + depends on BR2_PACKAGE_HAS_LIBGBM > > + > > +config BR2_PACKAGE_LIBGBM_HAS_FEATURE_FORMAT_MODIFIER_PLANE_COUNT > > + bool > > + depends on BR2_PACKAGE_HAS_LIBGBM > > + > > +# gbm implementations should select this option if they provide the > > +# format modifier plane count feature. This API was initially introduced > > +# in mesa3d version 17. A gbm implementation provides this feature if it > > +# is implement function gbm_device_get_format_modifier_plane_count. > > The comment should be before the option. > > > +config BR2_PACKAGE_LIBGBM_HAS_FEATURE_DMA_BUF > > + bool > > + depends on BR2_PACKAGE_HAS_LIBGBM > > + > > +# gbm implementations should select this option if they provide the > > +# dma buffer feature. This API was initially introduced in mesa3d > > +# version 10. A gbm implementation provides this feature if it > > +# is implement function gbm_bo_get_fd. > > Ditto. > > Note: no need to resend to address those comments, they can be fixed up > when applying. > > Best regards, > > Thomas > -- > Thomas Petazzoni, co-owner and CEO, Bootlin > Embedded Linux and Kernel engineering and training > https://bootlin.com -- Kamel Bouhara, Bootlin Embedded Linux and kernel engineering https://bootlin.com
On Sun, Oct 03, 2021 at 11:05:17PM +0200, Yann E. MORIN wrote: > Kamel, All, > Hello, > Your series has had a few comments. Most are minor, but one or two need > a bit more insights and can't be fixed while applying. As such, I've > marked it as Changes Requested in Patchwork. > > Also, could you write a cover-letter that exlpains the goal behind that > series and how it is organised? A cover letter usually helps reviewers > udnerstand the bigger picture of a series, while the individual patches > describe more localised changes. > Of course, I actually sent the cover-letter in the first version and I tough it was not nececessary to re-sent it again. "This series introduce a virtual/package for the libgbm. We also introduce two gbm features so that packages depending on it can select which version of the api is required. Note that we only added sunxi, gcnano and mesa3d api but other implementations can be added later based on this series." Regards, Kamel > Regards, > Yann E. MORIN. > > On 2021-09-29 00:37 +0200, Kamel Bouhara spake thusly: > > From: Bernd Kuhls <bernd.kuhls@t-online.de> > > > > Kodi 18.0-Leia will implement stand-alone gbm support alongside x11 & > > wayland. To enable building libgbm in mesa3d without x11 & wayland we > > need to create a virtual package for libgbm. > > > > Also other packages besides mesa3d may provide libgbm.so, see > > http://patchwork.ozlabs.org/patch/647235/ > > http://patchwork.ozlabs.org/patch/939703/ > > > > We also introduce two feature that shall help user choosing the version > > implemented by a libgbm provider. This foresightly avoid building package > > without having the required libgbm version (e.g. kmscube, qt5, sdl2 > > etc.) > > > > Signed-off-by: Bernd Kuhls <bernd.kuhls@t-online.de> > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com> > > Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com> > > [ Kamel : introduce gbm api features ] > > --- > > Changes v1 -> v2: > > - Squashed patch 1/2 > > - Added more relevant comment for each features added > > > > package/opengl/Config.in | 1 + > > package/opengl/libgbm/Config.in | 24 ++++++++++++++++++++++++ > > package/opengl/libgbm/libgbm.mk | 9 +++++++++ > > 3 files changed, 34 insertions(+) > > create mode 100644 package/opengl/libgbm/Config.in > > create mode 100644 package/opengl/libgbm/libgbm.mk > > > > diff --git a/package/opengl/Config.in b/package/opengl/Config.in > > index cbc001427d..cfa51def45 100644 > > --- a/package/opengl/Config.in > > +++ b/package/opengl/Config.in > > @@ -1,5 +1,6 @@ > > source "package/opengl/libgl/Config.in" > > source "package/opengl/libegl/Config.in" > > +source "package/opengl/libgbm/Config.in" > > source "package/opengl/libgles/Config.in" > > source "package/opengl/libopencl/Config.in" > > source "package/opengl/libopenvg/Config.in" > > diff --git a/package/opengl/libgbm/Config.in b/package/opengl/libgbm/Config.in > > new file mode 100644 > > index 0000000000..7aa3efb97a > > --- /dev/null > > +++ b/package/opengl/libgbm/Config.in > > @@ -0,0 +1,24 @@ > > +config BR2_PACKAGE_HAS_LIBGBM > > + bool > > + > > +config BR2_PACKAGE_PROVIDES_LIBGBM > > + string > > + depends on BR2_PACKAGE_HAS_LIBGBM > > + > > +config BR2_PACKAGE_LIBGBM_HAS_FEATURE_FORMAT_MODIFIER_PLANE_COUNT > > + bool > > + depends on BR2_PACKAGE_HAS_LIBGBM > > + > > +# gbm implementations should select this option if they provide the > > +# format modifier plane count feature. This API was initially introduced > > +# in mesa3d version 17. A gbm implementation provides this feature if it > > +# is implement function gbm_device_get_format_modifier_plane_count. > > + > > +config BR2_PACKAGE_LIBGBM_HAS_FEATURE_DMA_BUF > > + bool > > + depends on BR2_PACKAGE_HAS_LIBGBM > > + > > +# gbm implementations should select this option if they provide the > > +# dma buffer feature. This API was initially introduced in mesa3d > > +# version 10. A gbm implementation provides this feature if it > > +# is implement function gbm_bo_get_fd. > > diff --git a/package/opengl/libgbm/libgbm.mk b/package/opengl/libgbm/libgbm.mk > > new file mode 100644 > > index 0000000000..ecab234720 > > --- /dev/null > > +++ b/package/opengl/libgbm/libgbm.mk > > @@ -0,0 +1,9 @@ > > +################################################################################ > > +# > > +# libgbm > > +# > > +################################################################################ > > + > > +# This package requires to install a gbm.pc which needs > > +# to be provided by GBM providers. > > +$(eval $(virtual-package)) > > -- > > 2.30.2 > > > > _______________________________________________ > > buildroot mailing list > > buildroot@buildroot.org > > https://lists.buildroot.org/mailman/listinfo/buildroot > > -- > .-----------------.--------------------.------------------.--------------------. > | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: | > | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ | > | +33 561 099 427 `------------.-------: X AGAINST | \e/ There is no | > | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. | > '------------------------------^-------^------------------^--------------------' -- Kamel Bouhara, Bootlin Embedded Linux and kernel engineering https://bootlin.com
Hello Kamel, On Thu, 7 Oct 2021 11:35:34 +0200 Kamel Bouhara <kamel.bouhara@bootlin.com> wrote: > > Question: do we have in the tree some libgbm implementation that > > support neither > > BR2_PACKAGE_LIBGBM_HAS_FEATURE_FORMAT_MODIFIER_PLANE_COUNT nor > > BR2_PACKAGE_LIBGBM_HAS_FEATURE_DMA_BUF ? > > So far, the dma buf feature (mesa gbm v10) is provided by every > implementation I compared (gcnano, ti, sunxi, imx-gpu). So what made you think we need to identify this feature separately from others? The whole point of this "feature" mechanism is to be able to distinguish the different GBM implementations in the features they provide. Thanks! Thomas
On Fri, 8 Oct 2021 14:47:45 +0200 Paul Kocialkowski <paul.kocialkowski@bootlin.com> wrote: > > So what made you think we need to identify this feature separately from > > others? > > I seem to recall that earlier versions of the sunxi blobs didn't have > dma-buf support, and they eventually released some that did. > > So my two cents would be that dma-buf is not part of the "base" set of > features from the GBM API and it makes sense to recognize it as an > additional feature. Thanks, useful feedback (should I say "as usual" ?). Should perhaps be captured "somewhere" (commit log ? comment above the feature option ?). Thanks! Thomas
diff --git a/package/opengl/Config.in b/package/opengl/Config.in index cbc001427d..cfa51def45 100644 --- a/package/opengl/Config.in +++ b/package/opengl/Config.in @@ -1,5 +1,6 @@ source "package/opengl/libgl/Config.in" source "package/opengl/libegl/Config.in" +source "package/opengl/libgbm/Config.in" source "package/opengl/libgles/Config.in" source "package/opengl/libopencl/Config.in" source "package/opengl/libopenvg/Config.in" diff --git a/package/opengl/libgbm/Config.in b/package/opengl/libgbm/Config.in new file mode 100644 index 0000000000..7aa3efb97a --- /dev/null +++ b/package/opengl/libgbm/Config.in @@ -0,0 +1,24 @@ +config BR2_PACKAGE_HAS_LIBGBM + bool + +config BR2_PACKAGE_PROVIDES_LIBGBM + string + depends on BR2_PACKAGE_HAS_LIBGBM + +config BR2_PACKAGE_LIBGBM_HAS_FEATURE_FORMAT_MODIFIER_PLANE_COUNT + bool + depends on BR2_PACKAGE_HAS_LIBGBM + +# gbm implementations should select this option if they provide the +# format modifier plane count feature. This API was initially introduced +# in mesa3d version 17. A gbm implementation provides this feature if it +# is implement function gbm_device_get_format_modifier_plane_count. + +config BR2_PACKAGE_LIBGBM_HAS_FEATURE_DMA_BUF + bool + depends on BR2_PACKAGE_HAS_LIBGBM + +# gbm implementations should select this option if they provide the +# dma buffer feature. This API was initially introduced in mesa3d +# version 10. A gbm implementation provides this feature if it +# is implement function gbm_bo_get_fd. diff --git a/package/opengl/libgbm/libgbm.mk b/package/opengl/libgbm/libgbm.mk new file mode 100644 index 0000000000..ecab234720 --- /dev/null +++ b/package/opengl/libgbm/libgbm.mk @@ -0,0 +1,9 @@ +################################################################################ +# +# libgbm +# +################################################################################ + +# This package requires to install a gbm.pc which needs +# to be provided by GBM providers. +$(eval $(virtual-package))