Re: [PATCH 1/4] drm/i915: Delay first PSR activation.

["Vivi, Rodrigo" <rodrigo.vivi@intel.com>]

On Thu, 2015-11-12 at 13:50 +0000, R, Durgadoss wrote:
> Hi Rodrigo,
> 
> > -----Original Message-----
> > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On 
> > Behalf Of Rodrigo Vivi
> > Sent: Thursday, November 12, 2015 1:07 AM
> > To: intel-gfx@lists.freedesktop.org
> > Cc: Vivi, Rodrigo
> > Subject: [Intel-gfx] [PATCH 1/4] drm/i915: Delay first PSR 
> > activation.
> > 
> > When debuging the frozen screen caused by HW tracking with low
> > power state I noticed that if we keep moving the mouse non stop
> > you will miss the screen updates for a while. At least
> > until we stop moving the mouse for a small time and move again.
> > 
> > The actual enabling should happen immediately after
> > Display Port enabling sequence finished with links trained and
> > everything enabled. However we face many issues when enabling PSR
> > right after a modeset.
> > 
> > On VLV/CHV we face blank screens on this scenario and on HSW+
> > we face a recoverable frozen screen, at least until next
> > exit-activate sequence.
> > 
> > Another workaround for the same issue here would be to increase
> > re-enable idle time from 100 to 500 as we did for VLV/CHV.
> > However this patch workaround this issue in a better
> > way since it doesn't reduce PSR residency and also
> > allow us to reduce the delay time between re-enables at least
> > on VLV/CHV.
> > 
> > This is also important to make the sysfs toggle working properly.
> > 
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_psr.c | 18 ++++++++++++++++--
> > 1 file changed, 16 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c 
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 213581c..6b24c24 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -427,6 +427,19 @@ void intel_psr_enable(struct intel_dp 
> > *intel_dp)
> > 		vlv_psr_enable_source(intel_dp);
> > 	}
> > 
> > +	/*
> > +	 * FIXME: Activation should happen immediately since this 
> > function
> > +	 * is just called after pipe is fully trained and enabled.
> > +	 * However on every platform we face issues when first 
> > activation
> > +	 * follows a modeset so quickly.
> > +	 *     - On VLV/CHV we get bank screen on first activation
> > +	 *     - On HSW/BDW we get a recoverable frozen screen 
> > until next
> > +	 *       exit-activate sequence.
> > +	 */
> > +	if (INTEL_INFO(dev)->gen < 9)
> > +		schedule_delayed_work(&dev_priv->psr.work,
> > +				      msecs_to_jiffies(intel_dp
> > ->panel_power_cycle_delay * 5));
> > +
> > 	dev_priv->psr.enabled = intel_dp;
> > unlock:
> > 	mutex_unlock(&dev_priv->psr.lock);
> > @@ -735,8 +748,9 @@ void intel_psr_flush(struct drm_device *dev,
> > 	}
> > 
> > 	if (!dev_priv->psr.active && !dev_priv
> > ->psr.busy_frontbuffer_bits)
> > -		schedule_delayed_work(&dev_priv->psr.work,
> > -				      msecs_to_jiffies(delay_ms));
> > +		if (!work_busy(&dev_priv->psr.work.work))
> > +			schedule_delayed_work(&dev_priv->psr.work,
> > +					     
> >  msecs_to_jiffies(delay_ms));
> 
> Agree with the theory of the patch as such.. But, Is there any 
> specific reason for
> the !work_busy() check here ?
> 
> I believe when the later work runs, it will anyway bail out in 
> _activate
> function, if it sees PSR_ENABLE bit set already. So, is this check 
> just to
> prevent scheduling one more work item when there is one pending
> already ? (or it helps in something else also ?)

The !work_busy is to prevent that eventual _activate call reduce the
first activation time.

for instance:

0s - we enable and schedule first activation to 2.5s
1s - we got a page flip that flushed fb tracking and called
psr_activation to 0.1s 
1.1s - psr is activated

while we want

0s - we enable and schedule first activation to 2.5s
1s - we got a page
flip that flushed fb tracking and called psr_activation to 0.1s # just
ignore and move ahead since we are going to activate it soon.
2.5s - psr
is activated

I'm open to hear ideas to make it better or more clear.


> 
> Thanks,
> Durga

Thank you very much for all the reviews!

> 
> > 	mutex_unlock(&dev_priv->psr.lock);
> > }
> > 
> > --
> > 2.4.3
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx