[PATCH 1/7] drm/i915: Avoid a full port detection in the first eDP short pulse

[PATCH 1/7] drm/i915: Avoid a full port detection in the first eDP short pulse

[José Roberto de Souza]

Some eDP panels do not set a valid sink count value and even for the
ones that sets is should always be one for eDP, that is why it is not
cached in intel_edp_init_dpcd().

But intel_dp_short_pulse() compares the old count with the read one
if there is a mistmatch a full port detection will be executed, what
was happening in the first short pulse interruption of eDP panels
that sets sink count.

Instead of just skip the compasison for eDP panels, lets not read
the sink count at all for eDP.

v2: the previous version of this patch was caching the sink count
in intel_edp_init_dpcd() but I was pointed out by Ville a patch that
handled a case of a eDP panel that do not set sink count

Cc: Ville Syrjälä <ville.syrjala@linux.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_dp.c | 44 +++++++++++++++++++--------------
 1 file changed, 26 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 2b090609bee2..577c166f6483 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3936,8 +3936,6 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp)
 static bool
 intel_dp_get_dpcd(struct intel_dp *intel_dp)
 {
-	u8 sink_count;
-
 	if (!intel_dp_read_dpcd(intel_dp))
 		return false;
 
@@ -3947,25 +3945,35 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 		intel_dp_set_common_rates(intel_dp);
 	}
 
-	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_SINK_COUNT, &sink_count) <= 0)
-		return false;
-
 	/*
-	 * Sink count can change between short pulse hpd hence
-	 * a member variable in intel_dp will track any changes
-	 * between short pulse interrupts.
+	 * Some eDP panels do not set a valid value for sink count, that is why
+	 * it don't care about read it here and in intel_edp_init_dpcd().
 	 */
-	intel_dp->sink_count = DP_GET_SINK_COUNT(sink_count);
+	if (!intel_dp_is_edp(intel_dp)) {
+		u8 count;
+		ssize_t r;
 
-	/*
-	 * SINK_COUNT == 0 and DOWNSTREAM_PORT_PRESENT == 1 implies that
-	 * a dongle is present but no display. Unless we require to know
-	 * if a dongle is present or not, we don't need to update
-	 * downstream port information. So, an early return here saves
-	 * time from performing other operations which are not required.
-	 */
-	if (!intel_dp_is_edp(intel_dp) && !intel_dp->sink_count)
-		return false;
+		r = drm_dp_dpcd_readb(&intel_dp->aux, DP_SINK_COUNT, &count);
+		if (r < 1)
+			return false;
+
+		/*
+		 * Sink count can change between short pulse hpd hence
+		 * a member variable in intel_dp will track any changes
+		 * between short pulse interrupts.
+		 */
+		intel_dp->sink_count = DP_GET_SINK_COUNT(count);
+
+		/*
+		 * SINK_COUNT == 0 and DOWNSTREAM_PORT_PRESENT == 1 implies that
+		 * a dongle is present but no display. Unless we require to know
+		 * if a dongle is present or not, we don't need to update
+		 * downstream port information. So, an early return here saves
+		 * time from performing other operations which are not required.
+		 */
+		if (!intel_dp->sink_count)
+			return false;
+	}
 
 	if (!drm_dp_is_branch(intel_dp->dpcd))
 		return true; /* native DP sink */
-- 
2.19.1

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

[PATCH 2/7] drm/i915: Check PSR errors instead of retrain while PSR is enabled

[José Roberto de Souza]

When a PSR error happens sink sets the PSR errors register and also
set the link to a error status.
So in the short pulse handling it was returning earlier and doing a
full detection and attempting to retrain but it fails as PSR HW is
in change of the main-link.

Just call intel_psr_short_pulse() before
intel_dp_needs_link_retrain() is not the right fix as
intel_dp_needs_link_retrain() would return true and trigger a full
detection while PSR HW is still in change of main-link.

Check for PSR active is also not safe as it could be inactive due a
frontbuffer invalidate and still doing the PSR exit sequence.

v3: added comment in intel_dp_needs_link_retrain()

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  | 11 +++++++++++
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 drivers/gpu/drm/i915/intel_psr.c | 15 +++++++++++++++
 3 files changed, 27 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 577c166f6483..158c6f25d2e2 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4383,6 +4383,17 @@ intel_dp_needs_link_retrain(struct intel_dp *intel_dp)
 	if (!intel_dp->link_trained)
 		return false;
 
+	/*
+	 * While PSR source HW is enabled, it will control main-link sending
+	 * frames, enabling and disabling it so trying to do a retrain will fail
+	 * as the link would or not be on or it could mix training patterns
+	 * and frame data at the same time causing retrain to fail.
+	 * Also when exiting PSR, HW will retrain the link anyways fixing
+	 * any link status error.
+	 */
+	if (intel_psr_enabled(intel_dp))
+		return false;
+
 	if (!intel_dp_get_link_status(intel_dp, link_status))
 		return false;
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index cc7fab2b61f4..12f96297299e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -2046,6 +2046,7 @@ void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir);
 void intel_psr_short_pulse(struct intel_dp *intel_dp);
 int intel_psr_wait_for_idle(const struct intel_crtc_state *new_crtc_state,
 			    u32 *out_value);
+bool intel_psr_enabled(struct intel_dp *intel_dp);
 
 /* intel_quirks.c */
 void intel_init_quirks(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 48df16a02fac..f940305b72e4 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -1107,3 +1107,18 @@ void intel_psr_short_pulse(struct intel_dp *intel_dp)
 exit:
 	mutex_unlock(&psr->lock);
 }
+
+bool intel_psr_enabled(struct intel_dp *intel_dp)
+{
+	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
+	bool ret;
+
+	if (!CAN_PSR(dev_priv) || !intel_dp_is_edp(intel_dp))
+		return false;
+
+	mutex_lock(&dev_priv->psr.lock);
+	ret = (dev_priv->psr.dp == intel_dp && dev_priv->psr.enabled);
+	mutex_unlock(&dev_priv->psr.lock);
+
+	return ret;
+}
-- 
2.19.1

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

[PATCH 3/7] drm/i915: Do not enable PSR in the next modeset after a error

[José Roberto de Souza]

When we detect a error and disable PSR, it is kept disable until the
next modeset but as the sink already show signs that it do not
properly work with PSR lets disabled it for good to avoid any
additional flickering.

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 | 10 +++++++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 08d25aa480f7..e13222518c1b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -641,6 +641,7 @@ struct i915_psr {
 	u8 sink_sync_latency;
 	ktime_t last_entry_attempt;
 	ktime_t last_exit;
+	bool sink_not_reliable;
 };
 
 enum intel_pch {
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index f940305b72e4..cc738497d551 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -508,6 +508,11 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
 		return;
 	}
 
+	if (dev_priv->psr.sink_not_reliable) {
+		DRM_DEBUG_KMS("Sink not reliable set\n");
+		return;
+	}
+
 	if (IS_HASWELL(dev_priv) &&
 	    I915_READ(HSW_STEREO_3D_CTL(crtc_state->cpu_transcoder)) &
 		      S3D_ENABLE) {
@@ -1083,6 +1088,7 @@ void intel_psr_short_pulse(struct intel_dp *intel_dp)
 	if ((val & DP_PSR_SINK_STATE_MASK) == DP_PSR_SINK_INTERNAL_ERROR) {
 		DRM_DEBUG_KMS("PSR sink internal error, disabling PSR\n");
 		intel_psr_disable_locked(intel_dp);
+		psr->sink_not_reliable = true;
 	}
 
 	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_ERROR_STATUS, &val) != 1) {
@@ -1100,8 +1106,10 @@ void intel_psr_short_pulse(struct intel_dp *intel_dp)
 	if (val & ~errors)
 		DRM_ERROR("PSR_ERROR_STATUS unhandled errors %x\n",
 			  val & ~errors);
-	if (val & errors)
+	if (val & errors) {
 		intel_psr_disable_locked(intel_dp);
+		psr->sink_not_reliable = true;
+	}
 	/* clear status register */
 	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_ERROR_STATUS, val);
 exit:
-- 
2.19.1

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

[PATCH 4/7] drm/i915: Disable PSR when a PSR aux error happen

[José Roberto de Souza]

While PSR is active hardware will do aux transactions by it self to
wakeup sink to receive a new frame when necessary. If that
transaction is not acked by sink, hardware will trigger this
interruption.

So let's disable PSR as it is a hint that there is problem with this
sink.

The removed FIXME was asking to manually train the link but we don't
need to do that as by spec sink should do a short pulse when it is
out of sync with source, we just need to make sure it is awaken and
the SDP header with PSR disable will trigger this condition.

v3: added workarround to fix scheduled work starvation cause by
to frequent PSR error interruption
v4: only setting irq_aux_error as we don't care in clear it and
not using dev_priv->irq_lock as consequence.

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 | 41 ++++++++++++++++++++++++++++----
 2 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e13222518c1b..4022a317cf05 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -642,6 +642,7 @@ struct i915_psr {
 	ktime_t last_entry_attempt;
 	ktime_t last_exit;
 	bool sink_not_reliable;
+	bool irq_aux_error;
 };
 
 enum intel_pch {
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index cc738497d551..93d8538a2383 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -152,6 +152,7 @@ void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir)
 	u32 transcoders = BIT(TRANSCODER_EDP);
 	enum transcoder cpu_transcoder;
 	ktime_t time_ns =  ktime_get();
+	u32 mask = 0;
 
 	if (INTEL_GEN(dev_priv) >= 8)
 		transcoders |= BIT(TRANSCODER_A) |
@@ -159,10 +160,22 @@ void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir)
 			       BIT(TRANSCODER_C);
 
 	for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {
-		/* FIXME: Exit PSR and link train manually when this happens. */
-		if (psr_iir & EDP_PSR_ERROR(cpu_transcoder))
-			DRM_DEBUG_KMS("[transcoder %s] PSR aux error\n",
-				      transcoder_name(cpu_transcoder));
+		if (psr_iir & EDP_PSR_ERROR(cpu_transcoder)) {
+			DRM_WARN("[transcoder %s] PSR aux error\n",
+				 transcoder_name(cpu_transcoder));
+
+			dev_priv->psr.irq_aux_error = true;
+
+			/*
+			 * If this interruption is not masked it will keep
+			 * interrupting so fast that it prevents the scheduled
+			 * work to run.
+			 * Also after a PSR error, we don't want to arm PSR
+			 * again so we don't care about unmask the interruption
+			 * or unset irq_aux_error.
+			 */
+			mask |= EDP_PSR_ERROR(cpu_transcoder);
+		}
 
 		if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
 			dev_priv->psr.last_entry_attempt = time_ns;
@@ -184,6 +197,13 @@ void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir)
 			}
 		}
 	}
