| Message ID | 72746f6d9c47f09fc057ad7a4bbb3b7f423af803.1689252746.git.geert@linux-m68k.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: fb-helper/ssd130x: Add support for DRM_FORMAT_R1 | expand |
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>
Hi Javier, On Fri, Jul 14, 2023 at 12:14 PM Javier Martinez Canillas <javierm@redhat.com> wrote: > Geert Uytterhoeven <geert@linux-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@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 ? 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@redhat.com> Thanks! Gr{oetje,eeting}s, Geert
Geert Uytterhoeven <geert@linux-m68k.org> writes: Hello Geert, > Hi Javier, > > On Fri, Jul 14, 2023 at 12:14 PM Javier Martinez Canillas > <javierm@redhat.com> wrote: >> Geert Uytterhoeven <geert@linux-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@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 ? > > I did mean R1. I think you missed the double negation. > I did indeed. As a non-native english speaker, I find it very hard to parse double negations :) >> 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()? > I think so, yeah. >> > 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. > That was precisely my point, that there could be atomic commits even after the driver has been removed (e.g: if using DRM fbdev emulation, user-space can keep the /dev/fb0 opened and continue updating the framebuffer. That's not released until the fd is closed and struct fb_ops .fb_destroy called. But that's a general rule in DRM, any user-visible resource must not be allocated using device managed resources and instead use the drm_device managed resources helpers. To make sure that are not released until the last call to drm_dev_put(): https://docs.kernel.org/gpu/drm-internals.html#device-instance-and-driver-handling
Hi Javier, On Fri, Jul 14, 2023 at 2:35 PM Javier Martinez Canillas <javierm@redhat.com> wrote: > Geert Uytterhoeven <geert@linux-m68k.org> writes: > > On Fri, Jul 14, 2023 at 12:14 PM Javier Martinez Canillas > > <javierm@redhat.com> wrote: > >> Geert Uytterhoeven <geert@linux-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@linux-m68k.org> > >> > 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. > > That was precisely my point, that there could be atomic commits even after > the driver has been removed (e.g: if using DRM fbdev emulation, user-space > can keep the /dev/fb0 opened and continue updating the framebuffer. That's > not released until the fd is closed and struct fb_ops .fb_destroy called. > > But that's a general rule in DRM, any user-visible resource must not be > allocated using device managed resources and instead use the drm_device > managed resources helpers. To make sure that are not released until the > last call to drm_dev_put(): These buffers are not user-visible, so they should not be accessed after .remove(). When these are accessed, the next step would be to write the buffer data to the device, which would also fail miserably, as the regmap, GPIO, and regulator are hardware resources managed through devm_*(). Gr{oetje,eeting}s, Geert
Geert Uytterhoeven <geert@linux-m68k.org> writes: Hello Geert, > Hi Javier, [...] >> > >> > The display should not be updated after .remove(), so I think plain >> > devm_kcalloc() should be fine. >> >> That was precisely my point, that there could be atomic commits even after >> the driver has been removed (e.g: if using DRM fbdev emulation, user-space >> can keep the /dev/fb0 opened and continue updating the framebuffer. That's >> not released until the fd is closed and struct fb_ops .fb_destroy called. >> >> But that's a general rule in DRM, any user-visible resource must not be >> allocated using device managed resources and instead use the drm_device >> managed resources helpers. To make sure that are not released until the >> last call to drm_dev_put(): > > These buffers are not user-visible, so they should not be accessed > after .remove(). When these are accessed, the next step would be > to write the buffer data to the device, which would also fail miserably, > as the regmap, GPIO, and regulator are hardware resources managed > through devm_*(). > Right, given that we do the shadow buffer -> native buffer copy, I thought of it as if as a user-visible (well, user-accessible I guess) but you are correct that's not and trying to write to the device will fail anyways. So devm_* should be enough indeed and if there are problems with that, it would be a different bug in the driver to fix.
diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c index 8ef5f61854fd7340..130e33a1ba3cba00 100644 --- a/drivers/gpu/drm/solomon/ssd130x.c +++ b/drivers/gpu/drm/solomon/ssd130x.c @@ -466,15 +466,14 @@ static int ssd130x_init(struct ssd130x_device *ssd130x) SSD130X_SET_ADDRESS_MODE_HORIZONTAL); } -static int ssd130x_update_rect(struct ssd130x_device *ssd130x, struct drm_rect *rect) +static int ssd130x_update_rect(struct ssd130x_device *ssd130x, u8 *buf, + unsigned int pitch, struct drm_rect *rect) { unsigned int x = rect->x1; unsigned int y = rect->y1; - u8 *buf = ssd130x->buffer; u8 *data_array = ssd130x->data_array; unsigned int width = drm_rect_width(rect); unsigned int height = drm_rect_height(rect); - unsigned int line_length = DIV_ROUND_UP(width, 8); unsigned int page_height = ssd130x->device_info->page_height; unsigned int pages = DIV_ROUND_UP(height, page_height); struct drm_device *drm = &ssd130x->drm; @@ -534,7 +533,7 @@ if (!data_array) { pr_info("%s: data_array not yet initialized\n", __func__); re u8 data = 0; for (k = 0; k < m; k++) { - u8 byte = buf[(8 * i + k) * line_length + j / 8]; + u8 byte = buf[(8 * i + k) * pitch + j / 8]; u8 bit = (byte >> (j % 8)) & 1; data |= bit << k; @@ -570,6 +569,8 @@ if (!data_array) { pr_info("%s: data_array not yet initialized\n", __func__); re static void ssd130x_clear_screen(struct ssd130x_device *ssd130x) { + unsigned int pitch = DIV_ROUND_UP(ssd130x->width, 8); + u8 *buf = ssd130x->buffer; struct drm_rect fullscreen = { .x1 = 0, .x2 = ssd130x->width, @@ -577,7 +578,7 @@ static void ssd130x_clear_screen(struct ssd130x_device *ssd130x) .y2 = ssd130x->height, }; - ssd130x_update_rect(ssd130x, &fullscreen); + ssd130x_update_rect(ssd130x, buf, pitch, &fullscreen); } static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_map *vmap, @@ -588,27 +589,48 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_m struct iosys_map dst; unsigned int dst_pitch; int ret = 0; - u8 *buf = ssd130x->buffer; - - if (!buf) - return 0; + u8 *buf; /* Align y to display page boundaries */ rect->y1 = round_down(rect->y1, page_height); rect->y2 = min_t(unsigned int, round_up(rect->y2, page_height), ssd130x->height); - dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), 8); + switch (fb->format->format) { + case DRM_FORMAT_R1: + /* Align x to byte boundaries */ + rect->x1 = round_down(rect->x1, 8); + rect->x2 = round_up(rect->x2, 8); - ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE); - if (ret) - return ret; + ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE); + if (ret) + return ret; - iosys_map_set_vaddr(&dst, buf); - drm_fb_xrgb8888_to_mono(&dst, &dst_pitch, vmap, fb, rect); + dst_pitch = fb->pitches[0]; + buf = vmap[0].vaddr + rect->y1 * dst_pitch + rect->x1 / 8; - drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE); + ssd130x_update_rect(ssd130x, buf, dst_pitch, rect); - ssd130x_update_rect(ssd130x, rect); + drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE); + break; + + case DRM_FORMAT_XRGB8888: + dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), 8); + buf = ssd130x->buffer; + if (!buf) + return 0; + + ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE); + if (ret) + return ret; + + iosys_map_set_vaddr(&dst, buf); + drm_fb_xrgb8888_to_mono(&dst, &dst_pitch, vmap, fb, rect); + + drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE); + + ssd130x_update_rect(ssd130x, buf, dst_pitch, rect); + break; + } return ret; } @@ -797,6 +819,7 @@ static const struct drm_mode_config_funcs ssd130x_mode_config_funcs = { }; static const uint32_t ssd130x_formats[] = { + DRM_FORMAT_R1, DRM_FORMAT_XRGB8888, };
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. Probably ssd130x->buffer should be allocated on first use. And why not allocate the buffers using devm_kcalloc()? --- drivers/gpu/drm/solomon/ssd130x.c | 57 ++++++++++++++++++++++--------- 1 file changed, 40 insertions(+), 17 deletions(-)