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

[Emil Velikov]

On 3 July 2017 at 14:22, Daniel Stone <daniel at fooishbar.org> wrote:
> Hi Emil,
>
> On 3 July 2017 at 12:06, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>> On 16 June 2017 at 18:14, Daniel Stone <daniels at collabora.com> 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_XRGB8888, WL_DRM_FORMAT_ARGB8888,
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_SARGB8888 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:
> <!-- The drm format codes match the #defines in drm_fourcc.h. -->
>
> 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