| Message ID | 20231019055619.19358-16-shawn.sung@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add display driver for MT8188 VDOSYS1 | expand |
Il 19/10/23 07:56, Hsiao Chien Sung ha scritto: > Display modules will be powered on when .atomic_enable(), > there is no need to do it again in mtk_crtc_ddp_hw_init(). > Besides, the DRM devices are created manually when mtk-mmsys > is probed and there is no power domain linked to it. > > Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC MT8173.") > > Signed-off-by: Hsiao Chien Sung <shawn.sung@mediatek.com> > --- > drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 18 +++--------------- > 1 file changed, 3 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c > index bc4cc75cca18..c7edd80be428 100644 > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c > @@ -6,7 +6,6 @@ > #include <linux/clk.h> > #include <linux/dma-mapping.h> > #include <linux/mailbox_controller.h> > -#include <linux/pm_runtime.h> > #include <linux/soc/mediatek/mtk-cmdq.h> > #include <linux/soc/mediatek/mtk-mmsys.h> > #include <linux/soc/mediatek/mtk-mutex.h> > @@ -362,22 +361,16 @@ static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc, struct drm_atomic > drm_connector_list_iter_end(&conn_iter); > } > > - ret = pm_runtime_resume_and_get(crtc->dev->dev); > - if (ret < 0) { > - DRM_ERROR("Failed to enable power domain: %d\n", ret); > - return ret; > - } > - Are you really sure that writes to DISP_REG_OVL_xxx and others in other modules, called by the .layer_config() callback, can be successfully done on an unpowered and/or unclocked module, on all MediaTek SoCs? This looks a bit odd. > ret = mtk_mutex_prepare(mtk_crtc->mutex); > if (ret < 0) { > DRM_ERROR("Failed to enable mutex clock: %d\n", ret); > - goto err_pm_runtime_put; > + goto error; > } > > ret = mtk_crtc_ddp_clk_enable(mtk_crtc); > if (ret < 0) { > DRM_ERROR("Failed to enable component clocks: %d\n", ret); > - goto err_mutex_unprepare; > + goto error; > } > > for (i = 0; i < mtk_crtc->ddp_comp_nr - 1; i++) { > @@ -426,16 +419,13 @@ static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc, struct drm_atomic > ...because you could otherwise just call pm_runtime_put() here, instead of removing the pm_runtime_resume_and_get() call, which is something I would advise to do. Regards, Angelo > return 0; > > -err_mutex_unprepare: > +error: > mtk_mutex_unprepare(mtk_crtc->mutex); > -err_pm_runtime_put: > - pm_runtime_put(crtc->dev->dev); > return ret; > } > > static void mtk_crtc_ddp_hw_fini(struct mtk_drm_crtc *mtk_crtc) > { > - struct drm_device *drm = mtk_crtc->base.dev; > struct drm_crtc *crtc = &mtk_crtc->base; > int i; > > @@ -465,8 +455,6 @@ static void mtk_crtc_ddp_hw_fini(struct mtk_drm_crtc *mtk_crtc) > mtk_crtc_ddp_clk_disable(mtk_crtc); > mtk_mutex_unprepare(mtk_crtc->mutex); > > - pm_runtime_put(drm->dev); > - > if (crtc->state->event && !crtc->state->active) { > spin_lock_irq(&crtc->dev->event_lock); > drm_crtc_send_vblank_event(crtc, crtc->state->event);
Hi Angelo, On Thu, 2023-10-19 at 11:07 +0200, AngeloGioacchino Del Regno wrote: > Il 19/10/23 07:56, Hsiao Chien Sung ha scritto: > > Display modules will be powered on when .atomic_enable(), > > there is no need to do it again in mtk_crtc_ddp_hw_init(). > > Besides, the DRM devices are created manually when mtk-mmsys > > is probed and there is no power domain linked to it. > > > > Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC > > MT8173.") > > > > Signed-off-by: Hsiao Chien Sung <shawn.sung@mediatek.com> > > --- > > drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 18 +++--------------- > > 1 file changed, 3 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c > > b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c > > index bc4cc75cca18..c7edd80be428 100644 > > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c > > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c > > @@ -6,7 +6,6 @@ > > #include <linux/clk.h> > > #include <linux/dma-mapping.h> > > #include <linux/mailbox_controller.h> > > -#include <linux/pm_runtime.h> > > #include <linux/soc/mediatek/mtk-cmdq.h> > > #include <linux/soc/mediatek/mtk-mmsys.h> > > #include <linux/soc/mediatek/mtk-mutex.h> > > @@ -362,22 +361,16 @@ static int mtk_crtc_ddp_hw_init(struct > > mtk_drm_crtc *mtk_crtc, struct drm_atomic > > drm_connector_list_iter_end(&conn_iter); > > } > > > > - ret = pm_runtime_resume_and_get(crtc->dev->dev); > > - if (ret < 0) { > > - DRM_ERROR("Failed to enable power domain: %d\n", ret); > > - return ret; > > - } > > - > > Are you really sure that writes to DISP_REG_OVL_xxx and others in > other modules, > called by the .layer_config() callback, can be successfully done on > an unpowered > and/or unclocked module, on all MediaTek SoCs? > This looks a bit odd. Not sure if I get your point correctly. We removed this PM API because: 1. mtk_crtc_ddp_hw_init() is called by mtk_drm_crtc_atomic_enable(), and the new inline function mtk_ddp_comp_power_on() is called before hw init, we can make sure the power is on before configuring the hardware. 2. The device "crtc->dev->dev" here is assigned by the probe function of mtk-mmsys, which will be look like "mediatek-drm.auto.13", and this device is not linked to any power domain. > > > ret = mtk_mutex_prepare(mtk_crtc->mutex); > > if (ret < 0) { > > DRM_ERROR("Failed to enable mutex clock: %d\n", ret); > > - goto err_pm_runtime_put; > > + goto error; > > } > > > > ret = mtk_crtc_ddp_clk_enable(mtk_crtc); > > if (ret < 0) { > > DRM_ERROR("Failed to enable component clocks: %d\n", > > ret); > > - goto err_mutex_unprepare; > > + goto error; > > } > > > > for (i = 0; i < mtk_crtc->ddp_comp_nr - 1; i++) { > > @@ -426,16 +419,13 @@ static int mtk_crtc_ddp_hw_init(struct > > mtk_drm_crtc *mtk_crtc, struct drm_atomic > > > > ...because you could otherwise just call pm_runtime_put() here, > instead of removing > the pm_runtime_resume_and_get() call, which is something I would > advise to do. > > Regards, > Angelo > Thanks, Shawn
Il 19/10/23 11:52, Shawn Sung (宋孝謙) ha scritto: > Hi Angelo, > > On Thu, 2023-10-19 at 11:07 +0200, AngeloGioacchino Del Regno wrote: >> Il 19/10/23 07:56, Hsiao Chien Sung ha scritto: >>> Display modules will be powered on when .atomic_enable(), >>> there is no need to do it again in mtk_crtc_ddp_hw_init(). >>> Besides, the DRM devices are created manually when mtk-mmsys >>> is probed and there is no power domain linked to it. >>> >>> Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC >>> MT8173.") >>> >>> Signed-off-by: Hsiao Chien Sung <shawn.sung@mediatek.com> >>> --- >>> drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 18 +++--------------- >>> 1 file changed, 3 insertions(+), 15 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c >>> b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c >>> index bc4cc75cca18..c7edd80be428 100644 >>> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c >>> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c >>> @@ -6,7 +6,6 @@ >>> #include <linux/clk.h> >>> #include <linux/dma-mapping.h> >>> #include <linux/mailbox_controller.h> >>> -#include <linux/pm_runtime.h> >>> #include <linux/soc/mediatek/mtk-cmdq.h> >>> #include <linux/soc/mediatek/mtk-mmsys.h> >>> #include <linux/soc/mediatek/mtk-mutex.h> >>> @@ -362,22 +361,16 @@ static int mtk_crtc_ddp_hw_init(struct >>> mtk_drm_crtc *mtk_crtc, struct drm_atomic >>> drm_connector_list_iter_end(&conn_iter); >>> } >>> >>> - ret = pm_runtime_resume_and_get(crtc->dev->dev); >>> - if (ret < 0) { >>> - DRM_ERROR("Failed to enable power domain: %d\n", ret); >>> - return ret; >>> - } >>> - >> >> Are you really sure that writes to DISP_REG_OVL_xxx and others in >> other modules, >> called by the .layer_config() callback, can be successfully done on >> an unpowered >> and/or unclocked module, on all MediaTek SoCs? >> This looks a bit odd. > > Not sure if I get your point correctly. We removed this PM API because: > > 1. mtk_crtc_ddp_hw_init() is called by mtk_drm_crtc_atomic_enable(), > and the new inline function mtk_ddp_comp_power_on() is called before hw > init, we can make sure the power is on before configuring the hardware. > > 2. The device "crtc->dev->dev" here is assigned by the probe function > of mtk-mmsys, which will be look like "mediatek-drm.auto.13", and this > device is not linked to any power domain. > Thanks for the clarification. In this case: Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> >> >>> ret = mtk_mutex_prepare(mtk_crtc->mutex); >>> if (ret < 0) { >>> DRM_ERROR("Failed to enable mutex clock: %d\n", ret); >>> - goto err_pm_runtime_put; >>> + goto error; >>> } >>> >>> ret = mtk_crtc_ddp_clk_enable(mtk_crtc); >>> if (ret < 0) { >>> DRM_ERROR("Failed to enable component clocks: %d\n", >>> ret); >>> - goto err_mutex_unprepare; >>> + goto error; >>> } >>> >>> for (i = 0; i < mtk_crtc->ddp_comp_nr - 1; i++) { >>> @@ -426,16 +419,13 @@ static int mtk_crtc_ddp_hw_init(struct >>> mtk_drm_crtc *mtk_crtc, struct drm_atomic >>> >> >> ...because you could otherwise just call pm_runtime_put() here, >> instead of removing >> the pm_runtime_resume_and_get() call, which is something I would >> advise to do. >> >> Regards, >> Angelo >> > > Thanks, > Shawn
Hi, Hsiao-chien: On Thu, 2023-10-19 at 13:56 +0800, Hsiao Chien Sung wrote: > Display modules will be powered on when .atomic_enable(), > there is no need to do it again in mtk_crtc_ddp_hw_init(). > Besides, the DRM devices are created manually when mtk-mmsys > is probed and there is no power domain linked to it. > > Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC > MT8173.") > > Signed-off-by: Hsiao Chien Sung <shawn.sung@mediatek.com> > --- > drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 18 +++--------------- > 1 file changed, 3 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c > b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c > index bc4cc75cca18..c7edd80be428 100644 > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c > @@ -6,7 +6,6 @@ > #include <linux/clk.h> > #include <linux/dma-mapping.h> > #include <linux/mailbox_controller.h> > -#include <linux/pm_runtime.h> > #include <linux/soc/mediatek/mtk-cmdq.h> > #include <linux/soc/mediatek/mtk-mmsys.h> > #include <linux/soc/mediatek/mtk-mutex.h> > @@ -362,22 +361,16 @@ static int mtk_crtc_ddp_hw_init(struct > mtk_drm_crtc *mtk_crtc, struct drm_atomic > drm_connector_list_iter_end(&conn_iter); > } > > - ret = pm_runtime_resume_and_get(crtc->dev->dev); crtc->dev->dev is mmsys device. In mt8173.dtsi, you could find that mmsys has its own power. So I think we should keep this. Regards, CK > - if (ret < 0) { > - DRM_ERROR("Failed to enable power domain: %d\n", ret); > - return ret; > - } > - > ret = mtk_mutex_prepare(mtk_crtc->mutex); > if (ret < 0) { > DRM_ERROR("Failed to enable mutex clock: %d\n", ret); > - goto err_pm_runtime_put; > + goto error; > } > > ret = mtk_crtc_ddp_clk_enable(mtk_crtc); > if (ret < 0) { > DRM_ERROR("Failed to enable component clocks: %d\n", > ret); > - goto err_mutex_unprepare; > + goto error; > } > > for (i = 0; i < mtk_crtc->ddp_comp_nr - 1; i++) { > @@ -426,16 +419,13 @@ static int mtk_crtc_ddp_hw_init(struct > mtk_drm_crtc *mtk_crtc, struct drm_atomic > > return 0; > > -err_mutex_unprepare: > +error: > mtk_mutex_unprepare(mtk_crtc->mutex); > -err_pm_runtime_put: > - pm_runtime_put(crtc->dev->dev); > return ret; > } > > static void mtk_crtc_ddp_hw_fini(struct mtk_drm_crtc *mtk_crtc) > { > - struct drm_device *drm = mtk_crtc->base.dev; > struct drm_crtc *crtc = &mtk_crtc->base; > int i; > > @@ -465,8 +455,6 @@ static void mtk_crtc_ddp_hw_fini(struct > mtk_drm_crtc *mtk_crtc) > mtk_crtc_ddp_clk_disable(mtk_crtc); > mtk_mutex_unprepare(mtk_crtc->mutex); > > - pm_runtime_put(drm->dev); > - > if (crtc->state->event && !crtc->state->active) { > spin_lock_irq(&crtc->dev->event_lock); > drm_crtc_send_vblank_event(crtc, crtc->state->event);
Hi CK, On Tue, 2023-10-24 at 09:25 +0000, CK Hu (胡俊光) wrote: > Hi, Hsiao-chien: > > On Thu, 2023-10-19 at 13:56 +0800, Hsiao Chien Sung wrote: > > Display modules will be powered on when .atomic_enable(), > > there is no need to do it again in mtk_crtc_ddp_hw_init(). > > Besides, the DRM devices are created manually when mtk-mmsys > > is probed and there is no power domain linked to it. > > > > Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC > > MT8173.") > > > > Signed-off-by: Hsiao Chien Sung <shawn.sung@mediatek.com> > > --- > > drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 18 +++--------------- > > 1 file changed, 3 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c > > b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c > > index bc4cc75cca18..c7edd80be428 100644 > > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c > > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c > > @@ -6,7 +6,6 @@ > > #include <linux/clk.h> > > #include <linux/dma-mapping.h> > > #include <linux/mailbox_controller.h> > > -#include <linux/pm_runtime.h> > > #include <linux/soc/mediatek/mtk-cmdq.h> > > #include <linux/soc/mediatek/mtk-mmsys.h> > > #include <linux/soc/mediatek/mtk-mutex.h> > > @@ -362,22 +361,16 @@ static int mtk_crtc_ddp_hw_init(struct > > mtk_drm_crtc *mtk_crtc, struct drm_atomic > > drm_connector_list_iter_end(&conn_iter); > > } > > > > - ret = pm_runtime_resume_and_get(crtc->dev->dev); > > crtc->dev->dev is mmsys device. In mt8173.dtsi, you could find that > mmsys has its own power. So I think we should keep this. > > Regards, > CK Didn't notice this difference in the dts, thank you for checking. Will remove this patch in the next version. Thanks, Shawn
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c index bc4cc75cca18..c7edd80be428 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c @@ -6,7 +6,6 @@ #include <linux/clk.h> #include <linux/dma-mapping.h> #include <linux/mailbox_controller.h> -#include <linux/pm_runtime.h> #include <linux/soc/mediatek/mtk-cmdq.h> #include <linux/soc/mediatek/mtk-mmsys.h> #include <linux/soc/mediatek/mtk-mutex.h> @@ -362,22 +361,16 @@ static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc, struct drm_atomic drm_connector_list_iter_end(&conn_iter); } - ret = pm_runtime_resume_and_get(crtc->dev->dev); - if (ret < 0) { - DRM_ERROR("Failed to enable power domain: %d\n", ret); - return ret; - } - ret = mtk_mutex_prepare(mtk_crtc->mutex); if (ret < 0) { DRM_ERROR("Failed to enable mutex clock: %d\n", ret); - goto err_pm_runtime_put; + goto error; } ret = mtk_crtc_ddp_clk_enable(mtk_crtc); if (ret < 0) { DRM_ERROR("Failed to enable component clocks: %d\n", ret); - goto err_mutex_unprepare; + goto error; } for (i = 0; i < mtk_crtc->ddp_comp_nr - 1; i++) { @@ -426,16 +419,13 @@ static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc, struct drm_atomic return 0; -err_mutex_unprepare: +error: mtk_mutex_unprepare(mtk_crtc->mutex); -err_pm_runtime_put: - pm_runtime_put(crtc->dev->dev); return ret; } static void mtk_crtc_ddp_hw_fini(struct mtk_drm_crtc *mtk_crtc) { - struct drm_device *drm = mtk_crtc->base.dev; struct drm_crtc *crtc = &mtk_crtc->base; int i; @@ -465,8 +455,6 @@ static void mtk_crtc_ddp_hw_fini(struct mtk_drm_crtc *mtk_crtc) mtk_crtc_ddp_clk_disable(mtk_crtc); mtk_mutex_unprepare(mtk_crtc->mutex); - pm_runtime_put(drm->dev); - if (crtc->state->event && !crtc->state->active) { spin_lock_irq(&crtc->dev->event_lock); drm_crtc_send_vblank_event(crtc, crtc->state->event);
Display modules will be powered on when .atomic_enable(), there is no need to do it again in mtk_crtc_ddp_hw_init(). Besides, the DRM devices are created manually when mtk-mmsys is probed and there is no power domain linked to it. Fixes: 119f5173628a ("drm/mediatek: Add DRM Driver for Mediatek SoC MT8173.") Signed-off-by: Hsiao Chien Sung <shawn.sung@mediatek.com> --- drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-)