[Intel-gfx] [PATCH v3 03/23] drm/i915/psr: Only handle interruptions of the transcoder in use
Lucas De Marchi
lucas.demarchi at intel.com
Wed Aug 28 22:16:06 UTC 2019
On Wed, Aug 28, 2019 at 07:29:38PM +0300, Imre Deak wrote:
>On Tue, Aug 27, 2019 at 09:50:24AM -0700, Lucas De Marchi wrote:
>> On Mon, Aug 26, 2019 at 08:28:33PM +0300, Imre Deak wrote:
>> > On Fri, Aug 23, 2019 at 01:20:35AM -0700, Lucas De Marchi wrote:
>> > > From: José Roberto de Souza <jose.souza at intel.com>
>> > >
>> > > It was enabling and checking PSR interruptions in every transcoder
>> > > while it should keep the interruptions on the non-used transcoders
>> > > masked.
>> > >
>> > > This also already prepares for future when more than one PSR instance
>> > > will be allowed.
>> > >
>> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
>> > > Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
>> > > Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>> > > ---
>> > > drivers/gpu/drm/i915/display/intel_psr.c | 140 +++++++++--------------
>> > > drivers/gpu/drm/i915/i915_reg.h | 13 +--
>> > > 2 files changed, 59 insertions(+), 94 deletions(-)
>> > >
>> > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
>> > > index 28b62e587204..81e3619cd905 100644
>> > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
>> > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
>> > > @@ -88,48 +88,23 @@ 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;
>> > > - }
>> > > -}
>> > > + enum transcoder trans = dev_priv->psr.transcoder;
>> >
>> > This is called from intel_psr_debug_set() where psr.transcoder may be
>> > uninited.
>> >
>> > > + u32 val, mask;
>> > >
>> > > -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);
>> > > - }
>> > > + 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);
>> > >
>> > > - if (debug & I915_PSR_DEBUG_IRQ)
>> > > - mask |= debug_mask;
>> > > -
>> > > - I915_WRITE(EDP_PSR_IMR, ~mask);
>> > > + /*
>> > > + * TODO: when handling multiple PSR instances a global spinlock will be
>> > > + * needed to synchronize the value of shared register
>> > > + */
>> > > + val = I915_READ(EDP_PSR_IMR);
>> > > + val &= ~EDP_PSR_TRANS_MASK(trans);
>> > > + val |= ~mask;
>> > > + I915_WRITE(EDP_PSR_IMR, val);
>> > > }
>> > >
>> > > static void psr_event_print(u32 val, bool psr2_enabled)
>> > > @@ -171,63 +146,54 @@ 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);
>> > > + if (psr_iir & EDP_PSR_ERROR(cpu_transcoder)) {
>> > > + u32 val;
>> > >
>> > > - for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {
>> >
>> > I think we should still catch all interrupts, log the unexpected ones
>> > and react only on the expected one in intel_psr_work().
>>
>> could you expand more on this? there is only one PSR instance hence only
>> one possible transcoder coming from dev_priv->psr.transcoder. Looping here just to
>> warn seems wasteful.
>
>I think we should do what the HW tells us and make sure we clear all the
>interrupts that may have happened rather than assume that the interrupt
>that happened was the one corresponding to psr.transcoder.
if psr can only be enabled in a single transcoder, how the transcoder
could be any different for that interrupt?
>psr.transcoder is also only protected by a mutex, so we can't use its
>value in the interrupt handler.
psr.transcoder is only set before enabling the source hence should
remain the same for any interrupt received.
>
>It's weird that there is no per-PSR instance IIRs in the misc interrupt
>register. Because of that we'd need a PSR software IRQ mask that could
>be set from psr_irq_control(). We also have to make sure to clear/mask a
>transcoder's PSR interupts and sync against the interrupt handler when
>turning off the transcoder power well. It looks like the transcoder
>power well is the same as that of the transcoder's pipe power well, so
>we could do this in gen8_irq_power_well_pre_disable().
>
>By doing the above (not use psr.transcoder in the interrupt handler,
>rather use a separate psr_irq software mask) we could also keep the
>interrupt handling independent of the modeset code (which is what sets
>psr.transcoder).
I have another version that should fix the problem with psr.transcoder. After
that I don't see the issue with the current approach.
Lucas De Marchi
>
>>
>> >
>> > > - int shift = edp_psr_shift(cpu_transcoder);
>> > > + DRM_WARN("[transcoder %s] PSR aux error\n",
>> > > + transcoder_name(cpu_transcoder));
>> > >
>> > > - if (psr_iir & EDP_PSR_ERROR(shift)) {
>> > > - DRM_WARN("[transcoder %s] PSR aux error\n",
>> > > - transcoder_name(cpu_transcoder));
>> > > + dev_priv->psr.irq_aux_error = true;
>> > >
>> > > - dev_priv->psr.irq_aux_error = true;
>> > > + /*
>> > > + * 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.
>> > > + *
>> > > + * TODO: when handling multiple PSR instances a global spinlock
>> > > + * will be needed to synchronize the value of shared register
>>
>> I'm not really a fan of these TODO for multiple PSR instances. When/if
>> we add them, we won't really be able to rely on these TODO comments and
>> will rather need to evaluate the whole scenario.
>>
>> > > + */
>> > > + val = I915_READ(EDP_PSR_IMR);
>> > > + val |= EDP_PSR_ERROR(cpu_transcoder);
>> > > + I915_WRITE(EDP_PSR_IMR, val);
>> > >
>> > > - /*
>> > > - * 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);
>> > > - }
>> > > + schedule_work(&dev_priv->psr.work);
>> >
>> > Would be better not to reorder intel_psr_work() and printing the events
>> > below.
>> >
>> > > + }
>> > >
>> > > - 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));
>> > > - }
>> > > + 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_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_POST_EXIT(cpu_transcoder)) {
>> > > + dev_priv->psr.last_exit = time_ns;
>> > > + DRM_DEBUG_KMS("[transcoder %s] PSR exit completed\n",
>> > > + transcoder_name(cpu_transcoder));
>> > >
>> > > - if (INTEL_GEN(dev_priv) >= 9) {
>> > > - u32 val = I915_READ(PSR_EVENT(cpu_transcoder));
>> > > - bool psr2_enabled = dev_priv->psr.psr2_enabled;
>> > > + if (INTEL_GEN(dev_priv) >= 9) {
>> > > + u32 val = I915_READ(PSR_EVENT(cpu_transcoder));
>> > > + bool psr2_enabled = dev_priv->psr.psr2_enabled;
>> > >
>> > > - I915_WRITE(PSR_EVENT(cpu_transcoder), val);
>> > > - psr_event_print(val, psr2_enabled);
>> > > - }
>> > > + I915_WRITE(PSR_EVENT(cpu_transcoder), val);
>> > > + psr_event_print(val, psr2_enabled);
>> > > }
>> > > }
>> > > -
>> > > - if (mask) {
>> > > - mask |= I915_READ(EDP_PSR_IMR);
>> > > - I915_WRITE(EDP_PSR_IMR, mask);
>> > > -
>> > > - schedule_work(&dev_priv->psr.work);
>> > > - }
>> > > }
>> > >
>> > > static bool intel_dp_get_alpm_status(struct intel_dp *intel_dp)
>> > > @@ -737,7 +703,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,
>> > > @@ -762,7 +728,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");
>> > > @@ -1110,7 +1076,7 @@ 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);
>> > > + 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..1c6d99944630 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) + 1) * 8)
>> >
>> > (trans - TRANSCODER_A) + 1
>> >
>> > to not depend on the enum value of TRANSCODER_A.
>>
>> agreed
>>
>> thanks
>> Lucas De Marchi
>>
>> >
>> > > +#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 at lists.freedesktop.org
>> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list