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

  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(+)
  

On Wed, Nov 23, 2022 at 05:43:10PM +0100, 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>
> ---
> 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

Why do we need the #ifdef here? Iirc some hw has big endian flags in the
scanout registers, so could supprt this unconditionally if there's no
#ifdef around the format defines. Some drivers might then also want a
DRM_FORMAT_FOO_BE define to simplify tables and stuff, but that's more a
bikeshed.

Otherwise this makes sense to me.
-Daniel

> +		{ .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 },
> +#endif
>  		{ .format = DRM_FORMAT_RGB888,		.depth = 24, .num_planes = 1, .cpp = { 3, 0, 0 }, .hsub = 1, .vsub = 1 },
>  		{ .format = DRM_FORMAT_BGR888,		.depth = 24, .num_planes = 1, .cpp = { 3, 0, 0 }, .hsub = 1, .vsub = 1 },
>  		{ .format = DRM_FORMAT_XRGB8888,	.depth = 24, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
> -- 
> 2.25.1
>
  

Hi Daniel,

On Wed, Nov 23, 2022 at 5:57 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Nov 23, 2022 at 05:43:10PM +0100, 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>
> > ---
> > 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.

> > --- 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
>
> Why do we need the #ifdef here? Iirc some hw has big endian flags in the
> scanout registers, so could supprt this unconditionally if there's no
> #ifdef around the format defines. Some drivers might then also want a
> DRM_FORMAT_FOO_BE define to simplify tables and stuff, but that's more a
> bikeshed.

 "Limit this to big-endian platforms, as there is currently no need
 to support these formats on little-endian platforms."

Will anyone make use of this? In theory, all of the 16-bpp formats
can have a big-endian counterpart.

I seem to be the first one running into this ;-)

> > +             { .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 },
> > +#endif
> >               { .format = DRM_FORMAT_RGB888,          .depth = 24, .num_planes = 1, .cpp = { 3, 0, 0 }, .hsub = 1, .vsub = 1 },
> >               { .format = DRM_FORMAT_BGR888,          .depth = 24, .num_planes = 1, .cpp = { 3, 0, 0 }, .hsub = 1, .vsub = 1 },
> >               { .format = DRM_FORMAT_XRGB8888,        .depth = 24, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },

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
  

On Wed, 23 Nov 2022 at 18:05, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Daniel,
>
> On Wed, Nov 23, 2022 at 5:57 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Wed, Nov 23, 2022 at 05:43:10PM +0100, 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>
> > > ---
> > > 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.
>
> > > --- 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
> >
> > Why do we need the #ifdef here? Iirc some hw has big endian flags in the
> > scanout registers, so could supprt this unconditionally if there's no
> > #ifdef around the format defines. Some drivers might then also want a
> > DRM_FORMAT_FOO_BE define to simplify tables and stuff, but that's more a
> > bikeshed.
>
>  "Limit this to big-endian platforms, as there is currently no need
>  to support these formats on little-endian platforms."
>
> Will anyone make use of this? In theory, all of the 16-bpp formats
> can have a big-endian counterpart.
>
> I seem to be the first one running into this ;-)

It should make testing easier, at least for drivers that can be used
on both be and le platforms. But also, we can fix that whenever
someone actually asks for it, same that we can add the #define since
the full flag version is a bit long.

I'll apply the patch to drm-misc-next.
-Daniel

>
> > > +             { .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 },
> > > +#endif
> > >               { .format = DRM_FORMAT_RGB888,          .depth = 24, .num_planes = 1, .cpp = { 3, 0, 0 }, .hsub = 1, .vsub = 1 },
> > >               { .format = DRM_FORMAT_BGR888,          .depth = 24, .num_planes = 1, .cpp = { 3, 0, 0 }, .hsub = 1, .vsub = 1 },
> > >               { .format = DRM_FORMAT_XRGB8888,        .depth = 24, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
>
> 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
  

Hi,

> > > +#ifdef __BIG_ENDIAN
> >
> > Why do we need the #ifdef here? Iirc some hw has big endian flags in the
> > scanout registers, so could supprt this unconditionally if there's no
> > #ifdef around the format defines. Some drivers might then also want a
> > DRM_FORMAT_FOO_BE define to simplify tables and stuff, but that's more a
> > bikeshed.
> 
>  "Limit this to big-endian platforms, as there is currently no need
>  to support these formats on little-endian platforms."
> 
> Will anyone make use of this? In theory, all of the 16-bpp formats
> can have a big-endian counterpart.

Highly unlikely.  Dealing with 16-bpp formats in non-native byte order
is a PITA because it isn't enough to simply adjust the masks and shifts
to pick the correct bits and be done with it.

The qemu stdvga happens to have a register to switch framebuffer
byteorder (so both x64 and ppc are happy), and the bochs drm driver
actually supports that no matter what the cpu byte order is, but it
supports only DRM_FORMAT_XRGB8888 + DRM_FORMAT_BGRX8888.

Supporting 16 bpp in the driver wouldn't be that much of a problem, but
processing the framebuffer on the host side when emulating a big endian
guest on a little endian host is painful.  I think I can't ask pixman to
do a conversation from DRM_FORMAT_RGB565 | DRM_FORMAT_BIG_ENDIAN to
DRM_FORMAT_XRGB8888 on a little endian machine.

take care,
  Gerd
  

Hi Gerd,

On Thu, Nov 24, 2022 at 8:29 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
> > > > +#ifdef __BIG_ENDIAN
> > >
> > > Why do we need the #ifdef here? Iirc some hw has big endian flags in the
> > > scanout registers, so could supprt this unconditionally if there's no
> > > #ifdef around the format defines. Some drivers might then also want a
> > > DRM_FORMAT_FOO_BE define to simplify tables and stuff, but that's more a
> > > bikeshed.
> >
> >  "Limit this to big-endian platforms, as there is currently no need
> >  to support these formats on little-endian platforms."
> >
> > Will anyone make use of this? In theory, all of the 16-bpp formats
> > can have a big-endian counterpart.
>
> Highly unlikely.  Dealing with 16-bpp formats in non-native byte order
> is a PITA because it isn't enough to simply adjust the masks and shifts
> to pick the correct bits and be done with it.
>
> The qemu stdvga happens to have a register to switch framebuffer
> byteorder (so both x64 and ppc are happy), and the bochs drm driver
> actually supports that no matter what the cpu byte order is, but it
> supports only DRM_FORMAT_XRGB8888 + DRM_FORMAT_BGRX8888.
>
> Supporting 16 bpp in the driver wouldn't be that much of a problem, but
> processing the framebuffer on the host side when emulating a big endian
> guest on a little endian host is painful.  I think I can't ask pixman to
> do a conversation from DRM_FORMAT_RGB565 | DRM_FORMAT_BIG_ENDIAN to
> DRM_FORMAT_XRGB8888 on a little endian machine.

Indeed. But you can do a quick 16-bit byteswap, and convert from
DRM_FORMAT_RGB565 to DRM_FORMAT_XRGB8888?

There's a similar issue with Cairo, cfr. '[PATCH libdrm v2 08/10]
util: Fix pwetty on big-endian"[1].

BTW, does pixman support converting DRM_FORMAT_RGB565 to
DRM_FORMAT_XRGB8888 on a big-endian machine?

[1] https://lore.kernel.org/all/e8597038478f12e9eda5e86b309b52988f69f2eb.1657302103.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
  

Hi

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.

Best regards
Thomas

> +#endif
>   		{ .format = DRM_FORMAT_RGB888,		.depth = 24, .num_planes = 1, .cpp = { 3, 0, 0 }, .hsub = 1, .vsub = 1 },
>   		{ .format = DRM_FORMAT_BGR888,		.depth = 24, .num_planes = 1, .cpp = { 3, 0, 0 }, .hsub = 1, .vsub = 1 },
>   		{ .format = DRM_FORMAT_XRGB8888,	.depth = 24, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },

-- 
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
  

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.

[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
  

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.

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.
-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
  

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
  

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
  

Hi,

> > Supporting 16 bpp in the driver wouldn't be that much of a problem, but
> > processing the framebuffer on the host side when emulating a big endian
> > guest on a little endian host is painful.  I think I can't ask pixman to
> > do a conversation from DRM_FORMAT_RGB565 | DRM_FORMAT_BIG_ENDIAN to
> > DRM_FORMAT_XRGB8888 on a little endian machine.
> 
> Indeed. But you can do a quick 16-bit byteswap, and convert from
> DRM_FORMAT_RGB565 to DRM_FORMAT_XRGB8888?

Sure doable, but it's an extra step in a rarely tested code path ...

> BTW, does pixman support converting DRM_FORMAT_RGB565 to
> DRM_FORMAT_XRGB8888 on a big-endian machine?

I don't think so.  When you can get the color bits with shifting and
masking pixman is happy.  For rgb565 (and xrgb1555) that works only on
native byte order.

take care,
  Gerd