[v8,1/3] drm/i915/psr: Make PSR registers relative to transcoders
diff mbox series

Message ID 20190820223325.27490-1-jose.souza@intel.com
State New
Headers show
Series
  • [v8,1/3] drm/i915/psr: Make PSR registers relative to transcoders
Related show

Commit Message

Souza, Jose Aug. 20, 2019, 10:33 p.m. UTC
PSR registers are a mess, some have the full address while others just
have the additional offset from psr_mmio_base.

For BDW+ psr_mmio_base is nothing more than TRANSCODER_EDP_OFFSET +
0x800 and using it makes more difficult for people with an PSR
register address or PSR register name from from BSpec as i915 also
don't match the BSpec names.
For HSW psr_mmio_base is _DDI_BUF_CTL_A + 0x800 and PSR registers are
only available in DDIA.

Other reason to make relative to transcoder is that since BDW every
transcoder have PSR registers, so in theory it should be possible to
have PSR enabled in a non-eDP transcoder.

So for BDW+ we can use _TRANS2() to get the register offset of any
PSR register in any transcoder while for HSW we have _HSW_PSR_ADJ
that will calculate the register offset for the single PSR instance,
noting that we are already guarded about trying to enable PSR in other
port than DDIA on HSW by the 'if (dig_port->base.port != PORT_A)' in
intel_psr_compute_config(), this check should only be valid for HSW
and will be changed in future.
PSR2 registers and PSR_EVENT was added after Haswell so that is why
_PSR_ADJ() is not used in some macros.

The only registers that can not be relative to transcoder are
PSR_IMR and PSR_IIR that are not relative to anything, so keeping it
hardcoded. That changed for TGL but it will be handled in another
patch.

Also removing BDW_EDP_PSR_BASE from GVT because it is not used as it
is the only PSR register that GVT have.

v5:
- Macros changed to be more explicit about HSW (Dhinakaran)
- Squashed with the patch that added the tran parameter to the
macros (Dhinakaran)

v6:
- Checking for interruption errors after module reload in the
transcoder that will be used (Dhinakaran)
- Using lowercase to the registers offsets

v7:
- Removing IS_HASWELL() from registers macros(Jani)

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Zhi Wang <zhi.a.wang@intel.com>
Reviewed-by: Lucas De Marchi <lucas.demarchi@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 | 104 +++++++++++++----------
 drivers/gpu/drm/i915/gvt/handlers.c      |   2 +-
 drivers/gpu/drm/i915/i915_debugfs.c      |  18 ++--
 drivers/gpu/drm/i915/i915_drv.h          |   5 +-
 drivers/gpu/drm/i915/i915_reg.h          |  57 +++++++++----
 5 files changed, 113 insertions(+), 73 deletions(-)

Comments

Anshuman Gupta Aug. 21, 2019, 4:36 a.m. UTC | #1
On 2019-08-20 at 15:33:23 -0700, José Roberto de Souza wrote:
> PSR registers are a mess, some have the full address while others just
> have the additional offset from psr_mmio_base.
> 
> For BDW+ psr_mmio_base is nothing more than TRANSCODER_EDP_OFFSET +
> 0x800 and using it makes more difficult for people with an PSR
> register address or PSR register name from from BSpec as i915 also
> don't match the BSpec names.
> For HSW psr_mmio_base is _DDI_BUF_CTL_A + 0x800 and PSR registers are
> only available in DDIA.
> 
> Other reason to make relative to transcoder is that since BDW every
> transcoder have PSR registers, so in theory it should be possible to
> have PSR enabled in a non-eDP transcoder.
> 
> So for BDW+ we can use _TRANS2() to get the register offset of any
> PSR register in any transcoder while for HSW we have _HSW_PSR_ADJ
> that will calculate the register offset for the single PSR instance,
> noting that we are already guarded about trying to enable PSR in other
> port than DDIA on HSW by the 'if (dig_port->base.port != PORT_A)' in
> intel_psr_compute_config(), this check should only be valid for HSW
> and will be changed in future.
> PSR2 registers and PSR_EVENT was added after Haswell so that is why
> _PSR_ADJ() is not used in some macros.
> 
> The only registers that can not be relative to transcoder are
> PSR_IMR and PSR_IIR that are not relative to anything, so keeping it
> hardcoded. That changed for TGL but it will be handled in another
> patch.
> 
> Also removing BDW_EDP_PSR_BASE from GVT because it is not used as it
> is the only PSR register that GVT have.
> 
> v5:
> - Macros changed to be more explicit about HSW (Dhinakaran)
> - Squashed with the patch that added the tran parameter to the
> macros (Dhinakaran)
> 
> v6:
> - Checking for interruption errors after module reload in the
> transcoder that will be used (Dhinakaran)
> - Using lowercase to the registers offsets
> 
> v7:
> - Removing IS_HASWELL() from registers macros(Jani)
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Zhi Wang <zhi.a.wang@intel.com>
> Reviewed-by: Lucas De Marchi <lucas.demarchi@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 | 104 +++++++++++++----------
>  drivers/gpu/drm/i915/gvt/handlers.c      |   2 +-
>  drivers/gpu/drm/i915/i915_debugfs.c      |  18 ++--
>  drivers/gpu/drm/i915/i915_drv.h          |   5 +-
>  drivers/gpu/drm/i915/i915_reg.h          |  57 +++++++++----
>  5 files changed, 113 insertions(+), 73 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> index 3bfb720560c2..77232f6bca17 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -390,7 +390,7 @@ static void hsw_psr_setup_aux(struct intel_dp *intel_dp)
>  
>  	BUILD_BUG_ON(sizeof(aux_msg) > 20);
>  	for (i = 0; i < sizeof(aux_msg); i += 4)
> -		I915_WRITE(EDP_PSR_AUX_DATA(i >> 2),
> +		I915_WRITE(EDP_PSR_AUX_DATA(dev_priv->psr.transcoder, i >> 2),
>  			   intel_dp_pack_aux(&aux_msg[i], sizeof(aux_msg) - i));
>  
>  	aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, 0);
> @@ -401,7 +401,7 @@ static void hsw_psr_setup_aux(struct intel_dp *intel_dp)
>  
>  	/* Select only valid bits for SRD_AUX_CTL */
>  	aux_ctl &= psr_aux_mask;
> -	I915_WRITE(EDP_PSR_AUX_CTL, aux_ctl);
> +	I915_WRITE(EDP_PSR_AUX_CTL(dev_priv->psr.transcoder), aux_ctl);
>  }
>  
>  static void intel_psr_enable_sink(struct intel_dp *intel_dp)
> @@ -491,8 +491,9 @@ static void hsw_activate_psr1(struct intel_dp *intel_dp)
>  	if (INTEL_GEN(dev_priv) >= 8)
>  		val |= EDP_PSR_CRC_ENABLE;
>  
> -	val |= I915_READ(EDP_PSR_CTL) & EDP_PSR_RESTORE_PSR_ACTIVE_CTX_MASK;
> -	I915_WRITE(EDP_PSR_CTL, val);
> +	val |= (I915_READ(EDP_PSR_CTL(dev_priv->psr.transcoder)) &
> +		EDP_PSR_RESTORE_PSR_ACTIVE_CTX_MASK);
> +	I915_WRITE(EDP_PSR_CTL(dev_priv->psr.transcoder), val);
>  }
>  
>  static void hsw_activate_psr2(struct intel_dp *intel_dp)
> @@ -528,9 +529,9 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
>  	 * PSR2 HW is incorrectly using EDP_PSR_TP1_TP3_SEL and BSpec is
>  	 * recommending keep this bit unset while PSR2 is enabled.
>  	 */
> -	I915_WRITE(EDP_PSR_CTL, 0);
> +	I915_WRITE(EDP_PSR_CTL(dev_priv->psr.transcoder), 0);
>  
> -	I915_WRITE(EDP_PSR2_CTL, val);
> +	I915_WRITE(EDP_PSR2_CTL(dev_priv->psr.transcoder), val);
>  }
>  
>  static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
> @@ -606,10 +607,9 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
>  
>  	/*
>  	 * HSW spec explicitly says PSR is tied to port A.
> -	 * BDW+ platforms with DDI implementation of PSR have different
> -	 * PSR registers per transcoder and we only implement transcoder EDP
> -	 * ones. Since by Display design transcoder EDP is tied to port A
> -	 * we can safely escape based on the port A.
> +	 * BDW+ platforms have a instance of PSR registers per transcoder but
> +	 * for now it only supports one instance of PSR, so lets keep it
> +	 * hardcoded to PORT_A
>  	 */
>  	if (dig_port->base.port != PORT_A) {
>  		DRM_DEBUG_KMS("PSR condition failed: Port not supported\n");
> @@ -649,8 +649,8 @@ 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)
> -		WARN_ON(I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE);
> -	WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
> +		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);
>  	lockdep_assert_held(&dev_priv->psr.lock);
>  
> @@ -720,19 +720,37 @@ static void intel_psr_enable_source(struct intel_dp *intel_dp,
>  	if (INTEL_GEN(dev_priv) < 11)
>  		mask |= EDP_PSR_DEBUG_MASK_DISP_REG_WRITE;
>  
> -	I915_WRITE(EDP_PSR_DEBUG, mask);
> +	I915_WRITE(EDP_PSR_DEBUG(dev_priv->psr.transcoder), mask);
>  }
>  
>  static void intel_psr_enable_locked(struct drm_i915_private *dev_priv,
>  				    const struct intel_crtc_state *crtc_state)
>  {
>  	struct intel_dp *intel_dp = dev_priv->psr.dp;
> +	u32 val;
>  
>  	WARN_ON(dev_priv->psr.enabled);
>  
>  	dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv, crtc_state);
>  	dev_priv->psr.busy_frontbuffer_bits = 0;
>  	dev_priv->psr.pipe = to_intel_crtc(crtc_state->base.crtc)->pipe;
> +	dev_priv->psr.transcoder = crtc_state->cpu_transcoder;
> +
> +	/*
> +	 * If a PSR error happened and the driver is reloaded, the EDP_PSR_IIR
> +	 * will still keep the error set even after the reset done in the
> +	 * irq_preinstall and irq_uninstall hooks.
> +	 * And enabling in this situation cause the screen to freeze in the
> +	 * 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(edp_psr_shift(dev_priv->psr.transcoder));
> +	if (val) {
> +		dev_priv->psr.sink_not_reliable = true;
> +		DRM_DEBUG_KMS("PSR interruption error set, not enabling PSR\n");
> +		return;
> +	}
>  
>  	DRM_DEBUG_KMS("Enabling PSR%s\n",
>  		      dev_priv->psr.psr2_enabled ? "2" : "1");
> @@ -782,20 +800,27 @@ static void intel_psr_exit(struct drm_i915_private *dev_priv)
>  	u32 val;
>  
>  	if (!dev_priv->psr.active) {
> -		if (INTEL_GEN(dev_priv) >= 9)
> -			WARN_ON(I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE);
> -		WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
> +		if (INTEL_GEN(dev_priv) >= 9) {
> +			val = I915_READ(EDP_PSR2_CTL(dev_priv->psr.transcoder));
> +			WARN_ON(val & EDP_PSR2_ENABLE);
> +		}
> +
> +		val = I915_READ(EDP_PSR_CTL(dev_priv->psr.transcoder));
> +		WARN_ON(val & EDP_PSR_ENABLE);
> +
>  		return;
>  	}
>  
>  	if (dev_priv->psr.psr2_enabled) {
> -		val = I915_READ(EDP_PSR2_CTL);
> +		val = I915_READ(EDP_PSR2_CTL(dev_priv->psr.transcoder));
>  		WARN_ON(!(val & EDP_PSR2_ENABLE));
> -		I915_WRITE(EDP_PSR2_CTL, val & ~EDP_PSR2_ENABLE);
> +		val &= ~EDP_PSR2_ENABLE;
> +		I915_WRITE(EDP_PSR2_CTL(dev_priv->psr.transcoder), val);
>  	} else {
> -		val = I915_READ(EDP_PSR_CTL);
> +		val = I915_READ(EDP_PSR_CTL(dev_priv->psr.transcoder));
>  		WARN_ON(!(val & EDP_PSR_ENABLE));
> -		I915_WRITE(EDP_PSR_CTL, val & ~EDP_PSR_ENABLE);
> +		val &= ~EDP_PSR_ENABLE;
> +		I915_WRITE(EDP_PSR_CTL(dev_priv->psr.transcoder), val);
>  	}
>  	dev_priv->psr.active = false;
>  }
> @@ -817,10 +842,10 @@ static void intel_psr_disable_locked(struct intel_dp *intel_dp)
>  	intel_psr_exit(dev_priv);
>  
>  	if (dev_priv->psr.psr2_enabled) {
> -		psr_status = EDP_PSR2_STATUS;
> +		psr_status = EDP_PSR2_STATUS(dev_priv->psr.transcoder);
>  		psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
>  	} else {
> -		psr_status = EDP_PSR_STATUS;
> +		psr_status = EDP_PSR_STATUS(dev_priv->psr.transcoder);
>  		psr_status_mask = EDP_PSR_STATUS_STATE_MASK;
>  	}
>  
> @@ -963,7 +988,8 @@ int intel_psr_wait_for_idle(const struct intel_crtc_state *new_crtc_state,
>  	 * defensive enough to cover everything.
>  	 */
>  
> -	return __intel_wait_for_register(&dev_priv->uncore, EDP_PSR_STATUS,
> +	return __intel_wait_for_register(&dev_priv->uncore,
> +					 EDP_PSR_STATUS(dev_priv->psr.transcoder),
>  					 EDP_PSR_STATUS_STATE_MASK,
>  					 EDP_PSR_STATUS_STATE_IDLE, 2, 50,
>  					 out_value);
> @@ -979,10 +1005,10 @@ static bool __psr_wait_for_idle_locked(struct drm_i915_private *dev_priv)
>  		return false;
>  
>  	if (dev_priv->psr.psr2_enabled) {
> -		reg = EDP_PSR2_STATUS;
> +		reg = EDP_PSR2_STATUS(dev_priv->psr.transcoder);
>  		mask = EDP_PSR2_STATUS_STATE_MASK;
>  	} else {
> -		reg = EDP_PSR_STATUS;
> +		reg = EDP_PSR_STATUS(dev_priv->psr.transcoder);
>  		mask = EDP_PSR_STATUS_STATE_MASK;
>  	}
>  
> @@ -1208,36 +1234,24 @@ void intel_psr_flush(struct drm_i915_private *dev_priv,
>   */
>  void intel_psr_init(struct drm_i915_private *dev_priv)
>  {
> -	u32 val;
> -
>  	if (!HAS_PSR(dev_priv))
>  		return;
>  
> -	dev_priv->psr_mmio_base = IS_HASWELL(dev_priv) ?
> -		HSW_EDP_PSR_BASE : BDW_EDP_PSR_BASE;
> -
>  	if (!dev_priv->psr.sink_support)
>  		return;
>  
> +	if (IS_HASWELL(dev_priv))
> +		/*
> +		 * HSW don't have PSR registers on the same space as transcoder
> +		 * so set this to a value that when subtract to the register
> +		 * in transcoder space results in the right offset for HSW
> +		 */
> +		dev_priv->hsw_psr_mmio_adjust = _SRD_CTL_EDP - _HSW_EDP_PSR_BASE;
> +
>  	if (i915_modparams.enable_psr == -1)
>  		if (INTEL_GEN(dev_priv) < 9 || !dev_priv->vbt.psr.enable)
>  			i915_modparams.enable_psr = 0;
>  
> -	/*
> -	 * If a PSR error happened and the driver is reloaded, the EDP_PSR_IIR
> -	 * will still keep the error set even after the reset done in the
> -	 * irq_preinstall and irq_uninstall hooks.
> -	 * And enabling in this situation cause the screen to freeze in the
> -	 * 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(edp_psr_shift(TRANSCODER_EDP));
> -	if (val) {
> -		DRM_DEBUG_KMS("PSR interruption error set\n");
> -		dev_priv->psr.sink_not_reliable = true;
> -	}
> -
Earlier EDP_PSR_IIR was being checked only in driver init path, 
now it has been checked for every modeset/fastset path in
intel_psr_enable_locked(). Is it ok ?
If it is justified why are we not checking it in intel_psr_flush()
there also it enables psr.
>  	/* Set link_standby x link_off defaults */
>  	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
>  		/* HSW and BDW require workarounds that we don't implement. */
> diff --git a/drivers/gpu/drm/i915/gvt/handlers.c b/drivers/gpu/drm/i915/gvt/handlers.c
> index 25f78196b964..45a9124e53b6 100644
> --- a/drivers/gpu/drm/i915/gvt/handlers.c
> +++ b/drivers/gpu/drm/i915/gvt/handlers.c
> @@ -2796,7 +2796,7 @@ static int init_broadwell_mmio_info(struct intel_gvt *gvt)
>  	MMIO_D(CHICKEN_PIPESL_1(PIPE_C), D_BDW_PLUS);
>  
>  	MMIO_D(WM_MISC, D_BDW);
> -	MMIO_D(_MMIO(BDW_EDP_PSR_BASE), D_BDW);
> +	MMIO_D(_MMIO(_SRD_CTL_EDP), D_BDW);
>  
>  	MMIO_D(_MMIO(0x6671c), D_BDW_PLUS);
>  	MMIO_D(_MMIO(0x66c00), D_BDW_PLUS);
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index b39226d7f8d2..6e4824daafae 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2132,7 +2132,7 @@ psr_source_status(struct drm_i915_private *dev_priv, struct seq_file *m)
>  			"BUF_ON",
>  			"TG_ON"
>  		};
> -		val = I915_READ(EDP_PSR2_STATUS);
> +		val = I915_READ(EDP_PSR2_STATUS(dev_priv->psr.transcoder));
>  		status_val = (val & EDP_PSR2_STATUS_STATE_MASK) >>
>  			      EDP_PSR2_STATUS_STATE_SHIFT;
>  		if (status_val < ARRAY_SIZE(live_status))
> @@ -2148,7 +2148,7 @@ psr_source_status(struct drm_i915_private *dev_priv, struct seq_file *m)
>  			"SRDOFFACK",
>  			"SRDENT_ON",
>  		};
> -		val = I915_READ(EDP_PSR_STATUS);
> +		val = I915_READ(EDP_PSR_STATUS(dev_priv->psr.transcoder));
>  		status_val = (val & EDP_PSR_STATUS_STATE_MASK) >>
>  			      EDP_PSR_STATUS_STATE_SHIFT;
>  		if (status_val < ARRAY_SIZE(live_status))
> @@ -2191,10 +2191,10 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
>  		goto unlock;
>  
>  	if (psr->psr2_enabled) {
> -		val = I915_READ(EDP_PSR2_CTL);
> +		val = I915_READ(EDP_PSR2_CTL(dev_priv->psr.transcoder));
>  		enabled = val & EDP_PSR2_ENABLE;
>  	} else {
> -		val = I915_READ(EDP_PSR_CTL);
> +		val = I915_READ(EDP_PSR_CTL(dev_priv->psr.transcoder));
>  		enabled = val & EDP_PSR_ENABLE;
>  	}
>  	seq_printf(m, "Source PSR ctl: %s [0x%08x]\n",
> @@ -2207,7 +2207,8 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
>  	 * SKL+ Perf counter is reset to 0 everytime DC state is entered
>  	 */
>  	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
> -		val = I915_READ(EDP_PSR_PERF_CNT) & EDP_PSR_PERF_CNT_MASK;
> +		val = I915_READ(EDP_PSR_PERF_CNT(dev_priv->psr.transcoder));
> +		val &= EDP_PSR_PERF_CNT_MASK;
>  		seq_printf(m, "Performance counter: %u\n", val);
>  	}
>  
> @@ -2225,8 +2226,11 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
>  		 * Reading all 3 registers before hand to minimize crossing a
>  		 * frame boundary between register reads
>  		 */
> -		for (frame = 0; frame < PSR2_SU_STATUS_FRAMES; frame += 3)
> -			su_frames_val[frame / 3] = I915_READ(PSR2_SU_STATUS(frame));
> +		for (frame = 0; frame < PSR2_SU_STATUS_FRAMES; frame += 3) {
> +			val = I915_READ(PSR2_SU_STATUS(dev_priv->psr.transcoder,
> +						       frame));
> +			su_frames_val[frame / 3] = val;
> +		}
>  
>  		seq_puts(m, "Frame:\tPSR2 SU blocks:\n");
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index eb31c1656cea..be999791abca 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -479,6 +479,7 @@ struct i915_psr {
>  	bool enabled;
>  	struct intel_dp *dp;
>  	enum pipe pipe;
> +	enum transcoder transcoder;
>  	bool active;
>  	struct work_struct work;
>  	unsigned busy_frontbuffer_bits;
> @@ -1330,11 +1331,11 @@ struct drm_i915_private {
>  	 */
>  	u32 gpio_mmio_base;
>  
> +	u32 hsw_psr_mmio_adjust;
> +
>  	/* MMIO base address for MIPI regs */
>  	u32 mipi_mmio_base;
>  
> -	u32 psr_mmio_base;
> -
>  	u32 pps_mmio_base;
>  
>  	wait_queue_head_t gmbus_wait_queue;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 2abd199093c5..a092b34c269d 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4186,10 +4186,17 @@ enum {
>  #define PIPESRC(trans)		_MMIO_TRANS2(trans, _PIPEASRC)
>  #define PIPE_MULT(trans)	_MMIO_TRANS2(trans, _PIPE_MULT_A)
>  
> -/* HSW+ eDP PSR registers */
> -#define HSW_EDP_PSR_BASE	0x64800
> -#define BDW_EDP_PSR_BASE	0x6f800
> -#define EDP_PSR_CTL				_MMIO(dev_priv->psr_mmio_base + 0)
> +/*
> + * HSW+ eDP PSR registers
> + *
> + * HSW PSR registers are relative to DDIA(_DDI_BUF_CTL_A + 0x800) with just one
> + * instance of it
> + */
> +#define _HSW_EDP_PSR_BASE			0x64800
> +#define _SRD_CTL_A				0x60800
> +#define _SRD_CTL_EDP				0x6f800
> +#define _PSR_ADJ(tran, reg)			(_TRANS2(tran, reg) - dev_priv->hsw_psr_mmio_adjust)
> +#define EDP_PSR_CTL(tran)			_MMIO(_PSR_ADJ(tran, _SRD_CTL_A))
>  #define   EDP_PSR_ENABLE			(1 << 31)
>  #define   BDW_PSR_SINGLE_FRAME			(1 << 30)
>  #define   EDP_PSR_RESTORE_PSR_ACTIVE_CTX_MASK	(1 << 29) /* SW can't modify */
> @@ -4226,16 +4233,22 @@ enum {
>  #define   EDP_PSR_TRANSCODER_A_SHIFT		8
>  #define   EDP_PSR_TRANSCODER_EDP_SHIFT		0
>  
> -#define EDP_PSR_AUX_CTL				_MMIO(dev_priv->psr_mmio_base + 0x10)
> +#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))
>  #define   EDP_PSR_AUX_CTL_TIME_OUT_MASK		(3 << 26)
>  #define   EDP_PSR_AUX_CTL_MESSAGE_SIZE_MASK	(0x1f << 20)
>  #define   EDP_PSR_AUX_CTL_PRECHARGE_2US_MASK	(0xf << 16)
>  #define   EDP_PSR_AUX_CTL_ERROR_INTERRUPT	(1 << 11)
>  #define   EDP_PSR_AUX_CTL_BIT_CLOCK_2X_MASK	(0x7ff)
>  
> -#define EDP_PSR_AUX_DATA(i)			_MMIO(dev_priv->psr_mmio_base + 0x14 + (i) * 4) /* 5 registers */
> +#define _SRD_AUX_DATA_A				0x60814
> +#define _SRD_AUX_DATA_EDP			0x6f814
> +#define EDP_PSR_AUX_DATA(tran, i)		_MMIO(_PSR_ADJ(tran, _SRD_AUX_DATA_A) + (i) + 4) /* 5 registers */
>  
> -#define EDP_PSR_STATUS				_MMIO(dev_priv->psr_mmio_base + 0x40)
> +#define _SRD_STATUS_A				0x60840
> +#define _SRD_STATUS_EDP				0x6f840
> +#define EDP_PSR_STATUS(tran)			_MMIO(_PSR_ADJ(tran, _SRD_STATUS_A))
>  #define   EDP_PSR_STATUS_STATE_MASK		(7 << 29)
>  #define   EDP_PSR_STATUS_STATE_SHIFT		29
>  #define   EDP_PSR_STATUS_STATE_IDLE		(0 << 29)
> @@ -4260,10 +4273,15 @@ enum {
>  #define   EDP_PSR_STATUS_SENDING_TP1		(1 << 4)
>  #define   EDP_PSR_STATUS_IDLE_MASK		0xf
>  
> -#define EDP_PSR_PERF_CNT		_MMIO(dev_priv->psr_mmio_base + 0x44)
> +#define _SRD_PERF_CNT_A			0x60844
> +#define _SRD_PERF_CNT_EDP		0x6f844
> +#define EDP_PSR_PERF_CNT(tran)		_MMIO(_PSR_ADJ(tran, _SRD_PERF_CNT_A))
>  #define   EDP_PSR_PERF_CNT_MASK		0xffffff
>  
> -#define EDP_PSR_DEBUG				_MMIO(dev_priv->psr_mmio_base + 0x60) /* PSR_MASK on SKL+ */
> +/* PSR_MASK on SKL+ */
> +#define _SRD_DEBUG_A				0x60860
> +#define _SRD_DEBUG_EDP				0x6f860
> +#define EDP_PSR_DEBUG(tran)			_MMIO(_PSR_ADJ(tran, _SRD_DEBUG_A))
>  #define   EDP_PSR_DEBUG_MASK_MAX_SLEEP         (1 << 28)
>  #define   EDP_PSR_DEBUG_MASK_LPSP              (1 << 27)
>  #define   EDP_PSR_DEBUG_MASK_MEMUP             (1 << 26)
> @@ -4271,7 +4289,9 @@ enum {
>  #define   EDP_PSR_DEBUG_MASK_DISP_REG_WRITE    (1 << 16) /* Reserved in ICL+ */
>  #define   EDP_PSR_DEBUG_EXIT_ON_PIXEL_UNDERRUN (1 << 15) /* SKL+ */
>  
> -#define EDP_PSR2_CTL			_MMIO(0x6f900)
> +#define _PSR2_CTL_A			0x60900
> +#define _PSR2_CTL_EDP			0x6f900
> +#define EDP_PSR2_CTL(tran)		_MMIO_TRANS2(tran, _PSR2_CTL_A)
>  #define   EDP_PSR2_ENABLE		(1 << 31)
>  #define   EDP_SU_TRACK_ENABLE		(1 << 30)
>  #define   EDP_Y_COORDINATE_VALID	(1 << 26) /* GLK and CNL+ */
> @@ -4293,8 +4313,8 @@ enum {
>  #define _PSR_EVENT_TRANS_B			0x61848
>  #define _PSR_EVENT_TRANS_C			0x62848
>  #define _PSR_EVENT_TRANS_D			0x63848
> -#define _PSR_EVENT_TRANS_EDP			0x6F848
> -#define PSR_EVENT(trans)			_MMIO_TRANS2(trans, _PSR_EVENT_TRANS_A)
> +#define _PSR_EVENT_TRANS_EDP			0x6f848
> +#define PSR_EVENT(tran)				_MMIO_TRANS2(tran, _PSR_EVENT_TRANS_A)
>  #define  PSR_EVENT_PSR2_WD_TIMER_EXPIRE		(1 << 17)
>  #define  PSR_EVENT_PSR2_DISABLED		(1 << 16)
>  #define  PSR_EVENT_SU_DIRTY_FIFO_UNDERRUN	(1 << 15)
> @@ -4312,15 +4332,16 @@ enum {
>  #define  PSR_EVENT_LPSP_MODE_EXIT		(1 << 1)
>  #define  PSR_EVENT_PSR_DISABLE			(1 << 0)
>  
> -#define EDP_PSR2_STATUS			_MMIO(0x6f940)
> +#define _PSR2_STATUS_A			0x60940
> +#define _PSR2_STATUS_EDP		0x6f940
> +#define EDP_PSR2_STATUS(tran)		_MMIO_TRANS2(tran, _PSR2_STATUS_A)
>  #define EDP_PSR2_STATUS_STATE_MASK     (0xf << 28)
>  #define EDP_PSR2_STATUS_STATE_SHIFT    28
>  
> -#define _PSR2_SU_STATUS_0		0x6F914
> -#define _PSR2_SU_STATUS_1		0x6F918
> -#define _PSR2_SU_STATUS_2		0x6F91C
> -#define _PSR2_SU_STATUS(index)		_MMIO(_PICK_EVEN((index), _PSR2_SU_STATUS_0, _PSR2_SU_STATUS_1))
> -#define PSR2_SU_STATUS(frame)		(_PSR2_SU_STATUS((frame) / 3))
> +#define _PSR2_SU_STATUS_A		0x60914
> +#define _PSR2_SU_STATUS_EDP		0x6f914
> +#define _PSR2_SU_STATUS(tran, index)	_MMIO(_TRANS2(tran, _PSR2_SU_STATUS_A) + (index) * 4)
> +#define PSR2_SU_STATUS(tran, frame)	(_PSR2_SU_STATUS(tran, (frame) / 3))
>  #define PSR2_SU_STATUS_SHIFT(frame)	(((frame) % 3) * 10)
>  #define PSR2_SU_STATUS_MASK(frame)	(0x3ff << PSR2_SU_STATUS_SHIFT(frame))
>  #define PSR2_SU_STATUS_FRAMES		8
> -- 
> 2.22.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Souza, Jose Aug. 21, 2019, 8:06 p.m. UTC | #2
On Wed, 2019-08-21 at 10:06 +0530, Anshuman Gupta wrote:
> On 2019-08-20 at 15:33:23 -0700, José Roberto de Souza wrote:
> > PSR registers are a mess, some have the full address while others
> > just
> > have the additional offset from psr_mmio_base.
> > 
> > For BDW+ psr_mmio_base is nothing more than TRANSCODER_EDP_OFFSET +
> > 0x800 and using it makes more difficult for people with an PSR
> > register address or PSR register name from from BSpec as i915 also
> > don't match the BSpec names.
> > For HSW psr_mmio_base is _DDI_BUF_CTL_A + 0x800 and PSR registers
> > are
> > only available in DDIA.
> > 
> > Other reason to make relative to transcoder is that since BDW every
> > transcoder have PSR registers, so in theory it should be possible
> > to
> > have PSR enabled in a non-eDP transcoder.
> > 
> > So for BDW+ we can use _TRANS2() to get the register offset of any
> > PSR register in any transcoder while for HSW we have _HSW_PSR_ADJ
> > that will calculate the register offset for the single PSR
> > instance,
> > noting that we are already guarded about trying to enable PSR in
> > other
> > port than DDIA on HSW by the 'if (dig_port->base.port != PORT_A)'
> > in
> > intel_psr_compute_config(), this check should only be valid for HSW
> > and will be changed in future.
> > PSR2 registers and PSR_EVENT was added after Haswell so that is why
> > _PSR_ADJ() is not used in some macros.
> > 
> > The only registers that can not be relative to transcoder are
> > PSR_IMR and PSR_IIR that are not relative to anything, so keeping
> > it
> > hardcoded. That changed for TGL but it will be handled in another
> > patch.
> > 
> > Also removing BDW_EDP_PSR_BASE from GVT because it is not used as
> > it
> > is the only PSR register that GVT have.
> > 
> > v5:
> > - Macros changed to be more explicit about HSW (Dhinakaran)
> > - Squashed with the patch that added the tran parameter to the
> > macros (Dhinakaran)
> > 
> > v6:
> > - Checking for interruption errors after module reload in the
> > transcoder that will be used (Dhinakaran)
> > - Using lowercase to the registers offsets
> > 
> > v7:
> > - Removing IS_HASWELL() from registers macros(Jani)
> > 
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Zhi Wang <zhi.a.wang@intel.com>
> > Reviewed-by: Lucas De Marchi <lucas.demarchi@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 | 104 +++++++++++++----
> > ------
> >  drivers/gpu/drm/i915/gvt/handlers.c      |   2 +-
> >  drivers/gpu/drm/i915/i915_debugfs.c      |  18 ++--
> >  drivers/gpu/drm/i915/i915_drv.h          |   5 +-
> >  drivers/gpu/drm/i915/i915_reg.h          |  57 +++++++++----
> >  5 files changed, 113 insertions(+), 73 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > b/drivers/gpu/drm/i915/display/intel_psr.c
> > index 3bfb720560c2..77232f6bca17 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -390,7 +390,7 @@ static void hsw_psr_setup_aux(struct intel_dp
> > *intel_dp)
> >  
> >  	BUILD_BUG_ON(sizeof(aux_msg) > 20);
> >  	for (i = 0; i < sizeof(aux_msg); i += 4)
> > -		I915_WRITE(EDP_PSR_AUX_DATA(i >> 2),
> > +		I915_WRITE(EDP_PSR_AUX_DATA(dev_priv->psr.transcoder, i
> > >> 2),
> >  			   intel_dp_pack_aux(&aux_msg[i],
> > sizeof(aux_msg) - i));
> >  
> >  	aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp,
> > 0);
> > @@ -401,7 +401,7 @@ static void hsw_psr_setup_aux(struct intel_dp
> > *intel_dp)
> >  
> >  	/* Select only valid bits for SRD_AUX_CTL */
> >  	aux_ctl &= psr_aux_mask;
> > -	I915_WRITE(EDP_PSR_AUX_CTL, aux_ctl);
> > +	I915_WRITE(EDP_PSR_AUX_CTL(dev_priv->psr.transcoder), aux_ctl);
> >  }
> >  
> >  static void intel_psr_enable_sink(struct intel_dp *intel_dp)
> > @@ -491,8 +491,9 @@ static void hsw_activate_psr1(struct intel_dp
> > *intel_dp)
> >  	if (INTEL_GEN(dev_priv) >= 8)
> >  		val |= EDP_PSR_CRC_ENABLE;
> >  
> > -	val |= I915_READ(EDP_PSR_CTL) &
> > EDP_PSR_RESTORE_PSR_ACTIVE_CTX_MASK;
> > -	I915_WRITE(EDP_PSR_CTL, val);
> > +	val |= (I915_READ(EDP_PSR_CTL(dev_priv->psr.transcoder)) &
> > +		EDP_PSR_RESTORE_PSR_ACTIVE_CTX_MASK);
> > +	I915_WRITE(EDP_PSR_CTL(dev_priv->psr.transcoder), val);
> >  }
> >  
> >  static void hsw_activate_psr2(struct intel_dp *intel_dp)
> > @@ -528,9 +529,9 @@ static void hsw_activate_psr2(struct intel_dp
> > *intel_dp)
> >  	 * PSR2 HW is incorrectly using EDP_PSR_TP1_TP3_SEL and BSpec
> > is
> >  	 * recommending keep this bit unset while PSR2 is enabled.
> >  	 */
> > -	I915_WRITE(EDP_PSR_CTL, 0);
> > +	I915_WRITE(EDP_PSR_CTL(dev_priv->psr.transcoder), 0);
> >  
> > -	I915_WRITE(EDP_PSR2_CTL, val);
> > +	I915_WRITE(EDP_PSR2_CTL(dev_priv->psr.transcoder), val);
> >  }
> >  
> >  static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
> > @@ -606,10 +607,9 @@ void intel_psr_compute_config(struct intel_dp
> > *intel_dp,
> >  
> >  	/*
> >  	 * HSW spec explicitly says PSR is tied to port A.
> > -	 * BDW+ platforms with DDI implementation of PSR have different
> > -	 * PSR registers per transcoder and we only implement
> > transcoder EDP
> > -	 * ones. Since by Display design transcoder EDP is tied to port
> > A
> > -	 * we can safely escape based on the port A.
> > +	 * BDW+ platforms have a instance of PSR registers per
> > transcoder but
> > +	 * for now it only supports one instance of PSR, so lets keep
> > it
> > +	 * hardcoded to PORT_A
> >  	 */
> >  	if (dig_port->base.port != PORT_A) {
> >  		DRM_DEBUG_KMS("PSR condition failed: Port not
> > supported\n");
> > @@ -649,8 +649,8 @@ 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)
> > -		WARN_ON(I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE);
> > -	WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
> > +		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);
> >  	lockdep_assert_held(&dev_priv->psr.lock);
> >  
> > @@ -720,19 +720,37 @@ static void intel_psr_enable_source(struct
> > intel_dp *intel_dp,
> >  	if (INTEL_GEN(dev_priv) < 11)
> >  		mask |= EDP_PSR_DEBUG_MASK_DISP_REG_WRITE;
> >  
> > -	I915_WRITE(EDP_PSR_DEBUG, mask);
> > +	I915_WRITE(EDP_PSR_DEBUG(dev_priv->psr.transcoder), mask);
> >  }
> >  
> >  static void intel_psr_enable_locked(struct drm_i915_private
> > *dev_priv,
> >  				    const struct intel_crtc_state
> > *crtc_state)
> >  {
> >  	struct intel_dp *intel_dp = dev_priv->psr.dp;
> > +	u32 val;
> >  
> >  	WARN_ON(dev_priv->psr.enabled);
> >  
> >  	dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv,
> > crtc_state);
> >  	dev_priv->psr.busy_frontbuffer_bits = 0;
> >  	dev_priv->psr.pipe = to_intel_crtc(crtc_state->base.crtc)-
> > >pipe;
> > +	dev_priv->psr.transcoder = crtc_state->cpu_transcoder;
> > +
> > +	/*
> > +	 * If a PSR error happened and the driver is reloaded, the
> > EDP_PSR_IIR
> > +	 * will still keep the error set even after the reset done in
> > the
> > +	 * irq_preinstall and irq_uninstall hooks.
> > +	 * And enabling in this situation cause the screen to freeze in
> > the
> > +	 * 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(edp_psr_shift(dev_priv->psr.transcoder));
> > +	if (val) {
> > +		dev_priv->psr.sink_not_reliable = true;
> > +		DRM_DEBUG_KMS("PSR interruption error set, not enabling
> > PSR\n");
> > +		return;
> > +	}
> >  
> >  	DRM_DEBUG_KMS("Enabling PSR%s\n",
> >  		      dev_priv->psr.psr2_enabled ? "2" : "1");
> > @@ -782,20 +800,27 @@ static void intel_psr_exit(struct
> > drm_i915_private *dev_priv)
> >  	u32 val;
> >  
> >  	if (!dev_priv->psr.active) {
> > -		if (INTEL_GEN(dev_priv) >= 9)
> > -			WARN_ON(I915_READ(EDP_PSR2_CTL) &
> > EDP_PSR2_ENABLE);
> > -		WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
> > +		if (INTEL_GEN(dev_priv) >= 9) {
> > +			val = I915_READ(EDP_PSR2_CTL(dev_priv-
> > >psr.transcoder));
> > +			WARN_ON(val & EDP_PSR2_ENABLE);
> > +		}
> > +
> > +		val = I915_READ(EDP_PSR_CTL(dev_priv->psr.transcoder));
> > +		WARN_ON(val & EDP_PSR_ENABLE);
> > +
> >  		return;
> >  	}
> >  
> >  	if (dev_priv->psr.psr2_enabled) {
> > -		val = I915_READ(EDP_PSR2_CTL);
> > +		val = I915_READ(EDP_PSR2_CTL(dev_priv-
> > >psr.transcoder));
> >  		WARN_ON(!(val & EDP_PSR2_ENABLE));
> > -		I915_WRITE(EDP_PSR2_CTL, val & ~EDP_PSR2_ENABLE);
> > +		val &= ~EDP_PSR2_ENABLE;
> > +		I915_WRITE(EDP_PSR2_CTL(dev_priv->psr.transcoder),
> > val);
> >  	} else {
> > -		val = I915_READ(EDP_PSR_CTL);
> > +		val = I915_READ(EDP_PSR_CTL(dev_priv->psr.transcoder));
> >  		WARN_ON(!(val & EDP_PSR_ENABLE));
> > -		I915_WRITE(EDP_PSR_CTL, val & ~EDP_PSR_ENABLE);
> > +		val &= ~EDP_PSR_ENABLE;
> > +		I915_WRITE(EDP_PSR_CTL(dev_priv->psr.transcoder), val);
> >  	}
> >  	dev_priv->psr.active = false;
> >  }
> > @@ -817,10 +842,10 @@ static void intel_psr_disable_locked(struct
> > intel_dp *intel_dp)
> >  	intel_psr_exit(dev_priv);
> >  
> >  	if (dev_priv->psr.psr2_enabled) {
> > -		psr_status = EDP_PSR2_STATUS;
> > +		psr_status = EDP_PSR2_STATUS(dev_priv->psr.transcoder);
> >  		psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
> >  	} else {
> > -		psr_status = EDP_PSR_STATUS;
> > +		psr_status = EDP_PSR_STATUS(dev_priv->psr.transcoder);
> >  		psr_status_mask = EDP_PSR_STATUS_STATE_MASK;
> >  	}
> >  
> > @@ -963,7 +988,8 @@ int intel_psr_wait_for_idle(const struct
> > intel_crtc_state *new_crtc_state,
> >  	 * defensive enough to cover everything.
> >  	 */
> >  
> > -	return __intel_wait_for_register(&dev_priv->uncore,
> > EDP_PSR_STATUS,
> > +	return __intel_wait_for_register(&dev_priv->uncore,
> > +					 EDP_PSR_STATUS(dev_priv-
> > >psr.transcoder),
> >  					 EDP_PSR_STATUS_STATE_MASK,
> >  					 EDP_PSR_STATUS_STATE_IDLE, 2,
> > 50,
> >  					 out_value);
> > @@ -979,10 +1005,10 @@ static bool
> > __psr_wait_for_idle_locked(struct drm_i915_private *dev_priv)
> >  		return false;
> >  
> >  	if (dev_priv->psr.psr2_enabled) {
> > -		reg = EDP_PSR2_STATUS;
> > +		reg = EDP_PSR2_STATUS(dev_priv->psr.transcoder);
> >  		mask = EDP_PSR2_STATUS_STATE_MASK;
> >  	} else {
> > -		reg = EDP_PSR_STATUS;
> > +		reg = EDP_PSR_STATUS(dev_priv->psr.transcoder);
> >  		mask = EDP_PSR_STATUS_STATE_MASK;
> >  	}
> >  
> > @@ -1208,36 +1234,24 @@ void intel_psr_flush(struct
> > drm_i915_private *dev_priv,
> >   */
> >  void intel_psr_init(struct drm_i915_private *dev_priv)
> >  {
> > -	u32 val;
> > -
> >  	if (!HAS_PSR(dev_priv))
> >  		return;
> >  
> > -	dev_priv->psr_mmio_base = IS_HASWELL(dev_priv) ?
> > -		HSW_EDP_PSR_BASE : BDW_EDP_PSR_BASE;
> > -
> >  	if (!dev_priv->psr.sink_support)
> >  		return;
> >  
> > +	if (IS_HASWELL(dev_priv))
> > +		/*
> > +		 * HSW don't have PSR registers on the same space as
> > transcoder
> > +		 * so set this to a value that when subtract to the
> > register
> > +		 * in transcoder space results in the right offset for
> > HSW
> > +		 */
> > +		dev_priv->hsw_psr_mmio_adjust = _SRD_CTL_EDP -
> > _HSW_EDP_PSR_BASE;
> > +
> >  	if (i915_modparams.enable_psr == -1)
> >  		if (INTEL_GEN(dev_priv) < 9 || !dev_priv-
> > >vbt.psr.enable)
> >  			i915_modparams.enable_psr = 0;
> >  
> > -	/*
> > -	 * If a PSR error happened and the driver is reloaded, the
> > EDP_PSR_IIR
> > -	 * will still keep the error set even after the reset done in
> > the
> > -	 * irq_preinstall and irq_uninstall hooks.
> > -	 * And enabling in this situation cause the screen to freeze in
> > the
> > -	 * 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(edp_psr_shift(TRANSCODER_EDP));
> > -	if (val) {
> > -		DRM_DEBUG_KMS("PSR interruption error set\n");
> > -		dev_priv->psr.sink_not_reliable = true;
> > -	}
> > -
> Earlier EDP_PSR_IIR was being checked only in driver init path, 
> now it has been checked for every modeset/fastset path in
> intel_psr_enable_locked(). Is it ok ?
> If it is justified why are we not checking it in intel_psr_flush()
> there also it enables psr.

I moved it primarily because on intel_psr_enable_locked() we have the
transcoder that will be used, doing on init we would need to check for
all transcoders and in case of a error set in one transcoder we would
need to fail initialization as PSR could be enabled on that transcoder.

No need to do that on intel_psr_flush() because PSR interruptions can
be triggered even with PSR inactive.


> >  	/* Set link_standby x link_off defaults */
> >  	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> >  		/* HSW and BDW require workarounds that we don't
> > implement. */
> > diff --git a/drivers/gpu/drm/i915/gvt/handlers.c
> > b/drivers/gpu/drm/i915/gvt/handlers.c
> > index 25f78196b964..45a9124e53b6 100644
> > --- a/drivers/gpu/drm/i915/gvt/handlers.c
> > +++ b/drivers/gpu/drm/i915/gvt/handlers.c
> > @@ -2796,7 +2796,7 @@ static int init_broadwell_mmio_info(struct
> > intel_gvt *gvt)
> >  	MMIO_D(CHICKEN_PIPESL_1(PIPE_C), D_BDW_PLUS);
> >  
> >  	MMIO_D(WM_MISC, D_BDW);
> > -	MMIO_D(_MMIO(BDW_EDP_PSR_BASE), D_BDW);
> > +	MMIO_D(_MMIO(_SRD_CTL_EDP), D_BDW);
> >  
> >  	MMIO_D(_MMIO(0x6671c), D_BDW_PLUS);
> >  	MMIO_D(_MMIO(0x66c00), D_BDW_PLUS);
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > b/drivers/gpu/drm/i915/i915_debugfs.c
> > index b39226d7f8d2..6e4824daafae 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -2132,7 +2132,7 @@ psr_source_status(struct drm_i915_private
> > *dev_priv, struct seq_file *m)
> >  			"BUF_ON",
> >  			"TG_ON"
> >  		};
> > -		val = I915_READ(EDP_PSR2_STATUS);
> > +		val = I915_READ(EDP_PSR2_STATUS(dev_priv-
> > >psr.transcoder));
> >  		status_val = (val & EDP_PSR2_STATUS_STATE_MASK) >>
> >  			      EDP_PSR2_STATUS_STATE_SHIFT;
> >  		if (status_val < ARRAY_SIZE(live_status))
> > @@ -2148,7 +2148,7 @@ psr_source_status(struct drm_i915_private
> > *dev_priv, struct seq_file *m)
> >  			"SRDOFFACK",
> >  			"SRDENT_ON",
> >  		};
> > -		val = I915_READ(EDP_PSR_STATUS);
> > +		val = I915_READ(EDP_PSR_STATUS(dev_priv-
> > >psr.transcoder));
> >  		status_val = (val & EDP_PSR_STATUS_STATE_MASK) >>
> >  			      EDP_PSR_STATUS_STATE_SHIFT;
> >  		if (status_val < ARRAY_SIZE(live_status))
> > @@ -2191,10 +2191,10 @@ static int i915_edp_psr_status(struct
> > seq_file *m, void *data)
> >  		goto unlock;
> >  
> >  	if (psr->psr2_enabled) {
> > -		val = I915_READ(EDP_PSR2_CTL);
> > +		val = I915_READ(EDP_PSR2_CTL(dev_priv-
> > >psr.transcoder));
> >  		enabled = val & EDP_PSR2_ENABLE;
> >  	} else {
> > -		val = I915_READ(EDP_PSR_CTL);
> > +		val = I915_READ(EDP_PSR_CTL(dev_priv->psr.transcoder));
> >  		enabled = val & EDP_PSR_ENABLE;
> >  	}
> >  	seq_printf(m, "Source PSR ctl: %s [0x%08x]\n",
> > @@ -2207,7 +2207,8 @@ static int i915_edp_psr_status(struct
> > seq_file *m, void *data)
> >  	 * SKL+ Perf counter is reset to 0 everytime DC state is
> > entered
> >  	 */
> >  	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
> > -		val = I915_READ(EDP_PSR_PERF_CNT) &
> > EDP_PSR_PERF_CNT_MASK;
> > +		val = I915_READ(EDP_PSR_PERF_CNT(dev_priv-
> > >psr.transcoder));
> > +		val &= EDP_PSR_PERF_CNT_MASK;
> >  		seq_printf(m, "Performance counter: %u\n", val);
> >  	}
> >  
> > @@ -2225,8 +2226,11 @@ static int i915_edp_psr_status(struct
> > seq_file *m, void *data)
> >  		 * Reading all 3 registers before hand to minimize
> > crossing a
> >  		 * frame boundary between register reads
> >  		 */
> > -		for (frame = 0; frame < PSR2_SU_STATUS_FRAMES; frame +=
> > 3)
> > -			su_frames_val[frame / 3] =
> > I915_READ(PSR2_SU_STATUS(frame));
> > +		for (frame = 0; frame < PSR2_SU_STATUS_FRAMES; frame +=
> > 3) {
> > +			val = I915_READ(PSR2_SU_STATUS(dev_priv-
> > >psr.transcoder,
> > +						       frame));
> > +			su_frames_val[frame / 3] = val;
> > +		}
> >  
> >  		seq_puts(m, "Frame:\tPSR2 SU blocks:\n");
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index eb31c1656cea..be999791abca 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -479,6 +479,7 @@ struct i915_psr {
> >  	bool enabled;
> >  	struct intel_dp *dp;
> >  	enum pipe pipe;
> > +	enum transcoder transcoder;
> >  	bool active;
> >  	struct work_struct work;
> >  	unsigned busy_frontbuffer_bits;
> > @@ -1330,11 +1331,11 @@ struct drm_i915_private {
> >  	 */
> >  	u32 gpio_mmio_base;
> >  
> > +	u32 hsw_psr_mmio_adjust;
> > +
> >  	/* MMIO base address for MIPI regs */
> >  	u32 mipi_mmio_base;
> >  
> > -	u32 psr_mmio_base;
> > -
> >  	u32 pps_mmio_base;
> >  
> >  	wait_queue_head_t gmbus_wait_queue;
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 2abd199093c5..a092b34c269d 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -4186,10 +4186,17 @@ enum {
> >  #define PIPESRC(trans)		_MMIO_TRANS2(trans, _PIPEASRC)
> >  #define PIPE_MULT(trans)	_MMIO_TRANS2(trans, _PIPE_MULT_A)
> >  
> > -/* HSW+ eDP PSR registers */
> > -#define HSW_EDP_PSR_BASE	0x64800
> > -#define BDW_EDP_PSR_BASE	0x6f800
> > -#define EDP_PSR_CTL				_MMIO(dev_priv-
> > >psr_mmio_base + 0)
> > +/*
> > + * HSW+ eDP PSR registers
> > + *
> > + * HSW PSR registers are relative to DDIA(_DDI_BUF_CTL_A + 0x800)
> > with just one
> > + * instance of it
> > + */
> > +#define _HSW_EDP_PSR_BASE			0x64800
> > +#define _SRD_CTL_A				0x60800
> > +#define _SRD_CTL_EDP				0x6f800
> > +#define _PSR_ADJ(tran, reg)			(_TRANS2(tran,
> > reg) - dev_priv->hsw_psr_mmio_adjust)
> > +#define EDP_PSR_CTL(tran)			_MMIO(_PSR_ADJ(tran,
> > _SRD_CTL_A))
> >  #define   EDP_PSR_ENABLE			(1 << 31)
> >  #define   BDW_PSR_SINGLE_FRAME			(1 << 30)
> >  #define   EDP_PSR_RESTORE_PSR_ACTIVE_CTX_MASK	(1 << 29) /* SW
> > can't modify */
> > @@ -4226,16 +4233,22 @@ enum {
> >  #define   EDP_PSR_TRANSCODER_A_SHIFT		8
> >  #define   EDP_PSR_TRANSCODER_EDP_SHIFT		0
> >  
> > -#define EDP_PSR_AUX_CTL				_MMIO(dev_priv-
> > >psr_mmio_base + 0x10)
> > +#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))
> >  #define   EDP_PSR_AUX_CTL_TIME_OUT_MASK		(3 << 26)
> >  #define   EDP_PSR_AUX_CTL_MESSAGE_SIZE_MASK	(0x1f << 20)
> >  #define   EDP_PSR_AUX_CTL_PRECHARGE_2US_MASK	(0xf << 16)
> >  #define   EDP_PSR_AUX_CTL_ERROR_INTERRUPT	(1 << 11)
> >  #define   EDP_PSR_AUX_CTL_BIT_CLOCK_2X_MASK	(0x7ff)
> >  
> > -#define EDP_PSR_AUX_DATA(i)			_MMIO(dev_priv-
> > >psr_mmio_base + 0x14 + (i) * 4) /* 5 registers */
> > +#define _SRD_AUX_DATA_A				0x60814
> > +#define _SRD_AUX_DATA_EDP			0x6f814
> > +#define EDP_PSR_AUX_DATA(tran, i)		_MMIO(_PSR_ADJ(tran,
> > _SRD_AUX_DATA_A) + (i) + 4) /* 5 registers */
> >  
> > -#define EDP_PSR_STATUS				_MMIO(dev_priv-
> > >psr_mmio_base + 0x40)
> > +#define _SRD_STATUS_A				0x60840
> > +#define _SRD_STATUS_EDP				0x6f840
> > +#define EDP_PSR_STATUS(tran)			_MMIO(_PSR_ADJ(
> > tran, _SRD_STATUS_A))
> >  #define   EDP_PSR_STATUS_STATE_MASK		(7 << 29)
> >  #define   EDP_PSR_STATUS_STATE_SHIFT		29
> >  #define   EDP_PSR_STATUS_STATE_IDLE		(0 << 29)
> > @@ -4260,10 +4273,15 @@ enum {
> >  #define   EDP_PSR_STATUS_SENDING_TP1		(1 << 4)
> >  #define   EDP_PSR_STATUS_IDLE_MASK		0xf
> >  
> > -#define EDP_PSR_PERF_CNT		_MMIO(dev_priv->psr_mmio_base +
> > 0x44)
> > +#define _SRD_PERF_CNT_A			0x60844
> > +#define _SRD_PERF_CNT_EDP		0x6f844
> > +#define EDP_PSR_PERF_CNT(tran)		_MMIO(_PSR_ADJ(tran,
> > _SRD_PERF_CNT_A))
> >  #define   EDP_PSR_PERF_CNT_MASK		0xffffff
> >  
> > -#define EDP_PSR_DEBUG				_MMIO(dev_priv-
> > >psr_mmio_base + 0x60) /* PSR_MASK on SKL+ */
> > +/* PSR_MASK on SKL+ */
> > +#define _SRD_DEBUG_A				0x60860
> > +#define _SRD_DEBUG_EDP				0x6f860
> > +#define EDP_PSR_DEBUG(tran)			_MMIO(_PSR_ADJ(
> > tran, _SRD_DEBUG_A))
> >  #define   EDP_PSR_DEBUG_MASK_MAX_SLEEP         (1 << 28)
> >  #define   EDP_PSR_DEBUG_MASK_LPSP              (1 << 27)
> >  #define   EDP_PSR_DEBUG_MASK_MEMUP             (1 << 26)
> > @@ -4271,7 +4289,9 @@ enum {
> >  #define   EDP_PSR_DEBUG_MASK_DISP_REG_WRITE    (1 << 16) /*
> > Reserved in ICL+ */
> >  #define   EDP_PSR_DEBUG_EXIT_ON_PIXEL_UNDERRUN (1 << 15) /* SKL+
> > */
> >  
> > -#define EDP_PSR2_CTL			_MMIO(0x6f900)
> > +#define _PSR2_CTL_A			0x60900
> > +#define _PSR2_CTL_EDP			0x6f900
> > +#define EDP_PSR2_CTL(tran)		_MMIO_TRANS2(tran, _PSR2_CTL_A)
> >  #define   EDP_PSR2_ENABLE		(1 << 31)
> >  #define   EDP_SU_TRACK_ENABLE		(1 << 30)
> >  #define   EDP_Y_COORDINATE_VALID	(1 << 26) /* GLK and CNL+ */
> > @@ -4293,8 +4313,8 @@ enum {
> >  #define _PSR_EVENT_TRANS_B			0x61848
> >  #define _PSR_EVENT_TRANS_C			0x62848
> >  #define _PSR_EVENT_TRANS_D			0x63848
> > -#define _PSR_EVENT_TRANS_EDP			0x6F848
> > -#define PSR_EVENT(trans)			_MMIO_TRANS2(trans,
> > _PSR_EVENT_TRANS_A)
> > +#define _PSR_EVENT_TRANS_EDP			0x6f848
> > +#define PSR_EVENT(tran)				_MMIO_TRANS2(tr
> > an, _PSR_EVENT_TRANS_A)
> >  #define  PSR_EVENT_PSR2_WD_TIMER_EXPIRE		(1 << 17)
> >  #define  PSR_EVENT_PSR2_DISABLED		(1 << 16)
> >  #define  PSR_EVENT_SU_DIRTY_FIFO_UNDERRUN	(1 << 15)
> > @@ -4312,15 +4332,16 @@ enum {
> >  #define  PSR_EVENT_LPSP_MODE_EXIT		(1 << 1)
> >  #define  PSR_EVENT_PSR_DISABLE			(1 << 0)
> >  
> > -#define EDP_PSR2_STATUS			_MMIO(0x6f940)
> > +#define _PSR2_STATUS_A			0x60940
> > +#define _PSR2_STATUS_EDP		0x6f940
> > +#define EDP_PSR2_STATUS(tran)		_MMIO_TRANS2(tran,
> > _PSR2_STATUS_A)
> >  #define EDP_PSR2_STATUS_STATE_MASK     (0xf << 28)
> >  #define EDP_PSR2_STATUS_STATE_SHIFT    28
> >  
> > -#define _PSR2_SU_STATUS_0		0x6F914
> > -#define _PSR2_SU_STATUS_1		0x6F918
> > -#define _PSR2_SU_STATUS_2		0x6F91C
> > -#define _PSR2_SU_STATUS(index)		_MMIO(_PICK_EVEN((index
> > ), _PSR2_SU_STATUS_0, _PSR2_SU_STATUS_1))
> > -#define PSR2_SU_STATUS(frame)		(_PSR2_SU_STATUS((frame
> > ) / 3))
> > +#define _PSR2_SU_STATUS_A		0x60914
> > +#define _PSR2_SU_STATUS_EDP		0x6f914
> > +#define _PSR2_SU_STATUS(tran, index)	_MMIO(_TRANS2(tran,
> > _PSR2_SU_STATUS_A) + (index) * 4)
> > +#define PSR2_SU_STATUS(tran, frame)	(_PSR2_SU_STATUS(tran,
> > (frame) / 3))
> >  #define PSR2_SU_STATUS_SHIFT(frame)	(((frame) % 3) * 10)
> >  #define PSR2_SU_STATUS_MASK(frame)	(0x3ff <<
> > PSR2_SU_STATUS_SHIFT(frame))
> >  #define PSR2_SU_STATUS_FRAMES		8
> > -- 
> > 2.22.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Anshuman Gupta Aug. 22, 2019, 4:07 p.m. UTC | #3
On 8/22/2019 1:36 AM, Souza, Jose wrote:
> On Wed, 2019-08-21 at 10:06 +0530, Anshuman Gupta wrote:
>> On 2019-08-20 at 15:33:23 -0700, José Roberto de Souza wrote:
>>> PSR registers are a mess, some have the full address while others
>>> just
>>> have the additional offset from psr_mmio_base.
>>>
>>> For BDW+ psr_mmio_base is nothing more than TRANSCODER_EDP_OFFSET +
>>> 0x800 and using it makes more difficult for people with an PSR
>>> register address or PSR register name from from BSpec as i915 also
>>> don't match the BSpec names.
>>> For HSW psr_mmio_base is _DDI_BUF_CTL_A + 0x800 and PSR registers
>>> are
>>> only available in DDIA.
>>>
>>> Other reason to make relative to transcoder is that since BDW every
>>> transcoder have PSR registers, so in theory it should be possible
>>> to
>>> have PSR enabled in a non-eDP transcoder.
>>>
>>> So for BDW+ we can use _TRANS2() to get the register offset of any
>>> PSR register in any transcoder while for HSW we have _HSW_PSR_ADJ
>>> that will calculate the register offset for the single PSR
>>> instance,
>>> noting that we are already guarded about trying to enable PSR in
>>> other
>>> port than DDIA on HSW by the 'if (dig_port->base.port != PORT_A)'
>>> in
>>> intel_psr_compute_config(), this check should only be valid for HSW
>>> and will be changed in future.
>>> PSR2 registers and PSR_EVENT was added after Haswell so that is why
>>> _PSR_ADJ() is not used in some macros.
>>>
>>> The only registers that can not be relative to transcoder are
>>> PSR_IMR and PSR_IIR that are not relative to anything, so keeping
>>> it
>>> hardcoded. That changed for TGL but it will be handled in another
>>> patch.
>>>
>>> Also removing BDW_EDP_PSR_BASE from GVT because it is not used as
>>> it
>>> is the only PSR register that GVT have.
>>>
>>> v5:
>>> - Macros changed to be more explicit about HSW (Dhinakaran)
>>> - Squashed with the patch that added the tran parameter to the
>>> macros (Dhinakaran)
>>>
>>> v6:
>>> - Checking for interruption errors after module reload in the
>>> transcoder that will be used (Dhinakaran)
>>> - Using lowercase to the registers offsets
>>>
>>> v7:
>>> - Removing IS_HASWELL() from registers macros(Jani)
>>>
>>> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>> Cc: Jani Nikula <jani.nikula@linux.intel.com>
>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> Cc: Zhi Wang <zhi.a.wang@intel.com>
>>> Reviewed-by: Lucas De Marchi <lucas.demarchi@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 | 104 +++++++++++++----
>>> ------
>>>   drivers/gpu/drm/i915/gvt/handlers.c      |   2 +-
>>>   drivers/gpu/drm/i915/i915_debugfs.c      |  18 ++--
>>>   drivers/gpu/drm/i915/i915_drv.h          |   5 +-
>>>   drivers/gpu/drm/i915/i915_reg.h          |  57 +++++++++----
>>>   5 files changed, 113 insertions(+), 73 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
>>> b/drivers/gpu/drm/i915/display/intel_psr.c
>>> index 3bfb720560c2..77232f6bca17 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_psr.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
>>> @@ -390,7 +390,7 @@ static void hsw_psr_setup_aux(struct intel_dp
>>> *intel_dp)
>>>   
>>>   	BUILD_BUG_ON(sizeof(aux_msg) > 20);
>>>   	for (i = 0; i < sizeof(aux_msg); i += 4)
>>> -		I915_WRITE(EDP_PSR_AUX_DATA(i >> 2),
>>> +		I915_WRITE(EDP_PSR_AUX_DATA(dev_priv->psr.transcoder, i
>>>>> 2),
>>>   			   intel_dp_pack_aux(&aux_msg[i],
>>> sizeof(aux_msg) - i));
>>>   
>>>   	aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp,
>>> 0);
>>> @@ -401,7 +401,7 @@ static void hsw_psr_setup_aux(struct intel_dp
>>> *intel_dp)
>>>   
>>>   	/* Select only valid bits for SRD_AUX_CTL */
>>>   	aux_ctl &= psr_aux_mask;
>>> -	I915_WRITE(EDP_PSR_AUX_CTL, aux_ctl);
>>> +	I915_WRITE(EDP_PSR_AUX_CTL(dev_priv->psr.transcoder), aux_ctl);
>>>   }
>>>   
>>>   static void intel_psr_enable_sink(struct intel_dp *intel_dp)
>>> @@ -491,8 +491,9 @@ static void hsw_activate_psr1(struct intel_dp
>>> *intel_dp)
>>>   	if (INTEL_GEN(dev_priv) >= 8)
>>>   		val |= EDP_PSR_CRC_ENABLE;
>>>   
>>> -	val |= I915_READ(EDP_PSR_CTL) &
>>> EDP_PSR_RESTORE_PSR_ACTIVE_CTX_MASK;
>>> -	I915_WRITE(EDP_PSR_CTL, val);
>>> +	val |= (I915_READ(EDP_PSR_CTL(dev_priv->psr.transcoder)) &
>>> +		EDP_PSR_RESTORE_PSR_ACTIVE_CTX_MASK);
>>> +	I915_WRITE(EDP_PSR_CTL(dev_priv->psr.transcoder), val);
>>>   }
>>>   
>>>   static void hsw_activate_psr2(struct intel_dp *intel_dp)
>>> @@ -528,9 +529,9 @@ static void hsw_activate_psr2(struct intel_dp
>>> *intel_dp)
>>>   	 * PSR2 HW is incorrectly using EDP_PSR_TP1_TP3_SEL and BSpec
>>> is
>>>   	 * recommending keep this bit unset while PSR2 is enabled.
>>>   	 */
>>> -	I915_WRITE(EDP_PSR_CTL, 0);
>>> +	I915_WRITE(EDP_PSR_CTL(dev_priv->psr.transcoder), 0);
>>>   
>>> -	I915_WRITE(EDP_PSR2_CTL, val);
>>> +	I915_WRITE(EDP_PSR2_CTL(dev_priv->psr.transcoder), val);
>>>   }
>>>   
>>>   static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
>>> @@ -606,10 +607,9 @@ void intel_psr_compute_config(struct intel_dp
>>> *intel_dp,
>>>   
>>>   	/*
>>>   	 * HSW spec explicitly says PSR is tied to port A.
>>> -	 * BDW+ platforms with DDI implementation of PSR have different
>>> -	 * PSR registers per transcoder and we only implement
>>> transcoder EDP
>>> -	 * ones. Since by Display design transcoder EDP is tied to port
>>> A
>>> -	 * we can safely escape based on the port A.
>>> +	 * BDW+ platforms have a instance of PSR registers per
>>> transcoder but
>>> +	 * for now it only supports one instance of PSR, so lets keep
>>> it
>>> +	 * hardcoded to PORT_A
>>>   	 */
>>>   	if (dig_port->base.port != PORT_A) {
>>>   		DRM_DEBUG_KMS("PSR condition failed: Port not
>>> supported\n");
>>> @@ -649,8 +649,8 @@ 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)
>>> -		WARN_ON(I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE);
>>> -	WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
>>> +		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);
>>>   	lockdep_assert_held(&dev_priv->psr.lock);
>>>   
>>> @@ -720,19 +720,37 @@ static void intel_psr_enable_source(struct
>>> intel_dp *intel_dp,
>>>   	if (INTEL_GEN(dev_priv) < 11)
>>>   		mask |= EDP_PSR_DEBUG_MASK_DISP_REG_WRITE;
>>>   
>>> -	I915_WRITE(EDP_PSR_DEBUG, mask);
>>> +	I915_WRITE(EDP_PSR_DEBUG(dev_priv->psr.transcoder), mask);
>>>   }
>>>   
>>>   static void intel_psr_enable_locked(struct drm_i915_private
>>> *dev_priv,
>>>   				    const struct intel_crtc_state
>>> *crtc_state)
>>>   {
>>>   	struct intel_dp *intel_dp = dev_priv->psr.dp;
>>> +	u32 val;
>>>   
>>>   	WARN_ON(dev_priv->psr.enabled);
>>>   
>>>   	dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv,
>>> crtc_state);
>>>   	dev_priv->psr.busy_frontbuffer_bits = 0;
>>>   	dev_priv->psr.pipe = to_intel_crtc(crtc_state->base.crtc)-
>>>> pipe;
>>> +	dev_priv->psr.transcoder = crtc_state->cpu_transcoder;
>>> +
>>> +	/*
>>> +	 * If a PSR error happened and the driver is reloaded, the
>>> EDP_PSR_IIR
>>> +	 * will still keep the error set even after the reset done in
>>> the
>>> +	 * irq_preinstall and irq_uninstall hooks.
>>> +	 * And enabling in this situation cause the screen to freeze in
>>> the
>>> +	 * 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(edp_psr_shift(dev_priv->psr.transcoder));
>>> +	if (val) {
>>> +		dev_priv->psr.sink_not_reliable = true;
>>> +		DRM_DEBUG_KMS("PSR interruption error set, not enabling
>>> PSR\n");
>>> +		return;
>>> +	}
>>>   
>>>   	DRM_DEBUG_KMS("Enabling PSR%s\n",
>>>   		      dev_priv->psr.psr2_enabled ? "2" : "1");
>>> @@ -782,20 +800,27 @@ static void intel_psr_exit(struct
>>> drm_i915_private *dev_priv)
>>>   	u32 val;
>>>   
>>>   	if (!dev_priv->psr.active) {
>>> -		if (INTEL_GEN(dev_priv) >= 9)
>>> -			WARN_ON(I915_READ(EDP_PSR2_CTL) &
>>> EDP_PSR2_ENABLE);
>>> -		WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
>>> +		if (INTEL_GEN(dev_priv) >= 9) {
>>> +			val = I915_READ(EDP_PSR2_CTL(dev_priv-
>>>> psr.transcoder));
>>> +			WARN_ON(val & EDP_PSR2_ENABLE);
>>> +		}
>>> +
>>> +		val = I915_READ(EDP_PSR_CTL(dev_priv->psr.transcoder));
>>> +		WARN_ON(val & EDP_PSR_ENABLE);
>>> +
>>>   		return;
>>>   	}
>>>   
>>>   	if (dev_priv->psr.psr2_enabled) {
>>> -		val = I915_READ(EDP_PSR2_CTL);
>>> +		val = I915_READ(EDP_PSR2_CTL(dev_priv-
>>>> psr.transcoder));
>>>   		WARN_ON(!(val & EDP_PSR2_ENABLE));
>>> -		I915_WRITE(EDP_PSR2_CTL, val & ~EDP_PSR2_ENABLE);
>>> +		val &= ~EDP_PSR2_ENABLE;
>>> +		I915_WRITE(EDP_PSR2_CTL(dev_priv->psr.transcoder),
>>> val);
>>>   	} else {
>>> -		val = I915_READ(EDP_PSR_CTL);
>>> +		val = I915_READ(EDP_PSR_CTL(dev_priv->psr.transcoder));
>>>   		WARN_ON(!(val & EDP_PSR_ENABLE));
>>> -		I915_WRITE(EDP_PSR_CTL, val & ~EDP_PSR_ENABLE);
>>> +		val &= ~EDP_PSR_ENABLE;
>>> +		I915_WRITE(EDP_PSR_CTL(dev_priv->psr.transcoder), val);
>>>   	}
>>>   	dev_priv->psr.active = false;
>>>   }
>>> @@ -817,10 +842,10 @@ static void intel_psr_disable_locked(struct
>>> intel_dp *intel_dp)
>>>   	intel_psr_exit(dev_priv);
>>>   
>>>   	if (dev_priv->psr.psr2_enabled) {
>>> -		psr_status = EDP_PSR2_STATUS;
>>> +		psr_status = EDP_PSR2_STATUS(dev_priv->psr.transcoder);
>>>   		psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
>>>   	} else {
>>> -		psr_status = EDP_PSR_STATUS;
>>> +		psr_status = EDP_PSR_STATUS(dev_priv->psr.transcoder);
>>>   		psr_status_mask = EDP_PSR_STATUS_STATE_MASK;
>>>   	}
>>>   
>>> @@ -963,7 +988,8 @@ int intel_psr_wait_for_idle(const struct
>>> intel_crtc_state *new_crtc_state,
>>>   	 * defensive enough to cover everything.
>>>   	 */
>>>   
>>> -	return __intel_wait_for_register(&dev_priv->uncore,
>>> EDP_PSR_STATUS,
>>> +	return __intel_wait_for_register(&dev_priv->uncore,
>>> +					 EDP_PSR_STATUS(dev_priv-
>>>> psr.transcoder),
>>>   					 EDP_PSR_STATUS_STATE_MASK,
>>>   					 EDP_PSR_STATUS_STATE_IDLE, 2,
>>> 50,
>>>   					 out_value);
>>> @@ -979,10 +1005,10 @@ static bool
>>> __psr_wait_for_idle_locked(struct drm_i915_private *dev_priv)
>>>   		return false;
>>>   
>>>   	if (dev_priv->psr.psr2_enabled) {
>>> -		reg = EDP_PSR2_STATUS;
>>> +		reg = EDP_PSR2_STATUS(dev_priv->psr.transcoder);
>>>   		mask = EDP_PSR2_STATUS_STATE_MASK;
>>>   	} else {
>>> -		reg = EDP_PSR_STATUS;
>>> +		reg = EDP_PSR_STATUS(dev_priv->psr.transcoder);
>>>   		mask = EDP_PSR_STATUS_STATE_MASK;
>>>   	}
>>>   
>>> @@ -1208,36 +1234,24 @@ void intel_psr_flush(struct
>>> drm_i915_private *dev_priv,
>>>    */
>>>   void intel_psr_init(struct drm_i915_private *dev_priv)
>>>   {
>>> -	u32 val;
>>> -
>>>   	if (!HAS_PSR(dev_priv))
>>>   		return;
>>>   
>>> -	dev_priv->psr_mmio_base = IS_HASWELL(dev_priv) ?
>>> -		HSW_EDP_PSR_BASE : BDW_EDP_PSR_BASE;
>>> -
>>>   	if (!dev_priv->psr.sink_support)
>>>   		return;
>>>   
>>> +	if (IS_HASWELL(dev_priv))
>>> +		/*
>>> +		 * HSW don't have PSR registers on the same space as
>>> transcoder
>>> +		 * so set this to a value that when subtract to the
>>> register
>>> +		 * in transcoder space results in the right offset for
>>> HSW
>>> +		 */
>>> +		dev_priv->hsw_psr_mmio_adjust = _SRD_CTL_EDP -
>>> _HSW_EDP_PSR_BASE;
>>> +
>>>   	if (i915_modparams.enable_psr == -1)
>>>   		if (INTEL_GEN(dev_priv) < 9 || !dev_priv-
>>>> vbt.psr.enable)
>>>   			i915_modparams.enable_psr = 0;
>>>   
>>> -	/*
>>> -	 * If a PSR error happened and the driver is reloaded, the
>>> EDP_PSR_IIR
>>> -	 * will still keep the error set even after the reset done in
>>> the
>>> -	 * irq_preinstall and irq_uninstall hooks.
>>> -	 * And enabling in this situation cause the screen to freeze in
>>> the
>>> -	 * 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(edp_psr_shift(TRANSCODER_EDP));
>>> -	if (val) {
>>> -		DRM_DEBUG_KMS("PSR interruption error set\n");
>>> -		dev_priv->psr.sink_not_reliable = true;
>>> -	}
>>> -
>> Earlier EDP_PSR_IIR was being checked only in driver init path,
>> now it has been checked for every modeset/fastset path in
>> intel_psr_enable_locked(). Is it ok ?
>> If it is justified why are we not checking it in intel_psr_flush()
>> there also it enables psr.
> 
> I moved it primarily because on intel_psr_enable_locked() we have the
> transcoder that will be used, doing on init we would need to check for
> all transcoders and in case of a error set in one transcoder we would
> need to fail initialization as PSR could be enabled on that transcoder.
That is correct, but with this EDP_PSR_IIR error is getting checked in 
every modeset/fastset which is redundant, as it is already being check 
at interrupt handler.
IMO it should be only checked somehow at initel_psr_init(), even comment 
also reflect same thing ("If a PSR error happened and the driver is 
reloaded...").
If there no other way to handle this, comment should be change accordingly.
> 
> No need to do that on intel_psr_flush() because PSR interruptions can
> be triggered even with PSR inactive.
> 
> 
>>>   	/* Set link_standby x link_off defaults */
>>>   	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
>>>   		/* HSW and BDW require workarounds that we don't
>>> implement. */
>>> diff --git a/drivers/gpu/drm/i915/gvt/handlers.c
>>> b/drivers/gpu/drm/i915/gvt/handlers.c
>>> index 25f78196b964..45a9124e53b6 100644
>>> --- a/drivers/gpu/drm/i915/gvt/handlers.c
>>> +++ b/drivers/gpu/drm/i915/gvt/handlers.c
>>> @@ -2796,7 +2796,7 @@ static int init_broadwell_mmio_info(struct
>>> intel_gvt *gvt)
>>>   	MMIO_D(CHICKEN_PIPESL_1(PIPE_C), D_BDW_PLUS);
>>>   
>>>   	MMIO_D(WM_MISC, D_BDW);
>>> -	MMIO_D(_MMIO(BDW_EDP_PSR_BASE), D_BDW);
>>> +	MMIO_D(_MMIO(_SRD_CTL_EDP), D_BDW);
>>>   
>>>   	MMIO_D(_MMIO(0x6671c), D_BDW_PLUS);
>>>   	MMIO_D(_MMIO(0x66c00), D_BDW_PLUS);
>>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
>>> b/drivers/gpu/drm/i915/i915_debugfs.c
>>> index b39226d7f8d2..6e4824daafae 100644
>>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>>> @@ -2132,7 +2132,7 @@ psr_source_status(struct drm_i915_private
>>> *dev_priv, struct seq_file *m)
>>>   			"BUF_ON",
>>>   			"TG_ON"
>>>   		};
>>> -		val = I915_READ(EDP_PSR2_STATUS);
>>> +		val = I915_READ(EDP_PSR2_STATUS(dev_priv-
>>>> psr.transcoder));
>>>   		status_val = (val & EDP_PSR2_STATUS_STATE_MASK) >>
>>>   			      EDP_PSR2_STATUS_STATE_SHIFT;
>>>   		if (status_val < ARRAY_SIZE(live_status))
>>> @@ -2148,7 +2148,7 @@ psr_source_status(struct drm_i915_private
>>> *dev_priv, struct seq_file *m)
>>>   			"SRDOFFACK",
>>>   			"SRDENT_ON",
>>>   		};
>>> -		val = I915_READ(EDP_PSR_STATUS);
>>> +		val = I915_READ(EDP_PSR_STATUS(dev_priv-
>>>> psr.transcoder));
>>>   		status_val = (val & EDP_PSR_STATUS_STATE_MASK) >>
>>>   			      EDP_PSR_STATUS_STATE_SHIFT;
>>>   		if (status_val < ARRAY_SIZE(live_status))
>>> @@ -2191,10 +2191,10 @@ static int i915_edp_psr_status(struct
>>> seq_file *m, void *data)
>>>   		goto unlock;
>>>   
>>>   	if (psr->psr2_enabled) {
>>> -		val = I915_READ(EDP_PSR2_CTL);
>>> +		val = I915_READ(EDP_PSR2_CTL(dev_priv-
>>>> psr.transcoder));
>>>   		enabled = val & EDP_PSR2_ENABLE;
>>>   	} else {
>>> -		val = I915_READ(EDP_PSR_CTL);
>>> +		val = I915_READ(EDP_PSR_CTL(dev_priv->psr.transcoder));
>>>   		enabled = val & EDP_PSR_ENABLE;
>>>   	}
>>>   	seq_printf(m, "Source PSR ctl: %s [0x%08x]\n",
>>> @@ -2207,7 +2207,8 @@ static int i915_edp_psr_status(struct
>>> seq_file *m, void *data)
>>>   	 * SKL+ Perf counter is reset to 0 everytime DC state is
>>> entered
>>>   	 */
>>>   	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
>>> -		val = I915_READ(EDP_PSR_PERF_CNT) &
>>> EDP_PSR_PERF_CNT_MASK;
>>> +		val = I915_READ(EDP_PSR_PERF_CNT(dev_priv-
>>>> psr.transcoder));
>>> +		val &= EDP_PSR_PERF_CNT_MASK;
>>>   		seq_printf(m, "Performance counter: %u\n", val);
>>>   	}
>>>   
>>> @@ -2225,8 +2226,11 @@ static int i915_edp_psr_status(struct
>>> seq_file *m, void *data)
>>>   		 * Reading all 3 registers before hand to minimize
>>> crossing a
>>>   		 * frame boundary between register reads
>>>   		 */
>>> -		for (frame = 0; frame < PSR2_SU_STATUS_FRAMES; frame +=
>>> 3)
>>> -			su_frames_val[frame / 3] =
>>> I915_READ(PSR2_SU_STATUS(frame));
>>> +		for (frame = 0; frame < PSR2_SU_STATUS_FRAMES; frame +=
>>> 3) {
>>> +			val = I915_READ(PSR2_SU_STATUS(dev_priv-
>>>> psr.transcoder,
>>> +						       frame));
>>> +			su_frames_val[frame / 3] = val;
>>> +		}
>>>   
>>>   		seq_puts(m, "Frame:\tPSR2 SU blocks:\n");
>>>   
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>>> b/drivers/gpu/drm/i915/i915_drv.h
>>> index eb31c1656cea..be999791abca 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -479,6 +479,7 @@ struct i915_psr {
>>>   	bool enabled;
>>>   	struct intel_dp *dp;
>>>   	enum pipe pipe;
>>> +	enum transcoder transcoder;
>>>   	bool active;
>>>   	struct work_struct work;
>>>   	unsigned busy_frontbuffer_bits;
>>> @@ -1330,11 +1331,11 @@ struct drm_i915_private {
>>>   	 */
>>>   	u32 gpio_mmio_base;
>>>   
>>> +	u32 hsw_psr_mmio_adjust;
>>> +
>>>   	/* MMIO base address for MIPI regs */
>>>   	u32 mipi_mmio_base;
>>>   
>>> -	u32 psr_mmio_base;
>>> -
>>>   	u32 pps_mmio_base;
>>>   
>>>   	wait_queue_head_t gmbus_wait_queue;
>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>>> b/drivers/gpu/drm/i915/i915_reg.h
>>> index 2abd199093c5..a092b34c269d 100644
>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>> @@ -4186,10 +4186,17 @@ enum {
>>>   #define PIPESRC(trans)		_MMIO_TRANS2(trans, _PIPEASRC)
>>>   #define PIPE_MULT(trans)	_MMIO_TRANS2(trans, _PIPE_MULT_A)
>>>   
>>> -/* HSW+ eDP PSR registers */
>>> -#define HSW_EDP_PSR_BASE	0x64800
>>> -#define BDW_EDP_PSR_BASE	0x6f800
>>> -#define EDP_PSR_CTL				_MMIO(dev_priv-
>>>> psr_mmio_base + 0)
>>> +/*
>>> + * HSW+ eDP PSR registers
>>> + *
>>> + * HSW PSR registers are relative to DDIA(_DDI_BUF_CTL_A + 0x800)
>>> with just one
>>> + * instance of it
>>> + */
>>> +#define _HSW_EDP_PSR_BASE			0x64800
>>> +#define _SRD_CTL_A				0x60800
>>> +#define _SRD_CTL_EDP				0x6f800
>>> +#define _PSR_ADJ(tran, reg)			(_TRANS2(tran,
>>> reg) - dev_priv->hsw_psr_mmio_adjust)
>>> +#define EDP_PSR_CTL(tran)			_MMIO(_PSR_ADJ(tran,
>>> _SRD_CTL_A))
>>>   #define   EDP_PSR_ENABLE			(1 << 31)
>>>   #define   BDW_PSR_SINGLE_FRAME			(1 << 30)
>>>   #define   EDP_PSR_RESTORE_PSR_ACTIVE_CTX_MASK	(1 << 29) /* SW
>>> can't modify */
>>> @@ -4226,16 +4233,22 @@ enum {
>>>   #define   EDP_PSR_TRANSCODER_A_SHIFT		8
>>>   #define   EDP_PSR_TRANSCODER_EDP_SHIFT		0
>>>   
>>> -#define EDP_PSR_AUX_CTL				_MMIO(dev_priv-
>>>> psr_mmio_base + 0x10)
>>> +#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))
>>>   #define   EDP_PSR_AUX_CTL_TIME_OUT_MASK		(3 << 26)
>>>   #define   EDP_PSR_AUX_CTL_MESSAGE_SIZE_MASK	(0x1f << 20)
>>>   #define   EDP_PSR_AUX_CTL_PRECHARGE_2US_MASK	(0xf << 16)
>>>   #define   EDP_PSR_AUX_CTL_ERROR_INTERRUPT	(1 << 11)
>>>   #define   EDP_PSR_AUX_CTL_BIT_CLOCK_2X_MASK	(0x7ff)
>>>   
>>> -#define EDP_PSR_AUX_DATA(i)			_MMIO(dev_priv-
>>>> psr_mmio_base + 0x14 + (i) * 4) /* 5 registers */
>>> +#define _SRD_AUX_DATA_A				0x60814
>>> +#define _SRD_AUX_DATA_EDP			0x6f814
>>> +#define EDP_PSR_AUX_DATA(tran, i)		_MMIO(_PSR_ADJ(tran,
>>> _SRD_AUX_DATA_A) + (i) + 4) /* 5 registers */
>>>   
>>> -#define EDP_PSR_STATUS				_MMIO(dev_priv-
>>>> psr_mmio_base + 0x40)
>>> +#define _SRD_STATUS_A				0x60840
>>> +#define _SRD_STATUS_EDP				0x6f840
>>> +#define EDP_PSR_STATUS(tran)			_MMIO(_PSR_ADJ(
>>> tran, _SRD_STATUS_A))
>>>   #define   EDP_PSR_STATUS_STATE_MASK		(7 << 29)
>>>   #define   EDP_PSR_STATUS_STATE_SHIFT		29
>>>   #define   EDP_PSR_STATUS_STATE_IDLE		(0 << 29)
>>> @@ -4260,10 +4273,15 @@ enum {
>>>   #define   EDP_PSR_STATUS_SENDING_TP1		(1 << 4)
>>>   #define   EDP_PSR_STATUS_IDLE_MASK		0xf
>>>   
>>> -#define EDP_PSR_PERF_CNT		_MMIO(dev_priv->psr_mmio_base +
>>> 0x44)
>>> +#define _SRD_PERF_CNT_A			0x60844
>>> +#define _SRD_PERF_CNT_EDP		0x6f844
>>> +#define EDP_PSR_PERF_CNT(tran)		_MMIO(_PSR_ADJ(tran,
>>> _SRD_PERF_CNT_A))
>>>   #define   EDP_PSR_PERF_CNT_MASK		0xffffff
>>>   
>>> -#define EDP_PSR_DEBUG				_MMIO(dev_priv-
>>>> psr_mmio_base + 0x60) /* PSR_MASK on SKL+ */
>>> +/* PSR_MASK on SKL+ */
>>> +#define _SRD_DEBUG_A				0x60860
>>> +#define _SRD_DEBUG_EDP				0x6f860
>>> +#define EDP_PSR_DEBUG(tran)			_MMIO(_PSR_ADJ(
>>> tran, _SRD_DEBUG_A))
>>>   #define   EDP_PSR_DEBUG_MASK_MAX_SLEEP         (1 << 28)
>>>   #define   EDP_PSR_DEBUG_MASK_LPSP              (1 << 27)
>>>   #define   EDP_PSR_DEBUG_MASK_MEMUP             (1 << 26)
>>> @@ -4271,7 +4289,9 @@ enum {
>>>   #define   EDP_PSR_DEBUG_MASK_DISP_REG_WRITE    (1 << 16) /*
>>> Reserved in ICL+ */
>>>   #define   EDP_PSR_DEBUG_EXIT_ON_PIXEL_UNDERRUN (1 << 15) /* SKL+
>>> */
>>>   
>>> -#define EDP_PSR2_CTL			_MMIO(0x6f900)
>>> +#define _PSR2_CTL_A			0x60900
>>> +#define _PSR2_CTL_EDP			0x6f900
>>> +#define EDP_PSR2_CTL(tran)		_MMIO_TRANS2(tran, _PSR2_CTL_A)
>>>   #define   EDP_PSR2_ENABLE		(1 << 31)
>>>   #define   EDP_SU_TRACK_ENABLE		(1 << 30)
>>>   #define   EDP_Y_COORDINATE_VALID	(1 << 26) /* GLK and CNL+ */
>>> @@ -4293,8 +4313,8 @@ enum {
>>>   #define _PSR_EVENT_TRANS_B			0x61848
>>>   #define _PSR_EVENT_TRANS_C			0x62848
>>>   #define _PSR_EVENT_TRANS_D			0x63848
>>> -#define _PSR_EVENT_TRANS_EDP			0x6F848
>>> -#define PSR_EVENT(trans)			_MMIO_TRANS2(trans,
>>> _PSR_EVENT_TRANS_A)
>>> +#define _PSR_EVENT_TRANS_EDP			0x6f848
>>> +#define PSR_EVENT(tran)				_MMIO_TRANS2(tr
>>> an, _PSR_EVENT_TRANS_A)
>>>   #define  PSR_EVENT_PSR2_WD_TIMER_EXPIRE		(1 << 17)
>>>   #define  PSR_EVENT_PSR2_DISABLED		(1 << 16)
>>>   #define  PSR_EVENT_SU_DIRTY_FIFO_UNDERRUN	(1 << 15)
>>> @@ -4312,15 +4332,16 @@ enum {
>>>   #define  PSR_EVENT_LPSP_MODE_EXIT		(1 << 1)
>>>   #define  PSR_EVENT_PSR_DISABLE			(1 << 0)
>>>   
>>> -#define EDP_PSR2_STATUS			_MMIO(0x6f940)
>>> +#define _PSR2_STATUS_A			0x60940
>>> +#define _PSR2_STATUS_EDP		0x6f940
>>> +#define EDP_PSR2_STATUS(tran)		_MMIO_TRANS2(tran,
>>> _PSR2_STATUS_A)
>>>   #define EDP_PSR2_STATUS_STATE_MASK     (0xf << 28)
>>>   #define EDP_PSR2_STATUS_STATE_SHIFT    28
>>>   
>>> -#define _PSR2_SU_STATUS_0		0x6F914
>>> -#define _PSR2_SU_STATUS_1		0x6F918
>>> -#define _PSR2_SU_STATUS_2		0x6F91C
>>> -#define _PSR2_SU_STATUS(index)		_MMIO(_PICK_EVEN((index
>>> ), _PSR2_SU_STATUS_0, _PSR2_SU_STATUS_1))
>>> -#define PSR2_SU_STATUS(frame)		(_PSR2_SU_STATUS((frame
>>> ) / 3))
>>> +#define _PSR2_SU_STATUS_A		0x60914
>>> +#define _PSR2_SU_STATUS_EDP		0x6f914
>>> +#define _PSR2_SU_STATUS(tran, index)	_MMIO(_TRANS2(tran,
>>> _PSR2_SU_STATUS_A) + (index) * 4)
>>> +#define PSR2_SU_STATUS(tran, frame)	(_PSR2_SU_STATUS(tran,
>>> (frame) / 3))
>>>   #define PSR2_SU_STATUS_SHIFT(frame)	(((frame) % 3) * 10)
>>>   #define PSR2_SU_STATUS_MASK(frame)	(0x3ff <<
>>> PSR2_SU_STATUS_SHIFT(frame))
>>>   #define PSR2_SU_STATUS_FRAMES		8
>>> -- 
>>> 2.22.1
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Souza, Jose Aug. 22, 2019, 4:49 p.m. UTC | #4
On Thu, 2019-08-22 at 21:37 +0530, Gupta, Anshuman wrote:
> 
> On 8/22/2019 1:36 AM, Souza, Jose wrote:
> > On Wed, 2019-08-21 at 10:06 +0530, Anshuman Gupta wrote:
> > > On 2019-08-20 at 15:33:23 -0700, José Roberto de Souza wrote:
> > > > PSR registers are a mess, some have the full address while
> > > > others
> > > > just
> > > > have the additional offset from psr_mmio_base.
> > > > 
> > > > For BDW+ psr_mmio_base is nothing more than
> > > > TRANSCODER_EDP_OFFSET +
> > > > 0x800 and using it makes more difficult for people with an PSR
> > > > register address or PSR register name from from BSpec as i915
> > > > also
> > > > don't match the BSpec names.
> > > > For HSW psr_mmio_base is _DDI_BUF_CTL_A + 0x800 and PSR
> > > > registers
> > > > are
> > > > only available in DDIA.
> > > > 
> > > > Other reason to make relative to transcoder is that since BDW
> > > > every
> > > > transcoder have PSR registers, so in theory it should be
> > > > possible
> > > > to
> > > > have PSR enabled in a non-eDP transcoder.
> > > > 
> > > > So for BDW+ we can use _TRANS2() to get the register offset of
> > > > any
> > > > PSR register in any transcoder while for HSW we have
> > > > _HSW_PSR_ADJ
> > > > that will calculate the register offset for the single PSR
> > > > instance,
> > > > noting that we are already guarded about trying to enable PSR
> > > > in
> > > > other
> > > > port than DDIA on HSW by the 'if (dig_port->base.port !=
> > > > PORT_A)'
> > > > in
> > > > intel_psr_compute_config(), this check should only be valid for
> > > > HSW
> > > > and will be changed in future.
> > > > PSR2 registers and PSR_EVENT was added after Haswell so that is
> > > > why
> > > > _PSR_ADJ() is not used in some macros.
> > > > 
> > > > The only registers that can not be relative to transcoder are
> > > > PSR_IMR and PSR_IIR that are not relative to anything, so
> > > > keeping
> > > > it
> > > > hardcoded. That changed for TGL but it will be handled in
> > > > another
> > > > patch.
> > > > 
> > > > Also removing BDW_EDP_PSR_BASE from GVT because it is not used
> > > > as
> > > > it
> > > > is the only PSR register that GVT have.
> > > > 
> > > > v5:
> > > > - Macros changed to be more explicit about HSW (Dhinakaran)
> > > > - Squashed with the patch that added the tran parameter to the
> > > > macros (Dhinakaran)
> > > > 
> > > > v6:
> > > > - Checking for interruption errors after module reload in the
> > > > transcoder that will be used (Dhinakaran)
> > > > - Using lowercase to the registers offsets
> > > > 
> > > > v7:
> > > > - Removing IS_HASWELL() from registers macros(Jani)
> > > > 
> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Cc: Zhi Wang <zhi.a.wang@intel.com>
> > > > Reviewed-by: Lucas De Marchi <lucas.demarchi@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 | 104 +++++++++++++-
> > > > ---
> > > > ------
> > > >   drivers/gpu/drm/i915/gvt/handlers.c      |   2 +-
> > > >   drivers/gpu/drm/i915/i915_debugfs.c      |  18 ++--
> > > >   drivers/gpu/drm/i915/i915_drv.h          |   5 +-
> > > >   drivers/gpu/drm/i915/i915_reg.h          |  57 +++++++++----
> > > >   5 files changed, 113 insertions(+), 73 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > index 3bfb720560c2..77232f6bca17 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > @@ -390,7 +390,7 @@ static void hsw_psr_setup_aux(struct
> > > > intel_dp
> > > > *intel_dp)
> > > >   
> > > >   	BUILD_BUG_ON(sizeof(aux_msg) > 20);
> > > >   	for (i = 0; i < sizeof(aux_msg); i += 4)
> > > > -		I915_WRITE(EDP_PSR_AUX_DATA(i >> 2),
> > > > +		I915_WRITE(EDP_PSR_AUX_DATA(dev_priv-
> > > > >psr.transcoder, i
> > > > > > 2),
> > > >   			   intel_dp_pack_aux(&aux_msg[i],
> > > > sizeof(aux_msg) - i));
> > > >   
> > > >   	aux_clock_divider = intel_dp-
> > > > >get_aux_clock_divider(intel_dp,
> > > > 0);
> > > > @@ -401,7 +401,7 @@ static void hsw_psr_setup_aux(struct
> > > > intel_dp
> > > > *intel_dp)
> > > >   
> > > >   	/* Select only valid bits for SRD_AUX_CTL */
> > > >   	aux_ctl &= psr_aux_mask;
> > > > -	I915_WRITE(EDP_PSR_AUX_CTL, aux_ctl);
> > > > +	I915_WRITE(EDP_PSR_AUX_CTL(dev_priv->psr.transcoder),
> > > > aux_ctl);
> > > >   }
> > > >   
> > > >   static void intel_psr_enable_sink(struct intel_dp *intel_dp)
> > > > @@ -491,8 +491,9 @@ static void hsw_activate_psr1(struct
> > > > intel_dp
> > > > *intel_dp)
> > > >   	if (INTEL_GEN(dev_priv) >= 8)
> > > >   		val |= EDP_PSR_CRC_ENABLE;
> > > >   
> > > > -	val |= I915_READ(EDP_PSR_CTL) &
> > > > EDP_PSR_RESTORE_PSR_ACTIVE_CTX_MASK;
> > > > -	I915_WRITE(EDP_PSR_CTL, val);
> > > > +	val |= (I915_READ(EDP_PSR_CTL(dev_priv-
> > > > >psr.transcoder)) &
> > > > +		EDP_PSR_RESTORE_PSR_ACTIVE_CTX_MASK);
> > > > +	I915_WRITE(EDP_PSR_CTL(dev_priv->psr.transcoder), val);
> > > >   }
> > > >   
> > > >   static void hsw_activate_psr2(struct intel_dp *intel_dp)
> > > > @@ -528,9 +529,9 @@ static void hsw_activate_psr2(struct
> > > > intel_dp
> > > > *intel_dp)
> > > >   	 * PSR2 HW is incorrectly using EDP_PSR_TP1_TP3_SEL and
> > > > BSpec
> > > > is
> > > >   	 * recommending keep this bit unset while PSR2 is
> > > > enabled.
> > > >   	 */
> > > > -	I915_WRITE(EDP_PSR_CTL, 0);
> > > > +	I915_WRITE(EDP_PSR_CTL(dev_priv->psr.transcoder), 0);
> > > >   
> > > > -	I915_WRITE(EDP_PSR2_CTL, val);
> > > > +	I915_WRITE(EDP_PSR2_CTL(dev_priv->psr.transcoder),
> > > > val);
> > > >   }
> > > >   
> > > >   static bool intel_psr2_config_valid(struct intel_dp
> > > > *intel_dp,
> > > > @@ -606,10 +607,9 @@ void intel_psr_compute_config(struct
> > > > intel_dp
> > > > *intel_dp,
> > > >   
> > > >   	/*
> > > >   	 * HSW spec explicitly says PSR is tied to port A.
> > > > -	 * BDW+ platforms with DDI implementation of PSR have
> > > > different
> > > > -	 * PSR registers per transcoder and we only implement
> > > > transcoder EDP
> > > > -	 * ones. Since by Display design transcoder EDP is tied
> > > > to port
> > > > A
> > > > -	 * we can safely escape based on the port A.
> > > > +	 * BDW+ platforms have a instance of PSR registers per
> > > > transcoder but
> > > > +	 * for now it only supports one instance of PSR, so
> > > > lets keep
> > > > it
> > > > +	 * hardcoded to PORT_A
> > > >   	 */
> > > >   	if (dig_port->base.port != PORT_A) {
> > > >   		DRM_DEBUG_KMS("PSR condition failed: Port not
> > > > supported\n");
> > > > @@ -649,8 +649,8 @@ 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)
> > > > -		WARN_ON(I915_READ(EDP_PSR2_CTL) &
> > > > EDP_PSR2_ENABLE);
> > > > -	WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
> > > > +		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);
> > > >   	lockdep_assert_held(&dev_priv->psr.lock);
> > > >   
> > > > @@ -720,19 +720,37 @@ static void
> > > > intel_psr_enable_source(struct
> > > > intel_dp *intel_dp,
> > > >   	if (INTEL_GEN(dev_priv) < 11)
> > > >   		mask |= EDP_PSR_DEBUG_MASK_DISP_REG_WRITE;
> > > >   
> > > > -	I915_WRITE(EDP_PSR_DEBUG, mask);
> > > > +	I915_WRITE(EDP_PSR_DEBUG(dev_priv->psr.transcoder),
> > > > mask);
> > > >   }
> > > >   
> > > >   static void intel_psr_enable_locked(struct drm_i915_private
> > > > *dev_priv,
> > > >   				    const struct
> > > > intel_crtc_state
> > > > *crtc_state)
> > > >   {
> > > >   	struct intel_dp *intel_dp = dev_priv->psr.dp;
> > > > +	u32 val;
> > > >   
> > > >   	WARN_ON(dev_priv->psr.enabled);
> > > >   
> > > >   	dev_priv->psr.psr2_enabled =
> > > > intel_psr2_enabled(dev_priv,
> > > > crtc_state);
> > > >   	dev_priv->psr.busy_frontbuffer_bits = 0;
> > > >   	dev_priv->psr.pipe = to_intel_crtc(crtc_state-
> > > > >base.crtc)-
> > > > > pipe;
> > > > +	dev_priv->psr.transcoder = crtc_state->cpu_transcoder;
> > > > +
> > > > +	/*
> > > > +	 * If a PSR error happened and the driver is reloaded,
> > > > the
> > > > EDP_PSR_IIR
> > > > +	 * will still keep the error set even after the reset
> > > > done in
> > > > the
> > > > +	 * irq_preinstall and irq_uninstall hooks.
> > > > +	 * And enabling in this situation cause the screen to
> > > > freeze in
> > > > the
> > > > +	 * 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(edp_psr_shift(dev_priv-
> > > > >psr.transcoder));
> > > > +	if (val) {
> > > > +		dev_priv->psr.sink_not_reliable = true;
> > > > +		DRM_DEBUG_KMS("PSR interruption error set, not
> > > > enabling
> > > > PSR\n");
> > > > +		return;
> > > > +	}
> > > >   
> > > >   	DRM_DEBUG_KMS("Enabling PSR%s\n",
> > > >   		      dev_priv->psr.psr2_enabled ? "2" : "1");
> > > > @@ -782,20 +800,27 @@ static void intel_psr_exit(struct
> > > > drm_i915_private *dev_priv)
> > > >   	u32 val;
> > > >   
> > > >   	if (!dev_priv->psr.active) {
> > > > -		if (INTEL_GEN(dev_priv) >= 9)
> > > > -			WARN_ON(I915_READ(EDP_PSR2_CTL) &
> > > > EDP_PSR2_ENABLE);
> > > > -		WARN_ON(I915_READ(EDP_PSR_CTL) &
> > > > EDP_PSR_ENABLE);
> > > > +		if (INTEL_GEN(dev_priv) >= 9) {
> > > > +			val = I915_READ(EDP_PSR2_CTL(dev_priv-
> > > > > psr.transcoder));
> > > > +			WARN_ON(val & EDP_PSR2_ENABLE);
> > > > +		}
> > > > +
> > > > +		val = I915_READ(EDP_PSR_CTL(dev_priv-
> > > > >psr.transcoder));
> > > > +		WARN_ON(val & EDP_PSR_ENABLE);
> > > > +
> > > >   		return;
> > > >   	}
> > > >   
> > > >   	if (dev_priv->psr.psr2_enabled) {
> > > > -		val = I915_READ(EDP_PSR2_CTL);
> > > > +		val = I915_READ(EDP_PSR2_CTL(dev_priv-
> > > > > psr.transcoder));
> > > >   		WARN_ON(!(val & EDP_PSR2_ENABLE));
> > > > -		I915_WRITE(EDP_PSR2_CTL, val &
> > > > ~EDP_PSR2_ENABLE);
> > > > +		val &= ~EDP_PSR2_ENABLE;
> > > > +		I915_WRITE(EDP_PSR2_CTL(dev_priv-
> > > > >psr.transcoder),
> > > > val);
> > > >   	} else {
> > > > -		val = I915_READ(EDP_PSR_CTL);
> > > > +		val = I915_READ(EDP_PSR_CTL(dev_priv-
> > > > >psr.transcoder));
> > > >   		WARN_ON(!(val & EDP_PSR_ENABLE));
> > > > -		I915_WRITE(EDP_PSR_CTL, val & ~EDP_PSR_ENABLE);
> > > > +		val &= ~EDP_PSR_ENABLE;
> > > > +		I915_WRITE(EDP_PSR_CTL(dev_priv-
> > > > >psr.transcoder), val);
> > > >   	}
> > > >   	dev_priv->psr.active = false;
> > > >   }
> > > > @@ -817,10 +842,10 @@ static void
> > > > intel_psr_disable_locked(struct
> > > > intel_dp *intel_dp)
> > > >   	intel_psr_exit(dev_priv);
> > > >   
> > > >   	if (dev_priv->psr.psr2_enabled) {
> > > > -		psr_status = EDP_PSR2_STATUS;
> > > > +		psr_status = EDP_PSR2_STATUS(dev_priv-
> > > > >psr.transcoder);
> > > >   		psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
> > > >   	} else {
> > > > -		psr_status = EDP_PSR_STATUS;
> > > > +		psr_status = EDP_PSR_STATUS(dev_priv-
> > > > >psr.transcoder);
> > > >   		psr_status_mask = EDP_PSR_STATUS_STATE_MASK;
> > > >   	}
> > > >   
> > > > @@ -963,7 +988,8 @@ int intel_psr_wait_for_idle(const struct
> > > > intel_crtc_state *new_crtc_state,
> > > >   	 * defensive enough to cover everything.
> > > >   	 */
> > > >   
> > > > -	return __intel_wait_for_register(&dev_priv->uncore,
> > > > EDP_PSR_STATUS,
> > > > +	return __intel_wait_for_register(&dev_priv->uncore,
> > > > +					 EDP_PSR_STATUS(dev_pri
> > > > v-
> > > > > psr.transcoder),
> > > >   					 EDP_PSR_STATUS_STATE_M
> > > > ASK,
> > > >   					 EDP_PSR_STATUS_STATE_I
> > > > DLE, 2,
> > > > 50,
> > > >   					 out_value);
> > > > @@ -979,10 +1005,10 @@ static bool
> > > > __psr_wait_for_idle_locked(struct drm_i915_private *dev_priv)
> > > >   		return false;
> > > >   
> > > >   	if (dev_priv->psr.psr2_enabled) {
> > > > -		reg = EDP_PSR2_STATUS;
> > > > +		reg = EDP_PSR2_STATUS(dev_priv-
> > > > >psr.transcoder);
> > > >   		mask = EDP_PSR2_STATUS_STATE_MASK;
> > > >   	} else {
> > > > -		reg = EDP_PSR_STATUS;
> > > > +		reg = EDP_PSR_STATUS(dev_priv->psr.transcoder);
> > > >   		mask = EDP_PSR_STATUS_STATE_MASK;
> > > >   	}
> > > >   
> > > > @@ -1208,36 +1234,24 @@ void intel_psr_flush(struct
> > > > drm_i915_private *dev_priv,
> > > >    */
> > > >   void intel_psr_init(struct drm_i915_private *dev_priv)
> > > >   {
> > > > -	u32 val;
> > > > -
> > > >   	if (!HAS_PSR(dev_priv))
> > > >   		return;
> > > >   
> > > > -	dev_priv->psr_mmio_base = IS_HASWELL(dev_priv) ?
> > > > -		HSW_EDP_PSR_BASE : BDW_EDP_PSR_BASE;
> > > > -
> > > >   	if (!dev_priv->psr.sink_support)
> > > >   		return;
> > > >   
> > > > +	if (IS_HASWELL(dev_priv))
> > > > +		/*
> > > > +		 * HSW don't have PSR registers on the same
> > > > space as
> > > > transcoder
> > > > +		 * so set this to a value that when subtract to
> > > > the
> > > > register
> > > > +		 * in transcoder space results in the right
> > > > offset for
> > > > HSW
> > > > +		 */
> > > > +		dev_priv->hsw_psr_mmio_adjust = _SRD_CTL_EDP -
> > > > _HSW_EDP_PSR_BASE;
> > > > +
> > > >   	if (i915_modparams.enable_psr == -1)
> > > >   		if (INTEL_GEN(dev_priv) < 9 || !dev_priv-
> > > > > vbt.psr.enable)
> > > >   			i915_modparams.enable_psr = 0;
> > > >   
> > > > -	/*
> > > > -	 * If a PSR error happened and the driver is reloaded,
> > > > the
> > > > EDP_PSR_IIR
> > > > -	 * will still keep the error set even after the reset
> > > > done in
> > > > the
> > > > -	 * irq_preinstall and irq_uninstall hooks.
> > > > -	 * And enabling in this situation cause the screen to
> > > > freeze in
> > > > the
> > > > -	 * 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(edp_psr_shift(TRANSCODER_EDP));
> > > > -	if (val) {
> > > > -		DRM_DEBUG_KMS("PSR interruption error set\n");
> > > > -		dev_priv->psr.sink_not_reliable = true;
> > > > -	}
> > > > -
> > > Earlier EDP_PSR_IIR was being checked only in driver init path,
> > > now it has been checked for every modeset/fastset path in
> > > intel_psr_enable_locked(). Is it ok ?
> > > If it is justified why are we not checking it in
> > > intel_psr_flush()
> > > there also it enables psr.
> > 
> > I moved it primarily because on intel_psr_enable_locked() we have
> > the
> > transcoder that will be used, doing on init we would need to check
> > for
> > all transcoders and in case of a error set in one transcoder we
> > would
> > need to fail initialization as PSR could be enabled on that
> > transcoder.
> That is correct, but with this EDP_PSR_IIR error is getting checked
> in 
> every modeset/fastset which is redundant, as it is already being
> check 
> at interrupt handler.
> IMO it should be only checked somehow at initel_psr_init(), even
> comment 
> also reflect same thing ("If a PSR error happened and the driver is 
> reloaded...").
> If there no other way to handle this, comment should be change
> accordingly.

Only on full modesets, on real world it will happen only once at every
boot also read a register is not a expensive operation.

I will merge this as it is, if someone else also thinks that the
comment is necessary I will add on top.


> > No need to do that on intel_psr_flush() because PSR interruptions
> > can
> > be triggered even with PSR inactive.
> > 
> > 
> > > >   	/* Set link_standby x link_off defaults */
> > > >   	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> > > >   		/* HSW and BDW require workarounds that we
> > > > don't
> > > > implement. */
> > > > diff --git a/drivers/gpu/drm/i915/gvt/handlers.c
> > > > b/drivers/gpu/drm/i915/gvt/handlers.c
> > > > index 25f78196b964..45a9124e53b6 100644
> > > > --- a/drivers/gpu/drm/i915/gvt/handlers.c
> > > > +++ b/drivers/gpu/drm/i915/gvt/handlers.c
> > > > @@ -2796,7 +2796,7 @@ static int
> > > > init_broadwell_mmio_info(struct
> > > > intel_gvt *gvt)
> > > >   	MMIO_D(CHICKEN_PIPESL_1(PIPE_C), D_BDW_PLUS);
> > > >   
> > > >   	MMIO_D(WM_MISC, D_BDW);
> > > > -	MMIO_D(_MMIO(BDW_EDP_PSR_BASE), D_BDW);
> > > > +	MMIO_D(_MMIO(_SRD_CTL_EDP), D_BDW);
> > > >   
> > > >   	MMIO_D(_MMIO(0x6671c), D_BDW_PLUS);
> > > >   	MMIO_D(_MMIO(0x66c00), D_BDW_PLUS);
> > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > index b39226d7f8d2..6e4824daafae 100644
> > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > @@ -2132,7 +2132,7 @@ psr_source_status(struct drm_i915_private
> > > > *dev_priv, struct seq_file *m)
> > > >   			"BUF_ON",
> > > >   			"TG_ON"
> > > >   		};
> > > > -		val = I915_READ(EDP_PSR2_STATUS);
> > > > +		val = I915_READ(EDP_PSR2_STATUS(dev_priv-
> > > > > psr.transcoder));
> > > >   		status_val = (val & EDP_PSR2_STATUS_STATE_MASK)
> > > > >>
> > > >   			      EDP_PSR2_STATUS_STATE_SHIFT;
> > > >   		if (status_val < ARRAY_SIZE(live_status))
> > > > @@ -2148,7 +2148,7 @@ psr_source_status(struct drm_i915_private
> > > > *dev_priv, struct seq_file *m)
> > > >   			"SRDOFFACK",
> > > >   			"SRDENT_ON",
> > > >   		};
> > > > -		val = I915_READ(EDP_PSR_STATUS);
> > > > +		val = I915_READ(EDP_PSR_STATUS(dev_priv-
> > > > > psr.transcoder));
> > > >   		status_val = (val & EDP_PSR_STATUS_STATE_MASK)
> > > > >>
> > > >   			      EDP_PSR_STATUS_STATE_SHIFT;
> > > >   		if (status_val < ARRAY_SIZE(live_status))
> > > > @@ -2191,10 +2191,10 @@ static int i915_edp_psr_status(struct
> > > > seq_file *m, void *data)
> > > >   		goto unlock;
> > > >   
> > > >   	if (psr->psr2_enabled) {
> > > > -		val = I915_READ(EDP_PSR2_CTL);
> > > > +		val = I915_READ(EDP_PSR2_CTL(dev_priv-
> > > > > psr.transcoder));
> > > >   		enabled = val & EDP_PSR2_ENABLE;
> > > >   	} else {
> > > > -		val = I915_READ(EDP_PSR_CTL);
> > > > +		val = I915_READ(EDP_PSR_CTL(dev_priv-
> > > > >psr.transcoder));
> > > >   		enabled = val & EDP_PSR_ENABLE;
> > > >   	}
> > > >   	seq_printf(m, "Source PSR ctl: %s [0x%08x]\n",
> > > > @@ -2207,7 +2207,8 @@ static int i915_edp_psr_status(struct
> > > > seq_file *m, void *data)
> > > >   	 * SKL+ Perf counter is reset to 0 everytime DC state
> > > > is
> > > > entered
> > > >   	 */
> > > >   	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
> > > > -		val = I915_READ(EDP_PSR_PERF_CNT) &
> > > > EDP_PSR_PERF_CNT_MASK;
> > > > +		val = I915_READ(EDP_PSR_PERF_CNT(dev_priv-
> > > > > psr.transcoder));
> > > > +		val &= EDP_PSR_PERF_CNT_MASK;
> > > >   		seq_printf(m, "Performance counter: %u\n",
> > > > val);
> > > >   	}
> > > >   
> > > > @@ -2225,8 +2226,11 @@ static int i915_edp_psr_status(struct
> > > > seq_file *m, void *data)
> > > >   		 * Reading all 3 registers before hand to
> > > > minimize
> > > > crossing a
> > > >   		 * frame boundary between register reads
> > > >   		 */
> > > > -		for (frame = 0; frame < PSR2_SU_STATUS_FRAMES;
> > > > frame +=
> > > > 3)
> > > > -			su_frames_val[frame / 3] =
> > > > I915_READ(PSR2_SU_STATUS(frame));
> > > > +		for (frame = 0; frame < PSR2_SU_STATUS_FRAMES;
> > > > frame +=
> > > > 3) {
> > > > +			val =
> > > > I915_READ(PSR2_SU_STATUS(dev_priv-
> > > > > psr.transcoder,
> > > > +						       frame));
> > > > +			su_frames_val[frame / 3] = val;
> > > > +		}
> > > >   
> > > >   		seq_puts(m, "Frame:\tPSR2 SU blocks:\n");
> > > >   
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > index eb31c1656cea..be999791abca 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -479,6 +479,7 @@ struct i915_psr {
> > > >   	bool enabled;
> > > >   	struct intel_dp *dp;
> > > >   	enum pipe pipe;
> > > > +	enum transcoder transcoder;
> > > >   	bool active;
> > > >   	struct work_struct work;
> > > >   	unsigned busy_frontbuffer_bits;
> > > > @@ -1330,11 +1331,11 @@ struct drm_i915_private {
> > > >   	 */
> > > >   	u32 gpio_mmio_base;
> > > >   
> > > > +	u32 hsw_psr_mmio_adjust;
> > > > +
> > > >   	/* MMIO base address for MIPI regs */
> > > >   	u32 mipi_mmio_base;
> > > >   
> > > > -	u32 psr_mmio_base;
> > > > -
> > > >   	u32 pps_mmio_base;
> > > >   
> > > >   	wait_queue_head_t gmbus_wait_queue;
> > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > > b/drivers/gpu/drm/i915/i915_reg.h
> > > > index 2abd199093c5..a092b34c269d 100644
> > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > @@ -4186,10 +4186,17 @@ enum {
> > > >   #define PIPESRC(trans)		_MMIO_TRANS2(trans,
> > > > _PIPEASRC)
> > > >   #define PIPE_MULT(trans)	_MMIO_TRANS2(trans,
> > > > _PIPE_MULT_A)
> > > >   
> > > > -/* HSW+ eDP PSR registers */
> > > > -#define HSW_EDP_PSR_BASE	0x64800
> > > > -#define BDW_EDP_PSR_BASE	0x6f800
> > > > -#define EDP_PSR_CTL				_MMIO(dev_priv-
> > > > > psr_mmio_base + 0)
> > > > +/*
> > > > + * HSW+ eDP PSR registers
> > > > + *
> > > > + * HSW PSR registers are relative to DDIA(_DDI_BUF_CTL_A +
> > > > 0x800)
> > > > with just one
> > > > + * instance of it
> > > > + */
> > > > +#define _HSW_EDP_PSR_BASE			0x64800
> > > > +#define _SRD_CTL_A				0x60800
> > > > +#define _SRD_CTL_EDP				0x6f800
> > > > +#define _PSR_ADJ(tran, reg)			(_TRANS2(tran,
> > > > reg) - dev_priv->hsw_psr_mmio_adjust)
> > > > +#define EDP_PSR_CTL(tran)			_MMIO(_PSR_ADJ(
> > > > tran,
> > > > _SRD_CTL_A))
> > > >   #define   EDP_PSR_ENABLE			(1 << 31)
> > > >   #define   BDW_PSR_SINGLE_FRAME			(1 <<
> > > > 30)
> > > >   #define   EDP_PSR_RESTORE_PSR_ACTIVE_CTX_MASK	(1 << 29) /* SW
> > > > can't modify */
> > > > @@ -4226,16 +4233,22 @@ enum {
> > > >   #define   EDP_PSR_TRANSCODER_A_SHIFT		8
> > > >   #define   EDP_PSR_TRANSCODER_EDP_SHIFT		0
> > > >   
> > > > -#define EDP_PSR_AUX_CTL				_MMIO(d
> > > > ev_priv-
> > > > > psr_mmio_base + 0x10)
> > > > +#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))
> > > >   #define   EDP_PSR_AUX_CTL_TIME_OUT_MASK		(3 <<
> > > > 26)
> > > >   #define   EDP_PSR_AUX_CTL_MESSAGE_SIZE_MASK	(0x1f << 20)
> > > >   #define   EDP_PSR_AUX_CTL_PRECHARGE_2US_MASK	(0xf << 16)
> > > >   #define   EDP_PSR_AUX_CTL_ERROR_INTERRUPT	(1 << 11)
> > > >   #define   EDP_PSR_AUX_CTL_BIT_CLOCK_2X_MASK	(0x7ff)
> > > >   
> > > > -#define EDP_PSR_AUX_DATA(i)			_MMIO(dev_priv-
> > > > > psr_mmio_base + 0x14 + (i) * 4) /* 5 registers */
> > > > +#define _SRD_AUX_DATA_A				0x60814
> > > > +#define _SRD_AUX_DATA_EDP			0x6f814
> > > > +#define EDP_PSR_AUX_DATA(tran, i)		_MMIO(_PSR_ADJ(
> > > > tran,
> > > > _SRD_AUX_DATA_A) + (i) + 4) /* 5 registers */
> > > >   
> > > > -#define EDP_PSR_STATUS				_MMIO(dev_priv-
> > > > > psr_mmio_base + 0x40)
> > > > +#define _SRD_STATUS_A				0x60840
> > > > +#define _SRD_STATUS_EDP				0x6f840
> > > > +#define EDP_PSR_STATUS(tran)			_MMIO(_PSR_ADJ(
> > > > tran, _SRD_STATUS_A))
> > > >   #define   EDP_PSR_STATUS_STATE_MASK		(7 << 29)
> > > >   #define   EDP_PSR_STATUS_STATE_SHIFT		29
> > > >   #define   EDP_PSR_STATUS_STATE_IDLE		(0 << 29)
> > > > @@ -4260,10 +4273,15 @@ enum {
> > > >   #define   EDP_PSR_STATUS_SENDING_TP1		(1 << 4)
> > > >   #define   EDP_PSR_STATUS_IDLE_MASK		0xf
> > > >   
> > > > -#define EDP_PSR_PERF_CNT		_MMIO(dev_priv-
> > > > >psr_mmio_base +
> > > > 0x44)
> > > > +#define _SRD_PERF_CNT_A			0x60844
> > > > +#define _SRD_PERF_CNT_EDP		0x6f844
> > > > +#define EDP_PSR_PERF_CNT(tran)		_MMIO(_PSR_ADJ(tran,
> > > > _SRD_PERF_CNT_A))
> > > >   #define   EDP_PSR_PERF_CNT_MASK		0xffffff
> > > >   
> > > > -#define EDP_PSR_DEBUG				_MMIO(dev_priv-
> > > > > psr_mmio_base + 0x60) /* PSR_MASK on SKL+ */
> > > > +/* PSR_MASK on SKL+ */
> > > > +#define _SRD_DEBUG_A				0x60860
> > > > +#define _SRD_DEBUG_EDP				0x6f860
> > > > +#define EDP_PSR_DEBUG(tran)			_MMIO(_PSR_ADJ(
> > > > tran, _SRD_DEBUG_A))
> > > >   #define   EDP_PSR_DEBUG_MASK_MAX_SLEEP         (1 << 28)
> > > >   #define   EDP_PSR_DEBUG_MASK_LPSP              (1 << 27)
> > > >   #define   EDP_PSR_DEBUG_MASK_MEMUP             (1 << 26)
> > > > @@ -4271,7 +4289,9 @@ enum {
> > > >   #define   EDP_PSR_DEBUG_MASK_DISP_REG_WRITE    (1 << 16) /*
> > > > Reserved in ICL+ */
> > > >   #define   EDP_PSR_DEBUG_EXIT_ON_PIXEL_UNDERRUN (1 << 15) /*
> > > > SKL+
> > > > */
> > > >   
> > > > -#define EDP_PSR2_CTL			_MMIO(0x6f900)
> > > > +#define _PSR2_CTL_A			0x60900
> > > > +#define _PSR2_CTL_EDP			0x6f900
> > > > +#define EDP_PSR2_CTL(tran)		_MMIO_TRANS2(tran,
> > > > _PSR2_CTL_A)
> > > >   #define   EDP_PSR2_ENABLE		(1 << 31)
> > > >   #define   EDP_SU_TRACK_ENABLE		(1 << 30)
> > > >   #define   EDP_Y_COORDINATE_VALID	(1 << 26) /* GLK and
> > > > CNL+ */
> > > > @@ -4293,8 +4313,8 @@ enum {
> > > >   #define _PSR_EVENT_TRANS_B			0x61848
> > > >   #define _PSR_EVENT_TRANS_C			0x62848
> > > >   #define _PSR_EVENT_TRANS_D			0x63848
> > > > -#define _PSR_EVENT_TRANS_EDP			0x6F848
> > > > -#define PSR_EVENT(trans)			_MMIO_TRANS2(tr
> > > > ans,
> > > > _PSR_EVENT_TRANS_A)
> > > > +#define _PSR_EVENT_TRANS_EDP			0x6f848
> > > > +#define PSR_EVENT(tran)				_MMIO_T
> > > > RANS2(tr
> > > > an, _PSR_EVENT_TRANS_A)
> > > >   #define  PSR_EVENT_PSR2_WD_TIMER_EXPIRE		(1 <<
> > > > 17)
> > > >   #define  PSR_EVENT_PSR2_DISABLED		(1 << 16)
> > > >   #define  PSR_EVENT_SU_DIRTY_FIFO_UNDERRUN	(1 << 15)
> > > > @@ -4312,15 +4332,16 @@ enum {
> > > >   #define  PSR_EVENT_LPSP_MODE_EXIT		(1 << 1)
> > > >   #define  PSR_EVENT_PSR_DISABLE			(1 <<
> > > > 0)
> > > >   
> > > > -#define EDP_PSR2_STATUS			_MMIO(0x6f940)
> > > > +#define _PSR2_STATUS_A			0x60940
> > > > +#define _PSR2_STATUS_EDP		0x6f940
> > > > +#define EDP_PSR2_STATUS(tran)		_MMIO_TRANS2(tran,
> > > > _PSR2_STATUS_A)
> > > >   #define EDP_PSR2_STATUS_STATE_MASK     (0xf << 28)
> > > >   #define EDP_PSR2_STATUS_STATE_SHIFT    28
> > > >   
> > > > -#define _PSR2_SU_STATUS_0		0x6F914
> > > > -#define _PSR2_SU_STATUS_1		0x6F918
> > > > -#define _PSR2_SU_STATUS_2		0x6F91C
> > > > -#define _PSR2_SU_STATUS(index)		_MMIO(_PICK_EVEN((index
> > > > ), _PSR2_SU_STATUS_0, _PSR2_SU_STATUS_1))
> > > > -#define PSR2_SU_STATUS(frame)		(_PSR2_SU_STATUS((frame
> > > > ) / 3))
> > > > +#define _PSR2_SU_STATUS_A		0x60914
> > > > +#define _PSR2_SU_STATUS_EDP		0x6f914
> > > > +#define _PSR2_SU_STATUS(tran, index)	_MMIO(_TRANS2(tran,
> > > > _PSR2_SU_STATUS_A) + (index) * 4)
> > > > +#define PSR2_SU_STATUS(tran, frame)	(_PSR2_SU_STATUS(tran,
> > > > (frame) / 3))
> > > >   #define PSR2_SU_STATUS_SHIFT(frame)	(((frame) % 3) * 10)
> > > >   #define PSR2_SU_STATUS_MASK(frame)	(0x3ff <<
> > > > PSR2_SU_STATUS_SHIFT(frame))
> > > >   #define PSR2_SU_STATUS_FRAMES		8
> > > > -- 
> > > > 2.22.1
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Anshuman Gupta Aug. 22, 2019, 5:09 p.m. UTC | #5
On 8/22/2019 10:19 PM, Souza, Jose wrote:
> On Thu, 2019-08-22 at 21:37 +0530, Gupta, Anshuman wrote:
>>
>> On 8/22/2019 1:36 AM, Souza, Jose wrote:
>>> On Wed, 2019-08-21 at 10:06 +0530, Anshuman Gupta wrote:
>>>> On 2019-08-20 at 15:33:23 -0700, José Roberto de Souza wrote:
>>>>> PSR registers are a mess, some have the full address while
>>>>> others
>>>>> just
>>>>> have the additional offset from psr_mmio_base.
>>>>>
>>>>> For BDW+ psr_mmio_base is nothing more than
>>>>> TRANSCODER_EDP_OFFSET +
>>>>> 0x800 and using it makes more difficult for people with an PSR
>>>>> register address or PSR register name from from BSpec as i915
>>>>> also
>>>>> don't match the BSpec names.
>>>>> For HSW psr_mmio_base is _DDI_BUF_CTL_A + 0x800 and PSR
>>>>> registers
>>>>> are
>>>>> only available in DDIA.
>>>>>
>>>>> Other reason to make relative to transcoder is that since BDW
>>>>> every
>>>>> transcoder have PSR registers, so in theory it should be
>>>>> possible
>>>>> to
>>>>> have PSR enabled in a non-eDP transcoder.
>>>>>
>>>>> So for BDW+ we can use _TRANS2() to get the register offset of
>>>>> any
>>>>> PSR register in any transcoder while for HSW we have
>>>>> _HSW_PSR_ADJ
>>>>> that will calculate the register offset for the single PSR
>>>>> instance,
>>>>> noting that we are already guarded about trying to enable PSR
>>>>> in
>>>>> other
>>>>> port than DDIA on HSW by the 'if (dig_port->base.port !=
>>>>> PORT_A)'
>>>>> in
>>>>> intel_psr_compute_config(), this check should only be valid for
>>>>> HSW
>>>>> and will be changed in future.
>>>>> PSR2 registers and PSR_EVENT was added after Haswell so that is
>>>>> why
>>>>> _PSR_ADJ() is not used in some macros.
>>>>>
>>>>> The only registers that can not be relative to transcoder are
>>>>> PSR_IMR and PSR_IIR that are not relative to anything, so
>>>>> keeping
>>>>> it
>>>>> hardcoded. That changed for TGL but it will be handled in
>>>>> another
>>>>> patch.
>>>>>
>>>>> Also removing BDW_EDP_PSR_BASE from GVT because it is not used
>>>>> as
>>>>> it
>>>>> is the only PSR register that GVT have.
>>>>>
>>>>> v5:
>>>>> - Macros changed to be more explicit about HSW (Dhinakaran)
>>>>> - Squashed with the patch that added the tran parameter to the
>>>>> macros (Dhinakaran)
>>>>>
>>>>> v6:
>>>>> - Checking for interruption errors after module reload in the
>>>>> transcoder that will be used (Dhinakaran)
>>>>> - Using lowercase to the registers offsets
>>>>>
>>>>> v7:
>>>>> - Removing IS_HASWELL() from registers macros(Jani)
>>>>>
>>>>> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>>>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>>>> Cc: Jani Nikula <jani.nikula@linux.intel.com>
>>>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>> Cc: Zhi Wang <zhi.a.wang@intel.com>
>>>>> Reviewed-by: Lucas De Marchi <lucas.demarchi@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 | 104 +++++++++++++-
>>>>> ---
>>>>> ------
>>>>>    drivers/gpu/drm/i915/gvt/handlers.c      |   2 +-
>>>>>    drivers/gpu/drm/i915/i915_debugfs.c      |  18 ++--
>>>>>    drivers/gpu/drm/i915/i915_drv.h          |   5 +-
>>>>>    drivers/gpu/drm/i915/i915_reg.h          |  57 +++++++++----
>>>>>    5 files changed, 113 insertions(+), 73 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
>>>>> b/drivers/gpu/drm/i915/display/intel_psr.c
>>>>> index 3bfb720560c2..77232f6bca17 100644
>>>>> --- a/drivers/gpu/drm/i915/display/intel_psr.c
>>>>> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
>>>>> @@ -390,7 +390,7 @@ static void hsw_psr_setup_aux(struct
>>>>> intel_dp
>>>>> *intel_dp)
>>>>>    
>>>>>    	BUILD_BUG_ON(sizeof(aux_msg) > 20);
>>>>>    	for (i = 0; i < sizeof(aux_msg); i += 4)
>>>>> -		I915_WRITE(EDP_PSR_AUX_DATA(i >> 2),
>>>>> +		I915_WRITE(EDP_PSR_AUX_DATA(dev_priv-
>>>>>> psr.transcoder, i
>>>>>>> 2),
>>>>>    			   intel_dp_pack_aux(&aux_msg[i],
>>>>> sizeof(aux_msg) - i));
>>>>>    
>>>>>    	aux_clock_divider = intel_dp-
>>>>>> get_aux_clock_divider(intel_dp,
>>>>> 0);
>>>>> @@ -401,7 +401,7 @@ static void hsw_psr_setup_aux(struct
>>>>> intel_dp
>>>>> *intel_dp)
>>>>>    
>>>>>    	/* Select only valid bits for SRD_AUX_CTL */
>>>>>    	aux_ctl &= psr_aux_mask;
>>>>> -	I915_WRITE(EDP_PSR_AUX_CTL, aux_ctl);
>>>>> +	I915_WRITE(EDP_PSR_AUX_CTL(dev_priv->psr.transcoder),
>>>>> aux_ctl);
>>>>>    }
>>>>>    
>>>>>    static void intel_psr_enable_sink(struct intel_dp *intel_dp)
>>>>> @@ -491,8 +491,9 @@ static void hsw_activate_psr1(struct
>>>>> intel_dp
>>>>> *intel_dp)
>>>>>    	if (INTEL_GEN(dev_priv) >= 8)
>>>>>    		val |= EDP_PSR_CRC_ENABLE;
>>>>>    
>>>>> -	val |= I915_READ(EDP_PSR_CTL) &
>>>>> EDP_PSR_RESTORE_PSR_ACTIVE_CTX_MASK;
>>>>> -	I915_WRITE(EDP_PSR_CTL, val);
>>>>> +	val |= (I915_READ(EDP_PSR_CTL(dev_priv-
>>>>>> psr.transcoder)) &
>>>>> +		EDP_PSR_RESTORE_PSR_ACTIVE_CTX_MASK);
>>>>> +	I915_WRITE(EDP_PSR_CTL(dev_priv->psr.transcoder), val);
>>>>>    }
>>>>>    
>>>>>    static void hsw_activate_psr2(struct intel_dp *intel_dp)
>>>>> @@ -528,9 +529,9 @@ static void hsw_activate_psr2(struct
>>>>> intel_dp
>>>>> *intel_dp)
>>>>>    	 * PSR2 HW is incorrectly using EDP_PSR_TP1_TP3_SEL and
>>>>> BSpec
>>>>> is
>>>>>    	 * recommending keep this bit unset while PSR2 is
>>>>> enabled.
>>>>>    	 */
>>>>> -	I915_WRITE(EDP_PSR_CTL, 0);
>>>>> +	I915_WRITE(EDP_PSR_CTL(dev_priv->psr.transcoder), 0);
>>>>>    
>>>>> -	I915_WRITE(EDP_PSR2_CTL, val);
>>>>> +	I915_WRITE(EDP_PSR2_CTL(dev_priv->psr.transcoder),
>>>>> val);
>>>>>    }
>>>>>    
>>>>>    static bool intel_psr2_config_valid(struct intel_dp
>>>>> *intel_dp,
>>>>> @@ -606,10 +607,9 @@ void intel_psr_compute_config(struct
>>>>> intel_dp
>>>>> *intel_dp,
>>>>>    
>>>>>    	/*
>>>>>    	 * HSW spec explicitly says PSR is tied to port A.
>>>>> -	 * BDW+ platforms with DDI implementation of PSR have
>>>>> different
>>>>> -	 * PSR registers per transcoder and we only implement
>>>>> transcoder EDP
>>>>> -	 * ones. Since by Display design transcoder EDP is tied
>>>>> to port
>>>>> A
>>>>> -	 * we can safely escape based on the port A.
>>>>> +	 * BDW+ platforms have a instance of PSR registers per
>>>>> transcoder but
>>>>> +	 * for now it only supports one instance of PSR, so
>>>>> lets keep
>>>>> it
>>>>> +	 * hardcoded to PORT_A
>>>>>    	 */
>>>>>    	if (dig_port->base.port != PORT_A) {
>>>>>    		DRM_DEBUG_KMS("PSR condition failed: Port not
>>>>> supported\n");
>>>>> @@ -649,8 +649,8 @@ 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)
>>>>> -		WARN_ON(I915_READ(EDP_PSR2_CTL) &
>>>>> EDP_PSR2_ENABLE);
>>>>> -	WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
>>>>> +		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);
>>>>>    	lockdep_assert_held(&dev_priv->psr.lock);
>>>>>    
>>>>> @@ -720,19 +720,37 @@ static void
>>>>> intel_psr_enable_source(struct
>>>>> intel_dp *intel_dp,
>>>>>    	if (INTEL_GEN(dev_priv) < 11)
>>>>>    		mask |= EDP_PSR_DEBUG_MASK_DISP_REG_WRITE;
>>>>>    
>>>>> -	I915_WRITE(EDP_PSR_DEBUG, mask);
>>>>> +	I915_WRITE(EDP_PSR_DEBUG(dev_priv->psr.transcoder),
>>>>> mask);
>>>>>    }
>>>>>    
>>>>>    static void intel_psr_enable_locked(struct drm_i915_private
>>>>> *dev_priv,
>>>>>    				    const struct
>>>>> intel_crtc_state
>>>>> *crtc_state)
>>>>>    {
>>>>>    	struct intel_dp *intel_dp = dev_priv->psr.dp;
>>>>> +	u32 val;
>>>>>    
>>>>>    	WARN_ON(dev_priv->psr.enabled);
>>>>>    
>>>>>    	dev_priv->psr.psr2_enabled =
>>>>> intel_psr2_enabled(dev_priv,
>>>>> crtc_state);
>>>>>    	dev_priv->psr.busy_frontbuffer_bits = 0;
>>>>>    	dev_priv->psr.pipe = to_intel_crtc(crtc_state-
>>>>>> base.crtc)-
>>>>>> pipe;
>>>>> +	dev_priv->psr.transcoder = crtc_state->cpu_transcoder;
>>>>> +
>>>>> +	/*
>>>>> +	 * If a PSR error happened and the driver is reloaded,
>>>>> the
>>>>> EDP_PSR_IIR
>>>>> +	 * will still keep the error set even after the reset
>>>>> done in
>>>>> the
>>>>> +	 * irq_preinstall and irq_uninstall hooks.
>>>>> +	 * And enabling in this situation cause the screen to
>>>>> freeze in
>>>>> the
>>>>> +	 * 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(edp_psr_shift(dev_priv-
>>>>>> psr.transcoder));
>>>>> +	if (val) {
>>>>> +		dev_priv->psr.sink_not_reliable = true;
>>>>> +		DRM_DEBUG_KMS("PSR interruption error set, not
>>>>> enabling
>>>>> PSR\n");
>>>>> +		return;
>>>>> +	}
>>>>>    
>>>>>    	DRM_DEBUG_KMS("Enabling PSR%s\n",
>>>>>    		      dev_priv->psr.psr2_enabled ? "2" : "1");
>>>>> @@ -782,20 +800,27 @@ static void intel_psr_exit(struct
>>>>> drm_i915_private *dev_priv)
>>>>>    	u32 val;
>>>>>    
>>>>>    	if (!dev_priv->psr.active) {
>>>>> -		if (INTEL_GEN(dev_priv) >= 9)
>>>>> -			WARN_ON(I915_READ(EDP_PSR2_CTL) &
>>>>> EDP_PSR2_ENABLE);
>>>>> -		WARN_ON(I915_READ(EDP_PSR_CTL) &
>>>>> EDP_PSR_ENABLE);
>>>>> +		if (INTEL_GEN(dev_priv) >= 9) {
>>>>> +			val = I915_READ(EDP_PSR2_CTL(dev_priv-
>>>>>> psr.transcoder));
>>>>> +			WARN_ON(val & EDP_PSR2_ENABLE);
>>>>> +		}
>>>>> +
>>>>> +		val = I915_READ(EDP_PSR_CTL(dev_priv-
>>>>>> psr.transcoder));
>>>>> +		WARN_ON(val & EDP_PSR_ENABLE);
>>>>> +
>>>>>    		return;
>>>>>    	}
>>>>>    
>>>>>    	if (dev_priv->psr.psr2_enabled) {
>>>>> -		val = I915_READ(EDP_PSR2_CTL);
>>>>> +		val = I915_READ(EDP_PSR2_CTL(dev_priv-
>>>>>> psr.transcoder));
>>>>>    		WARN_ON(!(val & EDP_PSR2_ENABLE));
>>>>> -		I915_WRITE(EDP_PSR2_CTL, val &
>>>>> ~EDP_PSR2_ENABLE);
>>>>> +		val &= ~EDP_PSR2_ENABLE;
>>>>> +		I915_WRITE(EDP_PSR2_CTL(dev_priv-
>>>>>> psr.transcoder),
>>>>> val);
>>>>>    	} else {
>>>>> -		val = I915_READ(EDP_PSR_CTL);
>>>>> +		val = I915_READ(EDP_PSR_CTL(dev_priv-
>>>>>> psr.transcoder));
>>>>>    		WARN_ON(!(val & EDP_PSR_ENABLE));
>>>>> -		I915_WRITE(EDP_PSR_CTL, val & ~EDP_PSR_ENABLE);
>>>>> +		val &= ~EDP_PSR_ENABLE;
>>>>> +		I915_WRITE(EDP_PSR_CTL(dev_priv-
>>>>>> psr.transcoder), val);
>>>>>    	}
>>>>>    	dev_priv->psr.active = false;
>>>>>    }
>>>>> @@ -817,10 +842,10 @@ static void
>>>>> intel_psr_disable_locked(struct
>>>>> intel_dp *intel_dp)
>>>>>    	intel_psr_exit(dev_priv);
>>>>>    
>>>>>    	if (dev_priv->psr.psr2_enabled) {
>>>>> -		psr_status = EDP_PSR2_STATUS;
>>>>> +		psr_status = EDP_PSR2_STATUS(dev_priv-
>>>>>> psr.transcoder);
>>>>>    		psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
>>>>>    	} else {
>>>>> -		psr_status = EDP_PSR_STATUS;
>>>>> +		psr_status = EDP_PSR_STATUS(dev_priv-
>>>>>> psr.transcoder);
>>>>>    		psr_status_mask = EDP_PSR_STATUS_STATE_MASK;
>>>>>    	}
>>>>>    
>>>>> @@ -963,7 +988,8 @@ int intel_psr_wait_for_idle(const struct
>>>>> intel_crtc_state *new_crtc_state,
>>>>>    	 * defensive enough to cover everything.
>>>>>    	 */
>>>>>    
>>>>> -	return __intel_wait_for_register(&dev_priv->uncore,
>>>>> EDP_PSR_STATUS,
>>>>> +	return __intel_wait_for_register(&dev_priv->uncore,
>>>>> +					 EDP_PSR_STATUS(dev_pri
>>>>> v-
>>>>>> psr.transcoder),
>>>>>    					 EDP_PSR_STATUS_STATE_M
>>>>> ASK,
>>>>>    					 EDP_PSR_STATUS_STATE_I
>>>>> DLE, 2,
>>>>> 50,
>>>>>    					 out_value);
>>>>> @@ -979,10 +1005,10 @@ static bool
>>>>> __psr_wait_for_idle_locked(struct drm_i915_private *dev_priv)
>>>>>    		return false;
>>>>>    
>>>>>    	if (dev_priv->psr.psr2_enabled) {
>>>>> -		reg = EDP_PSR2_STATUS;
>>>>> +		reg = EDP_PSR2_STATUS(dev_priv-
>>>>>> psr.transcoder);
>>>>>    		mask = EDP_PSR2_STATUS_STATE_MASK;
>>>>>    	} else {
>>>>> -		reg = EDP_PSR_STATUS;
>>>>> +		reg = EDP_PSR_STATUS(dev_priv->psr.transcoder);
>>>>>    		mask = EDP_PSR_STATUS_STATE_MASK;
>>>>>    	}
>>>>>    
>>>>> @@ -1208,36 +1234,24 @@ void intel_psr_flush(struct
>>>>> drm_i915_private *dev_priv,
>>>>>     */
>>>>>    void intel_psr_init(struct drm_i915_private *dev_priv)
>>>>>    {
>>>>> -	u32 val;
>>>>> -
>>>>>    	if (!HAS_PSR(dev_priv))
>>>>>    		return;
>>>>>    
>>>>> -	dev_priv->psr_mmio_base = IS_HASWELL(dev_priv) ?
>>>>> -		HSW_EDP_PSR_BASE : BDW_EDP_PSR_BASE;
>>>>> -
>>>>>    	if (!dev_priv->psr.sink_support)
>>>>>    		return;
>>>>>    
>>>>> +	if (IS_HASWELL(dev_priv))
>>>>> +		/*
>>>>> +		 * HSW don't have PSR registers on the same
>>>>> space as
>>>>> transcoder
>>>>> +		 * so set this to a value that when subtract to
>>>>> the
>>>>> register
>>>>> +		 * in transcoder space results in the right
>>>>> offset for
>>>>> HSW
>>>>> +		 */
>>>>> +		dev_priv->hsw_psr_mmio_adjust = _SRD_CTL_EDP -
>>>>> _HSW_EDP_PSR_BASE;
>>>>> +
>>>>>    	if (i915_modparams.enable_psr == -1)
>>>>>    		if (INTEL_GEN(dev_priv) < 9 || !dev_priv-
>>>>>> vbt.psr.enable)
>>>>>    			i915_modparams.enable_psr = 0;
>>>>>    
>>>>> -	/*
>>>>> -	 * If a PSR error happened and the driver is reloaded,
>>>>> the
>>>>> EDP_PSR_IIR
>>>>> -	 * will still keep the error set even after the reset
>>>>> done in
>>>>> the
>>>>> -	 * irq_preinstall and irq_uninstall hooks.
>>>>> -	 * And enabling in this situation cause the screen to
>>>>> freeze in
>>>>> the
>>>>> -	 * 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(edp_psr_shift(TRANSCODER_EDP));
>>>>> -	if (val) {
>>>>> -		DRM_DEBUG_KMS("PSR interruption error set\n");
>>>>> -		dev_priv->psr.sink_not_reliable = true;
>>>>> -	}
>>>>> -
>>>> Earlier EDP_PSR_IIR was being checked only in driver init path,
>>>> now it has been checked for every modeset/fastset path in
>>>> intel_psr_enable_locked(). Is it ok ?
>>>> If it is justified why are we not checking it in
>>>> intel_psr_flush()
>>>> there also it enables psr.
>>>
>>> I moved it primarily because on intel_psr_enable_locked() we have
>>> the
>>> transcoder that will be used, doing on init we would need to check
>>> for
>>> all transcoders and in case of a error set in one transcoder we
>>> would
>>> need to fail initialization as PSR could be enabled on that
>>> transcoder.
>> That is correct, but with this EDP_PSR_IIR error is getting checked
>> in
>> every modeset/fastset which is redundant, as it is already being
>> check
>> at interrupt handler.
>> IMO it should be only checked somehow at initel_psr_init(), even
>> comment
>> also reflect same thing ("If a PSR error happened and the driver is
>> reloaded...").
>> If there no other way to handle this, comment should be change
>> accordingly.
> 
> Only on full modesets, on real world it will happen only once at every
AFAIU intel_psr_update() may also call intel_psr_enable_locked(), which 
means even on fastset this this will get checked ?
> boot also read a register is not a expensive operation.
> 
> I will merge this as it is, if someone else also thinks that the
> comment is necessary I will add on top.
> 
> 
>>> No need to do that on intel_psr_flush() because PSR interruptions
>>> can
>>> be triggered even with PSR inactive.
>>>
>>>
>>>>>    	/* Set link_standby x link_off defaults */
>>>>>    	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
>>>>>    		/* HSW and BDW require workarounds that we
>>>>> don't
>>>>> implement. */
>>>>> diff --git a/drivers/gpu/drm/i915/gvt/handlers.c
>>>>> b/drivers/gpu/drm/i915/gvt/handlers.c
>>>>> index 25f78196b964..45a9124e53b6 100644
>>>>> --- a/drivers/gpu/drm/i915/gvt/handlers.c
>>>>> +++ b/drivers/gpu/drm/i915/gvt/handlers.c
>>>>> @@ -2796,7 +2796,7 @@ static int
>>>>> init_broadwell_mmio_info(struct
>>>>> intel_gvt *gvt)
>>>>>    	MMIO_D(CHICKEN_PIPESL_1(PIPE_C), D_BDW_PLUS);
>>>>>    
>>>>>    	MMIO_D(WM_MISC, D_BDW);
>>>>> -	MMIO_D(_MMIO(BDW_EDP_PSR_BASE), D_BDW);
>>>>> +	MMIO_D(_MMIO(_SRD_CTL_EDP), D_BDW);
>>>>>    
>>>>>    	MMIO_D(_MMIO(0x6671c), D_BDW_PLUS);
>>>>>    	MMIO_D(_MMIO(0x66c00), D_BDW_PLUS);
>>>>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
>>>>> b/drivers/gpu/drm/i915/i915_debugfs.c
>>>>> index b39226d7f8d2..6e4824daafae 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>>>>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>>>>> @@ -2132,7 +2132,7 @@ psr_source_status(struct drm_i915_private
>>>>> *dev_priv, struct seq_file *m)
>>>>>    			"BUF_ON",
>>>>>    			"TG_ON"
>>>>>    		};
>>>>> -		val = I915_READ(EDP_PSR2_STATUS);
>>>>> +		val = I915_READ(EDP_PSR2_STATUS(dev_priv-
>>>>>> psr.transcoder));
>>>>>    		status_val = (val & EDP_PSR2_STATUS_STATE_MASK)
>>>>>>>
>>>>>    			      EDP_PSR2_STATUS_STATE_SHIFT;
>>>>>    		if (status_val < ARRAY_SIZE(live_status))
>>>>> @@ -2148,7 +2148,7 @@ psr_source_status(struct drm_i915_private
>>>>> *dev_priv, struct seq_file *m)
>>>>>    			"SRDOFFACK",
>>>>>    			"SRDENT_ON",
>>>>>    		};
>>>>> -		val = I915_READ(EDP_PSR_STATUS);
>>>>> +		val = I915_READ(EDP_PSR_STATUS(dev_priv-
>>>>>> psr.transcoder));
>>>>>    		status_val = (val & EDP_PSR_STATUS_STATE_MASK)
>>>>>>>
>>>>>    			      EDP_PSR_STATUS_STATE_SHIFT;
>>>>>    		if (status_val < ARRAY_SIZE(live_status))
>>>>> @@ -2191,10 +2191,10 @@ static int i915_edp_psr_status(struct
>>>>> seq_file *m, void *data)
>>>>>    		goto unlock;
>>>>>    
>>>>>    	if (psr->psr2_enabled) {
>>>>> -		val = I915_READ(EDP_PSR2_CTL);
>>>>> +		val = I915_READ(EDP_PSR2_CTL(dev_priv-
>>>>>> psr.transcoder));
>>>>>    		enabled = val & EDP_PSR2_ENABLE;
>>>>>    	} else {
>>>>> -		val = I915_READ(EDP_PSR_CTL);
>>>>> +		val = I915_READ(EDP_PSR_CTL(dev_priv-
>>>>>> psr.transcoder));
>>>>>    		enabled = val & EDP_PSR_ENABLE;
>>>>>    	}
>>>>>    	seq_printf(m, "Source PSR ctl: %s [0x%08x]\n",
>>>>> @@ -2207,7 +2207,8 @@ static int i915_edp_psr_status(struct
>>>>> seq_file *m, void *data)
>>>>>    	 * SKL+ Perf counter is reset to 0 everytime DC state
>>>>> is
>>>>> entered
>>>>>    	 */
>>>>>    	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
>>>>> -		val = I915_READ(EDP_PSR_PERF_CNT) &
>>>>> EDP_PSR_PERF_CNT_MASK;
>>>>> +		val = I915_READ(EDP_PSR_PERF_CNT(dev_priv-
>>>>>> psr.transcoder));
>>>>> +		val &= EDP_PSR_PERF_CNT_MASK;
>>>>>    		seq_printf(m, "Performance counter: %u\n",
>>>>> val);
>>>>>    	}
>>>>>    
>>>>> @@ -2225,8 +2226,11 @@ static int i915_edp_psr_status(struct
>>>>> seq_file *m, void *data)
>>>>>    		 * Reading all 3 registers before hand to
>>>>> minimize
>>>>> crossing a
>>>>>    		 * frame boundary between register reads
>>>>>    		 */
>>>>> -		for (frame = 0; frame < PSR2_SU_STATUS_FRAMES;
>>>>> frame +=
>>>>> 3)
>>>>> -			su_frames_val[frame / 3] =
>>>>> I915_READ(PSR2_SU_STATUS(frame));
>>>>> +		for (frame = 0; frame < PSR2_SU_STATUS_FRAMES;
>>>>> frame +=
>>>>> 3) {
>>>>> +			val =
>>>>> I915_READ(PSR2_SU_STATUS(dev_priv-
>>>>>> psr.transcoder,
>>>>> +						       frame));
>>>>> +			su_frames_val[frame / 3] = val;
>>>>> +		}
>>>>>    
>>>>>    		seq_puts(m, "Frame:\tPSR2 SU blocks:\n");
>>>>>    
>>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>>>>> b/drivers/gpu/drm/i915/i915_drv.h
>>>>> index eb31c1656cea..be999791abca 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>>> @@ -479,6 +479,7 @@ struct i915_psr {
>>>>>    	bool enabled;
>>>>>    	struct intel_dp *dp;
>>>>>    	enum pipe pipe;
>>>>> +	enum transcoder transcoder;
>>>>>    	bool active;
>>>>>    	struct work_struct work;
>>>>>    	unsigned busy_frontbuffer_bits;
>>>>> @@ -1330,11 +1331,11 @@ struct drm_i915_private {
>>>>>    	 */
>>>>>    	u32 gpio_mmio_base;
>>>>>    
>>>>> +	u32 hsw_psr_mmio_adjust;
>>>>> +
>>>>>    	/* MMIO base address for MIPI regs */
>>>>>    	u32 mipi_mmio_base;
>>>>>    
>>>>> -	u32 psr_mmio_base;
>>>>> -
>>>>>    	u32 pps_mmio_base;
>>>>>    
>>>>>    	wait_queue_head_t gmbus_wait_queue;
>>>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>>>>> b/drivers/gpu/drm/i915/i915_reg.h
>>>>> index 2abd199093c5..a092b34c269d 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>>>> @@ -4186,10 +4186,17 @@ enum {
>>>>>    #define PIPESRC(trans)		_MMIO_TRANS2(trans,
>>>>> _PIPEASRC)
>>>>>    #define PIPE_MULT(trans)	_MMIO_TRANS2(trans,
>>>>> _PIPE_MULT_A)
>>>>>    
>>>>> -/* HSW+ eDP PSR registers */
>>>>> -#define HSW_EDP_PSR_BASE	0x64800
>>>>> -#define BDW_EDP_PSR_BASE	0x6f800
>>>>> -#define EDP_PSR_CTL				_MMIO(dev_priv-
>>>>>> psr_mmio_base + 0)
>>>>> +/*
>>>>> + * HSW+ eDP PSR registers
>>>>> + *
>>>>> + * HSW PSR registers are relative to DDIA(_DDI_BUF_CTL_A +
>>>>> 0x800)
>>>>> with just one
>>>>> + * instance of it
>>>>> + */
>>>>> +#define _HSW_EDP_PSR_BASE			0x64800
>>>>> +#define _SRD_CTL_A				0x60800
>>>>> +#define _SRD_CTL_EDP				0x6f800
>>>>> +#define _PSR_ADJ(tran, reg)			(_TRANS2(tran,
>>>>> reg) - dev_priv->hsw_psr_mmio_adjust)
>>>>> +#define EDP_PSR_CTL(tran)			_MMIO(_PSR_ADJ(
>>>>> tran,
>>>>> _SRD_CTL_A))
>>>>>    #define   EDP_PSR_ENABLE			(1 << 31)
>>>>>    #define   BDW_PSR_SINGLE_FRAME			(1 <<
>>>>> 30)
>>>>>    #define   EDP_PSR_RESTORE_PSR_ACTIVE_CTX_MASK	(1 << 29) /* SW
>>>>> can't modify */
>>>>> @@ -4226,16 +4233,22 @@ enum {
>>>>>    #define   EDP_PSR_TRANSCODER_A_SHIFT		8
>>>>>    #define   EDP_PSR_TRANSCODER_EDP_SHIFT		0
>>>>>    
>>>>> -#define EDP_PSR_AUX_CTL				_MMIO(d
>>>>> ev_priv-
>>>>>> psr_mmio_base + 0x10)
>>>>> +#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))
>>>>>    #define   EDP_PSR_AUX_CTL_TIME_OUT_MASK		(3 <<
>>>>> 26)
>>>>>    #define   EDP_PSR_AUX_CTL_MESSAGE_SIZE_MASK	(0x1f << 20)
>>>>>    #define   EDP_PSR_AUX_CTL_PRECHARGE_2US_MASK	(0xf << 16)
>>>>>    #define   EDP_PSR_AUX_CTL_ERROR_INTERRUPT	(1 << 11)
>>>>>    #define   EDP_PSR_AUX_CTL_BIT_CLOCK_2X_MASK	(0x7ff)
>>>>>    
>>>>> -#define EDP_PSR_AUX_DATA(i)			_MMIO(dev_priv-
>>>>>> psr_mmio_base + 0x14 + (i) * 4) /* 5 registers */
>>>>> +#define _SRD_AUX_DATA_A				0x60814
>>>>> +#define _SRD_AUX_DATA_EDP			0x6f814
>>>>> +#define EDP_PSR_AUX_DATA(tran, i)		_MMIO(_PSR_ADJ(
>>>>> tran,
>>>>> _SRD_AUX_DATA_A) + (i) + 4) /* 5 registers */
>>>>>    
>>>>> -#define EDP_PSR_STATUS				_MMIO(dev_priv-
>>>>>> psr_mmio_base + 0x40)
>>>>> +#define _SRD_STATUS_A				0x60840
>>>>> +#define _SRD_STATUS_EDP				0x6f840
>>>>> +#define EDP_PSR_STATUS(tran)			_MMIO(_PSR_ADJ(
>>>>> tran, _SRD_STATUS_A))
>>>>>    #define   EDP_PSR_STATUS_STATE_MASK		(7 << 29)
>>>>>    #define   EDP_PSR_STATUS_STATE_SHIFT		29
>>>>>    #define   EDP_PSR_STATUS_STATE_IDLE		(0 << 29)
>>>>> @@ -4260,10 +4273,15 @@ enum {
>>>>>    #define   EDP_PSR_STATUS_SENDING_TP1		(1 << 4)
>>>>>    #define   EDP_PSR_STATUS_IDLE_MASK		0xf
>>>>>    
>>>>> -#define EDP_PSR_PERF_CNT		_MMIO(dev_priv-
>>>>>> psr_mmio_base +
>>>>> 0x44)
>>>>> +#define _SRD_PERF_CNT_A			0x60844
>>>>> +#define _SRD_PERF_CNT_EDP		0x6f844
>>>>> +#define EDP_PSR_PERF_CNT(tran)		_MMIO(_PSR_ADJ(tran,
>>>>> _SRD_PERF_CNT_A))
>>>>>    #define   EDP_PSR_PERF_CNT_MASK		0xffffff
>>>>>    
>>>>> -#define EDP_PSR_DEBUG				_MMIO(dev_priv-
>>>>>> psr_mmio_base + 0x60) /* PSR_MASK on SKL+ */
>>>>> +/* PSR_MASK on SKL+ */
>>>>> +#define _SRD_DEBUG_A				0x60860
>>>>> +#define _SRD_DEBUG_EDP				0x6f860
>>>>> +#define EDP_PSR_DEBUG(tran)			_MMIO(_PSR_ADJ(
>>>>> tran, _SRD_DEBUG_A))
>>>>>    #define   EDP_PSR_DEBUG_MASK_MAX_SLEEP         (1 << 28)
>>>>>    #define   EDP_PSR_DEBUG_MASK_LPSP              (1 << 27)
>>>>>    #define   EDP_PSR_DEBUG_MASK_MEMUP             (1 << 26)
>>>>> @@ -4271,7 +4289,9 @@ enum {
>>>>>    #define   EDP_PSR_DEBUG_MASK_DISP_REG_WRITE    (1 << 16) /*
>>>>> Reserved in ICL+ */
>>>>>    #define   EDP_PSR_DEBUG_EXIT_ON_PIXEL_UNDERRUN (1 << 15) /*
>>>>> SKL+
>>>>> */
>>>>>    
>>>>> -#define EDP_PSR2_CTL			_MMIO(0x6f900)
>>>>> +#define _PSR2_CTL_A			0x60900
>>>>> +#define _PSR2_CTL_EDP			0x6f900
>>>>> +#define EDP_PSR2_CTL(tran)		_MMIO_TRANS2(tran,
>>>>> _PSR2_CTL_A)
>>>>>    #define   EDP_PSR2_ENABLE		(1 << 31)
>>>>>    #define   EDP_SU_TRACK_ENABLE		(1 << 30)
>>>>>    #define   EDP_Y_COORDINATE_VALID	(1 << 26) /* GLK and
>>>>> CNL+ */
>>>>> @@ -4293,8 +4313,8 @@ enum {
>>>>>    #define _PSR_EVENT_TRANS_B			0x61848
>>>>>    #define _PSR_EVENT_TRANS_C			0x62848
>>>>>    #define _PSR_EVENT_TRANS_D			0x63848
>>>>> -#define _PSR_EVENT_TRANS_EDP			0x6F848
>>>>> -#define PSR_EVENT(trans)			_MMIO_TRANS2(tr
>>>>> ans,
>>>>> _PSR_EVENT_TRANS_A)
>>>>> +#define _PSR_EVENT_TRANS_EDP			0x6f848
>>>>> +#define PSR_EVENT(tran)				_MMIO_T
>>>>> RANS2(tr
>>>>> an, _PSR_EVENT_TRANS_A)
>>>>>    #define  PSR_EVENT_PSR2_WD_TIMER_EXPIRE		(1 <<
>>>>> 17)
>>>>>    #define  PSR_EVENT_PSR2_DISABLED		(1 << 16)
>>>>>    #define  PSR_EVENT_SU_DIRTY_FIFO_UNDERRUN	(1 << 15)
>>>>> @@ -4312,15 +4332,16 @@ enum {
>>>>>    #define  PSR_EVENT_LPSP_MODE_EXIT		(1 << 1)
>>>>>    #define  PSR_EVENT_PSR_DISABLE			(1 <<
>>>>> 0)
>>>>>    
>>>>> -#define EDP_PSR2_STATUS			_MMIO(0x6f940)
>>>>> +#define _PSR2_STATUS_A			0x60940
>>>>> +#define _PSR2_STATUS_EDP		0x6f940
>>>>> +#define EDP_PSR2_STATUS(tran)		_MMIO_TRANS2(tran,
>>>>> _PSR2_STATUS_A)
>>>>>    #define EDP_PSR2_STATUS_STATE_MASK     (0xf << 28)
>>>>>    #define EDP_PSR2_STATUS_STATE_SHIFT    28
>>>>>    
>>>>> -#define _PSR2_SU_STATUS_0		0x6F914
>>>>> -#define _PSR2_SU_STATUS_1		0x6F918
>>>>> -#define _PSR2_SU_STATUS_2		0x6F91C
>>>>> -#define _PSR2_SU_STATUS(index)		_MMIO(_PICK_EVEN((index
>>>>> ), _PSR2_SU_STATUS_0, _PSR2_SU_STATUS_1))
>>>>> -#define PSR2_SU_STATUS(frame)		(_PSR2_SU_STATUS((frame
>>>>> ) / 3))
>>>>> +#define _PSR2_SU_STATUS_A		0x60914
>>>>> +#define _PSR2_SU_STATUS_EDP		0x6f914
>>>>> +#define _PSR2_SU_STATUS(tran, index)	_MMIO(_TRANS2(tran,
>>>>> _PSR2_SU_STATUS_A) + (index) * 4)
>>>>> +#define PSR2_SU_STATUS(tran, frame)	(_PSR2_SU_STATUS(tran,
>>>>> (frame) / 3))
>>>>>    #define PSR2_SU_STATUS_SHIFT(frame)	(((frame) % 3) * 10)
>>>>>    #define PSR2_SU_STATUS_MASK(frame)	(0x3ff <<
>>>>> PSR2_SU_STATUS_SHIFT(frame))
>>>>>    #define PSR2_SU_STATUS_FRAMES		8
>>>>> -- 
>>>>> 2.22.1
>>>>>
>>>>> _______________________________________________
>>>>> Intel-gfx mailing list
>>>>> Intel-gfx@lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Souza, Jose Aug. 22, 2019, 5:22 p.m. UTC | #6
On Thu, 2019-08-22 at 22:39 +0530, Gupta, Anshuman wrote:
> 
> On 8/22/2019 10:19 PM, Souza, Jose wrote:
> > On Thu, 2019-08-22 at 21:37 +0530, Gupta, Anshuman wrote:
> > > On 8/22/2019 1:36 AM, Souza, Jose wrote:
> > > > On Wed, 2019-08-21 at 10:06 +0530, Anshuman Gupta wrote:
> > > > > On 2019-08-20 at 15:33:23 -0700, José Roberto de Souza wrote:
> > > > > > PSR registers are a mess, some have the full address while
> > > > > > others
> > > > > > just
> > > > > > have the additional offset from psr_mmio_base.
> > > > > > 
> > > > > > For BDW+ psr_mmio_base is nothing more than
> > > > > > TRANSCODER_EDP_OFFSET +
> > > > > > 0x800 and using it makes more difficult for people with an
> > > > > > PSR
> > > > > > register address or PSR register name from from BSpec as
> > > > > > i915
> > > > > > also
> > > > > > don't match the BSpec names.
> > > > > > For HSW psr_mmio_base is _DDI_BUF_CTL_A + 0x800 and PSR
> > > > > > registers
> > > > > > are
> > > > > > only available in DDIA.
> > > > > > 
> > > > > > Other reason to make relative to transcoder is that since
> > > > > > BDW
> > > > > > every
> > > > > > transcoder have PSR registers, so in theory it should be
> > > > > > possible
> > > > > > to
> > > > > > have PSR enabled in a non-eDP transcoder.
> > > > > > 
> > > > > > So for BDW+ we can use _TRANS2() to get the register offset
> > > > > > of
> > > > > > any
> > > > > > PSR register in any transcoder while for HSW we have
> > > > > > _HSW_PSR_ADJ
> > > > > > that will calculate the register offset for the single PSR
> > > > > > instance,
> > > > > > noting that we are already guarded about trying to enable
> > > > > > PSR
> > > > > > in
> > > > > > other
> > > > > > port than DDIA on HSW by the 'if (dig_port->base.port !=
> > > > > > PORT_A)'
> > > > > > in
> > > > > > intel_psr_compute_config(), this check should only be valid
> > > > > > for
> > > > > > HSW
> > > > > > and will be changed in future.
> > > > > > PSR2 registers and PSR_EVENT was added after Haswell so
> > > > > > that is
> > > > > > why
> > > > > > _PSR_ADJ() is not used in some macros.
> > > > > > 
> > > > > > The only registers that can not be relative to transcoder
> > > > > > are
> > > > > > PSR_IMR and PSR_IIR that are not relative to anything, so
> > > > > > keeping
> > > > > > it
> > > > > > hardcoded. That changed for TGL but it will be handled in
> > > > > > another
> > > > > > patch.
> > > > > > 
> > > > > > Also removing BDW_EDP_PSR_BASE from GVT because it is not
> > > > > > used
> > > > > > as
> > > > > > it
> > > > > > is the only PSR register that GVT have.
> > > > > > 
> > > > > > v5:
> > > > > > - Macros changed to be more explicit about HSW (Dhinakaran)
> > > > > > - Squashed with the patch that added the tran parameter to
> > > > > > the
> > > > > > macros (Dhinakaran)
> > > > > > 
> > > > > > v6:
> > > > > > - Checking for interruption errors after module reload in
> > > > > > the
> > > > > > transcoder that will be used (Dhinakaran)
> > > > > > - Using lowercase to the registers offsets
> > > > > > 
> > > > > > v7:
> > > > > > - Removing IS_HASWELL() from registers macros(Jani)
> > > > > > 
> > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > Cc: Zhi Wang <zhi.a.wang@intel.com>
> > > > > > Reviewed-by: Lucas De Marchi <lucas.demarchi@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 | 104
> > > > > > +++++++++++++-
> > > > > > ---
> > > > > > ------
> > > > > >    drivers/gpu/drm/i915/gvt/handlers.c      |   2 +-
> > > > > >    drivers/gpu/drm/i915/i915_debugfs.c      |  18 ++--
> > > > > >    drivers/gpu/drm/i915/i915_drv.h          |   5 +-
> > > > > >    drivers/gpu/drm/i915/i915_reg.h          |  57
> > > > > > +++++++++----
> > > > > >    5 files changed, 113 insertions(+), 73 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > index 3bfb720560c2..77232f6bca17 100644
> > > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > @@ -390,7 +390,7 @@ static void hsw_psr_setup_aux(struct
> > > > > > intel_dp
> > > > > > *intel_dp)
> > > > > >    
> > > > > >    	BUILD_BUG_ON(sizeof(aux_msg) > 20);
> > > > > >    	for (i = 0; i < sizeof(aux_msg); i += 4)
> > > > > > -		I915_WRITE(EDP_PSR_AUX_DATA(i >> 2),
> > > > > > +		I915_WRITE(EDP_PSR_AUX_DATA(dev_priv-
> > > > > > > psr.transcoder, i
> > > > > > > > 2),
> > > > > >    			   intel_dp_pack_aux(&aux_msg[i],
> > > > > > sizeof(aux_msg) - i));
> > > > > >    
> > > > > >    	aux_clock_divider = intel_dp-
> > > > > > > get_aux_clock_divider(intel_dp,
> > > > > > 0);
> > > > > > @@ -401,7 +401,7 @@ static void hsw_psr_setup_aux(struct
> > > > > > intel_dp
> > > > > > *intel_dp)
> > > > > >    
> > > > > >    	/* Select only valid bits for SRD_AUX_CTL */
> > > > > >    	aux_ctl &= psr_aux_mask;
> > > > > > -	I915_WRITE(EDP_PSR_AUX_CTL, aux_ctl);
> > > > > > +	I915_WRITE(EDP_PSR_AUX_CTL(dev_priv->psr.transcoder),
> > > > > > aux_ctl);
> > > > > >    }
> > > > > >    
> > > > > >    static void intel_psr_enable_sink(struct intel_dp
> > > > > > *intel_dp)
> > > > > > @@ -491,8 +491,9 @@ static void hsw_activate_psr1(struct
> > > > > > intel_dp
> > > > > > *intel_dp)
> > > > > >    	if (INTEL_GEN(dev_priv) >= 8)
> > > > > >    		val |= EDP_PSR_CRC_ENABLE;
> > > > > >    
> > > > > > -	val |= I915_READ(EDP_PSR_CTL) &
> > > > > > EDP_PSR_RESTORE_PSR_ACTIVE_CTX_MASK;
> > > > > > -	I915_WRITE(EDP_PSR_CTL, val);
> > > > > > +	val |= (I915_READ(EDP_PSR_CTL(dev_priv-
> > > > > > > psr.transcoder)) &
> > > > > > +		EDP_PSR_RESTORE_PSR_ACTIVE_CTX_MASK);
> > > > > > +	I915_WRITE(EDP_PSR_CTL(dev_priv->psr.transcoder), val);
> > > > > >    }
> > > > > >    
> > > > > >    static void hsw_activate_psr2(struct intel_dp *intel_dp)
> > > > > > @@ -528,9 +529,9 @@ static void hsw_activate_psr2(struct
> > > > > > intel_dp
> > > > > > *intel_dp)
> > > > > >    	 * PSR2 HW is incorrectly using EDP_PSR_TP1_TP3_SEL and
> > > > > > BSpec
> > > > > > is
> > > > > >    	 * recommending keep this bit unset while PSR2 is
> > > > > > enabled.
> > > > > >    	 */
> > > > > > -	I915_WRITE(EDP_PSR_CTL, 0);
> > > > > > +	I915_WRITE(EDP_PSR_CTL(dev_priv->psr.transcoder), 0);
> > > > > >    
> > > > > > -	I915_WRITE(EDP_PSR2_CTL, val);
> > > > > > +	I915_WRITE(EDP_PSR2_CTL(dev_priv->psr.transcoder),
> > > > > > val);
> > > > > >    }
> > > > > >    
> > > > > >    static bool intel_psr2_config_valid(struct intel_dp
> > > > > > *intel_dp,
> > > > > > @@ -606,10 +607,9 @@ void intel_psr_compute_config(struct
> > > > > > intel_dp
> > > > > > *intel_dp,
> > > > > >    
> > > > > >    	/*
> > > > > >    	 * HSW spec explicitly says PSR is tied to port A.
> > > > > > -	 * BDW+ platforms with DDI implementation of PSR have
> > > > > > different
> > > > > > -	 * PSR registers per transcoder and we only implement
> > > > > > transcoder EDP
> > > > > > -	 * ones. Since by Display design transcoder EDP is tied
> > > > > > to port
> > > > > > A
> > > > > > -	 * we can safely escape based on the port A.
> > > > > > +	 * BDW+ platforms have a instance of PSR registers per
> > > > > > transcoder but
> > > > > > +	 * for now it only supports one instance of PSR, so
> > > > > > lets keep
> > > > > > it
> > > > > > +	 * hardcoded to PORT_A
> > > > > >    	 */
> > > > > >    	if (dig_port->base.port != PORT_A) {
> > > > > >    		DRM_DEBUG_KMS("PSR condition failed: Port not
> > > > > > supported\n");
> > > > > > @@ -649,8 +649,8 @@ 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)
> > > > > > -		WARN_ON(I915_READ(EDP_PSR2_CTL) &
> > > > > > EDP_PSR2_ENABLE);
> > > > > > -	WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
> > > > > > +		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);
> > > > > >    	lockdep_assert_held(&dev_priv->psr.lock);
> > > > > >    
> > > > > > @@ -720,19 +720,37 @@ static void
> > > > > > intel_psr_enable_source(struct
> > > > > > intel_dp *intel_dp,
> > > > > >    	if (INTEL_GEN(dev_priv) < 11)
> > > > > >    		mask |= EDP_PSR_DEBUG_MASK_DISP_REG_WRITE;
> > > > > >    
> > > > > > -	I915_WRITE(EDP_PSR_DEBUG, mask);
> > > > > > +	I915_WRITE(EDP_PSR_DEBUG(dev_priv->psr.transcoder),
> > > > > > mask);
> > > > > >    }
> > > > > >    
> > > > > >    static void intel_psr_enable_locked(struct
> > > > > > drm_i915_private
> > > > > > *dev_priv,
> > > > > >    				    const struct
> > > > > > intel_crtc_state
> > > > > > *crtc_state)
> > > > > >    {
> > > > > >    	struct intel_dp *intel_dp = dev_priv->psr.dp;
> > > > > > +	u32 val;
> > > > > >    
> > > > > >    	WARN_ON(dev_priv->psr.enabled);
> > > > > >    
> > > > > >    	dev_priv->psr.psr2_enabled =
> > > > > > intel_psr2_enabled(dev_priv,
> > > > > > crtc_state);
> > > > > >    	dev_priv->psr.busy_frontbuffer_bits = 0;
> > > > > >    	dev_priv->psr.pipe = to_intel_crtc(crtc_state-
> > > > > > > base.crtc)-
> > > > > > > pipe;
> > > > > > +	dev_priv->psr.transcoder = crtc_state->cpu_transcoder;
> > > > > > +
> > > > > > +	/*
> > > > > > +	 * If a PSR error happened and the driver is reloaded,
> > > > > > the
> > > > > > EDP_PSR_IIR
> > > > > > +	 * will still keep the error set even after the reset
> > > > > > done in
> > > > > > the
> > > > > > +	 * irq_preinstall and irq_uninstall hooks.
> > > > > > +	 * And enabling in this situation cause the screen to
> > > > > > freeze in
> > > > > > the
> > > > > > +	 * 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(edp_psr_shift(dev_priv-
> > > > > > > psr.transcoder));
> > > > > > +	if (val) {
> > > > > > +		dev_priv->psr.sink_not_reliable = true;
> > > > > > +		DRM_DEBUG_KMS("PSR interruption error set, not
> > > > > > enabling
> > > > > > PSR\n");
> > > > > > +		return;
> > > > > > +	}
> > > > > >    
> > > > > >    	DRM_DEBUG_KMS("Enabling PSR%s\n",
> > > > > >    		      dev_priv->psr.psr2_enabled ? "2" : "1");
> > > > > > @@ -782,20 +800,27 @@ static void intel_psr_exit(struct
> > > > > > drm_i915_private *dev_priv)
> > > > > >    	u32 val;
> > > > > >    
> > > > > >    	if (!dev_priv->psr.active) {
> > > > > > -		if (INTEL_GEN(dev_priv) >= 9)
> > > > > > -			WARN_ON(I915_READ(EDP_PSR2_CTL) &
> > > > > > EDP_PSR2_ENABLE);
> > > > > > -		WARN_ON(I915_READ(EDP_PSR_CTL) &
> > > > > > EDP_PSR_ENABLE);
> > > > > > +		if (INTEL_GEN(dev_priv) >= 9) {
> > > > > > +			val = I915_READ(EDP_PSR2_CTL(dev_priv-
> > > > > > > psr.transcoder));
> > > > > > +			WARN_ON(val & EDP_PSR2_ENABLE);
> > > > > > +		}
> > > > > > +
> > > > > > +		val = I915_READ(EDP_PSR_CTL(dev_priv-
> > > > > > > psr.transcoder));
> > > > > > +		WARN_ON(val & EDP_PSR_ENABLE);
> > > > > > +
> > > > > >    		return;
> > > > > >    	}
> > > > > >    
> > > > > >    	if (dev_priv->psr.psr2_enabled) {
> > > > > > -		val = I915_READ(EDP_PSR2_CTL);
> > > > > > +		val = I915_READ(EDP_PSR2_CTL(dev_priv-
> > > > > > > psr.transcoder));
> > > > > >    		WARN_ON(!(val & EDP_PSR2_ENABLE));
> > > > > > -		I915_WRITE(EDP_PSR2_CTL, val &
> > > > > > ~EDP_PSR2_ENABLE);
> > > > > > +		val &= ~EDP_PSR2_ENABLE;
> > > > > > +		I915_WRITE(EDP_PSR2_CTL(dev_priv-
> > > > > > > psr.transcoder),
> > > > > > val);
> > > > > >    	} else {
> > > > > > -		val = I915_READ(EDP_PSR_CTL);
> > > > > > +		val = I915_READ(EDP_PSR_CTL(dev_priv-
> > > > > > > psr.transcoder));
> > > > > >    		WARN_ON(!(val & EDP_PSR_ENABLE));
> > > > > > -		I915_WRITE(EDP_PSR_CTL, val & ~EDP_PSR_ENABLE);
> > > > > > +		val &= ~EDP_PSR_ENABLE;
> > > > > > +		I915_WRITE(EDP_PSR_CTL(dev_priv-
> > > > > > > psr.transcoder), val);
> > > > > >    	}
> > > > > >    	dev_priv->psr.active = false;
> > > > > >    }
> > > > > > @@ -817,10 +842,10 @@ static void
> > > > > > intel_psr_disable_locked(struct
> > > > > > intel_dp *intel_dp)
> > > > > >    	intel_psr_exit(dev_priv);
> > > > > >    
> > > > > >    	if (dev_priv->psr.psr2_enabled) {
> > > > > > -		psr_status = EDP_PSR2_STATUS;
> > > > > > +		psr_status = EDP_PSR2_STATUS(dev_priv-
> > > > > > > psr.transcoder);
> > > > > >    		psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
> > > > > >    	} else {
> > > > > > -		psr_status = EDP_PSR_STATUS;
> > > > > > +		psr_status = EDP_PSR_STATUS(dev_priv-
> > > > > > > psr.transcoder);
> > > > > >    		psr_status_mask = EDP_PSR_STATUS_STATE_MASK;
> > > > > >    	}
> > > > > >    
> > > > > > @@ -963,7 +988,8 @@ int intel_psr_wait_for_idle(const
> > > > > > struct
> > > > > > intel_crtc_state *new_crtc_state,
> > > > > >    	 * defensive enough to cover everything.
> > > > > >    	 */
> > > > > >    
> > > > > > -	return __intel_wait_for_register(&dev_priv->uncore,
> > > > > > EDP_PSR_STATUS,
> > > > > > +	return __intel_wait_for_register(&dev_priv->uncore,
> > > > > > +					 EDP_PSR_STATUS(dev_pri
> > > > > > v-
> > > > > > > psr.transcoder),
> > > > > >    					 EDP_PSR_STATUS_STATE_M
> > > > > > ASK,
> > > > > >    					 EDP_PSR_STATUS_STATE_I
> > > > > > DLE, 2,
> > > > > > 50,
> > > > > >    					 out_value);
> > > > > > @@ -979,10 +1005,10 @@ static bool
> > > > > > __psr_wait_for_idle_locked(struct drm_i915_private
> > > > > > *dev_priv)
> > > > > >    		return false;
> > > > > >    
> > > > > >    	if (dev_priv->psr.psr2_enabled) {
> > > > > > -		reg = EDP_PSR2_STATUS;
> > > > > > +		reg = EDP_PSR2_STATUS(dev_priv-
> > > > > > > psr.transcoder);
> > > > > >    		mask = EDP_PSR2_STATUS_STATE_MASK;
> > > > > >    	} else {
> > > > > > -		reg = EDP_PSR_STATUS;
> > > > > > +		reg = EDP_PSR_STATUS(dev_priv->psr.transcoder);
> > > > > >    		mask = EDP_PSR_STATUS_STATE_MASK;
> > > > > >    	}
> > > > > >    
> > > > > > @@ -1208,36 +1234,24 @@ void intel_psr_flush(struct
> > > > > > drm_i915_private *dev_priv,
> > > > > >     */
> > > > > >    void intel_psr_init(struct drm_i915_private *dev_priv)
> > > > > >    {
> > > > > > -	u32 val;
> > > > > > -
> > > > > >    	if (!HAS_PSR(dev_priv))
> > > > > >    		return;
> > > > > >    
> > > > > > -	dev_priv->psr_mmio_base = IS_HASWELL(dev_priv) ?
> > > > > > -		HSW_EDP_PSR_BASE : BDW_EDP_PSR_BASE;
> > > > > > -
> > > > > >    	if (!dev_priv->psr.sink_support)
> > > > > >    		return;
> > > > > >    
> > > > > > +	if (IS_HASWELL(dev_priv))
> > > > > > +		/*
> > > > > > +		 * HSW don't have PSR registers on the same
> > > > > > space as
> > > > > > transcoder
> > > > > > +		 * so set this to a value that when subtract to
> > > > > > the
> > > > > > register
> > > > > > +		 * in transcoder space results in the right
> > > > > > offset for
> > > > > > HSW
> > > > > > +		 */
> > > > > > +		dev_priv->hsw_psr_mmio_adjust = _SRD_CTL_EDP -
> > > > > > _HSW_EDP_PSR_BASE;
> > > > > > +
> > > > > >    	if (i915_modparams.enable_psr == -1)
> > > > > >    		if (INTEL_GEN(dev_priv) < 9 || !dev_priv-
> > > > > > > vbt.psr.enable)
> > > > > >    			i915_modparams.enable_psr = 0;
> > > > > >    
> > > > > > -	/*
> > > > > > -	 * If a PSR error happened and the driver is reloaded,
> > > > > > the
> > > > > > EDP_PSR_IIR
> > > > > > -	 * will still keep the error set even after the reset
> > > > > > done in
> > > > > > the
> > > > > > -	 * irq_preinstall and irq_uninstall hooks.
> > > > > > -	 * And enabling in this situation cause the screen to
> > > > > > freeze in
> > > > > > the
> > > > > > -	 * 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(edp_psr_shift(TRANSCODER_EDP));
> > > > > > -	if (val) {
> > > > > > -		DRM_DEBUG_KMS("PSR interruption error set\n");
> > > > > > -		dev_priv->psr.sink_not_reliable = true;
> > > > > > -	}
> > > > > > -
> > > > > Earlier EDP_PSR_IIR was being checked only in driver init
> > > > > path,
> > > > > now it has been checked for every modeset/fastset path in
> > > > > intel_psr_enable_locked(). Is it ok ?
> > > > > If it is justified why are we not checking it in
> > > > > intel_psr_flush()
> > > > > there also it enables psr.
> > > > 
> > > > I moved it primarily because on intel_psr_enable_locked() we
> > > > have
> > > > the
> > > > transcoder that will be used, doing on init we would need to
> > > > check
> > > > for
> > > > all transcoders and in case of a error set in one transcoder we
> > > > would
> > > > need to fail initialization as PSR could be enabled on that
> > > > transcoder.
> > > That is correct, but with this EDP_PSR_IIR error is getting
> > > checked
> > > in
> > > every modeset/fastset which is redundant, as it is already being
> > > check
> > > at interrupt handler.
> > > IMO it should be only checked somehow at initel_psr_init(), even
> > > comment
> > > also reflect same thing ("If a PSR error happened and the driver
> > > is
> > > reloaded...").
> > > If there no other way to handle this, comment should be change
> > > accordingly.
> > 
> > Only on full modesets, on real world it will happen only once at
> > every
> AFAIU intel_psr_update() may also call intel_psr_enable_locked(),
> which 
> means even on fastset this this will get checked ?

Yes but in 99% of the times it will fall into the case were PSR was
enabled and the modeset still allows PSR, so it will do nothing.


> > boot also read a register is not a expensive operation.
> > 
> > I will merge this as it is, if someone else also thinks that the
> > comment is necessary I will add on top.
> > 
> > 
> > > > No need to do that on intel_psr_flush() because PSR
> > > > interruptions
> > > > can
> > > > be triggered even with PSR inactive.
> > > > 
> > > > 
> > > > > >    	/* Set link_standby x link_off defaults */
> > > > > >    	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> > > > > >    		/* HSW and BDW require workarounds that we
> > > > > > don't
> > > > > > implement. */
> > > > > > diff --git a/drivers/gpu/drm/i915/gvt/handlers.c
> > > > > > b/drivers/gpu/drm/i915/gvt/handlers.c
> > > > > > index 25f78196b964..45a9124e53b6 100644
> > > > > > --- a/drivers/gpu/drm/i915/gvt/handlers.c
> > > > > > +++ b/drivers/gpu/drm/i915/gvt/handlers.c
> > > > > > @@ -2796,7 +2796,7 @@ static int
> > > > > > init_broadwell_mmio_info(struct
> > > > > > intel_gvt *gvt)
> > > > > >    	MMIO_D(CHICKEN_PIPESL_1(PIPE_C), D_BDW_PLUS);
> > > > > >    
> > > > > >    	MMIO_D(WM_MISC, D_BDW);
> > > > > > -	MMIO_D(_MMIO(BDW_EDP_PSR_BASE), D_BDW);
> > > > > > +	MMIO_D(_MMIO(_SRD_CTL_EDP), D_BDW);
> > > > > >    
> > > > > >    	MMIO_D(_MMIO(0x6671c), D_BDW_PLUS);
> > > > > >    	MMIO_D(_MMIO(0x66c00), D_BDW_PLUS);
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > > > b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > > > index b39226d7f8d2..6e4824daafae 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > > > @@ -2132,7 +2132,7 @@ psr_source_status(struct
> > > > > > drm_i915_private
> > > > > > *dev_priv, struct seq_file *m)
> > > > > >    			"BUF_ON",
> > > > > >    			"TG_ON"
> > > > > >    		};
> > > > > > -		val = I915_READ(EDP_PSR2_STATUS);
> > > > > > +		val = I915_READ(EDP_PSR2_STATUS(dev_priv-
> > > > > > > psr.transcoder));
> > > > > >    		status_val = (val & EDP_PSR2_STATUS_STATE_MASK)
> > > > > >    			      EDP_PSR2_STATUS_STATE_SHIFT;
> > > > > >    		if (status_val < ARRAY_SIZE(live_status))
> > > > > > @@ -2148,7 +2148,7 @@ psr_source_status(struct
> > > > > > drm_i915_private
> > > > > > *dev_priv, struct seq_file *m)
> > > > > >    			"SRDOFFACK",
> > > > > >    			"SRDENT_ON",
> > > > > >    		};
> > > > > > -		val = I915_READ(EDP_PSR_STATUS);
> > > > > > +		val = I915_READ(EDP_PSR_STATUS(dev_priv-
> > > > > > > psr.transcoder));
> > > > > >    		status_val = (val & EDP_PSR_STATUS_STATE_MASK)
> > > > > >    			      EDP_PSR_STATUS_STATE_SHIFT;
> > > > > >    		if (status_val < ARRAY_SIZE(live_status))
> > > > > > @@ -2191,10 +2191,10 @@ static int
> > > > > > i915_edp_psr_status(struct
> > > > > > seq_file *m, void *data)
> > > > > >    		goto unlock;
> > > > > >    
> > > > > >    	if (psr->psr2_enabled) {
> > > > > > -		val = I915_READ(EDP_PSR2_CTL);
> > > > > > +		val = I915_READ(EDP_PSR2_CTL(dev_priv-
> > > > > > > psr.transcoder));
> > > > > >    		enabled = val & EDP_PSR2_ENABLE;
> > > > > >    	} else {
> > > > > > -		val = I915_READ(EDP_PSR_CTL);
> > > > > > +		val = I915_READ(EDP_PSR_CTL(dev_priv-
> > > > > > > psr.transcoder));
> > > > > >    		enabled = val & EDP_PSR_ENABLE;
> > > > > >    	}
> > > > > >    	seq_printf(m, "Source PSR ctl: %s [0x%08x]\n",
> > > > > > @@ -2207,7 +2207,8 @@ static int i915_edp_psr_status(struct
> > > > > > seq_file *m, void *data)
> > > > > >    	 * SKL+ Perf counter is reset to 0 everytime DC state
> > > > > > is
> > > > > > entered
> > > > > >    	 */
> > > > > >    	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
> > > > > > -		val = I915_READ(EDP_PSR_PERF_CNT) &
> > > > > > EDP_PSR_PERF_CNT_MASK;
> > > > > > +		val = I915_READ(EDP_PSR_PERF_CNT(dev_priv-
> > > > > > > psr.transcoder));
> > > > > > +		val &= EDP_PSR_PERF_CNT_MASK;
> > > > > >    		seq_printf(m, "Performance counter: %u\n",
> > > > > > val);
> > > > > >    	}
> > > > > >    
> > > > > > @@ -2225,8 +2226,11 @@ static int
> > > > > > i915_edp_psr_status(struct
> > > > > > seq_file *m, void *data)
> > > > > >    		 * Reading all 3 registers before hand to
> > > > > > minimize
> > > > > > crossing a
> > > > > >    		 * frame boundary between register reads
> > > > > >    		 */
> > > > > > -		for (frame = 0; frame < PSR2_SU_STATUS_FRAMES;
> > > > > > frame +=
> > > > > > 3)
> > > > > > -			su_frames_val[frame / 3] =
> > > > > > I915_READ(PSR2_SU_STATUS(frame));
> > > > > > +		for (frame = 0; frame < PSR2_SU_STATUS_FRAMES;
> > > > > > frame +=
> > > > > > 3) {
> > > > > > +			val =
> > > > > > I915_READ(PSR2_SU_STATUS(dev_priv-
> > > > > > > psr.transcoder,
> > > > > > +						       frame));
> > > > > > +			su_frames_val[frame / 3] = val;
> > > > > > +		}
> > > > > >    
> > > > > >    		seq_puts(m, "Frame:\tPSR2 SU blocks:\n");
> > > > > >    
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > > > index eb31c1656cea..be999791abca 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > > > @@ -479,6 +479,7 @@ struct i915_psr {
> > > > > >    	bool enabled;
> > > > > >    	struct intel_dp *dp;
> > > > > >    	enum pipe pipe;
> > > > > > +	enum transcoder transcoder;
> > > > > >    	bool active;
> > > > > >    	struct work_struct work;
> > > > > >    	unsigned busy_frontbuffer_bits;
> > > > > > @@ -1330,11 +1331,11 @@ struct drm_i915_private {
> > > > > >    	 */
> > > > > >    	u32 gpio_mmio_base;
> > > > > >    
> > > > > > +	u32 hsw_psr_mmio_adjust;
> > > > > > +
> > > > > >    	/* MMIO base address for MIPI regs */
> > > > > >    	u32 mipi_mmio_base;
> > > > > >    
> > > > > > -	u32 psr_mmio_base;
> > > > > > -
> > > > > >    	u32 pps_mmio_base;
> > > > > >    
> > > > > >    	wait_queue_head_t gmbus_wait_queue;
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > > > > b/drivers/gpu/drm/i915/i915_reg.h
> > > > > > index 2abd199093c5..a092b34c269d 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > > > @@ -4186,10 +4186,17 @@ enum {
> > > > > >    #define PIPESRC(trans)		_MMIO_TRANS2(trans,
> > > > > > _PIPEASRC)
> > > > > >    #define PIPE_MULT(trans)	_MMIO_TRANS2(trans,
> > > > > > _PIPE_MULT_A)
> > > > > >    
> > > > > > -/* HSW+ eDP PSR registers */
> > > > > > -#define HSW_EDP_PSR_BASE	0x64800
> > > > > > -#define BDW_EDP_PSR_BASE	0x6f800
> > > > > > -#define EDP_PSR_CTL				_MMIO(d
> > > > > > ev_priv-
> > > > > > > psr_mmio_base + 0)
> > > > > > +/*
> > > > > > + * HSW+ eDP PSR registers
> > > > > > + *
> > > > > > + * HSW PSR registers are relative to DDIA(_DDI_BUF_CTL_A +
> > > > > > 0x800)
> > > > > > with just one
> > > > > > + * instance of it
> > > > > > + */
> > > > > > +#define _HSW_EDP_PSR_BASE			0x64800
> > > > > > +#define _SRD_CTL_A				0x60800
> > > > > > +#define _SRD_CTL_EDP				0x6f800
> > > > > > +#define _PSR_ADJ(tran, reg)			(_TRANS
> > > > > > 2(tran,
> > > > > > reg) - dev_priv->hsw_psr_mmio_adjust)
> > > > > > +#define EDP_PSR_CTL(tran)			_MMIO(_PSR_ADJ(
> > > > > > tran,
> > > > > > _SRD_CTL_A))
> > > > > >    #define   EDP_PSR_ENABLE			(1 << 31)
> > > > > >    #define   BDW_PSR_SINGLE_FRAME			(1 <<
> > > > > > 30)
> > > > > >    #define   EDP_PSR_RESTORE_PSR_ACTIVE_CTX_MASK	(1 <<
> > > > > > 29) /* SW
> > > > > > can't modify */
> > > > > > @@ -4226,16 +4233,22 @@ enum {
> > > > > >    #define   EDP_PSR_TRANSCODER_A_SHIFT		8
> > > > > >    #define   EDP_PSR_TRANSCODER_EDP_SHIFT		0
> > > > > >    
> > > > > > -#define EDP_PSR_AUX_CTL				_MMIO(d
> > > > > > ev_priv-
> > > > > > > psr_mmio_base + 0x10)
> > > > > > +#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))
> > > > > >    #define   EDP_PSR_AUX_CTL_TIME_OUT_MASK		(3 <<
> > > > > > 26)
> > > > > >    #define   EDP_PSR_AUX_CTL_MESSAGE_SIZE_MASK	(0x1f
> > > > > > << 20)
> > > > > >    #define   EDP_PSR_AUX_CTL_PRECHARGE_2US_MASK	(0xf <<
> > > > > > 16)
> > > > > >    #define   EDP_PSR_AUX_CTL_ERROR_INTERRUPT	(1 <<
> > > > > > 11)
> > > > > >    #define   EDP_PSR_AUX_CTL_BIT_CLOCK_2X_MASK	(0x7ff)
> > > > > >    
> > > > > > -#define EDP_PSR_AUX_DATA(i)			_MMIO(d
> > > > > > ev_priv-
> > > > > > > psr_mmio_base + 0x14 + (i) * 4) /* 5 registers */
> > > > > > +#define _SRD_AUX_DATA_A				0x60814
> > > > > > +#define _SRD_AUX_DATA_EDP			0x6f814
> > > > > > +#define EDP_PSR_AUX_DATA(tran, i)		_MMIO(_PSR_ADJ(
> > > > > > tran,
> > > > > > _SRD_AUX_DATA_A) + (i) + 4) /* 5 registers */
> > > > > >    
> > > > > > -#define EDP_PSR_STATUS				_MMIO(d
> > > > > > ev_priv-
> > > > > > > psr_mmio_base + 0x40)
> > > > > > +#define _SRD_STATUS_A				0x60840
> > > > > > +#define _SRD_STATUS_EDP				0x6f840
> > > > > > +#define EDP_PSR_STATUS(tran)			_MMIO(_
> > > > > > PSR_ADJ(
> > > > > > tran, _SRD_STATUS_A))
> > > > > >    #define   EDP_PSR_STATUS_STATE_MASK		(7 <<
> > > > > > 29)
> > > > > >    #define   EDP_PSR_STATUS_STATE_SHIFT		29
> > > > > >    #define   EDP_PSR_STATUS_STATE_IDLE		(0 <<
> > > > > > 29)
> > > > > > @@ -4260,10 +4273,15 @@ enum {
> > > > > >    #define   EDP_PSR_STATUS_SENDING_TP1		(1 <<
> > > > > > 4)
> > > > > >    #define   EDP_PSR_STATUS_IDLE_MASK		0xf
> > > > > >    
> > > > > > -#define EDP_PSR_PERF_CNT		_MMIO(dev_priv-
> > > > > > > psr_mmio_base +
> > > > > > 0x44)
> > > > > > +#define _SRD_PERF_CNT_A			0x60844
> > > > > > +#define _SRD_PERF_CNT_EDP		0x6f844
> > > > > > +#define EDP_PSR_PERF_CNT(tran)		_MMIO(_PSR_ADJ(
> > > > > > tran,
> > > > > > _SRD_PERF_CNT_A))
> > > > > >    #define   EDP_PSR_PERF_CNT_MASK		0xffffff
> > > > > >    
> > > > > > -#define EDP_PSR_DEBUG				_MMIO(d
> > > > > > ev_priv-
> > > > > > > psr_mmio_base + 0x60) /* PSR_MASK on SKL+ */
> > > > > > +/* PSR_MASK on SKL+ */
> > > > > > +#define _SRD_DEBUG_A				0x60860
> > > > > > +#define _SRD_DEBUG_EDP				0x6f860
> > > > > > +#define EDP_PSR_DEBUG(tran)			_MMIO(_
> > > > > > PSR_ADJ(
> > > > > > tran, _SRD_DEBUG_A))
> > > > > >    #define   EDP_PSR_DEBUG_MASK_MAX_SLEEP         (1 << 28)
> > > > > >    #define   EDP_PSR_DEBUG_MASK_LPSP              (1 << 27)
> > > > > >    #define   EDP_PSR_DEBUG_MASK_MEMUP             (1 << 26)
> > > > > > @@ -4271,7 +4289,9 @@ enum {
> > > > > >    #define   EDP_PSR_DEBUG_MASK_DISP_REG_WRITE    (1 << 16)
> > > > > > /*
> > > > > > Reserved in ICL+ */
> > > > > >    #define   EDP_PSR_DEBUG_EXIT_ON_PIXEL_UNDERRUN (1 << 15)
> > > > > > /*
> > > > > > SKL+
> > > > > > */
> > > > > >    
> > > > > > -#define EDP_PSR2_CTL			_MMIO(0x6f900)
> > > > > > +#define _PSR2_CTL_A			0x60900
> > > > > > +#define _PSR2_CTL_EDP			0x6f900
> > > > > > +#define EDP_PSR2_CTL(tran)		_MMIO_TRANS2(tran,
> > > > > > _PSR2_CTL_A)
> > > > > >    #define   EDP_PSR2_ENABLE		(1 << 31)
> > > > > >    #define   EDP_SU_TRACK_ENABLE		(1 << 30)
> > > > > >    #define   EDP_Y_COORDINATE_VALID	(1 << 26) /* GLK and
> > > > > > CNL+ */
> > > > > > @@ -4293,8 +4313,8 @@ enum {
> > > > > >    #define _PSR_EVENT_TRANS_B			0x61848
> > > > > >    #define _PSR_EVENT_TRANS_C			0x62848
> > > > > >    #define _PSR_EVENT_TRANS_D			0x63848
> > > > > > -#define _PSR_EVENT_TRANS_EDP			0x6F848
> > > > > > -#define PSR_EVENT(trans)			_MMIO_TRANS2(tr
> > > > > > ans,
> > > > > > _PSR_EVENT_TRANS_A)
> > > > > > +#define _PSR_EVENT_TRANS_EDP			0x6f848
> > > > > > +#define PSR_EVENT(tran)				_MMIO_T
> > > > > > RANS2(tr
> > > > > > an, _PSR_EVENT_TRANS_A)
> > > > > >    #define  PSR_EVENT_PSR2_WD_TIMER_EXPIRE		(1 <<
> > > > > > 17)
> > > > > >    #define  PSR_EVENT_PSR2_DISABLED		(1 << 16)
> > > > > >    #define  PSR_EVENT_SU_DIRTY_FIFO_UNDERRUN	(1 <<
> > > > > > 15)
> > > > > > @@ -4312,15 +4332,16 @@ enum {
> > > > > >    #define  PSR_EVENT_LPSP_MODE_EXIT		(1 <<
> > > > > > 1)
> > > > > >    #define  PSR_EVENT_PSR_DISABLE			(1 <<
> > > > > > 0)
> > > > > >    
> > > > > > -#define EDP_PSR2_STATUS			_MMIO(0x6f940)
> > > > > > +#define _PSR2_STATUS_A			0x60940
> > > > > > +#define _PSR2_STATUS_EDP		0x6f940
> > > > > > +#define EDP_PSR2_STATUS(tran)		_MMIO_TRANS2(tr
> > > > > > an,
> > > > > > _PSR2_STATUS_A)
> > > > > >    #define EDP_PSR2_STATUS_STATE_MASK     (0xf << 28)
> > > > > >    #define EDP_PSR2_STATUS_STATE_SHIFT    28
> > > > > >    
> > > > > > -#define _PSR2_SU_STATUS_0		0x6F914
> > > > > > -#define _PSR2_SU_STATUS_1		0x6F918
> > > > > > -#define _PSR2_SU_STATUS_2		0x6F91C
> > > > > > -#define _PSR2_SU_STATUS(index)		_MMIO(_PICK_EVE
> > > > > > N((index
> > > > > > ), _PSR2_SU_STATUS_0, _PSR2_SU_STATUS_1))
> > > > > > -#define PSR2_SU_STATUS(frame)		(_PSR2_SU_STATU
> > > > > > S((frame
> > > > > > ) / 3))
> > > > > > +#define _PSR2_SU_STATUS_A		0x60914
> > > > > > +#define _PSR2_SU_STATUS_EDP		0x6f914
> > > > > > +#define _PSR2_SU_STATUS(tran, index)	_MMIO(_TRANS2(t
> > > > > > ran,
> > > > > > _PSR2_SU_STATUS_A) + (index) * 4)
> > > > > > +#define PSR2_SU_STATUS(tran, frame)	(_PSR2_SU_STATU
> > > > > > S(tran,
> > > > > > (frame) / 3))
> > > > > >    #define PSR2_SU_STATUS_SHIFT(frame)	(((frame) % 3)
> > > > > > * 10)
> > > > > >    #define PSR2_SU_STATUS_MASK(frame)	(0x3ff <<
> > > > > > PSR2_SU_STATUS_SHIFT(frame))
> > > > > >    #define PSR2_SU_STATUS_FRAMES		8
> > > > > > -- 
> > > > > > 2.22.1
> > > > > > 
> > > > > > _______________________________________________
> > > > > > Intel-gfx mailing list
> > > > > > Intel-gfx@lists.freedesktop.org
> > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Souza, Jose Aug. 22, 2019, 8:24 p.m. UTC | #7
On Wed, 2019-08-21 at 13:13 +0000, Patchwork wrote:
> == Series Details ==
> 
> Series: series starting with [v8,1/3] drm/i915/psr: Make PSR
> registers relative to transcoders
> URL   : https://patchwork.freedesktop.org/series/65507/
> State : success
> 
> == Summary ==
> 
> CI Bug Log - changes from CI_DRM_6750_full -> Patchwork_14112_full
> ====================================================
> 
> Summary
> -------
> 
>   **SUCCESS**
> 
>   No regressions found.
> 

Pushed to dinq, thanks for the reviews Lucas and Anshuman

>   
> 
> Known issues
> ------------
> 
>   Here are the changes found in Patchwork_14112_full that come from
> known issues:
> 
> ### IGT changes ###
> 
> #### Issues hit ####
> 
>   * igt@gem_ctx_isolation@rcs0-s3:
>     - shard-apl:          [PASS][1] -> [DMESG-WARN][2] ([fdo#108566])
> +5 similar issues
>    [1]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6750/shard-apl8/igt@gem_ctx_isolation@rcs0-s3.html
>    [2]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14112/shard-apl8/igt@gem_ctx_isolation@rcs0-s3.html
> 
>   * igt@gem_ctx_shared@exec-single-timeline-bsd:
>     - shard-iclb:         [PASS][3] -> [SKIP][4] ([fdo#110841])
>    [3]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6750/shard-iclb8/igt@gem_ctx_shared@exec-single-timeline-bsd.html
>    [4]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14112/shard-iclb4/igt@gem_ctx_shared@exec-single-timeline-bsd.html
> 
>   * igt@gem_exec_schedule@preempt-other-chain-bsd:
>     - shard-iclb:         [PASS][5] -> [SKIP][6] ([fdo#111325]) +5
> similar issues
>    [5]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6750/shard-iclb5/igt@gem_exec_schedule@preempt-other-chain-bsd.html
>    [6]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14112/shard-iclb2/igt@gem_exec_schedule@preempt-other-chain-bsd.html
> 
>   * igt@i915_pm_rpm@basic-pci-d3-state:
>     - shard-apl:          [PASS][7] -> [INCOMPLETE][8] ([fdo#103927])
> +2 similar issues
>    [7]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6750/shard-apl1/igt@i915_pm_rpm@basic-pci-d3-state.html
>    [8]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14112/shard-apl2/igt@i915_pm_rpm@basic-pci-d3-state.html
> 
>   * igt@kms_cursor_legacy@2x-long-cursor-vs-flip-atomic:
>     - shard-hsw:          [PASS][9] -> [FAIL][10] ([fdo#105767])
>    [9]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6750/shard-hsw7/igt@kms_cursor_legacy@2x-long-cursor-vs-flip-atomic.html
>    [10]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14112/shard-hsw8/igt@kms_cursor_legacy@2x-long-cursor-vs-flip-atomic.html
> 
>   * igt@kms_flip@flip-vs-expired-vblank:
>     - shard-skl:          [PASS][11] -> [FAIL][12] ([fdo#105363]) +1
> similar issue
>    [11]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6750/shard-skl5/igt@kms_flip@flip-vs-expired-vblank.html
>    [12]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14112/shard-skl8/igt@kms_flip@flip-vs-expired-vblank.html
> 
>   * igt@kms
> _frontbuffer_tracking@fbc-1p-primscrn-pri-indfb-draw-pwrite:
>     - shard-iclb:         [PASS][13] -> [INCOMPLETE][14]
> ([fdo#107713])
>    [13]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6750/shard-iclb7/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-indfb-draw-pwrite.html
>    [14]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14112/shard-iclb7/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-indfb-draw-pwrite.html
> 
>   * igt@kms_frontbuffer_tracking@fbc-1p-rte:
>     - shard-iclb:         [PASS][15] -> [FAIL][16] ([fdo#103167] /
> [fdo#110378])
>    [15]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6750/shard-iclb1/igt@kms_frontbuffer_tracking@fbc-1p-rte.html
>    [16]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14112/shard-iclb2/igt@kms_frontbuffer_tracking@fbc-1p-rte.html
> 
>   * igt@kms_frontbuffer_tracking@fbc-suspend:
>     - shard-skl:          [PASS][17] -> [INCOMPLETE][18]
> ([fdo#104108]) +1 similar issue
>    [17]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6750/shard-skl5/igt@kms_frontbuffer_tracking@fbc-suspend.html
>    [18]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14112/shard-skl8/igt@kms_frontbuffer_tracking@fbc-suspend.html
> 
>   * igt@kms
> _frontbuffer_tracking@fbcpsr-1p-primscrn-pri-shrfb-draw-render:
>     - shard-iclb:         [PASS][19] -> [FAIL][20] ([fdo#103167]) +2
> similar issues
>    [19]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6750/shard-iclb3/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-shrfb-draw-render.html
>    [20]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14112/shard-iclb7/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-pri-shrfb-draw-render.html
> 
>   * igt@kms_plane_alpha_blend@pipe-c-coverage-7efc:
>     - shard-skl:          [PASS][21] -> [FAIL][22] ([fdo#108145] /
> [fdo#110403])
>    [21]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6750/shard-skl4/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html
>    [22]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14112/shard-skl7/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html
> 
>   * igt@kms_psr2_su@frontbuffer:
>     - shard-iclb:         [PASS][23] -> [SKIP][24] ([fdo#109642] /
> [fdo#111068])
>    [23]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6750/shard-iclb2/igt@kms_psr2_su@frontbuffer.html
>    [24]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14112/shard-iclb1/igt@kms_psr2_su@frontbuffer.html
> 
>   * igt@kms_psr@psr2_cursor_mmap_cpu:
>     - shard-iclb:         [PASS][25] -> [SKIP][26] ([fdo#109441]) +2
> similar issues
>    [25]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6750/shard-iclb2/igt@kms_psr@psr2_cursor_mmap_cpu.html
>    [26]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14112/shard-iclb4/igt@kms_psr@psr2_cursor_mmap_cpu.html
> 
>   * igt@perf@polling:
>     - shard-skl:          [PASS][27] -> [FAIL][28] ([fdo#110728])
>    [27]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6750/shard-skl3/igt@perf@polling.html
>    [28]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14112/shard-skl4/igt@perf@polling.html
> 
>   * igt@prime_busy@hang-bsd2:
>     - shard-iclb:         [PASS][29] -> [SKIP][30] ([fdo#109276]) +10
> similar issues
>    [29]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6750/shard-iclb4/igt@prime_busy@hang-bsd2.html
>    [30]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14112/shard-iclb8/igt@prime_busy@hang-bsd2.html
> 
>   
> #### Possible fixes ####
> 
>   * igt@gem_eio@reset-stress:
>     - shard-skl:          [FAIL][31] ([fdo#109661]) -> [PASS][32]
>    [31]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6750/shard-skl5/igt@gem_eio@reset-stress.html
>    [32]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14112/shard-skl5/igt@gem_eio@reset-stress.html
> 
>   * igt@gem_exec_balancer@smoke:
>     - shard-iclb:         [SKIP][33] ([fdo#110854]) -> [PASS][34]
>    [33]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6750/shard-iclb8/igt@gem_exec_balancer@smoke.html
>    [34]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14112/shard-iclb1/igt@gem_exec_balancer@smoke.html
> 
>   * igt@gem_exec_schedule@promotion-bsd1:
>     - shard-iclb:         [SKIP][35] ([fdo#109276]) -> [PASS][36] +15
> similar issues
>    [35]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6750/shard-iclb8/igt@gem_exec_schedule@promotion-bsd1.html
>    [36]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14112/shard-iclb4/igt@gem_exec_schedule@promotion-bsd1.html
> 
>   * igt@gem_exec_schedule@reorder-wide-bsd:
>     - shard-iclb:         [SKIP][37] ([fdo#111325]) -> [PASS][38] +2
> similar issues
>    [37]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6750/shard-iclb1/igt@gem_exec_schedule@reorder-wide-bsd.html
>    [38]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14112/shard-iclb5/igt@gem_exec_schedule@reorder-wide-bsd.html
> 
>   * igt@i915_pm_rps@min-max-config-loaded:
>     - shard-iclb:         [FAIL][39] ([fdo#111409]) -> [PASS][40]
>    [39]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6750/shard-iclb7/igt@i915_pm_rps@min-max-config-loaded.html
>    [40]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14112/shard-iclb3/igt@i915_pm_rps@min-max-config-loaded.html
> 
>   * igt@i915_suspend@sysfs-reader:
>     - shard-apl:          [DMESG-WARN][41] ([fdo#108566]) ->
> [PASS][42] +3 similar issues
>    [41]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6750/shard-apl7/igt@i915_suspend@sysfs-reader.html
>    [42]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14112/shard-apl5/igt@i915_suspend@sysfs-reader.html
> 
>   * igt@kms_cursor_crc@pipe-c-cursor-256x85-sliding:
>     - shard-iclb:         [INCOMPLETE][43] ([fdo#107713]) ->
> [PASS][44]
>    [43]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6750/shard-iclb7/igt@kms_cursor_crc@pipe-c-cursor-256x85-sliding.html
>    [44]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14112/shard-iclb5/igt@kms_cursor_crc@pipe-c-cursor-256x85-sliding.html
> 
>   * igt@kms_cursor_legacy@2x-long-cursor-vs-flip-legacy:
>     - shard-hsw:          [FAIL][45] ([fdo#105767]) -> [PASS][46]
>    [45]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6750/shard-hsw6/igt@kms_cursor_legacy@2x-long-cursor-vs-flip-legacy.html
>    [46]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14112/shard-hsw1/igt@kms_cursor_legacy@2x-long-cursor-vs-flip-legacy.html
> 
>   * igt@kms_fbcon_fbt@psr-suspend:
>     - shard-skl:          [INCOMPLETE][47] ([fdo#104108]) ->
> [PASS][48]
>    [47]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6750/shard-skl10/igt@kms_fbcon_fbt@psr-suspend.html
>    [48]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14112/shard-skl1/igt@kms_fbcon_fbt@psr-suspend.html
> 
>   * igt@kms_flip@blocking-wf_vblank:
>     - shard-apl:          [INCOMPLETE][49] ([fdo#103927]) ->
> [PASS][50] +1 similar issue
>    [49]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6750/shard-apl4/igt@kms_flip@blocking-wf_vblank.html
>    [50]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14112/shard-apl1/igt@kms_flip@blocking-wf_vblank.html
> 
>   * igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-indfb-draw-blt:
>     - shard-skl:          [FAIL][51] ([fdo#108040]) -> [PASS][52]
>    [51]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6750/shard-skl7/igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-indfb-draw-blt.html
>    [52]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14112/shard-skl10/igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-indfb-draw-blt.html
> 
>   * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-shrfb-plflip-blt:
>     - shard-iclb:         [FAIL][53] ([fdo#103167]) -> [PASS][54] +2
> similar issues
>    [53]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6750/shard-iclb4/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-shrfb-plflip-blt.html
>    [54]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14112/shard-iclb8/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-shrfb-plflip-blt.html
> 
>   * igt@kms_frontbuffer_tracking@psr-1p-offscren-pri-indfb-draw-blt:
>     - shard-skl:          [FAIL][55] ([fdo#103167]) -> [PASS][56]
>    [55]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6750/shard-skl1/igt@kms_frontbuffer_tracking@psr-1p-offscren-pri-indfb-draw-blt.html
>    [56]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14112/shard-skl3/igt@kms_frontbuffer_tracking@psr-1p-offscren-pri-indfb-draw-blt.html
> 
>   * igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min:
>     - shard-skl:          [FAIL][57] ([fdo#108145]) -> [PASS][58] +1
> similar issue
>    [57]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6750/shard-skl1/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html
>    [58]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14112/shard-skl3/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html
> 
>   * igt@kms_plane_alpha_blend@pipe-b-coverage-7efc:
>     - shard-skl:          [FAIL][59] ([fdo#108145] / [fdo#110403]) ->
> [PASS][60]
>    [59]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6750/shard-skl6/igt@kms_plane_alpha_blend@pipe-b-coverage-7efc.html
>    [60]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14112/shard-skl3/igt@kms_plane_alpha_blend@pipe-b-coverage-7efc.html
> 
>   * igt@kms_psr@psr2_primary_render:
>     - shard-iclb:         [SKIP][61] ([fdo#109441]) -> [PASS][62]
>    [61]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6750/shard-iclb1/igt@kms_psr@psr2_primary_render.html
>    [62]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14112/shard-iclb2/igt@kms_psr@psr2_primary_render.html
> 
>   * igt@kms_setmode@basic:
>     - shard-kbl:          [FAIL][63] ([fdo#99912]) -> [PASS][64]
>    [63]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6750/shard-kbl6/igt@kms_setmode@basic.html
>    [64]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14112/shard-kbl2/igt@kms_setmode@basic.html
> 
>   
> #### Warnings ####
> 
>   * igt@gem_ctx_isolation@vcs1-nonpriv:
>     - shard-iclb:         [FAIL][65] ([fdo#111329]) -> [SKIP][66]
> ([fdo#109276])
>    [65]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6750/shard-iclb1/igt@gem_ctx_isolation@vcs1-nonpriv.html
>    [66]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14112/shard-iclb5/igt@gem_ctx_isolation@vcs1-nonpriv.html
> 
>   * igt@gem_mocs_settings@mocs-rc6-bsd2:
>     - shard-iclb:         [SKIP][67] ([fdo#109276]) -> [FAIL][68]
> ([fdo#111330])
>    [67]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6750/shard-iclb3/igt@gem_mocs_settings@mocs-rc6-bsd2.html
>    [68]: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14112/shard-iclb4/igt@gem_mocs_settings@mocs-rc6-bsd2.html
> 
>   
>   [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
>   [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
>   [fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
>   [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
>   [fdo#105767]: https://bugs.freedesktop.org/show_bug.cgi?id=105767
>   [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
>   [fdo#108040]: https://bugs.freedesktop.org/show_bug.cgi?id=108040
>   [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
>   [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
>   [fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
>   [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
>   [fdo#109642]: https://bugs.freedesktop.org/show_bug.cgi?id=109642
>   [fdo#109661]: https://bugs.freedesktop.org/show_bug.cgi?id=109661
>   [fdo#110378]: https://bugs.freedesktop.org/show_bug.cgi?id=110378
>   [fdo#110403]: https://bugs.freedesktop.org/show_bug.cgi?id=110403
>   [fdo#110728]: https://bugs.freedesktop.org/show_bug.cgi?id=110728
>   [fdo#110841]: https://bugs.freedesktop.org/show_bug.cgi?id=110841
>   [fdo#110854]: https://bugs.freedesktop.org/show_bug.cgi?id=110854
>   [fdo#111068]: https://bugs.freedesktop.org/show_bug.cgi?id=111068
>   [fdo#111325]: https://bugs.freedesktop.org/show_bug.cgi?id=111325
>   [fdo#111329]: https://bugs.freedesktop.org/show_bug.cgi?id=111329
>   [fdo#111330]: https://bugs.freedesktop.org/show_bug.cgi?id=111330
>   [fdo#111409]: https://bugs.freedesktop.org/show_bug.cgi?id=111409
>   [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912
> 
> 
> Participating hosts (10 -> 10)
> ------------------------------
> 
>   No changes in participating hosts
> 
> 
> Build changes
> -------------
> 
>   * CI: CI-20190529 -> None
>   * Linux: CI_DRM_6750 -> Patchwork_14112
> 
>   CI-20190529: 20190529
>   CI_DRM_6750: ba37f74dbdc1e78e70a5a2e5f4ce8d762d06eaa7 @
> git://anongit.freedesktop.org/gfx-ci/linux
>   IGT_5146: 357dbe1869d88a2f08bcee4eebceff4ee9014424 @
> git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
>   Patchwork_14112: 1794a17edda30562d292dc4a140797ae38feb71b @
> git://anongit.freedesktop.org/gfx-ci/linux
>   piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @
> git://anongit.freedesktop.org/piglit
> 
> == Logs ==
> 
> For more details see: 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_14112/

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
index 3bfb720560c2..77232f6bca17 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -390,7 +390,7 @@  static void hsw_psr_setup_aux(struct intel_dp *intel_dp)
 
 	BUILD_BUG_ON(sizeof(aux_msg) > 20);
 	for (i = 0; i < sizeof(aux_msg); i += 4)
-		I915_WRITE(EDP_PSR_AUX_DATA(i >> 2),
+		I915_WRITE(EDP_PSR_AUX_DATA(dev_priv->psr.transcoder, i >> 2),
 			   intel_dp_pack_aux(&aux_msg[i], sizeof(aux_msg) - i));
 
 	aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, 0);
@@ -401,7 +401,7 @@  static void hsw_psr_setup_aux(struct intel_dp *intel_dp)
 
 	/* Select only valid bits for SRD_AUX_CTL */
 	aux_ctl &= psr_aux_mask;
-	I915_WRITE(EDP_PSR_AUX_CTL, aux_ctl);
+	I915_WRITE(EDP_PSR_AUX_CTL(dev_priv->psr.transcoder), aux_ctl);
 }
 
 static void intel_psr_enable_sink(struct intel_dp *intel_dp)
@@ -491,8 +491,9 @@  static void hsw_activate_psr1(struct intel_dp *intel_dp)
 	if (INTEL_GEN(dev_priv) >= 8)
 		val |= EDP_PSR_CRC_ENABLE;
 
-	val |= I915_READ(EDP_PSR_CTL) & EDP_PSR_RESTORE_PSR_ACTIVE_CTX_MASK;
-	I915_WRITE(EDP_PSR_CTL, val);
+	val |= (I915_READ(EDP_PSR_CTL(dev_priv->psr.transcoder)) &
+		EDP_PSR_RESTORE_PSR_ACTIVE_CTX_MASK);
+	I915_WRITE(EDP_PSR_CTL(dev_priv->psr.transcoder), val);
 }
 
 static void hsw_activate_psr2(struct intel_dp *intel_dp)
@@ -528,9 +529,9 @@  static void hsw_activate_psr2(struct intel_dp *intel_dp)
 	 * PSR2 HW is incorrectly using EDP_PSR_TP1_TP3_SEL and BSpec is
 	 * recommending keep this bit unset while PSR2 is enabled.
 	 */
-	I915_WRITE(EDP_PSR_CTL, 0);
+	I915_WRITE(EDP_PSR_CTL(dev_priv->psr.transcoder), 0);
 
-	I915_WRITE(EDP_PSR2_CTL, val);
+	I915_WRITE(EDP_PSR2_CTL(dev_priv->psr.transcoder), val);
 }
 
 static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
@@ -606,10 +607,9 @@  void intel_psr_compute_config(struct intel_dp *intel_dp,
 
 	/*
 	 * HSW spec explicitly says PSR is tied to port A.
-	 * BDW+ platforms with DDI implementation of PSR have different
-	 * PSR registers per transcoder and we only implement transcoder EDP
-	 * ones. Since by Display design transcoder EDP is tied to port A
-	 * we can safely escape based on the port A.
+	 * BDW+ platforms have a instance of PSR registers per transcoder but
+	 * for now it only supports one instance of PSR, so lets keep it
+	 * hardcoded to PORT_A
 	 */
 	if (dig_port->base.port != PORT_A) {
 		DRM_DEBUG_KMS("PSR condition failed: Port not supported\n");
@@ -649,8 +649,8 @@  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)
-		WARN_ON(I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE);
-	WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
+		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);
 	lockdep_assert_held(&dev_priv->psr.lock);
 
@@ -720,19 +720,37 @@  static void intel_psr_enable_source(struct intel_dp *intel_dp,
 	if (INTEL_GEN(dev_priv) < 11)
 		mask |= EDP_PSR_DEBUG_MASK_DISP_REG_WRITE;
 
-	I915_WRITE(EDP_PSR_DEBUG, mask);
+	I915_WRITE(EDP_PSR_DEBUG(dev_priv->psr.transcoder), mask);
 }
 
 static void intel_psr_enable_locked(struct drm_i915_private *dev_priv,
 				    const struct intel_crtc_state *crtc_state)
 {
 	struct intel_dp *intel_dp = dev_priv->psr.dp;
+	u32 val;
 
 	WARN_ON(dev_priv->psr.enabled);
 
 	dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv, crtc_state);
 	dev_priv->psr.busy_frontbuffer_bits = 0;
 	dev_priv->psr.pipe = to_intel_crtc(crtc_state->base.crtc)->pipe;
+	dev_priv->psr.transcoder = crtc_state->cpu_transcoder;
+
+	/*
+	 * If a PSR error happened and the driver is reloaded, the EDP_PSR_IIR
+	 * will still keep the error set even after the reset done in the
+	 * irq_preinstall and irq_uninstall hooks.
+	 * And enabling in this situation cause the screen to freeze in the
+	 * 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(edp_psr_shift(dev_priv->psr.transcoder));
+	if (val) {
+		dev_priv->psr.sink_not_reliable = true;
+		DRM_DEBUG_KMS("PSR interruption error set, not enabling PSR\n");
+		return;
+	}
 
 	DRM_DEBUG_KMS("Enabling PSR%s\n",
 		      dev_priv->psr.psr2_enabled ? "2" : "1");
@@ -782,20 +800,27 @@  static void intel_psr_exit(struct drm_i915_private *dev_priv)
 	u32 val;
 
 	if (!dev_priv->psr.active) {
-		if (INTEL_GEN(dev_priv) >= 9)
-			WARN_ON(I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE);
-		WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
+		if (INTEL_GEN(dev_priv) >= 9) {
+			val = I915_READ(EDP_PSR2_CTL(dev_priv->psr.transcoder));
+			WARN_ON(val & EDP_PSR2_ENABLE);
+		}
+
+		val = I915_READ(EDP_PSR_CTL(dev_priv->psr.transcoder));
+		WARN_ON(val & EDP_PSR_ENABLE);
+
 		return;
 	}
 
 	if (dev_priv->psr.psr2_enabled) {
-		val = I915_READ(EDP_PSR2_CTL);
+		val = I915_READ(EDP_PSR2_CTL(dev_priv->psr.transcoder));
 		WARN_ON(!(val & EDP_PSR2_ENABLE));
-		I915_WRITE(EDP_PSR2_CTL, val & ~EDP_PSR2_ENABLE);
+		val &= ~EDP_PSR2_ENABLE;
+		I915_WRITE(EDP_PSR2_CTL(dev_priv->psr.transcoder), val);
 	} else {
-		val = I915_READ(EDP_PSR_CTL);
+		val = I915_READ(EDP_PSR_CTL(dev_priv->psr.transcoder));
 		WARN_ON(!(val & EDP_PSR_ENABLE));
-		I915_WRITE(EDP_PSR_CTL, val & ~EDP_PSR_ENABLE);
+		val &= ~EDP_PSR_ENABLE;
+		I915_WRITE(EDP_PSR_CTL(dev_priv->psr.transcoder), val);
 	}
 	dev_priv->psr.active = false;
 }
@@ -817,10 +842,10 @@  static void intel_psr_disable_locked(struct intel_dp *intel_dp)
 	intel_psr_exit(dev_priv);
 
 	if (dev_priv->psr.psr2_enabled) {
-		psr_status = EDP_PSR2_STATUS;
+		psr_status = EDP_PSR2_STATUS(dev_priv->psr.transcoder);
 		psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
 	} else {
-		psr_status = EDP_PSR_STATUS;
+		psr_status = EDP_PSR_STATUS(dev_priv->psr.transcoder);
 		psr_status_mask = EDP_PSR_STATUS_STATE_MASK;
 	}
 
@@ -963,7 +988,8 @@  int intel_psr_wait_for_idle(const struct intel_crtc_state *new_crtc_state,
 	 * defensive enough to cover everything.
 	 */
 
-	return __intel_wait_for_register(&dev_priv->uncore, EDP_PSR_STATUS,
+	return __intel_wait_for_register(&dev_priv->uncore,
+					 EDP_PSR_STATUS(dev_priv->psr.transcoder),
 					 EDP_PSR_STATUS_STATE_MASK,
 					 EDP_PSR_STATUS_STATE_IDLE, 2, 50,
 					 out_value);
@@ -979,10 +1005,10 @@  static bool __psr_wait_for_idle_locked(struct drm_i915_private *dev_priv)
 		return false;
 
 	if (dev_priv->psr.psr2_enabled) {
-		reg = EDP_PSR2_STATUS;
+		reg = EDP_PSR2_STATUS(dev_priv->psr.transcoder);
 		mask = EDP_PSR2_STATUS_STATE_MASK;
 	} else {
-		reg = EDP_PSR_STATUS;
+		reg = EDP_PSR_STATUS(dev_priv->psr.transcoder);
 		mask = EDP_PSR_STATUS_STATE_MASK;
 	}
 
@@ -1208,36 +1234,24 @@  void intel_psr_flush(struct drm_i915_private *dev_priv,
  */
 void intel_psr_init(struct drm_i915_private *dev_priv)
 {
-	u32 val;
-
 	if (!HAS_PSR(dev_priv))
 		return;
 
-	dev_priv->psr_mmio_base = IS_HASWELL(dev_priv) ?
-		HSW_EDP_PSR_BASE : BDW_EDP_PSR_BASE;
-
 	if (!dev_priv->psr.sink_support)
 		return;
 
+	if (IS_HASWELL(dev_priv))
+		/*
+		 * HSW don't have PSR registers on the same space as transcoder
+		 * so set this to a value that when subtract to the register
+		 * in transcoder space results in the right offset for HSW
+		 */
+		dev_priv->hsw_psr_mmio_adjust = _SRD_CTL_EDP - _HSW_EDP_PSR_BASE;
+
 	if (i915_modparams.enable_psr == -1)
 		if (INTEL_GEN(dev_priv) < 9 || !dev_priv->vbt.psr.enable)
 			i915_modparams.enable_psr = 0;
 
-	/*
-	 * If a PSR error happened and the driver is reloaded, the EDP_PSR_IIR
-	 * will still keep the error set even after the reset done in the
-	 * irq_preinstall and irq_uninstall hooks.
-	 * And enabling in this situation cause the screen to freeze in the
-	 * 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(edp_psr_shift(TRANSCODER_EDP));
-	if (val) {
-		DRM_DEBUG_KMS("PSR interruption error set\n");
-		dev_priv->psr.sink_not_reliable = true;
-	}
-
 	/* Set link_standby x link_off defaults */
 	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
 		/* HSW and BDW require workarounds that we don't implement. */
diff --git a/drivers/gpu/drm/i915/gvt/handlers.c b/drivers/gpu/drm/i915/gvt/handlers.c
index 25f78196b964..45a9124e53b6 100644
--- a/drivers/gpu/drm/i915/gvt/handlers.c
+++ b/drivers/gpu/drm/i915/gvt/handlers.c
@@ -2796,7 +2796,7 @@  static int init_broadwell_mmio_info(struct intel_gvt *gvt)
 	MMIO_D(CHICKEN_PIPESL_1(PIPE_C), D_BDW_PLUS);
 
 	MMIO_D(WM_MISC, D_BDW);
-	MMIO_D(_MMIO(BDW_EDP_PSR_BASE), D_BDW);
+	MMIO_D(_MMIO(_SRD_CTL_EDP), D_BDW);
 
 	MMIO_D(_MMIO(0x6671c), D_BDW_PLUS);
 	MMIO_D(_MMIO(0x66c00), D_BDW_PLUS);
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index b39226d7f8d2..6e4824daafae 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2132,7 +2132,7 @@  psr_source_status(struct drm_i915_private *dev_priv, struct seq_file *m)
 			"BUF_ON",
 			"TG_ON"
 		};
-		val = I915_READ(EDP_PSR2_STATUS);
+		val = I915_READ(EDP_PSR2_STATUS(dev_priv->psr.transcoder));
 		status_val = (val & EDP_PSR2_STATUS_STATE_MASK) >>
 			      EDP_PSR2_STATUS_STATE_SHIFT;
 		if (status_val < ARRAY_SIZE(live_status))
@@ -2148,7 +2148,7 @@  psr_source_status(struct drm_i915_private *dev_priv, struct seq_file *m)
 			"SRDOFFACK",
 			"SRDENT_ON",
 		};
-		val = I915_READ(EDP_PSR_STATUS);
+		val = I915_READ(EDP_PSR_STATUS(dev_priv->psr.transcoder));
 		status_val = (val & EDP_PSR_STATUS_STATE_MASK) >>
 			      EDP_PSR_STATUS_STATE_SHIFT;
 		if (status_val < ARRAY_SIZE(live_status))
@@ -2191,10 +2191,10 @@  static int i915_edp_psr_status(struct seq_file *m, void *data)
 		goto unlock;
 
 	if (psr->psr2_enabled) {
-		val = I915_READ(EDP_PSR2_CTL);
+		val = I915_READ(EDP_PSR2_CTL(dev_priv->psr.transcoder));
 		enabled = val & EDP_PSR2_ENABLE;
 	} else {
-		val = I915_READ(EDP_PSR_CTL);
+		val = I915_READ(EDP_PSR_CTL(dev_priv->psr.transcoder));
 		enabled = val & EDP_PSR_ENABLE;
 	}
 	seq_printf(m, "Source PSR ctl: %s [0x%08x]\n",
@@ -2207,7 +2207,8 @@  static int i915_edp_psr_status(struct seq_file *m, void *data)
 	 * SKL+ Perf counter is reset to 0 everytime DC state is entered
 	 */
 	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
-		val = I915_READ(EDP_PSR_PERF_CNT) & EDP_PSR_PERF_CNT_MASK;
+		val = I915_READ(EDP_PSR_PERF_CNT(dev_priv->psr.transcoder));
+		val &= EDP_PSR_PERF_CNT_MASK;
 		seq_printf(m, "Performance counter: %u\n", val);
 	}
 
@@ -2225,8 +2226,11 @@  static int i915_edp_psr_status(struct seq_file *m, void *data)
 		 * Reading all 3 registers before hand to minimize crossing a
 		 * frame boundary between register reads
 		 */
-		for (frame = 0; frame < PSR2_SU_STATUS_FRAMES; frame += 3)
-			su_frames_val[frame / 3] = I915_READ(PSR2_SU_STATUS(frame));
+		for (frame = 0; frame < PSR2_SU_STATUS_FRAMES; frame += 3) {
+			val = I915_READ(PSR2_SU_STATUS(dev_priv->psr.transcoder,
+						       frame));
+			su_frames_val[frame / 3] = val;
+		}
 
 		seq_puts(m, "Frame:\tPSR2 SU blocks:\n");
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index eb31c1656cea..be999791abca 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -479,6 +479,7 @@  struct i915_psr {
 	bool enabled;
 	struct intel_dp *dp;
 	enum pipe pipe;
+	enum transcoder transcoder;
 	bool active;
 	struct work_struct work;
 	unsigned busy_frontbuffer_bits;
@@ -1330,11 +1331,11 @@  struct drm_i915_private {
 	 */
 	u32 gpio_mmio_base;
 
+	u32 hsw_psr_mmio_adjust;
+
 	/* MMIO base address for MIPI regs */
 	u32 mipi_mmio_base;
 
-	u32 psr_mmio_base;
-
 	u32 pps_mmio_base;
 
 	wait_queue_head_t gmbus_wait_queue;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 2abd199093c5..a092b34c269d 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4186,10 +4186,17 @@  enum {
 #define PIPESRC(trans)		_MMIO_TRANS2(trans, _PIPEASRC)
 #define PIPE_MULT(trans)	_MMIO_TRANS2(trans, _PIPE_MULT_A)
 
-/* HSW+ eDP PSR registers */
-#define HSW_EDP_PSR_BASE	0x64800
-#define BDW_EDP_PSR_BASE	0x6f800
-#define EDP_PSR_CTL				_MMIO(dev_priv->psr_mmio_base + 0)
+/*
+ * HSW+ eDP PSR registers
+ *
+ * HSW PSR registers are relative to DDIA(_DDI_BUF_CTL_A + 0x800) with just one
+ * instance of it
+ */
+#define _HSW_EDP_PSR_BASE			0x64800
+#define _SRD_CTL_A				0x60800
+#define _SRD_CTL_EDP				0x6f800
+#define _PSR_ADJ(tran, reg)			(_TRANS2(tran, reg) - dev_priv->hsw_psr_mmio_adjust)
+#define EDP_PSR_CTL(tran)			_MMIO(_PSR_ADJ(tran, _SRD_CTL_A))
 #define   EDP_PSR_ENABLE			(1 << 31)
 #define   BDW_PSR_SINGLE_FRAME			(1 << 30)
 #define   EDP_PSR_RESTORE_PSR_ACTIVE_CTX_MASK	(1 << 29) /* SW can't modify */
@@ -4226,16 +4233,22 @@  enum {
 #define   EDP_PSR_TRANSCODER_A_SHIFT		8
 #define   EDP_PSR_TRANSCODER_EDP_SHIFT		0
 
-#define EDP_PSR_AUX_CTL				_MMIO(dev_priv->psr_mmio_base + 0x10)
+#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))
 #define   EDP_PSR_AUX_CTL_TIME_OUT_MASK		(3 << 26)
 #define   EDP_PSR_AUX_CTL_MESSAGE_SIZE_MASK	(0x1f << 20)
 #define   EDP_PSR_AUX_CTL_PRECHARGE_2US_MASK	(0xf << 16)
 #define   EDP_PSR_AUX_CTL_ERROR_INTERRUPT	(1 << 11)
 #define   EDP_PSR_AUX_CTL_BIT_CLOCK_2X_MASK	(0x7ff)
 
-#define EDP_PSR_AUX_DATA(i)			_MMIO(dev_priv->psr_mmio_base + 0x14 + (i) * 4) /* 5 registers */
+#define _SRD_AUX_DATA_A				0x60814
+#define _SRD_AUX_DATA_EDP			0x6f814
+#define EDP_PSR_AUX_DATA(tran, i)		_MMIO(_PSR_ADJ(tran, _SRD_AUX_DATA_A) + (i) + 4) /* 5 registers */
 
-#define EDP_PSR_STATUS				_MMIO(dev_priv->psr_mmio_base + 0x40)
+#define _SRD_STATUS_A				0x60840
+#define _SRD_STATUS_EDP				0x6f840
+#define EDP_PSR_STATUS(tran)			_MMIO(_PSR_ADJ(tran, _SRD_STATUS_A))
 #define   EDP_PSR_STATUS_STATE_MASK		(7 << 29)
 #define   EDP_PSR_STATUS_STATE_SHIFT		29
 #define   EDP_PSR_STATUS_STATE_IDLE		(0 << 29)
@@ -4260,10 +4273,15 @@  enum {
 #define   EDP_PSR_STATUS_SENDING_TP1		(1 << 4)
 #define   EDP_PSR_STATUS_IDLE_MASK		0xf
 
-#define EDP_PSR_PERF_CNT		_MMIO(dev_priv->psr_mmio_base + 0x44)
+#define _SRD_PERF_CNT_A			0x60844
+#define _SRD_PERF_CNT_EDP		0x6f844
+#define EDP_PSR_PERF_CNT(tran)		_MMIO(_PSR_ADJ(tran, _SRD_PERF_CNT_A))
 #define   EDP_PSR_PERF_CNT_MASK		0xffffff
 
-#define EDP_PSR_DEBUG				_MMIO(dev_priv->psr_mmio_base + 0x60) /* PSR_MASK on SKL+ */
+/* PSR_MASK on SKL+ */
+#define _SRD_DEBUG_A				0x60860
+#define _SRD_DEBUG_EDP				0x6f860
+#define EDP_PSR_DEBUG(tran)			_MMIO(_PSR_ADJ(tran, _SRD_DEBUG_A))
 #define   EDP_PSR_DEBUG_MASK_MAX_SLEEP         (1 << 28)
 #define   EDP_PSR_DEBUG_MASK_LPSP              (1 << 27)
 #define   EDP_PSR_DEBUG_MASK_MEMUP             (1 << 26)
@@ -4271,7 +4289,9 @@  enum {
 #define   EDP_PSR_DEBUG_MASK_DISP_REG_WRITE    (1 << 16) /* Reserved in ICL+ */
 #define   EDP_PSR_DEBUG_EXIT_ON_PIXEL_UNDERRUN (1 << 15) /* SKL+ */
 
-#define EDP_PSR2_CTL			_MMIO(0x6f900)
+#define _PSR2_CTL_A			0x60900
+#define _PSR2_CTL_EDP			0x6f900
+#define EDP_PSR2_CTL(tran)		_MMIO_TRANS2(tran, _PSR2_CTL_A)
 #define   EDP_PSR2_ENABLE		(1 << 31)
 #define   EDP_SU_TRACK_ENABLE		(1 << 30)
 #define   EDP_Y_COORDINATE_VALID	(1 << 26) /* GLK and CNL+ */
@@ -4293,8 +4313,8 @@  enum {
 #define _PSR_EVENT_TRANS_B			0x61848
 #define _PSR_EVENT_TRANS_C			0x62848
 #define _PSR_EVENT_TRANS_D			0x63848
-#define _PSR_EVENT_TRANS_EDP			0x6F848
-#define PSR_EVENT(trans)			_MMIO_TRANS2(trans, _PSR_EVENT_TRANS_A)
+#define _PSR_EVENT_TRANS_EDP			0x6f848
+#define PSR_EVENT(tran)				_MMIO_TRANS2(tran, _PSR_EVENT_TRANS_A)
 #define  PSR_EVENT_PSR2_WD_TIMER_EXPIRE		(1 << 17)
 #define  PSR_EVENT_PSR2_DISABLED		(1 << 16)
 #define  PSR_EVENT_SU_DIRTY_FIFO_UNDERRUN	(1 << 15)
@@ -4312,15 +4332,16 @@  enum {
 #define  PSR_EVENT_LPSP_MODE_EXIT		(1 << 1)
 #define  PSR_EVENT_PSR_DISABLE			(1 << 0)
 
-#define EDP_PSR2_STATUS			_MMIO(0x6f940)
+#define _PSR2_STATUS_A			0x60940
+#define _PSR2_STATUS_EDP		0x6f940
+#define EDP_PSR2_STATUS(tran)		_MMIO_TRANS2(tran, _PSR2_STATUS_A)
 #define EDP_PSR2_STATUS_STATE_MASK     (0xf << 28)
 #define EDP_PSR2_STATUS_STATE_SHIFT    28
 
-#define _PSR2_SU_STATUS_0		0x6F914
-#define _PSR2_SU_STATUS_1		0x6F918
-#define _PSR2_SU_STATUS_2		0x6F91C
-#define _PSR2_SU_STATUS(index)		_MMIO(_PICK_EVEN((index), _PSR2_SU_STATUS_0, _PSR2_SU_STATUS_1))
-#define PSR2_SU_STATUS(frame)		(_PSR2_SU_STATUS((frame) / 3))
+#define _PSR2_SU_STATUS_A		0x60914
+#define _PSR2_SU_STATUS_EDP		0x6f914
+#define _PSR2_SU_STATUS(tran, index)	_MMIO(_TRANS2(tran, _PSR2_SU_STATUS_A) + (index) * 4)
+#define PSR2_SU_STATUS(tran, frame)	(_PSR2_SU_STATUS(tran, (frame) / 3))
 #define PSR2_SU_STATUS_SHIFT(frame)	(((frame) % 3) * 10)
 #define PSR2_SU_STATUS_MASK(frame)	(0x3ff << PSR2_SU_STATUS_SHIFT(frame))
 #define PSR2_SU_STATUS_FRAMES		8