Re: [Mesa-dev] [PATCH 04/11] gbm: Axe buffer import format conversion table

[Emil Velikov]

On 3 July 2017 at 14:22, Daniel Stone  wrote:
> Hi Emil,
>
> On 3 July 2017 at 12:06, Emil Velikov  wrote:
>> On 16 June 2017 at 18:14, Daniel Stone  wrote:
>>> +  /* GBM_FORMAT_* is identical to WL_DRM_FORMAT_*, so no conversion
>>> +   * required. */
>>> +  gbm_format = wb->format;
>> AFACIT not all the WL_DRM_FORMAT_* have a GBM_FORMAT_* equivalent.
>>
>> Regardless, I think we're safe atm. (as the formats supported are
>> identical on both gbm/wl side) but it will cause subtle breakage as
>> one enhances the WL side. Please add a note if you prefer to keep the
>> hunk.
>
> Well, both wl_drm and GBM are defined to be exactly drm_fourcc.h. So
> there's no chance of them having conflicting format mappings;
Grr I got this wrong - not all gbm formats have a wl_drm equivalent.
By my count gbm.h lists 60, while wayland-drm.h shows 58.

And I fully agree they are/should be identical to drm_fourcc.h

> any
> patch to either which extended beyond that would be rejected.
In a perfect world, yes. I might be too paranoid.

> Beyond
> that, we already have to consider what happens to clients when we add
> new formats, so I'm not really sure what an explicit conversion table
> gives us, except for another place to forget to patch when new formats
> are added.
>
Agreed. Adding/maintaining these is not good.

Keep in mind that this patch effectively allows more formats than were
previously ignored/forbidden.
Can you sneak a small note about it in the commit summary - rough example:

"
..., since they are (almost) completely compatible.

This implies that gbm_bo_import() accepts more formats. For example:

Before this patch, Wayland buffers only with the following formats were allowed.
WL_DRM_FORMAT_XRGB, WL_DRM_FORMAT_ARGB,
WL_DRM_FORMAT_RGB565,WL_DRM_FORMAT_YUYV.
"


> What kind of comment would you like? /* When updating one of these,
> don't forget to update the other, and also drm_fourcc.h which is where
> they come from. */
>
Focus is on: the odd header may be missing a define, yet things should
still work.

"In the odd case, a wl_drm format corresponding to the gbm one may be
missing. That should be fine, since those are identical/subset of
drm_fourcc.h. Which is what the driver uses, internally."

I'm not 100% sure on the last sentence.

>> Formats have bit us in the past, so it's better to catch that before
>> we break Gnome/mutter, KDE, Weston etc.
>
> What breakage are you thinking of? I understand the general
> conservatism but can't think of anything off the top of my head.
>
839793680f99b8387bee9489733d5071c10f3ace,
ccdcf91104a5f07127b5b8d8570b5c4bbcf86647.

>> To minimise test/script update the test and ensure the headers are
>> sane for consumption, I'm thinking of the following:
>>  - script(?) that greps for GBM_FORMAT_* gbm.h + __DRI_IMAGE_FOURCC
>> dri_interface.h
>>  - compares the values - say a simple C program but anything will do
>>
>> #include "gbm.h"
>> #include "dri_interface.h"
>>
>> int
>> main(void)
>> {
>>assert(GBM_FORMAT_FOO == __DRI_IMAGE_FOURCC_FOO);
>>
>> }
>
> __DRI_IMAGE_FourCC is purely (except, as noted,
> __DRI_IMAGE_FOURCC_SARGB which has _no_ equivalent in
> GBM/wl_drm/KMS; we would not add one) a subset of the drm_fourcc.h
> formats: there are 24 such formats. GBM_FORMAT_* is a copy of the
> drm_fourcc.h formats: there are 60 such formats.
>
> There's already this comment in dri_interface.h above the
> __DRI_IMAGE_FOURCC_* formats, which I'd hope reviewers would notice:
> /**
>  * Four CC formats that matches with WL_DRM_FORMAT_* from wayland_drm.h,
>  * GBM_FORMAT_* from gbm.h, and DRM_FORMAT_* from drm_fourcc.h.
>  */
>
> And this one in wayland-drm.xml:
> 
>
> wl_drm also should never be updated; it's superseded by the dmabuf
> protocol used in 11/11.
>
Agreed that we want to move away from wl_drm, even if we synced the
Xserver copy a few days ago ;-)
That will take some time, though.

