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
