[PATCH v2 01/11] drm/i915: Disable PSR in Apple panels

[PATCH v2 01/11] 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.

v2: Renamed DP_DPCD_QUIRK_PSR_NOT_CURRENTLY_SUPPORTED to
DP_DPCD_QUIRK_NO_PSR (Ville)

Fixes: 598c6cfe0690 (drm/i915/psr: Enable PSR1 on gen-9+ HW)
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
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 2d6c491a0542..b00fd5ced0a0 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_NO_PSR) }
 };
 
 #undef OUI
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 2084784f320d..40ca6cc43cc4 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -278,6 +278,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_NO_PSR)) {
+		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 5736c942c85b..047314ce25d6 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -1365,6 +1365,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_NO_PSR,
 };
 
 /**
-- 
2.19.2

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

[PATCH v2 02/11] 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.

v2: Moving the set of DP_PSR_CRC_VERIFICATION to the else block
of 'if (dev_priv->psr.psr2_enabled)' to another patch. (Rodrigo)

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 | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 40ca6cc43cc4..8515f4a6f4f1 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -395,10 +395,11 @@ 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 (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);
-- 
2.19.2

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

[PATCH v2 03/11] drm/i915/psr: Set PSR CRC verification bit in sink inside PSR1 block

[José Roberto de Souza]

As we have a else block for the 'if (dev_priv->psr.psr2_enabled) {'
and this bit is only set for PSR1 move it to that block to make it
more easy to read.

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 | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 8515f4a6f4f1..b04472e637c8 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -398,10 +398,11 @@ static void intel_psr_enable_sink(struct intel_dp *intel_dp)
 	} 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.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

[PATCH v2 04/11] 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>
Reviewed-by: 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 b04472e637c8..77162c469079 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -394,7 +394,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 v2 05/11] 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>
Reviewed-by: 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 77162c469079..c4a8f476eea9 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -649,17 +649,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 v2 06/11] drm: Add the PSR SU granularity registers offsets

[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 registers offsets.

v2: Also adding DP_PSR2_SU_Y_GRANULARITY(Rodrigo)

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 | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 047314ce25d6..0e04b2db3dde 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -314,6 +314,10 @@
 # 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 */
+#define DP_PSR2_SU_Y_GRANULARITY	    0x074 /* 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 v2 07/11] drm/i915/psr: Check if resolution is supported by default SU granularity

[José Roberto de Souza]

Selective updates have a default granularity requirements as stated
by eDP spec, so check if HW can match those requirements before
enable PSR2.

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 | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index c4a8f476eea9..282ff1bc68a7 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -539,6 +539,18 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
 		return false;
 	}
 
+	/* 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 % 4) {
+		DRM_DEBUG_KMS("PSR2 not enabled, default SU granularity not match\n");
+		return false;
+	}
+
 	return true;
 }
 
-- 
2.19.2

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

[PATCH v2 08/11] 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 if 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 | 21 ++++++++++++++++++++-
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 43ac6873a2bb..0727d8051dd3 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -507,6 +507,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 282ff1bc68a7..f9eccaac850a 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -261,6 +261,23 @@ 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;
+	ssize_t r;
+
+	if (!(intel_dp->psr_dpcd[1] & DP_PSR2_SU_GRANULARITY_REQUIRED)) {
+		/* Returning the default X granularity */
+		return 4;
+	}
+
+	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 =
@@ -315,6 +332,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);
 		}
 	}
 }
@@ -546,7 +565,7 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
 	 * at each 4 lines with height of 4 lines, what eDP states
 	 * that sink should support.
 	 */