In case it got lost - I'm saying that adding a test _after_ the series
is merged, may be a good idea.

Thanks
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 04/11] gbm: Axe buffer import format conversion table

[Daniel Stone]

Hi Emil,

On 3 July 2017 at 12:06, Emil Velikov  wrote:
> On 16 June 2017 at 18:14, Daniel Stone  wrote:
>> +  /* GBM_FORMAT_* is identical to WL_DRM_FORMAT_*, so no conversion
>> +   * required. */
>> +  gbm_format = wb->format;
> AFACIT not all the WL_DRM_FORMAT_* have a GBM_FORMAT_* equivalent.
>
> Regardless, I think we're safe atm. (as the formats supported are
> identical on both gbm/wl side) but it will cause subtle breakage as
> one enhances the WL side. Please add a note if you prefer to keep the
> hunk.

Well, both wl_drm and GBM are defined to be exactly drm_fourcc.h. So
there's no chance of them having conflicting format mappings; any
patch to either which extended beyond that would be rejected. Beyond
that, we already have to consider what happens to clients when we add
new formats, so I'm not really sure what an explicit conversion table
gives us, except for another place to forget to patch when new formats
are added.

What kind of comment would you like? /* When updating one of these,
don't forget to update the other, and also drm_fourcc.h which is where
they come from. */

> Formats have bit us in the past, so it's better to catch that before
> we break Gnome/mutter, KDE, Weston etc.

What breakage are you thinking of? I understand the general
conservatism but can't think of anything off the top of my head.

> To minimise test/script update the test and ensure the headers are
> sane for consumption, I'm thinking of the following:
>  - script(?) that greps for GBM_FORMAT_* gbm.h + __DRI_IMAGE_FOURCC
> dri_interface.h
>  - compares the values - say a simple C program but anything will do
>
> #include "gbm.h"
> #include "dri_interface.h"
>
> int
> main(void)
> {
>assert(GBM_FORMAT_FOO == __DRI_IMAGE_FOURCC_FOO);
>
> }

__DRI_IMAGE_FourCC is purely (except, as noted,
__DRI_IMAGE_FOURCC_SARGB which has _no_ equivalent in
GBM/wl_drm/KMS; we would not add one) a subset of the drm_fourcc.h
formats: there are 24 such formats. GBM_FORMAT_* is a copy of the
drm_fourcc.h formats: there are 60 such formats.

There's already this comment in dri_interface.h above the
__DRI_IMAGE_FOURCC_* formats, which I'd hope reviewers would notice:
/**
 * Four CC formats that matches with WL_DRM_FORMAT_* from wayland_drm.h,
 * GBM_FORMAT_* from gbm.h, and DRM_FORMAT_* from drm_fourcc.h.
 */

And this one in wayland-drm.xml:


wl_drm also should never be updated; it's superseded by the dmabuf
protocol used in 11/11.

Cheers,
Daniel
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 04/11] gbm: Axe buffer import format conversion table

[Emil Velikov]

Hi Dan,

