RE: [PATCH v3 25/32] drm/exynos: Clean up FIMD power on/off routines

From: Inki Dae <inki.dae@samsung.com>
To: 'Sean Paul' <seanpaul@chromium.org>, dri-devel@lists.freedesktop.org
Cc: marcheu@chromium.org
Subject: RE: [PATCH v3 25/32] drm/exynos: Clean up FIMD power on/off routines
Date: Thu, 31 Oct 2013 19:54:18 +0900	[thread overview]
Message-ID: <031901ced627$8c7fafd0$a57f0f70$%dae@samsung.com> (raw)
In-Reply-To: <1383063198-10526-26-git-send-email-seanpaul@chromium.org>

As I mentioned already, I'll not merge eDP related codes yet. So can you
rebase patche 25 through 26 to top of patch 20?

Thanks,
Inki Dae

> -----Original Message-----
> From: Sean Paul [mailto:seanpaul@chromium.org]
> Sent: Wednesday, October 30, 2013 1:13 AM
> To: dri-devel@lists.freedesktop.org; inki.dae@samsung.com
> Cc: airlied@linux.ie; tomasz.figa@gmail.com; marcheu@chromium.org; Sean
> Paul
> Subject: [PATCH v3 25/32] drm/exynos: Clean up FIMD power on/off routines
> 
> This patch separates the fimd_activate function into poweron/poweroff
> functions to be more consistent with the other drivers in exynos drm. It
> also properly cleans up after failures in poweron. The functions have
> also been shuffled around such that they are all in the same
> spot in the file and poweron/poweroff can be called from the dpms
function.
> 
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
> 
> Changes in v2:
> 	- Added to the patchset
> Changes in v3: None
> 
>  drivers/gpu/drm/exynos/exynos_drm_fimd.c | 247 +++++++++++++++++---------
> -----
>  1 file changed, 135 insertions(+), 112 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> index c6a05f6..ba12916 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> @@ -324,6 +324,12 @@ static void fimd_win_commit(struct exynos_drm_manager
> *mgr, int zpos)
> 
>  	win_data = &ctx->win_data[win];
> 
> +	/* If suspended, enable this on resume */
> +	if (ctx->suspended) {
> +		win_data->resume = true;
> +		return;
> +	}
> +
>  	/*
>  	 * SHADOWCON/PRTCON register is used for enabling timing.
>  	 *
> @@ -505,35 +511,6 @@ static void fimd_mgr_remove(struct exynos_drm_manager
> *mgr)
>  		drm_iommu_detach_device(ctx->drm_dev, ctx->dev);
>  }
> 
> -static void fimd_dpms(struct exynos_drm_manager *mgr, int mode)
> -{
> -	struct fimd_context *ctx = mgr->ctx;
> -
> -	DRM_DEBUG_KMS("%d\n", mode);
> -
> -	switch (mode) {
> -	case DRM_MODE_DPMS_ON:
> -		/*
> -		 * enable fimd hardware only if suspended status.
> -		 *
> -		 * P.S. fimd_dpms function would be called at booting time
so
> -		 * clk_enable could be called double time.
> -		 */
> -		if (ctx->suspended)
> -			pm_runtime_get_sync(ctx->dev);
> -		break;
> -	case DRM_MODE_DPMS_STANDBY:
> -	case DRM_MODE_DPMS_SUSPEND:
> -	case DRM_MODE_DPMS_OFF:
> -		if (!ctx->suspended)
> -			pm_runtime_put_sync(ctx->dev);
> -		break;
> -	default:
> -		DRM_DEBUG_KMS("unspecified mode %d\n", mode);
> -		break;
> -	}
> -}
> -
>  static u32 fimd_calc_clkdiv(struct fimd_context *ctx,
>  		const struct drm_display_mode *mode)
>  {
> @@ -726,6 +703,130 @@ static void fimd_wait_for_vblank(struct
> exynos_drm_manager *mgr)
>  		DRM_DEBUG_KMS("vblank wait timed out.\n");
>  }
> 
> +static void fimd_window_suspend(struct exynos_drm_manager *mgr)
> +{
> +	struct fimd_context *ctx = mgr->ctx;
> +	struct fimd_win_data *win_data;
> +	int i;
> +
> +	for (i = 0; i < WINDOWS_NR; i++) {
> +		win_data = &ctx->win_data[i];
> +		win_data->resume = win_data->enabled;
> +		if (win_data->enabled)
> +			fimd_win_disable(mgr, i);
> +	}
> +	fimd_wait_for_vblank(mgr);
> +}
> +
> +static void fimd_window_resume(struct exynos_drm_manager *mgr)
> +{
> +	struct fimd_context *ctx = mgr->ctx;
> +	struct fimd_win_data *win_data;
> +	int i;
> +
> +	for (i = 0; i < WINDOWS_NR; i++) {
> +		win_data = &ctx->win_data[i];
> +		win_data->enabled = win_data->resume;
> +		win_data->resume = false;
> +	}
> +}
> +
> +static int fimd_poweron(struct exynos_drm_manager *mgr)
> +{
> +	struct fimd_context *ctx = mgr->ctx;
> +	int ret;
> +
> +	if (!ctx->suspended)
> +		return 0;
> +
> +	ctx->suspended = false;
> +
> +	ret = clk_prepare_enable(ctx->bus_clk);
> +	if (ret < 0) {
> +		DRM_ERROR("Failed to prepare_enable the bus clk [%d]\n",
> ret);
> +		goto bus_clk_err;
> +	}
> +
> +	ret = clk_prepare_enable(ctx->lcd_clk);
> +	if  (ret < 0) {
> +		DRM_ERROR("Failed to prepare_enable the lcd clk [%d]\n",
> ret);
> +		goto lcd_clk_err;
> +	}
> +
> +	/* if vblank was enabled status, enable it again. */
> +	if (test_and_clear_bit(0, &ctx->irq_flags)) {
> +		ret = fimd_enable_vblank(mgr);
> +		if (ret) {
> +			DRM_ERROR("Failed to re-enable vblank [%d]\n", ret);
> +			goto enable_vblank_err;
> +		}
> +	}
> +
> +	fimd_window_resume(mgr);
> +
> +	fimd_apply(mgr);
> +
> +	return 0;
> +
> +enable_vblank_err:
> +	clk_disable_unprepare(ctx->lcd_clk);
> +lcd_clk_err:
> +	clk_disable_unprepare(ctx->bus_clk);
> +bus_clk_err:
> +	ctx->suspended = true;
> +	return ret;
> +}
> +
> +static int fimd_poweroff(struct exynos_drm_manager *mgr)
> +{
> +	struct fimd_context *ctx = mgr->ctx;
> +
> +	if (ctx->suspended)
> +		return 0;
> +
> +	/*
> +	 * We need to make sure that all windows are disabled before we
> +	 * suspend that connector. Otherwise we might try to scan from
> +	 * a destroyed buffer later.
> +	 */
> +	fimd_window_suspend(mgr);
> +
> +	clk_disable_unprepare(ctx->lcd_clk);
> +	clk_disable_unprepare(ctx->bus_clk);
> +
> +	ctx->suspended = true;
> +	return 0;
> +}
> +
> +static void fimd_dpms(struct exynos_drm_manager *mgr, int mode)
> +{
> +	struct fimd_context *ctx = mgr->ctx;
> +
> +	DRM_DEBUG_KMS("%s, %d\n", __FILE__, mode);
> +
> +	switch (mode) {
> +	case DRM_MODE_DPMS_ON:
> +		/*
> +		 * enable fimd hardware only if suspended status.
> +		 *
> +		 * P.S. fimd_dpms function would be called at booting time
so
> +		 * clk_enable could be called double time.
> +		 */
> +		if (ctx->suspended)
> +			pm_runtime_get_sync(ctx->dev);
> +		break;
> +	case DRM_MODE_DPMS_STANDBY:
> +	case DRM_MODE_DPMS_SUSPEND:
> +	case DRM_MODE_DPMS_OFF:
> +		if (!ctx->suspended)
> +			pm_runtime_put_sync(ctx->dev);
> +		break;
> +	default:
> +		DRM_DEBUG_KMS("unspecified mode %d\n", mode);
> +		break;
> +	}
> +}
> +
>  static struct exynos_drm_manager_ops fimd_manager_ops = {
>  	.initialize = fimd_mgr_initialize,
>  	.remove = fimd_mgr_remove,
> @@ -786,85 +887,6 @@ static void fimd_clear_win(struct fimd_context *ctx,
> int win)
>  	fimd_shadow_protect_win(ctx, win, false);
>  }
> 
> -static int fimd_clock(struct fimd_context *ctx, bool enable)
> -{
> -	if (enable) {
> -		int ret;
> -
> -		ret = clk_prepare_enable(ctx->bus_clk);
> -		if (ret < 0)
> -			return ret;
> -
> -		ret = clk_prepare_enable(ctx->lcd_clk);
> -		if  (ret < 0) {
> -			clk_disable_unprepare(ctx->bus_clk);
> -			return ret;
> -		}
> -	} else {
> -		clk_disable_unprepare(ctx->lcd_clk);
> -		clk_disable_unprepare(ctx->bus_clk);
> -	}
> -
> -	return 0;
> -}
> -
> -static void fimd_window_suspend(struct exynos_drm_manager *mgr)
> -{
> -	struct fimd_context *ctx = mgr->ctx;
> -	struct fimd_win_data *win_data;
> -	int i;
> -
> -	for (i = 0; i < WINDOWS_NR; i++) {
> -		win_data = &ctx->win_data[i];
> -		win_data->resume = win_data->enabled;
> -		fimd_win_disable(mgr, i);
> -	}
> -	fimd_wait_for_vblank(mgr);
> -}
> -
> -static void fimd_window_resume(struct exynos_drm_manager *mgr)
> -{
> -	struct fimd_context *ctx = mgr->ctx;
> -	struct fimd_win_data *win_data;
> -	int i;
> -
> -	for (i = 0; i < WINDOWS_NR; i++) {
> -		win_data = &ctx->win_data[i];
> -		win_data->enabled = win_data->resume;
> -		win_data->resume = false;
> -	}
> -}
> -
> -static int fimd_activate(struct exynos_drm_manager *mgr, bool enable)
> -{
> -	struct fimd_context *ctx = mgr->ctx;
> -
> -	if (enable) {
> -		int ret;
> -
> -		ret = fimd_clock(ctx, true);
> -		if (ret < 0)
> -			return ret;
> -
> -		ctx->suspended = false;
> -
> -		/* if vblank was enabled status, enable it again. */
> -		if (test_and_clear_bit(0, &ctx->irq_flags))
> -			fimd_enable_vblank(mgr);
> -
> -		fimd_window_resume(mgr);
> -
> -		fimd_apply(mgr);
> -	} else {
> -		fimd_window_suspend(mgr);
> -
> -		fimd_clock(ctx, false);
> -		ctx->suspended = true;
> -	}
> -
> -	return 0;
> -}
> -
>  static int fimd_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -881,6 +903,7 @@ static int fimd_probe(struct platform_device *pdev)
>  		return -ENOMEM;
> 
>  	ctx->dev = dev;
> +	ctx->suspended = true;
> 
>  	if (of_property_read_bool(dev->of_node, "samsung,invert-vden"))
>  		ctx->vidcon1 |= VIDCON1_INV_VDEN;
> @@ -967,7 +990,7 @@ static int fimd_suspend(struct device *dev)
>  	 * because the usage_count of pm runtime is more than 1.
>  	 */
>  	if (!pm_runtime_suspended(dev))
> -		return fimd_activate(mgr, false);
> +		return fimd_poweroff(mgr);
> 
>  	return 0;
>  }
> @@ -984,7 +1007,7 @@ static int fimd_resume(struct device *dev)
>  	if (pm_runtime_suspended(dev))
>  		return 0;
> 
> -	return fimd_activate(mgr, true);
> +	return fimd_poweron(mgr);
>  }
>  #endif
> 
> @@ -993,14 +1016,14 @@ static int fimd_runtime_suspend(struct device *dev)
>  {
>  	struct exynos_drm_manager *mgr = get_fimd_manager(dev);
> 
> -	return fimd_activate(mgr, false);
> +	return fimd_poweroff(mgr);
>  }
> 
>  static int fimd_runtime_resume(struct device *dev)
>  {
>  	struct exynos_drm_manager *mgr = get_fimd_manager(dev);
> 
> -	return fimd_activate(mgr, true);
> +	return fimd_poweron(mgr);
>  }
>  #endif
> 
> --
> 1.8.4