[PATCH 0/3] Enable PSR2 for idle screen

[PATCH 0/3] Enable PSR2 for idle screen

[vathsala nagaraju]

Enables PSR2 for idle screen.
with PSR2, system can enter deep sleep state.
PSR2 live status is reported in PSR2_STATUS
register, bit 31:28.

Patches are tested on sharp edp 1.4 panel,
32X18 , model:Lq0dasb165

vathsala nagaraju (3):
  drm/i915/psr:Adds Y-cordinate to skl_psr_setup_vsc
  drm/i915/psr: enable PSR2 for idle screen
  drm/i915/psr: adds psr2 status to debugfs

 drivers/gpu/drm/i915/i915_debugfs.c |  36 ++++-
 drivers/gpu/drm/i915/i915_drv.h     |   2 +
 drivers/gpu/drm/i915/i915_reg.h     |  23 ++-
 drivers/gpu/drm/i915/intel_dp.c     |  22 +++
 drivers/gpu/drm/i915/intel_psr.c    | 275 ++++++++++++++++++++++++------------
 include/drm/drm_dp_helper.h         |   6 +-
 6 files changed, 269 insertions(+), 95 deletions(-)

-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/3] drm/i915/psr:Adds Y-cordinate to skl_psr_setup_vsc

[vathsala nagaraju]

Adds Y-co-ordinate support to skl_psr_setup_vsc as
per edp 1.4 spec,table 6-11:VSC SDP HEADER
Extension for psr2 support.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: vathsala nagaraju <vathsala.nagaraju@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  |  2 ++
 drivers/gpu/drm/i915/intel_dp.c  | 22 ++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_psr.c | 13 ++++++++++++-
 include/drm/drm_dp_helper.h      |  5 ++++-
 4 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7f2754a..79ce64f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1022,6 +1022,8 @@ struct i915_psr {
 	bool psr2_support;
 	bool aux_frame_sync;
 	bool link_standby;
+	bool y_cord_support;
+	bool colorimetry_support;
 };
 
 enum intel_pch {
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 364db90..19e9ecf 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3439,6 +3439,28 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp)
 		dev_priv->psr.psr2_support = dev_priv->psr.aux_frame_sync;
 		DRM_DEBUG_KMS("PSR2 %s on sink",
 			      dev_priv->psr.psr2_support ? "supported" : "not supported");
+
+		if (dev_priv->psr.psr2_support) {
+			uint8_t psr_caps, dprx;
+
+			/*check if panel supports Y-Cordinate*/
+			drm_dp_dpcd_readb(&intel_dp->aux,
+					DP_PSR_CAPS,
+					&psr_caps);
+			if (psr_caps & DP_PSR_Y_COORDINATE)
+				dev_priv->psr.y_cord_support = true;
+			else
+				dev_priv->psr.y_cord_support = false;
+			/* check for COLORIMETRY SUPPORT */
+			drm_dp_dpcd_readb(&intel_dp->aux,
+					DPRX_FEATURE_ENUMERATION_LIST,
+					&dprx);
+			if (dprx & VSC_SDP_EXT_FOR_COLORIMETRY_SUPPORTED)
+				dev_priv->psr.colorimetry_support = true;
+			else
+				dev_priv->psr.colorimetry_support = false;
+		}
+
 	}
 
 	/* Read the eDP Display control capabilities registers */
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 59a21c9..76a630b 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -122,13 +122,24 @@ static void vlv_psr_setup_vsc(struct intel_dp *intel_dp)
 static void skl_psr_setup_su_vsc(struct intel_dp *intel_dp)
 {
 	struct edp_vsc_psr psr_vsc;
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct drm_device *dev = intel_dig_port->base.base.dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
 
 	/* Prepare VSC Header for SU as per EDP 1.4 spec, Table 6.11 */
 	memset(&psr_vsc, 0, sizeof(psr_vsc));
 	psr_vsc.sdp_header.HB0 = 0;
 	psr_vsc.sdp_header.HB1 = 0x7;
 	psr_vsc.sdp_header.HB2 = 0x3;
-	psr_vsc.sdp_header.HB3 = 0xb;
+	psr_vsc.sdp_header.HB3 = 0xc;
+	if (dev_priv->psr.y_cord_support &&
+		dev_priv->psr.colorimetry_support) {
+		psr_vsc.sdp_header.HB2 = 0x5;
+		psr_vsc.sdp_header.HB3 = 0x13;
+	} else {
+		psr_vsc.sdp_header.HB2 = 0x4;
+		psr_vsc.sdp_header.HB3 = 0xe;
+	}
 	intel_psr_write_vsc(intel_dp, &psr_vsc);
 }
 
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 63b8bd5..3d875c0 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -194,7 +194,7 @@
 # define DP_PSR_SETUP_TIME_0                (6 << 1)
 # define DP_PSR_SETUP_TIME_MASK             (7 << 1)
 # define DP_PSR_SETUP_TIME_SHIFT            1
-
+# define DP_PSR_Y_COORDINATE		    (1 << 4)
 /*
  * 0x80-0x8f describe downstream port capabilities, but there are two layouts
  * based on whether DP_DETAILED_CAP_INFO_AVAILABLE was set.  If it was not,
@@ -640,6 +640,9 @@ struct edp_sdp_header {
 #define EDP_SDP_HEADER_REVISION_MASK		0x1F
 #define EDP_SDP_HEADER_VALID_PAYLOAD_BYTES	0x1F
 
+#define DPRX_FEATURE_ENUMERATION_LIST  0x02210
+#define VSC_SDP_EXT_FOR_COLORIMETRY_SUPPORTED	(1 << 3)
+
 struct edp_vsc_psr {
 	struct edp_sdp_header sdp_header;
 	u8 DB0; /* Stereo Interface */
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/3] drm/i915/psr: enable PSR2 for idle screen

[vathsala nagaraju]