On 16 June 2017 at 18:14, Daniel Stone  wrote:
> Wayland buffers coming from wl_drm use the WL_DRM_FORMAT_* enums, which
> are identical to GBM_FORMAT_*. Similarly, FD imports do not need to
> convert between GBM and DRI FourCC, since they are (almost) completely
> compatible.
>
> Signed-off-by: Daniel Stone 
> ---
>  src/gbm/backends/dri/gbm_dri.c | 62 
> +++---
>  1 file changed, 22 insertions(+), 40 deletions(-)
>
> diff --git a/src/gbm/backends/dri/gbm_dri.c b/src/gbm/backends/dri/gbm_dri.c
> index 19be440d48..84f37d4cf5 100644
> --- a/src/gbm/backends/dri/gbm_dri.c
> +++ b/src/gbm/backends/dri/gbm_dri.c
> @@ -859,23 +859,9 @@ gbm_dri_bo_import(struct gbm_device *gbm,
>
>image = dri->image->dupImage(wb->driver_buffer, NULL);
>
> -  switch (wb->format) {
> -  case WL_DRM_FORMAT_XRGB:
> - gbm_format = GBM_FORMAT_XRGB;
> - break;
> -  case WL_DRM_FORMAT_ARGB:
> - gbm_format = GBM_FORMAT_ARGB;
> - break;
> -  case WL_DRM_FORMAT_RGB565:
> - gbm_format = GBM_FORMAT_RGB565;
> - break;
> -  case WL_DRM_FORMAT_YUYV:
> - gbm_format = GBM_FORMAT_YUYV;
> - break;
> -  default:
> - dri->image->destroyImage(image);
> - return NULL;
> -  }
> +  /* GBM_FORMAT_* is identical to WL_DRM_FORMAT_*, so no conversion
> +   * required. */
> +  gbm_format = wb->format;
AFACIT not all the WL_DRM_FORMAT_* have a GBM_FORMAT_* equivalent.

Regardless, I think we're safe atm. (as the formats supported are
identical on both gbm/wl side) but it will cause subtle breakage as
one enhances the WL side. Please add a note if you prefer to keep the
hunk.

>break;
> }
>  #endif
> @@ -904,23 +890,27 @@ gbm_dri_bo_import(struct gbm_device *gbm,
> {
>struct gbm_import_fd_data *fd_data = buffer;
>int stride = fd_data->stride, offset = 0;
> -  int dri_format;
> +  int fourcc;
>
> +  /* GBM's GBM_FORMAT_* tokens are a strict superset of the DRI FourCC
> +   * tokens accepted by createImageFromFds, except for not supporting
> +   * the sARGB format. Also, GBM_BO_FORMAT_* are defined differently to
> +   * their GBM_FORMAT_* equivalents, so remap them here. */
>switch (fd_data->format) {
>case GBM_BO_FORMAT_XRGB:
> - dri_format = GBM_FORMAT_XRGB;
> + fourcc = GBM_FORMAT_XRGB;
>   break;
>case GBM_BO_FORMAT_ARGB:
> - dri_format = GBM_FORMAT_ARGB;
> + fourcc = GBM_FORMAT_ARGB;
>   break;
>default:
> - dri_format = fd_data->format;
> + fourcc = fd_data->format;
>}
>
This and below is fine, but I think we'd want to have a "make check"
test at a later stage.

With the WL addressed (left as original or with a comment), patch is
Reviewed-by: Emil Velikov 

-Emil


Formats have bit us in the past, so it's better to catch that before
we break Gnome/mutter, KDE, Weston etc.

To minimise test/script update the test and ensure the headers are
sane for consumption, I'm thinking of the following:
 - script(?) that greps for GBM_FORMAT_* gbm.h + __DRI_IMAGE_FOURCC
dri_interface.h
 - compares the values - say a simple C program but anything will do

#include "gbm.h"
#include "dri_interface.h"

int
main(void)
{
   assert(GBM_FORMAT_FOO == __DRI_IMAGE_FOURCC_FOO);
   
}
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 04/11] gbm: Axe buffer import format conversion table

[Daniel Stone]

Wayland buffers coming from wl_drm use the WL_DRM_FORMAT_* enums, which
are identical to GBM_FORMAT_*. Similarly, FD imports do not need to
convert between GBM and DRI FourCC, since they are (almost) completely
compatible.

Signed-off-by: Daniel Stone 
---
 src/gbm/backends/dri/gbm_dri.c | 62 +++---
 1 file changed, 22 insertions(+), 40 deletions(-)

diff --git a/src/gbm/backends/dri/gbm_dri.c b/src/gbm/backends/dri/gbm_dri.c
index 19be440d48..84f37d4cf5 100644
--- a/src/gbm/backends/dri/gbm_dri.c
+++ b/src/gbm/backends/dri/gbm_dri.c
@@ -859,23 +859,9 @@ gbm_dri_bo_import(struct gbm_device *gbm,
 
   image = dri->image->dupImage(wb->driver_buffer, NULL);
 
-  switch (wb->format) {
-  case WL_DRM_FORMAT_XRGB:
- gbm_format = GBM_FORMAT_XRGB;
- break;
-  case WL_DRM_FORMAT_ARGB:
- gbm_format = GBM_FORMAT_ARGB;
- break;
-  case WL_DRM_FORMAT_RGB565:
- gbm_format = GBM_FORMAT_RGB565;
- break;
-  case WL_DRM_FORMAT_YUYV:
- gbm_format = GBM_FORMAT_YUYV;
- break;
-  default:
- dri->image->destroyImage(image);
- return NULL;
-  }
+  /* GBM_FORMAT_* is identical to WL_DRM_FORMAT_*, so no conversion
+   * required. */
+  gbm_format = wb->format;
   break;
}
 #endif
@@ -904,23 +890,27 @@ gbm_dri_bo_import(struct gbm_device *gbm,
{
   struct gbm_import_fd_data *fd_data = buffer;
   int stride = fd_data->stride, offset = 0;
-  int dri_format;
+  int fourcc;
 
+  /* GBM's GBM_FORMAT_* tokens are a strict superset of the DRI FourCC
+   * tokens accepted by createImageFromFds, except for not supporting
+   * the sARGB format. Also, GBM_BO_FORMAT_* are defined differently to
+   * their GBM_FORMAT_* equivalents, so remap them here. */
   switch (fd_data->format) {
   case GBM_BO_FORMAT_XRGB:
- dri_format = GBM_FORMAT_XRGB;
+ fourcc = GBM_FORMAT_XRGB;
  break;
   case GBM_BO_FORMAT_ARGB:
- dri_format = GBM_FORMAT_ARGB;
+ fourcc = GBM_FORMAT_ARGB;
  break;
   default:
- dri_format = fd_data->format;
+ fourcc = fd_data->format;
   }
 
   image = dri->image->createImageFromFds(dri->screen,
  fd_data->width,
  fd_data->height,
- dri_format,
+ fourcc,
  _data->fd, 1,
  , ,
  NULL);
@@ -945,27 +935,19 @@ gbm_dri_bo_import(struct gbm_device *gbm,
  return NULL;
   }
 
-  switch(fd_data->format) {
-  case GBM_FORMAT_RGB565:
- fourcc = __DRI_IMAGE_FOURCC_RGB565;
- break;
-  case GBM_FORMAT_ARGB:
-  case GBM_BO_FORMAT_ARGB:
- fourcc = __DRI_IMAGE_FOURCC_ARGB;
- break;
-  case GBM_FORMAT_XRGB:
+  /* GBM's GBM_FORMAT_* tokens are a strict superset of the DRI FourCC
+   * tokens accepted by createImageFromDmaBufs2, except for not supporting
+   * the sARGB format. Also, GBM_BO_FORMAT_* are defined differently to
+   * their GBM_FORMAT_* equivalents, so remap them here. */
+  switch (fd_data->format) {
   case GBM_BO_FORMAT_XRGB:
- fourcc = __DRI_IMAGE_FOURCC_XRGB;
- break;
-  case GBM_FORMAT_ABGR:
- fourcc = __DRI_IMAGE_FOURCC_ABGR;
+ fourcc = GBM_FORMAT_XRGB;
  break;
-  case GBM_FORMAT_XBGR:
- fourcc = __DRI_IMAGE_FOURCC_XBGR;
+  case GBM_BO_FORMAT_ARGB:
+ fourcc = GBM_FORMAT_ARGB;
  break;
   default:
- errno = EINVAL;
- return NULL;
+ fourcc = fd_data->format;
   }
 
   image = dri->image->createImageFromDmaBufs2(dri->screen, fd_data->width,
@@ -982,7 +964,7 @@ gbm_dri_bo_import(struct gbm_device *gbm,
  return NULL;
   }
 
-  gbm_format = fd_data->format;
+  gbm_format = fourcc;
   break;
}
 
-- 
2.13.0

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev