Jocelyn Falempe <[email protected]> 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 <[email protected]> > --- > 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 <[email protected]> -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
