[PATCH 1/9] drm/i915: Disable PSR in Apple panels

[PATCH 1/9] drm/i915: Disable PSR in Apple panels

[José Roberto de Souza]

i915 yet don't support PSR in Apple panels, so lets keep it disabled
while we work on that.

Fixes: 598c6cfe0690 (drm/i915/psr: Enable PSR1 on gen-9+ HW)
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/drm_dp_helper.c  | 2 ++
 drivers/gpu/drm/i915/intel_psr.c | 6 ++++++
 include/drm/drm_dp_helper.h      | 1 +
 3 files changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 6d483487f2b4..6b5a19d3e347 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -1273,6 +1273,8 @@ static const struct dpcd_quirk dpcd_quirk_list[] = {
 	{ OUI(0x00, 0x22, 0xb9), DEVICE_ID_ANY, true, BIT(DP_DPCD_QUIRK_CONSTANT_N) },
 	/* LG LP140WF6-SPM1 eDP panel */
 	{ OUI(0x00, 0x22, 0xb9), DEVICE_ID('s', 'i', 'v', 'a', 'r', 'T'), false, BIT(DP_DPCD_QUIRK_CONSTANT_N) },
+	/* Apple panels needs some additional handling to support PSR */
+	{ OUI(0x00, 0x10, 0xfa), DEVICE_ID_ANY, false, BIT(DP_DPCD_QUIRK_PSR_NOT_CURRENTLY_SUPPORTED) }
 };
 
 #undef OUI
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 572e626eadff..f5d27a02eb28 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -274,6 +274,12 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp)
 		DRM_DEBUG_KMS("Panel lacks power state control, PSR cannot be enabled\n");
 		return;
 	}
+
+	if (drm_dp_has_quirk(&intel_dp->desc, DP_DPCD_QUIRK_PSR_NOT_CURRENTLY_SUPPORTED)) {
+		DRM_DEBUG_KMS("PSR support not currently available for this panel\n");
+		return;
+	}
+
 	dev_priv->psr.sink_support = true;
 	dev_priv->psr.sink_sync_latency =
 		intel_dp_get_sink_sync_latency(intel_dp);
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 3314e91f6eb3..db516c48cda3 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -1364,6 +1364,7 @@ enum drm_dp_quirk {
 	 * to 16 bits. So will give a constant value (0x8000) for compatability.
 	 */
 	DP_DPCD_QUIRK_CONSTANT_N,
+	DP_DPCD_QUIRK_PSR_NOT_CURRENTLY_SUPPORTED,
 };
 
 /**
-- 
2.19.2

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

[PATCH 2/9] drm/i915/psr: Don't tell sink that main link will be active while is active PSR2

[José Roberto de Souza]

For PSR2 there is no register to tell HW to keep main link enabled
while PSR2 is active, so don't configure sink DPCD with a
misleading value.

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index f5d27a02eb28..888e348cc1b4 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -391,12 +391,14 @@ static void intel_psr_enable_sink(struct intel_dp *intel_dp)
 		drm_dp_dpcd_writeb(&intel_dp->aux, DP_RECEIVER_ALPM_CONFIG,
 				   DP_ALPM_ENABLE);
 		dpcd_val |= DP_PSR_ENABLE_PSR2;
+	} else {
+		if (dev_priv->psr.link_standby)
+			dpcd_val |= DP_PSR_MAIN_LINK_ACTIVE;
+
+		if (INTEL_GEN(dev_priv) >= 8)
+			dpcd_val |= DP_PSR_CRC_VERIFICATION;
 	}
 
-	if (dev_priv->psr.link_standby)
-		dpcd_val |= DP_PSR_MAIN_LINK_ACTIVE;
-	if (!dev_priv->psr.psr2_enabled && INTEL_GEN(dev_priv) >= 8)
-		dpcd_val |= DP_PSR_CRC_VERIFICATION;
 	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, dpcd_val);
 
 	drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
-- 
2.19.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/9] drm/i915/psr: Enable sink to trigger a interruption on PSR2 CRC mismatch

[José Roberto de Souza]

eDP spec states 2 different bits to enable sink to trigger a
interruption when there is a CRC mismatch.
DP_PSR_CRC_VERIFICATION is for PSR only and
DP_PSR_IRQ_HPD_WITH_CRC_ERRORS is for PSR2 only.

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 888e348cc1b4..607c3ec41679 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -390,7 +390,7 @@ static void intel_psr_enable_sink(struct intel_dp *intel_dp)
 	if (dev_priv->psr.psr2_enabled) {
 		drm_dp_dpcd_writeb(&intel_dp->aux, DP_RECEIVER_ALPM_CONFIG,
 				   DP_ALPM_ENABLE);
-		dpcd_val |= DP_PSR_ENABLE_PSR2;
+		dpcd_val |= DP_PSR_ENABLE_PSR2 | DP_PSR_IRQ_HPD_WITH_CRC_ERRORS;
 	} else {
 		if (dev_priv->psr.link_standby)
 			dpcd_val |= DP_PSR_MAIN_LINK_ACTIVE;
-- 
2.19.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/9] drm/i915/icl: Do not change reserved registers related to PSR2

[José Roberto de Souza]

For ICL the bit 12 of CHICKEN_TRANS is reserved so we should not
touch it and as by default VSC_DATA_SEL_SOFTWARE_CONTROL is already
unset in gen10 + GLK we can just drop it and fix for both gens.

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 607c3ec41679..7607a58a6ec0 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -635,17 +635,14 @@ static void intel_psr_enable_source(struct intel_dp *intel_dp,
 	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
 		hsw_psr_setup_aux(intel_dp);
 
-	if (dev_priv->psr.psr2_enabled) {
+	if (dev_priv->psr.psr2_enabled && (IS_GEN9(dev_priv) &&
+					   !IS_GEMINILAKE(dev_priv))) {
 		i915_reg_t reg = gen9_chicken_trans_reg(dev_priv,
 							cpu_transcoder);
 		u32 chicken = I915_READ(reg);
 
-		if (IS_GEN9(dev_priv) && !IS_GEMINILAKE(dev_priv))
-			chicken |= (PSR2_VSC_ENABLE_PROG_HEADER
-				   | PSR2_ADD_VERTICAL_LINE_COUNT);
-
-		else
-			chicken &= ~VSC_DATA_SEL_SOFTWARE_CONTROL;
+		chicken |= PSR2_VSC_ENABLE_PROG_HEADER |
+			   PSR2_ADD_VERTICAL_LINE_COUNT;
 		I915_WRITE(reg, chicken);
 	}
 
-- 
2.19.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 5/9] drm: Add offset of PSR2 SU X granularity value

[José Roberto de Souza]

Source is required to comply to sink SU granularity when
DP_PSR2_SU_GRANULARITY_REQUIRED is set in DP_PSR_CAPS,
so adding the register here.

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 include/drm/drm_dp_helper.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index db516c48cda3..acc7ccfd2044 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -314,6 +314,9 @@
 # define DP_PSR_SETUP_TIME_SHIFT            1
 # define DP_PSR2_SU_Y_COORDINATE_REQUIRED   (1 << 4)  /* eDP 1.4a */
 # define DP_PSR2_SU_GRANULARITY_REQUIRED    (1 << 5)  /* eDP 1.4b */
+
+#define DP_PSR2_SU_X_GRANULARITY	    0x072 /* eDP 1.4b */
+
 /*
  * 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,
-- 
2.19.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 6/9] drm/i915/psr: Check if source supports sink specific SU granularity

[José Roberto de Souza]

According to eDP spec, sink can required specific selective update
granularity that source must comply.
Here caching the value if required and checking source supports it.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
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  |  1 +
 drivers/gpu/drm/i915/intel_psr.c | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f763b30f98d9..cbcd85af95bf 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -506,6 +506,7 @@ struct i915_psr {
 	ktime_t last_exit;
 	bool sink_not_reliable;
 	bool irq_aux_error;
+	u16 su_x_granularity;
 };
 
 enum intel_pch {
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 7607a58a6ec0..9215c9052381 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -257,6 +257,21 @@ static u8 intel_dp_get_sink_sync_latency(struct intel_dp *intel_dp)
 	return val;
 }
 
+static u16 intel_dp_get_su_x_granulartiy(struct intel_dp *intel_dp)
+{
+	u16 val = 0;
+	ssize_t r;
+
+	if (!(intel_dp->psr_dpcd[1] & DP_PSR2_SU_GRANULARITY_REQUIRED))
+		return val;
+
+	r = drm_dp_dpcd_read(&intel_dp->aux, DP_PSR2_SU_X_GRANULARITY, &val, 2);
+	if (r != 2)
+		DRM_WARN("Unable to read DP_PSR2_SU_X_GRANULARITY\n");
+
+	return val;
+}
+
 void intel_psr_init_dpcd(struct intel_dp *intel_dp)
 {
 	struct drm_i915_private *dev_priv =
@@ -311,6 +326,8 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp)
 		if (dev_priv->psr.sink_psr2_support) {
 			dev_priv->psr.colorimetry_support =
 				intel_dp_get_colorimetry_status(intel_dp);
+			dev_priv->psr.su_x_granularity =
+				intel_dp_get_su_x_granulartiy(intel_dp);
 		}
 	}
 }
@@ -525,6 +542,21 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
 		return false;
 	}
 
+	if (dev_priv->psr.su_x_granularity) {
+		/*
+		 * HW will always send full lines in SU blocks, so X will
+		 * always be 0 and we only need to check the width to validate
+		 * horizontal granularity.
+		 * About vertical granularity HW works by SU blocks starting
+		 * at each 4 lines with height of 4 lines, what eDP states
+		 * that sink should support.
+		 */
+		if (crtc_hdisplay % dev_priv->psr.su_x_granularity) {
+			DRM_DEBUG_KMS("PSR2 not enabled, HW can not match sink SU granularity requirement\n");
+			return false;
+		}
+	}
+
 	return true;
 }
 
-- 
2.19.2

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

[PATCH 7/9] drm/i915/psr: Rename PSR2 macros to better match meaning

[José Roberto de Souza]

The first 8 bits of PSR2_CTL have 2 fields to set frames count, the
first one is to set how many idle frames PSR2 HW needs to wait before
enter in deep sleep and the second one it is how many frames(it don't
need to be idle frames) PSR2 HW will wait before start the PSR
activation sequence.
The previous names was really misleading and caused wrong values being
set so better rename to make it clear.

