| Message ID | 20240820090908.151042-2-jfalempe@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] drm/virtio: Use XRGB8888 also for big endian systems | expand |
Hi Am 20.08.24 um 11:07 schrieb Jocelyn Falempe: > The colors are inverted when testing a s390x VM on a s390x host. > Changing the conversion from DRM_FORMAT -> VIRTIO_GPU_FORMAT on big > endian guests fixes the colors. But it may break big-endian guest on > little-endian host. In this case, the fix should be in qemu, because > the host endianess is not known in the guest VM. > > Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> > Acked-by: Javier Martinez Canillas <javierm@redhat.com> > --- > drivers/gpu/drm/virtio/virtgpu_plane.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c > index 860b5757ec3fc..0ec6ecc96eb13 100644 > --- a/drivers/gpu/drm/virtio/virtgpu_plane.c > +++ b/drivers/gpu/drm/virtio/virtgpu_plane.c > @@ -37,16 +37,24 @@ static const uint32_t virtio_gpu_cursor_formats[] = { > DRM_FORMAT_ARGB8888, > }; > > +#ifdef __BIG_ENDIAN > +#define VIRTIO_GPU_HOST_XRGB8888 VIRTIO_GPU_FORMAT_X8R8G8B8_UNORM > +#define VIRTIO_GPU_HOST_ARGB8888 VIRTIO_GPU_FORMAT_A8R8G8B8_UNORM > +#else > +#define VIRTIO_GPU_HOST_XRGB8888 VIRTIO_GPU_FORMAT_B8G8R8X8_UNORM > +#define VIRTIO_GPU_HOST_ARGB8888 VIRTIO_GPU_FORMAT_B8G8R8A8_UNORM > +#endif As these defines are only used here, would it be beneficial to put the __BIG_ENDIAN branch directly around the switch statement? Best regards Thomas > + > uint32_t virtio_gpu_translate_format(uint32_t drm_fourcc) > { > uint32_t format; > > switch (drm_fourcc) { > case DRM_FORMAT_XRGB8888: > - format = VIRTIO_GPU_FORMAT_B8G8R8X8_UNORM; > + format = VIRTIO_GPU_HOST_XRGB8888; > break; > case DRM_FORMAT_ARGB8888: > - format = VIRTIO_GPU_FORMAT_B8G8R8A8_UNORM; > + format = VIRTIO_GPU_HOST_ARGB8888; > break; > default: > /*
On 20/08/2024 14:48, Thomas Zimmermann wrote: > Hi > > Am 20.08.24 um 11:07 schrieb Jocelyn Falempe: >> The colors are inverted when testing a s390x VM on a s390x host. >> Changing the conversion from DRM_FORMAT -> VIRTIO_GPU_FORMAT on big >> endian guests fixes the colors. But it may break big-endian guest on >> little-endian host. In this case, the fix should be in qemu, because >> the host endianess is not known in the guest VM. >> >> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> >> Acked-by: Javier Martinez Canillas <javierm@redhat.com> >> --- >> drivers/gpu/drm/virtio/virtgpu_plane.c | 12 ++++++++++-- >> 1 file changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c >> b/drivers/gpu/drm/virtio/virtgpu_plane.c >> index 860b5757ec3fc..0ec6ecc96eb13 100644 >> --- a/drivers/gpu/drm/virtio/virtgpu_plane.c >> +++ b/drivers/gpu/drm/virtio/virtgpu_plane.c >> @@ -37,16 +37,24 @@ static const uint32_t virtio_gpu_cursor_formats[] = { >> DRM_FORMAT_ARGB8888, >> }; >> +#ifdef __BIG_ENDIAN >> +#define VIRTIO_GPU_HOST_XRGB8888 VIRTIO_GPU_FORMAT_X8R8G8B8_UNORM >> +#define VIRTIO_GPU_HOST_ARGB8888 VIRTIO_GPU_FORMAT_A8R8G8B8_UNORM >> +#else >> +#define VIRTIO_GPU_HOST_XRGB8888 VIRTIO_GPU_FORMAT_B8G8R8X8_UNORM >> +#define VIRTIO_GPU_HOST_ARGB8888 VIRTIO_GPU_FORMAT_B8G8R8A8_UNORM >> +#endif > > As these defines are only used here, would it be beneficial to put the > __BIG_ENDIAN branch directly around the switch statement? That was my first version, but I found it difficult to read, when I mix #ifdef in a switch case. or maybe something like the following would be better ? 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; #else case DRM_FORMAT_XRGB8888: format = VIRTIO_GPU_FORMAT_B8G8R8X8_UNORM; break; case DRM_FORMAT_ARGB8888: format = VIRTIO_GPU_FORMAT_B8G8R8A8_UNORM; break; #endif > > Best regards > Thomas > >> + >> uint32_t virtio_gpu_translate_format(uint32_t drm_fourcc) >> { >> uint32_t format; >> switch (drm_fourcc) { >> case DRM_FORMAT_XRGB8888: >> - format = VIRTIO_GPU_FORMAT_B8G8R8X8_UNORM; >> + format = VIRTIO_GPU_HOST_XRGB8888; >> break; >> case DRM_FORMAT_ARGB8888: >> - format = VIRTIO_GPU_FORMAT_B8G8R8A8_UNORM; >> + format = VIRTIO_GPU_HOST_ARGB8888; >> break; >> default: >> /* >
Hi Am 20.08.24 um 14:55 schrieb Jocelyn Falempe: > On 20/08/2024 14:48, Thomas Zimmermann wrote: >> Hi >> >> Am 20.08.24 um 11:07 schrieb Jocelyn Falempe: >>> The colors are inverted when testing a s390x VM on a s390x host. >>> Changing the conversion from DRM_FORMAT -> VIRTIO_GPU_FORMAT on big >>> endian guests fixes the colors. But it may break big-endian guest on >>> little-endian host. In this case, the fix should be in qemu, because >>> the host endianess is not known in the guest VM. >>> >>> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> >>> Acked-by: Javier Martinez Canillas <javierm@redhat.com> >>> --- >>> drivers/gpu/drm/virtio/virtgpu_plane.c | 12 ++++++++++-- >>> 1 file changed, 10 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c >>> b/drivers/gpu/drm/virtio/virtgpu_plane.c >>> index 860b5757ec3fc..0ec6ecc96eb13 100644 >>> --- a/drivers/gpu/drm/virtio/virtgpu_plane.c >>> +++ b/drivers/gpu/drm/virtio/virtgpu_plane.c >>> @@ -37,16 +37,24 @@ static const uint32_t >>> virtio_gpu_cursor_formats[] = { >>> DRM_FORMAT_ARGB8888, >>> }; >>> +#ifdef __BIG_ENDIAN >>> +#define VIRTIO_GPU_HOST_XRGB8888 VIRTIO_GPU_FORMAT_X8R8G8B8_UNORM >>> +#define VIRTIO_GPU_HOST_ARGB8888 VIRTIO_GPU_FORMAT_A8R8G8B8_UNORM >>> +#else >>> +#define VIRTIO_GPU_HOST_XRGB8888 VIRTIO_GPU_FORMAT_B8G8R8X8_UNORM >>> +#define VIRTIO_GPU_HOST_ARGB8888 VIRTIO_GPU_FORMAT_B8G8R8A8_UNORM >>> +#endif >> >> As these defines are only used here, would it be beneficial to put >> the __BIG_ENDIAN branch directly around the switch statement? > > That was my first version, but I found it difficult to read, when I > mix #ifdef in a switch case. > > > or maybe something like the following would be better ? > > > 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; > #else > case DRM_FORMAT_XRGB8888: > format = VIRTIO_GPU_FORMAT_B8G8R8X8_UNORM; > break; > case DRM_FORMAT_ARGB8888: > format = VIRTIO_GPU_FORMAT_B8G8R8A8_UNORM; > break; > #endif I have no preference. Maybe the virtio devs can comment. Best regards Thomas >> >> Best regards >> Thomas >> >>> + >>> uint32_t virtio_gpu_translate_format(uint32_t drm_fourcc) >>> { >>> uint32_t format; >>> switch (drm_fourcc) { >>> case DRM_FORMAT_XRGB8888: >>> - format = VIRTIO_GPU_FORMAT_B8G8R8X8_UNORM; >>> + format = VIRTIO_GPU_HOST_XRGB8888; >>> break; >>> case DRM_FORMAT_ARGB8888: >>> - format = VIRTIO_GPU_FORMAT_B8G8R8A8_UNORM; >>> + format = VIRTIO_GPU_HOST_ARGB8888; >>> break; >>> default: >>> /* >> >
Jocelyn Falempe <jfalempe@redhat.com> writes: > On 20/08/2024 14:48, Thomas Zimmermann wrote: >> Hi >> >> Am 20.08.24 um 11:07 schrieb Jocelyn Falempe: >>> The colors are inverted when testing a s390x VM on a s390x host. >>> Changing the conversion from DRM_FORMAT -> VIRTIO_GPU_FORMAT on big >>> endian guests fixes the colors. But it may break big-endian guest on >>> little-endian host. In this case, the fix should be in qemu, because >>> the host endianess is not known in the guest VM. >>> >>> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> >>> Acked-by: Javier Martinez Canillas <javierm@redhat.com> >>> --- >>> drivers/gpu/drm/virtio/virtgpu_plane.c | 12 ++++++++++-- >>> 1 file changed, 10 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c >>> b/drivers/gpu/drm/virtio/virtgpu_plane.c >>> index 860b5757ec3fc..0ec6ecc96eb13 100644 >>> --- a/drivers/gpu/drm/virtio/virtgpu_plane.c >>> +++ b/drivers/gpu/drm/virtio/virtgpu_plane.c >>> @@ -37,16 +37,24 @@ static const uint32_t virtio_gpu_cursor_formats[] = { >>> DRM_FORMAT_ARGB8888, >>> }; >>> +#ifdef __BIG_ENDIAN >>> +#define VIRTIO_GPU_HOST_XRGB8888 VIRTIO_GPU_FORMAT_X8R8G8B8_UNORM >>> +#define VIRTIO_GPU_HOST_ARGB8888 VIRTIO_GPU_FORMAT_A8R8G8B8_UNORM >>> +#else >>> +#define VIRTIO_GPU_HOST_XRGB8888 VIRTIO_GPU_FORMAT_B8G8R8X8_UNORM >>> +#define VIRTIO_GPU_HOST_ARGB8888 VIRTIO_GPU_FORMAT_B8G8R8A8_UNORM >>> +#endif >> >> As these defines are only used here, would it be beneficial to put the >> __BIG_ENDIAN branch directly around the switch statement? > > That was my first version, but I found it difficult to read, when I mix > #ifdef in a switch case. > > > or maybe something like the following would be better ? > > > 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; > #else > case DRM_FORMAT_XRGB8888: > format = VIRTIO_GPU_FORMAT_B8G8R8X8_UNORM; > break; > case DRM_FORMAT_ARGB8888: > format = VIRTIO_GPU_FORMAT_B8G8R8A8_UNORM; > break; > #endif IMO your current patch is easier to read than having the ifdefery in the switch statement.
On Tue, Aug 20, 2024 at 11:07:41AM GMT, Jocelyn Falempe wrote: > The colors are inverted when testing a s390x VM on a s390x host. > Changing the conversion from DRM_FORMAT -> VIRTIO_GPU_FORMAT on big > endian guests fixes the colors. But it may break big-endian guest on > little-endian host. In this case, the fix should be in qemu, because > the host endianess is not known in the guest VM. > > Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com> > Acked-by: Javier Martinez Canillas <javierm@redhat.com> > --- > drivers/gpu/drm/virtio/virtgpu_plane.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c > index 860b5757ec3fc..0ec6ecc96eb13 100644 > --- a/drivers/gpu/drm/virtio/virtgpu_plane.c > +++ b/drivers/gpu/drm/virtio/virtgpu_plane.c > @@ -37,16 +37,24 @@ static const uint32_t virtio_gpu_cursor_formats[] = { > DRM_FORMAT_ARGB8888, > }; > > +#ifdef __BIG_ENDIAN > +#define VIRTIO_GPU_HOST_XRGB8888 VIRTIO_GPU_FORMAT_X8R8G8B8_UNORM > +#define VIRTIO_GPU_HOST_ARGB8888 VIRTIO_GPU_FORMAT_A8R8G8B8_UNORM > +#else > +#define VIRTIO_GPU_HOST_XRGB8888 VIRTIO_GPU_FORMAT_B8G8R8X8_UNORM > +#define VIRTIO_GPU_HOST_ARGB8888 VIRTIO_GPU_FORMAT_B8G8R8A8_UNORM > +#endif VIRTIO_GPU_FORMAT_* is little endian (like DRM_FORMAT_*), there should be no need to do anything byte order specific here. This looks like you are papering over a bug somewhere else. take care, Gerd
diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c index 860b5757ec3fc..0ec6ecc96eb13 100644 --- a/drivers/gpu/drm/virtio/virtgpu_plane.c +++ b/drivers/gpu/drm/virtio/virtgpu_plane.c @@ -37,16 +37,24 @@ static const uint32_t virtio_gpu_cursor_formats[] = { DRM_FORMAT_ARGB8888, }; +#ifdef __BIG_ENDIAN +#define VIRTIO_GPU_HOST_XRGB8888 VIRTIO_GPU_FORMAT_X8R8G8B8_UNORM +#define VIRTIO_GPU_HOST_ARGB8888 VIRTIO_GPU_FORMAT_A8R8G8B8_UNORM +#else +#define VIRTIO_GPU_HOST_XRGB8888 VIRTIO_GPU_FORMAT_B8G8R8X8_UNORM +#define VIRTIO_GPU_HOST_ARGB8888 VIRTIO_GPU_FORMAT_B8G8R8A8_UNORM +#endif + uint32_t virtio_gpu_translate_format(uint32_t drm_fourcc) { uint32_t format; switch (drm_fourcc) { case DRM_FORMAT_XRGB8888: - format = VIRTIO_GPU_FORMAT_B8G8R8X8_UNORM; + format = VIRTIO_GPU_HOST_XRGB8888; break; case DRM_FORMAT_ARGB8888: - format = VIRTIO_GPU_FORMAT_B8G8R8A8_UNORM; + format = VIRTIO_GPU_HOST_ARGB8888; break; default: /*