On 3 July 2017 at 14:22, Daniel Stone <dan...@fooishbar.org> wrote: > Hi Emil, > > On 3 July 2017 at 12:06, Emil Velikov <emil.l.veli...@gmail.com> wrote: >> On 16 June 2017 at 18:14, Daniel Stone <dani...@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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev