[PATCH] drm: fourcc byteorder: brings header file comments in line with reality.

[PATCH] drm: fourcc byteorder: brings header file comments in line with reality.

[Gerd Hoffmann]

While working on graphics support for virtual machines on ppc64 (which
exists in both little and big endian variants) I've figured the comments
for various drm fourcc formats in the header file don't match reality.

Comments says the RGB formats are little endian, but in practice they
are native endian.  Look at the drm_mode_legacy_fb_format() helper.  It
maps -- for example -- bpp/depth 32/24 to DRM_FORMAT_XRGB8888, no matter
whenever the machine is little endian or big endian.  The users of this
function (fbdev emulation, DRM_IOCTL_MODE_ADDFB) expect the framebuffer
is native endian, not little endian.  Most userspace also operates on
native endian only.

So, go update the comments for all 16+24+32 bpp RGB formats.

Leaving the yuv formats as-is.  I have no idea if and how those are used
on bigendian machines.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Pekka Paalanen <ppaalanen@gmail.com>
Cc: Ilia Mirkin <imirkin@alum.mit.edu>
Cc: Michel Dänzer <michel@daenzer.net>
Cc: Alex Deucher <alexdeucher@gmail.com>
Cc: amd-gfx@lists.freedesktop.org
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/uapi/drm/drm_fourcc.h | 82 +++++++++++++++++++++----------------------
 1 file changed, 41 insertions(+), 41 deletions(-)

diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
index 995c8f9..1579765 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -42,68 +42,68 @@ extern "C" {
 #define DRM_FORMAT_R8		fourcc_code('R', '8', ' ', ' ') /* [7:0] R */
 
 /* 16 bpp Red */
-#define DRM_FORMAT_R16		fourcc_code('R', '1', '6', ' ') /* [15:0] R little endian */
+#define DRM_FORMAT_R16		fourcc_code('R', '1', '6', ' ') /* [15:0] R native endian */
 
 /* 16 bpp RG */
-#define DRM_FORMAT_RG88		fourcc_code('R', 'G', '8', '8') /* [15:0] R:G 8:8 little endian */
-#define DRM_FORMAT_GR88		fourcc_code('G', 'R', '8', '8') /* [15:0] G:R 8:8 little endian */
+#define DRM_FORMAT_RG88		fourcc_code('R', 'G', '8', '8') /* [15:0] R:G 8:8 native endian */
+#define DRM_FORMAT_GR88		fourcc_code('G', 'R', '8', '8') /* [15:0] G:R 8:8 native endian */
 
 /* 32 bpp RG */
-#define DRM_FORMAT_RG1616	fourcc_code('R', 'G', '3', '2') /* [31:0] R:G 16:16 little endian */
-#define DRM_FORMAT_GR1616	fourcc_code('G', 'R', '3', '2') /* [31:0] G:R 16:16 little endian */
+#define DRM_FORMAT_RG1616	fourcc_code('R', 'G', '3', '2') /* [31:0] R:G 16:16 native endian */
+#define DRM_FORMAT_GR1616	fourcc_code('G', 'R', '3', '2') /* [31:0] G:R 16:16 native endian */
 
 /* 8 bpp RGB */
 #define DRM_FORMAT_RGB332	fourcc_code('R', 'G', 'B', '8') /* [7:0] R:G:B 3:3:2 */
 #define DRM_FORMAT_BGR233	fourcc_code('B', 'G', 'R', '8') /* [7:0] B:G:R 2:3:3 */
 
 /* 16 bpp RGB */
-#define DRM_FORMAT_XRGB4444	fourcc_code('X', 'R', '1', '2') /* [15:0] x:R:G:B 4:4:4:4 little endian */
-#define DRM_FORMAT_XBGR4444	fourcc_code('X', 'B', '1', '2') /* [15:0] x:B:G:R 4:4:4:4 little endian */
-#define DRM_FORMAT_RGBX4444	fourcc_code('R', 'X', '1', '2') /* [15:0] R:G:B:x 4:4:4:4 little endian */
-#define DRM_FORMAT_BGRX4444	fourcc_code('B', 'X', '1', '2') /* [15:0] B:G:R:x 4:4:4:4 little endian */
+#define DRM_FORMAT_XRGB4444	fourcc_code('X', 'R', '1', '2') /* [15:0] x:R:G:B 4:4:4:4 native endian */
+#define DRM_FORMAT_XBGR4444	fourcc_code('X', 'B', '1', '2') /* [15:0] x:B:G:R 4:4:4:4 native endian */
+#define DRM_FORMAT_RGBX4444	fourcc_code('R', 'X', '1', '2') /* [15:0] R:G:B:x 4:4:4:4 native endian */
+#define DRM_FORMAT_BGRX4444	fourcc_code('B', 'X', '1', '2') /* [15:0] B:G:R:x 4:4:4:4 native endian */
 
-#define DRM_FORMAT_ARGB4444	fourcc_code('A', 'R', '1', '2') /* [15:0] A:R:G:B 4:4:4:4 little endian */
-#define DRM_FORMAT_ABGR4444	fourcc_code('A', 'B', '1', '2') /* [15:0] A:B:G:R 4:4:4:4 little endian */
-#define DRM_FORMAT_RGBA4444	fourcc_code('R', 'A', '1', '2') /* [15:0] R:G:B:A 4:4:4:4 little endian */
-#define DRM_FORMAT_BGRA4444	fourcc_code('B', 'A', '1', '2') /* [15:0] B:G:R:A 4:4:4:4 little endian */
+#define DRM_FORMAT_ARGB4444	fourcc_code('A', 'R', '1', '2') /* [15:0] A:R:G:B 4:4:4:4 native endian */
+#define DRM_FORMAT_ABGR4444	fourcc_code('A', 'B', '1', '2') /* [15:0] A:B:G:R 4:4:4:4 native endian */
+#define DRM_FORMAT_RGBA4444	fourcc_code('R', 'A', '1', '2') /* [15:0] R:G:B:A 4:4:4:4 native endian */
+#define DRM_FORMAT_BGRA4444	fourcc_code('B', 'A', '1', '2') /* [15:0] B:G:R:A 4:4:4:4 native endian */
 
-#define DRM_FORMAT_XRGB1555	fourcc_code('X', 'R', '1', '5') /* [15:0] x:R:G:B 1:5:5:5 little endian */
-#define DRM_FORMAT_XBGR1555	fourcc_code('X', 'B', '1', '5') /* [15:0] x:B:G:R 1:5:5:5 little endian */
-#define DRM_FORMAT_RGBX5551	fourcc_code('R', 'X', '1', '5') /* [15:0] R:G:B:x 5:5:5:1 little endian */
-#define DRM_FORMAT_BGRX5551	fourcc_code('B', 'X', '1', '5') /* [15:0] B:G:R:x 5:5:5:1 little endian */
+#define DRM_FORMAT_XRGB1555	fourcc_code('X', 'R', '1', '5') /* [15:0] x:R:G:B 1:5:5:5 native endian */
+#define DRM_FORMAT_XBGR1555	fourcc_code('X', 'B', '1', '5') /* [15:0] x:B:G:R 1:5:5:5 native endian */
+#define DRM_FORMAT_RGBX5551	fourcc_code('R', 'X', '1', '5') /* [15:0] R:G:B:x 5:5:5:1 native endian */
+#define DRM_FORMAT_BGRX5551	fourcc_code('B', 'X', '1', '5') /* [15:0] B:G:R:x 5:5:5:1 native endian */
 
-#define DRM_FORMAT_ARGB1555	fourcc_code('A', 'R', '1', '5') /* [15:0] A:R:G:B 1:5:5:5 little endian */
-#define DRM_FORMAT_ABGR1555	fourcc_code('A', 'B', '1', '5') /* [15:0] A:B:G:R 1:5:5:5 little endian */
-#define DRM_FORMAT_RGBA5551	fourcc_code('R', 'A', '1', '5') /* [15:0] R:G:B:A 5:5:5:1 little endian */
-#define DRM_FORMAT_BGRA5551	fourcc_code('B', 'A', '1', '5') /* [15:0] B:G:R:A 5:5:5:1 little endian */
+#define DRM_FORMAT_ARGB1555	fourcc_code('A', 'R', '1', '5') /* [15:0] A:R:G:B 1:5:5:5 native endian */
+#define DRM_FORMAT_ABGR1555	fourcc_code('A', 'B', '1', '5') /* [15:0] A:B:G:R 1:5:5:5 native endian */
+#define DRM_FORMAT_RGBA5551	fourcc_code('R', 'A', '1', '5') /* [15:0] R:G:B:A 5:5:5:1 native endian */
+#define DRM_FORMAT_BGRA5551	fourcc_code('B', 'A', '1', '5') /* [15:0] B:G:R:A 5:5:5:1 native endian */
 
-#define DRM_FORMAT_RGB565	fourcc_code('R', 'G', '1', '6') /* [15:0] R:G:B 5:6:5 little endian */
-#define DRM_FORMAT_BGR565	fourcc_code('B', 'G', '1', '6') /* [15:0] B:G:R 5:6:5 little endian */
+#define DRM_FORMAT_RGB565	fourcc_code('R', 'G', '1', '6') /* [15:0] R:G:B 5:6:5 native endian */
+#define DRM_FORMAT_BGR565	fourcc_code('B', 'G', '1', '6') /* [15:0] B:G:R 5:6:5 native endian */
 
 /* 24 bpp RGB */
-#define DRM_FORMAT_RGB888	fourcc_code('R', 'G', '2', '4') /* [23:0] R:G:B little endian */
-#define DRM_FORMAT_BGR888	fourcc_code('B', 'G', '2', '4') /* [23:0] B:G:R little endian */
+#define DRM_FORMAT_RGB888	fourcc_code('R', 'G', '2', '4') /* [23:0] R:G:B native endian */
+#define DRM_FORMAT_BGR888	fourcc_code('B', 'G', '2', '4') /* [23:0] B:G:R native endian */
 
 /* 32 bpp RGB */
-#define DRM_FORMAT_XRGB8888	fourcc_code('X', 'R', '2', '4') /* [31:0] x:R:G:B 8:8:8:8 little endian */
-#define DRM_FORMAT_XBGR8888	fourcc_code('X', 'B', '2', '4') /* [31:0] x:B:G:R 8:8:8:8 little endian */
-#define DRM_FORMAT_RGBX8888	fourcc_code('R', 'X', '2', '4') /* [31:0] R:G:B:x 8:8:8:8 little endian */
-#define DRM_FORMAT_BGRX8888	fourcc_code('B', 'X', '2', '4') /* [31:0] B:G:R:x 8:8:8:8 little endian */
+#define DRM_FORMAT_XRGB8888	fourcc_code('X', 'R', '2', '4') /* [31:0] x:R:G:B 8:8:8:8 native endian */
+#define DRM_FORMAT_XBGR8888	fourcc_code('X', 'B', '2', '4') /* [31:0] x:B:G:R 8:8:8:8 native endian */
+#define DRM_FORMAT_RGBX8888	fourcc_code('R', 'X', '2', '4') /* [31:0] R:G:B:x 8:8:8:8 native endian */
+#define DRM_FORMAT_BGRX8888	fourcc_code('B', 'X', '2', '4') /* [31:0] B:G:R:x 8:8:8:8 native endian */
 
-#define DRM_FORMAT_ARGB8888	fourcc_code('A', 'R', '2', '4') /* [31:0] A:R:G:B 8:8:8:8 little endian */
-#define DRM_FORMAT_ABGR8888	fourcc_code('A', 'B', '2', '4') /* [31:0] A:B:G:R 8:8:8:8 little endian */
-#define DRM_FORMAT_RGBA8888	fourcc_code('R', 'A', '2', '4') /* [31:0] R:G:B:A 8:8:8:8 little endian */
-#define DRM_FORMAT_BGRA8888	fourcc_code('B', 'A', '2', '4') /* [31:0] B:G:R:A 8:8:8:8 little endian */
+#define DRM_FORMAT_ARGB8888	fourcc_code('A', 'R', '2', '4') /* [31:0] A:R:G:B 8:8:8:8 native endian */
+#define DRM_FORMAT_ABGR8888	fourcc_code('A', 'B', '2', '4') /* [31:0] A:B:G:R 8:8:8:8 native endian */
+#define DRM_FORMAT_RGBA8888	fourcc_code('R', 'A', '2', '4') /* [31:0] R:G:B:A 8:8:8:8 native endian */
+#define DRM_FORMAT_BGRA8888	fourcc_code('B', 'A', '2', '4') /* [31:0] B:G:R:A 8:8:8:8 native endian */
 
-#define DRM_FORMAT_XRGB2101010	fourcc_code('X', 'R', '3', '0') /* [31:0] x:R:G:B 2:10:10:10 little endian */
-#define DRM_FORMAT_XBGR2101010	fourcc_code('X', 'B', '3', '0') /* [31:0] x:B:G:R 2:10:10:10 little endian */
-#define DRM_FORMAT_RGBX1010102	fourcc_code('R', 'X', '3', '0') /* [31:0] R:G:B:x 10:10:10:2 little endian */
-#define DRM_FORMAT_BGRX1010102	fourcc_code('B', 'X', '3', '0') /* [31:0] B:G:R:x 10:10:10:2 little endian */
+#define DRM_FORMAT_XRGB2101010	fourcc_code('X', 'R', '3', '0') /* [31:0] x:R:G:B 2:10:10:10 native endian */
+#define DRM_FORMAT_XBGR2101010	fourcc_code('X', 'B', '3', '0') /* [31:0] x:B:G:R 2:10:10:10 native endian */
+#define DRM_FORMAT_RGBX1010102	fourcc_code('R', 'X', '3', '0') /* [31:0] R:G:B:x 10:10:10:2 native endian */
+#define DRM_FORMAT_BGRX1010102	fourcc_code('B', 'X', '3', '0') /* [31:0] B:G:R:x 10:10:10:2 native endian */
 
-#define DRM_FORMAT_ARGB2101010	fourcc_code('A', 'R', '3', '0') /* [31:0] A:R:G:B 2:10:10:10 little endian */
-#define DRM_FORMAT_ABGR2101010	fourcc_code('A', 'B', '3', '0') /* [31:0] A:B:G:R 2:10:10:10 little endian */
-#define DRM_FORMAT_RGBA1010102	fourcc_code('R', 'A', '3', '0') /* [31:0] R:G:B:A 10:10:10:2 little endian */
-#define DRM_FORMAT_BGRA1010102	fourcc_code('B', 'A', '3', '0') /* [31:0] B:G:R:A 10:10:10:2 little endian */
+#define DRM_FORMAT_ARGB2101010	fourcc_code('A', 'R', '3', '0') /* [31:0] A:R:G:B 2:10:10:10 native endian */
+#define DRM_FORMAT_ABGR2101010	fourcc_code('A', 'B', '3', '0') /* [31:0] A:B:G:R 2:10:10:10 native endian */
+#define DRM_FORMAT_RGBA1010102	fourcc_code('R', 'A', '3', '0') /* [31:0] R:G:B:A 10:10:10:2 native endian */
+#define DRM_FORMAT_BGRA1010102	fourcc_code('B', 'A', '3', '0') /* [31:0] B:G:R:A 10:10:10:2 native endian */
 
 /* packed YCbCr */
 #define DRM_FORMAT_YUYV		fourcc_code('Y', 'U', 'Y', 'V') /* [31:0] Cr0:Y1:Cb0:Y0 8:8:8:8 little endian */
-- 
2.9.3

Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 1993 bytes --]

On Fri, 21 Apr 2017 09:58:24 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

> While working on graphics support for virtual machines on ppc64 (which
> exists in both little and big endian variants) I've figured the comments
> for various drm fourcc formats in the header file don't match reality.
> 
> Comments says the RGB formats are little endian, but in practice they
> are native endian.  Look at the drm_mode_legacy_fb_format() helper.  It
> maps -- for example -- bpp/depth 32/24 to DRM_FORMAT_XRGB8888, no matter
> whenever the machine is little endian or big endian.  The users of this
> function (fbdev emulation, DRM_IOCTL_MODE_ADDFB) expect the framebuffer
> is native endian, not little endian.  Most userspace also operates on
> native endian only.
> 
> So, go update the comments for all 16+24+32 bpp RGB formats.
> 
> Leaving the yuv formats as-is.  I have no idea if and how those are used
> on bigendian machines.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Pekka Paalanen <ppaalanen@gmail.com>
> Cc: Ilia Mirkin <imirkin@alum.mit.edu>
> Cc: Michel Dänzer <michel@daenzer.net>
> Cc: Alex Deucher <alexdeucher@gmail.com>
> Cc: amd-gfx@lists.freedesktop.org
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/uapi/drm/drm_fourcc.h | 82 +++++++++++++++++++++----------------------
>  1 file changed, 41 insertions(+), 41 deletions(-)

Hi,

just an idea - since we are not sure how the remaining formats are being
used, should those be marked somehow uncertain whether they are little
or native endian?

Otherwise the documentation will guide people to believe those are
certain, which OTOH might not be bad, because then it will make them
certain over time. Unless we would prefer everything to be native
endian?

This might be a chance to choose the endianess (native vs. little) for
all formats. Should we take it? Prefer native?


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.

[Gerd Hoffmann]

  Hi,

> > Leaving the yuv formats as-is.  I have no idea if and how those are used
> > on bigendian machines.

> just an idea - since we are not sure how the remaining formats are being
> used, should those be marked somehow uncertain whether they are little
> or native endian?

ATM the yuv don't have any byte order annotations, and I simply left
them that way.  So it is as clear/unclear as before.

IIRC someone mentioned that for the yuv fourccs there actually is some
standard about the exact ordering.  Anyone has a good reference?  We
could stick a link to it into a comment.

cheers,
  Gerd

Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.

[Ville Syrjälä]

On Fri, Apr 21, 2017 at 11:38:28AM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > > Leaving the yuv formats as-is.  I have no idea if and how those are used
> > > on bigendian machines.
> 
> > just an idea - since we are not sure how the remaining formats are being
> > used, should those be marked somehow uncertain whether they are little
> > or native endian?
> 
> ATM the yuv don't have any byte order annotations, and I simply left
> them that way.  So it is as clear/unclear as before.

Eh? Everything that is affected by byte order has the relevant comments.
If they don't, then that's a bug.

> 
> IIRC someone mentioned that for the yuv fourccs there actually is some
> standard about the exact ordering.  Anyone has a good reference?  We
> could stick a link to it into a comment.

The "standard" is fourcc. Whether there is any official reference for
that is unclear. That's exactly why I added the explicit comments into
drm_fourcc.h so that people don't have to go trawling the internets
looking for information on what each pixel format might mean.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.

[Ville Syrjälä]

On Fri, Apr 21, 2017 at 09:58:24AM +0200, Gerd Hoffmann wrote:
> While working on graphics support for virtual machines on ppc64 (which
> exists in both little and big endian variants) I've figured the comments
> for various drm fourcc formats in the header file don't match reality.
> 
> Comments says the RGB formats are little endian, but in practice they
> are native endian.  Look at the drm_mode_legacy_fb_format() helper.  It
> maps -- for example -- bpp/depth 32/24 to DRM_FORMAT_XRGB8888, no matter
> whenever the machine is little endian or big endian.  The users of this
> function (fbdev emulation, DRM_IOCTL_MODE_ADDFB) expect the framebuffer
> is native endian, not little endian.  Most userspace also operates on
> native endian only.

I'm not a fan of "native". Native to what? "CPU" or "host" is what I'd
call it.

And what about the mxied endian case? Are you just going to pretend it
doesn't exist or what?

> 
> So, go update the comments for all 16+24+32 bpp RGB formats.
> 
> Leaving the yuv formats as-is.  I have no idea if and how those are used
> on bigendian machines.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Pekka Paalanen <ppaalanen@gmail.com>
> Cc: Ilia Mirkin <imirkin@alum.mit.edu>
> Cc: Michel Dänzer <michel@daenzer.net>
> Cc: Alex Deucher <alexdeucher@gmail.com>
> Cc: amd-gfx@lists.freedesktop.org
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/uapi/drm/drm_fourcc.h | 82 +++++++++++++++++++++----------------------
>  1 file changed, 41 insertions(+), 41 deletions(-)
> 
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index 995c8f9..1579765 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -42,68 +42,68 @@ extern "C" {
>  #define DRM_FORMAT_R8		fourcc_code('R', '8', ' ', ' ') /* [7:0] R */
>  
>  /* 16 bpp Red */
> -#define DRM_FORMAT_R16		fourcc_code('R', '1', '6', ' ') /* [15:0] R little endian */
> +#define DRM_FORMAT_R16		fourcc_code('R', '1', '6', ' ') /* [15:0] R native endian */
>  
>  /* 16 bpp RG */
> -#define DRM_FORMAT_RG88		fourcc_code('R', 'G', '8', '8') /* [15:0] R:G 8:8 little endian */
> -#define DRM_FORMAT_GR88		fourcc_code('G', 'R', '8', '8') /* [15:0] G:R 8:8 little endian */
> +#define DRM_FORMAT_RG88		fourcc_code('R', 'G', '8', '8') /* [15:0] R:G 8:8 native endian */
> +#define DRM_FORMAT_GR88		fourcc_code('G', 'R', '8', '8') /* [15:0] G:R 8:8 native endian */
>  
>  /* 32 bpp RG */
> -#define DRM_FORMAT_RG1616	fourcc_code('R', 'G', '3', '2') /* [31:0] R:G 16:16 little endian */
> -#define DRM_FORMAT_GR1616	fourcc_code('G', 'R', '3', '2') /* [31:0] G:R 16:16 little endian */
> +#define DRM_FORMAT_RG1616	fourcc_code('R', 'G', '3', '2') /* [31:0] R:G 16:16 native endian */
> +#define DRM_FORMAT_GR1616	fourcc_code('G', 'R', '3', '2') /* [31:0] G:R 16:16 native endian */
>  
>  /* 8 bpp RGB */
>  #define DRM_FORMAT_RGB332	fourcc_code('R', 'G', 'B', '8') /* [7:0] R:G:B 3:3:2 */
>  #define DRM_FORMAT_BGR233	fourcc_code('B', 'G', 'R', '8') /* [7:0] B:G:R 2:3:3 */
>  
>  /* 16 bpp RGB */
> -#define DRM_FORMAT_XRGB4444	fourcc_code('X', 'R', '1', '2') /* [15:0] x:R:G:B 4:4:4:4 little endian */
> -#define DRM_FORMAT_XBGR4444	fourcc_code('X', 'B', '1', '2') /* [15:0] x:B:G:R 4:4:4:4 little endian */
> -#define DRM_FORMAT_RGBX4444	fourcc_code('R', 'X', '1', '2') /* [15:0] R:G:B:x 4:4:4:4 little endian */
> -#define DRM_FORMAT_BGRX4444	fourcc_code('B', 'X', '1', '2') /* [15:0] B:G:R:x 4:4:4:4 little endian */
> +#define DRM_FORMAT_XRGB4444	fourcc_code('X', 'R', '1', '2') /* [15:0] x:R:G:B 4:4:4:4 native endian */
> +#define DRM_FORMAT_XBGR4444	fourcc_code('X', 'B', '1', '2') /* [15:0] x:B:G:R 4:4:4:4 native endian */
> +#define DRM_FORMAT_RGBX4444	fourcc_code('R', 'X', '1', '2') /* [15:0] R:G:B:x 4:4:4:4 native endian */
> +#define DRM_FORMAT_BGRX4444	fourcc_code('B', 'X', '1', '2') /* [15:0] B:G:R:x 4:4:4:4 native endian */
>  
> -#define DRM_FORMAT_ARGB4444	fourcc_code('A', 'R', '1', '2') /* [15:0] A:R:G:B 4:4:4:4 little endian */
> -#define DRM_FORMAT_ABGR4444	fourcc_code('A', 'B', '1', '2') /* [15:0] A:B:G:R 4:4:4:4 little endian */
> -#define DRM_FORMAT_RGBA4444	fourcc_code('R', 'A', '1', '2') /* [15:0] R:G:B:A 4:4:4:4 little endian */
> -#define DRM_FORMAT_BGRA4444	fourcc_code('B', 'A', '1', '2') /* [15:0] B:G:R:A 4:4:4:4 little endian */
> +#define DRM_FORMAT_ARGB4444	fourcc_code('A', 'R', '1', '2') /* [15:0] A:R:G:B 4:4:4:4 native endian */
> +#define DRM_FORMAT_ABGR4444	fourcc_code('A', 'B', '1', '2') /* [15:0] A:B:G:R 4:4:4:4 native endian */
> +#define DRM_FORMAT_RGBA4444	fourcc_code('R', 'A', '1', '2') /* [15:0] R:G:B:A 4:4:4:4 native endian */
> +#define DRM_FORMAT_BGRA4444	fourcc_code('B', 'A', '1', '2') /* [15:0] B:G:R:A 4:4:4:4 native endian */
>  
> -#define DRM_FORMAT_XRGB1555	fourcc_code('X', 'R', '1', '5') /* [15:0] x:R:G:B 1:5:5:5 little endian */
> -#define DRM_FORMAT_XBGR1555	fourcc_code('X', 'B', '1', '5') /* [15:0] x:B:G:R 1:5:5:5 little endian */
> -#define DRM_FORMAT_RGBX5551	fourcc_code('R', 'X', '1', '5') /* [15:0] R:G:B:x 5:5:5:1 little endian */
> -#define DRM_FORMAT_BGRX5551	fourcc_code('B', 'X', '1', '5') /* [15:0] B:G:R:x 5:5:5:1 little endian */
> +#define DRM_FORMAT_XRGB1555	fourcc_code('X', 'R', '1', '5') /* [15:0] x:R:G:B 1:5:5:5 native endian */
> +#define DRM_FORMAT_XBGR1555	fourcc_code('X', 'B', '1', '5') /* [15:0] x:B:G:R 1:5:5:5 native endian */
> +#define DRM_FORMAT_RGBX5551	fourcc_code('R', 'X', '1', '5') /* [15:0] R:G:B:x 5:5:5:1 native endian */
> +#define DRM_FORMAT_BGRX5551	fourcc_code('B', 'X', '1', '5') /* [15:0] B:G:R:x 5:5:5:1 native endian */
>  
> -#define DRM_FORMAT_ARGB1555	fourcc_code('A', 'R', '1', '5') /* [15:0] A:R:G:B 1:5:5:5 little endian */
> -#define DRM_FORMAT_ABGR1555	fourcc_code('A', 'B', '1', '5') /* [15:0] A:B:G:R 1:5:5:5 little endian */
> -#define DRM_FORMAT_RGBA5551	fourcc_code('R', 'A', '1', '5') /* [15:0] R:G:B:A 5:5:5:1 little endian */
> -#define DRM_FORMAT_BGRA5551	fourcc_code('B', 'A', '1', '5') /* [15:0] B:G:R:A 5:5:5:1 little endian */
> +#define DRM_FORMAT_ARGB1555	fourcc_code('A', 'R', '1', '5') /* [15:0] A:R:G:B 1:5:5:5 native endian */
> +#define DRM_FORMAT_ABGR1555	fourcc_code('A', 'B', '1', '5') /* [15:0] A:B:G:R 1:5:5:5 native endian */
> +#define DRM_FORMAT_RGBA5551	fourcc_code('R', 'A', '1', '5') /* [15:0] R:G:B:A 5:5:5:1 native endian */
> +#define DRM_FORMAT_BGRA5551	fourcc_code('B', 'A', '1', '5') /* [15:0] B:G:R:A 5:5:5:1 native endian */
>  
> -#define DRM_FORMAT_RGB565	fourcc_code('R', 'G', '1', '6') /* [15:0] R:G:B 5:6:5 little endian */
> -#define DRM_FORMAT_BGR565	fourcc_code('B', 'G', '1', '6') /* [15:0] B:G:R 5:6:5 little endian */
> +#define DRM_FORMAT_RGB565	fourcc_code('R', 'G', '1', '6') /* [15:0] R:G:B 5:6:5 native endian */
> +#define DRM_FORMAT_BGR565	fourcc_code('B', 'G', '1', '6') /* [15:0] B:G:R 5:6:5 native endian */
>  
>  /* 24 bpp RGB */
> -#define DRM_FORMAT_RGB888	fourcc_code('R', 'G', '2', '4') /* [23:0] R:G:B little endian */
> -#define DRM_FORMAT_BGR888	fourcc_code('B', 'G', '2', '4') /* [23:0] B:G:R little endian */
> +#define DRM_FORMAT_RGB888	fourcc_code('R', 'G', '2', '4') /* [23:0] R:G:B native endian */
> +#define DRM_FORMAT_BGR888	fourcc_code('B', 'G', '2', '4') /* [23:0] B:G:R native endian */
>  
>  /* 32 bpp RGB */
> -#define DRM_FORMAT_XRGB8888	fourcc_code('X', 'R', '2', '4') /* [31:0] x:R:G:B 8:8:8:8 little endian */
> -#define DRM_FORMAT_XBGR8888	fourcc_code('X', 'B', '2', '4') /* [31:0] x:B:G:R 8:8:8:8 little endian */
> -#define DRM_FORMAT_RGBX8888	fourcc_code('R', 'X', '2', '4') /* [31:0] R:G:B:x 8:8:8:8 little endian */
> -#define DRM_FORMAT_BGRX8888	fourcc_code('B', 'X', '2', '4') /* [31:0] B:G:R:x 8:8:8:8 little endian */
> +#define DRM_FORMAT_XRGB8888	fourcc_code('X', 'R', '2', '4') /* [31:0] x:R:G:B 8:8:8:8 native endian */
> +#define DRM_FORMAT_XBGR8888	fourcc_code('X', 'B', '2', '4') /* [31:0] x:B:G:R 8:8:8:8 native endian */
> +#define DRM_FORMAT_RGBX8888	fourcc_code('R', 'X', '2', '4') /* [31:0] R:G:B:x 8:8:8:8 native endian */
> +#define DRM_FORMAT_BGRX8888	fourcc_code('B', 'X', '2', '4') /* [31:0] B:G:R:x 8:8:8:8 native endian */
>  
> -#define DRM_FORMAT_ARGB8888	fourcc_code('A', 'R', '2', '4') /* [31:0] A:R:G:B 8:8:8:8 little endian */
> -#define DRM_FORMAT_ABGR8888	fourcc_code('A', 'B', '2', '4') /* [31:0] A:B:G:R 8:8:8:8 little endian */
> -#define DRM_FORMAT_RGBA8888	fourcc_code('R', 'A', '2', '4') /* [31:0] R:G:B:A 8:8:8:8 little endian */
> -#define DRM_FORMAT_BGRA8888	fourcc_code('B', 'A', '2', '4') /* [31:0] B:G:R:A 8:8:8:8 little endian */
> +#define DRM_FORMAT_ARGB8888	fourcc_code('A', 'R', '2', '4') /* [31:0] A:R:G:B 8:8:8:8 native endian */
> +#define DRM_FORMAT_ABGR8888	fourcc_code('A', 'B', '2', '4') /* [31:0] A:B:G:R 8:8:8:8 native endian */
> +#define DRM_FORMAT_RGBA8888	fourcc_code('R', 'A', '2', '4') /* [31:0] R:G:B:A 8:8:8:8 native endian */
> +#define DRM_FORMAT_BGRA8888	fourcc_code('B', 'A', '2', '4') /* [31:0] B:G:R:A 8:8:8:8 native endian */
>  
> -#define DRM_FORMAT_XRGB2101010	fourcc_code('X', 'R', '3', '0') /* [31:0] x:R:G:B 2:10:10:10 little endian */
> -#define DRM_FORMAT_XBGR2101010	fourcc_code('X', 'B', '3', '0') /* [31:0] x:B:G:R 2:10:10:10 little endian */
> -#define DRM_FORMAT_RGBX1010102	fourcc_code('R', 'X', '3', '0') /* [31:0] R:G:B:x 10:10:10:2 little endian */
> -#define DRM_FORMAT_BGRX1010102	fourcc_code('B', 'X', '3', '0') /* [31:0] B:G:R:x 10:10:10:2 little endian */
> +#define DRM_FORMAT_XRGB2101010	fourcc_code('X', 'R', '3', '0') /* [31:0] x:R:G:B 2:10:10:10 native endian */
> +#define DRM_FORMAT_XBGR2101010	fourcc_code('X', 'B', '3', '0') /* [31:0] x:B:G:R 2:10:10:10 native endian */
> +#define DRM_FORMAT_RGBX1010102	fourcc_code('R', 'X', '3', '0') /* [31:0] R:G:B:x 10:10:10:2 native endian */
> +#define DRM_FORMAT_BGRX1010102	fourcc_code('B', 'X', '3', '0') /* [31:0] B:G:R:x 10:10:10:2 native endian */
>  
> -#define DRM_FORMAT_ARGB2101010	fourcc_code('A', 'R', '3', '0') /* [31:0] A:R:G:B 2:10:10:10 little endian */
> -#define DRM_FORMAT_ABGR2101010	fourcc_code('A', 'B', '3', '0') /* [31:0] A:B:G:R 2:10:10:10 little endian */
> -#define DRM_FORMAT_RGBA1010102	fourcc_code('R', 'A', '3', '0') /* [31:0] R:G:B:A 10:10:10:2 little endian */
> -#define DRM_FORMAT_BGRA1010102	fourcc_code('B', 'A', '3', '0') /* [31:0] B:G:R:A 10:10:10:2 little endian */
> +#define DRM_FORMAT_ARGB2101010	fourcc_code('A', 'R', '3', '0') /* [31:0] A:R:G:B 2:10:10:10 native endian */
> +#define DRM_FORMAT_ABGR2101010	fourcc_code('A', 'B', '3', '0') /* [31:0] A:B:G:R 2:10:10:10 native endian */
> +#define DRM_FORMAT_RGBA1010102	fourcc_code('R', 'A', '3', '0') /* [31:0] R:G:B:A 10:10:10:2 native endian */
> +#define DRM_FORMAT_BGRA1010102	fourcc_code('B', 'A', '3', '0') /* [31:0] B:G:R:A 10:10:10:2 native endian */
>  
>  /* packed YCbCr */
>  #define DRM_FORMAT_YUYV		fourcc_code('Y', 'U', 'Y', 'V') /* [31:0] Cr0:Y1:Cb0:Y0 8:8:8:8 little endian */
> -- 
> 2.9.3

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.

[Gerd Hoffmann]

On Fr, 2017-04-21 at 12:25 +0300, Ville Syrjälä wrote:
> On Fri, Apr 21, 2017 at 09:58:24AM +0200, Gerd Hoffmann wrote:
> > While working on graphics support for virtual machines on ppc64 (which
> > exists in both little and big endian variants) I've figured the comments
> > for various drm fourcc formats in the header file don't match reality.
> > 
> > Comments says the RGB formats are little endian, but in practice they
> > are native endian.  Look at the drm_mode_legacy_fb_format() helper.  It
> > maps -- for example -- bpp/depth 32/24 to DRM_FORMAT_XRGB8888, no matter
> > whenever the machine is little endian or big endian.  The users of this
> > function (fbdev emulation, DRM_IOCTL_MODE_ADDFB) expect the framebuffer
> > is native endian, not little endian.  Most userspace also operates on
> > native endian only.
> 
> I'm not a fan of "native". Native to what? "CPU" or "host" is what I'd
> call it.

native == whatever the cpu is using.

I personally find "native" more intuitive, but at the end of the day I
don't mind much.  If people prefer "host" over "native" I'll change it.

> And what about the mxied endian case? Are you just going to pretend it
> doesn't exist or what?

What exactly do you mean with "mixed endian"?  The powerpc case, where
kernel + userspace can run in either big or little endian mode?  Or
something else?

cheers,
  Gerd

Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.

[Ville Syrjälä]

On Fri, Apr 21, 2017 at 11:50:18AM +0200, Gerd Hoffmann wrote:
> On Fr, 2017-04-21 at 12:25 +0300, Ville Syrjälä wrote:
> > On Fri, Apr 21, 2017 at 09:58:24AM +0200, Gerd Hoffmann wrote:
> > > While working on graphics support for virtual machines on ppc64 (which
> > > exists in both little and big endian variants) I've figured the comments
> > > for various drm fourcc formats in the header file don't match reality.
> > > 
> > > Comments says the RGB formats are little endian, but in practice they
> > > are native endian.  Look at the drm_mode_legacy_fb_format() helper.  It
> > > maps -- for example -- bpp/depth 32/24 to DRM_FORMAT_XRGB8888, no matter
> > > whenever the machine is little endian or big endian.  The users of this
> > > function (fbdev emulation, DRM_IOCTL_MODE_ADDFB) expect the framebuffer
> > > is native endian, not little endian.  Most userspace also operates on
> > > native endian only.
> > 
> > I'm not a fan of "native". Native to what? "CPU" or "host" is what I'd
> > call it.
> 
> native == whatever the cpu is using.
> 
> I personally find "native" more intuitive, but at the end of the day I
> don't mind much.  If people prefer "host" over "native" I'll change it.
> 
> > And what about the mxied endian case? Are you just going to pretend it
> > doesn't exist or what?
> 
> What exactly do you mean with "mixed endian"?  The powerpc case, where
> kernel + userspace can run in either big or little endian mode?  Or
> something else?

Big endian CPU and little endian GPU. I think that should be the most
common case these days.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.

[Ville Syrjälä]

On Fri, Apr 21, 2017 at 11:50:18AM +0200, Gerd Hoffmann wrote:
> On Fr, 2017-04-21 at 12:25 +0300, Ville Syrjälä wrote:
> > On Fri, Apr 21, 2017 at 09:58:24AM +0200, Gerd Hoffmann wrote:
> > > While working on graphics support for virtual machines on ppc64 (which
> > > exists in both little and big endian variants) I've figured the comments
> > > for various drm fourcc formats in the header file don't match reality.
> > > 
> > > Comments says the RGB formats are little endian, but in practice they
> > > are native endian.  Look at the drm_mode_legacy_fb_format() helper.  It
> > > maps -- for example -- bpp/depth 32/24 to DRM_FORMAT_XRGB8888, no matter
> > > whenever the machine is little endian or big endian.  The users of this
> > > function (fbdev emulation, DRM_IOCTL_MODE_ADDFB) expect the framebuffer
> > > is native endian, not little endian.  Most userspace also operates on
> > > native endian only.
> > 
> > I'm not a fan of "native". Native to what? "CPU" or "host" is what I'd
> > call it.
> 
> native == whatever the cpu is using.
> 
> I personally find "native" more intuitive, but at the end of the day I
> don't mind much.  If people prefer "host" over "native" I'll change it.

"native" to me feels more like "native to the GPU" since these things
really are tied to the GPU not the CPU. That's also why I went with the
explicit endianness originally so that the driver could properly declare
what the GPU supports.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 2120 bytes --]

