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,