Messages in this thread | ![/](/images/icornerl.gif) | | From | Jason-JH Lin (林睿祥) <> | Subject | Re: [PATCH RESEND v4 2/2] drm/mediatek: Fix iommu fault during crtc enabling | Date | Tue, 8 Aug 2023 15:04:11 +0000 |
| |
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@mediatek.com> > > --- > > 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, >
| ![\](/images/icornerr.gif) |