Hello Geert,

Thanks for your feedback.

On 2/12/22 16:54, Geert Uytterhoeven wrote:

[snip]

quoted

+
+               for (i = start; i < end; i++) {
+                       unsigned int x = xb * 8 + i;
+
+                       byte >>= 1;
+                       if (src[x] >> 7)
+                               byte |= BIT(7);
+               }

    x = xb * 8;
    for (i = start; i < end; i++) {
            byte >>= 1;
            byte |= src[x + i] & BIT(7);
    }

I think the original version from Noralf is easier to read and understand.

It makes explicit that the bit is set if the grayscale value is >= 128.

[snip]

quoted

+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)
+{

[snip]

quoted

+       u8 *mono = dst, *gray8;
+       u32 *src32;

[snip]

quoted

+        *
+        * 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;

The cast can be removed if src32 is changed to "void *".
For symmetry, gray8 and mono can be changed to "void *", too.

Yes, but these being "u32 *" and "u8 *" also express that the source buffer
contains 32-bit XRGB8888 pixels, the intermediate buffer a 8-bit grayscale
pixel format and the destination buffer a 8-bit packed reversed monochrome.

Using "void *" for these would make that less clear when reading the code IMO.

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat