| Message ID | 20190103142107.18304-4-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/6] drm/i915/psr: Allow PSR2 to be enabled when debugfs asks | expand |
On Thu, 2019-01-03 at 06:21 -0800, José Roberto de Souza wrote: > PSR2 only trigger interruptions for AUX error, so let's not print > useless information in debugfs. > Also adding a comment to intel_psr_irq_handler() about that. > Is it worth keeping this stuff for PSR1 if PSR2 is not supported, did not work well enough for PSR1 IGTs either. In any case, are these interrupts present on ICL? > v2: Warning and not letting user set PSR_DEBUG_IRQ when PSR2 is > enabled(Dhinakaran) > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 6 +++++- > drivers/gpu/drm/i915/intel_psr.c | 1 + > 2 files changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > b/drivers/gpu/drm/i915/i915_debugfs.c > index 77b097b50fd5..5ebf0e647ac7 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -2621,7 +2621,7 @@ static int i915_edp_psr_status(struct seq_file > *m, void *data) > seq_printf(m, "Performance counter: %u\n", val); > } > > - if (psr->debug & I915_PSR_DEBUG_IRQ) { > + if ((psr->debug & I915_PSR_DEBUG_IRQ) && !psr->psr2_enabled) { Is there a case where PSR2 and IRQ debug are enabled now that you disallow setting of this value? > seq_printf(m, "Last attempted entry at: %lld\n", > psr->last_entry_attempt); > seq_printf(m, "Last exit at: %lld\n", psr->last_exit); > @@ -2676,6 +2676,10 @@ i915_edp_psr_debug_set(void *data, u64 val) > skip_mode: > if (!ret) { > mutex_lock(&dev_priv->psr.lock); > + if (dev_priv->psr.psr2_enabled && (val & > I915_PSR_DEBUG_IRQ)) { > + val &= ~I915_PSR_DEBUG_IRQ; > + DRM_WARN("PSR debug IRQ cannot be enabled with > PSR2"); WARN is inconsistent with DEBUG level logging that this function already uses. > + } > dev_priv->psr.debug = val; > intel_psr_irq_control(dev_priv, dev_priv->psr.debug); We are accessing hardware outside of rpm_get()/rpm_put() here. > mutex_unlock(&dev_priv->psr.lock); > diff --git a/drivers/gpu/drm/i915/intel_psr.c > b/drivers/gpu/drm/i915/intel_psr.c > index bba4f7da68b3..a875546880fa 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -201,6 +201,7 @@ void intel_psr_irq_handler(struct > drm_i915_private *dev_priv, u32 psr_iir) > mask |= EDP_PSR_ERROR(shift); > } > > + /* PSR2 don't trigger PRE_ENTRY and POST_EXIT > interruptions */ > if (psr_iir & EDP_PSR_PRE_ENTRY(shift)) { > dev_priv->psr.last_entry_attempt = time_ns; > DRM_DEBUG_KMS("[transcoder %s] PSR entry > attempt in 2 vblanks\n",
On Wed, 2019-01-09 at 17:24 -0800, Dhinakaran Pandiyan wrote: > On Thu, 2019-01-03 at 06:21 -0800, José Roberto de Souza wrote: > > PSR2 only trigger interruptions for AUX error, so let's not print > > useless information in debugfs. > > Also adding a comment to intel_psr_irq_handler() about that. > > > Is it worth keeping this stuff for PSR1 if PSR2 is not supported, did > not work well enough for PSR1 IGTs either. In any case, are these > interrupts present on ICL? The PSR1 interrupts are present for ICL. I guess I only used this once to debug PSR1 do you have used it, it was useful? If not we should remove it as you suggested. But anyway as Maarten commented in the previous patch, he suggested us to follow the approach to test the real path when changing PSR state from debugfs after enable fastboot. So I will hold this patch and the previous one. > > > > v2: Warning and not letting user set PSR_DEBUG_IRQ when PSR2 is > > enabled(Dhinakaran) > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > drivers/gpu/drm/i915/i915_debugfs.c | 6 +++++- > > drivers/gpu/drm/i915/intel_psr.c | 1 + > > 2 files changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > > b/drivers/gpu/drm/i915/i915_debugfs.c > > index 77b097b50fd5..5ebf0e647ac7 100644 > > --- a/drivers/gpu/drm/i915/i915_debugfs.c > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > > @@ -2621,7 +2621,7 @@ static int i915_edp_psr_status(struct > > seq_file > > *m, void *data) > > seq_printf(m, "Performance counter: %u\n", val); > > } > > > > - if (psr->debug & I915_PSR_DEBUG_IRQ) { > > + if ((psr->debug & I915_PSR_DEBUG_IRQ) && !psr->psr2_enabled) { > Is there a case where PSR2 and IRQ debug are enabled now that you > disallow setting of this value? > > > > seq_printf(m, "Last attempted entry at: %lld\n", > > psr->last_entry_attempt); > > seq_printf(m, "Last exit at: %lld\n", psr->last_exit); > > @@ -2676,6 +2676,10 @@ i915_edp_psr_debug_set(void *data, u64 val) > > skip_mode: > > if (!ret) { > > mutex_lock(&dev_priv->psr.lock); > > + if (dev_priv->psr.psr2_enabled && (val & > > I915_PSR_DEBUG_IRQ)) { > > + val &= ~I915_PSR_DEBUG_IRQ; > > + DRM_WARN("PSR debug IRQ cannot be enabled with > > PSR2"); > > WARN is inconsistent with DEBUG level logging that this function > already uses. I guess in this case a warn is necessary otherwise if user din't had increase the drm debug level and tried to enable PSR IRQ interruptions while PSR2 is actived, this user would not get any error or warning. > > > + } > > dev_priv->psr.debug = val; > > intel_psr_irq_control(dev_priv, dev_priv->psr.debug); > We are accessing hardware outside of rpm_get()/rpm_put() here. nice catch. Thanks > > > > mutex_unlock(&dev_priv->psr.lock); > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > b/drivers/gpu/drm/i915/intel_psr.c > > index bba4f7da68b3..a875546880fa 100644 > > --- a/drivers/gpu/drm/i915/intel_psr.c > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > @@ -201,6 +201,7 @@ void intel_psr_irq_handler(struct > > drm_i915_private *dev_priv, u32 psr_iir) > > mask |= EDP_PSR_ERROR(shift); > > } > > > > + /* PSR2 don't trigger PRE_ENTRY and POST_EXIT > > interruptions */ > > if (psr_iir & EDP_PSR_PRE_ENTRY(shift)) { > > dev_priv->psr.last_entry_attempt = time_ns; > > DRM_DEBUG_KMS("[transcoder %s] PSR entry > > attempt in 2 vblanks\n",
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 77b097b50fd5..5ebf0e647ac7 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -2621,7 +2621,7 @@ static int i915_edp_psr_status(struct seq_file *m, void *data) seq_printf(m, "Performance counter: %u\n", val); } - if (psr->debug & I915_PSR_DEBUG_IRQ) { + if ((psr->debug & I915_PSR_DEBUG_IRQ) && !psr->psr2_enabled) { seq_printf(m, "Last attempted entry at: %lld\n", psr->last_entry_attempt); seq_printf(m, "Last exit at: %lld\n", psr->last_exit); @@ -2676,6 +2676,10 @@ i915_edp_psr_debug_set(void *data, u64 val) skip_mode: if (!ret) { mutex_lock(&dev_priv->psr.lock); + if (dev_priv->psr.psr2_enabled && (val & I915_PSR_DEBUG_IRQ)) { + val &= ~I915_PSR_DEBUG_IRQ; + DRM_WARN("PSR debug IRQ cannot be enabled with PSR2"); + } dev_priv->psr.debug = val; intel_psr_irq_control(dev_priv, dev_priv->psr.debug); mutex_unlock(&dev_priv->psr.lock); diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index bba4f7da68b3..a875546880fa 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -201,6 +201,7 @@ void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir) mask |= EDP_PSR_ERROR(shift); } + /* PSR2 don't trigger PRE_ENTRY and POST_EXIT interruptions */ if (psr_iir & EDP_PSR_PRE_ENTRY(shift)) { dev_priv->psr.last_entry_attempt = time_ns; DRM_DEBUG_KMS("[transcoder %s] PSR entry attempt in 2 vblanks\n",
PSR2 only trigger interruptions for AUX error, so let's not print useless information in debugfs. Also adding a comment to intel_psr_irq_handler() about that. v2: Warning and not letting user set PSR_DEBUG_IRQ when PSR2 is enabled(Dhinakaran) Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- drivers/gpu/drm/i915/i915_debugfs.c | 6 +++++- drivers/gpu/drm/i915/intel_psr.c | 1 + 2 files changed, 6 insertions(+), 1 deletion(-)