[PATCH 0/5] drm: Fix monochrome conversion for sdd130x

[PATCH 0/5] drm: Fix monochrome conversion for sdd130x

[Geert Uytterhoeven]

	Hi all,

This patch series contains fixes and improvements for the XRGB888 to
monochrome conversion in the DRM core, and for its users.

This has been tested on an Adafruit FeatherWing 128x32 OLED, connected
to an OrangeCrab ECP5 FPGA board running a 64 MHz VexRiscv RISC-V
softcore, using a text console with 7x14 and 8x8 fonts.

Thanks for your comments!

Geert Uytterhoeven (5):
  drm/format-helper: Rename drm_fb_xrgb8888_to_mono_reversed()
  drm/format-helper: Fix XRGB888 to monochrome conversion
  drm: ssd130x: Fix rectangle updates
  drm: ssd130x: Reduce temporary buffer sizes
  drm/repaper: Reduce temporary buffer size in repaper_fb_dirty()

 drivers/gpu/drm/drm_format_helper.c | 74 +++++++++++------------------
 drivers/gpu/drm/solomon/ssd130x.c   | 24 +++++++---
 drivers/gpu/drm/tiny/repaper.c      |  4 +-
 include/drm/drm_format_helper.h     |  5 +-
 4 files changed, 48 insertions(+), 59 deletions(-)

-- 
2.25.1

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

[PATCH 1/5] drm/format-helper: Rename drm_fb_xrgb8888_to_mono_reversed()

[Geert Uytterhoeven]

There is no "reversed" handling in drm_fb_xrgb8888_to_mono_reversed():
the function just converts from color to grayscale, and reduces the
number of grayscale levels from 256 to 2 (i.e. brightness 0-127 is
mapped to 0, 128-255 to 1).  All "reversed" handling is done in the
repaper driver, where this function originated.

Hence make this clear by renaming drm_fb_xrgb8888_to_mono_reversed() to
drm_fb_xrgb8888_to_mono(), and documenting the black/white pixel
mapping.

Fixes: bcf8b616deb87941 ("drm/format-helper: Add drm_fb_xrgb8888_to_mono_reversed()")
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
 drivers/gpu/drm/drm_format_helper.c | 32 ++++++++++++++---------------
 drivers/gpu/drm/solomon/ssd130x.c   |  2 +-
 drivers/gpu/drm/tiny/repaper.c      |  2 +-
 include/drm/drm_format_helper.h     |  5 ++---
 4 files changed, 20 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
index bc0f49773868a9b0..b68aa857c6514529 100644
--- a/drivers/gpu/drm/drm_format_helper.c
+++ b/drivers/gpu/drm/drm_format_helper.c
@@ -594,8 +594,8 @@ 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_reversed_line(u8 *dst, const u8 *src, unsigned int pixels,
-					       unsigned int start_offset, unsigned int end_len)
+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;
 
@@ -621,8 +621,8 @@ static void drm_fb_gray8_to_mono_reversed_line(u8 *dst, const u8 *src, unsigned
 }
 
 /**
- * drm_fb_xrgb8888_to_mono_reversed - Convert XRGB8888 to reversed monochrome
- * @dst: reversed monochrome destination buffer
+ * drm_fb_xrgb8888_to_mono - Convert XRGB8888 to monochrome
+ * @dst: monochrome destination buffer (0=black, 1=white)
  * @dst_pitch: Number of bytes between two consecutive scanlines within dst
  * @src: XRGB8888 source buffer
  * @fb: DRM framebuffer
@@ -633,10 +633,10 @@ static void drm_fb_gray8_to_mono_reversed_line(u8 *dst, const u8 *src, unsigned
  * and use this function to convert to the native format.
  *
  * This function uses drm_fb_xrgb8888_to_gray8() to convert to grayscale and
- * then the result is converted from grayscale to reversed monohrome.
+ * then the result is converted from grayscale to monochrome.
  */
-void drm_fb_xrgb8888_to_mono_reversed(void *dst, unsigned int dst_pitch, const void *vaddr,
-				      const struct drm_framebuffer *fb, const struct drm_rect *clip)
+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;
@@ -652,8 +652,8 @@ void drm_fb_xrgb8888_to_mono_reversed(void *dst, unsigned int dst_pitch, const v
 		return;
 
 	/*
-	 * The reversed 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 and
+	 * destination scanlines have to be in multiple of 8 pixels.
 	 */
 	if (!dst_pitch)
 		dst_pitch = DIV_ROUND_UP(linepixels, 8);
@@ -664,9 +664,9 @@ void drm_fb_xrgb8888_to_mono_reversed(void *dst, unsigned int dst_pitch, const v
 	 * The cma memory is write-combined so reads are uncached.
 	 * Speed up by fetching one line at a time.
 	 *
-	 * Also, format conversion from XR24 to reversed monochrome
-	 * are done line-by-line but are converted to 8-bit grayscale
-	 * as an intermediate step.
+	 * Also, format conversion from XR24 to monochrome are done
+	 * line-by-line but are converted to 8-bit grayscale as an
+	 * intermediate step.
 	 *
 	 * Allocate a buffer to be used for both copying from the cma
 	 * memory and to store the intermediate grayscale line pixels.
@@ -683,7 +683,7 @@ void drm_fb_xrgb8888_to_mono_reversed(void *dst, unsigned int dst_pitch, const v
 	 * are not aligned to multiple of 8.
 	 *
 	 * Calculate if the start and end pixels are not aligned and set the
-	 * offsets for the reversed mono line conversion function to adjust.
+	 * offsets for the mono line conversion function to adjust.
 	 */
 	start_offset = clip->x1 % 8;
 	end_len = clip->x2 % 8;
@@ -692,12 +692,12 @@ void drm_fb_xrgb8888_to_mono_reversed(void *dst, unsigned int dst_pitch, const v
 	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_reversed_line(mono, gray8, dst_pitch,
-						   start_offset, end_len);
+		drm_fb_gray8_to_mono_line(mono, gray8, dst_pitch, start_offset,
+					  end_len);
 		vaddr += fb->pitches[0];
 		mono += dst_pitch;
 	}
 
 	kfree(src32);
 }
-EXPORT_SYMBOL(drm_fb_xrgb8888_to_mono_reversed);
+EXPORT_SYMBOL(drm_fb_xrgb8888_to_mono);
diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index d08d86ef07bcbe7f..caee851efd5726e7 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -458,7 +458,7 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m
 	if (!buf)
 		return -ENOMEM;
 
-	drm_fb_xrgb8888_to_mono_reversed(buf, 0, vmap, fb, rect);
+	drm_fb_xrgb8888_to_mono(buf, 0, vmap, fb, rect);
 
 	ssd130x_update_rect(ssd130x, buf, rect);
 
diff --git a/drivers/gpu/drm/tiny/repaper.c b/drivers/gpu/drm/tiny/repaper.c
index 37b6bb90e46e1fe8..a096fb8b83e99dc8 100644
--- a/drivers/gpu/drm/tiny/repaper.c
+++ b/drivers/gpu/drm/tiny/repaper.c
@@ -540,7 +540,7 @@ static int repaper_fb_dirty(struct drm_framebuffer *fb)
 	if (ret)
 		goto out_free;
 
-	drm_fb_xrgb8888_to_mono_reversed(buf, 0, cma_obj->vaddr, fb, &clip);
+	drm_fb_xrgb8888_to_mono(buf, 0, cma_obj->vaddr, fb, &clip);
 
 	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
 
diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
index 0b0937c0b2f6324e..55145eca07828321 100644
--- a/include/drm/drm_format_helper.h
+++ b/include/drm/drm_format_helper.h
@@ -43,8 +43,7 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for
 		     const void *vmap, const struct drm_framebuffer *fb,
 		     const struct drm_rect *rect);
 
-void drm_fb_xrgb8888_to_mono_reversed(void *dst, unsigned int dst_pitch, const void *src,
-				      const struct drm_framebuffer *fb,
-				      const struct drm_rect *clip);
+void drm_fb_xrgb8888_to_mono(void *dst, unsigned int dst_pitch, const void *src,
+			     const struct drm_framebuffer *fb, const struct drm_rect *clip);
 
 #endif /* __LINUX_DRM_FORMAT_HELPER_H */
-- 
2.25.1

Re: [PATCH 1/5] drm/format-helper: Rename drm_fb_xrgb8888_to_mono_reversed()

[Javier Martinez Canillas]

Hello Geert,

Thanks for your patch.

On 3/15/22 12:07, Geert Uytterhoeven wrote:
> There is no "reversed" handling in drm_fb_xrgb8888_to_mono_reversed():
> the function just converts from color to grayscale, and reduces the
> number of grayscale levels from 256 to 2 (i.e. brightness 0-127 is
> mapped to 0, 128-255 to 1).  All "reversed" handling is done in the
> repaper driver, where this function originated.
> 
> Hence make this clear by renaming drm_fb_xrgb8888_to_mono_reversed() to
> drm_fb_xrgb8888_to_mono(), and documenting the black/white pixel
> mapping.
> 
> Fixes: bcf8b616deb87941 ("drm/format-helper: Add drm_fb_xrgb8888_to_mono_reversed()")
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
As you mentioned the function originally came from the repaper driver
(that uses white-on-black) and I wrongly assumed that the destination
buffer for the OLED panels were also monochrome reversed.

Acked-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 1/5] drm/format-helper: Rename drm_fb_xrgb8888_to_mono_reversed()

