[PATCH v2 0/6] drm: byteorder fixes

[PATCH v2 0/6] drm: byteorder fixes

[Gerd Hoffmann]

Patch series adds some convinience #defines for host byteoder drm
formats.  It fixes drm_mode_addfb() behavior on bigendian machines.  For
bug compatibility reasons a mode_config quirk activates the fix.
bochs and virtio-gpu drivers are updated to use the new #defines, set
the new driver feature flag and fix some issues.

Gerd Hoffmann (6):
  drm: replace DRIVER_PREFER_XBGR_30BPP driver flag with mode_config
    quirk
  drm: byteorder: add DRM_FORMAT_HOST_*
  drm: do not mask out DRM_FORMAT_BIG_ENDIAN
  drm: fix drm_mode_addfb() on big endian machines.
  drm/bochs: fix DRM_FORMAT_* handling for big endian machines.
  drm/virtio: fix DRM_FORMAT_* handling

 include/drm/drm_drv.h                    |  1 -
 include/drm/drm_fourcc.h                 | 22 +++++++++++++
 include/drm/drm_mode_config.h            | 15 +++++++++
 drivers/gpu/drm/bochs/bochs_fbdev.c      |  5 ++-
 drivers/gpu/drm/bochs/bochs_kms.c        | 34 +++++++++++++++++++-
 drivers/gpu/drm/bochs/bochs_mm.c         |  2 +-
 drivers/gpu/drm/drm_framebuffer.c        | 17 ++++++++--
 drivers/gpu/drm/nouveau/dispnv50/disp.c  |  2 +-
 drivers/gpu/drm/virtio/virtgpu_display.c |  5 +++
 drivers/gpu/drm/virtio/virtgpu_fb.c      |  2 +-
 drivers/gpu/drm/virtio/virtgpu_gem.c     |  7 +++--
 drivers/gpu/drm/virtio/virtgpu_plane.c   | 54 ++------------------------------
 12 files changed, 101 insertions(+), 65 deletions(-)

-- 
2.9.3

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

[PATCH v2 1/6] drm: replace DRIVER_PREFER_XBGR_30BPP driver flag with mode_config quirk

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/drm/drm_drv.h                   | 1 -
 include/drm/drm_mode_config.h           | 1 +
 drivers/gpu/drm/drm_framebuffer.c       | 4 ++--
 drivers/gpu/drm/nouveau/dispnv50/disp.c | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index 46a8009784..23b9678137 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -56,7 +56,6 @@ struct drm_printer;
 #define DRIVER_ATOMIC			0x10000
 #define DRIVER_KMS_LEGACY_CONTEXT	0x20000
 #define DRIVER_SYNCOBJ                  0x40000
-#define DRIVER_PREFER_XBGR_30BPP        0x80000
 
 /**
  * struct drm_driver - DRM driver structure
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index a0b202e1d6..5d29f4ba6f 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -809,6 +809,7 @@ struct drm_mode_config {
 
 	/* dumb ioctl parameters */
 	uint32_t preferred_depth, prefer_shadow;
+	bool quirk_addfb_prefer_xbgr_30bpp;
 
 	/**
 	 * @async_page_flip: Does this device support async flips on the primary
diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index 781af1d42d..17b7b8944d 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -120,8 +120,8 @@ int drm_mode_addfb(struct drm_device *dev, struct drm_mode_fb_cmd *or,
 	r.pixel_format = drm_mode_legacy_fb_format(or->bpp, or->depth);
 	r.handles[0] = or->handle;
 
-	if (r.pixel_format == DRM_FORMAT_XRGB2101010 &&
-	    dev->driver->driver_features & DRIVER_PREFER_XBGR_30BPP)
+	if (dev->mode_config.quirk_addfb_prefer_xbgr_30bpp &&
+	    r.pixel_format == DRM_FORMAT_XRGB2101010)
 		r.pixel_format = DRM_FORMAT_XBGR2101010;
 
 	ret = drm_mode_addfb2(dev, &r, file_priv);
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index 8412119bd9..a9bb656058 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -2174,7 +2174,7 @@ nv50_display_create(struct drm_device *dev)
 	nouveau_display(dev)->fini = nv50_display_fini;
 	disp->disp = &nouveau_display(dev)->disp;
 	dev->mode_config.funcs = &nv50_disp_func;
-	dev->driver->driver_features |= DRIVER_PREFER_XBGR_30BPP;
+	dev->mode_config.quirk_addfb_prefer_xbgr_30bpp = true;
 
 	/* small shared memory area we use for notifiers and semaphores */
 	ret = nouveau_bo_new(&drm->client, 4096, 0x1000, TTM_PL_FLAG_VRAM,
-- 
2.9.3

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

[PATCH v2 2/6] drm: byteorder: add DRM_FORMAT_HOST_*

[Gerd Hoffmann]

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

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

diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
index f9c15845f4..fac831c401 100644
--- a/include/drm/drm_fourcc.h
+++ b/include/drm/drm_fourcc.h
@@ -25,6 +25,28 @@
 #include <linux/types.h>
 #include <uapi/drm/drm_fourcc.h>
 
+/*
+ * DRM formats are little endian.  Define host endian variants for the
+ * most common formats here, to reduce the #ifdefs needed in drivers.
+ *
+ * Note that the DRM_FORMAT_BIG_ENDIAN flag should only be used in
+ * case the format can't be specified otherwise, so we don't end up
+ * with two values describing the same format.
+ */
+#ifdef __BIG_ENDIAN
+# define DRM_FORMAT_HOST_XRGB1555     (DRM_FORMAT_XRGB1555         |	\
+				       DRM_FORMAT_BIG_ENDIAN)
+# define DRM_FORMAT_HOST_RGB565       (DRM_FORMAT_RGB565           |	\
+				       DRM_FORMAT_BIG_ENDIAN)
+# define DRM_FORMAT_HOST_XRGB8888     DRM_FORMAT_BGRX8888
+# define DRM_FORMAT_HOST_ARGB8888     DRM_FORMAT_BGRA8888
+#else
+# define DRM_FORMAT_HOST_XRGB1555     DRM_FORMAT_XRGB1555
+# define DRM_FORMAT_HOST_RGB565       DRM_FORMAT_RGB565
+# define DRM_FORMAT_HOST_XRGB8888     DRM_FORMAT_XRGB8888
+# define DRM_FORMAT_HOST_ARGB8888     DRM_FORMAT_ARGB8888
+#endif
+
 struct drm_device;
 struct drm_mode_fb_cmd2;
 
-- 
2.9.3

[PATCH v2 3/6] drm: do not mask out DRM_FORMAT_BIG_ENDIAN

[Gerd Hoffmann]

framebuffer_check() expects that drm_get_format_info() will not fail if
the __drm_format_info() call was successful.  That'll work only in case
both are called with the same pixel_format value, so masking out the
DRM_FORMAT_BIG_ENDIAN flag isn't a good idea.

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

diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index 17b7b8944d..888c4d53cf 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -164,7 +164,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;
 
-- 
2.9.3

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

[PATCH v2 4/6] drm: fix drm_mode_addfb() on big endian machines.

[Gerd Hoffmann]

Userspace on big endian machhines typically expects the ADDFB ioctl
returns a big endian framebuffer.  drm_mode_addfb() will call
drm_mode_addfb2() unconditionally with little endian DRM_FORMAT_*
values though, which is wrong.  This patch fixes that.

Drivers (both kernel and xorg) have quirks in place to deal with the
broken drm_mode_addfb() behavior.  Because of this we can't just change
drm_mode_addfb() behavior for everybody without breaking things.  Add
the quirk_addfb_prefer_host_byte_order field to mode_config, so drivers
can opt-in.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/drm/drm_mode_config.h     | 14 ++++++++++++++
 drivers/gpu/drm/drm_framebuffer.c | 11 +++++++++++
 2 files changed, 25 insertions(+)

diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index 5d29f4ba6f..65020086c9 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -811,6 +811,20 @@ struct drm_mode_config {
 	uint32_t preferred_depth, prefer_shadow;
 	bool quirk_addfb_prefer_xbgr_30bpp;
 
+	/*
+	 * @quirk_addfb_prefer_host_byte_order:
+	 *
+	 * When set to true drm_mode_addfb() will pick host byte order
+	 * pixel_format when calling drm_mode_addfb2().  This is how
+	 * drm_mode_addfb() should have worked from day one.  It
+	 * didn't though, so we ended up with quirks in both kernel
+	 * and userspace drivers to deal with the broken behavior.
+	 * Simply fixing drm_mode_addfb() unconditionally would break
+	 * these drivers, so add a quirk bit here to allow drivers
+	 * opt-in.
+	 */
+	bool quirk_addfb_prefer_host_byte_order;
+
 	/**
 	 * @async_page_flip: Does this device support async flips on the primary
 	 * plane?
diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index 888c4d53cf..f863f8a20f 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -124,6 +124,17 @@ int drm_mode_addfb(struct drm_device *dev, struct drm_mode_fb_cmd *or,
 	    r.pixel_format == DRM_FORMAT_XRGB2101010)
 		r.pixel_format = DRM_FORMAT_XBGR2101010;
 
+	if (dev->mode_config.quirk_addfb_prefer_host_byte_order) {
+		if (r.pixel_format == DRM_FORMAT_XRGB8888)
+			r.pixel_format = DRM_FORMAT_HOST_XRGB8888;
+		if (r.pixel_format == DRM_FORMAT_ARGB8888)
+			r.pixel_format = DRM_FORMAT_HOST_ARGB8888;
+		if (r.pixel_format == DRM_FORMAT_RGB565)
+			r.pixel_format = DRM_FORMAT_HOST_RGB565;
+		if (r.pixel_format == DRM_FORMAT_XRGB1555)
+			r.pixel_format = DRM_FORMAT_HOST_XRGB1555;
+	}
+
 	ret = drm_mode_addfb2(dev, &r, file_priv);
 	if (ret)
 		return ret;
-- 
2.9.3

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

Re: [PATCH v2 4/6] drm: fix drm_mode_addfb() on big endian machines.

[Daniel Vetter]

On Wed, Sep 05, 2018 at 08:04:43AM +0200, Gerd Hoffmann wrote:
> Userspace on big endian machhines typically expects the ADDFB ioctl
> returns a big endian framebuffer.  drm_mode_addfb() will call
> drm_mode_addfb2() unconditionally with little endian DRM_FORMAT_*
> values though, which is wrong.  This patch fixes that.
> 
> Drivers (both kernel and xorg) have quirks in place to deal with the
> broken drm_mode_addfb() behavior.  Because of this we can't just change
> drm_mode_addfb() behavior for everybody without breaking things.  Add
> the quirk_addfb_prefer_host_byte_order field to mode_config, so drivers
> can opt-in.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/drm/drm_mode_config.h     | 14 ++++++++++++++
>  drivers/gpu/drm/drm_framebuffer.c | 11 +++++++++++
>  2 files changed, 25 insertions(+)
> 
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index 5d29f4ba6f..65020086c9 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -811,6 +811,20 @@ struct drm_mode_config {
>  	uint32_t preferred_depth, prefer_shadow;
>  	bool quirk_addfb_prefer_xbgr_30bpp;
>  
> +	/*

This doesn't parse, needs /**

$ make htmldocs

to check it.

> +	 * @quirk_addfb_prefer_host_byte_order:
> +	 *
> +	 * When set to true drm_mode_addfb() will pick host byte order
> +	 * pixel_format when calling drm_mode_addfb2().  This is how
> +	 * drm_mode_addfb() should have worked from day one.  It
> +	 * didn't though, so we ended up with quirks in both kernel
> +	 * and userspace drivers to deal with the broken behavior.
> +	 * Simply fixing drm_mode_addfb() unconditionally would break
> +	 * these drivers, so add a quirk bit here to allow drivers
> +	 * opt-in.
> +	 */
> +	bool quirk_addfb_prefer_host_byte_order;

I wonder whether we should reject addFB2 on big endian hosts if this isn't
set. Otherwise we need addfb3, aka will never be able to fix this. Anyway,
that might be good work for a follow-up ...

Up to this patch series (with the kerneldoc fixed):

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +
>  	/**
>  	 * @async_page_flip: Does this device support async flips on the primary
>  	 * plane?
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index 888c4d53cf..f863f8a20f 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -124,6 +124,17 @@ int drm_mode_addfb(struct drm_device *dev, struct drm_mode_fb_cmd *or,
>  	    r.pixel_format == DRM_FORMAT_XRGB2101010)
>  		r.pixel_format = DRM_FORMAT_XBGR2101010;
>  
> +	if (dev->mode_config.quirk_addfb_prefer_host_byte_order) {
> +		if (r.pixel_format == DRM_FORMAT_XRGB8888)
> +			r.pixel_format = DRM_FORMAT_HOST_XRGB8888;
> +		if (r.pixel_format == DRM_FORMAT_ARGB8888)
> +			r.pixel_format = DRM_FORMAT_HOST_ARGB8888;
> +		if (r.pixel_format == DRM_FORMAT_RGB565)
> +			r.pixel_format = DRM_FORMAT_HOST_RGB565;
> +		if (r.pixel_format == DRM_FORMAT_XRGB1555)
> +			r.pixel_format = DRM_FORMAT_HOST_XRGB1555;
> +	}
> +
>  	ret = drm_mode_addfb2(dev, &r, file_priv);
>  	if (ret)
>  		return ret;
> -- 
> 2.9.3
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/6] drm: fix drm_mode_addfb() on big endian machines.

[Gerd Hoffmann]

  Hi,

> > +	bool quirk_addfb_prefer_host_byte_order;
> 
> I wonder whether we should reject addFB2 on big endian hosts if this isn't
> set. Otherwise we need addfb3, aka will never be able to fix this. Anyway,
> that might be good work for a follow-up ...

Sounds sensible, I'll send a patch.

> Up to this patch series (with the kerneldoc fixed):
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Fixed, patches 1-4 pushed.

Any takers for 5+6 review?  Dave?

thanks,
  Gerd

[PATCH v2 5/6] drm/bochs: fix DRM_FORMAT_* handling for big endian machines.

[Gerd Hoffmann]

Use DRM_FORMAT_HOST_XRGB8888, so we are using the correct format code
on bigendian machines.  Also set the quirk_addfb_prefer_host_byte_order
mode_config bit so drm_mode_addfb() asks for the correct format code.

Create our own plane and use drm_crtc_init_with_planes() instead of
depending on the default created by drm_crtc_init().  That way the plane
format list is correct on bigendian machines.

With this patch applied both ADDFB and ADDFB2 ioctls work correctly in
the bochs-drm.ko driver on big endian machines.  Without the patch only
ADDFB (which still seems to be used by the majority of userspace) works
correctly.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/bochs/bochs_fbdev.c |  5 ++---
 drivers/gpu/drm/bochs/bochs_kms.c   | 34 +++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/bochs/bochs_mm.c    |  2 +-
 3 files changed, 36 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/bochs/bochs_fbdev.c b/drivers/gpu/drm/bochs/bochs_fbdev.c
index 14eb8d0d5a..bf728790fa 100644
--- a/drivers/gpu/drm/bochs/bochs_fbdev.c
+++ b/drivers/gpu/drm/bochs/bochs_fbdev.c
@@ -64,9 +64,8 @@ static int bochsfb_create(struct drm_fb_helper *helper,
 
 	mode_cmd.width = sizes->surface_width;
 	mode_cmd.height = sizes->surface_height;
-	mode_cmd.pitches[0] = mode_cmd.width * ((sizes->surface_bpp + 7) / 8);
-	mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
-							  sizes->surface_depth);
+	mode_cmd.pitches[0] = sizes->surface_width * 4;
+	mode_cmd.pixel_format = DRM_FORMAT_HOST_XRGB8888;
 	size = mode_cmd.pitches[0] * mode_cmd.height;
 
 	/* alloc, pin & map bo */
diff --git a/drivers/gpu/drm/bochs/bochs_kms.c b/drivers/gpu/drm/bochs/bochs_kms.c
index ca5a9afdd5..b40077d04b 100644
--- a/drivers/gpu/drm/bochs/bochs_kms.c
+++ b/drivers/gpu/drm/bochs/bochs_kms.c
@@ -129,12 +129,43 @@ static const struct drm_crtc_helper_funcs bochs_helper_funcs = {
 	.commit = bochs_crtc_commit,
 };
 
+static const uint32_t bochs_formats[] = {
+	DRM_FORMAT_HOST_XRGB8888,
+};
+
+static struct drm_plane *bochs_primary_plane(struct drm_device *dev)
+{
+	struct drm_plane *primary;
+	int ret;
+
+	primary = kzalloc(sizeof(*primary), GFP_KERNEL);
+	if (primary == NULL) {
+		DRM_DEBUG_KMS("Failed to allocate primary plane\n");
+		return NULL;
+	}
+
+	ret = drm_universal_plane_init(dev, primary, 0,
+				       &drm_primary_helper_funcs,
+				       bochs_formats,
+				       ARRAY_SIZE(bochs_formats),
+				       NULL,
+				       DRM_PLANE_TYPE_PRIMARY, NULL);
+	if (ret) {
+		kfree(primary);
+		primary = NULL;
+	}
+
+	return primary;
+}
+
 static void bochs_crtc_init(struct drm_device *dev)
 {
 	struct bochs_device *bochs = dev->dev_private;
 	struct drm_crtc *crtc = &bochs->crtc;
+	struct drm_plane *primary = bochs_primary_plane(dev);
 
-	drm_crtc_init(dev, crtc, &bochs_crtc_funcs);
+	drm_crtc_init_with_planes(dev, crtc, primary, NULL,
+				  &bochs_crtc_funcs, NULL);
 	drm_crtc_helper_add(crtc, &bochs_helper_funcs);
 }
 
@@ -253,6 +284,7 @@ int bochs_kms_init(struct bochs_device *bochs)
 	bochs->dev->mode_config.fb_base = bochs->fb_base;
 	bochs->dev->mode_config.preferred_depth = 24;
 	bochs->dev->mode_config.prefer_shadow = 0;
+	bochs->dev->mode_config.quirk_addfb_prefer_host_byte_order = true;
 
 	bochs->dev->mode_config.funcs = &bochs_mode_funcs;
 
diff --git a/drivers/gpu/drm/bochs/bochs_mm.c b/drivers/gpu/drm/bochs/bochs_mm.c
index c9c7097030..fdf151fbdb 100644
--- a/drivers/gpu/drm/bochs/bochs_mm.c
+++ b/drivers/gpu/drm/bochs/bochs_mm.c
@@ -506,7 +506,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_HOST_XRGB8888)
 		return ERR_PTR(-ENOENT);
 
 	obj = drm_gem_object_lookup(filp, mode_cmd->handles[0]);