Enables PSR2 for idle screen.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: vathsala nagaraju <vathsala.nagaraju@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h  |  21 +++-
 drivers/gpu/drm/i915/intel_psr.c | 262 ++++++++++++++++++++++++++-------------
 include/drm/drm_dp_helper.h      |   1 +
 3 files changed, 195 insertions(+), 89 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index da82744..49682f5 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3301,9 +3301,12 @@ enum {
 #define   EDP_PSR_PERF_CNT_MASK		0xffffff
 
 #define EDP_PSR_DEBUG_CTL		_MMIO(dev_priv->psr_mmio_base + 0x60)
-#define   EDP_PSR_DEBUG_MASK_LPSP	(1<<27)
-#define   EDP_PSR_DEBUG_MASK_MEMUP	(1<<26)
-#define   EDP_PSR_DEBUG_MASK_HPD	(1<<25)
+#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)
+#define   EDP_PSR_DEBUG_MASK_HPD		(1<<25)
+#define   EDP_PSR_DEBUG_MASK_DISP_REG_WRITE	(1<<16)
+#define   EDP_PSR_DEBUG_EXIT_ON_PIXEL_UNDERRUN	(1<<15)
 
 #define EDP_PSR2_CTL			_MMIO(0x6f900)
 #define   EDP_PSR2_ENABLE		(1<<31)
@@ -3318,6 +3321,11 @@ enum {
 #define   EDP_PSR2_FRAME_BEFORE_SU_SHIFT 4
 #define   EDP_PSR2_FRAME_BEFORE_SU_MASK	(0xf<<4)
 #define   EDP_PSR2_IDLE_MASK		0xf
+#define   EDP_FRAMES_BEFORE_SU_ENTRY	(1<<4)
+
+#define EDP_PSR2_STATUS_CTL		_MMIO(0x6f940)
+#define EDP_PSR2_STATUS_STATE_MASK	(0xf<<28)
+#define EDP_PSR2_STATUS_STATE_IDLE	0
 
 /* VGA port control */
 #define ADPA			_MMIO(0x61100)
@@ -6133,6 +6141,13 @@ enum {
 #define  BDW_DPRS_MASK_VBLANK_SRD	(1 << 0)
 #define CHICKEN_PIPESL_1(pipe) _MMIO_PIPE(pipe, _CHICKEN_PIPESL_1_A, _CHICKEN_PIPESL_1_B)
 
+#define CHICKEN_TRANS_A         0x420c0
+#define CHICKEN_TRANS_B         0x420c4
+#define CHICKEN_TRANS(trans) _MMIO_TRANS(trans, CHICKEN_TRANS_A, CHICKEN_TRANS_B)
+#define TRANS_EDP		3
+#define CHICKEN_TRANS_BIT12	(1<<12)
+#define CHICKEN_TRANS_BIT15	(1<<15)
+
 #define DISP_ARB_CTL	_MMIO(0x45000)
 #define  DISP_FBC_MEMORY_WAKE		(1<<31)
 #define  DISP_TILE_SURFACE_SWIZZLING	(1<<13)
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 76a630b..12429c2 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -197,6 +197,7 @@ static void hsw_psr_enable_sink(struct intel_dp *intel_dp)
 	enum port port = dig_port->port;
 	u32 aux_ctl;
 	int i;
+	uint8_t enable_psr2 = 0;
 
 	BUILD_BUG_ON(sizeof(aux_msg) > 20);
 
@@ -207,13 +208,32 @@ static void hsw_psr_enable_sink(struct intel_dp *intel_dp)
 		drm_dp_dpcd_writeb(&intel_dp->aux,
 				DP_SINK_DEVICE_AUX_FRAME_SYNC_CONF,
 				DP_AUX_FRAME_SYNC_ENABLE);
+	/* enable ALPM for PSR2 */
+	if (dev_priv->psr.psr2_support) {
+		uint8_t alpm_caps;
+
+		/* confirm panel supports ALPM */
+		drm_dp_dpcd_readb(&intel_dp->aux,
+				DP_RECEIVER_ALPM_CAP,
+				&alpm_caps);
+		if (alpm_caps & DP_ALPM_CAP)
+		drm_dp_dpcd_writeb(&intel_dp->aux,
+				DP_RECEIVER_ALPM_CONFIG,
+				DP_ALPM_ENABLE);
+
+		enable_psr2 = DP_PSR2_PROTOCOL;
+	}
+
 
 	if (dev_priv->psr.link_standby)
-		drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
-				   DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE);
+		drm_dp_dpcd_writeb(&intel_dp->aux,
+				DP_PSR_EN_CFG,
+				DP_PSR_ENABLE |
+				DP_PSR_MAIN_LINK_ACTIVE |
+				enable_psr2);
 	else
 		drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
-				   DP_PSR_ENABLE);
+				   DP_PSR_ENABLE | enable_psr2);
 
 	aux_ctl_reg = psr_aux_ctl_reg(dev_priv, port);
 
@@ -276,59 +296,75 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp)
 	uint32_t idle_frames = dev_priv->vbt.psr.idle_frames + 1;
 	uint32_t val = EDP_PSR_ENABLE;
 
-	val |= max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT;
-	val |= idle_frames << EDP_PSR_IDLE_FRAME_SHIFT;
+	if (!dev_priv->psr.psr2_support) {
+		val |= max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT;
+		val |= idle_frames << EDP_PSR_IDLE_FRAME_SHIFT;
 
-	if (IS_HASWELL(dev))
-		val |= EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES;
+		if (IS_HASWELL(dev))
+			val |= EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES;
 
-	if (dev_priv->psr.link_standby)
-		val |= EDP_PSR_LINK_STANDBY;
-
-	if (dev_priv->vbt.psr.tp1_wakeup_time > 5)
-		val |= EDP_PSR_TP1_TIME_2500us;
-	else if (dev_priv->vbt.psr.tp1_wakeup_time > 1)
-		val |= EDP_PSR_TP1_TIME_500us;
-	else if (dev_priv->vbt.psr.tp1_wakeup_time > 0)
-		val |= EDP_PSR_TP1_TIME_100us;
-	else
-		val |= EDP_PSR_TP1_TIME_0us;
-
-	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 5)
-		val |= EDP_PSR_TP2_TP3_TIME_2500us;
-	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 1)
-		val |= EDP_PSR_TP2_TP3_TIME_500us;
-	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 0)
-		val |= EDP_PSR_TP2_TP3_TIME_100us;
-	else
-		val |= EDP_PSR_TP2_TP3_TIME_0us;
+		if (dev_priv->psr.link_standby)
+			val |= EDP_PSR_LINK_STANDBY;
 
-	if (intel_dp_source_supports_hbr2(intel_dp) &&
-	    drm_dp_tps3_supported(intel_dp->dpcd))
-		val |= EDP_PSR_TP1_TP3_SEL;
-	else
-		val |= EDP_PSR_TP1_TP2_SEL;
+		if (dev_priv->vbt.psr.tp1_wakeup_time > 5)
+			val |= EDP_PSR_TP1_TIME_2500us;
+		else if (dev_priv->vbt.psr.tp1_wakeup_time > 1)
+			val |= EDP_PSR_TP1_TIME_500us;
+		else if (dev_priv->vbt.psr.tp1_wakeup_time > 0)
+			val |= EDP_PSR_TP1_TIME_100us;
+		else
+			val |= EDP_PSR_TP1_TIME_0us;
+
+		if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 5)
+			val |= EDP_PSR_TP2_TP3_TIME_2500us;
+		else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 1)
+			val |= EDP_PSR_TP2_TP3_TIME_500us;
+		else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 0)
+			val |= EDP_PSR_TP2_TP3_TIME_100us;
+		else
+			val |= EDP_PSR_TP2_TP3_TIME_0us;
 
-	I915_WRITE(EDP_PSR_CTL, val);
+		if (intel_dp_source_supports_hbr2(intel_dp) &
+			drm_dp_tps3_supported(intel_dp->dpcd))
+			val |= EDP_PSR_TP1_TP3_SEL;
+		else
+			val |= EDP_PSR_TP1_TP2_SEL;
 