[Andy Shevchenko]

On Tue, Mar 15, 2022 at 12:07:03PM +0100, Geert Uytterhoeven wrote:
> There is no "reversed" handling in drm_fb_xrgb8888_to_mono_reversed():
> the function just converts from color to grayscale, and reduces the
> number of grayscale levels from 256 to 2 (i.e. brightness 0-127 is
> mapped to 0, 128-255 to 1).  All "reversed" handling is done in the
> repaper driver, where this function originated.
> 
> Hence make this clear by renaming drm_fb_xrgb8888_to_mono_reversed() to
> drm_fb_xrgb8888_to_mono(), and documenting the black/white pixel
> mapping.

W/ or w/o the below remark being addressed
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Fixes: bcf8b616deb87941 ("drm/format-helper: Add drm_fb_xrgb8888_to_mono_reversed()")
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
>  drivers/gpu/drm/drm_format_helper.c | 32 ++++++++++++++---------------
>  drivers/gpu/drm/solomon/ssd130x.c   |  2 +-
>  drivers/gpu/drm/tiny/repaper.c      |  2 +-
>  include/drm/drm_format_helper.h     |  5 ++---
>  4 files changed, 20 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
> index bc0f49773868a9b0..b68aa857c6514529 100644
> --- a/drivers/gpu/drm/drm_format_helper.c
> +++ b/drivers/gpu/drm/drm_format_helper.c
> @@ -594,8 +594,8 @@ 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_reversed_line(u8 *dst, const u8 *src, unsigned int pixels,
> -					       unsigned int start_offset, unsigned int end_len)
> +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;
>  
> @@ -621,8 +621,8 @@ static void drm_fb_gray8_to_mono_reversed_line(u8 *dst, const u8 *src, unsigned
>  }
>  
>  /**
> - * drm_fb_xrgb8888_to_mono_reversed - Convert XRGB8888 to reversed monochrome
> - * @dst: reversed monochrome destination buffer
> + * drm_fb_xrgb8888_to_mono - Convert XRGB8888 to monochrome
> + * @dst: monochrome destination buffer (0=black, 1=white)
>   * @dst_pitch: Number of bytes between two consecutive scanlines within dst
>   * @src: XRGB8888 source buffer
>   * @fb: DRM framebuffer
> @@ -633,10 +633,10 @@ static void drm_fb_gray8_to_mono_reversed_line(u8 *dst, const u8 *src, unsigned
>   * and use this function to convert to the native format.
>   *
>   * This function uses drm_fb_xrgb8888_to_gray8() to convert to grayscale and
> - * then the result is converted from grayscale to reversed monohrome.
> + * then the result is converted from grayscale to monochrome.
>   */
> -void drm_fb_xrgb8888_to_mono_reversed(void *dst, unsigned int dst_pitch, const void *vaddr,
> -				      const struct drm_framebuffer *fb, const struct drm_rect *clip)
> +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;
> @@ -652,8 +652,8 @@ void drm_fb_xrgb8888_to_mono_reversed(void *dst, unsigned int dst_pitch, const v
>  		return;
>  
>  	/*
> -	 * The reversed 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 and
> +	 * destination scanlines have to be in multiple of 8 pixels.
>  	 */
>  	if (!dst_pitch)
>  		dst_pitch = DIV_ROUND_UP(linepixels, 8);
> @@ -664,9 +664,9 @@ void drm_fb_xrgb8888_to_mono_reversed(void *dst, unsigned int dst_pitch, const v
>  	 * The cma memory is write-combined so reads are uncached.
>  	 * Speed up by fetching one line at a time.
>  	 *
> -	 * Also, format conversion from XR24 to reversed monochrome
> -	 * are done line-by-line but are converted to 8-bit grayscale
> -	 * as an intermediate step.
> +	 * Also, format conversion from XR24 to monochrome are done
> +	 * line-by-line but are converted to 8-bit grayscale as an
> +	 * intermediate step.
>  	 *
>  	 * Allocate a buffer to be used for both copying from the cma
>  	 * memory and to store the intermediate grayscale line pixels.
> @@ -683,7 +683,7 @@ void drm_fb_xrgb8888_to_mono_reversed(void *dst, unsigned int dst_pitch, const v
>  	 * are not aligned to multiple of 8.
>  	 *
>  	 * Calculate if the start and end pixels are not aligned and set the
> -	 * offsets for the reversed mono line conversion function to adjust.
> +	 * offsets for the mono line conversion function to adjust.
>  	 */
>  	start_offset = clip->x1 % 8;
>  	end_len = clip->x2 % 8;
> @@ -692,12 +692,12 @@ void drm_fb_xrgb8888_to_mono_reversed(void *dst, unsigned int dst_pitch, const v
>  	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_reversed_line(mono, gray8, dst_pitch,
> -						   start_offset, end_len);

> +		drm_fb_gray8_to_mono_line(mono, gray8, dst_pitch, start_offset,
> +					  end_len);

Can be one line now (definition is already quite behind 80 limit).

>  		vaddr += fb->pitches[0];
>  		mono += dst_pitch;
>  	}
>  
>  	kfree(src32);
>  }
> -EXPORT_SYMBOL(drm_fb_xrgb8888_to_mono_reversed);
> +EXPORT_SYMBOL(drm_fb_xrgb8888_to_mono);
> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
> index d08d86ef07bcbe7f..caee851efd5726e7 100644
> --- a/drivers/gpu/drm/solomon/ssd130x.c
> +++ b/drivers/gpu/drm/solomon/ssd130x.c
> @@ -458,7 +458,7 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m
>  	if (!buf)
>  		return -ENOMEM;
>  
> -	drm_fb_xrgb8888_to_mono_reversed(buf, 0, vmap, fb, rect);
> +	drm_fb_xrgb8888_to_mono(buf, 0, vmap, fb, rect);
>  
>  	ssd130x_update_rect(ssd130x, buf, rect);
>  
> diff --git a/drivers/gpu/drm/tiny/repaper.c b/drivers/gpu/drm/tiny/repaper.c
> index 37b6bb90e46e1fe8..a096fb8b83e99dc8 100644
> --- a/drivers/gpu/drm/tiny/repaper.c
> +++ b/drivers/gpu/drm/tiny/repaper.c
> @@ -540,7 +540,7 @@ static int repaper_fb_dirty(struct drm_framebuffer *fb)
>  	if (ret)
>  		goto out_free;
>  
> -	drm_fb_xrgb8888_to_mono_reversed(buf, 0, cma_obj->vaddr, fb, &clip);
> +	drm_fb_xrgb8888_to_mono(buf, 0, cma_obj->vaddr, fb, &clip);
>  
>  	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
>  
> diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
> index 0b0937c0b2f6324e..55145eca07828321 100644
> --- a/include/drm/drm_format_helper.h
> +++ b/include/drm/drm_format_helper.h
> @@ -43,8 +43,7 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for
>  		     const void *vmap, const struct drm_framebuffer *fb,
>  		     const struct drm_rect *rect);
>  
> -void drm_fb_xrgb8888_to_mono_reversed(void *dst, unsigned int dst_pitch, const void *src,
> -				      const struct drm_framebuffer *fb,
> -				      const struct drm_rect *clip);
> +void drm_fb_xrgb8888_to_mono(void *dst, unsigned int dst_pitch, const void *src,
> +			     const struct drm_framebuffer *fb, const struct drm_rect *clip);
>  
>  #endif /* __LINUX_DRM_FORMAT_HELPER_H */
> -- 
> 2.25.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/5] drm/format-helper: Rename drm_fb_xrgb8888_to_mono_reversed()

[Geert Uytterhoeven]

Hi Andy,

On Tue, Mar 15, 2022 at 2:33 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Tue, Mar 15, 2022 at 12:07:03PM +0100, Geert Uytterhoeven wrote:
> > There is no "reversed" handling in drm_fb_xrgb8888_to_mono_reversed():
> > the function just converts from color to grayscale, and reduces the
> > number of grayscale levels from 256 to 2 (i.e. brightness 0-127 is
> > mapped to 0, 128-255 to 1).  All "reversed" handling is done in the
> > repaper driver, where this function originated.
> >
> > Hence make this clear by renaming drm_fb_xrgb8888_to_mono_reversed() to
> > drm_fb_xrgb8888_to_mono(), and documenting the black/white pixel
> > mapping.
>
> W/ or w/o the below remark being addressed
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thanks!

> > --- a/drivers/gpu/drm/drm_format_helper.c
> > +++ b/drivers/gpu/drm/drm_format_helper.c
> > @@ -692,12 +692,12 @@ void drm_fb_xrgb8888_to_mono_reversed(void *dst, unsigned int dst_pitch, const v
> >       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_reversed_line(mono, gray8, dst_pitch,
> > -                                                start_offset, end_len);
>
> > +             drm_fb_gray8_to_mono_line(mono, gray8, dst_pitch, start_offset,
> > +                                       end_len);
>
> Can be one line now (definition is already quite behind 80 limit).

Yeah, but the code isn't.
Nevertheless, this will be shortened in a later patch.

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

[PATCH 2/5] drm/format-helper: Fix XRGB888 to monochrome conversion

[Geert Uytterhoeven]

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(-)

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;
 	}
-- 
2.25.1

Re: [PATCH 2/5] drm/format-helper: Fix XRGB888 to monochrome conversion

[Javier Martinez Canillas]

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;
>  	}

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 2/5] drm/format-helper: Fix XRGB888 to monochrome conversion

[Geert Uytterhoeven]

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

Re: [PATCH 2/5] drm/format-helper: Fix XRGB888 to monochrome conversion

[Andy Shevchenko]

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;
> >  	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/5] drm/format-helper: Fix XRGB888 to monochrome conversion

[Andy Shevchenko]

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
> 

-- 
With Best Regards,
Andy Shevchenko

[PATCH 3/5] drm: ssd130x: Fix rectangle updates

[Geert Uytterhoeven]

The rectangle update functions ssd130x_fb_blit_rect() and
ssd130x_update_rect() do not behave correctly when x1 != 0 or y1 !=
0, or when y1 or y2 are not aligned to display page boundaries.
E.g. when used as a text console, only the first line of text is shown
on the display.

  1. The buffer passed by ssd130x_fb_blit_rect() points to the first
     byte of monochrome bitmap data, and thus has its origin at (x1,
     y1), while ssd130x_update_rect() assumes it is at (0, 0).
     Fix ssd130x_update_rect() by changing the vertical and horizontal
     loop ranges, and adding the offsets only when needed.

  2. In ssd130x_fb_blit_rect(), align y1 and y2 to the display page
     boundaries before doing the color conversion, so the full page
     is converted and updated.
     Remove the correction for an unaligned y1 from
     ssd130x_update_rect(), and add a check to make sure y1 is aligned.

Fixes: a61732e808672cfa ("drm: Add driver for Solomon SSD130x OLED displays")
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
Note that instead of calling drm_fb_xrgb8888_to_mono() and transposing
the bitmap, the image data could be converted to the transposed format
directly.  However, that would preclude exposing a monochrome format to
userspace when a fourcc for such a monochrome format is introduced.
---
 drivers/gpu/drm/solomon/ssd130x.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index caee851efd5726e7..7c99af4ce9dd4e5c 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -355,11 +355,14 @@ static int ssd130x_update_rect(struct ssd130x_device *ssd130x, u8 *buf,
 	unsigned int width = drm_rect_width(rect);
 	unsigned int height = drm_rect_height(rect);
 	unsigned int line_length = DIV_ROUND_UP(width, 8);
-	unsigned int pages = DIV_ROUND_UP(y % 8 + height, 8);
+	unsigned int pages = DIV_ROUND_UP(height, 8);
+	struct drm_device *drm = &ssd130x->drm;
 	u32 array_idx = 0;
 	int ret, i, j, k;
 	u8 *data_array = NULL;
 
+	drm_WARN_ONCE(drm, y % 8 != 0, "y must be aligned to screen page\n");
+
 	data_array = kcalloc(width, pages, GFP_KERNEL);
 	if (!data_array)
 		return -ENOMEM;
@@ -401,13 +404,13 @@ static int ssd130x_update_rect(struct ssd130x_device *ssd130x, u8 *buf,
 	if (ret < 0)
 		goto out_free;
 
-	for (i = y / 8; i < y / 8 + pages; i++) {
+	for (i = 0; i < pages; i++) {
 		int m = 8;
 
 		/* Last page may be partial */
-		if (8 * (i + 1) > ssd130x->height)
+		if (8 * (y / 8 + i + 1) > ssd130x->height)
 			m = ssd130x->height % 8;
-		for (j = x; j < x + width; j++) {
+		for (j = 0; j < width; j++) {
 			u8 data = 0;
 
 			for (k = 0; k < m; k++) {
@@ -454,6 +457,10 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m
 	int ret = 0;
 	u8 *buf = NULL;
 
+	/* Align y to display page boundaries */
+	rect->y1 = round_down(rect->y1, 8);
+	rect->y2 = min_t(unsigned int, round_up(rect->y2, 8), ssd130x->height);
+
 	buf = kcalloc(fb->width, fb->height, GFP_KERNEL);
 	if (!buf)
 		return -ENOMEM;
-- 
2.25.1

Re: [PATCH 3/5] drm: ssd130x: Fix rectangle updates

[Javier Martinez Canillas]

On 3/15/22 12:07, Geert Uytterhoeven wrote:
> The rectangle update functions ssd130x_fb_blit_rect() and
> ssd130x_update_rect() do not behave correctly when x1 != 0 or y1 !=
> 0, or when y1 or y2 are not aligned to display page boundaries.
> E.g. when used as a text console, only the first line of text is shown
> on the display.
> 
>   1. The buffer passed by ssd130x_fb_blit_rect() points to the first
>      byte of monochrome bitmap data, and thus has its origin at (x1,
>      y1), while ssd130x_update_rect() assumes it is at (0, 0).
>      Fix ssd130x_update_rect() by changing the vertical and horizontal
>      loop ranges, and adding the offsets only when needed.
> 
>   2. In ssd130x_fb_blit_rect(), align y1 and y2 to the display page
>      boundaries before doing the color conversion, so the full page
>      is converted and updated.
>      Remove the correction for an unaligned y1 from
>      ssd130x_update_rect(), and add a check to make sure y1 is aligned.
> 
> Fixes: a61732e808672cfa ("drm: Add driver for Solomon SSD130x OLED displays")
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---

Thanks for fixing this too.

Acked-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

[PATCH 4/5] drm: ssd130x: Reduce temporary buffer sizes

[Geert Uytterhoeven]

ssd130x_clear_screen() allocates a temporary buffer sized to hold one
byte per pixel, while it only needs to hold one bit per pixel.

ssd130x_fb_blit_rect() allocates a temporary buffer sized to hold one
byte per pixel for the whole frame buffer, while it only needs to hold
one bit per pixel for the part that is to be updated.
Pass dst_pitch to drm_fb_xrgb8888_to_mono_reversed(), as we have already
calculated it anyway.

Fixes: a61732e808672cfa ("drm: Add driver for Solomon SSD130x OLED displays")
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
 drivers/gpu/drm/solomon/ssd130x.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index 7c99af4ce9dd4e5c..38b6c2c14f53644b 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -440,7 +440,8 @@ static void ssd130x_clear_screen(struct ssd130x_device *ssd130x)
 		.y2 = ssd130x->height,
 	};
 
-	buf = kcalloc(ssd130x->width, ssd130x->height, GFP_KERNEL);
+	buf = kcalloc(DIV_ROUND_UP(ssd130x->width, 8), ssd130x->height,
+		      GFP_KERNEL);
 	if (!buf)
 		return;
 
@@ -454,6 +455,7 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m
 {
 	struct ssd130x_device *ssd130x = drm_to_ssd130x(fb->dev);
 	void *vmap = map->vaddr; /* TODO: Use mapping abstraction properly */
+	unsigned int dst_pitch;
 	int ret = 0;
 	u8 *buf = NULL;
 
@@ -461,11 +463,12 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m
 	rect->y1 = round_down(rect->y1, 8);
 	rect->y2 = min_t(unsigned int, round_up(rect->y2, 8), ssd130x->height);
 
-	buf = kcalloc(fb->width, fb->height, GFP_KERNEL);
+	dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), 8);
+	buf = kcalloc(dst_pitch, drm_rect_height(rect), GFP_KERNEL);
 	if (!buf)
 		return -ENOMEM;
 
-	drm_fb_xrgb8888_to_mono(buf, 0, vmap, fb, rect);
+	drm_fb_xrgb8888_to_mono(buf, dst_pitch, vmap, fb, rect);
 
 	ssd130x_update_rect(ssd130x, buf, rect);
 
-- 
2.25.1

Re: [PATCH 4/5] drm: ssd130x: Reduce temporary buffer sizes

[Javier Martinez Canillas]

On 3/15/22 12:07, Geert Uytterhoeven wrote:
> ssd130x_clear_screen() allocates a temporary buffer sized to hold one
> byte per pixel, while it only needs to hold one bit per pixel.
> 
> ssd130x_fb_blit_rect() allocates a temporary buffer sized to hold one
> byte per pixel for the whole frame buffer, while it only needs to hold
> one bit per pixel for the part that is to be updated.
> Pass dst_pitch to drm_fb_xrgb8888_to_mono_reversed(), as we have already

Just drm_fb_xrgb8888_to_mono() since you already fixed the name in patch 1/5.

> calculated it anyway.
> 
> Fixes: a61732e808672cfa ("drm: Add driver for Solomon SSD130x OLED displays")
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---

Indeed. I haven't noticed that got the calculation wrong until you pointed out.

Acked-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 4/5] drm: ssd130x: Reduce temporary buffer sizes

[Geert Uytterhoeven]

Hi Javier,

On Tue, Mar 15, 2022 at 1:32 PM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> On 3/15/22 12:07, Geert Uytterhoeven wrote:
> > ssd130x_clear_screen() allocates a temporary buffer sized to hold one
> > byte per pixel, while it only needs to hold one bit per pixel.
> >
> > ssd130x_fb_blit_rect() allocates a temporary buffer sized to hold one
> > byte per pixel for the whole frame buffer, while it only needs to hold
> > one bit per pixel for the part that is to be updated.
> > Pass dst_pitch to drm_fb_xrgb8888_to_mono_reversed(), as we have already
>
> Just drm_fb_xrgb8888_to_mono() since you already fixed the name in patch 1/5.

Bummer, that happens when reshuffling patches :-(
Fixed for v2.

> > calculated it anyway.
> >
> > Fixes: a61732e808672cfa ("drm: Add driver for Solomon SSD130x OLED displays")
> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > ---
>
> Indeed. I haven't noticed that got the calculation wrong until you pointed out.
>
> Acked-by: Javier Martinez Canillas <javierm@redhat.com>

Thanks!

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

Re: [PATCH 4/5] drm: ssd130x: Reduce temporary buffer sizes

[Andy Shevchenko]

On Tue, Mar 15, 2022 at 12:07:06PM +0100, Geert Uytterhoeven wrote:
> ssd130x_clear_screen() allocates a temporary buffer sized to hold one
> byte per pixel, while it only needs to hold one bit per pixel.
> 
> ssd130x_fb_blit_rect() allocates a temporary buffer sized to hold one
> byte per pixel for the whole frame buffer, while it only needs to hold
> one bit per pixel for the part that is to be updated.
> Pass dst_pitch to drm_fb_xrgb8888_to_mono_reversed(), as we have already
> calculated it anyway.

Can we use bitmap API? bitmap_zalloc() / etc ?


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/5] drm: ssd130x: Reduce temporary buffer sizes

[Geert Uytterhoeven]

Hi Andy,

On Tue, Mar 15, 2022 at 2:50 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Tue, Mar 15, 2022 at 12:07:06PM +0100, Geert Uytterhoeven wrote:
> > ssd130x_clear_screen() allocates a temporary buffer sized to hold one
> > byte per pixel, while it only needs to hold one bit per pixel.
> >
> > ssd130x_fb_blit_rect() allocates a temporary buffer sized to hold one
> > byte per pixel for the whole frame buffer, while it only needs to hold
> > one bit per pixel for the part that is to be updated.
> > Pass dst_pitch to drm_fb_xrgb8888_to_mono_reversed(), as we have already
> > calculated it anyway.
>
> Can we use bitmap API? bitmap_zalloc() / etc ?

Why? There is no need to operate on an array of longs, only on an
array of bytes. Going to longs would expose us to endianness.
There's also no need to use any of the bitmap operations to modify the data.

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

[PATCH 5/5] drm/repaper: Reduce temporary buffer size in repaper_fb_dirty()

[Geert Uytterhoeven]

As the temporary buffer is no longer used to store 8-bit grayscale data,
its size can be reduced to the size needed to store the monochrome
bitmap data.

Fixes: 24c6bedefbe71de9 ("drm/repaper: Use format helper for xrgb8888 to monochrome conversion")
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
Untested due to lack of hardware.

I replaced kmalloc_array() by kmalloc() to match size calculations in
other locations in this driver.  There is no point in handling a
possible multiplication overflow only here.
---
 drivers/gpu/drm/tiny/repaper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tiny/repaper.c b/drivers/gpu/drm/tiny/repaper.c
index a096fb8b83e99dc8..7738b87f370ad147 100644
--- a/drivers/gpu/drm/tiny/repaper.c
+++ b/drivers/gpu/drm/tiny/repaper.c
@@ -530,7 +530,7 @@ static int repaper_fb_dirty(struct drm_framebuffer *fb)
 	DRM_DEBUG("Flushing [FB:%d] st=%ums\n", fb->base.id,
 		  epd->factored_stage_time);
 
-	buf = kmalloc_array(fb->width, fb->height, GFP_KERNEL);
+	buf = kmalloc(fb->width * fb->height / 8, GFP_KERNEL);
 	if (!buf) {
 		ret = -ENOMEM;
 		goto out_exit;
-- 
2.25.1

Re: [PATCH 5/5] drm/repaper: Reduce temporary buffer size in repaper_fb_dirty()

[Javier Martinez Canillas]

On 3/15/22 12:07, Geert Uytterhoeven wrote:
> As the temporary buffer is no longer used to store 8-bit grayscale data,
> its size can be reduced to the size needed to store the monochrome
> bitmap data.
> 
> Fixes: 24c6bedefbe71de9 ("drm/repaper: Use format helper for xrgb8888 to monochrome conversion")
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
> Untested due to lack of hardware.
> 

Patch looks good to me but I also don't have this hardware.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

--
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 5/5] drm/repaper: Reduce temporary buffer size in repaper_fb_dirty()

[Andy Shevchenko]

On Tue, Mar 15, 2022 at 12:07:07PM +0100, Geert Uytterhoeven wrote:
> As the temporary buffer is no longer used to store 8-bit grayscale data,
> its size can be reduced to the size needed to store the monochrome
> bitmap data.

bitmap API?

-- 
With Best Regards,
Andy Shevchenko