[PATCH v2 1/6] drm/i915/psr: Use intel_psr_exit() in intel_psr_disable_source()

[PATCH v2 1/6] drm/i915/psr: Use intel_psr_exit() in intel_psr_disable_source()

[José Roberto de Souza]

Both functions have the same code to disable PSR, so let's reuse that
code instead of duplicate.

Suggested-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@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 | 50 ++++++++++++++------------------
 1 file changed, 21 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 423cdf84059c..f698b3f45c6d 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -657,6 +657,25 @@ void intel_psr_enable(struct intel_dp *intel_dp,
 	mutex_unlock(&dev_priv->psr.lock);
 }
 
+static void intel_psr_exit(struct drm_i915_private *dev_priv)
+{
+	u32 val;
+
+	if (!dev_priv->psr.active)
+		return;
+
+	if (dev_priv->psr.psr2_enabled) {
+		val = I915_READ(EDP_PSR2_CTL);
+		WARN_ON(!(val & EDP_PSR2_ENABLE));
+		I915_WRITE(EDP_PSR2_CTL, val & ~EDP_PSR2_ENABLE);
+	} else {
+		val = I915_READ(EDP_PSR_CTL);
+		WARN_ON(!(val & EDP_PSR_ENABLE));
+		I915_WRITE(EDP_PSR_CTL, val & ~EDP_PSR_ENABLE);
+	}
+	dev_priv->psr.active = false;
+}
+
 static void
 intel_psr_disable_source(struct intel_dp *intel_dp)
 {
@@ -666,20 +685,14 @@ intel_psr_disable_source(struct intel_dp *intel_dp)
 		i915_reg_t psr_status;
 		u32 psr_status_mask;
 
+		intel_psr_exit(dev_priv);
+
 		if (dev_priv->psr.psr2_enabled) {
 			psr_status = EDP_PSR2_STATUS;
 			psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
-
-			I915_WRITE(EDP_PSR2_CTL,
-				   I915_READ(EDP_PSR2_CTL) &
-				   ~(EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE));
-
 		} else {
 			psr_status = EDP_PSR_STATUS;
 			psr_status_mask = EDP_PSR_STATUS_STATE_MASK;
-
-			I915_WRITE(EDP_PSR_CTL,
-				   I915_READ(EDP_PSR_CTL) & ~EDP_PSR_ENABLE);
 		}
 
 		/* Wait till PSR is idle */
@@ -687,8 +700,6 @@ intel_psr_disable_source(struct intel_dp *intel_dp)
 					    psr_status, psr_status_mask, 0,
 					    2000))
 			DRM_ERROR("Timed out waiting for PSR Idle State\n");
-
-		dev_priv->psr.active = false;
 	} else {
 		if (dev_priv->psr.psr2_enabled)
 			WARN_ON(I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE);
@@ -926,25 +937,6 @@ static void intel_psr_work(struct work_struct *work)
 	mutex_unlock(&dev_priv->psr.lock);
 }
 
-static void intel_psr_exit(struct drm_i915_private *dev_priv)
-{
-	u32 val;
-
-	if (!dev_priv->psr.active)
-		return;
-
-	if (dev_priv->psr.psr2_enabled) {
-		val = I915_READ(EDP_PSR2_CTL);
-		WARN_ON(!(val & EDP_PSR2_ENABLE));
-		I915_WRITE(EDP_PSR2_CTL, val & ~EDP_PSR2_ENABLE);
-	} else {
-		val = I915_READ(EDP_PSR_CTL);
-		WARN_ON(!(val & EDP_PSR_ENABLE));
-		I915_WRITE(EDP_PSR_CTL, val & ~EDP_PSR_ENABLE);
-	}
-	dev_priv->psr.active = false;
-}
-
 /**
  * intel_psr_invalidate - Invalidade PSR
  * @dev_priv: i915 device
-- 
2.19.1

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

[PATCH v2 2/6] drm/i915/psr: Always wait for idle state when disabling PSR

[José Roberto de Souza]

It should always wait for idle state when disabling PSR because PSR
could be inactive due a call to intel_psr_exit() and while PSR is
still being disabled asynchronously userspace could change the
modeset causing a call to psr_disable() that will not wait for PSR
idle and then PSR will be enabled again while PSR is still not idle.

v2: rebased on top of the patch reusing psr_exit()

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

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index f698b3f45c6d..16d0e3df7de0 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -661,8 +661,12 @@ static void intel_psr_exit(struct drm_i915_private *dev_priv)
 {
 	u32 val;
 
-	if (!dev_priv->psr.active)
+	if (!dev_priv->psr.active) {
+		if (INTEL_GEN(dev_priv) >= 9)
+			WARN_ON(I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE);
+		WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
 		return;
+	}
 
 	if (dev_priv->psr.psr2_enabled) {
 		val = I915_READ(EDP_PSR2_CTL);
@@ -680,32 +684,23 @@ static void
 intel_psr_disable_source(struct intel_dp *intel_dp)
 {
 	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
+	i915_reg_t psr_status;
+	u32 psr_status_mask;
 
-	if (dev_priv->psr.active) {
-		i915_reg_t psr_status;
-		u32 psr_status_mask;
-
-		intel_psr_exit(dev_priv);
+	intel_psr_exit(dev_priv);
 
-		if (dev_priv->psr.psr2_enabled) {
-			psr_status = EDP_PSR2_STATUS;
-			psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
-		} else {
-			psr_status = EDP_PSR_STATUS;
-			psr_status_mask = EDP_PSR_STATUS_STATE_MASK;
-		}
-
-		/* Wait till PSR is idle */
-		if (intel_wait_for_register(dev_priv,
-					    psr_status, psr_status_mask, 0,
-					    2000))
-			DRM_ERROR("Timed out waiting for PSR Idle State\n");
+	if (dev_priv->psr.psr2_enabled) {
+		psr_status = EDP_PSR2_STATUS;
+		psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
 	} else {
-		if (dev_priv->psr.psr2_enabled)
-			WARN_ON(I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE);
-		else
-			WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
+		psr_status = EDP_PSR_STATUS;
+		psr_status_mask = EDP_PSR_STATUS_STATE_MASK;
 	}
+
+	/* Wait till PSR is idle */
+	if (intel_wait_for_register(dev_priv, psr_status, psr_status_mask, 0,
+				    2000))
+		DRM_ERROR("Timed out waiting for PSR Idle State\n");
 }
 
 static void intel_psr_disable_locked(struct intel_dp *intel_dp)
-- 
2.19.1

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

[PATCH v2 3/6] drm/i915/psr: Move intel_psr_disable_source() code to intel_psr_disable_locked()

[José Roberto de Souza]

In the past we had hooks to configure HW for VLV/CHV too, in the drop
of VLV/CHV support the intel_psr_disable_source() code was not moved
to the caller, so doing it here.

Suggested-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@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 | 25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 16d0e3df7de0..70d4e26e17b5 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -680,13 +680,20 @@ static void intel_psr_exit(struct drm_i915_private *dev_priv)
 	dev_priv->psr.active = false;
 }
 
-static void
-intel_psr_disable_source(struct intel_dp *intel_dp)
+static void intel_psr_disable_locked(struct intel_dp *intel_dp)
 {
 	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
 	i915_reg_t psr_status;
 	u32 psr_status_mask;
 
+	lockdep_assert_held(&dev_priv->psr.lock);
+
+	if (!dev_priv->psr.enabled)
+		return;
+
+	DRM_DEBUG_KMS("Disabling PSR%s\n",
+		      dev_priv->psr.psr2_enabled ? "2" : "1");
+
 	intel_psr_exit(dev_priv);
 
 	if (dev_priv->psr.psr2_enabled) {
@@ -701,20 +708,6 @@ intel_psr_disable_source(struct intel_dp *intel_dp)
 	if (intel_wait_for_register(dev_priv, psr_status, psr_status_mask, 0,
 				    2000))
 		DRM_ERROR("Timed out waiting for PSR Idle State\n");
-}
-
-static void intel_psr_disable_locked(struct intel_dp *intel_dp)
-{
-	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
-
-	lockdep_assert_held(&dev_priv->psr.lock);
-
-	if (!dev_priv->psr.enabled)
-		return;
-
-	DRM_DEBUG_KMS("Disabling PSR%s\n",
-		      dev_priv->psr.psr2_enabled ? "2" : "1");
-	intel_psr_disable_source(intel_dp);
 
 	/* Disable PSR on Sink */
 	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, 0);
-- 
2.19.1

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

[PATCH v2 4/6] 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.

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

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 3017ef037fed..e8ba00dd2c51 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -638,6 +638,7 @@ struct i915_psr {
 	u8 sink_sync_latency;
 	ktime_t last_entry_attempt;
 	ktime_t last_exit;
+	u32 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 70d4e26e17b5..ad09130cb4ad 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -159,10 +159,16 @@ 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));
+
+			spin_lock(&dev_priv->irq_lock);
+			dev_priv->psr.irq_aux_error |= BIT(cpu_transcoder);
+			spin_unlock(&dev_priv->irq_lock);
+
+			schedule_work(&dev_priv->psr.work);
+		}
 
 		if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
 			dev_priv->psr.last_entry_attempt = time_ns;
