| Message ID | 20221121104532.8301-6-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/mipi-dbi: Convert to shadow-plane helpers | expand |
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@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? > 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. > 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]; > 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]; > 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. With the comments considered: Reviewed-by: Noralf Trønnes <noralf@tronnes.org> > 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
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@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@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
>> >> 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. And after looking at the impact and churn, I rather go with the existing code that initializes from the GEM DMA object. Noralf, is there a reason why most of MIPI DBI uses DMA helpers? In terms of flexibility and resource consumption, wouldn't SHMEM helpers be a better fit? Best regards Thomas > >> >> With the comments considered: >> >> Reviewed-by: Noralf Trønnes <noralf@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 >
Den 02.12.2022 12.27, skrev Thomas Zimmermann: > 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@suse.de> >>> --- >>> @@ -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. > It happens with imported buffers, iosys_map_clear() looks at the uninitialized boolean variable ->is_iomem: drm_gem_fb_vmap -> ... -> dma_buf_vmap -> iosys_map_clear static inline void iosys_map_clear(struct iosys_map *map) { if (map->is_iomem) { map->vaddr_iomem = NULL; map->is_iomem = false; } else { map->vaddr = NULL; } } Noralf.
Den 02.12.2022 12.50, skrev Thomas Zimmermann: > >>> >>> 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. > > And after looking at the impact and churn, I rather go with the existing > code that initializes from the GEM DMA object. > > Noralf, is there a reason why most of MIPI DBI uses DMA helpers? In > terms of flexibility and resource consumption, wouldn't SHMEM helpers be > a better fit? > The SHMEM helper didn't exist at the time. The SPI subsystem doesn't have an interface for scatter/gather transfers and DMA is needed in order to run at full speed. SPI does convert an is_vmalloc_addr() buffer to an sg list of pages in spi_map_buf() so it solves the missing interface that way. I have never tried to pass a shmem buffer to spi_sync() so I don't know if it works, but I guess that it will work. Bare in mind that theses buffers are at most 400k in size so I'm not sure there's much to gain in term of resources at least. Noralf.
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, + }; 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]; 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]; 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]; 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); + 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
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@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(-)