| Message ID | 20180320224151.6009-6-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 >
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 > >
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 > > > >
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
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 --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;
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(-)