-	if (!dev_priv->psr.psr2_support)
-		return;
+		I915_WRITE(EDP_PSR_CTL, val);
 
-	/* 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;
-
-	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 5)
-		val |= EDP_PSR2_TP2_TIME_2500;
-	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 1)
-		val |= EDP_PSR2_TP2_TIME_500;
-	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 0)
-		val |= EDP_PSR2_TP2_TIME_100;
-	else
-		val |= EDP_PSR2_TP2_TIME_50;
+	} else {
+		/* 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 |
+			EDP_FRAMES_BEFORE_SU_ENTRY;
+
+		if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 5)
+			val |= EDP_PSR2_TP2_TIME_2500;
+		else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 1)
+			val |= EDP_PSR2_TP2_TIME_500;
+		else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 0)
+			val |= EDP_PSR2_TP2_TIME_100;
+		else
+			val |= EDP_PSR2_TP2_TIME_50;
 
-	I915_WRITE(EDP_PSR2_CTL, val);
+		if (idle_frames < 4)
+			idle_frames = 4;
+
+		val |= idle_frames << EDP_PSR_IDLE_FRAME_SHIFT;
+
+		if (dev_priv->psr.y_cord_support)
+			I915_WRITE(CHICKEN_TRANS(TRANS_EDP),
+				CHICKEN_TRANS_BIT12 |
+				CHICKEN_TRANS_BIT15);
+		else
+			I915_WRITE(CHICKEN_TRANS(TRANS_EDP),
+				CHICKEN_TRANS_BIT12);
+
+		I915_WRITE(EDP_PSR2_CTL, val);
+	}
 }
 
 static bool intel_psr_match_conditions(struct intel_dp *intel_dp)
@@ -408,7 +444,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);
 
@@ -460,8 +499,6 @@ void intel_psr_enable(struct intel_dp *intel_dp)
 	dev_priv->psr.busy_frontbuffer_bits = 0;
 
 	if (HAS_DDI(dev)) {
-		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 ||
@@ -469,17 +506,27 @@ void intel_psr_enable(struct intel_dp *intel_dp)
 				dev_priv->psr.psr2_support = false;
 			else
 				skl_psr_setup_su_vsc(intel_dp);
+				I915_WRITE(EDP_PSR_DEBUG_CTL,
+					EDP_PSR_DEBUG_MASK_MEMUP |
+					EDP_PSR_DEBUG_MASK_HPD |
+					EDP_PSR_DEBUG_MASK_LPSP |
+					EDP_PSR_DEBUG_MASK_MAX_SLEEP |
+					EDP_PSR_DEBUG_MASK_DISP_REG_WRITE);
+		} else {
+
+			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 might block runtime_pm besides
+			 * preventing  other hw tracking issues now we can rely
+			 * on frontbuffer tracking.
+			 */
+			I915_WRITE(EDP_PSR_DEBUG_CTL,
+				EDP_PSR_DEBUG_MASK_MEMUP |
+				EDP_PSR_DEBUG_MASK_HPD |
+				EDP_PSR_DEBUG_MASK_LPSP);
 		}
-
-		/*
-		 * Per Spec: Avoid continuous PSR exit by masking MEMUP and HPD.
-		 * Also mask LPSP to avoid dependency on other drivers that
-		 * might block runtime_pm besides preventing other hw tracking
-		 * issues now we can rely on frontbuffer tracking.
-		 */
-		I915_WRITE(EDP_PSR_DEBUG_CTL, EDP_PSR_DEBUG_MASK_MEMUP |
-			   EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP);
-
 		/* Enable PSR on the panel */
 		hsw_psr_enable_sink(intel_dp);
 
@@ -555,20 +602,41 @@ 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))
-			DRM_ERROR("Timed out waiting for PSR Idle State\n");
-
+		if (dev_priv->psr.psr2_support) {
+			/* disable AUX frame sync */
+			if (dev_priv->psr.aux_frame_sync)
+				drm_dp_dpcd_writeb(&intel_dp->aux,
+					DP_SINK_DEVICE_AUX_FRAME_SYNC_CONF,
+						0);
+			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 PSR 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);
 	}
 }
 
@@ -619,13 +687,25 @@ 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,
@@ -667,11 +747,21 @@ 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) {
+			/*disable aux-frame sync */
+			if (dev_priv->psr.aux_frame_sync)
+				drm_dp_dpcd_writeb(&intel_dp->aux,
+					DP_SINK_DEVICE_AUX_FRAME_SYNC_CONF,
+					0);
+
+			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));
 
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 3d875c0..6303a3a 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -343,6 +343,7 @@
 # define DP_PSR_FRAME_CAPTURE		    (1 << 3)
 # define DP_PSR_SELECTIVE_UPDATE	    (1 << 4)
 # define DP_PSR_IRQ_HPD_WITH_CRC_ERRORS     (1 << 5)
+# define DP_PSR2_PROTOCOL		(1 << 6)
 
 #define DP_ADAPTER_CTRL			    0x1a0
 # define DP_ADAPTER_CTRL_FORCE_LOAD_SENSE   (1 << 0)
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/3] drm/i915/psr: adds psr2 status to debugfs

[vathsala nagaraju]

Reads from EDP_PSR2_CTL for psr2 and reports
live state of psr2

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: vathsala nagaraju <vathsala.nagaraju@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 36 ++++++++++++++++++++++++++++++++----
 drivers/gpu/drm/i915/i915_reg.h     |  2 ++
 2 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 83f40e8..cfe238c 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2673,9 +2673,12 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 	seq_printf(m, "Re-enable work scheduled: %s\n",
 		   yesno(work_busy(&dev_priv->psr.work.work)));
 
-	if (HAS_DDI(dev))
-		enabled = I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE;
-	else {
+	if (HAS_DDI(dev)) {
+		if (dev_priv->psr.psr2_support)
+			enabled = I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE;
+		else
+			enabled = I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE;
+	} else {
 		for_each_pipe(dev_priv, pipe) {
 			stat[pipe] = I915_READ(VLV_PSRSTAT(pipe)) &
 				VLV_EDP_PSR_CURR_STATE_MASK;
@@ -2684,7 +2687,6 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 				enabled = true;
 		}
 	}
-
 	seq_printf(m, "Main link in standby mode: %s\n",
 		   yesno(dev_priv->psr.link_standby));
 
@@ -2708,6 +2710,32 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 
 		seq_printf(m, "Performance_Counter: %u\n", psrperf);
 	}
