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, 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




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

  Powered by Linux