[PATCH 0/6] drm: tackle byteorder issues, take two

[PATCH 0/6] drm: tackle byteorder issues, take two

[Gerd Hoffmann]

  Hi,

Ok, different approach up for discussion.  Given that userspace didn't
made the transition from ADDFB to ADDFB2 yet it seems we still can muck
with the fourcc codes without breaking everything, as long as we
maintain ADDFB and fbdev behavior (use cpu byte order format) so nothing
changes for userspace.

So, this series basically makes drm_mode_legacy_fb_format return correct
formats in bigendian mode and adapts the bochs and virtio drivers to
this change.  Other drivers must be adapted to this change too.

Ilia Mirkin figured the dispnv04 backend in nouveau turns on/off
byteswapping depending on cpu byte order.  So, one way to adapt the
driver would be to simply use the new #defines added by patch #2.  The
other way would be to support both XRGB and BGRX and turn on/off
byteswapping depending on framebuffer format instead of cpu byte order.

cheers,
  Gerd

Gerd Hoffmann (6):
  drm: fourcc byteorder: drop DRM_FORMAT_BIG_ENDIAN
  drm: fourcc byteorder: add DRM_FORMAT_CPU_*
  drm: fourcc byteorder: add bigendian support to
    drm_mode_legacy_fb_format
  drm: fourcc byteorder: adapt bochs-drm to drm_mode_legacy_fb_format
    update
  drm: fourcc byteorder: adapt virtio to drm_mode_legacy_fb_format
    update
  drm: fourcc byteorder: virtio restrict to XRGB8888

 include/drm/drm_fourcc.h               | 12 ++++++++++
 include/uapi/drm/drm_fourcc.h          |  2 --
 drivers/gpu/drm/bochs/bochs_mm.c       |  2 +-
 drivers/gpu/drm/drm_fourcc.c           | 27 +++++++++++++++++++++--
 drivers/gpu/drm/drm_framebuffer.c      |  2 +-
 drivers/gpu/drm/virtio/virtgpu_gem.c   |  7 ++++--
 drivers/gpu/drm/virtio/virtgpu_plane.c | 40 +---------------------------------
 7 files changed, 45 insertions(+), 47 deletions(-)

-- 
2.9.3

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 1/6] drm: fourcc byteorder: drop DRM_FORMAT_BIG_ENDIAN

[Gerd Hoffmann]

It's unused.

Suggested-by: Daniel Vetter <daniel.vetter@intel.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/uapi/drm/drm_fourcc.h     | 2 --
 drivers/gpu/drm/drm_fourcc.c      | 3 +--
 drivers/gpu/drm/drm_framebuffer.c | 2 +-
 3 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
index 995c8f9c69..305bc34be0 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -33,8 +33,6 @@ extern "C" {
 #define fourcc_code(a, b, c, d) ((__u32)(a) | ((__u32)(b) << 8) | \
 				 ((__u32)(c) << 16) | ((__u32)(d) << 24))
 
-#define DRM_FORMAT_BIG_ENDIAN (1<<31) /* format is big endian instead of little endian */
-
 /* color index */
 #define DRM_FORMAT_C8		fourcc_code('C', '8', ' ', ' ') /* [7:0] C */
 
diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
index 9c0152df45..adb3ff59a4 100644
--- a/drivers/gpu/drm/drm_fourcc.c
+++ b/drivers/gpu/drm/drm_fourcc.c
@@ -86,12 +86,11 @@ EXPORT_SYMBOL(drm_mode_legacy_fb_format);
 const char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf)
 {
 	snprintf(buf->str, sizeof(buf->str),
-		 "%c%c%c%c %s-endian (0x%08x)",
+		 "%c%c%c%c (0x%08x)",
 		 printable_char(format & 0xff),
 		 printable_char((format >> 8) & 0xff),
 		 printable_char((format >> 16) & 0xff),
 		 printable_char((format >> 24) & 0x7f),
-		 format & DRM_FORMAT_BIG_ENDIAN ? "big" : "little",
 		 format);
 
 	return buf->str;
diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index fc8ef42203..efe8b5ece5 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -152,7 +152,7 @@ static int framebuffer_check(struct drm_device *dev,
 	int i;
 
 	/* check if the format is supported at all */
-	info = __drm_format_info(r->pixel_format & ~DRM_FORMAT_BIG_ENDIAN);
+	info = __drm_format_info(r->pixel_format);
 	if (!info) {
 		struct drm_format_name_buf format_name;
 		DRM_DEBUG_KMS("bad framebuffer format %s\n",
-- 
2.9.3

[PATCH 1/6] drm: fourcc byteorder: drop DRM_FORMAT_BIG_ENDIAN

[Gerd Hoffmann]

It's unused.

Suggested-by: Daniel Vetter <daniel.vetter@intel.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/uapi/drm/drm_fourcc.h     | 2 --
 drivers/gpu/drm/drm_fourcc.c      | 3 +--
 drivers/gpu/drm/drm_framebuffer.c | 2 +-
 3 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
index 995c8f9c69..305bc34be0 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -33,8 +33,6 @@ extern "C" {
 #define fourcc_code(a, b, c, d) ((__u32)(a) | ((__u32)(b) << 8) | \
 				 ((__u32)(c) << 16) | ((__u32)(d) << 24))
 
-#define DRM_FORMAT_BIG_ENDIAN (1<<31) /* format is big endian instead of little endian */
-
 /* color index */
 #define DRM_FORMAT_C8		fourcc_code('C', '8', ' ', ' ') /* [7:0] C */
 
diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
index 9c0152df45..adb3ff59a4 100644
--- a/drivers/gpu/drm/drm_fourcc.c
+++ b/drivers/gpu/drm/drm_fourcc.c
@@ -86,12 +86,11 @@ EXPORT_SYMBOL(drm_mode_legacy_fb_format);
 const char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf)
 {
 	snprintf(buf->str, sizeof(buf->str),
-		 "%c%c%c%c %s-endian (0x%08x)",
+		 "%c%c%c%c (0x%08x)",
 		 printable_char(format & 0xff),
 		 printable_char((format >> 8) & 0xff),
 		 printable_char((format >> 16) & 0xff),
 		 printable_char((format >> 24) & 0x7f),
-		 format & DRM_FORMAT_BIG_ENDIAN ? "big" : "little",
 		 format);
 
 	return buf->str;
diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index fc8ef42203..efe8b5ece5 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -152,7 +152,7 @@ static int framebuffer_check(struct drm_device *dev,
 	int i;
 
 	/* check if the format is supported at all */
-	info = __drm_format_info(r->pixel_format & ~DRM_FORMAT_BIG_ENDIAN);
+	info = __drm_format_info(r->pixel_format);
 	if (!info) {
 		struct drm_format_name_buf format_name;
 		DRM_DEBUG_KMS("bad framebuffer format %s\n",
-- 
2.9.3

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/6] drm: fourcc byteorder: add DRM_FORMAT_CPU_*

[Gerd Hoffmann]

Add fourcc variants in cpu byte order.  With these at hand we don't
need #ifdefs in drivers want support framebuffers in cpu endianess.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/drm/drm_fourcc.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
index 6942e84b6e..cae05153e8 100644
--- a/include/drm/drm_fourcc.h
+++ b/include/drm/drm_fourcc.h
@@ -25,6 +25,18 @@
 #include <linux/types.h>
 #include <uapi/drm/drm_fourcc.h>
 
+/*
+ * DRM formats are little endian.  define cpu endian variants here, to
+ * reduce the #ifdefs needed in drivers.
+ */
+#ifdef __BIG_ENDIAN
+# define DRM_FORMAT_CPU_XRGB8888 DRM_FORMAT_BGRX8888
+# define DRM_FORMAT_CPU_ARGB8888 DRM_FORMAT_BGRA8888
+#else
+# define DRM_FORMAT_CPU_XRGB8888 DRM_FORMAT_XRGB8888
+# define DRM_FORMAT_CPU_ARGB8888 DRM_FORMAT_ARGB8888
+#endif
+
 struct drm_device;
 struct drm_mode_fb_cmd2;
 
-- 
2.9.3

[PATCH 2/6] drm: fourcc byteorder: add DRM_FORMAT_CPU_*

[Gerd Hoffmann]

Add fourcc variants in cpu byte order.  With these at hand we don't
need #ifdefs in drivers want support framebuffers in cpu endianess.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/drm/drm_fourcc.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
index 6942e84b6e..cae05153e8 100644
--- a/include/drm/drm_fourcc.h
+++ b/include/drm/drm_fourcc.h
@@ -25,6 +25,18 @@
 #include <linux/types.h>
 #include <uapi/drm/drm_fourcc.h>
 
+/*
+ * DRM formats are little endian.  define cpu endian variants here, to
+ * reduce the #ifdefs needed in drivers.
+ */
+#ifdef __BIG_ENDIAN
+# define DRM_FORMAT_CPU_XRGB8888 DRM_FORMAT_BGRX8888
+# define DRM_FORMAT_CPU_ARGB8888 DRM_FORMAT_BGRA8888
+#else
+# define DRM_FORMAT_CPU_XRGB8888 DRM_FORMAT_XRGB8888
+# define DRM_FORMAT_CPU_ARGB8888 DRM_FORMAT_ARGB8888
+#endif
+
 struct drm_device;
 struct drm_mode_fb_cmd2;
 
-- 
2.9.3

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 3/6] drm: fourcc byteorder: add bigendian support to drm_mode_legacy_fb_format

[Gerd Hoffmann]

Return correct fourcc codes on bigendian.  Drivers must be adapted to
this change.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/drm_fourcc.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
index adb3ff59a4..28401d3745 100644
--- a/drivers/gpu/drm/drm_fourcc.c
+++ b/drivers/gpu/drm/drm_fourcc.c
@@ -42,11 +42,34 @@ static char printable_char(int c)
  *
  * Computes a drm fourcc pixel format code for the given @bpp/@depth values.
  * Useful in fbdev emulation code, since that deals in those values.
+ *
+ * DRM_FORMAT_* are little endian, we'll pick cpu endian here, therefore we
+ * results differ depending on byte order.
  */
 uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth)
 {
 	uint32_t fmt;
 
+#ifdef __BIG_ENDIAN
+	switch (bpp) {
+	case 8:
+		fmt = DRM_FORMAT_C8;
+		break;
+	case 24:
+		fmt = DRM_FORMAT_BGR888;
+		break;
+	case 32:
+		if (depth == 24)
+			fmt = DRM_FORMAT_BGRX8888;
+		else
+			fmt = DRM_FORMAT_BGRA8888;
+		break;
+	default:
+		DRM_ERROR("bad bpp, assuming b8g8r8x8 pixel format\n");
+		fmt = DRM_FORMAT_BGRX8888;
+		break;
+	}
+#else
 	switch (bpp) {
 	case 8:
 		fmt = DRM_FORMAT_C8;
@@ -73,6 +96,7 @@ uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth)
 		fmt = DRM_FORMAT_XRGB8888;
 		break;
 	}
+#endif
 
 	return fmt;
 }
-- 
2.9.3

[PATCH 3/6] drm: fourcc byteorder: add bigendian support to drm_mode_legacy_fb_format

[Gerd Hoffmann]

Return correct fourcc codes on bigendian.  Drivers must be adapted to
this change.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/drm_fourcc.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
index adb3ff59a4..28401d3745 100644
--- a/drivers/gpu/drm/drm_fourcc.c
+++ b/drivers/gpu/drm/drm_fourcc.c
@@ -42,11 +42,34 @@ static char printable_char(int c)
  *
  * Computes a drm fourcc pixel format code for the given @bpp/@depth values.
  * Useful in fbdev emulation code, since that deals in those values.
+ *
+ * DRM_FORMAT_* are little endian, we'll pick cpu endian here, therefore we
+ * results differ depending on byte order.
  */
 uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth)
 {
 	uint32_t fmt;
 
+#ifdef __BIG_ENDIAN
+	switch (bpp) {
+	case 8:
+		fmt = DRM_FORMAT_C8;
+		break;
+	case 24:
+		fmt = DRM_FORMAT_BGR888;
+		break;
+	case 32:
+		if (depth == 24)
+			fmt = DRM_FORMAT_BGRX8888;
+		else
+			fmt = DRM_FORMAT_BGRA8888;
+		break;
+	default:
+		DRM_ERROR("bad bpp, assuming b8g8r8x8 pixel format\n");
+		fmt = DRM_FORMAT_BGRX8888;
+		break;
+	}
+#else
 	switch (bpp) {
 	case 8:
 		fmt = DRM_FORMAT_C8;
@@ -73,6 +96,7 @@ uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth)
 		fmt = DRM_FORMAT_XRGB8888;
 		break;
 	}
+#endif
 
 	return fmt;
 }
-- 
2.9.3

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

[PATCH 4/6] drm: fourcc byteorder: adapt bochs-drm to drm_mode_legacy_fb_format update

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/bochs/bochs_mm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bochs/bochs_mm.c b/drivers/gpu/drm/bochs/bochs_mm.c
index 857755ac2d..781d35bdff 100644
--- a/drivers/gpu/drm/bochs/bochs_mm.c
+++ b/drivers/gpu/drm/bochs/bochs_mm.c
@@ -508,7 +508,7 @@ bochs_user_framebuffer_create(struct drm_device *dev,
 	       (mode_cmd->pixel_format >> 16) & 0xff,
 	       (mode_cmd->pixel_format >> 24) & 0xff);
 
-	if (mode_cmd->pixel_format != DRM_FORMAT_XRGB8888)
+	if (mode_cmd->pixel_format != DRM_FORMAT_CPU_XRGB8888)
 		return ERR_PTR(-ENOENT);
 
 	obj = drm_gem_object_lookup(filp, mode_cmd->handles[0]);
-- 
2.9.3

[PATCH 4/6] drm: fourcc byteorder: adapt bochs-drm to drm_mode_legacy_fb_format update

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/bochs/bochs_mm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bochs/bochs_mm.c b/drivers/gpu/drm/bochs/bochs_mm.c
index 857755ac2d..781d35bdff 100644
--- a/drivers/gpu/drm/bochs/bochs_mm.c
+++ b/drivers/gpu/drm/bochs/bochs_mm.c
@@ -508,7 +508,7 @@ bochs_user_framebuffer_create(struct drm_device *dev,
 	       (mode_cmd->pixel_format >> 16) & 0xff,
 	       (mode_cmd->pixel_format >> 24) & 0xff);
 
-	if (mode_cmd->pixel_format != DRM_FORMAT_XRGB8888)
+	if (mode_cmd->pixel_format != DRM_FORMAT_CPU_XRGB8888)
 		return ERR_PTR(-ENOENT);
 
 	obj = drm_gem_object_lookup(filp, mode_cmd->handles[0]);
-- 
2.9.3

[PATCH 4/6] drm: fourcc byteorder: adapt bochs-drm to drm_mode_legacy_fb_format update

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/bochs/bochs_mm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bochs/bochs_mm.c b/drivers/gpu/drm/bochs/bochs_mm.c
index 857755ac2d..781d35bdff 100644
--- a/drivers/gpu/drm/bochs/bochs_mm.c
+++ b/drivers/gpu/drm/bochs/bochs_mm.c
@@ -508,7 +508,7 @@ bochs_user_framebuffer_create(struct drm_device *dev,
 	       (mode_cmd->pixel_format >> 16) & 0xff,
 	       (mode_cmd->pixel_format >> 24) & 0xff);
 
-	if (mode_cmd->pixel_format != DRM_FORMAT_XRGB8888)
+	if (mode_cmd->pixel_format != DRM_FORMAT_CPU_XRGB8888)
 		return ERR_PTR(-ENOENT);
 
 	obj = drm_gem_object_lookup(filp, mode_cmd->handles[0]);
-- 
2.9.3

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 5/6] drm: fourcc byteorder: adapt virtio to drm_mode_legacy_fb_format update

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/virtio/virtgpu_gem.c   |  2 +-
 drivers/gpu/drm/virtio/virtgpu_plane.c | 31 -------------------------------
 2 files changed, 1 insertion(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_gem.c b/drivers/gpu/drm/virtio/virtgpu_gem.c
index cc025d8fbe..4f2c2dc731 100644
--- a/drivers/gpu/drm/virtio/virtgpu_gem.c
+++ b/drivers/gpu/drm/virtio/virtgpu_gem.c
@@ -99,7 +99,7 @@ int virtio_gpu_mode_dumb_create(struct drm_file *file_priv,
 	if (ret)
 		goto fail;
 
-	format = virtio_gpu_translate_format(DRM_FORMAT_XRGB8888);
+	format = virtio_gpu_translate_format(DRM_FORMAT_CPU_XRGB8888);
 	virtio_gpu_resource_id_get(vgdev, &resid);
 	virtio_gpu_cmd_create_resource(vgdev, resid, format,
 				       args->width, args->height);
diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c
index adcdbd0abe..f40ffc9a70 100644
--- a/drivers/gpu/drm/virtio/virtgpu_plane.c
+++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
@@ -39,11 +39,7 @@ static const uint32_t virtio_gpu_formats[] = {
 };
 
 static const uint32_t virtio_gpu_cursor_formats[] = {
-#ifdef __BIG_ENDIAN
-	DRM_FORMAT_BGRA8888,
-#else
 	DRM_FORMAT_ARGB8888,
-#endif
 };
 
 uint32_t virtio_gpu_translate_format(uint32_t drm_fourcc)
@@ -51,32 +47,6 @@ uint32_t virtio_gpu_translate_format(uint32_t drm_fourcc)
 	uint32_t format;
 
 	switch (drm_fourcc) {
-#ifdef __BIG_ENDIAN
-	case DRM_FORMAT_XRGB8888:
-		format = VIRTIO_GPU_FORMAT_X8R8G8B8_UNORM;
-		break;
-	case DRM_FORMAT_ARGB8888:
-		format = VIRTIO_GPU_FORMAT_A8R8G8B8_UNORM;
-		break;
-	case DRM_FORMAT_BGRX8888:
-		format = VIRTIO_GPU_FORMAT_B8G8R8X8_UNORM;
-		break;
-	case DRM_FORMAT_BGRA8888:
-		format = VIRTIO_GPU_FORMAT_B8G8R8A8_UNORM;
-		break;
-	case DRM_FORMAT_RGBX8888:
-		format = VIRTIO_GPU_FORMAT_R8G8B8X8_UNORM;
-		break;
-	case DRM_FORMAT_RGBA8888:
-		format = VIRTIO_GPU_FORMAT_R8G8B8A8_UNORM;
-		break;
-	case DRM_FORMAT_XBGR8888:
-		format = VIRTIO_GPU_FORMAT_X8B8G8R8_UNORM;
-		break;
-	case DRM_FORMAT_ABGR8888:
-		format = VIRTIO_GPU_FORMAT_A8B8G8R8_UNORM;
-		break;
-#else
 	case DRM_FORMAT_XRGB8888:
 		format = VIRTIO_GPU_FORMAT_B8G8R8X8_UNORM;
 		break;
@@ -101,7 +71,6 @@ uint32_t virtio_gpu_translate_format(uint32_t drm_fourcc)
 	case DRM_FORMAT_ABGR8888:
 		format = VIRTIO_GPU_FORMAT_R8G8B8A8_UNORM;
 		break;
-#endif
 	default:
 		/*
 		 * This should not happen, we handle everything listed
-- 
2.9.3

[PATCH 5/6] drm: fourcc byteorder: adapt virtio to drm_mode_legacy_fb_format update

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/virtio/virtgpu_gem.c   |  2 +-
 drivers/gpu/drm/virtio/virtgpu_plane.c | 31 -------------------------------
 2 files changed, 1 insertion(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_gem.c b/drivers/gpu/drm/virtio/virtgpu_gem.c
index cc025d8fbe..4f2c2dc731 100644
--- a/drivers/gpu/drm/virtio/virtgpu_gem.c
+++ b/drivers/gpu/drm/virtio/virtgpu_gem.c
@@ -99,7 +99,7 @@ int virtio_gpu_mode_dumb_create(struct drm_file *file_priv,
 	if (ret)
 		goto fail;
 
-	format = virtio_gpu_translate_format(DRM_FORMAT_XRGB8888);
+	format = virtio_gpu_translate_format(DRM_FORMAT_CPU_XRGB8888);
 	virtio_gpu_resource_id_get(vgdev, &resid);
 	virtio_gpu_cmd_create_resource(vgdev, resid, format,
 				       args->width, args->height);
diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c
index adcdbd0abe..f40ffc9a70 100644
--- a/drivers/gpu/drm/virtio/virtgpu_plane.c
+++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
@@ -39,11 +39,7 @@ static const uint32_t virtio_gpu_formats[] = {
 };
 
 static const uint32_t virtio_gpu_cursor_formats[] = {
-#ifdef __BIG_ENDIAN
-	DRM_FORMAT_BGRA8888,
-#else
 	DRM_FORMAT_ARGB8888,
-#endif
 };
 
 uint32_t virtio_gpu_translate_format(uint32_t drm_fourcc)
@@ -51,32 +47,6 @@ uint32_t virtio_gpu_translate_format(uint32_t drm_fourcc)
 	uint32_t format;
 
 	switch (drm_fourcc) {
-#ifdef __BIG_ENDIAN
-	case DRM_FORMAT_XRGB8888:
-		format = VIRTIO_GPU_FORMAT_X8R8G8B8_UNORM;
-		break;
-	case DRM_FORMAT_ARGB8888:
-		format = VIRTIO_GPU_FORMAT_A8R8G8B8_UNORM;
-		break;
-	case DRM_FORMAT_BGRX8888:
-		format = VIRTIO_GPU_FORMAT_B8G8R8X8_UNORM;
-		break;
-	case DRM_FORMAT_BGRA8888:
-		format = VIRTIO_GPU_FORMAT_B8G8R8A8_UNORM;
-		break;
-	case DRM_FORMAT_RGBX8888:
-		format = VIRTIO_GPU_FORMAT_R8G8B8X8_UNORM;
-		break;
-	case DRM_FORMAT_RGBA8888:
-		format = VIRTIO_GPU_FORMAT_R8G8B8A8_UNORM;
-		break;
-	case DRM_FORMAT_XBGR8888:
-		format = VIRTIO_GPU_FORMAT_X8B8G8R8_UNORM;
-		break;
-	case DRM_FORMAT_ABGR8888:
-		format = VIRTIO_GPU_FORMAT_A8B8G8R8_UNORM;
-		break;
-#else
 	case DRM_FORMAT_XRGB8888:
 		format = VIRTIO_GPU_FORMAT_B8G8R8X8_UNORM;
 		break;
@@ -101,7 +71,6 @@ uint32_t virtio_gpu_translate_format(uint32_t drm_fourcc)
 	case DRM_FORMAT_ABGR8888:
 		format = VIRTIO_GPU_FORMAT_R8G8B8A8_UNORM;
 		break;
-#endif
 	default:
 		/*
 		 * This should not happen, we handle everything listed
-- 
2.9.3

[PATCH 5/6] drm: fourcc byteorder: adapt virtio to drm_mode_legacy_fb_format update

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/virtio/virtgpu_gem.c   |  2 +-
 drivers/gpu/drm/virtio/virtgpu_plane.c | 31 -------------------------------
 2 files changed, 1 insertion(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_gem.c b/drivers/gpu/drm/virtio/virtgpu_gem.c
index cc025d8fbe..4f2c2dc731 100644
--- a/drivers/gpu/drm/virtio/virtgpu_gem.c
+++ b/drivers/gpu/drm/virtio/virtgpu_gem.c
@@ -99,7 +99,7 @@ int virtio_gpu_mode_dumb_create(struct drm_file *file_priv,
 	if (ret)
 		goto fail;
 
-	format = virtio_gpu_translate_format(DRM_FORMAT_XRGB8888);
+	format = virtio_gpu_translate_format(DRM_FORMAT_CPU_XRGB8888);
 	virtio_gpu_resource_id_get(vgdev, &resid);
 	virtio_gpu_cmd_create_resource(vgdev, resid, format,
 				       args->width, args->height);
diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c
index adcdbd0abe..f40ffc9a70 100644
--- a/drivers/gpu/drm/virtio/virtgpu_plane.c
+++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
@@ -39,11 +39,7 @@ static const uint32_t virtio_gpu_formats[] = {
 };
 
 static const uint32_t virtio_gpu_cursor_formats[] = {
-#ifdef __BIG_ENDIAN
-	DRM_FORMAT_BGRA8888,
-#else
 	DRM_FORMAT_ARGB8888,
-#endif
 };
 
 uint32_t virtio_gpu_translate_format(uint32_t drm_fourcc)
@@ -51,32 +47,6 @@ uint32_t virtio_gpu_translate_format(uint32_t drm_fourcc)
 	uint32_t format;
 
 	switch (drm_fourcc) {
-#ifdef __BIG_ENDIAN
-	case DRM_FORMAT_XRGB8888:
-		format = VIRTIO_GPU_FORMAT_X8R8G8B8_UNORM;
-		break;
-	case DRM_FORMAT_ARGB8888:
-		format = VIRTIO_GPU_FORMAT_A8R8G8B8_UNORM;
-		break;
-	case DRM_FORMAT_BGRX8888:
-		format = VIRTIO_GPU_FORMAT_B8G8R8X8_UNORM;
-		break;
-	case DRM_FORMAT_BGRA8888:
-		format = VIRTIO_GPU_FORMAT_B8G8R8A8_UNORM;
-		break;
-	case DRM_FORMAT_RGBX8888:
-		format = VIRTIO_GPU_FORMAT_R8G8B8X8_UNORM;
-		break;
-	case DRM_FORMAT_RGBA8888:
-		format = VIRTIO_GPU_FORMAT_R8G8B8A8_UNORM;
-		break;
-	case DRM_FORMAT_XBGR8888:
-		format = VIRTIO_GPU_FORMAT_X8B8G8R8_UNORM;
-		break;
-	case DRM_FORMAT_ABGR8888:
-		format = VIRTIO_GPU_FORMAT_A8B8G8R8_UNORM;
-		break;
-#else
 	case DRM_FORMAT_XRGB8888:
 		format = VIRTIO_GPU_FORMAT_B8G8R8X8_UNORM;
 		break;
@@ -101,7 +71,6 @@ uint32_t virtio_gpu_translate_format(uint32_t drm_fourcc)
 	case DRM_FORMAT_ABGR8888:
 		format = VIRTIO_GPU_FORMAT_R8G8B8A8_UNORM;
 		break;
-#endif
 	default:
 		/*
 		 * This should not happen, we handle everything listed
-- 
2.9.3

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 6/6] drm: fourcc byteorder: virtio restrict to XRGB8888

[Gerd Hoffmann]

While wading through the code I've noticed we have a little issue in
virtio:  We attach a format to the bo when it is created
(DRM_IOCTL_MODE_CREATE_DUMB), not when we map it as framebuffer
(DRM_IOCTL_MODE_ADDFB).

Easy way out:  support a single format only.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/virtio/virtgpu_gem.c   | 5 ++++-
 drivers/gpu/drm/virtio/virtgpu_plane.c | 9 +--------
 2 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_gem.c b/drivers/gpu/drm/virtio/virtgpu_gem.c
index 4f2c2dc731..b09e5e5ae4 100644
--- a/drivers/gpu/drm/virtio/virtgpu_gem.c
+++ b/drivers/gpu/drm/virtio/virtgpu_gem.c
@@ -90,7 +90,10 @@ int virtio_gpu_mode_dumb_create(struct drm_file *file_priv,
 	uint32_t resid;
 	uint32_t format;
 
-	pitch = args->width * ((args->bpp + 1) / 8);
+	if (args->bpp != 32)
+		return -EINVAL;
+
+	pitch = args->width * 4;
 	args->size = pitch * args->height;
 	args->size = ALIGN(args->size, PAGE_SIZE);
 
diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c
index f40ffc9a70..3a4498a223 100644
--- a/drivers/gpu/drm/virtio/virtgpu_plane.c
+++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
@@ -28,14 +28,7 @@
 #include <drm/drm_atomic_helper.h>
 
 static const uint32_t virtio_gpu_formats[] = {
-	DRM_FORMAT_XRGB8888,
-	DRM_FORMAT_ARGB8888,
-	DRM_FORMAT_BGRX8888,
-	DRM_FORMAT_BGRA8888,
-	DRM_FORMAT_RGBX8888,
-	DRM_FORMAT_RGBA8888,
-	DRM_FORMAT_XBGR8888,
-	DRM_FORMAT_ABGR8888,
+	DRM_FORMAT_CPU_XRGB8888,
 };
 
 static const uint32_t virtio_gpu_cursor_formats[] = {
-- 
2.9.3

[PATCH 6/6] drm: fourcc byteorder: virtio restrict to XRGB8888

[Gerd Hoffmann]

While wading through the code I've noticed we have a little issue in
virtio:  We attach a format to the bo when it is created
(DRM_IOCTL_MODE_CREATE_DUMB), not when we map it as framebuffer
(DRM_IOCTL_MODE_ADDFB).

Easy way out:  support a single format only.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/virtio/virtgpu_gem.c   | 5 ++++-
 drivers/gpu/drm/virtio/virtgpu_plane.c | 9 +--------
 2 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_gem.c b/drivers/gpu/drm/virtio/virtgpu_gem.c
index 4f2c2dc731..b09e5e5ae4 100644
--- a/drivers/gpu/drm/virtio/virtgpu_gem.c
+++ b/drivers/gpu/drm/virtio/virtgpu_gem.c
@@ -90,7 +90,10 @@ int virtio_gpu_mode_dumb_create(struct drm_file *file_priv,
 	uint32_t resid;
 	uint32_t format;
 
-	pitch = args->width * ((args->bpp + 1) / 8);
+	if (args->bpp != 32)
+		return -EINVAL;
+
+	pitch = args->width * 4;
 	args->size = pitch * args->height;
 	args->size = ALIGN(args->size, PAGE_SIZE);
 
diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c
index f40ffc9a70..3a4498a223 100644
--- a/drivers/gpu/drm/virtio/virtgpu_plane.c
+++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
@@ -28,14 +28,7 @@
 #include <drm/drm_atomic_helper.h>
 
 static const uint32_t virtio_gpu_formats[] = {
-	DRM_FORMAT_XRGB8888,
-	DRM_FORMAT_ARGB8888,
-	DRM_FORMAT_BGRX8888,
-	DRM_FORMAT_BGRA8888,
-	DRM_FORMAT_RGBX8888,
-	DRM_FORMAT_RGBA8888,
-	DRM_FORMAT_XBGR8888,
-	DRM_FORMAT_ABGR8888,
+	DRM_FORMAT_CPU_XRGB8888,
 };
 
 static const uint32_t virtio_gpu_cursor_formats[] = {
-- 
2.9.3

Re: [PATCH 0/6] drm: tackle byteorder issues, take two

[Michel Dänzer]

On 24/04/17 03:25 PM, Gerd Hoffmann wrote:
>   Hi,
> 
> Ok, different approach up for discussion.  Given that userspace didn't
> made the transition from ADDFB to ADDFB2 yet it seems we still can muck
> with the fourcc codes without breaking everything, as long as we
> maintain ADDFB and fbdev behavior (use cpu byte order format) so nothing
> changes for userspace.
> 
> So, this series basically makes drm_mode_legacy_fb_format return correct
> formats in bigendian mode and adapts the bochs and virtio drivers to
> this change.  Other drivers must be adapted to this change too.
> 
> Ilia Mirkin figured the dispnv04 backend in nouveau turns on/off
> byteswapping depending on cpu byte order.  So, one way to adapt the
> driver would be to simply use the new #defines added by patch #2.  The
> other way would be to support both XRGB and BGRX and turn on/off
> byteswapping depending on framebuffer format instead of cpu byte order.
> 
> cheers,
>   Gerd
> 
> Gerd Hoffmann (6):
>   drm: fourcc byteorder: drop DRM_FORMAT_BIG_ENDIAN

I don't see how it can be dropped. It's only optional for formats where
all components have 8 bits.


>   drm: fourcc byteorder: add DRM_FORMAT_CPU_*
>   drm: fourcc byteorder: add bigendian support to
>     drm_mode_legacy_fb_format

As I explained in my last followup in the "[PATCH] drm: fourcc
byteorder: brings header file comments in line with reality." thread,
the mapping between GPU and CPU formats has to be provided by the
driver, it cannot be done statically.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 0/6] drm: tackle byteorder issues, take two

[Gerd Hoffmann]

  Hi,

> > Gerd Hoffmann (6):
> >   drm: fourcc byteorder: drop DRM_FORMAT_BIG_ENDIAN
> 
> I don't see how it can be dropped. It's only optional for formats where
> all components have 8 bits.

Well, we can, because it is simply not used anywhere ...

We indeed can't specify RGB565 or XRGB2101010 in bigendian byte order
without this.  Apparently nobody needed that so far.  Should the need to
support this arise I'd rather define new fourcc codes, because I think
we should have exactly one fourcc per format.

With the bigendian flag all 8bit component formats have two:  XRGB8888
in bigendian can be "XRGB8888 | BIGENDIAN" and "BGRX8888".

> >   drm: fourcc byteorder: add DRM_FORMAT_CPU_*
> >   drm: fourcc byteorder: add bigendian support to
> >     drm_mode_legacy_fb_format
> 
> As I explained in my last followup in the "[PATCH] drm: fourcc
> byteorder: brings header file comments in line with reality." thread,
> the mapping between GPU and CPU formats has to be provided by the
> driver, it cannot be done statically.

Well, the drm fourcc codes represent the cpu view (i.e. what userspace
will fill the ADDFB2-created framebuffers with).  The gpu view can
certainly differ from that.  Implementing this is up to the driver IMO.

When running on dumb framebuffers userspace doesn't need to know what
the gpu view is.

When running in opengl mode there will be a hardware-specific mesa
driver in userspace, which will either know what the gpu view is (for
example because there is only one way to implement this in hardware) or
it can use hardware-specific ioctls to ask the kernel driver what the
gpu view is.

So, I can't see the problem you are seeing here.  Did I miss something?

cheers,
  Gerd

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 0/6] drm: tackle byteorder issues, take two

[Michel Dänzer]

On 24/04/17 04:36 PM, Gerd Hoffmann wrote:
> 
>>>   drm: fourcc byteorder: add DRM_FORMAT_CPU_*
>>>   drm: fourcc byteorder: add bigendian support to
>>>     drm_mode_legacy_fb_format
>>
>> As I explained in my last followup in the "[PATCH] drm: fourcc
>> byteorder: brings header file comments in line with reality." thread,
>> the mapping between GPU and CPU formats has to be provided by the
>> driver, it cannot be done statically.
> 
> Well, the drm fourcc codes represent the cpu view (i.e. what userspace
> will fill the ADDFB2-created framebuffers with).

Ville is adamant that they represent the GPU view. This needs to be
resolved one way or the other.


> The gpu view can certainly differ from that.  Implementing this is up
> to the driver IMO.
> 
> When running on dumb framebuffers userspace doesn't need to know what
> the gpu view is.
> 
> When running in opengl mode there will be a hardware-specific mesa
> driver in userspace, which will either know what the gpu view is (for
> example because there is only one way to implement this in hardware) or
> it can use hardware-specific ioctls to ask the kernel driver what the
> gpu view is.

Not sure this can be hidden in the OpenGL driver. How would e.g. a
Wayland compositor or the Xorg modesetting driver know which OpenGL
format corresponds to a given DRM_FORMAT?


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/6] drm: tackle byteorder issues, take two

[Ville Syrjälä]

On Mon, Apr 24, 2017 at 04:54:25PM +0900, Michel Dänzer wrote:
> On 24/04/17 04:36 PM, Gerd Hoffmann wrote:
> > 
> >>>   drm: fourcc byteorder: add DRM_FORMAT_CPU_*
> >>>   drm: fourcc byteorder: add bigendian support to
> >>>     drm_mode_legacy_fb_format
> >>
> >> As I explained in my last followup in the "[PATCH] drm: fourcc
> >> byteorder: brings header file comments in line with reality." thread,
> >> the mapping between GPU and CPU formats has to be provided by the
> >> driver, it cannot be done statically.
> > 
> > Well, the drm fourcc codes represent the cpu view (i.e. what userspace
> > will fill the ADDFB2-created framebuffers with).
> 
> Ville is adamant that they represent the GPU view. This needs to be
> resolved one way or the other.

Since the byte swapping can happen either for CPU or display access
I guess we can't just consider the GPU and display as a single entity.

We may need to consider several agents:
1. display
2. GPU
3. CPU
4. other DMA

Not sure what we can say about 4. I presume it's going to be like the
GPU or the CPU in the sense that it might go through the CPU byte
swapping logic or not. I'm just going to ignore it.

Let's say we have the following bytes in memory
(in order of increasing address): A,B,C,D
We'll assume GPU and display are LE natively. Each component will see
the resulting 32bpp 8888 pixel as follows (msb left->lsb right):

LE CPU w/ no byte swapping:
 display: DCBA
 GPU: DCBA
 CPU: DCBA
 = everyone agrees

BE CPU w/ no byte swapping:
 display: DCBA
 GPU: DCBA
 CPU: ABCD
 = GPU and display agree

BE CPU w/ display byte swapping:
 display: ABCD
 GPU: DCBA
 CPU: ABCD
 = CPU and display agree

BE CPU w/ CPU access byte swapping:
 display: DCBA
 GPU: DCBA
 CPU: DCBA
 = everyone agrees

BE CPU w/ both display and CPU byte swapping:
 display: ABCD
 GPU: DCBA
 CPU: DCBA
 = CPU and GPU agree (doesn't seem all that useful)

The different byte swapping tricks must have seemed like a great idea to
someone, but in the end they're just making our life more miserable.

> > The gpu view can certainly differ from that.  Implementing this is up
> > to the driver IMO.
> > 
> > When running on dumb framebuffers userspace doesn't need to know what
> > the gpu view is.

True. So for that we'd just need to consider whether the CPU and display
agree or disagree on the byte order. And I guess we'd have to pick from
the following choices for a BE CPU:

CPU and display agree:
 * FB is big endian, or FB is host endian (or whatever we would call it)
CPU and display disagree:
 * FB is little endian, or FB is foreign endian (or whatever)

> > 
> > When running in opengl mode there will be a hardware-specific mesa
> > driver in userspace, which will either know what the gpu view is (for
> > example because there is only one way to implement this in hardware) or
> > it can use hardware-specific ioctls to ask the kernel driver what the
> > gpu view is.
> 
> Not sure this can be hidden in the OpenGL driver. How would e.g. a
> Wayland compositor or the Xorg modesetting driver know which OpenGL
> format corresponds to a given DRM_FORMAT?

How are GL formats defined? /me needs to go read the spec again.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 0/6] drm: tackle byteorder issues, take two

[Daniel Stone]

Hi,

On 24 April 2017 at 15:26, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Mon, Apr 24, 2017 at 04:54:25PM +0900, Michel Dänzer wrote:
>> On 24/04/17 04:36 PM, Gerd Hoffmann wrote:
>> > When running in opengl mode there will be a hardware-specific mesa
>> > driver in userspace, which will either know what the gpu view is (for
>> > example because there is only one way to implement this in hardware) or
>> > it can use hardware-specific ioctls to ask the kernel driver what the
>> > gpu view is.
>>
>> Not sure this can be hidden in the OpenGL driver. How would e.g. a
>> Wayland compositor or the Xorg modesetting driver know which OpenGL
>> format corresponds to a given DRM_FORMAT?
>
> How are GL formats defined? /me needs to go read the spec again.

They aren't, per se. Only relative to 'native formats', which for this
discussion is the set of GBM formats, which is in turn just
drm_fourcc.h.

Cheers,
Daniel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/6] drm: tackle byteorder issues, take two

[Ville Syrjälä]

On Mon, Apr 24, 2017 at 05:26:03PM +0300, Ville Syrjälä wrote:
> On Mon, Apr 24, 2017 at 04:54:25PM +0900, Michel Dänzer wrote:
> > On 24/04/17 04:36 PM, Gerd Hoffmann wrote:
> > > 
> > >>>   drm: fourcc byteorder: add DRM_FORMAT_CPU_*
> > >>>   drm: fourcc byteorder: add bigendian support to
> > >>>     drm_mode_legacy_fb_format
> > >>
> > >> As I explained in my last followup in the "[PATCH] drm: fourcc
> > >> byteorder: brings header file comments in line with reality." thread,
> > >> the mapping between GPU and CPU formats has to be provided by the
> > >> driver, it cannot be done statically.
> > > 
> > > Well, the drm fourcc codes represent the cpu view (i.e. what userspace
> > > will fill the ADDFB2-created framebuffers with).
> > 
> > Ville is adamant that they represent the GPU view. This needs to be
> > resolved one way or the other.
> 
> Since the byte swapping can happen either for CPU or display access
> I guess we can't just consider the GPU and display as a single entity.
> 
> We may need to consider several agents:
> 1. display
> 2. GPU
> 3. CPU
> 4. other DMA
> 
> Not sure what we can say about 4. I presume it's going to be like the
> GPU or the CPU in the sense that it might go through the CPU byte
> swapping logic or not. I'm just going to ignore it.
> 
> Let's say we have the following bytes in memory
> (in order of increasing address): A,B,C,D
> We'll assume GPU and display are LE natively. Each component will see
> the resulting 32bpp 8888 pixel as follows (msb left->lsb right):
> 
> LE CPU w/ no byte swapping:
>  display: DCBA
>  GPU: DCBA
>  CPU: DCBA
>  = everyone agrees
> 
> BE CPU w/ no byte swapping:
>  display: DCBA
>  GPU: DCBA
>  CPU: ABCD
>  = GPU and display agree
> 
> BE CPU w/ display byte swapping:
>  display: ABCD
>  GPU: DCBA
>  CPU: ABCD
>  = CPU and display agree

So after some further thought this seems like a somewhat crazy
combination. It does make sense from the simplicity POV in that 
the CPU byte swapping isn't needed, and thus the problems with
concurrent access to buffers with different pixel sizes vanish.

However the GPU has to somehow be able to produce data the display
can consume, so presumably there must be some knobs in the GPU to do
the opposite byte swapping that the display does, or the GPU must be
restricted to only use framebuffers in formats like 8888.

> 
> BE CPU w/ CPU access byte swapping:
>  display: DCBA
>  GPU: DCBA
>  CPU: DCBA
>  = everyone agrees
> 
> BE CPU w/ both display and CPU byte swapping:
>  display: ABCD
>  GPU: DCBA
>  CPU: DCBA
>  = CPU and GPU agree (doesn't seem all that useful)
> 
> The different byte swapping tricks must have seemed like a great idea to
> someone, but in the end they're just making our life more miserable.
> 
> > > The gpu view can certainly differ from that.  Implementing this is up
> > > to the driver IMO.
> > > 
> > > When running on dumb framebuffers userspace doesn't need to know what
> > > the gpu view is.
> 
> True. So for that we'd just need to consider whether the CPU and display
> agree or disagree on the byte order. And I guess we'd have to pick from
> the following choices for a BE CPU:
> 
> CPU and display agree:
>  * FB is big endian, or FB is host endian (or whatever we would call it)
> CPU and display disagree:
>  * FB is little endian, or FB is foreign endian (or whatever)
> 
> > > 
> > > When running in opengl mode there will be a hardware-specific mesa
> > > driver in userspace, which will either know what the gpu view is (for
> > > example because there is only one way to implement this in hardware) or
> > > it can use hardware-specific ioctls to ask the kernel driver what the
> > > gpu view is.
> > 
> > Not sure this can be hidden in the OpenGL driver. How would e.g. a
> > Wayland compositor or the Xorg modesetting driver know which OpenGL
> > format corresponds to a given DRM_FORMAT?
> 
> How are GL formats defined? /me needs to go read the spec again.
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 0/6] drm: tackle byteorder issues, take two

[Michel Dänzer]

On 24/04/17 11:26 PM, Ville Syrjälä wrote:
> On Mon, Apr 24, 2017 at 04:54:25PM +0900, Michel Dänzer wrote:
>> On 24/04/17 04:36 PM, Gerd Hoffmann wrote:
>>>
>>>>>   drm: fourcc byteorder: add DRM_FORMAT_CPU_*
>>>>>   drm: fourcc byteorder: add bigendian support to
>>>>>     drm_mode_legacy_fb_format
>>>>
>>>> As I explained in my last followup in the "[PATCH] drm: fourcc
>>>> byteorder: brings header file comments in line with reality." thread,
>>>> the mapping between GPU and CPU formats has to be provided by the
>>>> driver, it cannot be done statically.
>>>
>>> Well, the drm fourcc codes represent the cpu view (i.e. what userspace
>>> will fill the ADDFB2-created framebuffers with).
>>
>> Ville is adamant that they represent the GPU view. This needs to be
>> resolved one way or the other.
> 
> Since the byte swapping can happen either for CPU or display access
> I guess we can't just consider the GPU and display as a single entity.
> 
> We may need to consider several agents:
> 1. display
> 2. GPU
> 3. CPU
> 4. other DMA
> 
> Not sure what we can say about 4. I presume it's going to be like the
> GPU or the CPU in the sense that it might go through the CPU byte
> swapping logic or not. I'm just going to ignore it.
> 
> Let's say we have the following bytes in memory
> (in order of increasing address): A,B,C,D
> We'll assume GPU and display are LE natively. Each component will see
> the resulting 32bpp 8888 pixel as follows (msb left->lsb right):
> 
> LE CPU w/ no byte swapping:
>  display: DCBA
>  GPU: DCBA
>  CPU: DCBA
>  = everyone agrees
> 
> BE CPU w/ no byte swapping:
>  display: DCBA
>  GPU: DCBA
>  CPU: ABCD
>  = GPU and display agree
> 
> BE CPU w/ display byte swapping:
>  display: ABCD
>  GPU: DCBA
>  CPU: ABCD
>  = CPU and display agree
> 
> BE CPU w/ CPU access byte swapping:
>  display: DCBA
>  GPU: DCBA
>  CPU: DCBA
>  = everyone agrees

Beware that for this list, you're using a format definition which is
based on a packed 32-bit value. This does *not* match the current
DRM_FORMAT_*8888 definitions. E.g. in the last case, display and GPU use
the same DRM_FORMAT, but the CPU uses the "inverse" one.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 3/6] drm: fourcc byteorder: add bigendian support to drm_mode_legacy_fb_format

[Michel Dänzer]

On 24/04/17 03:25 PM, Gerd Hoffmann wrote:
> Return correct fourcc codes on bigendian.  Drivers must be adapted to
> this change.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Just to reiterate, this won't work for the radeon driver, which programs
the GPU to use (effectively, per the current definition that these are
little endian GPU formats) DRM_FORMAT_XRGB8888 with pre-R600 and
DRM_FORMAT_BGRX8888 with >= R600.


> +#ifdef __BIG_ENDIAN
> +	switch (bpp) {
> +	case 8:
> +		fmt = DRM_FORMAT_C8;
> +		break;
> +	case 24:
> +		fmt = DRM_FORMAT_BGR888;
> +		break;

BTW, endianness as a concept cannot apply to 8 or 24 bpp formats.


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

Re: [PATCH 3/6] drm: fourcc byteorder: add bigendian support to drm_mode_legacy_fb_format

[Michel Dänzer]

On 24/04/17 03:25 PM, Gerd Hoffmann wrote:
> Return correct fourcc codes on bigendian.  Drivers must be adapted to
> this change.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Just to reiterate, this won't work for the radeon driver, which programs
the GPU to use (effectively, per the current definition that these are
little endian GPU formats) DRM_FORMAT_XRGB8888 with pre-R600 and
DRM_FORMAT_BGRX8888 with >= R600.


> +#ifdef __BIG_ENDIAN
> +	switch (bpp) {
> +	case 8:
> +		fmt = DRM_FORMAT_C8;
> +		break;
> +	case 24:
> +		fmt = DRM_FORMAT_BGR888;
> +		break;

BTW, endianness as a concept cannot apply to 8 or 24 bpp formats.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 0/6] drm: tackle byteorder issues, take two

[Ville Syrjälä]

On Tue, Apr 25, 2017 at 09:49:38AM +0900, Michel Dänzer wrote:
> On 24/04/17 11:26 PM, Ville Syrjälä wrote:
> > On Mon, Apr 24, 2017 at 04:54:25PM +0900, Michel Dänzer wrote:
> >> On 24/04/17 04:36 PM, Gerd Hoffmann wrote:
> >>>
> >>>>>   drm: fourcc byteorder: add DRM_FORMAT_CPU_*
> >>>>>   drm: fourcc byteorder: add bigendian support to
> >>>>>     drm_mode_legacy_fb_format
> >>>>
> >>>> As I explained in my last followup in the "[PATCH] drm: fourcc
> >>>> byteorder: brings header file comments in line with reality." thread,
> >>>> the mapping between GPU and CPU formats has to be provided by the
> >>>> driver, it cannot be done statically.
> >>>
> >>> Well, the drm fourcc codes represent the cpu view (i.e. what userspace
> >>> will fill the ADDFB2-created framebuffers with).
> >>
> >> Ville is adamant that they represent the GPU view. This needs to be
> >> resolved one way or the other.
> > 
> > Since the byte swapping can happen either for CPU or display access
> > I guess we can't just consider the GPU and display as a single entity.
> > 
> > We may need to consider several agents:
> > 1. display
> > 2. GPU
> > 3. CPU
> > 4. other DMA
> > 
> > Not sure what we can say about 4. I presume it's going to be like the
> > GPU or the CPU in the sense that it might go through the CPU byte
> > swapping logic or not. I'm just going to ignore it.
> > 
> > Let's say we have the following bytes in memory
> > (in order of increasing address): A,B,C,D
> > We'll assume GPU and display are LE natively. Each component will see
> > the resulting 32bpp 8888 pixel as follows (msb left->lsb right):
> > 
> > LE CPU w/ no byte swapping:
> >  display: DCBA
> >  GPU: DCBA
> >  CPU: DCBA
> >  = everyone agrees
> > 
> > BE CPU w/ no byte swapping:
> >  display: DCBA
> >  GPU: DCBA
> >  CPU: ABCD
> >  = GPU and display agree
> > 
> > BE CPU w/ display byte swapping:
> >  display: ABCD
> >  GPU: DCBA
> >  CPU: ABCD
> >  = CPU and display agree
> > 
> > BE CPU w/ CPU access byte swapping:
> >  display: DCBA
> >  GPU: DCBA
> >  CPU: DCBA
> >  = everyone agrees
> 
> Beware that for this list, you're using a format definition

Actually I'm not using any format definitions here.

> which is
> based on a packed 32-bit value. This does *not* match the current
> DRM_FORMAT_*8888 definitions. E.g. in the last case, display and GPU use
> the same DRM_FORMAT, but the CPU uses the "inverse" one.

I wasn't concerned about the specific drm format, just what kind
of a 32bpp value everyone will see given the same memory contents.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 3/6] drm: fourcc byteorder: add bigendian support to drm_mode_legacy_fb_format

[Ville Syrjälä]

On Tue, Apr 25, 2017 at 12:18:52PM +0900, Michel Dänzer wrote:
> On 24/04/17 03:25 PM, Gerd Hoffmann wrote:
> > Return correct fourcc codes on bigendian.  Drivers must be adapted to
> > this change.
> > 
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> 
> Just to reiterate, this won't work for the radeon driver, which programs
> the GPU to use (effectively, per the current definition that these are
> little endian GPU formats) DRM_FORMAT_XRGB8888 with pre-R600 and
> DRM_FORMAT_BGRX8888 with >= R600.
> 
> 
> > +#ifdef __BIG_ENDIAN
> > +	switch (bpp) {
> > +	case 8:
> > +		fmt = DRM_FORMAT_C8;
> > +		break;
> > +	case 24:
> > +		fmt = DRM_FORMAT_BGR888;
> > +		break;
> 
> BTW, endianness as a concept cannot apply to 8 or 24 bpp formats.

To 8bpp no, but it can easily apply to 24bpp. Same was as it applies to
16bpp. Neither matches the word size of the CPU or anything like that
but still the bytes have to stored in memory in some order.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 3/6] drm: fourcc byteorder: add bigendian support to drm_mode_legacy_fb_format

[Michel Dänzer]

On 25/04/17 06:52 PM, Ville Syrjälä wrote:
> On Tue, Apr 25, 2017 at 12:18:52PM +0900, Michel Dänzer wrote:
>> On 24/04/17 03:25 PM, Gerd Hoffmann wrote:
>>> +#ifdef __BIG_ENDIAN
>>> +	switch (bpp) {
>>> +	case 8:
>>> +		fmt = DRM_FORMAT_C8;
>>> +		break;
>>> +	case 24:
>>> +		fmt = DRM_FORMAT_BGR888;
>>> +		break;
>>
>> BTW, endianness as a concept cannot apply to 8 or 24 bpp formats.
> 
> To 8bpp no, but it can easily apply to 24bpp.

Any byte swapping rips apart the bytes of a 24bpp pixel, so those
formats only make sense as straight array formats.


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

Re: [PATCH 3/6] drm: fourcc byteorder: add bigendian support to drm_mode_legacy_fb_format

[Michel Dänzer]

On 25/04/17 06:52 PM, Ville Syrjälä wrote:
> On Tue, Apr 25, 2017 at 12:18:52PM +0900, Michel Dänzer wrote:
>> On 24/04/17 03:25 PM, Gerd Hoffmann wrote:
>>> +#ifdef __BIG_ENDIAN
>>> +	switch (bpp) {
>>> +	case 8:
>>> +		fmt = DRM_FORMAT_C8;
>>> +		break;
>>> +	case 24:
>>> +		fmt = DRM_FORMAT_BGR888;
>>> +		break;
>>
>> BTW, endianness as a concept cannot apply to 8 or 24 bpp formats.
> 
> To 8bpp no, but it can easily apply to 24bpp.

Any byte swapping rips apart the bytes of a 24bpp pixel, so those
formats only make sense as straight array formats.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 3/6] drm: fourcc byteorder: add bigendian support to drm_mode_legacy_fb_format

[Gerd Hoffmann]

On Di, 2017-04-25 at 12:18 +0900, Michel Dänzer wrote:
> On 24/04/17 03:25 PM, Gerd Hoffmann wrote:
> > Return correct fourcc codes on bigendian.  Drivers must be adapted to
> > this change.
> > 
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> 
> Just to reiterate, this won't work for the radeon driver, which programs
> the GPU to use (effectively, per the current definition that these are
> little endian GPU formats) DRM_FORMAT_XRGB8888 with pre-R600 and
> DRM_FORMAT_BGRX8888 with >= R600.

Hmm, ok, how does bigendian fbdev emulation work on pre-R600 then?

> > +#ifdef __BIG_ENDIAN
> > +	switch (bpp) {
> > +	case 8:
> > +		fmt = DRM_FORMAT_C8;
> > +		break;
> > +	case 24:
> > +		fmt = DRM_FORMAT_BGR888;
> > +		break;
> 
> BTW, endianness as a concept cannot apply to 8 or 24 bpp formats.

I could move the 8 bpp case out of the #ifdef somehow, but code
readability will suffer then I think ...

For 24 we have different byte orderings, but yes, you can't switch from
one to the other with byteswapping.  Probably one of the reasons why
this format is pretty much out of fashion these days ...

cheers,
  Gerd

Re: [PATCH 3/6] drm: fourcc byteorder: add bigendian support to drm_mode_legacy_fb_format

[Gerd Hoffmann]

On Di, 2017-04-25 at 12:18 +0900, Michel Dänzer wrote:
> On 24/04/17 03:25 PM, Gerd Hoffmann wrote:
> > Return correct fourcc codes on bigendian.  Drivers must be adapted to
> > this change.
> > 
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> 
> Just to reiterate, this won't work for the radeon driver, which programs
> the GPU to use (effectively, per the current definition that these are
> little endian GPU formats) DRM_FORMAT_XRGB8888 with pre-R600 and
> DRM_FORMAT_BGRX8888 with >= R600.

Hmm, ok, how does bigendian fbdev emulation work on pre-R600 then?

> > +#ifdef __BIG_ENDIAN
> > +	switch (bpp) {
> > +	case 8:
> > +		fmt = DRM_FORMAT_C8;
> > +		break;
> > +	case 24:
> > +		fmt = DRM_FORMAT_BGR888;
> > +		break;
> 
> BTW, endianness as a concept cannot apply to 8 or 24 bpp formats.

I could move the 8 bpp case out of the #ifdef somehow, but code
readability will suffer then I think ...

For 24 we have different byte orderings, but yes, you can't switch from
one to the other with byteswapping.  Probably one of the reasons why
this format is pretty much out of fashion these days ...

cheers,
  Gerd

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 3/6] drm: fourcc byteorder: add bigendian support to drm_mode_legacy_fb_format

[Michel Dänzer]

On 26/04/17 02:53 PM, Gerd Hoffmann wrote:
> On Di, 2017-04-25 at 12:18 +0900, Michel Dänzer wrote:
>> On 24/04/17 03:25 PM, Gerd Hoffmann wrote:
>>> Return correct fourcc codes on bigendian.  Drivers must be adapted to
>>> this change.
>>>
>>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>>
>> Just to reiterate, this won't work for the radeon driver, which programs
>> the GPU to use (effectively, per the current definition that these are
>> little endian GPU formats) DRM_FORMAT_XRGB8888 with pre-R600 and
>> DRM_FORMAT_BGRX8888 with >= R600.
> 
> Hmm, ok, how does bigendian fbdev emulation work on pre-R600 then?

Using a GPU byte swapping mechanism which only affects CPU access to
video RAM.


>>> +#ifdef __BIG_ENDIAN
>>> +	switch (bpp) {
>>> +	case 8:
>>> +		fmt = DRM_FORMAT_C8;
>>> +		break;
>>> +	case 24:
>>> +		fmt = DRM_FORMAT_BGR888;
>>> +		break;
>>
>> BTW, endianness as a concept cannot apply to 8 or 24 bpp formats.
> 
> I could move the 8 bpp case out of the #ifdef somehow, but code
> readability will suffer then I think ...

How so?

At least it would make clearer which formats are affected by endianness
and which aren't.


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

Re: [PATCH 3/6] drm: fourcc byteorder: add bigendian support to drm_mode_legacy_fb_format

[Michel Dänzer]

On 26/04/17 02:53 PM, Gerd Hoffmann wrote:
> On Di, 2017-04-25 at 12:18 +0900, Michel Dänzer wrote:
>> On 24/04/17 03:25 PM, Gerd Hoffmann wrote:
>>> Return correct fourcc codes on bigendian.  Drivers must be adapted to
>>> this change.
>>>
>>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>>
>> Just to reiterate, this won't work for the radeon driver, which programs
>> the GPU to use (effectively, per the current definition that these are
>> little endian GPU formats) DRM_FORMAT_XRGB8888 with pre-R600 and
>> DRM_FORMAT_BGRX8888 with >= R600.
> 
> Hmm, ok, how does bigendian fbdev emulation work on pre-R600 then?

Using a GPU byte swapping mechanism which only affects CPU access to
video RAM.


>>> +#ifdef __BIG_ENDIAN
>>> +	switch (bpp) {
>>> +	case 8:
>>> +		fmt = DRM_FORMAT_C8;
>>> +		break;
>>> +	case 24:
>>> +		fmt = DRM_FORMAT_BGR888;
>>> +		break;
>>
>> BTW, endianness as a concept cannot apply to 8 or 24 bpp formats.
> 
> I could move the 8 bpp case out of the #ifdef somehow, but code
> readability will suffer then I think ...

How so?

At least it would make clearer which formats are affected by endianness
and which aren't.


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

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

Re: [PATCH 3/6] drm: fourcc byteorder: add bigendian support to drm_mode_legacy_fb_format

[Gerd Hoffmann]

  Hi,

> >> Just to reiterate, this won't work for the radeon driver, which programs
> >> the GPU to use (effectively, per the current definition that these are
> >> little endian GPU formats) DRM_FORMAT_XRGB8888 with pre-R600 and
> >> DRM_FORMAT_BGRX8888 with >= R600.
> > 
> > Hmm, ok, how does bigendian fbdev emulation work on pre-R600 then?
> 
> Using a GPU byte swapping mechanism which only affects CPU access to
> video RAM.

That is done using the RADEON_TILING_SWAP_{16,32}BIT flag mentioned in
another thread?

Ok, so the cpu view to fbdev is DRM_FORMAT_BGRX8888 in all cases.

What about dumb bos?  You've mentioned the swap flag isn't used for
those.  Which implies they are in little endian byte order (both gpu and
cpu view).  Does the modesetting driver work correctly on that hardware?

cheers,
  Gerd

Re: [PATCH 3/6] drm: fourcc byteorder: add bigendian support to drm_mode_legacy_fb_format

[Gerd Hoffmann]

  Hi,

> >> Just to reiterate, this won't work for the radeon driver, which programs
> >> the GPU to use (effectively, per the current definition that these are
> >> little endian GPU formats) DRM_FORMAT_XRGB8888 with pre-R600 and
> >> DRM_FORMAT_BGRX8888 with >= R600.
> > 
> > Hmm, ok, how does bigendian fbdev emulation work on pre-R600 then?
> 
> Using a GPU byte swapping mechanism which only affects CPU access to
> video RAM.

That is done using the RADEON_TILING_SWAP_{16,32}BIT flag mentioned in
another thread?

Ok, so the cpu view to fbdev is DRM_FORMAT_BGRX8888 in all cases.

What about dumb bos?  You've mentioned the swap flag isn't used for
those.  Which implies they are in little endian byte order (both gpu and
cpu view).  Does the modesetting driver work correctly on that hardware?

cheers,
  Gerd

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 3/6] drm: fourcc byteorder: add bigendian support to drm_mode_legacy_fb_format

[Eric Engestrom]

On Wednesday, 2017-04-26 07:53:10 +0200, Gerd Hoffmann wrote:
> On Di, 2017-04-25 at 12:18 +0900, Michel Dänzer wrote:
> > On 24/04/17 03:25 PM, Gerd Hoffmann wrote:
> > > Return correct fourcc codes on bigendian.  Drivers must be adapted to
> > > this change.
> > > 
> > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > 
> > Just to reiterate, this won't work for the radeon driver, which programs
> > the GPU to use (effectively, per the current definition that these are
> > little endian GPU formats) DRM_FORMAT_XRGB8888 with pre-R600 and
> > DRM_FORMAT_BGRX8888 with >= R600.
> 
> Hmm, ok, how does bigendian fbdev emulation work on pre-R600 then?
> 
> > > +#ifdef __BIG_ENDIAN
> > > +	switch (bpp) {
> > > +	case 8:
> > > +		fmt = DRM_FORMAT_C8;
> > > +		break;
> > > +	case 24:
> > > +		fmt = DRM_FORMAT_BGR888;
> > > +		break;
> > 
> > BTW, endianness as a concept cannot apply to 8 or 24 bpp formats.
> 
> I could move the 8 bpp case out of the #ifdef somehow, but code
> readability will suffer then I think ...

How about something like this?

	uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth)
	{
		uint32_t fmt;
	#ifdef __BIG_ENDIAN
		enum { LITTLE_ENDIAN = 0 };
	#else
		enum { LITTLE_ENDIAN = 1 };
	#endif
	/* ... */

(using an enum for compile-time constness)

and then
	fmt = DRM_FORMAT_ARGB8888;
becomes
	fmt = LITTLE_ENDIAN ? DRM_FORMAT_ARGB8888 : DRM_FORMAT_BGRA8888;

Might be easier to read than duplicating the whole switch?

> 
> For 24 we have different byte orderings, but yes, you can't switch from
> one to the other with byteswapping.  Probably one of the reasons why
> this format is pretty much out of fashion these days ...
> 
> cheers,
>   Gerd
> 

Re: [PATCH 3/6] drm: fourcc byteorder: add bigendian support to drm_mode_legacy_fb_format

[Eric Engestrom]

On Wednesday, 2017-04-26 07:53:10 +0200, Gerd Hoffmann wrote:
> On Di, 2017-04-25 at 12:18 +0900, Michel Dänzer wrote:
> > On 24/04/17 03:25 PM, Gerd Hoffmann wrote:
> > > Return correct fourcc codes on bigendian.  Drivers must be adapted to
> > > this change.
> > > 
> > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > 
> > Just to reiterate, this won't work for the radeon driver, which programs
> > the GPU to use (effectively, per the current definition that these are
> > little endian GPU formats) DRM_FORMAT_XRGB8888 with pre-R600 and
> > DRM_FORMAT_BGRX8888 with >= R600.
> 
> Hmm, ok, how does bigendian fbdev emulation work on pre-R600 then?
> 
> > > +#ifdef __BIG_ENDIAN
> > > +	switch (bpp) {
> > > +	case 8:
> > > +		fmt = DRM_FORMAT_C8;
> > > +		break;
> > > +	case 24:
> > > +		fmt = DRM_FORMAT_BGR888;
> > > +		break;
> > 
> > BTW, endianness as a concept cannot apply to 8 or 24 bpp formats.
> 
> I could move the 8 bpp case out of the #ifdef somehow, but code
> readability will suffer then I think ...

How about something like this?

	uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth)
	{
		uint32_t fmt;
	#ifdef __BIG_ENDIAN
		enum { LITTLE_ENDIAN = 0 };
	#else
		enum { LITTLE_ENDIAN = 1 };
	#endif
	/* ... */

(using an enum for compile-time constness)

and then
	fmt = DRM_FORMAT_ARGB8888;
becomes
	fmt = LITTLE_ENDIAN ? DRM_FORMAT_ARGB8888 : DRM_FORMAT_BGRA8888;

Might be easier to read than duplicating the whole switch?

> 
> For 24 we have different byte orderings, but yes, you can't switch from
> one to the other with byteswapping.  Probably one of the reasons why
> this format is pretty much out of fashion these days ...
> 
> cheers,
>   Gerd
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 3/6] drm: fourcc byteorder: add bigendian support to drm_mode_legacy_fb_format

[Gerd Hoffmann]

> 	uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth)
> 	{
> 		uint32_t fmt;
> 	#ifdef __BIG_ENDIAN
> 		enum { LITTLE_ENDIAN = 0 };
> 	#else
> 		enum { LITTLE_ENDIAN = 1 };
> 	#endif
> 	/* ... */
> 
> (using an enum for compile-time constness)
> 
> and then
> 	fmt = DRM_FORMAT_ARGB8888;
> becomes
> 	fmt = LITTLE_ENDIAN ? DRM_FORMAT_ARGB8888 : DRM_FORMAT_BGRA8888;
> 
> Might be easier to read than duplicating the whole switch?

Well, there are more differences, like rgb565 and xrgb2101010 not being
supported for bigendian, so it isn't *that* simple.

cheers,
  Gerd

Re: [PATCH 3/6] drm: fourcc byteorder: add bigendian support to drm_mode_legacy_fb_format

[Gerd Hoffmann]

> 	uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth)
> 	{
> 		uint32_t fmt;
> 	#ifdef __BIG_ENDIAN
> 		enum { LITTLE_ENDIAN = 0 };
> 	#else
> 		enum { LITTLE_ENDIAN = 1 };
> 	#endif
> 	/* ... */
> 
> (using an enum for compile-time constness)
> 
> and then
> 	fmt = DRM_FORMAT_ARGB8888;
> becomes
> 	fmt = LITTLE_ENDIAN ? DRM_FORMAT_ARGB8888 : DRM_FORMAT_BGRA8888;
> 
> Might be easier to read than duplicating the whole switch?

Well, there are more differences, like rgb565 and xrgb2101010 not being
supported for bigendian, so it isn't *that* simple.

cheers,
  Gerd

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 3/6] drm: fourcc byteorder: add bigendian support to drm_mode_legacy_fb_format

[Ville Syrjälä]

On Wed, Apr 26, 2017 at 11:00:09AM +0900, Michel Dänzer wrote:
> On 25/04/17 06:52 PM, Ville Syrjälä wrote:
> > On Tue, Apr 25, 2017 at 12:18:52PM +0900, Michel Dänzer wrote:
> >> On 24/04/17 03:25 PM, Gerd Hoffmann wrote:
> >>> +#ifdef __BIG_ENDIAN
> >>> +	switch (bpp) {
> >>> +	case 8:
> >>> +		fmt = DRM_FORMAT_C8;
> >>> +		break;
> >>> +	case 24:
> >>> +		fmt = DRM_FORMAT_BGR888;
> >>> +		break;
> >>
> >> BTW, endianness as a concept cannot apply to 8 or 24 bpp formats.
> > 
> > To 8bpp no, but it can easily apply to 24bpp.
> 
> Any byte swapping rips apart the bytes of a 24bpp pixel, so those
> formats only make sense as straight array formats.

In my book little endian just means "lsb is stored in the lowest
memory address". The fact that your CPU/GPU can't do 3 byte swaps
is not relevant for that definition IMO.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 3/6] drm: fourcc byteorder: add bigendian support to drm_mode_legacy_fb_format

[Ville Syrjälä]

On Wed, Apr 26, 2017 at 11:00:09AM +0900, Michel Dänzer wrote:
> On 25/04/17 06:52 PM, Ville Syrjälä wrote:
> > On Tue, Apr 25, 2017 at 12:18:52PM +0900, Michel Dänzer wrote:
> >> On 24/04/17 03:25 PM, Gerd Hoffmann wrote:
> >>> +#ifdef __BIG_ENDIAN
> >>> +	switch (bpp) {
> >>> +	case 8:
> >>> +		fmt = DRM_FORMAT_C8;
> >>> +		break;
> >>> +	case 24:
> >>> +		fmt = DRM_FORMAT_BGR888;
> >>> +		break;
> >>
> >> BTW, endianness as a concept cannot apply to 8 or 24 bpp formats.
> > 
> > To 8bpp no, but it can easily apply to 24bpp.
> 
> Any byte swapping rips apart the bytes of a 24bpp pixel, so those
> formats only make sense as straight array formats.

In my book little endian just means "lsb is stored in the lowest
memory address". The fact that your CPU/GPU can't do 3 byte swaps
is not relevant for that definition IMO.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 3/6] drm: fourcc byteorder: add bigendian support to drm_mode_legacy_fb_format

[Michel Dänzer]

On 26/04/17 09:11 PM, Gerd Hoffmann wrote:
>   Hi,
> 
>>>> Just to reiterate, this won't work for the radeon driver, which programs
>>>> the GPU to use (effectively, per the current definition that these are
>>>> little endian GPU formats) DRM_FORMAT_XRGB8888 with pre-R600 and
>>>> DRM_FORMAT_BGRX8888 with >= R600.
>>>
>>> Hmm, ok, how does bigendian fbdev emulation work on pre-R600 then?
>>
>> Using a GPU byte swapping mechanism which only affects CPU access to
>> video RAM.
> 
> That is done using the RADEON_TILING_SWAP_{16,32}BIT flag mentioned in
> another thread?

Right.


> What about dumb bos?  You've mentioned the swap flag isn't used for
> those.  Which implies they are in little endian byte order (both gpu and
> cpu view).

Right, AFAICT from looking at the code.


> Does the modesetting driver work correctly on that hardware?

Not sure.


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

Re: [PATCH 3/6] drm: fourcc byteorder: add bigendian support to drm_mode_legacy_fb_format

[Michel Dänzer]

On 26/04/17 09:11 PM, Gerd Hoffmann wrote:
>   Hi,
> 
>>>> Just to reiterate, this won't work for the radeon driver, which programs
>>>> the GPU to use (effectively, per the current definition that these are
>>>> little endian GPU formats) DRM_FORMAT_XRGB8888 with pre-R600 and
>>>> DRM_FORMAT_BGRX8888 with >= R600.
>>>
>>> Hmm, ok, how does bigendian fbdev emulation work on pre-R600 then?
>>
>> Using a GPU byte swapping mechanism which only affects CPU access to
>> video RAM.
> 
> That is done using the RADEON_TILING_SWAP_{16,32}BIT flag mentioned in
> another thread?

Right.


> What about dumb bos?  You've mentioned the swap flag isn't used for
> those.  Which implies they are in little endian byte order (both gpu and
> cpu view).

Right, AFAICT from looking at the code.


> Does the modesetting driver work correctly on that hardware?

Not sure.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/6] drm: fourcc byteorder: add bigendian support to drm_mode_legacy_fb_format

[Gerd Hoffmann]

  Hi,

> > That is done using the RADEON_TILING_SWAP_{16,32}BIT flag mentioned in
> > another thread?
> 
> Right.
> 
> 
> > What about dumb bos?  You've mentioned the swap flag isn't used for
> > those.  Which implies they are in little endian byte order (both gpu and
> > cpu view).
> 
> Right, AFAICT from looking at the code.

Ok.

And I also don't see an easy way to make them big endian (cpu view)
using swapping with the existing drm interfaces, given we apply a format
when we put the bo into use as framebuffer, not when creating it.  So
userspace can: (1) create dumb bo, (2) map bo, (3) write something bo,
(4) create fb + attach to crtc.  And at (3) we don't know the format
yet, so we can't configure swapping accordingly.

So just not using the swapping indeed looks like the only sensible
option.  Which in turn implies there is no BGRA8888 support for dumb
bos.  Hmm, I can see the problem.  Userspace expectation appears to be
that ADDFB configures a native endian framebuffer, which the driver
simply can't do on bigendian.

So, what can/should the driver do here?  Throw errors for ADDFB and
force userspace to use ADDFB2?  From a design point of view the best
option, but in the other hand I suspect that could break the xorg radeon
driver ...

cheers,
  Gerd

Re: [PATCH 3/6] drm: fourcc byteorder: add bigendian support to drm_mode_legacy_fb_format

[Gerd Hoffmann]

  Hi,

> > That is done using the RADEON_TILING_SWAP_{16,32}BIT flag mentioned in
> > another thread?
> 
> Right.
> 
> 
> > What about dumb bos?  You've mentioned the swap flag isn't used for
> > those.  Which implies they are in little endian byte order (both gpu and
> > cpu view).
> 
> Right, AFAICT from looking at the code.

Ok.

And I also don't see an easy way to make them big endian (cpu view)
using swapping with the existing drm interfaces, given we apply a format
when we put the bo into use as framebuffer, not when creating it.  So
userspace can: (1) create dumb bo, (2) map bo, (3) write something bo,
(4) create fb + attach to crtc.  And at (3) we don't know the format
yet, so we can't configure swapping accordingly.

So just not using the swapping indeed looks like the only sensible
option.  Which in turn implies there is no BGRA8888 support for dumb
bos.  Hmm, I can see the problem.  Userspace expectation appears to be
that ADDFB configures a native endian framebuffer, which the driver
simply can't do on bigendian.

So, what can/should the driver do here?  Throw errors for ADDFB and
force userspace to use ADDFB2?  From a design point of view the best
option, but in the other hand I suspect that could break the xorg radeon
driver ...

cheers,
  Gerd

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 3/6] drm: fourcc byteorder: add bigendian support to drm_mode_legacy_fb_format

[Michel Dänzer]

On 27/04/17 03:45 PM, Gerd Hoffmann wrote:
>   Hi,
> 
>>> That is done using the RADEON_TILING_SWAP_{16,32}BIT flag mentioned in
>>> another thread?
>>
>> Right.
>>
>>
>>> What about dumb bos?  You've mentioned the swap flag isn't used for
>>> those.  Which implies they are in little endian byte order (both gpu and
>>> cpu view).
>>
>> Right, AFAICT from looking at the code.
> 
> Ok.
> 
> And I also don't see an easy way to make them big endian (cpu view)
> using swapping with the existing drm interfaces, given we apply a format
> when we put the bo into use as framebuffer, not when creating it.  So
> userspace can: (1) create dumb bo, (2) map bo, (3) write something bo,
> (4) create fb + attach to crtc.  And at (3) we don't know the format
> yet, so we can't configure swapping accordingly.
> 
> So just not using the swapping indeed looks like the only sensible
> option.  Which in turn implies there is no BGRA8888 support for dumb
> bos.  Hmm, I can see the problem.  Userspace expectation appears to be
> that ADDFB configures a native endian framebuffer, which the driver
> simply can't do on bigendian.

... with pre-R600 GPUs.


> So, what can/should the driver do here?  Throw errors for ADDFB and
> force userspace to use ADDFB2?  From a design point of view the best
> option, but in the other hand I suspect that could break the xorg radeon
> driver ...

