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

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

On 2014년 05월 27일 18:55, Rahul Sharma wrote:
> <Gentle Reminder>
> 
> On 26 May 2014 14:21, Rahul Sharma <rahul.sharma@samsung.com> wrote:
>> Hi Daniel,
>>
>> On 26 May 2014 13:11, Daniel Kurtz <djkurtz@chromium.org> wrote:
>>> On Mon, May 26, 2014 at 2:59 PM, Rahul Sharma <rahul.sharma@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@lists.freedesktop.org
>>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>