Re: [PATCH v3 26/32] drm/exynos: Consolidate suspend/resume in drm_drv

[Tomasz Figa <t.figa@samsung.com>]

Hi Sean,

On Tuesday 29 of October 2013 12:13:12 Sean Paul wrote:
> This patch removes all of the suspend/resume logic from the individual
> drivers and consolidates it in drm_drv. This consolidation reduces the
> number of functions which enable/disable the hardware to just one -- the
> dpms callback. This ensures that we always power up/down in a consistent
> manner.
> 
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
> 
> Changes in v2:
> 	- Added to the patchset
> Changes in v3:
> 	- Made appropriate changes to vidi as well (removed pm_ops)
> 
>  drivers/gpu/drm/exynos/exynos_drm_drv.c  |  97 +++++++++++++++++++++++++
>  drivers/gpu/drm/exynos/exynos_drm_fimd.c |  91 ++++-------------------
>  drivers/gpu/drm/exynos/exynos_drm_vidi.c | 119 +++++++++++++------------------
>  drivers/gpu/drm/exynos/exynos_hdmi.c     |  82 +--------------------
>  drivers/gpu/drm/exynos/exynos_mixer.c    |  75 ++++---------------
>  5 files changed, 176 insertions(+), 288 deletions(-)
[snip]
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> index ba12916..208e013 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> @@ -741,6 +741,8 @@ static int fimd_poweron(struct exynos_drm_manager *mgr)
>  
>  	ctx->suspended = false;
>  
> +	pm_runtime_get_sync(ctx->dev);
> +
>  	ret = clk_prepare_enable(ctx->bus_clk);
>  	if (ret < 0) {
>  		DRM_ERROR("Failed to prepare_enable the bus clk [%d]\n", ret);
> @@ -794,32 +796,24 @@ static int fimd_poweroff(struct exynos_drm_manager *mgr)
>  	clk_disable_unprepare(ctx->lcd_clk);
>  	clk_disable_unprepare(ctx->bus_clk);
>  
> +	pm_runtime_put_sync(ctx->dev);
> +
>  	ctx->suspended = true;
>  	return 0;
>  }
[snip]
> @@ -950,8 +944,14 @@ static int fimd_probe(struct platform_device *pdev)
>  	fimd_manager.ctx = ctx;
>  	exynos_drm_manager_register(&fimd_manager);
>  
> +	/*
> +	 * We need to runtime pm to enable/disable sysmmu since it is a child of
> +	 * this driver. Ideally, this would hang off the drm driver's runtime
> +	 * operations, but we're not quite there yet.

You also need runtime PM to control state of power domains. I don't think
we should be going away of runtime PM API. Instead DPMS callbacks could
simply call pm_runtime_get_sync/put() whenever the hardware is supposed
to go up or down, just as done above in fimd_poweron/off(). This would
allow any platform-specific PM logic placed outside of DRM subsystem (such
as power domains and IOMMU) to operate.

> +	 *
> +	 * Tracked in crbug.com/264312
> +	 */
>  	pm_runtime_enable(dev);
> -	pm_runtime_get_sync(dev);
>  
>  	for (win = 0; win < WINDOWS_NR; win++)
>  		fimd_clear_win(ctx, win);
[snip]
> diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
> index 2c127b9..c6561fe 100644
> --- a/drivers/gpu/drm/exynos/exynos_hdmi.c
> +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
[snip]
> @@ -2030,8 +2024,6 @@ static int hdmi_probe(struct platform_device *pdev)
>  	hdmi_display.ctx = hdata;
>  	exynos_drm_display_register(&hdmi_display);
>  
> -	pm_runtime_enable(dev);
> -

That's plain wrong. You need runtime PM to enable LCD power domain in
which the HDMI is placed.

>  	return 0;
>  
>  err_hdmiphy:
> @@ -2047,88 +2039,18 @@ static int hdmi_remove(struct platform_device *pdev)
>  	struct exynos_drm_display *display = get_hdmi_display(dev);
>  	struct hdmi_context *hdata = display->ctx;
>  
> -	pm_runtime_disable(dev);
> -
>  	put_device(&hdata->hdmiphy_port->dev);
>  	put_device(&hdata->ddc_port->dev);
>  
>  	return 0;
>  }
[snip]
> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
> index 39aed3e..985391d 100644
> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
[snip]
> @@ -1239,6 +1239,13 @@ static int mixer_probe(struct platform_device *pdev)
>  	platform_set_drvdata(pdev, &mixer_manager);
>  	exynos_drm_manager_register(&mixer_manager);
>  
> +	/*
> +	 * We need to runtime pm to enable/disable sysmmu since it is a child of
> +	 * this driver. Ideally, this would hang off the drm driver's runtime
> +	 * operations, but we're not quite there yet.

Same comment as for FIMD and HDMI.

Best regards,
Tomasz