Yes, it would.

One thing we could do is provide a way for userspace to query the
effective format(s) as seen by the GPU and/or CPU.

It might also make sense for the radeon driver to set the
RADEON_TILING_SWAP_{16,32}BIT flags for dumb BOs. I wonder about the
status of apps using dumb BOs directly wrt this discussion.


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

Re: [PATCH 3/6] drm: fourcc byteorder: add bigendian support to drm_mode_legacy_fb_format

[Gerd Hoffmann]

  Hi,

> > So just not using the swapping indeed looks like the only sensible
> > option.  Which in turn implies there is no BGRA8888 support for dumb
> > bos.  Hmm, I can see the problem.  Userspace expectation appears to be
> > that ADDFB configures a native endian framebuffer, which the driver
> > simply can't do on bigendian.
> 
> ... with pre-R600 GPUs.

Sure.

> > So, what can/should the driver do here?  Throw errors for ADDFB and
> > force userspace to use ADDFB2?  From a design point of view the best
> > option, but in the other hand I suspect that could break the xorg radeon
> > driver ...
> 
> Yes, it would.

> One thing we could do is provide a way for userspace to query the
> effective format(s) as seen by the GPU and/or CPU.

We already have almost no testing on bigendian.  I doubt adding generic
interfaces specifically to handle this case is going to work because
most userspace will simply not implement that correctly (or at all).