-- 
2.9.3

[PATCH v2 6/6] drm/virtio: fix DRM_FORMAT_* handling

[Gerd Hoffmann]

Use DRM_FORMAT_HOST_XRGB8888, so we are using the correct format code
on bigendian machines.  Also set the quirk_addfb_prefer_host_byte_order
mode_config bit so drm_mode_addfb() asks for the correct format code.

Both DRM_FORMAT_* and VIRTIO_GPU_FORMAT_* are defined to be little
endian, so using a different mapping on bigendian machines is wrong.
It's there because of broken drm_mode_addfb() behavior.  So with
drm_mode_addfb() being fixed we can fix this too.

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.
Pick DRM_FORMAT_HOST_XRGB8888, it is the only one actually used in
practice.  Drop unused mappings in virtio_gpu_translate_format().

With this patch applied both ADDFB and ADDFB2 ioctls work correctly in
the virtio-gpu.ko driver on big endian machines.  Without the patch only
ADDFB (which still seems to be used by the majority of userspace) works
correctly.

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

diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c
index 25503b9335..14a13edc02 100644
--- a/drivers/gpu/drm/virtio/virtgpu_display.c
+++ b/drivers/gpu/drm/virtio/virtgpu_display.c
@@ -306,6 +306,10 @@ virtio_gpu_user_framebuffer_create(struct drm_device *dev,
 	struct virtio_gpu_framebuffer *virtio_gpu_fb;
 	int ret;
 
+	if (mode_cmd->pixel_format != DRM_FORMAT_HOST_XRGB8888 &&
+	    mode_cmd->pixel_format != DRM_FORMAT_HOST_ARGB8888)
+		return ERR_PTR(-ENOENT);
+
 	/* lookup object associated with res handle */
 	obj = drm_gem_object_lookup(file_priv, mode_cmd->handles[0]);
 	if (!obj)
@@ -354,6 +358,7 @@ int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev)
 	int i;
 
 	drm_mode_config_init(vgdev->ddev);
+	vgdev->ddev->mode_config.quirk_addfb_prefer_host_byte_order = true;
 	vgdev->ddev->mode_config.funcs = &virtio_gpu_mode_funcs;
 	vgdev->ddev->mode_config.helper_private = &virtio_mode_config_helpers;
 
diff --git a/drivers/gpu/drm/virtio/virtgpu_fb.c b/drivers/gpu/drm/virtio/virtgpu_fb.c
index a121b1c795..9d87ebd645 100644
--- a/drivers/gpu/drm/virtio/virtgpu_fb.c
+++ b/drivers/gpu/drm/virtio/virtgpu_fb.c
@@ -233,7 +233,7 @@ static int virtio_gpufb_create(struct drm_fb_helper *helper,
 	mode_cmd.width = sizes->surface_width;
 	mode_cmd.height = sizes->surface_height;
 	mode_cmd.pitches[0] = mode_cmd.width * 4;
-	mode_cmd.pixel_format = drm_mode_legacy_fb_format(32, 24);
+	mode_cmd.pixel_format = DRM_FORMAT_HOST_XRGB8888;
 
 	format = virtio_gpu_translate_format(mode_cmd.pixel_format);
 	if (format == 0)
diff --git a/drivers/gpu/drm/virtio/virtgpu_gem.c b/drivers/gpu/drm/virtio/virtgpu_gem.c
index 0f2768eaca..82c817f37c 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);
 
@@ -99,7 +102,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_HOST_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 dc5b5b2b7a..3221d50b9a 100644
--- a/drivers/gpu/drm/virtio/virtgpu_plane.c
+++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
@@ -28,22 +28,11 @@
 #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_HOST_XRGB8888,
 };
 
 static const uint32_t virtio_gpu_cursor_formats[] = {
-#ifdef __BIG_ENDIAN
-	DRM_FORMAT_BGRA8888,
-#else
-	DRM_FORMAT_ARGB8888,
-#endif
+	DRM_FORMAT_HOST_ARGB8888,
 };
 
 uint32_t virtio_gpu_translate_format(uint32_t drm_fourcc)
@@ -51,32 +40,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;
@@ -89,19 +52,6 @@ uint32_t virtio_gpu_translate_format(uint32_t drm_fourcc)
 	case DRM_FORMAT_BGRA8888:
 		format = VIRTIO_GPU_FORMAT_A8R8G8B8_UNORM;
 		break;
-	case DRM_FORMAT_RGBX8888:
-		format = VIRTIO_GPU_FORMAT_X8B8G8R8_UNORM;
-		break;
-	case DRM_FORMAT_RGBA8888:
-		format = VIRTIO_GPU_FORMAT_A8B8G8R8_UNORM;
-		break;
-	case DRM_FORMAT_XBGR8888:
-		format = VIRTIO_GPU_FORMAT_R8G8B8X8_UNORM;
-		break;
-	case DRM_FORMAT_ABGR8888:
-		format = VIRTIO_GPU_FORMAT_R8G8B8A8_UNORM;
-		break;
-#endif
 	default:
 		/*
 		 * This should not happen, we handle everything listed
-- 
2.9.3

Re: [PATCH v2 0/6] drm: byteorder fixes

[Ilia Mirkin]

Hi Gerd,

What happened with this series (and the next one)?

Semi-relatedly, I wonder if it wouldn't be better to just dump the
BIG_ENDIAN flag and just define the "host" format to be the right one
for a consistent little-endian interpretation.

Cheers,

  -ilia


On Wed, Sep 5, 2018 at 2:04 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> Patch series adds some convinience #defines for host byteoder drm
> formats.  It fixes drm_mode_addfb() behavior on bigendian machines.  For
> bug compatibility reasons a mode_config quirk activates the fix.
> bochs and virtio-gpu drivers are updated to use the new #defines, set
> the new driver feature flag and fix some issues.
>
> Gerd Hoffmann (6):
>   drm: replace DRIVER_PREFER_XBGR_30BPP driver flag with mode_config
>     quirk
>   drm: byteorder: add DRM_FORMAT_HOST_*
>   drm: do not mask out DRM_FORMAT_BIG_ENDIAN
>   drm: fix drm_mode_addfb() on big endian machines.
>   drm/bochs: fix DRM_FORMAT_* handling for big endian machines.
>   drm/virtio: fix DRM_FORMAT_* handling
>
>  include/drm/drm_drv.h                    |  1 -
>  include/drm/drm_fourcc.h                 | 22 +++++++++++++
>  include/drm/drm_mode_config.h            | 15 +++++++++
>  drivers/gpu/drm/bochs/bochs_fbdev.c      |  5 ++-
>  drivers/gpu/drm/bochs/bochs_kms.c        | 34 +++++++++++++++++++-
>  drivers/gpu/drm/bochs/bochs_mm.c         |  2 +-
>  drivers/gpu/drm/drm_framebuffer.c        | 17 ++++++++--
>  drivers/gpu/drm/nouveau/dispnv50/disp.c  |  2 +-
>  drivers/gpu/drm/virtio/virtgpu_display.c |  5 +++
>  drivers/gpu/drm/virtio/virtgpu_fb.c      |  2 +-
>  drivers/gpu/drm/virtio/virtgpu_gem.c     |  7 +++--
>  drivers/gpu/drm/virtio/virtgpu_plane.c   | 54 ++------------------------------
>  12 files changed, 101 insertions(+), 65 deletions(-)
>
> --
> 2.9.3
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/6] drm: byteorder fixes

[Gerd Hoffmann]

On Fri, Jan 11, 2019 at 01:17:47AM -0500, Ilia Mirkin wrote:
> Hi Gerd,
> 
> What happened with this series (and the next one)?

Landed upstream in 4.20.

> Semi-relatedly, I wonder if it wouldn't be better to just dump the
> BIG_ENDIAN flag and just define the "host" format to be the right one
> for a consistent little-endian interpretation.

Not fully sure what you mean here.

The big endian flag is needed to specify RGB565 or XRGB1555 format in
big endian byte order.  For 32 bit depth we don't need it because
XRGB8888 big endian == BGRX8888 little endian (see also
include/drm/drm_fourcc.h).

cheers,
  Gerd

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

Re: [PATCH v2 0/6] drm: byteorder fixes

[Daniel Vetter]

On Fri, Jan 11, 2019 at 10:08 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> On Fri, Jan 11, 2019 at 01:17:47AM -0500, Ilia Mirkin wrote:
> > Hi Gerd,
> >
> > What happened with this series (and the next one)?
>
> Landed upstream in 4.20.
>
> > Semi-relatedly, I wonder if it wouldn't be better to just dump the
> > BIG_ENDIAN flag and just define the "host" format to be the right one
> > for a consistent little-endian interpretation.
>
> Not fully sure what you mean here.
>
> The big endian flag is needed to specify RGB565 or XRGB1555 format in
> big endian byte order.  For 32 bit depth we don't need it because
> XRGB8888 big endian == BGRX8888 little endian (see also
> include/drm/drm_fourcc.h).

Yeah we'd need to add a few more fourcc codes for the missing
big-endian versions. Aside from that I like Ilia's idea of removing
the BIG_ENDIAN flag, it causes lots of aliasing among formats, doesn't
change anything with others, and we probably have a huge mess already
...
-Daniel

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



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/6] drm: byteorder fixes

[Gerd Hoffmann]

On Fri, Jan 11, 2019 at 10:11:09AM +0100, Daniel Vetter wrote:
> On Fri, Jan 11, 2019 at 10:08 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> > On Fri, Jan 11, 2019 at 01:17:47AM -0500, Ilia Mirkin wrote:
> > > Hi Gerd,
> > >
> > > What happened with this series (and the next one)?
> >
> > Landed upstream in 4.20.
> >
> > > Semi-relatedly, I wonder if it wouldn't be better to just dump the
> > > BIG_ENDIAN flag and just define the "host" format to be the right one
> > > for a consistent little-endian interpretation.
> >
> > Not fully sure what you mean here.
> >
> > The big endian flag is needed to specify RGB565 or XRGB1555 format in
> > big endian byte order.  For 32 bit depth we don't need it because
> > XRGB8888 big endian == BGRX8888 little endian (see also
> > include/drm/drm_fourcc.h).
> 
> Yeah we'd need to add a few more fourcc codes for the missing
> big-endian versions.

If we do that then yes we can drop the BIG_ENDIAN flag.

Not sure what the userspace API implications are,
ADDFB2 ioctl comes to mind.

> Aside from that I like Ilia's idea of removing
> the BIG_ENDIAN flag, it causes lots of aliasing among formats, doesn't
> change anything with others, and we probably have a huge mess already
> ...

Shouldn't be too messy.  The place where the DRM_FORMAT_HOST_* formats
are defined (include/drm/drm_fourcc.h) is almost the only place where
DRM_FORMAT_BIG_ENDIAN is used, and for the DRM_FORMAT_HOST_* users the
switch should be transparent.

cheers,
  Gerd

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

Re: [PATCH v2 0/6] drm: byteorder fixes

[Daniel Vetter]

On Fri, Jan 11, 2019 at 10:43:42AM +0100, Gerd Hoffmann wrote:
> On Fri, Jan 11, 2019 at 10:11:09AM +0100, Daniel Vetter wrote:
> > On Fri, Jan 11, 2019 at 10:08 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
> > >
> > > On Fri, Jan 11, 2019 at 01:17:47AM -0500, Ilia Mirkin wrote:
> > > > Hi Gerd,
> > > >
> > > > What happened with this series (and the next one)?
> > >
> > > Landed upstream in 4.20.
> > >
> > > > Semi-relatedly, I wonder if it wouldn't be better to just dump the
> > > > BIG_ENDIAN flag and just define the "host" format to be the right one
> > > > for a consistent little-endian interpretation.
> > >
> > > Not fully sure what you mean here.
> > >
> > > The big endian flag is needed to specify RGB565 or XRGB1555 format in
> > > big endian byte order.  For 32 bit depth we don't need it because
> > > XRGB8888 big endian == BGRX8888 little endian (see also
> > > include/drm/drm_fourcc.h).
> > 
> > Yeah we'd need to add a few more fourcc codes for the missing
> > big-endian versions.
> 
> If we do that then yes we can drop the BIG_ENDIAN flag.
> 
> Not sure what the userspace API implications are,
> ADDFB2 ioctl comes to mind.
> 
> > Aside from that I like Ilia's idea of removing
> > the BIG_ENDIAN flag, it causes lots of aliasing among formats, doesn't
> > change anything with others, and we probably have a huge mess already
> > ...
> 
> Shouldn't be too messy.  The place where the DRM_FORMAT_HOST_* formats
> are defined (include/drm/drm_fourcc.h) is almost the only place where
> DRM_FORMAT_BIG_ENDIAN is used, and for the DRM_FORMAT_HOST_* users the
> switch should be transparent.

Yeah we'd need to review current userspace, but I suspect the overlap of
"uses addfb2" and "runs on BE platform" is 0.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/6] drm: byteorder fixes

[Ilia Mirkin]

[-- Attachment #1.1: Type: text/plain, Size: 2313 bytes --]

On Fri, Jan 11, 2019, 05:26 Daniel Vetter <daniel@ffwll.ch wrote:

> On Fri, Jan 11, 2019 at 10:43:42AM +0100, Gerd Hoffmann wrote:
> > On Fri, Jan 11, 2019 at 10:11:09AM +0100, Daniel Vetter wrote:
> > > On Fri, Jan 11, 2019 at 10:08 AM Gerd Hoffmann <kraxel@redhat.com>
> wrote:
> > > >
> > > > On Fri, Jan 11, 2019 at 01:17:47AM -0500, Ilia Mirkin wrote:
> > > > > Hi Gerd,
> > > > >
> > > > > What happened with this series (and the next one)?
> > > >
> > > > Landed upstream in 4.20.
>

Huh. I must have been looking at an option tree.

> > >
> > > > > Semi-relatedly, I wonder if it wouldn't be better to just dump the
> > > > > BIG_ENDIAN flag and just define the "host" format to be the right
> one
> > > > > for a consistent little-endian interpretation.
> > > >
> > > > Not fully sure what you mean here.
> > > >
> > > > The big endian flag is needed to specify RGB565 or XRGB1555 format in
> > > > big endian byte order.  For 32 bit depth we don't need it because
> > > > XRGB8888 big endian == BGRX8888 little endian (see also
> > > > include/drm/drm_fourcc.h).
>

Bleargh. I was thinking xrgb1555 le == bgrx5551 be, but that's clearly
false. Made sense to me last night though. That makes my suggestion
considerably less appealing.

> >
> > > Yeah we'd need to add a few more fourcc codes for the missing
> > > big-endian versions.
> >
> > If we do that then yes we can drop the BIG_ENDIAN flag.
> >
> > Not sure what the userspace API implications are,
> > ADDFB2 ioctl comes to mind.
> >
> > > Aside from that I like Ilia's idea of removing
> > > the BIG_ENDIAN flag, it causes lots of aliasing among formats, doesn't
> > > change anything with others, and we probably have a huge mess already
> > > ...
> >
> > Shouldn't be too messy.  The place where the DRM_FORMAT_HOST_* formats
> > are defined (include/drm/drm_fourcc.h) is almost the only place where
> > DRM_FORMAT_BIG_ENDIAN is used, and for the DRM_FORMAT_HOST_* users the
> > switch should be transparent.
>
> Yeah we'd need to review current userspace, but I suspect the overlap of
> "uses addfb2" and "runs on BE platform" is 0.
>

Most importantly, "uses bigendian flag" is 0 since we would previously
error out on such modesets.

Although the need to double up on the 16bpp formats makes this not really
worth it. Ohhh well.

  -ilia

[-- Attachment #1.2: Type: text/html, Size: 3722 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH v2 0/6] drm: byteorder fixes

[Daniel Vetter]

On Fri, Jan 11, 2019 at 2:08 PM Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>
>
>
> On Fri, Jan 11, 2019, 05:26 Daniel Vetter <daniel@ffwll.ch wrote:
>>
>> On Fri, Jan 11, 2019 at 10:43:42AM +0100, Gerd Hoffmann wrote:
>> > On Fri, Jan 11, 2019 at 10:11:09AM +0100, Daniel Vetter wrote:
>> > > On Fri, Jan 11, 2019 at 10:08 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
>> > > >
>> > > > On Fri, Jan 11, 2019 at 01:17:47AM -0500, Ilia Mirkin wrote:
>> > > > > Hi Gerd,
>> > > > >
>> > > > > What happened with this series (and the next one)?
>> > > >
>> > > > Landed upstream in 4.20.
>
>
> Huh. I must have been looking at an option tree.
>
>> > > >
>> > > > > Semi-relatedly, I wonder if it wouldn't be better to just dump the
>> > > > > BIG_ENDIAN flag and just define the "host" format to be the right one
>> > > > > for a consistent little-endian interpretation.
>> > > >
>> > > > Not fully sure what you mean here.
>> > > >
>> > > > The big endian flag is needed to specify RGB565 or XRGB1555 format in
>> > > > big endian byte order.  For 32 bit depth we don't need it because
>> > > > XRGB8888 big endian == BGRX8888 little endian (see also
>> > > > include/drm/drm_fourcc.h).
>
>
> Bleargh. I was thinking xrgb1555 le == bgrx5551 be, but that's clearly false. Made sense to me last night though. That makes my suggestion considerably less appealing.
>
>> > >
>> > > Yeah we'd need to add a few more fourcc codes for the missing
>> > > big-endian versions.
>> >
>> > If we do that then yes we can drop the BIG_ENDIAN flag.
>> >
>> > Not sure what the userspace API implications are,
>> > ADDFB2 ioctl comes to mind.
>> >
>> > > Aside from that I like Ilia's idea of removing
>> > > the BIG_ENDIAN flag, it causes lots of aliasing among formats, doesn't
>> > > change anything with others, and we probably have a huge mess already
>> > > ...
>> >
>> > Shouldn't be too messy.  The place where the DRM_FORMAT_HOST_* formats
>> > are defined (include/drm/drm_fourcc.h) is almost the only place where
>> > DRM_FORMAT_BIG_ENDIAN is used, and for the DRM_FORMAT_HOST_* users the
>> > switch should be transparent.
>>
>> Yeah we'd need to review current userspace, but I suspect the overlap of
>> "uses addfb2" and "runs on BE platform" is 0.
>
>
> Most importantly, "uses bigendian flag" is 0 since we would previously error out on such modesets.
>
> Although the need to double up on the 16bpp formats makes this not really worth it. Ohhh well.

I think even with a few formats doubled up it'll be worth it, since we
remove all the confusion around the formats where the BE flag is not
needed/confusion/causes aliasing with another format. Also given that
even ppc is switching to LE I think making LE the forced default
assumption for everything will work much better long term. At least
with the explicit fourcc code, legacy addfb will pick native endianess
(at least since 4.20).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel