Nouveau - Sep 2024 - [PATCH v3 1/2] drm/panic: Add ABGR2101010 support

Jocelyn Falempe <jfalempe at redhat.com> writes:

Hello Jocelyn,
> Add support for ABGR2101010, used by the nouveau driver.
>
> Signed-off-by: Jocelyn Falempe <jfalempe at redhat.com>
> ---
>  drivers/gpu/drm/drm_panic.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
> index 74412b7bf936..0a9ecc1380d2 100644
> --- a/drivers/gpu/drm/drm_panic.c
> +++ b/drivers/gpu/drm/drm_panic.c
> @@ -209,6 +209,14 @@ static u32 convert_xrgb8888_to_argb2101010(u32 pix)
>  	return GENMASK(31, 30) /* set alpha bits */ | pix | ((pix >> 8)
& 0x00300C03);
>  }
>  
> +static u32 convert_xrgb8888_to_abgr2101010(u32 pix)
> +{
> +	pix = ((pix & 0x00FF0000) >> 14) |
> +	      ((pix & 0x0000FF00) << 4) |
> +	      ((pix & 0x000000FF) << 22);
> +	return GENMASK(31, 30) /* set alpha bits */ | pix | ((pix >> 8)
& 0x00300C03);
> +}
Maybe we can move this format conversion helper and the others in the
driver to drivers/gpu/drm/drm_format_helper.c ?
> +
>  /*
>   * convert_from_xrgb8888 - convert one pixel from xrgb8888 to the desired
format
>   * @color: input color, in xrgb8888 format
> @@ -242,6 +250,8 @@ static u32 convert_from_xrgb8888(u32 color, u32 format)
>  		return convert_xrgb8888_to_xrgb2101010(color);
>  	case DRM_FORMAT_ARGB2101010:
>  		return convert_xrgb8888_to_argb2101010(color);
> +	case DRM_FORMAT_ABGR2101010:
> +		return convert_xrgb8888_to_abgr2101010(color);
>  	default:
>  		WARN_ONCE(1, "Can't convert to %p4cc\n", &format);
>  		return 0;
> -- 
> 2.46.0
>

The patch looks good to me.

Reviewed-by: Javier Martinez Canillas <javierm at redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

On 13/09/2024 09:22, Javier Martinez Canillas wrote:
> Jocelyn Falempe <jfalempe at redhat.com> writes:
> 
> Hello Jocelyn,
> 
>> Add support for ABGR2101010, used by the nouveau driver.
>>
>> Signed-off-by: Jocelyn Falempe <jfalempe at redhat.com>
>> ---
>>   drivers/gpu/drm/drm_panic.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
>> index 74412b7bf936..0a9ecc1380d2 100644
>> --- a/drivers/gpu/drm/drm_panic.c
>> +++ b/drivers/gpu/drm/drm_panic.c
>> @@ -209,6 +209,14 @@ static u32 convert_xrgb8888_to_argb2101010(u32
pix)
>>   	return GENMASK(31, 30) /* set alpha bits */ | pix | ((pix >>
8) & 0x00300C03);
>>   }
>>   
>> +static u32 convert_xrgb8888_to_abgr2101010(u32 pix)
>> +{
>> +	pix = ((pix & 0x00FF0000) >> 14) |
>> +	      ((pix & 0x0000FF00) << 4) |
>> +	      ((pix & 0x000000FF) << 22);
>> +	return GENMASK(31, 30) /* set alpha bits */ | pix | ((pix >> 8)
& 0x00300C03);
>> +}
> 
> Maybe we can move this format conversion helper and the others in the
> driver to drivers/gpu/drm/drm_format_helper.c ?
I think there are still a few issues with that. First is that 
drm_format_helper.c is in a separate module, so you can't call its 
functions from the main drm module, where drm_panic is.

In my drm_log series, https://patchwork.freedesktop.org/series/136789/ I 
moved this to drm_draw.c, and maybe drm_format_helper could re-use that ?
> 
>> +
>>   /*
>>    * convert_from_xrgb8888 - convert one pixel from xrgb8888 to the
desired format
>>    * @color: input color, in xrgb8888 format
>> @@ -242,6 +250,8 @@ static u32 convert_from_xrgb8888(u32 color, u32
format)
>>   		return convert_xrgb8888_to_xrgb2101010(color);
>>   	case DRM_FORMAT_ARGB2101010:
>>   		return convert_xrgb8888_to_argb2101010(color);
>> +	case DRM_FORMAT_ABGR2101010:
>> +		return convert_xrgb8888_to_abgr2101010(color);
>>   	default:
>>   		WARN_ONCE(1, "Can't convert to %p4cc\n",
&format);
>>   		return 0;
>> -- 
>> 2.46.0
>>
> 
> 
> The patch looks good to me.
> 
> Reviewed-by: Javier Martinez Canillas <javierm at redhat.com>
>