linux-kernel - Re: [PATCH 4/8] drm/ssd130x: Add support for DRM_FORMAT

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAMuHMdV1MXexXuRuvW2ap5KA51q_3h9X8jXdYXtFb2RF-BBLnw@mail.gmail.com>
Date:   Fri, 14 Jul 2023 13:26:47 +0200
From:   Geert Uytterhoeven <geert@...ux-m68k.org>
To:     Javier Martinez Canillas <javierm@...hat.com>
Cc:     Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/8] drm/ssd130x: Add support for DRM_FORMAT_R1

Hi Javier,

On Fri, Jul 14, 2023 at 12:14 PM Javier Martinez Canillas
<javierm@...hat.com> wrote:
> Geert Uytterhoeven <geert@...ux-m68k.org> writes:
> Thanks a lot for your patch, this has been on my TODO for some time!
>
> > The native display format is monochrome light-on-dark (R1).
> > Hence add support for R1, so monochrome applications can avoid the
> > overhead of back-and-forth conversions between R1 and XR24.
> >
> > Signed-off-by: Geert Uytterhoeven <geert@...ux-m68k.org>
> > ---
> > This work interfered with commit 49d7d581ceaf4cf8 ("drm/ssd130x: Don't
> > allocate buffers on each plane update") in drm-misc/for-linux-next,
> > which always allocates the buffer upfront, while it is no longer needed
> > when never using XR24.
>
> you mean R1 here, right ?

I did mean R1. I think you missed the double negation.

> It's still used in ssd130x_clear_screen() though.

I guess it became worthwhile to make ssd130x_clear_screen()
do memset(data_array, 0, ...) and call ssd130x_write_data() directly,
avoiding the pointless reshuffling of black pixels in
ssd130x_update_rect()?

> > Probably ssd130x->buffer should be allocated on first use.
>
> Yes, that makes sense.
>
> > And why not allocate the buffers using devm_kcalloc()?
>
> I think there are some lifetimes discrepancies between struct device and
> struct drm_device objects. But we could use drm_device managed resources
> helpers, i.e: drmm_kzalloc().

The display should not be updated after .remove(), so I think plain
devm_kcalloc() should be fine.

> >  drivers/gpu/drm/solomon/ssd130x.c | 57 ++++++++++++++++++++++---------
> >  1 file changed, 40 insertions(+), 17 deletions(-)
> >
>
> [...]
>
> > +     case DRM_FORMAT_XRGB8888:
> > +             dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), 8);
> > +             buf = ssd130x->buffer;
> > +             if (!buf)
> > +                     return 0;
> > +
>
> I think this check is not needed anymore now that the driver won't attempt
> to update planes for disabled CRTCs ?

Probably. We do need it here when allocating on first use.

> It's OK for me to be paranoid though, specially after the other issue that
> you found. So I'll let you decide if you think is worth to keep the check.

I kept the check as I only moved/shifted that part of the code.

> Reviewed-by: Javier Martinez Canillas <javierm@...hat.com>

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds