Hello Geert,
Thanks a lot for your feedback.
On 3/8/22 17:13, Geert Uytterhoeven wrote:
[snip]
+static void drm_fb_gray8_to_mono_reversed_line(u8 *dst, const u8 *src, unsigned int pixels,
"pixels" is not the number of pixels to process, but the number of bytes in the monochrome destination buffer.
Right, that parameter name is misleading / incorrect indeed. Probably should be changed by dst_pitch or dst_stride.
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;}The above is IMHO very hard to read. I think it can be made simpler by passing the total number of pixels to process instead of "pixels" (which is bytes) and "end_len".
Agreed that's hard to read. I think is better if you propose a patch with your idea to make it simpler.
[snip]
+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;drm_rect_height(clip)?
Yes, unsure why didn't use it since used drm_rect_width() for the width.
unsigned int cpp = fb->format->cpp[0];unsigned int len_src32 = linepixels * cpp;struct drm_device *dev = fb->dev;unsigned int start_offset, end_len;unsigned int y;u8 *mono = dst, *gray8;u32 *src32;if (drm_WARN_ON(dev, 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);This is not correct when clip->x1 is not a multiple of 8 pixels. Should be:
DIV_ROUND_UP(linepixels + clip->x1 % 8, 8);
Agreed.
drm_WARN_ONCE(dev, dst_pitch % 8 != 0, "dst_pitch is not a multiple of 8\n");This triggers for me: dst_pitch = 1. Which is perfectly fine, when flashing an 8-pixel wide cursor ;-)
Indeed. I think we should just drop that warn.
Do you want me to post patches for all these or would you do it when simplifying the drm_fb_gray8_to_mono_reversed_line() logic ?