[Intel-gfx] [PATCH v5 8/9] drm/i915: Force PSR exit when getting pipe CRC

[Souza, Jose]

On Thu, 2019-03-07 at 13:25 -0800, Dhinakaran Pandiyan wrote:
> On Tue, 2019-03-05 at 22:47 -0800, José Roberto de Souza wrote:
> > If PSR is active when pipe CRC is enabled the CRC calculations will
> > be inhibit by the transition to low power states that PSR brings.
> The MMIO write to enable CRCs should bring the hardware out from PSR,
> but I can imagine some initial CRCs  are going to be corrupt or
> unavailable.

That do not happen, if PSR is active and CRC is configured PSR will
remain active and no CRC will be calculated.

> 
> > So lets for a PSR exit and as soon as pipe CRC is enabled it will
> There is a missing word.

Thanks

"So lets force a PSR exit and as soon as pipe CRC is enabled it will
block PSR activation and avoid CRC timeouts when running IGT tests."


> 
> > block PSR activation avoid CRC timeouts when running IGT tests.
> 
> This surely has fdo bugs, please include them in the commit message.

I did this patch because of the regressions that I got from CI in my
testings, the only bug that I found related is 
https://bugs.freedesktop.org/show_bug.cgi?id=107814 but we don't have
PSR enabled by default on HSW.

> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_psr.c | 36 ++++++++++++++++++++--------
> > ----
> >  1 file changed, 23 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index d3e3996551c6..5d66e7313c75 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -452,6 +452,7 @@ static void hsw_activate_psr1(struct intel_dp
> > *intel_dp)
> >  	 * frames, we'll go with 9 frames for now
> >  	 */
> >  	idle_frames = max(idle_frames, dev_priv->psr.sink_sync_latency
> > + 1);
> > +
> >  	val |= idle_frames << EDP_PSR_IDLE_FRAME_SHIFT;
> >  
> >  	val |= max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT;
> > @@ -851,6 +852,20 @@ void intel_psr_disable(struct intel_dp
> > *intel_dp,
> >  	cancel_work_sync(&dev_priv->psr.work);
> >  }
> >  
> > +static void psr_force_hw_tracking_exit(struct drm_i915_private
> > *dev_priv)
> > +{
> > +	/*
> > +	 * Display WA #0884: all
> > +	 * This documented WA for bxt can be safely applied
> > +	 * broadly so we can force HW tracking to exit PSR
> > +	 * instead of disabling and re-enabling.
> > +	 * Workaround tells us to write 0 to CUR_SURFLIVE_A,
> > +	 * but it makes more sense write to the current active
> > +	 * pipe.
> > +	 */
> > +	I915_WRITE(CURSURFLIVE(dev_priv->psr.pipe), 0);
> > +}
> > +
> >  /**
> >   * intel_psr_update - Update PSR state
> >   * @intel_dp: Intel DP
> > @@ -875,8 +890,13 @@ void intel_psr_update(struct intel_dp
> > *intel_dp,
> >  	enable = crtc_state->has_psr && psr_global_enabled(psr->debug);
> >  	psr2_enable = intel_psr2_enabled(dev_priv, crtc_state);
> >  
> > -	if (enable == psr->enabled && psr2_enable == psr->psr2_enabled)
> > +	if (enable == psr->enabled && psr2_enable == psr-
> > >psr2_enabled) 
> > {
> 
> PSR2 is enabled, then user requests CRCs, the mode_changed commit
> switches to PSR1. The above condition isn't true in that case.

Not sure if I understood the question but if PSR2 is enabled and user
request CRC it will switch to PSR1 already forcing a PSR exit so we
don't need to call psr_force_hw_tracking_exit() in this case.

> 
> Also, since the CRC workaround is done before enabling pipe CRC,
> isn't
> there a possibility that the idle frame counter times out and
> activates
> PSR1 before CRC is enabled?

There still the possibility since syscalls can be preempted too but
this is a huge improvement over current scenario, now it will give at
least the time of 6 idle frames between
intel_crtc_crc_setup_workarounds() and the I915_WRITE() to the PIPE CRC
registers and it happens right after intel_crtc_crc_setup_workarounds()
returns.

> 
> > +		/* Force a PSR exit when enabling CRC to avoid CRC
> > timeouts */
> > +		if (crtc_state->crc_enabled && psr->enabled)
> > +			psr_force_hw_tracking_exit(dev_priv);
> The patch fixes a PSR1 issue, why isn't there any reference to PSR1
> anywhere?

fair, going to update the commit message.

> 
> 
> > +
> >  		goto unlock;
> > +	}
> >  
> >  	if (psr->enabled)
> >  		intel_psr_disable_locked(intel_dp);
> > @@ -1146,18 +1166,8 @@ void intel_psr_flush(struct drm_i915_private
> > *dev_priv,
> >  	dev_priv->psr.busy_frontbuffer_bits &= ~frontbuffer_bits;
> >  
> >  	/* By definition flush = invalidate + flush */
> > -	if (frontbuffer_bits) {
> > -		/*
> > -		 * Display WA #0884: all
> > -		 * This documented WA for bxt can be safely applied
> > -		 * broadly so we can force HW tracking to exit PSR
> > -		 * instead of disabling and re-enabling.
> > -		 * Workaround tells us to write 0 to CUR_SURFLIVE_A,
> > -		 * but it makes more sense write to the current active
> > -		 * pipe.
> > -		 */
> > -		I915_WRITE(CURSURFLIVE(dev_priv->psr.pipe), 0);
> > -	}
> > +	if (frontbuffer_bits)
> > +		psr_force_hw_tracking_exit(dev_priv);
> >  
> >  	if (!dev_priv->psr.active && !dev_priv-
> > > psr.busy_frontbuffer_bits)
> >  		schedule_work(&dev_priv->psr.work);
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: This is a digitally signed message part
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20190307/ebaa0ed6/attachment.sig>