On Fri, 21 Apr 2017 14:08:04 +0300
Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:

> On Fri, Apr 21, 2017 at 11:50:18AM +0200, Gerd Hoffmann wrote:
> > On Fr, 2017-04-21 at 12:25 +0300, Ville Syrjälä wrote:  
> > > On Fri, Apr 21, 2017 at 09:58:24AM +0200, Gerd Hoffmann wrote:  
> > > > While working on graphics support for virtual machines on ppc64 (which
> > > > exists in both little and big endian variants) I've figured the comments
> > > > for various drm fourcc formats in the header file don't match reality.
> > > > 
> > > > Comments says the RGB formats are little endian, but in practice they
> > > > are native endian.  Look at the drm_mode_legacy_fb_format() helper.  It
> > > > maps -- for example -- bpp/depth 32/24 to DRM_FORMAT_XRGB8888, no matter
> > > > whenever the machine is little endian or big endian.  The users of this
> > > > function (fbdev emulation, DRM_IOCTL_MODE_ADDFB) expect the framebuffer
> > > > is native endian, not little endian.  Most userspace also operates on
> > > > native endian only.  
> > > 
> > > I'm not a fan of "native". Native to what? "CPU" or "host" is what I'd
> > > call it.  
> > 
> > native == whatever the cpu is using.
> > 
> > I personally find "native" more intuitive, but at the end of the day I
> > don't mind much.  If people prefer "host" over "native" I'll change it.  
> 
> "native" to me feels more like "native to the GPU" since these things
> really are tied to the GPU not the CPU. That's also why I went with the
> explicit endianness originally so that the driver could properly declare
> what the GPU supports.

Hi,

yeah, one should really be explicit on which component's endianess does
"native" refer to. I just can't imagine "GPU native" to ever be an
option, because then userspace needs a way to discover what the
GPU endianess is, and I believe that would only deepen the swamp, not
drain it, because suddenly you need two enums to describe one format.

Ville, wording aside, what do think about changing the endianess
definition? Is it going in the right direction?


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.

[Ville Syrjälä]

On Fri, Apr 21, 2017 at 02:40:18PM +0300, Pekka Paalanen wrote:
> On Fri, 21 Apr 2017 14:08:04 +0300
> Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> 
> > On Fri, Apr 21, 2017 at 11:50:18AM +0200, Gerd Hoffmann wrote:
> > > On Fr, 2017-04-21 at 12:25 +0300, Ville Syrjälä wrote:  
> > > > On Fri, Apr 21, 2017 at 09:58:24AM +0200, Gerd Hoffmann wrote:  
> > > > > While working on graphics support for virtual machines on ppc64 (which
> > > > > exists in both little and big endian variants) I've figured the comments
> > > > > for various drm fourcc formats in the header file don't match reality.
> > > > > 
> > > > > Comments says the RGB formats are little endian, but in practice they
> > > > > are native endian.  Look at the drm_mode_legacy_fb_format() helper.  It
> > > > > maps -- for example -- bpp/depth 32/24 to DRM_FORMAT_XRGB8888, no matter
> > > > > whenever the machine is little endian or big endian.  The users of this
> > > > > function (fbdev emulation, DRM_IOCTL_MODE_ADDFB) expect the framebuffer
> > > > > is native endian, not little endian.  Most userspace also operates on
> > > > > native endian only.  
> > > > 
> > > > I'm not a fan of "native". Native to what? "CPU" or "host" is what I'd
> > > > call it.  
> > > 
> > > native == whatever the cpu is using.
> > > 
> > > I personally find "native" more intuitive, but at the end of the day I
> > > don't mind much.  If people prefer "host" over "native" I'll change it.  
> > 
> > "native" to me feels more like "native to the GPU" since these things
> > really are tied to the GPU not the CPU. That's also why I went with the
> > explicit endianness originally so that the driver could properly declare
> > what the GPU supports.
> 
> Hi,
> 
> yeah, one should really be explicit on which component's endianess does
> "native" refer to. I just can't imagine "GPU native" to ever be an
> option, because then userspace needs a way to discover what the
> GPU endianess is,

It has to know that. How else would it know how to write the bytes into
memory in the right order for the GPU to consume, or read the stuff the
GPU produced?

> and I believe that would only deepen the swamp, not
> drain it, because suddenly you need two enums to describe one format.
> 
> Ville, wording aside, what do think about changing the endianess
> definition? Is it going in the right direction?

I don't think so, but I guess I'm in the minority.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.

[Christian König]

Am 21.04.2017 um 13:49 schrieb Ville Syrjälä:
> On Fri, Apr 21, 2017 at 02:40:18PM +0300, Pekka Paalanen wrote:
>> On Fri, 21 Apr 2017 14:08:04 +0300
>> Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>>
>>> On Fri, Apr 21, 2017 at 11:50:18AM +0200, Gerd Hoffmann wrote:
>>>> On Fr, 2017-04-21 at 12:25 +0300, Ville Syrjälä wrote:
>>>>> On Fri, Apr 21, 2017 at 09:58:24AM +0200, Gerd Hoffmann wrote:
>>>>>> While working on graphics support for virtual machines on ppc64 (which
>>>>>> exists in both little and big endian variants) I've figured the comments
>>>>>> for various drm fourcc formats in the header file don't match reality.
>>>>>>
>>>>>> Comments says the RGB formats are little endian, but in practice they
>>>>>> are native endian.  Look at the drm_mode_legacy_fb_format() helper.  It
>>>>>> maps -- for example -- bpp/depth 32/24 to DRM_FORMAT_XRGB8888, no matter
>>>>>> whenever the machine is little endian or big endian.  The users of this
>>>>>> function (fbdev emulation, DRM_IOCTL_MODE_ADDFB) expect the framebuffer
>>>>>> is native endian, not little endian.  Most userspace also operates on
>>>>>> native endian only.
>>>>> I'm not a fan of "native". Native to what? "CPU" or "host" is what I'd
>>>>> call it.
>>>> native == whatever the cpu is using.
>>>>
>>>> I personally find "native" more intuitive, but at the end of the day I
>>>> don't mind much.  If people prefer "host" over "native" I'll change it.
>>> "native" to me feels more like "native to the GPU" since these things
>>> really are tied to the GPU not the CPU. That's also why I went with the
>>> explicit endianness originally so that the driver could properly declare
>>> what the GPU supports.
>> Hi,
>>
>> yeah, one should really be explicit on which component's endianess does
>> "native" refer to. I just can't imagine "GPU native" to ever be an
>> option, because then userspace needs a way to discover what the
>> GPU endianess is,
> It has to know that. How else would it know how to write the bytes into
> memory in the right order for the GPU to consume, or read the stuff the
> GPU produced?
>
>> and I believe that would only deepen the swamp, not
>> drain it, because suddenly you need two enums to describe one format.
>>
>> Ville, wording aside, what do think about changing the endianess
>> definition? Is it going in the right direction?
> I don't think so, but I guess I'm in the minority.
I don't think your are in the minority. At least I would clearly say 
those formats should be in a fixed byte order and don't care about the 
CPU in the system.

What I need from the driver side is a consistent description of how the 
bytes in memory map to my hardware. What CPU is in use in the system is 
completely irrelevant for that.

Regards,
Christian.

Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.

[Gerd Hoffmann]

  Hi,

> > I personally find "native" more intuitive, but at the end of the day I
> > don't mind much.  If people prefer "host" over "native" I'll change it.
> 
> "native" to me feels more like "native to the GPU" since these things
> really are tied to the GPU not the CPU.

Ok, then maybe "host" is the better term then, to make clear we talk
about cpu/kernel/userspace byteorder here.

cheers,
  Gerd

Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.

[Christian König]

Am 21.04.2017 um 13:08 schrieb Ville Syrjälä:
> On Fri, Apr 21, 2017 at 11:50:18AM +0200, Gerd Hoffmann wrote:
>> On Fr, 2017-04-21 at 12:25 +0300, Ville Syrjälä wrote:
>>> On Fri, Apr 21, 2017 at 09:58:24AM +0200, Gerd Hoffmann wrote:
>>>> While working on graphics support for virtual machines on ppc64 (which
>>>> exists in both little and big endian variants) I've figured the comments
>>>> for various drm fourcc formats in the header file don't match reality.
>>>>
>>>> Comments says the RGB formats are little endian, but in practice they
>>>> are native endian.  Look at the drm_mode_legacy_fb_format() helper.  It
>>>> maps -- for example -- bpp/depth 32/24 to DRM_FORMAT_XRGB8888, no matter
>>>> whenever the machine is little endian or big endian.  The users of this
>>>> function (fbdev emulation, DRM_IOCTL_MODE_ADDFB) expect the framebuffer
>>>> is native endian, not little endian.  Most userspace also operates on
>>>> native endian only.
>>> I'm not a fan of "native". Native to what? "CPU" or "host" is what I'd
>>> call it.
>> native == whatever the cpu is using.
>>
>> I personally find "native" more intuitive, but at the end of the day I
>> don't mind much.  If people prefer "host" over "native" I'll change it.
> "native" to me feels more like "native to the GPU" since these things
> really are tied to the GPU not the CPU. That's also why I went with the
> explicit endianness originally so that the driver could properly declare
> what the GPU supports.
And to be honest I would really prefer to stick with that approach for 
exactly that reason.

The proposed change would require that drivers have different code path 
for different CPU byte order. Those code path tend to be not tested very 
well and are additional complexity we probably don't want inside the driver.

My personal opinion is that formats in drm_fourcc.h should be 
independent of the CPU byte order and the function 
drm_mode_legacy_fb_format() and drivers depending on that incorrect 
assumption be fixed instead.

Regards,
Christian.

Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.

[Gerd Hoffmann]

  Hi,

> > "native" to me feels more like "native to the GPU" since these things
> > really are tied to the GPU not the CPU. That's also why I went with the
> > explicit endianness originally so that the driver could properly declare
> > what the GPU supports.
> And to be honest I would really prefer to stick with that approach for 
> exactly that reason.
> 
> The proposed change would require that drivers have different code path 
> for different CPU byte order. Those code path tend to be not tested very 
> well and are additional complexity we probably don't want inside the driver.

We can add fixed-endian #defines without too much effort, at least for
the 8 bits per color formats.  In qemu we have the same problem, only
with pixman.  Those formats are native endian too, but often we have to
handle a fixed format, so we did this:

/*
 * pixman image formats are defined to be native endian,
 * that means host byte order on qemu.  So we go define
 * fixed formats here for cases where it is needed, like
 * feeding libjpeg / libpng and writing screenshots.
 */

#ifdef HOST_WORDS_BIGENDIAN
# define PIXMAN_BE_r8g8b8     PIXMAN_r8g8b8
# define PIXMAN_BE_x8r8g8b8   PIXMAN_x8r8g8b8
# define PIXMAN_BE_a8r8g8b8   PIXMAN_a8r8g8b8
# define PIXMAN_BE_b8g8r8x8   PIXMAN_b8g8r8x8
# define PIXMAN_BE_b8g8r8a8   PIXMAN_b8g8r8a8
# define PIXMAN_BE_r8g8b8x8   PIXMAN_r8g8b8x8
# define PIXMAN_BE_r8g8b8a8   PIXMAN_r8g8b8a8
# define PIXMAN_BE_x8b8g8r8   PIXMAN_x8b8g8r8
# define PIXMAN_BE_a8b8g8r8   PIXMAN_a8b8g8r8
# define PIXMAN_LE_x8r8g8b8   PIXMAN_b8g8r8x8
#else
# define PIXMAN_BE_r8g8b8     PIXMAN_b8g8r8
# define PIXMAN_BE_x8r8g8b8   PIXMAN_b8g8r8x8
# define PIXMAN_BE_a8r8g8b8   PIXMAN_b8g8r8a8
# define PIXMAN_BE_b8g8r8x8   PIXMAN_x8r8g8b8
# define PIXMAN_BE_b8g8r8a8   PIXMAN_a8r8g8b8
# define PIXMAN_BE_r8g8b8x8   PIXMAN_x8b8g8r8
# define PIXMAN_BE_r8g8b8a8   PIXMAN_a8b8g8r8
# define PIXMAN_BE_x8b8g8r8   PIXMAN_r8g8b8x8
# define PIXMAN_BE_a8b8g8r8   PIXMAN_r8g8b8a8
# define PIXMAN_LE_x8r8g8b8   PIXMAN_x8r8g8b8
#endif

> My personal opinion is that formats in drm_fourcc.h should be 
> independent of the CPU byte order and the function 
> drm_mode_legacy_fb_format() and drivers depending on that incorrect 
> assumption be fixed instead.

The problem is this isn't a kernel-internal thing any more.  With the
addition of the ADDFB2 ioctl the fourcc codes became part of the
kernel/userspace abi ...

cheers,
  Gerd

Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.

[Christian König]

Am 21.04.2017 um 15:12 schrieb Gerd Hoffmann:
>    Hi,
>
>>> "native" to me feels more like "native to the GPU" since these things
>>> really are tied to the GPU not the CPU. That's also why I went with the
>>> explicit endianness originally so that the driver could properly declare
>>> what the GPU supports.
>> And to be honest I would really prefer to stick with that approach for
>> exactly that reason.
>>
>> The proposed change would require that drivers have different code path
>> for different CPU byte order. Those code path tend to be not tested very
>> well and are additional complexity we probably don't want inside the driver.
> We can add fixed-endian #defines without too much effort, at least for
> the 8 bits per color formats.  In qemu we have the same problem, only
> with pixman.  Those formats are native endian too, but often we have to
> handle a fixed format, so we did this:
>
> /*
>   * pixman image formats are defined to be native endian,
>   * that means host byte order on qemu.  So we go define
>   * fixed formats here for cases where it is needed, like
>   * feeding libjpeg / libpng and writing screenshots.
>   */
>
> #ifdef HOST_WORDS_BIGENDIAN
> # define PIXMAN_BE_r8g8b8     PIXMAN_r8g8b8
> # define PIXMAN_BE_x8r8g8b8   PIXMAN_x8r8g8b8
> # define PIXMAN_BE_a8r8g8b8   PIXMAN_a8r8g8b8
> # define PIXMAN_BE_b8g8r8x8   PIXMAN_b8g8r8x8
> # define PIXMAN_BE_b8g8r8a8   PIXMAN_b8g8r8a8
> # define PIXMAN_BE_r8g8b8x8   PIXMAN_r8g8b8x8
> # define PIXMAN_BE_r8g8b8a8   PIXMAN_r8g8b8a8
> # define PIXMAN_BE_x8b8g8r8   PIXMAN_x8b8g8r8
> # define PIXMAN_BE_a8b8g8r8   PIXMAN_a8b8g8r8
> # define PIXMAN_LE_x8r8g8b8   PIXMAN_b8g8r8x8
> #else
> # define PIXMAN_BE_r8g8b8     PIXMAN_b8g8r8
> # define PIXMAN_BE_x8r8g8b8   PIXMAN_b8g8r8x8
> # define PIXMAN_BE_a8r8g8b8   PIXMAN_b8g8r8a8
> # define PIXMAN_BE_b8g8r8x8   PIXMAN_x8r8g8b8
> # define PIXMAN_BE_b8g8r8a8   PIXMAN_a8r8g8b8
> # define PIXMAN_BE_r8g8b8x8   PIXMAN_x8b8g8r8
> # define PIXMAN_BE_r8g8b8a8   PIXMAN_a8b8g8r8
> # define PIXMAN_BE_x8b8g8r8   PIXMAN_r8g8b8x8
> # define PIXMAN_BE_a8b8g8r8   PIXMAN_r8g8b8a8
> # define PIXMAN_LE_x8r8g8b8   PIXMAN_x8r8g8b8
> #endif

Exactly what Mesa did as well.

>> My personal opinion is that formats in drm_fourcc.h should be
>> independent of the CPU byte order and the function
>> drm_mode_legacy_fb_format() and drivers depending on that incorrect
>> assumption be fixed instead.
> The problem is this isn't a kernel-internal thing any more.  With the
> addition of the ADDFB2 ioctl the fourcc codes became part of the
> kernel/userspace abi ...

I know and that's exactly the reason I'm going to object those changes.

The kernel/userspace abi is fixed and changing it like this could 
potentially break drivers I'm the co-maintainer of. So that whole 
approach is a clear NAK from my side.

If you find a driver or userspace which doesn't use the formats as 
defined in the comments of drm_fourcc.h the fix the driver instead of 
trying to adjust the common header to broken behavior. Cause the later 
will clearly cause problems with drivers who correctly implemented the 
interface.

Regards,
Christian.

>
> cheers,
>    Gerd
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.

[Christian König]

Adding Harry to this mail thread as well, cause is one of the people 
really affected by this.

Christian.

Am 21.04.2017 um 15:21 schrieb Christian König:
> Am 21.04.2017 um 15:12 schrieb Gerd Hoffmann:
>>    Hi,
>>
>>>> "native" to me feels more like "native to the GPU" since these things
>>>> really are tied to the GPU not the CPU. That's also why I went with 
>>>> the
>>>> explicit endianness originally so that the driver could properly 
>>>> declare
>>>> what the GPU supports.
>>> And to be honest I would really prefer to stick with that approach for
>>> exactly that reason.
>>>
>>> The proposed change would require that drivers have different code path
>>> for different CPU byte order. Those code path tend to be not tested 
>>> very
>>> well and are additional complexity we probably don't want inside the 
>>> driver.
>> We can add fixed-endian #defines without too much effort, at least for
>> the 8 bits per color formats.  In qemu we have the same problem, only
>> with pixman.  Those formats are native endian too, but often we have to
>> handle a fixed format, so we did this:
>>
>> /*
>>   * pixman image formats are defined to be native endian,
>>   * that means host byte order on qemu.  So we go define
>>   * fixed formats here for cases where it is needed, like
>>   * feeding libjpeg / libpng and writing screenshots.
>>   */
>>
>> #ifdef HOST_WORDS_BIGENDIAN
>> # define PIXMAN_BE_r8g8b8     PIXMAN_r8g8b8
>> # define PIXMAN_BE_x8r8g8b8   PIXMAN_x8r8g8b8
>> # define PIXMAN_BE_a8r8g8b8   PIXMAN_a8r8g8b8
>> # define PIXMAN_BE_b8g8r8x8   PIXMAN_b8g8r8x8
>> # define PIXMAN_BE_b8g8r8a8   PIXMAN_b8g8r8a8
>> # define PIXMAN_BE_r8g8b8x8   PIXMAN_r8g8b8x8
>> # define PIXMAN_BE_r8g8b8a8   PIXMAN_r8g8b8a8
>> # define PIXMAN_BE_x8b8g8r8   PIXMAN_x8b8g8r8
>> # define PIXMAN_BE_a8b8g8r8   PIXMAN_a8b8g8r8
>> # define PIXMAN_LE_x8r8g8b8   PIXMAN_b8g8r8x8
>> #else
>> # define PIXMAN_BE_r8g8b8     PIXMAN_b8g8r8
>> # define PIXMAN_BE_x8r8g8b8   PIXMAN_b8g8r8x8
>> # define PIXMAN_BE_a8r8g8b8   PIXMAN_b8g8r8a8
>> # define PIXMAN_BE_b8g8r8x8   PIXMAN_x8r8g8b8
>> # define PIXMAN_BE_b8g8r8a8   PIXMAN_a8r8g8b8
>> # define PIXMAN_BE_r8g8b8x8   PIXMAN_x8b8g8r8
>> # define PIXMAN_BE_r8g8b8a8   PIXMAN_a8b8g8r8
>> # define PIXMAN_BE_x8b8g8r8   PIXMAN_r8g8b8x8
>> # define PIXMAN_BE_a8b8g8r8   PIXMAN_r8g8b8a8
>> # define PIXMAN_LE_x8r8g8b8   PIXMAN_x8r8g8b8
>> #endif
>
> Exactly what Mesa did as well.
>
>>> My personal opinion is that formats in drm_fourcc.h should be
>>> independent of the CPU byte order and the function
>>> drm_mode_legacy_fb_format() and drivers depending on that incorrect
>>> assumption be fixed instead.
>> The problem is this isn't a kernel-internal thing any more. With the
>> addition of the ADDFB2 ioctl the fourcc codes became part of the
>> kernel/userspace abi ...
>
> I know and that's exactly the reason I'm going to object those changes.
>
> The kernel/userspace abi is fixed and changing it like this could 
> potentially break drivers I'm the co-maintainer of. So that whole 
> approach is a clear NAK from my side.
>
> If you find a driver or userspace which doesn't use the formats as 
> defined in the comments of drm_fourcc.h the fix the driver instead of 
> trying to adjust the common header to broken behavior. Cause the later 
> will clearly cause problems with drivers who correctly implemented the 
> interface.
>
> Regards,
> Christian.
>
>>
>> cheers,
>>    Gerd
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.

[Harry Wentland]

Thanks, Christian for adding me.

On 2017-04-21 09:27 AM, Christian König wrote:
> Adding Harry to this mail thread as well, cause is one of the people
> really affected by this.
>
> Christian.
>
> Am 21.04.2017 um 15:21 schrieb Christian König:
>> Am 21.04.2017 um 15:12 schrieb Gerd Hoffmann:
>>>    Hi,
>>>
>>>>> "native" to me feels more like "native to the GPU" since these things
>>>>> really are tied to the GPU not the CPU. That's also why I went with
>>>>> the
>>>>> explicit endianness originally so that the driver could properly
>>>>> declare
>>>>> what the GPU supports.
>>>> And to be honest I would really prefer to stick with that approach for
>>>> exactly that reason.
>>>>

I strongly agree with Christian and Ville on this. I understand fourcc 
endianness as GPU endianness. Usermode needs to be clear on the 
framebuffer format, whether it's GPU or CPU rendered, so kernel should 
define this clearly.

In practice it probably doesn't currently make much difference for AMD 
GPUs. I've never heard of anyone using them on big-endian systems.

Harry


>>>> The proposed change would require that drivers have different code path
>>>> for different CPU byte order. Those code path tend to be not tested
>>>> very
>>>> well and are additional complexity we probably don't want inside the
>>>> driver.
>>> We can add fixed-endian #defines without too much effort, at least for
>>> the 8 bits per color formats.  In qemu we have the same problem, only
>>> with pixman.  Those formats are native endian too, but often we have to
>>> handle a fixed format, so we did this:
>>>
>>> /*
>>>   * pixman image formats are defined to be native endian,
>>>   * that means host byte order on qemu.  So we go define
>>>   * fixed formats here for cases where it is needed, like
>>>   * feeding libjpeg / libpng and writing screenshots.
>>>   */
>>>
>>> #ifdef HOST_WORDS_BIGENDIAN
>>> # define PIXMAN_BE_r8g8b8     PIXMAN_r8g8b8
>>> # define PIXMAN_BE_x8r8g8b8   PIXMAN_x8r8g8b8
>>> # define PIXMAN_BE_a8r8g8b8   PIXMAN_a8r8g8b8
>>> # define PIXMAN_BE_b8g8r8x8   PIXMAN_b8g8r8x8
>>> # define PIXMAN_BE_b8g8r8a8   PIXMAN_b8g8r8a8
>>> # define PIXMAN_BE_r8g8b8x8   PIXMAN_r8g8b8x8
>>> # define PIXMAN_BE_r8g8b8a8   PIXMAN_r8g8b8a8
>>> # define PIXMAN_BE_x8b8g8r8   PIXMAN_x8b8g8r8
>>> # define PIXMAN_BE_a8b8g8r8   PIXMAN_a8b8g8r8
>>> # define PIXMAN_LE_x8r8g8b8   PIXMAN_b8g8r8x8
>>> #else
>>> # define PIXMAN_BE_r8g8b8     PIXMAN_b8g8r8
>>> # define PIXMAN_BE_x8r8g8b8   PIXMAN_b8g8r8x8
>>> # define PIXMAN_BE_a8r8g8b8   PIXMAN_b8g8r8a8
>>> # define PIXMAN_BE_b8g8r8x8   PIXMAN_x8r8g8b8
>>> # define PIXMAN_BE_b8g8r8a8   PIXMAN_a8r8g8b8
>>> # define PIXMAN_BE_r8g8b8x8   PIXMAN_x8b8g8r8
>>> # define PIXMAN_BE_r8g8b8a8   PIXMAN_a8b8g8r8
>>> # define PIXMAN_BE_x8b8g8r8   PIXMAN_r8g8b8x8
>>> # define PIXMAN_BE_a8b8g8r8   PIXMAN_r8g8b8a8
>>> # define PIXMAN_LE_x8r8g8b8   PIXMAN_x8r8g8b8
>>> #endif
>>
>> Exactly what Mesa did as well.
>>
>>>> My personal opinion is that formats in drm_fourcc.h should be
>>>> independent of the CPU byte order and the function
>>>> drm_mode_legacy_fb_format() and drivers depending on that incorrect
>>>> assumption be fixed instead.
>>> The problem is this isn't a kernel-internal thing any more. With the
>>> addition of the ADDFB2 ioctl the fourcc codes became part of the
>>> kernel/userspace abi ...
>>
>> I know and that's exactly the reason I'm going to object those changes.
>>
>> The kernel/userspace abi is fixed and changing it like this could
>> potentially break drivers I'm the co-maintainer of. So that whole
>> approach is a clear NAK from my side.
>>
>> If you find a driver or userspace which doesn't use the formats as
>> defined in the comments of drm_fourcc.h the fix the driver instead of
>> trying to adjust the common header to broken behavior. Cause the later
>> will clearly cause problems with drivers who correctly implemented the
>> interface.
>>
>> Regards,
>> Christian.
>>
>>>
>>> cheers,
>>>    Gerd
>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
>

Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.

[Gerd Hoffmann]

  Hi,

> > My personal opinion is that formats in drm_fourcc.h should be 
> > independent of the CPU byte order and the function 
> > drm_mode_legacy_fb_format() and drivers depending on that incorrect 
> > assumption be fixed instead.
> 
> The problem is this isn't a kernel-internal thing any more.  With the
> addition of the ADDFB2 ioctl the fourcc codes became part of the
> kernel/userspace abi ...

Ok, added some printk's to the ADDFB and ADDFB2 code paths and tested a
bit.  Apparently pretty much all userspace still uses the ADDFB ioctl.
xorg (modesetting driver) does.  gnome-shell in wayland mode does.
Seems the big transition to ADDFB2 didn't happen yet.

I guess that makes changing drm_mode_legacy_fb_format + drivers a
reasonable option ...

cheers,
  Gerd

Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.

[Ville Syrjälä]

On Fri, Apr 21, 2017 at 06:14:31PM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > > My personal opinion is that formats in drm_fourcc.h should be 
> > > independent of the CPU byte order and the function 
> > > drm_mode_legacy_fb_format() and drivers depending on that incorrect 
> > > assumption be fixed instead.
> > 
> > The problem is this isn't a kernel-internal thing any more.  With the
> > addition of the ADDFB2 ioctl the fourcc codes became part of the
> > kernel/userspace abi ...
> 
> Ok, added some printk's to the ADDFB and ADDFB2 code paths and tested a
> bit.  Apparently pretty much all userspace still uses the ADDFB ioctl.
> xorg (modesetting driver) does.  gnome-shell in wayland mode does.
> Seems the big transition to ADDFB2 didn't happen yet.
> 
> I guess that makes changing drm_mode_legacy_fb_format + drivers a
> reasonable option ...

Yeah, I came to the same conclusion after chatting with some
folks on irc.

So my current idea is that we change any driver that wants to follow the
CPU endianness to declare support for big endian formats if the CPU is
big endian. Presumably these are mostly the virtual GPU drivers.

Additonally we'll make the mapping performed by drm_mode_legacy_fb_format()
driver controlled. That way drivers that got changed to follow CPU
endianness can return a framebuffer that matches CPU endianness. And
drivers that expect the GPU endianness to not depend on the CPU
endianness will keep working as they do now. The downside is that users
of the legacy addfb ioctl will need to magically know which endianness
they will get, but that is apparently already the case. And users of
addfb2 will keep on specifying the endianness explicitly with
DRM_FORMAT_BIG_ENDIAN vs. 0.

Does that sound like a workable solution?

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.

[Gerd Hoffmann]

  Hi,

> > I guess that makes changing drm_mode_legacy_fb_format + drivers a
> > reasonable option ...
> 
> Yeah, I came to the same conclusion after chatting with some
> folks on irc.
> 
> So my current idea is that we change any driver that wants to follow the
> CPU endianness to declare support for big endian formats if the CPU is
> big endian. Presumably these are mostly the virtual GPU drivers.

Agree.  Easy.

> Additonally we'll make the mapping performed by drm_mode_legacy_fb_format()
> driver controlled.

I don't think this is useful.  IMO drm_mode_legacy_fb_format should
return host endian formats unconditionally.

> That way drivers that got changed to follow CPU
> endianness can return a framebuffer that matches CPU endianness. And
> drivers that expect the GPU endianness to not depend on the CPU
> endianness will keep working as they do now. The downside is that users
> of the legacy addfb ioctl will need to magically know which endianness
> they will get, but that is apparently already the case.

Existing userspace expects host endian, and IMO we should maintain that
behavior.

> And users of
> addfb2 will keep on specifying the endianness explicitly with
> DRM_FORMAT_BIG_ENDIAN vs. 0.

I'd drop DRM_FORMAT_BIG_ENDIAN.

At least for the virt drivers it doesn't buy us anything.  They support
32bpp / 8 bpc formats only[1], and for those I can specify the
byteswapped format version without a bigendian flag because we have
fourccs for everything we need.


There is a WIP patch series at
https://www.kraxel.org/cgit/linux/log/?h=drm-byteorder

Needs more testing and better commit messages.  /me plans to polish &
post next week, but feel free to look and comment.

cheers,
  Gerd

[1] Everything else is a PITA to deal with on the host side because
    I can't offload that to pixman.  There is no support for
    PIXMAN_r5g6b5 or PIXMAN_x2b10g10r10 in non-host byte order.

Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.

[Michel Dänzer]

On 22/04/17 07:05 PM, Ville Syrjälä wrote:
> On Fri, Apr 21, 2017 at 06:14:31PM +0200, Gerd Hoffmann wrote:
>>   Hi,
>>
>>>> My personal opinion is that formats in drm_fourcc.h should be 
>>>> independent of the CPU byte order and the function 
>>>> drm_mode_legacy_fb_format() and drivers depending on that incorrect 
>>>> assumption be fixed instead.
>>>
>>> The problem is this isn't a kernel-internal thing any more.  With the
>>> addition of the ADDFB2 ioctl the fourcc codes became part of the
>>> kernel/userspace abi ...
>>
>> Ok, added some printk's to the ADDFB and ADDFB2 code paths and tested a
>> bit.  Apparently pretty much all userspace still uses the ADDFB ioctl.
>> xorg (modesetting driver) does.  gnome-shell in wayland mode does.
>> Seems the big transition to ADDFB2 didn't happen yet.
>>
>> I guess that makes changing drm_mode_legacy_fb_format + drivers a
>> reasonable option ...
> 
> Yeah, I came to the same conclusion after chatting with some
> folks on irc.
> 
> So my current idea is that we change any driver that wants to follow the
> CPU endianness

This isn't really optional for various reasons, some of which have been
covered in this discussion.


> to declare support for big endian formats if the CPU is
> big endian. Presumably these are mostly the virtual GPU drivers.
> 
> Additonally we'll make the mapping performed by drm_mode_legacy_fb_format()
> driver controlled. That way drivers that got changed to follow CPU
> endianness can return a framebuffer that matches CPU endianness. And
> drivers that expect the GPU endianness to not depend on the CPU
> endianness will keep working as they do now. The downside is that users
> of the legacy addfb ioctl will need to magically know which endianness
> they will get, but that is apparently already the case. And users of
> addfb2 will keep on specifying the endianness explicitly with
> DRM_FORMAT_BIG_ENDIAN vs. 0.

I'm afraid it's not that simple.

The display hardware of older (pre-R600 generation) Radeon GPUs does not
support the "big endian" formats directly. In order to allow userspace
to access pixel data in native endianness with the CPU, we instead use
byte-swapping functionality which only affects CPU access. This means
that the GPU and CPU effectively see different representations of the
same video memory contents.

Userspace code dealing with GPU access to pixel data needs to know the
format as seen by the GPU, whereas code dealing with CPU access needs to
know the format as seen by the CPU. I don't see any way to express this
with a single format definition.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer

Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.

[Ville Syrjälä]

On Mon, Apr 24, 2017 at 03:57:02PM +0900, Michel Dänzer wrote:
> On 22/04/17 07:05 PM, Ville Syrjälä wrote:
> > On Fri, Apr 21, 2017 at 06:14:31PM +0200, Gerd Hoffmann wrote:
> >>   Hi,
> >>
> >>>> My personal opinion is that formats in drm_fourcc.h should be 
> >>>> independent of the CPU byte order and the function 
> >>>> drm_mode_legacy_fb_format() and drivers depending on that incorrect 
> >>>> assumption be fixed instead.
> >>>
> >>> The problem is this isn't a kernel-internal thing any more.  With the
> >>> addition of the ADDFB2 ioctl the fourcc codes became part of the
> >>> kernel/userspace abi ...
> >>
> >> Ok, added some printk's to the ADDFB and ADDFB2 code paths and tested a
> >> bit.  Apparently pretty much all userspace still uses the ADDFB ioctl.
> >> xorg (modesetting driver) does.  gnome-shell in wayland mode does.
> >> Seems the big transition to ADDFB2 didn't happen yet.
> >>
> >> I guess that makes changing drm_mode_legacy_fb_format + drivers a
> >> reasonable option ...
> > 
> > Yeah, I came to the same conclusion after chatting with some
> > folks on irc.
> > 
> > So my current idea is that we change any driver that wants to follow the
> > CPU endianness
> 
> This isn't really optional for various reasons, some of which have been
> covered in this discussion.
> 
> 
> > to declare support for big endian formats if the CPU is
> > big endian. Presumably these are mostly the virtual GPU drivers.
> > 
> > Additonally we'll make the mapping performed by drm_mode_legacy_fb_format()
> > driver controlled. That way drivers that got changed to follow CPU
> > endianness can return a framebuffer that matches CPU endianness. And
> > drivers that expect the GPU endianness to not depend on the CPU
> > endianness will keep working as they do now. The downside is that users
> > of the legacy addfb ioctl will need to magically know which endianness
> > they will get, but that is apparently already the case. And users of
> > addfb2 will keep on specifying the endianness explicitly with
> > DRM_FORMAT_BIG_ENDIAN vs. 0.
> 
> I'm afraid it's not that simple.
> 
> The display hardware of older (pre-R600 generation) Radeon GPUs does not
> support the "big endian" formats directly. In order to allow userspace
> to access pixel data in native endianness with the CPU, we instead use
> byte-swapping functionality which only affects CPU access.

OK, I'm getting confused. Based on our irc discussion I got the
impression you don't byte swap CPU accesses. But since you do, how
do you deal with mixing 8bpp vs. 16bpp vs. 32bpp?

> This means
> that the GPU and CPU effectively see different representations of the
> same video memory contents.
> 
> Userspace code dealing with GPU access to pixel data needs to know the
> format as seen by the GPU, whereas code dealing with CPU access needs to
> know the format as seen by the CPU. I don't see any way to express this
> with a single format definition.

Hmm. Well that certainly makes life even more interesting.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.

[Michel Dänzer]

