Jocelyn Falempe <jfalempe@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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 ?
> {
> 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?).
> + 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.
Reviewed-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
