diff mbox

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

Message ID 20180320224151.6009-6-dhinakaran.pandiyan@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Dhinakaran Pandiyan March 20, 2018, 10:41 p.m. UTC
Timestamps are useful for IGT tests that trigger PSR exit and/or wait for
PSR entry.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@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(-)

Comments

Ville Syrjala March 21, 2018, 7:48 p.m. UTC | #1
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@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@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?

> +	}
> +
>  	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
Dhinakaran Pandiyan March 22, 2018, 1:05 a.m. UTC | #2
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@linux.intel.com>

> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>

> > Cc: Daniel Vetter <daniel.vetter@intel.com>

> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@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.

> 

> > +	}

> > +

> >  	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

>
Ville Syrjala March 22, 2018, 9:21 a.m. UTC | #3
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@linux.intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@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.

> 
> > 
> > > +	}
> > > +
> > >  	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
> >
Dhinakaran Pandiyan March 22, 2018, 8:59 p.m. UTC | #4
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@linux.intel.com>

> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>

> > > > Cc: Daniel Vetter <daniel.vetter@intel.com>

> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@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

> > > 

>
Chris Wilson March 22, 2018, 9:08 p.m. UTC | #5
Quoting Dhinakaran Pandiyan (2018-03-20 22:41:51)
> 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);

You are inside a hardirq handler. irqsave/irqrestore are not required.

Even a seqlock here looks overkill, but whatever. (I don't buy that you
need that precise level of coordination for a slow debugfs interface.)

>         for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {
> +               bool handled = false;
> +
> +               /* PSR supported only on one transcoder currently */
> +               WARN_ON_ONCE(handled);

Now this is just silly. At least get the check right.
-Chris
Dhinakaran Pandiyan March 22, 2018, 11:55 p.m. UTC | #6
On Thu, 2018-03-22 at 21:08 +0000, Chris Wilson wrote:
> Quoting Dhinakaran Pandiyan (2018-03-20 22:41:51)

> > 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);

> 

> You are inside a hardirq handler. irqsave/irqrestore are not required.

> 

> Even a seqlock here looks overkill, but whatever. (I don't buy that you

> need that precise level of coordination for a slow debugfs interface.)

> 


Looks like I'll make two reviewers happy without the seqlock, will
remove it in the next version :)

> >         for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {

> > +               bool handled = false;

> > +

> > +               /* PSR supported only on one transcoder currently */

> > +               WARN_ON_ONCE(handled);

> 

> Now this is just silly. At least get the check right.


Argh, I should have caught it. Thanks.


> -Chris

> _______________________________________________

> Intel-gfx mailing list

> Intel-gfx@lists.freedesktop.org

> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox

Patch

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));
+	}
+
 	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;