Messages in this thread | | | From | Javier Martinez Canillas <> | Subject | Re: [PATCH 4/8] drm/ssd130x: Add support for DRM_FORMAT_R1 | Date | Fri, 14 Jul 2023 12:14:44 +0200 |
| |
Geert Uytterhoeven <geert@linux-m68k.org> writes:
Hello Geert,
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@linux-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 ? It's still used in ssd130x_clear_screen() though.
> 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().
> --- > 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 ?
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.
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
-- Best regards,
Javier Martinez Canillas Core Platforms Red Hat
| |