+
+	if (mask) {
+		mask |= I915_READ(EDP_PSR_IMR);
+		I915_WRITE(EDP_PSR_IMR, mask);
+
+		schedule_work(&dev_priv->psr.work);
+	}
 }
 
 static bool intel_dp_get_colorimetry_status(struct intel_dp *intel_dp)
@@ -898,6 +918,16 @@ int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv,
 	return ret;
 }
 
+static void intel_psr_handle_irq(struct drm_i915_private *dev_priv)
+{
+	struct i915_psr *psr = &dev_priv->psr;
+
+	intel_psr_disable_locked(psr->dp);
+	psr->sink_not_reliable = true;
+	/* let's make sure that sink is awaken */
+	drm_dp_dpcd_writeb(&psr->dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
+}
+
 static void intel_psr_work(struct work_struct *work)
 {
 	struct drm_i915_private *dev_priv =
@@ -908,6 +938,9 @@ static void intel_psr_work(struct work_struct *work)
 	if (!dev_priv->psr.enabled)
 		goto unlock;
 
+	if (READ_ONCE(dev_priv->psr.irq_aux_error))
+		intel_psr_handle_irq(dev_priv);
+
 	/*
 	 * We have to make sure PSR is ready for re-enable
 	 * otherwise it keeps disabled until next full enable/disable cycle.
-- 
2.19.1

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

[PATCH 5/7] drm/i915: Keep PSR disabled after a driver reload after a PSR error

[José Roberto de Souza]

If a PSR error happened and the driver is reloaded, the EDP_PSR_IIR
will still keep the error set even after the reset done in the
irq_preinstall and irq_uninstall hooks.
And enabling in this situation cause the screen to freeze in the
first time that PSR HW tries to activate so lets keep PSR disabled
to avoid any rendering problems.

v4: Moved handling from intel_psr_compute_config() to
intel_psr_init() to avoid hardware access during compute(Ville)

Cc: Ville Syrjälä <ville.syrjala@linux.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 | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 93d8538a2383..e505b0b9ae47 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -1084,6 +1084,20 @@ void intel_psr_init(struct drm_i915_private *dev_priv)
 		if (INTEL_GEN(dev_priv) < 9 || !dev_priv->vbt.psr.enable)
 			i915_modparams.enable_psr = 0;
 
+	/*
+	 * If a PSR error happened and the driver is reloaded, the EDP_PSR_IIR
+	 * will still keep the error set even after the reset done in the
+	 * irq_preinstall and irq_uninstall hooks.
+	 * And enabling in this situation cause the screen to freeze in the
+	 * first time that PSR HW tries to activate so lets keep PSR disabled
+	 * to avoid any rendering problems.
+	 */
+	if (I915_READ(EDP_PSR_IIR) & EDP_PSR_ERROR(TRANSCODER_EDP)) {
+		DRM_DEBUG_KMS("PSR interruption error set\n");
+		dev_priv->psr.sink_not_reliable = true;
+		return;
+	}
+
 	/* Set link_standby x link_off defaults */
 	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
 		/* HSW and BDW require workarounds that we don't implement. */
-- 
2.19.1

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

[PATCH 6/7] drm/i915/hsw: Drop the stereo 3D enabled check in psr_compute_config()

[José Roberto de Souza]

We should not access hardware while computing config also we don't
support stereo 3D so this tests was never true.

Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index e505b0b9ae47..853e3f1370a0 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -533,13 +533,6 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
 		return;
 	}
 
-	if (IS_HASWELL(dev_priv) &&
-	    I915_READ(HSW_STEREO_3D_CTL(crtc_state->cpu_transcoder)) &
-		      S3D_ENABLE) {
-		DRM_DEBUG_KMS("PSR condition failed: Stereo 3D is Enabled\n");
-		return;
-	}
-
 	if (IS_HASWELL(dev_priv) &&
 	    adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) {
 		DRM_DEBUG_KMS("PSR condition failed: Interlaced is Enabled\n");
-- 
2.19.1

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

[PATCH 7/7] drm/i915/psr: Disable DRRS if enabled when enabling PSR from debugfs

[José Roberto de Souza]

If panel supports DRRS and PSR and if driver is loaded without PSR
enabled, driver will enable DRRS as expected but if PSR is enabled by
debugfs latter it will keep PSR and DRRS enabled causing possible
problems as DRRS will lower the refresh rate while PSR enabled.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108341
Cc: Maarten Lankhorst <maarten.lankhorst@linux.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 | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 853e3f1370a0..bfc6a08b5cf4 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -904,8 +904,11 @@ int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv,
 
 	intel_psr_irq_control(dev_priv, dev_priv->psr.debug);
 
-	if (dev_priv->psr.prepared && enable)
+	if (dev_priv->psr.prepared && enable) {
+		if (crtc_state)
+			intel_edp_drrs_disable(dp, crtc_state);
 		intel_psr_enable_locked(dev_priv, crtc_state);
+	}
 
 	mutex_unlock(&dev_priv->psr.lock);
 	return ret;
-- 
2.19.1

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

✗ Fi.CI.CHECKPATCH: warning for series starting with [1/7] drm/i915: Avoid a full port detection in the first eDP short pulse

[Patchwork]

== Series Details ==

Series: series starting with [1/7] drm/i915: Avoid a full port detection in the first eDP short pulse
URL   : https://patchwork.freedesktop.org/series/52313/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
eb5d436583c5 drm/i915: Avoid a full port detection in the first eDP short pulse
d964efb6165c drm/i915: Check PSR errors instead of retrain while PSR is enabled
6075713b1f8e drm/i915: Do not enable PSR in the next modeset after a error
-:25: CHECK:BOOL_MEMBER: Avoid using bool structure members because of possible alignment issues - see: https://lkml.org/lkml/2017/11/21/384
#25: FILE: drivers/gpu/drm/i915/i915_drv.h:644:
+	bool sink_not_reliable;

total: 0 errors, 0 warnings, 1 checks, 36 lines checked
b0512ffaa456 drm/i915: Disable PSR when a PSR aux error happen
-:38: CHECK:BOOL_MEMBER: Avoid using bool structure members because of possible alignment issues - see: https://lkml.org/lkml/2017/11/21/384
#38: FILE: drivers/gpu/drm/i915/i915_drv.h:645:
+	bool irq_aux_error;

total: 0 errors, 0 warnings, 1 checks, 78 lines checked
e17ce4464620 drm/i915: Keep PSR disabled after a driver reload after a PSR error
0061dd4a67b5 drm/i915/hsw: Drop the stereo 3D enabled check in psr_compute_config()
e157bc678463 drm/i915/psr: Disable DRRS if enabled when enabling PSR from debugfs

_______________________________________________
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/7] drm/i915: Avoid a full port detection in the first eDP short pulse

[Patchwork]

== Series Details ==

Series: series starting with [1/7] drm/i915: Avoid a full port detection in the first eDP short pulse
URL   : https://patchwork.freedesktop.org/series/52313/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Sparse version: v0.5.2
Commit: drm/i915: Avoid a full port detection in the first eDP short pulse
Okay!

Commit: drm/i915: Check PSR errors instead of retrain while PSR is enabled
Okay!

Commit: drm/i915: Do not enable PSR in the next modeset after a error
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3714:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3715:16: warning: expression using sizeof(void)

Commit: drm/i915: Disable PSR when a PSR aux error happen
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3715:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3716:16: warning: expression using sizeof(void)

Commit: drm/i915: Keep PSR disabled after a driver reload after a PSR error
Okay!

Commit: drm/i915/hsw: Drop the stereo 3D enabled check in psr_compute_config()
Okay!

Commit: drm/i915/psr: Disable DRRS if enabled when enabling PSR from debugfs
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/7] drm/i915: Avoid a full port detection in the first eDP short pulse

[Patchwork]

== Series Details ==

Series: series starting with [1/7] drm/i915: Avoid a full port detection in the first eDP short pulse
URL   : https://patchwork.freedesktop.org/series/52313/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_5117 -> Patchwork_10800 =

== Summary - FAILURE ==

  Serious unknown changes coming with Patchwork_10800 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_10800, 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/52313/revisions/1/mbox/

== Possible new issues ==

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

  === IGT changes ===

    ==== Possible regressions ====

    igt@drv_selftest@live_execlists:
      fi-cfl-8109u:       PASS -> DMESG-WARN

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_selftest@live_contexts:
      fi-icl-u2:          NOTRUN -> DMESG-FAIL (fdo#108569)

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

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-apl-guc:         PASS -> DMESG-WARN (fdo#108566)

    igt@pm_rpm@module-reload:
      fi-skl-6600u:       PASS -> INCOMPLETE (fdo#107807)

    
    ==== Possible fixes ====

    igt@gem_ctx_create@basic-files:
      fi-icl-u2:          DMESG-WARN (fdo#107724) -> PASS
      fi-bsw-kefka:       FAIL (fdo#108656) -> PASS

    igt@gem_exec_reloc@basic-write-read:
      fi-icl-u2:          DMESG-WARN -> PASS

    igt@kms_flip@basic-flip-vs-modeset:
      fi-hsw-4770r:       DMESG-WARN (fdo#105602) -> PASS

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:
      fi-byt-clapper:     FAIL (fdo#107362, fdo#103191) -> PASS +1

    
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107724 https://bugs.freedesktop.org/show_bug.cgi?id=107724
  fdo#107807 https://bugs.freedesktop.org/show_bug.cgi?id=107807
  fdo#108566 https://bugs.freedesktop.org/show_bug.cgi?id=108566
  fdo#108569 https://bugs.freedesktop.org/show_bug.cgi?id=108569
  fdo#108656 https://bugs.freedesktop.org/show_bug.cgi?id=108656


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

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


== Build changes ==

    * Linux: CI_DRM_5117 -> Patchwork_10800

  CI_DRM_5117: 5d5c30c5b1ff128ceaca95ef7148b5a696fc6645 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4714: cab148ca3ec904a94d0cd43476cf7e1f8663f906 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10800: e157bc67846311786746c90b94f8ffab6b7e2db1 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

e157bc678463 drm/i915/psr: Disable DRRS if enabled when enabling PSR from debugfs
0061dd4a67b5 drm/i915/hsw: Drop the stereo 3D enabled check in psr_compute_config()
e17ce4464620 drm/i915: Keep PSR disabled after a driver reload after a PSR error
b0512ffaa456 drm/i915: Disable PSR when a PSR aux error happen
6075713b1f8e drm/i915: Do not enable PSR in the next modeset after a error
d964efb6165c drm/i915: Check PSR errors instead of retrain while PSR is enabled
eb5d436583c5 drm/i915: Avoid a full port detection in the first eDP short pulse

== Logs ==

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

Re: [PATCH 7/7] drm/i915/psr: Disable DRRS if enabled when enabling PSR from debugfs

[Maarten Lankhorst]

Op 09-11-18 om 21:20 schreef José Roberto de Souza:
> If panel supports DRRS and PSR and if driver is loaded without PSR
> enabled, driver will enable DRRS as expected but if PSR is enabled by
> debugfs latter it will keep PSR and DRRS enabled causing possible
> problems as DRRS will lower the refresh rate while PSR enabled.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108341
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.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 | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 853e3f1370a0..bfc6a08b5cf4 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -904,8 +904,11 @@ int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv,
>  
>  	intel_psr_irq_control(dev_priv, dev_priv->psr.debug);
>  
> -	if (dev_priv->psr.prepared && enable)
> +	if (dev_priv->psr.prepared && enable) {
> +		if (crtc_state)
> +			intel_edp_drrs_disable(dp, crtc_state);
>  		intel_psr_enable_locked(dev_priv, crtc_state);
> +	}
>  
>  	mutex_unlock(&dev_priv->psr.lock);
>  	return ret;

I've considered this, but I thought it was a feature, not a bug. It's a pain to track
how we handle this as intended.

kms_frontbuffer_tracking is also controlling DRRS during the test, so perhaps simply
fix the test?

It seems the no_drrs test simply checks that if PSR is enabled, we don't have drrs
enabled. We probably care about the default configuration, so I would simply disable
the pipe, update the PSR flag, and then start running the tests. Else the only thing
we test is that debugfs disables DRRS. Not that the default modeset path prevents
PSR and DRRS simultaneously.

~Maarten

Maybe something like below?

Perhaps move the drrs manipulation functions from kms_frontbuffer_tracking to lib/kms_psr.c

----8<-------
diff --git a/tests/kms_psr.c b/tests/kms_psr.c
index 9767f475bf23..ffc356df06ce 100644
--- a/tests/kms_psr.c
+++ b/tests/kms_psr.c
@@ -414,9 +414,6 @@ int main(int argc, char *argv[])
 		kmstest_set_vt_graphics_mode();
 		data.devid = intel_get_drm_devid(data.drm_fd);
 
-		if (!data.with_psr_disabled)
-			psr_enable(data.debugfs_fd);
-
 		igt_require_f(sink_support(&data),
 			      "Sink does not support PSR\n");
 
@@ -428,18 +425,25 @@ int main(int argc, char *argv[])
 	}
 
 	igt_subtest("basic") {
-		setup_test_plane(&data, DRM_PLANE_TYPE_PRIMARY);
-		igt_assert(psr_wait_entry_if_enabled(&data));
-		test_cleanup(&data);
-	}
+		/* Disable display to get a default setup. */
+		igt_display_commit2(&data.display, data.display.is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
+
+		if (!data.with_psr_disabled)
+			psr_enable(data.debugfs_fd);
 
-	igt_subtest("no_drrs") {
 		setup_test_plane(&data, DRM_PLANE_TYPE_PRIMARY);
 		igt_assert(psr_wait_entry_if_enabled(&data));
 		igt_assert(drrs_disabled(&data));
 		test_cleanup(&data);
 	}
 
+	igt_fixture {
+		drrs_disable();
+
+		if (!data.with_psr_disabled)
+			psr_enable(data.debugfs_fd);
+	}
+
 	for (op = PAGE_FLIP; op <= RENDER; op++) {
 		igt_subtest_f("primary_%s", op_str(op)) {
 			data.op = op;

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

Re: [PATCH 1/7] drm/i915: Avoid a full port detection in the first eDP short pulse

[Ville Syrjälä]

On Fri, Nov 09, 2018 at 12:20:10PM -0800, José Roberto de Souza wrote:
> Some eDP panels do not set a valid sink count value and even for the
> ones that sets is should always be one for eDP, that is why it is not
> cached in intel_edp_init_dpcd().
> 
> But intel_dp_short_pulse() compares the old count with the read one
> if there is a mistmatch a full port detection will be executed, what
> was happening in the first short pulse interruption of eDP panels
> that sets sink count.
> 
> Instead of just skip the compasison for eDP panels, lets not read
> the sink count at all for eDP.
> 
> v2: the previous version of this patch was caching the sink count
> in intel_edp_init_dpcd() but I was pointed out by Ville a patch that
> handled a case of a eDP panel that do not set sink count
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>

Yeah, I guess we concluded that never reading it is totally fine.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_dp.c | 44 +++++++++++++++++++--------------
>  1 file changed, 26 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 2b090609bee2..577c166f6483 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3936,8 +3936,6 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp)
>  static bool
>  intel_dp_get_dpcd(struct intel_dp *intel_dp)
>  {
> -	u8 sink_count;
> -
>  	if (!intel_dp_read_dpcd(intel_dp))
>  		return false;
>  
> @@ -3947,25 +3945,35 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>  		intel_dp_set_common_rates(intel_dp);
>  	}
>  
> -	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_SINK_COUNT, &sink_count) <= 0)
> -		return false;
> -
>  	/*
> -	 * Sink count can change between short pulse hpd hence
> -	 * a member variable in intel_dp will track any changes
> -	 * between short pulse interrupts.
> +	 * Some eDP panels do not set a valid value for sink count, that is why
> +	 * it don't care about read it here and in intel_edp_init_dpcd().
>  	 */
> -	intel_dp->sink_count = DP_GET_SINK_COUNT(sink_count);
> +	if (!intel_dp_is_edp(intel_dp)) {
> +		u8 count;
> +		ssize_t r;
>  
> -	/*
> -	 * SINK_COUNT == 0 and DOWNSTREAM_PORT_PRESENT == 1 implies that
> -	 * a dongle is present but no display. Unless we require to know
> -	 * if a dongle is present or not, we don't need to update
> -	 * downstream port information. So, an early return here saves
> -	 * time from performing other operations which are not required.
> -	 */
> -	if (!intel_dp_is_edp(intel_dp) && !intel_dp->sink_count)
> -		return false;
> +		r = drm_dp_dpcd_readb(&intel_dp->aux, DP_SINK_COUNT, &count);
> +		if (r < 1)
> +			return false;
> +
> +		/*
> +		 * Sink count can change between short pulse hpd hence
> +		 * a member variable in intel_dp will track any changes
> +		 * between short pulse interrupts.
> +		 */
> +		intel_dp->sink_count = DP_GET_SINK_COUNT(count);
> +
> +		/*
> +		 * SINK_COUNT == 0 and DOWNSTREAM_PORT_PRESENT == 1 implies that
> +		 * a dongle is present but no display. Unless we require to know
> +		 * if a dongle is present or not, we don't need to update
> +		 * downstream port information. So, an early return here saves
> +		 * time from performing other operations which are not required.
> +		 */
> +		if (!intel_dp->sink_count)
> +			return false;
> +	}
>  
>  	if (!drm_dp_is_branch(intel_dp->dpcd))
>  		return true; /* native DP sink */
> -- 
> 2.19.1

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

Re: [PATCH 7/7] drm/i915/psr: Disable DRRS if enabled when enabling PSR from debugfs

[Souza, Jose]

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

On Mon, 2018-11-12 at 11:17 +0100, Maarten Lankhorst wrote:
> Op 09-11-18 om 21:20 schreef José Roberto de Souza:
> > If panel supports DRRS and PSR and if driver is loaded without PSR
> > enabled, driver will enable DRRS as expected but if PSR is enabled
> > by
> > debugfs latter it will keep PSR and DRRS enabled causing possible
> > problems as DRRS will lower the refresh rate while PSR enabled.
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108341
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.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 | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 853e3f1370a0..bfc6a08b5cf4 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -904,8 +904,11 @@ int intel_psr_set_debugfs_mode(struct
> > drm_i915_private *dev_priv,
> >  
> >  	intel_psr_irq_control(dev_priv, dev_priv->psr.debug);
> >  
> > -	if (dev_priv->psr.prepared && enable)
> > +	if (dev_priv->psr.prepared && enable) {
> > +		if (crtc_state)
> > +			intel_edp_drrs_disable(dp, crtc_state);
> >  		intel_psr_enable_locked(dev_priv, crtc_state);
> > +	}
> >  
> >  	mutex_unlock(&dev_priv->psr.lock);
> >  	return ret;
> 
> I've considered this, but I thought it was a feature, not a bug. It's
> a pain to track
> how we handle this as intended.
> 
> kms_frontbuffer_tracking is also controlling DRRS during the test, so
> perhaps simply
> fix the test?
> 
> It seems the no_drrs test simply checks that if PSR is enabled, we
> don't have drrs
> enabled. We probably care about the default configuration, so I would
> simply disable
> the pipe, update the PSR flag, and then start running the tests. Else
> the only thing
> we test is that debugfs disables DRRS. Not that the default modeset
> path prevents
> PSR and DRRS simultaneously.


Yeah, I think we should force a modeset from debugs to test the default
modeset path, fix the tests I think is a bad idea as it could leave
some test cases unhandled.

Also looking at DRRS it looks complete broken for page flips, it would
kept set to DRRS_LOW forever.


> 
> ~Maarten
> 
> Maybe something like below?
> 
> Perhaps move the drrs manipulation functions from
> kms_frontbuffer_tracking to lib/kms_psr.c
> 
> ----8<-------
> diff --git a/tests/kms_psr.c b/tests/kms_psr.c
> index 9767f475bf23..ffc356df06ce 100644
> --- a/tests/kms_psr.c
> +++ b/tests/kms_psr.c
> @@ -414,9 +414,6 @@ int main(int argc, char *argv[])
>  		kmstest_set_vt_graphics_mode();
>  		data.devid = intel_get_drm_devid(data.drm_fd);
>  
> -		if (!data.with_psr_disabled)
> -			psr_enable(data.debugfs_fd);
> -
>  		igt_require_f(sink_support(&data),
>  			      "Sink does not support PSR\n");
>  
> @@ -428,18 +425,25 @@ int main(int argc, char *argv[])
>  	}
>  
>  	igt_subtest("basic") {
> -		setup_test_plane(&data, DRM_PLANE_TYPE_PRIMARY);
> -		igt_assert(psr_wait_entry_if_enabled(&data));
> -		test_cleanup(&data);
> -	}
> +		/* Disable display to get a default setup. */
> +		igt_display_commit2(&data.display,
> data.display.is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
> +
> +		if (!data.with_psr_disabled)
> +			psr_enable(data.debugfs_fd);
>  
> -	igt_subtest("no_drrs") {
>  		setup_test_plane(&data, DRM_PLANE_TYPE_PRIMARY);
>  		igt_assert(psr_wait_entry_if_enabled(&data));
>  		igt_assert(drrs_disabled(&data));
>  		test_cleanup(&data);
>  	}
>  
> +	igt_fixture {
> +		drrs_disable();
> +
> +		if (!data.with_psr_disabled)
> +			psr_enable(data.debugfs_fd);
> +	}
> +
>  	for (op = PAGE_FLIP; op <= RENDER; op++) {
>  		igt_subtest_f("primary_%s", op_str(op)) {
>  			data.op = op;
> 

[-- 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/7] drm/i915: Check PSR errors instead of retrain while PSR is enabled

[Rodrigo Vivi]

On Fri, Nov 09, 2018 at 12:20:11PM -0800, José Roberto de Souza wrote:
> When a PSR error happens sink sets the PSR errors register and also
> set the link to a error status.
> So in the short pulse handling it was returning earlier and doing a
> full detection and attempting to retrain but it fails as PSR HW is
> in change of the main-link.
> 
> Just call intel_psr_short_pulse() before
> intel_dp_needs_link_retrain() is not the right fix as
> intel_dp_needs_link_retrain() would return true and trigger a full
> detection while PSR HW is still in change of main-link.
> 
> Check for PSR active is also not safe as it could be inactive due a
> frontbuffer invalidate and still doing the PSR exit sequence.
> 
> v3: added comment in intel_dp_needs_link_retrain()
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>

I wonder if we shouldn't disable psr and then move with
link training on situations like this instead of dropping some
link trainings... Not sure if I trust PSR more to take care of
this case. But well, maybe this case is only happening on
recoverable PSR errors where hpd pulses end up happening so
this should be ok.

Let's try this approach, but please consider the option
to disable psr instead if we face some issues around here...

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

> ---
>  drivers/gpu/drm/i915/intel_dp.c  | 11 +++++++++++
>  drivers/gpu/drm/i915/intel_drv.h |  1 +
>  drivers/gpu/drm/i915/intel_psr.c | 15 +++++++++++++++
>  3 files changed, 27 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 577c166f6483..158c6f25d2e2 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4383,6 +4383,17 @@ intel_dp_needs_link_retrain(struct intel_dp *intel_dp)
>  	if (!intel_dp->link_trained)
>  		return false;
>  
> +	/*
> +	 * While PSR source HW is enabled, it will control main-link sending
> +	 * frames, enabling and disabling it so trying to do a retrain will fail
> +	 * as the link would or not be on or it could mix training patterns
> +	 * and frame data at the same time causing retrain to fail.
> +	 * Also when exiting PSR, HW will retrain the link anyways fixing
> +	 * any link status error.
> +	 */
> +	if (intel_psr_enabled(intel_dp))
> +		return false;
> +
>  	if (!intel_dp_get_link_status(intel_dp, link_status))
>  		return false;
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index cc7fab2b61f4..12f96297299e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -2046,6 +2046,7 @@ void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir);
>  void intel_psr_short_pulse(struct intel_dp *intel_dp);
>  int intel_psr_wait_for_idle(const struct intel_crtc_state *new_crtc_state,
>  			    u32 *out_value);
> +bool intel_psr_enabled(struct intel_dp *intel_dp);
>  
>  /* intel_quirks.c */
>  void intel_init_quirks(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 48df16a02fac..f940305b72e4 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -1107,3 +1107,18 @@ void intel_psr_short_pulse(struct intel_dp *intel_dp)
>  exit:
>  	mutex_unlock(&psr->lock);
>  }
> +
> +bool intel_psr_enabled(struct intel_dp *intel_dp)
> +{
> +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> +	bool ret;
> +
> +	if (!CAN_PSR(dev_priv) || !intel_dp_is_edp(intel_dp))
> +		return false;
> +
> +	mutex_lock(&dev_priv->psr.lock);
> +	ret = (dev_priv->psr.dp == intel_dp && dev_priv->psr.enabled);
> +	mutex_unlock(&dev_priv->psr.lock);
> +
> +	return ret;
> +}
> -- 
> 2.19.1
> 
> _______________________________________________
> 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 3/7] drm/i915: Do not enable PSR in the next modeset after a error

[Rodrigo Vivi]

On Fri, Nov 09, 2018 at 12:20:12PM -0800, José Roberto de Souza wrote:
> When we detect a error and disable PSR, it is kept disable until the
> next modeset but as the sink already show signs that it do not
> properly work with PSR lets disabled it for good to avoid any
> additional flickering.
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>

Maybe you can now just ignore my comment on the previous patch! :)

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



> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  1 +
>  drivers/gpu/drm/i915/intel_psr.c | 10 +++++++++-
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 08d25aa480f7..e13222518c1b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -641,6 +641,7 @@ struct i915_psr {
>  	u8 sink_sync_latency;
>  	ktime_t last_entry_attempt;
>  	ktime_t last_exit;
> +	bool sink_not_reliable;
>  };
>  
>  enum intel_pch {
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index f940305b72e4..cc738497d551 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -508,6 +508,11 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
>  		return;
>  	}
>  
> +	if (dev_priv->psr.sink_not_reliable) {
> +		DRM_DEBUG_KMS("Sink not reliable set\n");
> +		return;
> +	}
> +
>  	if (IS_HASWELL(dev_priv) &&
>  	    I915_READ(HSW_STEREO_3D_CTL(crtc_state->cpu_transcoder)) &
>  		      S3D_ENABLE) {
> @@ -1083,6 +1088,7 @@ void intel_psr_short_pulse(struct intel_dp *intel_dp)
>  	if ((val & DP_PSR_SINK_STATE_MASK) == DP_PSR_SINK_INTERNAL_ERROR) {
>  		DRM_DEBUG_KMS("PSR sink internal error, disabling PSR\n");
>  		intel_psr_disable_locked(intel_dp);
> +		psr->sink_not_reliable = true;
>  	}
>  
>  	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_ERROR_STATUS, &val) != 1) {
> @@ -1100,8 +1106,10 @@ void intel_psr_short_pulse(struct intel_dp *intel_dp)
>  	if (val & ~errors)
>  		DRM_ERROR("PSR_ERROR_STATUS unhandled errors %x\n",
>  			  val & ~errors);
> -	if (val & errors)
> +	if (val & errors) {
>  		intel_psr_disable_locked(intel_dp);
> +		psr->sink_not_reliable = true;
> +	}
>  	/* clear status register */
>  	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_ERROR_STATUS, val);
>  exit:
> -- 
> 2.19.1
> 
> _______________________________________________
> 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/7] drm/i915: Disable PSR when a PSR aux error happen

[Rodrigo Vivi]

On Fri, Nov 09, 2018 at 12:20:13PM -0800, José Roberto de Souza wrote:
> While PSR is active hardware will do aux transactions by it self to
> wakeup sink to receive a new frame when necessary. If that
> transaction is not acked by sink, hardware will trigger this
> interruption.
> 
> So let's disable PSR as it is a hint that there is problem with this
> sink.
> 
> The removed FIXME was asking to manually train the link but we don't
> need to do that as by spec sink should do a short pulse when it is
> out of sync with source, we just need to make sure it is awaken and
> the SDP header with PSR disable will trigger this condition.
> 
> v3: added workarround to fix scheduled work starvation cause by
> to frequent PSR error interruption
> v4: only setting irq_aux_error as we don't care in clear it and
> not using dev_priv->irq_lock as consequence.
> 
> 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 | 41 ++++++++++++++++++++++++++++----
>  2 files changed, 38 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e13222518c1b..4022a317cf05 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -642,6 +642,7 @@ struct i915_psr {
>  	ktime_t last_entry_attempt;
>  	ktime_t last_exit;
>  	bool sink_not_reliable;
> +	bool irq_aux_error;
>  };
>  
>  enum intel_pch {
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index cc738497d551..93d8538a2383 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -152,6 +152,7 @@ void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir)
>  	u32 transcoders = BIT(TRANSCODER_EDP);
>  	enum transcoder cpu_transcoder;
>  	ktime_t time_ns =  ktime_get();
> +	u32 mask = 0;
>  
>  	if (INTEL_GEN(dev_priv) >= 8)
>  		transcoders |= BIT(TRANSCODER_A) |
> @@ -159,10 +160,22 @@ void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir)
>  			       BIT(TRANSCODER_C);
>  
>  	for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {
> -		/* FIXME: Exit PSR and link train manually when this happens. */
> -		if (psr_iir & EDP_PSR_ERROR(cpu_transcoder))
> -			DRM_DEBUG_KMS("[transcoder %s] PSR aux error\n",
> -				      transcoder_name(cpu_transcoder));
> +		if (psr_iir & EDP_PSR_ERROR(cpu_transcoder)) {
> +			DRM_WARN("[transcoder %s] PSR aux error\n",
> +				 transcoder_name(cpu_transcoder));
> +
> +			dev_priv->psr.irq_aux_error = true;
> +
> +			/*
> +			 * If this interruption is not masked it will keep
> +			 * interrupting so fast that it prevents the scheduled
> +			 * work to run.
> +			 * Also after a PSR error, we don't want to arm PSR
> +			 * again so we don't care about unmask the interruption
> +			 * or unset irq_aux_error.
> +			 */
> +			mask |= EDP_PSR_ERROR(cpu_transcoder);
> +		}
>  
>  		if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
>  			dev_priv->psr.last_entry_attempt = time_ns;
> @@ -184,6 +197,13 @@ void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir)
>  			}
>  		}
>  	}
> +
> +	if (mask) {
> +		mask |= I915_READ(EDP_PSR_IMR);
> +		I915_WRITE(EDP_PSR_IMR, mask);
> +
> +		schedule_work(&dev_priv->psr.work);
> +	}
>  }
>  
>  static bool intel_dp_get_colorimetry_status(struct intel_dp *intel_dp)
> @@ -898,6 +918,16 @@ int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv,
>  	return ret;
>  }
>  
> +static void intel_psr_handle_irq(struct drm_i915_private *dev_priv)
> +{
> +	struct i915_psr *psr = &dev_priv->psr;
> +
> +	intel_psr_disable_locked(psr->dp);
> +	psr->sink_not_reliable = true;
> +	/* let's make sure that sink is awaken */
> +	drm_dp_dpcd_writeb(&psr->dp->aux, DP_SET_POWER, DP_SET_POWER_D0);

Hmmmm... my gut feeling and my bad memory about PSR tells this can flicker
in some panels. But at least you are disabling PSR for good after that what
is a good thing.

I wonder what other alternatives we would have here?
maybe the one suggested on FIXME? disable PSR and train link manually?

> +}
> +
>  static void intel_psr_work(struct work_struct *work)
>  {
>  	struct drm_i915_private *dev_priv =
> @@ -908,6 +938,9 @@ static void intel_psr_work(struct work_struct *work)
>  	if (!dev_priv->psr.enabled)
>  		goto unlock;
>  
> +	if (READ_ONCE(dev_priv->psr.irq_aux_error))
> +		intel_psr_handle_irq(dev_priv);
> +
>  	/*
>  	 * We have to make sure PSR is ready for re-enable
>  	 * otherwise it keeps disabled until next full enable/disable cycle.
> -- 
> 2.19.1
> 
> _______________________________________________
> 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 5/7] drm/i915: Keep PSR disabled after a driver reload after a PSR error

[Rodrigo Vivi]

On Fri, Nov 09, 2018 at 12:20:14PM -0800, José Roberto de Souza wrote:
> If a PSR error happened and the driver is reloaded, the EDP_PSR_IIR
> will still keep the error set even after the reset done in the
> irq_preinstall and irq_uninstall hooks.
> And enabling in this situation cause the screen to freeze in the
> first time that PSR HW tries to activate so lets keep PSR disabled
> to avoid any rendering problems.
> 
> v4: Moved handling from intel_psr_compute_config() to
> intel_psr_init() to avoid hardware access during compute(Ville)
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@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 | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 93d8538a2383..e505b0b9ae47 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -1084,6 +1084,20 @@ void intel_psr_init(struct drm_i915_private *dev_priv)
>  		if (INTEL_GEN(dev_priv) < 9 || !dev_priv->vbt.psr.enable)
>  			i915_modparams.enable_psr = 0;
>  
> +	/*
> +	 * If a PSR error happened and the driver is reloaded, the EDP_PSR_IIR
> +	 * will still keep the error set even after the reset done in the
> +	 * irq_preinstall and irq_uninstall hooks.
> +	 * And enabling in this situation cause the screen to freeze in the
> +	 * first time that PSR HW tries to activate so lets keep PSR disabled
> +	 * to avoid any rendering problems.
> +	 */
> +	if (I915_READ(EDP_PSR_IIR) & EDP_PSR_ERROR(TRANSCODER_EDP)) {
> +		DRM_DEBUG_KMS("PSR interruption error set\n");
> +		dev_priv->psr.sink_not_reliable = true;
> +		return;
> +	}
> +
>  	/* Set link_standby x link_off defaults */
>  	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
>  		/* HSW and BDW require workarounds that we don't implement. */
> -- 
> 2.19.1
> 
> _______________________________________________
> 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 6/7] drm/i915/hsw: Drop the stereo 3D enabled check in psr_compute_config()

[Rodrigo Vivi]

On Fri, Nov 09, 2018 at 12:20:15PM -0800, José Roberto de Souza wrote:
> We should not access hardware while computing config also we don't
> support stereo 3D so this tests was never true.

yeap... it was there becase at some point we were planing to enabled that S3D
and if that happen we would easily forgot about the PSR case and have
bad bugs. But I don't see that happening any time soon, so, let's clean it up.


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



> 
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index e505b0b9ae47..853e3f1370a0 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -533,13 +533,6 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
>  		return;
>  	}
>  
> -	if (IS_HASWELL(dev_priv) &&
> -	    I915_READ(HSW_STEREO_3D_CTL(crtc_state->cpu_transcoder)) &
> -		      S3D_ENABLE) {
> -		DRM_DEBUG_KMS("PSR condition failed: Stereo 3D is Enabled\n");
> -		return;
> -	}
> -
>  	if (IS_HASWELL(dev_priv) &&
>  	    adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) {
>  		DRM_DEBUG_KMS("PSR condition failed: Interlaced is Enabled\n");
> -- 
> 2.19.1
> 
> _______________________________________________
> 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/7] drm/i915: Disable PSR when a PSR aux error happen

[Souza, Jose]

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

On Tue, 2018-11-20 at 14:47 -0800, Rodrigo Vivi wrote:
> On Fri, Nov 09, 2018 at 12:20:13PM -0800, José Roberto de Souza
> wrote:
> > While PSR is active hardware will do aux transactions by it self to
> > wakeup sink to receive a new frame when necessary. If that
> > transaction is not acked by sink, hardware will trigger this
> > interruption.
> > 
> > So let's disable PSR as it is a hint that there is problem with
> > this
> > sink.
> > 
> > The removed FIXME was asking to manually train the link but we
> > don't
> > need to do that as by spec sink should do a short pulse when it is
> > out of sync with source, we just need to make sure it is awaken and
> > the SDP header with PSR disable will trigger this condition.
> > 
> > v3: added workarround to fix scheduled work starvation cause by
> > to frequent PSR error interruption
> > v4: only setting irq_aux_error as we don't care in clear it and
> > not using dev_priv->irq_lock as consequence.
> > 
> > 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 | 41
> > ++++++++++++++++++++++++++++----
> >  2 files changed, 38 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index e13222518c1b..4022a317cf05 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -642,6 +642,7 @@ struct i915_psr {
> >  	ktime_t last_entry_attempt;
> >  	ktime_t last_exit;
> >  	bool sink_not_reliable;
> > +	bool irq_aux_error;
> >  };
> >  
> >  enum intel_pch {
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index cc738497d551..93d8538a2383 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -152,6 +152,7 @@ void intel_psr_irq_handler(struct
> > drm_i915_private *dev_priv, u32 psr_iir)
> >  	u32 transcoders = BIT(TRANSCODER_EDP);
> >  	enum transcoder cpu_transcoder;
> >  	ktime_t time_ns =  ktime_get();
> > +	u32 mask = 0;
> >  
> >  	if (INTEL_GEN(dev_priv) >= 8)
> >  		transcoders |= BIT(TRANSCODER_A) |
> > @@ -159,10 +160,22 @@ void intel_psr_irq_handler(struct
> > drm_i915_private *dev_priv, u32 psr_iir)
> >  			       BIT(TRANSCODER_C);
> >  
> >  	for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder,
> > transcoders) {
> > -		/* FIXME: Exit PSR and link train manually when this
> > happens. */
> > -		if (psr_iir & EDP_PSR_ERROR(cpu_transcoder))
> > -			DRM_DEBUG_KMS("[transcoder %s] PSR aux
> > error\n",
> > -				      transcoder_name(cpu_transcoder));
> > +		if (psr_iir & EDP_PSR_ERROR(cpu_transcoder)) {
> > +			DRM_WARN("[transcoder %s] PSR aux error\n",
> > +				 transcoder_name(cpu_transcoder));
> > +
> > +			dev_priv->psr.irq_aux_error = true;
> > +
> > +			/*
> > +			 * If this interruption is not masked it will
> > keep
> > +			 * interrupting so fast that it prevents the
> > scheduled
> > +			 * work to run.
> > +			 * Also after a PSR error, we don't want to arm
> > PSR
> > +			 * again so we don't care about unmask the
> > interruption
> > +			 * or unset irq_aux_error.
> > +			 */
> > +			mask |= EDP_PSR_ERROR(cpu_transcoder);
> > +		}
> >  
> >  		if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
> >  			dev_priv->psr.last_entry_attempt = time_ns;
> > @@ -184,6 +197,13 @@ void intel_psr_irq_handler(struct
> > drm_i915_private *dev_priv, u32 psr_iir)
> >  			}
> >  		}
> >  	}
> > +
> > +	if (mask) {
> > +		mask |= I915_READ(EDP_PSR_IMR);
> > +		I915_WRITE(EDP_PSR_IMR, mask);
> > +
> > +		schedule_work(&dev_priv->psr.work);
> > +	}
> >  }
> >  
> >  static bool intel_dp_get_colorimetry_status(struct intel_dp
> > *intel_dp)
> > @@ -898,6 +918,16 @@ int intel_psr_set_debugfs_mode(struct
> > drm_i915_private *dev_priv,
> >  	return ret;
> >  }
> >  
> > +static void intel_psr_handle_irq(struct drm_i915_private
> > *dev_priv)
> > +{
> > +	struct i915_psr *psr = &dev_priv->psr;
> > +
> > +	intel_psr_disable_locked(psr->dp);
> > +	psr->sink_not_reliable = true;
> > +	/* let's make sure that sink is awaken */
> > +	drm_dp_dpcd_writeb(&psr->dp->aux, DP_SET_POWER,
> > DP_SET_POWER_D0);
> 
> Hmmmm... my gut feeling and my bad memory about PSR tells this can
> flicker
> in some panels. But at least you are disabling PSR for good after
> that what
> is a good thing.
> 
> I wonder what other alternatives we would have here?
> maybe the one suggested on FIXME? disable PSR and train link
> manually?


I think that sinks will trigger a short pulse with a bad link status
set right after receive the first frame after disable PSR, unfortunally
eDP spec don't cover error cases like this.
So if sink does the short pulse and PSR is disabled we are covered
about the retrain with the current code, we only must be sure that sink
is awaken.

DK suggested to read the DPCD to check if sink is awaken but if the
DPCD write failed again(one from hardware and other from driver that
already do multiple tries) it means that sink is in a really bad state
and it will not recover without turning it off and on again.

> 
> > +}
> > +
> >  static void intel_psr_work(struct work_struct *work)
> >  {
> >  	struct drm_i915_private *dev_priv =
> > @@ -908,6 +938,9 @@ static void intel_psr_work(struct work_struct
> > *work)
> >  	if (!dev_priv->psr.enabled)
> >  		goto unlock;
> >  
> > +	if (READ_ONCE(dev_priv->psr.irq_aux_error))
> > +		intel_psr_handle_irq(dev_priv);
> > +
> >  	/*
> >  	 * We have to make sure PSR is ready for re-enable
> >  	 * otherwise it keeps disabled until next full enable/disable
> > cycle.
> > -- 
> > 2.19.1
> > 
> > _______________________________________________
> > 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 4/7] drm/i915: Disable PSR when a PSR aux error happen

[Rodrigo Vivi]

On Tue, Nov 20, 2018 at 03:05:07PM -0800, Souza, Jose wrote:
> On Tue, 2018-11-20 at 14:47 -0800, Rodrigo Vivi wrote:
> > On Fri, Nov 09, 2018 at 12:20:13PM -0800, José Roberto de Souza
> > wrote:
> > > While PSR is active hardware will do aux transactions by it self to
> > > wakeup sink to receive a new frame when necessary. If that
> > > transaction is not acked by sink, hardware will trigger this
> > > interruption.
> > > 
> > > So let's disable PSR as it is a hint that there is problem with
> > > this
> > > sink.
> > > 
> > > The removed FIXME was asking to manually train the link but we
> > > don't
> > > need to do that as by spec sink should do a short pulse when it is
> > > out of sync with source, we just need to make sure it is awaken and
> > > the SDP header with PSR disable will trigger this condition.
> > > 
> > > v3: added workarround to fix scheduled work starvation cause by
> > > to frequent PSR error interruption
> > > v4: only setting irq_aux_error as we don't care in clear it and
> > > not using dev_priv->irq_lock as consequence.
> > > 
> > > 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 | 41
> > > ++++++++++++++++++++++++++++----
> > >  2 files changed, 38 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index e13222518c1b..4022a317cf05 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -642,6 +642,7 @@ struct i915_psr {
> > >  	ktime_t last_entry_attempt;
> > >  	ktime_t last_exit;
> > >  	bool sink_not_reliable;
> > > +	bool irq_aux_error;
> > >  };
> > >  
> > >  enum intel_pch {
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index cc738497d551..93d8538a2383 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -152,6 +152,7 @@ void intel_psr_irq_handler(struct
> > > drm_i915_private *dev_priv, u32 psr_iir)
> > >  	u32 transcoders = BIT(TRANSCODER_EDP);
> > >  	enum transcoder cpu_transcoder;
> > >  	ktime_t time_ns =  ktime_get();
> > > +	u32 mask = 0;
> > >  
> > >  	if (INTEL_GEN(dev_priv) >= 8)
> > >  		transcoders |= BIT(TRANSCODER_A) |
> > > @@ -159,10 +160,22 @@ void intel_psr_irq_handler(struct
> > > drm_i915_private *dev_priv, u32 psr_iir)
> > >  			       BIT(TRANSCODER_C);
> > >  
> > >  	for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder,
> > > transcoders) {
> > > -		/* FIXME: Exit PSR and link train manually when this
> > > happens. */
> > > -		if (psr_iir & EDP_PSR_ERROR(cpu_transcoder))
> > > -			DRM_DEBUG_KMS("[transcoder %s] PSR aux
> > > error\n",
> > > -				      transcoder_name(cpu_transcoder));
> > > +		if (psr_iir & EDP_PSR_ERROR(cpu_transcoder)) {
> > > +			DRM_WARN("[transcoder %s] PSR aux error\n",
> > > +				 transcoder_name(cpu_transcoder));
> > > +
> > > +			dev_priv->psr.irq_aux_error = true;
> > > +
> > > +			/*
> > > +			 * If this interruption is not masked it will
> > > keep
> > > +			 * interrupting so fast that it prevents the
> > > scheduled
> > > +			 * work to run.
> > > +			 * Also after a PSR error, we don't want to arm
> > > PSR
> > > +			 * again so we don't care about unmask the
> > > interruption
> > > +			 * or unset irq_aux_error.
> > > +			 */
> > > +			mask |= EDP_PSR_ERROR(cpu_transcoder);
> > > +		}
> > >  
> > >  		if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
> > >  			dev_priv->psr.last_entry_attempt = time_ns;
> > > @@ -184,6 +197,13 @@ void intel_psr_irq_handler(struct
> > > drm_i915_private *dev_priv, u32 psr_iir)
> > >  			}
> > >  		}
> > >  	}
> > > +
> > > +	if (mask) {
> > > +		mask |= I915_READ(EDP_PSR_IMR);
> > > +		I915_WRITE(EDP_PSR_IMR, mask);
> > > +
> > > +		schedule_work(&dev_priv->psr.work);
> > > +	}
> > >  }
> > >  
> > >  static bool intel_dp_get_colorimetry_status(struct intel_dp
> > > *intel_dp)
> > > @@ -898,6 +918,16 @@ int intel_psr_set_debugfs_mode(struct
> > > drm_i915_private *dev_priv,
> > >  	return ret;
> > >  }
> > >  
> > > +static void intel_psr_handle_irq(struct drm_i915_private
> > > *dev_priv)
> > > +{
> > > +	struct i915_psr *psr = &dev_priv->psr;
> > > +
> > > +	intel_psr_disable_locked(psr->dp);
> > > +	psr->sink_not_reliable = true;
> > > +	/* let's make sure that sink is awaken */
> > > +	drm_dp_dpcd_writeb(&psr->dp->aux, DP_SET_POWER,
> > > DP_SET_POWER_D0);
> > 
> > Hmmmm... my gut feeling and my bad memory about PSR tells this can
> > flicker
> > in some panels. But at least you are disabling PSR for good after
> > that what
> > is a good thing.
> > 
> > I wonder what other alternatives we would have here?
> > maybe the one suggested on FIXME? disable PSR and train link
> > manually?
> 
> 
> I think that sinks will trigger a short pulse with a bad link status
> set right after receive the first frame after disable PSR, unfortunally
> eDP spec don't cover error cases like this.
> So if sink does the short pulse and PSR is disabled we are covered
> about the retrain with the current code, we only must be sure that sink
> is awaken.
> 
> DK suggested to read the DPCD to check if sink is awaken but if the
> DPCD write failed again(one from hardware and other from driver that
> already do multiple tries) it means that sink is in a really bad state
> and it will not recover without turning it off and on again.

after our irc chat where you confirmed this path got tested on ICL
and giving the fact that this is disabling and blocking PSR for good
after this I feel more comfortable in putting my:

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

> 
> > 
> > > +}
> > > +
> > >  static void intel_psr_work(struct work_struct *work)
> > >  {
> > >  	struct drm_i915_private *dev_priv =
> > > @@ -908,6 +938,9 @@ static void intel_psr_work(struct work_struct
> > > *work)
> > >  	if (!dev_priv->psr.enabled)
> > >  		goto unlock;
> > >  
> > > +	if (READ_ONCE(dev_priv->psr.irq_aux_error))
> > > +		intel_psr_handle_irq(dev_priv);
> > > +
> > >  	/*
> > >  	 * We have to make sure PSR is ready for re-enable
> > >  	 * otherwise it keeps disabled until next full enable/disable
> > > cycle.
> > > -- 
> > > 2.19.1
> > > 
> > > _______________________________________________
> > > 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 7/7] drm/i915/psr: Disable DRRS if enabled when enabling PSR from debugfs

[Dhinakaran Pandiyan]

On Thu, 2018-11-15 at 12:57 -0800, Souza, Jose wrote:
> On Mon, 2018-11-12 at 11:17 +0100, Maarten Lankhorst wrote:
> > Op 09-11-18 om 21:20 schreef José Roberto de Souza:
> > > If panel supports DRRS and PSR and if driver is loaded without
> > > PSR
> > > enabled, driver will enable DRRS as expected but if PSR is
> > > enabled
> > > by
> > > debugfs latter it will keep PSR and DRRS enabled causing possible
> > > problems as DRRS will lower the refresh rate while PSR enabled.
> > > 
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108341
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.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 | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index 853e3f1370a0..bfc6a08b5cf4 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -904,8 +904,11 @@ int intel_psr_set_debugfs_mode(struct
> > > drm_i915_private *dev_priv,
> > >  
> > >  	intel_psr_irq_control(dev_priv, dev_priv->psr.debug);
> > >  
> > > -	if (dev_priv->psr.prepared && enable)
> > > +	if (dev_priv->psr.prepared && enable) {
> > > +		if (crtc_state)
> > > +			intel_edp_drrs_disable(dp, crtc_state);
> > >  		intel_psr_enable_locked(dev_priv, crtc_state);
> > > +	}
> > >  
> > >  	mutex_unlock(&dev_priv->psr.lock);
> > >  	return ret;
> > 
> > I've considered this, but I thought it was a feature, not a bug.
> > It's
> > a pain to track
> > how we handle this as intended.
> > 
> > kms_frontbuffer_tracking is also controlling DRRS during the test,
> > so
> > perhaps simply
> > fix the test?
> > 
> > It seems the no_drrs test simply checks that if PSR is enabled, we
> > don't have drrs
> > enabled. We probably care about the default configuration, so I
> > would
> > simply disable
> > the pipe, update the PSR flag, and then start running the tests.
> > Else
> > the only thing
> > we test is that debugfs disables DRRS. Not that the default modeset
> > path prevents
> > PSR and DRRS simultaneously.
> 
> 
> Yeah, I think we should force a modeset from debugs to test the
> default
> modeset path, fix the tests I think is a bad idea as it could leave
> some test cases unhandled.

Maarten,

Can you comment on the Jose's suggestion above? I recall the idea
behind your patches was to avoid modesets while enabling and disabling
PSR. Jose's latest version otoh forces a modeset.

-DK

> 
> Also looking at DRRS it looks complete broken for page flips, it
> would
> kept set to DRRS_LOW forever.
> 
> 
> > 
> > ~Maarten
> > 
> > Maybe something like below?
> > 
> > Perhaps move the drrs manipulation functions from
> > kms_frontbuffer_tracking to lib/kms_psr.c
> > 
> > ----8<-------
> > diff --git a/tests/kms_psr.c b/tests/kms_psr.c
> > index 9767f475bf23..ffc356df06ce 100644
> > --- a/tests/kms_psr.c
> > +++ b/tests/kms_psr.c
> > @@ -414,9 +414,6 @@ int main(int argc, char *argv[])
> >  		kmstest_set_vt_graphics_mode();
> >  		data.devid = intel_get_drm_devid(data.drm_fd);
> >  
> > -		if (!data.with_psr_disabled)
> > -			psr_enable(data.debugfs_fd);
> > -
> >  		igt_require_f(sink_support(&data),
> >  			      "Sink does not support PSR\n");
> >  
> > @@ -428,18 +425,25 @@ int main(int argc, char *argv[])
> >  	}
> >  
> >  	igt_subtest("basic") {
> > -		setup_test_plane(&data, DRM_PLANE_TYPE_PRIMARY);
> > -		igt_assert(psr_wait_entry_if_enabled(&data));
> > -		test_cleanup(&data);
> > -	}
> > +		/* Disable display to get a default setup. */
> > +		igt_display_commit2(&data.display,
> > data.display.is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
> > +
> > +		if (!data.with_psr_disabled)
> > +			psr_enable(data.debugfs_fd);
> >  
> > -	igt_subtest("no_drrs") {
> >  		setup_test_plane(&data, DRM_PLANE_TYPE_PRIMARY);
> >  		igt_assert(psr_wait_entry_if_enabled(&data));
> >  		igt_assert(drrs_disabled(&data));
> >  		test_cleanup(&data);
> >  	}
> >  
> > +	igt_fixture {
> > +		drrs_disable();
> > +
> > +		if (!data.with_psr_disabled)
> > +			psr_enable(data.debugfs_fd);
> > +	}
> > +
> >  	for (op = PAGE_FLIP; op <= RENDER; op++) {
> >  		igt_subtest_f("primary_%s", op_str(op)) {
> >  			data.op = op;
> > 

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

Re: [PATCH 7/7] drm/i915/psr: Disable DRRS if enabled when enabling PSR from debugfs

[Dhinakaran Pandiyan]

On Mon, 2018-11-12 at 11:17 +0100, Maarten Lankhorst wrote:
> Op 09-11-18 om 21:20 schreef José Roberto de Souza:
> > If panel supports DRRS and PSR and if driver is loaded without PSR
> > enabled, driver will enable DRRS as expected but if PSR is enabled
> > by
> > debugfs latter it will keep PSR and DRRS enabled causing possible
> > problems as DRRS will lower the refresh rate while PSR enabled.
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108341
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.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 | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 853e3f1370a0..bfc6a08b5cf4 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -904,8 +904,11 @@ int intel_psr_set_debugfs_mode(struct
> > drm_i915_private *dev_priv,
> >  
> >  	intel_psr_irq_control(dev_priv, dev_priv->psr.debug);
> >  
> > -	if (dev_priv->psr.prepared && enable)
> > +	if (dev_priv->psr.prepared && enable) {
> > +		if (crtc_state)
> > +			intel_edp_drrs_disable(dp, crtc_state);
> >  		intel_psr_enable_locked(dev_priv, crtc_state);
> > +	}
> >  
> >  	mutex_unlock(&dev_priv->psr.lock);
> >  	return ret;
> 
> I've considered this, but I thought it was a feature, not a bug. It's
> a pain to track
> how we handle this as intended.
> 
> kms_frontbuffer_tracking is also controlling DRRS during the test, so
> perhaps simply
> fix the test?
> 
> It seems the no_drrs test simply checks that if PSR is enabled, we
> don't have drrs
> enabled. We probably care about the default configuration, so I would
> simply disable
> the pipe, update the PSR flag, and then start running the tests. Else
> the only thing
> we test is that debugfs disables DRRS. Not that the default modeset
> path prevents
> PSR and DRRS simultaneously.
> 
> ~Maarten
> 
> Maybe something like below?
> 
> Perhaps move the drrs manipulation functions from
> kms_frontbuffer_tracking to lib/kms_psr.c
> 
> ----8<-------
> diff --git a/tests/kms_psr.c b/tests/kms_psr.c
> index 9767f475bf23..ffc356df06ce 100644
> --- a/tests/kms_psr.c
> +++ b/tests/kms_psr.c
> @@ -414,9 +414,6 @@ int main(int argc, char *argv[])
>  		kmstest_set_vt_graphics_mode();
>  		data.devid = intel_get_drm_devid(data.drm_fd);
>  
> -		if (!data.with_psr_disabled)
> -			psr_enable(data.debugfs_fd);
> -
>  		igt_require_f(sink_support(&data),
>  			      "Sink does not support PSR\n");
>  
> @@ -428,18 +425,25 @@ int main(int argc, char *argv[])
>  	}
>  
>  	igt_subtest("basic") {
> -		setup_test_plane(&data, DRM_PLANE_TYPE_PRIMARY);
> -		igt_assert(psr_wait_entry_if_enabled(&data));
> -		test_cleanup(&data);
> -	}
> +		/* Disable display to get a default setup. */
> +		igt_display_commit2(&data.display,
> data.display.is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
> +
> +		if (!data.with_psr_disabled)
> +			psr_enable(data.debugfs_fd);
>  
> -	igt_subtest("no_drrs") {
>  		setup_test_plane(&data, DRM_PLANE_TYPE_PRIMARY);
>  		igt_assert(psr_wait_entry_if_enabled(&data));
>  		igt_assert(drrs_disabled(&data));
>  		test_cleanup(&data);
This makes a lot more sense to me, ensuring that DRRS does not get
enabled in the default code path was the goal of the no-drrs test.

-DK

>  	}
>  
> +	igt_fixture {
> +		drrs_disable();
> +
> +		if (!data.with_psr_disabled)
> +			psr_enable(data.debugfs_fd);
> +	}
> +
>  	for (op = PAGE_FLIP; op <= RENDER; op++) {
>  		igt_subtest_f("primary_%s", op_str(op)) {
>  			data.op = op;
> 

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

Re: [PATCH 7/7] drm/i915/psr: Disable DRRS if enabled when enabling PSR from debugfs

[Souza, Jose]

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

On Tue, 2018-12-11 at 14:02 -0800, Dhinakaran Pandiyan wrote:
> On Mon, 2018-11-12 at 11:17 +0100, Maarten Lankhorst wrote:
> > Op 09-11-18 om 21:20 schreef José Roberto de Souza:
> > > If panel supports DRRS and PSR and if driver is loaded without
> > > PSR
> > > enabled, driver will enable DRRS as expected but if PSR is
> > > enabled
> > > by
> > > debugfs latter it will keep PSR and DRRS enabled causing possible
> > > problems as DRRS will lower the refresh rate while PSR enabled.
> > > 
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108341
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.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 | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index 853e3f1370a0..bfc6a08b5cf4 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -904,8 +904,11 @@ int intel_psr_set_debugfs_mode(struct
> > > drm_i915_private *dev_priv,
> > >  
> > >  	intel_psr_irq_control(dev_priv, dev_priv->psr.debug);
> > >  
> > > -	if (dev_priv->psr.prepared && enable)
> > > +	if (dev_priv->psr.prepared && enable) {
> > > +		if (crtc_state)
> > > +			intel_edp_drrs_disable(dp, crtc_state);
> > >  		intel_psr_enable_locked(dev_priv, crtc_state);
> > > +	}
> > >  
> > >  	mutex_unlock(&dev_priv->psr.lock);
> > >  	return ret;
> > 
> > I've considered this, but I thought it was a feature, not a bug.
> > It's
> > a pain to track
> > how we handle this as intended.
> > 
> > kms_frontbuffer_tracking is also controlling DRRS during the test,
> > so
> > perhaps simply
> > fix the test?
> > 
> > It seems the no_drrs test simply checks that if PSR is enabled, we
> > don't have drrs
> > enabled. We probably care about the default configuration, so I
> > would
> > simply disable
> > the pipe, update the PSR flag, and then start running the tests.
> > Else
> > the only thing
> > we test is that debugfs disables DRRS. Not that the default modeset
> > path prevents
> > PSR and DRRS simultaneously.
> > 
> > ~Maarten
> > 
> > Maybe something like below?
> > 
> > Perhaps move the drrs manipulation functions from
> > kms_frontbuffer_tracking to lib/kms_psr.c
> > 
> > ----8<-------
> > diff --git a/tests/kms_psr.c b/tests/kms_psr.c
> > index 9767f475bf23..ffc356df06ce 100644
> > --- a/tests/kms_psr.c
> > +++ b/tests/kms_psr.c
> > @@ -414,9 +414,6 @@ int main(int argc, char *argv[])
> >  		kmstest_set_vt_graphics_mode();
> >  		data.devid = intel_get_drm_devid(data.drm_fd);
> >  
> > -		if (!data.with_psr_disabled)
> > -			psr_enable(data.debugfs_fd);
> > -
> >  		igt_require_f(sink_support(&data),
> >  			      "Sink does not support PSR\n");
> >  
> > @@ -428,18 +425,25 @@ int main(int argc, char *argv[])
> >  	}
> >  
> >  	igt_subtest("basic") {
> > -		setup_test_plane(&data, DRM_PLANE_TYPE_PRIMARY);
> > -		igt_assert(psr_wait_entry_if_enabled(&data));
> > -		test_cleanup(&data);
> > -	}
> > +		/* Disable display to get a default setup. */
> > +		igt_display_commit2(&data.display,
> > data.display.is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
> > +
> > +		if (!data.with_psr_disabled)
> > +			psr_enable(data.debugfs_fd);
> >  
> > -	igt_subtest("no_drrs") {
> >  		setup_test_plane(&data, DRM_PLANE_TYPE_PRIMARY);
> >  		igt_assert(psr_wait_entry_if_enabled(&data));
> >  		igt_assert(drrs_disabled(&data));
> >  		test_cleanup(&data);
> This makes a lot more sense to me, ensuring that DRRS does not get
> enabled in the default code path was the goal of the no-drrs test.

The problem with this approach is if user wants to run just one test
the basic test will not run and DRRS will be kept on, it will not fail
the no_drrs test but DRRS could interfere with other PSR tests.

If the call to disable display is moved to igt_fixture() it would work
fine but in terms of modesets this approach have just one more modeset
than forcing a modeset every time we write to PSR i915_edp_psr_debug,
for just the cost of one modeset in my opnion is better drop the
aditional PSR code path and just test the main one.

> 
> -DK
> 
> >  	}
> >  
> > +	igt_fixture {
> > +		drrs_disable();
> > +
> > +		if (!data.with_psr_disabled)
> > +			psr_enable(data.debugfs_fd);
> > +	}
> > +
> >  	for (op = PAGE_FLIP; op <= RENDER; op++) {
> >  		igt_subtest_f("primary_%s", op_str(op)) {
> >  			data.op = op;
> > 

[-- 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 7/7] drm/i915/psr: Disable DRRS if enabled when enabling PSR from debugfs

[Dhinakaran Pandiyan]

On Wed, 2018-12-12 at 05:02 -0800, Souza, Jose wrote:
> On Tue, 2018-12-11 at 14:02 -0800, Dhinakaran Pandiyan wrote:
> > On Mon, 2018-11-12 at 11:17 +0100, Maarten Lankhorst wrote:
> > > Op 09-11-18 om 21:20 schreef José Roberto de Souza:
> > > > If panel supports DRRS and PSR and if driver is loaded without
> > > > PSR
> > > > enabled, driver will enable DRRS as expected but if PSR is
> > > > enabled
> > > > by
> > > > debugfs latter it will keep PSR and DRRS enabled causing
> > > > possible
> > > > problems as DRRS will lower the refresh rate while PSR enabled.
> > > > 
> > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108341
> > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.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 | 5 ++++-
> > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > index 853e3f1370a0..bfc6a08b5cf4 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -904,8 +904,11 @@ int intel_psr_set_debugfs_mode(struct
> > > > drm_i915_private *dev_priv,
> > > >  
> > > >  	intel_psr_irq_control(dev_priv, dev_priv->psr.debug);
> > > >  
> > > > -	if (dev_priv->psr.prepared && enable)
> > > > +	if (dev_priv->psr.prepared && enable) {
> > > > +		if (crtc_state)
> > > > +			intel_edp_drrs_disable(dp, crtc_state);
> > > >  		intel_psr_enable_locked(dev_priv, crtc_state);
> > > > +	}
> > > >  
> > > >  	mutex_unlock(&dev_priv->psr.lock);
> > > >  	return ret;
> > > 
> > > I've considered this, but I thought it was a feature, not a bug.
> > > It's
> > > a pain to track
> > > how we handle this as intended.
> > > 
> > > kms_frontbuffer_tracking is also controlling DRRS during the
> > > test,
> > > so
> > > perhaps simply
> > > fix the test?
> > > 
> > > It seems the no_drrs test simply checks that if PSR is enabled,
> > > we
> > > don't have drrs
> > > enabled. We probably care about the default configuration, so I
> > > would
> > > simply disable
> > > the pipe, update the PSR flag, and then start running the tests.
> > > Else
> > > the only thing
> > > we test is that debugfs disables DRRS. Not that the default
> > > modeset
> > > path prevents
> > > PSR and DRRS simultaneously.
> > > 
> > > ~Maarten
> > > 
> > > Maybe something like below?
> > > 
> > > Perhaps move the drrs manipulation functions from
> > > kms_frontbuffer_tracking to lib/kms_psr.c
> > > 
> > > ----8<-------
> > > diff --git a/tests/kms_psr.c b/tests/kms_psr.c
> > > index 9767f475bf23..ffc356df06ce 100644
> > > --- a/tests/kms_psr.c
> > > +++ b/tests/kms_psr.c
> > > @@ -414,9 +414,6 @@ int main(int argc, char *argv[])
> > >  		kmstest_set_vt_graphics_mode();
> > >  		data.devid = intel_get_drm_devid(data.drm_fd);
> > >  
> > > -		if (!data.with_psr_disabled)
> > > -			psr_enable(data.debugfs_fd);
> > > -
> > >  		igt_require_f(sink_support(&data),
> > >  			      "Sink does not support PSR\n");
> > >  
> > > @@ -428,18 +425,25 @@ int main(int argc, char *argv[])
> > >  	}
> > >  
> > >  	igt_subtest("basic") {
> > > -		setup_test_plane(&data, DRM_PLANE_TYPE_PRIMARY);
> > > -		igt_assert(psr_wait_entry_if_enabled(&data));
> > > -		test_cleanup(&data);
> > > -	}
> > > +		/* Disable display to get a default setup. */
> > > +		igt_display_commit2(&data.display,
> > > data.display.is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
> > > +
> > > +		if (!data.with_psr_disabled)
> > > +			psr_enable(data.debugfs_fd);
> > >  
> > > -	igt_subtest("no_drrs") {
> > >  		setup_test_plane(&data, DRM_PLANE_TYPE_PRIMARY);
> > >  		igt_assert(psr_wait_entry_if_enabled(&data));
> > >  		igt_assert(drrs_disabled(&data));
> > >  		test_cleanup(&data);
> > 
> > This makes a lot more sense to me, ensuring that DRRS does not get
> > enabled in the default code path was the goal of the no-drrs test.
> 
> The problem with this approach is if user wants to run just one test
> the basic test will not run and DRRS will be kept on, it will not
> fail
> the no_drrs test but DRRS could interfere with other PSR tests.
Disable DRRS for the other sub-tests, doesn't this work?
	echo 0 >  /sys/kernel/debug/dri/0/i915_drrs_ctl
> 
> If the call to disable display is moved to igt_fixture() it would
> work
> fine but in terms of modesets this approach have just one more
> modeset
> than forcing a modeset every time we write to PSR i915_edp_psr_debug,
> for just the cost of one modeset in my opnion is better drop the
> aditional PSR code path and just test the main one.
> 
> > 
> > -DK
> > 
> > >  	}
> > >  
> > > +	igt_fixture {
> > > +		drrs_disable();
> > > +
> > > +		if (!data.with_psr_disabled)
> > > +			psr_enable(data.debugfs_fd);
> > > +	}
> > > +
> > >  	for (op = PAGE_FLIP; op <= RENDER; op++) {
> > >  		igt_subtest_f("primary_%s", op_str(op)) {
> > >  			data.op = op;
> > > 

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

Re: [PATCH 7/7] drm/i915/psr: Disable DRRS if enabled when enabling PSR from debugfs

[Souza, Jose]

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

On Wed, 2018-12-12 at 17:11 -0800, Dhinakaran Pandiyan wrote:
> On Wed, 2018-12-12 at 05:02 -0800, Souza, Jose wrote:
> > On Tue, 2018-12-11 at 14:02 -0800, Dhinakaran Pandiyan wrote:
> > > On Mon, 2018-11-12 at 11:17 +0100, Maarten Lankhorst wrote:
> > > > Op 09-11-18 om 21:20 schreef José Roberto de Souza:
> > > > > If panel supports DRRS and PSR and if driver is loaded
> > > > > without
> > > > > PSR
> > > > > enabled, driver will enable DRRS as expected but if PSR is
> > > > > enabled
> > > > > by
> > > > > debugfs latter it will keep PSR and DRRS enabled causing
> > > > > possible
> > > > > problems as DRRS will lower the refresh rate while PSR
> > > > > enabled.
> > > > > 
> > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108341
> > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.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 | 5 ++++-
> > > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > > index 853e3f1370a0..bfc6a08b5cf4 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > > @@ -904,8 +904,11 @@ int intel_psr_set_debugfs_mode(struct
> > > > > drm_i915_private *dev_priv,
> > > > >  
> > > > >  	intel_psr_irq_control(dev_priv, dev_priv->psr.debug);
> > > > >  
> > > > > -	if (dev_priv->psr.prepared && enable)
> > > > > +	if (dev_priv->psr.prepared && enable) {
> > > > > +		if (crtc_state)
> > > > > +			intel_edp_drrs_disable(dp, crtc_state);
> > > > >  		intel_psr_enable_locked(dev_priv, crtc_state);
> > > > > +	}
> > > > >  
> > > > >  	mutex_unlock(&dev_priv->psr.lock);
> > > > >  	return ret;
> > > > 
> > > > I've considered this, but I thought it was a feature, not a
> > > > bug.
> > > > It's
> > > > a pain to track
> > > > how we handle this as intended.
> > > > 
> > > > kms_frontbuffer_tracking is also controlling DRRS during the
> > > > test,
> > > > so
> > > > perhaps simply
> > > > fix the test?
> > > > 
> > > > It seems the no_drrs test simply checks that if PSR is enabled,
> > > > we
> > > > don't have drrs
> > > > enabled. We probably care about the default configuration, so I
> > > > would
> > > > simply disable
> > > > the pipe, update the PSR flag, and then start running the
> > > > tests.
> > > > Else
> > > > the only thing
> > > > we test is that debugfs disables DRRS. Not that the default
> > > > modeset
> > > > path prevents
> > > > PSR and DRRS simultaneously.
> > > > 
> > > > ~Maarten
> > > > 
> > > > Maybe something like below?
> > > > 
> > > > Perhaps move the drrs manipulation functions from
> > > > kms_frontbuffer_tracking to lib/kms_psr.c
> > > > 
> > > > ----8<-------
> > > > diff --git a/tests/kms_psr.c b/tests/kms_psr.c
> > > > index 9767f475bf23..ffc356df06ce 100644
> > > > --- a/tests/kms_psr.c
> > > > +++ b/tests/kms_psr.c
> > > > @@ -414,9 +414,6 @@ int main(int argc, char *argv[])
> > > >  		kmstest_set_vt_graphics_mode();
> > > >  		data.devid = intel_get_drm_devid(data.drm_fd);
> > > >  
> > > > -		if (!data.with_psr_disabled)
> > > > -			psr_enable(data.debugfs_fd);
> > > > -
> > > >  		igt_require_f(sink_support(&data),
> > > >  			      "Sink does not support PSR\n");
> > > >  
> > > > @@ -428,18 +425,25 @@ int main(int argc, char *argv[])
> > > >  	}
> > > >  
> > > >  	igt_subtest("basic") {
> > > > -		setup_test_plane(&data,
> > > > DRM_PLANE_TYPE_PRIMARY);
> > > > -		igt_assert(psr_wait_entry_if_enabled(&data));
> > > > -		test_cleanup(&data);
> > > > -	}
> > > > +		/* Disable display to get a default setup. */
> > > > +		igt_display_commit2(&data.display,
> > > > data.display.is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
> > > > +
> > > > +		if (!data.with_psr_disabled)
> > > > +			psr_enable(data.debugfs_fd);
> > > >  
> > > > -	igt_subtest("no_drrs") {
> > > >  		setup_test_plane(&data,
> > > > DRM_PLANE_TYPE_PRIMARY);
> > > >  		igt_assert(psr_wait_entry_if_enabled(&data));
> > > >  		igt_assert(drrs_disabled(&data));
> > > >  		test_cleanup(&data);
> > > 
> > > This makes a lot more sense to me, ensuring that DRRS does not
> > > get
> > > enabled in the default code path was the goal of the no-drrs
> > > test.
> > 
> > The problem with this approach is if user wants to run just one
> > test
> > the basic test will not run and DRRS will be kept on, it will not
> > fail
> > the no_drrs test but DRRS could interfere with other PSR tests.
> Disable DRRS for the other sub-tests, doesn't this work?
> 	echo 0 >  /sys/kernel/debug/dri/0/i915_drrs_ctl

It would work but with this we are not testing or stressing any code
path that users will go through.

> > If the call to disable display is moved to igt_fixture() it would
> > work
> > fine but in terms of modesets this approach have just one more
> > modeset
> > than forcing a modeset every time we write to PSR
> > i915_edp_psr_debug,
> > for just the cost of one modeset in my opnion is better drop the
> > aditional PSR code path and just test the main one.
> > 
> > > -DK
> > > 
> > > >  	}
> > > >  
> > > > +	igt_fixture {
> > > > +		drrs_disable();
> > > > +
> > > > +		if (!data.with_psr_disabled)
> > > > +			psr_enable(data.debugfs_fd);
> > > > +	}
> > > > +
> > > >  	for (op = PAGE_FLIP; op <= RENDER; op++) {
> > > >  		igt_subtest_f("primary_%s", op_str(op)) {
> > > >  			data.op = op;
> > > > 

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