diff mbox series

[2/4] drm/format-helper: Add drm_fb_gray8_to_mono_reversed()

Message ID 20220131201225.2324984-3-javierm@redhat.com (mailing list archive)
State Handled Elsewhere
Headers show
Series drm/tiny: Add driver for Solomon SSD1307 OLED displays | expand

Commit Message

Javier Martinez Canillas Jan. 31, 2022, 8:12 p.m. UTC
Add support to convert 8-bit grayscale to reversed monochrome for drivers
that control monochromatic displays, that only have 1 bit per pixel depth.

This helper function was based on repaper_gray8_to_mono_reversed() from
the drivers/gpu/drm/tiny/repaper.c driver.

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

 drivers/gpu/drm/drm_format_helper.c | 35 +++++++++++++++++++++++++++++
 include/drm/drm_format_helper.h     |  2 ++
 2 files changed, 37 insertions(+)

Comments

Thomas Zimmermann Feb. 1, 2022, 9:59 a.m. UTC | #1
Hi

Am 31.01.22 um 21:12 schrieb Javier Martinez Canillas:
> Add support to convert 8-bit grayscale to reversed monochrome for drivers
> that control monochromatic displays, that only have 1 bit per pixel depth.
> 
> This helper function was based on repaper_gray8_to_mono_reversed() from
> the drivers/gpu/drm/tiny/repaper.c driver.

You could convert repaper to the new helper.

> 
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> 
>   drivers/gpu/drm/drm_format_helper.c | 35 +++++++++++++++++++++++++++++
>   include/drm/drm_format_helper.h     |  2 ++
>   2 files changed, 37 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
> index 0f28dd2bdd72..bf477c136082 100644
> --- a/drivers/gpu/drm/drm_format_helper.c
> +++ b/drivers/gpu/drm/drm_format_helper.c
> @@ -584,3 +584,38 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for
>   	return -EINVAL;
>   }
>   EXPORT_SYMBOL(drm_fb_blit_toio);
> +
> +/**
> + * drm_fb_gray8_to_mono_reversed - Convert grayscale to reversed monochrome
> + * @dst: reversed monochrome destination buffer
> + * @src: 8-bit grayscale source buffer
> + * @clip: Clip rectangle area to copy
> + *
> + * DRM doesn't have native monochrome or grayscale support.
> + * Such drivers can announce the commonly supported XR24 format to userspace
> + * and use drm_fb_xrgb8888_to_gray8() to convert to grayscale and then this
> + * helper function to convert to the native format.
> + */
> +void drm_fb_gray8_to_mono_reversed(void *dst, void *src, const struct drm_rect *clip)

IMHO it would be better to have a function that converts xrgb8888 to 
mono and let it handle the intermediate step of gray8.

> +{
> +	size_t width = drm_rect_width(clip);
> +	size_t height = drm_rect_width(clip);
> +
> +	u8 *mono = dst, *gray8 = src;
> +	unsigned int y, xb, i;
> +
> +	for (y = 0; y < height; y++)
> +		for (xb = 0; xb < width / 8; xb++) {

The inner loop should probably go into a separate helper function. See 
the drm_fb_*_line() helpers in this file

At some point, we's want to have a single blit helper that takes source 
and destination formats/buffers. It would then pick the correct per-line 
helper for the conversion. So yeah, we'd want something composable.

Best regards
Thomas

> +			u8 byte = 0x00;
> +
> +			for (i = 0; i < 8; i++) {
> +				int x = xb * 8 + i;
> +
> +				byte >>= 1;
> +				if (gray8[y * width + x] >> 7)
> +					byte |= BIT(7);
> +			}
> +			*mono++ = byte;
> +		}
> +}
> +EXPORT_SYMBOL(drm_fb_gray8_to_mono_reversed);
> diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
> index b30ed5de0a33..cd4c8b7c78de 100644
> --- a/include/drm/drm_format_helper.h
> +++ b/include/drm/drm_format_helper.h
> @@ -43,4 +43,6 @@ 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_gray8_to_mono_reversed(void *dst, void *vaddr, const struct drm_rect *clip);
> +
>   #endif /* __LINUX_DRM_FORMAT_HELPER_H */
Pekka Paalanen Feb. 1, 2022, 11:13 a.m. UTC | #2
On Tue, 1 Feb 2022 10:59:43 +0100
Thomas Zimmermann <tzimmermann@suse.de> wrote:

> Hi
> 
> Am 31.01.22 um 21:12 schrieb Javier Martinez Canillas:
> > Add support to convert 8-bit grayscale to reversed monochrome for drivers
> > that control monochromatic displays, that only have 1 bit per pixel depth.
> > 
> > This helper function was based on repaper_gray8_to_mono_reversed() from
> > the drivers/gpu/drm/tiny/repaper.c driver.  
> 
> You could convert repaper to the new helper.
> 
> > 
> > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> > ---
> > 
> >   drivers/gpu/drm/drm_format_helper.c | 35 +++++++++++++++++++++++++++++
> >   include/drm/drm_format_helper.h     |  2 ++
> >   2 files changed, 37 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
> > index 0f28dd2bdd72..bf477c136082 100644
> > --- a/drivers/gpu/drm/drm_format_helper.c
> > +++ b/drivers/gpu/drm/drm_format_helper.c
> > @@ -584,3 +584,38 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for
> >   	return -EINVAL;
> >   }
> >   EXPORT_SYMBOL(drm_fb_blit_toio);
> > +
> > +/**
> > + * drm_fb_gray8_to_mono_reversed - Convert grayscale to reversed monochrome
> > + * @dst: reversed monochrome destination buffer
> > + * @src: 8-bit grayscale source buffer
> > + * @clip: Clip rectangle area to copy
> > + *
> > + * DRM doesn't have native monochrome or grayscale support.
> > + * Such drivers can announce the commonly supported XR24 format to userspace
> > + * and use drm_fb_xrgb8888_to_gray8() to convert to grayscale and then this
> > + * helper function to convert to the native format.
> > + */
> > +void drm_fb_gray8_to_mono_reversed(void *dst, void *src, const struct drm_rect *clip)  
> 
> IMHO it would be better to have a function that converts xrgb8888 to 
> mono and let it handle the intermediate step of gray8.
> 
> > +{
> > +	size_t width = drm_rect_width(clip);
> > +	size_t height = drm_rect_width(clip);
> > +
> > +	u8 *mono = dst, *gray8 = src;
> > +	unsigned int y, xb, i;
> > +
> > +	for (y = 0; y < height; y++)
> > +		for (xb = 0; xb < width / 8; xb++) {  
> 
> The inner loop should probably go into a separate helper function. See 
> the drm_fb_*_line() helpers in this file
> 
> At some point, we's want to have a single blit helper that takes source 
> and destination formats/buffers. It would then pick the correct per-line 
> helper for the conversion. So yeah, we'd want something composable.

Btw. VKMS is going to do blending, and it needs to support various
formats, so the latest patches from Igor probably do something similar.


Thanks,
pq


> 
> Best regards
> Thomas
> 
> > +			u8 byte = 0x00;
> > +
> > +			for (i = 0; i < 8; i++) {
> > +				int x = xb * 8 + i;
> > +
> > +				byte >>= 1;
> > +				if (gray8[y * width + x] >> 7)
> > +					byte |= BIT(7);
> > +			}
> > +			*mono++ = byte;
> > +		}
> > +}
> > +EXPORT_SYMBOL(drm_fb_gray8_to_mono_reversed);
Javier Martinez Canillas Feb. 1, 2022, 11:48 a.m. UTC | #3
Hello Thomas,

Thanks a lot for your feedback.

On 2/1/22 10:59, Thomas Zimmermann wrote:
> Hi
> 
> Am 31.01.22 um 21:12 schrieb Javier Martinez Canillas:
>> Add support to convert 8-bit grayscale to reversed monochrome for drivers
>> that control monochromatic displays, that only have 1 bit per pixel depth.
>>
>> This helper function was based on repaper_gray8_to_mono_reversed() from
>> the drivers/gpu/drm/tiny/repaper.c driver.
> 
> You could convert repaper to the new helper.
>

Yes, I plan to do it but was out of scope for this patch series.

>>
>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>> ---
>>
>>   drivers/gpu/drm/drm_format_helper.c | 35 +++++++++++++++++++++++++++++
>>   include/drm/drm_format_helper.h     |  2 ++
>>   2 files changed, 37 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
>> index 0f28dd2bdd72..bf477c136082 100644
>> --- a/drivers/gpu/drm/drm_format_helper.c
>> +++ b/drivers/gpu/drm/drm_format_helper.c
>> @@ -584,3 +584,38 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for
>>   	return -EINVAL;
>>   }
>>   EXPORT_SYMBOL(drm_fb_blit_toio);
>> +
>> +/**
>> + * drm_fb_gray8_to_mono_reversed - Convert grayscale to reversed monochrome
>> + * @dst: reversed monochrome destination buffer
>> + * @src: 8-bit grayscale source buffer
>> + * @clip: Clip rectangle area to copy
>> + *
>> + * DRM doesn't have native monochrome or grayscale support.
>> + * Such drivers can announce the commonly supported XR24 format to userspace
>> + * and use drm_fb_xrgb8888_to_gray8() to convert to grayscale and then this
>> + * helper function to convert to the native format.
>> + */
>> +void drm_fb_gray8_to_mono_reversed(void *dst, void *src, const struct drm_rect *clip)
> 
> IMHO it would be better to have a function that converts xrgb8888 to 
> mono and let it handle the intermediate step of gray8.
>

That's a good idea. I'll add that too.

>> +{
>> +	size_t width = drm_rect_width(clip);
>> +	size_t height = drm_rect_width(clip);
>> +
>> +	u8 *mono = dst, *gray8 = src;
>> +	unsigned int y, xb, i;
>> +
>> +	for (y = 0; y < height; y++)
>> +		for (xb = 0; xb < width / 8; xb++) {
> 
> The inner loop should probably go into a separate helper function. See 
> the drm_fb_*_line() helpers in this file
>
> At some point, we's want to have a single blit helper that takes source 
> and destination formats/buffers. It would then pick the correct per-line 
> helper for the conversion. So yeah, we'd want something composable.
>

Agreed. I'll split that into a separate helper function.

Best regards,
Geert Uytterhoeven March 14, 2022, 1:40 p.m. UTC | #4
Hi Javier,

On Mon, Jan 31, 2022 at 9:12 PM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> Add support to convert 8-bit grayscale to reversed monochrome for drivers
> that control monochromatic displays, that only have 1 bit per pixel depth.
>
> This helper function was based on repaper_gray8_to_mono_reversed() from
> the drivers/gpu/drm/tiny/repaper.c driver.
>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

> --- a/drivers/gpu/drm/drm_format_helper.c
> +++ b/drivers/gpu/drm/drm_format_helper.c
> @@ -584,3 +584,38 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for
>         return -EINVAL;
>  }
>  EXPORT_SYMBOL(drm_fb_blit_toio);
> +
> +/**
> + * drm_fb_gray8_to_mono_reversed - Convert grayscale to reversed monochrome
> + * @dst: reversed monochrome destination buffer

What's the meaning of "reversed"?
During the last few days, I've been balancing between (a) "reverse
video" and (b) "reverse bit order", but none of them seems to be true.

(a) The code maps 0-127 to 0 and 8-255 to 1, which just reduces from
    256 to 2 grayscale levels, without inversion. The result is also
    white-on-black on my ssd130x OLED.
(b) On little-endian, the CFB drawops use little-endian bit order,
    which is what ends up in "byte" in the code below.

> + * @src: 8-bit grayscale source buffer
> + * @clip: Clip rectangle area to copy
> + *
> + * DRM doesn't have native monochrome or grayscale support.
> + * Such drivers can announce the commonly supported XR24 format to userspace
> + * and use drm_fb_xrgb8888_to_gray8() to convert to grayscale and then this
> + * helper function to convert to the native format.
> + */
> +void drm_fb_gray8_to_mono_reversed(void *dst, void *src, const struct drm_rect *clip)
> +{
> +       size_t width = drm_rect_width(clip);
> +       size_t height = drm_rect_width(clip);
> +
> +       u8 *mono = dst, *gray8 = src;
> +       unsigned int y, xb, i;
> +
> +       for (y = 0; y < height; y++)
> +               for (xb = 0; xb < width / 8; xb++) {
> +                       u8 byte = 0x00;
> +
> +                       for (i = 0; i < 8; i++) {
> +                               int x = xb * 8 + i;
> +
> +                               byte >>= 1;
> +                               if (gray8[y * width + x] >> 7)
> +                                       byte |= BIT(7);
> +                       }
> +                       *mono++ = 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
Javier Martinez Canillas March 14, 2022, 2:07 p.m. UTC | #5
Hello Geert,

On 3/14/22 14:40, Geert Uytterhoeven wrote:
> Hi Javier,
> 
> On Mon, Jan 31, 2022 at 9:12 PM Javier Martinez Canillas
> <javierm@redhat.com> wrote:
>> Add support to convert 8-bit grayscale to reversed monochrome for drivers
>> that control monochromatic displays, that only have 1 bit per pixel depth.
>>
>> This helper function was based on repaper_gray8_to_mono_reversed() from
>> the drivers/gpu/drm/tiny/repaper.c driver.
>>
>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> 
>> --- a/drivers/gpu/drm/drm_format_helper.c
>> +++ b/drivers/gpu/drm/drm_format_helper.c
>> @@ -584,3 +584,38 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for
>>         return -EINVAL;
>>  }
>>  EXPORT_SYMBOL(drm_fb_blit_toio);
>> +
>> +/**
>> + * drm_fb_gray8_to_mono_reversed - Convert grayscale to reversed monochrome
>> + * @dst: reversed monochrome destination buffer
> 
> What's the meaning of "reversed"?

Originally I took this helper from the repaper driver and it was called
repaper_gray8_to_mono_reversed(), so the naming just got carried over.

> During the last few days, I've been balancing between (a) "reverse
> video" and (b) "reverse bit order", but none of them seems to be true.
>
> (a) The code maps 0-127 to 0 and 8-255 to 1, which just reduces from
>     256 to 2 grayscale levels, without inversion. The result is also
>     white-on-black on my ssd130x OLED.
> (b) On little-endian, the CFB drawops use little-endian bit order,
>     which is what ends up in "byte" in the code below.
>

As far as I understand is (a) from the options since the repaper display
uses black-on-white. But as you pointed out the ssd130x OLEDs instead do
white-on-black.

I should had probably just name the helper drm_fb_gray8_to_monochrome()
or maybe drm_fb_gray8_to_gray1() ?
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
index 0f28dd2bdd72..bf477c136082 100644
--- a/drivers/gpu/drm/drm_format_helper.c
+++ b/drivers/gpu/drm/drm_format_helper.c
@@ -584,3 +584,38 @@  int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for
 	return -EINVAL;
 }
 EXPORT_SYMBOL(drm_fb_blit_toio);
+
+/**
+ * drm_fb_gray8_to_mono_reversed - Convert grayscale to reversed monochrome
+ * @dst: reversed monochrome destination buffer
+ * @src: 8-bit grayscale source buffer
+ * @clip: Clip rectangle area to copy
+ *
+ * DRM doesn't have native monochrome or grayscale support.
+ * Such drivers can announce the commonly supported XR24 format to userspace
+ * and use drm_fb_xrgb8888_to_gray8() to convert to grayscale and then this
+ * helper function to convert to the native format.
+ */
+void drm_fb_gray8_to_mono_reversed(void *dst, void *src, const struct drm_rect *clip)
+{
+	size_t width = drm_rect_width(clip);
+	size_t height = drm_rect_width(clip);
+
+	u8 *mono = dst, *gray8 = src;
+	unsigned int y, xb, i;
+
+	for (y = 0; y < height; y++)
+		for (xb = 0; xb < width / 8; xb++) {
+			u8 byte = 0x00;
+
+			for (i = 0; i < 8; i++) {
+				int x = xb * 8 + i;
+
+				byte >>= 1;
+				if (gray8[y * width + x] >> 7)
+					byte |= BIT(7);
+			}
+			*mono++ = byte;
+		}
+}
+EXPORT_SYMBOL(drm_fb_gray8_to_mono_reversed);
diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
index b30ed5de0a33..cd4c8b7c78de 100644
--- a/include/drm/drm_format_helper.h
+++ b/include/drm/drm_format_helper.h
@@ -43,4 +43,6 @@  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_gray8_to_mono_reversed(void *dst, void *vaddr, const struct drm_rect *clip);
+
 #endif /* __LINUX_DRM_FORMAT_HELPER_H */