After a loooong break, here is the next version of the patch series. It adds some convinience #defines for host byteoder drm formats. It fixes drm_mode_addfb() behavior on bigendian machines. For bug compatibility reasons a driver feature flag 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 (5): 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 +++++++++++++ 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 +- drivers/gpu/drm/drm_framebuffer.c | 13 +++++++- 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 ++------------------------------ 12 files changed, 87 insertions(+), 63 deletions(-)
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;
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;
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;
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
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.
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
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.
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
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. :)
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
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
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]);
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
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Gerd Hoffmann -
Ilia Mirkin -
Michel Dänzer