[PATCH v5] drm/exynos: enable fimd clocks in probe before accessing fimd registers

[Rahul Sharma]

Thanks Inki,

I removed fimd_clear_win from fimd_probe and verified this change in chrome
setup. Not seeing any noticeable difference.

I have posted another patch at:
http://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg31629.html

Thanks everybody for your review effort.

Regards,
Rahul Sharma.


On 27 May 2014 18:28, Inki Dae <inki.dae at samsung.com> wrote:
> On 2014년 05월 27일 18:55, Rahul Sharma wrote:
>> <Gentle Reminder>
>>
>> On 26 May 2014 14:21, Rahul Sharma <rahul.sharma at samsung.com> wrote:
>>> Hi Daniel,
>>>
>>> On 26 May 2014 13:11, Daniel Kurtz <djkurtz at chromium.org> wrote:
>>>> On Mon, May 26, 2014 at 2:59 PM, Rahul Sharma <rahul.sharma at samsung.com> wrote:
>>>>>
>>>>> Hi Inki,
>>>>>
>>>>> Please review this patch.
>>> [snip]
>>>>>> +
>>>>>> +       ret = clk_prepare_enable(ctx->lcd_clk);
>>>>
>>>> Hi Rahul,
>>>>
>>>> Can you explain why exactly we are "clearing windows" here in probe(), anyway?
>>>
>>> I am not sure why it was added there but it is present since the first
>>> version of this
>>> file. Probably Inki can explain about this :). I can see the change
>>> coming from his
>>> first patch for adding drm fimd driver.
>>>
>>>>
>>>> IIUC, bus_clk is the clock that enables FIMD register access, and
>>>> lcd_clk clocks the scan out engine.
>>>> Therefore, if we only need to read/write some registers, we only need
>>>> the bus_clk, not lcd_clk, right?
>>>>
>>>
>>> Correct, bus_clk should be sufficient to access the registers. But unless we
>>> are confident about all implicit clock requirements in all SoCs, it is
>>> safer to follow
>>> the power_on/off sequence. This implementation is as good as DPMS on -> perform
>>> reg operation -> DPMS Off. It was same in the original version but
>>> later clock enables
>>> were moved out of the probe.
>>>
>>>> However, fimd_clear_win() actually clears per-window registers.
>>>> Writes to per-window registers typically do not take effect until the
>>>> next vblank.
>>>> Therefore we do would need to enable lcd_clk to ensure that these
>>>> changes take effect.
>>>> Furthermore, to ensure the window clear completes during probe(), we
>>>> would also need to synchronously wait for the next vblank here - but
>>>> only if FIMD scanout is actually enabled already, otherwise there will
>>>> never be a next scanout, so we must check for that first.
>>>> Lastly, waiting around for a vblank could take a while.  Doing so in
>>>> probe() is not very friendly to boot up time, so the waiting should
>>>> probably be moved out of the main probe() thread into some sort of
>>>> asynchronous handler, which could then signal back when the clear is
>>>> complete.
>>>>
>>>> Do you agree, or am I missing something?
>>>
>>> I agree. There seems a room for improvement. But at present we have two options,
>>> either fix the current implementation and try to improve it as you mentioned
>>> above. OR remove fimd_clear_win from probe if it is just a legacy code which
>
> Just let's remove fimd_clear_win. it wouldn't need to disable all
> hardware overlays at probe.
>
> Thanks,
> Inki Dae
>
>>> is no more required.
>>>
>>> @Inki, need your inputs here.
>>>
>>> Regards,
>>> Rahul Sharma.
>>>
>>>>
>>>> Thanks,
>>>> -djk
>>>>
>>>>>
>>>>>> +       if (ret) {
>>> [snip]
>>>>>> +pm_put_err:
>>>>>> +       return ret;
>>>>>>  }
>>>>>>
>>>>>>  static void fimd_unbind(struct device *dev, struct device *master,
>>>>>> --
>>>>>> 1.7.9.5
>>>>>>
>>>>> _______________________________________________
>>>>> dri-devel mailing list
>>>>> dri-devel at lists.freedesktop.org
>>>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel at lists.freedesktop.org
>>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel