Re: [PATCH v3 02/32] drm/exynos: Merge overlay_ops into manager_ops

From: Tomasz Figa <tomasz.figa@gmail.com>
To: Sean Paul <seanpaul@chromium.org>
Cc: dri-devel@lists.freedesktop.org, marcheu@chromium.org
Subject: Re: [PATCH v3 02/32] drm/exynos: Merge overlay_ops into manager_ops
Date: Fri, 01 Nov 2013 00:39:35 +0100	[thread overview]
Message-ID: <1403888.Y8mcJtf5WK@flatron> (raw)
In-Reply-To: <1383063198-10526-3-git-send-email-seanpaul@chromium.org>

Hi Sean,

On Tuesday 29 of October 2013 12:12:48 Sean Paul wrote:
> This patch merges overlay_ops into manager_ops. In all cases,
> overlay_ops is implemented in the same place as manager ops, it doesn't
> serve a functional purpose, and doesn't make things more clear.
> 
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
> 
> Changes in v2: None
> Changes in v3: None
> 
>  drivers/gpu/drm/exynos/exynos_drm_drv.h     |  29 +--
>  drivers/gpu/drm/exynos/exynos_drm_encoder.c |  26 +-
>  drivers/gpu/drm/exynos/exynos_drm_fimd.c    | 363
> ++++++++++++++-------------- drivers/gpu/drm/exynos/exynos_drm_hdmi.c  
>  |  36 ++-
>  drivers/gpu/drm/exynos/exynos_drm_vidi.c    |  29 +--
>  drivers/gpu/drm/exynos/exynos_mixer.c       |   2 -
>  6 files changed, 229 insertions(+), 256 deletions(-)
[snip]
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> b/drivers/gpu/drm/exynos/exynos_drm_fimd.c index 868a14d..f271f22
> 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> @@ -181,185 +181,6 @@ static struct exynos_drm_display_ops
> fimd_display_ops = { .power_on = fimd_display_power_on,
>  };
> 
> -static void fimd_dpms(struct device *subdrv_dev, int mode)
> -{
> -	struct fimd_context *ctx = get_fimd_context(subdrv_dev);
> -
> -	DRM_DEBUG_KMS("%d\n", mode);
> -
> -	mutex_lock(&ctx->lock);
> -
> -	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(subdrv_dev);
> -		break;
> -	case DRM_MODE_DPMS_STANDBY:
> -	case DRM_MODE_DPMS_SUSPEND:
> -	case DRM_MODE_DPMS_OFF:
> -		if (!ctx->suspended)
> -			pm_runtime_put_sync(subdrv_dev);
> -		break;
> -	default:
> -		DRM_DEBUG_KMS("unspecified mode %d\n", mode);
> -		break;
> -	}
> -
> -	mutex_unlock(&ctx->lock);
> -}
[snip]
> -static void fimd_wait_for_vblank(struct device *dev)
> -{
> -	struct fimd_context *ctx = get_fimd_context(dev);
> -
> -	if (ctx->suspended)
> -		return;
> -
> -	atomic_set(&ctx->wait_vsync_event, 1);
> -
> -	/*
> -	 * wait for FIMD to signal VSYNC interrupt or return after
> -	 * timeout which is set to 50ms (refresh rate of 20).
> -	 */
> -	if (!wait_event_timeout(ctx->wait_vsync_queue,
> -				!atomic_read(&ctx->wait_vsync_event),
> -				DRM_HZ/20))
> -		DRM_DEBUG_KMS("vblank wait timed out.\n");
> -}

Do you need all the churn of moving all the functions above? I believe it 
would be enough to simply move the structure. This would greatly decrease 
the diffstat and chances of possible merge conflicts.

Otherwise the patch looks good to me and improves things indeed.

Best regards,
Tomasz