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@xxxxxxxxxxxxxxx/msg31629.html Thanks everybody for your review effort. Regards, Rahul Sharma. On 27 May 2014 18:28, Inki Dae <inki.dae@xxxxxxxxxxx> wrote: > On 2014년 05월 27일 18:55, Rahul Sharma wrote: >> <Gentle Reminder> >> >> On 26 May 2014 14:21, Rahul Sharma <rahul.sharma@xxxxxxxxxxx> wrote: >>> 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 > > 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@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 >> > > _______________________________________________ > dri-devel mailing list > dri-devel@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/dri-devel -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
 |