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

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

On Fri, 2015-11-13 at 09:09 +0000, R, Durgadoss wrote:
> > -----Original Message-----
> > From: Vivi, Rodrigo
> > Sent: Friday, November 13, 2015 3:08 AM
> > To: intel-gfx@lists.freedesktop.org; R, Durgadoss
> > Subject: Re: [Intel-gfx] [PATCH 1/4] drm/i915: Delay first PSR 
> > activation.
> > 
> > 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;
> 
> Should we set this before scheduling the delayed work ?
> 
> > > > 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.
> 
> Yes, this is what I understood from the code. Just wanted to confirm 
> whether
> you meant the same.
> 
> The other thing I am thinking is:
> Inside intel_psr_enable() we call _activate() for DDI platforms & 
> gen>=9.
> For others, we schedule a work.
> 
> May be we should have only the worker thread do _activate() for every
> Platform .. I believe this would simplify things a lot. Not sure 
> whether this
> Will impact gen>=9 platforms in any way..

Yeap, good idea. It will make things standardized and simpler with
almost no impact on the feature and better for stability in case we
start facing this issue on newer platforms again...
I'll do an extra/separated patch for that.

> 
> Anyway, we can have that as a separate change if required and valid.
> So, for this patch:
> Reviewed-by: Durgadoss R <durgadoss.r@intel.com>

Thanks

> 
> Thanks,
> Durga
> 
> > 
> > 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