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.szyprow...@samsung.com>
>>> Tested-by: Javier Martinez Canillas <javier.marti...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
