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

From: Geert Uytterhoeven <geert@linux-m68k.org>
To: "Michel Dänzer" <michel.daenzer@mailbox.org>
Cc: "Maarten Lankhorst" <maarten.lankhorst@linux.intel.com>,
	"Maxime Ripard" <mripard@kernel.org>,
	"Thomas Zimmermann" <tzimmermann@suse.de>,
	"David Airlie" <airlied@linux.ie>,
	"Daniel Vetter" <daniel@ffwll.ch>,
	"Noralf Trønnes" <noralf@tronnes.org>,
	"Gerd Hoffmann" <kraxel@redhat.com>,
	"Linux Fbdev development list" <linux-fbdev@vger.kernel.org>,
	Linux/m68k <linux-m68k@vger.kernel.org>,
	"DRI Development" <dri-devel@lists.freedesktop.org>,
	"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/3] drm/fourcc: Add missing big-endian XRGB1555 and RGB565 formats
Date: Mon, 11 Jul 2022 17:30:30 +0200	[thread overview]
Message-ID: <CAMuHMdUgdbZeoFLFL8+Hm-6fG9cg5Wzq++JED3KR5P9YZtRQ4A@mail.gmail.com> (raw)
In-Reply-To: <96a87833-d878-dde9-e335-9ea51a4ba406@mailbox.org>

Hi Michel,

On Mon, Jul 11, 2022 at 5:23 PM Michel Dänzer
<michel.daenzer@mailbox.org> wrote:
> On 2022-07-08 20:21, Geert Uytterhoeven wrote:
> > 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>
> > ---
> > Cirrus is the only driver setting quirk_addfb_prefer_host_byte_order
> > and supporting RGB565 or XRGB1555, but no one tried that on big-endian?
> > Cirrus does not support DRM_FORMAT_RGB565 | DRM_FORMAT_BIG_ENDIAN
> > in cirrus_fb_create, so you cannot get a graphical text console.
> >
> > Do we need these definitions on little-endian platforms, too?
> > Would it be better to use "DRM_FORMAT_{XRGB1555,RGB565} |
> > DRM_FORMAT_BIG_ENDIAN" instead of "DRM_FORMAT_HOST_{XRGB1555,RGB565}" in
> > formats[]?
>
> The intention of DRM_FORMAT_HOST_* is that they are macros in include/drm/drm_fourcc.h which just map to little endian formats defined in drivers/gpu/drm/drm_fourcc.c. Since this is not possible for big endian hosts for XRGB1555 or RGB565 (or any other format with non-8-bit components), this isn't applicable here.

I read that as that you prefer to write
"DRM_FORMAT_{XRGB1555,RGB565} | DRM_FORMAT_BIG_ENDIAN" in formats[]?

> It's also doubtful that Cirrus hardware would access these formats as big endian (drivers/gpu/drm/tiny/cirrus.c has no endianness references at all, and the hardware was surely designed for x86 first and foremost).
>
> Instead, fbcon (and user space) needs to convert to little endian when using DRM_FORMAT_HOST_{XRGB1555,RGB565} with the cirrus driver on big endian hosts.

Yeah, probably the cirrus driver can use some fixes...

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