-	if (crtc_hdisplay % 4) {
+	if (crtc_hdisplay % dev_priv->psr.su_x_granularity) {
 		DRM_DEBUG_KMS("PSR2 not enabled, default SU granularity not match\n");
 		return false;
 	}
-- 
2.19.2

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

[PATCH v2 09/11] 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.

Acked-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Reviewed-by: 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 f9eccaac850a..0257dbcf9384 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -495,9 +495,6 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
 	idle_frames = max(idle_frames, dev_priv->psr.sink_sync_latency + 1);
 	val = idle_frames << EDP_PSR2_IDLE_FRAME_SHIFT;
 
-	/* FIXME: selective update is probably totally broken because it doesn't
-	 * mesh at all with our frontbuffer tracking. And the hw alone isn't
-	 * good enough. */
 	val |= EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE;
 	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

[PATCH v2 10/11] drm/i915: Improve PSR2 CTL macros

[José Roberto de Souza]

- Reusing the EDP_PSR2_FRAME_BEFORE_SU_SHIFT in EDP_PSR2_FRAME_BEFORE_SU
- Removing unused EDP_PSR2_FRAME_BEFORE_SU_MASK
- Adding EDP_PSR2_FRAME_BEFORE_SU_MAX
- Adding EDP_PSR2_IDLE_FRAME()
- Adding EDP_PSR2_IDLE_FRAME_MAX

In the next patch the new macros will be used.

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/i915_reg.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index d3ef97915455..9e46da5032c0 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4216,10 +4216,11 @@ enum {
 #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_FRAME_BEFORE_SU_MAX	0xf
+#define   EDP_PSR2_FRAME_BEFORE_SU(a)	((a) << EDP_PSR2_FRAME_BEFORE_SU_SHIFT)
 #define   EDP_PSR2_IDLE_FRAME_SHIFT	0
+#define   EDP_PSR2_IDLE_FRAME_MAX	0xf
+#define   EDP_PSR2_IDLE_FRAME(a)	((a) << EDP_PSR2_IDLE_FRAME_SHIFT)
 
 #define _PSR_EVENT_TRANS_A			0x60848
 #define _PSR_EVENT_TRANS_B			0x61848
-- 
2.19.2

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

[PATCH v2 11/11] drm/i915/psr: Set the right frames values

[José Roberto de Souza]

EDP_PSR2_FRAME_BEFORE_SU() is the number of frames that PSR2 HW will
wait before enter in PSR2 activation state, important to note here is
that it will wait for X frames not X idle frames.
So lets reuse the previous approch to get the maximum number of
frames between 6 and sink_sync_latency to enter in PSR2 activation
state and just remove the VBT idle_frames.

And EDP_PSR2_FRAME_BEFORE_SU() is the number of idle frames that
PSR2 HW will wait before enter in PSR2 deep sleep when PSR2 is
active.
Important note here is that HW will need to go to PSR2 idle state
every time it exits PSR2 deep sleep, so avoid as much as possible
deep sleep will provide in overal more power savings as PSR2 sleep
will save some power as memory will not be read in the idle frames
and screen will be partialy updated without exit PSR2.

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, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 0257dbcf9384..36c2eb27ed8d 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -489,18 +489,20 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
 
 	/* 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, so sink_sync_latency + 1.
 	 */
-	int idle_frames = max(6, dev_priv->vbt.psr.idle_frames);
+	val = max(6, dev_priv->psr.sink_sync_latency + 1);
+	val = min_t(u32, val, EDP_PSR2_FRAME_BEFORE_SU_MAX);
+	val = EDP_PSR2_FRAME_BEFORE_SU(val);
 
-	idle_frames = max(idle_frames, dev_priv->psr.sink_sync_latency + 1);
-	val = idle_frames << EDP_PSR2_IDLE_FRAME_SHIFT;
+	/* Avoid deep sleep as much as possible to avoid PSR2 idle state */
+	val |= EDP_PSR2_IDLE_FRAME(EDP_PSR2_IDLE_FRAME_MAX);
 
 	val |= EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE;
 	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);
-
 	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

✗ Fi.CI.SPARSE: warning for series starting with [v2,01/11] drm/i915: Disable PSR in Apple panels

[Patchwork]

== Series Details ==

Series: series starting with [v2,01/11] drm/i915: Disable PSR in Apple panels
URL   : https://patchwork.freedesktop.org/series/53291/
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: Set PSR CRC verification bit in sink inside PSR1 block
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 the PSR SU granularity registers offsets
Okay!

Commit: drm/i915/psr: Check if resolution is supported by default SU granularity
Okay!

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

Commit: drm/i915: Remove old PSR2 FIXME about frontbuffer tracking
-O:drivers/gpu/drm/i915/intel_psr.c:495:23: warning: expression using sizeof(void)
-O:drivers/gpu/drm/i915/intel_psr.c:495:23: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_psr.c:495:23: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_psr.c:495:23: warning: expression using sizeof(void)

Commit: drm/i915: Improve PSR2 CTL macros
Okay!

Commit: drm/i915/psr: Set the right frames values
-O:drivers/gpu/drm/i915/intel_psr.c:493:27: warning: expression using sizeof(void)
-O:drivers/gpu/drm/i915/intel_psr.c:495:23: warning: expression using sizeof(void)
-O:drivers/gpu/drm/i915/intel_psr.c:495:23: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_psr.c:495:15: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_psr.c:496:15: warning: expression using sizeof(void)

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

✓ Fi.CI.BAT: success for series starting with [v2,01/11] drm/i915: Disable PSR in Apple panels

[Patchwork]

== Series Details ==

Series: series starting with [v2,01/11] drm/i915: Disable PSR in Apple panels
URL   : https://patchwork.freedesktop.org/series/53291/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5227 -> Patchwork_10970
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

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

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

### IGT changes ###

#### Possible regressions ####

  * {igt@runner@aborted}:
    - fi-gdg-551:         NOTRUN -> FAIL

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_ctx_create@basic-files:
    - fi-bsw-n3050:       PASS -> FAIL [fdo#108656]

  * igt@i915_selftest@live_sanitycheck:
    - fi-gdg-551:         PASS -> INCOMPLETE [fdo#108789] / [fdo#108799]

  * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a:
    - fi-byt-clapper:     PASS -> FAIL [fdo#107362]

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
    - fi-cfl-8109u:       PASS -> FAIL [fdo#103375]

  
#### Possible fixes ####

  * igt@gem_ctx_create@basic-files:
    - fi-bsw-kefka:       FAIL [fdo#108656] -> PASS

  * igt@kms_frontbuffer_tracking@basic:
    - fi-byt-clapper:     FAIL [fdo#103167] -> 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#103375]: https://bugs.freedesktop.org/show_bug.cgi?id=103375
  [fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
  [fdo#108315]: https://bugs.freedesktop.org/show_bug.cgi?id=108315
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#108656]: https://bugs.freedesktop.org/show_bug.cgi?id=108656
  [fdo#108789]: https://bugs.freedesktop.org/show_bug.cgi?id=108789
  [fdo#108799]: https://bugs.freedesktop.org/show_bug.cgi?id=108799


Participating hosts (50 -> 43)
------------------------------

  Additional (1): fi-glk-j4005 
  Missing    (8): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-icl-y fi-bdw-samus 


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

    * Linux: CI_DRM_5227 -> Patchwork_10970

  CI_DRM_5227: 95052693524067ba66e1a6733355739fbcc8d5b6 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4736: 285ebfb3b7adc56586031afa5150c4e5ad40c229 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10970: f6b9aebc65d056b226fcb35a98df4dc29b6a24b9 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

f6b9aebc65d0 drm/i915/psr: Set the right frames values
1f218a0551bc drm/i915: Improve PSR2 CTL macros
59231e6f0ec9 drm/i915: Remove old PSR2 FIXME about frontbuffer tracking
444694c12dd5 drm/i915/psr: Check if source supports sink specific SU granularity
c95e21227a07 drm/i915/psr: Check if resolution is supported by default SU granularity
c93f4d286ca8 drm: Add the PSR SU granularity registers offsets
25972c915887 drm/i915/icl: Do not change reserved registers related to PSR2
067e97b989a3 drm/i915/psr: Enable sink to trigger a interruption on PSR2 CRC mismatch
0633d5d17f2c drm/i915/psr: Set PSR CRC verification bit in sink inside PSR1 block
3893e24d1033 drm/i915/psr: Don't tell sink that main link will be active while is active PSR2
5eb1e02ad202 drm/i915: Disable PSR in Apple panels

== Logs ==

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

✓ Fi.CI.IGT: success for series starting with [v2,01/11] drm/i915: Disable PSR in Apple panels

[Patchwork]

== Series Details ==

Series: series starting with [v2,01/11] drm/i915: Disable PSR in Apple panels
URL   : https://patchwork.freedesktop.org/series/53291/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5227_full -> Patchwork_10970_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_ppgtt@blt-vs-render-ctx0:
    - shard-skl:          NOTRUN -> TIMEOUT [fdo#108039]

  * igt@gem_ppgtt@blt-vs-render-ctxn:
    - shard-kbl:          PASS -> INCOMPLETE [fdo#103665] / [fdo#106023] / [fdo#106887]

  * igt@i915_suspend@shrink:
    - shard-skl:          NOTRUN -> DMESG-WARN [fdo#108784]

  * igt@kms_chv_cursor_fail@pipe-a-128x128-bottom-edge:
    - shard-skl:          PASS -> FAIL [fdo#104671]

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

  * igt@kms_cursor_crc@cursor-64x64-onscreen:
    - shard-apl:          PASS -> FAIL [fdo#103232] +1

  * igt@kms_cursor_crc@cursor-64x64-random:
    - shard-skl:          PASS -> FAIL [fdo#103232] +1

  * igt@kms_cursor_crc@cursor-64x64-suspend:
    - {shard-iclb}:       NOTRUN -> FAIL [fdo#103232]

  * igt@kms_draw_crc@draw-method-xrgb8888-mmap-wc-untiled:
    - shard-skl:          PASS -> FAIL [fdo#108145]

  * igt@kms_draw_crc@draw-method-xrgb8888-render-ytiled:
    - {shard-iclb}:       PASS -> WARN [fdo#108336]

  * igt@kms_flip@dpms-off-confusion:
    - {shard-iclb}:       PASS -> DMESG-WARN [fdo#107724] +10

  * igt@kms_flip@dpms-vs-vblank-race:
    - shard-skl:          PASS -> FAIL [fdo#103060]

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

  * igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-shrfb-draw-mmap-wc:
    - shard-skl:          PASS -> FAIL [fdo#105682]

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

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

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-spr-indfb-move:
    - {shard-iclb}:       PASS -> FAIL [fdo#103167] +3

  * igt@kms_frontbuffer_tracking@psr-rgb101010-draw-blt:
    - shard-skl:          PASS -> FAIL [fdo#103167] +2

  * igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes:
    - {shard-iclb}:       PASS -> INCOMPLETE [fdo#107713]

  * igt@kms_plane_alpha_blend@pipe-b-alpha-basic:
    - shard-skl:          NOTRUN -> FAIL [fdo#107815] / [fdo#108145]

  * igt@kms_plane_alpha_blend@pipe-b-alpha-opaque-fb:
    - shard-glk:          PASS -> FAIL [fdo#108145] +2

  * igt@kms_plane_alpha_blend@pipe-b-coverage-7efc:
    - shard-skl:          PASS -> FAIL [fdo#107815]

  * igt@kms_plane_multiple@atomic-pipe-c-tiling-y:
    - shard-glk:          PASS -> FAIL [fdo#103166] +1

  * igt@kms_plane_multiple@atomic-pipe-c-tiling-yf:
    - shard-apl:          PASS -> FAIL [fdo#103166] +1

  * igt@kms_rmfb@rmfb-ioctl:
    - {shard-iclb}:       NOTRUN -> DMESG-WARN [fdo#107724] +1

  * igt@kms_rotation_crc@primary-rotation-180:
    - shard-skl:          PASS -> FAIL [fdo#103925] / [fdo#107815]

  * igt@kms_rotation_crc@primary-y-tiled-reflect-x-270:
    - {shard-iclb}:       PASS -> DMESG-WARN [fdo#107724] / [fdo#108336] +1

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

  * igt@pm_backlight@fade_with_suspend:
    - {shard-iclb}:       NOTRUN -> FAIL [fdo#107847]

  * igt@pm_rpm@basic-rte:
    - {shard-iclb}:       PASS -> INCOMPLETE [fdo#108840]

  * igt@pm_rpm@debugfs-forcewake-user:
    - shard-skl:          PASS -> INCOMPLETE [fdo#107807]

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

  * igt@pm_rpm@pm-tiling:
    - {shard-iclb}:       PASS -> INCOMPLETE [fdo#107713] / [fdo#108840]

  * {igt@runner@aborted}:
    - {shard-iclb}:       NOTRUN -> FAIL [fdo#108756]

  
#### Possible fixes ####

  * igt@kms_chv_cursor_fail@pipe-a-128x128-top-edge:
    - {shard-iclb}:       DMESG-WARN [fdo#107724] / [fdo#108336] -> PASS +1

  * igt@kms_cursor_crc@cursor-128x128-random:
    - shard-apl:          FAIL [fdo#103232] -> PASS

  * igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
    - shard-glk:          FAIL [fdo#105363] -> PASS +1

  * igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-indfb-draw-mmap-cpu:
    - {shard-iclb}:       DMESG-FAIL [fdo#107724] -> PASS

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

  * igt@kms_frontbuffer_tracking@psr-1p-primscrn-spr-indfb-draw-pwrite:
    - {shard-iclb}:       FAIL [fdo#103167] -> PASS +2

  * igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes:
    - {shard-iclb}:       INCOMPLETE [fdo#107713] -> PASS

  * igt@kms_plane@plane-position-covered-pipe-c-planes:
    - shard-apl:          FAIL [fdo#103166] -> PASS +3
    - shard-glk:          FAIL [fdo#103166] -> PASS

  * igt@kms_plane_alpha_blend@pipe-c-coverage-7efc:
    - shard-skl:          FAIL [fdo#107815] -> PASS

  * igt@kms_plane_multiple@atomic-pipe-b-tiling-x:
    - {shard-iclb}:       FAIL [fdo#103166] -> PASS +1

  * igt@kms_vblank@pipe-c-query-idle:
    - {shard-iclb}:       DMESG-WARN [fdo#107724] -> PASS +4

  * igt@pm_rpm@dpms-mode-unset-lpsp:
    - shard-skl:          INCOMPLETE [fdo#107807] -> PASS

  
#### Warnings ####

  * igt@i915_suspend@shrink:
    - shard-snb:          DMESG-WARN [fdo#108784] -> INCOMPLETE [fdo#105411] / [fdo#106886]

  * igt@kms_hdmi_inject@inject-audio:
    - {shard-iclb}:       DMESG-FAIL [fdo#107724] -> FAIL [fdo#102370]

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

  [fdo#102370]: https://bugs.freedesktop.org/show_bug.cgi?id=102370
  [fdo#103060]: https://bugs.freedesktop.org/show_bug.cgi?id=103060
  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
  [fdo#103925]: https://bugs.freedesktop.org/show_bug.cgi?id=103925
  [fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
  [fdo#104671]: https://bugs.freedesktop.org/show_bug.cgi?id=104671
  [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
  [fdo#105411]: https://bugs.freedesktop.org/show_bug.cgi?id=105411
  [fdo#105682]: https://bugs.freedesktop.org/show_bug.cgi?id=105682
  [fdo#106023]: https://bugs.freedesktop.org/show_bug.cgi?id=106023
  [fdo#106886]: https://bugs.freedesktop.org/show_bug.cgi?id=106886
  [fdo#106887]: https://bugs.freedesktop.org/show_bug.cgi?id=106887
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#107807]: https://bugs.freedesktop.org/show_bug.cgi?id=107807
  [fdo#107815]: https://bugs.freedesktop.org/show_bug.cgi?id=107815
  [fdo#107847]: https://bugs.freedesktop.org/show_bug.cgi?id=107847
  [fdo#108039]: https://bugs.freedesktop.org/show_bug.cgi?id=108039
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108336]: https://bugs.freedesktop.org/show_bug.cgi?id=108336
  [fdo#108654]: https://bugs.freedesktop.org/show_bug.cgi?id=108654
  [fdo#108756]: https://bugs.freedesktop.org/show_bug.cgi?id=108756
  [fdo#108784]: https://bugs.freedesktop.org/show_bug.cgi?id=108784
  [fdo#108840]: https://bugs.freedesktop.org/show_bug.cgi?id=108840
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912


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

  No changes in participating hosts


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

    * Linux: CI_DRM_5227 -> Patchwork_10970

  CI_DRM_5227: 95052693524067ba66e1a6733355739fbcc8d5b6 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4736: 285ebfb3b7adc56586031afa5150c4e5ad40c229 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10970: f6b9aebc65d056b226fcb35a98df4dc29b6a24b9 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

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

Re: [PATCH v2 01/11] drm/i915: Disable PSR in Apple panels

[Dhinakaran Pandiyan]

On Thu, 2018-11-29 at 18:25 -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.
> 
> v2: Renamed DP_DPCD_QUIRK_PSR_NOT_CURRENTLY_SUPPORTED to
> DP_DPCD_QUIRK_NO_PSR (Ville)
> 
> Fixes: 598c6cfe0690 (drm/i915/psr: Enable PSR1 on gen-9+ HW)
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 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 2d6c491a0542..b00fd5ced0a0 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_NO_PSR) }
>  };
>  
>  #undef OUI
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index 2084784f320d..40ca6cc43cc4 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -278,6 +278,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_NO_PSR)) {
> +		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 5736c942c85b..047314ce25d6 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -1365,6 +1365,7 @@ enum drm_dp_quirk {
>  	 * to 16 bits. So will give a constant value (0x8000) for
> compatability.
>  	 */
>  	DP_DPCD_QUIRK_CONSTANT_N,

nit: Documentation missing here. I guess we need something along the
lines of "PSR not supported" without referring to the specific DP
device. With that,
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>


> +	DP_DPCD_QUIRK_NO_PSR,
>  };
>  
>  /**

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

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

[Dhinakaran Pandiyan]

On Thu, 2018-11-29 at 18:25 -0800, José Roberto de Souza wrote:
> For PSR2 there is no register to tell HW to keep main link enabled
Right, there is no bit in PSR2_CTL
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>


> while PSR2 is active, so don't configure sink DPCD with a
> misleading value.
> 
> v2: Moving the set of DP_PSR_CRC_VERIFICATION to the else block
> of 'if (dev_priv->psr.psr2_enabled)' to another patch. (Rodrigo)
> 
> 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 | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index 40ca6cc43cc4..8515f4a6f4f1 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -395,10 +395,11 @@ 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 (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);

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

Re: [PATCH v2 03/11] drm/i915/psr: Set PSR CRC verification bit in sink inside PSR1 block

[Dhinakaran Pandiyan]

On Thu, 2018-11-29 at 18:25 -0800, José Roberto de Souza wrote:
> As we have a else block for the 'if (dev_priv->psr.psr2_enabled) {'
> and this bit is only set for PSR1 move it to that block to make it
> more easy to read.
> 
> 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 | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index 8515f4a6f4f1..b04472e637c8 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -398,10 +398,11 @@ static void intel_psr_enable_sink(struct
> intel_dp *intel_dp)
>  	} 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.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);

Do we need this DPCD write? The panel should already be awake by this
point, I think it's worth removing it if there's no regression.

Your change in this patch looks good, so
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>




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

Re: [PATCH v2 04/11] drm/i915/psr: Enable sink to trigger a interruption on PSR2 CRC mismatch

[Dhinakaran Pandiyan]

On Thu, 2018-11-29 at 18:25 -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.

With PSR short pulse handling implemented, I think we are ready for
this.

Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Reviewed-by: 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 b04472e637c8..77162c469079 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -394,7 +394,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;

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

Re: [PATCH v2 06/11] drm: Add the PSR SU granularity registers offsets

[Dhinakaran Pandiyan]

On Thu, 2018-11-29 at 18:25 -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 registers offsets.
> 
> v2: Also adding DP_PSR2_SU_Y_GRANULARITY(Rodrigo)
> 
> 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 | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/drm/drm_dp_helper.h
> b/include/drm/drm_dp_helper.h
> index 047314ce25d6..0e04b2db3dde 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -314,6 +314,10 @@
>  # 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 */
> +#define DP_PSR2_SU_Y_GRANULARITY	    0x074 /* eDP 1.4b */
Definitions above use spaces instead of tabs, so it'd have been good to
be consistent. But, there are places in the file where tabs are used
too, so will leave it to you if you want to switch.

> +
Verified against eDP spec 1.4b
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

>  /*
>   * 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,

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

Re: [PATCH v2 07/11] drm/i915/psr: Check if resolution is supported by default SU granularity

[Dhinakaran Pandiyan]

On Thu, 2018-11-29 at 18:25 -0800, José Roberto de Souza wrote:
> Selective updates have a default granularity requirements as stated
> by eDP spec
Needs reference to the location in the spec.

> , so check if HW can match those requirements before
> enable PSR2.
typo: enabling*

> 
> 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 | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index c4a8f476eea9..282ff1bc68a7 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -539,6 +539,18 @@ static bool intel_psr2_config_valid(struct
> intel_dp *intel_dp,
>  		return false;
>  	}
>  
> +	/* HW will always send full lines in SU blocks, so X will
s/X/starting X coordinate

> +	 * 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.
How about rewriting this as -  
"HW sends SU blocks of size four scan lines, which means the starting X
coordinate and Y granularity requirements will always be met. We only
need to validate the SU block width is a multiple of 4."?




> +	 */
> +	if (crtc_hdisplay % 4) {
> +		DRM_DEBUG_KMS("PSR2 not enabled, default SU granularity
> not match\n");
"PSR2 not enabled, hdisplay(%d) not multiple of 4\n"

With nits addressed,

Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

> +		return false;
> +	}
> +
>  	return true;
>  }
>  

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

Re: [PATCH v2 01/11] drm/i915: Disable PSR in Apple panels

[Jani Nikula]

On Thu, 29 Nov 2018, José Roberto de Souza <jose.souza@intel.com> wrote:
> i915 yet don't support PSR in Apple panels, so lets keep it disabled
> while we work on that.
>
> v2: Renamed DP_DPCD_QUIRK_PSR_NOT_CURRENTLY_SUPPORTED to
> DP_DPCD_QUIRK_NO_PSR (Ville)
>
> Fixes: 598c6cfe0690 (drm/i915/psr: Enable PSR1 on gen-9+ HW)
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 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 2d6c491a0542..b00fd5ced0a0 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 */

nitpick "panels needs"

> +	{ OUI(0x00, 0x10, 0xfa), DEVICE_ID_ANY, false, BIT(DP_DPCD_QUIRK_NO_PSR) }
>  };
>  
>  #undef OUI
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 2084784f320d..40ca6cc43cc4 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -278,6 +278,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_NO_PSR)) {
> +		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 5736c942c85b..047314ce25d6 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -1365,6 +1365,7 @@ enum drm_dp_quirk {
>  	 * to 16 bits. So will give a constant value (0x8000) for compatability.
>  	 */
>  	DP_DPCD_QUIRK_CONSTANT_N,

Please add a proper comment here, similar to above
DP_DPCD_QUIRK_CONSTANT_N.

BR,
Jani.

> +	DP_DPCD_QUIRK_NO_PSR,
>  };
>  
>  /**

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

Re: [PATCH v2 01/11] drm/i915: Disable PSR in Apple panels

[Souza, Jose]

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

On Fri, 2018-11-30 at 15:35 -0800, Dhinakaran Pandiyan wrote:
> On Thu, 2018-11-29 at 18:25 -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.
> > 
> > v2: Renamed DP_DPCD_QUIRK_PSR_NOT_CURRENTLY_SUPPORTED to
> > DP_DPCD_QUIRK_NO_PSR (Ville)
> > 
> > Fixes: 598c6cfe0690 (drm/i915/psr: Enable PSR1 on gen-9+ HW)
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 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 2d6c491a0542..b00fd5ced0a0 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_NO_PSR) }
> >  };
> >  
> >  #undef OUI
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 2084784f320d..40ca6cc43cc4 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -278,6 +278,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_NO_PSR)) {
> > +		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 5736c942c85b..047314ce25d6 100644
> > --- a/include/drm/drm_dp_helper.h
> > +++ b/include/drm/drm_dp_helper.h
> > @@ -1365,6 +1365,7 @@ enum drm_dp_quirk {
> >  	 * to 16 bits. So will give a constant value (0x8000) for
> > compatability.
> >  	 */
> >  	DP_DPCD_QUIRK_CONSTANT_N,
> 
> nit: Documentation missing here. I guess we need something along the
> lines of "PSR not supported" without referring to the specific DP
> device. With that,
> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>


Adding here:

	/**
	 * @DP_DPCD_QUIRK_NO_PSR
	 *
	 * The device don't properly support PSR even if reports that
it
	 * supports or driver still need to implement proper handling
for such
	 * device.
	 */


> 
> 
> > +	DP_DPCD_QUIRK_NO_PSR,
> >  };
> >  
> >  /**

[-- 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 v2 03/11] drm/i915/psr: Set PSR CRC verification bit in sink inside PSR1 block

[Souza, Jose]

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

On Fri, 2018-11-30 at 15:54 -0800, Dhinakaran Pandiyan wrote:
> On Thu, 2018-11-29 at 18:25 -0800, José Roberto de Souza wrote:
> > As we have a else block for the 'if (dev_priv->psr.psr2_enabled) {'
> > and this bit is only set for PSR1 move it to that block to make it
> > more easy to read.
> > 
> > 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 | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 8515f4a6f4f1..b04472e637c8 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -398,10 +398,11 @@ static void intel_psr_enable_sink(struct
> > intel_dp *intel_dp)
> >  	} 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.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);
> 
> Do we need this DPCD write? The panel should already be awake by this
> point, I think it's worth removing it if there's no regression.
> 
> Your change in this patch looks good, so
> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> 

Added to the list to things to TODO.

Thanks for the review.

> 
> 
> 

[-- 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 v2 01/11] drm/i915: Disable PSR in Apple panels

[Dhinakaran Pandiyan]

On Fri, 2018-11-30 at 15:35 -0800, Dhinakaran Pandiyan wrote:
> On Thu, 2018-11-29 at 18:25 -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.
> > 
> > v2: Renamed DP_DPCD_QUIRK_PSR_NOT_CURRENTLY_SUPPORTED to
> > DP_DPCD_QUIRK_NO_PSR (Ville)
> > 
> > Fixes: 598c6cfe0690 (drm/i915/psr: Enable PSR1 on gen-9+ HW)
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 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 2d6c491a0542..b00fd5ced0a0 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_NO_PSR) }
> >  };
> >  
> >  #undef OUI
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 2084784f320d..40ca6cc43cc4 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -278,6 +278,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_NO_PSR)) {
> > +		DRM_DEBUG_KMS("PSR support not currently available for
> > this panel\n");
> > +		return;
> > +	}
Another nitpick: While you make other changes, please also move this
above the power state check. Checking for power state control is not
very useful if we are never going to enable PSR on this panel.

> > +
> >  	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 5736c942c85b..047314ce25d6 100644
> > --- a/include/drm/drm_dp_helper.h
> > +++ b/include/drm/drm_dp_helper.h
> > @@ -1365,6 +1365,7 @@ enum drm_dp_quirk {
> >  	 * to 16 bits. So will give a constant value (0x8000) for
> > compatability.
> >  	 */
> >  	DP_DPCD_QUIRK_CONSTANT_N,
> 
> nit: Documentation missing here. I guess we need something along the
> lines of "PSR not supported" without referring to the specific DP
> device. With that,
> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> 
> 
> > +	DP_DPCD_QUIRK_NO_PSR,
> >  };
> >  
> >  /**

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

Re: [PATCH v2 07/11] drm/i915/psr: Check if resolution is supported by default SU granularity

[Souza, Jose]

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

On Fri, 2018-11-30 at 16:37 -0800, Dhinakaran Pandiyan wrote:
> On Thu, 2018-11-29 at 18:25 -0800, José Roberto de Souza wrote:
> > Selective updates have a default granularity requirements as stated
> > by eDP spec
> Needs reference to the location in the spec.

Done

> 
> > , so check if HW can match those requirements before
> > enable PSR2.
> typo: enabling*

Done

> 
> > 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 | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index c4a8f476eea9..282ff1bc68a7 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -539,6 +539,18 @@ static bool intel_psr2_config_valid(struct
> > intel_dp *intel_dp,
> >  		return false;
> >  	}
> >  
> > +	/* HW will always send full lines in SU blocks, so X will
> s/X/starting X coordinate
> 
> > +	 * 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.
> How about rewriting this as -  
> "HW sends SU blocks of size four scan lines, which means the starting
> X
> coordinate and Y granularity requirements will always be met. We only
> need to validate the SU block width is a multiple of 4."?
> 
> 

Sounds better, thanks

> 
> 
> > +	 */
> > +	if (crtc_hdisplay % 4) {
> > +		DRM_DEBUG_KMS("PSR2 not enabled, default SU granularity
> > not match\n");
> "PSR2 not enabled, hdisplay(%d) not multiple of 4\n"

Done.

> 
> With nits addressed,
> 
> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> 

Thanks

> > +		return false;
> > +	}
> > +
> >  	return true;
> >  }
> >  

[-- 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 v2 08/11] drm/i915/psr: Check if source supports sink specific SU granularity

[Dhinakaran Pandiyan]

On Thu, 2018-11-29 at 18:25 -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 if 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 | 21 ++++++++++++++++++++-
>  2 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 43ac6873a2bb..0727d8051dd3 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -507,6 +507,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 282ff1bc68a7..f9eccaac850a 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -261,6 +261,23 @@ 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;
> +	ssize_t r;
> +
> +	if (!(intel_dp->psr_dpcd[1] & DP_PSR2_SU_GRANULARITY_REQUIRED))
> {
> +		/* Returning the default X granularity */
> +		return 4;
> +	}
nit: Braces not needed, you could move the comment a line above.

A value of 0 in this DPCD indicates there is no granularity
requirement, why assume 4? 

> +
> +	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");
Please change this to the warning level that we use elsewhere for aux
failures.

If I'm reading the spec correctly, a value of 0 in this DPCD means the
sink expects a granularity of 4, so returning 0 would be incorrect.

> +
> +	return val;
Assume the default value of 4 if aux read fails (after printing an
error)

> +}
> +
>  void intel_psr_init_dpcd(struct intel_dp *intel_dp)
>  {
>  	struct drm_i915_private *dev_priv =
> @@ -315,6 +332,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)
> ;
>  		}
>  	}
>  }
> @@ -546,7 +565,7 @@ static bool intel_psr2_config_valid(struct
> intel_dp *intel_dp,
>  	 * at each 4 lines with height of 4 lines, what eDP states
>  	 * that sink should support.
>  	 */
> -	if (crtc_hdisplay % 4) {
> +	if (crtc_hdisplay % dev_priv->psr.su_x_granularity) {
>  		DRM_DEBUG_KMS("PSR2 not enabled, default SU granularity
> not match\n");
>  		return false;
>  	}

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

Re: [PATCH v2 08/11] drm/i915/psr: Check if source supports sink specific SU granularity

[Souza, Jose]

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

On Mon, 2018-12-03 at 12:59 -0800, Dhinakaran Pandiyan wrote:
> On Thu, 2018-11-29 at 18:25 -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 if 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 | 21 ++++++++++++++++++++-
> >  2 files changed, 21 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 43ac6873a2bb..0727d8051dd3 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -507,6 +507,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 282ff1bc68a7..f9eccaac850a 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -261,6 +261,23 @@ 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;
> > +	ssize_t r;
> > +
> > +	if (!(intel_dp->psr_dpcd[1] & DP_PSR2_SU_GRANULARITY_REQUIRED))
> > {
> > +		/* Returning the default X granularity */
> > +		return 4;
> > +	}
> nit: Braces not needed, you could move the comment a line above.
> 
> A value of 0 in this DPCD indicates there is no granularity
> requirement, why assume 4? 

Like you said bellow, 4 is the default granularity if this is not set.

> 
> > +
> > +	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");
> Please change this to the warning level that we use elsewhere for aux
> failures.

So changing to DRM_DEBUG_KMS()

> 
> If I'm reading the spec correctly, a value of 0 in this DPCD means
> the
> sink expects a granularity of 4, so returning 0 would be incorrect.
> 
> > +
> > +	return val;
> Assume the default value of 4 if aux read fails (after printing an
> error)
> 

Done


> > +}
> > +
> >  void intel_psr_init_dpcd(struct intel_dp *intel_dp)
> >  {
> >  	struct drm_i915_private *dev_priv =
> > @@ -315,6 +332,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)
> > ;
> >  		}
> >  	}
> >  }
> > @@ -546,7 +565,7 @@ static bool intel_psr2_config_valid(struct
> > intel_dp *intel_dp,
> >  	 * at each 4 lines with height of 4 lines, what eDP states
> >  	 * that sink should support.
> >  	 */
> > -	if (crtc_hdisplay % 4) {
> > +	if (crtc_hdisplay % dev_priv->psr.su_x_granularity) {
> >  		DRM_DEBUG_KMS("PSR2 not enabled, default SU granularity
> > not match\n");
> >  		return false;
> >  	}

[-- 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 v2 01/11] drm/i915: Disable PSR in Apple panels

[Dhinakaran Pandiyan]

On Mon, 2018-12-03 at 12:14 -0800, Souza, Jose wrote:
> On Fri, 2018-11-30 at 15:35 -0800, Dhinakaran Pandiyan wrote:
> > On Thu, 2018-11-29 at 18:25 -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.
> > > 
> > > v2: Renamed DP_DPCD_QUIRK_PSR_NOT_CURRENTLY_SUPPORTED to
> > > DP_DPCD_QUIRK_NO_PSR (Ville)
> > > 
> > > Fixes: 598c6cfe0690 (drm/i915/psr: Enable PSR1 on gen-9+ HW)
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 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 2d6c491a0542..b00fd5ced0a0 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_NO_PSR) }
> > >  };
> > >  
> > >  #undef OUI
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index 2084784f320d..40ca6cc43cc4 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -278,6 +278,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_NO_PSR)) {
> > > +		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 5736c942c85b..047314ce25d6 100644
> > > --- a/include/drm/drm_dp_helper.h
> > > +++ b/include/drm/drm_dp_helper.h
> > > @@ -1365,6 +1365,7 @@ enum drm_dp_quirk {
> > >  	 * to 16 bits. So will give a constant value (0x8000) for
> > > compatability.
> > >  	 */
> > >  	DP_DPCD_QUIRK_CONSTANT_N,
> > 
> > nit: Documentation missing here. I guess we need something along
> > the
> > lines of "PSR not supported" without referring to the specific DP
> > device. With that,
> > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> 
> 
> Adding here:
> 
> 	/**
> 	 * @DP_DPCD_QUIRK_NO_PSR
> 	 *
> 	 * The device don't properly support PSR even if reports that
nit: "device does not support"
> it
> 	 * supports or driver still need to implement proper handling
				    ^needs
> for such
> 	 * device.
> 	 */
> 
> > 
> > 
> > > +	DP_DPCD_QUIRK_NO_PSR,
> > >  };
> > >  
> > >  /**

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

Re: [PATCH v2 01/11] drm/i915: Disable PSR in Apple panels

[Souza, Jose]

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

On Mon, 2018-12-03 at 14:45 -0800, Dhinakaran Pandiyan wrote:
> On Mon, 2018-12-03 at 12:14 -0800, Souza, Jose wrote:
> > On Fri, 2018-11-30 at 15:35 -0800, Dhinakaran Pandiyan wrote:
> > > On Thu, 2018-11-29 at 18:25 -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.
> > > > 
> > > > v2: Renamed DP_DPCD_QUIRK_PSR_NOT_CURRENTLY_SUPPORTED to
> > > > DP_DPCD_QUIRK_NO_PSR (Ville)
> > > > 
> > > > Fixes: 598c6cfe0690 (drm/i915/psr: Enable PSR1 on gen-9+ HW)
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 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 2d6c491a0542..b00fd5ced0a0 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_NO_PSR) }
> > > >  };
> > > >  
> > > >  #undef OUI
> > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > index 2084784f320d..40ca6cc43cc4 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -278,6 +278,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_NO_PSR)) {
> > > > +		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 5736c942c85b..047314ce25d6 100644
> > > > --- a/include/drm/drm_dp_helper.h
> > > > +++ b/include/drm/drm_dp_helper.h
> > > > @@ -1365,6 +1365,7 @@ enum drm_dp_quirk {
> > > >  	 * to 16 bits. So will give a constant value (0x8000)
> > > > for
> > > > compatability.
> > > >  	 */
> > > >  	DP_DPCD_QUIRK_CONSTANT_N,
> > > 
> > > nit: Documentation missing here. I guess we need something along
> > > the
> > > lines of "PSR not supported" without referring to the specific DP
> > > device. With that,
> > > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > 
> > Adding here:
> > 
> > 	/**
> > 	 * @DP_DPCD_QUIRK_NO_PSR
> > 	 *
> > 	 * The device don't properly support PSR even if reports that
> nit: "device does not support"

Thanks

> > it
> > 	 * supports or driver still need to implement proper handling
> 				    ^needs
> > for such
> > 	 * device.
> > 	 */
> > 
> > > 
> > > > +	DP_DPCD_QUIRK_NO_PSR,
> > > >  };
> > > >  
> > > >  /**

[-- 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 v2 10/11] drm/i915: Improve PSR2 CTL macros

[Dhinakaran Pandiyan]

On Thu, 2018-11-29 at 18:25 -0800, José Roberto de Souza wrote:
> - Reusing the EDP_PSR2_FRAME_BEFORE_SU_SHIFT in
> EDP_PSR2_FRAME_BEFORE_SU
> - Removing unused EDP_PSR2_FRAME_BEFORE_SU_MASK
> - Adding EDP_PSR2_FRAME_BEFORE_SU_MAX
> - Adding EDP_PSR2_IDLE_FRAME()
> - Adding EDP_PSR2_IDLE_FRAME_MAX
> 
> In the next patch the new macros will be used.
> 
> 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/i915_reg.h | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> index d3ef97915455..9e46da5032c0 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4216,10 +4216,11 @@ enum {
>  #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)
_MASKs are useful when we want to read back the register for debugging.
So, I'm not fully convinced this is an improvement.

Rodrigo, your thoughts on this?


> -#define   EDP_PSR2_FRAME_BEFORE_SU(a)	((a) << 4)
> -#define   EDP_PSR2_IDLE_FRAME_MASK	0xf
> +#define   EDP_PSR2_FRAME_BEFORE_SU_MAX	0xf
> +#define   EDP_PSR2_FRAME_BEFORE_SU(a)	((a) <<
> EDP_PSR2_FRAME_BEFORE_SU_SHIFT)
>  #define   EDP_PSR2_IDLE_FRAME_SHIFT	0
> +#define   EDP_PSR2_IDLE_FRAME_MAX	0xf
Not sure if this is better than re-using _MASK here. I'm sure there are
places in the driver where we already do that.

> +#define   EDP_PSR2_IDLE_FRAME(a)	((a) <<
> EDP_PSR2_IDLE_FRAME_SHIFT)
>  
>  #define _PSR_EVENT_TRANS_A			0x60848
>  #define _PSR_EVENT_TRANS_B			0x61848

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

Re: [PATCH v2 10/11] drm/i915: Improve PSR2 CTL macros

[Rodrigo Vivi]

On Mon, Dec 03, 2018 at 03:03:52PM -0800, Dhinakaran Pandiyan wrote:
> On Thu, 2018-11-29 at 18:25 -0800, José Roberto de Souza wrote:
> > - Reusing the EDP_PSR2_FRAME_BEFORE_SU_SHIFT in
> > EDP_PSR2_FRAME_BEFORE_SU
> > - Removing unused EDP_PSR2_FRAME_BEFORE_SU_MASK
> > - Adding EDP_PSR2_FRAME_BEFORE_SU_MAX
> > - Adding EDP_PSR2_IDLE_FRAME()
> > - Adding EDP_PSR2_IDLE_FRAME_MAX
> > 
> > In the next patch the new macros will be used.
> > 
> > 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/i915_reg.h | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index d3ef97915455..9e46da5032c0 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -4216,10 +4216,11 @@ enum {
> >  #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)
> _MASKs are useful when we want to read back the register for debugging.
> So, I'm not fully convinced this is an improvement.
> 
> Rodrigo, your thoughts on this?

I agree with DK.

But I also don't mind removing if we aren't using.

> 
> 
> > -#define   EDP_PSR2_FRAME_BEFORE_SU(a)	((a) << 4)
> > -#define   EDP_PSR2_IDLE_FRAME_MASK	0xf
> > +#define   EDP_PSR2_FRAME_BEFORE_SU_MAX	0xf
> > +#define   EDP_PSR2_FRAME_BEFORE_SU(a)	((a) <<
> > EDP_PSR2_FRAME_BEFORE_SU_SHIFT)
> >  #define   EDP_PSR2_IDLE_FRAME_SHIFT	0
> > +#define   EDP_PSR2_IDLE_FRAME_MAX	0xf
> Not sure if this is better than re-using _MASK here. I'm sure there are
> places in the driver where we already do that.

I think it depends on the use actually...

> 
> > +#define   EDP_PSR2_IDLE_FRAME(a)	((a) <<
> > EDP_PSR2_IDLE_FRAME_SHIFT)
> >  
> >  #define _PSR_EVENT_TRANS_A			0x60848
> >  #define _PSR_EVENT_TRANS_B			0x61848
> 
> _______________________________________________
> 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 v2 08/11] drm/i915/psr: Check if source supports sink specific SU granularity

[Pandiyan, Dhinakaran]

On Mon, 2018-12-03 at 14:45 -0800, Souza, Jose wrote:
> On Mon, 2018-12-03 at 12:59 -0800, Dhinakaran Pandiyan wrote:
> > On Thu, 2018-11-29 at 18:25 -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 if 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 | 21 ++++++++++++++++++++-
> > >  2 files changed, 21 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index 43ac6873a2bb..0727d8051dd3 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -507,6 +507,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 282ff1bc68a7..f9eccaac850a 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -261,6 +261,23 @@ 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;
> > > +	ssize_t r;
> > > +
> > > +	if (!(intel_dp->psr_dpcd[1] & DP_PSR2_SU_GRANULARITY_REQUIRED))
> > > {
> > > +		/* Returning the default X granularity */
> > > +		return 4;
> > > +	}
> > 
> > nit: Braces not needed, you could move the comment a line above.
> > 
> > A value of 0 in this DPCD indicates there is no granularity
> > requirement, why assume 4? 
> 
> Like you said bellow, 4 is the default granularity if this is not
> set.
> 
The spec states 0 means no granularity requirements, return 1 here?

> > 
> > > +
> > > +	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");
> > 
> > Please change this to the warning level that we use elsewhere for
> > aux
> > failures.
> 
> So changing to DRM_DEBUG_KMS()
> 
> > 
> > If I'm reading the spec correctly, a value of 0 in this DPCD means
> > the
> > sink expects a granularity of 4, so returning 0 would be incorrect.
> > 
> > > +
> > > +	return val;
> > 
> > Assume the default value of 4 if aux read fails (after printing an
> > error)
> > 
> 
> Done
> 
> 
> > > +}
> > > +
> > >  void intel_psr_init_dpcd(struct intel_dp *intel_dp)
> > >  {
> > >  	struct drm_i915_private *dev_priv =
> > > @@ -315,6 +332,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)
> > > ;
> > >  		}
> > >  	}
> > >  }
> > > @@ -546,7 +565,7 @@ static bool intel_psr2_config_valid(struct
> > > intel_dp *intel_dp,
> > >  	 * at each 4 lines with height of 4 lines, what eDP states
> > >  	 * that sink should support.
> > >  	 */
> > > -	if (crtc_hdisplay % 4) {
> > > +	if (crtc_hdisplay % dev_priv->psr.su_x_granularity) {
> > >  		DRM_DEBUG_KMS("PSR2 not enabled, default SU granularity
> > > not match\n");
> > >  		return false;
> > >  	}
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 08/11] drm/i915/psr: Check if source supports sink specific SU granularity

[Souza, Jose]

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

On Mon, 2018-12-03 at 15:12 -0800, Pandiyan, Dhinakaran wrote:
> On Mon, 2018-12-03 at 14:45 -0800, Souza, Jose wrote:
> > On Mon, 2018-12-03 at 12:59 -0800, Dhinakaran Pandiyan wrote:
> > > On Thu, 2018-11-29 at 18:25 -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 if 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 | 21 ++++++++++++++++++++-
> > > >  2 files changed, 21 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > index 43ac6873a2bb..0727d8051dd3 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -507,6 +507,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 282ff1bc68a7..f9eccaac850a 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -261,6 +261,23 @@ 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;
> > > > +	ssize_t r;
> > > > +
> > > > +	if (!(intel_dp->psr_dpcd[1] &
> > > > DP_PSR2_SU_GRANULARITY_REQUIRED))
> > > > {
> > > > +		/* Returning the default X granularity */
> > > > +		return 4;
> > > > +	}
> > > 
> > > nit: Braces not needed, you could move the comment a line above.
> > > 
> > > A value of 0 in this DPCD indicates there is no granularity
> > > requirement, why assume 4? 
> > 
> > Like you said bellow, 4 is the default granularity if this is not
> > set.
> > 
> The spec states 0 means no granularity requirements, return 1 here?


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

> 
> > > > +
> > > > +	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");
> > > 
> > > Please change this to the warning level that we use elsewhere for
> > > aux
> > > failures.
> > 
> > So changing to DRM_DEBUG_KMS()
> > 
> > > If I'm reading the spec correctly, a value of 0 in this DPCD
> > > means
> > > the
> > > sink expects a granularity of 4, so returning 0 would be
> > > incorrect.
> > > 
> > > > +
> > > > +	return val;
> > > 
> > > Assume the default value of 4 if aux read fails (after printing
> > > an
> > > error)
> > > 
> > 
> > Done
> > 
> > 
> > > > +}
> > > > +
> > > >  void intel_psr_init_dpcd(struct intel_dp *intel_dp)
> > > >  {
> > > >  	struct drm_i915_private *dev_priv =
> > > > @@ -315,6 +332,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(i
> > > > ntel_dp)
> > > > ;
> > > >  		}
> > > >  	}
> > > >  }
> > > > @@ -546,7 +565,7 @@ static bool intel_psr2_config_valid(struct
> > > > intel_dp *intel_dp,
> > > >  	 * at each 4 lines with height of 4 lines, what eDP
> > > > states
> > > >  	 * that sink should support.
> > > >  	 */
> > > > -	if (crtc_hdisplay % 4) {
> > > > +	if (crtc_hdisplay % dev_priv->psr.su_x_granularity) {
> > > >  		DRM_DEBUG_KMS("PSR2 not enabled, default SU
> > > > granularity
> > > > not match\n");
> > > >  		return false;
> > > >  	}

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