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

[Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>]

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.

>>   		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
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu