Re: [PATCH v3] drm/i915/psr: Fix PSR_IMR/IIR field handling

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

 



On Tue, 2022-09-27 at 14:09 +0300, Jouni Högander wrote:
> Current PSR code is supposed to use TRANSCODER_EDP to force 0 shift for
> bits in PSR_IMR/IIR registers:
> 
> /*
>  * gen12+ has registers relative to transcoder and one per transcoder
>  * using the same bit definition: handle it as TRANSCODER_EDP to force
>  * 0 shift in bit definition
>  */
> 
> At the time of writing the code assumption "TRANSCODER_EDP == 0" was made.
> This is not the case and all fields in PSR_IMR and PSR_IIR are shifted
> incorrectly if DISPLAY_VER >= 12.
> 
> Fix this by adding separate register field defines for >=12 and add bit
> getter functions to keep code readability.
> 
> v3:
>  - Add separate register field defines (José)
>  - Add bit getter functions (José)
> v2:
>  - Improve commit message (José)
> 
> Signed-off-by: Jouni Högander <jouni.hogander@xxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/display/intel_psr.c | 84 ++++++++++++++----------
>  drivers/gpu/drm/i915/i915_reg.h          | 16 +++--
>  2 files changed, 62 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> index 9def8d9fade6..d7b08a7da9e9 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -116,34 +116,56 @@ static bool psr2_global_enabled(struct intel_dp *intel_dp)
>  	}
>  }
>  
> +static u32 psr_irq_psr_error_bit_get(struct intel_dp *intel_dp)
> +{
> +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> +
> +	return DISPLAY_VER(dev_priv) >= 12 ? TGL_EDP_PSR_ERROR :
> +		EDP_PSR_ERROR(intel_dp->psr.transcoder);

Drop the "_EDP", just go with TGL_PSR_ERROR... there is no reference to EDP or any transcoder in TGL+ it is one register per transcoder.

> +}
> +
> +static u32 psr_irq_post_exit_bit_get(struct intel_dp *intel_dp)
> +{
> +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> +
> +	return DISPLAY_VER(dev_priv) >= 12 ? TGL_EDP_PSR_POST_EXIT :
> +		EDP_PSR_POST_EXIT(intel_dp->psr.transcoder);
> +}
> +
> +static u32 psr_irq_pre_entry_bit_get(struct intel_dp *intel_dp)
> +{
> +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> +
> +	return DISPLAY_VER(dev_priv) >= 12 ? TGL_EDP_PSR_PRE_ENTRY :
> +		EDP_PSR_PRE_ENTRY(intel_dp->psr.transcoder);
> +}
> +
> +static u32 psr_irq_mask_get(struct intel_dp *intel_dp)
> +{
> +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> +
> +	return DISPLAY_VER(dev_priv) >= 12 ? TGL_EDP_PSR_MASK :
> +		EDP_PSR_MASK(intel_dp->psr.transcoder);
> +}
> +
>  static void psr_irq_control(struct intel_dp *intel_dp)
>  {
>  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> -	enum transcoder trans_shift;
>  	i915_reg_t imr_reg;
>  	u32 mask, val;
>  
> -	/*
> -	 * gen12+ has registers relative to transcoder and one per transcoder
> -	 * using the same bit definition: handle it as TRANSCODER_EDP to force
> -	 * 0 shift in bit definition
> -	 */
> -	if (DISPLAY_VER(dev_priv) >= 12) {
> -		trans_shift = 0;
> +	if (DISPLAY_VER(dev_priv) >= 12)
>  		imr_reg = TRANS_PSR_IMR(intel_dp->psr.transcoder);
> -	} else {
> -		trans_shift = intel_dp->psr.transcoder;
> +	else
>  		imr_reg = EDP_PSR_IMR;
> -	}
>  
> -	mask = EDP_PSR_ERROR(trans_shift);
> +	mask = psr_irq_psr_error_bit_get(intel_dp);
>  	if (intel_dp->psr.debug & I915_PSR_DEBUG_IRQ)
> -		mask |= EDP_PSR_POST_EXIT(trans_shift) |
> -			EDP_PSR_PRE_ENTRY(trans_shift);
> +		mask |= psr_irq_post_exit_bit_get(intel_dp) |
> +			psr_irq_pre_entry_bit_get(intel_dp);
>  
> -	/* Warning: it is masking/setting reserved bits too */
>  	val = intel_de_read(dev_priv, imr_reg);
> -	val &= ~EDP_PSR_TRANS_MASK(trans_shift);
> +	val &= ~psr_irq_mask_get(intel_dp);
>  	val |= ~mask;
>  	intel_de_write(dev_priv, imr_reg, val);
>  }
> @@ -191,25 +213,21 @@ void intel_psr_irq_handler(struct intel_dp *intel_dp, u32 psr_iir)
>  	enum transcoder cpu_transcoder = intel_dp->psr.transcoder;
>  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
>  	ktime_t time_ns =  ktime_get();
> -	enum transcoder trans_shift;
>  	i915_reg_t imr_reg;
>  
> -	if (DISPLAY_VER(dev_priv) >= 12) {
> -		trans_shift = 0;
> +	if (DISPLAY_VER(dev_priv) >= 12)
>  		imr_reg = TRANS_PSR_IMR(intel_dp->psr.transcoder);
> -	} else {
> -		trans_shift = intel_dp->psr.transcoder;
> +	else
>  		imr_reg = EDP_PSR_IMR;
> -	}
>  
> -	if (psr_iir & EDP_PSR_PRE_ENTRY(trans_shift)) {
> +	if (psr_iir & psr_irq_pre_entry_bit_get(intel_dp)) {
>  		intel_dp->psr.last_entry_attempt = time_ns;
>  		drm_dbg_kms(&dev_priv->drm,
>  			    "[transcoder %s] PSR entry attempt in 2 vblanks\n",
>  			    transcoder_name(cpu_transcoder));
>  	}
>  
> -	if (psr_iir & EDP_PSR_POST_EXIT(trans_shift)) {
> +	if (psr_iir & psr_irq_post_exit_bit_get(intel_dp)) {
>  		intel_dp->psr.last_exit = time_ns;
>  		drm_dbg_kms(&dev_priv->drm,
>  			    "[transcoder %s] PSR exit completed\n",
> @@ -226,7 +244,7 @@ void intel_psr_irq_handler(struct intel_dp *intel_dp, u32 psr_iir)
>  		}
>  	}
>  
> -	if (psr_iir & EDP_PSR_ERROR(trans_shift)) {
> +	if (psr_iir & psr_irq_psr_error_bit_get(intel_dp)) {
>  		u32 val;
>  
>  		drm_warn(&dev_priv->drm, "[transcoder %s] PSR aux error\n",
> @@ -243,7 +261,7 @@ void intel_psr_irq_handler(struct intel_dp *intel_dp, u32 psr_iir)
>  		 * or unset irq_aux_error.
>  		 */
>  		val = intel_de_read(dev_priv, imr_reg);
> -		val |= EDP_PSR_ERROR(trans_shift);
> +		val |= psr_irq_psr_error_bit_get(intel_dp);
>  		intel_de_write(dev_priv, imr_reg, val);
>  
>  		schedule_work(&intel_dp->psr.work);
> @@ -1194,14 +1212,12 @@ static bool psr_interrupt_error_check(struct intel_dp *intel_dp)
>  	 * first time that PSR HW tries to activate so lets keep PSR disabled
>  	 * to avoid any rendering problems.
>  	 */
> -	if (DISPLAY_VER(dev_priv) >= 12) {
> +	if (DISPLAY_VER(dev_priv) >= 12)
>  		val = intel_de_read(dev_priv,
>  				    TRANS_PSR_IIR(intel_dp->psr.transcoder));
> -		val &= EDP_PSR_ERROR(0);
> -	} else {
> +	else
>  		val = intel_de_read(dev_priv, EDP_PSR_IIR);
> -		val &= EDP_PSR_ERROR(intel_dp->psr.transcoder);
> -	}
> +	val &= psr_irq_psr_error_bit_get(intel_dp);
>  	if (val) {
>  		intel_dp->psr.sink_not_reliable = true;
>  		drm_dbg_kms(&dev_priv->drm,
> @@ -2158,9 +2174,9 @@ static void intel_psr_work(struct work_struct *work)
>  
>  	/*
>  	 * We have to make sure PSR is ready for re-enable
> -	 * otherwise it keeps disabled until next full enable/disable cycle.
> -	 * PSR might take some time to get fully disabled
> -	 * and be ready for re-enable.
> +	 * otherwise it keeps disabled until next full enable/disable
> +	 * cycle. PSR might take some time to get fully disabled and
> +	 * be ready for re-enable.

Non-related change.

>  	 */
>  	if (!__psr_wait_for_idle_locked(intel_dp))
>  		goto unlock;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 5003a5ffbc6a..3c103aeaa2e4 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2168,10 +2168,18 @@
>  #define TRANS_PSR_IIR(tran)			_MMIO_TRANS2(tran, _PSR_IIR_A)
>  #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   TGL_EDP_PSR_MASK			(0x7)
> +#define   TGL_EDP_PSR_ERROR			(1 << 2)
> +#define   TGL_EDP_PSR_POST_EXIT			(1 << 1)
> +#define   TGL_EDP_PSR_PRE_ENTRY			(1 << 0)

For new stuff REG_BIT() should be used.

> +#define   EDP_PSR_MASK(trans)			(TGL_EDP_PSR_MASK <<	\
> +						 _EDP_PSR_TRANS_SHIFT(trans))
> +#define   EDP_PSR_ERROR(trans)			(TGL_EDP_PSR_ERROR <<	\
> +						 _EDP_PSR_TRANS_SHIFT(trans))
> +#define   EDP_PSR_POST_EXIT(trans)		(TGL_EDP_PSR_POST_EXIT << \
> +						 _EDP_PSR_TRANS_SHIFT(trans))
> +#define   EDP_PSR_PRE_ENTRY(trans)		(TGL_EDP_PSR_PRE_ENTRY << \
> +						 _EDP_PSR_TRANS_SHIFT(trans))
>  
>  #define _SRD_AUX_DATA_A				0x60814
>  #define _SRD_AUX_DATA_EDP			0x6f814





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

  Powered by Linux