Hi Am 24.11.22 um 10:04 schrieb Daniel Vetter: > On Thu, Nov 24, 2022 at 09:55:18AM +0100, Geert Uytterhoeven wrote: >> Hi Thomas, >> >> On Thu, Nov 24, 2022 at 9:47 AM Thomas Zimmermann wrote: >>> Am 23.11.22 um 17:43 schrieb Geert Uytterhoeven: >>>> As of commit eae06120f1974e1a ("drm: refuse ADDFB2 ioctl for broken >>>> bigendian drivers"), drivers must set the >>>> quirk_addfb_prefer_host_byte_order quirk to make the drm_mode_addfb() >>>> compat code work correctly on big-endian machines. >>>> >>>> While that works fine for big-endian XRGB8888 and ARGB8888, which are >>>> mapped to the existing little-endian BGRX8888 and BGRA8888 formats, it >>>> does not work for big-endian XRGB1555 and RGB565, as the latter are not >>>> listed in the format database. >>>> >>>> Fix this by adding the missing formats. Limit this to big-endian >>>> platforms, as there is currently no need to support these formats on >>>> little-endian platforms. >>>> >>>> Fixes: 6960e6da9cec3f66 ("drm: fix drm_mode_addfb() on big endian machines.") >>>> Signed-off-by: Geert Uytterhoeven >>>> --- >>>> v2: >>>> - Use "DRM_FORMAT_foo | DRM_FORMAT_BIG_ENDIAN" instead of >>>> "DRM_FORMAT_HOST_foo", >>>> - Turn into a lone patch, as all other patches from series >>>> https://lore.kernel.org/r/cover.1657300532.git.geert@linux-m68k.org >>>> were applied to drm-misc/for-linux-next. >>>> --- >>>> drivers/gpu/drm/drm_fourcc.c | 4 ++++ >>>> 1 file changed, 4 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c >>>> index e09331bb3bc73f21..265671a7f9134c1f 100644 >>>> --- a/drivers/gpu/drm/drm_fourcc.c >>>> +++ b/drivers/gpu/drm/drm_fourcc.c >>>> @@ -190,6 +190,10 @@ const struct drm_format_info *__drm_format_info(u32 format) >>>> { .format = DRM_FORMAT_BGRA5551, .depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha = true }, >>>> { .format = DRM_FORMAT_RGB565, .depth = 16, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 }, >>>> { .format = DRM_FORMAT_BGR565, .depth = 16, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 }, >>>> +#ifdef __BIG_ENDIAN >>>> + { .format = DRM_FORMAT_XRGB1555 | DRM_FORMAT_BIG_ENDIAN, .depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 }, >>>> + { .format = DRM_FORMAT_RGB565 | DRM_FORMAT_BIG_ENDIAN, .depth = 16, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 }, >>> >>> Getting back to the discussion on endianess, I don't understand why the >>> BIG_ENDIAN flag is set here. AFAIK these formats are always little >>> endian. And the BE flag is set by drivers/userspace if a framebuffer >>> has a BE ordering. >>> >>> It would be better to filter the BE flag in __drm_format_info() before >>> the function does the lookup. >> >> I mentioned that alternative in [2], but rejected it because of the >> disadvantages: >> - {,__}drm_format_info() returns a pointer to a const object, >> whose .format field won't have the DRM_FORMAT_BIG_ENDIAN flag set, >> complicating callers, >> - All callers need to be updated, >> - It is difficult to know which big-endian formats are really >> supported, especially as only a few are needed. > > fwiw this last point is why I think this is the right approach. Long term > we might want to add _BE variants of these #defines so that they can be > used everywhere and are easy to grep. As long as it's just a handful of > places then the very verboy | DRM_FORMAT_BIG_ENDIAN is ok too. Doesn't that contradict the comment at [1] to some extend? 'DRM formats are little endian.' and extra defines are only made for simplifying drivers. [1] https://elixir.bootlin.com/linux/latest/source/include/drm/drm_fourcc.h#L33 > > With this approach we can make it _very_ explicit what big endian formats > are supported by a driver or other piece in the stack (like fbdev > emulation), and I think explicit is what we want with be because it's > become such an exception. Otherwise we'll just end up with more terrible > cruft like the host endian hacks in the addfb compat code. To give a different perspective, with format-conversion helpers the destination buffer is usually a hardware buffer that can have big-endian ordering. So we sometimes have to swap byteorder to make output colors look correct. That is the easiest if all formats are in LE and the BIG_ENDIAN flag tells us when the swap. With the current multitude of formats and B_E flags that can describe the same result, it's all just more complicated. Best regards Thomas > -Daniel >> >> [2] [PATCH 1/3] drm/fourcc: Add missing big-endian XRGB1555 and RGB565 formats >> https://lore.kernel.org/all/0744671ac096a12f0d538906bd324efa71b11400.1657300532.git.geert@linux-m68k.org >> >> Gr{oetje,eeting}s, >> >> Geert >> >> -- >> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org >> >> In personal conversations with technical people, I call myself a hacker. But >> when I'm talking to journalists I just say "programmer" or something like that. >> -- Linus Torvalds > -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev