Re: [PATCH v3 03/23] drm/i915/psr: Only handle interruptions of the transcoder in use

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux