On Tue, 2019-09-03 at 14:42 -0700, Matt Roper wrote:
> On Thu, Aug 29, 2019 at 02:25:48AM -0700, Lucas De Marchi wrote:
> > From: José Roberto de Souza <jose.souza@xxxxxxxxx>
> >
> > It was enabling and checking PSR interruptions in every transcoder
> > while it should keep the interruptions on the non-used transcoders
> > masked.
> >
> > While doing this it gives us trouble on Tiger Lake if we are
> > reading/writing to registers of disabled transcoders since from
> > gen12
> > onwards the registers are relative to the transcoder. Instead of
> > forcing
> > them ON to access those registers, just avoid the accesses as they
> > are
> > not needed.
> >
> > v2 (Lucas):
> > - Explain why we can't keep accessing all transcoders
> > - Remove TODO about extending the irq handling to multiple
> > instances:
> > when/if implementing multiple instances it's pretty clear by
> > the
> > singleton psr that it needs to be extended
> > - Fix intel_psr_debug_set() calling psr_irq_control() with
> > psr.transcoder not set yet (from Imre). Now we only set the
> > debug
> > register right away if psr is already enabled. Otherwise we
> > just
> > record the value to be set when enabling the source.
> > - Do not depend on the value of TRANSCODER_A. Just be relative to
> > it
> > (from Imre)
> > - handle psr error last so we don't schedule the work before
> > handling
> > the other flags
> >
> > Cc: Imre Deak <imre.deak@xxxxxxxxx>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx>
> > Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx>
> > Signed-off-by: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/display/intel_psr.c | 137 +++++++++--------
> > ------
> > drivers/gpu/drm/i915/i915_reg.h | 13 +--
> > 2 files changed, 57 insertions(+), 93 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > b/drivers/gpu/drm/i915/display/intel_psr.c
> > index 629b8b98a97f..6f2bf50b6d80 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -88,48 +88,19 @@ static bool intel_psr2_enabled(struct
> > drm_i915_private *dev_priv,
> > }
> > }
> >
> > -static int edp_psr_shift(enum transcoder cpu_transcoder)
> > +static void psr_irq_control(struct drm_i915_private *dev_priv)
> > {
> > - switch (cpu_transcoder) {
> > - case TRANSCODER_A:
> > - return EDP_PSR_TRANSCODER_A_SHIFT;
> > - case TRANSCODER_B:
> > - return EDP_PSR_TRANSCODER_B_SHIFT;
> > - case TRANSCODER_C:
> > - return EDP_PSR_TRANSCODER_C_SHIFT;
> > - default:
> > - MISSING_CASE(cpu_transcoder);
> > - /* fallthrough */
> > - case TRANSCODER_EDP:
> > - return EDP_PSR_TRANSCODER_EDP_SHIFT;
> > - }
> > -}
> > -
> > -static void psr_irq_control(struct drm_i915_private *dev_priv, u32
> > debug)
> > -{
> > - u32 debug_mask, mask;
> > - enum transcoder cpu_transcoder;
> > - u32 transcoders = BIT(TRANSCODER_EDP);
> > -
> > - if (INTEL_GEN(dev_priv) >= 8)
> > - transcoders |= BIT(TRANSCODER_A) |
> > - BIT(TRANSCODER_B) |
> > - BIT(TRANSCODER_C);
> > -
> > - debug_mask = 0;
> > - mask = 0;
> > - for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder,
> > transcoders) {
> > - int shift = edp_psr_shift(cpu_transcoder);
> > -
> > - mask |= EDP_PSR_ERROR(shift);
> > - debug_mask |= EDP_PSR_POST_EXIT(shift) |
> > - EDP_PSR_PRE_ENTRY(shift);
> > - }
> > + enum transcoder trans = dev_priv->psr.transcoder;
> > + u32 val, mask;
> >
> > - if (debug & I915_PSR_DEBUG_IRQ)
> > - mask |= debug_mask;
> > + mask = EDP_PSR_ERROR(trans);
> > + if (dev_priv->psr.debug & I915_PSR_DEBUG_IRQ)
> > + mask |= EDP_PSR_POST_EXIT(trans) |
> > EDP_PSR_PRE_ENTRY(trans);
> >
> > - I915_WRITE(EDP_PSR_IMR, ~mask);
> > + val = I915_READ(EDP_PSR_IMR);
> > + val &= ~EDP_PSR_TRANS_MASK(trans);
> > + val |= ~mask;
> > + I915_WRITE(EDP_PSR_IMR, val);
>
> I guess we've done this all along, but it jumped out at me during the
> review here that we're setting a bunch of bits to 1 that I don't
> think
> have a defined meaning. I.e., the bspec explicitly indicates that
> 0x07070707 would be "all interrupts masked" whereas we're setting
> something more along the lines of 0xFFFFBFF (for an example with PSR
> on
> transcoder A).
>
> That's apparently fine for current platforms (since it's what we've
> been
> doing all along), but it makes me slightly more nervous on TGL and
> beyond given that the next patch switches to the per-transcoder
> PSR_IMR
> registers and those explicitly say that bits 31:4 are reserved and
> must-be-zero. Maybe we should add a code comment about this just in
> case it comes back to bite us on a future platform?
Like you said we do it for all other platforms and for all other
interruption registers but anyways I can add a comment on top:
/* Masking/setting reserved bits too */
It is enough? do you have any other suggestion?
>
>
> Matt
>
> > }
> >
> > static void psr_event_print(u32 val, bool psr2_enabled)
> > @@ -171,60 +142,48 @@ static void psr_event_print(u32 val, bool
> > psr2_enabled)
> >
> > void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32
> > psr_iir)
> > {
> > - u32 transcoders = BIT(TRANSCODER_EDP);
> > - enum transcoder cpu_transcoder;
> > + enum transcoder cpu_transcoder = dev_priv->psr.transcoder;
> > ktime_t time_ns = ktime_get();
> > - u32 mask = 0;
> > -
> > - if (INTEL_GEN(dev_priv) >= 8)
> > - transcoders |= BIT(TRANSCODER_A) |
> > - BIT(TRANSCODER_B) |
> > - BIT(TRANSCODER_C);
> >
> > - for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder,
> > transcoders) {
> > - int shift = edp_psr_shift(cpu_transcoder);
> > + if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
> > + 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_ERROR(shift)) {
> > - DRM_WARN("[transcoder %s] PSR aux error\n",
> > - transcoder_name(cpu_transcoder));
> > + if (psr_iir & EDP_PSR_POST_EXIT(cpu_transcoder)) {
> > + dev_priv->psr.last_exit = time_ns;
> > + DRM_DEBUG_KMS("[transcoder %s] PSR exit completed\n",
> > + transcoder_name(cpu_transcoder));
> >
> > - dev_priv->psr.irq_aux_error = true;
> > + if (INTEL_GEN(dev_priv) >= 9) {
> > + u32 val = I915_READ(PSR_EVENT(cpu_transcoder));
> > + bool psr2_enabled = dev_priv->psr.psr2_enabled;
> >
> > - /*
> > - * If this interruption is not masked it will
> > keep
> > - * interrupting so fast that it prevents the
> > scheduled
> > - * work to run.
> > - * Also after a PSR error, we don't want to arm
> > PSR
> > - * again so we don't care about unmask the
> > interruption
> > - * or unset irq_aux_error.
> > - */
> > - mask |= EDP_PSR_ERROR(shift);
> > - }
> > -
> > - 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",
> > - transcoder_name(cpu_transcoder));
> > + I915_WRITE(PSR_EVENT(cpu_transcoder), val);
> > + psr_event_print(val, psr2_enabled);
> > }
> > + }
> >
> > - if (psr_iir & EDP_PSR_POST_EXIT(shift)) {
> > - dev_priv->psr.last_exit = time_ns;
> > - DRM_DEBUG_KMS("[transcoder %s] PSR exit
> > completed\n",
> > - transcoder_name(cpu_transcoder));
> > + if (psr_iir & EDP_PSR_ERROR(cpu_transcoder)) {
> > + u32 val;
> >
> > - if (INTEL_GEN(dev_priv) >= 9) {
> > - u32 val =
> > I915_READ(PSR_EVENT(cpu_transcoder));
> > - bool psr2_enabled = dev_priv-
> > >psr.psr2_enabled;
> > + DRM_WARN("[transcoder %s] PSR aux error\n",
> > + transcoder_name(cpu_transcoder));
> >
> > - I915_WRITE(PSR_EVENT(cpu_transcoder),
> > val);
> > - psr_event_print(val, psr2_enabled);
> > - }
> > - }
> > - }
> > + dev_priv->psr.irq_aux_error = true;
> >
> > - if (mask) {
> > - mask |= I915_READ(EDP_PSR_IMR);
> > - I915_WRITE(EDP_PSR_IMR, mask);
> > + /*
> > + * If this interruption is not masked it will keep
> > + * interrupting so fast that it prevents the scheduled
> > + * work to run.
> > + * Also after a PSR error, we don't want to arm PSR
> > + * again so we don't care about unmask the interruption
> > + * or unset irq_aux_error.
> > + */
> > + val = I915_READ(EDP_PSR_IMR);
> > + val |= EDP_PSR_ERROR(cpu_transcoder);
> > + I915_WRITE(EDP_PSR_IMR, val);
> >
> > schedule_work(&dev_priv->psr.work);
> > }
> > @@ -747,7 +706,7 @@ static void intel_psr_enable_source(struct
> > intel_dp *intel_dp,
> >
> > I915_WRITE(EDP_PSR_DEBUG(dev_priv->psr.transcoder), mask);
> >
> > - psr_irq_control(dev_priv, dev_priv->psr.debug);
> > + psr_irq_control(dev_priv);
> > }
> >
> > static void intel_psr_enable_locked(struct drm_i915_private
> > *dev_priv,
> > @@ -772,7 +731,7 @@ static void intel_psr_enable_locked(struct
> > drm_i915_private *dev_priv,
> > * to avoid any rendering problems.
> > */
> > val = I915_READ(EDP_PSR_IIR);
> > - val &= EDP_PSR_ERROR(edp_psr_shift(dev_priv->psr.transcoder));
> > + val &= EDP_PSR_ERROR(dev_priv->psr.transcoder);
> > if (val) {
> > dev_priv->psr.sink_not_reliable = true;
> > DRM_DEBUG_KMS("PSR interruption error set, not enabling
> > PSR\n");
> > @@ -1120,7 +1079,13 @@ int intel_psr_debug_set(struct
> > drm_i915_private *dev_priv, u64 val)
> >
> > old_mode = dev_priv->psr.debug & I915_PSR_DEBUG_MODE_MASK;
> > dev_priv->psr.debug = val;
> > - psr_irq_control(dev_priv, dev_priv->psr.debug);
> > +
> > + /*
> > + * Do it right away if it's already enabled, otherwise it will
> > be done
> > + * when enabling the source.
> > + */
> > + if (dev_priv->psr.enabled)
> > + psr_irq_control(dev_priv);
> >
> > mutex_unlock(&dev_priv->psr.lock);
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 02e1ef10c47e..6e7db9c65981 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -4225,13 +4225,12 @@ enum {
> > /* Bspec claims those aren't shifted but stay at 0x64800 */
> > #define EDP_PSR_IMR _MMIO(0x64834)
> > #define EDP_PSR_IIR _MMIO(0x64838)
> > -#define EDP_PSR_ERROR(shift) (1 << ((shift)
> > + 2))
> > -#define EDP_PSR_POST_EXIT(shift) (1 << ((shift) + 1))
> > -#define EDP_PSR_PRE_ENTRY(shift) (1 << (shift))
> > -#define EDP_PSR_TRANSCODER_C_SHIFT 24
> > -#define EDP_PSR_TRANSCODER_B_SHIFT 16
> > -#define EDP_PSR_TRANSCODER_A_SHIFT 8
> > -#define EDP_PSR_TRANSCODER_EDP_SHIFT 0
> > +#define _EDP_PSR_TRANS_SHIFT(trans) ((trans) ==
> > TRANSCODER_EDP ? \
> > + 0 : ((trans) -
> > TRANSCODER_A + 1) * 8)
> > +#define EDP_PSR_TRANS_MASK(trans) (0x7 <<
> > _EDP_PSR_TRANS_SHIFT(trans))
> > +#define EDP_PSR_ERROR(trans) (0x4 <<
> > _EDP_PSR_TRANS_SHIFT(trans))
> > +#define EDP_PSR_POST_EXIT(trans) (0x2 <<
> > _EDP_PSR_TRANS_SHIFT(trans))
> > +#define EDP_PSR_PRE_ENTRY(trans) (0x1 <<
> > _EDP_PSR_TRANS_SHIFT(trans))
> >
> > #define _SRD_AUX_CTL_A 0x60810
> > #define _SRD_AUX_CTL_EDP 0x6f810
> > --
> > 2.23.0
> >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
