[PATCH 5/8] drm/mipi-dbi: Prepare framebuffer copy operation in pipe-update helpers
Thomas Zimmermann
tzimmermann at suse.de
Fri Dec 2 11:27:13 UTC 2022
Hi
Am 25.11.22 um 18:39 schrieb Noralf Trønnes:
>
>
> Den 21.11.2022 11.45, skrev Thomas Zimmermann:
>> Move the vmap/vunmap blocks from the inner fb_dirty helpers into the
>> MIPI DBI update helpers. The function calls can result in waiting and/or
>> processing overhead. Reduce the penalties by executing the functions once
>> in the outer-most function of the pipe update.
>>
>> This change also prepares for MIPI DBI for shadow-plane helpers. With
>> shadow-plane helpers, transfer source buffers are mapped into kernel
>> address space automatically.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
>> ---
>> drivers/gpu/drm/drm_mipi_dbi.c | 60 +++++++++++++++++++---------------
>> drivers/gpu/drm/tiny/ili9225.c | 31 ++++++++++++++----
>> drivers/gpu/drm/tiny/st7586.c | 28 +++++++++++-----
>> include/drm/drm_mipi_dbi.h | 6 ++--
>> 4 files changed, 81 insertions(+), 44 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
>> index a6ac565808765..40e59a3a6481e 100644
>> --- a/drivers/gpu/drm/drm_mipi_dbi.c
>> +++ b/drivers/gpu/drm/drm_mipi_dbi.c
>> @@ -192,6 +192,7 @@ EXPORT_SYMBOL(mipi_dbi_command_stackbuf);
>> /**
>> * mipi_dbi_buf_copy - Copy a framebuffer, transforming it if necessary
>> * @dst: The destination buffer
>> + * @src: The source buffer
>> * @fb: The source framebuffer
>> * @clip: Clipping rectangle of the area to be copied
>> * @swap: When true, swap MSB/LSB of 16-bit values
>> @@ -199,12 +200,13 @@ EXPORT_SYMBOL(mipi_dbi_command_stackbuf);
>> * Returns:
>> * Zero on success, negative error code on failure.
>> */
>> -int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
>> +int mipi_dbi_buf_copy(void *dst, struct iosys_map *src, struct drm_framebuffer *fb,
>> struct drm_rect *clip, bool swap)
>> {
>> struct drm_gem_object *gem = drm_gem_fb_get_obj(fb, 0);
>> - struct iosys_map map[DRM_FORMAT_MAX_PLANES];
>> - struct iosys_map data[DRM_FORMAT_MAX_PLANES];
>> + struct iosys_map data[DRM_FORMAT_MAX_PLANES] = {
>> + *src,
>> + };
>
> I would prefer that you used src directly when calling the drm_fb_*
> functions, data can be anything and to me it isn't clear what that
> contains when I see it (ofc now I know, but next year I've forgotten).
> And is the array necessary, this helper will only ever support one color
> plane after all?
Will be fixed.
>
>> struct iosys_map dst_map = IOSYS_MAP_INIT_VADDR(dst);
>> int ret;
>>
>> @@ -212,10 +214,6 @@ int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
>> if (ret)
>> return ret;
>>
>> - ret = drm_gem_fb_vmap(fb, map, data);
>> - if (ret)
>> - goto out_drm_gem_fb_end_cpu_access;
>> -
>> switch (fb->format->format) {
>> case DRM_FORMAT_RGB565:
>> if (swap)
>> @@ -232,8 +230,6 @@ int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
>> ret = -EINVAL;
>> }
>>
>> - drm_gem_fb_vunmap(fb, map);
>> -out_drm_gem_fb_end_cpu_access:
>> drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
>>
>> return ret;
>> @@ -257,10 +253,9 @@ static void mipi_dbi_set_window_address(struct mipi_dbi_dev *dbidev,
>> ys & 0xff, (ye >> 8) & 0xff, ye & 0xff);
>> }
>>
>> -static void mipi_dbi_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
>> +static void mipi_dbi_fb_dirty(struct iosys_map *src, struct drm_framebuffer *fb,
>> + struct drm_rect *rect)
>> {
>> - struct iosys_map map[DRM_FORMAT_MAX_PLANES];
>> - struct iosys_map data[DRM_FORMAT_MAX_PLANES];
>> struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(fb->dev);
>> unsigned int height = rect->y2 - rect->y1;
>> unsigned int width = rect->x2 - rect->x1;
>> @@ -270,16 +265,9 @@ static void mipi_dbi_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
>> bool full;
>> void *tr;
>>
>> - if (WARN_ON(!fb))
>> - return;
>> -
>> if (!drm_dev_enter(fb->dev, &idx))
>> return;
>>
>> - ret = drm_gem_fb_vmap(fb, map, data);
>> - if (ret)
>> - goto err_drm_dev_exit;
>> -
>> full = width == fb->width && height == fb->height;
>>
>> DRM_DEBUG_KMS("Flushing [FB:%d] " DRM_RECT_FMT "\n", fb->base.id, DRM_RECT_ARG(rect));
>> @@ -287,11 +275,11 @@ static void mipi_dbi_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
>> if (!dbi->dc || !full || swap ||
>> fb->format->format == DRM_FORMAT_XRGB8888) {
>> tr = dbidev->tx_buf;
>> - ret = mipi_dbi_buf_copy(dbidev->tx_buf, fb, rect, swap);
>> + ret = mipi_dbi_buf_copy(tr, src, fb, rect, swap);
>> if (ret)
>> goto err_msg;
>> } else {
>> - tr = data[0].vaddr; /* TODO: Use mapping abstraction properly */
>> + tr = src->vaddr; /* TODO: Use mapping abstraction properly */
>> }
>>
>> mipi_dbi_set_window_address(dbidev, rect->x1, rect->x2 - 1, rect->y1,
>> @@ -303,9 +291,6 @@ static void mipi_dbi_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
>> if (ret)
>> drm_err_once(fb->dev, "Failed to update display %d\n", ret);
>>
>> - drm_gem_fb_vunmap(fb, map);
>> -
>> -err_drm_dev_exit:
>> drm_dev_exit(idx);
>> }
>>
>> @@ -338,14 +323,27 @@ EXPORT_SYMBOL(mipi_dbi_pipe_mode_valid);
>> void mipi_dbi_pipe_update(struct drm_simple_display_pipe *pipe,
>> struct drm_plane_state *old_state)
>> {
>> + struct iosys_map map[DRM_FORMAT_MAX_PLANES];
>> + struct iosys_map data[DRM_FORMAT_MAX_PLANES];
>
> These should have been zeroed to avoid UBSAN complaint, but you'll
> remove these later so not so important.
Will be fixed.
But do you know how these warnings happen? I went through the helpers
before and changed them to only access the format-info's relevant
planes. No unintialized memory access should be reported.
>
>> struct drm_plane_state *state = pipe->plane.state;
>> + struct drm_framebuffer *fb = state->fb;
>> struct drm_rect rect;
>> + int ret;
>>
>> if (!pipe->crtc.state->active)
>> return;
>>
>> + if (WARN_ON(!fb))
>> + return;
>> +
>> + ret = drm_gem_fb_vmap(fb, map, data);
>> + if (ret)
>> + return;
>> +
>> if (drm_atomic_helper_damage_merged(old_state, state, &rect))
>> - mipi_dbi_fb_dirty(state->fb, &rect);
>> + mipi_dbi_fb_dirty(&data[0], fb, &rect);
>> +
>> + drm_gem_fb_vunmap(fb, map);
>> }
>> EXPORT_SYMBOL(mipi_dbi_pipe_update);
>>
>> @@ -373,14 +371,22 @@ void mipi_dbi_enable_flush(struct mipi_dbi_dev *dbidev,
>> .y1 = 0,
>> .y2 = fb->height,
>> };
>> - int idx;
>> + struct iosys_map map[DRM_FORMAT_MAX_PLANES];
>> + struct iosys_map data[DRM_FORMAT_MAX_PLANES];
>> + int idx, ret;
>>
>> if (!drm_dev_enter(&dbidev->drm, &idx))
>> return;
>>
>> - mipi_dbi_fb_dirty(fb, &rect);
>> + ret = drm_gem_fb_vmap(fb, map, data);
>> + if (ret)
>> + goto err_drm_dev_exit;
>> +
>> + mipi_dbi_fb_dirty(&data[0], fb, &rect);
>> backlight_enable(dbidev->backlight);
>>
>> + drm_gem_fb_vunmap(fb, map);
>> +err_drm_dev_exit:
>> drm_dev_exit(idx);
>> }
>> EXPORT_SYMBOL(mipi_dbi_enable_flush);
>> diff --git a/drivers/gpu/drm/tiny/ili9225.c b/drivers/gpu/drm/tiny/ili9225.c
>> index f05a2d25866c1..0115c4090f9e7 100644
>> --- a/drivers/gpu/drm/tiny/ili9225.c
>> +++ b/drivers/gpu/drm/tiny/ili9225.c
>> @@ -25,6 +25,7 @@
>> #include <drm/drm_framebuffer.h>
>> #include <drm/drm_gem_atomic_helper.h>
>> #include <drm/drm_gem_dma_helper.h>
>> +#include <drm/drm_gem_framebuffer_helper.h>
>> #include <drm/drm_managed.h>
>> #include <drm/drm_mipi_dbi.h>
>> #include <drm/drm_rect.h>
>> @@ -76,9 +77,9 @@ static inline int ili9225_command(struct mipi_dbi *dbi, u8 cmd, u16 data)
>> return mipi_dbi_command_buf(dbi, cmd, par, 2);
>> }
>>
>> -static void ili9225_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
>> +static void ili9225_fb_dirty(struct iosys_map *src, struct drm_framebuffer *fb,
>> + struct drm_rect *rect)
>> {
>> - struct drm_gem_dma_object *dma_obj = drm_fb_dma_get_gem_obj(fb, 0);
>> struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(fb->dev);
>> unsigned int height = rect->y2 - rect->y1;
>> unsigned int width = rect->x2 - rect->x1;
>> @@ -100,11 +101,11 @@ static void ili9225_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
>> if (!dbi->dc || !full || swap ||
>> fb->format->format == DRM_FORMAT_XRGB8888) {
>> tr = dbidev->tx_buf;
>> - ret = mipi_dbi_buf_copy(dbidev->tx_buf, fb, rect, swap);
>> + ret = mipi_dbi_buf_copy(tr, src, fb, rect, swap);
>> if (ret)
>> goto err_msg;
>> } else {
>> - tr = dma_obj->vaddr;
>> + tr = src->vaddr; /* TODO: Use mapping abstraction properly */
>> }
>>
>> switch (dbidev->rotation) {
>> @@ -162,14 +163,24 @@ static void ili9225_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
>> static void ili9225_pipe_update(struct drm_simple_display_pipe *pipe,
>> struct drm_plane_state *old_state)
>> {
>> + struct iosys_map map[DRM_FORMAT_MAX_PLANES];
>> + struct iosys_map data[DRM_FORMAT_MAX_PLANES];
Will be fixed.
>> struct drm_plane_state *state = pipe->plane.state;
>> + struct drm_framebuffer *fb = state->fb;
>> struct drm_rect rect;
>> + int ret;
>>
>> if (!pipe->crtc.state->active)
>> return;
>>
>> + ret = drm_gem_fb_vmap(fb, map, data);
>> + if (ret)
>> + return;
>> +
>> if (drm_atomic_helper_damage_merged(old_state, state, &rect))
>> - ili9225_fb_dirty(state->fb, &rect);
>> + ili9225_fb_dirty(&data[0], fb, &rect);
>> +
>> + drm_gem_fb_vunmap(fb, map);
>> }
>>
>> static void ili9225_pipe_enable(struct drm_simple_display_pipe *pipe,
>> @@ -186,6 +197,8 @@ static void ili9225_pipe_enable(struct drm_simple_display_pipe *pipe,
>> .y1 = 0,
>> .y2 = fb->height,
>> };
>> + struct iosys_map map[DRM_FORMAT_MAX_PLANES];
>> + struct iosys_map data[DRM_FORMAT_MAX_PLANES];
Will be fixed.
>> int ret, idx;
>> u8 am_id;
>>
>> @@ -276,7 +289,13 @@ static void ili9225_pipe_enable(struct drm_simple_display_pipe *pipe,
>>
>> ili9225_command(dbi, ILI9225_DISPLAY_CONTROL_1, 0x1017);
>>
>> - ili9225_fb_dirty(fb, &rect);
>> + ret = drm_gem_fb_vmap(fb, map, data);
>> + if (ret)
>> + goto out_exit;
>> +
>> + ili9225_fb_dirty(&data[0], fb, &rect);
>> +
>> + drm_gem_fb_vunmap(fb, map);
>> out_exit:
>> drm_dev_exit(idx);
>> }
>> diff --git a/drivers/gpu/drm/tiny/st7586.c b/drivers/gpu/drm/tiny/st7586.c
>> index 6bdd23e2a47c7..e773b1f2fd5f3 100644
>> --- a/drivers/gpu/drm/tiny/st7586.c
>> +++ b/drivers/gpu/drm/tiny/st7586.c
>> @@ -92,25 +92,24 @@ static void st7586_xrgb8888_to_gray332(u8 *dst, void *vaddr,
>> kfree(buf);
>> }
>>
>> -static int st7586_buf_copy(void *dst, struct drm_framebuffer *fb,
>> +static int st7586_buf_copy(void *dst, struct iosys_map *src, struct drm_framebuffer *fb,
>> struct drm_rect *clip)
>> {
>> - struct drm_gem_dma_object *dma_obj = drm_fb_dma_get_gem_obj(fb, 0);
>> - void *src = dma_obj->vaddr;
>> - int ret = 0;
>> + int ret;
>>
>> ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
>> if (ret)
>> return ret;
>>
>> - st7586_xrgb8888_to_gray332(dst, src, fb, clip);
>> + st7586_xrgb8888_to_gray332(dst, src->vaddr, fb, clip);
>>
>> drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
>>
>> return 0;
>> }
>>
>> -static void st7586_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
>> +static void st7586_fb_dirty(struct iosys_map *src, struct drm_framebuffer *fb,
>> + struct drm_rect *rect)
>> {
>> struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(fb->dev);
>> struct mipi_dbi *dbi = &dbidev->dbi;
>> @@ -125,7 +124,7 @@ static void st7586_fb_dirty(struct drm_framebuffer *fb, struct drm_rect *rect)
>>
>> DRM_DEBUG_KMS("Flushing [FB:%d] " DRM_RECT_FMT "\n", fb->base.id, DRM_RECT_ARG(rect));
>>
>> - ret = st7586_buf_copy(dbidev->tx_buf, fb, rect);
>> + ret = st7586_buf_copy(dbidev->tx_buf, src, fb, rect);
>> if (ret)
>> goto err_msg;
>>
>> @@ -154,13 +153,19 @@ static void st7586_pipe_update(struct drm_simple_display_pipe *pipe,
>> struct drm_plane_state *old_state)
>> {
>> struct drm_plane_state *state = pipe->plane.state;
>> + struct drm_framebuffer *fb = state->fb;
>> + struct drm_gem_dma_object *dma_obj;
>> + struct iosys_map src;
>> struct drm_rect rect;
>>
>> if (!pipe->crtc.state->active)
>> return;
>>
>> + dma_obj = drm_fb_dma_get_gem_obj(fb, 0);
>> + iosys_map_set_vaddr(&src, dma_obj->vaddr);
>> +
>
> You use drm_gem_fb_vmap() in the other places but here you access the
> object directly (and in the next hunk), but again not so important since
> it goes away in a later patch.
I'll update this patch to use drm_gem_fb_vmap() consistently.
>
> With the comments considered:
>
> Reviewed-by: Noralf Trønnes <noralf at tronnes.org>
Thanks.
Best regards
Thomas
>
>> if (drm_atomic_helper_damage_merged(old_state, state, &rect))
>> - st7586_fb_dirty(state->fb, &rect);
>> + st7586_fb_dirty(&src, fb, &rect);
>> }
>>
>> static void st7586_pipe_enable(struct drm_simple_display_pipe *pipe,
>> @@ -176,6 +181,8 @@ static void st7586_pipe_enable(struct drm_simple_display_pipe *pipe,
>> .y1 = 0,
>> .y2 = fb->height,
>> };
>> + struct drm_gem_dma_object *dma_obj;
>> + struct iosys_map src;
>> int idx, ret;
>> u8 addr_mode;
>>
>> @@ -235,7 +242,10 @@ static void st7586_pipe_enable(struct drm_simple_display_pipe *pipe,
>>
>> msleep(100);
>>
>> - st7586_fb_dirty(fb, &rect);
>> + dma_obj = drm_fb_dma_get_gem_obj(fb, 0);
>> + iosys_map_set_vaddr(&src, dma_obj->vaddr);
>> +
>> + st7586_fb_dirty(&src, fb, &rect);
>>
>> mipi_dbi_command(dbi, MIPI_DCS_SET_DISPLAY_ON);
>> out_exit:
>> diff --git a/include/drm/drm_mipi_dbi.h b/include/drm/drm_mipi_dbi.h
>> index 8c4ea7956d61d..36ac8495566b0 100644
>> --- a/include/drm/drm_mipi_dbi.h
>> +++ b/include/drm/drm_mipi_dbi.h
>> @@ -13,9 +13,10 @@
>> #include <drm/drm_simple_kms_helper.h>
>>
>> struct drm_rect;
>> -struct spi_device;
>> struct gpio_desc;
>> +struct iosys_map;
>> struct regulator;
>> +struct spi_device;
>>
>> /**
>> * struct mipi_dbi - MIPI DBI interface
>> @@ -176,8 +177,9 @@ int mipi_dbi_command_read(struct mipi_dbi *dbi, u8 cmd, u8 *val);
>> int mipi_dbi_command_buf(struct mipi_dbi *dbi, u8 cmd, u8 *data, size_t len);
>> int mipi_dbi_command_stackbuf(struct mipi_dbi *dbi, u8 cmd, const u8 *data,
>> size_t len);
>> -int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
>> +int mipi_dbi_buf_copy(void *dst, struct iosys_map *src, struct drm_framebuffer *fb,
>> struct drm_rect *clip, bool swap);
>> +
>> /**
>> * mipi_dbi_command - MIPI DCS command with optional parameter(s)
>> * @dbi: MIPI DBI structure
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20221202/7e2eab6c/attachment-0001.sig>
More information about the dri-devel
mailing list