Re: [PATCH 03/10] drm/i915/psr: fix blank screen issue for psr2

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

 

On Monday 09 January 2017 10:47 PM, Vivi, Rodrigo wrote:

On Mon, 2017-01-09 at 18:26 +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: (Rodrigo)
- Rename hsw_enable_source_psr* to intel_enable_source_psr*

v3: (Rodrigo)
- In hsw_psr_disable ,
   1) for psr active case, handle psr2 followed by psr1.
   2) psr inactive case, handle psr2 followed by psr1

much better, thanks, but still has one blocking issue imho:


Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
Cc: Jim Bride <jim.bride@xxxxxxxxxxxxxxx>
Signed-off-by: Vathsala Nagaraju <vathsala.nagaraju@xxxxxxxxx>
Signed-off-by: Patil Deepti <deepti.patil@xxxxxxxxx>
---
  drivers/gpu/drm/i915/i915_reg.h  |   3 +
  drivers/gpu/drm/i915/intel_psr.c | 126 +++++++++++++++++++++++++++++----------
  2 files changed, 97 insertions(+), 32 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..6c161aa 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;

+			hsw_psr_setup_vsc(intel_dp);

without this you will end up enabling psr1 but not setting up the vsc.


But also if I understood correctly this will fail anyways right?
So before this patch we need one that move this restriction to
match_conditions function returning false and fully blocking PSR.

thanks. I have posted a new patch. please review



  			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,35 @@ 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);
-
-		/* 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) {
+			I915_WRITE(EDP_PSR2_CTL,
+				I915_READ(EDP_PSR2_CTL) &
+					~(EDP_PSR2_ENABLE |
+					EDP_SU_TRACK_ENABLE));
+			/* Wait till PSR2 is idle */
+			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 {
+			I915_WRITE(EDP_PSR_CTL,
+				   I915_READ(EDP_PSR_CTL) & ~EDP_PSR_ENABLE);
+			/* Wait till PSR1 is idle */
+			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 +668,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 +727,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));


_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux