<pre>
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,
>

</pre><!--type:text--><!--{--><pre>************* MEDIATEK Confidentiality Notice
 ********************
The information contained in this e-mail message (including any 
attachments) may be confidential, proprietary, privileged, or otherwise
exempt from disclosure under applicable laws. It is intended to be 
conveyed only to the designated recipient(s). Any use, dissemination, 
distribution, printing, retaining or copying of this e-mail (including its 
attachments) by unintended recipient(s) is strictly prohibited and may 
be unlawful. If you are not an intended recipient of this e-mail, or believe
 
that you have received this e-mail in error, please notify the sender 
immediately (by replying to this e-mail), delete any and all copies of 
this e-mail (including any attachments) from your system, and do not
disclose the content of this e-mail to any other person. Thank you!
</pre><!--}-->