On 24/04/17 10:03 PM, Ville Syrjälä wrote:
> On Mon, Apr 24, 2017 at 03:57:02PM +0900, Michel Dänzer wrote:
>> On 22/04/17 07:05 PM, Ville Syrjälä wrote:
>>> On Fri, Apr 21, 2017 at 06:14:31PM +0200, Gerd Hoffmann wrote:
>>>>   Hi,
>>>>
>>>>>> My personal opinion is that formats in drm_fourcc.h should be 
>>>>>> independent of the CPU byte order and the function 
>>>>>> drm_mode_legacy_fb_format() and drivers depending on that incorrect 
>>>>>> assumption be fixed instead.
>>>>>
>>>>> The problem is this isn't a kernel-internal thing any more.  With the
>>>>> addition of the ADDFB2 ioctl the fourcc codes became part of the
>>>>> kernel/userspace abi ...
>>>>
>>>> Ok, added some printk's to the ADDFB and ADDFB2 code paths and tested a
>>>> bit.  Apparently pretty much all userspace still uses the ADDFB ioctl.
>>>> xorg (modesetting driver) does.  gnome-shell in wayland mode does.
>>>> Seems the big transition to ADDFB2 didn't happen yet.
>>>>
>>>> I guess that makes changing drm_mode_legacy_fb_format + drivers a
>>>> reasonable option ...
>>>
>>> Yeah, I came to the same conclusion after chatting with some
>>> folks on irc.
>>>
>>> So my current idea is that we change any driver that wants to follow the
>>> CPU endianness
>>
>> This isn't really optional for various reasons, some of which have been
>> covered in this discussion.
>>
>>
>>> to declare support for big endian formats if the CPU is
>>> big endian. Presumably these are mostly the virtual GPU drivers.
>>>
>>> Additonally we'll make the mapping performed by drm_mode_legacy_fb_format()
>>> driver controlled. That way drivers that got changed to follow CPU
>>> endianness can return a framebuffer that matches CPU endianness. And
>>> drivers that expect the GPU endianness to not depend on the CPU
>>> endianness will keep working as they do now. The downside is that users
>>> of the legacy addfb ioctl will need to magically know which endianness
>>> they will get, but that is apparently already the case. And users of
>>> addfb2 will keep on specifying the endianness explicitly with
>>> DRM_FORMAT_BIG_ENDIAN vs. 0.
>>
>> I'm afraid it's not that simple.
>>
>> The display hardware of older (pre-R600 generation) Radeon GPUs does not
>> support the "big endian" formats directly. In order to allow userspace
>> to access pixel data in native endianness with the CPU, we instead use
>> byte-swapping functionality which only affects CPU access.
> 
> OK, I'm getting confused. Based on our irc discussion I got the
> impression you don't byte swap CPU accesses.

Sorry for the confusion. The radeon kernel driver does support
byte-swapping for CPU access to VRAM with pre-R600 GPUs, and this is
used for fbdev emulation. What I meant on IRC is that the xf86-video-ati
radeon driver doesn't make use of this, mostly because it only applies
while a BO is in VRAM, and userspace can't control when that's the case
(while a BO isn't being scanned out).


> But since you do, how do you deal with mixing 8bpp vs. 16bpp vs. 32bpp?

The byte-swapping is configured per-BO via the
RADEON_TILING_SWAP_16/32BIT flags.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer

Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.

[Michel Dänzer]

On 25/04/17 10:12 AM, Michel Dänzer wrote:
> On 24/04/17 10:03 PM, Ville Syrjälä wrote:
>> On Mon, Apr 24, 2017 at 03:57:02PM +0900, Michel Dänzer wrote:
>>> On 22/04/17 07:05 PM, Ville Syrjälä wrote:
>>>> On Fri, Apr 21, 2017 at 06:14:31PM +0200, Gerd Hoffmann wrote:
>>>>>   Hi,
>>>>>
>>>>>>> My personal opinion is that formats in drm_fourcc.h should be 
>>>>>>> independent of the CPU byte order and the function 
>>>>>>> drm_mode_legacy_fb_format() and drivers depending on that incorrect 
>>>>>>> assumption be fixed instead.
>>>>>>
>>>>>> The problem is this isn't a kernel-internal thing any more.  With the
>>>>>> addition of the ADDFB2 ioctl the fourcc codes became part of the
>>>>>> kernel/userspace abi ...
>>>>>
>>>>> Ok, added some printk's to the ADDFB and ADDFB2 code paths and tested a
>>>>> bit.  Apparently pretty much all userspace still uses the ADDFB ioctl.
>>>>> xorg (modesetting driver) does.  gnome-shell in wayland mode does.
>>>>> Seems the big transition to ADDFB2 didn't happen yet.
>>>>>
>>>>> I guess that makes changing drm_mode_legacy_fb_format + drivers a
>>>>> reasonable option ...
>>>>
>>>> Yeah, I came to the same conclusion after chatting with some
>>>> folks on irc.
>>>>
>>>> So my current idea is that we change any driver that wants to follow the
>>>> CPU endianness
>>>
>>> This isn't really optional for various reasons, some of which have been
>>> covered in this discussion.
>>>
>>>
>>>> to declare support for big endian formats if the CPU is
>>>> big endian. Presumably these are mostly the virtual GPU drivers.
>>>>
>>>> Additonally we'll make the mapping performed by drm_mode_legacy_fb_format()
>>>> driver controlled. That way drivers that got changed to follow CPU
>>>> endianness can return a framebuffer that matches CPU endianness. And
>>>> drivers that expect the GPU endianness to not depend on the CPU
>>>> endianness will keep working as they do now. The downside is that users
>>>> of the legacy addfb ioctl will need to magically know which endianness
>>>> they will get, but that is apparently already the case. And users of
>>>> addfb2 will keep on specifying the endianness explicitly with
>>>> DRM_FORMAT_BIG_ENDIAN vs. 0.
>>>
>>> I'm afraid it's not that simple.
>>>
>>> The display hardware of older (pre-R600 generation) Radeon GPUs does not
>>> support the "big endian" formats directly. In order to allow userspace
>>> to access pixel data in native endianness with the CPU, we instead use
>>> byte-swapping functionality which only affects CPU access.
>>
>> OK, I'm getting confused. Based on our irc discussion I got the
>> impression you don't byte swap CPU accesses.
> 
> Sorry for the confusion. The radeon kernel driver does support
> byte-swapping for CPU access to VRAM with pre-R600 GPUs, and this is
> used for fbdev emulation. What I meant on IRC is that the xf86-video-ati
> radeon driver doesn't make use of this, mostly because it only applies
> while a BO is in VRAM, and userspace can't control when that's the case
> (while a BO isn't being scanned out).
> 
> 
>> But since you do, how do you deal with mixing 8bpp vs. 16bpp vs. 32bpp?
> 
> The byte-swapping is configured per-BO via the
> RADEON_TILING_SWAP_16/32BIT flags.

... which means that it's disabled by default, so it shouldn't affect
generic userspace. So exposing the GPU format directly should be
feasible in this case as well after all. Sorry for the noise. :(


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer

Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.

[Ville Syrjälä]

On Tue, Apr 25, 2017 at 10:12:37AM +0900, Michel Dänzer wrote:
> On 24/04/17 10:03 PM, Ville Syrjälä wrote:
> > On Mon, Apr 24, 2017 at 03:57:02PM +0900, Michel Dänzer wrote:
> >> On 22/04/17 07:05 PM, Ville Syrjälä wrote:
> >>> On Fri, Apr 21, 2017 at 06:14:31PM +0200, Gerd Hoffmann wrote:
> >>>>   Hi,
> >>>>
> >>>>>> My personal opinion is that formats in drm_fourcc.h should be 
> >>>>>> independent of the CPU byte order and the function 
> >>>>>> drm_mode_legacy_fb_format() and drivers depending on that incorrect 
> >>>>>> assumption be fixed instead.
> >>>>>
> >>>>> The problem is this isn't a kernel-internal thing any more.  With the
> >>>>> addition of the ADDFB2 ioctl the fourcc codes became part of the
> >>>>> kernel/userspace abi ...
> >>>>
> >>>> Ok, added some printk's to the ADDFB and ADDFB2 code paths and tested a
> >>>> bit.  Apparently pretty much all userspace still uses the ADDFB ioctl.
> >>>> xorg (modesetting driver) does.  gnome-shell in wayland mode does.
> >>>> Seems the big transition to ADDFB2 didn't happen yet.
> >>>>
> >>>> I guess that makes changing drm_mode_legacy_fb_format + drivers a
> >>>> reasonable option ...
> >>>
> >>> Yeah, I came to the same conclusion after chatting with some
> >>> folks on irc.
> >>>
> >>> So my current idea is that we change any driver that wants to follow the
> >>> CPU endianness
> >>
> >> This isn't really optional for various reasons, some of which have been
> >> covered in this discussion.
> >>
> >>
> >>> to declare support for big endian formats if the CPU is
> >>> big endian. Presumably these are mostly the virtual GPU drivers.
> >>>
> >>> Additonally we'll make the mapping performed by drm_mode_legacy_fb_format()
> >>> driver controlled. That way drivers that got changed to follow CPU
> >>> endianness can return a framebuffer that matches CPU endianness. And
> >>> drivers that expect the GPU endianness to not depend on the CPU
> >>> endianness will keep working as they do now. The downside is that users
> >>> of the legacy addfb ioctl will need to magically know which endianness
> >>> they will get, but that is apparently already the case. And users of
> >>> addfb2 will keep on specifying the endianness explicitly with
> >>> DRM_FORMAT_BIG_ENDIAN vs. 0.
> >>
> >> I'm afraid it's not that simple.
> >>
> >> The display hardware of older (pre-R600 generation) Radeon GPUs does not
> >> support the "big endian" formats directly. In order to allow userspace
> >> to access pixel data in native endianness with the CPU, we instead use
> >> byte-swapping functionality which only affects CPU access.
> > 
> > OK, I'm getting confused. Based on our irc discussion I got the
> > impression you don't byte swap CPU accesses.
> 
> Sorry for the confusion. The radeon kernel driver does support
> byte-swapping for CPU access to VRAM with pre-R600 GPUs, and this is
> used for fbdev emulation. What I meant on IRC is that the xf86-video-ati
> radeon driver doesn't make use of this, mostly because it only applies
> while a BO is in VRAM, and userspace can't control when that's the case
> (while a BO isn't being scanned out).

So that was my other question. So if someone just creates a bo, I presume
ttm can more or less move it between system memory and vram at any
time. So if we then mmap the bo, does it mean the CPU will see the bytes
in different order depending on where the bo happens to live at
the time the CPU access happens?

And how would that work wih dumb bos? Would they be forced to live in vram?
I see it's passing VRAM as the initial domain, but I can't quickly see
whether that would mean it can't even be moved out.

> 
> 
> > But since you do, how do you deal with mixing 8bpp vs. 16bpp vs. 32bpp?
> 
> The byte-swapping is configured per-BO via the
> RADEON_TILING_SWAP_16/32BIT flags.

Which translates into usage of the surface regs it seems. So I wasn't
totally crazy to think that such things existed :)

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.

[Michel Dänzer]

On 25/04/17 07:26 PM, Ville Syrjälä wrote:
> On Tue, Apr 25, 2017 at 10:12:37AM +0900, Michel Dänzer wrote:
>> On 24/04/17 10:03 PM, Ville Syrjälä wrote:
>>> On Mon, Apr 24, 2017 at 03:57:02PM +0900, Michel Dänzer wrote:
>>>> On 22/04/17 07:05 PM, Ville Syrjälä wrote:
>>>>> On Fri, Apr 21, 2017 at 06:14:31PM +0200, Gerd Hoffmann wrote:
>>>>>>   Hi,
>>>>>>
>>>>>>>> My personal opinion is that formats in drm_fourcc.h should be 
>>>>>>>> independent of the CPU byte order and the function 
>>>>>>>> drm_mode_legacy_fb_format() and drivers depending on that incorrect 
>>>>>>>> assumption be fixed instead.
>>>>>>>
>>>>>>> The problem is this isn't a kernel-internal thing any more.  With the
>>>>>>> addition of the ADDFB2 ioctl the fourcc codes became part of the
>>>>>>> kernel/userspace abi ...
>>>>>>
>>>>>> Ok, added some printk's to the ADDFB and ADDFB2 code paths and tested a
>>>>>> bit.  Apparently pretty much all userspace still uses the ADDFB ioctl.
>>>>>> xorg (modesetting driver) does.  gnome-shell in wayland mode does.
>>>>>> Seems the big transition to ADDFB2 didn't happen yet.
>>>>>>
>>>>>> I guess that makes changing drm_mode_legacy_fb_format + drivers a
>>>>>> reasonable option ...
>>>>>
>>>>> Yeah, I came to the same conclusion after chatting with some
>>>>> folks on irc.
>>>>>
>>>>> So my current idea is that we change any driver that wants to follow the
>>>>> CPU endianness
>>>>
>>>> This isn't really optional for various reasons, some of which have been
>>>> covered in this discussion.
>>>>
>>>>
>>>>> to declare support for big endian formats if the CPU is
>>>>> big endian. Presumably these are mostly the virtual GPU drivers.
>>>>>
>>>>> Additonally we'll make the mapping performed by drm_mode_legacy_fb_format()
>>>>> driver controlled. That way drivers that got changed to follow CPU
>>>>> endianness can return a framebuffer that matches CPU endianness. And
>>>>> drivers that expect the GPU endianness to not depend on the CPU
>>>>> endianness will keep working as they do now. The downside is that users
>>>>> of the legacy addfb ioctl will need to magically know which endianness
>>>>> they will get, but that is apparently already the case. And users of
>>>>> addfb2 will keep on specifying the endianness explicitly with
>>>>> DRM_FORMAT_BIG_ENDIAN vs. 0.
>>>>
>>>> I'm afraid it's not that simple.
>>>>
>>>> The display hardware of older (pre-R600 generation) Radeon GPUs does not
>>>> support the "big endian" formats directly. In order to allow userspace
>>>> to access pixel data in native endianness with the CPU, we instead use
>>>> byte-swapping functionality which only affects CPU access.
>>>
>>> OK, I'm getting confused. Based on our irc discussion I got the
>>> impression you don't byte swap CPU accesses.
>>
>> Sorry for the confusion. The radeon kernel driver does support
>> byte-swapping for CPU access to VRAM with pre-R600 GPUs, and this is
>> used for fbdev emulation. What I meant on IRC is that the xf86-video-ati
>> radeon driver doesn't make use of this, mostly because it only applies
>> while a BO is in VRAM, and userspace can't control when that's the case
>> (while a BO isn't being scanned out).
> 
> So that was my other question. So if someone just creates a bo, I presume
> ttm can more or less move it between system memory and vram at any
> time. So if we then mmap the bo, does it mean the CPU will see the bytes
> in different order depending on where the bo happens to live at
> the time the CPU access happens?

If either of the RADEON_TILING_SWAP_16/32BIT flags was set when the BO
was created, yes. That's why the xf86-video-ati radeon driver doesn't
use this functionality.

> And how would that work wih dumb bos?

radeon_mode_dumb_create doesn't set the RADEON_TILING_SWAP_16/32BIT
flags, so no byte swapping is performed for dumb BOs even in VRAM.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer

Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2136 bytes --]

On Fri 2017-04-21 14:08:04, Ville Syrjälä wrote:
> On Fri, Apr 21, 2017 at 11:50:18AM +0200, Gerd Hoffmann wrote:
> > On Fr, 2017-04-21 at 12:25 +0300, Ville Syrjälä wrote:
> > > On Fri, Apr 21, 2017 at 09:58:24AM +0200, Gerd Hoffmann wrote:
> > > > While working on graphics support for virtual machines on ppc64 (which
> > > > exists in both little and big endian variants) I've figured the comments
> > > > for various drm fourcc formats in the header file don't match reality.
> > > > 
> > > > Comments says the RGB formats are little endian, but in practice they
> > > > are native endian.  Look at the drm_mode_legacy_fb_format() helper.  It
> > > > maps -- for example -- bpp/depth 32/24 to DRM_FORMAT_XRGB8888, no matter
> > > > whenever the machine is little endian or big endian.  The users of this
> > > > function (fbdev emulation, DRM_IOCTL_MODE_ADDFB) expect the framebuffer
> > > > is native endian, not little endian.  Most userspace also operates on
> > > > native endian only.
> > > 
> > > I'm not a fan of "native". Native to what? "CPU" or "host" is what I'd
> > > call it.
> > 
> > native == whatever the cpu is using.
> > 
> > I personally find "native" more intuitive, but at the end of the day I
> > don't mind much.  If people prefer "host" over "native" I'll change it.
> 
> "native" to me feels more like "native to the GPU" since these things
> really are tied to the GPU not the CPU. That's also why I went with the
> explicit endianness originally so that the driver could properly declare
> what the GPU supports.

You can easily have more than one GPU in the system. Plus these are
used by cameras / frame grabbers, too. So anything else than CPU
endianness is badly defined.

(And I agree with the rest of the thread -- we should really be
explicit; fourcc should specify what format the image data are in, and
it should be possible to write fourcc + raw data into file and
transfer it between machines.)


									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.

[Ville Syrjälä]

On Thu, May 11, 2017 at 11:23:11PM +0200, Pavel Machek wrote:
> On Fri 2017-04-21 14:08:04, Ville Syrjälä wrote:
> > On Fri, Apr 21, 2017 at 11:50:18AM +0200, Gerd Hoffmann wrote:
> > > On Fr, 2017-04-21 at 12:25 +0300, Ville Syrjälä wrote:
> > > > On Fri, Apr 21, 2017 at 09:58:24AM +0200, Gerd Hoffmann wrote:
> > > > > While working on graphics support for virtual machines on ppc64 (which
> > > > > exists in both little and big endian variants) I've figured the comments
> > > > > for various drm fourcc formats in the header file don't match reality.
> > > > > 
> > > > > Comments says the RGB formats are little endian, but in practice they
> > > > > are native endian.  Look at the drm_mode_legacy_fb_format() helper.  It
> > > > > maps -- for example -- bpp/depth 32/24 to DRM_FORMAT_XRGB8888, no matter
> > > > > whenever the machine is little endian or big endian.  The users of this
> > > > > function (fbdev emulation, DRM_IOCTL_MODE_ADDFB) expect the framebuffer
> > > > > is native endian, not little endian.  Most userspace also operates on
> > > > > native endian only.
> > > > 
> > > > I'm not a fan of "native". Native to what? "CPU" or "host" is what I'd
> > > > call it.
> > > 
> > > native == whatever the cpu is using.
> > > 
> > > I personally find "native" more intuitive, but at the end of the day I
> > > don't mind much.  If people prefer "host" over "native" I'll change it.
> > 
> > "native" to me feels more like "native to the GPU" since these things
> > really are tied to the GPU not the CPU. That's also why I went with the
> > explicit endianness originally so that the driver could properly declare
> > what the GPU supports.
> 
> You can easily have more than one GPU in the system. Plus these are
> used by cameras / frame grabbers, too. So anything else than CPU
> endianness is badly defined.

The framebuffer has very little to do with the CPU. The display
controller is the only consumer, and the producer could be
whatever.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.

[Ilia Mirkin]

On Fri, Apr 21, 2017 at 3:58 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> While working on graphics support for virtual machines on ppc64 (which
> exists in both little and big endian variants) I've figured the comments
> for various drm fourcc formats in the header file don't match reality.
>
> Comments says the RGB formats are little endian, but in practice they
> are native endian.  Look at the drm_mode_legacy_fb_format() helper.  It
> maps -- for example -- bpp/depth 32/24 to DRM_FORMAT_XRGB8888, no matter
> whenever the machine is little endian or big endian.  The users of this
> function (fbdev emulation, DRM_IOCTL_MODE_ADDFB) expect the framebuffer
> is native endian, not little endian.  Most userspace also operates on
> native endian only.
>
> So, go update the comments for all 16+24+32 bpp RGB formats.
>
> Leaving the yuv formats as-is.  I have no idea if and how those are used
> on bigendian machines.

I think this is premature. The current situation is that I can't get
modetest to work *at all* on my NV34 / BE setup (I mean, it runs, just
the colors displayed are wrong). I believe that currently it packs
things in "cpu native endian". I've tried futzing with that without
much success, although I didn't spend too much time on it. I have a
NV34 plugged into my LE setup as well although I haven't tested to
double-check that it all works there. However I'm quite sure it used
to, as I used modetest to help develop the YUV overlay support for
those GPUs.

Does modetest work for you, under any interpretation of the formats?

There's an additional complication wrt looking at what fbcon does,
since it will feed the data in via special accel interfaces in fbdev,
which at least on nouveau, may end up byteswapping the data on upload
to VRAM (I'm not 100% clear on whether they do or not). However
modetest, which is creating its own fb, likely won't get such
treatment.

This is a shitty situation - we have hardware we don't know how it
works, tools we don't know whether they're broken, and comments we're
pretty sure are at least somewhat wrong. Furthermore the hardware is
relatively rare and developers with time to work on improving it are
even rarer.

I'd like to reiterate that the status quo does end up in a functioning
system. Let's try not to break that.

Cheers,

  -ilia

>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Pekka Paalanen <ppaalanen@gmail.com>
> Cc: Ilia Mirkin <imirkin@alum.mit.edu>
> Cc: Michel Dänzer <michel@daenzer.net>
> Cc: Alex Deucher <alexdeucher@gmail.com>
> Cc: amd-gfx@lists.freedesktop.org
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/uapi/drm/drm_fourcc.h | 82 +++++++++++++++++++++----------------------
>  1 file changed, 41 insertions(+), 41 deletions(-)
>
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index 995c8f9..1579765 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -42,68 +42,68 @@ extern "C" {
>  #define DRM_FORMAT_R8          fourcc_code('R', '8', ' ', ' ') /* [7:0] R */
>
>  /* 16 bpp Red */
> -#define DRM_FORMAT_R16         fourcc_code('R', '1', '6', ' ') /* [15:0] R little endian */
> +#define DRM_FORMAT_R16         fourcc_code('R', '1', '6', ' ') /* [15:0] R native endian */
>
>  /* 16 bpp RG */
> -#define DRM_FORMAT_RG88                fourcc_code('R', 'G', '8', '8') /* [15:0] R:G 8:8 little endian */
> -#define DRM_FORMAT_GR88                fourcc_code('G', 'R', '8', '8') /* [15:0] G:R 8:8 little endian */
> +#define DRM_FORMAT_RG88                fourcc_code('R', 'G', '8', '8') /* [15:0] R:G 8:8 native endian */
> +#define DRM_FORMAT_GR88                fourcc_code('G', 'R', '8', '8') /* [15:0] G:R 8:8 native endian */
>
>  /* 32 bpp RG */
> -#define DRM_FORMAT_RG1616      fourcc_code('R', 'G', '3', '2') /* [31:0] R:G 16:16 little endian */
> -#define DRM_FORMAT_GR1616      fourcc_code('G', 'R', '3', '2') /* [31:0] G:R 16:16 little endian */
> +#define DRM_FORMAT_RG1616      fourcc_code('R', 'G', '3', '2') /* [31:0] R:G 16:16 native endian */
> +#define DRM_FORMAT_GR1616      fourcc_code('G', 'R', '3', '2') /* [31:0] G:R 16:16 native endian */
>
>  /* 8 bpp RGB */
>  #define DRM_FORMAT_RGB332      fourcc_code('R', 'G', 'B', '8') /* [7:0] R:G:B 3:3:2 */
>  #define DRM_FORMAT_BGR233      fourcc_code('B', 'G', 'R', '8') /* [7:0] B:G:R 2:3:3 */
>
>  /* 16 bpp RGB */
> -#define DRM_FORMAT_XRGB4444    fourcc_code('X', 'R', '1', '2') /* [15:0] x:R:G:B 4:4:4:4 little endian */
> -#define DRM_FORMAT_XBGR4444    fourcc_code('X', 'B', '1', '2') /* [15:0] x:B:G:R 4:4:4:4 little endian */
> -#define DRM_FORMAT_RGBX4444    fourcc_code('R', 'X', '1', '2') /* [15:0] R:G:B:x 4:4:4:4 little endian */
> -#define DRM_FORMAT_BGRX4444    fourcc_code('B', 'X', '1', '2') /* [15:0] B:G:R:x 4:4:4:4 little endian */
> +#define DRM_FORMAT_XRGB4444    fourcc_code('X', 'R', '1', '2') /* [15:0] x:R:G:B 4:4:4:4 native endian */
> +#define DRM_FORMAT_XBGR4444    fourcc_code('X', 'B', '1', '2') /* [15:0] x:B:G:R 4:4:4:4 native endian */
> +#define DRM_FORMAT_RGBX4444    fourcc_code('R', 'X', '1', '2') /* [15:0] R:G:B:x 4:4:4:4 native endian */
> +#define DRM_FORMAT_BGRX4444    fourcc_code('B', 'X', '1', '2') /* [15:0] B:G:R:x 4:4:4:4 native endian */
>
> -#define DRM_FORMAT_ARGB4444    fourcc_code('A', 'R', '1', '2') /* [15:0] A:R:G:B 4:4:4:4 little endian */
> -#define DRM_FORMAT_ABGR4444    fourcc_code('A', 'B', '1', '2') /* [15:0] A:B:G:R 4:4:4:4 little endian */
> -#define DRM_FORMAT_RGBA4444    fourcc_code('R', 'A', '1', '2') /* [15:0] R:G:B:A 4:4:4:4 little endian */
> -#define DRM_FORMAT_BGRA4444    fourcc_code('B', 'A', '1', '2') /* [15:0] B:G:R:A 4:4:4:4 little endian */
> +#define DRM_FORMAT_ARGB4444    fourcc_code('A', 'R', '1', '2') /* [15:0] A:R:G:B 4:4:4:4 native endian */
> +#define DRM_FORMAT_ABGR4444    fourcc_code('A', 'B', '1', '2') /* [15:0] A:B:G:R 4:4:4:4 native endian */
> +#define DRM_FORMAT_RGBA4444    fourcc_code('R', 'A', '1', '2') /* [15:0] R:G:B:A 4:4:4:4 native endian */
> +#define DRM_FORMAT_BGRA4444    fourcc_code('B', 'A', '1', '2') /* [15:0] B:G:R:A 4:4:4:4 native endian */
>
> -#define DRM_FORMAT_XRGB1555    fourcc_code('X', 'R', '1', '5') /* [15:0] x:R:G:B 1:5:5:5 little endian */
> -#define DRM_FORMAT_XBGR1555    fourcc_code('X', 'B', '1', '5') /* [15:0] x:B:G:R 1:5:5:5 little endian */
> -#define DRM_FORMAT_RGBX5551    fourcc_code('R', 'X', '1', '5') /* [15:0] R:G:B:x 5:5:5:1 little endian */
> -#define DRM_FORMAT_BGRX5551    fourcc_code('B', 'X', '1', '5') /* [15:0] B:G:R:x 5:5:5:1 little endian */
> +#define DRM_FORMAT_XRGB1555    fourcc_code('X', 'R', '1', '5') /* [15:0] x:R:G:B 1:5:5:5 native endian */
> +#define DRM_FORMAT_XBGR1555    fourcc_code('X', 'B', '1', '5') /* [15:0] x:B:G:R 1:5:5:5 native endian */
> +#define DRM_FORMAT_RGBX5551    fourcc_code('R', 'X', '1', '5') /* [15:0] R:G:B:x 5:5:5:1 native endian */
> +#define DRM_FORMAT_BGRX5551    fourcc_code('B', 'X', '1', '5') /* [15:0] B:G:R:x 5:5:5:1 native endian */
>
> -#define DRM_FORMAT_ARGB1555    fourcc_code('A', 'R', '1', '5') /* [15:0] A:R:G:B 1:5:5:5 little endian */
> -#define DRM_FORMAT_ABGR1555    fourcc_code('A', 'B', '1', '5') /* [15:0] A:B:G:R 1:5:5:5 little endian */
> -#define DRM_FORMAT_RGBA5551    fourcc_code('R', 'A', '1', '5') /* [15:0] R:G:B:A 5:5:5:1 little endian */
> -#define DRM_FORMAT_BGRA5551    fourcc_code('B', 'A', '1', '5') /* [15:0] B:G:R:A 5:5:5:1 little endian */
> +#define DRM_FORMAT_ARGB1555    fourcc_code('A', 'R', '1', '5') /* [15:0] A:R:G:B 1:5:5:5 native endian */
> +#define DRM_FORMAT_ABGR1555    fourcc_code('A', 'B', '1', '5') /* [15:0] A:B:G:R 1:5:5:5 native endian */
> +#define DRM_FORMAT_RGBA5551    fourcc_code('R', 'A', '1', '5') /* [15:0] R:G:B:A 5:5:5:1 native endian */
> +#define DRM_FORMAT_BGRA5551    fourcc_code('B', 'A', '1', '5') /* [15:0] B:G:R:A 5:5:5:1 native endian */
>
> -#define DRM_FORMAT_RGB565      fourcc_code('R', 'G', '1', '6') /* [15:0] R:G:B 5:6:5 little endian */
> -#define DRM_FORMAT_BGR565      fourcc_code('B', 'G', '1', '6') /* [15:0] B:G:R 5:6:5 little endian */
> +#define DRM_FORMAT_RGB565      fourcc_code('R', 'G', '1', '6') /* [15:0] R:G:B 5:6:5 native endian */
> +#define DRM_FORMAT_BGR565      fourcc_code('B', 'G', '1', '6') /* [15:0] B:G:R 5:6:5 native endian */
>
>  /* 24 bpp RGB */
> -#define DRM_FORMAT_RGB888      fourcc_code('R', 'G', '2', '4') /* [23:0] R:G:B little endian */
> -#define DRM_FORMAT_BGR888      fourcc_code('B', 'G', '2', '4') /* [23:0] B:G:R little endian */
> +#define DRM_FORMAT_RGB888      fourcc_code('R', 'G', '2', '4') /* [23:0] R:G:B native endian */
> +#define DRM_FORMAT_BGR888      fourcc_code('B', 'G', '2', '4') /* [23:0] B:G:R native endian */
>
>  /* 32 bpp RGB */
> -#define DRM_FORMAT_XRGB8888    fourcc_code('X', 'R', '2', '4') /* [31:0] x:R:G:B 8:8:8:8 little endian */
> -#define DRM_FORMAT_XBGR8888    fourcc_code('X', 'B', '2', '4') /* [31:0] x:B:G:R 8:8:8:8 little endian */
> -#define DRM_FORMAT_RGBX8888    fourcc_code('R', 'X', '2', '4') /* [31:0] R:G:B:x 8:8:8:8 little endian */
> -#define DRM_FORMAT_BGRX8888    fourcc_code('B', 'X', '2', '4') /* [31:0] B:G:R:x 8:8:8:8 little endian */
> +#define DRM_FORMAT_XRGB8888    fourcc_code('X', 'R', '2', '4') /* [31:0] x:R:G:B 8:8:8:8 native endian */
> +#define DRM_FORMAT_XBGR8888    fourcc_code('X', 'B', '2', '4') /* [31:0] x:B:G:R 8:8:8:8 native endian */
> +#define DRM_FORMAT_RGBX8888    fourcc_code('R', 'X', '2', '4') /* [31:0] R:G:B:x 8:8:8:8 native endian */
> +#define DRM_FORMAT_BGRX8888    fourcc_code('B', 'X', '2', '4') /* [31:0] B:G:R:x 8:8:8:8 native endian */
>
> -#define DRM_FORMAT_ARGB8888    fourcc_code('A', 'R', '2', '4') /* [31:0] A:R:G:B 8:8:8:8 little endian */
> -#define DRM_FORMAT_ABGR8888    fourcc_code('A', 'B', '2', '4') /* [31:0] A:B:G:R 8:8:8:8 little endian */
> -#define DRM_FORMAT_RGBA8888    fourcc_code('R', 'A', '2', '4') /* [31:0] R:G:B:A 8:8:8:8 little endian */
> -#define DRM_FORMAT_BGRA8888    fourcc_code('B', 'A', '2', '4') /* [31:0] B:G:R:A 8:8:8:8 little endian */
> +#define DRM_FORMAT_ARGB8888    fourcc_code('A', 'R', '2', '4') /* [31:0] A:R:G:B 8:8:8:8 native endian */
> +#define DRM_FORMAT_ABGR8888    fourcc_code('A', 'B', '2', '4') /* [31:0] A:B:G:R 8:8:8:8 native endian */
> +#define DRM_FORMAT_RGBA8888    fourcc_code('R', 'A', '2', '4') /* [31:0] R:G:B:A 8:8:8:8 native endian */
> +#define DRM_FORMAT_BGRA8888    fourcc_code('B', 'A', '2', '4') /* [31:0] B:G:R:A 8:8:8:8 native endian */
>
> -#define DRM_FORMAT_XRGB2101010 fourcc_code('X', 'R', '3', '0') /* [31:0] x:R:G:B 2:10:10:10 little endian */
> -#define DRM_FORMAT_XBGR2101010 fourcc_code('X', 'B', '3', '0') /* [31:0] x:B:G:R 2:10:10:10 little endian */
> -#define DRM_FORMAT_RGBX1010102 fourcc_code('R', 'X', '3', '0') /* [31:0] R:G:B:x 10:10:10:2 little endian */
> -#define DRM_FORMAT_BGRX1010102 fourcc_code('B', 'X', '3', '0') /* [31:0] B:G:R:x 10:10:10:2 little endian */
> +#define DRM_FORMAT_XRGB2101010 fourcc_code('X', 'R', '3', '0') /* [31:0] x:R:G:B 2:10:10:10 native endian */
> +#define DRM_FORMAT_XBGR2101010 fourcc_code('X', 'B', '3', '0') /* [31:0] x:B:G:R 2:10:10:10 native endian */
> +#define DRM_FORMAT_RGBX1010102 fourcc_code('R', 'X', '3', '0') /* [31:0] R:G:B:x 10:10:10:2 native endian */
> +#define DRM_FORMAT_BGRX1010102 fourcc_code('B', 'X', '3', '0') /* [31:0] B:G:R:x 10:10:10:2 native endian */
>
> -#define DRM_FORMAT_ARGB2101010 fourcc_code('A', 'R', '3', '0') /* [31:0] A:R:G:B 2:10:10:10 little endian */
> -#define DRM_FORMAT_ABGR2101010 fourcc_code('A', 'B', '3', '0') /* [31:0] A:B:G:R 2:10:10:10 little endian */
> -#define DRM_FORMAT_RGBA1010102 fourcc_code('R', 'A', '3', '0') /* [31:0] R:G:B:A 10:10:10:2 little endian */
> -#define DRM_FORMAT_BGRA1010102 fourcc_code('B', 'A', '3', '0') /* [31:0] B:G:R:A 10:10:10:2 little endian */
> +#define DRM_FORMAT_ARGB2101010 fourcc_code('A', 'R', '3', '0') /* [31:0] A:R:G:B 2:10:10:10 native endian */
> +#define DRM_FORMAT_ABGR2101010 fourcc_code('A', 'B', '3', '0') /* [31:0] A:B:G:R 2:10:10:10 native endian */
> +#define DRM_FORMAT_RGBA1010102 fourcc_code('R', 'A', '3', '0') /* [31:0] R:G:B:A 10:10:10:2 native endian */
> +#define DRM_FORMAT_BGRA1010102 fourcc_code('B', 'A', '3', '0') /* [31:0] B:G:R:A 10:10:10:2 native endian */
>
>  /* packed YCbCr */
>  #define DRM_FORMAT_YUYV                fourcc_code('Y', 'U', 'Y', 'V') /* [31:0] Cr0:Y1:Cb0:Y0 8:8:8:8 little endian */
> --
> 2.9.3
>

Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.

[Ville Syrjälä]

On Fri, Apr 21, 2017 at 10:49:49AM -0400, Ilia Mirkin wrote:
> On Fri, Apr 21, 2017 at 3:58 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> > While working on graphics support for virtual machines on ppc64 (which
> > exists in both little and big endian variants) I've figured the comments
> > for various drm fourcc formats in the header file don't match reality.
> >
> > Comments says the RGB formats are little endian, but in practice they
> > are native endian.  Look at the drm_mode_legacy_fb_format() helper.  It
> > maps -- for example -- bpp/depth 32/24 to DRM_FORMAT_XRGB8888, no matter
> > whenever the machine is little endian or big endian.  The users of this
> > function (fbdev emulation, DRM_IOCTL_MODE_ADDFB) expect the framebuffer
> > is native endian, not little endian.  Most userspace also operates on
> > native endian only.
> >
> > So, go update the comments for all 16+24+32 bpp RGB formats.
> >
> > Leaving the yuv formats as-is.  I have no idea if and how those are used
> > on bigendian machines.
> 
> I think this is premature. The current situation is that I can't get
> modetest to work *at all* on my NV34 / BE setup (I mean, it runs, just
> the colors displayed are wrong). I believe that currently it packs
> things in "cpu native endian". I've tried futzing with that without
> much success, although I didn't spend too much time on it. I have a
> NV34 plugged into my LE setup as well although I haven't tested to
> double-check that it all works there. However I'm quite sure it used
> to, as I used modetest to help develop the YUV overlay support for
> those GPUs.

I just took a quick stab at fixing modetest to respect the current
wording in drm_fourcc.h:

git://github.com/vsyrjala/libdrm.git modetest_endian

> 
> Does modetest work for you, under any interpretation of the formats?
> 
> There's an additional complication wrt looking at what fbcon does,
> since it will feed the data in via special accel interfaces in fbdev,
> which at least on nouveau, may end up byteswapping the data on upload
> to VRAM (I'm not 100% clear on whether they do or not). However
> modetest, which is creating its own fb, likely won't get such
> treatment.
> 
> This is a shitty situation - we have hardware we don't know how it
> works, tools we don't know whether they're broken, and comments we're
> pretty sure are at least somewhat wrong. Furthermore the hardware is
> relatively rare and developers with time to work on improving it are
> even rarer.
> 
> I'd like to reiterate that the status quo does end up in a functioning
> system. Let's try not to break that.
> 
> Cheers,
> 
>   -ilia
> 
> >
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Pekka Paalanen <ppaalanen@gmail.com>
> > Cc: Ilia Mirkin <imirkin@alum.mit.edu>
> > Cc: Michel Dänzer <michel@daenzer.net>
> > Cc: Alex Deucher <alexdeucher@gmail.com>
> > Cc: amd-gfx@lists.freedesktop.org
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > ---
> >  include/uapi/drm/drm_fourcc.h | 82 +++++++++++++++++++++----------------------
> >  1 file changed, 41 insertions(+), 41 deletions(-)
> >
> > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> > index 995c8f9..1579765 100644
> > --- a/include/uapi/drm/drm_fourcc.h
> > +++ b/include/uapi/drm/drm_fourcc.h
> > @@ -42,68 +42,68 @@ extern "C" {
> >  #define DRM_FORMAT_R8          fourcc_code('R', '8', ' ', ' ') /* [7:0] R */
> >
> >  /* 16 bpp Red */
> > -#define DRM_FORMAT_R16         fourcc_code('R', '1', '6', ' ') /* [15:0] R little endian */
> > +#define DRM_FORMAT_R16         fourcc_code('R', '1', '6', ' ') /* [15:0] R native endian */
> >
> >  /* 16 bpp RG */
> > -#define DRM_FORMAT_RG88                fourcc_code('R', 'G', '8', '8') /* [15:0] R:G 8:8 little endian */
> > -#define DRM_FORMAT_GR88                fourcc_code('G', 'R', '8', '8') /* [15:0] G:R 8:8 little endian */
> > +#define DRM_FORMAT_RG88                fourcc_code('R', 'G', '8', '8') /* [15:0] R:G 8:8 native endian */
> > +#define DRM_FORMAT_GR88                fourcc_code('G', 'R', '8', '8') /* [15:0] G:R 8:8 native endian */
> >
> >  /* 32 bpp RG */
> > -#define DRM_FORMAT_RG1616      fourcc_code('R', 'G', '3', '2') /* [31:0] R:G 16:16 little endian */
> > -#define DRM_FORMAT_GR1616      fourcc_code('G', 'R', '3', '2') /* [31:0] G:R 16:16 little endian */
> > +#define DRM_FORMAT_RG1616      fourcc_code('R', 'G', '3', '2') /* [31:0] R:G 16:16 native endian */
> > +#define DRM_FORMAT_GR1616      fourcc_code('G', 'R', '3', '2') /* [31:0] G:R 16:16 native endian */
> >
> >  /* 8 bpp RGB */
> >  #define DRM_FORMAT_RGB332      fourcc_code('R', 'G', 'B', '8') /* [7:0] R:G:B 3:3:2 */
> >  #define DRM_FORMAT_BGR233      fourcc_code('B', 'G', 'R', '8') /* [7:0] B:G:R 2:3:3 */
> >
> >  /* 16 bpp RGB */
> > -#define DRM_FORMAT_XRGB4444    fourcc_code('X', 'R', '1', '2') /* [15:0] x:R:G:B 4:4:4:4 little endian */
> > -#define DRM_FORMAT_XBGR4444    fourcc_code('X', 'B', '1', '2') /* [15:0] x:B:G:R 4:4:4:4 little endian */
> > -#define DRM_FORMAT_RGBX4444    fourcc_code('R', 'X', '1', '2') /* [15:0] R:G:B:x 4:4:4:4 little endian */
> > -#define DRM_FORMAT_BGRX4444    fourcc_code('B', 'X', '1', '2') /* [15:0] B:G:R:x 4:4:4:4 little endian */
> > +#define DRM_FORMAT_XRGB4444    fourcc_code('X', 'R', '1', '2') /* [15:0] x:R:G:B 4:4:4:4 native endian */
> > +#define DRM_FORMAT_XBGR4444    fourcc_code('X', 'B', '1', '2') /* [15:0] x:B:G:R 4:4:4:4 native endian */
> > +#define DRM_FORMAT_RGBX4444    fourcc_code('R', 'X', '1', '2') /* [15:0] R:G:B:x 4:4:4:4 native endian */
> > +#define DRM_FORMAT_BGRX4444    fourcc_code('B', 'X', '1', '2') /* [15:0] B:G:R:x 4:4:4:4 native endian */
> >
> > -#define DRM_FORMAT_ARGB4444    fourcc_code('A', 'R', '1', '2') /* [15:0] A:R:G:B 4:4:4:4 little endian */
> > -#define DRM_FORMAT_ABGR4444    fourcc_code('A', 'B', '1', '2') /* [15:0] A:B:G:R 4:4:4:4 little endian */
> > -#define DRM_FORMAT_RGBA4444    fourcc_code('R', 'A', '1', '2') /* [15:0] R:G:B:A 4:4:4:4 little endian */
> > -#define DRM_FORMAT_BGRA4444    fourcc_code('B', 'A', '1', '2') /* [15:0] B:G:R:A 4:4:4:4 little endian */
> > +#define DRM_FORMAT_ARGB4444    fourcc_code('A', 'R', '1', '2') /* [15:0] A:R:G:B 4:4:4:4 native endian */
> > +#define DRM_FORMAT_ABGR4444    fourcc_code('A', 'B', '1', '2') /* [15:0] A:B:G:R 4:4:4:4 native endian */
> > +#define DRM_FORMAT_RGBA4444    fourcc_code('R', 'A', '1', '2') /* [15:0] R:G:B:A 4:4:4:4 native endian */
> > +#define DRM_FORMAT_BGRA4444    fourcc_code('B', 'A', '1', '2') /* [15:0] B:G:R:A 4:4:4:4 native endian */
> >
> > -#define DRM_FORMAT_XRGB1555    fourcc_code('X', 'R', '1', '5') /* [15:0] x:R:G:B 1:5:5:5 little endian */
> > -#define DRM_FORMAT_XBGR1555    fourcc_code('X', 'B', '1', '5') /* [15:0] x:B:G:R 1:5:5:5 little endian */
> > -#define DRM_FORMAT_RGBX5551    fourcc_code('R', 'X', '1', '5') /* [15:0] R:G:B:x 5:5:5:1 little endian */
> > -#define DRM_FORMAT_BGRX5551    fourcc_code('B', 'X', '1', '5') /* [15:0] B:G:R:x 5:5:5:1 little endian */
> > +#define DRM_FORMAT_XRGB1555    fourcc_code('X', 'R', '1', '5') /* [15:0] x:R:G:B 1:5:5:5 native endian */
> > +#define DRM_FORMAT_XBGR1555    fourcc_code('X', 'B', '1', '5') /* [15:0] x:B:G:R 1:5:5:5 native endian */
> > +#define DRM_FORMAT_RGBX5551    fourcc_code('R', 'X', '1', '5') /* [15:0] R:G:B:x 5:5:5:1 native endian */
> > +#define DRM_FORMAT_BGRX5551    fourcc_code('B', 'X', '1', '5') /* [15:0] B:G:R:x 5:5:5:1 native endian */
> >
> > -#define DRM_FORMAT_ARGB1555    fourcc_code('A', 'R', '1', '5') /* [15:0] A:R:G:B 1:5:5:5 little endian */
> > -#define DRM_FORMAT_ABGR1555    fourcc_code('A', 'B', '1', '5') /* [15:0] A:B:G:R 1:5:5:5 little endian */
> > -#define DRM_FORMAT_RGBA5551    fourcc_code('R', 'A', '1', '5') /* [15:0] R:G:B:A 5:5:5:1 little endian */
> > -#define DRM_FORMAT_BGRA5551    fourcc_code('B', 'A', '1', '5') /* [15:0] B:G:R:A 5:5:5:1 little endian */
> > +#define DRM_FORMAT_ARGB1555    fourcc_code('A', 'R', '1', '5') /* [15:0] A:R:G:B 1:5:5:5 native endian */
> > +#define DRM_FORMAT_ABGR1555    fourcc_code('A', 'B', '1', '5') /* [15:0] A:B:G:R 1:5:5:5 native endian */
> > +#define DRM_FORMAT_RGBA5551    fourcc_code('R', 'A', '1', '5') /* [15:0] R:G:B:A 5:5:5:1 native endian */
> > +#define DRM_FORMAT_BGRA5551    fourcc_code('B', 'A', '1', '5') /* [15:0] B:G:R:A 5:5:5:1 native endian */
> >
> > -#define DRM_FORMAT_RGB565      fourcc_code('R', 'G', '1', '6') /* [15:0] R:G:B 5:6:5 little endian */
> > -#define DRM_FORMAT_BGR565      fourcc_code('B', 'G', '1', '6') /* [15:0] B:G:R 5:6:5 little endian */
> > +#define DRM_FORMAT_RGB565      fourcc_code('R', 'G', '1', '6') /* [15:0] R:G:B 5:6:5 native endian */
> > +#define DRM_FORMAT_BGR565      fourcc_code('B', 'G', '1', '6') /* [15:0] B:G:R 5:6:5 native endian */
> >
> >  /* 24 bpp RGB */
> > -#define DRM_FORMAT_RGB888      fourcc_code('R', 'G', '2', '4') /* [23:0] R:G:B little endian */
> > -#define DRM_FORMAT_BGR888      fourcc_code('B', 'G', '2', '4') /* [23:0] B:G:R little endian */
> > +#define DRM_FORMAT_RGB888      fourcc_code('R', 'G', '2', '4') /* [23:0] R:G:B native endian */
> > +#define DRM_FORMAT_BGR888      fourcc_code('B', 'G', '2', '4') /* [23:0] B:G:R native endian */
> >
> >  /* 32 bpp RGB */
> > -#define DRM_FORMAT_XRGB8888    fourcc_code('X', 'R', '2', '4') /* [31:0] x:R:G:B 8:8:8:8 little endian */
> > -#define DRM_FORMAT_XBGR8888    fourcc_code('X', 'B', '2', '4') /* [31:0] x:B:G:R 8:8:8:8 little endian */
> > -#define DRM_FORMAT_RGBX8888    fourcc_code('R', 'X', '2', '4') /* [31:0] R:G:B:x 8:8:8:8 little endian */
> > -#define DRM_FORMAT_BGRX8888    fourcc_code('B', 'X', '2', '4') /* [31:0] B:G:R:x 8:8:8:8 little endian */
> > +#define DRM_FORMAT_XRGB8888    fourcc_code('X', 'R', '2', '4') /* [31:0] x:R:G:B 8:8:8:8 native endian */
> > +#define DRM_FORMAT_XBGR8888    fourcc_code('X', 'B', '2', '4') /* [31:0] x:B:G:R 8:8:8:8 native endian */
> > +#define DRM_FORMAT_RGBX8888    fourcc_code('R', 'X', '2', '4') /* [31:0] R:G:B:x 8:8:8:8 native endian */
> > +#define DRM_FORMAT_BGRX8888    fourcc_code('B', 'X', '2', '4') /* [31:0] B:G:R:x 8:8:8:8 native endian */
> >
> > -#define DRM_FORMAT_ARGB8888    fourcc_code('A', 'R', '2', '4') /* [31:0] A:R:G:B 8:8:8:8 little endian */
> > -#define DRM_FORMAT_ABGR8888    fourcc_code('A', 'B', '2', '4') /* [31:0] A:B:G:R 8:8:8:8 little endian */
> > -#define DRM_FORMAT_RGBA8888    fourcc_code('R', 'A', '2', '4') /* [31:0] R:G:B:A 8:8:8:8 little endian */
> > -#define DRM_FORMAT_BGRA8888    fourcc_code('B', 'A', '2', '4') /* [31:0] B:G:R:A 8:8:8:8 little endian */
> > +#define DRM_FORMAT_ARGB8888    fourcc_code('A', 'R', '2', '4') /* [31:0] A:R:G:B 8:8:8:8 native endian */
> > +#define DRM_FORMAT_ABGR8888    fourcc_code('A', 'B', '2', '4') /* [31:0] A:B:G:R 8:8:8:8 native endian */
> > +#define DRM_FORMAT_RGBA8888    fourcc_code('R', 'A', '2', '4') /* [31:0] R:G:B:A 8:8:8:8 native endian */
> > +#define DRM_FORMAT_BGRA8888    fourcc_code('B', 'A', '2', '4') /* [31:0] B:G:R:A 8:8:8:8 native endian */
> >
> > -#define DRM_FORMAT_XRGB2101010 fourcc_code('X', 'R', '3', '0') /* [31:0] x:R:G:B 2:10:10:10 little endian */
> > -#define DRM_FORMAT_XBGR2101010 fourcc_code('X', 'B', '3', '0') /* [31:0] x:B:G:R 2:10:10:10 little endian */
> > -#define DRM_FORMAT_RGBX1010102 fourcc_code('R', 'X', '3', '0') /* [31:0] R:G:B:x 10:10:10:2 little endian */
> > -#define DRM_FORMAT_BGRX1010102 fourcc_code('B', 'X', '3', '0') /* [31:0] B:G:R:x 10:10:10:2 little endian */
> > +#define DRM_FORMAT_XRGB2101010 fourcc_code('X', 'R', '3', '0') /* [31:0] x:R:G:B 2:10:10:10 native endian */
> > +#define DRM_FORMAT_XBGR2101010 fourcc_code('X', 'B', '3', '0') /* [31:0] x:B:G:R 2:10:10:10 native endian */
> > +#define DRM_FORMAT_RGBX1010102 fourcc_code('R', 'X', '3', '0') /* [31:0] R:G:B:x 10:10:10:2 native endian */
> > +#define DRM_FORMAT_BGRX1010102 fourcc_code('B', 'X', '3', '0') /* [31:0] B:G:R:x 10:10:10:2 native endian */
> >
> > -#define DRM_FORMAT_ARGB2101010 fourcc_code('A', 'R', '3', '0') /* [31:0] A:R:G:B 2:10:10:10 little endian */
> > -#define DRM_FORMAT_ABGR2101010 fourcc_code('A', 'B', '3', '0') /* [31:0] A:B:G:R 2:10:10:10 little endian */
> > -#define DRM_FORMAT_RGBA1010102 fourcc_code('R', 'A', '3', '0') /* [31:0] R:G:B:A 10:10:10:2 little endian */
> > -#define DRM_FORMAT_BGRA1010102 fourcc_code('B', 'A', '3', '0') /* [31:0] B:G:R:A 10:10:10:2 little endian */
> > +#define DRM_FORMAT_ARGB2101010 fourcc_code('A', 'R', '3', '0') /* [31:0] A:R:G:B 2:10:10:10 native endian */
> > +#define DRM_FORMAT_ABGR2101010 fourcc_code('A', 'B', '3', '0') /* [31:0] A:B:G:R 2:10:10:10 native endian */
> > +#define DRM_FORMAT_RGBA1010102 fourcc_code('R', 'A', '3', '0') /* [31:0] R:G:B:A 10:10:10:2 native endian */
> > +#define DRM_FORMAT_BGRA1010102 fourcc_code('B', 'A', '3', '0') /* [31:0] B:G:R:A 10:10:10:2 native endian */
> >
> >  /* packed YCbCr */
> >  #define DRM_FORMAT_YUYV                fourcc_code('Y', 'U', 'Y', 'V') /* [31:0] Cr0:Y1:Cb0:Y0 8:8:8:8 little endian */
> > --
> > 2.9.3
> >

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.

[Ilia Mirkin]

On Fri, Apr 21, 2017 at 12:59 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Fri, Apr 21, 2017 at 10:49:49AM -0400, Ilia Mirkin wrote:
>> On Fri, Apr 21, 2017 at 3:58 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>> > While working on graphics support for virtual machines on ppc64 (which
>> > exists in both little and big endian variants) I've figured the comments
>> > for various drm fourcc formats in the header file don't match reality.
>> >
>> > Comments says the RGB formats are little endian, but in practice they
>> > are native endian.  Look at the drm_mode_legacy_fb_format() helper.  It
>> > maps -- for example -- bpp/depth 32/24 to DRM_FORMAT_XRGB8888, no matter
>> > whenever the machine is little endian or big endian.  The users of this
>> > function (fbdev emulation, DRM_IOCTL_MODE_ADDFB) expect the framebuffer
>> > is native endian, not little endian.  Most userspace also operates on
>> > native endian only.
>> >
>> > So, go update the comments for all 16+24+32 bpp RGB formats.
>> >
>> > Leaving the yuv formats as-is.  I have no idea if and how those are used
>> > on bigendian machines.
>>
>> I think this is premature. The current situation is that I can't get
>> modetest to work *at all* on my NV34 / BE setup (I mean, it runs, just
>> the colors displayed are wrong). I believe that currently it packs
>> things in "cpu native endian". I've tried futzing with that without
>> much success, although I didn't spend too much time on it. I have a
>> NV34 plugged into my LE setup as well although I haven't tested to
>> double-check that it all works there. However I'm quite sure it used
>> to, as I used modetest to help develop the YUV overlay support for
>> those GPUs.
>
> I just took a quick stab at fixing modetest to respect the current
> wording in drm_fourcc.h:
>
> git://github.com/vsyrjala/libdrm.git modetest_endian

Looks like there was some careless testing on my part :( So ... it
looks like the current modetest without those changes does, in fact,
work on NV34/BE. With the changes, it breaks (and the handling of the
b* modes is a little broken in those patches -- they're not selectable
from the cmdline.) Which means that, as Michel & co predicted, it
appears to be taking BE input not LE input. This is very surprising to
me, but it is what it is. As I mentioned before, the details of how
the "BE" mode works on the GPUs is largely unknown to us beyond a few
basics. Note that only XR24 works, AR24 ends up with all black
displayed. This also happens on LE.

Furthermore, all of YUYV, UYVY, and NV12 plane overlays don't appear
to work properly. The first half of the overlay is OK (but I think
compressed), while the second half is gibberish. Testing this on my
board plugged into a LE CPU, I also get the same type of issue, so I'm
guessing that it's just bitrot of the feature. (Or modetest gained a
bug.)

Cheers,

  -ilia

Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.

[Ville Syrjälä]

On Sat, Apr 22, 2017 at 01:07:57AM -0400, Ilia Mirkin wrote:
> On Fri, Apr 21, 2017 at 12:59 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> > On Fri, Apr 21, 2017 at 10:49:49AM -0400, Ilia Mirkin wrote:
> >> On Fri, Apr 21, 2017 at 3:58 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> >> > While working on graphics support for virtual machines on ppc64 (which
> >> > exists in both little and big endian variants) I've figured the comments
> >> > for various drm fourcc formats in the header file don't match reality.
> >> >
> >> > Comments says the RGB formats are little endian, but in practice they
> >> > are native endian.  Look at the drm_mode_legacy_fb_format() helper.  It
> >> > maps -- for example -- bpp/depth 32/24 to DRM_FORMAT_XRGB8888, no matter
> >> > whenever the machine is little endian or big endian.  The users of this
> >> > function (fbdev emulation, DRM_IOCTL_MODE_ADDFB) expect the framebuffer
> >> > is native endian, not little endian.  Most userspace also operates on
> >> > native endian only.
> >> >
> >> > So, go update the comments for all 16+24+32 bpp RGB formats.
> >> >
> >> > Leaving the yuv formats as-is.  I have no idea if and how those are used
> >> > on bigendian machines.
> >>
> >> I think this is premature. The current situation is that I can't get
> >> modetest to work *at all* on my NV34 / BE setup (I mean, it runs, just
> >> the colors displayed are wrong). I believe that currently it packs
> >> things in "cpu native endian". I've tried futzing with that without
> >> much success, although I didn't spend too much time on it. I have a
> >> NV34 plugged into my LE setup as well although I haven't tested to
> >> double-check that it all works there. However I'm quite sure it used
> >> to, as I used modetest to help develop the YUV overlay support for
> >> those GPUs.
> >
> > I just took a quick stab at fixing modetest to respect the current
> > wording in drm_fourcc.h:
> >
> > git://github.com/vsyrjala/libdrm.git modetest_endian
> 
> Looks like there was some careless testing on my part :( So ... it
> looks like the current modetest without those changes does, in fact,
> work on NV34/BE. With the changes, it breaks (and the handling of the
> b* modes is a little broken in those patches -- they're not selectable
> from the cmdline.) Which means that, as Michel & co predicted, it
> appears to be taking BE input not LE input. This is very surprising to
> me, but it is what it is. As I mentioned before, the details of how
> the "BE" mode works on the GPUs is largely unknown to us beyond a few
> basics. Note that only XR24 works, AR24 ends up with all black
> displayed. This also happens on LE.

Did you try 8bpp or 16bpp formats? I expect that if you've just blindly
enabled some magic byte swapper in the hardware it will only for
a specific pixel size.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.

[Ilia Mirkin]

On Sat, Apr 22, 2017 at 5:50 AM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Sat, Apr 22, 2017 at 01:07:57AM -0400, Ilia Mirkin wrote:
>> On Fri, Apr 21, 2017 at 12:59 PM, Ville Syrjälä
>> <ville.syrjala@linux.intel.com> wrote:
>> > On Fri, Apr 21, 2017 at 10:49:49AM -0400, Ilia Mirkin wrote:
>> >> On Fri, Apr 21, 2017 at 3:58 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>> >> > While working on graphics support for virtual machines on ppc64 (which
>> >> > exists in both little and big endian variants) I've figured the comments
>> >> > for various drm fourcc formats in the header file don't match reality.
>> >> >
>> >> > Comments says the RGB formats are little endian, but in practice they
>> >> > are native endian.  Look at the drm_mode_legacy_fb_format() helper.  It
>> >> > maps -- for example -- bpp/depth 32/24 to DRM_FORMAT_XRGB8888, no matter
>> >> > whenever the machine is little endian or big endian.  The users of this
>> >> > function (fbdev emulation, DRM_IOCTL_MODE_ADDFB) expect the framebuffer
>> >> > is native endian, not little endian.  Most userspace also operates on
>> >> > native endian only.
>> >> >
>> >> > So, go update the comments for all 16+24+32 bpp RGB formats.
>> >> >
>> >> > Leaving the yuv formats as-is.  I have no idea if and how those are used
>> >> > on bigendian machines.
>> >>
>> >> I think this is premature. The current situation is that I can't get
>> >> modetest to work *at all* on my NV34 / BE setup (I mean, it runs, just
>> >> the colors displayed are wrong). I believe that currently it packs
>> >> things in "cpu native endian". I've tried futzing with that without
>> >> much success, although I didn't spend too much time on it. I have a
>> >> NV34 plugged into my LE setup as well although I haven't tested to
>> >> double-check that it all works there. However I'm quite sure it used
>> >> to, as I used modetest to help develop the YUV overlay support for
>> >> those GPUs.
>> >
>> > I just took a quick stab at fixing modetest to respect the current
>> > wording in drm_fourcc.h:
>> >
>> > git://github.com/vsyrjala/libdrm.git modetest_endian
>>
>> Looks like there was some careless testing on my part :( So ... it
>> looks like the current modetest without those changes does, in fact,
>> work on NV34/BE. With the changes, it breaks (and the handling of the
>> b* modes is a little broken in those patches -- they're not selectable
>> from the cmdline.) Which means that, as Michel & co predicted, it
>> appears to be taking BE input not LE input. This is very surprising to
>> me, but it is what it is. As I mentioned before, the details of how
>> the "BE" mode works on the GPUs is largely unknown to us beyond a few
>> basics. Note that only XR24 works, AR24 ends up with all black
>> displayed. This also happens on LE.
>
> Did you try 8bpp or 16bpp formats? I expect that if you've just blindly
> enabled some magic byte swapper in the hardware it will only for
> a specific pixel size.

Thankfully dispnv04 exposes no such madness - just XR24 (and AR24,
although that doesn't appear functional). Yes, it's likely that
there's a byteswap happening somewhere. In fact the copy engines have
parameters somewhere to tell how the swap should be done (basically
what the element size is). I don't quite know how to set that though
on this generation. I should poke at VRAM via the mmio peephole and
see what's actually being stored. Although of course MMIO accesses are
also auto-byteswapped. It's all just one big massive headache.

  -ilia

Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.

[Ilia Mirkin]

On Sat, Apr 22, 2017 at 9:40 AM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
> On Sat, Apr 22, 2017 at 5:50 AM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
>> On Sat, Apr 22, 2017 at 01:07:57AM -0400, Ilia Mirkin wrote:
>>> On Fri, Apr 21, 2017 at 12:59 PM, Ville Syrjälä
>>> <ville.syrjala@linux.intel.com> wrote:
>>> > On Fri, Apr 21, 2017 at 10:49:49AM -0400, Ilia Mirkin wrote:
>>> >> On Fri, Apr 21, 2017 at 3:58 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>>> >> > While working on graphics support for virtual machines on ppc64 (which
>>> >> > exists in both little and big endian variants) I've figured the comments
>>> >> > for various drm fourcc formats in the header file don't match reality.
>>> >> >
>>> >> > Comments says the RGB formats are little endian, but in practice they
>>> >> > are native endian.  Look at the drm_mode_legacy_fb_format() helper.  It
>>> >> > maps -- for example -- bpp/depth 32/24 to DRM_FORMAT_XRGB8888, no matter
>>> >> > whenever the machine is little endian or big endian.  The users of this
>>> >> > function (fbdev emulation, DRM_IOCTL_MODE_ADDFB) expect the framebuffer
>>> >> > is native endian, not little endian.  Most userspace also operates on
>>> >> > native endian only.
>>> >> >
>>> >> > So, go update the comments for all 16+24+32 bpp RGB formats.
>>> >> >
>>> >> > Leaving the yuv formats as-is.  I have no idea if and how those are used
>>> >> > on bigendian machines.
>>> >>
>>> >> I think this is premature. The current situation is that I can't get
>>> >> modetest to work *at all* on my NV34 / BE setup (I mean, it runs, just
>>> >> the colors displayed are wrong). I believe that currently it packs
>>> >> things in "cpu native endian". I've tried futzing with that without
>>> >> much success, although I didn't spend too much time on it. I have a
>>> >> NV34 plugged into my LE setup as well although I haven't tested to
>>> >> double-check that it all works there. However I'm quite sure it used
>>> >> to, as I used modetest to help develop the YUV overlay support for
>>> >> those GPUs.
>>> >
>>> > I just took a quick stab at fixing modetest to respect the current
>>> > wording in drm_fourcc.h:
>>> >
>>> > git://github.com/vsyrjala/libdrm.git modetest_endian
>>>
>>> Looks like there was some careless testing on my part :( So ... it
>>> looks like the current modetest without those changes does, in fact,
>>> work on NV34/BE. With the changes, it breaks (and the handling of the
>>> b* modes is a little broken in those patches -- they're not selectable
>>> from the cmdline.) Which means that, as Michel & co predicted, it
>>> appears to be taking BE input not LE input. This is very surprising to
>>> me, but it is what it is. As I mentioned before, the details of how
>>> the "BE" mode works on the GPUs is largely unknown to us beyond a few
>>> basics. Note that only XR24 works, AR24 ends up with all black
>>> displayed. This also happens on LE.
>>
>> Did you try 8bpp or 16bpp formats? I expect that if you've just blindly
>> enabled some magic byte swapper in the hardware it will only for
>> a specific pixel size.
>
> Thankfully dispnv04 exposes no such madness - just XR24 (and AR24,
> although that doesn't appear functional). Yes, it's likely that
> there's a byteswap happening somewhere. In fact the copy engines have
> parameters somewhere to tell how the swap should be done (basically
> what the element size is). I don't quite know how to set that though
> on this generation. I should poke at VRAM via the mmio peephole and
> see what's actually being stored. Although of course MMIO accesses are
> also auto-byteswapped. It's all just one big massive headache.

Or it could just be the obvious:

static void nv_crtc_commit(struct drm_crtc *crtc)
{
        struct drm_device *dev = crtc->dev;
        const struct drm_crtc_helper_funcs *funcs = crtc->helper_private;
        struct nouveau_crtc *nv_crtc = nouveau_crtc(crtc);

        nouveau_hw_load_state(dev, nv_crtc->index,
&nv04_display(dev)->mode_reg);
        nv04_crtc_mode_set_base(crtc, crtc->x, crtc->y, NULL);

#ifdef __BIG_ENDIAN
        /* turn on LFB swapping */
        {
                uint8_t tmp = NVReadVgaCrtc(dev, nv_crtc->index,
NV_CIO_CRE_RCR);
                tmp |= MASK(NV_CIO_CRE_RCR_ENDIAN_BIG);
                NVWriteVgaCrtc(dev, nv_crtc->index, NV_CIO_CRE_RCR, tmp);
        }
#endif

        funcs->dpms(crtc, DRM_MODE_DPMS_ON);
        drm_crtc_vblank_on(crtc);
}

So presumably instead of that __BIG_ENDIAN thing, it should instead be
if crtc->primary->fb->fourcc & BIG_ENDIAN. (Although that's probably
going to break the universe.) There's also some questionable support
for 16-bit modes in the code, I'm going to see how easy it is to flip
on.

  -ilia

Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.

[Ilia Mirkin]

On Sat, Apr 22, 2017 at 9:48 AM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
> On Sat, Apr 22, 2017 at 9:40 AM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>> On Sat, Apr 22, 2017 at 5:50 AM, Ville Syrjälä
>> <ville.syrjala@linux.intel.com> wrote:
>>> On Sat, Apr 22, 2017 at 01:07:57AM -0400, Ilia Mirkin wrote:
>>>> On Fri, Apr 21, 2017 at 12:59 PM, Ville Syrjälä
>>>> <ville.syrjala@linux.intel.com> wrote:
>>>> > On Fri, Apr 21, 2017 at 10:49:49AM -0400, Ilia Mirkin wrote:
>>>> >> On Fri, Apr 21, 2017 at 3:58 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>>>> >> > While working on graphics support for virtual machines on ppc64 (which
>>>> >> > exists in both little and big endian variants) I've figured the comments
>>>> >> > for various drm fourcc formats in the header file don't match reality.
>>>> >> >
>>>> >> > Comments says the RGB formats are little endian, but in practice they
>>>> >> > are native endian.  Look at the drm_mode_legacy_fb_format() helper.  It
>>>> >> > maps -- for example -- bpp/depth 32/24 to DRM_FORMAT_XRGB8888, no matter
>>>> >> > whenever the machine is little endian or big endian.  The users of this
>>>> >> > function (fbdev emulation, DRM_IOCTL_MODE_ADDFB) expect the framebuffer
>>>> >> > is native endian, not little endian.  Most userspace also operates on
>>>> >> > native endian only.
>>>> >> >
>>>> >> > So, go update the comments for all 16+24+32 bpp RGB formats.
>>>> >> >
>>>> >> > Leaving the yuv formats as-is.  I have no idea if and how those are used
>>>> >> > on bigendian machines.
>>>> >>
>>>> >> I think this is premature. The current situation is that I can't get
>>>> >> modetest to work *at all* on my NV34 / BE setup (I mean, it runs, just
>>>> >> the colors displayed are wrong). I believe that currently it packs
>>>> >> things in "cpu native endian". I've tried futzing with that without
>>>> >> much success, although I didn't spend too much time on it. I have a
>>>> >> NV34 plugged into my LE setup as well although I haven't tested to
>>>> >> double-check that it all works there. However I'm quite sure it used
>>>> >> to, as I used modetest to help develop the YUV overlay support for
>>>> >> those GPUs.
>>>> >
>>>> > I just took a quick stab at fixing modetest to respect the current
>>>> > wording in drm_fourcc.h:
>>>> >
>>>> > git://github.com/vsyrjala/libdrm.git modetest_endian
>>>>
>>>> Looks like there was some careless testing on my part :( So ... it
>>>> looks like the current modetest without those changes does, in fact,
>>>> work on NV34/BE. With the changes, it breaks (and the handling of the
>>>> b* modes is a little broken in those patches -- they're not selectable
>>>> from the cmdline.) Which means that, as Michel & co predicted, it
>>>> appears to be taking BE input not LE input. This is very surprising to
>>>> me, but it is what it is. As I mentioned before, the details of how
>>>> the "BE" mode works on the GPUs is largely unknown to us beyond a few
>>>> basics. Note that only XR24 works, AR24 ends up with all black
>>>> displayed. This also happens on LE.
>>>
>>> Did you try 8bpp or 16bpp formats? I expect that if you've just blindly
>>> enabled some magic byte swapper in the hardware it will only for
>>> a specific pixel size.
>>
>> Thankfully dispnv04 exposes no such madness - just XR24 (and AR24,
>> although that doesn't appear functional). Yes, it's likely that
>> there's a byteswap happening somewhere. In fact the copy engines have
>> parameters somewhere to tell how the swap should be done (basically
>> what the element size is). I don't quite know how to set that though
>> on this generation. I should poke at VRAM via the mmio peephole and
>> see what's actually being stored. Although of course MMIO accesses are
>> also auto-byteswapped. It's all just one big massive headache.
>
> Or it could just be the obvious:
>
> static void nv_crtc_commit(struct drm_crtc *crtc)
> {
>         struct drm_device *dev = crtc->dev;
>         const struct drm_crtc_helper_funcs *funcs = crtc->helper_private;
>         struct nouveau_crtc *nv_crtc = nouveau_crtc(crtc);
>
>         nouveau_hw_load_state(dev, nv_crtc->index,
> &nv04_display(dev)->mode_reg);
>         nv04_crtc_mode_set_base(crtc, crtc->x, crtc->y, NULL);
>
> #ifdef __BIG_ENDIAN
>         /* turn on LFB swapping */
>         {
>                 uint8_t tmp = NVReadVgaCrtc(dev, nv_crtc->index,
> NV_CIO_CRE_RCR);
>                 tmp |= MASK(NV_CIO_CRE_RCR_ENDIAN_BIG);
>                 NVWriteVgaCrtc(dev, nv_crtc->index, NV_CIO_CRE_RCR, tmp);
>         }
> #endif
>
>         funcs->dpms(crtc, DRM_MODE_DPMS_ON);
>         drm_crtc_vblank_on(crtc);
> }
>
> So presumably instead of that __BIG_ENDIAN thing, it should instead be
> if crtc->primary->fb->fourcc & BIG_ENDIAN. (Although that's probably
> going to break the universe.) There's also some questionable support
> for 16-bit modes in the code, I'm going to see how easy it is to flip
> on.

OK, so just to follow up, I'd like to note a few things that were not
obvious to me, but perhaps were to all of you. I'll say them anyways.
As a preface, this is all addressing the pre-nv50 nouveau support.
This support is a port of a port of a port of the (rather old)
xf86-video-nv code, which exists in no less than 3 places in the
kernel, among other things. It largely lives in
drivers/gpu/drm/nouveau/dispnv04. The reason for this is that my G5
PowerMac7,3 only has PCI and AGP slots, and I don't have any nv50+ hw
that'll fit. I'll try to look for some though, I hear it exists (and
according to some very rare reports, also has BE issues).

First of all, while nv50+ nouveau supports all the new formats/etc
hotness, dispnv04 code was using drm_crtc_init. This is a helper that
is used by many drivers and provides XRGB8888 and ARGB8888 formats, no
questions asked. My guess is that this happened *well* after the code
was ported to drm. As-is, these are the only formats exposed to the
world.

The dispnv04 backend logic supports both XRGB8888, RGB565 (not
exposed), and XRGB1555 (not exposed). The ARGB variants don't appear
to work, although I haven't investigated why. It is also configured
to, based on a compile time setting, enable the byteswap logic in the
CRTC unit (or RAMDAC, who knows what the full pipeline is). I tested
this works seemingly correctly for all three formats, by adapting
Ville's updated libdrm branch with modetest. (There may theoretically
be a paletted mode as well, but since it's 8-bit and not properly set
up, figured it wasn't worth trying to work out.)

The drm helper code isn't ready for DRM_FORMAT_BIG_ENDIAN support.
Among other things, drm_format_info() fails to find the format, and
even if it's fixed to ignore that bit, whether the fb is in a pixel
format with that flag defined is lost after the fb is created. fbdev
also creates fb's that expect cpu endianness, as disabling the
byteswap logic caused a green fbcon terminal to show up. (So at least
something somewhere in the fbcon -> nouveau's fbdev emulation pipeline
is expecting cpu endianness. This happens both with nouveau's fbdev
accel logic and without.)

So I think the current situation, at least wrt pre-nv50 nouveau, is
that XRGB/ARGB8888 are "special", since they are the only things
exposed by drm_crtc_init. I believe those definitions should be
updated to note that they're cpu-endian-specific (or another way of
phrasing it more diplomatically is that they're array formats rather
than packed formats). The rest of the formats aren't needed/used by
the legacy world, and can have whatever proper definitions we want,
including better support for the BIG_ENDIAN flag. We can also define
new RGBA8 formats that are defined to be packed in the declared
endianness. But I don't see a way out of dealing with the fact that
every legacy driver (which, like it or not, is mostly what BE hw gets
stuck with), expects these to be in cpu-endian formats, and as does
the rest of the existing userspace stack.

Cheers,

  -ilia

Re: [PATCH] drm: fourcc byteorder: brings header file comments in line with reality.

[Michel Dänzer]

On 23/04/17 04:24 AM, Ilia Mirkin wrote:
> 
> fbdev also creates fb's that expect cpu endianness, as disabling the
> byteswap logic caused a green fbcon terminal to show up. (So at least
> something somewhere in the fbcon -> nouveau's fbdev emulation pipeline
> is expecting cpu endianness. This happens both with nouveau's fbdev
> accel logic and without.)

In theory, there's FB_FOREIGN_ENDIAN for that. But in practice it's
probably useless because little if any userspace even checks for it, let
alone handles it correctly.


> So I think the current situation, at least wrt pre-nv50 nouveau, is
> that XRGB/ARGB8888 are "special", since they are the only things
> exposed by drm_crtc_init. I believe those definitions should be
> updated to note that they're cpu-endian-specific (or another way of
> phrasing it more diplomatically is that they're array formats rather
> than packed formats).

That would be incorrect. :) The memory layout of 8-bit-per-component
array formats doesn't depend on endianness, that of packed formats does.
(DRM_FORMAT_*8 as currently defined are thus effectively array formats)


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer