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

[Rahul Sharma]

<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
> 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