Re: [PATCH v2 1/3] drm/exynos: fimd: ensure proper hw state in fimd_clear_channel()

[Inki Dae <inki.dae@samsung.com>]

On 2015년 06월 12일 18:05, Marek Szyprowski wrote:
> Hello,
> 
> On 2015-06-11 17:04, Inki Dae wrote:
>> On 2015년 06월 03일 17:26, Marek Szyprowski wrote:
>>> One should not do any assumptions on the stare of the fimd hardware
>>> during driver initialization, so to properly reset fimd before enabling
>>> IOMMU, one should ensure that all power domains and clocks are really
>>> enabled. This patch adds pm_runtime and clocks management in the
>>> fimd_clear_channel() function to ensure that any access to fimd
>>> registers will be performed with clocks and power domains enabled.
>>>
>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>> Tested-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
>>> ---
>>>   drivers/gpu/drm/exynos/exynos_drm_fimd.c | 22 ++++++++++++++++++----
>>>   1 file changed, 18 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>>> b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>>> index 96618534358e..3ec9d4299a86 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>>> @@ -242,12 +242,21 @@ static void
>>> fimd_enable_shadow_channel_path(struct fimd_context *ctx,
>>>       writel(val, ctx->regs + SHADOWCON);
>>>   }
>>>   +static int fimd_enable_vblank(struct exynos_drm_crtc *crtc);
>>> +static void fimd_disable_vblank(struct exynos_drm_crtc *crtc);
>> You can remove abvoe declarations. See the below comment.
>>
>>> +
>>>   static void fimd_clear_channel(struct fimd_context *ctx)
>>>   {
>>>       unsigned int win, ch_enabled = 0;
>>>         DRM_DEBUG_KMS("%s\n", __FILE__);
>>>   +    /* Hardware is in unknown state, so ensure it gets enabled
>>> properly */
>>> +    pm_runtime_get_sync(ctx->dev);
>>> +
>>> +    clk_prepare_enable(ctx->bus_clk);
>>> +    clk_prepare_enable(ctx->lcd_clk);
>>> +
>>>       /* Check if any channel is enabled. */
>>>       for (win = 0; win < WINDOWS_NR; win++) {
>>>           u32 val = readl(ctx->regs + WINCON(win));
>>> @@ -265,12 +274,17 @@ static void fimd_clear_channel(struct
>>> fimd_context *ctx)
>>>         /* Wait for vsync, as disable channel takes effect at next
>>> vsync */
>>>       if (ch_enabled) {
>>> -        unsigned int state = ctx->suspended;
>>> -
>>> -        ctx->suspended = 0;
>>> +        ctx->suspended = false;
>>> +        fimd_enable_vblank(ctx->crtc);
>> I think you can call enable_vblank callback instead of
>> fimd_enable_vblank function because ctx object has exynos_crtc object.
>>
>> i.e.,
>>         struct exynos_drm_crtc_ops *ops = ctx->crtc->ops;
>>         ...
>>         if (ops->enable_vblank)
>>             ops->enable_vblank(ctx->crtc);
>>         ...
> 
> Well, I don't like such indirect calls to known functions, but if you
> prefer
> this approach I will send an updated patch in a minute. There is also
> alternative
> way of getting rid of forward declarations. Code of fimd_enable_vblank and
> fimd_disable_vblank can be moved closer to fimd_wait_vblank function.
> I will also send such alternative patch. Feel free to select the one
> that better
> fits your preferences.

Latter one it's better.

Thanks,
Inki Dae

> 
>>>           fimd_wait_for_vblank(ctx->crtc);
>>> -        ctx->suspended = state;
>>> +        fimd_disable_vblank(ctx->crtc);
>> Ditto.
>>
>> Thanks,
>> Inki Dae
>>
>>> +        ctx->suspended = true;
>>>       }
>>> +
>>> +    clk_disable_unprepare(ctx->lcd_clk);
>>> +    clk_disable_unprepare(ctx->bus_clk);
>>> +
>>> +    pm_runtime_put(ctx->dev);
>>>   }
>>>     static int fimd_iommu_attach_devices(struct fimd_context *ctx,
>>>
>>
> 
> Best regards