Hi Am 04.02.22 um 20:31 schrieb Javier Martinez Canillas:
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.
After sending the mail, I realized that some code copies only parts of the source around; specifically for damage handling. None of this is aligned to multiple of 8. So the copying could start and end in the middle of bytes. You'd need a pixel-offset value of some sort.
If you don't want to handle this now, maybe at least detect this case and put a warning somewhere.
[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 ?
I mean that if there are 80 pixel on a scanline, the value of dst_pitch is already 10. These pitch values are always given in bytes.
Best regards Thomas
+ 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,
-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
