Hi Daniel,
On 26 May 2014 13:11, Daniel Kurtz <djkurtz@xxxxxxxxxxxx> wrote:
> On Mon, May 26, 2014 at 2:59 PM, Rahul Sharma <rahul.sharma@xxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
