| Message ID | 20240531080750.765982-2-jfalempe@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/panic: Add a kmsg dump screen | expand |
Jocelyn Falempe <jfalempe@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@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 ? > { > 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@redhat.com>
On 31/05/2024 11:15, Javier Martinez Canillas wrote: > Jocelyn Falempe <jfalempe@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@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@redhat.com> > Thanks for the reviews, -- Jocelyn
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) { 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++) + 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; for (y = 0; y < height; y++) { for (x = 0; x < width; x++) { u32 off = y * dpitch + x * 3; - val32 = (sbuf8[(y * spitch) + x / 8] & (0x80 >> (x % 8))) ? fg32 : bg32; - - /* write blue-green-red to output in little endianness */ - iosys_map_wr(dmap, off, u8, (val32 & 0x000000FF) >> 0); - iosys_map_wr(dmap, off + 1, u8, (val32 & 0x0000FF00) >> 8); - iosys_map_wr(dmap, off + 2, u8, (val32 & 0x00FF0000) >> 16); + if (sbuf8[(y * spitch) + x / 8] & (0x80 >> (x % 8))) { + /* write blue-green-red to output in little endianness */ + iosys_map_wr(dmap, off, u8, (color & 0x000000FF) >> 0); + iosys_map_wr(dmap, off + 1, u8, (color & 0x0000FF00) >> 8); + iosys_map_wr(dmap, off + 2, u8, (color & 0x00FF0000) >> 16); + } } } } @@ -235,17 +231,14 @@ static void drm_panic_blit24(struct iosys_map *dmap, unsigned int dpitch, static void drm_panic_blit32(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; - for (y = 0; y < height; y++) { - for (x = 0; x < width; x++) { - val32 = (sbuf8[(y * spitch) + x / 8] & (0x80 >> (x % 8))) ? fg32 : bg32; - iosys_map_wr(dmap, y * dpitch + x * sizeof(u32), u32, val32); - } - } + for (y = 0; y < height; y++) + for (x = 0; x < width; x++) + if (sbuf8[(y * spitch) + x / 8] & (0x80 >> (x % 8))) + iosys_map_wr(dmap, y * dpitch + x * sizeof(u32), u32, color); } /* @@ -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 * @pixel_width: pixel width in bytes. * * This can be used to draw a font character, which is a monochrome image, to a @@ -266,21 +258,20 @@ static void drm_panic_blit32(struct iosys_map *dmap, unsigned int dpitch, static void drm_panic_blit(struct iosys_map *dmap, unsigned int dpitch, const u8 *sbuf8, unsigned int spitch, unsigned int height, unsigned int width, - u32 fg_color, u32 bg_color, - unsigned int pixel_width) + u32 color, unsigned int pixel_width) { switch (pixel_width) { case 2: drm_panic_blit16(dmap, dpitch, sbuf8, spitch, - height, width, fg_color, bg_color); + height, width, color); break; case 3: drm_panic_blit24(dmap, dpitch, sbuf8, spitch, - height, width, fg_color, bg_color); + height, width, color); break; case 4: drm_panic_blit32(dmap, dpitch, sbuf8, spitch, - height, width, fg_color, bg_color); + height, width, color); break; default: WARN_ONCE(1, "Can't blit with pixel width %d\n", pixel_width); @@ -381,8 +372,7 @@ static void draw_txt_rectangle(struct drm_scanout_buffer *sb, unsigned int msg_lines, bool centered, struct drm_rect *clip, - u32 fg_color, - u32 bg_color) + u32 color) { int i, j; const u8 *src; @@ -404,8 +394,7 @@ static void draw_txt_rectangle(struct drm_scanout_buffer *sb, for (j = 0; j < line_len; j++) { src = get_char_bitmap(font, msg[i].txt[j], font_pitch); drm_panic_blit(&dst, sb->pitch[0], src, font_pitch, - font->height, font->width, - fg_color, bg_color, px_width); + font->height, font->width, color, px_width); iosys_map_incr(&dst, font->width * px_width); } } @@ -445,9 +434,9 @@ static void draw_panic_static(struct drm_scanout_buffer *sb) if ((r_msg.x1 >= drm_rect_width(&r_logo) || r_msg.y1 >= drm_rect_height(&r_logo)) && drm_rect_width(&r_logo) < sb->width && drm_rect_height(&r_logo) < sb->height) { - draw_txt_rectangle(sb, font, logo, logo_lines, false, &r_logo, fg_color, bg_color); + draw_txt_rectangle(sb, font, logo, logo_lines, false, &r_logo, fg_color); } - draw_txt_rectangle(sb, font, panic_msg, msg_lines, true, &r_msg, fg_color, bg_color); + draw_txt_rectangle(sb, font, panic_msg, msg_lines, true, &r_msg, fg_color); } /*
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@redhat.com> --- drivers/gpu/drm/drm_panic.c | 63 +++++++++++++++---------------------- 1 file changed, 26 insertions(+), 37 deletions(-)