Also taking the oportunity to improve those macros.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h  | 35 ++++++++++++++++----------------
 drivers/gpu/drm/i915/intel_psr.c |  7 ++++---
 2 files changed, 22 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 47baf2fe8f71..73046bb9ec7c 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4203,23 +4203,24 @@ enum {
 #define   EDP_PSR_DEBUG_MASK_DISP_REG_WRITE    (1 << 16) /* Reserved in ICL+ */
 #define   EDP_PSR_DEBUG_EXIT_ON_PIXEL_UNDERRUN (1 << 15) /* SKL+ */
 
-#define EDP_PSR2_CTL			_MMIO(0x6f900)
-#define   EDP_PSR2_ENABLE		(1 << 31)
-#define   EDP_SU_TRACK_ENABLE		(1 << 30)
-#define   EDP_Y_COORDINATE_VALID	(1 << 26) /* GLK and CNL+ */
-#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_FRAME_BEFORE_SU_SHIFT 4
-#define   EDP_PSR2_FRAME_BEFORE_SU_MASK	(0xf << 4)
-#define   EDP_PSR2_FRAME_BEFORE_SU(a)	((a) << 4)
-#define   EDP_PSR2_IDLE_FRAME_MASK	0xf
-#define   EDP_PSR2_IDLE_FRAME_SHIFT	0
+#define EDP_PSR2_CTL					_MMIO(0x6f900)
+#define   EDP_PSR2_ENABLE				(1 << 31)
+#define   EDP_SU_TRACK_ENABLE				(1 << 30)
+#define   EDP_Y_COORDINATE_VALID			(1 << 26) /* GLK and CNL+ */
+#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_FRAMES_BEFORE_ACTIVATE_SHIFT		(4)
+#define   EDP_PSR2_FRAMES_BEFORE_ACTIVATE_MASK		(0xf << EDP_PSR2_FRAMES_BEFORE_ACTIVATE_SHIFT)
+#define   EDP_PSR2_FRAMES_BEFORE_ACTIVATE(n)		(((n) << EDP_PSR2_FRAMES_BEFORE_ACTIVATE_SHIFT) & EDP_PSR2_FRAMES_BEFORE_ACTIVATE_MASK)
+#define   EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP_MASK	(0xf)
+#define   EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP_SHIFT	(0)
+#define   EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP(n)		(((n) << EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP_SHIFT) & EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP_MASK)
 
 #define _PSR_EVENT_TRANS_A			0x60848
 #define _PSR_EVENT_TRANS_B			0x61848
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 9215c9052381..ba7bbe3f8df2 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -479,6 +479,7 @@ static void hsw_activate_psr1(struct intel_dp *intel_dp)
 static void hsw_activate_psr2(struct intel_dp *intel_dp)
 {
 	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
+	struct i915_psr *psr = &dev_priv->psr;
 	u32 val;
 
 	/* Let's use 6 as the minimum to cover all known cases including the
@@ -486,8 +487,8 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
 	 */
 	int idle_frames = max(6, dev_priv->vbt.psr.idle_frames);
 
-	idle_frames = max(idle_frames, dev_priv->psr.sink_sync_latency + 1);
-	val = idle_frames << EDP_PSR2_IDLE_FRAME_SHIFT;
+	idle_frames = max(idle_frames, psr->sink_sync_latency + 1);
+	val = EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP(idle_frames);
 
 	/* FIXME: selective update is probably totally broken because it doesn't
 	 * mesh at all with our frontbuffer tracking. And the hw alone isn't
@@ -496,7 +497,7 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
 	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
 		val |= EDP_Y_COORDINATE_ENABLE;
 
-	val |= EDP_PSR2_FRAME_BEFORE_SU(dev_priv->psr.sink_sync_latency + 1);
+	val |= EDP_PSR2_FRAMES_BEFORE_ACTIVATE(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)
-- 
2.19.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 8/9] drm/i915/psr: Set the right frames values

[José Roberto de Souza]

EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP() was being set with the number of
frames that it should wait to enter PSR, what is wrong.
Here it is setting this field with the highest value to avoid PSR2
exits frequently, as when HW exit deep sleep it needs to go to idle
state causing a PSR exit for then waiting a few frames before
activate PSR2 again.
This will result in more power saving as the sleep state also provide
some power savings by doing selective updates instead of full screen
updates.

About EDP_PSR2_FRAMES_BEFORE_ACTIVATE() it is the number of frames
(not idle frames) that PSR2 hardware will wait to activate PSR2, so
lets keep using the sink sync latency.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index ba7bbe3f8df2..6fd793fec5e9 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -482,13 +482,13 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
 	struct i915_psr *psr = &dev_priv->psr;
 	u32 val;
 
-	/* Let's use 6 as the minimum to cover all known cases including the
-	 * off-by-one issue that HW has in some cases.
+	/* sink_sync_latency of 8 means source has to wait for more than 8
+	 * frames, we'll go with 9 frames for now
 	 */
-	int idle_frames = max(6, dev_priv->vbt.psr.idle_frames);
+	val = EDP_PSR2_FRAMES_BEFORE_ACTIVATE(psr->sink_sync_latency + 1);
 
-	idle_frames = max(idle_frames, psr->sink_sync_latency + 1);
-	val = EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP(idle_frames);
+	/* Avoid deep sleep as much as possible to avoid PSR2 idle state */
+	val |= EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP(15);
 
 	/* FIXME: selective update is probably totally broken because it doesn't
 	 * mesh at all with our frontbuffer tracking. And the hw alone isn't
@@ -497,8 +497,6 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
 	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
 		val |= EDP_Y_COORDINATE_ENABLE;
 
-	val |= EDP_PSR2_FRAMES_BEFORE_ACTIVATE(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)
 		val |= EDP_PSR2_TP2_TIME_50us;
-- 
2.19.2

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

[PATCH 9/9] drm/i915: Remove old PSR2 FIXME about frontbuffer tracking

[José Roberto de Souza]

Our frontbuffer tracking improved over the years + the WA #0884
helped us keep PSR2 enabled while triggering screen updates when
necessary so this FIXME is not valid anymore.

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 6fd793fec5e9..a1bde8bbd85b 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -490,9 +490,6 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
 	/* Avoid deep sleep as much as possible to avoid PSR2 idle state */
 	val |= EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP(15);
 
-	/* 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 (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
 		val |= EDP_Y_COORDINATE_ENABLE;
-- 
2.19.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

✗ Fi.CI.CHECKPATCH: warning for series starting with [1/9] drm/i915: Disable PSR in Apple panels

[Patchwork]

== Series Details ==

Series: series starting with [1/9] drm/i915: Disable PSR in Apple panels
URL   : https://patchwork.freedesktop.org/series/53043/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
ccc567f41e7e drm/i915: Disable PSR in Apple panels
-:26: WARNING:LONG_LINE: line over 100 characters
#26: FILE: drivers/gpu/drm/drm_dp_helper.c:1277:
+	{ OUI(0x00, 0x10, 0xfa), DEVICE_ID_ANY, false, BIT(DP_DPCD_QUIRK_PSR_NOT_CURRENTLY_SUPPORTED) }

total: 0 errors, 1 warnings, 0 checks, 27 lines checked
408a8c4f5a1c drm/i915/psr: Don't tell sink that main link will be active while is active PSR2
1034461f117c drm/i915/psr: Enable sink to trigger a interruption on PSR2 CRC mismatch
5908f8782685 drm/i915/icl: Do not change reserved registers related to PSR2
eb77d5d439c3 drm: Add offset of PSR2 SU X granularity value
d727b3d0455b drm/i915/psr: Check if source supports sink specific SU granularity
6f091e28a04e drm/i915/psr: Rename PSR2 macros to better match meaning
-:61: WARNING:LONG_LINE: line over 100 characters
#61: FILE: drivers/gpu/drm/i915/i915_reg.h:4219:
+#define   EDP_PSR2_FRAMES_BEFORE_ACTIVATE_MASK		(0xf << EDP_PSR2_FRAMES_BEFORE_ACTIVATE_SHIFT)

-:62: WARNING:LONG_LINE: line over 100 characters
#62: FILE: drivers/gpu/drm/i915/i915_reg.h:4220:
+#define   EDP_PSR2_FRAMES_BEFORE_ACTIVATE(n)		(((n) << EDP_PSR2_FRAMES_BEFORE_ACTIVATE_SHIFT) & EDP_PSR2_FRAMES_BEFORE_ACTIVATE_MASK)

-:65: WARNING:LONG_LINE: line over 100 characters
#65: FILE: drivers/gpu/drm/i915/i915_reg.h:4223:
+#define   EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP(n)		(((n) << EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP_SHIFT) & EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP_MASK)

total: 0 errors, 3 warnings, 0 checks, 66 lines checked
2d3fa3a77422 drm/i915/psr: Set the right frames values
8e2a53689edf drm/i915: Remove old PSR2 FIXME about frontbuffer tracking

_______________________________________________
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/9] drm/i915: Disable PSR in Apple panels

[Patchwork]

== Series Details ==

Series: series starting with [1/9] drm/i915: Disable PSR in Apple panels
URL   : https://patchwork.freedesktop.org/series/53043/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Sparse version: v0.5.2
Commit: drm/i915: Disable PSR in Apple panels
Okay!

Commit: drm/i915/psr: Don't tell sink that main link will be active while is active PSR2
Okay!

Commit: drm/i915/psr: Enable sink to trigger a interruption on PSR2 CRC mismatch
Okay!

Commit: drm/i915/icl: Do not change reserved registers related to PSR2
Okay!

Commit: drm: Add offset of PSR2 SU X granularity value
Okay!

Commit: drm/i915/psr: Check if source supports sink specific SU granularity
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3569:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3570:16: warning: expression using sizeof(void)

Commit: drm/i915/psr: Rename PSR2 macros to better match meaning
-O:drivers/gpu/drm/i915/intel_psr.c:487:27: warning: expression using sizeof(void)
-O:drivers/gpu/drm/i915/intel_psr.c:489:23: warning: expression using sizeof(void)
-O:drivers/gpu/drm/i915/intel_psr.c:489:23: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_psr.c:488:27: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_psr.c:490:23: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_psr.c:490:23: warning: expression using sizeof(void)

Commit: drm/i915/psr: Set the right frames values
-O:drivers/gpu/drm/i915/intel_psr.c:488:27: warning: expression using sizeof(void)
-O:drivers/gpu/drm/i915/intel_psr.c:490:23: warning: expression using sizeof(void)
-O:drivers/gpu/drm/i915/intel_psr.c:490:23: warning: expression using sizeof(void)

Commit: drm/i915: Remove old PSR2 FIXME about frontbuffer tracking
Okay!

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

✗ Fi.CI.BAT: failure for series starting with [1/9] drm/i915: Disable PSR in Apple panels

[Patchwork]

== Series Details ==

Series: series starting with [1/9] drm/i915: Disable PSR in Apple panels
URL   : https://patchwork.freedesktop.org/series/53043/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_5207 -> Patchwork_10907 =

== Summary - FAILURE ==

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

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

== Possible new issues ==

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

  === IGT changes ===

    ==== Possible regressions ====

    igt@gem_exec_suspend@basic-s4-devices:
      fi-ivb-3520m:       PASS -> FAIL

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_exec_suspend@basic-s4-devices:
      fi-blb-e6850:       PASS -> INCOMPLETE (fdo#107718)

    igt@i915_module_load@reload:
      fi-ilk-650:         PASS -> DMESG-WARN (fdo#106387)

    igt@i915_selftest@live_execlists:
      fi-apl-guc:         PASS -> INCOMPLETE (fdo#103927)

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
      fi-byt-clapper:     PASS -> FAIL (fdo#107362, fdo#103191)

    
    ==== Possible fixes ====

    igt@gem_ctx_create@basic-files:
      {fi-icl-u3}:        DMESG-WARN (fdo#107724) -> PASS

    igt@gem_ctx_switch@basic-default:
      fi-icl-u2:          DMESG-WARN (fdo#107724) -> PASS

    igt@kms_frontbuffer_tracking@basic:
      fi-byt-clapper:     FAIL (fdo#103167) -> PASS

    igt@kms_pipe_crc_basic@hang-read-crc-pipe-a:
      fi-byt-clapper:     FAIL (fdo#107362, fdo#103191) -> PASS

    
    ==== Warnings ====

    igt@i915_selftest@live_contexts:
      {fi-icl-u3}:        INCOMPLETE (fdo#108315) -> DMESG-FAIL (fdo#108569)

    
  {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#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#106387 https://bugs.freedesktop.org/show_bug.cgi?id=106387
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107718 https://bugs.freedesktop.org/show_bug.cgi?id=107718
  fdo#107724 https://bugs.freedesktop.org/show_bug.cgi?id=107724
  fdo#108315 https://bugs.freedesktop.org/show_bug.cgi?id=108315
  fdo#108569 https://bugs.freedesktop.org/show_bug.cgi?id=108569


== Participating hosts (52 -> 46) ==

  Missing    (6): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 


== Build changes ==

    * Linux: CI_DRM_5207 -> Patchwork_10907

  CI_DRM_5207: 5931122c0786671d3f6a46bd672d4e7a3960345e @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4729: 2388bbd062c17b5912039101efd4603e8d876c88 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10907: 8e2a53689edf77b9f12ff848d78a5a7b6dfb2c5f @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

8e2a53689edf drm/i915: Remove old PSR2 FIXME about frontbuffer tracking
2d3fa3a77422 drm/i915/psr: Set the right frames values
6f091e28a04e drm/i915/psr: Rename PSR2 macros to better match meaning
d727b3d0455b drm/i915/psr: Check if source supports sink specific SU granularity
eb77d5d439c3 drm: Add offset of PSR2 SU X granularity value
5908f8782685 drm/i915/icl: Do not change reserved registers related to PSR2
1034461f117c drm/i915/psr: Enable sink to trigger a interruption on PSR2 CRC mismatch
408a8c4f5a1c drm/i915/psr: Don't tell sink that main link will be active while is active PSR2
ccc567f41e7e drm/i915: Disable PSR in Apple panels

== Logs ==

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

Re: ✗ Fi.CI.BAT: failure for series starting with [1/9] drm/i915: Disable PSR in Apple panels

[Saarinen, Jani]

HI, 

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
> Patchwork
> Sent: tiistai 27. marraskuuta 2018 3.16
> To: Souza, Jose <jose.souza@intel.com>
> Cc: intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] ✗ Fi.CI.BAT: failure for series starting with [1/9] drm/i915:
> Disable PSR in Apple panels
> 
> == Series Details ==
> 
> Series: series starting with [1/9] drm/i915: Disable PSR in Apple panels
> URL   : https://patchwork.freedesktop.org/series/53043/
> State : failure
> 
> == Summary ==
> 
> = CI Bug Log - changes from CI_DRM_5207 -> Patchwork_10907 =
> 
> == Summary - FAILURE ==
> 
>   Serious unknown changes coming with Patchwork_10907 absolutely need to be
>   verified manually.
> 
>   If you think the reported changes have nothing to do with the changes
>   introduced in Patchwork_10907, please notify your bug team to allow them
>   to document this new failure mode, which will reduce false positives in CI.
> 
>   External URL:
> https://patchwork.freedesktop.org/api/1.0/series/53043/revisions/1/mbox/
> 
> == Possible new issues ==
> 
>   Here are the unknown changes that may have been introduced in
> Patchwork_10907:
> 
>   === IGT changes ===
> 
>     ==== Possible regressions ====
> 
>     igt@gem_exec_suspend@basic-s4-devices:
>       fi-ivb-3520m:       PASS -> FAIL
I guess not noise as did exactly same on try-bot
"    igt@gem_exec_suspend@basic-s4-devices:
      fi-ivb-3520m:       PASS -> FAIL"

> 
> 
> == Known issues ==
> 
>   Here are the changes found in Patchwork_10907 that come from known issues:
> 
>   === IGT changes ===
> 
>     ==== Issues hit ====
> 
>     igt@gem_exec_suspend@basic-s4-devices:
>       fi-blb-e6850:       PASS -> INCOMPLETE (fdo#107718)
> 
>     igt@i915_module_load@reload:
>       fi-ilk-650:         PASS -> DMESG-WARN (fdo#106387)
> 
>     igt@i915_selftest@live_execlists:
>       fi-apl-guc:         PASS -> INCOMPLETE (fdo#103927)
> 
>     igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
>       fi-byt-clapper:     PASS -> FAIL (fdo#107362, fdo#103191)
> 
> 
>     ==== Possible fixes ====
> 
>     igt@gem_ctx_create@basic-files:
>       {fi-icl-u3}:        DMESG-WARN (fdo#107724) -> PASS
> 
>     igt@gem_ctx_switch@basic-default:
>       fi-icl-u2:          DMESG-WARN (fdo#107724) -> PASS
> 
>     igt@kms_frontbuffer_tracking@basic:
>       fi-byt-clapper:     FAIL (fdo#103167) -> PASS
> 
>     igt@kms_pipe_crc_basic@hang-read-crc-pipe-a:
>       fi-byt-clapper:     FAIL (fdo#107362, fdo#103191) -> PASS
> 
> 
>     ==== Warnings ====
> 
>     igt@i915_selftest@live_contexts:
>       {fi-icl-u3}:        INCOMPLETE (fdo#108315) -> DMESG-FAIL (fdo#108569)
> 
> 
>   {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#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
>   fdo#106387 https://bugs.freedesktop.org/show_bug.cgi?id=106387
>   fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
>   fdo#107718 https://bugs.freedesktop.org/show_bug.cgi?id=107718
>   fdo#107724 https://bugs.freedesktop.org/show_bug.cgi?id=107724
>   fdo#108315 https://bugs.freedesktop.org/show_bug.cgi?id=108315
>   fdo#108569 https://bugs.freedesktop.org/show_bug.cgi?id=108569
> 
> 
> == Participating hosts (52 -> 46) ==
> 
>   Missing    (6): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan
> fi-ctg-p8600
> 
> 
> == Build changes ==
> 
>     * Linux: CI_DRM_5207 -> Patchwork_10907
> 
>   CI_DRM_5207: 5931122c0786671d3f6a46bd672d4e7a3960345e @
> git://anongit.freedesktop.org/gfx-ci/linux
>   IGT_4729: 2388bbd062c17b5912039101efd4603e8d876c88 @
> git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
>   Patchwork_10907: 8e2a53689edf77b9f12ff848d78a5a7b6dfb2c5f @
> git://anongit.freedesktop.org/gfx-ci/linux
> 
> 
> == Linux commits ==
> 
> 8e2a53689edf drm/i915: Remove old PSR2 FIXME about frontbuffer tracking
> 2d3fa3a77422 drm/i915/psr: Set the right frames values 6f091e28a04e
> drm/i915/psr: Rename PSR2 macros to better match meaning d727b3d0455b
> drm/i915/psr: Check if source supports sink specific SU granularity
> eb77d5d439c3 drm: Add offset of PSR2 SU X granularity value
> 5908f8782685 drm/i915/icl: Do not change reserved registers related to PSR2
> 1034461f117c drm/i915/psr: Enable sink to trigger a interruption on PSR2 CRC
> mismatch 408a8c4f5a1c drm/i915/psr: Don't tell sink that main link will be active
> while is active PSR2 ccc567f41e7e drm/i915: Disable PSR in Apple panels
> 
> == Logs ==
> 
> For more details see: https://intel-gfx-ci.01.org/tree/drm-
> tip/Patchwork_10907/
> _______________________________________________
> 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/9] drm/i915: Disable PSR in Apple panels

[Ville Syrjälä]

On Mon, Nov 26, 2018 at 04:37:02PM -0800, José Roberto de Souza wrote:
> i915 yet don't support PSR in Apple panels, so lets keep it disabled
> while we work on that.
> 
> Fixes: 598c6cfe0690 (drm/i915/psr: Enable PSR1 on gen-9+ HW)
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/drm_dp_helper.c  | 2 ++
>  drivers/gpu/drm/i915/intel_psr.c | 6 ++++++
>  include/drm/drm_dp_helper.h      | 1 +
>  3 files changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 6d483487f2b4..6b5a19d3e347 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -1273,6 +1273,8 @@ static const struct dpcd_quirk dpcd_quirk_list[] = {
>  	{ OUI(0x00, 0x22, 0xb9), DEVICE_ID_ANY, true, BIT(DP_DPCD_QUIRK_CONSTANT_N) },
>  	/* LG LP140WF6-SPM1 eDP panel */
>  	{ OUI(0x00, 0x22, 0xb9), DEVICE_ID('s', 'i', 'v', 'a', 'r', 'T'), false, BIT(DP_DPCD_QUIRK_CONSTANT_N) },
> +	/* Apple panels needs some additional handling to support PSR */
> +	{ OUI(0x00, 0x10, 0xfa), DEVICE_ID_ANY, false, BIT(DP_DPCD_QUIRK_PSR_NOT_CURRENTLY_SUPPORTED) }
>  };
>  
>  #undef OUI
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 572e626eadff..f5d27a02eb28 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -274,6 +274,12 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp)
>  		DRM_DEBUG_KMS("Panel lacks power state control, PSR cannot be enabled\n");
>  		return;
>  	}
> +
> +	if (drm_dp_has_quirk(&intel_dp->desc, DP_DPCD_QUIRK_PSR_NOT_CURRENTLY_SUPPORTED)) {
> +		DRM_DEBUG_KMS("PSR support not currently available for this panel\n");
> +		return;
> +	}
> +
>  	dev_priv->psr.sink_support = true;
>  	dev_priv->psr.sink_sync_latency =
>  		intel_dp_get_sink_sync_latency(intel_dp);
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 3314e91f6eb3..db516c48cda3 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -1364,6 +1364,7 @@ enum drm_dp_quirk {
>  	 * to 16 bits. So will give a constant value (0x8000) for compatability.
>  	 */
>  	DP_DPCD_QUIRK_CONSTANT_N,
> +	DP_DPCD_QUIRK_PSR_NOT_CURRENTLY_SUPPORTED,

Why such a convoluted name? DP_DPCD_QUIRK_NO_PSR?

>  };
>  
>  /**
> -- 
> 2.19.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 1/9] drm/i915: Disable PSR in Apple panels

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 24045 bytes --]

Hi José,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on v4.20-rc4 next-20181126]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Jos-Roberto-de-Souza/drm-i915-Disable-PSR-in-Apple-panels/20181127-085802
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   include/net/mac80211.h:477: warning: cannot understand function prototype: 'struct ieee80211_ftm_responder_params '
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'rx_stats_avg' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'rx_stats_avg.signal' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'rx_stats_avg.chain_signal' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.filtered' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.retry_failed' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.retry_count' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.lost_packets' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.last_tdls_pkt_time' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.msdu_retries' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.msdu_failed' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.last_ack' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.last_ack_signal' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.ack_signal_filled' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.avg_ack_signal' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'tx_stats.packets' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'tx_stats.bytes' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'tx_stats.last_rate' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'tx_stats.msdu' not described in 'sta_info'
   kernel/rcu/tree.c:685: warning: Excess function parameter 'irq' description in 'rcu_nmi_exit'
   include/linux/dma-buf.h:304: warning: Function parameter or member 'cb_excl.cb' not described in 'dma_buf'
   include/linux/dma-buf.h:304: warning: Function parameter or member 'cb_excl.poll' not described in 'dma_buf'
   include/linux/dma-buf.h:304: warning: Function parameter or member 'cb_excl.active' not described in 'dma_buf'
   include/linux/dma-buf.h:304: warning: Function parameter or member 'cb_shared.cb' not described in 'dma_buf'
   include/linux/dma-buf.h:304: warning: Function parameter or member 'cb_shared.poll' not described in 'dma_buf'
   include/linux/dma-buf.h:304: warning: Function parameter or member 'cb_shared.active' not described in 'dma_buf'
   include/linux/dma-fence-array.h:54: warning: Function parameter or member 'work' not described in 'dma_fence_array'
   include/linux/gpio/driver.h:375: warning: Function parameter or member 'init_valid_mask' not described in 'gpio_chip'
   include/linux/iio/hw-consumer.h:1: warning: no structured comments found
   include/linux/input/sparse-keymap.h:46: warning: Function parameter or member 'sw' not described in 'key_entry'
   include/linux/regulator/driver.h:227: warning: Function parameter or member 'resume' not described in 'regulator_ops'
   arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.esw0' not described in 'irb'
   arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.esw1' not described in 'irb'
   arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.esw2' not described in 'irb'
   arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.esw3' not described in 'irb'
   arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.eadm' not described in 'irb'
   drivers/slimbus/stream.c:1: warning: no structured comments found
   include/linux/spi/spi.h:177: warning: Function parameter or member 'driver_override' not described in 'spi_device'
   drivers/target/target_core_device.c:1: warning: no structured comments found
   drivers/usb/typec/bus.c:1: warning: no structured comments found
   drivers/usb/typec/class.c:1: warning: no structured comments found
   include/linux/w1.h:281: warning: Function parameter or member 'of_match_table' not described in 'w1_family'
   fs/direct-io.c:257: warning: Excess function parameter 'offset' description in 'dio_complete'
   fs/file_table.c:1: warning: no structured comments found
   fs/libfs.c:477: warning: Excess function parameter 'available' description in 'simple_write_end'
   fs/posix_acl.c:646: warning: Function parameter or member 'inode' not described in 'posix_acl_update_mode'
   fs/posix_acl.c:646: warning: Function parameter or member 'mode_p' not described in 'posix_acl_update_mode'
   fs/posix_acl.c:646: warning: Function parameter or member 'acl' not described in 'posix_acl_update_mode'
   drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c:183: warning: Function parameter or member 'blockable' not described in 'amdgpu_mn_read_lock'
   drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c:254: warning: Function parameter or member 'blockable' not described in 'amdgpu_mn_invalidate_range_start_gfx'
   drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c:302: warning: Function parameter or member 'blockable' not described in 'amdgpu_mn_invalidate_range_start_hsa'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:382: warning: cannot understand function prototype: 'struct amdgpu_vm_pt_cursor '
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:383: warning: cannot understand function prototype: 'struct amdgpu_vm_pt_cursor '
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:555: warning: Function parameter or member 'adev' not described in 'for_each_amdgpu_vm_pt_leaf'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:555: warning: Function parameter or member 'vm' not described in 'for_each_amdgpu_vm_pt_leaf'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:555: warning: Function parameter or member 'start' not described in 'for_each_amdgpu_vm_pt_leaf'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:555: warning: Function parameter or member 'end' not described in 'for_each_amdgpu_vm_pt_leaf'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:555: warning: Function parameter or member 'cursor' not described in 'for_each_amdgpu_vm_pt_leaf'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:603: warning: Function parameter or member 'adev' not described in 'for_each_amdgpu_vm_pt_dfs_safe'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:603: warning: Function parameter or member 'vm' not described in 'for_each_amdgpu_vm_pt_dfs_safe'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:603: warning: Function parameter or member 'cursor' not described in 'for_each_amdgpu_vm_pt_dfs_safe'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:603: warning: Function parameter or member 'entry' not described in 'for_each_amdgpu_vm_pt_dfs_safe'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:848: warning: Function parameter or member 'level' not described in 'amdgpu_vm_bo_param'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1356: warning: Function parameter or member 'params' not described in 'amdgpu_vm_update_func'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1356: warning: Function parameter or member 'bo' not described in 'amdgpu_vm_update_func'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1356: warning: Function parameter or member 'pe' not described in 'amdgpu_vm_update_func'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1356: warning: Function parameter or member 'addr' not described in 'amdgpu_vm_update_func'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1356: warning: Function parameter or member 'count' not described in 'amdgpu_vm_update_func'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1356: warning: Function parameter or member 'incr' not described in 'amdgpu_vm_update_func'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1356: warning: Function parameter or member 'flags' not described in 'amdgpu_vm_update_func'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1523: warning: Function parameter or member 'params' not described in 'amdgpu_vm_update_huge'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1523: warning: Function parameter or member 'bo' not described in 'amdgpu_vm_update_huge'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1523: warning: Function parameter or member 'level' not described in 'amdgpu_vm_update_huge'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1523: warning: Function parameter or member 'pe' not described in 'amdgpu_vm_update_huge'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1523: warning: Function parameter or member 'addr' not described in 'amdgpu_vm_update_huge'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1523: warning: Function parameter or member 'count' not described in 'amdgpu_vm_update_huge'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1523: warning: Function parameter or member 'incr' not described in 'amdgpu_vm_update_huge'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1523: warning: Function parameter or member 'flags' not described in 'amdgpu_vm_update_huge'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:3098: warning: Function parameter or member 'pasid' not described in 'amdgpu_vm_make_compute'
   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:171: warning: Function parameter or member 'backlight_link' not described in 'amdgpu_display_manager'
   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:171: warning: Function parameter or member 'freesync_module' not described in 'amdgpu_display_manager'
   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:171: warning: Function parameter or member 'fw_dmcu' not described in 'amdgpu_display_manager'
   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:171: warning: Function parameter or member 'dmcu_fw_version' not described in 'amdgpu_display_manager'
   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c:1: warning: no structured comments found
   include/drm/drm_drv.h:609: warning: Function parameter or member 'gem_prime_pin' not described in 'drm_driver'
   include/drm/drm_drv.h:609: warning: Function parameter or member 'gem_prime_unpin' not described in 'drm_driver'
   include/drm/drm_drv.h:609: warning: Function parameter or member 'gem_prime_res_obj' not described in 'drm_driver'
   include/drm/drm_drv.h:609: warning: Function parameter or member 'gem_prime_get_sg_table' not described in 'drm_driver'
   include/drm/drm_drv.h:609: warning: Function parameter or member 'gem_prime_import_sg_table' not described in 'drm_driver'
   include/drm/drm_drv.h:609: warning: Function parameter or member 'gem_prime_vmap' not described in 'drm_driver'
   include/drm/drm_drv.h:609: warning: Function parameter or member 'gem_prime_vunmap' not described in 'drm_driver'
   include/drm/drm_drv.h:609: warning: Function parameter or member 'gem_prime_mmap' not described in 'drm_driver'
   include/drm/drm_atomic_state_helper.h:1: warning: no structured comments found
>> include/drm/drm_dp_helper.h:1369: warning: Enum value 'DP_DPCD_QUIRK_PSR_NOT_CURRENTLY_SUPPORTED' not described in enum 'drm_dp_quirk'
   drivers/gpu/drm/drm_dp_helper.c:1364: warning: Function parameter or member 'dsc_dpcd' not described in 'drm_dp_dsc_sink_max_slice_count'
   drivers/gpu/drm/drm_dp_helper.c:1364: warning: Function parameter or member 'is_edp' not described in 'drm_dp_dsc_sink_max_slice_count'
   Error: Cannot open file drivers/gpu/drm/drm_global.c
   Error: Cannot open file drivers/gpu/drm/drm_global.c
   WARNING: kernel-doc 'scripts/kernel-doc -rst -enable-lineno -export drivers/gpu/drm/drm_global.c' failed with return code 2
   drivers/gpu/drm/i915/i915_vma.h:49: warning: cannot understand function prototype: 'struct i915_vma '
   drivers/gpu/drm/i915/i915_vma.h:1: warning: no structured comments found
   drivers/gpu/drm/i915/intel_guc_fwif.h:536: warning: cannot understand function prototype: 'struct guc_log_buffer_state '
   drivers/gpu/drm/i915/i915_trace.h:1: warning: no structured comments found
   include/linux/skbuff.h:862: warning: Function parameter or member 'dev_scratch' not described in 'sk_buff'
   include/linux/skbuff.h:862: warning: Function parameter or member 'list' not described in 'sk_buff'
   include/linux/skbuff.h:862: warning: Function parameter or member 'ip_defrag_offset' not described in 'sk_buff'
   include/linux/skbuff.h:862: warning: Function parameter or member 'skb_mstamp_ns' not described in 'sk_buff'
   include/linux/skbuff.h:862: warning: Function parameter or member '__cloned_offset' not described in 'sk_buff'
   include/linux/skbuff.h:862: warning: Function parameter or member 'head_frag' not described in 'sk_buff'
   include/linux/skbuff.h:862: warning: Function parameter or member '__pkt_type_offset' not described in 'sk_buff'
   include/linux/skbuff.h:862: warning: Function parameter or member 'encapsulation' not described in 'sk_buff'
   include/linux/skbuff.h:862: warning: Function parameter or member 'encap_hdr_csum' not described in 'sk_buff'
   include/linux/skbuff.h:862: warning: Function parameter or member 'csum_valid' not described in 'sk_buff'
   include/linux/skbuff.h:862: warning: Function parameter or member 'csum_complete_sw' not described in 'sk_buff'
   include/linux/skbuff.h:862: warning: Function parameter or member 'csum_level' not described in 'sk_buff'
   include/linux/skbuff.h:862: warning: Function parameter or member 'inner_protocol_type' not described in 'sk_buff'
   include/linux/skbuff.h:862: warning: Function parameter or member 'remcsum_offload' not described in 'sk_buff'
   include/linux/skbuff.h:862: warning: Function parameter or member 'offload_fwd_mark' not described in 'sk_buff'
   include/linux/skbuff.h:862: warning: Function parameter or member 'offload_mr_fwd_mark' not described in 'sk_buff'
   include/linux/skbuff.h:862: warning: Function parameter or member 'sender_cpu' not described in 'sk_buff'
   include/linux/skbuff.h:862: warning: Function parameter or member 'reserved_tailroom' not described in 'sk_buff'
   include/linux/skbuff.h:862: warning: Function parameter or member 'inner_ipproto' not described in 'sk_buff'
   include/net/sock.h:238: warning: Function parameter or member 'skc_addrpair' not described in 'sock_common'
   include/net/sock.h:238: warning: Function parameter or member 'skc_portpair' not described in 'sock_common'
   include/net/sock.h:238: warning: Function parameter or member 'skc_ipv6only' not described in 'sock_common'
   include/net/sock.h:238: warning: Function parameter or member 'skc_net_refcnt' not described in 'sock_common'
   include/net/sock.h:238: warning: Function parameter or member 'skc_v6_daddr' not described in 'sock_common'
   include/net/sock.h:238: warning: Function parameter or member 'skc_v6_rcv_saddr' not described in 'sock_common'
   include/net/sock.h:238: warning: Function parameter or member 'skc_cookie' not described in 'sock_common'
   include/net/sock.h:238: warning: Function parameter or member 'skc_listener' not described in 'sock_common'
   include/net/sock.h:238: warning: Function parameter or member 'skc_tw_dr' not described in 'sock_common'
   include/net/sock.h:238: warning: Function parameter or member 'skc_rcv_wnd' not described in 'sock_common'
   include/net/sock.h:238: warning: Function parameter or member 'skc_tw_rcv_nxt' not described in 'sock_common'
   include/net/sock.h:509: warning: Function parameter or member 'sk_backlog.rmem_alloc' not described in 'sock'
   include/net/sock.h:509: warning: Function parameter or member 'sk_backlog.len' not described in 'sock'
   include/net/sock.h:509: warning: Function parameter or member 'sk_backlog.head' not described in 'sock'
   include/net/sock.h:509: warning: Function parameter or member 'sk_backlog.tail' not described in 'sock'
   include/net/sock.h:509: warning: Function parameter or member 'sk_wq_raw' not described in 'sock'
   include/net/sock.h:509: warning: Function parameter or member 'tcp_rtx_queue' not described in 'sock'
   include/net/sock.h:509: warning: Function parameter or member 'sk_route_forced_caps' not described in 'sock'
   include/net/sock.h:509: warning: Function parameter or member 'sk_txtime_report_errors' not described in 'sock'
   include/net/sock.h:509: warning: Function parameter or member 'sk_validate_xmit_skb' not described in 'sock'
   include/linux/netdevice.h:2052: warning: Function parameter or member 'adj_list.upper' not described in 'net_device'
   include/linux/netdevice.h:2052: warning: Function parameter or member 'adj_list.lower' not described in 'net_device'
   include/linux/netdevice.h:2052: warning: Function parameter or member 'gso_partial_features' not described in 'net_device'
   include/linux/netdevice.h:2052: warning: Function parameter or member 'switchdev_ops' not described in 'net_device'
   include/linux/netdevice.h:2052: warning: Function parameter or member 'l3mdev_ops' not described in 'net_device'
   include/linux/netdevice.h:2052: warning: Function parameter or member 'xfrmdev_ops' not described in 'net_device'
   include/linux/netdevice.h:2052: warning: Function parameter or member 'tlsdev_ops' not described in 'net_device'
   include/linux/netdevice.h:2052: warning: Function parameter or member 'name_assign_type' not described in 'net_device'
   include/linux/netdevice.h:2052: warning: Function parameter or member 'ieee802154_ptr' not described in 'net_device'
   include/linux/netdevice.h:2052: warning: Function parameter or member 'mpls_ptr' not described in 'net_device'
   include/linux/netdevice.h:2052: warning: Function parameter or member 'xdp_prog' not described in 'net_device'
   include/linux/netdevice.h:2052: warning: Function parameter or member 'gro_flush_timeout' not described in 'net_device'
   include/linux/netdevice.h:2052: warning: Function parameter or member 'nf_hooks_ingress' not described in 'net_device'
   include/linux/netdevice.h:2052: warning: Function parameter or member '____cacheline_aligned_in_smp' not described in 'net_device'
   include/linux/netdevice.h:2052: warning: Function parameter or member 'qdisc_hash' not described in 'net_device'
   include/linux/netdevice.h:2052: warning: Function parameter or member 'xps_cpus_map' not described in 'net_device'
   include/linux/netdevice.h:2052: warning: Function parameter or member 'xps_rxqs_map' not described in 'net_device'
   include/linux/phylink.h:56: warning: Function parameter or member '__ETHTOOL_DECLARE_LINK_MODE_MASK(advertising' not described in 'phylink_link_state'
   include/linux/phylink.h:56: warning: Function parameter or member '__ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising' not described in 'phylink_link_state'
   Documentation/admin-guide/cgroup-v2.rst:1507: WARNING: Block quote ends without a blank line; unexpected unindent.
   Documentation/admin-guide/cgroup-v2.rst:1509: WARNING: Block quote ends without a blank line; unexpected unindent.
   Documentation/admin-guide/cgroup-v2.rst:1510: WARNING: Block quote ends without a blank line; unexpected unindent.
   include/net/mac80211.h:1211: ERROR: Unexpected indentation.
   include/net/mac80211.h:1218: WARNING: Block quote ends without a blank line; unexpected unindent.
   include/linux/wait.h:110: WARNING: Block quote ends without a blank line; unexpected unindent.
   include/linux/wait.h:113: ERROR: Unexpected indentation.
   include/linux/wait.h:115: WARNING: Block quote ends without a blank line; unexpected unindent.
   kernel/time/hrtimer.c:1129: WARNING: Block quote ends without a blank line; unexpected unindent.
   kernel/signal.c:344: WARNING: Inline literal start-string without end-string.
   include/linux/kernel.h:137: WARNING: Inline interpreted text or phrase reference start-string without end-string.
   include/uapi/linux/firewire-cdev.h:312: WARNING: Inline literal start-string without end-string.
   Documentation/driver-api/gpio/board.rst:209: ERROR: Unexpected indentation.
   drivers/ata/libata-core.c:5958: ERROR: Unknown target name: "hw".
   drivers/message/fusion/mptbase.c:5057: WARNING: Definition list ends without a blank line; unexpected unindent.
   drivers/tty/serial/serial_core.c:1938: WARNING: Definition list ends without a blank line; unexpected unindent.
   include/linux/mtd/rawnand.h:1189: WARNING: Inline strong start-string without end-string.
   include/linux/mtd/rawnand.h:1191: WARNING: Inline strong start-string without end-string.
   include/linux/regulator/driver.h:286: ERROR: Unknown target name: "regulator_regmap_x_voltage".
   Documentation/driver-api/soundwire/locking.rst:50: ERROR: Inconsistent literal block quoting.
   Documentation/driver-api/soundwire/locking.rst:51: WARNING: Line block ends without a blank line.
   Documentation/driver-api/soundwire/locking.rst:55: WARNING: Inline substitution_reference start-string without end-string.
   Documentation/driver-api/soundwire/locking.rst:56: WARNING: Line block ends without a blank line.
   include/linux/spi/spi.h:365: ERROR: Unexpected indentation.
   Documentation/driver-api/usb/typec_bus.rst:76: WARNING: Definition list ends without a blank line; unexpected unindent.
   block/bio.c:883: WARNING: Inline emphasis start-string without end-string.
   fs/posix_acl.c:635: WARNING: Inline emphasis start-string without end-string.
   drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c:1573: WARNING: Inline emphasis start-string without end-string.
   drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c:1575: WARNING: Inline emphasis start-string without end-string.
   include/drm/drm_drv.h:656: ERROR: Unknown target name: "driver".
   Documentation/laptops/lg-laptop.rst:2: WARNING: Explicit markup ends without a blank line; unexpected unindent.
   Documentation/laptops/lg-laptop.rst:16: ERROR: Unexpected indentation.
   Documentation/laptops/lg-laptop.rst:17: WARNING: Block quote ends without a blank line; unexpected unindent.

vim +1369 include/drm/drm_dp_helper.h

76fa998a Jani Nikula 2017-05-18 @1369  

:::::: The code at line 1369 was first introduced by commit
:::::: 76fa998acd86b6b40d0217e12af39c2406bdcd2b drm/dp: start a DPCD based DP sink/branch device quirk database

:::::: TO: Jani Nikula <jani.nikula@intel.com>
:::::: CC: Jani Nikula <jani.nikula@intel.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 6597 bytes --]

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

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

Re: [PATCH 1/9] drm/i915: Disable PSR in Apple panels

[Souza, Jose]

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

On Tue, 2018-11-27 at 15:38 +0200, Ville Syrjälä wrote:
> On Mon, Nov 26, 2018 at 04:37:02PM -0800, José Roberto de Souza
> wrote:
> > i915 yet don't support PSR in Apple panels, so lets keep it
> > disabled
> > while we work on that.
> > 
> > Fixes: 598c6cfe0690 (drm/i915/psr: Enable PSR1 on gen-9+ HW)
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/drm_dp_helper.c  | 2 ++
> >  drivers/gpu/drm/i915/intel_psr.c | 6 ++++++
> >  include/drm/drm_dp_helper.h      | 1 +
> >  3 files changed, 9 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_dp_helper.c
> > b/drivers/gpu/drm/drm_dp_helper.c
> > index 6d483487f2b4..6b5a19d3e347 100644
> > --- a/drivers/gpu/drm/drm_dp_helper.c
> > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > @@ -1273,6 +1273,8 @@ static const struct dpcd_quirk
> > dpcd_quirk_list[] = {
> >  	{ OUI(0x00, 0x22, 0xb9), DEVICE_ID_ANY, true,
> > BIT(DP_DPCD_QUIRK_CONSTANT_N) },
> >  	/* LG LP140WF6-SPM1 eDP panel */
> >  	{ OUI(0x00, 0x22, 0xb9), DEVICE_ID('s', 'i', 'v', 'a', 'r',
> > 'T'), false, BIT(DP_DPCD_QUIRK_CONSTANT_N) },
> > +	/* Apple panels needs some additional handling to support PSR
> > */
> > +	{ OUI(0x00, 0x10, 0xfa), DEVICE_ID_ANY, false,
> > BIT(DP_DPCD_QUIRK_PSR_NOT_CURRENTLY_SUPPORTED) }
> >  };
> >  
> >  #undef OUI
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 572e626eadff..f5d27a02eb28 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -274,6 +274,12 @@ void intel_psr_init_dpcd(struct intel_dp
> > *intel_dp)
> >  		DRM_DEBUG_KMS("Panel lacks power state control, PSR
> > cannot be enabled\n");
> >  		return;
> >  	}
> > +
> > +	if (drm_dp_has_quirk(&intel_dp->desc,
> > DP_DPCD_QUIRK_PSR_NOT_CURRENTLY_SUPPORTED)) {
> > +		DRM_DEBUG_KMS("PSR support not currently available for
> > this panel\n");
> > +		return;
> > +	}
> > +
> >  	dev_priv->psr.sink_support = true;
> >  	dev_priv->psr.sink_sync_latency =
> >  		intel_dp_get_sink_sync_latency(intel_dp);
> > diff --git a/include/drm/drm_dp_helper.h
> > b/include/drm/drm_dp_helper.h
> > index 3314e91f6eb3..db516c48cda3 100644
> > --- a/include/drm/drm_dp_helper.h
> > +++ b/include/drm/drm_dp_helper.h
> > @@ -1364,6 +1364,7 @@ enum drm_dp_quirk {
> >  	 * to 16 bits. So will give a constant value (0x8000) for
> > compatability.
> >  	 */
> >  	DP_DPCD_QUIRK_CONSTANT_N,
> > +	DP_DPCD_QUIRK_PSR_NOT_CURRENTLY_SUPPORTED,
> 
> Why such a convoluted name? DP_DPCD_QUIRK_NO_PSR?

Okay changing to DP_DPCD_QUIRK_NO_PSR.

> 
> >  };
> >  
> >  /**
> > -- 
> > 2.19.2
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[-- 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

Re: [PATCH 2/9] drm/i915/psr: Don't tell sink that main link will be active while is active PSR2

[Rodrigo Vivi]

On Mon, Nov 26, 2018 at 04:37:03PM -0800, José Roberto de Souza wrote:
> For PSR2 there is no register to tell HW to keep main link enabled
> while PSR2 is active, so don't configure sink DPCD with a
> misleading value.
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index f5d27a02eb28..888e348cc1b4 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -391,12 +391,14 @@ static void intel_psr_enable_sink(struct intel_dp *intel_dp)
>  		drm_dp_dpcd_writeb(&intel_dp->aux, DP_RECEIVER_ALPM_CONFIG,
>  				   DP_ALPM_ENABLE);
>  		dpcd_val |= DP_PSR_ENABLE_PSR2;
> +	} else {
> +		if (dev_priv->psr.link_standby)
> +			dpcd_val |= DP_PSR_MAIN_LINK_ACTIVE;
> +
> +		if (INTEL_GEN(dev_priv) >= 8)
> +			dpcd_val |= DP_PSR_CRC_VERIFICATION;

commit message only mention the link stand-by...
could you please do this in a separated patch?

>  	}
>  
> -	if (dev_priv->psr.link_standby)
> -		dpcd_val |= DP_PSR_MAIN_LINK_ACTIVE;
> -	if (!dev_priv->psr.psr2_enabled && INTEL_GEN(dev_priv) >= 8)
> -		dpcd_val |= DP_PSR_CRC_VERIFICATION;
>  	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, dpcd_val);
>  
>  	drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
> -- 
> 2.19.2
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/9] drm/i915/psr: Don't tell sink that main link will be active while is active PSR2

[Souza, Jose]

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

On Wed, 2018-11-28 at 11:02 -0800, Rodrigo Vivi wrote:
> On Mon, Nov 26, 2018 at 04:37:03PM -0800, José Roberto de Souza
> wrote:
> > For PSR2 there is no register to tell HW to keep main link enabled
> > while PSR2 is active, so don't configure sink DPCD with a
> > misleading value.
> > 
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_psr.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index f5d27a02eb28..888e348cc1b4 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -391,12 +391,14 @@ static void intel_psr_enable_sink(struct
> > intel_dp *intel_dp)
> >  		drm_dp_dpcd_writeb(&intel_dp->aux,
> > DP_RECEIVER_ALPM_CONFIG,
> >  				   DP_ALPM_ENABLE);
> >  		dpcd_val |= DP_PSR_ENABLE_PSR2;
> > +	} else {
> > +		if (dev_priv->psr.link_standby)
> > +			dpcd_val |= DP_PSR_MAIN_LINK_ACTIVE;
> > +
> > +		if (INTEL_GEN(dev_priv) >= 8)
> > +			dpcd_val |= DP_PSR_CRC_VERIFICATION;
> 
> commit message only mention the link stand-by...
> could you please do this in a separated patch?

We were already doing it for PSR1, I just grouped all the PSR1 checks
inside of this else block, so there is no functional change in CRC
verification but I can move it to a separated patch if you want.


> 
> >  	}
> >  
> > -	if (dev_priv->psr.link_standby)
> > -		dpcd_val |= DP_PSR_MAIN_LINK_ACTIVE;
> > -	if (!dev_priv->psr.psr2_enabled && INTEL_GEN(dev_priv) >= 8)
> > -		dpcd_val |= DP_PSR_CRC_VERIFICATION;
> >  	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, dpcd_val);
> >  
> >  	drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER,
> > DP_SET_POWER_D0);
> > -- 
> > 2.19.2
> > 

[-- 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 --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/9] drm/i915/psr: Enable sink to trigger a interruption on PSR2 CRC mismatch

[Rodrigo Vivi]

On Mon, Nov 26, 2018 at 04:37:04PM -0800, José Roberto de Souza wrote:
> eDP spec states 2 different bits to enable sink to trigger a
> interruption when there is a CRC mismatch.
> DP_PSR_CRC_VERIFICATION is for PSR only and
> DP_PSR_IRQ_HPD_WITH_CRC_ERRORS is for PSR2 only.
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 888e348cc1b4..607c3ec41679 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -390,7 +390,7 @@ static void intel_psr_enable_sink(struct intel_dp *intel_dp)
>  	if (dev_priv->psr.psr2_enabled) {
>  		drm_dp_dpcd_writeb(&intel_dp->aux, DP_RECEIVER_ALPM_CONFIG,
>  				   DP_ALPM_ENABLE);
> -		dpcd_val |= DP_PSR_ENABLE_PSR2;
> +		dpcd_val |= DP_PSR_ENABLE_PSR2 | DP_PSR_IRQ_HPD_WITH_CRC_ERRORS;

good catch!


Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>



>  	} else {
>  		if (dev_priv->psr.link_standby)
>  			dpcd_val |= DP_PSR_MAIN_LINK_ACTIVE;
> -- 
> 2.19.2
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/9] drm/i915/icl: Do not change reserved registers related to PSR2

[Rodrigo Vivi]

On Mon, Nov 26, 2018 at 04:37:05PM -0800, José Roberto de Souza wrote:
> For ICL the bit 12 of CHICKEN_TRANS is reserved so we should not
> touch it and as by default VSC_DATA_SEL_SOFTWARE_CONTROL is already
> unset in gen10 + GLK we can just drop it and fix for both gens.
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>

Ok, this patch seems right according to spec.

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

But I wonder now if we need intel_psr_setup_vsc() at all for
platforms different than gen9.

Because description of this bit is:
This field enables the programmable header for the PSR2 VSC packet.

Without the programmable version I would assume display
engine is now responsible for setting header entirely?

Art?

> ---
>  drivers/gpu/drm/i915/intel_psr.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 607c3ec41679..7607a58a6ec0 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -635,17 +635,14 @@ static void intel_psr_enable_source(struct intel_dp *intel_dp,
>  	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
>  		hsw_psr_setup_aux(intel_dp);
>  
> -	if (dev_priv->psr.psr2_enabled) {
> +	if (dev_priv->psr.psr2_enabled && (IS_GEN9(dev_priv) &&
> +					   !IS_GEMINILAKE(dev_priv))) {
>  		i915_reg_t reg = gen9_chicken_trans_reg(dev_priv,
>  							cpu_transcoder);
>  		u32 chicken = I915_READ(reg);
>  
> -		if (IS_GEN9(dev_priv) && !IS_GEMINILAKE(dev_priv))
> -			chicken |= (PSR2_VSC_ENABLE_PROG_HEADER
> -				   | PSR2_ADD_VERTICAL_LINE_COUNT);
> -
> -		else
> -			chicken &= ~VSC_DATA_SEL_SOFTWARE_CONTROL;
> +		chicken |= PSR2_VSC_ENABLE_PROG_HEADER |
> +			   PSR2_ADD_VERTICAL_LINE_COUNT;
>  		I915_WRITE(reg, chicken);
>  	}
>  
> -- 
> 2.19.2
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/9] drm: Add offset of PSR2 SU X granularity value

[Rodrigo Vivi]

On Mon, Nov 26, 2018 at 04:37:06PM -0800, José Roberto de Souza wrote:
> Source is required to comply to sink SU granularity when
> DP_PSR2_SU_GRANULARITY_REQUIRED is set in DP_PSR_CAPS,
> so adding the register here.
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> ---
>  include/drm/drm_dp_helper.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index db516c48cda3..acc7ccfd2044 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -314,6 +314,9 @@
>  # define DP_PSR_SETUP_TIME_SHIFT            1
>  # define DP_PSR2_SU_Y_COORDINATE_REQUIRED   (1 << 4)  /* eDP 1.4a */
>  # define DP_PSR2_SU_GRANULARITY_REQUIRED    (1 << 5)  /* eDP 1.4b */
> +
> +#define DP_PSR2_SU_X_GRANULARITY	    0x072 /* eDP 1.4b */
> +
>  /*
>   * 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,
> -- 
> 2.19.2
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/9] drm/i915/psr: Check if source supports sink specific SU granularity

[Rodrigo Vivi]

On Mon, Nov 26, 2018 at 04:37:07PM -0800, José Roberto de Souza wrote:
> According to eDP spec, sink can required specific selective update
> granularity that source must comply.
> Here caching the value if required and checking source supports it.
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 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  |  1 +
>  drivers/gpu/drm/i915/intel_psr.c | 32 ++++++++++++++++++++++++++++++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index f763b30f98d9..cbcd85af95bf 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -506,6 +506,7 @@ struct i915_psr {
>  	ktime_t last_exit;
>  	bool sink_not_reliable;
>  	bool irq_aux_error;
> +	u16 su_x_granularity;
>  };
>  
>  enum intel_pch {
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 7607a58a6ec0..9215c9052381 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -257,6 +257,21 @@ static u8 intel_dp_get_sink_sync_latency(struct intel_dp *intel_dp)
>  	return val;
>  }
>  
> +static u16 intel_dp_get_su_x_granulartiy(struct intel_dp *intel_dp)
> +{
> +	u16 val = 0;
> +	ssize_t r;
> +
> +	if (!(intel_dp->psr_dpcd[1] & DP_PSR2_SU_GRANULARITY_REQUIRED))
> +		return val;
> +
> +	r = drm_dp_dpcd_read(&intel_dp->aux, DP_PSR2_SU_X_GRANULARITY, &val, 2);
> +	if (r != 2)
> +		DRM_WARN("Unable to read DP_PSR2_SU_X_GRANULARITY\n");
> +
> +	return val;
> +}
> +
>  void intel_psr_init_dpcd(struct intel_dp *intel_dp)
>  {
>  	struct drm_i915_private *dev_priv =
> @@ -311,6 +326,8 @@ void intel_psr_init_dpcd(struct intel_dp *intel_dp)
>  		if (dev_priv->psr.sink_psr2_support) {
>  			dev_priv->psr.colorimetry_support =
>  				intel_dp_get_colorimetry_status(intel_dp);
> +			dev_priv->psr.su_x_granularity =
> +				intel_dp_get_su_x_granulartiy(intel_dp);
>  		}
>  	}
>  }
> @@ -525,6 +542,21 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
>  		return false;
>  	}
>  
> +	if (dev_priv->psr.su_x_granularity) {
> +		/*
> +		 * HW will always send full lines in SU blocks, so X will
> +		 * always be 0 and we only need to check the width to validate
> +		 * horizontal granularity.
> +		 * About vertical granularity HW works by SU blocks starting
> +		 * at each 4 lines with height of 4 lines, what eDP states
> +		 * that sink should support.
> +		 */
> +		if (crtc_hdisplay % dev_priv->psr.su_x_granularity) {
> +			DRM_DEBUG_KMS("PSR2 not enabled, HW can not match sink SU granularity requirement\n");
> +			return false;


I wonder if regardless this bit we still need to do a sort of
check anyway because spec states:

"
Sets the grid pattern granularity in the X direction.
A value of 0 indicates that no X-coordinate granularity requirement exists other than
the standard restrictions, wherein the:
•
 Starting X-coordinate must be evenly divisible by 16
 •
 Rectangle width must be evenly divisible by 4
"

Also, why we are just checking X granularity and not Y? (0074h)

(maybe it would be useful to introduce along with previous patch
that I had just reviewed)

> +		}
> +	}
> +
>  	return true;
>  }
>  
> -- 
> 2.19.2
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 7/9] drm/i915/psr: Rename PSR2 macros to better match meaning

[Rodrigo Vivi]

On Mon, Nov 26, 2018 at 04:37:08PM -0800, José Roberto de Souza wrote:
> The first 8 bits of PSR2_CTL have 2 fields to set frames count, the
> first one is to set how many idle frames PSR2 HW needs to wait before
> enter in deep sleep and the second one it is how many frames(it don't
> need to be idle frames) PSR2 HW will wait before start the PSR
> activation sequence.
> The previous names was really misleading and caused wrong values being
> set so better rename to make it clear.

I honestly prefer the old names for 2 reasons:

- they are shorter
- they follow the exact name we have on spec

> 
> Also taking the oportunity to improve those macros.
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h  | 35 ++++++++++++++++----------------
>  drivers/gpu/drm/i915/intel_psr.c |  7 ++++---
>  2 files changed, 22 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 47baf2fe8f71..73046bb9ec7c 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4203,23 +4203,24 @@ enum {
>  #define   EDP_PSR_DEBUG_MASK_DISP_REG_WRITE    (1 << 16) /* Reserved in ICL+ */
>  #define   EDP_PSR_DEBUG_EXIT_ON_PIXEL_UNDERRUN (1 << 15) /* SKL+ */
>  
> -#define EDP_PSR2_CTL			_MMIO(0x6f900)
> -#define   EDP_PSR2_ENABLE		(1 << 31)
> -#define   EDP_SU_TRACK_ENABLE		(1 << 30)
> -#define   EDP_Y_COORDINATE_VALID	(1 << 26) /* GLK and CNL+ */
> -#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_FRAME_BEFORE_SU_SHIFT 4
> -#define   EDP_PSR2_FRAME_BEFORE_SU_MASK	(0xf << 4)
> -#define   EDP_PSR2_FRAME_BEFORE_SU(a)	((a) << 4)
> -#define   EDP_PSR2_IDLE_FRAME_MASK	0xf
> -#define   EDP_PSR2_IDLE_FRAME_SHIFT	0
> +#define EDP_PSR2_CTL					_MMIO(0x6f900)
> +#define   EDP_PSR2_ENABLE				(1 << 31)
> +#define   EDP_SU_TRACK_ENABLE				(1 << 30)
> +#define   EDP_Y_COORDINATE_VALID			(1 << 26) /* GLK and CNL+ */
> +#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_FRAMES_BEFORE_ACTIVATE_SHIFT		(4)
> +#define   EDP_PSR2_FRAMES_BEFORE_ACTIVATE_MASK		(0xf << EDP_PSR2_FRAMES_BEFORE_ACTIVATE_SHIFT)
> +#define   EDP_PSR2_FRAMES_BEFORE_ACTIVATE(n)		(((n) << EDP_PSR2_FRAMES_BEFORE_ACTIVATE_SHIFT) & EDP_PSR2_FRAMES_BEFORE_ACTIVATE_MASK)
> +#define   EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP_MASK	(0xf)
> +#define   EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP_SHIFT	(0)
> +#define   EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP(n)		(((n) << EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP_SHIFT) & EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP_MASK)
>  
>  #define _PSR_EVENT_TRANS_A			0x60848
>  #define _PSR_EVENT_TRANS_B			0x61848
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 9215c9052381..ba7bbe3f8df2 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -479,6 +479,7 @@ static void hsw_activate_psr1(struct intel_dp *intel_dp)
>  static void hsw_activate_psr2(struct intel_dp *intel_dp)
>  {
>  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> +	struct i915_psr *psr = &dev_priv->psr;
>  	u32 val;
>  
>  	/* Let's use 6 as the minimum to cover all known cases including the
> @@ -486,8 +487,8 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
>  	 */
>  	int idle_frames = max(6, dev_priv->vbt.psr.idle_frames);
>  
> -	idle_frames = max(idle_frames, dev_priv->psr.sink_sync_latency + 1);
> -	val = idle_frames << EDP_PSR2_IDLE_FRAME_SHIFT;
> +	idle_frames = max(idle_frames, psr->sink_sync_latency + 1);
> +	val = EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP(idle_frames);
>  
>  	/* FIXME: selective update is probably totally broken because it doesn't
>  	 * mesh at all with our frontbuffer tracking. And the hw alone isn't
> @@ -496,7 +497,7 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
>  	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
>  		val |= EDP_Y_COORDINATE_ENABLE;
>  
> -	val |= EDP_PSR2_FRAME_BEFORE_SU(dev_priv->psr.sink_sync_latency + 1);
> +	val |= EDP_PSR2_FRAMES_BEFORE_ACTIVATE(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)
> -- 
> 2.19.2
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 8/9] drm/i915/psr: Set the right frames values

[Rodrigo Vivi]

On Mon, Nov 26, 2018 at 04:37:09PM -0800, José Roberto de Souza wrote:
> EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP() was being set with the number of
> frames that it should wait to enter PSR, what is wrong.
> Here it is setting this field with the highest value to avoid PSR2
> exits frequently, as when HW exit deep sleep it needs to go to idle
> state causing a PSR exit for then waiting a few frames before
> activate PSR2 again.
> This will result in more power saving as the sleep state also provide
> some power savings by doing selective updates instead of full screen
> updates.
> 
> About EDP_PSR2_FRAMES_BEFORE_ACTIVATE() it is the number of frames
> (not idle frames) that PSR2 hardware will wait to activate PSR2, so
> lets keep using the sink sync latency.
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index ba7bbe3f8df2..6fd793fec5e9 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -482,13 +482,13 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
>  	struct i915_psr *psr = &dev_priv->psr;
>  	u32 val;
>  
> -	/* Let's use 6 as the minimum to cover all known cases including the
> -	 * off-by-one issue that HW has in some cases.
> +	/* sink_sync_latency of 8 means source has to wait for more than 8
> +	 * frames, we'll go with 9 frames for now
>  	 */
> -	int idle_frames = max(6, dev_priv->vbt.psr.idle_frames);

Too many changes in a single patch that I couldn't understand why we
are removing the minimal of 6 that was our safe net.

> +	val = EDP_PSR2_FRAMES_BEFORE_ACTIVATE(psr->sink_sync_latency + 1);
>  
> -	idle_frames = max(idle_frames, psr->sink_sync_latency + 1);
> -	val = EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP(idle_frames);
> +	/* Avoid deep sleep as much as possible to avoid PSR2 idle state */
> +	val |= EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP(15);
>  
>  	/* FIXME: selective update is probably totally broken because it doesn't
>  	 * mesh at all with our frontbuffer tracking. And the hw alone isn't
> @@ -497,8 +497,6 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
>  	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
>  		val |= EDP_Y_COORDINATE_ENABLE;
>  
> -	val |= EDP_PSR2_FRAMES_BEFORE_ACTIVATE(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)
>  		val |= EDP_PSR2_TP2_TIME_50us;
> -- 
> 2.19.2
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 9/9] drm/i915: Remove old PSR2 FIXME about frontbuffer tracking

[Rodrigo Vivi]

On Mon, Nov 26, 2018 at 04:37:10PM -0800, José Roberto de Souza wrote:
> Our frontbuffer tracking improved over the years + the WA #0884
> helped us keep PSR2 enabled while triggering screen updates when
> necessary so this FIXME is not valid anymore.
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_psr.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 6fd793fec5e9..a1bde8bbd85b 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -490,9 +490,6 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
>  	/* Avoid deep sleep as much as possible to avoid PSR2 idle state */
>  	val |= EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP(15);
>  
> -	/* 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 (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
>  		val |= EDP_Y_COORDINATE_ENABLE;
> -- 
> 2.19.2
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 7/9] drm/i915/psr: Rename PSR2 macros to better match meaning

[Dhinakaran Pandiyan]

On Thu, 2018-11-29 at 15:07 -0800, Rodrigo Vivi wrote:
> On Mon, Nov 26, 2018 at 04:37:08PM -0800, José Roberto de Souza
> wrote:
> > The first 8 bits of PSR2_CTL have 2 fields to set frames count, the
> > first one is to set how many idle frames PSR2 HW needs to wait
> > before
> > enter in deep sleep and the second one it is how many frames(it
> > don't
> > need to be idle frames) PSR2 HW will wait before start the PSR
> > activation sequence.
> > The previous names was really misleading and caused wrong values 
The idea was to setup a conservative configuration for PSR2 until we
were ready to enable the feature and some testing was done. Not sure
why you think the values are wrong.

> > being
> > set so better rename to make it clear.
> 
> I honestly prefer the old names for 2 reasons:
> 
> - they are shorter
> - they follow the exact name we have on spec
+1 for the above reason.

> 
> > 
> > Also taking the oportunity to improve those macros.
> > 
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h  | 35 ++++++++++++++++----------
> > ------
> >  drivers/gpu/drm/i915/intel_psr.c |  7 ++++---
> >  2 files changed, 22 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 47baf2fe8f71..73046bb9ec7c 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -4203,23 +4203,24 @@ enum {
> >  #define   EDP_PSR_DEBUG_MASK_DISP_REG_WRITE    (1 << 16) /*
> > Reserved in ICL+ */
> >  #define   EDP_PSR_DEBUG_EXIT_ON_PIXEL_UNDERRUN (1 << 15) /* SKL+
> > */
> >  
> > -#define EDP_PSR2_CTL			_MMIO(0x6f900)
> > -#define   EDP_PSR2_ENABLE		(1 << 31)
> > -#define   EDP_SU_TRACK_ENABLE		(1 << 30)
> > -#define   EDP_Y_COORDINATE_VALID	(1 << 26) /* GLK and CNL+ */
> > -#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_FRAME_BEFORE_SU_SHIFT 4
> > -#define   EDP_PSR2_FRAME_BEFORE_SU_MASK	(0xf << 4)
> > -#define   EDP_PSR2_FRAME_BEFORE_SU(a)	((a) << 4)
> > -#define   EDP_PSR2_IDLE_FRAME_MASK	0xf
> > -#define   EDP_PSR2_IDLE_FRAME_SHIFT	0
> > +#define EDP_PSR2_CTL					_MMIO(0
> > x6f900)
> > +#define   EDP_PSR2_ENABLE				(1 << 31)
> > +#define   EDP_SU_TRACK_ENABLE				(1 <<
> > 30)
> > +#define   EDP_Y_COORDINATE_VALID			(1 << 26) /*
> > GLK and CNL+ */
> > +#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_FRAMES_BEFORE_ACTIVATE_SHIFT		(4)
> > +#define   EDP_PSR2_FRAMES_BEFORE_ACTIVATE_MASK		(0xf <<
> > EDP_PSR2_FRAMES_BEFORE_ACTIVATE_SHIFT)
> > +#define   EDP_PSR2_FRAMES_BEFORE_ACTIVATE(n)		(((n)
> > << EDP_PSR2_FRAMES_BEFORE_ACTIVATE_SHIFT) &
> > EDP_PSR2_FRAMES_BEFORE_ACTIVATE_MASK)
> > +#define   EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP_MASK	(0xf)
> > +#define   EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP_SHIFT	(0)
> > +#define   EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP(n)		(((n)
> > << EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP_SHIFT) &
> > EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP_MASK)
> >  
> >  #define _PSR_EVENT_TRANS_A			0x60848
> >  #define _PSR_EVENT_TRANS_B			0x61848
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 9215c9052381..ba7bbe3f8df2 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -479,6 +479,7 @@ static void hsw_activate_psr1(struct intel_dp
> > *intel_dp)
> >  static void hsw_activate_psr2(struct intel_dp *intel_dp)
> >  {
> >  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > +	struct i915_psr *psr = &dev_priv->psr;
> >  	u32 val;
> >  
> >  	/* Let's use 6 as the minimum to cover all known cases
> > including the
> > @@ -486,8 +487,8 @@ static void hsw_activate_psr2(struct intel_dp
> > *intel_dp)
> >  	 */
> >  	int idle_frames = max(6, dev_priv->vbt.psr.idle_frames);
> >  
> > -	idle_frames = max(idle_frames, dev_priv->psr.sink_sync_latency
> > + 1);
> > -	val = idle_frames << EDP_PSR2_IDLE_FRAME_SHIFT;
> > +	idle_frames = max(idle_frames, psr->sink_sync_latency + 1);
> > +	val = EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP(idle_frames);
> >  
> >  	/* FIXME: selective update is probably totally broken because
> > it doesn't
> >  	 * mesh at all with our frontbuffer tracking. And the hw alone
> > isn't
> > @@ -496,7 +497,7 @@ static void hsw_activate_psr2(struct intel_dp
> > *intel_dp)
> >  	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> >  		val |= EDP_Y_COORDINATE_ENABLE;
> >  
> > -	val |= EDP_PSR2_FRAME_BEFORE_SU(dev_priv->psr.sink_sync_latency 
> > + 1);
> > +	val |= EDP_PSR2_FRAMES_BEFORE_ACTIVATE(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)
> > -- 
> > 2.19.2
> > 

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

Re: [PATCH 9/9] drm/i915: Remove old PSR2 FIXME about frontbuffer tracking

[Dhinakaran Pandiyan]

On Thu, 2018-11-29 at 15:11 -0800, Rodrigo Vivi wrote:
> On Mon, Nov 26, 2018 at 04:37:10PM -0800, José Roberto de Souza
> wrote:
> > Our frontbuffer tracking improved over the years + the WA #0884
> > helped us keep PSR2 enabled while triggering screen updates when
> > necessary so this FIXME is not valid anymore.
> > 
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Acked-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>


> 
> > ---
> >  drivers/gpu/drm/i915/intel_psr.c | 3 ---
> >  1 file changed, 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 6fd793fec5e9..a1bde8bbd85b 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -490,9 +490,6 @@ static void hsw_activate_psr2(struct intel_dp
> > *intel_dp)
> >  	/* Avoid deep sleep as much as possible to avoid PSR2 idle
> > state */
> >  	val |= EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP(15);
> >  
> > -	/* 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 (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> >  		val |= EDP_Y_COORDINATE_ENABLE;
> > -- 
> > 2.19.2
> > 

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

Re: [PATCH 8/9] drm/i915/psr: Set the right frames values

[Dhinakaran Pandiyan]

On Mon, 2018-11-26 at 16:37 -0800, José Roberto de Souza wrote:
> EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP() was being set with the number of
> frames that it should wait to enter PSR, what is wrong.
> Here it is setting this field with the highest value to avoid PSR2
> exits frequently, as when HW exit deep sleep it needs to go to idle
> state causing a PSR exit for then waiting a few frames before
> activate PSR2 again.
> This will result in more power saving as the sleep state also provide
> some power savings by doing selective updates instead of full screen
> updates.
> 
> About EDP_PSR2_FRAMES_BEFORE_ACTIVATE() it is the number of frames
> (not idle frames) that PSR2 hardware will wait to activate PSR2, so
> lets keep using the sink sync latency.
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index ba7bbe3f8df2..6fd793fec5e9 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -482,13 +482,13 @@ static void hsw_activate_psr2(struct intel_dp
> *intel_dp)
>  	struct i915_psr *psr = &dev_priv->psr;
>  	u32 val;
>  
> -	/* Let's use 6 as the minimum to cover all known cases
> including the
> -	 * off-by-one issue that HW has in some cases.
> +	/* sink_sync_latency of 8 means source has to wait for more
> than 8
> +	 * frames, we'll go with 9 frames for now
>  	 */
> -	int idle_frames = max(6, dev_priv->vbt.psr.idle_frames);
> +	val = EDP_PSR2_FRAMES_BEFORE_ACTIVATE(psr->sink_sync_latency +
> 1);
>  
> -	idle_frames = max(idle_frames, psr->sink_sync_latency + 1);
> -	val = EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP(idle_frames);
> +	/* Avoid deep sleep as much as possible to avoid PSR2 idle
> state */
> +	val |= EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP(15);
Avoid deep sleep as much as possible? Why? We get the best power
savings in deep sleep, why make it harder to achieve that?


> 
>  
>  	/* FIXME: selective update is probably totally broken because
> it doesn't
>  	 * mesh at all with our frontbuffer tracking. And the hw alone
> isn't
> @@ -497,8 +497,6 @@ static void hsw_activate_psr2(struct intel_dp
> *intel_dp)
>  	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
>  		val |= EDP_Y_COORDINATE_ENABLE;
>  
> -	val |= EDP_PSR2_FRAMES_BEFORE_ACTIVATE(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)
>  		val |= EDP_PSR2_TP2_TIME_50us;

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

Re: [PATCH 3/9] drm/i915/psr: Enable sink to trigger a interruption on PSR2 CRC mismatch

[Dhinakaran Pandiyan]

On Thu, 2018-11-29 at 14:04 -0800, Rodrigo Vivi wrote:
> On Mon, Nov 26, 2018 at 04:37:04PM -0800, José Roberto de Souza
> wrote:
> > eDP spec states 2 different bits to enable sink to trigger a
> > interruption when there is a CRC mismatch.
> > DP_PSR_CRC_VERIFICATION is for PSR only and
> > DP_PSR_IRQ_HPD_WITH_CRC_ERRORS is for PSR2 only.
> > 
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_psr.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 888e348cc1b4..607c3ec41679 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -390,7 +390,7 @@ static void intel_psr_enable_sink(struct
> > intel_dp *intel_dp)
> >  	if (dev_priv->psr.psr2_enabled) {
> >  		drm_dp_dpcd_writeb(&intel_dp->aux,
> > DP_RECEIVER_ALPM_CONFIG,
> >  				   DP_ALPM_ENABLE);
> > -		dpcd_val |= DP_PSR_ENABLE_PSR2;
> > +		dpcd_val |= DP_PSR_ENABLE_PSR2 |
> > DP_PSR_IRQ_HPD_WITH_CRC_ERRORS;
> 
> good catch!
> 
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Is there a commit that this patch Fixes?

> 
> 
> 
> >  	} else {
> >  		if (dev_priv->psr.link_standby)
> >  			dpcd_val |= DP_PSR_MAIN_LINK_ACTIVE;
> > -- 
> > 2.19.2
> > 

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

Re: [PATCH 1/9] drm/i915: Disable PSR in Apple panels

[Dhinakaran Pandiyan]

On Tue, 2018-11-27 at 13:55 -0800, Souza, Jose wrote:
> On Tue, 2018-11-27 at 15:38 +0200, Ville Syrjälä wrote:
> > On Mon, Nov 26, 2018 at 04:37:02PM -0800, José Roberto de Souza
> > wrote:
> > > i915 yet don't support PSR in Apple panels, so lets keep
Replace "Apple" with specific model name?

> > > disabled
> > > while we work on that.
> > > 
> > > Fixes: 598c6cfe0690 (drm/i915/psr: Enable PSR1 on gen-9+ HW)
Bugzilla please. Also Cc the bug reporter?


> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_dp_helper.c  | 2 ++
> > >  drivers/gpu/drm/i915/intel_psr.c | 6 ++++++
> > >  include/drm/drm_dp_helper.h      | 1 +
> > >  3 files changed, 9 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_dp_helper.c
> > > b/drivers/gpu/drm/drm_dp_helper.c
> > > index 6d483487f2b4..6b5a19d3e347 100644
> > > --- a/drivers/gpu/drm/drm_dp_helper.c
> > > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > > @@ -1273,6 +1273,8 @@ static const struct dpcd_quirk
> > > dpcd_quirk_list[] = {
> > >  	{ OUI(0x00, 0x22, 0xb9), DEVICE_ID_ANY, true,
> > > BIT(DP_DPCD_QUIRK_CONSTANT_N) },
> > >  	/* LG LP140WF6-SPM1 eDP panel */
> > >  	{ OUI(0x00, 0x22, 0xb9), DEVICE_ID('s', 'i', 'v', 'a', 'r',
> > > 'T'), false, BIT(DP_DPCD_QUIRK_CONSTANT_N) },
> > > +	/* Apple panels needs some additional handling to support PSR
> > > */
> > > +	{ OUI(0x00, 0x10, 0xfa), DEVICE_ID_ANY, false,
> > > BIT(DP_DPCD_QUIRK_PSR_NOT_CURRENTLY_SUPPORTED) }
> > >  };
> > >  
> > >  #undef OUI
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index 572e626eadff..f5d27a02eb28 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -274,6 +274,12 @@ void intel_psr_init_dpcd(struct intel_dp
> > > *intel_dp)
> > >  		DRM_DEBUG_KMS("Panel lacks power state control, PSR
> > > cannot be enabled\n");
> > >  		return;
> > >  	}
> > > +
> > > +	if (drm_dp_has_quirk(&intel_dp->desc,
> > > DP_DPCD_QUIRK_PSR_NOT_CURRENTLY_SUPPORTED)) {
> > > +		DRM_DEBUG_KMS("PSR support not currently available for
> > > this panel\n");
> > > +		return;
> > > +	}
> > > +
> > >  	dev_priv->psr.sink_support = true;
> > >  	dev_priv->psr.sink_sync_latency =
> > >  		intel_dp_get_sink_sync_latency(intel_dp);
> > > diff --git a/include/drm/drm_dp_helper.h
> > > b/include/drm/drm_dp_helper.h
> > > index 3314e91f6eb3..db516c48cda3 100644
> > > --- a/include/drm/drm_dp_helper.h
> > > +++ b/include/drm/drm_dp_helper.h
> > > @@ -1364,6 +1364,7 @@ enum drm_dp_quirk {
> > >  	 * to 16 bits. So will give a constant value (0x8000) for
> > > compatability.
> > >  	 */
> > >  	DP_DPCD_QUIRK_CONSTANT_N,
> > > +	DP_DPCD_QUIRK_PSR_NOT_CURRENTLY_SUPPORTED,
> > 
> > Why such a convoluted name? DP_DPCD_QUIRK_NO_PSR?
> 
> Okay changing to DP_DPCD_QUIRK_NO_PSR.
> 
> > 
> > >  };
> > >  
> > >  /**
> > > -- 
> > > 2.19.2
> > > 
> > > _______________________________________________
> > > 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 4/9] drm/i915/icl: Do not change reserved registers related to PSR2

[Souza, Jose]

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

On Thu, 2018-11-29 at 14:15 -0800, Rodrigo Vivi wrote:
> On Mon, Nov 26, 2018 at 04:37:05PM -0800, José Roberto de Souza
> wrote:
> > For ICL the bit 12 of CHICKEN_TRANS is reserved so we should not
> > touch it and as by default VSC_DATA_SEL_SOFTWARE_CONTROL is already
> > unset in gen10 + GLK we can just drop it and fix for both gens.
> > 
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> 
> Ok, this patch seems right according to spec.
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> But I wonder now if we need intel_psr_setup_vsc() at all for
> platforms different than gen9.
> 
> Because description of this bit is:
> This field enables the programmable header for the PSR2 VSC packet.
> 
> Without the programmable version I would assume display
> engine is now responsible for setting header entirely?

As this was in a chicken register in gen 9 my guess is that it was
fixed on newer gens and as it is required for PSR2 we don't need to
manually enable it in driver but Art could confirm it.

> 
> Art?
> 
> > ---
> >  drivers/gpu/drm/i915/intel_psr.c | 11 ++++-------
> >  1 file changed, 4 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 607c3ec41679..7607a58a6ec0 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -635,17 +635,14 @@ static void intel_psr_enable_source(struct
> > intel_dp *intel_dp,
> >  	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> >  		hsw_psr_setup_aux(intel_dp);
> >  
> > -	if (dev_priv->psr.psr2_enabled) {
> > +	if (dev_priv->psr.psr2_enabled && (IS_GEN9(dev_priv) &&
> > +					   !IS_GEMINILAKE(dev_priv))) {
> >  		i915_reg_t reg = gen9_chicken_trans_reg(dev_priv,
> >  							cpu_transcoder)
> > ;
> >  		u32 chicken = I915_READ(reg);
> >  
> > -		if (IS_GEN9(dev_priv) && !IS_GEMINILAKE(dev_priv))
> > -			chicken |= (PSR2_VSC_ENABLE_PROG_HEADER
> > -				   | PSR2_ADD_VERTICAL_LINE_COUNT);
> > -
> > -		else
> > -			chicken &= ~VSC_DATA_SEL_SOFTWARE_CONTROL;
> > +		chicken |= PSR2_VSC_ENABLE_PROG_HEADER |
> > +			   PSR2_ADD_VERTICAL_LINE_COUNT;
> >  		I915_WRITE(reg, chicken);
> >  	}
> >  
> > -- 
> > 2.19.2
> > 

[-- 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

Re: [PATCH 6/9] drm/i915/psr: Check if source supports sink specific SU granularity

[Souza, Jose]

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

On Thu, 2018-11-29 at 15:03 -0800, Rodrigo Vivi wrote:
> On Mon, Nov 26, 2018 at 04:37:07PM -0800, José Roberto de Souza
> wrote:
> > According to eDP spec, sink can required specific selective update
> > granularity that source must comply.
> > Here caching the value if required and checking source supports it.
> > 
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > 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  |  1 +
> >  drivers/gpu/drm/i915/intel_psr.c | 32
> > ++++++++++++++++++++++++++++++++
> >  2 files changed, 33 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index f763b30f98d9..cbcd85af95bf 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -506,6 +506,7 @@ struct i915_psr {
> >  	ktime_t last_exit;
> >  	bool sink_not_reliable;
> >  	bool irq_aux_error;
> > +	u16 su_x_granularity;
> >  };
> >  
> >  enum intel_pch {
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 7607a58a6ec0..9215c9052381 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -257,6 +257,21 @@ static u8
> > intel_dp_get_sink_sync_latency(struct intel_dp *intel_dp)
> >  	return val;
> >  }
> >  
> > +static u16 intel_dp_get_su_x_granulartiy(struct intel_dp
> > *intel_dp)
> > +{
> > +	u16 val = 0;
> > +	ssize_t r;
> > +
> > +	if (!(intel_dp->psr_dpcd[1] & DP_PSR2_SU_GRANULARITY_REQUIRED))
> > +		return val;
> > +
> > +	r = drm_dp_dpcd_read(&intel_dp->aux, DP_PSR2_SU_X_GRANULARITY,
> > &val, 2);
> > +	if (r != 2)
> > +		DRM_WARN("Unable to read DP_PSR2_SU_X_GRANULARITY\n");
> > +
> > +	return val;
> > +}
> > +
> >  void intel_psr_init_dpcd(struct intel_dp *intel_dp)
> >  {
> >  	struct drm_i915_private *dev_priv =
> > @@ -311,6 +326,8 @@ void intel_psr_init_dpcd(struct intel_dp
> > *intel_dp)
> >  		if (dev_priv->psr.sink_psr2_support) {
> >  			dev_priv->psr.colorimetry_support =
> >  				intel_dp_get_colorimetry_status(intel_d
> > p);
> > +			dev_priv->psr.su_x_granularity =
> > +				intel_dp_get_su_x_granulartiy(intel_dp)
> > ;
> >  		}
> >  	}
> >  }
> > @@ -525,6 +542,21 @@ static bool intel_psr2_config_valid(struct
> > intel_dp *intel_dp,
> >  		return false;
> >  	}
> >  
> > +	if (dev_priv->psr.su_x_granularity) {
> > +		/*
> > +		 * HW will always send full lines in SU blocks, so X
> > will
> > +		 * always be 0 and we only need to check the width to
> > validate
> > +		 * horizontal granularity.
> > +		 * About vertical granularity HW works by SU blocks
> > starting
> > +		 * at each 4 lines with height of 4 lines, what eDP
> > states
> > +		 * that sink should support.
> > +		 */
> > +		if (crtc_hdisplay % dev_priv->psr.su_x_granularity) {
> > +			DRM_DEBUG_KMS("PSR2 not enabled, HW can not
> > match sink SU granularity requirement\n");
> > +			return false;
> 
> I wonder if regardless this bit we still need to do a sort of
> check anyway because spec states:
> 
> "
> Sets the grid pattern granularity in the X direction.
> A value of 0 indicates that no X-coordinate granularity requirement
> exists other than
> the standard restrictions, wherein the:
> •
>  Starting X-coordinate must be evenly divisible by 16
>  •
>  Rectangle width must be evenly divisible by 4
> "

Hum, the x will always be 0 so it will always be divisible by 16 but
the width could not be divisible by 4 in the odd resolutions. I will
add this check, thanks.


> 
> Also, why we are just checking X granularity and not Y? (0074h)
> 
> (maybe it would be useful to introduce along with previous patch
> that I had just reviewed)


It is in the comment above but coping from eDP spec:

A value of 00h, 01h, 02h, or 04h should be supported by the Sink device
to ensure
interoperability with various Source devices. A value of 08h or 10h may
be considered
for system-specific implementations.

Our height is multiple of 4 so we are safe even if sink have a y
granularity set.

> 
> > +		}
> > +	}
> > +
> >  	return true;
> >  }
> >  
> > -- 
> > 2.19.2
> > 