Having support for this in the radeon ioctls might work, because only
radeon kernel + xorg driver have to get things right then.  But I
suspect we already have that.  You've mentioned elsewhere in the thread
that the xorg driver doesn't turn on byteswapping, so the ability to
configure that seems to be somewhere in the API ...

> It might also make sense for the radeon driver to set the
> RADEON_TILING_SWAP_{16,32}BIT flags for dumb BOs.

That could work.  But I guess someone with test hardware needs to try,
to make sure we don't miss corner cases here.

cheers,
  Gerd

Re: [PATCH 3/6] drm: fourcc byteorder: add bigendian support to drm_mode_legacy_fb_format

[Gerd Hoffmann]

  Hi,

> > So just not using the swapping indeed looks like the only sensible
> > option.  Which in turn implies there is no BGRA8888 support for dumb
> > bos.  Hmm, I can see the problem.  Userspace expectation appears to be
> > that ADDFB configures a native endian framebuffer, which the driver
> > simply can't do on bigendian.
> 
> ... with pre-R600 GPUs.

Sure.

> > So, what can/should the driver do here?  Throw errors for ADDFB and
> > force userspace to use ADDFB2?  From a design point of view the best
> > option, but in the other hand I suspect that could break the xorg radeon
> > driver ...
> 
> Yes, it would.

> One thing we could do is provide a way for userspace to query the
> effective format(s) as seen by the GPU and/or CPU.

We already have almost no testing on bigendian.  I doubt adding generic
interfaces specifically to handle this case is going to work because
most userspace will simply not implement that correctly (or at all).

Having support for this in the radeon ioctls might work, because only
radeon kernel + xorg driver have to get things right then.  But I
suspect we already have that.  You've mentioned elsewhere in the thread
that the xorg driver doesn't turn on byteswapping, so the ability to
configure that seems to be somewhere in the API ...

> It might also make sense for the radeon driver to set the
> RADEON_TILING_SWAP_{16,32}BIT flags for dumb BOs.

That could work.  But I guess someone with test hardware needs to try,
to make sure we don't miss corner cases here.

cheers,
  Gerd

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx