[PATCH v3 1/2] drm/panic: Add ABGR2101010 support

[Jocelyn Falempe]

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