[PATCH 1/5] drm: byteorder: add DRM_FORMAT_HOST_*

[PATCH 1/5] 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 2/5] 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 781af1d42d..88758096d5 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

[PATCH 3/5] 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.  So add
a new driver feature flag DRIVER_PREFER_HOST_BYTE_ORDER, so drivers can
opt-in.

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

diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index 46a8009784..9cf12596cd 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -57,6 +57,7 @@ struct drm_printer;
 #define DRIVER_KMS_LEGACY_CONTEXT	0x20000
 #define DRIVER_SYNCOBJ                  0x40000
 #define DRIVER_PREFER_XBGR_30BPP        0x80000
+#define DRIVER_PREFER_HOST_BYTE_ORDER   0x100000
 
 /**
  * struct drm_driver - DRM driver structure
diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index 88758096d5..ccbda8a2e9 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,
 	    dev->driver->driver_features & DRIVER_PREFER_XBGR_30BPP)
 		r.pixel_format = DRM_FORMAT_XBGR2101010;
 
+	if (dev->driver->driver_features & DRIVER_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

Re: [PATCH 3/5] drm: fix drm_mode_addfb() on big endian machines.

[Daniel Vetter]

On Mon, Sep 03, 2018 at 12:57:54PM +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.  So add
> a new driver feature flag DRIVER_PREFER_HOST_BYTE_ORDER, so drivers can
> opt-in.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/drm/drm_drv.h             |  1 +
>  drivers/gpu/drm/drm_framebuffer.c | 11 +++++++++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index 46a8009784..9cf12596cd 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -57,6 +57,7 @@ struct drm_printer;
>  #define DRIVER_KMS_LEGACY_CONTEXT	0x20000
>  #define DRIVER_SYNCOBJ                  0x40000
>  #define DRIVER_PREFER_XBGR_30BPP        0x80000
> +#define DRIVER_PREFER_HOST_BYTE_ORDER   0x100000

Hm, not a huge fan of using driver_flags for random little quirks. I think
a boolean in sturct drm_mode_config would be much better. Bonus if you
also move the 30bpp hack over to that. Something like
mode_config.quirk_addfb_host_byte_order and
mode_config.quirk_addfb_prefer_xbgr_30bpp or whatever. That has the upside
of giving us a really nice place to put a huge comment about what this is
supposed to do.

I think otherwise this looks overall rather reasonable. I think the only
other driver that ever cared about big endian was radeon/amdgpu. Would be
good to get at least an ack from amd folks, or a "meh, stopped caring".
-Daniel

>  
>  /**
>   * struct drm_driver - DRM driver structure
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index 88758096d5..ccbda8a2e9 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,
>  	    dev->driver->driver_features & DRIVER_PREFER_XBGR_30BPP)
>  		r.pixel_format = DRM_FORMAT_XBGR2101010;
>  
> +	if (dev->driver->driver_features & DRIVER_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

Re: [PATCH 3/5] drm: fix drm_mode_addfb() on big endian machines.

[Michel Dänzer]

On 2018-09-03 6:45 p.m., Daniel Vetter wrote:
> On Mon, Sep 03, 2018 at 12:57:54PM +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.  So add
>> a new driver feature flag DRIVER_PREFER_HOST_BYTE_ORDER, so drivers can
>> opt-in.

Since the changes are opt-in now, they shouldn't affect drivers which
don't opt in; they should work as well (or as badly :) after these
changes as they did before. So no concerns from my side anymore.


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

Re: [PATCH 3/5] drm: fix drm_mode_addfb() on big endian machines.

[Ilia Mirkin]

On Mon, Sep 3, 2018 at 12:45 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Sep 03, 2018 at 12:57:54PM +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.  So add
>> a new driver feature flag DRIVER_PREFER_HOST_BYTE_ORDER, so drivers can
>> opt-in.
>>
>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>> ---
>>  include/drm/drm_drv.h             |  1 +
>>  drivers/gpu/drm/drm_framebuffer.c | 11 +++++++++++
>>  2 files changed, 12 insertions(+)
>>
>> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
>> index 46a8009784..9cf12596cd 100644
>> --- a/include/drm/drm_drv.h
>> +++ b/include/drm/drm_drv.h
>> @@ -57,6 +57,7 @@ struct drm_printer;
>>  #define DRIVER_KMS_LEGACY_CONTEXT    0x20000
>>  #define DRIVER_SYNCOBJ                  0x40000
>>  #define DRIVER_PREFER_XBGR_30BPP        0x80000
>> +#define DRIVER_PREFER_HOST_BYTE_ORDER   0x100000
>
> Hm, not a huge fan of using driver_flags for random little quirks. I think
> a boolean in sturct drm_mode_config would be much better. Bonus if you
> also move the 30bpp hack over to that. Something like
> mode_config.quirk_addfb_host_byte_order and
> mode_config.quirk_addfb_prefer_xbgr_30bpp or whatever. That has the upside
> of giving us a really nice place to put a huge comment about what this is
> supposed to do.
>
> I think otherwise this looks overall rather reasonable. I think the only
> other driver that ever cared about big endian was radeon/amdgpu. Would be
> good to get at least an ack from amd folks, or a "meh, stopped caring".

and nouveau.

I do believe that ADDFB should be made to always prefer host byte
order -- this is how all of the existing implementations work in
practice. However e.g. nouveau wants DRM_FORMAT_XRGB8888. But it still
treats it as host byte order. This will become more important in a
world where ADDFB2 is more common.

So, I think that this change should be applied, drivers (I suspect
just nouveau and radeon) fixed up to consume the "new" formats, and
then made to be the one-and-only way for ADDFB to function. That way
we'll no longer be lying about the DRM_FORMAT being used.

Cheers,

  -ilia

Re: [PATCH 3/5] drm: fix drm_mode_addfb() on big endian machines.

[Michel Dänzer]

On 2018-09-03 7:07 p.m., Ilia Mirkin wrote:
> On Mon, Sep 3, 2018 at 12:45 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Mon, Sep 03, 2018 at 12:57:54PM +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.  So add
>>> a new driver feature flag DRIVER_PREFER_HOST_BYTE_ORDER, so drivers can
>>> opt-in.
>>>
>>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>>> ---
>>>  include/drm/drm_drv.h             |  1 +
>>>  drivers/gpu/drm/drm_framebuffer.c | 11 +++++++++++
>>>  2 files changed, 12 insertions(+)
>>>
>>> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
>>> index 46a8009784..9cf12596cd 100644
>>> --- a/include/drm/drm_drv.h
>>> +++ b/include/drm/drm_drv.h
>>> @@ -57,6 +57,7 @@ struct drm_printer;
>>>  #define DRIVER_KMS_LEGACY_CONTEXT    0x20000
>>>  #define DRIVER_SYNCOBJ                  0x40000
>>>  #define DRIVER_PREFER_XBGR_30BPP        0x80000
>>> +#define DRIVER_PREFER_HOST_BYTE_ORDER   0x100000
>>
>> Hm, not a huge fan of using driver_flags for random little quirks. I think
>> a boolean in sturct drm_mode_config would be much better. Bonus if you
>> also move the 30bpp hack over to that. Something like
>> mode_config.quirk_addfb_host_byte_order and
>> mode_config.quirk_addfb_prefer_xbgr_30bpp or whatever. That has the upside
>> of giving us a really nice place to put a huge comment about what this is
>> supposed to do.
>>
>> I think otherwise this looks overall rather reasonable. I think the only
>> other driver that ever cared about big endian was radeon/amdgpu. Would be
>> good to get at least an ack from amd folks, or a "meh, stopped caring".
> 
> and nouveau.
> 
> I do believe that ADDFB should be made to always prefer host byte
> order -- this is how all of the existing implementations work in
> practice. However e.g. nouveau wants DRM_FORMAT_XRGB8888. But it still
> treats it as host byte order. This will become more important in a
> world where ADDFB2 is more common.
> 
> So, I think that this change should be applied, drivers (I suspect
> just nouveau and radeon) fixed up to consume the "new" formats, [...]

As explained before, that would break radeon userspace on big endian hosts.


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

Re: [PATCH 3/5] drm: fix drm_mode_addfb() on big endian machines.

[Ilia Mirkin]

On Tue, Sep 4, 2018 at 4:00 AM, Michel Dänzer <michel@daenzer.net> wrote:
> On 2018-09-03 7:07 p.m., Ilia Mirkin wrote:
>> On Mon, Sep 3, 2018 at 12:45 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
>>> On Mon, Sep 03, 2018 at 12:57:54PM +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.  So add
>>>> a new driver feature flag DRIVER_PREFER_HOST_BYTE_ORDER, so drivers can
>>>> opt-in.
>>>>
>>>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>>>> ---
>>>>  include/drm/drm_drv.h             |  1 +
>>>>  drivers/gpu/drm/drm_framebuffer.c | 11 +++++++++++
>>>>  2 files changed, 12 insertions(+)
>>>>
>>>> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
>>>> index 46a8009784..9cf12596cd 100644
>>>> --- a/include/drm/drm_drv.h
>>>> +++ b/include/drm/drm_drv.h
>>>> @@ -57,6 +57,7 @@ struct drm_printer;
>>>>  #define DRIVER_KMS_LEGACY_CONTEXT    0x20000
>>>>  #define DRIVER_SYNCOBJ                  0x40000
>>>>  #define DRIVER_PREFER_XBGR_30BPP        0x80000
>>>> +#define DRIVER_PREFER_HOST_BYTE_ORDER   0x100000
>>>
>>> Hm, not a huge fan of using driver_flags for random little quirks. I think
>>> a boolean in sturct drm_mode_config would be much better. Bonus if you
>>> also move the 30bpp hack over to that. Something like
>>> mode_config.quirk_addfb_host_byte_order and
>>> mode_config.quirk_addfb_prefer_xbgr_30bpp or whatever. That has the upside
>>> of giving us a really nice place to put a huge comment about what this is
>>> supposed to do.
>>>
>>> I think otherwise this looks overall rather reasonable. I think the only
>>> other driver that ever cared about big endian was radeon/amdgpu. Would be
>>> good to get at least an ack from amd folks, or a "meh, stopped caring".
>>
>> and nouveau.
>>
>> I do believe that ADDFB should be made to always prefer host byte
>> order -- this is how all of the existing implementations work in
>> practice. However e.g. nouveau wants DRM_FORMAT_XRGB8888. But it still
>> treats it as host byte order. This will become more important in a
>> world where ADDFB2 is more common.
>>
>> So, I think that this change should be applied, drivers (I suspect
>> just nouveau and radeon) fixed up to consume the "new" formats, [...]
>
> As explained before, that would break radeon userspace on big endian hosts.

We last discussed this about a year ago, so I hope you'll forgive my
lapse in memory...

There's userspace that uses ADDFB2 with DRM_FORMAT_XRGB8888 but
expects it to be host-endian? If so, that'll be a problem for nouveau
as well...

  -ilia

Re: [PATCH 3/5] drm: fix drm_mode_addfb() on big endian machines.

[Michel Dänzer]

On 2018-09-04 3:05 p.m., Ilia Mirkin wrote:
> On Tue, Sep 4, 2018 at 4:00 AM, Michel Dänzer <michel@daenzer.net> wrote:
>> On 2018-09-03 7:07 p.m., Ilia Mirkin wrote:
>>> On Mon, Sep 3, 2018 at 12:45 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
>>>> On Mon, Sep 03, 2018 at 12:57:54PM +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.  So add
>>>>> a new driver feature flag DRIVER_PREFER_HOST_BYTE_ORDER, so drivers can
>>>>> opt-in.
>>>>>
>>>>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>>>>> ---
>>>>>  include/drm/drm_drv.h             |  1 +
>>>>>  drivers/gpu/drm/drm_framebuffer.c | 11 +++++++++++
>>>>>  2 files changed, 12 insertions(+)
>>>>>
>>>>> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
>>>>> index 46a8009784..9cf12596cd 100644
>>>>> --- a/include/drm/drm_drv.h
>>>>> +++ b/include/drm/drm_drv.h
>>>>> @@ -57,6 +57,7 @@ struct drm_printer;
>>>>>  #define DRIVER_KMS_LEGACY_CONTEXT    0x20000
>>>>>  #define DRIVER_SYNCOBJ                  0x40000
>>>>>  #define DRIVER_PREFER_XBGR_30BPP        0x80000
>>>>> +#define DRIVER_PREFER_HOST_BYTE_ORDER   0x100000
>>>>
>>>> Hm, not a huge fan of using driver_flags for random little quirks. I think
>>>> a boolean in sturct drm_mode_config would be much better. Bonus if you
>>>> also move the 30bpp hack over to that. Something like
>>>> mode_config.quirk_addfb_host_byte_order and
>>>> mode_config.quirk_addfb_prefer_xbgr_30bpp or whatever. That has the upside
>>>> of giving us a really nice place to put a huge comment about what this is
>>>> supposed to do.
>>>>
>>>> I think otherwise this looks overall rather reasonable. I think the only
>>>> other driver that ever cared about big endian was radeon/amdgpu. Would be
>>>> good to get at least an ack from amd folks, or a "meh, stopped caring".
>>>
>>> and nouveau.
>>>
>>> I do believe that ADDFB should be made to always prefer host byte
>>> order -- this is how all of the existing implementations work in
>>> practice. However e.g. nouveau wants DRM_FORMAT_XRGB8888. But it still
>>> treats it as host byte order. This will become more important in a
>>> world where ADDFB2 is more common.
>>>
>>> So, I think that this change should be applied, drivers (I suspect
>>> just nouveau and radeon) fixed up to consume the "new" formats, [...]
>>
>> As explained before, that would break radeon userspace on big endian hosts.
> 
> We last discussed this about a year ago, so I hope you'll forgive my
> lapse in memory...
> 
> There's userspace that uses ADDFB2 with DRM_FORMAT_XRGB8888 but
> expects it to be host-endian?

ADDFB, not ADDFB2. The latter probably didn't even exist yet when this
was made to work. :)


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

Re: [PATCH 3/5] drm: fix drm_mode_addfb() on big endian machines.

[Ilia Mirkin]

On Tue, Sep 4, 2018 at 11:02 AM, Michel Dänzer <michel@daenzer.net> wrote:
> On 2018-09-04 3:05 p.m., Ilia Mirkin wrote:
>> On Tue, Sep 4, 2018 at 4:00 AM, Michel Dänzer <michel@daenzer.net> wrote:
>>> On 2018-09-03 7:07 p.m., Ilia Mirkin wrote:
>>>> On Mon, Sep 3, 2018 at 12:45 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
>>>>> On Mon, Sep 03, 2018 at 12:57:54PM +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.  So add
>>>>>> a new driver feature flag DRIVER_PREFER_HOST_BYTE_ORDER, so drivers can
>>>>>> opt-in.
>>>>>>
>>>>>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>>>>>> ---
>>>>>>  include/drm/drm_drv.h             |  1 +
>>>>>>  drivers/gpu/drm/drm_framebuffer.c | 11 +++++++++++
>>>>>>  2 files changed, 12 insertions(+)
>>>>>>
>>>>>> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
>>>>>> index 46a8009784..9cf12596cd 100644
>>>>>> --- a/include/drm/drm_drv.h
>>>>>> +++ b/include/drm/drm_drv.h
>>>>>> @@ -57,6 +57,7 @@ struct drm_printer;
>>>>>>  #define DRIVER_KMS_LEGACY_CONTEXT    0x20000
>>>>>>  #define DRIVER_SYNCOBJ                  0x40000
>>>>>>  #define DRIVER_PREFER_XBGR_30BPP        0x80000
>>>>>> +#define DRIVER_PREFER_HOST_BYTE_ORDER   0x100000
>>>>>
>>>>> Hm, not a huge fan of using driver_flags for random little quirks. I think
>>>>> a boolean in sturct drm_mode_config would be much better. Bonus if you
>>>>> also move the 30bpp hack over to that. Something like
>>>>> mode_config.quirk_addfb_host_byte_order and
>>>>> mode_config.quirk_addfb_prefer_xbgr_30bpp or whatever. That has the upside
>>>>> of giving us a really nice place to put a huge comment about what this is
>>>>> supposed to do.
>>>>>
>>>>> I think otherwise this looks overall rather reasonable. I think the only
>>>>> other driver that ever cared about big endian was radeon/amdgpu. Would be
>>>>> good to get at least an ack from amd folks, or a "meh, stopped caring".
>>>>
>>>> and nouveau.
>>>>
>>>> I do believe that ADDFB should be made to always prefer host byte
>>>> order -- this is how all of the existing implementations work in
>>>> practice. However e.g. nouveau wants DRM_FORMAT_XRGB8888. But it still
>>>> treats it as host byte order. This will become more important in a
>>>> world where ADDFB2 is more common.
>>>>
>>>> So, I think that this change should be applied, drivers (I suspect
>>>> just nouveau and radeon) fixed up to consume the "new" formats, [...]
>>>
>>> As explained before, that would break radeon userspace on big endian hosts.
>>
>> We last discussed this about a year ago, so I hope you'll forgive my
>> lapse in memory...
>>
>> There's userspace that uses ADDFB2 with DRM_FORMAT_XRGB8888 but
>> expects it to be host-endian?
>
> ADDFB, not ADDFB2. The latter probably didn't even exist yet when this
> was made to work. :)

Right, but ADDFB doesn't know or care about DRM_FORMAT_*. That's what
I'm saying -- keep ADDFB working, and fix up the DRM_FORMAT_*
underneath it both in the conversion and in the driver. Gerd's patch
allows us to do this incrementally, eventually truing up the
DRM_FORMAT_* in the driver, enabling ADDFB2 to work as expected.

  -ilia

Re: [PATCH 3/5] drm: fix drm_mode_addfb() on big endian machines.

[Gerd Hoffmann]

  Hi,

> >>> As explained before, that would break radeon userspace on big endian hosts.
> >>
> >> We last discussed this about a year ago, so I hope you'll forgive my
> >> lapse in memory...
> >>
> >> There's userspace that uses ADDFB2 with DRM_FORMAT_XRGB8888 but
> >> expects it to be host-endian?
> >
> > ADDFB, not ADDFB2. The latter probably didn't even exist yet when this
> > was made to work. :)
> 
> Right, but ADDFB doesn't know or care about DRM_FORMAT_*. That's what
> I'm saying -- keep ADDFB working, and fix up the DRM_FORMAT_*
> underneath it both in the conversion and in the driver. Gerd's patch
> allows us to do this incrementally, eventually truing up the
> DRM_FORMAT_* in the driver, enabling ADDFB2 to work as expected.

If it is that simple then yes, we should be able to fix the radeon kms
driver, then drop the quirk once all kms drivers are fixed.

But IIRC there are some radeon-sepcific calls used by the radeon xorg
driver affected too (thats why the commit message says "... both xorg and
kernel drivers ..."), so fixing it for radeon isn't that easy ...

cheers,
  Gerd

[PATCH 4/5] 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 add DRIVER_PREFER_HOST_BYTE_ORDER driver
feature flag 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_drv.c   |  3 ++-
 drivers/gpu/drm/bochs/bochs_fbdev.c |  5 ++---
 drivers/gpu/drm/bochs/bochs_kms.c   | 33 ++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/bochs/bochs_mm.c    |  2 +-
 4 files changed, 37 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bochs/bochs_drv.c b/drivers/gpu/drm/bochs/bochs_drv.c
index 7b20318483..fa84a3c841 100644
--- a/drivers/gpu/drm/bochs/bochs_drv.c
+++ b/drivers/gpu/drm/bochs/bochs_drv.c
@@ -81,7 +81,8 @@ static const struct file_operations bochs_fops = {
 };
 
 static struct drm_driver bochs_driver = {
-	.driver_features	= DRIVER_GEM | DRIVER_MODESET,
+	.driver_features	= (DRIVER_GEM | DRIVER_MODESET |
+				   DRIVER_PREFER_HOST_BYTE_ORDER),
 	.load			= bochs_load,
 	.unload			= bochs_unload,
 	.fops			= &bochs_fops,
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..2662cdcf2d 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);
 }
 
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 5/5] 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 add DRIVER_PREFER_HOST_BYTE_ORDER driver
feature flag 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 |  4 +++
 drivers/gpu/drm/virtio/virtgpu_drv.c     |  4 ++-
 drivers/gpu/drm/virtio/virtgpu_fb.c      |  2 +-
 drivers/gpu/drm/virtio/virtgpu_gem.c     |  7 +++--
 drivers/gpu/drm/virtio/virtgpu_plane.c   | 54 ++------------------------------
 5 files changed, 15 insertions(+), 56 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c
index 25503b9335..f6c4af1db4 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)
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c
index d9287c144f..4b28f41ac1 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.c
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
@@ -114,7 +114,9 @@ static const struct file_operations virtio_gpu_driver_fops = {
 };
 
 static struct drm_driver driver = {
-	.driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME | DRIVER_RENDER | DRIVER_ATOMIC,
+	.driver_features = (DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME |
+			    DRIVER_RENDER | DRIVER_ATOMIC |
+			    DRIVER_PREFER_HOST_BYTE_ORDER),
 	.load = virtio_gpu_driver_load,
 	.unload = virtio_gpu_driver_unload,
 	.open = virtio_gpu_driver_open,
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