Re: [Intel-gfx] [PATCH] drm/i915: Improve PSR activation timing

Re: [Intel-gfx] [PATCH] drm/i915: Improve PSR activation timing

[Pandiyan, Dhinakaran]

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()?

For e.g, flushes at 0, 16, 32...96 will have work_busy() returning false
until 100ms.

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.




> 
> 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/i915: Improve PSR activation timing

[Rodrigo Vivi]

Rodrigo Vivi <rodrigo.vivi@intel.com> writes:

> "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com> 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"
>
> 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.
>>
>> 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.
>>
>>
>>
>>
>>>
>>> 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@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Improve PSR activation timing

[Andy Lutomirski]

On Fri, Feb 9, 2018 at 7:39 AM, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> Rodrigo Vivi <rodrigo.vivi@intel.com> writes:
>
>> "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com> 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"
>>
>> 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!

This like ("Work busy!") is intel_psr_flush() noticing that
work_busy() returns true (which it does indeed do when the work is
scheduled for the future).  But this means that intel_psr_flush()
wants to keep PSR off for a little bit (10s with your patch applied)
because the screen isn't idle.

> [   21.983060] [drm:intel_psr_work [i915]] *ERROR* I915-DEBUG: Work running

And here's intel_psr_work() turning PSR back on 3ms later.

So I think you're seeing exactly the bug I described.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Improve PSR activation timing

[Pandiyan, Dhinakaran]

On Thu, 2018-02-08 at 23:39 -0800, Rodrigo Vivi wrote:
> Rodrigo Vivi <rodrigo.vivi@intel.com> writes:
> 
> > "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com> 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@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/i915: Improve PSR activation timing

[Chris Wilson]

Quoting Andy Lutomirski (2018-02-09 17:55:38)
> On Fri, Feb 9, 2018 at 7:39 AM, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > Rodrigo Vivi <rodrigo.vivi@intel.com> writes:
> > 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!
> 
> This like ("Work busy!") is intel_psr_flush() noticing that
> work_busy() returns true (which it does indeed do when the work is
> scheduled for the future).  But this means that intel_psr_flush()
> wants to keep PSR off for a little bit (10s with your patch applied)
> because the screen isn't idle.
> 
> > [   21.983060] [drm:intel_psr_work [i915]] *ERROR* I915-DEBUG: Work running
> 
> And here's intel_psr_work() turning PSR back on 3ms later.
> 
> So I think you're seeing exactly the bug I described.

It's also evident in the tests that PSR isn't being disabled as
often/quick as we need for frontbuffer updates to be seen.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Improve PSR activation timing

[Rodrigo Vivi]

On Sat, Feb 10, 2018 at 09:43:59AM +0000, Chris Wilson wrote:
> Quoting Andy Lutomirski (2018-02-09 17:55:38)
> > On Fri, Feb 9, 2018 at 7:39 AM, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > > Rodrigo Vivi <rodrigo.vivi@intel.com> writes:
> > > 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!
> > 
> > This like ("Work busy!") is intel_psr_flush() noticing that
> > work_busy() returns true (which it does indeed do when the work is
> > scheduled for the future).  But this means that intel_psr_flush()
> > wants to keep PSR off for a little bit (10s with your patch applied)
> > because the screen isn't idle.
> > 
> > > [   21.983060] [drm:intel_psr_work [i915]] *ERROR* I915-DEBUG: Work running
> > 
> > And here's intel_psr_work() turning PSR back on 3ms later.
> > 
> > So I think you're seeing exactly the bug I described.

Well, not in the way you described though.

As we can see on the log work running get executed only 10 seconds after
intel_psr_enable wanted. None of the thousand flushes happening there will
trigger psr activate before the wanted 10s.

So the hack indeed work as expected.

Given that description I proved that it works as expected and move away.
But since you insisted I step back and re-read your patch like if you had
never mentioned the hack on intel_psr_enable ever, but only focusing
on the regular calls from intel_psr_flush and you are right,
we do have a bug that would affect VLV/CHV here.

In the current code the psr_activate can happen any moment from 0ms to 100ms
after the flush.

This 100ms would just reduce the amount of fast exit-activate cases,
but not eliminate the possibility of enabling before 100ms.

On core platforms it shouldn't be any problem because HW tracking would
take care of waiting the extra idle frames.

This could be a particular problem for VLV/CHV platforms where I believe
we should wait few frames before really activating it back.

