Re: [PATCH v3 08/23] drm/i915/tgl: Access the right register when handling PSR interruptions

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

 



On 2019-08-23 at 01:20:40 -0700, Lucas De Marchi wrote:
> From: José Roberto de Souza <jose.souza@xxxxxxxxx>
> 
> For older gens PSR IIR and IMR had a fixed address that was not
> relative to anything, but from TGL those registers moved to each
> transcoder offset.
> 
> So here adding a new macro and a new PSR irq handler with the
> transcoder parameter.
>
There are few minor comments below, apart from below comments
patch is looks ok to me.
Reviewed-by: Anshuman Gupta <anshuman.gupta@xxxxxxxxx>  
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx>
> Cc: Rodrigo Vivi <rodrigo.vivi@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 | 67 ++++++++++++++++++------
>  drivers/gpu/drm/i915/display/intel_psr.h |  1 +
>  drivers/gpu/drm/i915/i915_irq.c          | 52 +++++++++++++++---
>  drivers/gpu/drm/i915/i915_reg.h          | 10 +++-
>  4 files changed, 107 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> index 2429328f963e..c33aa16ed038 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -91,20 +91,33 @@ static bool intel_psr2_enabled(struct drm_i915_private *dev_priv,
>  static void psr_irq_control(struct drm_i915_private *dev_priv)
>  {
>  	enum transcoder trans = dev_priv->psr.transcoder;
> -	u32 val, mask;
> +	u32 psr_error, psr_entry, psr_exit, mask, val;
> +	i915_reg_t mask_reg;
> +
> +	if (INTEL_GEN(dev_priv) >= 12) {
> +		psr_error = TRANS_PSR_ERROR;
> +		psr_entry = TRANS_PSR_PRE_ENTRY;
> +		psr_exit = TRANS_PSR_POST_EXIT;
> +		mask_reg = TRANS_PSR_IMR(trans);
> +	} else {
> +		psr_error = EDP_PSR_ERROR(trans);
> +		psr_entry = EDP_PSR_PRE_ENTRY(trans);
> +		psr_exit = EDP_PSR_POST_EXIT(trans);
> +		mask_reg = EDP_PSR_IMR;
> +	}
>  
> -	mask = EDP_PSR_ERROR(trans);
> +	mask = psr_error;
>  	if (dev_priv->psr.debug & I915_PSR_DEBUG_IRQ)
> -		mask |= EDP_PSR_POST_EXIT(trans) | EDP_PSR_PRE_ENTRY(trans);
> +		mask |= psr_exit | psr_entry;
>  
>  	/*
>  	 * 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 = I915_READ(mask_reg);
> +	val &= ~(psr_error | psr_entry | psr_exit);
>  	val |= ~mask;
> -	I915_WRITE(EDP_PSR_IMR, val);
> +	I915_WRITE(mask_reg, val);
>  }
>  
>  static void psr_event_print(u32 val, bool psr2_enabled)
> @@ -147,9 +160,21 @@ static void psr_event_print(u32 val, bool psr2_enabled)
>  void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir)
>  {
>  	enum transcoder cpu_transcoder = dev_priv->psr.transcoder;
> +	u32 psr_error, psr_entry, psr_exit;
>  	ktime_t time_ns =  ktime_get();
>  
> -	if (psr_iir & EDP_PSR_ERROR(cpu_transcoder)) {
> +	if (INTEL_GEN(dev_priv) >= 12) {
> +		psr_error = TRANS_PSR_ERROR;
> +		psr_entry = TRANS_PSR_PRE_ENTRY;
> +		psr_exit = TRANS_PSR_POST_EXIT;
> +	} else {
> +		psr_error = EDP_PSR_ERROR(cpu_transcoder);
> +		psr_entry = EDP_PSR_PRE_ENTRY(cpu_transcoder);
> +		psr_exit = EDP_PSR_POST_EXIT(cpu_transcoder);
> +	}
> +
> +	if (psr_iir & psr_error) {
> +		i915_reg_t mask_reg;
>  		u32 val;
>  
>  		DRM_WARN("[transcoder %s] PSR aux error\n",
> @@ -168,20 +193,25 @@ void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir)
>  		 * 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 (INTEL_GEN(dev_priv) >= 12)
> +			mask_reg = TRANS_PSR_IMR(cpu_transcoder);
> +		else
> +			mask_reg = EDP_PSR_IMR;
> +
> +		val = I915_READ(mask_reg);
> +		val |= psr_error;
> +		I915_WRITE(mask_reg, val);
>  
>  		schedule_work(&dev_priv->psr.work);
>  	}
>  
> -	if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
> +	if (psr_iir & psr_entry) {
>  		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(cpu_transcoder)) {
> +	if (psr_iir & psr_exit) {
>  		dev_priv->psr.last_exit = time_ns;
>  		DRM_DEBUG_KMS("[transcoder %s] PSR exit completed\n",
>  			      transcoder_name(cpu_transcoder));
> @@ -632,7 +662,7 @@ static void intel_psr_activate(struct intel_dp *intel_dp)
>  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
>  
>  	if (INTEL_GEN(dev_priv) >= 9 &&
> -	    psr2_supported(dev_priv, dev_priv->psr.transcoder))
> +	    transcoder_has_psr2(dev_priv, dev_priv->psr.transcoder))
>  		WARN_ON(I915_READ(EDP_PSR2_CTL(dev_priv->psr.transcoder)) & EDP_PSR2_ENABLE);
>  	WARN_ON(I915_READ(EDP_PSR_CTL(dev_priv->psr.transcoder)) & EDP_PSR_ENABLE);
>  	WARN_ON(dev_priv->psr.active);
> @@ -730,8 +760,13 @@ static void intel_psr_enable_locked(struct drm_i915_private *dev_priv,
>  	 * first time that PSR HW tries to activate so lets keep PSR disabled
>  	 * to avoid any rendering problems.
>  	 */
> -	val = I915_READ(EDP_PSR_IIR);
> -	val &= EDP_PSR_ERROR(dev_priv->psr.transcoder);
> +	if (INTEL_GEN(dev_priv) >= 12) {
> +		val = I915_READ(TRANS_PSR_IIR(dev_priv->psr.transcoder));
> +		val &= TRANS_PSR_ERROR;
> +	} else {
> +		val = I915_READ(EDP_PSR_IIR);
> +		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");
> @@ -787,7 +822,7 @@ static void intel_psr_exit(struct drm_i915_private *dev_priv)
>  
>  	if (!dev_priv->psr.active) {
>  		if (INTEL_GEN(dev_priv) >= 9 &&
> -		    psr2_supported(dev_priv, dev_priv->psr.transcoder)) {
> +		    transcoder_has_psr2(dev_priv, dev_priv->psr.transcoder)) {
>  			val = I915_READ(EDP_PSR2_CTL(dev_priv->psr.transcoder));
>  			WARN_ON(val & EDP_PSR2_ENABLE);
>  		}
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.h b/drivers/gpu/drm/i915/display/intel_psr.h
> index 46e4de8b8cd5..6570a23a68b2 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.h
> +++ b/drivers/gpu/drm/i915/display/intel_psr.h
> @@ -6,6 +6,7 @@
>  #ifndef __INTEL_PSR_H__
>  #define __INTEL_PSR_H__
>  
> +#include "intel_display.h"
IMO we don't need to include this header.
>  #include "intel_frontbuffer.h"
>  
>  struct drm_i915_private;
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 77391d8325bf..6024a6ef1c76 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2655,11 +2655,22 @@ gen8_de_misc_irq_handler(struct drm_i915_private *dev_priv, u32 iir)
>  	}
>  
>  	if (iir & GEN8_DE_EDP_PSR) {
> -		u32 psr_iir = I915_READ(EDP_PSR_IIR);
> +		u32 psr_iir;
> +
> +		if (INTEL_GEN(dev_priv) >= 12) {
> +			enum transcoder trans = dev_priv->psr.transcoder;
> +
> +			psr_iir = I915_READ(TRANS_PSR_IIR(trans));
> +			I915_WRITE(TRANS_PSR_IIR(trans), psr_iir);
> +		} else {
> +			psr_iir = I915_READ(EDP_PSR_IIR);
> +			I915_WRITE(EDP_PSR_IIR, psr_iir);
> +		}
> +
> +		if (psr_iir)
> +			found = true;
>  
>  		intel_psr_irq_handler(dev_priv, psr_iir);
> -		I915_WRITE(EDP_PSR_IIR, psr_iir);
> -		found = true;
>  	}
>  
>  	if (!found)
> @@ -3279,8 +3290,23 @@ static void gen11_irq_reset(struct drm_i915_private *dev_priv)
>  
>  	intel_uncore_write(uncore, GEN11_DISPLAY_INT_CTL, 0);
>  
> -	intel_uncore_write(uncore, EDP_PSR_IMR, 0xffffffff);
> -	intel_uncore_write(uncore, EDP_PSR_IIR, 0xffffffff);
> +	if (INTEL_GEN(dev_priv) >= 12) {
> +		enum transcoder trans;
> +
> +		for (trans = TRANSCODER_A; trans <= TRANSCODER_D; trans++) {
> +			enum intel_display_power_domain domain;
> +
> +			domain = POWER_DOMAIN_TRANSCODER(trans);
> +			if (!intel_display_power_is_enabled(dev_priv, domain))
> +				continue;
> +
> +			intel_uncore_write(uncore, TRANS_PSR_IMR(trans), 0xffffffff);
> +			intel_uncore_write(uncore, TRANS_PSR_IIR(trans), 0xffffffff);
Lines over 80 char.
> +		}
> +	} else {
> +		intel_uncore_write(uncore, EDP_PSR_IMR, 0xffffffff);
> +		intel_uncore_write(uncore, EDP_PSR_IIR, 0xffffffff);
> +	}
>  
>  	for_each_pipe(dev_priv, pipe)
>  		if (intel_display_power_is_enabled(dev_priv,
> @@ -3793,7 +3819,21 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
>  	else if (IS_BROADWELL(dev_priv))
>  		de_port_enables |= GEN8_PORT_DP_A_HOTPLUG;
>  
> -	gen3_assert_iir_is_zero(uncore, EDP_PSR_IIR);
> +	if (INTEL_GEN(dev_priv) >= 12) {
> +		enum transcoder trans;
> +
> +		for (trans = TRANSCODER_A; trans <= TRANSCODER_D; trans++) {
> +			enum intel_display_power_domain domain;
> +
> +			domain = POWER_DOMAIN_TRANSCODER(trans);
> +			if (!intel_display_power_is_enabled(dev_priv, domain))
> +				continue;
> +
> +			gen3_assert_iir_is_zero(uncore, TRANS_PSR_IIR(trans));
> +		}
> +	} else {
> +		gen3_assert_iir_is_zero(uncore, EDP_PSR_IIR);
> +	}
>  
>  	for_each_pipe(dev_priv, pipe) {
>  		dev_priv->de_irq_mask[pipe] = ~de_pipe_masked;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 1c6d99944630..3de02683d856 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4222,7 +4222,7 @@ enum {
>  #define   EDP_PSR_TP1_TIME_0us			(3 << 4)
>  #define   EDP_PSR_IDLE_FRAME_SHIFT		0
>  
> -/* Bspec claims those aren't shifted but stay at 0x64800 */
> +/* Bspec claims those aren't shifted but stay at 0x64800 until TGL */
>  #define EDP_PSR_IMR				_MMIO(0x64834)
>  #define EDP_PSR_IIR				_MMIO(0x64838)
>  #define   _EDP_PSR_TRANS_SHIFT(trans)		((trans) == TRANSCODER_EDP ? \
> @@ -4232,6 +4232,14 @@ enum {
>  #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 _PSR_IMR_A				0x60814
> +#define _PSR_IIR_A				0x60818
> +#define TRANS_PSR_IMR(tran)			_MMIO_TRANS2(tran, _PSR_IMR_A) /* TGL+ */
> +#define TRANS_PSR_IIR(tran)			_MMIO_TRANS2(tran, _PSR_IIR_A) /* TGL+ */
IMO if /* TGL+ */ comment shifted above, it will satisfy the 80 char limit.  
> +#define   TRANS_PSR_ERROR			(1 << 2)
> +#define   TRANS_PSR_POST_EXIT			(1 << 1)
> +#define   TRANS_PSR_PRE_ENTRY			(1 << 0)
> +
>  #define _SRD_AUX_CTL_A				0x60810
>  #define _SRD_AUX_CTL_EDP			0x6f810
>  #define EDP_PSR_AUX_CTL(tran)			_MMIO(_PSR_ADJ(tran, _SRD_AUX_CTL_A))
> -- 
> 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