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