So, probably we can go ahead with your patch so we get this covered for
VLV/CHV, but I'd like a version without mentioning the intel_psr_enable
case that is already proven work as expected.

But meanwhile I will check back on the hack and see if we can already
remove that since many things got fixed and improved around eDP link training
and psr itself.

Also I will discuss with DK and check the possibility of a immediate activate
on platforms with HW tracking. Specially now that we are moving towards more
use of HW tracking.

In this case we would reduce this scheduled/timer based solution only for VLV/CHV.

And then on VLV/CHV front there are even more issues to sync with VBLANK etc in
a platform that probably went to the market without any single unit with PSR panels
that was to expensive when those platforms got released.

I even consider totally removing VLV/CHV code completely.

> 
> It's also evident in the tests that PSR isn't being disabled as
> often/quick as we need for frontbuffer updates to be seen.

Yeap, it is evident we have many bugs there, but I'm afraid this is not
the answer for the lag.
DK found some promising ones...

> -Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH] drm/i915: Improve PSR activation timing

[Rodrigo Vivi]

Rodrigo Vivi <rodrigo.vivi@intel.com> writes:

> On Sat, Feb 10, 2018 at 09:43:59AM +0000, Chris Wilson wrote:
>> Quoting Andy Lutomirski (2018-02-09 17:55:38)
>> > On Fri, Feb 9, 2018 at 7:39 AM, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
>> > > Rodrigo Vivi <rodrigo.vivi@intel.com> writes:
>> > > 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!
>> > 
>> > This like ("Work busy!") is intel_psr_flush() noticing that
>> > work_busy() returns true (which it does indeed do when the work is
>> > scheduled for the future).  But this means that intel_psr_flush()
>> > wants to keep PSR off for a little bit (10s with your patch applied)
>> > because the screen isn't idle.
>> > 
>> > > [   21.983060] [drm:intel_psr_work [i915]] *ERROR* I915-DEBUG: Work running
>> > 
>> > And here's intel_psr_work() turning PSR back on 3ms later.
>> > 
>> > So I think you're seeing exactly the bug I described.
>
> Well, not in the way you described though.
>
> As we can see on the log work running get executed only 10 seconds after
> intel_psr_enable wanted. None of the thousand flushes happening there will
> trigger psr activate before the wanted 10s.
>
> So the hack indeed work as expected.
>
> Given that description I proved that it works as expected and move away.
> But since you insisted I step back and re-read your patch like if you had
> never mentioned the hack on intel_psr_enable ever, but only focusing
> on the regular calls from intel_psr_flush and you are right,
> we do have a bug that would affect VLV/CHV here.
>
> In the current code the psr_activate can happen any moment from 0ms to 100ms
> after the flush.
>
> This 100ms would just reduce the amount of fast exit-activate cases,
> but not eliminate the possibility of enabling before 100ms.
>
> On core platforms it shouldn't be any problem because HW tracking would
> take care of waiting the extra idle frames.
>
> This could be a particular problem for VLV/CHV platforms where I believe
> we should wait few frames before really activating it back.
>
> So, probably we can go ahead with your patch so we get this covered for
> VLV/CHV, but I'd like a version without mentioning the intel_psr_enable
> case that is already proven work as expected.
>
> But meanwhile I will check back on the hack and see if we can already
> remove that since many things got fixed and improved around eDP link training
> and psr itself.
>
> Also I will discuss with DK and check the possibility of a immediate activate
> on platforms with HW tracking. Specially now that we are moving towards more
> use of HW tracking.
>
> In this case we would reduce this scheduled/timer based solution only for VLV/CHV.
>
> And then on VLV/CHV front there are even more issues to sync with VBLANK etc in
> a platform that probably went to the market without any single unit with PSR panels
> that was to expensive when those platforms got released.
>
> I even consider totally removing VLV/CHV code completely.
>
>> 
>> It's also evident in the tests that PSR isn't being disabled as
>> often/quick as we need for frontbuffer updates to be seen.
>
> Yeap, it is evident we have many bugs there, but I'm afraid this is not
> the answer for the lag.
> DK found some promising ones...

Ok. I was wrong. This also explains and fixes some cases here like
fbcon/fbdev... So:

Tested-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

I'd still want a version without mention to the schedule case at
intel_psr_enable to avoid confusion here. But since I'm now
basing my patches on top of this:

Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

>
>> -Chris
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel