Re: [PATCH 4/8] drm/ssd130x: Add support for DRM_FORMAT_R1

[Geert Uytterhoeven]

Hi Javier,

On Fri, Jul 14, 2023 at 12:14 PM Javier Martinez Canillas
<javierm@xxxxxxxxxx> wrote:
> Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx>

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

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