[03/10] drm/i915/psr: fix blank screen issue for psr2
diff mbox

Message ID 1483644359-17866-1-git-send-email-vathsala.nagaraju@intel.com
State New
Headers show

Commit Message

vathsala nagaraju Jan. 5, 2017, 7:25 p.m. UTC
Psr1 and psr2 are mutually exclusive,ie when psr2 is enabled,
psr1 should be disabled.When psr2 is exited , bit 31 of reg
PSR2_CTL must be set to 0 but currently bit 31 of SRD_CTL
(psr1 control register)is set to 0.
Also ,PSR2_IDLE state is looked up from SRD_STATUS(psr1 register)
instead of PSR2_STATUS register, which has wrong data, resulting
in blankscreen.
hsw_enable_source is split into hsw_enable_source_psr1 and
hsw_enable_source_psr2 for easier code review and maintenance,
as suggested by rodrigo and jim.

v2: (Vivi Rodrigo)
- Rename hsw_enable_source_psr* to intel_enable_source_psr*

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Jim Bride <jim.bride@linux.intel.com>
Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
Signed-off-by: Patil Deepti <deepti.patil@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h  |   3 +
 drivers/gpu/drm/i915/intel_psr.c | 124 +++++++++++++++++++++++++++++----------
 2 files changed, 97 insertions(+), 30 deletions(-)

Comments

jim.bride@linux.intel.com Jan. 5, 2017, 8:37 p.m. UTC | #1
On Fri, Jan 06, 2017 at 12:55:59AM +0530, vathsala nagaraju wrote:
> Psr1 and psr2 are mutually exclusive,ie when psr2 is enabled,
> psr1 should be disabled.When psr2 is exited , bit 31 of reg
> PSR2_CTL must be set to 0 but currently bit 31 of SRD_CTL
> (psr1 control register)is set to 0.
> Also ,PSR2_IDLE state is looked up from SRD_STATUS(psr1 register)
> instead of PSR2_STATUS register, which has wrong data, resulting
> in blankscreen.
> hsw_enable_source is split into hsw_enable_source_psr1 and
> hsw_enable_source_psr2 for easier code review and maintenance,
> as suggested by rodrigo and jim.
> 
> v2: (Vivi Rodrigo)
> - Rename hsw_enable_source_psr* to intel_enable_source_psr*
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Jim Bride <jim.bride@linux.intel.com>
> Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> Signed-off-by: Patil Deepti <deepti.patil@intel.com>

The new naming is much better!

Reviewed-by: Jim Bride <jim.bride@linux.intel.com>


> ---
>  drivers/gpu/drm/i915/i915_reg.h  |   3 +
>  drivers/gpu/drm/i915/intel_psr.c | 124 +++++++++++++++++++++++++++++----------
>  2 files changed, 97 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 00970aa..7830e6e 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3615,6 +3615,9 @@ enum {
>  #define   EDP_PSR2_FRAME_BEFORE_SU_MASK	(0xf<<4)
>  #define   EDP_PSR2_IDLE_MASK		0xf
>  
> +#define EDP_PSR2_STATUS_CTL            _MMIO(0x6f940)
> +#define EDP_PSR2_STATUS_STATE_MASK     (0xf<<28)
> +
>  /* VGA port control */
>  #define ADPA			_MMIO(0x61100)
>  #define PCH_ADPA                _MMIO(0xe1100)
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index c3aa649..d5e8bcc 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -261,12 +261,11 @@ static void vlv_psr_activate(struct intel_dp *intel_dp)
>  		   VLV_EDP_PSR_ACTIVE_ENTRY);
>  }
>  
> -static void hsw_psr_enable_source(struct intel_dp *intel_dp)
> +static void intel_enable_source_psr1(struct intel_dp *intel_dp)
>  {
>  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>  	struct drm_device *dev = dig_port->base.base.dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> -
>  	uint32_t max_sleep_time = 0x1f;
>  	/*
>  	 * Let's respect VBT in case VBT asks a higher idle_frame value.
> @@ -312,14 +311,30 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp)
>  		val |= EDP_PSR_TP1_TP2_SEL;
>  
>  	I915_WRITE(EDP_PSR_CTL, val);
> +}
>  
> -	if (!dev_priv->psr.psr2_support)
> -		return;
> +static void intel_enable_source_psr2(struct intel_dp *intel_dp)
> +{
> +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_device *dev = dig_port->base.base.dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +
> +	/*
> +	 * Let's respect VBT in case VBT asks a higher idle_frame value.
> +	 * Let's use 6 as the minimum to cover all known cases including
> +	 * the off-by-one issue that HW has in some cases. Also there are
> +	 * cases where sink should be able to train
> +	 * with the 5 or 6 idle patterns.
> +	 */
> +	uint32_t idle_frames = max(6, dev_priv->vbt.psr.idle_frames);
> +	uint32_t val = EDP_PSR_ENABLE;
> +
> +	val |= idle_frames << EDP_PSR_IDLE_FRAME_SHIFT;
>  
>  	/* FIXME: selective update is probably totally broken because it doesn't
>  	 * mesh at all with our frontbuffer tracking. And the hw alone isn't
>  	 * good enough. */
> -	val = EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE;
> +	val |= EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE;
>  
>  	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 5)
>  		val |= EDP_PSR2_TP2_TIME_2500;
> @@ -333,6 +348,20 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp)
>  	I915_WRITE(EDP_PSR2_CTL, val);
>  }
>  
> +
> +static void hsw_psr_enable_source(struct intel_dp *intel_dp)
> +{
> +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_device *dev = dig_port->base.base.dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +
> +	/* psr1 and psr2 are mutually exclusive.*/
> +	if (dev_priv->psr.psr2_support)
> +		intel_enable_source_psr2(intel_dp);
> +	else
> +		intel_enable_source_psr1(intel_dp);
> +}
> +
>  static bool intel_psr_match_conditions(struct intel_dp *intel_dp)
>  {
>  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> @@ -410,7 +439,10 @@ static void intel_psr_activate(struct intel_dp *intel_dp)
>  	struct drm_device *dev = intel_dig_port->base.base.dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  
> -	WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
> +	if (dev_priv->psr.psr2_support)
> +		WARN_ON(I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE);
> +	else
> +		WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
>  	WARN_ON(dev_priv->psr.active);
>  	lockdep_assert_held(&dev_priv->psr.lock);
>  
> @@ -462,8 +494,6 @@ void intel_psr_enable(struct intel_dp *intel_dp)
>  	dev_priv->psr.busy_frontbuffer_bits = 0;
>  
>  	if (HAS_DDI(dev_priv)) {
> -		hsw_psr_setup_vsc(intel_dp);
> -
>  		if (dev_priv->psr.psr2_support) {
>  			/* PSR2 is restricted to work with panel resolutions upto 3200x2000 */
>  			if (crtc->config->pipe_src_w > 3200 ||
> @@ -471,8 +501,10 @@ void intel_psr_enable(struct intel_dp *intel_dp)
>  				dev_priv->psr.psr2_support = false;
>  			else
>  				skl_psr_setup_su_vsc(intel_dp);
> +		} else {
> +			/* set up vsc header for psr1 */
> +			hsw_psr_setup_vsc(intel_dp);
>  		}
> -
>  		/*
>  		 * Per Spec: Avoid continuous PSR exit by masking MEMUP and HPD.
>  		 * Also mask LPSP to avoid dependency on other drivers that
> @@ -557,20 +589,37 @@ static void hsw_psr_disable(struct intel_dp *intel_dp)
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  
>  	if (dev_priv->psr.active) {
> -		I915_WRITE(EDP_PSR_CTL,
> -			   I915_READ(EDP_PSR_CTL) & ~EDP_PSR_ENABLE);
> +		if (dev_priv->psr.psr2_support)
> +			I915_WRITE(EDP_PSR2_CTL,
> +				I915_READ(EDP_PSR2_CTL) &
> +					~(EDP_PSR2_ENABLE |
> +					EDP_SU_TRACK_ENABLE));
> +		else
> +			I915_WRITE(EDP_PSR_CTL,
> +				   I915_READ(EDP_PSR_CTL) & ~EDP_PSR_ENABLE);
>  
>  		/* Wait till PSR is idle */
> -		if (intel_wait_for_register(dev_priv,
> -					    EDP_PSR_STATUS_CTL,
> -					    EDP_PSR_STATUS_STATE_MASK,
> -					    0,
> -					    2000))
> +		if (dev_priv->psr.psr2_support) {
> +			if (intel_wait_for_register(dev_priv,
> +						    EDP_PSR2_STATUS_CTL,
> +						    EDP_PSR2_STATUS_STATE_MASK,
> +						    0,
> +						    2000))
> +			DRM_ERROR("Timed out waiting for PSR2 Idle State\n");
> +		} else {
> +			if (intel_wait_for_register(dev_priv,
> +						    EDP_PSR_STATUS_CTL,
> +						    EDP_PSR_STATUS_STATE_MASK,
> +						    0,
> +						    2000))
>  			DRM_ERROR("Timed out waiting for PSR Idle State\n");
> -
> +		}
>  		dev_priv->psr.active = false;
>  	} else {
> -		WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
> +		if (dev_priv->psr.psr2_support)
> +			WARN_ON(I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE);
> +		else
> +			WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
>  	}
>  }
>  
> @@ -621,13 +670,24 @@ static void intel_psr_work(struct work_struct *work)
>  	 * and be ready for re-enable.
>  	 */
>  	if (HAS_DDI(dev_priv)) {
> -		if (intel_wait_for_register(dev_priv,
> -					    EDP_PSR_STATUS_CTL,
> -					    EDP_PSR_STATUS_STATE_MASK,
> -					    0,
> -					    50)) {
> -			DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n");
> -			return;
> +		if (dev_priv->psr.psr2_support) {
> +			if (intel_wait_for_register(dev_priv,
> +						EDP_PSR2_STATUS_CTL,
> +						EDP_PSR2_STATUS_STATE_MASK,
> +						0,
> +						50)) {
> +				DRM_ERROR("Timed out waiting for PSR2 Idle for re-enable\n");
> +				return;
> +			}
> +		} else {
> +			if (intel_wait_for_register(dev_priv,
> +						EDP_PSR_STATUS_CTL,
> +						EDP_PSR_STATUS_STATE_MASK,
> +						0,
> +						50)) {
> +				DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n");
> +				return;
> +			}
>  		}
>  	} else {
>  		if (intel_wait_for_register(dev_priv,
> @@ -669,11 +729,15 @@ static void intel_psr_exit(struct drm_i915_private *dev_priv)
>  		return;
>  
>  	if (HAS_DDI(dev_priv)) {
> -		val = I915_READ(EDP_PSR_CTL);
> -
> -		WARN_ON(!(val & EDP_PSR_ENABLE));
> -
> -		I915_WRITE(EDP_PSR_CTL, val & ~EDP_PSR_ENABLE);
> +		if (dev_priv->psr.psr2_support) {
> +			val = I915_READ(EDP_PSR2_CTL);
> +			WARN_ON(!(val & EDP_PSR2_ENABLE));
> +			I915_WRITE(EDP_PSR2_CTL, val & ~EDP_PSR2_ENABLE);
> +		} else {
> +			val = I915_READ(EDP_PSR_CTL);
> +			WARN_ON(!(val & EDP_PSR_ENABLE));
> +			I915_WRITE(EDP_PSR_CTL, val & ~EDP_PSR_ENABLE);
> +		}
>  	} else {
>  		val = I915_READ(VLV_PSRCTL(pipe));
>  
> -- 
> 1.9.1
Rodrigo Vivi Jan. 5, 2017, 8:41 p.m. UTC | #2
On Fri, 2017-01-06 at 00:55 +0530, vathsala nagaraju wrote:
> Psr1 and psr2 are mutually exclusive,ie when psr2 is enabled,

> psr1 should be disabled.When psr2 is exited , bit 31 of reg

> PSR2_CTL must be set to 0 but currently bit 31 of SRD_CTL

> (psr1 control register)is set to 0.

> Also ,PSR2_IDLE state is looked up from SRD_STATUS(psr1 register)

> instead of PSR2_STATUS register, which has wrong data, resulting

> in blankscreen.

> hsw_enable_source is split into hsw_enable_source_psr1 and

> hsw_enable_source_psr2 for easier code review and maintenance,

> as suggested by rodrigo and jim.

> 

> v2: (Vivi Rodrigo)

> - Rename hsw_enable_source_psr* to intel_enable_source_psr*

> 

> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>

> Cc: Jim Bride <jim.bride@linux.intel.com>

> Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>

> Signed-off-by: Patil Deepti <deepti.patil@intel.com>

> ---

>  drivers/gpu/drm/i915/i915_reg.h  |   3 +

>  drivers/gpu/drm/i915/intel_psr.c | 124 +++++++++++++++++++++++++++++----------

>  2 files changed, 97 insertions(+), 30 deletions(-)

> 

> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h

> index 00970aa..7830e6e 100644

> --- a/drivers/gpu/drm/i915/i915_reg.h

> +++ b/drivers/gpu/drm/i915/i915_reg.h

> @@ -3615,6 +3615,9 @@ enum {

>  #define   EDP_PSR2_FRAME_BEFORE_SU_MASK	(0xf<<4)

>  #define   EDP_PSR2_IDLE_MASK		0xf

>  

> +#define EDP_PSR2_STATUS_CTL            _MMIO(0x6f940)

> +#define EDP_PSR2_STATUS_STATE_MASK     (0xf<<28)

> +

>  /* VGA port control */

>  #define ADPA			_MMIO(0x61100)

>  #define PCH_ADPA                _MMIO(0xe1100)

> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c

> index c3aa649..d5e8bcc 100644

> --- a/drivers/gpu/drm/i915/intel_psr.c

> +++ b/drivers/gpu/drm/i915/intel_psr.c

> @@ -261,12 +261,11 @@ static void vlv_psr_activate(struct intel_dp *intel_dp)

>  		   VLV_EDP_PSR_ACTIVE_ENTRY);

>  }

>  

> -static void hsw_psr_enable_source(struct intel_dp *intel_dp)

> +static void intel_enable_source_psr1(struct intel_dp *intel_dp)

>  {

>  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);

>  	struct drm_device *dev = dig_port->base.base.dev;

>  	struct drm_i915_private *dev_priv = to_i915(dev);

> -

>  	uint32_t max_sleep_time = 0x1f;

>  	/*

>  	 * Let's respect VBT in case VBT asks a higher idle_frame value.

> @@ -312,14 +311,30 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp)

>  		val |= EDP_PSR_TP1_TP2_SEL;

>  

>  	I915_WRITE(EDP_PSR_CTL, val);

> +}

>  

> -	if (!dev_priv->psr.psr2_support)

> -		return;

> +static void intel_enable_source_psr2(struct intel_dp *intel_dp)

> +{

> +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);

> +	struct drm_device *dev = dig_port->base.base.dev;

> +	struct drm_i915_private *dev_priv = to_i915(dev);

> +

> +	/*

> +	 * Let's respect VBT in case VBT asks a higher idle_frame value.

> +	 * Let's use 6 as the minimum to cover all known cases including

> +	 * the off-by-one issue that HW has in some cases. Also there are

> +	 * cases where sink should be able to train

> +	 * with the 5 or 6 idle patterns.

> +	 */

> +	uint32_t idle_frames = max(6, dev_priv->vbt.psr.idle_frames);

> +	uint32_t val = EDP_PSR_ENABLE;

> +

> +	val |= idle_frames << EDP_PSR_IDLE_FRAME_SHIFT;

>  

>  	/* FIXME: selective update is probably totally broken because it doesn't

>  	 * mesh at all with our frontbuffer tracking. And the hw alone isn't

>  	 * good enough. */

> -	val = EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE;

> +	val |= EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE;

>  

>  	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 5)

>  		val |= EDP_PSR2_TP2_TIME_2500;

> @@ -333,6 +348,20 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp)

>  	I915_WRITE(EDP_PSR2_CTL, val);

>  }

>  

> +

> +static void hsw_psr_enable_source(struct intel_dp *intel_dp)

> +{

> +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);

> +	struct drm_device *dev = dig_port->base.base.dev;

> +	struct drm_i915_private *dev_priv = to_i915(dev);

> +

> +	/* psr1 and psr2 are mutually exclusive.*/

> +	if (dev_priv->psr.psr2_support)

> +		intel_enable_source_psr2(intel_dp);

> +	else

> +		intel_enable_source_psr1(intel_dp);

> +}

> +

>  static bool intel_psr_match_conditions(struct intel_dp *intel_dp)

>  {

>  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);

> @@ -410,7 +439,10 @@ static void intel_psr_activate(struct intel_dp *intel_dp)

>  	struct drm_device *dev = intel_dig_port->base.base.dev;

>  	struct drm_i915_private *dev_priv = to_i915(dev);

>  

> -	WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);

> +	if (dev_priv->psr.psr2_support)

> +		WARN_ON(I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE);

> +	else

> +		WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);

>  	WARN_ON(dev_priv->psr.active);

>  	lockdep_assert_held(&dev_priv->psr.lock);

>  

> @@ -462,8 +494,6 @@ void intel_psr_enable(struct intel_dp *intel_dp)

>  	dev_priv->psr.busy_frontbuffer_bits = 0;

>  

>  	if (HAS_DDI(dev_priv)) {

> -		hsw_psr_setup_vsc(intel_dp);

> -

>  		if (dev_priv->psr.psr2_support) {

>  			/* PSR2 is restricted to work with panel resolutions upto 3200x2000 */

>  			if (crtc->config->pipe_src_w > 3200 ||

> @@ -471,8 +501,10 @@ void intel_psr_enable(struct intel_dp *intel_dp)

>  				dev_priv->psr.psr2_support = false;

>  			else

>  				skl_psr_setup_su_vsc(intel_dp);

> +		} else {

> +			/* set up vsc header for psr1 */

> +			hsw_psr_setup_vsc(intel_dp);


This might fail if psr2_support = false 2 lines above happens...
So code will continue on psr1 without any vsc setup.
This logic needs to be reworked.

Or probably the psr2 check and restriction properly moved to
match_conditions function, where is the right place for all conditional
behavior checking. (Preferably in a separated patch before this one).

>  		}

> -

>  		/*

>  		 * Per Spec: Avoid continuous PSR exit by masking MEMUP and HPD.

>  		 * Also mask LPSP to avoid dependency on other drivers that

> @@ -557,20 +589,37 @@ static void hsw_psr_disable(struct intel_dp *intel_dp)

>  	struct drm_i915_private *dev_priv = to_i915(dev);

>  

>  	if (dev_priv->psr.active) {

> -		I915_WRITE(EDP_PSR_CTL,

> -			   I915_READ(EDP_PSR_CTL) & ~EDP_PSR_ENABLE);

> +		if (dev_priv->psr.psr2_support)

> +			I915_WRITE(EDP_PSR2_CTL,

> +				I915_READ(EDP_PSR2_CTL) &

> +					~(EDP_PSR2_ENABLE |

> +					EDP_SU_TRACK_ENABLE));

> +		else

> +			I915_WRITE(EDP_PSR_CTL,

> +				   I915_READ(EDP_PSR_CTL) & ~EDP_PSR_ENABLE);

>  

>  		/* Wait till PSR is idle */

> -		if (intel_wait_for_register(dev_priv,

> -					    EDP_PSR_STATUS_CTL,

> -					    EDP_PSR_STATUS_STATE_MASK,

> -					    0,

> -					    2000))

> +		if (dev_priv->psr.psr2_support) {

> +			if (intel_wait_for_register(dev_priv,

> +						    EDP_PSR2_STATUS_CTL,

> +						    EDP_PSR2_STATUS_STATE_MASK,

> +						    0,

> +						    2000))

> +			DRM_ERROR("Timed out waiting for PSR2 Idle State\n");

> +		} else {

> +			if (intel_wait_for_register(dev_priv,

> +						    EDP_PSR_STATUS_CTL,

> +						    EDP_PSR_STATUS_STATE_MASK,

> +						    0,

> +						    2000))

>  			DRM_ERROR("Timed out waiting for PSR Idle State\n");

> -

> +		}

>  		dev_priv->psr.active = false;

>  	} else {

> -		WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);

> +		if (dev_priv->psr.psr2_support)

> +			WARN_ON(I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE);

> +		else

> +			WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);

>  	}



All these 40 lines above deserve separated functions as well.

or at least invert the checks first psr2 then active... better for
reading one or another imo..


>  }

>  

> @@ -621,13 +670,24 @@ static void intel_psr_work(struct work_struct *work)

>  	 * and be ready for re-enable.

>  	 */

>  	if (HAS_DDI(dev_priv)) {

> -		if (intel_wait_for_register(dev_priv,

> -					    EDP_PSR_STATUS_CTL,

> -					    EDP_PSR_STATUS_STATE_MASK,

> -					    0,

> -					    50)) {

> -			DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n");

> -			return;

> +		if (dev_priv->psr.psr2_support) {

> +			if (intel_wait_for_register(dev_priv,

> +						EDP_PSR2_STATUS_CTL,

> +						EDP_PSR2_STATUS_STATE_MASK,

> +						0,

> +						50)) {

> +				DRM_ERROR("Timed out waiting for PSR2 Idle for re-enable\n");

> +				return;

> +			}

> +		} else {

> +			if (intel_wait_for_register(dev_priv,

> +						EDP_PSR_STATUS_CTL,

> +						EDP_PSR_STATUS_STATE_MASK,

> +						0,

> +						50)) {

> +				DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n");

> +				return;

> +			}

>  		}

>  	} else {

>  		if (intel_wait_for_register(dev_priv,

> @@ -669,11 +729,15 @@ static void intel_psr_exit(struct drm_i915_private *dev_priv)

>  		return;

>  

>  	if (HAS_DDI(dev_priv)) {

> -		val = I915_READ(EDP_PSR_CTL);

> -

> -		WARN_ON(!(val & EDP_PSR_ENABLE));

> -

> -		I915_WRITE(EDP_PSR_CTL, val & ~EDP_PSR_ENABLE);

> +		if (dev_priv->psr.psr2_support) {

> +			val = I915_READ(EDP_PSR2_CTL);

> +			WARN_ON(!(val & EDP_PSR2_ENABLE));

> +			I915_WRITE(EDP_PSR2_CTL, val & ~EDP_PSR2_ENABLE);

> +		} else {

> +			val = I915_READ(EDP_PSR_CTL);

> +			WARN_ON(!(val & EDP_PSR_ENABLE));

> +			I915_WRITE(EDP_PSR_CTL, val & ~EDP_PSR_ENABLE);

> +		}

>  	} else {

>  		val = I915_READ(VLV_PSRCTL(pipe));

>
vathsala nagaraju Jan. 6, 2017, 1:44 p.m. UTC | #3
Rodrigo,

The code is 
            if (dev_priv->psr.psr2_support) {
                        /* PSR2 is restricted to work with panel resolutions upto 3200x2000 */
                        if (crtc->config->pipe_src_w > 3200 ||
                                crtc->config->pipe_src_h > 2000)
                                dev_priv->psr.psr2_support = false;
                        else
                                skl_psr_setup_su_vsc(intel_dp);
                } else {
                        /* set up vsc header for psr1 */
                        hsw_psr_setup_vsc(intel_dp);
                }


If psr2_support=0 , then it calls  hsw_psr_setup_vsc(intel_dp); {psr1 vsc setup)
If psr2_support=1 then it calls skl_psr_setup_su_vsc(intel_dp); (psr2 vsc setup)

Regards,
vathsala


-----Original Message-----
From: Vivi, Rodrigo 

Sent: Friday, January 6, 2017 2:12 AM
To: Nagaraju, Vathsala <vathsala.nagaraju@intel.com>
Cc: dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; jim.bride@linux.intel.com; Patil, Deepti <deepti.patil@intel.com>
Subject: Re: [PATCH 03/10] drm/i915/psr: fix blank screen issue for psr2

On Fri, 2017-01-06 at 00:55 +0530, vathsala nagaraju wrote:
> Psr1 and psr2 are mutually exclusive,ie when psr2 is enabled,

> psr1 should be disabled.When psr2 is exited , bit 31 of reg PSR2_CTL 

> must be set to 0 but currently bit 31 of SRD_CTL

> (psr1 control register)is set to 0.

> Also ,PSR2_IDLE state is looked up from SRD_STATUS(psr1 register) 

> instead of PSR2_STATUS register, which has wrong data, resulting in 

> blankscreen.

> hsw_enable_source is split into hsw_enable_source_psr1 and

> hsw_enable_source_psr2 for easier code review and maintenance, as 

> suggested by rodrigo and jim.

> 

> v2: (Vivi Rodrigo)

> - Rename hsw_enable_source_psr* to intel_enable_source_psr*

> 

> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>

> Cc: Jim Bride <jim.bride@linux.intel.com>

> Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@intel.com>

> Signed-off-by: Patil Deepti <deepti.patil@intel.com>

> ---

>  drivers/gpu/drm/i915/i915_reg.h  |   3 +

>  drivers/gpu/drm/i915/intel_psr.c | 124 

> +++++++++++++++++++++++++++++----------

>  2 files changed, 97 insertions(+), 30 deletions(-)

> 

> diff --git a/drivers/gpu/drm/i915/i915_reg.h 

> b/drivers/gpu/drm/i915/i915_reg.h index 00970aa..7830e6e 100644

> --- a/drivers/gpu/drm/i915/i915_reg.h

> +++ b/drivers/gpu/drm/i915/i915_reg.h

> @@ -3615,6 +3615,9 @@ enum {

>  #define   EDP_PSR2_FRAME_BEFORE_SU_MASK	(0xf<<4)

>  #define   EDP_PSR2_IDLE_MASK		0xf

>  

> +#define EDP_PSR2_STATUS_CTL            _MMIO(0x6f940)

> +#define EDP_PSR2_STATUS_STATE_MASK     (0xf<<28)

> +

>  /* VGA port control */

>  #define ADPA			_MMIO(0x61100)

>  #define PCH_ADPA                _MMIO(0xe1100)

> diff --git a/drivers/gpu/drm/i915/intel_psr.c 

> b/drivers/gpu/drm/i915/intel_psr.c

> index c3aa649..d5e8bcc 100644

> --- a/drivers/gpu/drm/i915/intel_psr.c

> +++ b/drivers/gpu/drm/i915/intel_psr.c

> @@ -261,12 +261,11 @@ static void vlv_psr_activate(struct intel_dp *intel_dp)

>  		   VLV_EDP_PSR_ACTIVE_ENTRY);

>  }

>  

> -static void hsw_psr_enable_source(struct intel_dp *intel_dp)

> +static void intel_enable_source_psr1(struct intel_dp *intel_dp)

>  {

>  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);

>  	struct drm_device *dev = dig_port->base.base.dev;

>  	struct drm_i915_private *dev_priv = to_i915(dev);

> -

>  	uint32_t max_sleep_time = 0x1f;

>  	/*

>  	 * Let's respect VBT in case VBT asks a higher idle_frame value.

> @@ -312,14 +311,30 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp)

>  		val |= EDP_PSR_TP1_TP2_SEL;

>  

>  	I915_WRITE(EDP_PSR_CTL, val);

> +}

>  

> -	if (!dev_priv->psr.psr2_support)

> -		return;

> +static void intel_enable_source_psr2(struct intel_dp *intel_dp) {

> +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);

> +	struct drm_device *dev = dig_port->base.base.dev;

> +	struct drm_i915_private *dev_priv = to_i915(dev);

> +

> +	/*

> +	 * Let's respect VBT in case VBT asks a higher idle_frame value.

> +	 * Let's use 6 as the minimum to cover all known cases including

> +	 * the off-by-one issue that HW has in some cases. Also there are

> +	 * cases where sink should be able to train

> +	 * with the 5 or 6 idle patterns.

> +	 */

> +	uint32_t idle_frames = max(6, dev_priv->vbt.psr.idle_frames);

> +	uint32_t val = EDP_PSR_ENABLE;

> +

> +	val |= idle_frames << EDP_PSR_IDLE_FRAME_SHIFT;

>  

>  	/* FIXME: selective update is probably totally broken because it doesn't

>  	 * mesh at all with our frontbuffer tracking. And the hw alone isn't

>  	 * good enough. */

> -	val = EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE;

> +	val |= EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE;

>  

>  	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 5)

>  		val |= EDP_PSR2_TP2_TIME_2500;

> @@ -333,6 +348,20 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp)

>  	I915_WRITE(EDP_PSR2_CTL, val);

>  }

>  

> +

> +static void hsw_psr_enable_source(struct intel_dp *intel_dp) {

> +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);

> +	struct drm_device *dev = dig_port->base.base.dev;

> +	struct drm_i915_private *dev_priv = to_i915(dev);

> +

> +	/* psr1 and psr2 are mutually exclusive.*/

> +	if (dev_priv->psr.psr2_support)

> +		intel_enable_source_psr2(intel_dp);

> +	else

> +		intel_enable_source_psr1(intel_dp);

> +}

> +

>  static bool intel_psr_match_conditions(struct intel_dp *intel_dp)  {

>  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); @@ 

> -410,7 +439,10 @@ static void intel_psr_activate(struct intel_dp *intel_dp)

>  	struct drm_device *dev = intel_dig_port->base.base.dev;

>  	struct drm_i915_private *dev_priv = to_i915(dev);

>  

> -	WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);

> +	if (dev_priv->psr.psr2_support)

> +		WARN_ON(I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE);

> +	else

> +		WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);

>  	WARN_ON(dev_priv->psr.active);

>  	lockdep_assert_held(&dev_priv->psr.lock);

>  

> @@ -462,8 +494,6 @@ void intel_psr_enable(struct intel_dp *intel_dp)

>  	dev_priv->psr.busy_frontbuffer_bits = 0;

>  

>  	if (HAS_DDI(dev_priv)) {

> -		hsw_psr_setup_vsc(intel_dp);

> -

>  		if (dev_priv->psr.psr2_support) {

>  			/* PSR2 is restricted to work with panel resolutions upto 3200x2000 */

>  			if (crtc->config->pipe_src_w > 3200 || @@ -471,8 +501,10 @@ void 

> intel_psr_enable(struct intel_dp *intel_dp)

>  				dev_priv->psr.psr2_support = false;

>  			else

>  				skl_psr_setup_su_vsc(intel_dp);

> +		} else {

> +			/* set up vsc header for psr1 */

> +			hsw_psr_setup_vsc(intel_dp);


This might fail if psr2_support = false 2 lines above happens...
So code will continue on psr1 without any vsc setup.
This logic needs to be reworked.

Or probably the psr2 check and restriction properly moved to match_conditions function, where is the right place for all conditional behavior checking. (Preferably in a separated patch before this one).



>  		}

> -

>  		/*

>  		 * Per Spec: Avoid continuous PSR exit by masking MEMUP and HPD.

>  		 * Also mask LPSP to avoid dependency on other drivers that @@ 

> -557,20 +589,37 @@ static void hsw_psr_disable(struct intel_dp *intel_dp)

>  	struct drm_i915_private *dev_priv = to_i915(dev);

>  

>  	if (dev_priv->psr.active) {

> -		I915_WRITE(EDP_PSR_CTL,

> -			   I915_READ(EDP_PSR_CTL) & ~EDP_PSR_ENABLE);

> +		if (dev_priv->psr.psr2_support)

> +			I915_WRITE(EDP_PSR2_CTL,

> +				I915_READ(EDP_PSR2_CTL) &

> +					~(EDP_PSR2_ENABLE |

> +					EDP_SU_TRACK_ENABLE));

> +		else

> +			I915_WRITE(EDP_PSR_CTL,

> +				   I915_READ(EDP_PSR_CTL) & ~EDP_PSR_ENABLE);

>  

>  		/* Wait till PSR is idle */

> -		if (intel_wait_for_register(dev_priv,

> -					    EDP_PSR_STATUS_CTL,

> -					    EDP_PSR_STATUS_STATE_MASK,

> -					    0,

> -					    2000))

> +		if (dev_priv->psr.psr2_support) {

> +			if (intel_wait_for_register(dev_priv,

> +						    EDP_PSR2_STATUS_CTL,

> +						    EDP_PSR2_STATUS_STATE_MASK,

> +						    0,

> +						    2000))

> +			DRM_ERROR("Timed out waiting for PSR2 Idle State\n");

> +		} else {

> +			if (intel_wait_for_register(dev_priv,

> +						    EDP_PSR_STATUS_CTL,

> +						    EDP_PSR_STATUS_STATE_MASK,

> +						    0,

> +						    2000))

>  			DRM_ERROR("Timed out waiting for PSR Idle State\n");

> -

> +		}

>  		dev_priv->psr.active = false;

>  	} else {

> -		WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);

> +		if (dev_priv->psr.psr2_support)

> +			WARN_ON(I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE);

> +		else

> +			WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);

>  	}



All these 40 lines above deserve separated functions as well.

or at least invert the checks first psr2 then active... better for reading one or another imo..


>  }

>  

> @@ -621,13 +670,24 @@ static void intel_psr_work(struct work_struct *work)

>  	 * and be ready for re-enable.

>  	 */

>  	if (HAS_DDI(dev_priv)) {

> -		if (intel_wait_for_register(dev_priv,

> -					    EDP_PSR_STATUS_CTL,

> -					    EDP_PSR_STATUS_STATE_MASK,

> -					    0,

> -					    50)) {

> -			DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n");

> -			return;

> +		if (dev_priv->psr.psr2_support) {

> +			if (intel_wait_for_register(dev_priv,

> +						EDP_PSR2_STATUS_CTL,

> +						EDP_PSR2_STATUS_STATE_MASK,

> +						0,

> +						50)) {

> +				DRM_ERROR("Timed out waiting for PSR2 Idle for re-enable\n");

> +				return;

> +			}

> +		} else {

> +			if (intel_wait_for_register(dev_priv,

> +						EDP_PSR_STATUS_CTL,

> +						EDP_PSR_STATUS_STATE_MASK,

> +						0,

> +						50)) {

> +				DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n");

> +				return;

> +			}

>  		}

>  	} else {

>  		if (intel_wait_for_register(dev_priv,

> @@ -669,11 +729,15 @@ static void intel_psr_exit(struct drm_i915_private *dev_priv)

>  		return;

>  

>  	if (HAS_DDI(dev_priv)) {

> -		val = I915_READ(EDP_PSR_CTL);

> -

> -		WARN_ON(!(val & EDP_PSR_ENABLE));

> -

> -		I915_WRITE(EDP_PSR_CTL, val & ~EDP_PSR_ENABLE);

> +		if (dev_priv->psr.psr2_support) {

> +			val = I915_READ(EDP_PSR2_CTL);

> +			WARN_ON(!(val & EDP_PSR2_ENABLE));

> +			I915_WRITE(EDP_PSR2_CTL, val & ~EDP_PSR2_ENABLE);

> +		} else {

> +			val = I915_READ(EDP_PSR_CTL);

> +			WARN_ON(!(val & EDP_PSR_ENABLE));

> +			I915_WRITE(EDP_PSR_CTL, val & ~EDP_PSR_ENABLE);

> +		}

>  	} else {

>  		val = I915_READ(VLV_PSRCTL(pipe));

>

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 00970aa..7830e6e 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3615,6 +3615,9 @@  enum {
 #define   EDP_PSR2_FRAME_BEFORE_SU_MASK	(0xf<<4)
 #define   EDP_PSR2_IDLE_MASK		0xf
 
+#define EDP_PSR2_STATUS_CTL            _MMIO(0x6f940)
+#define EDP_PSR2_STATUS_STATE_MASK     (0xf<<28)
+
 /* VGA port control */
 #define ADPA			_MMIO(0x61100)
 #define PCH_ADPA                _MMIO(0xe1100)
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index c3aa649..d5e8bcc 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -261,12 +261,11 @@  static void vlv_psr_activate(struct intel_dp *intel_dp)
 		   VLV_EDP_PSR_ACTIVE_ENTRY);
 }
 
-static void hsw_psr_enable_source(struct intel_dp *intel_dp)
+static void intel_enable_source_psr1(struct intel_dp *intel_dp)
 {
 	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
 	struct drm_device *dev = dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
-
 	uint32_t max_sleep_time = 0x1f;
 	/*
 	 * Let's respect VBT in case VBT asks a higher idle_frame value.
@@ -312,14 +311,30 @@  static void hsw_psr_enable_source(struct intel_dp *intel_dp)
 		val |= EDP_PSR_TP1_TP2_SEL;
 
 	I915_WRITE(EDP_PSR_CTL, val);
+}
 
-	if (!dev_priv->psr.psr2_support)
-		return;
+static void intel_enable_source_psr2(struct intel_dp *intel_dp)
+{
+	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
+	struct drm_device *dev = dig_port->base.base.dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+
+	/*
+	 * Let's respect VBT in case VBT asks a higher idle_frame value.
+	 * Let's use 6 as the minimum to cover all known cases including
+	 * the off-by-one issue that HW has in some cases. Also there are
+	 * cases where sink should be able to train
+	 * with the 5 or 6 idle patterns.
+	 */
+	uint32_t idle_frames = max(6, dev_priv->vbt.psr.idle_frames);
+	uint32_t val = EDP_PSR_ENABLE;
+
+	val |= idle_frames << EDP_PSR_IDLE_FRAME_SHIFT;
 
 	/* FIXME: selective update is probably totally broken because it doesn't
 	 * mesh at all with our frontbuffer tracking. And the hw alone isn't
 	 * good enough. */
-	val = EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE;
+	val |= EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE;
 
 	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 5)
 		val |= EDP_PSR2_TP2_TIME_2500;
@@ -333,6 +348,20 @@  static void hsw_psr_enable_source(struct intel_dp *intel_dp)
 	I915_WRITE(EDP_PSR2_CTL, val);
 }
 
+
+static void hsw_psr_enable_source(struct intel_dp *intel_dp)
+{
+	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
+	struct drm_device *dev = dig_port->base.base.dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+
+	/* psr1 and psr2 are mutually exclusive.*/
+	if (dev_priv->psr.psr2_support)
+		intel_enable_source_psr2(intel_dp);
+	else
+		intel_enable_source_psr1(intel_dp);
+}
+
 static bool intel_psr_match_conditions(struct intel_dp *intel_dp)
 {
 	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
@@ -410,7 +439,10 @@  static void intel_psr_activate(struct intel_dp *intel_dp)
 	struct drm_device *dev = intel_dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 
-	WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
+	if (dev_priv->psr.psr2_support)
+		WARN_ON(I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE);
+	else
+		WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
 	WARN_ON(dev_priv->psr.active);
 	lockdep_assert_held(&dev_priv->psr.lock);
 
@@ -462,8 +494,6 @@  void intel_psr_enable(struct intel_dp *intel_dp)
 	dev_priv->psr.busy_frontbuffer_bits = 0;
 
 	if (HAS_DDI(dev_priv)) {
-		hsw_psr_setup_vsc(intel_dp);
-
 		if (dev_priv->psr.psr2_support) {
 			/* PSR2 is restricted to work with panel resolutions upto 3200x2000 */
 			if (crtc->config->pipe_src_w > 3200 ||
@@ -471,8 +501,10 @@  void intel_psr_enable(struct intel_dp *intel_dp)
 				dev_priv->psr.psr2_support = false;
 			else
 				skl_psr_setup_su_vsc(intel_dp);
+		} else {
+			/* set up vsc header for psr1 */
+			hsw_psr_setup_vsc(intel_dp);
 		}
-
 		/*
 		 * Per Spec: Avoid continuous PSR exit by masking MEMUP and HPD.
 		 * Also mask LPSP to avoid dependency on other drivers that
@@ -557,20 +589,37 @@  static void hsw_psr_disable(struct intel_dp *intel_dp)
 	struct drm_i915_private *dev_priv = to_i915(dev);
 
 	if (dev_priv->psr.active) {
-		I915_WRITE(EDP_PSR_CTL,
-			   I915_READ(EDP_PSR_CTL) & ~EDP_PSR_ENABLE);
+		if (dev_priv->psr.psr2_support)
+			I915_WRITE(EDP_PSR2_CTL,
+				I915_READ(EDP_PSR2_CTL) &
+					~(EDP_PSR2_ENABLE |
+					EDP_SU_TRACK_ENABLE));
+		else
+			I915_WRITE(EDP_PSR_CTL,
+				   I915_READ(EDP_PSR_CTL) & ~EDP_PSR_ENABLE);
 
 		/* Wait till PSR is idle */
-		if (intel_wait_for_register(dev_priv,
-					    EDP_PSR_STATUS_CTL,
-					    EDP_PSR_STATUS_STATE_MASK,
-					    0,
-					    2000))
+		if (dev_priv->psr.psr2_support) {
+			if (intel_wait_for_register(dev_priv,
+						    EDP_PSR2_STATUS_CTL,
+						    EDP_PSR2_STATUS_STATE_MASK,
+						    0,
+						    2000))
+			DRM_ERROR("Timed out waiting for PSR2 Idle State\n");
+		} else {
+			if (intel_wait_for_register(dev_priv,
+						    EDP_PSR_STATUS_CTL,
+						    EDP_PSR_STATUS_STATE_MASK,
+						    0,
+						    2000))
 			DRM_ERROR("Timed out waiting for PSR Idle State\n");
-
+		}
 		dev_priv->psr.active = false;
 	} else {
-		WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
+		if (dev_priv->psr.psr2_support)
+			WARN_ON(I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE);
+		else
+			WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
 	}
 }
 
@@ -621,13 +670,24 @@  static void intel_psr_work(struct work_struct *work)
 	 * and be ready for re-enable.
 	 */
 	if (HAS_DDI(dev_priv)) {
-		if (intel_wait_for_register(dev_priv,
-					    EDP_PSR_STATUS_CTL,
-					    EDP_PSR_STATUS_STATE_MASK,
-					    0,
-					    50)) {
-			DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n");
-			return;
+		if (dev_priv->psr.psr2_support) {
+			if (intel_wait_for_register(dev_priv,
+						EDP_PSR2_STATUS_CTL,
+						EDP_PSR2_STATUS_STATE_MASK,
+						0,
+						50)) {
+				DRM_ERROR("Timed out waiting for PSR2 Idle for re-enable\n");
+				return;
+			}
+		} else {
+			if (intel_wait_for_register(dev_priv,
+						EDP_PSR_STATUS_CTL,
+						EDP_PSR_STATUS_STATE_MASK,
+						0,
+						50)) {
+				DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n");
+				return;
+			}
 		}
 	} else {
 		if (intel_wait_for_register(dev_priv,
@@ -669,11 +729,15 @@  static void intel_psr_exit(struct drm_i915_private *dev_priv)
 		return;
 
 	if (HAS_DDI(dev_priv)) {
-		val = I915_READ(EDP_PSR_CTL);
-
-		WARN_ON(!(val & EDP_PSR_ENABLE));
-
-		I915_WRITE(EDP_PSR_CTL, val & ~EDP_PSR_ENABLE);
+		if (dev_priv->psr.psr2_support) {
+			val = I915_READ(EDP_PSR2_CTL);
+			WARN_ON(!(val & EDP_PSR2_ENABLE));
+			I915_WRITE(EDP_PSR2_CTL, val & ~EDP_PSR2_ENABLE);
+		} else {
+			val = I915_READ(EDP_PSR_CTL);
+			WARN_ON(!(val & EDP_PSR_ENABLE));
+			I915_WRITE(EDP_PSR_CTL, val & ~EDP_PSR_ENABLE);
+		}
 	} else {
 		val = I915_READ(VLV_PSRCTL(pipe));