Re: [PATCH v7 22/23] drm/mediatek: Power on devices in OVL adaptor when atomic enable

[CK Hu (胡俊光)]

Hi, Hsiao-chien:

On Fri, 2023-10-06 at 15:38 +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@xxxxxxxxxxxx>
> ---
>  drivers/gpu/drm/mediatek/mtk_disp_drv.h       |  1 +
>  .../gpu/drm/mediatek/mtk_disp_ovl_adaptor.c   | 49
> +++++++++++++++++++
>  drivers/gpu/drm/mediatek/mtk_drm_crtc.c       |  9 ++++
>  drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c   |  1 +
>  drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h   |  9 ++++
>  5 files changed, 69 insertions(+)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> index 45b30a2fe11a..971d64261fb9 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> @@ -107,6 +107,7 @@ 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);
>  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,
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
> b/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
> index c326a658dc63..ae3b6ba655b1 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
> @@ -242,6 +242,55 @@ 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 failure.
> + */
> +int mtk_ovl_adaptor_power_on(struct device *dev)
> +{
> +	int i, ret;
> +	struct device *comp;
> +	struct mtk_disp_ovl_adaptor *ovl_adaptor =
> dev_get_drvdata(dev);
> +
> +	for (i = 0; i < OVL_ADAPTOR_ID_MAX; i++) {
> +		comp = ovl_adaptor->ovl_adaptor_comp[i];
> +
> +		if (!comp)
> +			continue;
> +
> +		if (comp_matches[i].type != OVL_ADAPTOR_TYPE_MDP_RDMA)
> +			continue;
> +
> +		ret = pm_runtime_resume_and_get(comp);
> +		if (ret < 0) {
> +			dev_err(dev, "Failed to power on comp(%u):
> %d\n", i, ret);
> +			goto error;
> +		}
> +	}
> +	return 0;
> +error:
> +	while (--i >= 0) {
> +		comp = ovl_adaptor->ovl_adaptor_comp[i];
> +
> +		if (!comp)
> +			continue;
> +
> +		if (comp_matches[i].type != OVL_ADAPTOR_TYPE_MDP_RDMA)
> +			continue;
> +
> +		pm_runtime_put(comp);
> +	}
> +	return ret;
> +}
> +
>  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 b6fa4ad2f94d..5bd62027190b 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> @@ -364,6 +364,15 @@ static int mtk_crtc_ddp_hw_init(struct
> mtk_drm_crtc *mtk_crtc)
>  		return ret;
>  	}
>  
> +	for (i = 0; i < mtk_crtc->ddp_comp_nr; i++) {
> +		ret = mtk_ddp_comp_power_on(mtk_crtc->ddp_comp[i]);
> +		if (ret) {
> +			DRM_ERROR("Failed to power on %s: %d\n",
> +				  dev_name(mtk_crtc->ddp_comp[i]->dev), 
> ret);
> +			return ret;
> +		}
> +	}

I would like to replace below statement in mtk_drm_crtc_atomic_enable()

ret = pm_runtime_resume_and_get(comp-dev);

with

ret = mtk_ddp_comp_power_on(comp);

And define mtk_ddp_comp_power_on() as

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);
}

And you should also define mtk_ddp_comp_power_off().

Regards,
CK

> +
>  	ret = mtk_mutex_prepare(mtk_crtc->mutex);
>  	if (ret < 0) {
>  		DRM_ERROR("Failed to enable mutex clock: %d\n", ret);
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> index 771f4e173353..e39860f2be78 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> @@ -394,6 +394,7 @@ 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,
>  	.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 febcaeef16a1..4fef283f17d2 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> @@ -46,6 +46,7 @@ enum mtk_ddp_comp_type {
>  struct mtk_ddp_comp;
>  struct cmdq_pkt;
>  struct mtk_ddp_comp_funcs {
> +	int (*power_on)(struct device *dev);
>  	int (*clk_enable)(struct device *dev);
>  	void (*clk_disable)(struct device *dev);
>  	void (*config)(struct device *dev, unsigned int w,
> @@ -89,6 +90,14 @@ struct mtk_ddp_comp {
>  	const struct mtk_ddp_comp_funcs *funcs;
>  };
>  
> +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);
> +
> +	return 0;
> +}
> +
>  static inline int mtk_ddp_comp_clk_enable(struct mtk_ddp_comp *comp)
>  {
>  	if (comp->funcs && comp->funcs->clk_enable)