+
+	if (dev_priv->psr.psr2_support) {
+	static const char * const live_status[] = {
+				"idle",
+				"CAPTURE",
+				"CAPTURE_Fs",
+				"SLEEP",
+				"BUFON_FW",
+				"ML_UP",
+				"SU_STANDBY",
+				"FAST_SLEEP",
+				"DEEP_SLEEP",
+				"BUF_ON",
+				"TG_ON" };
+	u8 pos = (I915_READ(EDP_PSR2_STATUS_CTL) &
+		 EDP_PSR2_STATUS_STATE_MASK) >>
+		 EDP_PSR2_STATUS_STATE_SHIFT;
+
+		seq_printf(m, "PSR2_STATUS_EDP: %x\n",
+			  I915_READ(EDP_PSR2_STATUS_CTL));
+
+		if (pos <= EDP_PSR2_STATUS_TG_ON)
+			seq_printf(m, "PSR2 live state %s\n",
+				live_status[pos]);
+	}
+
 	mutex_unlock(&dev_priv->psr.lock);
 
 	intel_runtime_pm_put(dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 49682f5..969d754 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3326,6 +3326,8 @@ enum {
 #define EDP_PSR2_STATUS_CTL		_MMIO(0x6f940)
 #define EDP_PSR2_STATUS_STATE_MASK	(0xf<<28)
 #define EDP_PSR2_STATUS_STATE_IDLE	0
+#define EDP_PSR2_STATUS_STATE_SHIFT	28
+#define EDP_PSR2_STATUS_TG_ON		0xa
 
 /* VGA port control */
 #define ADPA			_MMIO(0x61100)
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915/psr:Adds Y-cordinate to skl_psr_setup_vsc

[Ville Syrjälä]

On Thu, Aug 11, 2016 at 01:07:50PM +0530, vathsala nagaraju wrote:
> Adds Y-co-ordinate support to skl_psr_setup_vsc as
> per edp 1.4 spec,table 6-11:VSC SDP HEADER
> Extension for psr2 support.
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: vathsala nagaraju <vathsala.nagaraju@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  2 ++
>  drivers/gpu/drm/i915/intel_dp.c  | 22 ++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_psr.c | 13 ++++++++++++-
>  include/drm/drm_dp_helper.h      |  5 ++++-
>  4 files changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 7f2754a..79ce64f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1022,6 +1022,8 @@ struct i915_psr {
>  	bool psr2_support;
>  	bool aux_frame_sync;
>  	bool link_standby;
> +	bool y_cord_support;
> +	bool colorimetry_support;
>  };
>  
>  enum intel_pch {
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 364db90..19e9ecf 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3439,6 +3439,28 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp)
>  		dev_priv->psr.psr2_support = dev_priv->psr.aux_frame_sync;
>  		DRM_DEBUG_KMS("PSR2 %s on sink",
>  			      dev_priv->psr.psr2_support ? "supported" : "not supported");
> +
> +		if (dev_priv->psr.psr2_support) {
> +			uint8_t psr_caps, dprx;
> +
> +			/*check if panel supports Y-Cordinate*/
> +			drm_dp_dpcd_readb(&intel_dp->aux,
> +					DP_PSR_CAPS,
> +					&psr_caps);

intel_dp->edp_dpcd[1]

We should probably add something resembling dp_link_status() for each
DPCD chunk we cache, to make it less confusing to use them.

> +			if (psr_caps & DP_PSR_Y_COORDINATE)
> +				dev_priv->psr.y_cord_support = true;
> +			else
> +				dev_priv->psr.y_cord_support = false;
> +			/* check for COLORIMETRY SUPPORT */
> +			drm_dp_dpcd_readb(&intel_dp->aux,
> +					DPRX_FEATURE_ENUMERATION_LIST,
> +					&dprx);
> +			if (dprx & VSC_SDP_EXT_FOR_COLORIMETRY_SUPPORTED)
> +				dev_priv->psr.colorimetry_support = true;
> +			else
> +				dev_priv->psr.colorimetry_support = false;
> +		}
> +
>  	}
>  
>  	/* Read the eDP Display control capabilities registers */
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 59a21c9..76a630b 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -122,13 +122,24 @@ static void vlv_psr_setup_vsc(struct intel_dp *intel_dp)
>  static void skl_psr_setup_su_vsc(struct intel_dp *intel_dp)
>  {
>  	struct edp_vsc_psr psr_vsc;
> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_device *dev = intel_dig_port->base.base.dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
>  
>  	/* Prepare VSC Header for SU as per EDP 1.4 spec, Table 6.11 */
>  	memset(&psr_vsc, 0, sizeof(psr_vsc));
>  	psr_vsc.sdp_header.HB0 = 0;
>  	psr_vsc.sdp_header.HB1 = 0x7;
>  	psr_vsc.sdp_header.HB2 = 0x3;
> -	psr_vsc.sdp_header.HB3 = 0xb;
> +	psr_vsc.sdp_header.HB3 = 0xc;
> +	if (dev_priv->psr.y_cord_support &&
> +		dev_priv->psr.colorimetry_support) {
> +		psr_vsc.sdp_header.HB2 = 0x5;
> +		psr_vsc.sdp_header.HB3 = 0x13;
> +	} else {
> +		psr_vsc.sdp_header.HB2 = 0x4;
> +		psr_vsc.sdp_header.HB3 = 0xe;
> +	}

That looks bogus. Why do we claim to have colorimetry data but
then don't fill it out?

Also you're not setting the actual y coordinate stuff anywhere, so why
would we want to indicate that we support it?

>  	intel_psr_write_vsc(intel_dp, &psr_vsc);
>  }
>  
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 63b8bd5..3d875c0 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -194,7 +194,7 @@
>  # define DP_PSR_SETUP_TIME_0                (6 << 1)
>  # define DP_PSR_SETUP_TIME_MASK             (7 << 1)
>  # define DP_PSR_SETUP_TIME_SHIFT            1
> -
> +# define DP_PSR_Y_COORDINATE		    (1 << 4)
>  /*
>   * 0x80-0x8f describe downstream port capabilities, but there are two layouts
>   * based on whether DP_DETAILED_CAP_INFO_AVAILABLE was set.  If it was not,
> @@ -640,6 +640,9 @@ struct edp_sdp_header {
>  #define EDP_SDP_HEADER_REVISION_MASK		0x1F
>  #define EDP_SDP_HEADER_VALID_PAYLOAD_BYTES	0x1F
>  
> +#define DPRX_FEATURE_ENUMERATION_LIST  0x02210
> +#define VSC_SDP_EXT_FOR_COLORIMETRY_SUPPORTED	(1 << 3)
> +
>  struct edp_vsc_psr {
>  	struct edp_sdp_header sdp_header;
>  	u8 DB0; /* Stereo Interface */
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Ro.CI.BAT: failure for Enable PSR2 for idle screen

[Patchwork]

== Series Details ==

Series: Enable PSR2 for idle screen
URL   : https://patchwork.freedesktop.org/series/10939/
State : failure

== Summary ==

Series 10939v1 Enable PSR2 for idle screen
http://patchwork.freedesktop.org/api/1.0/series/10939/revisions/1/mbox

Test gem_exec_suspend:
        Subgroup basic-s3:
                dmesg-warn -> PASS       (ro-bdw-i7-5600u)
Test kms_cursor_legacy:
        Subgroup basic-flip-vs-cursor-legacy:
                pass       -> FAIL       (ro-byt-n2820)
                fail       -> PASS       (ro-bdw-i5-5250u)
        Subgroup basic-flip-vs-cursor-varying-size:
                fail       -> PASS       (ro-byt-n2820)
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                dmesg-warn -> PASS       (ro-byt-n2820)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                dmesg-warn -> SKIP       (ro-bdw-i5-5250u)
        Subgroup suspend-read-crc-pipe-c:
                dmesg-warn -> PASS       (ro-bdw-i7-5600u)

ro-bdw-i5-5250u  total:240  pass:219  dwarn:2   dfail:0   fail:1   skip:18 
ro-bdw-i7-5600u  total:240  pass:207  dwarn:0   dfail:0   fail:1   skip:32 
ro-bsw-n3050     total:240  pass:194  dwarn:0   dfail:0   fail:4   skip:42 
ro-byt-n2820     total:240  pass:198  dwarn:0   dfail:0   fail:2   skip:40 
ro-hsw-i3-4010u  total:240  pass:214  dwarn:0   dfail:0   fail:0   skip:26 
ro-hsw-i7-4770r  total:240  pass:214  dwarn:0   dfail:0   fail:0   skip:26 
ro-ilk1-i5-650   total:235  pass:174  dwarn:0   dfail:0   fail:1   skip:60 
ro-ivb-i7-3770   total:240  pass:205  dwarn:0   dfail:0   fail:0   skip:35 
ro-ivb2-i7-3770  total:240  pass:209  dwarn:0   dfail:0   fail:0   skip:31 
ro-skl3-i5-6260u total:240  pass:222  dwarn:0   dfail:0   fail:4   skip:14 
ro-bdw-i7-5557U failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_1833/

fc1e1be drm-intel-nightly: 2016y-08m-10d-22h-09m-24s UTC integration manifest
c48b720 drm/i915/psr: adds psr2 status to debugfs
c2533a4 drm/i915/psr: enable PSR2 for idle screen
ff2f5f1 drm/i915/psr:Adds Y-cordinate to skl_psr_setup_vsc

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915/psr:Adds Y-cordinate to skl_psr_setup_vsc

[vathsala nagaraju]

On Thursday 11 August 2016 01:30 PM, Ville Syrjälä wrote:
> On Thu, Aug 11, 2016 at 01:07:50PM +0530, vathsala nagaraju wrote:
>> Adds Y-co-ordinate support to skl_psr_setup_vsc as
>> per edp 1.4 spec,table 6-11:VSC SDP HEADER
>> Extension for psr2 support.
>>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Signed-off-by: vathsala nagaraju <vathsala.nagaraju@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h  |  2 ++
>>   drivers/gpu/drm/i915/intel_dp.c  | 22 ++++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_psr.c | 13 ++++++++++++-
>>   include/drm/drm_dp_helper.h      |  5 ++++-
>>   4 files changed, 40 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 7f2754a..79ce64f 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1022,6 +1022,8 @@ struct i915_psr {
>>   	bool psr2_support;
>>   	bool aux_frame_sync;
>>   	bool link_standby;
>> +	bool y_cord_support;
>> +	bool colorimetry_support;
>>   };
>>   
>>   enum intel_pch {
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 364db90..19e9ecf 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -3439,6 +3439,28 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp)
>>   		dev_priv->psr.psr2_support = dev_priv->psr.aux_frame_sync;
>>   		DRM_DEBUG_KMS("PSR2 %s on sink",
>>   			      dev_priv->psr.psr2_support ? "supported" : "not supported");
>> +
>> +		if (dev_priv->psr.psr2_support) {
>> +			uint8_t psr_caps, dprx;
>> +
>> +			/*check if panel supports Y-Cordinate*/
>> +			drm_dp_dpcd_readb(&intel_dp->aux,
>> +					DP_PSR_CAPS,
>> +					&psr_caps);
> intel_dp->edp_dpcd[1]
>
> We should probably add something resembling dp_link_status() for each
> DPCD chunk we cache, to make it less confusing to use them.
>
>> +			if (psr_caps & DP_PSR_Y_COORDINATE)
>> +				dev_priv->psr.y_cord_support = true;
>> +			else
>> +				dev_priv->psr.y_cord_support = false;
>> +			/* check for COLORIMETRY SUPPORT */
>> +			drm_dp_dpcd_readb(&intel_dp->aux,
>> +					DPRX_FEATURE_ENUMERATION_LIST,
>> +					&dprx);
>> +			if (dprx & VSC_SDP_EXT_FOR_COLORIMETRY_SUPPORTED)
>> +				dev_priv->psr.colorimetry_support = true;
>> +			else
>> +				dev_priv->psr.colorimetry_support = false;
>> +		}
>> +
>>   	}
>>   
>>   	/* Read the eDP Display control capabilities registers */
>> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
>> index 59a21c9..76a630b 100644
>> --- a/drivers/gpu/drm/i915/intel_psr.c
>> +++ b/drivers/gpu/drm/i915/intel_psr.c
>> @@ -122,13 +122,24 @@ static void vlv_psr_setup_vsc(struct intel_dp *intel_dp)
>>   static void skl_psr_setup_su_vsc(struct intel_dp *intel_dp)
>>   {
>>   	struct edp_vsc_psr psr_vsc;
>> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>> +	struct drm_device *dev = intel_dig_port->base.base.dev;
>> +	struct drm_i915_private *dev_priv = to_i915(dev);
>>   
>>   	/* Prepare VSC Header for SU as per EDP 1.4 spec, Table 6.11 */
>>   	memset(&psr_vsc, 0, sizeof(psr_vsc));
>>   	psr_vsc.sdp_header.HB0 = 0;
>>   	psr_vsc.sdp_header.HB1 = 0x7;
>>   	psr_vsc.sdp_header.HB2 = 0x3;
>> -	psr_vsc.sdp_header.HB3 = 0xb;
>> +	psr_vsc.sdp_header.HB3 = 0xc;
>> +	if (dev_priv->psr.y_cord_support &&
>> +		dev_priv->psr.colorimetry_support) {
>> +		psr_vsc.sdp_header.HB2 = 0x5;
>> +		psr_vsc.sdp_header.HB3 = 0x13;
>> +	} else {
>> +		psr_vsc.sdp_header.HB2 = 0x4;
>> +		psr_vsc.sdp_header.HB3 = 0xe;
>> +	}
> That looks bogus. Why do we claim to have colorimetry data but
> then don't fill it out?
HB2  to be set  04 or 05
04h = 3D stereo + PSR/PSR2 + Y-coordinate.
05h = 3D stereo- + PSR/PSR2 + Y-coordinate + Pixel Encoding/Colorimetry 
Format

As of now it's defaulting to 0x4, will correct it.
>
> Also you're not setting the actual y coordinate stuff anywhere, so why
> would we want to indicate that we support it?
>
Bspec says to set CHICKEN_TRANS_EDP(0x420cc) bit 15 if Y coordinate is 
supported.
it set in patch 2.
>>   	intel_psr_write_vsc(intel_dp, &psr_vsc);
>>   }
>>   
>> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
>> index 63b8bd5..3d875c0 100644
>> --- a/include/drm/drm_dp_helper.h
>> +++ b/include/drm/drm_dp_helper.h
>> @@ -194,7 +194,7 @@
>>   # define DP_PSR_SETUP_TIME_0                (6 << 1)
>>   # define DP_PSR_SETUP_TIME_MASK             (7 << 1)
>>   # define DP_PSR_SETUP_TIME_SHIFT            1
>> -
>> +# define DP_PSR_Y_COORDINATE		    (1 << 4)
>>   /*
>>    * 0x80-0x8f describe downstream port capabilities, but there are two layouts
>>    * based on whether DP_DETAILED_CAP_INFO_AVAILABLE was set.  If it was not,
>> @@ -640,6 +640,9 @@ struct edp_sdp_header {
>>   #define EDP_SDP_HEADER_REVISION_MASK		0x1F
>>   #define EDP_SDP_HEADER_VALID_PAYLOAD_BYTES	0x1F
>>   
>> +#define DPRX_FEATURE_ENUMERATION_LIST  0x02210
>> +#define VSC_SDP_EXT_FOR_COLORIMETRY_SUPPORTED	(1 << 3)
>> +
>>   struct edp_vsc_psr {
>>   	struct edp_sdp_header sdp_header;
>>   	u8 DB0; /* Stereo Interface */
>> -- 
>> 2.7.4
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915/psr:Adds Y-cordinate to skl_psr_setup_vsc

[Ville Syrjälä]

On Fri, Aug 12, 2016 at 10:48:12AM +0530, vathsala nagaraju wrote:
> On Thursday 11 August 2016 01:30 PM, Ville Syrjälä wrote:
> > On Thu, Aug 11, 2016 at 01:07:50PM +0530, vathsala nagaraju wrote:
> >> Adds Y-co-ordinate support to skl_psr_setup_vsc as
> >> per edp 1.4 spec,table 6-11:VSC SDP HEADER
> >> Extension for psr2 support.
> >>
> >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >> Signed-off-by: vathsala nagaraju <vathsala.nagaraju@intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/i915_drv.h  |  2 ++
> >>   drivers/gpu/drm/i915/intel_dp.c  | 22 ++++++++++++++++++++++
> >>   drivers/gpu/drm/i915/intel_psr.c | 13 ++++++++++++-
> >>   include/drm/drm_dp_helper.h      |  5 ++++-
> >>   4 files changed, 40 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >> index 7f2754a..79ce64f 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> @@ -1022,6 +1022,8 @@ struct i915_psr {
> >>   	bool psr2_support;
> >>   	bool aux_frame_sync;
> >>   	bool link_standby;
> >> +	bool y_cord_support;
> >> +	bool colorimetry_support;
> >>   };
> >>   
> >>   enum intel_pch {
> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >> index 364db90..19e9ecf 100644
> >> --- a/drivers/gpu/drm/i915/intel_dp.c
> >> +++ b/drivers/gpu/drm/i915/intel_dp.c
> >> @@ -3439,6 +3439,28 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp)
> >>   		dev_priv->psr.psr2_support = dev_priv->psr.aux_frame_sync;
> >>   		DRM_DEBUG_KMS("PSR2 %s on sink",
> >>   			      dev_priv->psr.psr2_support ? "supported" : "not supported");
> >> +
> >> +		if (dev_priv->psr.psr2_support) {
> >> +			uint8_t psr_caps, dprx;
> >> +
> >> +			/*check if panel supports Y-Cordinate*/
> >> +			drm_dp_dpcd_readb(&intel_dp->aux,
> >> +					DP_PSR_CAPS,
> >> +					&psr_caps);
> > intel_dp->edp_dpcd[1]
> >
> > We should probably add something resembling dp_link_status() for each
> > DPCD chunk we cache, to make it less confusing to use them.
> >
> >> +			if (psr_caps & DP_PSR_Y_COORDINATE)
> >> +				dev_priv->psr.y_cord_support = true;
> >> +			else
> >> +				dev_priv->psr.y_cord_support = false;
> >> +			/* check for COLORIMETRY SUPPORT */
> >> +			drm_dp_dpcd_readb(&intel_dp->aux,
> >> +					DPRX_FEATURE_ENUMERATION_LIST,
> >> +					&dprx);
> >> +			if (dprx & VSC_SDP_EXT_FOR_COLORIMETRY_SUPPORTED)
> >> +				dev_priv->psr.colorimetry_support = true;
> >> +			else
> >> +				dev_priv->psr.colorimetry_support = false;
> >> +		}
> >> +
> >>   	}
> >>   
> >>   	/* Read the eDP Display control capabilities registers */
> >> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> >> index 59a21c9..76a630b 100644
> >> --- a/drivers/gpu/drm/i915/intel_psr.c
> >> +++ b/drivers/gpu/drm/i915/intel_psr.c
> >> @@ -122,13 +122,24 @@ static void vlv_psr_setup_vsc(struct intel_dp *intel_dp)
> >>   static void skl_psr_setup_su_vsc(struct intel_dp *intel_dp)
> >>   {
> >>   	struct edp_vsc_psr psr_vsc;
> >> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> >> +	struct drm_device *dev = intel_dig_port->base.base.dev;
> >> +	struct drm_i915_private *dev_priv = to_i915(dev);
> >>   
> >>   	/* Prepare VSC Header for SU as per EDP 1.4 spec, Table 6.11 */
> >>   	memset(&psr_vsc, 0, sizeof(psr_vsc));
> >>   	psr_vsc.sdp_header.HB0 = 0;
> >>   	psr_vsc.sdp_header.HB1 = 0x7;
> >>   	psr_vsc.sdp_header.HB2 = 0x3;
> >> -	psr_vsc.sdp_header.HB3 = 0xb;
> >> +	psr_vsc.sdp_header.HB3 = 0xc;
> >> +	if (dev_priv->psr.y_cord_support &&
> >> +		dev_priv->psr.colorimetry_support) {
> >> +		psr_vsc.sdp_header.HB2 = 0x5;
> >> +		psr_vsc.sdp_header.HB3 = 0x13;
> >> +	} else {
> >> +		psr_vsc.sdp_header.HB2 = 0x4;
> >> +		psr_vsc.sdp_header.HB3 = 0xe;
> >> +	}
> > That looks bogus. Why do we claim to have colorimetry data but
> > then don't fill it out?
> HB2  to be set  04 or 05
> 04h = 3D stereo + PSR/PSR2 + Y-coordinate.
> 05h = 3D stereo- + PSR/PSR2 + Y-coordinate + Pixel Encoding/Colorimetry 
> Format
> 
> As of now it's defaulting to 0x4, will correct it.
> >
> > Also you're not setting the actual y coordinate stuff anywhere, so why
> > would we want to indicate that we support it?
> >
> Bspec says to set CHICKEN_TRANS_EDP(0x420cc) bit 15 if Y coordinate is 
> supported.
> it set in patch 2.

This whole part of the spec looks a wee bit inadequate.

Hmm. So bit 25 of CHICKEN_TRANS_EDP seems to control whether the
hardware will generate part of the VSC SDP or not. But nowhere does it
explain which part that is. The sequence doesn't even mention that bit,
but it does mention bit 12 which means "This field enables the
programmable header for the PSR2 VSC packet.". Not sure what that means.
If that means the hardware can generate the HB0-3 bytes, it would be
nice to know what exactly it will put there. And because of this I can't
be sure why we have to switch to the manual header mode in PSR2. Maybe
it means the hardware generates a header that doesn't allow the Y
coordinate stuff. Hmm. Can we maybe read out the hardware generated data
via the VSC DIP?

And then there are these bits 11 and 15 which controls *something* about
the Y coordindate stuff. But it doesn't actually explain if the hardware
will fill those out or just send/not send based on these bits. If we
assume it will fill them out, then yes, I suppose we should make sure
the VSC SDP version and size are high enough to include them.

Also how does the hardware handle the SU granularity? I see no bits to
control that, so I can't see how the hardware could know what it has to
do here. Hmm. PSR2_MAN_TRK_CTL seems to say that when using manual
tracking, it's going to send chunks of 4 scanlines always, which I
suppose will satisfy the worst case for both the X and Y granularity.
So I guess the hardware tracking mode would just do the same.

Oh, and while reading the eDP spec, I noticed that the AUX frame sync
requires GTC, which we totally don't seem to even configure/enable.
We do however tell the sink to enable AUX frame sync whenever it
supports it. AUX frame sync is mandatory for PSR2 SU AFAICS, and
without SU it doesn't seem to be needed. I haven't spotted any patches
for GTC, so I'm asuming it's all still a bit broken.

> >>   	intel_psr_write_vsc(intel_dp, &psr_vsc);
> >>   }
> >>   
> >> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> >> index 63b8bd5..3d875c0 100644
> >> --- a/include/drm/drm_dp_helper.h
> >> +++ b/include/drm/drm_dp_helper.h
> >> @@ -194,7 +194,7 @@
> >>   # define DP_PSR_SETUP_TIME_0                (6 << 1)
> >>   # define DP_PSR_SETUP_TIME_MASK             (7 << 1)
> >>   # define DP_PSR_SETUP_TIME_SHIFT            1
> >> -
> >> +# define DP_PSR_Y_COORDINATE		    (1 << 4)
> >>   /*
> >>    * 0x80-0x8f describe downstream port capabilities, but there are two layouts
> >>    * based on whether DP_DETAILED_CAP_INFO_AVAILABLE was set.  If it was not,
> >> @@ -640,6 +640,9 @@ struct edp_sdp_header {
> >>   #define EDP_SDP_HEADER_REVISION_MASK		0x1F
> >>   #define EDP_SDP_HEADER_VALID_PAYLOAD_BYTES	0x1F
> >>   
> >> +#define DPRX_FEATURE_ENUMERATION_LIST  0x02210
> >> +#define VSC_SDP_EXT_FOR_COLORIMETRY_SUPPORTED	(1 << 3)
> >> +
> >>   struct edp_vsc_psr {
> >>   	struct edp_sdp_header sdp_header;
> >>   	u8 DB0; /* Stereo Interface */
> >> -- 
> >> 2.7.4
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915/psr:Adds Y-cordinate to skl_psr_setup_vsc

[vathsala nagaraju]

[-- Attachment #1.1: Type: text/plain, Size: 8352 bytes --]

On Friday 12 August 2016 12:02 PM, Ville Syrjälä wrote:
> On Fri, Aug 12, 2016 at 10:48:12AM +0530, vathsala nagaraju wrote:
>> On Thursday 11 August 2016 01:30 PM, Ville Syrjälä wrote:
>>> On Thu, Aug 11, 2016 at 01:07:50PM +0530, vathsala nagaraju wrote:
>>>> Adds Y-co-ordinate support to skl_psr_setup_vsc as
>>>> per edp 1.4 spec,table 6-11:VSC SDP HEADER
>>>> Extension for psr2 support.
>>>>
>>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>>> Signed-off-by: vathsala nagaraju <vathsala.nagaraju@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/i915_drv.h  |  2 ++
>>>>    drivers/gpu/drm/i915/intel_dp.c  | 22 ++++++++++++++++++++++
>>>>    drivers/gpu/drm/i915/intel_psr.c | 13 ++++++++++++-
>>>>    include/drm/drm_dp_helper.h      |  5 ++++-
>>>>    4 files changed, 40 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>>> index 7f2754a..79ce64f 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>> @@ -1022,6 +1022,8 @@ struct i915_psr {
>>>>    	bool psr2_support;
>>>>    	bool aux_frame_sync;
>>>>    	bool link_standby;
>>>> +	bool y_cord_support;
>>>> +	bool colorimetry_support;
>>>>    };
>>>>    
>>>>    enum intel_pch {
>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>>> index 364db90..19e9ecf 100644
>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>> @@ -3439,6 +3439,28 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp)
>>>>    		dev_priv->psr.psr2_support = dev_priv->psr.aux_frame_sync;
>>>>    		DRM_DEBUG_KMS("PSR2 %s on sink",
>>>>    			      dev_priv->psr.psr2_support ? "supported" : "not supported");
>>>> +
>>>> +		if (dev_priv->psr.psr2_support) {
>>>> +			uint8_t psr_caps, dprx;
>>>> +
>>>> +			/*check if panel supports Y-Cordinate*/
>>>> +			drm_dp_dpcd_readb(&intel_dp->aux,
>>>> +					DP_PSR_CAPS,
>>>> +					&psr_caps);
>>> intel_dp->edp_dpcd[1]
>>>
>>> We should probably add something resembling dp_link_status() for each
>>> DPCD chunk we cache, to make it less confusing to use them.
>>>
>>>> +			if (psr_caps & DP_PSR_Y_COORDINATE)
>>>> +				dev_priv->psr.y_cord_support = true;
>>>> +			else
>>>> +				dev_priv->psr.y_cord_support = false;
>>>> +			/* check for COLORIMETRY SUPPORT */
>>>> +			drm_dp_dpcd_readb(&intel_dp->aux,
>>>> +					DPRX_FEATURE_ENUMERATION_LIST,
>>>> +					&dprx);
>>>> +			if (dprx & VSC_SDP_EXT_FOR_COLORIMETRY_SUPPORTED)
>>>> +				dev_priv->psr.colorimetry_support = true;
>>>> +			else
>>>> +				dev_priv->psr.colorimetry_support = false;
>>>> +		}
>>>> +
>>>>    	}
>>>>    
>>>>    	/* Read the eDP Display control capabilities registers */
>>>> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
>>>> index 59a21c9..76a630b 100644
>>>> --- a/drivers/gpu/drm/i915/intel_psr.c
>>>> +++ b/drivers/gpu/drm/i915/intel_psr.c
>>>> @@ -122,13 +122,24 @@ static void vlv_psr_setup_vsc(struct intel_dp *intel_dp)
>>>>    static void skl_psr_setup_su_vsc(struct intel_dp *intel_dp)
>>>>    {
>>>>    	struct edp_vsc_psr psr_vsc;
>>>> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>>>> +	struct drm_device *dev = intel_dig_port->base.base.dev;
>>>> +	struct drm_i915_private *dev_priv = to_i915(dev);
>>>>    
>>>>    	/* Prepare VSC Header for SU as per EDP 1.4 spec, Table 6.11 */
>>>>    	memset(&psr_vsc, 0, sizeof(psr_vsc));
>>>>    	psr_vsc.sdp_header.HB0 = 0;
>>>>    	psr_vsc.sdp_header.HB1 = 0x7;
>>>>    	psr_vsc.sdp_header.HB2 = 0x3;
>>>> -	psr_vsc.sdp_header.HB3 = 0xb;
>>>> +	psr_vsc.sdp_header.HB3 = 0xc;
>>>> +	if (dev_priv->psr.y_cord_support &&
>>>> +		dev_priv->psr.colorimetry_support) {
>>>> +		psr_vsc.sdp_header.HB2 = 0x5;
>>>> +		psr_vsc.sdp_header.HB3 = 0x13;
>>>> +	} else {
>>>> +		psr_vsc.sdp_header.HB2 = 0x4;
>>>> +		psr_vsc.sdp_header.HB3 = 0xe;
>>>> +	}
>>> That looks bogus. Why do we claim to have colorimetry data but
>>> then don't fill it out?
>> HB2  to be set  04 or 05
>> 04h = 3D stereo + PSR/PSR2 + Y-coordinate.
>> 05h = 3D stereo- + PSR/PSR2 + Y-coordinate + Pixel Encoding/Colorimetry
>> Format
>>
>> As of now it's defaulting to 0x4, will correct it.
>>> Also you're not setting the actual y coordinate stuff anywhere, so why
>>> would we want to indicate that we support it?
>>>
>> Bspec says to set CHICKEN_TRANS_EDP(0x420cc) bit 15 if Y coordinate is
>> supported.
>> it set in patch 2.
> This whole part of the spec looks a wee bit inadequate.
>
> Hmm. So bit 25 of CHICKEN_TRANS_EDP seems to control whether the
> hardware will generate part of the VSC SDP or not. But nowhere does it
> explain which part that is. The sequence doesn't even mention that bit,
> but it does mention bit 12 which means "This field enables the
> programmable header for the PSR2 VSC packet.". Not sure what that means.
This means bit 12 must be enabled to program hb0 to hb3 from software
> If that means the hardware can generate the HB0-3 bytes, it would be
> nice to know what exactly it will put there. And because of this I can't
> be sure why we have to switch to the manual header mode in PSR2. Maybe
> it means the hardware generates a header that doesn't allow the Y
> coordinate stuff. Hmm. Can we maybe read out the hardware generated data
> via the VSC DIP?
hardware folks informed that it's not possible to read the data.
Only way is through connecting  dp analyser.
>
> And then there are these bits 11 and 15 which controls *something* about
> the Y coordindate stuff. But it doesn't actually explain if the hardware
> will fill those out or just send/not send based on these bits. If we
> assume it will fill them out, then yes, I suppose we should make sure
> the VSC SDP version and size are high enough to include them.

HW will send based on enable and disable setting of this bit.

bit 15

	

bit 11

	

Description

0

	

0

	

Y-coordinate (DB12-DB13) and Y-valid (DB1 bit6) are disabled.

0

	

1

	

Not a valid setting.

1

	

0

	

Y-coordinate (VSC bytes DB12-DB13) is sent and Y-valid (DB1 bit6) is 
included.

1

	

1

	

Y-coordinate (VSC bytes DB12-DB13) is sent and Y-valid (DB1 bit6) is not 
included.

>
> Also how does the hardware handle the SU granularity? I see no bits to
> control that, so I can't see how the hardware could know what it has to
> do here. Hmm. PSR2_MAN_TRK_CTL seems to say that when using manual
> tracking, it's going to send chunks of 4 scanlines always, which I
> suppose will satisfy the worst case for both the X and Y granularity.
> So I guess the hardware tracking mode would just do the same.
HW supports SU granularity of 4 lines.
> Oh, and while reading the eDP spec, I noticed that the AUX frame sync
> requires GTC, which we totally don't seem to even configure/enable.
> We do however tell the sink to enable AUX frame sync whenever it
> supports it. AUX frame sync is mandatory for PSR2 SU AFAICS, and
> without SU it doesn't seem to be needed. I haven't spotted any patches
> for GTC, so I'm asuming it's all still a bit broken.
>
>>>>    	intel_psr_write_vsc(intel_dp, &psr_vsc);
>>>>    }
>>>>    
>>>> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
>>>> index 63b8bd5..3d875c0 100644
>>>> --- a/include/drm/drm_dp_helper.h
>>>> +++ b/include/drm/drm_dp_helper.h
>>>> @@ -194,7 +194,7 @@
>>>>    # define DP_PSR_SETUP_TIME_0                (6 << 1)
>>>>    # define DP_PSR_SETUP_TIME_MASK             (7 << 1)
>>>>    # define DP_PSR_SETUP_TIME_SHIFT            1
>>>> -
>>>> +# define DP_PSR_Y_COORDINATE		    (1 << 4)
>>>>    /*
>>>>     * 0x80-0x8f describe downstream port capabilities, but there are two layouts
>>>>     * based on whether DP_DETAILED_CAP_INFO_AVAILABLE was set.  If it was not,
>>>> @@ -640,6 +640,9 @@ struct edp_sdp_header {
>>>>    #define EDP_SDP_HEADER_REVISION_MASK		0x1F
>>>>    #define EDP_SDP_HEADER_VALID_PAYLOAD_BYTES	0x1F
>>>>    
>>>> +#define DPRX_FEATURE_ENUMERATION_LIST  0x02210
>>>> +#define VSC_SDP_EXT_FOR_COLORIMETRY_SUPPORTED	(1 << 3)
>>>> +
>>>>    struct edp_vsc_psr {
>>>>    	struct edp_sdp_header sdp_header;
>>>>    	u8 DB0; /* Stereo Interface */
>>>> -- 
>>>> 2.7.4
>>>>
>>>> _______________________________________________
>>>> Intel-gfx mailing list
>>>> Intel-gfx@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx


[-- Attachment #1.2: Type: text/html, Size: 14013 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx