linux-kernel - Re: [PATCH v5 2/6] drm/format-helper: Add drm_fb_xrgb8888_to_mono

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <e50211b1-08fa-de09-8948-664493423c33@redhat.com>
Date:   Mon, 14 Feb 2022 09:57:21 +0100
From:   Javier Martinez Canillas <javierm@...hat.com>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Daniel Vetter <daniel.vetter@...ll.ch>,
        Linux Fbdev development list <linux-fbdev@...r.kernel.org>,
        Sam Ravnborg <sam@...nborg.org>,
        Maxime Ripard <maxime@...no.tech>,
        DRI Development <dri-devel@...ts.freedesktop.org>,
        Noralf Trønnes <noralf@...nnes.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Daniel Vetter <daniel@...ll.ch>,
        David Airlie <airlied@...ux.ie>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>
Subject: Re: [PATCH v5 2/6] drm/format-helper: Add
 drm_fb_xrgb8888_to_mono_reversed()

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