Linux Virtualization - Apr 2017 - DRM_FORMAT_* byte order (was: Re: [PATCH] drm: virtio: fix virtio_gpu_cursor_formats)

Hi,
> >  static const uint32_t virtio_gpu_cursor_formats[] = {
> > +#ifdef __BIG_ENDIAN
> > +	DRM_FORMAT_BGRA8888,
> > +#else
> >  	DRM_FORMAT_ARGB8888,
> > +#endif
> 
> DRM formats are supposed to be little endian, so this isn't really
> correct.
Well, maybe they where *intended* to be little endian at some point in
the past.  The actual code appears to interpret them as native endian
though.

Lets take a simple example, the bochs driver (qemu sdvga).  It supports
32 bpp with depth 24 (DRM_FORMAT_XRGB8888) as the one and only
framebuffer format (see bochs_user_framebuffer_create).  We still had to
add a special register to the virtual hardware so the guest can signal
to the host whenever the framebuffer is big endian or little endian (see
bochs_hw_init), so both ppc64 and ppc64le guests work properly with the
qemu stdvga.

So, bigendian guests assume that DRM_FORMAT_XRGB8888 is big endian not
little endian.  And given that the fourcc codes are used in the
userspace/kernel API too (see DRM_IOCTL_MODE_ADDFB2) I think we can't
change that any more ...

cheers,
  Gerd

On Thu, Apr 06, 2017 at 10:29:43AM +0200, Gerd Hoffmann
wrote:
>   Hi,
> 
> > >  static const uint32_t virtio_gpu_cursor_formats[] = {
> > > +#ifdef __BIG_ENDIAN
> > > +	DRM_FORMAT_BGRA8888,
> > > +#else
> > >  	DRM_FORMAT_ARGB8888,
> > > +#endif
> > 
> > DRM formats are supposed to be little endian, so this isn't really
> > correct.
> 
> Well, maybe they where *intended* to be little endian at some point in
> the past.  The actual code appears to interpret them as native endian
> though.
> 
> Lets take a simple example, the bochs driver (qemu sdvga).  It supports
> 32 bpp with depth 24 (DRM_FORMAT_XRGB8888) as the one and only
> framebuffer format (see bochs_user_framebuffer_create).  We still had to
> add a special register to the virtual hardware so the guest can signal
> to the host whenever the framebuffer is big endian or little endian (see
> bochs_hw_init), so both ppc64 and ppc64le guests work properly with the
> qemu stdvga.
> 
> So, bigendian guests assume that DRM_FORMAT_XRGB8888 is big endian not
> little endian.  And given that the fourcc codes are used in the
> userspace/kernel API too (see DRM_IOCTL_MODE_ADDFB2) I think we can't
> change that any more ...
Sigh. That makes mixed endian systems pretty much hopeless :(
It's a shame people didn't read the docuemntation.

It's also doubly disappointing because eg. the more standardized YUV
formats are definitely little endian as far the official fourccs are
concerned. So if we now make everything follow the host endianness
these things become a huge mess for anyone wanting to do video
playback etc.

Oh well, at least I tried to make it sane from the start. I'll just
go back to my blissful little endian world now.

-- 
Ville Syrj?l?
Intel OTC

On Thu, Apr 06, 2017 at 08:27:47PM +0300, Ville Syrj?l?
wrote:
> On Thu, Apr 06, 2017 at 10:29:43AM +0200, Gerd Hoffmann wrote:
> >   Hi,
> > 
> > > >  static const uint32_t virtio_gpu_cursor_formats[] = {
> > > > +#ifdef __BIG_ENDIAN
> > > > +	DRM_FORMAT_BGRA8888,
> > > > +#else
> > > >  	DRM_FORMAT_ARGB8888,
> > > > +#endif
> > > 
> > > DRM formats are supposed to be little endian, so this isn't
really
> > > correct.
> > 
> > Well, maybe they where *intended* to be little endian at some point in
> > the past.  The actual code appears to interpret them as native endian
> > though.
> > 
> > Lets take a simple example, the bochs driver (qemu sdvga).  It
supports
> > 32 bpp with depth 24 (DRM_FORMAT_XRGB8888) as the one and only
> > framebuffer format (see bochs_user_framebuffer_create).  We still had
to
> > add a special register to the virtual hardware so the guest can signal
> > to the host whenever the framebuffer is big endian or little endian
(see
> > bochs_hw_init), so both ppc64 and ppc64le guests work properly with
the
> > qemu stdvga.
> > 
> > So, bigendian guests assume that DRM_FORMAT_XRGB8888 is big endian not
> > little endian.  And given that the fourcc codes are used in the
> > userspace/kernel API too (see DRM_IOCTL_MODE_ADDFB2) I think we
can't
> > change that any more ...
> 
> Sigh. That makes mixed endian systems pretty much hopeless :(
Hmm. Maybe it's still possible to salvage something by redefining the
BIG_ENDIAN format bit to mean the "the other endianness". Ugly but it
might still result in something usable.

-- 
Ville Syrj?l?
Intel OTC

Hi,
> > So, bigendian guests assume that DRM_FORMAT_XRGB8888 is big endian not
> > little endian.  And given that the fourcc codes are used in the
> > userspace/kernel API too (see DRM_IOCTL_MODE_ADDFB2) I think we
can't
> > change that any more ...
> 
> Sigh. That makes mixed endian systems pretty much hopeless :(
At least you can't use the DRM_FORMAT_* (alone) to specify the format.
Still manageable, we have to do that to handle ppc64 and ppc64le ;)
> It's also doubly disappointing because eg. the more standardized YUV
> formats are definitely little endian as far the official fourccs are
> concerned. So if we now make everything follow the host endianness
> these things become a huge mess for anyone wanting to do video
> playback etc.
That one is up for discussion.  Guess I should brew a initial patch to
kickstart it.  At least in bigendian virtual machines (bochs and virtio
drivers) the YUV formats are not supported at all.  Dunno how things
look elsewhere.
> Oh well, at least I tried to make it sane from the start. I'll just
> go back to my blissful little endian world now.
ppc64le springing into live pretty much proves that little endian is
pretty close to reach world domination.  So maybe all our endian
troubles will be solved that way some day.

cheers,
  Gerd