[Intel-gfx] [PATCH 2/2] drm/i915/psr: Remove wait_for_idle() for PSR2

[Pandiyan, Dhinakaran]

On Mon, 2018-08-13 at 09:57 -0700, Rodrigo Vivi wrote:
> On Thu, Aug 09, 2018 at 05:41:35PM -0700, Dhinakaran Pandiyan wrote:
> > CI runs show PSR2 does not go to IDLE with selective update enabled
> > on
> > all PSR exit triggers. Specifically, logs indicate the hardware
> > enters
> > "SLEEP Selective Update" and not "IDLE Reset state' like the kernel
> > expects. This check was added for PSR1 but incorrectly extended to
> > PSR2,
> > remove this check for PSR2 as there is a plan to test only PSR1 on
> > PSR2
> > panels.
> > 
> > Also add bspec reference to the comment about idle timeout.
> > 
> > Cc: Tarun Vyas <tarun.vyas at intel.com>
> > Cc: José Roberto de Souza <jose.souza at intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_psr.c | 39 ++++++++++++--------------
> > ------
> >  1 file changed, 14 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 5686ddaa6a72..09be9bfee2be 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -722,37 +722,26 @@ int intel_psr_wait_for_idle(const struct
> > intel_crtc_state *new_crtc_state,
> >  {
> >  	struct intel_crtc *crtc = to_intel_crtc(new_crtc_state-
> > >base.crtc);
> >  	struct drm_i915_private *dev_priv = to_i915(crtc-
> > >base.dev);
> > -	i915_reg_t reg;
> > -	u32 mask;
> > -
> > -	if (!new_crtc_state->has_psr)
> > -		return 0;
> >  
> >  	/*
> > -	 * The sole user right now is intel_pipe_update_start(),
> > -	 * which won't race with psr_enable/disable, which is
> > -	 * where psr2_enabled is written to. So, we don't need
> > -	 * to acquire the psr.lock. More importantly, we want the
> > -	 * latency inside intel_pipe_update_start() to be as low
> > -	 * as possible, so no need to acquire psr.lock when it is
> > -	 * not needed and will induce latencies in the atomic
> > -	 * update path.
> > +	 * The sole user right now is intel_pipe_update_start(),
> > which won't
> > +	 * race with psr_enable/disable where psr2_enabled is
> > written to. So, we
> > +	 * don't need to acquire the psr.lock. More importantly,
> > we want the
> > +	 * latency inside intel_pipe_update_start() to be as low
> > as possible, so
> > +	 * no need to acquire psr.lock when it is not needed and
> > will induce
> > +	 * latencies in the atomic update path.
> >  	 */
> 
> I think we shouldn't change this format here to keep patch cleaner...
> if there is any change here I couldn't see because it is changing all
> lines and if there is no change I think it is better not to touch
> because
> it removes the focus of the real changes.

Okay.
> 
> > -	if (dev_priv->psr.psr2_enabled) {
> > -		reg = EDP_PSR2_STATUS;
> > -		mask = EDP_PSR2_STATUS_STATE_MASK;
> > -	} else {
> > -		reg = EDP_PSR_STATUS;
> > -		mask = EDP_PSR_STATUS_STATE_MASK;
> > -	}
> > +	if (!new_crtc_state->has_psr || READ_ONCE(dev_priv-
> > >psr.psr2_enabled))
> 
> I now see that we are removing psr2 of the picture, but I don't see
> how we are
> improving psr2 situation here.
> what am I missing?
> 
When the patch was written, we did not have sufficient tests to tell us
the wait_for_idle condition was wrong for PSR2. It was not known
whether the wait was *necessary* for PSR2, think of this as a partial
revert. Now that CI has pointed out, (and I checked with a PSR2 panel)
that the condition is wrong, we should be removing it for PSR2. If you
think about it, it does improve PSR2 my removing irrelevant code.


> > +		return 0;
> >  
> >  	/*
> > -	 * Max time for PSR to idle = Inverse of the refresh rate
> > +
> > -	 * 6 ms of exit training time + 1.5 ms of aux channel
> > -	 * handshake. 50 msec is defesive enough to cover
> > everything.
> > +	 * From Bspec Panel Self Refresh (BDW+):
> 
> This is another case, if we didn't change the format only this line ^
> would be in the patch and it would be cleaner and easier to review
> the
> changes.
> 
> but my biggest concern with this patch is how do we check now
> wait_psr2 idle
> 
> > +	 * Max. time for PSR to idle = inverse of the refresh rate
> > + 6 ms of
> > +	 * exit training time + 1.5 ms of aux channel handshake.
> > 50 ms is
> > +	 * defensive enough to cover everything.
> >  	 */
> > -
> > -	return __intel_wait_for_register(dev_priv, reg, mask,
> > +	return __intel_wait_for_register(dev_priv, EDP_PSR_STATUS,
> > +					 EDP_PSR_STATUS_STATE_MASK
> > ,
> >  					 EDP_PSR_STATUS_STATE_IDLE
> > , 2, 50,
> >  					 out_value);
> >  }
> > -- 
> > 2.17.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx