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@xxxxxxxxx>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx>
> > Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx>
> > ---
> > 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",
Attachment:
signature.asc
Description: This is a digitally signed message part
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
