[PATCH] drm: virtio: fix virtio_gpu_cursor_formats

[PATCH] drm: virtio: fix virtio_gpu_cursor_formats

[Laurent Vivier]

When we use virtio-vga with a big-endian guest,
the mouse pointer disappears.

To fix that, on big-endian use DRM_FORMAT_BGRA8888
instead of DRM_FORMAT_ARGB8888.

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 drivers/gpu/drm/virtio/virtgpu_plane.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c
index 11288ff..3ed7174 100644
--- a/drivers/gpu/drm/virtio/virtgpu_plane.c
+++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
@@ -39,7 +39,11 @@ static const uint32_t virtio_gpu_formats[] = {
 };
 
 static const uint32_t virtio_gpu_cursor_formats[] = {
+#ifdef __BIG_ENDIAN
+	DRM_FORMAT_BGRA8888,
+#else
 	DRM_FORMAT_ARGB8888,
+#endif
 };
 
 static void virtio_gpu_plane_destroy(struct drm_plane *plane)
-- 
2.9.3

Re: [PATCH] drm: virtio: fix virtio_gpu_cursor_formats

[Ville Syrjälä]

On Wed, Apr 05, 2017 at 10:09:15AM +0200, Laurent Vivier wrote:
> When we use virtio-vga with a big-endian guest,
> the mouse pointer disappears.
> 
> To fix that, on big-endian use DRM_FORMAT_BGRA8888
> instead of DRM_FORMAT_ARGB8888.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  drivers/gpu/drm/virtio/virtgpu_plane.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c
> index 11288ff..3ed7174 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_plane.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
> @@ -39,7 +39,11 @@ static const uint32_t virtio_gpu_formats[] = {
>  };
>  
>  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.

>  };
>  
>  static void virtio_gpu_plane_destroy(struct drm_plane *plane)
> -- 
> 2.9.3
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm: virtio: fix virtio_gpu_cursor_formats

[Laurent Vivier]

On 05/04/2017 19:11, Ville Syrjälä wrote:
> On Wed, Apr 05, 2017 at 10:09:15AM +0200, Laurent Vivier wrote:
>> When we use virtio-vga with a big-endian guest,
>> the mouse pointer disappears.
>>
>> To fix that, on big-endian use DRM_FORMAT_BGRA8888
>> instead of DRM_FORMAT_ARGB8888.
>>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> ---
>>  drivers/gpu/drm/virtio/virtgpu_plane.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c
>> index 11288ff..3ed7174 100644
>> --- a/drivers/gpu/drm/virtio/virtgpu_plane.c
>> +++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
>> @@ -39,7 +39,11 @@ static const uint32_t virtio_gpu_formats[] = {
>>  };
>>  
>>  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.

In a big endian kernel, we need to swap the bytes order, where do you
think is the best place to do that?

Thanks,
Laurent
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] drm: virtio: fix virtio_gpu_cursor_formats

[Daniel Vetter]

On Wed, Apr 05, 2017 at 08:11:25PM +0300, Ville Syrjälä wrote:
> On Wed, Apr 05, 2017 at 10:09:15AM +0200, Laurent Vivier wrote:
> > When we use virtio-vga with a big-endian guest,
> > the mouse pointer disappears.
> > 
> > To fix that, on big-endian use DRM_FORMAT_BGRA8888
> > instead of DRM_FORMAT_ARGB8888.
> > 
> > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> > ---
> >  drivers/gpu/drm/virtio/virtgpu_plane.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c
> > index 11288ff..3ed7174 100644
> > --- a/drivers/gpu/drm/virtio/virtgpu_plane.c
> > +++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
> > @@ -39,7 +39,11 @@ static const uint32_t virtio_gpu_formats[] = {
> >  };
> >  
> >  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.

Reality says they're native endian, and I asked Gerd Hoffman to do a
documentation update and get that reviewed by amd folks (the only other
ones who care).
-Daniel

> 
> >  };
> >  
> >  static void virtio_gpu_plane_destroy(struct drm_plane *plane)
> > -- 
> > 2.9.3
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> 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

DRM_FORMAT_* byte order (was: Re: [PATCH] drm: virtio: fix virtio_gpu_cursor_formats)

[Gerd Hoffmann]

  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

Re: DRM_FORMAT_* byte order (was: Re: [PATCH] drm: virtio: fix virtio_gpu_cursor_formats)

[Ville Syrjälä]

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

Re: DRM_FORMAT_* byte order (was: Re: [PATCH] drm: virtio: fix virtio_gpu_cursor_formats)

[Ville Syrjälä]

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

Re: DRM_FORMAT_* byte order (was: Re: [PATCH] drm: virtio: fix virtio_gpu_cursor_formats)

[Gerd Hoffmann]

  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

Re: DRM_FORMAT_* byte order (was: Re: [PATCH] drm: virtio: fix virtio_gpu_cursor_formats)

[Gerd Hoffmann]

  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

Re: DRM_FORMAT_* byte order (was: Re: [PATCH] drm: virtio: fix virtio_gpu_cursor_formats)

[Ville Syrjälä]

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

Re: DRM_FORMAT_* byte order (was: Re: [PATCH] drm: virtio: fix virtio_gpu_cursor_formats)

[Gerd Hoffmann]

On Fr, 2017-04-07 at 11:45 +0300, Ville Syrjälä wrote:
> 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.

Well, there is a reason only the 32bpp formats are supported.  With
those you just adjust your color shifts and you are done.  No need to
actually byte-swap.  In contrast handling 16bpp formats (5:6:5 or 5:5:5)
with the non-native byte order is a PITA.

The other reason of course is that this is the default format these
days.

So, do any "other formats" exist where the byteswapped variant is used
in practice?  Or can we just drop DRM_FORMAT_BIG_ENDIAN?

> 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.

Having two valid fourcc (DRM_FORMAT_XRGB8888 + (DRM_FORMAT_BGRX8888 |
DRM_FORMAT_BIG_ENDIAN)) for the same format is confusing IMO.  And I
doubt that it'll be properly implemented everywhere.

cheers,
  Gerd

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: DRM_FORMAT_* byte order (was: Re: [PATCH] drm: virtio: fix virtio_gpu_cursor_formats)

[Ville Syrjälä]

On Fri, Apr 07, 2017 at 12:06:26PM +0200, Gerd Hoffmann wrote:
> On Fr, 2017-04-07 at 11:45 +0300, Ville Syrjälä wrote:
> > 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.
> 
> Well, there is a reason only the 32bpp formats are supported.  With
> those you just adjust your color shifts and you are done.  No need to
> actually byte-swap.  In contrast handling 16bpp formats (5:6:5 or 5:5:5)
> with the non-native byte order is a PITA.

Yes, which is why I wanted to make the format endianness explicit from
the start so that you wouldn't have to guess whether you need to byte
swap or not.

> 
> The other reason of course is that this is the default format these
> days.
> 
> So, do any "other formats" exist where the byteswapped variant is used
> in practice?

I'm expecting people to move past 8bpc at some point. It's definitely
not enough for HDR stuff and whatnot. Even video content is already
moving to 10bpc.

So I think the question is better phrased as do mixed endian systems
exist? Or even if they do, does anyone care about them?

Also if someone goes and changes the DRM_FORMATs to follow host
endianness, they definitely have to figure out how to handle all
the YCbCr formats as well.

> Or can we just drop DRM_FORMAT_BIG_ENDIAN?
> 
> > 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.
> 
> Having two valid fourcc (DRM_FORMAT_XRGB8888 + (DRM_FORMAT_BGRX8888 |
> DRM_FORMAT_BIG_ENDIAN)) for the same format is confusing IMO.  And I
> doubt that it'll be properly implemented everywhere.

I think it would be if people actually handled any of the other formats.
It's a real shame these 8:8:8:8 formats were invented in the first place.
They allow people to be overly lazy and ignore endianness issues until
it's too late to fix things.

-- 
Ville Syrjälä
Intel OTC