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

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,
> 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.
Also at least for the virtual machine use case this doesn't buy us much.
The drm drivers (at least the ones used on both big and little endian
guests) support only 32 bpp + depth 24 formats.  And for these we don't
need a "other endian" flag because we have fourcc codes for all sorts
of
byte orders (i.e. DRM_FORMAT_XRGB8888 little endian =DRM_FORMAT_BGRX8888 big
endian).

The DRM_FORMAT_BIG_ENDIAN flags also seems not be used anywhere in the
code base (except in some format printing debug code ...).

cheers,
  Gerd

On Fri, Apr 07, 2017 at 10:29:00AM +0200, Gerd Hoffmann
wrote:
>   Hi,
> 
> > 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.
> 
> Also at least for the virtual machine use case this doesn't buy us
much.
> The drm drivers (at least the ones used on both big and little endian
> guests) support only 32 bpp + depth 24 formats.  And for these we don't
> need a "other endian" flag because we have fourcc codes for all
sorts of
> byte orders (i.e. DRM_FORMAT_XRGB8888 little endian =>
DRM_FORMAT_BGRX8888 big endian).
Yeah, those could be handled without the flag. But when mixed with any
other format the code would look a bit weird IMO. So my idea with the
flag was that if you display is big endian you always have the flag, and
then you don't have to think so much which way the bytes go for each
format. Less special casing is good IMO.
> 
> The DRM_FORMAT_BIG_ENDIAN flags also seems not be used anywhere in the
> code base (except in some format printing debug code ...).
That's because either no one has big endian display hardware or they
do but just didn't read the docs and cargo culted the format handling
from some driver for little endian display.

-- 
Ville Syrj?l?
Intel OTC