Re: [PATCH v2 2/2] drm/virtio: Fix host color format for big endian guests

[Javier Martinez Canillas <javierm@redhat.com>]

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.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat