| Message ID | 20220204134347.1187749-2-javierm@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | drm/tiny: Add driver for Solomon SSD1307 OLED displays | expand |
Hello Thomas, Thanks a lot for your feedback. On 2/4/22 16:52, Thomas Zimmermann wrote: [snip] >> +static void drm_fb_gray8_to_mono_reversed_line(u8 *dst, const u8 *src, size_t pixels) >> +{ >> + unsigned int xb, i; >> + >> + for (xb = 0; xb < pixels / 8; xb++) { > > In practice, all mode widths are multiples of 8 because VGA mandated it. > So it's ok-ish to assume this here. You should probably at least print a > warning somewhere if (pixels % 8 != 0) > Agreed. [snip] >> + * 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, unsigned int dst_pitch, const void *src, >> + const struct drm_rect *clip) > > There's a bug here. You want to pass in a drm_framebuffer as fourth > argument. > >> +{ >> + >> + size_t height = drm_rect_height(clip); >> + size_t width = drm_rect_width(clip); >> + unsigned int y; >> + const u8 *gray8 = src; >> + u8 *mono = dst; >> + >> + if (!dst_pitch) >> + dst_pitch = width; > > The dst_pitch is given in bytes. You have to device by 8. Here would be > a good place to warn if (width % 8 != 0). > Ok. >> + >> + for (y = 0; y < height; y++) { >> + drm_fb_gray8_to_mono_reversed_line(mono, gray8, dst_pitch); >> + mono += (dst_pitch / 8); > > The dst_pitch is already given in bytes. > Yes, I know but for reversed mono we want only 1/8 of the width since we are converting from 8 bits per pixel greyscale to 1 bit per pixel mono. Or am I misunderstanding what you meant ? >> + gray8 += dst_pitch; > > 'gray8 += fb->pitches[0]' would be correct. > Ok. [snip] >> + */ >> +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) >> +{ >> + if (WARN_ON(fb->format->format != DRM_FORMAT_XRGB8888)) >> + return; >> + >> + if (!dst_pitch) >> + dst_pitch = drm_rect_width(clip); >> + >> + drm_fb_xrgb8888_to_gray8(dst, dst_pitch, src, fb, clip); >> + drm_fb_gray8_to_mono_reversed(dst, dst_pitch, dst, fb, clip); > > Converting from dst into dst can give incorrect results. At some point > we probably want to add restrict qualifiers to these pointers, to help > the compiler with optimizing. > > A better approach here is to pull the per-line conversion from > drm_fb_xrgb8888_to_gray8() into a separate helper and implement a > line-by-line conversion here. something like this: > > drm_fb_xrgb8888_to_mono_reversed() > { > char *tmp = kmalloc(size of a single line of gray8) > > for (heigth) { > drm_fb_xrgb8888_to_gray8_line(tmp, ..., src, ...); > drm_fb_gray8_to_mono_reversed(dst, ..., tmp, ...); > > src += fb->pitches[0] > dst += dst_pitch; > } > > kfree(tmp); > } > I see. Yes, that sounds a much better approach. I'll change it in v3. Best regards,
On Fri, Feb 4, 2022 at 10:53 AM Thomas Zimmermann <tzimmermann@suse.de> wrote: > > Hi > > Am 04.02.22 um 14:43 schrieb Javier Martinez Canillas: > > Add support to convert XR24 and 8-bit grayscale to reversed monochrome for > > drivers that control monochromatic panels, that only have 1 bit per pixel. > > > > The drm_fb_gray8_to_mono_reversed() helper was based on the function that > > does the same in the drivers/gpu/drm/tiny/repaper.c driver. > > > > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> > > --- > > > > (no changes since v1) > > > > drivers/gpu/drm/drm_format_helper.c | 80 +++++++++++++++++++++++++++++ > > include/drm/drm_format_helper.h | 7 +++ > > 2 files changed, 87 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c > > index 0f28dd2bdd72..cdce4b7c25d9 100644 > > --- a/drivers/gpu/drm/drm_format_helper.c > > +++ b/drivers/gpu/drm/drm_format_helper.c > > @@ -584,3 +584,83 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for > > return -EINVAL; > > } > > EXPORT_SYMBOL(drm_fb_blit_toio); > > + > > +static void drm_fb_gray8_to_mono_reversed_line(u8 *dst, const u8 *src, size_t pixels) > > +{ > > + unsigned int xb, i; > > + > > + for (xb = 0; xb < pixels / 8; xb++) { > > In practice, all mode widths are multiples of 8 because VGA mandated it. > So it's ok-ish to assume this here. You should probably at least print a > warning somewhere if (pixels % 8 != 0) Not sure if it's relevant, but 1366x768 was a fairly popular laptop resolution. There's even a dedicated drm_mode_fixup_1366x768 in drm_edid.c. (Would it have killed them to add 2 more horizontal pixels? Apparently.) Cheers, -ilia
diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c index 0f28dd2bdd72..cdce4b7c25d9 100644 --- a/drivers/gpu/drm/drm_format_helper.c +++ b/drivers/gpu/drm/drm_format_helper.c @@ -584,3 +584,83 @@ int drm_fb_blit_toio(void __iomem *dst, unsigned int dst_pitch, uint32_t dst_for return -EINVAL; } EXPORT_SYMBOL(drm_fb_blit_toio); + +static void drm_fb_gray8_to_mono_reversed_line(u8 *dst, const u8 *src, size_t pixels) +{ + unsigned int xb, i; + + for (xb = 0; xb < pixels / 8; xb++) { + u8 byte = 0x00; + + for (i = 0; i < 8; i++) { + int x = xb * 8 + i; + + byte >>= 1; + if (src[x] >> 7) + byte |= BIT(7); + } + *dst++ = byte; + } +} + +/** + * drm_fb_gray8_to_mono_reversed - Convert grayscale to reversed monochrome + * @dst: reversed monochrome destination buffer + * @dst_pitch: Number of bytes between two consecutive scanlines within dst + * @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, unsigned int dst_pitch, const void *src, + const struct drm_rect *clip) +{ + + size_t height = drm_rect_height(clip); + size_t width = drm_rect_width(clip); + unsigned int y; + const u8 *gray8 = src; + u8 *mono = dst; + + if (!dst_pitch) + dst_pitch = width; + + for (y = 0; y < height; y++) { + drm_fb_gray8_to_mono_reversed_line(mono, gray8, dst_pitch); + mono += (dst_pitch / 8); + gray8 += dst_pitch; + } +} + +/** + * drm_fb_xrgb8888_to_mono_reversed - Convert XRGB8888 to reversed monochrome + * @dst: reversed monochrome destination buffer + * @dst_pitch: Number of bytes between two consecutive scanlines within dst + * @src: XRGB8888 source buffer + * @fb: DRM framebuffer + * @clip: Clip rectangle area to copy + * + * DRM doesn't have native monochrome support. + * Such drivers can announce the commonly supported XR24 format to userspace + * 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. + */ +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) +{ + if (WARN_ON(fb->format->format != DRM_FORMAT_XRGB8888)) + return; + + if (!dst_pitch) + dst_pitch = drm_rect_width(clip); + + drm_fb_xrgb8888_to_gray8(dst, dst_pitch, src, fb, clip); + drm_fb_gray8_to_mono_reversed(dst, dst_pitch, dst, fb, clip); +} +EXPORT_SYMBOL(drm_fb_xrgb8888_to_mono_reversed); diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h index b30ed5de0a33..85e551a5cbe6 100644 --- a/include/drm/drm_format_helper.h +++ b/include/drm/drm_format_helper.h @@ -43,4 +43,11 @@ 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, unsigned int dst_pitch, const void *src, + const struct drm_rect *clip); + +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); + #endif /* __LINUX_DRM_FORMAT_HELPER_H */
Add support to convert XR24 and 8-bit grayscale to reversed monochrome for drivers that control monochromatic panels, that only have 1 bit per pixel. The drm_fb_gray8_to_mono_reversed() helper was based on the function that does the same in the drivers/gpu/drm/tiny/repaper.c driver. Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> --- (no changes since v1) drivers/gpu/drm/drm_format_helper.c | 80 +++++++++++++++++++++++++++++ include/drm/drm_format_helper.h | 7 +++ 2 files changed, 87 insertions(+)