Re: [PATCH v5 2/6] drm/format-helper: Add drm_fb_xrgb8888_to_mono_reversed()

[Javier Martinez Canillas]

Hello Geert,

Thanks for your feedback.

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

[snip]

>> +
>> +               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]

>> +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]

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

[snip]

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