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 in OVL adaptor and
leads to power outage in the corresponding IOMMU.

To resolve the issue, we implement a function to power on the RDMAs
in OVL adaptor, and the system will make sure the IOMMU is powered on
as well because of the device link (iommus) in the RDMA nodes in DTS.

Fixes: 5db12f5d843b ("media: drm/mediatek: Add pm runtime support for ovl and rdma")

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   | 62 +++++++++++++++++++
 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_mdp_rdma.c       | 16 +++++
 6 files changed, 111 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_disp_drv.h b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
index e2b602037ac3..c44f5b31bab5 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 b80425360e76..8de57a5f5518 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
@@ -98,6 +98,8 @@ static const struct mtk_ddp_comp_funcs _padding = {
 };
 
 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,
 };
@@ -241,6 +243,66 @@ void mtk_ovl_adaptor_stop(struct device *dev)
 	}
 }
 
+/**
+ * mtk_ovl_adaptor_power_on - Power on 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.
+ *
+ * returns:
+ * zero on success, errno on failures.
+ */
+int mtk_ovl_adaptor_power_on(struct device *dev)
+{
+	int i, ret;
+	struct mtk_disp_ovl_adaptor *ovl_adaptor = dev_get_drvdata(dev);
+
+	for (i = 0; i < OVL_ADAPTOR_ID_MAX; i++) {
+		if (!ovl_adaptor->ovl_adaptor_comp[i] ||
+		    !comp_matches[i].funcs->power_on)
+			continue;
+
+		/*
+		 * do not power on the devices that don't define
+		 * .power_off() function
+		 */
+		if (!comp_matches[i].funcs->power_off) {
+			dev_warn(dev, ".power_off() is undefined\n");
+			continue;
+		}
+
+		ret = comp_matches[i].funcs->power_on(ovl_adaptor->ovl_adaptor_comp[i]);
+		if (ret < 0) {
+			mtk_ovl_adaptor_power_off(dev);
+			return ret;
+		}
+	}
+	return 0;
+}
+
+/**
+ * mtk_ovl_adaptor_power_off - Power off devices in OVL adaptor
+ * @dev: device to be powered off
+ *
+ * call .power_off() function if defined
+ */
+void mtk_ovl_adaptor_power_off(struct device *dev)
+{
+	int i;
+	struct mtk_disp_ovl_adaptor *ovl_adaptor = dev_get_drvdata(dev);
+
+	for (i = 0; i < OVL_ADAPTOR_ID_MAX; 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]);
+	}
+}
+
 int mtk_ovl_adaptor_clk_enable(struct device *dev)
 {
 	int i;
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
index a0b2ba3cbcdb..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);
@@ -774,7 +762,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 +770,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 +783,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 +813,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 cb36a961786f..8feeb6dce217 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);
-- 
2.18.0

Hi, Hsiao-chien:

On Mon, 2023-10-16 at 18:40 +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 in OVL adaptor and
> leads to power outage in the corresponding IOMMU.
> 
> To resolve the issue, we implement a function to power on the RDMAs
> in OVL adaptor, and the system will make sure the IOMMU is powered on
> as well because of the device link (iommus) in the RDMA nodes in DTS.
> 
> Fixes: 5db12f5d843b ("media: drm/mediatek: Add pm runtime support for
> ovl and rdma")
> 
> 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   | 62
> +++++++++++++++++++
>  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_mdp_rdma.c       | 16 +++++
>  6 files changed, 111 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> index e2b602037ac3..c44f5b31bab5 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 b80425360e76..8de57a5f5518 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
> @@ -98,6 +98,8 @@ static const struct mtk_ddp_comp_funcs _padding = {
>  };
>  
>  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,
>  };
> @@ -241,6 +243,66 @@ void mtk_ovl_adaptor_stop(struct device *dev)
>  	}
>  }
>  
> +/**
> + * mtk_ovl_adaptor_power_on - Power on 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.
> + *
> + * returns:
> + * zero on success, errno on failures.
> + */
> +int mtk_ovl_adaptor_power_on(struct device *dev)
> +{
> +	int i, ret;
> +	struct mtk_disp_ovl_adaptor *ovl_adaptor =
> dev_get_drvdata(dev);
> +
> +	for (i = 0; i < OVL_ADAPTOR_ID_MAX; i++) {
> +		if (!ovl_adaptor->ovl_adaptor_comp[i] ||
> +		    !comp_matches[i].funcs->power_on)
> +			continue;

To simplify the code, you could call mtk_ddp_comp_power_on() for all
sub device, and drop power_on()/power_off() of mdp_rdma.

Regards,
CK

> +
> +		/*
> +		 * do not power on the devices that don't define
> +		 * .power_off() function
> +		 */
> +		if (!comp_matches[i].funcs->power_off) {
> +			dev_warn(dev, ".power_off() is undefined\n");
> +			continue;
> +		}
> +
> +		ret = comp_matches[i].funcs->power_on(ovl_adaptor-
> >ovl_adaptor_comp[i]);
> +		if (ret < 0) {
> +			mtk_ovl_adaptor_power_off(dev);
> +			return ret;
> +		}
> +	}
> +	return 0;
> +}
> +
> +/**
> + * mtk_ovl_adaptor_power_off - Power off devices in OVL adaptor
> + * @dev: device to be powered off
> + *
> + * call .power_off() function if defined
> + */
> +void mtk_ovl_adaptor_power_off(struct device *dev)
> +{
> +	int i;
> +	struct mtk_disp_ovl_adaptor *ovl_adaptor =
> dev_get_drvdata(dev);
> +
> +	for (i = 0; i < OVL_ADAPTOR_ID_MAX; 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]);
> +	}
> +}
> +
>  int mtk_ovl_adaptor_clk_enable(struct device *dev)
>  {
>  	int i;
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> index a0b2ba3cbcdb..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);
> @@ -774,7 +762,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 +770,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 +783,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 +813,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 cb36a961786f..8feeb6dce217 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 CK,

On Wed, 2023-10-18 at 02:02 +0000, CK Hu (胡俊光) wrote:
> Hi, Hsiao-chien:
> 
> On Mon, 2023-10-16 at 18:40 +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 in OVL adaptor and
> > leads to power outage in the corresponding IOMMU.
> > 
... snip ...
> > +int mtk_ovl_adaptor_power_on(struct device *dev)
> > +{
> > +	int i, ret;
> > +	struct mtk_disp_ovl_adaptor *ovl_adaptor =
> > dev_get_drvdata(dev);
> > +
> > +	for (i = 0; i < OVL_ADAPTOR_ID_MAX; i++) {
> > +		if (!ovl_adaptor->ovl_adaptor_comp[i] ||
> > +		    !comp_matches[i].funcs->power_on)
> > +			continue;
> 
> To simplify the code, you could call mtk_ddp_comp_power_on() for all
> sub device, and drop power_on()/power_off() of mdp_rdma.
> 
> Regards,
> CK
> 

Since ovl_adaptor_comp is a `struct device`, we can't reuse
mtk_ddp_comp_power_on() here. Had submited a new version (v9) and wrap
the power off procedure as a static inline function to reuse it.

Regards,
Shawn

Il 16/10/23 12:40, 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 in OVL adaptor and
> leads to power outage in the corresponding IOMMU.
> 
> To resolve the issue, we implement a function to power on the RDMAs
> in OVL adaptor, and the system will make sure the IOMMU is powered on
> as well because of the device link (iommus) in the RDMA nodes in DTS.
> 
> Fixes: 5db12f5d843b ("media: drm/mediatek: Add pm runtime support for ovl and rdma")
> 
> 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   | 62 +++++++++++++++++++
>   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_mdp_rdma.c       | 16 +++++
>   6 files changed, 111 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_drv.h b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> index e2b602037ac3..c44f5b31bab5 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 b80425360e76..8de57a5f5518 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
> @@ -98,6 +98,8 @@ static const struct mtk_ddp_comp_funcs _padding = {
>   };
>   
>   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,
>   };
> @@ -241,6 +243,66 @@ void mtk_ovl_adaptor_stop(struct device *dev)
>   	}
>   }
>   
> +/**
> + * mtk_ovl_adaptor_power_on - Power on 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.
> + *
> + * returns:
> + * zero on success, errno on failures.

You're almost there! There's just one mistake making this invalid kerneldoc;
change to...

  * Return: Zero for success or negative number for failure.

https://docs.kernel.org/doc-guide/kernel-doc.html

> + */
> +int mtk_ovl_adaptor_power_on(struct device *dev)
> +{
> +	int i, ret;
> +	struct mtk_disp_ovl_adaptor *ovl_adaptor = dev_get_drvdata(dev);
> +
> +	for (i = 0; i < OVL_ADAPTOR_ID_MAX; i++) {
> +		if (!ovl_adaptor->ovl_adaptor_comp[i] ||
> +		    !comp_matches[i].funcs->power_on)
> +			continue;
> +
> +		/*
> +		 * do not power on the devices that don't define
> +		 * .power_off() function
> +		 */
> +		if (!comp_matches[i].funcs->power_off) {
> +			dev_warn(dev, ".power_off() is undefined\n");
> +			continue;
> +		}
> +
> +		ret = comp_matches[i].funcs->power_on(ovl_adaptor->ovl_adaptor_comp[i]);
> +		if (ret < 0) {
> +			mtk_ovl_adaptor_power_off(dev);
> +			return ret;
> +		}
> +	}
> +	return 0;
> +}
> +
> +/**
> + * mtk_ovl_adaptor_power_off - Power off devices in OVL adaptor
> + * @dev: device to be powered off
> + *
> + * call .power_off() function if defined

  * Calls the .power_off() ovl_adaptor component callback if it is present.

> + */

