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

From: Imre Deak <imre.deak@intel.com>
To: Lucas De Marchi <lucas.demarchi@intel.com>,
	Jose Souza <jose.souza@intel.com>
Cc: intel-gfx@lists.freedesktop.org,
	Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Subject: Re: [PATCH v3 03/23] drm/i915/psr: Only handle interruptions of the transcoder in use
Date: Mon, 26 Aug 2019 20:28:33 +0300	[thread overview]
Message-ID: <20190826172833.GC27192@ideak-desk.fi.intel.com> (raw)
In-Reply-To: <20190823082055.5992-4-lucas.demarchi@intel.com>

On Fri, Aug 23, 2019 at 01:20:35AM -0700, Lucas De Marchi wrote:
> From: José Roberto de Souza <jose.souza@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@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@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().

> -		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
> +		 */
> +		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.

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