| Message ID | 20220315110707.628166-3-geert@linux-m68k.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Fix monochrome conversion for sdd130x | expand |
On 3/15/22 12:07, Geert Uytterhoeven wrote: > The conversion functions drm_fb_xrgb8888_to_mono() and > drm_fb_gray8_to_mono_line() do not behave correctly when the > horizontal boundaries of the clip rectangle are not multiples of 8: > a. When x1 % 8 != 0, the calculated pitch is not correct, > b. When x2 % 8 != 0, the pixel data for the last byte is wrong. > Thanks a lot for tracking down and fixing these issues. > Simplify the code and fix (a) by: > 1. Removing start_offset, and always storing the first pixel in the > first bit of the monochrome destination buffer. > Drivers that require the first pixel in a byte to be located at an > x-coordinate that is a multiple of 8 can always align the clip > rectangle before calling drm_fb_xrgb8888_to_mono(). > Note that: > - The ssd130x driver does not need the alignment, as the > monochrome buffer is a temporary format, > - The repaper driver always updates the full screen, so the clip > rectangle is always aligned. > 2. Passing the number of pixels to drm_fb_gray8_to_mono_line(), > instead of the number of bytes, and the number of pixels in the > last byte. > > Fix (b) by explicitly setting the target bit, instead of always setting > bit 7 and shifting the value in each loop iteration. > > Remove the bogus pitch check, which operates on bytes instead of pixels, > and triggers when e.g. flashing the cursor on a text console with a font > that is 8 pixels wide. > > Drop the confusing comment about scanlines, as a pitch in bytes always > contains a multiple of 8 pixels. > > While at it, use the drm_rect_height() helper instead of open-coding the > same operation. > > Update the comments accordingly. > > Fixes: bcf8b616deb87941 ("drm/format-helper: Add drm_fb_xrgb8888_to_mono_reversed()") > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> > --- Acked-by: Javier Martinez Canillas <javierm@redhat.com> I just have a small comment below. [snip] > +static void drm_fb_gray8_to_mono_line(u8 *dst, const u8 *src, unsigned int pixels) > +{ > + while (pixels) { > + unsigned int i, bits = min(pixels, 8U); > + u8 byte = 0; > > - byte >>= 1; > - if (src[x] >> 7) > - byte |= BIT(7); > + for (i = 0; i < bits; i++, pixels--) { I think is worth to add a comment here explaining that the pixel is set to 1 for brightness > 127 and to 0 for brightness < 128. Or as kernel-doc for this helper function. > + if (*src++ & BIT(7)) Pekka also mentioned that if (*src++ > 127) would make this easier to read. > + byte |= BIT(i); > } > *dst++ = byte; > }
Hi Javier, On Tue, Mar 15, 2022 at 1:18 PM Javier Martinez Canillas <javierm@redhat.com> wrote: > On 3/15/22 12:07, Geert Uytterhoeven wrote: > > The conversion functions drm_fb_xrgb8888_to_mono() and > > drm_fb_gray8_to_mono_line() do not behave correctly when the > > horizontal boundaries of the clip rectangle are not multiples of 8: > > a. When x1 % 8 != 0, the calculated pitch is not correct, > > b. When x2 % 8 != 0, the pixel data for the last byte is wrong. > > > > Thanks a lot for tracking down and fixing these issues. > > > Simplify the code and fix (a) by: > > 1. Removing start_offset, and always storing the first pixel in the > > first bit of the monochrome destination buffer. > > Drivers that require the first pixel in a byte to be located at an > > x-coordinate that is a multiple of 8 can always align the clip > > rectangle before calling drm_fb_xrgb8888_to_mono(). > > Note that: > > - The ssd130x driver does not need the alignment, as the > > monochrome buffer is a temporary format, > > - The repaper driver always updates the full screen, so the clip > > rectangle is always aligned. > > 2. Passing the number of pixels to drm_fb_gray8_to_mono_line(), > > instead of the number of bytes, and the number of pixels in the > > last byte. > > > > Fix (b) by explicitly setting the target bit, instead of always setting > > bit 7 and shifting the value in each loop iteration. > > > > Remove the bogus pitch check, which operates on bytes instead of pixels, > > and triggers when e.g. flashing the cursor on a text console with a font > > that is 8 pixels wide. > > > > Drop the confusing comment about scanlines, as a pitch in bytes always > > contains a multiple of 8 pixels. > > > > While at it, use the drm_rect_height() helper instead of open-coding the > > same operation. > > > > Update the comments accordingly. > > > > Fixes: bcf8b616deb87941 ("drm/format-helper: Add drm_fb_xrgb8888_to_mono_reversed()") > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> > > --- > > Acked-by: Javier Martinez Canillas <javierm@redhat.com> Thanks! > I just have a small comment below. > > [snip] > > > +static void drm_fb_gray8_to_mono_line(u8 *dst, const u8 *src, unsigned int pixels) > > +{ > > + while (pixels) { > > + unsigned int i, bits = min(pixels, 8U); > > + u8 byte = 0; > > > > - byte >>= 1; > > - if (src[x] >> 7) > > - byte |= BIT(7); > > + for (i = 0; i < bits; i++, pixels--) { > > I think is worth to add a comment here explaining that the pixel is set to > 1 for brightness > 127 and to 0 for brightness < 128. Or as kernel-doc for > this helper function. > > > + if (*src++ & BIT(7)) > > Pekka also mentioned that if (*src++ > 127) would make this easier to read. Sure, will update. Nicely removes the need for a comment. > > + byte |= BIT(i); > > } > > *dst++ = byte; > > } Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Tue, Mar 15, 2022 at 12:07:04PM +0100, Geert Uytterhoeven wrote: > The conversion functions drm_fb_xrgb8888_to_mono() and > drm_fb_gray8_to_mono_line() do not behave correctly when the > horizontal boundaries of the clip rectangle are not multiples of 8: > a. When x1 % 8 != 0, the calculated pitch is not correct, > b. When x2 % 8 != 0, the pixel data for the last byte is wrong. > > Simplify the code and fix (a) by: > 1. Removing start_offset, and always storing the first pixel in the > first bit of the monochrome destination buffer. > Drivers that require the first pixel in a byte to be located at an > x-coordinate that is a multiple of 8 can always align the clip > rectangle before calling drm_fb_xrgb8888_to_mono(). > Note that: > - The ssd130x driver does not need the alignment, as the > monochrome buffer is a temporary format, > - The repaper driver always updates the full screen, so the clip > rectangle is always aligned. > 2. Passing the number of pixels to drm_fb_gray8_to_mono_line(), > instead of the number of bytes, and the number of pixels in the > last byte. > > Fix (b) by explicitly setting the target bit, instead of always setting > bit 7 and shifting the value in each loop iteration. > > Remove the bogus pitch check, which operates on bytes instead of pixels, > and triggers when e.g. flashing the cursor on a text console with a font > that is 8 pixels wide. > > Drop the confusing comment about scanlines, as a pitch in bytes always > contains a multiple of 8 pixels. > > While at it, use the drm_rect_height() helper instead of open-coding the > same operation. > > Update the comments accordingly. Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> See comment below. > Fixes: bcf8b616deb87941 ("drm/format-helper: Add drm_fb_xrgb8888_to_mono_reversed()") > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> > --- > I tried hard to fix this in small steps, but everything was no > intertangled that this turned out to be unfeasible. > > Note that making these changes does not introduce regressions in the > ssd130x driver, as the latter is broken for x1 != 0 or y1 != 0 anyway. > --- > drivers/gpu/drm/drm_format_helper.c | 56 ++++++++++------------------- > 1 file changed, 18 insertions(+), 38 deletions(-) > > diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c > index b68aa857c6514529..0d7cae921ed1134f 100644 > --- a/drivers/gpu/drm/drm_format_helper.c > +++ b/drivers/gpu/drm/drm_format_helper.c > @@ -594,27 +594,16 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for > } > EXPORT_SYMBOL(drm_fb_blit_toio); > > -static void drm_fb_gray8_to_mono_line(u8 *dst, const u8 *src, unsigned int pixels, > - unsigned int start_offset, unsigned int end_len) > -{ > - unsigned int xb, i; > - > - for (xb = 0; xb < pixels; xb++) { > - unsigned int start = 0, end = 8; > - u8 byte = 0x00; > - > - if (xb == 0 && start_offset) > - start = start_offset; > > - if (xb == pixels - 1 && end_len) > - end = end_len; > - > - for (i = start; i < end; i++) { > - unsigned int x = xb * 8 + i; > +static void drm_fb_gray8_to_mono_line(u8 *dst, const u8 *src, unsigned int pixels) > +{ > + while (pixels) { > + unsigned int i, bits = min(pixels, 8U); > + u8 byte = 0; > > - byte >>= 1; > - if (src[x] >> 7) > - byte |= BIT(7); > + for (i = 0; i < bits; i++, pixels--) { > + if (*src++ & BIT(7)) > + byte |= BIT(i); > } > *dst++ = byte; > } > @@ -634,16 +623,22 @@ static void drm_fb_gray8_to_mono_line(u8 *dst, const u8 *src, unsigned int pixel > * > * This function uses drm_fb_xrgb8888_to_gray8() to convert to grayscale and > * then the result is converted from grayscale to monochrome. > + * > + * The first pixel (upper left corner of the clip rectangle) will be converted > + * and copied to the first bit (LSB) in the first byte of the monochrome > + * destination buffer. > + * If the caller requires that the first pixel in a byte must be located at an > + * x-coordinate that is a multiple of 8, then the caller must take care itself > + * of supplying a suitable clip rectangle. > */ > void drm_fb_xrgb8888_to_mono(void *dst, unsigned int dst_pitch, const void *vaddr, > const struct drm_framebuffer *fb, const struct drm_rect *clip) > { > unsigned int linepixels = drm_rect_width(clip); > - unsigned int lines = clip->y2 - clip->y1; > + unsigned int lines = drm_rect_height(clip); > unsigned int cpp = fb->format->cpp[0]; > unsigned int len_src32 = linepixels * cpp; > struct drm_device *dev = fb->dev; > - unsigned int start_offset, end_len; > unsigned int y; > u8 *mono = dst, *gray8; > u32 *src32; > @@ -652,14 +647,11 @@ void drm_fb_xrgb8888_to_mono(void *dst, unsigned int dst_pitch, const void *vadd > return; > > /* > - * The mono destination buffer contains 1 bit per pixel and > - * destination scanlines have to be in multiple of 8 pixels. > + * The mono destination buffer contains 1 bit per pixel > */ Now it's one-line comment. > if (!dst_pitch) > dst_pitch = DIV_ROUND_UP(linepixels, 8); > > - drm_WARN_ONCE(dev, dst_pitch % 8 != 0, "dst_pitch is not a multiple of 8\n"); > - > /* > * The cma memory is write-combined so reads are uncached. > * Speed up by fetching one line at a time. > @@ -677,23 +669,11 @@ void drm_fb_xrgb8888_to_mono(void *dst, unsigned int dst_pitch, const void *vadd > > gray8 = (u8 *)src32 + len_src32; > > - /* > - * For damage handling, it is possible that only parts of the source > - * buffer is copied and this could lead to start and end pixels that > - * are not aligned to multiple of 8. > - * > - * Calculate if the start and end pixels are not aligned and set the > - * offsets for the mono line conversion function to adjust. > - */ > - start_offset = clip->x1 % 8; > - end_len = clip->x2 % 8; > - > vaddr += clip_offset(clip, fb->pitches[0], cpp); > for (y = 0; y < lines; y++) { > src32 = memcpy(src32, vaddr, len_src32); > drm_fb_xrgb8888_to_gray8_line(gray8, src32, linepixels); > - drm_fb_gray8_to_mono_line(mono, gray8, dst_pitch, start_offset, > - end_len); > + drm_fb_gray8_to_mono_line(mono, gray8, linepixels); Yep, with previously being on one line here will be less churn. > vaddr += fb->pitches[0]; > mono += dst_pitch; > } > -- > 2.25.1 >
On Tue, Mar 15, 2022 at 01:18:00PM +0100, Javier Martinez Canillas wrote: > On 3/15/22 12:07, Geert Uytterhoeven wrote: > > + for (i = 0; i < bits; i++, pixels--) { > > I think is worth to add a comment here explaining that the pixel is set to > 1 for brightness > 127 and to 0 for brightness < 128. Or as kernel-doc for > this helper function. > > > + if (*src++ & BIT(7)) > > Pekka also mentioned that if (*src++ > 127) would make this easier to read. >= 128 ? > > + byte |= BIT(i); > > } > > *dst++ = byte; > > }
diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c index b68aa857c6514529..0d7cae921ed1134f 100644 --- a/drivers/gpu/drm/drm_format_helper.c +++ b/drivers/gpu/drm/drm_format_helper.c @@ -594,27 +594,16 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for } EXPORT_SYMBOL(drm_fb_blit_toio); -static void drm_fb_gray8_to_mono_line(u8 *dst, const u8 *src, unsigned int pixels, - unsigned int start_offset, unsigned int end_len) -{ - unsigned int xb, i; - - for (xb = 0; xb < pixels; xb++) { - unsigned int start = 0, end = 8; - u8 byte = 0x00; - - if (xb == 0 && start_offset) - start = start_offset; - if (xb == pixels - 1 && end_len) - end = end_len; - - for (i = start; i < end; i++) { - unsigned int x = xb * 8 + i; +static void drm_fb_gray8_to_mono_line(u8 *dst, const u8 *src, unsigned int pixels) +{ + while (pixels) { + unsigned int i, bits = min(pixels, 8U); + u8 byte = 0; - byte >>= 1; - if (src[x] >> 7) - byte |= BIT(7); + for (i = 0; i < bits; i++, pixels--) { + if (*src++ & BIT(7)) + byte |= BIT(i); } *dst++ = byte; } @@ -634,16 +623,22 @@ static void drm_fb_gray8_to_mono_line(u8 *dst, const u8 *src, unsigned int pixel * * This function uses drm_fb_xrgb8888_to_gray8() to convert to grayscale and * then the result is converted from grayscale to monochrome. + * + * The first pixel (upper left corner of the clip rectangle) will be converted + * and copied to the first bit (LSB) in the first byte of the monochrome + * destination buffer. + * If the caller requires that the first pixel in a byte must be located at an + * x-coordinate that is a multiple of 8, then the caller must take care itself + * of supplying a suitable clip rectangle. */ void drm_fb_xrgb8888_to_mono(void *dst, unsigned int dst_pitch, const void *vaddr, const struct drm_framebuffer *fb, const struct drm_rect *clip) { unsigned int linepixels = drm_rect_width(clip); - unsigned int lines = clip->y2 - clip->y1; + unsigned int lines = drm_rect_height(clip); unsigned int cpp = fb->format->cpp[0]; unsigned int len_src32 = linepixels * cpp; struct drm_device *dev = fb->dev; - unsigned int start_offset, end_len; unsigned int y; u8 *mono = dst, *gray8; u32 *src32; @@ -652,14 +647,11 @@ void drm_fb_xrgb8888_to_mono(void *dst, unsigned int dst_pitch, const void *vadd return; /* - * The mono destination buffer contains 1 bit per pixel and - * destination scanlines have to be in multiple of 8 pixels. + * The mono destination buffer contains 1 bit per pixel */ if (!dst_pitch) dst_pitch = DIV_ROUND_UP(linepixels, 8); - drm_WARN_ONCE(dev, dst_pitch % 8 != 0, "dst_pitch is not a multiple of 8\n"); - /* * The cma memory is write-combined so reads are uncached. * Speed up by fetching one line at a time. @@ -677,23 +669,11 @@ void drm_fb_xrgb8888_to_mono(void *dst, unsigned int dst_pitch, const void *vadd gray8 = (u8 *)src32 + len_src32; - /* - * For damage handling, it is possible that only parts of the source - * buffer is copied and this could lead to start and end pixels that - * are not aligned to multiple of 8. - * - * Calculate if the start and end pixels are not aligned and set the - * offsets for the mono line conversion function to adjust. - */ - start_offset = clip->x1 % 8; - end_len = clip->x2 % 8; - vaddr += clip_offset(clip, fb->pitches[0], cpp); for (y = 0; y < lines; y++) { src32 = memcpy(src32, vaddr, len_src32); drm_fb_xrgb8888_to_gray8_line(gray8, src32, linepixels); - drm_fb_gray8_to_mono_line(mono, gray8, dst_pitch, start_offset, - end_len); + drm_fb_gray8_to_mono_line(mono, gray8, linepixels); vaddr += fb->pitches[0]; mono += dst_pitch; }
The conversion functions drm_fb_xrgb8888_to_mono() and drm_fb_gray8_to_mono_line() do not behave correctly when the horizontal boundaries of the clip rectangle are not multiples of 8: a. When x1 % 8 != 0, the calculated pitch is not correct, b. When x2 % 8 != 0, the pixel data for the last byte is wrong. Simplify the code and fix (a) by: 1. Removing start_offset, and always storing the first pixel in the first bit of the monochrome destination buffer. Drivers that require the first pixel in a byte to be located at an x-coordinate that is a multiple of 8 can always align the clip rectangle before calling drm_fb_xrgb8888_to_mono(). Note that: - The ssd130x driver does not need the alignment, as the monochrome buffer is a temporary format, - The repaper driver always updates the full screen, so the clip rectangle is always aligned. 2. Passing the number of pixels to drm_fb_gray8_to_mono_line(), instead of the number of bytes, and the number of pixels in the last byte. Fix (b) by explicitly setting the target bit, instead of always setting bit 7 and shifting the value in each loop iteration. Remove the bogus pitch check, which operates on bytes instead of pixels, and triggers when e.g. flashing the cursor on a text console with a font that is 8 pixels wide. Drop the confusing comment about scanlines, as a pitch in bytes always contains a multiple of 8 pixels. While at it, use the drm_rect_height() helper instead of open-coding the same operation. Update the comments accordingly. Fixes: bcf8b616deb87941 ("drm/format-helper: Add drm_fb_xrgb8888_to_mono_reversed()") Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> --- I tried hard to fix this in small steps, but everything was no intertangled that this turned out to be unfeasible. Note that making these changes does not introduce regressions in the ssd130x driver, as the latter is broken for x1 != 0 or y1 != 0 anyway. --- drivers/gpu/drm/drm_format_helper.c | 56 ++++++++++------------------- 1 file changed, 18 insertions(+), 38 deletions(-)