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@xxxxxxxxx> > > > > 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@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 | 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@xxxxxxxxxxxxxxxxxxxxx > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
