[PATCH 1/4] drm/i915/vbt: Add 'tp4' to varibles holding TP2/3/4 PSR wakeup time

[PATCH 1/4] drm/i915/vbt: Add 'tp4' to varibles holding TP2/3/4 PSR wakeup time

[José Roberto de Souza]

Recent update in spec made the field holding the TP2 and TP3 wakeup
time for PSR also hold the TP4, so lets rename the variables to
reflect that.

BSpec: 20131

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h       |  2 +-
 drivers/gpu/drm/i915/intel_bios.c     | 16 ++++++++--------
 drivers/gpu/drm/i915/intel_psr.c      | 14 +++++++-------
 drivers/gpu/drm/i915/intel_vbt_defs.h |  2 +-
 4 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 310d9e1e1620..e717c3132692 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -996,7 +996,7 @@ struct intel_vbt_data {
 		int idle_frames;
 		enum psr_lines_to_wait lines_to_wait;
 		int tp1_wakeup_time_us;
-		int tp2_tp3_wakeup_time_us;
+		int tp2_tp3_tp4_wakeup_time_us;
 	} psr;
 
 	struct {
diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 561a4f9f044c..cd99bf88bf6c 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -738,27 +738,27 @@ parse_psr(struct drm_i915_private *dev_priv, const struct bdb_header *bdb)
 			break;
 		}
 
-		switch (psr_table->tp2_tp3_wakeup_time) {
+		switch (psr_table->tp2_tp3_tp4_wakeup_time) {
 		case 0:
-			dev_priv->vbt.psr.tp2_tp3_wakeup_time_us = 500;
+			dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us = 500;
 			break;
 		case 1:
-			dev_priv->vbt.psr.tp2_tp3_wakeup_time_us = 100;
+			dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us = 100;
 			break;
 		case 3:
-			dev_priv->vbt.psr.tp2_tp3_wakeup_time_us = 0;
+			dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us = 0;
 			break;
 		default:
-			DRM_DEBUG_KMS("VBT tp2_tp3 wakeup time value %d is outside range[0-3], defaulting to max value 2500us\n",
-					psr_table->tp2_tp3_wakeup_time);
+			DRM_DEBUG_KMS("VBT tp2_tp3_tp4 wakeup time value %d is outside range[0-3], defaulting to max value 2500us\n",
+				      psr_table->tp2_tp3_tp4_wakeup_time);
 			/* fallthrough */
 		case 2:
-			dev_priv->vbt.psr.tp2_tp3_wakeup_time_us = 2500;
+			dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us = 2500;
 		break;
 		}
 	} else {
 		dev_priv->vbt.psr.tp1_wakeup_time_us = psr_table->tp1_wakeup_time * 100;
-		dev_priv->vbt.psr.tp2_tp3_wakeup_time_us = psr_table->tp2_tp3_wakeup_time * 100;
+		dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us = psr_table->tp2_tp3_tp4_wakeup_time * 100;
 	}
 }
 
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 0f6b2b4702e3..49b4b3371bef 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -468,11 +468,11 @@ static void hsw_activate_psr1(struct intel_dp *intel_dp)
 	else
 		val |= EDP_PSR_TP1_TIME_2500us;
 
-	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us == 0)
+	if (dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us == 0)
 		val |=  EDP_PSR_TP2_TP3_TIME_0us;
-	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 100)
+	else if (dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us <= 100)
 		val |= EDP_PSR_TP2_TP3_TIME_100us;
-	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 500)
+	else if (dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us <= 500)
 		val |= EDP_PSR_TP2_TP3_TIME_500us;
 	else
 		val |= EDP_PSR_TP2_TP3_TIME_2500us;
@@ -509,12 +509,12 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
 
 	val |= EDP_PSR2_FRAME_BEFORE_SU(dev_priv->psr.sink_sync_latency + 1);
 
-	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us >= 0 &&
-	    dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 50)
+	if (dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us >= 0 &&
+	    dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us <= 50)
 		val |= EDP_PSR2_TP2_TIME_50us;
-	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 100)
+	else if (dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us <= 100)
 		val |= EDP_PSR2_TP2_TIME_100us;
-	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 500)
+	else if (dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us <= 500)
 		val |= EDP_PSR2_TP2_TIME_500us;
 	else
 		val |= EDP_PSR2_TP2_TIME_2500us;
diff --git a/drivers/gpu/drm/i915/intel_vbt_defs.h b/drivers/gpu/drm/i915/intel_vbt_defs.h
index bf3662ad5fed..4ed66efde49f 100644
--- a/drivers/gpu/drm/i915/intel_vbt_defs.h
+++ b/drivers/gpu/drm/i915/intel_vbt_defs.h
@@ -771,7 +771,7 @@ struct psr_table {
 
 	/* TP wake up time in multiple of 100 */
 	u16 tp1_wakeup_time;
-	u16 tp2_tp3_wakeup_time;
+	u16 tp2_tp3_tp4_wakeup_time;
 } __packed;
 
 struct bdb_psr {
-- 
2.20.1

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

[PATCH 2/4] drm/i915/psr: Store VBT TP wakeup times into a enum

[José Roberto de Souza]

Newer VBTs and the PSR registers uses a enum to set the TPs wakeup
time, so lets use this format to store wakeup times and avoid
conversions every time that PSR is activated.

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h   | 12 +++++-
 drivers/gpu/drm/i915/i915_reg.h   | 16 ++------
 drivers/gpu/drm/i915/intel_bios.c | 65 +++++++++++++++----------------
 drivers/gpu/drm/i915/intel_psr.c  | 29 ++------------
 4 files changed, 47 insertions(+), 75 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e717c3132692..d9893d35f0e2 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -959,6 +959,14 @@ enum psr_lines_to_wait {
 	PSR_8_LINES_TO_WAIT
 };
 
+enum psr_tp_wakeup_time {
+	PSR_TP_WAKEUP_TIME_500USEC = 0,
+	PSR_TP_WAKEUP_TIME_100USEC,
+	PSR_TP_WAKEUP_TIME_2500USEC,
+	PSR_TP_WAKEUP_TIME_NONE,
+	PSR_TP_WAKEUP_TIME_LAST
+};
+
 struct intel_vbt_data {
 	struct drm_display_mode *lfp_lvds_vbt_mode; /* if any */
 	struct drm_display_mode *sdvo_lvds_vbt_mode; /* if any */
@@ -995,8 +1003,8 @@ struct intel_vbt_data {
 		bool require_aux_wakeup;
 		int idle_frames;
 		enum psr_lines_to_wait lines_to_wait;
-		int tp1_wakeup_time_us;
-		int tp2_tp3_tp4_wakeup_time_us;
+		enum psr_tp_wakeup_time tp1_wakeup_time;
+		enum psr_tp_wakeup_time tp2_tp3_tp4_wakeup_time;
 	} psr;
 
 	struct {
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index fad5a9e8b44d..5faca634ee70 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4161,14 +4161,8 @@ enum {
 #define   EDP_PSR_TP1_TP2_SEL			(0 << 11)
 #define   EDP_PSR_TP1_TP3_SEL			(1 << 11)
 #define   EDP_PSR_CRC_ENABLE			(1 << 10) /* BDW+ */
-#define   EDP_PSR_TP2_TP3_TIME_500us		(0 << 8)
-#define   EDP_PSR_TP2_TP3_TIME_100us		(1 << 8)
-#define   EDP_PSR_TP2_TP3_TIME_2500us		(2 << 8)
-#define   EDP_PSR_TP2_TP3_TIME_0us		(3 << 8)
-#define   EDP_PSR_TP1_TIME_500us		(0 << 4)
-#define   EDP_PSR_TP1_TIME_100us		(1 << 4)
-#define   EDP_PSR_TP1_TIME_2500us		(2 << 4)
-#define   EDP_PSR_TP1_TIME_0us			(3 << 4)
+#define   EDP_PSR_TP2_TP3_TIME_SHIFT		(8)
+#define   EDP_PSR_TP1_TIME_SHIFT		(4)
 #define   EDP_PSR_IDLE_FRAME_SHIFT		0
 
 /* Bspec claims those aren't shifted but stay at 0x64800 */
@@ -4234,11 +4228,7 @@ enum {
 #define   EDP_Y_COORDINATE_ENABLE	(1 << 25) /* GLK and CNL+ */
 #define   EDP_MAX_SU_DISABLE_TIME(t)	((t) << 20)
 #define   EDP_MAX_SU_DISABLE_TIME_MASK	(0x1f << 20)
-#define   EDP_PSR2_TP2_TIME_500us	(0 << 8)
-#define   EDP_PSR2_TP2_TIME_100us	(1 << 8)
-#define   EDP_PSR2_TP2_TIME_2500us	(2 << 8)
-#define   EDP_PSR2_TP2_TIME_50us	(3 << 8)
-#define   EDP_PSR2_TP2_TIME_MASK	(3 << 8)
+#define   EDP_PSR2_TP2_TP3_TIME_SHIFT	(8)
 #define   EDP_PSR2_FRAME_BEFORE_SU_SHIFT 4
 #define   EDP_PSR2_FRAME_BEFORE_SU_MASK	(0xf << 4)
 #define   EDP_PSR2_FRAME_BEFORE_SU(a)	((a) << 4)
diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index cd99bf88bf6c..6de6f6f1deec 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -719,46 +719,43 @@ parse_psr(struct drm_i915_private *dev_priv, const struct bdb_header *bdb)
 	if (bdb->version >= 205 &&
 	    (IS_GEN9_BC(dev_priv) || IS_GEMINILAKE(dev_priv) ||
 	     INTEL_GEN(dev_priv) >= 10)) {
-		switch (psr_table->tp1_wakeup_time) {
-		case 0:
-			dev_priv->vbt.psr.tp1_wakeup_time_us = 500;
-			break;
-		case 1:
-			dev_priv->vbt.psr.tp1_wakeup_time_us = 100;
-			break;
-		case 3:
-			dev_priv->vbt.psr.tp1_wakeup_time_us = 0;
-			break;
-		default:
+		dev_priv->vbt.psr.tp1_wakeup_time = psr_table->tp1_wakeup_time;
+		if (dev_priv->vbt.psr.tp1_wakeup_time >= PSR_TP_WAKEUP_TIME_LAST) {
 			DRM_DEBUG_KMS("VBT tp1 wakeup time value %d is outside range[0-3], defaulting to max value 2500us\n",
-					psr_table->tp1_wakeup_time);
-			/* fallthrough */
-		case 2:
-			dev_priv->vbt.psr.tp1_wakeup_time_us = 2500;
-			break;
+				      dev_priv->vbt.psr.tp1_wakeup_time);
+			dev_priv->vbt.psr.tp1_wakeup_time = PSR_TP_WAKEUP_TIME_2500USEC;
 		}
 
-		switch (psr_table->tp2_tp3_tp4_wakeup_time) {
-		case 0:
-			dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us = 500;
-			break;
-		case 1:
-			dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us = 100;
-			break;
-		case 3:
-			dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us = 0;
-			break;
-		default:
+		dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time = psr_table->tp2_tp3_tp4_wakeup_time;
+		if (dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time >= PSR_TP_WAKEUP_TIME_LAST) {
 			DRM_DEBUG_KMS("VBT tp2_tp3_tp4 wakeup time value %d is outside range[0-3], defaulting to max value 2500us\n",
-				      psr_table->tp2_tp3_tp4_wakeup_time);
-			/* fallthrough */
-		case 2:
-			dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us = 2500;
-		break;
+				      dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time);
+			dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time = PSR_TP_WAKEUP_TIME_2500USEC;
 		}
 	} else {
-		dev_priv->vbt.psr.tp1_wakeup_time_us = psr_table->tp1_wakeup_time * 100;
-		dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us = psr_table->tp2_tp3_tp4_wakeup_time * 100;
+		enum psr_tp_wakeup_time wakeup_time;
+
+		if (psr_table->tp1_wakeup_time == 0)
+			wakeup_time = PSR_TP_WAKEUP_TIME_NONE;
+		else if (psr_table->tp1_wakeup_time == 1)
+			wakeup_time = PSR_TP_WAKEUP_TIME_100USEC;
+		else if (psr_table->tp1_wakeup_time <= 5)
+			wakeup_time = PSR_TP_WAKEUP_TIME_500USEC;
+		else
+			wakeup_time = PSR_TP_WAKEUP_TIME_2500USEC;
+
+		dev_priv->vbt.psr.tp1_wakeup_time = wakeup_time;
+
+		if (psr_table->tp2_tp3_tp4_wakeup_time == 0)
+			wakeup_time = PSR_TP_WAKEUP_TIME_NONE;
+		else if (psr_table->tp2_tp3_tp4_wakeup_time == 1)
+			wakeup_time = PSR_TP_WAKEUP_TIME_100USEC;
+		else if (psr_table->tp2_tp3_tp4_wakeup_time <= 5)
+			wakeup_time = PSR_TP_WAKEUP_TIME_500USEC;
+		else
+			wakeup_time = PSR_TP_WAKEUP_TIME_2500USEC;
+
+		dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time = wakeup_time;
 	}
 }
 
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 49b4b3371bef..5daf0b9e2b42 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -459,23 +459,8 @@ static void hsw_activate_psr1(struct intel_dp *intel_dp)
 	if (dev_priv->psr.link_standby)
 		val |= EDP_PSR_LINK_STANDBY;
 
-	if (dev_priv->vbt.psr.tp1_wakeup_time_us == 0)
-		val |=  EDP_PSR_TP1_TIME_0us;
-	else if (dev_priv->vbt.psr.tp1_wakeup_time_us <= 100)
-		val |= EDP_PSR_TP1_TIME_100us;
-	else if (dev_priv->vbt.psr.tp1_wakeup_time_us <= 500)
-		val |= EDP_PSR_TP1_TIME_500us;
-	else
-		val |= EDP_PSR_TP1_TIME_2500us;
-
-	if (dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us == 0)
-		val |=  EDP_PSR_TP2_TP3_TIME_0us;
-	else if (dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us <= 100)
-		val |= EDP_PSR_TP2_TP3_TIME_100us;
-	else if (dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us <= 500)
-		val |= EDP_PSR_TP2_TP3_TIME_500us;
-	else
-		val |= EDP_PSR_TP2_TP3_TIME_2500us;
+	val |= dev_priv->vbt.psr.tp1_wakeup_time << EDP_PSR_TP1_TIME_SHIFT;
+	val |= dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time << EDP_PSR_TP2_TP3_TIME_SHIFT;
 
 	if (intel_dp_source_supports_hbr2(intel_dp) &&
 	    drm_dp_tps3_supported(intel_dp->dpcd))
@@ -509,15 +494,7 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
 
 	val |= EDP_PSR2_FRAME_BEFORE_SU(dev_priv->psr.sink_sync_latency + 1);
 
-	if (dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us >= 0 &&
-	    dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us <= 50)
-		val |= EDP_PSR2_TP2_TIME_50us;
-	else if (dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us <= 100)
-		val |= EDP_PSR2_TP2_TIME_100us;
-	else if (dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us <= 500)
-		val |= EDP_PSR2_TP2_TIME_500us;
-	else
-		val |= EDP_PSR2_TP2_TIME_2500us;
+	val |= dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time << EDP_PSR2_TP2_TP3_TIME_SHIFT;
 
 	I915_WRITE(EDP_PSR2_CTL, val);
 }
-- 
2.20.1

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

[PATCH 3/4] drm/i915/vbt: Parse and use the new field with PSR2 TP2/3/4 wakeup time

[José Roberto de Souza]

A new field with the training pattern(TP) wakeup time for PSR2 was
added to VBT, so lets use it when available otherwise it will
fallback to PSR1 wakeup time.

BSpec: 20131

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h       |  8 ++++++++
 drivers/gpu/drm/i915/intel_bios.c     | 10 ++++++++++
 drivers/gpu/drm/i915/intel_psr.c      |  2 +-
 drivers/gpu/drm/i915/intel_vbt_defs.h |  3 +++
 4 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d9893d35f0e2..e739ed9ce60c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -967,6 +967,13 @@ enum psr_tp_wakeup_time {
 	PSR_TP_WAKEUP_TIME_LAST
 };
 
+enum psr2_tp_wakeup_time {
+	PSR2_TP_WAKEUP_TIME_500USEC = 0,
+	PSR2_TP_WAKEUP_TIME_100USEC,
+	PSR2_TP_WAKEUP_TIME_2500USEC,
+	PSR2_TP_WAKEUP_TIME_50USEC
+};
+
 struct intel_vbt_data {
 	struct drm_display_mode *lfp_lvds_vbt_mode; /* if any */
 	struct drm_display_mode *sdvo_lvds_vbt_mode; /* if any */
@@ -1005,6 +1012,7 @@ struct intel_vbt_data {
 		enum psr_lines_to_wait lines_to_wait;
 		enum psr_tp_wakeup_time tp1_wakeup_time;
 		enum psr_tp_wakeup_time tp2_tp3_tp4_wakeup_time;
+		enum psr2_tp_wakeup_time psr2_tp2_tp3_tp4_wakeup_time;
 	} psr;
 
 	struct {
diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 6de6f6f1deec..23130e0d5e6c 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -757,6 +757,16 @@ parse_psr(struct drm_i915_private *dev_priv, const struct bdb_header *bdb)
 
 		dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time = wakeup_time;
 	}
+
+	if (bdb->version >= 226) {
+		u32 wakeup_time = psr_table->psr2_tp2_tp3_tp4_wakeup_time;
+
+		wakeup_time = (wakeup_time >> (2 * panel_type)) & 0x3;
+		dev_priv->vbt.psr.psr2_tp2_tp3_tp4_wakeup_time = wakeup_time;
+	} else {
+		/* Reusing PSR1 wakeup time for PSR2 in older VBTs */
+		dev_priv->vbt.psr.psr2_tp2_tp3_tp4_wakeup_time = dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time;
+	}
 }
 
 static void parse_dsi_backlight_ports(struct drm_i915_private *dev_priv,
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 5daf0b9e2b42..2fc537fb6e78 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -494,7 +494,7 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
 
 	val |= EDP_PSR2_FRAME_BEFORE_SU(dev_priv->psr.sink_sync_latency + 1);
 
-	val |= dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time << EDP_PSR2_TP2_TP3_TIME_SHIFT;
+	val |= dev_priv->vbt.psr.psr2_tp2_tp3_tp4_wakeup_time << EDP_PSR2_TP2_TP3_TIME_SHIFT;
 
 	I915_WRITE(EDP_PSR2_CTL, val);
 }
diff --git a/drivers/gpu/drm/i915/intel_vbt_defs.h b/drivers/gpu/drm/i915/intel_vbt_defs.h
index 4ed66efde49f..dc0a14977953 100644
--- a/drivers/gpu/drm/i915/intel_vbt_defs.h
+++ b/drivers/gpu/drm/i915/intel_vbt_defs.h
@@ -772,6 +772,9 @@ struct psr_table {
 	/* TP wake up time in multiple of 100 */
 	u16 tp1_wakeup_time;
 	u16 tp2_tp3_tp4_wakeup_time;
+
+	/* PSR2 TP2/TP3/TP4 wakeup time for 16 panels */
+	u32 psr2_tp2_tp3_tp4_wakeup_time;
 } __packed;
 
 struct bdb_psr {
-- 
2.20.1

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

[PATCH 4/4] drm/i915/psr: Add HBR3 support

[José Roberto de Souza]

If the sink and source supports HBR3, TP4 should be used as link
training pattern.
For PSR2 there is no register to set and enable TP4 but according to
eDP spec TP3 is still a training pattern acceptable for HBR3 panels.

Cc: Manasi Navare <manasi.d.navare@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---

Still trying to understand how PSR1 was working on ICL while sending
TP4 to a panel that only supports HBR2.

 drivers/gpu/drm/i915/i915_reg.h               |  1 +
 drivers/gpu/drm/i915/intel_dp_link_training.c |  2 +-
 drivers/gpu/drm/i915/intel_drv.h              |  1 +
 drivers/gpu/drm/i915/intel_psr.c              | 24 ++++++++++++++-----
 4 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 5faca634ee70..1e792309a79e 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4162,6 +4162,7 @@ enum {
 #define   EDP_PSR_TP1_TP3_SEL			(1 << 11)
 #define   EDP_PSR_CRC_ENABLE			(1 << 10) /* BDW+ */
 #define   EDP_PSR_TP2_TP3_TIME_SHIFT		(8)
+#define   EDP_PSR_TP4_TIME_SHIFT		(6) /* ICL+ */
 #define   EDP_PSR_TP1_TIME_SHIFT		(4)
 #define   EDP_PSR_IDLE_FRAME_SHIFT		0
 
diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
index 30be0e39bd5f..3e9798a5498c 100644
--- a/drivers/gpu/drm/i915/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
@@ -238,7 +238,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
  * or for 1.4 devices that support it, training Pattern 3 for HBR2
  * or 1.2 devices that support it, Training Pattern 2 otherwise.
  */
-static u32 intel_dp_training_pattern(struct intel_dp *intel_dp)
+u32 intel_dp_training_pattern(struct intel_dp *intel_dp)
 {
 	bool source_tps3, sink_tps3, source_tps4, sink_tps4;
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e5a436c33307..fc3e6ae92276 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1807,6 +1807,7 @@ int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
 					    int link_rate, uint8_t lane_count);
 void intel_dp_start_link_train(struct intel_dp *intel_dp);
 void intel_dp_stop_link_train(struct intel_dp *intel_dp);
+u32 intel_dp_training_pattern(struct intel_dp *intel_dp);
 int intel_dp_retrain_link(struct intel_encoder *encoder,
 			  struct drm_modeset_acquire_ctx *ctx);
 void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 2fc537fb6e78..b0525940e5e9 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -440,6 +440,7 @@ static void hsw_activate_psr1(struct intel_dp *intel_dp)
 	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
 	u32 max_sleep_time = 0x1f;
 	u32 val = EDP_PSR_ENABLE;
+	u32 tp;
 
 	/* Let's use 6 as the minimum to cover all known cases including the
 	 * off-by-one issue that HW has in some cases.
@@ -460,13 +461,24 @@ static void hsw_activate_psr1(struct intel_dp *intel_dp)
 		val |= EDP_PSR_LINK_STANDBY;
 
 	val |= dev_priv->vbt.psr.tp1_wakeup_time << EDP_PSR_TP1_TIME_SHIFT;
-	val |= dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time << EDP_PSR_TP2_TP3_TIME_SHIFT;
 
-	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;
+	tp = intel_dp_training_pattern(intel_dp);
+	if (tp == DP_TRAINING_PATTERN_4) {
+		/*
+		 * TP4 is selected by setting EDP_PSR_TP4_TIME with other value
+		 * than PSR_TP_WAKEUP_TIME_NONE
+		 */
+		val |= dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time << EDP_PSR_TP4_TIME_SHIFT;
+	} else {
+		if (INTEL_GEN(dev_priv) >= 11)
+			val |= PSR_TP_WAKEUP_TIME_NONE << EDP_PSR_TP4_TIME_SHIFT;
+
+		val |= dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time << EDP_PSR_TP2_TP3_TIME_SHIFT;
+		if (tp == DP_TRAINING_PATTERN_3)
+			val |= EDP_PSR_TP1_TP3_SEL;
+		else
+			val |= EDP_PSR_TP1_TP2_SEL;
+	}
 
 	if (INTEL_GEN(dev_priv) >= 8)
 		val |= EDP_PSR_CRC_ENABLE;
-- 
2.20.1

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

✗ Fi.CI.CHECKPATCH: warning for series starting with [1/4] drm/i915/vbt: Add 'tp4' to varibles holding TP2/3/4 PSR wakeup time

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915/vbt: Add 'tp4' to varibles holding TP2/3/4 PSR wakeup time
URL   : https://patchwork.freedesktop.org/series/55340/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
588f964609a1 drm/i915/vbt: Add 'tp4' to varibles holding TP2/3/4 PSR wakeup time
-:68: WARNING:LONG_LINE: line over 100 characters
#68: FILE: drivers/gpu/drm/i915/intel_bios.c:761:
+		dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us = psr_table->tp2_tp3_tp4_wakeup_time * 100;

total: 0 errors, 1 warnings, 0 checks, 81 lines checked
5c49dcef88ab drm/i915/psr: Store VBT TP wakeup times into a enum
f8b00a0df6fc drm/i915/vbt: Parse and use the new field with PSR2 TP2/3/4 wakeup time
-:61: WARNING:LONG_LINE: line over 100 characters
#61: FILE: drivers/gpu/drm/i915/intel_bios.c:768:
+		dev_priv->vbt.psr.psr2_tp2_tp3_tp4_wakeup_time = dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time;

total: 0 errors, 1 warnings, 0 checks, 53 lines checked
bd6cdba3da21 drm/i915/psr: Add HBR3 support

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

✗ Fi.CI.SPARSE: warning for series starting with [1/4] drm/i915/vbt: Add 'tp4' to varibles holding TP2/3/4 PSR wakeup time

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915/vbt: Add 'tp4' to varibles holding TP2/3/4 PSR wakeup time
URL   : https://patchwork.freedesktop.org/series/55340/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Sparse version: v0.5.2
Commit: drm/i915/vbt: Add 'tp4' to varibles holding TP2/3/4 PSR wakeup time
Okay!

Commit: drm/i915/psr: Store VBT TP wakeup times into a enum
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3546:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3554:16: warning: expression using sizeof(void)

Commit: drm/i915/vbt: Parse and use the new field with PSR2 TP2/3/4 wakeup time
+drivers/gpu/drm/i915/intel_bios.c:768:83:     int enum psr2_tp_wakeup_time 
+drivers/gpu/drm/i915/intel_bios.c:768:83:     int enum psr_tp_wakeup_time  versus
+drivers/gpu/drm/i915/intel_bios.c:768:83: warning: mixing different enum types
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3554:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3562:16: warning: expression using sizeof(void)

Commit: drm/i915/psr: Add HBR3 support
Okay!

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

✓ Fi.CI.BAT: success for series starting with [1/4] drm/i915/vbt: Add 'tp4' to varibles holding TP2/3/4 PSR wakeup time

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915/vbt: Add 'tp4' to varibles holding TP2/3/4 PSR wakeup time
URL   : https://patchwork.freedesktop.org/series/55340/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5439 -> Patchwork_11968
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/55340/revisions/1/mbox/

Known issues
------------

  Here are the changes found in Patchwork_11968 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_module_load@reload:
    - fi-blb-e6850:       NOTRUN -> INCOMPLETE [fdo#107718]

  * igt@i915_selftest@live_hangcheck:
    - fi-bwr-2160:        PASS -> DMESG-FAIL [fdo#108735]

  * igt@kms_frontbuffer_tracking@basic:
    - fi-icl-u3:          PASS -> FAIL [fdo#103167]

  * igt@kms_pipe_crc_basic@hang-read-crc-pipe-b:
    - fi-byt-clapper:     PASS -> FAIL [fdo#103191] / [fdo#107362] +1

  * igt@kms_pipe_crc_basic@read-crc-pipe-b:
    - fi-byt-clapper:     PASS -> FAIL [fdo#107362] +1

  
#### Possible fixes ####

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-7500u:       FAIL [fdo#108767] -> PASS

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
    - fi-byt-clapper:     FAIL [fdo#103191] / [fdo#107362] -> PASS +1

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
    - fi-blb-e6850:       INCOMPLETE [fdo#107718] -> PASS

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#108735]: https://bugs.freedesktop.org/show_bug.cgi?id=108735
  [fdo#108767]: https://bugs.freedesktop.org/show_bug.cgi?id=108767
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271


Participating hosts (48 -> 44)
------------------------------

  Missing    (4): fi-kbl-soraka fi-ilk-m540 fi-byt-squawks fi-bsw-cyan 


Build changes
-------------

    * Linux: CI_DRM_5439 -> Patchwork_11968

  CI_DRM_5439: 090b10b5d3ed38ed1033317fe7b902a87fdddf9f @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4777: 8614d5eb114a660c3bd7ff77eab8bed53424cd30 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_11968: bd6cdba3da21cee68f68dccaac5e75f8e115cdbd @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

bd6cdba3da21 drm/i915/psr: Add HBR3 support
f8b00a0df6fc drm/i915/vbt: Parse and use the new field with PSR2 TP2/3/4 wakeup time
5c49dcef88ab drm/i915/psr: Store VBT TP wakeup times into a enum
588f964609a1 drm/i915/vbt: Add 'tp4' to varibles holding TP2/3/4 PSR wakeup time

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_11968/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.IGT: failure for series starting with [1/4] drm/i915/vbt: Add 'tp4' to varibles holding TP2/3/4 PSR wakeup time

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915/vbt: Add 'tp4' to varibles holding TP2/3/4 PSR wakeup time
URL   : https://patchwork.freedesktop.org/series/55340/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_5439_full -> Patchwork_11968_full
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_11968_full absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_11968_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_11968_full:

### IGT changes ###

#### Possible regressions ####

  * igt@kms_rotation_crc@multiplane-rotation-cropping-top:
    - shard-iclb:         PASS -> FAIL

  
Known issues
------------

  Here are the changes found in Patchwork_11968_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_softpin@noreloc-s3:
    - shard-skl:          PASS -> INCOMPLETE [fdo#104108] / [fdo#107773]

  * igt@i915_selftest@live_workarounds:
    - shard-iclb:         PASS -> DMESG-FAIL [fdo#108954]

  * igt@kms_atomic_transition@plane-all-modeset-transition-internal-panels:
    - shard-iclb:         PASS -> DMESG-WARN [fdo#107724] +3

  * igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-b:
    - shard-iclb:         NOTRUN -> DMESG-WARN [fdo#107956] +2

  * igt@kms_ccs@pipe-b-crc-primary-basic:
    - shard-iclb:         NOTRUN -> FAIL [fdo#107725] +2

  * igt@kms_color@pipe-a-ctm-max:
    - shard-apl:          PASS -> FAIL [fdo#108147]

  * igt@kms_color@pipe-a-degamma:
    - shard-apl:          PASS -> FAIL [fdo#104782] / [fdo#108145]

  * igt@kms_cursor_crc@cursor-128x128-suspend:
    - shard-glk:          PASS -> FAIL [fdo#103232]
    - shard-skl:          NOTRUN -> INCOMPLETE [fdo#104108]

  * igt@kms_cursor_crc@cursor-256x256-random:
    - shard-iclb:         NOTRUN -> FAIL [fdo#103232] +3
    - shard-apl:          PASS -> FAIL [fdo#103232]

  * igt@kms_fbcon_fbt@psr-suspend:
    - shard-skl:          NOTRUN -> FAIL [fdo#107882]

  * igt@kms_flip@2x-busy-flip-interruptible:
    - shard-glk:          PASS -> INCOMPLETE [fdo#103359] / [k.org#198133]

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-wc:
    - shard-apl:          PASS -> FAIL [fdo#103167] +1

  * igt@kms_frontbuffer_tracking@fbc-1p-rte:
    - shard-apl:          PASS -> FAIL [fdo#103167] / [fdo#105682]

  * igt@kms_frontbuffer_tracking@fbc-2p-primscrn-cur-indfb-move:
    - shard-glk:          PASS -> FAIL [fdo#103167] +1

  * igt@kms_frontbuffer_tracking@fbc-stridechange:
    - shard-skl:          NOTRUN -> FAIL [fdo#105683]

  * igt@kms_frontbuffer_tracking@psr-1p-primscrn-spr-indfb-fullscreen:
    - shard-iclb:         PASS -> FAIL [fdo#103167] +1

  * igt@kms_frontbuffer_tracking@psr-1p-primscrn-spr-indfb-move:
    - shard-iclb:         NOTRUN -> FAIL [fdo#103167] +1

  * igt@kms_hdmi_inject@inject-audio:
    - shard-iclb:         NOTRUN -> FAIL [fdo#102370]

  * igt@kms_plane@pixel-format-pipe-b-planes-source-clamping:
    - shard-skl:          NOTRUN -> DMESG-WARN [fdo#106885]

  * igt@kms_plane@plane-panning-bottom-right-pipe-a-planes:
    - shard-iclb:         PASS -> DMESG-WARN [fdo#107724] / [fdo#108336] +2
    - shard-skl:          PASS -> FAIL [fdo#103166]

  * igt@kms_plane_alpha_blend@pipe-a-alpha-opaque-fb:
    - shard-skl:          NOTRUN -> FAIL [fdo#108145] +3

  * igt@kms_plane_multiple@atomic-pipe-a-tiling-none:
    - shard-iclb:         PASS -> FAIL [fdo#103166]

  * igt@kms_plane_multiple@atomic-pipe-a-tiling-x:
    - shard-apl:          PASS -> FAIL [fdo#103166]

  * igt@kms_setmode@basic:
    - shard-kbl:          PASS -> FAIL [fdo#99912]

  * igt@pm_rpm@fences-dpms:
    - shard-iclb:         PASS -> INCOMPLETE [fdo#108840]

  * igt@pm_rpm@gem-idle:
    - shard-iclb:         NOTRUN -> DMESG-WARN [fdo#107724] +2

  * igt@pm_rpm@universal-planes:
    - shard-skl:          PASS -> INCOMPLETE [fdo#107807]

  * igt@pm_rpm@universal-planes-dpms:
    - shard-iclb:         PASS -> DMESG-WARN [fdo#108654]

  
#### Possible fixes ####

  * igt@kms_chv_cursor_fail@pipe-b-128x128-left-edge:
    - shard-iclb:         DMESG-WARN [fdo#107724] / [fdo#108336] -> PASS

  * igt@kms_color@pipe-c-ctm-max:
    - shard-apl:          FAIL [fdo#108147] -> PASS

  * igt@kms_cursor_crc@cursor-128x42-onscreen:
    - shard-apl:          FAIL [fdo#103232] -> PASS +3

  * igt@kms_cursor_crc@cursor-256x256-sliding:
    - shard-glk:          FAIL [fdo#103232] -> PASS +2

  * igt@kms_draw_crc@draw-method-rgb565-pwrite-untiled:
    - shard-skl:          FAIL [fdo#103184] -> PASS

  * igt@kms_draw_crc@draw-method-xrgb8888-blt-untiled:
    - shard-skl:          FAIL [fdo#103232] / [fdo#108472] -> PASS

  * igt@kms_flip@flip-vs-expired-vblank:
    - shard-kbl:          FAIL [fdo#102887] / [fdo#105363] -> PASS

  * igt@kms_flip@flip-vs-expired-vblank-interruptible:
    - shard-skl:          FAIL [fdo#105363] -> PASS

  * igt@kms_flip@flip-vs-panning:
    - shard-iclb:         DMESG-WARN [fdo#107724] -> PASS +7

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-wc:
    - shard-glk:          FAIL [fdo#103167] -> PASS +2

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-pwrite:
    - shard-apl:          FAIL [fdo#103167] -> PASS

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-move:
    - shard-iclb:         FAIL [fdo#103167] -> PASS +6

  * igt@kms_frontbuffer_tracking@fbc-badstride:
    - shard-iclb:         DMESG-FAIL [fdo#107724] -> PASS +2

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-shrfb-draw-pwrite:
    - shard-skl:          FAIL [fdo#103167] -> PASS

  * igt@kms_frontbuffer_tracking@fbcpsr-stridechange:
    - shard-iclb:         FAIL [fdo#105683] / [fdo#108040] -> PASS

  * igt@kms_plane_multiple@atomic-pipe-b-tiling-none:
    - shard-glk:          FAIL [fdo#103166] -> PASS +2
    - shard-apl:          FAIL [fdo#103166] -> PASS +2

  * igt@kms_plane_multiple@atomic-pipe-c-tiling-x:
    - shard-iclb:         FAIL [fdo#103166] -> PASS

  * igt@kms_setmode@basic:
    - shard-apl:          FAIL [fdo#99912] -> PASS

  * igt@pm_rpm@sysfs-read:
    - shard-apl:          DMESG-WARN [fdo#103558] / [fdo#105602] -> PASS +3

  * igt@pm_rpm@universal-planes:
    - shard-iclb:         DMESG-WARN [fdo#108654] -> PASS

  
#### Warnings ####

  * igt@kms_plane_alpha_blend@pipe-b-constant-alpha-max:
    - shard-apl:          DMESG-FAIL [fdo#103558] / [fdo#105602] / [fdo#108145] -> FAIL [fdo#108145]

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#102370]: https://bugs.freedesktop.org/show_bug.cgi?id=102370
  [fdo#102887]: https://bugs.freedesktop.org/show_bug.cgi?id=102887
  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103184]: https://bugs.freedesktop.org/show_bug.cgi?id=103184
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103359]: https://bugs.freedesktop.org/show_bug.cgi?id=103359
  [fdo#103558]: https://bugs.freedesktop.org/show_bug.cgi?id=103558
  [fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
  [fdo#104782]: https://bugs.freedesktop.org/show_bug.cgi?id=104782
  [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
  [fdo#105602]: https://bugs.freedesktop.org/show_bug.cgi?id=105602
  [fdo#105682]: https://bugs.freedesktop.org/show_bug.cgi?id=105682
  [fdo#105683]: https://bugs.freedesktop.org/show_bug.cgi?id=105683
  [fdo#106885]: https://bugs.freedesktop.org/show_bug.cgi?id=106885
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#107725]: https://bugs.freedesktop.org/show_bug.cgi?id=107725
  [fdo#107773]: https://bugs.freedesktop.org/show_bug.cgi?id=107773
  [fdo#107807]: https://bugs.freedesktop.org/show_bug.cgi?id=107807
  [fdo#107882]: https://bugs.freedesktop.org/show_bug.cgi?id=107882
  [fdo#107956]: https://bugs.freedesktop.org/show_bug.cgi?id=107956
  [fdo#108040]: https://bugs.freedesktop.org/show_bug.cgi?id=108040
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108147]: https://bugs.freedesktop.org/show_bug.cgi?id=108147
  [fdo#108336]: https://bugs.freedesktop.org/show_bug.cgi?id=108336
  [fdo#108472]: https://bugs.freedesktop.org/show_bug.cgi?id=108472
  [fdo#108654]: https://bugs.freedesktop.org/show_bug.cgi?id=108654
  [fdo#108840]: https://bugs.freedesktop.org/show_bug.cgi?id=108840
  [fdo#108954]: https://bugs.freedesktop.org/show_bug.cgi?id=108954
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109274]: https://bugs.freedesktop.org/show_bug.cgi?id=109274
  [fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109279]: https://bugs.freedesktop.org/show_bug.cgi?id=109279
  [fdo#109280]: https://bugs.freedesktop.org/show_bug.cgi?id=109280
  [fdo#109281]: https://bugs.freedesktop.org/show_bug.cgi?id=109281
  [fdo#109283]: https://bugs.freedesktop.org/show_bug.cgi?id=109283
  [fdo#109284]: https://bugs.freedesktop.org/show_bug.cgi?id=109284
  [fdo#109286]: https://bugs.freedesktop.org/show_bug.cgi?id=109286
  [fdo#109287]: https://bugs.freedesktop.org/show_bug.cgi?id=109287
  [fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
  [fdo#109290]: https://bugs.freedesktop.org/show_bug.cgi?id=109290
  [fdo#109291]: https://bugs.freedesktop.org/show_bug.cgi?id=109291
  [fdo#109294]: https://bugs.freedesktop.org/show_bug.cgi?id=109294
  [fdo#109295]: https://bugs.freedesktop.org/show_bug.cgi?id=109295
  [fdo#109300]: https://bugs.freedesktop.org/show_bug.cgi?id=109300
  [fdo#109312]: https://bugs.freedesktop.org/show_bug.cgi?id=109312
  [fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
  [fdo#109350]: https://bugs.freedesktop.org/show_bug.cgi?id=109350
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912
  [k.org#198133]: https://bugzilla.kernel.org/show_bug.cgi?id=198133


Participating hosts (7 -> 7)
------------------------------

  No changes in participating hosts


Build changes
-------------

    * Linux: CI_DRM_5439 -> Patchwork_11968

  CI_DRM_5439: 090b10b5d3ed38ed1033317fe7b902a87fdddf9f @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4777: 8614d5eb114a660c3bd7ff77eab8bed53424cd30 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_11968: bd6cdba3da21cee68f68dccaac5e75f8e115cdbd @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_11968/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915/psr: Add HBR3 support

[Manasi Navare]

On Wed, Jan 16, 2019 at 03:43:20PM -0800, José Roberto de Souza wrote:
> If the sink and source supports HBR3, TP4 should be used as link
> training pattern.
> For PSR2 there is no register to set and enable TP4 but according to
> eDP spec TP3 is still a training pattern acceptable for HBR3 panels.
>

The spec refers to training pattern 4 as TPS4, 3 as TPS3 etc so its better
to stick with the same abbreviations in this patch commit message and comments
as well since the spec uses TP2/TP3 etc for test point 2/3.
 
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
> 
> Still trying to understand how PSR1 was working on ICL while sending
> TP4 to a panel that only supports HBR2.
> 
>  drivers/gpu/drm/i915/i915_reg.h               |  1 +
>  drivers/gpu/drm/i915/intel_dp_link_training.c |  2 +-
>  drivers/gpu/drm/i915/intel_drv.h              |  1 +
>  drivers/gpu/drm/i915/intel_psr.c              | 24 ++++++++++++++-----
>  4 files changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 5faca634ee70..1e792309a79e 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4162,6 +4162,7 @@ enum {
>  #define   EDP_PSR_TP1_TP3_SEL			(1 << 11)
>  #define   EDP_PSR_CRC_ENABLE			(1 << 10) /* BDW+ */
>  #define   EDP_PSR_TP2_TP3_TIME_SHIFT		(8)
> +#define   EDP_PSR_TP4_TIME_SHIFT		(6) /* ICL+ */
>  #define   EDP_PSR_TP1_TIME_SHIFT		(4)
>  #define   EDP_PSR_IDLE_FRAME_SHIFT		0
>  
> diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
> index 30be0e39bd5f..3e9798a5498c 100644
> --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> @@ -238,7 +238,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
>   * or for 1.4 devices that support it, training Pattern 3 for HBR2
>   * or 1.2 devices that support it, Training Pattern 2 otherwise.
>   */
> -static u32 intel_dp_training_pattern(struct intel_dp *intel_dp)
> +u32 intel_dp_training_pattern(struct intel_dp *intel_dp)
>  {
>  	bool source_tps3, sink_tps3, source_tps4, sink_tps4;
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index e5a436c33307..fc3e6ae92276 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1807,6 +1807,7 @@ int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
>  					    int link_rate, uint8_t lane_count);
>  void intel_dp_start_link_train(struct intel_dp *intel_dp);
>  void intel_dp_stop_link_train(struct intel_dp *intel_dp);
> +u32 intel_dp_training_pattern(struct intel_dp *intel_dp);
>  int intel_dp_retrain_link(struct intel_encoder *encoder,
>  			  struct drm_modeset_acquire_ctx *ctx);
>  void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 2fc537fb6e78..b0525940e5e9 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -440,6 +440,7 @@ static void hsw_activate_psr1(struct intel_dp *intel_dp)
>  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
>  	u32 max_sleep_time = 0x1f;
>  	u32 val = EDP_PSR_ENABLE;
> +	u32 tp;

I would prefer calling this tps like the intel_dp_training_pattern function
calls it source_tps sink_tps.

Apart from that looks good. So with these changes

Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>

Manasi
 
>  
>  	/* Let's use 6 as the minimum to cover all known cases including the
>  	 * off-by-one issue that HW has in some cases.
> @@ -460,13 +461,24 @@ static void hsw_activate_psr1(struct intel_dp *intel_dp)
>  		val |= EDP_PSR_LINK_STANDBY;
>  
>  	val |= dev_priv->vbt.psr.tp1_wakeup_time << EDP_PSR_TP1_TIME_SHIFT;
> -	val |= dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time << EDP_PSR_TP2_TP3_TIME_SHIFT;
>  
> -	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;
> +	tp = intel_dp_training_pattern(intel_dp);
> +	if (tp == DP_TRAINING_PATTERN_4) {
> +		/*
> +		 * TP4 is selected by setting EDP_PSR_TP4_TIME with other value
> +		 * than PSR_TP_WAKEUP_TIME_NONE
> +		 */
> +		val |= dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time << EDP_PSR_TP4_TIME_SHIFT;
> +	} else {
> +		if (INTEL_GEN(dev_priv) >= 11)
> +			val |= PSR_TP_WAKEUP_TIME_NONE << EDP_PSR_TP4_TIME_SHIFT;
> +
> +		val |= dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time << EDP_PSR_TP2_TP3_TIME_SHIFT;
> +		if (tp == DP_TRAINING_PATTERN_3)
> +			val |= EDP_PSR_TP1_TP3_SEL;
> +		else
> +			val |= EDP_PSR_TP1_TP2_SEL;
> +	}
>  
>  	if (INTEL_GEN(dev_priv) >= 8)
>  		val |= EDP_PSR_CRC_ENABLE;
> -- 
> 2.20.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/i915/vbt: Add 'tp4' to varibles holding TP2/3/4 PSR wakeup time

[Jani Nikula]

On Wed, 16 Jan 2019, José Roberto de Souza <jose.souza@intel.com> wrote:
> Recent update in spec made the field holding the TP2 and TP3 wakeup
> time for PSR also hold the TP4, so lets rename the variables to
> reflect that.
>
> BSpec: 20131
>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h       |  2 +-
>  drivers/gpu/drm/i915/intel_bios.c     | 16 ++++++++--------
>  drivers/gpu/drm/i915/intel_psr.c      | 14 +++++++-------
>  drivers/gpu/drm/i915/intel_vbt_defs.h |  2 +-
>  4 files changed, 17 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 310d9e1e1620..e717c3132692 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -996,7 +996,7 @@ struct intel_vbt_data {
>  		int idle_frames;
>  		enum psr_lines_to_wait lines_to_wait;
>  		int tp1_wakeup_time_us;
> -		int tp2_tp3_wakeup_time_us;
> +		int tp2_tp3_tp4_wakeup_time_us;

I think this is too long, perhaps just tp234_wakeup_time_us or
something?

BR,
Jani.



>  	} psr;
>  
>  	struct {
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index 561a4f9f044c..cd99bf88bf6c 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -738,27 +738,27 @@ parse_psr(struct drm_i915_private *dev_priv, const struct bdb_header *bdb)
>  			break;
>  		}
>  
> -		switch (psr_table->tp2_tp3_wakeup_time) {
> +		switch (psr_table->tp2_tp3_tp4_wakeup_time) {
>  		case 0:
> -			dev_priv->vbt.psr.tp2_tp3_wakeup_time_us = 500;
> +			dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us = 500;
>  			break;
>  		case 1:
> -			dev_priv->vbt.psr.tp2_tp3_wakeup_time_us = 100;
> +			dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us = 100;
>  			break;
>  		case 3:
> -			dev_priv->vbt.psr.tp2_tp3_wakeup_time_us = 0;
> +			dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us = 0;
>  			break;
>  		default:
> -			DRM_DEBUG_KMS("VBT tp2_tp3 wakeup time value %d is outside range[0-3], defaulting to max value 2500us\n",
> -					psr_table->tp2_tp3_wakeup_time);
> +			DRM_DEBUG_KMS("VBT tp2_tp3_tp4 wakeup time value %d is outside range[0-3], defaulting to max value 2500us\n",
> +				      psr_table->tp2_tp3_tp4_wakeup_time);
>  			/* fallthrough */
>  		case 2:
> -			dev_priv->vbt.psr.tp2_tp3_wakeup_time_us = 2500;
> +			dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us = 2500;
>  		break;
>  		}
>  	} else {
>  		dev_priv->vbt.psr.tp1_wakeup_time_us = psr_table->tp1_wakeup_time * 100;
> -		dev_priv->vbt.psr.tp2_tp3_wakeup_time_us = psr_table->tp2_tp3_wakeup_time * 100;
> +		dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us = psr_table->tp2_tp3_tp4_wakeup_time * 100;
>  	}
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 0f6b2b4702e3..49b4b3371bef 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -468,11 +468,11 @@ static void hsw_activate_psr1(struct intel_dp *intel_dp)
>  	else
>  		val |= EDP_PSR_TP1_TIME_2500us;
>  
> -	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us == 0)
> +	if (dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us == 0)
>  		val |=  EDP_PSR_TP2_TP3_TIME_0us;
> -	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 100)
> +	else if (dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us <= 100)
>  		val |= EDP_PSR_TP2_TP3_TIME_100us;
> -	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 500)
> +	else if (dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us <= 500)
>  		val |= EDP_PSR_TP2_TP3_TIME_500us;
>  	else
>  		val |= EDP_PSR_TP2_TP3_TIME_2500us;
> @@ -509,12 +509,12 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
>  
>  	val |= EDP_PSR2_FRAME_BEFORE_SU(dev_priv->psr.sink_sync_latency + 1);
>  
> -	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us >= 0 &&
> -	    dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 50)
> +	if (dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us >= 0 &&
> +	    dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us <= 50)
>  		val |= EDP_PSR2_TP2_TIME_50us;
> -	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 100)
> +	else if (dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us <= 100)
>  		val |= EDP_PSR2_TP2_TIME_100us;
> -	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 500)
> +	else if (dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us <= 500)
>  		val |= EDP_PSR2_TP2_TIME_500us;
>  	else
>  		val |= EDP_PSR2_TP2_TIME_2500us;
> diff --git a/drivers/gpu/drm/i915/intel_vbt_defs.h b/drivers/gpu/drm/i915/intel_vbt_defs.h
> index bf3662ad5fed..4ed66efde49f 100644
> --- a/drivers/gpu/drm/i915/intel_vbt_defs.h
> +++ b/drivers/gpu/drm/i915/intel_vbt_defs.h
> @@ -771,7 +771,7 @@ struct psr_table {
>  
>  	/* TP wake up time in multiple of 100 */
>  	u16 tp1_wakeup_time;
> -	u16 tp2_tp3_wakeup_time;
> +	u16 tp2_tp3_tp4_wakeup_time;
>  } __packed;
>  
>  struct bdb_psr {

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] drm/i915/psr: Store VBT TP wakeup times into a enum

[Jani Nikula]

On Wed, 16 Jan 2019, José Roberto de Souza <jose.souza@intel.com> wrote:
> Newer VBTs and the PSR registers uses a enum to set the TPs wakeup
> time, so lets use this format to store wakeup times and avoid
> conversions every time that PSR is activated.

The VBT is a messy blob of data, and intel_bios.c in many places tries
very hard to abstract and hide all the quirks in there from the
driver. Observe how intel_vbt_defs.h has extra safeguards to try to keep
it private to intel_bios.c. We don't want the VBT implementation details
to leak into the driver.

This patch goes against that abstraction. Please don't do it.

> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h   | 12 +++++-
>  drivers/gpu/drm/i915/i915_reg.h   | 16 ++------
>  drivers/gpu/drm/i915/intel_bios.c | 65 +++++++++++++++----------------
>  drivers/gpu/drm/i915/intel_psr.c  | 29 ++------------
>  4 files changed, 47 insertions(+), 75 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e717c3132692..d9893d35f0e2 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -959,6 +959,14 @@ enum psr_lines_to_wait {
>  	PSR_8_LINES_TO_WAIT
>  };
>  
> +enum psr_tp_wakeup_time {
> +	PSR_TP_WAKEUP_TIME_500USEC = 0,
> +	PSR_TP_WAKEUP_TIME_100USEC,
> +	PSR_TP_WAKEUP_TIME_2500USEC,
> +	PSR_TP_WAKEUP_TIME_NONE,
> +	PSR_TP_WAKEUP_TIME_LAST
> +};

Here's the problem. This enum now describes both the VBT internals as
well as the hardware register contents. Imagine either the VBT or the
hardware changing to a different bit layout. You'll be in trouble
decoupling them again. (Yeah, they'll say neither will change. They've
said things like that before.)

Moreover, this looks like an innocent enumeration here, separated from
both the VBT parsing and the hardware.

The VBT needs to be described in intel_vbt_defs.h and intel_bios.c, and
the register needs to be desribed in i915_regs.h.

> +
>  struct intel_vbt_data {
>  	struct drm_display_mode *lfp_lvds_vbt_mode; /* if any */
>  	struct drm_display_mode *sdvo_lvds_vbt_mode; /* if any */
> @@ -995,8 +1003,8 @@ struct intel_vbt_data {
>  		bool require_aux_wakeup;
>  		int idle_frames;
>  		enum psr_lines_to_wait lines_to_wait;
> -		int tp1_wakeup_time_us;
> -		int tp2_tp3_tp4_wakeup_time_us;
> +		enum psr_tp_wakeup_time tp1_wakeup_time;
> +		enum psr_tp_wakeup_time tp2_tp3_tp4_wakeup_time;
>  	} psr;
>  
>  	struct {
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index fad5a9e8b44d..5faca634ee70 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4161,14 +4161,8 @@ enum {
>  #define   EDP_PSR_TP1_TP2_SEL			(0 << 11)
>  #define   EDP_PSR_TP1_TP3_SEL			(1 << 11)
>  #define   EDP_PSR_CRC_ENABLE			(1 << 10) /* BDW+ */
> -#define   EDP_PSR_TP2_TP3_TIME_500us		(0 << 8)
> -#define   EDP_PSR_TP2_TP3_TIME_100us		(1 << 8)
> -#define   EDP_PSR_TP2_TP3_TIME_2500us		(2 << 8)
> -#define   EDP_PSR_TP2_TP3_TIME_0us		(3 << 8)
> -#define   EDP_PSR_TP1_TIME_500us		(0 << 4)
> -#define   EDP_PSR_TP1_TIME_100us		(1 << 4)
> -#define   EDP_PSR_TP1_TIME_2500us		(2 << 4)
> -#define   EDP_PSR_TP1_TIME_0us			(3 << 4)
> +#define   EDP_PSR_TP2_TP3_TIME_SHIFT		(8)
> +#define   EDP_PSR_TP1_TIME_SHIFT		(4)

Please do not remove these. They describe the register contents as we
understand them, and not everyone has access or cares to dig up bspec
every time they need to figure things out.

Side note, parens are superfluous for plain values.

>  #define   EDP_PSR_IDLE_FRAME_SHIFT		0
>  
>  /* Bspec claims those aren't shifted but stay at 0x64800 */
> @@ -4234,11 +4228,7 @@ enum {
>  #define   EDP_Y_COORDINATE_ENABLE	(1 << 25) /* GLK and CNL+ */
>  #define   EDP_MAX_SU_DISABLE_TIME(t)	((t) << 20)
>  #define   EDP_MAX_SU_DISABLE_TIME_MASK	(0x1f << 20)
> -#define   EDP_PSR2_TP2_TIME_500us	(0 << 8)
> -#define   EDP_PSR2_TP2_TIME_100us	(1 << 8)
> -#define   EDP_PSR2_TP2_TIME_2500us	(2 << 8)
> -#define   EDP_PSR2_TP2_TIME_50us	(3 << 8)
> -#define   EDP_PSR2_TP2_TIME_MASK	(3 << 8)
> +#define   EDP_PSR2_TP2_TP3_TIME_SHIFT	(8)

Ditto.

>  #define   EDP_PSR2_FRAME_BEFORE_SU_SHIFT 4
>  #define   EDP_PSR2_FRAME_BEFORE_SU_MASK	(0xf << 4)
>  #define   EDP_PSR2_FRAME_BEFORE_SU(a)	((a) << 4)
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index cd99bf88bf6c..6de6f6f1deec 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -719,46 +719,43 @@ parse_psr(struct drm_i915_private *dev_priv, const struct bdb_header *bdb)
>  	if (bdb->version >= 205 &&
>  	    (IS_GEN9_BC(dev_priv) || IS_GEMINILAKE(dev_priv) ||
>  	     INTEL_GEN(dev_priv) >= 10)) {
> -		switch (psr_table->tp1_wakeup_time) {
> -		case 0:
> -			dev_priv->vbt.psr.tp1_wakeup_time_us = 500;
> -			break;
> -		case 1:
> -			dev_priv->vbt.psr.tp1_wakeup_time_us = 100;
> -			break;
> -		case 3:
> -			dev_priv->vbt.psr.tp1_wakeup_time_us = 0;
> -			break;
> -		default:
> +		dev_priv->vbt.psr.tp1_wakeup_time = psr_table->tp1_wakeup_time;
> +		if (dev_priv->vbt.psr.tp1_wakeup_time >= PSR_TP_WAKEUP_TIME_LAST) {
>  			DRM_DEBUG_KMS("VBT tp1 wakeup time value %d is outside range[0-3], defaulting to max value 2500us\n",
> -					psr_table->tp1_wakeup_time);
> -			/* fallthrough */
> -		case 2:
> -			dev_priv->vbt.psr.tp1_wakeup_time_us = 2500;
> -			break;
> +				      dev_priv->vbt.psr.tp1_wakeup_time);
> +			dev_priv->vbt.psr.tp1_wakeup_time = PSR_TP_WAKEUP_TIME_2500USEC;
>  		}
>  
> -		switch (psr_table->tp2_tp3_tp4_wakeup_time) {
> -		case 0:
> -			dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us = 500;
> -			break;
> -		case 1:
> -			dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us = 100;
> -			break;
> -		case 3:
> -			dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us = 0;
> -			break;
> -		default:
> +		dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time = psr_table->tp2_tp3_tp4_wakeup_time;
> +		if (dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time >= PSR_TP_WAKEUP_TIME_LAST) {
>  			DRM_DEBUG_KMS("VBT tp2_tp3_tp4 wakeup time value %d is outside range[0-3], defaulting to max value 2500us\n",
> -				      psr_table->tp2_tp3_tp4_wakeup_time);
> -			/* fallthrough */
> -		case 2:
> -			dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us = 2500;
> -		break;
> +				      dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time);
> +			dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time = PSR_TP_WAKEUP_TIME_2500USEC;
>  		}
>  	} else {
> -		dev_priv->vbt.psr.tp1_wakeup_time_us = psr_table->tp1_wakeup_time * 100;
> -		dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us = psr_table->tp2_tp3_tp4_wakeup_time * 100;
> +		enum psr_tp_wakeup_time wakeup_time;
> +
> +		if (psr_table->tp1_wakeup_time == 0)
> +			wakeup_time = PSR_TP_WAKEUP_TIME_NONE;
> +		else if (psr_table->tp1_wakeup_time == 1)
> +			wakeup_time = PSR_TP_WAKEUP_TIME_100USEC;
> +		else if (psr_table->tp1_wakeup_time <= 5)
> +			wakeup_time = PSR_TP_WAKEUP_TIME_500USEC;
> +		else
> +			wakeup_time = PSR_TP_WAKEUP_TIME_2500USEC;
> +
> +		dev_priv->vbt.psr.tp1_wakeup_time = wakeup_time;
> +
> +		if (psr_table->tp2_tp3_tp4_wakeup_time == 0)
> +			wakeup_time = PSR_TP_WAKEUP_TIME_NONE;
> +		else if (psr_table->tp2_tp3_tp4_wakeup_time == 1)
> +			wakeup_time = PSR_TP_WAKEUP_TIME_100USEC;
> +		else if (psr_table->tp2_tp3_tp4_wakeup_time <= 5)
> +			wakeup_time = PSR_TP_WAKEUP_TIME_500USEC;
> +		else
> +			wakeup_time = PSR_TP_WAKEUP_TIME_2500USEC;
> +
> +		dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time = wakeup_time;
>  	}
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 49b4b3371bef..5daf0b9e2b42 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -459,23 +459,8 @@ static void hsw_activate_psr1(struct intel_dp *intel_dp)
>  	if (dev_priv->psr.link_standby)
>  		val |= EDP_PSR_LINK_STANDBY;
>  
> -	if (dev_priv->vbt.psr.tp1_wakeup_time_us == 0)
> -		val |=  EDP_PSR_TP1_TIME_0us;
> -	else if (dev_priv->vbt.psr.tp1_wakeup_time_us <= 100)
> -		val |= EDP_PSR_TP1_TIME_100us;
> -	else if (dev_priv->vbt.psr.tp1_wakeup_time_us <= 500)
> -		val |= EDP_PSR_TP1_TIME_500us;
> -	else
> -		val |= EDP_PSR_TP1_TIME_2500us;
> -
> -	if (dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us == 0)
> -		val |=  EDP_PSR_TP2_TP3_TIME_0us;
> -	else if (dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us <= 100)
> -		val |= EDP_PSR_TP2_TP3_TIME_100us;
> -	else if (dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us <= 500)
> -		val |= EDP_PSR_TP2_TP3_TIME_500us;
> -	else
> -		val |= EDP_PSR_TP2_TP3_TIME_2500us;
> +	val |= dev_priv->vbt.psr.tp1_wakeup_time << EDP_PSR_TP1_TIME_SHIFT;
> +	val |= dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time << EDP_PSR_TP2_TP3_TIME_SHIFT;
>  
>  	if (intel_dp_source_supports_hbr2(intel_dp) &&
>  	    drm_dp_tps3_supported(intel_dp->dpcd))
> @@ -509,15 +494,7 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
>  
>  	val |= EDP_PSR2_FRAME_BEFORE_SU(dev_priv->psr.sink_sync_latency + 1);
>  
> -	if (dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us >= 0 &&
> -	    dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us <= 50)
> -		val |= EDP_PSR2_TP2_TIME_50us;
> -	else if (dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us <= 100)
> -		val |= EDP_PSR2_TP2_TIME_100us;
> -	else if (dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time_us <= 500)
> -		val |= EDP_PSR2_TP2_TIME_500us;
> -	else
> -		val |= EDP_PSR2_TP2_TIME_2500us;
> +	val |= dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time << EDP_PSR2_TP2_TP3_TIME_SHIFT;

IMO usecs is the perfect abstraction between VBT and the driver for
describing timeouts. I'd prefer us not to go towards just blindly
writing hardware registers from the VBT.

BR,
Jani.


>  
>  	I915_WRITE(EDP_PSR2_CTL, val);
>  }

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915/vbt: Parse and use the new field with PSR2 TP2/3/4 wakeup time

[Jani Nikula]

On Wed, 16 Jan 2019, José Roberto de Souza <jose.souza@intel.com> wrote:
> A new field with the training pattern(TP) wakeup time for PSR2 was
> added to VBT, so lets use it when available otherwise it will
> fallback to PSR1 wakeup time.

Same problems as with the two previous patches:

- The new field name is too long.

- The abstraction between VBT and hardware is lost.

BR,
Jani.


>
> BSpec: 20131
>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h       |  8 ++++++++
>  drivers/gpu/drm/i915/intel_bios.c     | 10 ++++++++++
>  drivers/gpu/drm/i915/intel_psr.c      |  2 +-
>  drivers/gpu/drm/i915/intel_vbt_defs.h |  3 +++
>  4 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index d9893d35f0e2..e739ed9ce60c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -967,6 +967,13 @@ enum psr_tp_wakeup_time {
>  	PSR_TP_WAKEUP_TIME_LAST
>  };
>  
> +enum psr2_tp_wakeup_time {
> +	PSR2_TP_WAKEUP_TIME_500USEC = 0,
> +	PSR2_TP_WAKEUP_TIME_100USEC,
> +	PSR2_TP_WAKEUP_TIME_2500USEC,
> +	PSR2_TP_WAKEUP_TIME_50USEC
> +};
> +
>  struct intel_vbt_data {
>  	struct drm_display_mode *lfp_lvds_vbt_mode; /* if any */
>  	struct drm_display_mode *sdvo_lvds_vbt_mode; /* if any */
> @@ -1005,6 +1012,7 @@ struct intel_vbt_data {
>  		enum psr_lines_to_wait lines_to_wait;
>  		enum psr_tp_wakeup_time tp1_wakeup_time;
>  		enum psr_tp_wakeup_time tp2_tp3_tp4_wakeup_time;
> +		enum psr2_tp_wakeup_time psr2_tp2_tp3_tp4_wakeup_time;
>  	} psr;
>  
>  	struct {
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index 6de6f6f1deec..23130e0d5e6c 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -757,6 +757,16 @@ parse_psr(struct drm_i915_private *dev_priv, const struct bdb_header *bdb)
>  
>  		dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time = wakeup_time;
>  	}
> +
> +	if (bdb->version >= 226) {
> +		u32 wakeup_time = psr_table->psr2_tp2_tp3_tp4_wakeup_time;
> +
> +		wakeup_time = (wakeup_time >> (2 * panel_type)) & 0x3;
> +		dev_priv->vbt.psr.psr2_tp2_tp3_tp4_wakeup_time = wakeup_time;
> +	} else {
> +		/* Reusing PSR1 wakeup time for PSR2 in older VBTs */
> +		dev_priv->vbt.psr.psr2_tp2_tp3_tp4_wakeup_time = dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time;
> +	}
>  }
>  
>  static void parse_dsi_backlight_ports(struct drm_i915_private *dev_priv,
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 5daf0b9e2b42..2fc537fb6e78 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -494,7 +494,7 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
>  
>  	val |= EDP_PSR2_FRAME_BEFORE_SU(dev_priv->psr.sink_sync_latency + 1);
>  
> -	val |= dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time << EDP_PSR2_TP2_TP3_TIME_SHIFT;
> +	val |= dev_priv->vbt.psr.psr2_tp2_tp3_tp4_wakeup_time << EDP_PSR2_TP2_TP3_TIME_SHIFT;
>  
>  	I915_WRITE(EDP_PSR2_CTL, val);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_vbt_defs.h b/drivers/gpu/drm/i915/intel_vbt_defs.h
> index 4ed66efde49f..dc0a14977953 100644
> --- a/drivers/gpu/drm/i915/intel_vbt_defs.h
> +++ b/drivers/gpu/drm/i915/intel_vbt_defs.h
> @@ -772,6 +772,9 @@ struct psr_table {
>  	/* TP wake up time in multiple of 100 */
>  	u16 tp1_wakeup_time;
>  	u16 tp2_tp3_tp4_wakeup_time;
> +
> +	/* PSR2 TP2/TP3/TP4 wakeup time for 16 panels */
> +	u32 psr2_tp2_tp3_tp4_wakeup_time;
>  } __packed;
>  
>  struct bdb_psr {

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915/psr: Add HBR3 support

[Dhinakaran Pandiyan]

On Wed, 2019-01-16 at 15:43 -0800, José Roberto de Souza wrote:
> If the sink and source supports HBR3, TP4 should be used as link
> training pattern.
> For PSR2 there is no register to set and enable TP4 but according to
> eDP spec TP3 is still a training pattern acceptable for HBR3 panels.
> 
Sounds like TP3 and TP4 are used only with PSR1, please document that
in the commit message. 
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
> 
> Still trying to understand how PSR1 was working on ICL while sending
> TP4 to a panel that only supports HBR2.

That's a good point, along with that please find out what Bit 11: "TPS4
Control" does. I'd like us get these questions answered, if possible,
before merging this series.

> 
>  drivers/gpu/drm/i915/i915_reg.h               |  1 +
>  drivers/gpu/drm/i915/intel_dp_link_training.c |  2 +-
>  drivers/gpu/drm/i915/intel_drv.h              |  1 +
>  drivers/gpu/drm/i915/intel_psr.c              | 24 ++++++++++++++---
> --
>  4 files changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> index 5faca634ee70..1e792309a79e 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4162,6 +4162,7 @@ enum {
>  #define   EDP_PSR_TP1_TP3_SEL			(1 << 11)
>  #define   EDP_PSR_CRC_ENABLE			(1 << 10) /* BDW+ */
>  #define   EDP_PSR_TP2_TP3_TIME_SHIFT		(8)
> +#define   EDP_PSR_TP4_TIME_SHIFT		(6) /* ICL+ */
>  #define   EDP_PSR_TP1_TIME_SHIFT		(4)
>  #define   EDP_PSR_IDLE_FRAME_SHIFT		0
>  
> diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c
> b/drivers/gpu/drm/i915/intel_dp_link_training.c
> index 30be0e39bd5f..3e9798a5498c 100644
> --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> @@ -238,7 +238,7 @@ intel_dp_link_training_clock_recovery(struct
> intel_dp *intel_dp)
>   * or for 1.4 devices that support it, training Pattern 3 for HBR2
>   * or 1.2 devices that support it, Training Pattern 2 otherwise.
>   */
> -static u32 intel_dp_training_pattern(struct intel_dp *intel_dp)
> +u32 intel_dp_training_pattern(struct intel_dp *intel_dp)
>  {
>  	bool source_tps3, sink_tps3, source_tps4, sink_tps4;
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index e5a436c33307..fc3e6ae92276 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1807,6 +1807,7 @@ int
> intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
>  					    int link_rate, uint8_t
> lane_count);
>  void intel_dp_start_link_train(struct intel_dp *intel_dp);
>  void intel_dp_stop_link_train(struct intel_dp *intel_dp);
> +u32 intel_dp_training_pattern(struct intel_dp *intel_dp);
>  int intel_dp_retrain_link(struct intel_encoder *encoder,
>  			  struct drm_modeset_acquire_ctx *ctx);
>  void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index 2fc537fb6e78..b0525940e5e9 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -440,6 +440,7 @@ static void hsw_activate_psr1(struct intel_dp
> *intel_dp)
>  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
>  	u32 max_sleep_time = 0x1f;
>  	u32 val = EDP_PSR_ENABLE;
> +	u32 tp;
>  
>  	/* Let's use 6 as the minimum to cover all known cases
> including the
>  	 * off-by-one issue that HW has in some cases.
> @@ -460,13 +461,24 @@ static void hsw_activate_psr1(struct intel_dp
> *intel_dp)
>  		val |= EDP_PSR_LINK_STANDBY;
>  
>  	val |= dev_priv->vbt.psr.tp1_wakeup_time <<
> EDP_PSR_TP1_TIME_SHIFT;
> -	val |= dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time <<
> EDP_PSR_TP2_TP3_TIME_SHIFT;
>  
> -	if (intel_dp_source_supports_hbr2(intel_dp) &&
Now that you are removing a caller, make this function static and move
to intel_dp_link_tranining.c instead ? Or just inline the code there.

> -	    drm_dp_tps3_supported(intel_dp->dpcd))
> -		val |= EDP_PSR_TP1_TP3_SEL;
> -	else
> -		val |= EDP_PSR_TP1_TP2_SEL;
> +	tp = intel_dp_training_pattern(intel_dp);
> +	if (tp == DP_TRAINING_PATTERN_4) {
> +		/*
> +		 * TP4 is selected by setting EDP_PSR_TP4_TIME with
> other value
> +		 * than PSR_TP_WAKEUP_TIME_NONE
> +		 */
IMHO I think we should skip this comment, we'd have to write
documentation for every other register if we are going to do this :)

> +		val |= dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time <<
> EDP_PSR_TP4_TIME_SHIFT;

So, the EDP_PSR_TP1_TP3_SEL bit has no effect when the TP4 duration is
non-zero, seems like an indirect way of switching to TP4 compared to
TP2 and TP3.

> +	} else {
> +		if (INTEL_GEN(dev_priv) >= 11)
> +			val |= PSR_TP_WAKEUP_TIME_NONE <<
> EDP_PSR_TP4_TIME_SHIFT;
> +
> +		val |= dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time <<
> EDP_PSR_TP2_TP3_TIME_SHIFT;
> +		if (tp == DP_TRAINING_PATTERN_3)
> +			val |= EDP_PSR_TP1_TP3_SEL;
> +		else
> +			val |= EDP_PSR_TP1_TP2_SEL;
> +	}
>  
>  	if (INTEL_GEN(dev_priv) >= 8)
>  		val |= EDP_PSR_CRC_ENABLE;

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

Re: [PATCH 3/4] drm/i915/vbt: Parse and use the new field with PSR2 TP2/3/4 wakeup time

[Dhinakaran Pandiyan]

On Wed, 2019-01-16 at 15:43 -0800, José Roberto de Souza wrote:
> A new field with the training pattern(TP) wakeup time for PSR2 was
These values are for PSR1, aren't they? Like you write in Patch 4/4,
the PSR2 control register does not have a bit to set anything other
than Tp2.

-DK


> added to VBT, so lets use it when available otherwise it will
> fallback to PSR1 wakeup time.
> 
> BSpec: 20131
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h       |  8 ++++++++
>  drivers/gpu/drm/i915/intel_bios.c     | 10 ++++++++++
>  drivers/gpu/drm/i915/intel_psr.c      |  2 +-
>  drivers/gpu/drm/i915/intel_vbt_defs.h |  3 +++
>  4 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index d9893d35f0e2..e739ed9ce60c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -967,6 +967,13 @@ enum psr_tp_wakeup_time {
>  	PSR_TP_WAKEUP_TIME_LAST
>  };
>  
> +enum psr2_tp_wakeup_time {
> +	PSR2_TP_WAKEUP_TIME_500USEC = 0,
> +	PSR2_TP_WAKEUP_TIME_100USEC,
> +	PSR2_TP_WAKEUP_TIME_2500USEC,
> +	PSR2_TP_WAKEUP_TIME_50USEC
> +};
> +
>  struct intel_vbt_data {
>  	struct drm_display_mode *lfp_lvds_vbt_mode; /* if any */
>  	struct drm_display_mode *sdvo_lvds_vbt_mode; /* if any */
> @@ -1005,6 +1012,7 @@ struct intel_vbt_data {
>  		enum psr_lines_to_wait lines_to_wait;
>  		enum psr_tp_wakeup_time tp1_wakeup_time;
>  		enum psr_tp_wakeup_time tp2_tp3_tp4_wakeup_time;
> +		enum psr2_tp_wakeup_time psr2_tp2_tp3_tp4_wakeup_time;
>  	} psr;
>  
>  	struct {
> diff --git a/drivers/gpu/drm/i915/intel_bios.c
> b/drivers/gpu/drm/i915/intel_bios.c
> index 6de6f6f1deec..23130e0d5e6c 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -757,6 +757,16 @@ parse_psr(struct drm_i915_private *dev_priv,
> const struct bdb_header *bdb)
>  
>  		dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time =
> wakeup_time;
>  	}
> +
> +	if (bdb->version >= 226) {
> +		u32 wakeup_time = psr_table-
> >psr2_tp2_tp3_tp4_wakeup_time;
> +
> +		wakeup_time = (wakeup_time >> (2 * panel_type)) & 0x3;
> +		dev_priv->vbt.psr.psr2_tp2_tp3_tp4_wakeup_time =
> wakeup_time;
> +	} else {
> +		/* Reusing PSR1 wakeup time for PSR2 in older VBTs */
> +		dev_priv->vbt.psr.psr2_tp2_tp3_tp4_wakeup_time =
> dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time;
> +	}
>  }
>  
>  static void parse_dsi_backlight_ports(struct drm_i915_private
> *dev_priv,
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index 5daf0b9e2b42..2fc537fb6e78 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -494,7 +494,7 @@ static void hsw_activate_psr2(struct intel_dp
> *intel_dp)
>  
>  	val |= EDP_PSR2_FRAME_BEFORE_SU(dev_priv->psr.sink_sync_latency 
> + 1);
>  
> -	val |= dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time <<
> EDP_PSR2_TP2_TP3_TIME_SHIFT;
> +	val |= dev_priv->vbt.psr.psr2_tp2_tp3_tp4_wakeup_time <<
> EDP_PSR2_TP2_TP3_TIME_SHIFT;
>  
>  	I915_WRITE(EDP_PSR2_CTL, val);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_vbt_defs.h
> b/drivers/gpu/drm/i915/intel_vbt_defs.h
> index 4ed66efde49f..dc0a14977953 100644
> --- a/drivers/gpu/drm/i915/intel_vbt_defs.h
> +++ b/drivers/gpu/drm/i915/intel_vbt_defs.h
> @@ -772,6 +772,9 @@ struct psr_table {
>  	/* TP wake up time in multiple of 100 */
>  	u16 tp1_wakeup_time;
>  	u16 tp2_tp3_tp4_wakeup_time;
> +
> +	/* PSR2 TP2/TP3/TP4 wakeup time for 16 panels */
This needs to fixed as well.

> +	u32 psr2_tp2_tp3_tp4_wakeup_time;
>  } __packed;
>  
>  struct bdb_psr {

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

Re: [PATCH 4/4] drm/i915/psr: Add HBR3 support

[Souza, Jose]

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

On Tue, 2019-01-22 at 14:42 -0800, Dhinakaran Pandiyan wrote:
> On Wed, 2019-01-16 at 15:43 -0800, José Roberto de Souza wrote:
> > If the sink and source supports HBR3, TP4 should be used as link
> > training pattern.
> > For PSR2 there is no register to set and enable TP4 but according
> > to
> > eDP spec TP3 is still a training pattern acceptable for HBR3
> > panels.
> > 
> Sounds like TP3 and TP4 are used only with PSR1, please document that
> in the commit message. 


The line above is not enough?

> > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> > 
> > Still trying to understand how PSR1 was working on ICL while
> > sending
> > TP4 to a panel that only supports HBR2.
> 
> That's a good point, along with that please find out what Bit 11:
> "TPS4
> Control" does. I'd like us get these questions answered, if possible,
> before merging this series.

So according to eDP spec, DPCD 00071h, bit 0 - Link Training
Requirement of Sink on PSR Exit when Main-Link is OFF documentation:

"New to eDP v1.4, PSR2 Only: This bit is “Don’t Care” for devices
that support PSR2 because Fast Sleep/Wake support is required
for those devices."

So for sinks thats supports alpm is not necessary send the training
patterns, so it was sending TPS4 but sync was just ignoring it that is
why it was working on ICL.

But I guess is better for us to keep sending training patterns for now,
after a couple of kernel releases we could think in remove it.

I'm still looking for a panel without alpm to test if the current code
would break PSR on ICL.

> 
> >  drivers/gpu/drm/i915/i915_reg.h               |  1 +
> >  drivers/gpu/drm/i915/intel_dp_link_training.c |  2 +-
> >  drivers/gpu/drm/i915/intel_drv.h              |  1 +
> >  drivers/gpu/drm/i915/intel_psr.c              | 24 ++++++++++++++-
> > --
> > --
> >  4 files changed, 21 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 5faca634ee70..1e792309a79e 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -4162,6 +4162,7 @@ enum {
> >  #define   EDP_PSR_TP1_TP3_SEL			(1 << 11)
> >  #define   EDP_PSR_CRC_ENABLE			(1 << 10) /*
> > BDW+ */
> >  #define   EDP_PSR_TP2_TP3_TIME_SHIFT		(8)
> > +#define   EDP_PSR_TP4_TIME_SHIFT		(6) /* ICL+ */
> >  #define   EDP_PSR_TP1_TIME_SHIFT		(4)
> >  #define   EDP_PSR_IDLE_FRAME_SHIFT		0
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c
> > b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > index 30be0e39bd5f..3e9798a5498c 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > @@ -238,7 +238,7 @@ intel_dp_link_training_clock_recovery(struct
> > intel_dp *intel_dp)
> >   * or for 1.4 devices that support it, training Pattern 3 for HBR2
> >   * or 1.2 devices that support it, Training Pattern 2 otherwise.
> >   */
> > -static u32 intel_dp_training_pattern(struct intel_dp *intel_dp)
> > +u32 intel_dp_training_pattern(struct intel_dp *intel_dp)
> >  {
> >  	bool source_tps3, sink_tps3, source_tps4, sink_tps4;
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index e5a436c33307..fc3e6ae92276 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1807,6 +1807,7 @@ int
> > intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
> >  					    int link_rate, uint8_t
> > lane_count);
> >  void intel_dp_start_link_train(struct intel_dp *intel_dp);
> >  void intel_dp_stop_link_train(struct intel_dp *intel_dp);
> > +u32 intel_dp_training_pattern(struct intel_dp *intel_dp);
> >  int intel_dp_retrain_link(struct intel_encoder *encoder,
> >  			  struct drm_modeset_acquire_ctx *ctx);
> >  void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 2fc537fb6e78..b0525940e5e9 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -440,6 +440,7 @@ static void hsw_activate_psr1(struct intel_dp
> > *intel_dp)
> >  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> >  	u32 max_sleep_time = 0x1f;
> >  	u32 val = EDP_PSR_ENABLE;
> > +	u32 tp;
> >  
> >  	/* Let's use 6 as the minimum to cover all known cases
> > including the
> >  	 * off-by-one issue that HW has in some cases.
> > @@ -460,13 +461,24 @@ static void hsw_activate_psr1(struct intel_dp
> > *intel_dp)
> >  		val |= EDP_PSR_LINK_STANDBY;
> >  
> >  	val |= dev_priv->vbt.psr.tp1_wakeup_time <<
> > EDP_PSR_TP1_TIME_SHIFT;
> > -	val |= dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time <<
> > EDP_PSR_TP2_TP3_TIME_SHIFT;
> >  
> > -	if (intel_dp_source_supports_hbr2(intel_dp) &&
> Now that you are removing a caller, make this function static and
> move
> to intel_dp_link_tranining.c instead ? Or just inline the code there.
> 
> > -	    drm_dp_tps3_supported(intel_dp->dpcd))
> > -		val |= EDP_PSR_TP1_TP3_SEL;
> > -	else
> > -		val |= EDP_PSR_TP1_TP2_SEL;
> > +	tp = intel_dp_training_pattern(intel_dp);
> > +	if (tp == DP_TRAINING_PATTERN_4) {
> > +		/*
> > +		 * TP4 is selected by setting EDP_PSR_TP4_TIME with
> > other value
> > +		 * than PSR_TP_WAKEUP_TIME_NONE
> > +		 */
> IMHO I think we should skip this comment, we'd have to write
> documentation for every other register if we are going to do this :)
> 
> > +		val |= dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time <<
> > EDP_PSR_TP4_TIME_SHIFT;
> 
> So, the EDP_PSR_TP1_TP3_SEL bit has no effect when the TP4 duration
> is
> non-zero, seems like an indirect way of switching to TP4 compared to
> TP2 and TP3.
> 
> > +	} else {
> > +		if (INTEL_GEN(dev_priv) >= 11)
> > +			val |= PSR_TP_WAKEUP_TIME_NONE <<
> > EDP_PSR_TP4_TIME_SHIFT;
> > +
> > +		val |= dev_priv->vbt.psr.tp2_tp3_tp4_wakeup_time <<
> > EDP_PSR_TP2_TP3_TIME_SHIFT;
> > +		if (tp == DP_TRAINING_PATTERN_3)
> > +			val |= EDP_PSR_TP1_TP3_SEL;
> > +		else
> > +			val |= EDP_PSR_TP1_TP2_SEL;
> > +	}
> >  
> >  	if (INTEL_GEN(dev_priv) >= 8)
> >  		val |= EDP_PSR_CRC_ENABLE;

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 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