[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 <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; 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_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.

Cheers,
Daniel