On Tue, Sep 03, 2019 at 02:53:04PM -0700, Souza, Jose wrote:
> 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?
Yeah, I think a comment like that is probably good enough for now.
Aside from that you can consider the patch
Reviewed-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
>
> >
> >
> > 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
> > >
--
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
