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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxx> 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@xxxxxxxxxxxxxx>
---
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@xxxxxxxxxxxxxx
      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@xxxxxxxxxxxxxx

Gr{oetje,eeting}s,

                         Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

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: OpenPGP_signature
Description: OpenPGP digital signature


[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux