| Message ID | 20231019055619.19358-1-shawn.sung@mediatek.com |
|---|---|
| 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);
Il 19/10/23 07:56, Hsiao Chien Sung ha scritto: > Different from OVL, OVL adaptor is a pseudo device so we didn't > define it in the device tree, consequently, pm_runtime_resume_and_get() > called by .atomic_enable() powers on no device. For this reason, we > implement a function to power on the RDMAs in OVL adaptor, and the > system will make sure the IOMMUs are powered on as well because of the > device link (iommus) in the RDMA nodes in DTS. > > This patch separates power and clock management process, it would be > easier to maintain and extensions. > > Signed-off-by: Hsiao Chien Sung <shawn.sung@mediatek.com> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Il 19/10/23 07:56, Hsiao Chien Sung ha scritto: > Add MT8188 Padding to OVL adaptor to probe the driver. > > Reviewed-by: CK Hu <ck.hu@mediatek.com> > Signed-off-by: Hsiao Chien Sung <shawn.sung@mediatek.com> > --- > .../gpu/drm/mediatek/mtk_disp_ovl_adaptor.c | 26 +++++++++++++++++++ > 1 file changed, 26 insertions(+) > > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c b/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c > index f46ca1c68610..f693b47745ba 100644 > --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c > +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c > @@ -29,6 +29,7 @@ enum mtk_ovl_adaptor_comp_type { > OVL_ADAPTOR_TYPE_ETHDR, > OVL_ADAPTOR_TYPE_MDP_RDMA, > OVL_ADAPTOR_TYPE_MERGE, > + OVL_ADAPTOR_TYPE_PADDING, > OVL_ADAPTOR_TYPE_NUM, > }; > > @@ -46,6 +47,14 @@ enum mtk_ovl_adaptor_comp_id { > OVL_ADAPTOR_MERGE1, > OVL_ADAPTOR_MERGE2, > OVL_ADAPTOR_MERGE3, > + OVL_ADAPTOR_PADDING0, > + OVL_ADAPTOR_PADDING1, > + OVL_ADAPTOR_PADDING2, > + OVL_ADAPTOR_PADDING3, > + OVL_ADAPTOR_PADDING4, > + OVL_ADAPTOR_PADDING5, > + OVL_ADAPTOR_PADDING6, > + OVL_ADAPTOR_PADDING7, > OVL_ADAPTOR_ID_MAX > }; > > @@ -66,6 +75,7 @@ static const char * const private_comp_stem[OVL_ADAPTOR_TYPE_NUM] = { > [OVL_ADAPTOR_TYPE_ETHDR] = "ethdr", > [OVL_ADAPTOR_TYPE_MDP_RDMA] = "vdo1-rdma", > [OVL_ADAPTOR_TYPE_MERGE] = "merge", > + [OVL_ADAPTOR_TYPE_PADDING] = "padding", > }; > > static const struct mtk_ddp_comp_funcs ethdr = { > @@ -80,6 +90,13 @@ static const struct mtk_ddp_comp_funcs merge = { > .clk_disable = mtk_merge_clk_disable, > }; > > +static const struct mtk_ddp_comp_funcs padding = { > + .clk_enable = mtk_padding_clk_enable, > + .clk_disable = mtk_padding_clk_disable, > + .start = mtk_padding_start, > + .stop = mtk_padding_stop, > +}; > + > static const struct mtk_ddp_comp_funcs rdma = { > .power_on = mtk_mdp_rdma_power_on, > .power_off = mtk_mdp_rdma_power_off, > @@ -101,6 +118,14 @@ static const struct ovl_adaptor_comp_match comp_matches[OVL_ADAPTOR_ID_MAX] = { > [OVL_ADAPTOR_MERGE1] = { OVL_ADAPTOR_TYPE_MERGE, DDP_COMPONENT_MERGE2, 2, &merge }, > [OVL_ADAPTOR_MERGE2] = { OVL_ADAPTOR_TYPE_MERGE, DDP_COMPONENT_MERGE3, 3, &merge }, > [OVL_ADAPTOR_MERGE3] = { OVL_ADAPTOR_TYPE_MERGE, DDP_COMPONENT_MERGE4, 4, &merge }, > + [OVL_ADAPTOR_PADDING0] = { OVL_ADAPTOR_TYPE_PADDING, DDP_COMPONENT_PADDING0, 0, &padding }, > + [OVL_ADAPTOR_PADDING1] = { OVL_ADAPTOR_TYPE_PADDING, DDP_COMPONENT_PADDING1, 1, &padding }, > + [OVL_ADAPTOR_PADDING2] = { OVL_ADAPTOR_TYPE_PADDING, DDP_COMPONENT_PADDING2, 2, &padding }, > + [OVL_ADAPTOR_PADDING3] = { OVL_ADAPTOR_TYPE_PADDING, DDP_COMPONENT_PADDING3, 3, &padding }, > + [OVL_ADAPTOR_PADDING4] = { OVL_ADAPTOR_TYPE_PADDING, DDP_COMPONENT_PADDING4, 4, &padding }, > + [OVL_ADAPTOR_PADDING5] = { OVL_ADAPTOR_TYPE_PADDING, DDP_COMPONENT_PADDING5, 5, &padding }, > + [OVL_ADAPTOR_PADDING6] = { OVL_ADAPTOR_TYPE_PADDING, DDP_COMPONENT_PADDING6, 6, &padding }, > + [OVL_ADAPTOR_PADDING7] = { OVL_ADAPTOR_TYPE_PADDING, DDP_COMPONENT_PADDING7, 7, &padding }, > }; > > void mtk_ovl_adaptor_layer_config(struct device *dev, unsigned int idx, > @@ -436,6 +461,7 @@ static int ovl_adaptor_comp_get_id(struct device *dev, struct device_node *node, > } > > static const struct of_device_id mtk_ovl_adaptor_comp_dt_ids[] = { > + { .compatible = "mediatek,mt8188-padding", .data = (void *)OVL_ADAPTOR_TYPE_PADDING }, Uhm, for consistency I'd call this "mediatek,mt8188-disp-padding" (you don't have to drop Reviewed-by tags for such a change, not here and not in the yaml commit), but it's fine if you have reasons against that. So, regardless of this being changed or not Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > { .compatible = "mediatek,mt8195-disp-ethdr", .data = (void *)OVL_ADAPTOR_TYPE_ETHDR }, > { .compatible = "mediatek,mt8195-disp-merge", .data = (void *)OVL_ADAPTOR_TYPE_MERGE }, > { .compatible = "mediatek,mt8195-vdo1-rdma", .data = (void *)OVL_ADAPTOR_TYPE_MDP_RDMA },
Il 19/10/23 07:56, Hsiao Chien Sung ha scritto: > By registering component related functions to the pointers, > we can easily manage them within a for-loop and simplify the > logic of clock control significantly. > > Signed-off-by: Hsiao Chien Sung <shawn.sung@mediatek.com> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Hi Angelo, On Thu, 2023-10-19 at 11:10 +0200, AngeloGioacchino Del Regno wrote: > > static const struct of_device_id mtk_ovl_adaptor_comp_dt_ids[] = > > { > > + { .compatible = "mediatek,mt8188-padding", .data = (void > > *)OVL_ADAPTOR_TYPE_PADDING }, > > Uhm, for consistency I'd call this "mediatek,mt8188-disp-padding" > (you don't have > to drop Reviewed-by tags for such a change, not here and not in the > yaml commit), > but it's fine if you have reasons against that. > > So, regardless of this being changed or not > > Reviewed-by: AngeloGioacchino Del Regno < > angelogioacchino.delregno@collabora.com> > > > { .compatible = "mediatek,mt8195-disp-ethdr", .data = (void > > *)OVL_ADAPTOR_TYPE_ETHDR }, > > { .compatible = "mediatek,mt8195-disp-merge", .data = (void > > *)OVL_ADAPTOR_TYPE_MERGE }, > > { .compatible = "mediatek,mt8195-vdo1-rdma", .data = (void > > *)OVL_ADAPTOR_TYPE_MDP_RDMA }, > Thanks for pointing this out. Had changed Padding driver's name to "mtk-disp-padding", but I just notice that Padding will also be used by MDP and they will share the same driver with display. Should we change the name again or is it just fine to use "mtk-disp-padding"? Thanks, Shawn
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:20, Shawn Sung (宋孝謙) ha scritto: > Hi Angelo, > > On Thu, 2023-10-19 at 11:10 +0200, AngeloGioacchino Del Regno wrote: >>> static const struct of_device_id mtk_ovl_adaptor_comp_dt_ids[] = >>> { >>> + { .compatible = "mediatek,mt8188-padding", .data = (void >>> *)OVL_ADAPTOR_TYPE_PADDING }, >> >> Uhm, for consistency I'd call this "mediatek,mt8188-disp-padding" >> (you don't have >> to drop Reviewed-by tags for such a change, not here and not in the >> yaml commit), >> but it's fine if you have reasons against that. >> >> So, regardless of this being changed or not >> >> Reviewed-by: AngeloGioacchino Del Regno < >> angelogioacchino.delregno@collabora.com> >> >>> { .compatible = "mediatek,mt8195-disp-ethdr", .data = (void >>> *)OVL_ADAPTOR_TYPE_ETHDR }, >>> { .compatible = "mediatek,mt8195-disp-merge", .data = (void >>> *)OVL_ADAPTOR_TYPE_MERGE }, >>> { .compatible = "mediatek,mt8195-vdo1-rdma", .data = (void >>> *)OVL_ADAPTOR_TYPE_MDP_RDMA }, >> > > Thanks for pointing this out. Had changed Padding driver's name to > "mtk-disp-padding", but I just notice that Padding will also be used by > MDP and they will share the same driver with display. Should we change > the name again or is it just fine to use "mtk-disp-padding"? > That's like many other components in MediaTek, so we can keep the mtk-disp-padding name.... in devicetree, we will anyway use "mediatek,mt8195-mdp3-padding" as one of the compatible string(s). This is the only way that we have to actually distinguish between components used for MDP3 and components used for the display subsystem, if we keep them "generic" we won't understand what's going on in case of issues. The driver name should contain "disp" for consistency with all of the component drivers in mediatek-drm; if this wasn't in this folder, we could've dropped the "disp" in the name, but that's not the case. Consistency is #1. Cheers, 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 Angelo, On Thu, 2023-10-19 at 11:55 +0200, AngeloGioacchino Del Regno wrote: > > That's like many other components in MediaTek, so we can keep the > mtk-disp-padding > name.... in devicetree, we will anyway use "mediatek,mt8195-mdp3- > padding" as one of > the compatible string(s). > > This is the only way that we have to actually distinguish between > components used > for MDP3 and components used for the display subsystem, if we keep > them "generic" > we won't understand what's going on in case of issues. > > The driver name should contain "disp" for consistency with all of the > component > drivers in mediatek-drm; if this wasn't in this folder, we could've > dropped the > "disp" in the name, but that's not the case. > > Consistency is #1. > > Cheers, > Angelo > Got it. Thank you for making that clear. Will change it in the next version. Cheers, Shawn
Hi, Hsiao-chien: On Thu, 2023-10-19 at 13:56 +0800, Hsiao Chien Sung wrote: > By registering component related functions to the pointers, > we can easily manage them within a for-loop and simplify the > logic of clock control significantly. Reviewed-by: CK Hu <ck.hu@mediatek.com> > > Signed-off-by: Hsiao Chien Sung <shawn.sung@mediatek.com> > --- > .../gpu/drm/mediatek/mtk_disp_ovl_adaptor.c | 89 +++++++++------ > ---- > 1 file changed, 43 insertions(+), 46 deletions(-) > > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c > b/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c > index d55d8931a36f..81067f49ea69 100644 > --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c > +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c > @@ -53,6 +53,7 @@ struct ovl_adaptor_comp_match { > enum mtk_ovl_adaptor_comp_type type; > enum mtk_ddp_comp_id comp_id; > int alias_id; > + const struct mtk_ddp_comp_funcs *funcs; > }; > > struct mtk_disp_ovl_adaptor { > @@ -67,20 +68,35 @@ static const char * const > private_comp_stem[OVL_ADAPTOR_TYPE_NUM] = { > [OVL_ADAPTOR_TYPE_ETHDR] = "ethdr", > }; > > +static const struct mtk_ddp_comp_funcs ethdr = { > + .clk_enable = mtk_ethdr_clk_enable, > + .clk_disable = mtk_ethdr_clk_disable, > +}; > + > +static const struct mtk_ddp_comp_funcs merge = { > + .clk_enable = mtk_merge_clk_enable, > + .clk_disable = mtk_merge_clk_disable, > +}; > + > +static const struct mtk_ddp_comp_funcs rdma = { > + .clk_enable = mtk_mdp_rdma_clk_enable, > + .clk_disable = mtk_mdp_rdma_clk_disable, > +}; > + > static const struct ovl_adaptor_comp_match > comp_matches[OVL_ADAPTOR_ID_MAX] = { > - [OVL_ADAPTOR_MDP_RDMA0] = { OVL_ADAPTOR_TYPE_MDP_RDMA, > DDP_COMPONENT_MDP_RDMA0, 0 }, > - [OVL_ADAPTOR_MDP_RDMA1] = { OVL_ADAPTOR_TYPE_MDP_RDMA, > DDP_COMPONENT_MDP_RDMA1, 1 }, > - [OVL_ADAPTOR_MDP_RDMA2] = { OVL_ADAPTOR_TYPE_MDP_RDMA, > DDP_COMPONENT_MDP_RDMA2, 2 }, > - [OVL_ADAPTOR_MDP_RDMA3] = { OVL_ADAPTOR_TYPE_MDP_RDMA, > DDP_COMPONENT_MDP_RDMA3, 3 }, > - [OVL_ADAPTOR_MDP_RDMA4] = { OVL_ADAPTOR_TYPE_MDP_RDMA, > DDP_COMPONENT_MDP_RDMA4, 4 }, > - [OVL_ADAPTOR_MDP_RDMA5] = { OVL_ADAPTOR_TYPE_MDP_RDMA, > DDP_COMPONENT_MDP_RDMA5, 5 }, > - [OVL_ADAPTOR_MDP_RDMA6] = { OVL_ADAPTOR_TYPE_MDP_RDMA, > DDP_COMPONENT_MDP_RDMA6, 6 }, > - [OVL_ADAPTOR_MDP_RDMA7] = { OVL_ADAPTOR_TYPE_MDP_RDMA, > DDP_COMPONENT_MDP_RDMA7, 7 }, > - [OVL_ADAPTOR_MERGE0] = { OVL_ADAPTOR_TYPE_MERGE, > DDP_COMPONENT_MERGE1, 1 }, > - [OVL_ADAPTOR_MERGE1] = { OVL_ADAPTOR_TYPE_MERGE, > DDP_COMPONENT_MERGE2, 2 }, > - [OVL_ADAPTOR_MERGE2] = { OVL_ADAPTOR_TYPE_MERGE, > DDP_COMPONENT_MERGE3, 3 }, > - [OVL_ADAPTOR_MERGE3] = { OVL_ADAPTOR_TYPE_MERGE, > DDP_COMPONENT_MERGE4, 4 }, > - [OVL_ADAPTOR_ETHDR0] = { OVL_ADAPTOR_TYPE_ETHDR, > DDP_COMPONENT_ETHDR_MIXER, 0 }, > + [OVL_ADAPTOR_MDP_RDMA0] = { OVL_ADAPTOR_TYPE_MDP_RDMA, > DDP_COMPONENT_MDP_RDMA0, 0, &rdma }, > + [OVL_ADAPTOR_MDP_RDMA1] = { OVL_ADAPTOR_TYPE_MDP_RDMA, > DDP_COMPONENT_MDP_RDMA1, 1, &rdma }, > + [OVL_ADAPTOR_MDP_RDMA2] = { OVL_ADAPTOR_TYPE_MDP_RDMA, > DDP_COMPONENT_MDP_RDMA2, 2, &rdma }, > + [OVL_ADAPTOR_MDP_RDMA3] = { OVL_ADAPTOR_TYPE_MDP_RDMA, > DDP_COMPONENT_MDP_RDMA3, 3, &rdma }, > + [OVL_ADAPTOR_MDP_RDMA4] = { OVL_ADAPTOR_TYPE_MDP_RDMA, > DDP_COMPONENT_MDP_RDMA4, 4, &rdma }, > + [OVL_ADAPTOR_MDP_RDMA5] = { OVL_ADAPTOR_TYPE_MDP_RDMA, > DDP_COMPONENT_MDP_RDMA5, 5, &rdma }, > + [OVL_ADAPTOR_MDP_RDMA6] = { OVL_ADAPTOR_TYPE_MDP_RDMA, > DDP_COMPONENT_MDP_RDMA6, 6, &rdma }, > + [OVL_ADAPTOR_MDP_RDMA7] = { OVL_ADAPTOR_TYPE_MDP_RDMA, > DDP_COMPONENT_MDP_RDMA7, 7, &rdma }, > + [OVL_ADAPTOR_MERGE0] = { OVL_ADAPTOR_TYPE_MERGE, > DDP_COMPONENT_MERGE1, 1, &merge }, > + [OVL_ADAPTOR_MERGE1] = { OVL_ADAPTOR_TYPE_MERGE, > DDP_COMPONENT_MERGE2, 2, &merge }, > + [OVL_ADAPTOR_MERGE2] = { OVL_ADAPTOR_TYPE_MERGE, > DDP_COMPONENT_MERGE3, 3, &merge }, > + [OVL_ADAPTOR_MERGE3] = { OVL_ADAPTOR_TYPE_MERGE, > DDP_COMPONENT_MERGE4, 4, &merge }, > + [OVL_ADAPTOR_ETHDR0] = { OVL_ADAPTOR_TYPE_ETHDR, > DDP_COMPONENT_ETHDR_MIXER, 0, ðdr }, > }; > > void mtk_ovl_adaptor_layer_config(struct device *dev, unsigned int > idx, > @@ -196,40 +212,25 @@ int mtk_ovl_adaptor_clk_enable(struct device > *dev) > ret = pm_runtime_get_sync(comp); > if (ret < 0) { > dev_err(dev, "Failed to enable power domain %d, > err %d\n", i, ret); > - goto pwr_err; > + goto error; > } > } > > for (i = 0; i < OVL_ADAPTOR_ID_MAX; i++) { > comp = ovl_adaptor->ovl_adaptor_comp[i]; > - > - if (i < OVL_ADAPTOR_MERGE0) > - ret = mtk_mdp_rdma_clk_enable(comp); > - else if (i < OVL_ADAPTOR_ETHDR0) > - ret = mtk_merge_clk_enable(comp); > - else > - ret = mtk_ethdr_clk_enable(comp); > + if (!comp || !comp_matches[i].funcs->clk_enable) > + continue; > + ret = comp_matches[i].funcs->clk_enable(comp); > if (ret) { > dev_err(dev, "Failed to enable clock %d, err > %d\n", i, ret); > - goto clk_err; > + while (--i >= 0) > + comp_matches[i].funcs- > >clk_disable(comp); > + i = OVL_ADAPTOR_MERGE0; > + goto error; > } > } > - > - return ret; > - > -clk_err: > - while (--i >= 0) { > - comp = ovl_adaptor->ovl_adaptor_comp[i]; > - if (i < OVL_ADAPTOR_MERGE0) > - mtk_mdp_rdma_clk_disable(comp); > - else if (i < OVL_ADAPTOR_ETHDR0) > - mtk_merge_clk_disable(comp); > - else > - mtk_ethdr_clk_disable(comp); > - } > - i = OVL_ADAPTOR_MERGE0; > - > -pwr_err: > + return 0; > +error: > while (--i >= 0) > pm_runtime_put(ovl_adaptor->ovl_adaptor_comp[i]); > > @@ -244,15 +245,11 @@ void mtk_ovl_adaptor_clk_disable(struct device > *dev) > > for (i = 0; i < OVL_ADAPTOR_ID_MAX; i++) { > comp = ovl_adaptor->ovl_adaptor_comp[i]; > - > - if (i < OVL_ADAPTOR_MERGE0) { > - mtk_mdp_rdma_clk_disable(comp); > + if (!comp || !comp_matches[i].funcs->clk_disable) > + continue; > + comp_matches[i].funcs->clk_disable(comp); > + if (i < OVL_ADAPTOR_MERGE0) > pm_runtime_put(comp); > - } else if (i < OVL_ADAPTOR_ETHDR0) { > - mtk_merge_clk_disable(comp); > - } else { > - mtk_ethdr_clk_disable(comp); > - } > } > } >
Hi, Hsiao-chien: On Thu, 2023-10-19 at 13:56 +0800, Hsiao Chien Sung wrote: > Different from OVL, OVL adaptor is a pseudo device so we didn't > define it in the device tree, consequently, > pm_runtime_resume_and_get() > called by .atomic_enable() powers on no device. For this reason, we > implement a function to power on the RDMAs in OVL adaptor, and the > system will make sure the IOMMUs are powered on as well because of > the > device link (iommus) in the RDMA nodes in DTS. > > This patch separates power and clock management process, it would be > easier to maintain and extensions. Reviewed-by: CK Hu <ck.hu@mediatek.com> > > Signed-off-by: Hsiao Chien Sung <shawn.sung@mediatek.com> > --- > drivers/gpu/drm/mediatek/mtk_disp_drv.h | 4 + > .../gpu/drm/mediatek/mtk_disp_ovl_adaptor.c | 75 +++++++++++++++ > ---- > drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 10 +-- > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 2 + > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 20 +++++ > drivers/gpu/drm/mediatek/mtk_mdp_rdma.c | 16 ++++ > 6 files changed, 107 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_drv.h > b/drivers/gpu/drm/mediatek/mtk_disp_drv.h > index bf06ccb65652..8465beeab435 100644 > --- a/drivers/gpu/drm/mediatek/mtk_disp_drv.h > +++ b/drivers/gpu/drm/mediatek/mtk_disp_drv.h > @@ -109,6 +109,8 @@ void mtk_ovl_adaptor_connect(struct device *dev, > struct device *mmsys_dev, > unsigned int next); > void mtk_ovl_adaptor_disconnect(struct device *dev, struct device > *mmsys_dev, > unsigned int next); > +int mtk_ovl_adaptor_power_on(struct device *dev); > +void mtk_ovl_adaptor_power_off(struct device *dev); > int mtk_ovl_adaptor_clk_enable(struct device *dev); > void mtk_ovl_adaptor_clk_disable(struct device *dev); > void mtk_ovl_adaptor_config(struct device *dev, unsigned int w, > @@ -150,6 +152,8 @@ void mtk_rdma_disable_vblank(struct device *dev); > const u32 *mtk_rdma_get_formats(struct device *dev); > size_t mtk_rdma_get_num_formats(struct device *dev); > > +int mtk_mdp_rdma_power_on(struct device *dev); > +void mtk_mdp_rdma_power_off(struct device *dev); > int mtk_mdp_rdma_clk_enable(struct device *dev); > void mtk_mdp_rdma_clk_disable(struct device *dev); > void mtk_mdp_rdma_start(struct device *dev, struct cmdq_pkt > *cmdq_pkt); > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c > b/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c > index 81067f49ea69..aab98adcd9a4 100644 > --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c > +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c > @@ -79,6 +79,8 @@ static const struct mtk_ddp_comp_funcs merge = { > }; > > static const struct mtk_ddp_comp_funcs rdma = { > + .power_on = mtk_mdp_rdma_power_on, > + .power_off = mtk_mdp_rdma_power_off, > .clk_enable = mtk_mdp_rdma_clk_enable, > .clk_disable = mtk_mdp_rdma_clk_disable, > }; > @@ -200,21 +202,72 @@ void mtk_ovl_adaptor_stop(struct device *dev) > mtk_ethdr_stop(ovl_adaptor- > >ovl_adaptor_comp[OVL_ADAPTOR_ETHDR0]); > } > > -int mtk_ovl_adaptor_clk_enable(struct device *dev) > +/** > + * power_off - Power off the devices in OVL adaptor > + * @dev: Device to be powered off > + * @num: Number of the devices to be powered off > + * > + * Calls the .power_off() ovl_adaptor component callback if it is > present. > + */ > +static inline void power_off(struct device *dev, unsigned int num) > { > struct mtk_disp_ovl_adaptor *ovl_adaptor = > dev_get_drvdata(dev); > - struct device *comp; > - int ret; > int i; > > - for (i = 0; i < OVL_ADAPTOR_MERGE0; i++) { > - comp = ovl_adaptor->ovl_adaptor_comp[i]; > - ret = pm_runtime_get_sync(comp); > + if (num > OVL_ADAPTOR_ID_MAX) > + num = OVL_ADAPTOR_ID_MAX; > + > + for (i = num - 1; i >= 0; i--) { > + if (!ovl_adaptor->ovl_adaptor_comp[i] || > + !comp_matches[i].funcs->power_off) > + continue; > + > + comp_matches[i].funcs->power_off(ovl_adaptor- > >ovl_adaptor_comp[i]); > + } > +} > + > +/** > + * mtk_ovl_adaptor_power_on - Power on the devices in OVL adaptor > + * @dev: Device to be powered on > + * > + * Different from OVL, OVL adaptor is a pseudo device so > + * we didn't define it in the device tree, > pm_runtime_resume_and_get() > + * called by .atomic_enable() power on no device in OVL adaptor, > + * we have to implement a function to do the job instead. > + * > + * Return: Zero for success or negative number for failure. > + */ > +int mtk_ovl_adaptor_power_on(struct device *dev) > +{ > + struct mtk_disp_ovl_adaptor *ovl_adaptor = > dev_get_drvdata(dev); > + int i, ret; > + > + for (i = 0; i < OVL_ADAPTOR_ID_MAX; i++) { > + if (!ovl_adaptor->ovl_adaptor_comp[i] || > + !comp_matches[i].funcs->power_on) > + continue; > + > + ret = comp_matches[i].funcs->power_on(ovl_adaptor- > >ovl_adaptor_comp[i]); > if (ret < 0) { > dev_err(dev, "Failed to enable power domain %d, > err %d\n", i, ret); > - goto error; > + power_off(dev, i); > + return ret; > } > } > + return 0; > +} > + > +void mtk_ovl_adaptor_power_off(struct device *dev) > +{ > + power_off(dev, OVL_ADAPTOR_ID_MAX); > +} > + > +int mtk_ovl_adaptor_clk_enable(struct device *dev) > +{ > + struct mtk_disp_ovl_adaptor *ovl_adaptor = > dev_get_drvdata(dev); > + struct device *comp; > + int ret; > + int i; > > for (i = 0; i < OVL_ADAPTOR_ID_MAX; i++) { > comp = ovl_adaptor->ovl_adaptor_comp[i]; > @@ -225,16 +278,10 @@ int mtk_ovl_adaptor_clk_enable(struct device > *dev) > dev_err(dev, "Failed to enable clock %d, err > %d\n", i, ret); > while (--i >= 0) > comp_matches[i].funcs- > >clk_disable(comp); > - i = OVL_ADAPTOR_MERGE0; > - goto error; > + return ret; > } > } > return 0; > -error: > - while (--i >= 0) > - pm_runtime_put(ovl_adaptor->ovl_adaptor_comp[i]); > - > - return ret; > } > > void mtk_ovl_adaptor_clk_disable(struct device *dev) > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c > b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c > index a0b2ba3cbcdb..bc4cc75cca18 100644 > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c > @@ -774,7 +774,7 @@ static void mtk_drm_crtc_atomic_enable(struct > drm_crtc *crtc, > return; > } > > - ret = pm_runtime_resume_and_get(comp->dev); > + ret = mtk_ddp_comp_power_on(comp); > if (ret < 0) { > DRM_DEV_ERROR(comp->dev, "Failed to enable power > domain: %d\n", ret); > return; > @@ -782,7 +782,7 @@ static void mtk_drm_crtc_atomic_enable(struct > drm_crtc *crtc, > > ret = mtk_crtc_ddp_hw_init(mtk_crtc, state); > if (ret) { > - pm_runtime_put(comp->dev); > + mtk_ddp_comp_power_off(comp); > return; > } > > @@ -795,7 +795,7 @@ static void mtk_drm_crtc_atomic_disable(struct > drm_crtc *crtc, > { > struct mtk_drm_crtc *mtk_crtc = to_mtk_crtc(crtc); > struct mtk_ddp_comp *comp = mtk_crtc->ddp_comp[0]; > - int i, ret; > + int i; > > DRM_DEBUG_DRIVER("%s %d\n", __func__, crtc->base.id); > if (!mtk_crtc->enabled) > @@ -825,9 +825,7 @@ static void mtk_drm_crtc_atomic_disable(struct > drm_crtc *crtc, > > drm_crtc_vblank_off(crtc); > mtk_crtc_ddp_hw_fini(mtk_crtc); > - ret = pm_runtime_put(comp->dev); > - if (ret < 0) > - DRM_DEV_ERROR(comp->dev, "Failed to disable power > domain: %d\n", ret); > + mtk_ddp_comp_power_off(comp); > > mtk_crtc->enabled = false; > } > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c > b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c > index 10402e07a4a7..9940909c7435 100644 > --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c > +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c > @@ -396,6 +396,8 @@ static const struct mtk_ddp_comp_funcs ddp_ufoe = > { > }; > > static const struct mtk_ddp_comp_funcs ddp_ovl_adaptor = { > + .power_on = mtk_ovl_adaptor_power_on, > + .power_off = mtk_ovl_adaptor_power_off, > .clk_enable = mtk_ovl_adaptor_clk_enable, > .clk_disable = mtk_ovl_adaptor_clk_disable, > .config = mtk_ovl_adaptor_config, > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h > b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h > index 1c1d670cfe41..2597dd7ac0d2 100644 > --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h > +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h > @@ -7,6 +7,7 @@ > #define MTK_DRM_DDP_COMP_H > > #include <linux/io.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> > @@ -46,6 +47,8 @@ enum mtk_ddp_comp_type { > struct mtk_ddp_comp; > struct cmdq_pkt; > struct mtk_ddp_comp_funcs { > + int (*power_on)(struct device *dev); > + void (*power_off)(struct device *dev); > int (*clk_enable)(struct device *dev); > void (*clk_disable)(struct device *dev); > void (*config)(struct device *dev, unsigned int w, > @@ -91,6 +94,23 @@ struct mtk_ddp_comp { > int encoder_index; > }; > > +static inline int mtk_ddp_comp_power_on(struct mtk_ddp_comp *comp) > +{ > + if (comp->funcs && comp->funcs->power_on) > + return comp->funcs->power_on(comp->dev); > + else > + return pm_runtime_resume_and_get(comp->dev); > + return 0; > +} > + > +static inline void mtk_ddp_comp_power_off(struct mtk_ddp_comp *comp) > +{ > + if (comp->funcs && comp->funcs->power_off) > + comp->funcs->power_off(comp->dev); > + else > + pm_runtime_put(comp->dev); > +} > + > static inline int mtk_ddp_comp_clk_enable(struct mtk_ddp_comp *comp) > { > if (comp->funcs && comp->funcs->clk_enable) > diff --git a/drivers/gpu/drm/mediatek/mtk_mdp_rdma.c > b/drivers/gpu/drm/mediatek/mtk_mdp_rdma.c > index 5746f06220c1..769ae7564da2 100644 > --- a/drivers/gpu/drm/mediatek/mtk_mdp_rdma.c > +++ b/drivers/gpu/drm/mediatek/mtk_mdp_rdma.c > @@ -243,6 +243,22 @@ size_t mtk_mdp_rdma_get_num_formats(struct > device *dev) > return ARRAY_SIZE(formats); > } > > +int mtk_mdp_rdma_power_on(struct device *dev) > +{ > + int ret = pm_runtime_resume_and_get(dev); > + > + if (ret < 0) { > + dev_err(dev, "Failed to power on: %d\n", ret); > + return ret; > + } > + return 0; > +} > + > +void mtk_mdp_rdma_power_off(struct device *dev) > +{ > + pm_runtime_put(dev); > +} > + > int mtk_mdp_rdma_clk_enable(struct device *dev) > { > struct mtk_mdp_rdma *rdma = dev_get_drvdata(dev);
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
This series is based on mediatek-drm-next. Changes in v10: - Remove "Reviewed-by" tags of the following commits: - drm/mediatek: Power on/off devices with function pointers - drm/mediatek: Manage component's clock with function pointers - Separate the commit into smaller ones - (new) drm/mediatek: Remove the redundant driver data for DPI Changes in v9: - Add a static inline function to power off the device - Change driver name to "mediatek-disp-padding" - Fix typo and kernel doc format error Changes in v8: - Power on/off the components with .power_on() and .power_off() - Remove mtk_padding_config() - Remove "Reviewed-by" tags in "drm/mediatek: Add Padding to OVL adaptor" because of the modifications. Changes in v7: - Start/Stop the components in OVL Adaptor with function pointers - Refine Padding driver - Fix underrun when the layer is switching off Changes in v6: - Separate the commits into smaller ones - Add DPI input mode setting - Fix VDOSYS1 power-on issues Changes in v5: - Reuse .clk_enable/.clk_disable in struct mtk_ddp_comp_funcs in mtk_disp_ovl_adaptor.c - Adjust commits order Changes in v4: - Add new functions in mtk_disp_ovl_adaptor.c to enable/disable components and reuse them when clock enable/disable - Rename components in mtk_disp_ovl_adaptor.c and sort them in alphabetical order Changes in v3: - Define macro MMSYS_RST_NR in mtk-mmsys.h and update reset table - Fix typos (ETDHR -> ETHDR, VSNYC -> VSYNC) - Rebase dt-bindings on linux-next - Refine description of Padding - Squash reset bit map commits for VDO0 and VDO1 into one Changes in v2: - Remove redundant compatibles of MT8188 because it shares the same configuration with MT8195 - Separate dt-bindings by modules - Support reset bit mapping in mmsys driver Hsiao Chien Sung (24): dt-bindings: display: mediatek: ethdr: Add compatible for MT8188 dt-bindings: display: mediatek: mdp-rdma: Add compatible for MT8188 dt-bindings: display: mediatek: merge: Add compatible for MT8188 dt-bindings: display: mediatek: padding: Add MT8188 dt-bindings: arm: mediatek: Add compatible for MT8188 dt-bindings: reset: mt8188: Add VDOSYS reset control bits soc: mediatek: Support MT8188 VDOSYS1 in mtk-mmsys soc: mediatek: Support MT8188 VDOSYS1 Padding in mtk-mmsys soc: mediatek: Support reset bit mapping in mmsys driver soc: mediatek: Add MT8188 VDOSYS reset bit map drm/mediatek: Rename OVL_ADAPTOR_TYPE_RDMA drm/mediatek: Add component ID to component match structure drm/mediatek: Manage component's clock with function pointers drm/mediatek: Power on/off devices with function pointers drm/mediatek: Remove ineffectual power management codes drm/mediatek: Start/Stop components with function pointers drm/mediatek: Sort OVL adaptor components drm/mediatek: Refine device table of OVL adaptor drm/mediatek: Support MT8188 Padding in display driver drm/mediatek: Add Padding to OVL adaptor drm/mediatek: Return error if MDP RDMA failed to enable the clock drm/mediatek: Remove the redundant driver data for DPI drm/mediatek: Fix underrun in VDO1 when switches off the layer drm/mediatek: Support MT8188 VDOSYS1 in display driver .../bindings/arm/mediatek/mediatek,mmsys.yaml | 1 + .../display/mediatek/mediatek,ethdr.yaml | 6 +- .../display/mediatek/mediatek,mdp-rdma.yaml | 6 +- .../display/mediatek/mediatek,merge.yaml | 3 + .../display/mediatek/mediatek,padding.yaml | 81 ++++++ drivers/gpu/drm/mediatek/Makefile | 3 +- drivers/gpu/drm/mediatek/mtk_disp_drv.h | 8 + drivers/gpu/drm/mediatek/mtk_disp_merge.c | 2 +- .../gpu/drm/mediatek/mtk_disp_ovl_adaptor.c | 274 +++++++++++------- drivers/gpu/drm/mediatek/mtk_dpi.c | 16 +- drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 28 +- drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 2 + drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 20 ++ drivers/gpu/drm/mediatek/mtk_drm_drv.c | 5 +- drivers/gpu/drm/mediatek/mtk_drm_drv.h | 2 +- drivers/gpu/drm/mediatek/mtk_mdp_rdma.c | 19 +- drivers/gpu/drm/mediatek/mtk_padding.c | 160 ++++++++++ drivers/soc/mediatek/mt8188-mmsys.h | 210 ++++++++++++++ drivers/soc/mediatek/mtk-mmsys.c | 27 ++ drivers/soc/mediatek/mtk-mmsys.h | 32 ++ drivers/soc/mediatek/mtk-mutex.c | 51 ++++ include/dt-bindings/reset/mt8188-resets.h | 75 +++++ include/linux/soc/mediatek/mtk-mmsys.h | 8 + 23 files changed, 893 insertions(+), 146 deletions(-) create mode 100644 Documentation/devicetree/bindings/display/mediatek/mediatek,padding.yaml create mode 100644 drivers/gpu/drm/mediatek/mtk_padding.c -- 2.18.0