* Re: [PATCH RESEND v4 2/2] drm/mediatek: Fix iommu fault during crtc enabling
[not found] ` <20230807015110.30579-3-jason-jh.lin@mediatek.com>
@ 2023-08-07 8:59 ` CK Hu (胡俊光)
2023-08-08 8:54 ` Jason-JH Lin (林睿祥)
2023-08-07 9:45 ` Alexandre Mergnat
2023-08-08 11:55 ` Eugen Hristev
2 siblings, 1 reply; 6+ messages in thread
From: CK Hu (胡俊光) @ 2023-08-07 8:59 UTC (permalink / raw)
To: amergnat, Jason-JH Lin (林睿祥),
chunkuang.hu, angelogioacchino.delregno
Cc: linux-kernel, Singo Chang (張興國),
Johnson Wang (王聖鑫),
Jason-ch Chen (陳建豪),
Shawn Sung (宋孝謙),
linux-mediatek, Nancy Lin (林欣螢),
dri-devel, Project_Global_Chrome_Upstream_Group,
linux-arm-kernel
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@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);
> + /* 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()?
> + 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().
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,
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH RESEND v4 2/2] drm/mediatek: Fix iommu fault during crtc enabling
2023-08-07 8:59 ` [PATCH RESEND v4 2/2] drm/mediatek: Fix iommu fault during crtc enabling CK Hu (胡俊光)
@ 2023-08-08 8:54 ` Jason-JH Lin (林睿祥)
0 siblings, 0 replies; 6+ messages in thread
From: Jason-JH Lin (林睿祥) @ 2023-08-08 8:54 UTC (permalink / raw)
To: CK Hu (胡俊光),
amergnat, chunkuang.hu, angelogioacchino.delregno
Cc: linux-kernel, linux-mediatek,
Singo Chang (張興國),
Johnson Wang (王聖鑫),
Jason-ch Chen (陳建豪),
Shawn Sung (宋孝謙),
Nancy Lin (林欣螢),
dri-devel, Project_Global_Chrome_Upstream_Group,
linux-arm-kernel
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@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);
> > + /* 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,
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RESEND v4 2/2] drm/mediatek: Fix iommu fault during crtc enabling
[not found] ` <20230807015110.30579-3-jason-jh.lin@mediatek.com>
2023-08-07 8:59 ` [PATCH RESEND v4 2/2] drm/mediatek: Fix iommu fault during crtc enabling CK Hu (胡俊光)
@ 2023-08-07 9:45 ` Alexandre Mergnat
2023-08-08 8:38 ` Jason-JH Lin (林睿祥)
2023-08-08 11:55 ` Eugen Hristev
2 siblings, 1 reply; 6+ messages in thread
From: Alexandre Mergnat @ 2023-08-07 9:45 UTC (permalink / raw)
To: Jason-JH.Lin, Chun-Kuang Hu, AngeloGioacchino Del Regno
Cc: Jason-ch Chen, Johnson Wang, Singo Chang, Nancy Lin, Shawn Sung,
dri-devel, linux-mediatek, linux-arm-kernel, linux-kernel,
Project_Global_Chrome_Upstream_Group
Hi Jason,
You forgot to put the Reviewed-by got from the V3 in your commit message.
On 07/08/2023 03: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.
> ---
--
Regards,
Alexandre
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH RESEND v4 2/2] drm/mediatek: Fix iommu fault during crtc enabling
2023-08-07 9:45 ` Alexandre Mergnat
@ 2023-08-08 8:38 ` Jason-JH Lin (林睿祥)
0 siblings, 0 replies; 6+ messages in thread
From: Jason-JH Lin (林睿祥) @ 2023-08-08 8:38 UTC (permalink / raw)
To: amergnat, chunkuang.hu, angelogioacchino.delregno
Cc: linux-mediatek, Singo Chang (張興國),
Johnson Wang (王聖鑫),
Jason-ch Chen (陳建豪),
Shawn Sung (宋孝謙),
linux-kernel, Nancy Lin (林欣螢),
dri-devel, Project_Global_Chrome_Upstream_Group,
linux-arm-kernel
Hi Alexandre,
On Mon, 2023-08-07 at 11:45 +0200, Alexandre Mergnat wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> Hi Jason,
>
> You forgot to put the Reviewed-by got from the V3 in your commit
> message.
>
Since I changed the original method at the new version, I removed all
reviewed-by tags.
In order to avoid the version you have reviewed is too different.
Regards,
Jason-JH.Lin
>
> On 07/08/2023 03: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.
> > ---
>
>
> --
> Regards,
> Alexandre
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RESEND v4 2/2] drm/mediatek: Fix iommu fault during crtc enabling
[not found] ` <20230807015110.30579-3-jason-jh.lin@mediatek.com>
2023-08-07 8:59 ` [PATCH RESEND v4 2/2] drm/mediatek: Fix iommu fault during crtc enabling CK Hu (胡俊光)
2023-08-07 9:45 ` Alexandre Mergnat
@ 2023-08-08 11:55 ` Eugen Hristev
2023-08-08 15:04 ` Jason-JH Lin (林睿祥)
2 siblings, 1 reply; 6+ messages in thread
From: Eugen Hristev @ 2023-08-08 11:55 UTC (permalink / raw)
To: Jason-JH.Lin, Chun-Kuang Hu, AngeloGioacchino Del Regno,
Alexandre Mergnat
Cc: Jason-ch Chen, Johnson Wang, Singo Chang, Nancy Lin, Shawn Sung,
dri-devel, linux-mediatek, linux-arm-kernel, linux-kernel,
Project_Global_Chrome_Upstream_Group
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 ?
> + /* 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 ?
I see mtk_plane_update_new_state assumes new_state being a correct
state/pointer.
Regards,
> + 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;
> + }
> +
> 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,
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH RESEND v4 2/2] drm/mediatek: Fix iommu fault during crtc enabling
2023-08-08 11:55 ` Eugen Hristev
@ 2023-08-08 15:04 ` Jason-JH Lin (林睿祥)
0 siblings, 0 replies; 6+ messages in thread
From: Jason-JH Lin (林睿祥) @ 2023-08-08 15:04 UTC (permalink / raw)
To: amergnat, chunkuang.hu, angelogioacchino.delregno, eugen.hristev
Cc: linux-mediatek, Singo Chang (張興國),
Johnson Wang (王聖鑫),
Jason-ch Chen (陳建豪),
Shawn Sung (宋孝謙),
linux-kernel, Nancy Lin (林欣螢),
dri-devel, Project_Global_Chrome_Upstream_Group,
linux-arm-kernel
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,
>
^ permalink raw reply [flat|nested] 6+ messages in thread