[PATCH 1/3] drm/panic: only draw the foreground color in drm_panic_blit()

[Jocelyn Falempe]

On 31/05/2024 11:15, Javier Martinez Canillas wrote:
> Jocelyn Falempe <jfalempe at redhat.com> writes:
> 
> Hello Jocelyn,
> 
>> The whole framebuffer is cleared, so it's useless to rewrite the
>> background colored pixels. It allows to simplify the drawing
>> functions, and prepare the work for the set_pixel() callback.
>>
>> Signed-off-by: Jocelyn Falempe <jfalempe at redhat.com>
>> ---
>>   drivers/gpu/drm/drm_panic.c | 63 +++++++++++++++----------------------
>>   1 file changed, 26 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
>> index 7ece67086cec..9d95c7eaae83 100644
>> --- a/drivers/gpu/drm/drm_panic.c
>> +++ b/drivers/gpu/drm/drm_panic.c
>> @@ -197,37 +197,33 @@ static u32 convert_from_xrgb8888(u32 color, u32 format)
>>   static void drm_panic_blit16(struct iosys_map *dmap, unsigned int dpitch,
>>   			     const u8 *sbuf8, unsigned int spitch,
>>   			     unsigned int height, unsigned int width,
>> -			     u16 fg16, u16 bg16)
>> +			     u16 color)
> 
> What about calling this fg16 instead of color? That way is clear that only
> the fb is written and not the background ?

Yes I can keep the fg16 name.
> 
>>   {
>>   	unsigned int y, x;
>> -	u16 val16;
>>   
>> -	for (y = 0; y < height; y++) {
>> -		for (x = 0; x < width; x++) {
>> -			val16 = (sbuf8[(y * spitch) + x / 8] & (0x80 >> (x % 8))) ? fg16 : bg16;
>> -			iosys_map_wr(dmap, y * dpitch + x * sizeof(u16), u16, val16);
>> -		}
>> -	}
>> +	for (y = 0; y < height; y++)
>> +		for (x = 0; x < width; x++)
> 
> I would add here a comment that this check is about determining if a color
> is suitable for foreground or background, depending on the luminance
> threshold (which I understand is the 0x80 value?).

The source buffer is monochrome, so store 8 pixels per byte.
the (0x80 >> (x % 8)) is a bit mask, to check if the source pixel is 
foreground or background. I will add a comment about this, to make it clear.

> 
>> +			if (sbuf8[(y * spitch) + x / 8] & (0x80 >> (x % 8)))
>> +				iosys_map_wr(dmap, y * dpitch + x * sizeof(u16), u16, color);
>>   }
>>   
>>   static void drm_panic_blit24(struct iosys_map *dmap, unsigned int dpitch,
>>   			     const u8 *sbuf8, unsigned int spitch,
>>   			     unsigned int height, unsigned int width,
>> -			     u32 fg32, u32 bg32)
>> +			     u32 color)
>>   {
>>   	unsigned int y, x;
>> -	u32 val32;
>>
> 
> Same here, I would left the variable name as fg32.
> 
> [...]
> 
> and also here would add a comment or use a variable to make it more readable.
> 
> Same comments for drm_panic_blit32().
> 
> [...]
> 
>>   /*
>> @@ -256,8 +249,7 @@ static void drm_panic_blit32(struct iosys_map *dmap, unsigned int dpitch,
>>    * @spitch: source pitch in bytes
>>    * @height: height of the image to copy, in pixels
>>    * @width: width of the image to copy, in pixels
>> - * @fg_color: foreground color, in destination format
>> - * @bg_color: background color, in destination format
>> + * @color: foreground color, in destination format
> 
> Leaving as fg_color would even be consistent with your comment.
> 
> Feel free to ignore my comments though if you disagree, the patch looks
> good to me regardless.

sure I will keep the fg_* name
> 
> Reviewed-by: Javier Martinez Canillas <javierm at redhat.com>
> 

Thanks for the reviews,

--

Jocelyn