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

[Sean Paul <seanpaul@chromium.org>]

On Thu, Oct 31, 2013 at 7:39 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> 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.
>

Hi Tomasz,
I've done as you suggest, I'll post a new version once we settle on
the other issues you brought up.

Sean

> Otherwise the patch looks good to me and improves things indeed.
>
> Best regards,
> Tomasz
>