Regards,
Angelo

Hi Angelo,

On Tue, 2023-10-17 at 11:54 +0200, AngeloGioacchino Del Regno wrote:
> Il 16/10/23 12:40, 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 in OVL adaptor and
> > leads to power outage in the corresponding IOMMU.
> > 
> > To resolve the issue, we implement a function to power on the RDMAs
> > in OVL adaptor, and the system will make sure the IOMMU is powered
> > on
> > as well because of the device link (iommus) in the RDMA nodes in
> > DTS.
> > 
> > Fixes: 5db12f5d843b ("media: drm/mediatek: Add pm runtime support
> > for ovl and rdma")
> > 
> > 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   | 62
> > +++++++++++++++++++
> >   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_mdp_rdma.c       | 16 +++++
> >   6 files changed, 111 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> > b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> > index e2b602037ac3..c44f5b31bab5 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 b80425360e76..8de57a5f5518 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
> > @@ -98,6 +98,8 @@ static const struct mtk_ddp_comp_funcs _padding =
> > {
> >   };
> >   
> >   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,
> >   };
> > @@ -241,6 +243,66 @@ void mtk_ovl_adaptor_stop(struct device *dev)
> >   	}
> >   }
> >   
> > +/**
> > + * mtk_ovl_adaptor_power_on - Power on 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.
> > + *
> > + * returns:
> > + * zero on success, errno on failures.
> 
> You're almost there! There's just one mistake making this invalid
> kerneldoc;
> change to...
> 
>   * Return: Zero for success or negative number for failure.
> 
> 
https://urldefense.com/v3/__https://docs.kernel.org/doc-guide/kernel-doc.html__;!!CTRNKA9wMg0ARbw!kAfpT6aSoiHbv1EviMygmToxfzcWTHgbJ13w--JKWitq0e_Im6koKGFFXnCt_ZZ_BD4IoqqG9vuBBU20M8gW0hTHyG_mh7qX$
>  

Thank you for pointing this out and giving an example.
Will change this in the next version.

> > +/**
> > + * mtk_ovl_adaptor_power_off - Power off devices in OVL adaptor
> > + * @dev: device to be powered off
> > + *
> > + * call .power_off() function if defined
> 
>   * Calls the .power_off() ovl_adaptor component callback if it is
> present.
> 
> > + */
> 
> Regards,
> Angelo
> 

Got it, will modify it in the next version.

Thanks,
Shawn