[-- 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 --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 7/9] drm/i915/psr: Rename PSR2 macros to better match meaning

[Souza, Jose]

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

On Thu, 2018-11-29 at 15:25 -0800, Dhinakaran Pandiyan wrote:
> On Thu, 2018-11-29 at 15:07 -0800, Rodrigo Vivi wrote:
> > On Mon, Nov 26, 2018 at 04:37:08PM -0800, José Roberto de Souza
> > wrote:
> > > The first 8 bits of PSR2_CTL have 2 fields to set frames count,
> > > the
> > > first one is to set how many idle frames PSR2 HW needs to wait
> > > before
> > > enter in deep sleep and the second one it is how many frames(it
> > > don't
> > > need to be idle frames) PSR2 HW will wait before start the PSR
> > > activation sequence.
> > > The previous names was really misleading and caused wrong values 
> The idea was to setup a conservative configuration for PSR2 until we
> were ready to enable the feature and some testing was done. Not sure
> why you think the values are wrong.
> 
> > > being
> > > set so better rename to make it clear.
> > 
> > I honestly prefer the old names for 2 reasons:
> > 
> > - they are shorter
> > - they follow the exact name we have on spec
> +1 for the above reason.

Okay droping this patch.

> 
> > > Also taking the oportunity to improve those macros.
> > > 
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_reg.h  | 35 ++++++++++++++++----------
> > > ------
> > >  drivers/gpu/drm/i915/intel_psr.c |  7 ++++---
> > >  2 files changed, 22 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h
> > > index 47baf2fe8f71..73046bb9ec7c 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -4203,23 +4203,24 @@ enum {
> > >  #define   EDP_PSR_DEBUG_MASK_DISP_REG_WRITE    (1 << 16) /*
> > > Reserved in ICL+ */
> > >  #define   EDP_PSR_DEBUG_EXIT_ON_PIXEL_UNDERRUN (1 << 15) /* SKL+
> > > */
> > >  
> > > -#define EDP_PSR2_CTL			_MMIO(0x6f900)
> > > -#define   EDP_PSR2_ENABLE		(1 << 31)
> > > -#define   EDP_SU_TRACK_ENABLE		(1 << 30)
> > > -#define   EDP_Y_COORDINATE_VALID	(1 << 26) /* GLK and CNL+ */
> > > -#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_FRAME_BEFORE_SU_SHIFT 4
> > > -#define   EDP_PSR2_FRAME_BEFORE_SU_MASK	(0xf << 4)
> > > -#define   EDP_PSR2_FRAME_BEFORE_SU(a)	((a) << 4)
> > > -#define   EDP_PSR2_IDLE_FRAME_MASK	0xf
> > > -#define   EDP_PSR2_IDLE_FRAME_SHIFT	0
> > > +#define EDP_PSR2_CTL					_MMIO(0
> > > x6f900)
> > > +#define   EDP_PSR2_ENABLE				(1 <<
> > > 31)
> > > +#define   EDP_SU_TRACK_ENABLE				(1 <<
> > > 30)
> > > +#define   EDP_Y_COORDINATE_VALID			(1 << 26) /*
> > > GLK and CNL+ */
> > > +#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_FRAMES_BEFORE_ACTIVATE_SHIFT		(4)
> > > +#define   EDP_PSR2_FRAMES_BEFORE_ACTIVATE_MASK		(0xf <<
> > > EDP_PSR2_FRAMES_BEFORE_ACTIVATE_SHIFT)
> > > +#define   EDP_PSR2_FRAMES_BEFORE_ACTIVATE(n)		(((n)
> > > << EDP_PSR2_FRAMES_BEFORE_ACTIVATE_SHIFT) &
> > > EDP_PSR2_FRAMES_BEFORE_ACTIVATE_MASK)
> > > +#define   EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP_MASK	(0xf)
> > > +#define   EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP_SHIFT	(0)
> > > +#define   EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP(n)		(((n)
> > > << EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP_SHIFT) &
> > > EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP_MASK)
> > >  
> > >  #define _PSR_EVENT_TRANS_A			0x60848
> > >  #define _PSR_EVENT_TRANS_B			0x61848
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index 9215c9052381..ba7bbe3f8df2 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -479,6 +479,7 @@ static void hsw_activate_psr1(struct intel_dp
> > > *intel_dp)
> > >  static void hsw_activate_psr2(struct intel_dp *intel_dp)
> > >  {
> > >  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > > +	struct i915_psr *psr = &dev_priv->psr;
> > >  	u32 val;
> > >  
> > >  	/* Let's use 6 as the minimum to cover all known cases
> > > including the
> > > @@ -486,8 +487,8 @@ static void hsw_activate_psr2(struct intel_dp
> > > *intel_dp)
> > >  	 */
> > >  	int idle_frames = max(6, dev_priv->vbt.psr.idle_frames);
> > >  
> > > -	idle_frames = max(idle_frames, dev_priv->psr.sink_sync_latency
> > > + 1);
> > > -	val = idle_frames << EDP_PSR2_IDLE_FRAME_SHIFT;
> > > +	idle_frames = max(idle_frames, psr->sink_sync_latency + 1);
> > > +	val = EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP(idle_frames);
> > >  
> > >  	/* FIXME: selective update is probably totally broken because
> > > it doesn't
> > >  	 * mesh at all with our frontbuffer tracking. And the hw alone
> > > isn't
> > > @@ -496,7 +497,7 @@ static void hsw_activate_psr2(struct intel_dp
> > > *intel_dp)
> > >  	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> > >  		val |= EDP_Y_COORDINATE_ENABLE;
> > >  
> > > -	val |= EDP_PSR2_FRAME_BEFORE_SU(dev_priv-
> > > >psr.sink_sync_latency 
> > > + 1);
> > > +	val |= EDP_PSR2_FRAMES_BEFORE_ACTIVATE(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)
> > > -- 
> > > 2.19.2
> > > 

[-- 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

Re: [PATCH 8/9] drm/i915/psr: Set the right frames values

[Souza, Jose]

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

On Thu, 2018-11-29 at 15:10 -0800, Rodrigo Vivi wrote:
> On Mon, Nov 26, 2018 at 04:37:09PM -0800, José Roberto de Souza
> wrote:
> > EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP() was being set with the number
> > of
> > frames that it should wait to enter PSR, what is wrong.
> > Here it is setting this field with the highest value to avoid PSR2
> > exits frequently, as when HW exit deep sleep it needs to go to idle
> > state causing a PSR exit for then waiting a few frames before
> > activate PSR2 again.
> > This will result in more power saving as the sleep state also
> > provide
> > some power savings by doing selective updates instead of full
> > screen
> > updates.
> > 
> > About EDP_PSR2_FRAMES_BEFORE_ACTIVATE() it is the number of frames
> > (not idle frames) that PSR2 hardware will wait to activate PSR2, so
> > lets keep using the sink sync latency.
> > 
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_psr.c | 12 +++++-------
> >  1 file changed, 5 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index ba7bbe3f8df2..6fd793fec5e9 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -482,13 +482,13 @@ static void hsw_activate_psr2(struct intel_dp
> > *intel_dp)
> >  	struct i915_psr *psr = &dev_priv->psr;
> >  	u32 val;
> >  
> > -	/* Let's use 6 as the minimum to cover all known cases
> > including the
> > -	 * off-by-one issue that HW has in some cases.
> > +	/* sink_sync_latency of 8 means source has to wait for more
> > than 8
> > +	 * frames, we'll go with 9 frames for now
> >  	 */
> > -	int idle_frames = max(6, dev_priv->vbt.psr.idle_frames);
> 
> Too many changes in a single patch that I couldn't understand why we
> are removing the minimal of 6 that was our safe net.


The vbt idle_frames should not be used for PSR2 as PSR2 just wait for X
frames(idle or not) to activate PSR2 so just rely in sink sync_latency
should be enough but I can bring it back for safety.

> 
> > +	val = EDP_PSR2_FRAMES_BEFORE_ACTIVATE(psr->sink_sync_latency +
> > 1);
> >  
> > -	idle_frames = max(idle_frames, psr->sink_sync_latency + 1);
> > -	val = EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP(idle_frames);
> > +	/* Avoid deep sleep as much as possible to avoid PSR2 idle
> > state */
> > +	val |= EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP(15);
> >  
> >  	/* FIXME: selective update is probably totally broken because
> > it doesn't
> >  	 * mesh at all with our frontbuffer tracking. And the hw alone
> > isn't
> > @@ -497,8 +497,6 @@ static void hsw_activate_psr2(struct intel_dp
> > *intel_dp)
> >  	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> >  		val |= EDP_Y_COORDINATE_ENABLE;
> >  
> > -	val |= EDP_PSR2_FRAMES_BEFORE_ACTIVATE(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)
> >  		val |= EDP_PSR2_TP2_TIME_50us;
> > -- 
> > 2.19.2
> > 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

[-- 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 --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 8/9] drm/i915/psr: Set the right frames values

[Souza, Jose]

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

On Thu, 2018-11-29 at 15:33 -0800, Dhinakaran Pandiyan wrote:
> On Mon, 2018-11-26 at 16:37 -0800, José Roberto de Souza wrote:
> > EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP() was being set with the number
> > of
> > frames that it should wait to enter PSR, what is wrong.
> > Here it is setting this field with the highest value to avoid PSR2
> > exits frequently, as when HW exit deep sleep it needs to go to idle
> > state causing a PSR exit for then waiting a few frames before
> > activate PSR2 again.
> > This will result in more power saving as the sleep state also
> > provide
> > some power savings by doing selective updates instead of full
> > screen
> > updates.
> > 
> > About EDP_PSR2_FRAMES_BEFORE_ACTIVATE() it is the number of frames
> > (not idle frames) that PSR2 hardware will wait to activate PSR2, so
> > lets keep using the sink sync latency.
> > 
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_psr.c | 12 +++++-------
> >  1 file changed, 5 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index ba7bbe3f8df2..6fd793fec5e9 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -482,13 +482,13 @@ static void hsw_activate_psr2(struct intel_dp
> > *intel_dp)
> >  	struct i915_psr *psr = &dev_priv->psr;
> >  	u32 val;
> >  
> > -	/* Let's use 6 as the minimum to cover all known cases
> > including the
> > -	 * off-by-one issue that HW has in some cases.
> > +	/* sink_sync_latency of 8 means source has to wait for more
> > than 8
> > +	 * frames, we'll go with 9 frames for now
> >  	 */
> > -	int idle_frames = max(6, dev_priv->vbt.psr.idle_frames);
> > +	val = EDP_PSR2_FRAMES_BEFORE_ACTIVATE(psr->sink_sync_latency +
> > 1);
> >  
> > -	idle_frames = max(idle_frames, psr->sink_sync_latency + 1);
> > -	val = EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP(idle_frames);
> > +	/* Avoid deep sleep as much as possible to avoid PSR2 idle
> > state */
> > +	val |= EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP(15);
> Avoid deep sleep as much as possible? Why? We get the best power
> savings in deep sleep, why make it harder to achieve that?

As said in commit message a small frame count to enter in deep sleep
will cause frequent PSR exits and when HW comes back from deep sleep it
needs to go to idle state. So it will need to wait for
EDP_PSR2_FRAMES_BEFORE_ACTIVATE() frames before activate PSR again.

A regular productivity tools(Office and email) user would benefit from
that as the mouse cursor blinking would make PSR2 go from deep sleep to
idle state and stay in idle as long as cursor is blinking. With 15
frames user will stay most of the time in PSR2 sleep state that already
provide some power savings.

> 
> 
> >  
> >  	/* FIXME: selective update is probably totally broken because
> > it doesn't
> >  	 * mesh at all with our frontbuffer tracking. And the hw alone
> > isn't
> > @@ -497,8 +497,6 @@ static void hsw_activate_psr2(struct intel_dp
> > *intel_dp)
> >  	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> >  		val |= EDP_Y_COORDINATE_ENABLE;
> >  
> > -	val |= EDP_PSR2_FRAMES_BEFORE_ACTIVATE(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)
> >  		val |= EDP_PSR2_TP2_TIME_50us;

[-- 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

Re: [PATCH 2/9] drm/i915/psr: Don't tell sink that main link will be active while is active PSR2

[Rodrigo Vivi]

On Wed, Nov 28, 2018 at 12:13:30PM -0800, Souza, Jose wrote:
> On Wed, 2018-11-28 at 11:02 -0800, Rodrigo Vivi wrote:
> > On Mon, Nov 26, 2018 at 04:37:03PM -0800, José Roberto de Souza
> > wrote:
> > > For PSR2 there is no register to tell HW to keep main link enabled
> > > while PSR2 is active, so don't configure sink DPCD with a
> > > misleading value.
> > > 
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_psr.c | 10 ++++++----
> > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index f5d27a02eb28..888e348cc1b4 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -391,12 +391,14 @@ static void intel_psr_enable_sink(struct
> > > intel_dp *intel_dp)
> > >  		drm_dp_dpcd_writeb(&intel_dp->aux,
> > > DP_RECEIVER_ALPM_CONFIG,
> > >  				   DP_ALPM_ENABLE);
> > >  		dpcd_val |= DP_PSR_ENABLE_PSR2;
> > > +	} else {
> > > +		if (dev_priv->psr.link_standby)
> > > +			dpcd_val |= DP_PSR_MAIN_LINK_ACTIVE;
> > > +
> > > +		if (INTEL_GEN(dev_priv) >= 8)
> > > +			dpcd_val |= DP_PSR_CRC_VERIFICATION;
> > 
> > commit message only mention the link stand-by...
> > could you please do this in a separated patch?
> 
> We were already doing it for PSR1, I just grouped all the PSR1 checks
> inside of this else block, so there is no functional change in CRC
> verification but I can move it to a separated patch if you want.

I prefer the separated patch to make things clear.

The other option would be change the commit message and
subject to mention this clean-up here.

but up to you.

> 
> 
> > 
> > >  	}
> > >  
> > > -	if (dev_priv->psr.link_standby)
> > > -		dpcd_val |= DP_PSR_MAIN_LINK_ACTIVE;
> > > -	if (!dev_priv->psr.psr2_enabled && INTEL_GEN(dev_priv) >= 8)
> > > -		dpcd_val |= DP_PSR_CRC_VERIFICATION;
> > >  	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, dpcd_val);
> > >  
> > >  	drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER,
> > > DP_SET_POWER_D0);
> > > -- 
> > > 2.19.2
> > > 


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

Re: [PATCH 8/9] drm/i915/psr: Set the right frames values

[Dhinakaran Pandiyan]

On Thu, 2018-11-29 at 17:00 -0800, Souza, Jose wrote:
> On Thu, 2018-11-29 at 15:33 -0800, Dhinakaran Pandiyan wrote:
> > On Mon, 2018-11-26 at 16:37 -0800, José Roberto de Souza wrote:
> > > EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP() was being set with the
> > > number
> > > of
> > > frames that it should wait to enter PSR, what is wrong.
> > > Here it is setting this field with the highest value to avoid
> > > PSR2
> > > exits frequently, as when HW exit deep sleep it needs to go to
> > > idle
> > > state causing a PSR exit for then waiting a few frames before
> > > activate PSR2 again.
> > > This will result in more power saving as the sleep state also
> > > provide
> > > some power savings by doing selective updates instead of full
> > > screen
> > > updates.
> > > 
> > > About EDP_PSR2_FRAMES_BEFORE_ACTIVATE() it is the number of
> > > frames
> > > (not idle frames) that PSR2 hardware will wait to activate PSR2,
> > > so
> > > lets keep using the sink sync latency.
> > > 
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_psr.c | 12 +++++-------
> > >  1 file changed, 5 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index ba7bbe3f8df2..6fd793fec5e9 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -482,13 +482,13 @@ static void hsw_activate_psr2(struct
> > > intel_dp
> > > *intel_dp)
> > >  	struct i915_psr *psr = &dev_priv->psr;
> > >  	u32 val;
> > >  
> > > -	/* Let's use 6 as the minimum to cover all known cases
> > > including the
> > > -	 * off-by-one issue that HW has in some cases.
> > > +	/* sink_sync_latency of 8 means source has to wait for more
> > > than 8
> > > +	 * frames, we'll go with 9 frames for now
> > >  	 */
> > > -	int idle_frames = max(6, dev_priv->vbt.psr.idle_frames);
> > > +	val = EDP_PSR2_FRAMES_BEFORE_ACTIVATE(psr->sink_sync_latency +
> > > 1);
> > >  
> > > -	idle_frames = max(idle_frames, psr->sink_sync_latency + 1);
> > > -	val = EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP(idle_frames);
> > > +	/* Avoid deep sleep as much as possible to avoid PSR2 idle
> > > state */
> > > +	val |= EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP(15);
> > 
> > Avoid deep sleep as much as possible? Why? We get the best power
> > savings in deep sleep, why make it harder to achieve that?
> 
> As said in commit message a small frame count to enter in deep sleep
> will cause frequent PSR exits and when HW comes back from deep sleep
> it
> needs to go to idle state. So it will need to wait for
> EDP_PSR2_FRAMES_BEFORE_ACTIVATE() frames before activate PSR again.
> 
> A regular productivity tools(Office and email) user would benefit
> from
> that as the mouse cursor blinking would make PSR2 go from deep sleep
> to
> idle state and stay in idle as long as cursor is blinking. With 15
> frames user will stay most of the time in PSR2 sleep state that
> already
> provide some power savings.

Do you have any numbers to justify that not entering deep sleep (just
doing SU) is better than entering deep sleep and exiting?

Even with a blinking cursor at 2 flips/second, there is enough time to
wait for 9 idle frames (max currently), enter deep sleep and exit(~2
frames) between flips.

Why not leave EDP_PSR2_FRAMES_BEFORE_ACTIVATE as it is and reduce
EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP to the minimum? But then again, I'd
like to see some numbers if it's possible.

-DK

> 
> > 
> > 
> > >  
> > >  	/* FIXME: selective update is probably totally broken because
> > > it doesn't
> > >  	 * mesh at all with our frontbuffer tracking. And the hw alone
> > > isn't
> > > @@ -497,8 +497,6 @@ static void hsw_activate_psr2(struct intel_dp
> > > *intel_dp)
> > >  	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> > >  		val |= EDP_Y_COORDINATE_ENABLE;
> > >  
> > > -	val |= EDP_PSR2_FRAMES_BEFORE_ACTIVATE(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)
> > >  		val |= EDP_PSR2_TP2_TIME_50us;

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

Re: [PATCH 8/9] drm/i915/psr: Set the right frames values

[Souza, Jose]

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

On Fri, 2018-11-30 at 11:35 -0800, Dhinakaran Pandiyan wrote:
> On Thu, 2018-11-29 at 17:00 -0800, Souza, Jose wrote:
> > On Thu, 2018-11-29 at 15:33 -0800, Dhinakaran Pandiyan wrote:
> > > On Mon, 2018-11-26 at 16:37 -0800, José Roberto de Souza wrote:
> > > > EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP() was being set with the
> > > > number
> > > > of
> > > > frames that it should wait to enter PSR, what is wrong.
> > > > Here it is setting this field with the highest value to avoid
> > > > PSR2
> > > > exits frequently, as when HW exit deep sleep it needs to go to
> > > > idle
> > > > state causing a PSR exit for then waiting a few frames before
> > > > activate PSR2 again.
> > > > This will result in more power saving as the sleep state also
> > > > provide
> > > > some power savings by doing selective updates instead of full
> > > > screen
> > > > updates.
> > > > 
> > > > About EDP_PSR2_FRAMES_BEFORE_ACTIVATE() it is the number of
> > > > frames
> > > > (not idle frames) that PSR2 hardware will wait to activate
> > > > PSR2,
> > > > so
> > > > lets keep using the sink sync latency.
> > > > 
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_psr.c | 12 +++++-------
> > > >  1 file changed, 5 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > index ba7bbe3f8df2..6fd793fec5e9 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -482,13 +482,13 @@ static void hsw_activate_psr2(struct
> > > > intel_dp
> > > > *intel_dp)
> > > >  	struct i915_psr *psr = &dev_priv->psr;
> > > >  	u32 val;
> > > >  
> > > > -	/* Let's use 6 as the minimum to cover all known cases
> > > > including the
> > > > -	 * off-by-one issue that HW has in some cases.
> > > > +	/* sink_sync_latency of 8 means source has to wait for
> > > > more
> > > > than 8
> > > > +	 * frames, we'll go with 9 frames for now
> > > >  	 */
> > > > -	int idle_frames = max(6, dev_priv-
> > > > >vbt.psr.idle_frames);
> > > > +	val = EDP_PSR2_FRAMES_BEFORE_ACTIVATE(psr-
> > > > >sink_sync_latency +
> > > > 1);
> > > >  
> > > > -	idle_frames = max(idle_frames, psr->sink_sync_latency +
> > > > 1);
> > > > -	val = EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP(idle_frames);
> > > > +	/* Avoid deep sleep as much as possible to avoid PSR2
> > > > idle
> > > > state */
> > > > +	val |= EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP(15);
> > > 
> > > Avoid deep sleep as much as possible? Why? We get the best power
> > > savings in deep sleep, why make it harder to achieve that?
> > 
> > As said in commit message a small frame count to enter in deep
> > sleep
> > will cause frequent PSR exits and when HW comes back from deep
> > sleep
> > it
> > needs to go to idle state. So it will need to wait for
> > EDP_PSR2_FRAMES_BEFORE_ACTIVATE() frames before activate PSR again.
> > 
> > A regular productivity tools(Office and email) user would benefit
> > from
> > that as the mouse cursor blinking would make PSR2 go from deep
> > sleep
> > to
> > idle state and stay in idle as long as cursor is blinking. With 15
> > frames user will stay most of the time in PSR2 sleep state that
> > already
> > provide some power savings.
> 
> Do you have any numbers to justify that not entering deep sleep (just
> doing SU) is better than entering deep sleep and exiting?

I don't have power data, just stimations of how many frames it would
state in each state.

> 
> Even with a blinking cursor at 2 flips/second, there is enough time
> to
> wait for 9 idle frames (max currently), enter deep sleep and exit(~2
> frames) between flips.

2 flips per second is too low, lets take a 4 flips per seconds(user
will be typing faster than 2 keys per second):

# Base
Modeset at 60hz
9 frames to activate PSR2

# With 1 frame to enter deep sleep
36 frames of PSR2 idle
4 frames of sleep
20 frames of deep sleep
+ tranning at every PSR2 exit

# With 15 frames to enter deep sleep
9 frames of PSR2 idle
4 frames of sleep reading memory
47 frames of sleep without read memory
* It would not enter in deep sleep so in the next second it would be
4 frames of sleep reading memory
56 frames of sleep without read memory

And when screen is realy idle like a savings screen it would still go
to deep sleep.

> 
> Why not leave EDP_PSR2_FRAMES_BEFORE_ACTIVATE as it is and reduce
> EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP to the minimum? But then again,
> I'd
> like to see some numbers if it's possible.

Other problem of leaving the number of frames to enter deep sleep to
minumum is that it would almost never do selective update it would be
like a PSR1.


> 
> -DK
> 
> > > 
> > > >  
> > > >  	/* FIXME: selective update is probably totally broken
> > > > because
> > > > it doesn't
> > > >  	 * mesh at all with our frontbuffer tracking. And the
> > > > hw alone
> > > > isn't
> > > > @@ -497,8 +497,6 @@ static void hsw_activate_psr2(struct
> > > > intel_dp
> > > > *intel_dp)
> > > >  	if (INTEL_GEN(dev_priv) >= 10 ||
> > > > IS_GEMINILAKE(dev_priv))
> > > >  		val |= EDP_Y_COORDINATE_ENABLE;
> > > >  
> > > > -	val |= EDP_PSR2_FRAMES_BEFORE_ACTIVATE(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)
> > > >  		val |= EDP_PSR2_TP2_TIME_50us;

[-- 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

Re: [PATCH 4/9] drm/i915/icl: Do not change reserved registers related to PSR2

[Runyan, Arthur J]

I'll check on it.

> -----Original Message-----
> From: Souza, Jose
> Sent: Thursday, 29 November, 2018 3:47 PM
> To: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> Cc: dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org;
> Runyan, Arthur J <arthur.j.runyan@intel.com>; Pandiyan, Dhinakaran
> <dhinakaran.pandiyan@intel.com>
> Subject: Re: [PATCH 4/9] drm/i915/icl: Do not change reserved registers
> related to PSR2
> 
> On Thu, 2018-11-29 at 14:15 -0800, Rodrigo Vivi wrote:
> > On Mon, Nov 26, 2018 at 04:37:05PM -0800, José Roberto de Souza
> > wrote:
> > > For ICL the bit 12 of CHICKEN_TRANS is reserved so we should not
> > > touch it and as by default VSC_DATA_SEL_SOFTWARE_CONTROL is
> already
> > > unset in gen10 + GLK we can just drop it and fix for both gens.
> > >
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> >
> > Ok, this patch seems right according to spec.
> >
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >
> > But I wonder now if we need intel_psr_setup_vsc() at all for
> > platforms different than gen9.
> >
> > Because description of this bit is:
> > This field enables the programmable header for the PSR2 VSC packet.
> >
> > Without the programmable version I would assume display
> > engine is now responsible for setting header entirely?
> 
> As this was in a chicken register in gen 9 my guess is that it was
> fixed on newer gens and as it is required for PSR2 we don't need to
> manually enable it in driver but Art could confirm it.
> 
> >
> > Art?
> >
> > > ---
> > >  drivers/gpu/drm/i915/intel_psr.c | 11 ++++-------
> > >  1 file changed, 4 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index 607c3ec41679..7607a58a6ec0 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -635,17 +635,14 @@ static void intel_psr_enable_source(struct
> > > intel_dp *intel_dp,
> > >  	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> > >  		hsw_psr_setup_aux(intel_dp);
> > >
> > > -	if (dev_priv->psr.psr2_enabled) {
> > > +	if (dev_priv->psr.psr2_enabled && (IS_GEN9(dev_priv) &&
> > > +					   !IS_GEMINILAKE(dev_priv))) {
> > >  		i915_reg_t reg = gen9_chicken_trans_reg(dev_priv,
> > >  							cpu_transcoder)
> > > ;
> > >  		u32 chicken = I915_READ(reg);
> > >
> > > -		if (IS_GEN9(dev_priv) && !IS_GEMINILAKE(dev_priv))
> > > -			chicken |= (PSR2_VSC_ENABLE_PROG_HEADER
> > > -				   | PSR2_ADD_VERTICAL_LINE_COUNT);
> > > -
> > > -		else
> > > -			chicken &= ~VSC_DATA_SEL_SOFTWARE_CONTROL;
> > > +		chicken |= PSR2_VSC_ENABLE_PROG_HEADER |
> > > +			   PSR2_ADD_VERTICAL_LINE_COUNT;
> > >  		I915_WRITE(reg, chicken);
> > >  	}
> > >
> > > --
> > > 2.19.2
> > >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 8/9] drm/i915/psr: Set the right frames values

[Dhinakaran Pandiyan]

On Fri, 2018-11-30 at 13:18 -0800, Souza, Jose wrote:
> On Fri, 2018-11-30 at 11:35 -0800, Dhinakaran Pandiyan wrote:
> > On Thu, 2018-11-29 at 17:00 -0800, Souza, Jose wrote:
> > > On Thu, 2018-11-29 at 15:33 -0800, Dhinakaran Pandiyan wrote:
> > > > On Mon, 2018-11-26 at 16:37 -0800, José Roberto de Souza wrote:
> > > > > EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP() was being set with the
> > > > > number
> > > > > of
> > > > > frames that it should wait to enter PSR, what is wrong.
> > > > > Here it is setting this field with the highest value to avoid
> > > > > PSR2
> > > > > exits frequently, as when HW exit deep sleep it needs to go
> > > > > to
> > > > > idle
> > > > > state causing a PSR exit for then waiting a few frames before
> > > > > activate PSR2 again.
> > > > > This will result in more power saving as the sleep state also
> > > > > provide
> > > > > some power savings by doing selective updates instead of full
> > > > > screen
> > > > > updates.
> > > > > 
> > > > > About EDP_PSR2_FRAMES_BEFORE_ACTIVATE() it is the number of
> > > > > frames
> > > > > (not idle frames) that PSR2 hardware will wait to activate
> > > > > PSR2,
> > > > > so
> > > > > lets keep using the sink sync latency.
> > > > > 
> > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_psr.c | 12 +++++-------
> > > > >  1 file changed, 5 insertions(+), 7 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > > index ba7bbe3f8df2..6fd793fec5e9 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > > @@ -482,13 +482,13 @@ static void hsw_activate_psr2(struct
> > > > > intel_dp
> > > > > *intel_dp)
> > > > >  	struct i915_psr *psr = &dev_priv->psr;
> > > > >  	u32 val;
> > > > >  
> > > > > -	/* Let's use 6 as the minimum to cover all known cases
> > > > > including the
> > > > > -	 * off-by-one issue that HW has in some cases.
> > > > > +	/* sink_sync_latency of 8 means source has to wait for
> > > > > more
> > > > > than 8
> > > > > +	 * frames, we'll go with 9 frames for now
> > > > >  	 */
> > > > > -	int idle_frames = max(6, dev_priv-
> > > > > > vbt.psr.idle_frames);
> > > > > 
> > > > > +	val = EDP_PSR2_FRAMES_BEFORE_ACTIVATE(psr-
> > > > > > sink_sync_latency +
> > > > > 
> > > > > 1);
> > > > >  
> > > > > -	idle_frames = max(idle_frames, psr->sink_sync_latency +
> > > > > 1);
> > > > > -	val = EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP(idle_frames);
> > > > > +	/* Avoid deep sleep as much as possible to avoid PSR2
> > > > > idle
> > > > > state */
> > > > > +	val |= EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP(15);
> > > > 
> > > > Avoid deep sleep as much as possible? Why? We get the best
> > > > power
> > > > savings in deep sleep, why make it harder to achieve that?
> > > 
> > > As said in commit message a small frame count to enter in deep
> > > sleep
> > > will cause frequent PSR exits and when HW comes back from deep
> > > sleep
> > > it
> > > needs to go to idle state. So it will need to wait for
> > > EDP_PSR2_FRAMES_BEFORE_ACTIVATE() frames before activate PSR
> > > again.
> > > 
> > > A regular productivity tools(Office and email) user would benefit
> > > from
> > > that as the mouse cursor blinking would make PSR2 go from deep
> > > sleep
> > > to
> > > idle state and stay in idle as long as cursor is blinking. With
> > > 15
> > > frames user will stay most of the time in PSR2 sleep state that
> > > already
> > > provide some power savings.
> > 
> > Do you have any numbers to justify that not entering deep sleep
> > (just
> > doing SU) is better than entering deep sleep and exiting?
> 
> I don't have power data, just stimations of how many frames it would
> state in each state.
> 
> > 
> > Even with a blinking cursor at 2 flips/second, there is enough time
> > to
> > wait for 9 idle frames (max currently), enter deep sleep and
> > exit(~2
> > frames) between flips.
> 
> 2 flips per second is too low, lets take a 4 flips per seconds(user
> will be typing faster than 2 keys per second):
> 
> # Base
> Modeset at 60hz
> 9 frames to activate PSR2
> 
> # With 1 frame to enter deep sleep
> 36 frames of PSR2 idle
> 4 frames of sleep
> 20 frames of deep sleep
> + tranning at every PSR2 exit
> 
> # With 15 frames to enter deep sleep
> 9 frames of PSR2 idle
> 4 frames of sleep reading memory
> 47 frames of sleep without read memory

We are comparing apples to oranges, I have no idea how much power this
SU sleep state actually saves in comparison to deep sleep. Without any
numbers, I'm not sold on the optimization you are proposing.

-DK

> * It would not enter in deep sleep so in the next second it would be
> 4 frames of sleep reading memory
> 56 frames of sleep without read memory
> 
> And when screen is realy idle like a savings screen it would still go
> to deep sleep.
> 
> > 
> > Why not leave EDP_PSR2_FRAMES_BEFORE_ACTIVATE as it is and reduce
> > EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP to the minimum? But then again,
> > I'd
> > like to see some numbers if it's possible.
> 
> Other problem of leaving the number of frames to enter deep sleep to
> minumum is that it would almost never do selective update it would be
> like a PSR1.
> 
> 
> > 
> > -DK
> > 
> > > > 
> > > > >  
> > > > >  	/* FIXME: selective update is probably totally broken
> > > > > because
> > > > > it doesn't
> > > > >  	 * mesh at all with our frontbuffer tracking. And the
> > > > > hw alone
> > > > > isn't
> > > > > @@ -497,8 +497,6 @@ static void hsw_activate_psr2(struct
> > > > > intel_dp
> > > > > *intel_dp)
> > > > >  	if (INTEL_GEN(dev_priv) >= 10 ||
> > > > > IS_GEMINILAKE(dev_priv))
> > > > >  		val |= EDP_Y_COORDINATE_ENABLE;
> > > > >  
> > > > > -	val |= EDP_PSR2_FRAMES_BEFORE_ACTIVATE(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)
> > > > >  		val |= EDP_PSR2_TP2_TIME_50us;

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