@@ -893,11 +899,36 @@ 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;
+	u32 irq_aux_error;
+
+	spin_lock_irq(&dev_priv->irq_lock);
+	irq_aux_error = psr->irq_aux_error;
+	psr->irq_aux_error = 0;
+	spin_unlock_irq(&dev_priv->irq_lock);
+
+	/* right now PSR is only enabled in eDP */
+	WARN_ON(irq_aux_error & ~BIT(TRANSCODER_EDP));
+
+	mutex_lock(&psr->lock);
+
+	intel_psr_disable_locked(psr->dp);
+	/* let's make sure that sink is awaken */
+	drm_dp_dpcd_writeb(&psr->dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
+
+	mutex_unlock(&dev_priv->psr.lock);
+}
+
 static void intel_psr_work(struct work_struct *work)
 {
 	struct drm_i915_private *dev_priv =
 		container_of(work, typeof(*dev_priv), psr.work);
 
+	if (READ_ONCE(dev_priv->psr.irq_aux_error))
+		intel_psr_handle_irq(dev_priv);
+
 	mutex_lock(&dev_priv->psr.lock);
 
 	if (!dev_priv->psr.enabled)
-- 
2.19.1

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

[PATCH v2 5/6] 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 13ff89be6ad6..1826d491efd7 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4034,8 +4034,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;
 
@@ -4045,25 +4043,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 v2 6/6] drm/i915: Check PSR errors instead of retrain while PSR is enabled

[José Roberto de Souza]

When a PSR error happens sink also update the link status values
while source do not retrain link as required in the PSR exit
sequence.
So in the short pulse handling it was returning earlier and doing a
full detection and attempting to retrain but it fails because for
most sinks the main link is disabled while PSR is active, before even
check PSR errors.

Just call intel_psr_short_pulse() before
intel_dp_needs_link_retrain() is also not the right fix as
intel_dp_needs_link_retrain() would return true and trigger a full
detection and trying to retrain link while PSR HW is also doing that.

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

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  |  7 +++++++
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 drivers/gpu/drm/i915/intel_psr.c | 15 +++++++++++++++
 3 files changed, 23 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 1826d491efd7..2a21a9e29eb2 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4396,6 +4396,13 @@ intel_dp_needs_link_retrain(struct intel_dp *intel_dp)
 	if (!intel_dp->link_trained)
 		return false;
 
+	/*
+	 * While PSR is enabled, HW will control main-link and retrain when
+	 * exiting PSR
+	 */
+	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 3dea7a1bda7f..147a116cfc71 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1958,6 +1958,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_runtime_pm.c */
 int intel_power_domains_init(struct drm_i915_private *);
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index ad09130cb4ad..e4429f2f3034 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -1138,3 +1138,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

✗ Fi.CI.SPARSE: warning for series starting with [v2,1/6] drm/i915/psr: Use intel_psr_exit() in intel_psr_disable_source()

[Patchwork]

== Series Details ==

Series: series starting with [v2,1/6] drm/i915/psr: Use intel_psr_exit() in intel_psr_disable_source()
URL   : https://patchwork.freedesktop.org/series/50843/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Sparse version: v0.5.2
Commit: drm/i915/psr: Use intel_psr_exit() in intel_psr_disable_source()
Okay!

Commit: drm/i915/psr: Always wait for idle state when disabling PSR
Okay!

Commit: drm/i915/psr: Move intel_psr_disable_source() code to intel_psr_disable_locked()
Okay!

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

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!

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

✓ Fi.CI.BAT: success for series starting with [v2,1/6] drm/i915/psr: Use intel_psr_exit() in intel_psr_disable_source()

[Patchwork]

== Series Details ==

Series: series starting with [v2,1/6] drm/i915/psr: Use intel_psr_exit() in intel_psr_disable_source()
URL   : https://patchwork.freedesktop.org/series/50843/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4967 -> Patchwork_10420 =

== Summary - SUCCESS ==

  No regressions found.

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

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_exec_suspend@basic-s3:
      fi-cfl-8109u:       PASS -> DMESG-WARN (fdo#107345)

    
    ==== Possible fixes ====

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

    
    ==== Warnings ====

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-cfl-8109u:       DMESG-WARN (fdo#107345) -> INCOMPLETE (fdo#108126, fdo#106070)

    
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#106070 https://bugs.freedesktop.org/show_bug.cgi?id=106070
  fdo#107345 https://bugs.freedesktop.org/show_bug.cgi?id=107345
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#108126 https://bugs.freedesktop.org/show_bug.cgi?id=108126


== Participating hosts (45 -> 41) ==

  Additional (1): fi-pnv-d510 
  Missing    (5): fi-ctg-p8600 fi-bsw-cyan fi-ilk-m540 fi-byt-squawks fi-icl-u2 


== Build changes ==

    * Linux: CI_DRM_4967 -> Patchwork_10420

  CI_DRM_4967: 684ab6b2f3f8406c38726f4bca2749cf71b83292 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4673: 54cb1aeb4e50dea9f3abae632e317875d147c4ab @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10420: f6d1a298698cb989b2815418cf3a93ce0f0ecada @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

f6d1a298698c drm/i915: Check PSR errors instead of retrain while PSR is enabled
05c85ce0b5be drm/i915: Avoid a full port detection in the first eDP short pulse
360c870bdbd8 drm/i915: Disable PSR when a PSR aux error happen
41d5f03eb1ca drm/i915/psr: Move intel_psr_disable_source() code to intel_psr_disable_locked()
fe951f1f71bb drm/i915/psr: Always wait for idle state when disabling PSR
c0b0cfb53d72 drm/i915/psr: Use intel_psr_exit() in intel_psr_disable_source()

== Logs ==

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

✓ Fi.CI.IGT: success for series starting with [v2,1/6] drm/i915/psr: Use intel_psr_exit() in intel_psr_disable_source()

[Patchwork]

== Series Details ==

Series: series starting with [v2,1/6] drm/i915/psr: Use intel_psr_exit() in intel_psr_disable_source()
URL   : https://patchwork.freedesktop.org/series/50843/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4967_full -> Patchwork_10420_full =

== Summary - WARNING ==

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

  

== Possible new issues ==

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

  === IGT changes ===

    ==== Warnings ====

    igt@perf_pmu@rc6:
      shard-kbl:          SKIP -> PASS

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_suspend@fence-restore-tiled2untiled:
      shard-skl:          NOTRUN -> INCOMPLETE (fdo#104108, fdo#107773)

    igt@gem_exec_schedule@pi-ringfull-blt:
      shard-skl:          NOTRUN -> FAIL (fdo#103158)

    igt@gem_ppgtt@blt-vs-render-ctx0:
      shard-skl:          NOTRUN -> TIMEOUT (fdo#108039)

    igt@gem_userptr_blits@readonly-unsync:
      shard-skl:          NOTRUN -> INCOMPLETE (fdo#108074)

    igt@kms_addfb_basic@bo-too-small-due-to-tiling:
      shard-snb:          NOTRUN -> INCOMPLETE (fdo#105411)

    igt@kms_atomic_transition@2x-modeset-transitions-nonblocking-fencing:
      shard-glk:          PASS -> INCOMPLETE (k.org#198133, fdo#103359)

    igt@kms_available_modes_crc@available_mode_test_crc:
      shard-apl:          PASS -> FAIL (fdo#106641)

    igt@kms_busy@extended-modeset-hang-newfb-render-b:
      shard-skl:          NOTRUN -> DMESG-WARN (fdo#107956)

    igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-b:
      shard-snb:          NOTRUN -> DMESG-WARN (fdo#107956) +2

    igt@kms_cursor_crc@cursor-128x128-dpms:
      shard-apl:          PASS -> FAIL (fdo#103232)

    igt@kms_cursor_crc@cursor-64x64-dpms:
      shard-glk:          PASS -> FAIL (fdo#103232) +1

    igt@kms_cursor_legacy@cursora-vs-flipa-toggle:
      shard-glk:          PASS -> DMESG-WARN (fdo#106538, fdo#105763)

    igt@kms_draw_crc@draw-method-xrgb2101010-pwrite-xtiled:
      shard-skl:          PASS -> FAIL (fdo#103184)

    igt@kms_flip@flip-vs-expired-vblank-interruptible:
      shard-snb:          NOTRUN -> DMESG-WARN (fdo#107469)

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-pwrite:
      shard-apl:          PASS -> FAIL (fdo#103167) +1

    igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-draw-mmap-cpu:
      shard-glk:          PASS -> FAIL (fdo#103167) +3

    igt@kms_frontbuffer_tracking@fbcpsr-stridechange:
      shard-skl:          NOTRUN -> FAIL (fdo#105683)

    igt@kms_pipe_crc_basic@read-crc-pipe-c:
      shard-skl:          NOTRUN -> FAIL (fdo#107362) +1

    {igt@kms_plane_alpha_blend@pipe-c-constant-alpha-min}:
      shard-skl:          NOTRUN -> FAIL (fdo#108145) +2

    {igt@kms_plane_alpha_blend@pipe-c-coverage-7efc}:
      shard-skl:          NOTRUN -> FAIL (fdo#108146)

    igt@kms_plane_multiple@atomic-pipe-a-tiling-x:
      shard-apl:          PASS -> FAIL (fdo#103166) +2

    igt@kms_plane_multiple@atomic-pipe-a-tiling-y:
      shard-glk:          PASS -> FAIL (fdo#103166) +3

    igt@kms_setmode@basic:
      shard-apl:          PASS -> FAIL (fdo#99912)
      shard-kbl:          PASS -> FAIL (fdo#99912)
      shard-snb:          NOTRUN -> FAIL (fdo#99912)

    igt@perf_pmu@rc6-runtime-pm:
      shard-apl:          PASS -> FAIL (fdo#105010)

    
    ==== Possible fixes ====

    igt@kms_busy@extended-pageflip-hang-newfb-render-c:
      shard-glk:          DMESG-WARN (fdo#107956) -> PASS

    igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-b:
      shard-hsw:          DMESG-WARN (fdo#107956) -> PASS

    igt@kms_color@pipe-a-ctm-max:
      shard-skl:          FAIL -> PASS

    igt@kms_cursor_crc@cursor-128x128-random:
      shard-apl:          FAIL (fdo#103232) -> PASS +2

    igt@kms_cursor_legacy@cursorb-vs-flipb-toggle:
      shard-glk:          DMESG-WARN (fdo#106538, fdo#105763) -> PASS

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-blt:
      shard-apl:          FAIL (fdo#103167) -> PASS +1

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-onoff:
      shard-glk:          FAIL (fdo#103167) -> PASS +3

    igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-pri-indfb-draw-mmap-cpu:
      shard-glk:          DMESG-FAIL (fdo#106538) -> PASS

    igt@kms_frontbuffer_tracking@psr-farfromfence:
      shard-skl:          FAIL (fdo#103167) -> PASS

    {igt@kms_plane_alpha_blend@pipe-a-coverage-7efc}:
      shard-skl:          FAIL (fdo#108145) -> PASS

    {igt@kms_plane_alpha_blend@pipe-b-constant-alpha-max}:
      shard-glk:          FAIL (fdo#108145) -> PASS

    igt@kms_plane_multiple@atomic-pipe-b-tiling-y:
      shard-glk:          FAIL (fdo#103166) -> PASS

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

    igt@pm_rpm@legacy-planes:
      shard-skl:          INCOMPLETE (fdo#107807, fdo#105959) -> PASS

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

  fdo#103158 https://bugs.freedesktop.org/show_bug.cgi?id=103158
  fdo#103166 https://bugs.freedesktop.org/show_bug.cgi?id=103166
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103184 https://bugs.freedesktop.org/show_bug.cgi?id=103184
  fdo#103232 https://bugs.freedesktop.org/show_bug.cgi?id=103232
  fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
  fdo#104108 https://bugs.freedesktop.org/show_bug.cgi?id=104108
  fdo#105010 https://bugs.freedesktop.org/show_bug.cgi?id=105010
  fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
  fdo#105683 https://bugs.freedesktop.org/show_bug.cgi?id=105683
  fdo#105763 https://bugs.freedesktop.org/show_bug.cgi?id=105763
  fdo#105959 https://bugs.freedesktop.org/show_bug.cgi?id=105959
  fdo#106538 https://bugs.freedesktop.org/show_bug.cgi?id=106538
  fdo#106641 https://bugs.freedesktop.org/show_bug.cgi?id=106641
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107469 https://bugs.freedesktop.org/show_bug.cgi?id=107469
  fdo#107773 https://bugs.freedesktop.org/show_bug.cgi?id=107773
  fdo#107807 https://bugs.freedesktop.org/show_bug.cgi?id=107807
  fdo#107956 https://bugs.freedesktop.org/show_bug.cgi?id=107956
  fdo#108039 https://bugs.freedesktop.org/show_bug.cgi?id=108039
  fdo#108074 https://bugs.freedesktop.org/show_bug.cgi?id=108074
  fdo#108145 https://bugs.freedesktop.org/show_bug.cgi?id=108145
  fdo#108146 https://bugs.freedesktop.org/show_bug.cgi?id=108146
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
  k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133


== Participating hosts (6 -> 6) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4967 -> Patchwork_10420

  CI_DRM_4967: 684ab6b2f3f8406c38726f4bca2749cf71b83292 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4673: 54cb1aeb4e50dea9f3abae632e317875d147c4ab @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10420: f6d1a298698cb989b2815418cf3a93ce0f0ecada @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

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

Re: [PATCH v2 5/6] drm/i915: Avoid a full port detection in the first eDP short pulse

[Ville Syrjälä]

On Wed, Oct 10, 2018 at 05:41:23PM -0700, 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>
> ---
>  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 13ff89be6ad6..1826d491efd7 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4034,8 +4034,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;
>  
> @@ -4045,25 +4043,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().

Can't quite parse that.

"Some eDP panels do not set a valid value
 for sink count, so we ignore it."

or something like that perhaps.

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

My earlier suggestion was that we should keep reading this
anyway because some cts maybe required it. Would at least
avoid mixing in two functional changes into once patch.

> +
> +		/*
> +		 * 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 v2 1/6] drm/i915/psr: Use intel_psr_exit() in intel_psr_disable_source()

[Dhinakaran Pandiyan]

On Wednesday, October 10, 2018 5:41:19 PM PDT José Roberto de Souza wrote:
> Both functions have the same code to disable PSR, so let's reuse that
> code instead of duplicate.
> 
> Suggested-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 50 ++++++++++++++------------------
>  1 file changed, 21 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c index 423cdf84059c..f698b3f45c6d 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -657,6 +657,25 @@ void intel_psr_enable(struct intel_dp *intel_dp,
>  	mutex_unlock(&dev_priv->psr.lock);
>  }
> 
> +static void intel_psr_exit(struct drm_i915_private *dev_priv)
> +{
> +	u32 val;
> +
> +	if (!dev_priv->psr.active)
> +		return;
> +
> +	if (dev_priv->psr.psr2_enabled) {
> +		val = I915_READ(EDP_PSR2_CTL);
> +		WARN_ON(!(val & EDP_PSR2_ENABLE));
> +		I915_WRITE(EDP_PSR2_CTL, val & ~EDP_PSR2_ENABLE);
> +	} else {
> +		val = I915_READ(EDP_PSR_CTL);
> +		WARN_ON(!(val & EDP_PSR_ENABLE));
> +		I915_WRITE(EDP_PSR_CTL, val & ~EDP_PSR_ENABLE);
> +	}
> +	dev_priv->psr.active = false;
> +}
> +
>  static void
>  intel_psr_disable_source(struct intel_dp *intel_dp)
>  {
> @@ -666,20 +685,14 @@ intel_psr_disable_source(struct intel_dp *intel_dp)
>  		i915_reg_t psr_status;
>  		u32 psr_status_mask;
> 
> +		intel_psr_exit(dev_priv);
> +
>  		if (dev_priv->psr.psr2_enabled) {
>  			psr_status = EDP_PSR2_STATUS;
>  			psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
> -
> -			I915_WRITE(EDP_PSR2_CTL,
> -				   I915_READ(EDP_PSR2_CTL) &
> -				   ~(EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE));
> -
>  		} else {
>  			psr_status = EDP_PSR_STATUS;
>  			psr_status_mask = EDP_PSR_STATUS_STATE_MASK;
> -
> -			I915_WRITE(EDP_PSR_CTL,
> -				   I915_READ(EDP_PSR_CTL) & ~EDP_PSR_ENABLE);
>  		}
> 
>  		/* Wait till PSR is idle */
> @@ -687,8 +700,6 @@ intel_psr_disable_source(struct intel_dp *intel_dp)
>  					    psr_status, psr_status_mask, 0,
>  					    2000))
>  			DRM_ERROR("Timed out waiting for PSR Idle State\n");
> -
> -		dev_priv->psr.active = false;
>  	} else {
>  		if (dev_priv->psr.psr2_enabled)
>  			WARN_ON(I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE);
> @@ -926,25 +937,6 @@ static void intel_psr_work(struct work_struct *work)
>  	mutex_unlock(&dev_priv->psr.lock);
>  }
> 
> -static void intel_psr_exit(struct drm_i915_private *dev_priv)
> -{
> -	u32 val;
> -
> -	if (!dev_priv->psr.active)
> -		return;
> -
> -	if (dev_priv->psr.psr2_enabled) {
> -		val = I915_READ(EDP_PSR2_CTL);
> -		WARN_ON(!(val & EDP_PSR2_ENABLE));
> -		I915_WRITE(EDP_PSR2_CTL, val & ~EDP_PSR2_ENABLE);
> -	} else {
> -		val = I915_READ(EDP_PSR_CTL);
> -		WARN_ON(!(val & EDP_PSR_ENABLE));
> -		I915_WRITE(EDP_PSR_CTL, val & ~EDP_PSR_ENABLE);
> -	}
> -	dev_priv->psr.active = false;
> -}
> -
>  /**
>   * intel_psr_invalidate - Invalidade PSR
>   * @dev_priv: i915 device




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

Re: [PATCH v2 2/6] drm/i915/psr: Always wait for idle state when disabling PSR

[Dhinakaran Pandiyan]

On Wednesday, October 10, 2018 5:41:20 PM PDT José Roberto de Souza wrote:
> It should always wait for idle state when disabling PSR because PSR
> could be inactive due a call to intel_psr_exit() and while PSR is
> still being disabled asynchronously userspace could change the
> modeset causing a call to psr_disable() that will not wait for PSR
> idle and then PSR will be enabled again while PSR is still not idle.
> 
> v2: rebased on top of the patch reusing psr_exit()
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 41 ++++++++++++++------------------
>  1 file changed, 18 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c index f698b3f45c6d..16d0e3df7de0 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -661,8 +661,12 @@ static void intel_psr_exit(struct drm_i915_private
> *dev_priv) {
>  	u32 val;
> 
> -	if (!dev_priv->psr.active)
> +	if (!dev_priv->psr.active) {
> +		if (INTEL_GEN(dev_priv) >= 9)
> +			WARN_ON(I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE);
> +		WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);

Do we really need so many WARN_ONs needing MMIO reads? There are almost as 
many lines of warnings as code in this function. I think at this point, we 
know this code works and the warnings in psr_exit() should be removed.

>  		return;
> +	}
> 
>  	if (dev_priv->psr.psr2_enabled) {
>  		val = I915_READ(EDP_PSR2_CTL);
> @@ -680,32 +684,23 @@ static void
>  intel_psr_disable_source(struct intel_dp *intel_dp)
>  {
>  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> +	i915_reg_t psr_status;
> +	u32 psr_status_mask;
> 
> -	if (dev_priv->psr.active) {
> -		i915_reg_t psr_status;
> -		u32 psr_status_mask;
> -
> -		intel_psr_exit(dev_priv);
> +	intel_psr_exit(dev_priv);
> 
> -		if (dev_priv->psr.psr2_enabled) {
> -			psr_status = EDP_PSR2_STATUS;
> -			psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
> -		} else {
> -			psr_status = EDP_PSR_STATUS;
> -			psr_status_mask = EDP_PSR_STATUS_STATE_MASK;
> -		}
> -
> -		/* Wait till PSR is idle */
> -		if (intel_wait_for_register(dev_priv,
> -					    psr_status, psr_status_mask, 0,
> -					    2000))
> -			DRM_ERROR("Timed out waiting for PSR Idle State\n");
> +	if (dev_priv->psr.psr2_enabled) {
> +		psr_status = EDP_PSR2_STATUS;
> +		psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
>  	} else {
> -		if (dev_priv->psr.psr2_enabled)
> -			WARN_ON(I915_READ(EDP_PSR2_CTL) & EDP_PSR2_ENABLE);
> -		else
> -			WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
> +		psr_status = EDP_PSR_STATUS;
> +		psr_status_mask = EDP_PSR_STATUS_STATE_MASK;
>  	}
> +
> +	/* Wait till PSR is idle */
> +	if (intel_wait_for_register(dev_priv, psr_status, psr_status_mask, 0,
> +				    2000))
> +		DRM_ERROR("Timed out waiting for PSR Idle State\n");
>  }
> 
>  static void intel_psr_disable_locked(struct intel_dp *intel_dp)




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

Re: [PATCH v2 2/6] drm/i915/psr: Always wait for idle state when disabling PSR

[Dhinakaran Pandiyan]

On Fri, 2018-10-19 at 13:42 -0700, Dhinakaran Pandiyan wrote:
> On Wednesday, October 10, 2018 5:41:20 PM PDT José Roberto de Souza
> wrote:
> > It should always wait for idle state when disabling PSR because PSR
> > could be inactive due a call to intel_psr_exit() and while PSR is
> > still being disabled asynchronously userspace could change the
> > modeset causing a call to psr_disable() that will not wait for PSR
> > idle and then PSR will be enabled again while PSR is still not
> > idle.
> > 
> > v2: rebased on top of the patch reusing psr_exit()
> > 
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_psr.c | 41 ++++++++++++++------------
> > ------
> >  1 file changed, 18 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c index f698b3f45c6d..16d0e3df7de0
> > 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -661,8 +661,12 @@ static void intel_psr_exit(struct
> > drm_i915_private
> > *dev_priv) {
> >  	u32 val;
> > 
> > -	if (!dev_priv->psr.active)
> > +	if (!dev_priv->psr.active) {
> > +		if (INTEL_GEN(dev_priv) >= 9)
> > +			WARN_ON(I915_READ(EDP_PSR2_CTL) &
> > EDP_PSR2_ENABLE);
> > +		WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
> 
> Do we really need so many WARN_ONs needing MMIO reads? There are
> almost as 
> many lines of warnings as code in this function. I think at this
> point, we 
> know this code works and the warnings in psr_exit() should be
> removed.
> 
> >  		return;
> > +	}
> > 
> >  	if (dev_priv->psr.psr2_enabled) {
> >  		val = I915_READ(EDP_PSR2_CTL);
> > @@ -680,32 +684,23 @@ static void
> >  intel_psr_disable_source(struct intel_dp *intel_dp)
> >  {
> >  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > +	i915_reg_t psr_status;
> > +	u32 psr_status_mask;
> > 
> > -	if (dev_priv->psr.active) {
> > -		i915_reg_t psr_status;
> > -		u32 psr_status_mask;
> > -
> > -		intel_psr_exit(dev_priv);
> > +	intel_psr_exit(dev_priv);
> > 
> > -		if (dev_priv->psr.psr2_enabled) {
> > -			psr_status = EDP_PSR2_STATUS;
> > -			psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
> > -		} else {
> > -			psr_status = EDP_PSR_STATUS;
> > -			psr_status_mask = EDP_PSR_STATUS_STATE_MASK;
> > -		}
> > -
> > -		/* Wait till PSR is idle */
> > -		if (intel_wait_for_register(dev_priv,
> > -					    psr_status,
> > psr_status_mask, 0,
> > -					    2000))
> > -			DRM_ERROR("Timed out waiting for PSR Idle
> > State\n");
> > +	if (dev_priv->psr.psr2_enabled) {
> > +		psr_status = EDP_PSR2_STATUS;
> > +		psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
> >  	} else {
> > -		if (dev_priv->psr.psr2_enabled)
> > -			WARN_ON(I915_READ(EDP_PSR2_CTL) &
> > EDP_PSR2_ENABLE);
> > -		else
> > -			WARN_ON(I915_READ(EDP_PSR_CTL) &
> > EDP_PSR_ENABLE);
> > +		psr_status = EDP_PSR_STATUS;
> > +		psr_status_mask = EDP_PSR_STATUS_STATE_MASK;
> >  	}
> > +
> > +	/* Wait till PSR is idle */
> > +	if (intel_wait_for_register(dev_priv, psr_status,
> > psr_status_mask, 0,
> > +				    2000))
> > +		DRM_ERROR("Timed out waiting for PSR Idle State\n");
nitpick: Mind writing this as "Timed out waiting PSR idle state\n" if
you end up sending another version without the warnings?


> >  }
> > 
> >  static void intel_psr_disable_locked(struct intel_dp *intel_dp)
> 
> 
> 
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH v2 3/6] drm/i915/psr: Move intel_psr_disable_source() code to intel_psr_disable_locked()

[Dhinakaran Pandiyan]

On Wed, 2018-10-10 at 17:41 -0700, José Roberto de Souza wrote:
> In the past we had hooks to configure HW for VLV/CHV too, in the drop
> of VLV/CHV support the intel_psr_disable_source() code was not moved
> to the caller, so doing it here.
> 
> Suggested-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@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 | 25 +++++++++----------------
>  1 file changed, 9 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index 16d0e3df7de0..70d4e26e17b5 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -680,13 +680,20 @@ static void intel_psr_exit(struct
> drm_i915_private *dev_priv)
>  	dev_priv->psr.active = false;
>  }
>  
> -static void
> -intel_psr_disable_source(struct intel_dp *intel_dp)
> +static void intel_psr_disable_locked(struct intel_dp *intel_dp)
>  {
>  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
>  	i915_reg_t psr_status;
>  	u32 psr_status_mask;
>  
> +	lockdep_assert_held(&dev_priv->psr.lock);
> +
> +	if (!dev_priv->psr.enabled)
> +		return;
> +
> +	DRM_DEBUG_KMS("Disabling PSR%s\n",
> +		      dev_priv->psr.psr2_enabled ? "2" : "1");
> +
>  	intel_psr_exit(dev_priv);

The above lockdep_assert_held(&dev_priv->psr.lock) made me think we
need one in inside psr_exit() too. Not in this though, could perhaps be
added in patch 1.


>  
>  	if (dev_priv->psr.psr2_enabled) {
> @@ -701,20 +708,6 @@ intel_psr_disable_source(struct intel_dp
> *intel_dp)
>  	if (intel_wait_for_register(dev_priv, psr_status,
> psr_status_mask, 0,
>  				    2000))
>  		DRM_ERROR("Timed out waiting for PSR Idle State\n");
> -}
> -
> -static void intel_psr_disable_locked(struct intel_dp *intel_dp)
> -{
> -	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> -
> -	lockdep_assert_held(&dev_priv->psr.lock);
> -
> -	if (!dev_priv->psr.enabled)
> -		return;
> -
> -	DRM_DEBUG_KMS("Disabling PSR%s\n",
> -		      dev_priv->psr.psr2_enabled ? "2" : "1");
> -	intel_psr_disable_source(intel_dp);
>  
>  	/* Disable PSR on Sink */
>  	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, 0);

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

Re: [PATCH v2 6/6] drm/i915: Check PSR errors instead of retrain while PSR is enabled

[Dhinakaran Pandiyan]

On Wed, 2018-10-10 at 17:41 -0700, José Roberto de Souza wrote:
> When a PSR error happens sink also update the link status values
> while source do not retrain link as required in the PSR exit
> sequence.
> So in the short pulse handling it was returning earlier and doing a
> full detection and attempting to retrain but it fails because for
> most sinks the main link is disabled while PSR is active, before even
> check PSR errors.
> 
> Just call intel_psr_short_pulse() before
> intel_dp_needs_link_retrain() is also not the right fix as
> intel_dp_needs_link_retrain() would return true and trigger a full
> detection and trying to retrain link while PSR HW is also doing that.
> 
> Check for PSR active is also not safe as it could be inactive due a
> frontbuffer invalidate and still doing the PSR exit sequence.
> 
> 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  |  7 +++++++
>  drivers/gpu/drm/i915/intel_drv.h |  1 +
>  drivers/gpu/drm/i915/intel_psr.c | 15 +++++++++++++++
>  3 files changed, 23 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c
> b/drivers/gpu/drm/i915/intel_dp.c
> index 1826d491efd7..2a21a9e29eb2 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4396,6 +4396,13 @@ intel_dp_needs_link_retrain(struct intel_dp
> *intel_dp)
>  	if (!intel_dp->link_trained)
>  		return false;
>  
> +	/*
> +	 * While PSR is enabled, HW will control main-link and retrain
> when
> +	 * exiting PSR
> +	 */
> +	if (intel_psr_enabled(intel_dp))

You need a check for !link_standby. The link status is expected to be
good if we are not turning the link fully off.

-DK

> +		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 3dea7a1bda7f..147a116cfc71 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1958,6 +1958,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_runtime_pm.c */
>  int intel_power_domains_init(struct drm_i915_private *);
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index ad09130cb4ad..e4429f2f3034 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -1138,3 +1138,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;
> +}

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

Re: [PATCH v2 4/6] drm/i915: Disable PSR when a PSR aux error happen

[Dhinakaran Pandiyan]

On Wed, 2018-10-10 at 17:41 -0700, 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.
> 
> 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 | 39 ++++++++++++++++++++++++++++
> ----
>  2 files changed, 36 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 3017ef037fed..e8ba00dd2c51 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -638,6 +638,7 @@ struct i915_psr {
>  	u8 sink_sync_latency;
>  	ktime_t last_entry_attempt;
>  	ktime_t last_exit;
> +	u32 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 70d4e26e17b5..ad09130cb4ad 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -159,10 +159,16 @@ 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));
Downgrade this to debug since the error is handled in the driver? 

> +
> +			spin_lock(&dev_priv->irq_lock);
> +			dev_priv->psr.irq_aux_error |=
> BIT(cpu_transcoder);
Just ignore the non eDP bits, I don't think we want to do anything with
the information that some other bit was set.

> +			spin_unlock(&dev_priv->irq_lock);
> +
> +			schedule_work(&dev_priv->psr.work);
> +		}
>  
>  		if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
>  			dev_priv->psr.last_entry_attempt = time_ns;
> @@ -893,11 +899,36 @@ 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;
> +	u32 irq_aux_error;
> +
> +	spin_lock_irq(&dev_priv->irq_lock);
> +	irq_aux_error = psr->irq_aux_error;
> +	psr->irq_aux_error = 0;
A subsequent modeset will enable PSR again. I don't expect a modeset to
to be able to fix an AUX wake up failure, so might as well disable it
for good.

> +	spin_unlock_irq(&dev_priv->irq_lock);
> +
> +	/* right now PSR is only enabled in eDP */
"right now" implies that PSR could be enabled for non eDP ports, but
that's not the case.


> +	WARN_ON(irq_aux_error & ~BIT(TRANSCODER_EDP));
This should go away if you ignore non-EDP bits, and a stack trace isn't
particularly useful anyway.

> +
> +	mutex_lock(&psr->lock);
Is this sufficient? Don't we have to serialize against ongoing modesets
like we do for debugfs enable/disable. The disable sequence in bspec
calls out a running pipe and port as pre-requisites.

Ccing Ville and Maarten to get their opinion on this.
 
> +
> +	intel_psr_disable_locked(psr->dp);
> +	/* let's make sure that sink is awaken */
> +	drm_dp_dpcd_writeb(&psr->dp->aux, DP_SET_POWER,
> DP_SET_POWER_D0);
Given that the hardware initiated AUX write failed, I would recommend
reading back the sink PSR status to make sure disable worked.

> +
> +	mutex_unlock(&dev_priv->psr.lock);
> +}
> +
>  static void intel_psr_work(struct work_struct *work)
>  {
>  	struct drm_i915_private *dev_priv =
>  		container_of(work, typeof(*dev_priv), psr.work);
>  
> +	if (READ_ONCE(dev_priv->psr.irq_aux_error))
> +		intel_psr_handle_irq(dev_priv);
If psr_work() was already executing and past this check,
schedule_work() in intel_psr_irq_handler will return a failure and
disable PSR would now depend on getting an invalidate and flush
operation. We should disable PSR without any dependency on flush or
invalidate.


> +
>  	mutex_lock(&dev_priv->psr.lock);
>  
>  	if (!dev_priv->psr.enabled)

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

Re: [PATCH v2 5/6] drm/i915: Avoid a full port detection in the first eDP short pulse

[Dhinakaran Pandiyan]

On Thu, 2018-10-11 at 16:21 +0300, Ville Syrjälä wrote:
> On Wed, Oct 10, 2018 at 05:41:23PM -0700, 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>
> > ---
> >  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 13ff89be6ad6..1826d491efd7 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4034,8 +4034,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;
> >  
> > @@ -4045,25 +4043,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().
> 
> Can't quite parse that.
> 
> "Some eDP panels do not set a valid value
>  for sink count, so we ignore it."
> 
> or something like that perhaps.
> 
> >  	 */
> > -	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;
> 
> My earlier suggestion was that we should keep reading this
> anyway because some cts maybe required it. Would at least
> avoid mixing in two functional changes into once patch.

Documenting the outcome of IRC discussion so that we are on the same
page -  CTS does not test eDP, hence it is okay to skip reading sink
count for eDP panels. 



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

Re: [PATCH v2 2/6] drm/i915/psr: Always wait for idle state when disabling PSR

[Souza, Jose]

On Fri, 2018-10-19 at 13:42 -0700, Dhinakaran Pandiyan wrote:
> On Wednesday, October 10, 2018 5:41:20 PM PDT José Roberto de Souza
> wrote:
> > It should always wait for idle state when disabling PSR because PSR
> > could be inactive due a call to intel_psr_exit() and while PSR is
> > still being disabled asynchronously userspace could change the
> > modeset causing a call to psr_disable() that will not wait for PSR
> > idle and then PSR will be enabled again while PSR is still not
> > idle.
> > 
> > v2: rebased on top of the patch reusing psr_exit()
> > 
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_psr.c | 41 ++++++++++++++------------
> > ------
> >  1 file changed, 18 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c index f698b3f45c6d..16d0e3df7de0
> > 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -661,8 +661,12 @@ static void intel_psr_exit(struct
> > drm_i915_private
> > *dev_priv) {
> >  	u32 val;
> > 
> > -	if (!dev_priv->psr.active)
> > +	if (!dev_priv->psr.active) {
> > +		if (INTEL_GEN(dev_priv) >= 9)
> > +			WARN_ON(I915_READ(EDP_PSR2_CTL) &
> > EDP_PSR2_ENABLE);
> > +		WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
> 
> Do we really need so many WARN_ONs needing MMIO reads? There are
> almost as 
> many lines of warnings as code in this function. I think at this
> point, we 
> know this code works and the warnings in psr_exit() should be
> removed.

I'm just moving what intel_psr_disable_source() had but yes I think we
can remove some WARN_ON() in intel_psr but I guess is better do in
another patch.

> 
> >  		return;
> > +	}
> > 
> >  	if (dev_priv->psr.psr2_enabled) {
> >  		val = I915_READ(EDP_PSR2_CTL);
> > @@ -680,32 +684,23 @@ static void
> >  intel_psr_disable_source(struct intel_dp *intel_dp)
> >  {
> >  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > +	i915_reg_t psr_status;
> > +	u32 psr_status_mask;
> > 
> > -	if (dev_priv->psr.active) {
> > -		i915_reg_t psr_status;
> > -		u32 psr_status_mask;
> > -
> > -		intel_psr_exit(dev_priv);
> > +	intel_psr_exit(dev_priv);
> > 
> > -		if (dev_priv->psr.psr2_enabled) {
> > -			psr_status = EDP_PSR2_STATUS;
> > -			psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
> > -		} else {
> > -			psr_status = EDP_PSR_STATUS;
> > -			psr_status_mask = EDP_PSR_STATUS_STATE_MASK;
> > -		}
> > -
> > -		/* Wait till PSR is idle */
> > -		if (intel_wait_for_register(dev_priv,
> > -					    psr_status,
> > psr_status_mask, 0,
> > -					    2000))
> > -			DRM_ERROR("Timed out waiting for PSR Idle
> > State\n");
> > +	if (dev_priv->psr.psr2_enabled) {
> > +		psr_status = EDP_PSR2_STATUS;
> > +		psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
> >  	} else {
> > -		if (dev_priv->psr.psr2_enabled)
> > -			WARN_ON(I915_READ(EDP_PSR2_CTL) &
> > EDP_PSR2_ENABLE);
> > -		else
> > -			WARN_ON(I915_READ(EDP_PSR_CTL) &
> > EDP_PSR_ENABLE);
> > +		psr_status = EDP_PSR_STATUS;
> > +		psr_status_mask = EDP_PSR_STATUS_STATE_MASK;
> >  	}
> > +
> > +	/* Wait till PSR is idle */
> > +	if (intel_wait_for_register(dev_priv, psr_status,
> > psr_status_mask, 0,
> > +				    2000))
> > +		DRM_ERROR("Timed out waiting for PSR Idle State\n");
> >  }
> > 
> >  static void intel_psr_disable_locked(struct intel_dp *intel_dp)
> 
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 4/6] drm/i915: Disable PSR when a PSR aux error happen

[Souza, Jose]

On Fri, 2018-10-19 at 16:14 -0700, Dhinakaran Pandiyan wrote:
> On Wed, 2018-10-10 at 17:41 -0700, 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.
> > 
> > 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 | 39 ++++++++++++++++++++++++++++
> > ----
> >  2 files changed, 36 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 3017ef037fed..e8ba00dd2c51 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -638,6 +638,7 @@ struct i915_psr {
> >  	u8 sink_sync_latency;
> >  	ktime_t last_entry_attempt;
> >  	ktime_t last_exit;
> > +	u32 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 70d4e26e17b5..ad09130cb4ad 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -159,10 +159,16 @@ 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));
> Downgrade this to debug since the error is handled in the driver? 

I think is better keep as DRM_WARN so it is shown in regular kernel
logs this way if a user opens a bug complaning why PSR is disabled we
can check that is because of PSR aux error.

> 
> > +
> > +			spin_lock(&dev_priv->irq_lock);
> > +			dev_priv->psr.irq_aux_error |=
> > BIT(cpu_transcoder);
> Just ignore the non eDP bits, I don't think we want to do anything
> with
> the information that some other bit was set.
> 
> > +			spin_unlock(&dev_priv->irq_lock);
> > +
> > +			schedule_work(&dev_priv->psr.work);
> > +		}
> >  
> >  		if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
> >  			dev_priv->psr.last_entry_attempt = time_ns;
> > @@ -893,11 +899,36 @@ 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;
> > +	u32 irq_aux_error;
> > +
> > +	spin_lock_irq(&dev_priv->irq_lock);
> > +	irq_aux_error = psr->irq_aux_error;
> > +	psr->irq_aux_error = 0;
> A subsequent modeset will enable PSR again. I don't expect a modeset
> to
> to be able to fix an AUX wake up failure, so might as well disable it
> for good.

Add another field to do that or set sink_support=false? I guess PSR
short pulses errors should also disable it good too?

> 
> > +	spin_unlock_irq(&dev_priv->irq_lock);
> > +
> > +	/* right now PSR is only enabled in eDP */
> "right now" implies that PSR could be enabled for non eDP ports, but
> that's not the case.
> 
> 
> > +	WARN_ON(irq_aux_error & ~BIT(TRANSCODER_EDP));
> This should go away if you ignore non-EDP bits, and a stack trace
> isn't
> particularly useful anyway.

Okay I will remove this handlings for other transcoders.

> 
> > +
> > +	mutex_lock(&psr->lock);
> Is this sufficient? Don't we have to serialize against ongoing
> modesets
> like we do for debugfs enable/disable. The disable sequence in bspec
> calls out a running pipe and port as pre-requisites.

HW will only send a aux transaction when exiting PSR, in this cases
pipe will always be running:
- exiting because of changes in the screen
- exiting because pipe will be disabled
- exiting because of PSR error

> 
> Ccing Ville and Maarten to get their opinion on this.
>  
> > +
> > +	intel_psr_disable_locked(psr->dp);
> > +	/* let's make sure that sink is awaken */
> > +	drm_dp_dpcd_writeb(&psr->dp->aux, DP_SET_POWER,
> > DP_SET_POWER_D0);
> Given that the hardware initiated AUX write failed, I would recommend
> reading back the sink PSR status to make sure disable worked.

And in case of reading error or the value is not set try again? This
could fall into a infite loop. intel_dp_aux_xfer() already try to do
the transaction 5 times I guess if if failed the sink crashed and there
is no recover.

> 
> > +
> > +	mutex_unlock(&dev_priv->psr.lock);
> > +}
> > +
> >  static void intel_psr_work(struct work_struct *work)
> >  {
> >  	struct drm_i915_private *dev_priv =
> >  		container_of(work, typeof(*dev_priv), psr.work);
> >  
> > +	if (READ_ONCE(dev_priv->psr.irq_aux_error))
> > +		intel_psr_handle_irq(dev_priv);
> If psr_work() was already executing and past this check,
> schedule_work() in intel_psr_irq_handler will return a failure and
> disable PSR would now depend on getting an invalidate and flush
> operation. We should disable PSR without any dependency on flush or
> invalidate.

For what I checked in the schedule_work() code if the work is running
and there is a call to schedule_work() it will be schedule again.

> 
> 
> > +
> >  	mutex_lock(&dev_priv->psr.lock);
> >  
> >  	if (!dev_priv->psr.enabled)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 6/6] drm/i915: Check PSR errors instead of retrain while PSR is enabled

[Souza, Jose]

On Fri, 2018-10-19 at 14:02 -0700, Dhinakaran Pandiyan wrote:
> On Wed, 2018-10-10 at 17:41 -0700, José Roberto de Souza wrote:
> > When a PSR error happens sink also update the link status values
> > while source do not retrain link as required in the PSR exit
> > sequence.
> > So in the short pulse handling it was returning earlier and doing a
> > full detection and attempting to retrain but it fails because for
> > most sinks the main link is disabled while PSR is active, before
> > even
> > check PSR errors.
> > 
> > Just call intel_psr_short_pulse() before
> > intel_dp_needs_link_retrain() is also not the right fix as
> > intel_dp_needs_link_retrain() would return true and trigger a full
> > detection and trying to retrain link while PSR HW is also doing
> > that.
> > 
> > Check for PSR active is also not safe as it could be inactive due a
> > frontbuffer invalidate and still doing the PSR exit sequence.
> > 
> > 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  |  7 +++++++
> >  drivers/gpu/drm/i915/intel_drv.h |  1 +
> >  drivers/gpu/drm/i915/intel_psr.c | 15 +++++++++++++++
> >  3 files changed, 23 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 1826d491efd7..2a21a9e29eb2 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4396,6 +4396,13 @@ intel_dp_needs_link_retrain(struct intel_dp
> > *intel_dp)
> >  	if (!intel_dp->link_trained)
> >  		return false;
> >  
> > +	/*
> > +	 * While PSR is enabled, HW will control main-link and retrain
> > when
> > +	 * exiting PSR
> > +	 */
> > +	if (intel_psr_enabled(intel_dp))
> 
> You need a check for !link_standby. The link status is expected to be
> good if we are not turning the link fully off.

Even with link on while PSR is active, sink could lost sync with main
link.

> 
> -DK
> 
> > +		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 3dea7a1bda7f..147a116cfc71 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1958,6 +1958,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_runtime_pm.c */
> >  int intel_power_domains_init(struct drm_i915_private *);
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index ad09130cb4ad..e4429f2f3034 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -1138,3 +1138,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;
> > +}
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/6] drm/i915/psr: Always wait for idle state when disabling PSR

[Dhinakaran Pandiyan]

On Fri, 2018-10-19 at 23:46 +0000, Souza, Jose wrote:
> On Fri, 2018-10-19 at 13:42 -0700, Dhinakaran Pandiyan wrote:
> > On Wednesday, October 10, 2018 5:41:20 PM PDT José Roberto de Souza
> > wrote:
> > > It should always wait for idle state when disabling PSR because 
"It" is ambiguous here. 

How about something along the lines of -
"Unconditionally wait for PSR hardware to idle when disabling PSR. Even
if a preceding front buffer invalidate already disabled the feature, a
wait for idle is required before for re-enabling". Feel free to ignore.


> > > PSR
> > > could be inactive due a call to intel_psr_exit() and while PSR is
> > > still being disabled asynchronously userspace could change the
> > > modeset causing a call to psr_disable() that will not wait for
> > > PSR
> > > idle and then PSR will be enabled again while PSR is still not
> > > idle.
> > > 
> > > v2: rebased on top of the patch reusing psr_exit()
> > > 
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_psr.c | 41 ++++++++++++++------------
> > > ------
> > >  1 file changed, 18 insertions(+), 23 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c index
> > > f698b3f45c6d..16d0e3df7de0
> > > 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -661,8 +661,12 @@ static void intel_psr_exit(struct
> > > drm_i915_private
> > > *dev_priv) {
> > >  	u32 val;
> > > 
> > > -	if (!dev_priv->psr.active)
> > > +	if (!dev_priv->psr.active) {
> > > +		if (INTEL_GEN(dev_priv) >= 9)
> > > +			WARN_ON(I915_READ(EDP_PSR2_CTL) &
> > > EDP_PSR2_ENABLE);
> > > +		WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
> > 
> > Do we really need so many WARN_ONs needing MMIO reads? There are
> > almost as 
> > many lines of warnings as code in this function. I think at this
> > point, we 
> > know this code works and the warnings in psr_exit() should be
> > removed.
> 
> I'm just moving what intel_psr_disable_source() had but yes I think
> we
> can remove some WARN_ON() in intel_psr but I guess is better do in
> another patch.
> 
Please do so :)

Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > 
> > >  		return;
> > > +	}
> > > 
> > >  	if (dev_priv->psr.psr2_enabled) {
> > >  		val = I915_READ(EDP_PSR2_CTL);
> > > @@ -680,32 +684,23 @@ static void
> > >  intel_psr_disable_source(struct intel_dp *intel_dp)
> > >  {
> > >  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > > +	i915_reg_t psr_status;
> > > +	u32 psr_status_mask;
> > > 
> > > -	if (dev_priv->psr.active) {
> > > -		i915_reg_t psr_status;
> > > -		u32 psr_status_mask;
> > > -
> > > -		intel_psr_exit(dev_priv);
> > > +	intel_psr_exit(dev_priv);
> > > 
> > > -		if (dev_priv->psr.psr2_enabled) {
> > > -			psr_status = EDP_PSR2_STATUS;
> > > -			psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
> > > -		} else {
> > > -			psr_status = EDP_PSR_STATUS;
> > > -			psr_status_mask = EDP_PSR_STATUS_STATE_MASK;
> > > -		}
> > > -
> > > -		/* Wait till PSR is idle */
> > > -		if (intel_wait_for_register(dev_priv,
> > > -					    psr_status,
> > > psr_status_mask, 0,
> > > -					    2000))
> > > -			DRM_ERROR("Timed out waiting for PSR Idle
> > > State\n");
> > > +	if (dev_priv->psr.psr2_enabled) {
> > > +		psr_status = EDP_PSR2_STATUS;
> > > +		psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
> > >  	} else {
> > > -		if (dev_priv->psr.psr2_enabled)
> > > -			WARN_ON(I915_READ(EDP_PSR2_CTL) &
> > > EDP_PSR2_ENABLE);
> > > -		else
> > > -			WARN_ON(I915_READ(EDP_PSR_CTL) &
> > > EDP_PSR_ENABLE);
> > > +		psr_status = EDP_PSR_STATUS;
> > > +		psr_status_mask = EDP_PSR_STATUS_STATE_MASK;
> > >  	}
> > > +
> > > +	/* Wait till PSR is idle */
> > > +	if (intel_wait_for_register(dev_priv, psr_status,
> > > psr_status_mask, 0,
> > > +				    2000))
> > > +		DRM_ERROR("Timed out waiting for PSR Idle State\n");
> > >  }
> > > 
> > >  static void intel_psr_disable_locked(struct intel_dp *intel_dp)
> > 
> > 
> > 
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH v2 4/6] drm/i915: Disable PSR when a PSR aux error happen

[Dhinakaran Pandiyan]

On Sat, 2018-10-20 at 00:12 +0000, Souza, Jose wrote:
> On Fri, 2018-10-19 at 16:14 -0700, Dhinakaran Pandiyan wrote:
> > On Wed, 2018-10-10 at 17:41 -0700, 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.
> > > 
> > > 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 | 39
> > > ++++++++++++++++++++++++++++
> > > ----
> > >  2 files changed, 36 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index 3017ef037fed..e8ba00dd2c51 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -638,6 +638,7 @@ struct i915_psr {
> > >  	u8 sink_sync_latency;
> > >  	ktime_t last_entry_attempt;
> > >  	ktime_t last_exit;
> > > +	u32 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 70d4e26e17b5..ad09130cb4ad 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -159,10 +159,16 @@ 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));
> > 
> > Downgrade this to debug since the error is handled in the driver? 
> 
> I think is better keep as DRM_WARN so it is shown in regular kernel
> logs this way if a user opens a bug complaning why PSR is disabled we
> can check that is because of PSR aux error.
> 
> > 
> > > +
> > > +			spin_lock(&dev_priv->irq_lock);
This lock isn't needed either. How about setting a bool only if the
transcoder is eDP and then scheduling a disable.

> > > +			dev_priv->psr.irq_aux_error |=
> > > BIT(cpu_transcoder);
> > 
> > Just ignore the non eDP bits, I don't think we want to do anything
> > with
> > the information that some other bit was set.
> > 
> > > +			spin_unlock(&dev_priv->irq_lock);
> > > +
> > > +			schedule_work(&dev_priv->psr.work);
> > > +		}
> > >  
> > >  		if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
> > >  			dev_priv->psr.last_entry_attempt = time_ns;
> > > @@ -893,11 +899,36 @@ 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;
> > > +	u32 irq_aux_error;
> > > +
> > > +	spin_lock_irq(&dev_priv->irq_lock);
> > > +	irq_aux_error = psr->irq_aux_error;
> > > +	psr->irq_aux_error = 0;
> > 
> > A subsequent modeset will enable PSR again. I don't expect a
> > modeset
> > to
> > to be able to fix an AUX wake up failure, so might as well disable
> > it
> > for good.
> 
> Add another field to do that or set sink_support=false? I guess PSR
> short pulses errors should also disable it good too?
Reusing sink_support will get confusing, particularly because it is
exposed in debugfs.

> 
> > 
> > > +	spin_unlock_irq(&dev_priv->irq_lock);
> > > +
> > > +	/* right now PSR is only enabled in eDP */
> > 
> > "right now" implies that PSR could be enabled for non eDP ports,
> > but
> > that's not the case.
> > 
> > 
> > > +	WARN_ON(irq_aux_error & ~BIT(TRANSCODER_EDP));
> > 
> > This should go away if you ignore non-EDP bits, and a stack trace
> > isn't
> > particularly useful anyway.
> 
> Okay I will remove this handlings for other transcoders.
> 
> > 
> > > +
> > > +	mutex_lock(&psr->lock);
> > 
> > Is this sufficient? Don't we have to serialize against ongoing
> > modesets
> > like we do for debugfs enable/disable. The disable sequence in
> > bspec
> > calls out a running pipe and port as pre-requisites.
> 
> HW will only send a aux transaction when exiting PSR, in this cases
> pipe will always be running:
Sure, but psr_work() can run after the pipe is disabled.

However, psr.enabled should take care of not writing to PSR_CTL if the
pipe was already disabled. The question now is if we were in the middle
of a modeset, disabling PSR here would have no effect if encoders are
enabled after this point.


> - exiting because of changes in the screen
> - exiting because pipe will be disabled
> - exiting because of PSR error
> 
> > 
> > Ccing Ville and Maarten to get their opinion on this.
> >  
> > > +
> > > +	intel_psr_disable_locked(psr->dp);
> > > +	/* let's make sure that sink is awaken */
> > > +	drm_dp_dpcd_writeb(&psr->dp->aux, DP_SET_POWER,
> > > DP_SET_POWER_D0);
> > 
> > Given that the hardware initiated AUX write failed, I would
> > recommend
> > reading back the sink PSR status to make sure disable worked.
> 
> And in case of reading error or the value is not set try again? This
> could fall into a infite loop. intel_dp_aux_xfer() already try to do
> the transaction 5 times I guess if if failed the sink crashed and
> there
> is no recover.
> 
I was thinking of printing an error here so that we know error recovery
did not work.


> > 
> > > +
> > > +	mutex_unlock(&dev_priv->psr.lock);
> > > +}
> > > +
> > >  static void intel_psr_work(struct work_struct *work)
> > >  {
> > >  	struct drm_i915_private *dev_priv =
> > >  		container_of(work, typeof(*dev_priv), psr.work);
> > >  
> > > +	if (READ_ONCE(dev_priv->psr.irq_aux_error))
> > > +		intel_psr_handle_irq(dev_priv);

Why not create a new work item for disable? I don't see why
intel_psr_work() needs to be reused for a completely different reason.

> > 
> > If psr_work() was already executing and past this check,
> > schedule_work() in intel_psr_irq_handler will return a failure and
> > disable PSR would now depend on getting an invalidate and flush
> > operation. We should disable PSR without any dependency on flush or
> > invalidate.
> 
> For what I checked in the schedule_work() code if the work is running
> and there is a call to schedule_work() it will be schedule again.
> 
From the documentation, 
/**
 * schedule_work - put work task in global workqueue
 * @work: job to be done
 *
 * Returns %false if @work was already on the kernel-global workqueue
and
 * %true otherwise.
 *

> > 
> > 
> > > +
> > >  	mutex_lock(&dev_priv->psr.lock);
> > >  
> > >  	if (!dev_priv->psr.enabled)
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH v2 4/6] drm/i915: Disable PSR when a PSR aux error happen

[Dhinakaran Pandiyan]

On Wed, 2018-10-24 at 15:08 -0700, Dhinakaran Pandiyan wrote:
> On Sat, 2018-10-20 at 00:12 +0000, Souza, Jose wrote:
> > On Fri, 2018-10-19 at 16:14 -0700, Dhinakaran Pandiyan wrote:
> > > On Wed, 2018-10-10 at 17:41 -0700, 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.
> > > > 
> > > > 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 | 39
> > > > ++++++++++++++++++++++++++++
> > > > ----
> > > >  2 files changed, 36 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > index 3017ef037fed..e8ba00dd2c51 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -638,6 +638,7 @@ struct i915_psr {
> > > >  	u8 sink_sync_latency;
> > > >  	ktime_t last_entry_attempt;
> > > >  	ktime_t last_exit;
> > > > +	u32 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 70d4e26e17b5..ad09130cb4ad 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -159,10 +159,16 @@ 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_trans
> > > > coder));
> > > > +		if (psr_iir & EDP_PSR_ERROR(cpu_transcoder)) {
> > > > +			DRM_WARN("[transcoder %s] PSR aux
> > > > error\n",
> > > > +				 transcoder_name(cpu_transcoder
> > > > ));
> > > 
> > > Downgrade this to debug since the error is handled in the
> > > driver? 
> > 
> > I think is better keep as DRM_WARN so it is shown in regular kernel
> > logs this way if a user opens a bug complaning why PSR is disabled
> > we
> > can check that is because of PSR aux error.
> > 
> > > 
> > > > +
> > > > +			spin_lock(&dev_priv->irq_lock);
> 
> This lock isn't needed either. How about setting a bool only if the
> transcoder is eDP and then scheduling a disable.
> 
> > > > +			dev_priv->psr.irq_aux_error |=
> > > > BIT(cpu_transcoder);
> > > 
> > > Just ignore the non eDP bits, I don't think we want to do
> > > anything
> > > with
> > > the information that some other bit was set.
> > > 
> > > > +			spin_unlock(&dev_priv->irq_lock);
> > > > +
> > > > +			schedule_work(&dev_priv->psr.work);
> > > > +		}
> > > >  
> > > >  		if (psr_iir &
> > > > EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
> > > >  			dev_priv->psr.last_entry_attempt =
> > > > time_ns;
> > > > @@ -893,11 +899,36 @@ 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;
> > > > +	u32 irq_aux_error;
> > > > +
> > > > +	spin_lock_irq(&dev_priv->irq_lock);
> > > > +	irq_aux_error = psr->irq_aux_error;
> > > > +	psr->irq_aux_error = 0;
> > > 
> > > A subsequent modeset will enable PSR again. I don't expect a
> > > modeset
> > > to
> > > to be able to fix an AUX wake up failure, so might as well
> > > disable
> > > it
> > > for good.
> > 
> > Add another field to do that or set sink_support=false? I guess PSR
> > short pulses errors should also disable it good too?
> 
> Reusing sink_support will get confusing, particularly because it is
> exposed in debugfs.
> 
> > 
> > > 
> > > > +	spin_unlock_irq(&dev_priv->irq_lock);
> > > > +
> > > > +	/* right now PSR is only enabled in eDP */
> > > 
> > > "right now" implies that PSR could be enabled for non eDP ports,
> > > but
> > > that's not the case.
> > > 
> > > 
> > > > +	WARN_ON(irq_aux_error & ~BIT(TRANSCODER_EDP));
> > > 
> > > This should go away if you ignore non-EDP bits, and a stack trace
> > > isn't
> > > particularly useful anyway.
> > 
> > Okay I will remove this handlings for other transcoders.
> > 
> > > 
> > > > +
> > > > +	mutex_lock(&psr->lock);
> > > 
> > > Is this sufficient? Don't we have to serialize against ongoing
> > > modesets
> > > like we do for debugfs enable/disable. The disable sequence in
> > > bspec
> > > calls out a running pipe and port as pre-requisites.
> > 
> > HW will only send a aux transaction when exiting PSR, in this cases
> > pipe will always be running:
> 
> Sure, but psr_work() can run after the pipe is disabled.
> 
> However, psr.enabled should take care of not writing to PSR_CTL if
> the
> pipe was already disabled. The question now is if we were in the
> middle
> of a modeset, disabling PSR here would have no effect if encoders are
> enabled after this point.
> 
Another issue is the wait for idle under psr.lock, not a good idea to
have modesets waiting for that lock.

> 
> > - exiting because of changes in the screen
> > - exiting because pipe will be disabled
> > - exiting because of PSR error
> > 
> > > 
> > > Ccing Ville and Maarten to get their opinion on this.
> > >  
> > > > +
> > > > +	intel_psr_disable_locked(psr->dp);
> > > > +	/* let's make sure that sink is awaken */
> > > > +	drm_dp_dpcd_writeb(&psr->dp->aux, DP_SET_POWER,
> > > > DP_SET_POWER_D0);
> > > 
> > > Given that the hardware initiated AUX write failed, I would
> > > recommend
> > > reading back the sink PSR status to make sure disable worked.
> > 
> > And in case of reading error or the value is not set try again?
> > This
> > could fall into a infite loop. intel_dp_aux_xfer() already try to
> > do
> > the transaction 5 times I guess if if failed the sink crashed and
> > there
> > is no recover.
> > 
> 
> I was thinking of printing an error here so that we know error
> recovery
> did not work.
> 
> 
> > > 
> > > > +
> > > > +	mutex_unlock(&dev_priv->psr.lock);
> > > > +}
> > > > +
> > > >  static void intel_psr_work(struct work_struct *work)
> > > >  {
> > > >  	struct drm_i915_private *dev_priv =
> > > >  		container_of(work, typeof(*dev_priv),
> > > > psr.work);
> > > >  
> > > > +	if (READ_ONCE(dev_priv->psr.irq_aux_error))
> > > > +		intel_psr_handle_irq(dev_priv);
> 
> Why not create a new work item for disable? I don't see why
> intel_psr_work() needs to be reused for a completely different
> reason.
> 
> > > 
> > > If psr_work() was already executing and past this check,
> > > schedule_work() in intel_psr_irq_handler will return a failure
> > > and
> > > disable PSR would now depend on getting an invalidate and flush
> > > operation. We should disable PSR without any dependency on flush
> > > or
> > > invalidate.
> > 
> > For what I checked in the schedule_work() code if the work is
> > running
> > and there is a call to schedule_work() it will be schedule again.
> > 
> 
> From the documentation, 
> /**
>  * schedule_work - put work task in global workqueue
>  * @work: job to be done
>  *
>  * Returns %false if @work was already on the kernel-global workqueue
> and
>  * %true otherwise.
>  *
> 
> > > 
> > > 
> > > > +
> > > >  	mutex_lock(&dev_priv->psr.lock);
> > > >  
> > > >  	if (!dev_priv->psr.enabled)
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH v2 4/6] drm/i915: Disable PSR when a PSR aux error happen

[Souza, Jose]

On Wed, 2018-10-24 at 16:22 -0700, Dhinakaran Pandiyan wrote:
> On Wed, 2018-10-24 at 15:08 -0700, Dhinakaran Pandiyan wrote:
> > On Sat, 2018-10-20 at 00:12 +0000, Souza, Jose wrote:
> > > On Fri, 2018-10-19 at 16:14 -0700, Dhinakaran Pandiyan wrote:
> > > > On Wed, 2018-10-10 at 17:41 -0700, 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.
> > > > > 
> > > > > 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 | 39
> > > > > ++++++++++++++++++++++++++++
> > > > > ----
> > > > >  2 files changed, 36 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > > index 3017ef037fed..e8ba00dd2c51 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > > @@ -638,6 +638,7 @@ struct i915_psr {
> > > > >  	u8 sink_sync_latency;
> > > > >  	ktime_t last_entry_attempt;
> > > > >  	ktime_t last_exit;
> > > > > +	u32 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 70d4e26e17b5..ad09130cb4ad 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > > @@ -159,10 +159,16 @@ 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_trans
> > > > > coder));
> > > > > +		if (psr_iir & EDP_PSR_ERROR(cpu_transcoder)) {
> > > > > +			DRM_WARN("[transcoder %s] PSR aux
> > > > > error\n",
> > > > > +				 transcoder_name(cpu_transcoder
> > > > > ));
> > > > 
> > > > Downgrade this to debug since the error is handled in the
> > > > driver? 
> > > 
> > > I think is better keep as DRM_WARN so it is shown in regular
> > > kernel
> > > logs this way if a user opens a bug complaning why PSR is
> > > disabled
> > > we
> > > can check that is because of PSR aux error.
> > > 
> > > > > +
> > > > > +			spin_lock(&dev_priv->irq_lock);
> > 
> > This lock isn't needed either. How about setting a bool only if the
> > transcoder is eDP and then scheduling a disable.
> > 
> > > > > +			dev_priv->psr.irq_aux_error |=
> > > > > BIT(cpu_transcoder);
> > > > 
> > > > Just ignore the non eDP bits, I don't think we want to do
> > > > anything
> > > > with
> > > > the information that some other bit was set.
> > > > 
> > > > > +			spin_unlock(&dev_priv->irq_lock);
> > > > > +
> > > > > +			schedule_work(&dev_priv->psr.work);
> > > > > +		}
> > > > >  
> > > > >  		if (psr_iir &
> > > > > EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
> > > > >  			dev_priv->psr.last_entry_attempt =
> > > > > time_ns;
> > > > > @@ -893,11 +899,36 @@ 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;
> > > > > +	u32 irq_aux_error;
> > > > > +
> > > > > +	spin_lock_irq(&dev_priv->irq_lock);
> > > > > +	irq_aux_error = psr->irq_aux_error;
> > > > > +	psr->irq_aux_error = 0;
> > > > 
> > > > A subsequent modeset will enable PSR again. I don't expect a
> > > > modeset
> > > > to
> > > > to be able to fix an AUX wake up failure, so might as well
> > > > disable
> > > > it
> > > > for good.
> > > 
> > > Add another field to do that or set sink_support=false? I guess
> > > PSR
> > > short pulses errors should also disable it good too?
> > 
> > Reusing sink_support will get confusing, particularly because it is
> > exposed in debugfs.
> > 
> > > > > +	spin_unlock_irq(&dev_priv->irq_lock);
> > > > > +
> > > > > +	/* right now PSR is only enabled in eDP */
> > > > 
> > > > "right now" implies that PSR could be enabled for non eDP
> > > > ports,
> > > > but
> > > > that's not the case.
> > > > 
> > > > 
> > > > > +	WARN_ON(irq_aux_error & ~BIT(TRANSCODER_EDP));
> > > > 
> > > > This should go away if you ignore non-EDP bits, and a stack
> > > > trace
> > > > isn't
> > > > particularly useful anyway.
> > > 
> > > Okay I will remove this handlings for other transcoders.
> > > 
> > > > > +
> > > > > +	mutex_lock(&psr->lock);
> > > > 
> > > > Is this sufficient? Don't we have to serialize against ongoing
> > > > modesets
> > > > like we do for debugfs enable/disable. The disable sequence in
> > > > bspec
> > > > calls out a running pipe and port as pre-requisites.
> > > 
> > > HW will only send a aux transaction when exiting PSR, in this
> > > cases
> > > pipe will always be running:
> > 
> > Sure, but psr_work() can run after the pipe is disabled.
> > 
> > However, psr.enabled should take care of not writing to PSR_CTL if
> > the
> > pipe was already disabled. The question now is if we were in the
> > middle
> > of a modeset, disabling PSR here would have no effect if encoders
> > are
> > enabled after this point.
> > 
> Another issue is the wait for idle under psr.lock, not a good idea to
> have modesets waiting for that lock.

When doing a modeset it would wait for PSR idle anyways as we can only
disable the pipe if PSR is idle. But waiting here allow us to send the
wake command after hardware already tried to exit PSR.

> 
> > > - exiting because of changes in the screen
> > > - exiting because pipe will be disabled
> > > - exiting because of PSR error
> > > 
> > > > Ccing Ville and Maarten to get their opinion on this.
> > > >  
> > > > > +
> > > > > +	intel_psr_disable_locked(psr->dp);
> > > > > +	/* let's make sure that sink is awaken */
> > > > > +	drm_dp_dpcd_writeb(&psr->dp->aux, DP_SET_POWER,
> > > > > DP_SET_POWER_D0);
> > > > 
> > > > Given that the hardware initiated AUX write failed, I would
> > > > recommend
> > > > reading back the sink PSR status to make sure disable worked.
> > > 
> > > And in case of reading error or the value is not set try again?
> > > This
> > > could fall into a infite loop. intel_dp_aux_xfer() already try to
> > > do
> > > the transaction 5 times I guess if if failed the sink crashed and
> > > there
> > > is no recover.
> > > 
> > 
> > I was thinking of printing an error here so that we know error
> > recovery
> > did not work.
> > 
> > 
> > > > > +
> > > > > +	mutex_unlock(&dev_priv->psr.lock);
> > > > > +}
> > > > > +
> > > > >  static void intel_psr_work(struct work_struct *work)
> > > > >  {
> > > > >  	struct drm_i915_private *dev_priv =
> > > > >  		container_of(work, typeof(*dev_priv),
> > > > > psr.work);
> > > > >  
> > > > > +	if (READ_ONCE(dev_priv->psr.irq_aux_error))
> > > > > +		intel_psr_handle_irq(dev_priv);
> > 
> > Why not create a new work item for disable? I don't see why
> > intel_psr_work() needs to be reused for a completely different
> > reason.
> > 
> > > > If psr_work() was already executing and past this check,
> > > > schedule_work() in intel_psr_irq_handler will return a failure
> > > > and
> > > > disable PSR would now depend on getting an invalidate and flush
> > > > operation. We should disable PSR without any dependency on
> > > > flush
> > > > or
> > > > invalidate.
> > > 
> > > For what I checked in the schedule_work() code if the work is
> > > running
> > > and there is a call to schedule_work() it will be schedule again.
> > > 
> > 
> > From the documentation, 
> > /**
> >  * schedule_work - put work task in global workqueue
> >  * @work: job to be done
> >  *
> >  * Returns %false if @work was already on the kernel-global
> > workqueue
> > and
> >  * %true otherwise.
> >  *
> > 
> > > > 
> > > > > +
> > > > >  	mutex_lock(&dev_priv->psr.lock);
> > > > >  
> > > > >  	if (!dev_priv->psr.enabled)
> > > 
> > > _______________________________________________
> > > 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