Re: [v3, PATCH] drm/mediatek: add dma buffer control for drm plane disable

[Chun-Kuang Hu]

Hi, Yongqiang:

Yongqiang Niu <yongqiang.niu@xxxxxxxxxxxx> 於 2023年3月20日 週一 上午11:05寫道：
>
> Fixes: 41016fe1028e4 (drm: Rename plane->state variables in atomic update and disable)

[1] has introduction how to add Fixes tag, one information is:

please use the ‘Fixes:’ tag with the first 12 characters of the SHA-1
ID, and the one line summary. Do not split the tag across multiple
lines, tags are exempt from the “wrap at 75 columns” rule in order to
simplify parsing scripts.

And move this tag to the line before your sign-off tag.

[1] https://www.kernel.org/doc/html/v6.2/process/submitting-patches.html


> 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
>
> Signed-off-by: Yongqiang Niu <yongqiang.niu@xxxxxxxxxxxx>
> ---
>  drivers/gpu/drm/mediatek/mtk_drm_crtc.c  | 25 ++++++++++++++++++++++++
>  drivers/gpu/drm/mediatek/mtk_drm_plane.c | 17 ++++++++++++++++
>  drivers/gpu/drm/mediatek/mtk_drm_plane.h |  1 +
>  3 files changed, 43 insertions(+)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> index 5071f1263216..9cf1c1778868 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>
> @@ -282,6 +283,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)
>  {
> @@ -322,6 +340,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);
>  }
> @@ -614,9 +634,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_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> index d54fbf34b000..c169ca49129c 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"
> @@ -280,6 +281,22 @@ 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 (mtk_plane_state->pending.dma_buf) {

When this happen, the pending.dma_buf is still accessed by OVL, right?
So you free this buffer and m4u translation fault occur.

Regards,
Chun-Kuang.

> +                       dma_buf_put(mtk_plane_state->pending.dma_buf);
> +                       mtk_plane_state->pending.dma_buf = NULL;
> +               }
> +
> +               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 8f39011cdbfc..b724e56b7283 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 {
> --
> 2.25.1
>