| Message ID | 20230705060719.14700-1-yongqiang.niu@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v5] drm/mediatek: add dma buffer control for drm plane disable | expand |
Hi, On Wed, Jul 05, 2023 at 02:07:18PM +0800, Yongqiang Niu wrote: > dma buffer release before overlay disable, that will cause > m4u translation fault warning. > > add dma buffer control flow in mediatek driver: > get dma buffer when drm plane disable > put dma buffer when overlay really disable > > Fixes: 41016fe1028e ("drm: Rename plane->state variables in atomic update and disable") > Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com> I think we need more details in the commit message about what the issue is exactly and how it's fixed. This definitely feels like it's not something drivers should have to do. Maxime
Hi, Maxime: On Wed, 2023-07-05 at 09:48 +0200, Maxime Ripard wrote: > Hi, > > On Wed, Jul 05, 2023 at 02:07:18PM +0800, Yongqiang Niu wrote: > > dma buffer release before overlay disable, that will cause > > m4u translation fault warning. > > > > add dma buffer control flow in mediatek driver: > > get dma buffer when drm plane disable > > put dma buffer when overlay really disable > > > > Fixes: 41016fe1028e ("drm: Rename plane->state variables in atomic > > update and disable") > > Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com> > > I think we need more details in the commit message about what the > issue > is exactly and how it's fixed. > > This definitely feels like it's not something drivers should have to > do. Because MediaTek display hardware has no internal SRAM which could store the on screen buffer, plane should be updated in vblank period to prevent tearing effect. In MediaTek plane disable callback function, if we wait vblank period to disable plane, the software would be blocked for a long time. If there are four planes to be disabled, the total blocking time would be almost 4 vsync time. So this patch is just a work around to use get_dma_buf() to prevent buffer to be freed before vblank period and not block the software. For the behavior DRM core free plane buffer just after plane disable callback function return, I'm not sure it's DRM core's bug or it's DRM core's correct behavior. If this is correct behavior, it means that DRM core assume that hardware should have internal SRAM but MediaTek display hardware violate this assumption. If so, I think I could just accept this work around patch. Regards, CK > > Maxime
Hi, Maxime: On Tue, 2023-08-01 at 16:02 +0800, CK Hu wrote: > Hi, Maxime: > > On Wed, 2023-07-05 at 09:48 +0200, Maxime Ripard wrote: > > Hi, > > > > On Wed, Jul 05, 2023 at 02:07:18PM +0800, Yongqiang Niu wrote: > > > dma buffer release before overlay disable, that will cause > > > m4u translation fault warning. > > > > > > add dma buffer control flow in mediatek driver: > > > get dma buffer when drm plane disable > > > put dma buffer when overlay really disable > > > > > > Fixes: 41016fe1028e ("drm: Rename plane->state variables in > > > atomic > > > update and disable") > > > Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com> > > > > I think we need more details in the commit message about what the > > issue > > is exactly and how it's fixed. > > > > This definitely feels like it's not something drivers should have > > to > > do. > > Because MediaTek display hardware has no internal SRAM which could > store the on screen buffer, plane should be updated in vblank period > to > prevent tearing effect. In MediaTek plane disable callback function, > if > we wait vblank period to disable plane, the software would be blocked > for a long time. If there are four planes to be disabled, the total > blocking time would be almost 4 vsync time. So this patch is just a > work around to use get_dma_buf() to prevent buffer to be freed before > vblank period and not block the software. For the behavior DRM core > free plane buffer just after plane disable callback function return, > I'm not sure it's DRM core's bug or it's DRM core's correct behavior. > If this is correct behavior, it means that DRM core assume that > hardware should have internal SRAM but MediaTek display hardware > violate this assumption. If so, I think I could just accept this work > around patch. Sorry. I've found that drm_atomic_helper_commit_tail() has wait for vblank after commit. So I think that's MediaTek driver's problem. MediaTek driver should promise that configuration should take effect before send vblank event. Regards, CK > > Regards, > CK > > > > > > Maxime
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c index d40142842f85..49d671100785 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c @@ -4,6 +4,7 @@ */ #include <linux/clk.h> +#include <linux/dma-buf.h> #include <linux/dma-mapping.h> #include <linux/mailbox_controller.h> #include <linux/pm_runtime.h> @@ -283,6 +284,23 @@ struct mtk_ddp_comp *mtk_drm_ddp_comp_for_plane(struct drm_crtc *crtc, return NULL; } +static void mtk_drm_dma_buf_put(struct mtk_drm_crtc *mtk_crtc) +{ + unsigned int i; + + for (i = 0; i < mtk_crtc->layer_nr; i++) { + struct drm_plane *plane = &mtk_crtc->planes[i]; + struct mtk_plane_state *plane_state; + + plane_state = to_mtk_plane_state(plane->state); + + if (plane_state && plane_state->pending.dma_buf) { + dma_buf_put(plane_state->pending.dma_buf); + plane_state->pending.dma_buf = NULL; + } + } +} + #if IS_REACHABLE(CONFIG_MTK_CMDQ) static void ddp_cmdq_cb(struct mbox_client *cl, void *mssg) { @@ -323,6 +341,8 @@ static void ddp_cmdq_cb(struct mbox_client *cl, void *mssg) mtk_crtc->pending_async_planes = false; } + mtk_drm_dma_buf_put(mtk_crtc); + mtk_crtc->cmdq_vblank_cnt = 0; wake_up(&mtk_crtc->cb_blocking_queue); } @@ -624,9 +644,14 @@ static void mtk_crtc_ddp_irq(void *data) else if (mtk_crtc->cmdq_vblank_cnt > 0 && --mtk_crtc->cmdq_vblank_cnt == 0) DRM_ERROR("mtk_crtc %d CMDQ execute command timeout!\n", drm_crtc_index(&mtk_crtc->base)); + + if (!mtk_crtc->cmdq_client.chan) + mtk_drm_dma_buf_put(mtk_crtc); #else if (!priv->data->shadow_register) mtk_crtc_ddp_config(crtc, NULL); + + mtk_drm_dma_buf_put(mtk_crtc); #endif mtk_drm_finish_page_flip(mtk_crtc); } diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c index 6dcb4ba2466c..812f1667e070 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c @@ -993,4 +993,5 @@ module_exit(mtk_drm_exit); MODULE_AUTHOR("YT SHEN <yt.shen@mediatek.com>"); MODULE_DESCRIPTION("Mediatek SoC DRM driver"); +MODULE_IMPORT_NS(DMA_BUF); MODULE_LICENSE("GPL v2"); diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c index 31f9420aff6f..66e6393e45ee 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c @@ -12,6 +12,7 @@ #include <drm/drm_framebuffer.h> #include <drm/drm_gem_atomic_helper.h> #include <linux/align.h> +#include <linux/dma-buf.h> #include "mtk_drm_crtc.h" #include "mtk_drm_ddp_comp.h" @@ -266,6 +267,17 @@ static void mtk_plane_atomic_disable(struct drm_plane *plane, struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state, plane); struct mtk_plane_state *mtk_plane_state = to_mtk_plane_state(new_state); + struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(state, + plane); + + if (old_state && old_state->fb) { + struct drm_gem_object *gem = old_state->fb->obj[0]; + + if (gem && gem->dma_buf) { + get_dma_buf(gem->dma_buf); + mtk_plane_state->pending.dma_buf = gem->dma_buf; + } + } mtk_plane_state->pending.enable = false; wmb(); /* Make sure the above parameter is set before update */ mtk_plane_state->pending.dirty = true; diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.h b/drivers/gpu/drm/mediatek/mtk_drm_plane.h index 99aff7da0831..3aba0b58ef3c 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.h +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.h @@ -33,6 +33,7 @@ struct mtk_plane_pending_state { bool async_dirty; bool async_config; enum drm_color_encoding color_encoding; + struct dma_buf *dma_buf; }; struct mtk_plane_state {
dma buffer release before overlay disable, that will cause m4u translation fault warning. add dma buffer control flow in mediatek driver: get dma buffer when drm plane disable put dma buffer when overlay really disable Fixes: 41016fe1028e ("drm: Rename plane->state variables in atomic update and disable") Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com> --- drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 25 ++++++++++++++++++++++++ drivers/gpu/drm/mediatek/mtk_drm_drv.c | 1 + drivers/gpu/drm/mediatek/mtk_drm_plane.c | 12 ++++++++++++ drivers/gpu/drm/mediatek/mtk_drm_plane.h | 1 + 4 files changed, 39 insertions(+)