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

[Jason-JH Lin (林睿祥)]

Hi Eugen,

Thanks for the reviews.

On Tue, 2023-08-08 at 14:55 +0300, Eugen Hristev wrote:
> Hi Jason,
> 
> On 8/7/23 04:51, 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);
> 
> any reason why you moved the initialization of plane_state at the 
> declaration phase ?
> 
I just like to assign it at the declaration phase.
But it's not related to this fix patch, so I'll move it back.
Thanks.

> > +		/* 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);
> 
> Can drm_atomic_get_new_plane_state fail ? and new_state becomes null
> ?

drm_atomic_get_new_plane_state() won't fail and new_state won't be
NULL.

But if state->planes[i].ptr is NULL, it'll crash in drm_plane_index()
inside drm_atomic_get_new_plane_state().

> 
> I see mtk_plane_update_new_state assumes new_state being a correct 
> state/pointer.

The usage is the same as mtk_plane_atomic_update().

Regards,
Jason-JH.Lin

> 
> Regards,
>