On Thu, 2018-02-08 at 23:39 -0800, Rodrigo Vivi wrote:
> Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> writes:
>
> > "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@xxxxxxxxx> writes:
> >
> >> On Thu, 2018-02-08 at 14:48 -0800, Rodrigo Vivi wrote:
> >>> Hi Andy,
> >>>
> >>> thanks for getting involved with PSR and sorry for not replying sooner.
> >>>
> >>> I first saw this patch on that bugzilla entry but only now I stop to
> >>> really think why I have written the code that way.
> >>>
> >>> So some clarity below.
> >>>
> >>> On Mon, Feb 05, 2018 at 10:07:09PM +0000, Andy Lutomirski wrote:
> >>> > The current PSR code has a two call sites that each schedule delayed
> >>> > work to activate PSR. As far as I can tell, each call site intends
> >>> > to keep PSR inactive for the given amount of time and then allow it
> >>> > to be activated.
> >>> >
> >>> > The call sites are:
> >>> >
> >>> > - intel_psr_enable(), which explicitly states in a comment that
> >>> > it's trying to keep PSR off a short time after the dispay is
> >>> > initialized as a workaround.
> >>>
> >>> First of all I really want to kill this call here and remove the
> >>> FIXME. It was an ugly hack that I added to solve a corner case
> >>> that was leaving me with blank screens when activating so sooner.
> >>>
> >>> >
> >>> > - intel_psr_flush(). There isn't an explcit explanation, but the
> >>> > intent is presumably to keep PSR off until the display has been
> >>> > idle for 100ms.
> >>>
> >>> The reason for 100 is kind of ugly-nonsense-empirical value
> >>> I concluded from VLV/CHV experience.
> >>> On platforms with HW tracking HW waits few identical frames
> >>> until really activating PSR. VLV/CHV activation is immediate.
> >>> But HW is also different and there it seemed that hw needed a
> >>> few more time before starting the transitions.
> >>> Furthermore I didn't want to add that so quickly because I didn't
> >>> want to take the risk of killing battery with software tracking
> >>> when doing transitions so quickly using software tracking.
> >>>
> >>> >
> >>> > The current code doesn't actually accomplish either of these goals.
> >>> > Rather than keeping PSR inactive for the given amount of time, it
> >>> > will schedule PSR for activation after the given time, with the
> >>> > earliest target time in such a request winning.
> >>>
> >>> Putting that way I was asking myself how that hack had ever fixed
> >>> my issue. Because the way you explained here seems obvious that it
> >>> wouldn't ever fix my bug or any other.
> >>>
> >>> So I applied your patch and it made even more sense (without considering
> >>> the fact I want to kill the first call anyways).
> >>>
> >>> So I came back, removed your patch and tried to understand how did
> >>> it ever worked.
> >>>
> >>> So, the thing is that intel_psr_flush will never be really executed
> >>> if intel_psr_enable wasn't executed. That is guaranteed by:
> >>>
> >>> mutex_lock(&dev_priv->psr.lock);
> >>> if (!dev_priv->psr.enabled) {
> >>>
> >>> So, intel_psr_enable will be for sure the first one to schedule the
> >>> work delayed to the ugly higher delay.
> >>>
> >>> >
> >>> > In other words, if intel_psr_enable() is immediately followed by
> >>> > intel_psr_flush(), then PSR will be activated after 100ms even if
> >>> > intel_psr_enable() wanted a longer delay. And, if the screen is
> >>> > being constantly updated so that intel_psr_flush() is called once
> >>> > per frame at 60Hz, PSR will still be activated once every 100ms.
> >>>
> >>> During this time you are right, many calls of intel_psr_exit
> >>> coming from flush functions can be called... But none of
> >>> them will schedule the work with 100 delay.
> >>>
> >>> they will skip on
> >>> if (!work_busy(&dev_priv->psr.work.work))
> >>
> >> Wouldn't work_busy() return false until the work is actually queued
> >> which is 100ms after calling schedule_delayed_work()?
> >
> > That's not my understanding of work_busy man.
> >
> > "work_busy - test whether a work is currently pending or running"
Sorry about this, WORK_STRUCT_PENDING_BIT is indeed set as soon as
schedule_delayed_work() is called.
> >
> > I consider it as pending one...
> >
> > But yeap... it was a long time ago that I did this so I'm not sure...
>
> I wouldn't be able to sleep today if I hand't experiment this.
>
> So, I move the hacked scheduled to 10s and forced psr_activate 10ms
> after that and the result is this:
>
>
> [ 11.757909] [drm:intel_psr_enable [i915]] *ERROR* I915-DEBUG:
> Scheduling 10s
> [ 11.778586] [drm:intel_psr_flush [i915]] *ERROR* I915-DEBUG: Work busy!
> ...
> a bunch more of Work busy
> ...
> [ 21.980643] [drm:intel_psr_flush [i915]] *ERROR* I915-DEBUG: Work busy!
> [ 21.983060] [drm:intel_psr_work [i915]] *ERROR* I915-DEBUG: Work running
> [ 21.986353] [drm:intel_psr_flush [i915]] *ERROR* I915-DEBUG: Scheduling normal 100ms
> So, the schedules are working as expected, I'm not crazy and I will be
> able to sleep :)
>
> (I will also attach my patch and dmesg to the bugzilla entry and close
> the bug.)
>
> But again, thank you very much for jumping here on PSR and activelly
> working to improve the code. I really appreciate that.
>
> Oh well... maybe I won't be able to sleep... I could swear this hack
> here was for SKL, but when doing the patch I noticed that the hack is
> for gen < 9... I will have to check some git history now....
>
> Thanks,
> Rodrigo.
>
> >
> >>
> >> For e.g, flushes at 0, 16, 32...96 will have work_busy() returning false
> >> until 100ms.
> >>
My bad here.
> >> The first psr_work will end up getting scheduled at 100ms, which I
> >> believe is not what we want.
> >>
> >>
> >> However, I think
> >>
> >> if (dev_priv->psr.busy_frontbuffer_bits)
> >> goto unlock;
> >>
> >> intel_psr_activate(intel_dp);
> >>
> >> in psr_work might prevent activate being called at 100ms if an
> >> invalidate happened to be called before that.
> >>
> >>
This is still an issue in the logic like Andy said.
> >>
> >>
> >>>
> >>> So, the higher delayed *hack* will be respected and PSR won't get
> >>> activated before that.
> >>>
> >>> On the other hand you might ask what if,
> >>> for some strange reason,
> >>> (intel_dp->panel_power_cycle_delay * 5) is lesser than 100.
> >>> Well, on this case this would be ok, because it happens only
> >>> once and only on gen > 9 where hw tracking will wait the minimal
> >>> number of frames before the actual transition to PSR.
> >>>
> >>> In either cases I believe we are safe.
> >>>
> >>> Thanks,
> >>> Rodrigo.
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
