| Message ID | 20220211091927.2988283-3-javierm@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | drm: Add driver for Solomon SSD130x OLED displays | expand |
On Fri, Feb 11, 2022 at 10:19:23AM +0100, Javier Martinez Canillas wrote: > Add support to convert from XR24 to reversed monochrome for drivers that > control monochromatic display panels, that only have 1 bit per pixel. > > The function does a line-by-line conversion doing an intermediate step > first from XR24 to 8-bit grayscale and then to reversed monochrome. > > The drm_fb_gray8_to_mono_reversed_line() helper was based on code from > drivers/gpu/drm/tiny/repaper.c driver. ... > +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) > +{ > + 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; This is invariant to the loop, can be moved out. > + if (xb == pixels - 1 && end_len) > + end = end_len; Ditto. However it may require to factor out the following loop to a helper. > + for (i = start; i < end; i++) { > + unsigned int x = xb * 8 + i; > + > + byte >>= 1; > + if (src[x] >> 7) > + byte |= BIT(7); > + } > + *dst++ = byte; > + } > +} ... > + /* > + * The reversed 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); round_up() ? > + WARN_ONCE(dst_pitch % 8 != 0, "dst_pitch is not a multiple of 8\n"); I would move this to the if conditional, i.e. if (dst_pitch) WARN_ONCE(dst_pitch % 8 != 0, "dst_pitch is not a multiple of 8\n"); else dst_pitch = round_up(linepixels, 8); > + /* > + * The cma memory is write-combined so reads are uncached. CMA > + * 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. > + * > + * Allocate a buffer to be used for both copying from the cma > + * memory and to store the intermediate grayscale line pixels. > + */ > + src32 = kmalloc(len_src32 + linepixels, GFP_KERNEL); size_add() ? > + if (!src32) > + return; ... > + /* > + * 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 reversed mono line conversion function to adjust. > + */ > + start_offset = clip->x1 % 8; > + end_len = clip->x2 % 8; ALIGN() ?
Hello Andy, Thanks for your feedback. On 2/11/22 12:10, Andy Shevchenko wrote: [snip] >> +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) >> +{ >> + 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; > > This is invariant to the loop, can be moved out. > >> + if (xb == pixels - 1 && end_len) >> + end = end_len; > > Ditto. However it may require to factor out the following loop to a helper. > Not sure I'm following, it's not invariant since it depends on the loop iterator value. It only applies to the first and last pixels. [snip] >> + /* >> + * The reversed 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); > > round_up() ? > But it's not a round up operation but a div and round up. >> + WARN_ONCE(dst_pitch % 8 != 0, "dst_pitch is not a multiple of 8\n"); > > > I would move this to the if conditional, i.e. > > if (dst_pitch) > WARN_ONCE(dst_pitch % 8 != 0, "dst_pitch is not a multiple of 8\n"); > else > dst_pitch = round_up(linepixels, 8); > No, because we always need to div and round up. The warning is just printed to let know that the dst pitch is not a multiple of 8 as it should be. So callers could be fixed. >> + /* >> + * The cma memory is write-combined so reads are uncached. > > CMA > Yes, this bug me too. But other format helpers (e.g: drm_fb_xrgb8888_to_rgb565 and drm_fb_xrgb8888_to_gray8) had this comment with CMA in lower case. So did the same for consistency. >> + * 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. >> + * >> + * Allocate a buffer to be used for both copying from the cma >> + * memory and to store the intermediate grayscale line pixels. >> + */ >> + src32 = kmalloc(len_src32 + linepixels, GFP_KERNEL); > > size_add() ? > I wasn't familiar with this macro and git grep returned nothing. Then I noticed that it is fairly new, introduced in commit a66866cff71c ("overflow: Implement size_t saturating arithmetic helpers"). git tag --contains a66866cff71c | head -1 next-20220207 So I can't really use it since isn't yet in the latest drm-misc-next base tag :) >> + if (!src32) >> + return; > > ... > >> + /* >> + * 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 reversed mono line conversion function to adjust. >> + */ >> + start_offset = clip->x1 % 8; >> + end_len = clip->x2 % 8; > > ALIGN() ? > But we don't want to align here but to know what's the start and end if is not aligned since that would mean converting to mono in the middle of a byte. Best regards,
Hi Am 11.02.22 um 12:10 schrieb Andy Shevchenko: > On Fri, Feb 11, 2022 at 10:19:23AM +0100, Javier Martinez Canillas wrote: >> Add support to convert from XR24 to reversed monochrome for drivers that >> control monochromatic display panels, that only have 1 bit per pixel. >> >> The function does a line-by-line conversion doing an intermediate step >> first from XR24 to 8-bit grayscale and then to reversed monochrome. >> >> The drm_fb_gray8_to_mono_reversed_line() helper was based on code from >> drivers/gpu/drm/tiny/repaper.c driver. > > ... > >> +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) >> +{ >> + 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; > > This is invariant to the loop, can be moved out. > >> + if (xb == pixels - 1 && end_len) >> + end = end_len; > > Ditto. However it may require to factor out the following loop to a helper. Splitting the loop is much nicer, but leaves corner cases where start and end is in the same byte. Doing this sounds like a premature optimization to me. Best regards Thomas > >> + for (i = start; i < end; i++) { >> + unsigned int x = xb * 8 + i; >> + >> + byte >>= 1; >> + if (src[x] >> 7) >> + byte |= BIT(7); >> + } >> + *dst++ = byte; >> + } >> +} > > ... > >> + /* >> + * The reversed 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); > > round_up() ? > >> + WARN_ONCE(dst_pitch % 8 != 0, "dst_pitch is not a multiple of 8\n"); > > > I would move this to the if conditional, i.e. > > if (dst_pitch) > WARN_ONCE(dst_pitch % 8 != 0, "dst_pitch is not a multiple of 8\n"); > else > dst_pitch = round_up(linepixels, 8); > >> + /* >> + * The cma memory is write-combined so reads are uncached. > > CMA > >> + * 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. >> + * >> + * Allocate a buffer to be used for both copying from the cma >> + * memory and to store the intermediate grayscale line pixels. >> + */ >> + src32 = kmalloc(len_src32 + linepixels, GFP_KERNEL); > > size_add() ? > >> + if (!src32) >> + return; > > ... > >> + /* >> + * 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 reversed mono line conversion function to adjust. >> + */ >> + start_offset = clip->x1 % 8; >> + end_len = clip->x2 % 8; > > ALIGN() ? >
Hi Am 11.02.22 um 10:19 schrieb Javier Martinez Canillas: > Add support to convert from XR24 to reversed monochrome for drivers that > control monochromatic display panels, that only have 1 bit per pixel. > > The function does a line-by-line conversion doing an intermediate step > first from XR24 to 8-bit grayscale and then to reversed monochrome. > > The drm_fb_gray8_to_mono_reversed_line() helper was based on code from > drivers/gpu/drm/tiny/repaper.c driver. > > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> > Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > > Changes in v4: > - Rename end_offset to end_len (Thomas Zimmermann) > - Warn once if dst_pitch is not a multiple of 8 (Thomas Zimmermann) > - Drop drm_fb_gray8_to_mono_reversed() that's not used (Thomas Zimmermann) > - Allocate single buffer for both copy cma memory and gray8 (Thomas Zimmermann) > - Add Thomas Zimmermann Reviewed-by tag to patch adding XR24 -> mono helper. > > Changes in v3: > - Also add a drm_fb_xrgb8888_to_mono_reversed() helper (Thomas Zimmermann) > - Split lines copy to drm_fb_gray8_to_mono_reversed_line() (Thomas Zimmermann) > - Handle case where the source buffer is not aligned to 8 (Thomas Zimmermann) > > drivers/gpu/drm/drm_format_helper.c | 107 ++++++++++++++++++++++++++++ > include/drm/drm_format_helper.h | 4 ++ > 2 files changed, 111 insertions(+) > > diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c > index b981712623d3..ec4e3724ee79 100644 > --- a/drivers/gpu/drm/drm_format_helper.c > +++ b/drivers/gpu/drm/drm_format_helper.c > @@ -591,3 +591,110 @@ 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, 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; > + > + byte >>= 1; > + if (src[x] >> 7) > + byte |= BIT(7); > + } > + *dst++ = byte; > + } > +} > + > +/** > + * 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 *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 cpp = fb->format->cpp[0]; > + unsigned int len_src32 = linepixels * cpp; > + unsigned int start_offset, end_len; > + unsigned int y; > + u8 *mono = dst, *gray8; > + u32 *src32; > + > + if (WARN_ON(fb->format->format != DRM_FORMAT_XRGB8888)) > + return; These WARN macros are deprecated. Use drm_warn, drm_WARN_ONCE, etc instead. Best regards Thomas > + > + /* > + * The reversed 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); > + > + WARN_ONCE(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. > + * > + * Also, format conversion from XR24 to reversed 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. > + */ > + src32 = kmalloc(len_src32 + linepixels, GFP_KERNEL); > + if (!src32) > + return; > + > + 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 reversed 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_reversed_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); > diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h > index b30ed5de0a33..0b0937c0b2f6 100644 > --- a/include/drm/drm_format_helper.h > +++ b/include/drm/drm_format_helper.h > @@ -43,4 +43,8 @@ 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); > + > #endif /* __LINUX_DRM_FORMAT_HELPER_H */
On Fri, Feb 11, 2022 at 12:50:04PM +0100, Javier Martinez Canillas wrote: > On 2/11/22 12:10, Andy Shevchenko wrote: ... > >> + for (xb = 0; xb < pixels; xb++) { > >> + unsigned int start = 0, end = 8; > >> + u8 byte = 0x00; > > > >> + if (xb == 0 && start_offset) > >> + start = start_offset; > > > > This is invariant to the loop, can be moved out. > > > >> + if (xb == pixels - 1 && end_len) > >> + end = end_len; > > > > Ditto. However it may require to factor out the following loop to a helper. > > Not sure I'm following, it's not invariant since it depends on the > loop iterator value. It only applies to the first and last pixels. It's. You simply does it at the last iteration which may be perfectly done outside of the main (aligned) loop. ... > >> + dst_pitch = DIV_ROUND_UP(linepixels, 8); > > > > round_up() ? > > But it's not a round up operation but a div and round up. Indeed. ... > >> + WARN_ONCE(dst_pitch % 8 != 0, "dst_pitch is not a multiple of 8\n"); > > > > > > I would move this to the if conditional, i.e. > > > > if (dst_pitch) > > WARN_ONCE(dst_pitch % 8 != 0, "dst_pitch is not a multiple of 8\n"); > > else > > dst_pitch = round_up(linepixels, 8); > > No, because we always need to div and round up. The warning is just printed to > let know that the dst pitch is not a multiple of 8 as it should be. So callers > could be fixed. Okay, you expect that linepixels to be multiple of 64? Otherwise I didn't get what's going on with this warning. ... > >> + start_offset = clip->x1 % 8; > >> + end_len = clip->x2 % 8; > > > > ALIGN() ? > > But we don't want to align here but to know what's the start and end if is > not aligned since that would mean converting to mono in the middle of a byte. Indeed. Somehow I missed that it's a complimentary to ALIGN().
diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c index b981712623d3..ec4e3724ee79 100644 --- a/drivers/gpu/drm/drm_format_helper.c +++ b/drivers/gpu/drm/drm_format_helper.c @@ -591,3 +591,110 @@ 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, 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; + + byte >>= 1; + if (src[x] >> 7) + byte |= BIT(7); + } + *dst++ = byte; + } +} + +/** + * 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 *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 cpp = fb->format->cpp[0]; + unsigned int len_src32 = linepixels * cpp; + unsigned int start_offset, end_len; + unsigned int y; + u8 *mono = dst, *gray8; + u32 *src32; + + if (WARN_ON(fb->format->format != DRM_FORMAT_XRGB8888)) + return; + + /* + * The reversed 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); + + WARN_ONCE(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. + * + * Also, format conversion from XR24 to reversed 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. + */ + src32 = kmalloc(len_src32 + linepixels, GFP_KERNEL); + if (!src32) + return; + + 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 reversed 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_reversed_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); diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h index b30ed5de0a33..0b0937c0b2f6 100644 --- a/include/drm/drm_format_helper.h +++ b/include/drm/drm_format_helper.h @@ -43,4 +43,8 @@ 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); + #endif /* __LINUX_DRM_FORMAT_HELPER_H */