| Message ID | 20180905060445.15008-1-kraxel@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm: byteorder fixes | expand |
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 >
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
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
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
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
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 <div dir="auto"><div><br><br><div class="gmail_quote"><div dir="ltr">On Fri, Jan 11, 2019, 05:26 Daniel Vetter <<a href="mailto:daniel@ffwll.ch">daniel@ffwll.ch</a> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Fri, Jan 11, 2019 at 10:43:42AM +0100, Gerd Hoffmann wrote:<br> > On Fri, Jan 11, 2019 at 10:11:09AM +0100, Daniel Vetter wrote:<br> > > On Fri, Jan 11, 2019 at 10:08 AM Gerd Hoffmann <<a href="mailto:kraxel@redhat.com" target="_blank" rel="noreferrer">kraxel@redhat.com</a>> wrote:<br> > > ><br> > > > On Fri, Jan 11, 2019 at 01:17:47AM -0500, Ilia Mirkin wrote:<br> > > > > Hi Gerd,<br> > > > ><br> > > > > What happened with this series (and the next one)?<br> > > ><br> > > > Landed upstream in 4.20.<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">Huh. I must have been looking at an option tree.</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > > ><br> > > > > Semi-relatedly, I wonder if it wouldn't be better to just dump the<br> > > > > BIG_ENDIAN flag and just define the "host" format to be the right one<br> > > > > for a consistent little-endian interpretation.<br> > > ><br> > > > Not fully sure what you mean here.<br> > > ><br> > > > The big endian flag is needed to specify RGB565 or XRGB1555 format in<br> > > > big endian byte order. For 32 bit depth we don't need it because<br> > > > XRGB8888 big endian == BGRX8888 little endian (see also<br> > > > include/drm/drm_fourcc.h).<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">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.</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > > <br> > > Yeah we'd need to add a few more fourcc codes for the missing<br> > > big-endian versions.<br> > <br> > If we do that then yes we can drop the BIG_ENDIAN flag.<br> > <br> > Not sure what the userspace API implications are,<br> > ADDFB2 ioctl comes to mind.<br> > <br> > > Aside from that I like Ilia's idea of removing<br> > > the BIG_ENDIAN flag, it causes lots of aliasing among formats, doesn't<br> > > change anything with others, and we probably have a huge mess already<br> > > ...<br> > <br> > Shouldn't be too messy. The place where the DRM_FORMAT_HOST_* formats<br> > are defined (include/drm/drm_fourcc.h) is almost the only place where<br> > DRM_FORMAT_BIG_ENDIAN is used, and for the DRM_FORMAT_HOST_* users the<br> > switch should be transparent.<br> <br> Yeah we'd need to review current userspace, but I suspect the overlap of<br> "uses addfb2" and "runs on BE platform" is 0.<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">Most importantly, "uses bigendian flag" is 0 since we would previously error out on such modesets.</div><div dir="auto"><br></div><div dir="auto">Although the need to double up on the 16bpp formats makes this not really worth it. Ohhh well.</div><div dir="auto"><br></div><div dir="auto"> -ilia</div></div>
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
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(-)