lkml.org 
[lkml]   [2022]   [Nov]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH resend v2] drm/fourcc: Add missing big-endian XRGB1555 and RGB565 formats
From
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
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2022-11-24 10:22    [W:0.242 / U:0.464 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site