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

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Thomas,

On Thu, Nov 24, 2022 at 10:20 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> 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.

I'm happy to _not_ export the big-endian RGB565 format in atari_drm, and
just do the byte swapping when copying to the hardware frame buffer ;-)
(although that would preclude some (future) optimization handing out
buffers allocated from graphics memory to avoid any copying at all)

But currently, drivers on big-endian platforms must set the
quirk_addfb_prefer_host_byte_order quirk flag, and doing so forces
the frame buffer console emulation to use big-endian RGB565, requiring
the big-endian RGB565 format to be present in the formats[] array.

P.S. Ext2fs used have a big-endian variant.  It was dropped, and
     everyone settled on the little-endian variant, as it was much
     faster to always do the byte swapping on big-endian, than to handle
     both the little-endian and big-endian variants dynamically.
     Likewise, XFS stayed big-endian.
     DRM settled on little-endian-with-exceptions...

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