Re: [PATCH RESEND v4 2/2] drm/mediatek: Fix iommu fault during crtc enabling

[Jason-JH Lin (林睿祥)]

Hi CK,

On Mon, 2023-08-07 at 08:59 +0000, CK Hu (胡俊光) wrote:
> Hi, Jason:
> 
> On Mon, 2023-08-07 at 09:51 +0800, Jason-JH.Lin wrote:
> > The plane_state of drm_atomic_state is not sync to the
> > mtk_plane_state
> > stored in mtk_crtc during crtc enabling.
> > 
> > So we need to update the mtk_plane_state stored in mtk_crtc by the
> > drm_atomic_state carried from mtk_drm_crtc_atomic_enable().
> > 
> > While updating mtk_plane_state, OVL layer should be disabled when
> > the
> > fb
> > in plane_state of drm_atomic_state is NULL.
> > 
> > Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC
> > MT8173.")
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@xxxxxxxxxxxx>
> > ---
> > Change in RESEND v4:
> > Remove redundant plane_state assigning.
> > ---
> >  drivers/gpu/drm/mediatek/mtk_drm_crtc.c  | 14 ++++++++++----
> >  drivers/gpu/drm/mediatek/mtk_drm_plane.c | 11 ++++++++---
> >  drivers/gpu/drm/mediatek/mtk_drm_plane.h |  2 ++
> >  3 files changed, 20 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > index d40142842f85..7db4d6551da7 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > @@ -328,7 +328,7 @@ static void ddp_cmdq_cb(struct mbox_client *cl,
> > void *mssg)
> >  }
> >  #endif
> >  
> > -static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc)
> > +static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc,
> > struct drm_atomic_state *state)
> >  {
> >  	struct drm_crtc *crtc = &mtk_crtc->base;
> >  	struct drm_connector *connector;
> > @@ -405,11 +405,17 @@ static int mtk_crtc_ddp_hw_init(struct
> > mtk_drm_crtc *mtk_crtc)
> >  	/* Initially configure all planes */
> >  	for (i = 0; i < mtk_crtc->layer_nr; i++) {
> >  		struct drm_plane *plane = &mtk_crtc->planes[i];
> > -		struct mtk_plane_state *plane_state;
> > +		struct drm_plane_state *new_state;
> > +		struct mtk_plane_state *plane_state =
> > to_mtk_plane_state(plane->state);
> >  		struct mtk_ddp_comp *comp;
> >  		unsigned int local_layer;
> >  
> > -		plane_state = to_mtk_plane_state(plane->state);
> > +		/* sync the new plane state from drm_atomic_state */
> > +		if (state->planes[i].ptr) {
> > +			new_state =
> > drm_atomic_get_new_plane_state(state, state->planes[i].ptr);
> 
> for_each_new_plane_in_state()?

I'vd tried for_each_new_plane_in_state(), but it crashed in the fifth
round.
Because mode_config.num_total_plane in this macro is 8, but mtk_crtc-
>layer_nr is 4.

So I think we can't use this macro here.

> 
> > +			mtk_plane_update_new_state(new_state,
> > plane_state);
> > +		}
> > +
> >  		comp = mtk_drm_ddp_comp_for_plane(crtc, plane,
> > &local_layer);
> >  		if (comp)
> >  			mtk_ddp_comp_layer_config(comp, local_layer,
> > @@ -687,7 +693,7 @@ static void mtk_drm_crtc_atomic_enable(struct
> > drm_crtc *crtc,
> >  		return;
> >  	}
> >  
> > -	ret = mtk_crtc_ddp_hw_init(mtk_crtc);
> > +	ret = mtk_crtc_ddp_hw_init(mtk_crtc, state);
> >  	if (ret) {
> >  		pm_runtime_put(comp->dev);
> >  		return;
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> > b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> > index b1a918ffe457..ef4460f98c07 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> > @@ -134,8 +134,8 @@ static int mtk_plane_atomic_async_check(struct
> > drm_plane *plane,
> >  						   true, true);
> >  }
> >  
> > -static void mtk_plane_update_new_state(struct drm_plane_state
> > *new_state,
> > -				       struct mtk_plane_state
> > *mtk_plane_state)
> > +void mtk_plane_update_new_state(struct drm_plane_state *new_state,
> > +				struct mtk_plane_state
> > *mtk_plane_state)
> >  {
> >  	struct drm_framebuffer *fb = new_state->fb;
> >  	struct drm_gem_object *gem;
> > @@ -146,6 +146,11 @@ static void mtk_plane_update_new_state(struct
> > drm_plane_state *new_state,
> >  	dma_addr_t hdr_addr = 0;
> >  	unsigned int hdr_pitch = 0;
> >  
> > +	if (!fb) {
> > +		mtk_plane_state->pending.enable = false;
> > +		return;
> > +	}
> 
> This seems done in mtk_plane_atomic_update(), so you may call
> mtk_plane_atomic_update() instead of mtk_plane_update_new_state().

mtk_plane_atomic_update() won't update the mtk_plane_state in mtk_crtc,
so I use mtk_plane_update_new_state() here.

But I found that we can fix up iommu fault issue by handling the NULL
fb case.
So I'll change the previous method to disable the plane if its fb in
new state is NULL.

Regards,
Jason-JH.Lin

> 
> Regards,
> CK
> 
> > +
> >  	gem = fb->obj[0];
> >  	mtk_gem = to_mtk_gem_obj(gem);
> >  	addr = mtk_gem->dma_addr;
> > @@ -180,7 +185,7 @@ static void mtk_plane_update_new_state(struct
> > drm_plane_state *new_state,
> >  		       fb->format->cpp[0] * (x_offset_in_blocks + 1);
> >  	}
> >  
> > -	mtk_plane_state->pending.enable = true;
> > +	mtk_plane_state->pending.enable = new_state->visible;
> >  	mtk_plane_state->pending.pitch = pitch;
> >  	mtk_plane_state->pending.hdr_pitch = hdr_pitch;
> >  	mtk_plane_state->pending.format = format;
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.h
> > b/drivers/gpu/drm/mediatek/mtk_drm_plane.h
> > index 99aff7da0831..0a7d70d13e43 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.h
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.h
> > @@ -46,6 +46,8 @@ to_mtk_plane_state(struct drm_plane_state *state)
> >  	return container_of(state, struct mtk_plane_state, base);
> >  }
> >  
> > +void mtk_plane_update_new_state(struct drm_plane_state *new_state,
> > +				struct mtk_plane_state
> > *mtk_plane_state);
> >  int mtk_plane_init(struct drm_device *dev, struct drm_plane
> > *plane,
> >  		   unsigned long possible_crtcs, enum drm_plane_type
> > type,
> >  		   unsigned int supported_rotations, const u32
> > *formats,