[Intel-gfx] [PATCH 5/5] drm/i915/psr: Timestamps for PSR entry and exit interrupts.

[Pandiyan, Dhinakaran]

On Thu, 2018-03-22 at 11:21 +0200, Ville Syrjälä wrote:
> On Thu, Mar 22, 2018 at 01:05:24AM +0000, Pandiyan, Dhinakaran wrote:
> > On Wed, 2018-03-21 at 21:48 +0200, Ville Syrjälä wrote:
> > > On Tue, Mar 20, 2018 at 03:41:51PM -0700, Dhinakaran Pandiyan wrote:
> > > > Timestamps are useful for IGT tests that trigger PSR exit and/or wait for
> > > > PSR entry.
> > > > 
> > > > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > > > Cc: Daniel Vetter <daniel.vetter at intel.com>
> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_debugfs.c | 12 ++++++++++++
> > > >  drivers/gpu/drm/i915/i915_drv.h     |  3 +++
> > > >  drivers/gpu/drm/i915/intel_psr.c    | 21 +++++++++++++++++++--
> > > >  3 files changed, 34 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > index 669f3d56054a..d28dc4d8388e 100644
> > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > @@ -2686,6 +2686,18 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
> > > >  	}
> > > >  	mutex_unlock(&dev_priv->psr.lock);
> > > >  
> > > > +	if (READ_ONCE(dev_priv->psr.debug)) {
> > > > +		unsigned int seq;
> > > > +
> > > > +		do {
> > > > +			seq = read_seqbegin(&dev_priv->psr.debug_lock);
> > > > +			seq_printf(m, "Last attempted entry at: %lld\n",
> > > > +				   dev_priv->psr.last_entry_attempt);
> > > > +			seq_printf(m, "Last exit at: %lld\n",
> > > > +				   dev_priv->psr.last_exit);
> > > > +		} while (read_seqretry(&dev_priv->psr.debug_lock, seq));
> > > 
> > > What does the seqlock buy us?
> > 
> > Reading debugfs wouldn't block the update inside the irq handler,
> > compared to using a spinlock. We need to serialize the read and update
> > parts, don't we? Otherwise tests might end up reading partial updates.
> 
> Hmm. ktime_t is 64 bits so I guess on 32bit systems it could be a slight
> issue. Not sure we should care that much about PSR debug on 32bit though.
> It's a rather unlikely configuration, and at least we don't do that in 
> the ci.
> 

Another thing is, if two IIR bits are set at the same time. EXIT and
PRE_ENTRY probably won't be though. How about if an earlier interrupt
was not handled? Won't multiple IIR bits be set? Implementing the lock
is slightly future proof and doesn't slow down irq handling.

> > 
> > > 
> > > > +	}
> > > > +
> > > >  	intel_runtime_pm_put(dev_priv);
> > > >  	return 0;
> > > >  }
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > > index 136fa2267a66..b8170882e1ab 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -609,6 +609,9 @@ struct i915_psr {
> > > >  	bool alpm;
> > > >  	bool has_hw_tracking;
> > > >  	bool debug;
> > > > +	ktime_t last_entry_attempt;
> > > > +	ktime_t last_exit;
> > > > +	seqlock_t debug_lock;
> > > >  
> > > >  	void (*enable_source)(struct intel_dp *,
> > > >  			      const struct intel_crtc_state *);
> > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > > > index 64ecea80438d..a83d95b1b587 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -125,28 +125,44 @@ void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir)
> > > >  {
> > > >  	u32 transcoders = BIT(TRANSCODER_EDP);
> > > >  	enum transcoder cpu_transcoder;
> > > > +	ktime_t time_ns =  ktime_get();
> > > > +	unsigned long flags = 0;
> > > >  
> > > >  	if (INTEL_GEN(dev_priv) >= 8)
> > > >  		transcoders |= BIT(TRANSCODER_A) |
> > > >  			       BIT(TRANSCODER_B) |
> > > >  			       BIT(TRANSCODER_C);
> > > >  
> > > > +	write_seqlock_irqsave(&dev_priv->psr.debug_lock, flags);
> > > >  	for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {
> > > > +		bool handled = false;
> > > > +
> > > > +		/* PSR supported only on one transcoder currently */
> > > > +		WARN_ON_ONCE(handled);
> > > > +
> > > >  		/* FIXME: Exit PSR when this happens. */
> > > > -		if (psr_iir & EDP_PSR_ERROR(cpu_transcoder))
> > > > +		if (psr_iir & EDP_PSR_ERROR(cpu_transcoder)) {
> > > > +			handled = true;
> > > >  			DRM_DEBUG_KMS("[transcoder %s] PSR aux error\n",
> > > >  				      transcoder_name(cpu_transcoder));
> > > >  
> > > > +		}
> > > > +
> > > >  		if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
> > > > -			DRM_DEBUG_KMS("[transcoder %s] PSR entry in 2 vblanks\n",
> > > > +			handled = true;
> > > > +			dev_priv->psr.last_entry_attempt = time_ns;
> > > > +			DRM_DEBUG_KMS("[transcoder %s] PSR entry attempt in 2 vblanks\n",
> > > >  				      transcoder_name(cpu_transcoder));
> > > >  		}
> > > >  
> > > >  		if (psr_iir & EDP_PSR_POST_EXIT(cpu_transcoder)) {
> > > > +			handled = true;
> > > > +			dev_priv->psr.last_exit = time_ns;
> > > >  			DRM_DEBUG_KMS("[transcoder %s] PSR exit completed\n",
> > > >  				      transcoder_name(cpu_transcoder));
> > > >  		}
> > > >  	}
> > > > +	write_sequnlock_irqrestore(&dev_priv->psr.debug_lock, flags);
> > > >  }
> > > >  
> > > >  static bool intel_dp_get_y_cord_status(struct intel_dp *intel_dp)
> > > > @@ -1160,6 +1176,7 @@ void intel_psr_init(struct drm_i915_private *dev_priv)
> > > >  
> > > >  	INIT_DELAYED_WORK(&dev_priv->psr.work, intel_psr_work);
> > > >  	mutex_init(&dev_priv->psr.lock);
> > > > +	seqlock_init(&dev_priv->psr.debug_lock);
> > > >  
> > > >  	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> > > >  		dev_priv->psr.enable_source = vlv_psr_enable_source;
> > > > -- 
> > > > 2.14.1
> > > 
>