Re: [PATCH resend v2] drm/fourcc: Add missing big-endian XRGB1555 and RGB565 formats

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 5597 bytes --]

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 <tzimmermann@suse.de> 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 <geert@linux-m68k.org>
>>>> ---
>>>> 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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]