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

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

 



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




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

  Powered by Linux