linux-kernel - Re: [PATCH v6 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <809430bb-4ffb-3bd4-7062-ec8b78245387@redhat.com>
Date:   Wed, 9 Mar 2022 13:03:05 +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>,
        Linux Fbdev development list <linux-fbdev@...r.kernel.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Daniel Vetter <daniel.vetter@...ll.ch>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        Sam Ravnborg <sam@...nborg.org>,
        DRI Development <dri-devel@...ts.freedesktop.org>,
        Maxime Ripard <maxime@...no.tech>,
        Noralf Trønnes <noralf@...nnes.org>,
        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 v6 2/6] drm/format-helper: Add
 drm_fb_xrgb8888_to_mono_reversed()

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 ?

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat