[PATCH 1/4] drm/i915/psr: Always wait for idle state when disabling PSR

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

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 | 43 +++++++++++++++-----------------
 1 file changed, 20 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 423cdf84059c..cd9a60d1efa1 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -661,40 +661,37 @@ 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;
-
-		if (dev_priv->psr.psr2_enabled) {
-			psr_status = EDP_PSR2_STATUS;
-			psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
-
+		if (dev_priv->psr.psr2_enabled)
 			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_READ(EDP_PSR2_CTL) & ~EDP_PSR2_ENABLE);
+		else
 			I915_WRITE(EDP_PSR_CTL,
 				   I915_READ(EDP_PSR_CTL) & ~EDP_PSR_ENABLE);
-		}
-
-		/* 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");
-
-		dev_priv->psr.active = false;
 	} 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);
 	}
+
+	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");
+
+	dev_priv->psr.active = false;
 }
 
 static void intel_psr_disable_locked(struct intel_dp *intel_dp)
-- 
2.19.0

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

[PATCH 2/4] 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 794a8a03c7e6..efbebe1c2ba3 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 cd9a60d1efa1..74090fffea23 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;
@@ -891,11 +897,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.0

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

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

[Dhinakaran Pandiyan]

On Fri, 2018-10-05 at 16:35 -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.
That's a good point, the short pulse handler does handle retraining.

> 
> 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 794a8a03c7e6..efbebe1c2ba3 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 cd9a60d1efa1..74090fffea23 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;
> @@ -891,11 +897,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);

We should be making sure the sink exits PSR after psr_invalidate() ->
psr_exit() too? Which means, we have to figure out a cleaner way to
handle all of this. I am not sure, at this point, what a cleaner
solution will like. However, I'd like PSR disable from invalidate, PSR
disable from a modeset and PSR disable due to an error share code and
behavior. All of them should basically be
1) Disable PSR in PSR_CTL
2) Wait for idle in PSR_STATUS
3) Write to sink DP_SET_POWER



> +
> +	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)

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

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

[Souza, Jose]

On Mon, 2018-10-08 at 17:14 -0700, Dhinakaran Pandiyan wrote:
> On Fri, 2018-10-05 at 16:35 -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.
> That's a good point, the short pulse handler does handle retraining.
> 
> > 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 794a8a03c7e6..efbebe1c2ba3 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 cd9a60d1efa1..74090fffea23 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;
> > @@ -891,11 +897,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);
> 
> We should be making sure the sink exits PSR after psr_invalidate() ->
> psr_exit() too? Which means, we have to figure out a cleaner way to
> handle all of this. I am not sure, at this point, what a cleaner
> solution will like. However, I'd like PSR disable from invalidate,
> PSR
> disable from a modeset and PSR disable due to an error share code and
> behavior. All of them should basically be
> 1) Disable PSR in PSR_CTL
> 2) Wait for idle in PSR_STATUS
> 3) Write to sink DP_SET_POWER

We don't need to wait PSR to be disabled for invalidate(), hardware
will do the exit sequence including write to DP_SET_POWER, also in this
case we want activate PSR as soon as all frontbuffer changes was
commited and PSR is inactive.

> 
> 
> 
> > +
> > +	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)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Dhinakaran Pandiyan]

On Mon, 2018-10-08 at 17:30 -0700, Souza, Jose wrote:
> On Mon, 2018-10-08 at 17:14 -0700, Dhinakaran Pandiyan wrote:
> > On Fri, 2018-10-05 at 16:35 -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.
> > 
> > That's a good point, the short pulse handler does handle
> > retraining.
> > 
> > > 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 794a8a03c7e6..efbebe1c2ba3 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 cd9a60d1efa1..74090fffea23 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;
> > > @@ -891,11 +897,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);
> > 
> > We should be making sure the sink exits PSR after psr_invalidate()
> > ->
> > psr_exit() too? Which means, we have to figure out a cleaner way to
> > handle all of this. I am not sure, at this point, what a cleaner
> > solution will like. However, I'd like PSR disable from invalidate,
> > PSR
> > disable from a modeset and PSR disable due to an error share code
> > and
> > behavior. All of them should basically be
> > 1) Disable PSR in PSR_CTL
> > 2) Wait for idle in PSR_STATUS
> > 3) Write to sink DP_SET_POWER
> 
> We don't need to wait PSR to be disabled for invalidate(), hardware
> will do the exit sequence including write to DP_SET_POWER, also in
> this
Yeah, but doesn't work consistently on the panel that I have, writing
to the sink DP_SET_POWER dpcd is needed. I guess, we could add a panel
specific quirk to work around it.

-DK

> case we want activate PSR as soon as all frontbuffer changes was
> commited and PSR is inactive.
> 

> > 
> > 
> > 
> > > +
> > > +	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)

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

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

[Souza, Jose]

On Mon, 2018-10-08 at 17:49 -0700, Dhinakaran Pandiyan wrote:
> On Mon, 2018-10-08 at 17:30 -0700, Souza, Jose wrote:
> > On Mon, 2018-10-08 at 17:14 -0700, Dhinakaran Pandiyan wrote:
> > > On Fri, 2018-10-05 at 16:35 -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.
> > > 
> > > That's a good point, the short pulse handler does handle
> > > retraining.
> > > 
> > > > 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 794a8a03c7e6..efbebe1c2ba3 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 cd9a60d1efa1..74090fffea23 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
> > > > ));
> > > > +
> > > > +			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;
> > > > @@ -891,11 +897,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);
> > > 
> > > We should be making sure the sink exits PSR after
> > > psr_invalidate()
> > > ->
> > > psr_exit() too? Which means, we have to figure out a cleaner way
> > > to
> > > handle all of this. I am not sure, at this point, what a cleaner
> > > solution will like. However, I'd like PSR disable from
> > > invalidate,
> > > PSR
> > > disable from a modeset and PSR disable due to an error share code
> > > and
> > > behavior. All of them should basically be
> > > 1) Disable PSR in PSR_CTL
> > > 2) Wait for idle in PSR_STATUS
> > > 3) Write to sink DP_SET_POWER
> > 
> > We don't need to wait PSR to be disabled for invalidate(), hardware
> > will do the exit sequence including write to DP_SET_POWER, also in
> > this
> Yeah, but doesn't work consistently on the panel that I have, writing
> to the sink DP_SET_POWER dpcd is needed. I guess, we could add a
> panel
> specific quirk to work around it.

The DP_SET_POWER writes that HW does while PSR is enabled in HW should
also fail, that is really strange that only the psr_exit() fails.

> 
> -DK
> 
> > case we want activate PSR as soon as all frontbuffer changes was
> > commited and PSR is inactive.
> > 
> > > 
> > > 
> > > > +
> > > > +	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)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/4] drm/i915: Cache sink_count for eDP

[José Roberto de Souza]

For eDP panels all the DPCD and EDID data is cached when initializing
the eDP connector so in futher detection it do not call
intel_dp_detect_dpcd() for eDP.
The problem is on the first short pulse interruption it calls
intel_dp_get_dpcd() for eDP and DP and it will read and set the sink
count, causing a mismatch between old sink count and the new one
triggering a full detection without needed.

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 19f0c3f59cbe..4a1c31ec9065 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3926,6 +3926,7 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp)
 {
 	struct drm_i915_private *dev_priv =
 		to_i915(dp_to_dig_port(intel_dp)->base.base.dev);
+	u8 val;
 
 	/* this function is meant to be called only once */
 	WARN_ON(intel_dp->dpcd[DP_DPCD_REV] != 0);
@@ -3997,6 +3998,10 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp)
 
 	intel_dp_set_common_rates(intel_dp);
 
+	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_SINK_COUNT, &val) <= 0)
+		return false;
+	intel_dp->sink_count = DP_GET_SINK_COUNT(val);
+
 	return true;
 }
 
-- 
2.19.0

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

Re: [PATCH 3/4] drm/i915: Cache sink_count for eDP

[Dhinakaran Pandiyan]

On Fri, 2018-10-05 at 16:35 -0700, José Roberto de Souza wrote:
> For eDP panels all the DPCD and EDID data is cached when initializing
> the eDP connector so in futher detection it do not call
> intel_dp_detect_dpcd() for eDP.
> The problem is on the first short pulse interruption it calls
> intel_dp_get_dpcd() for eDP and DP and it will read and set the sink
> count, causing a mismatch between old sink count and the new one
> triggering a full detection without needed.
> 
> 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 | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c
> b/drivers/gpu/drm/i915/intel_dp.c
> index 19f0c3f59cbe..4a1c31ec9065 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3926,6 +3926,7 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp)
>  {
>  	struct drm_i915_private *dev_priv =
>  		to_i915(dp_to_dig_port(intel_dp)->base.base.dev);
> +	u8 val;
>  
>  	/* this function is meant to be called only once */
>  	WARN_ON(intel_dp->dpcd[DP_DPCD_REV] != 0);
> @@ -3997,6 +3998,10 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp)
>  
>  	intel_dp_set_common_rates(intel_dp);
>  
> +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_SINK_COUNT, &val) <=
> 0)
> +		return false;
> +	intel_dp->sink_count = DP_GET_SINK_COUNT(val);
Is this even relevant for eDPs? Seems unnecessary to read or compare
sink count for eDP. I'd suggest skipping DP_SINK_COUNT checks for eDP.


-DK

> +
>  	return true;
>  }
>  

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

Re: [PATCH 3/4] drm/i915: Cache sink_count for eDP

[Souza, Jose]

On Mon, 2018-10-08 at 17:19 -0700, Dhinakaran Pandiyan wrote:
> On Fri, 2018-10-05 at 16:35 -0700, José Roberto de Souza wrote:
> > For eDP panels all the DPCD and EDID data is cached when
> > initializing
> > the eDP connector so in futher detection it do not call
> > intel_dp_detect_dpcd() for eDP.
> > The problem is on the first short pulse interruption it calls
> > intel_dp_get_dpcd() for eDP and DP and it will read and set the
> > sink
> > count, causing a mismatch between old sink count and the new one
> > triggering a full detection without needed.
> > 
> > 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 | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 19f0c3f59cbe..4a1c31ec9065 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -3926,6 +3926,7 @@ intel_edp_init_dpcd(struct intel_dp
> > *intel_dp)
> >  {
> >  	struct drm_i915_private *dev_priv =
> >  		to_i915(dp_to_dig_port(intel_dp)->base.base.dev);
> > +	u8 val;
> >  
> >  	/* this function is meant to be called only once */
> >  	WARN_ON(intel_dp->dpcd[DP_DPCD_REV] != 0);
> > @@ -3997,6 +3998,10 @@ intel_edp_init_dpcd(struct intel_dp
> > *intel_dp)
> >  
> >  	intel_dp_set_common_rates(intel_dp);
> >  
> > +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_SINK_COUNT, &val) <=
> > 0)
> > +		return false;
> > +	intel_dp->sink_count = DP_GET_SINK_COUNT(val);
> Is this even relevant for eDPs? Seems unnecessary to read or compare
> sink count for eDP. I'd suggest skipping DP_SINK_COUNT checks for
> eDP.

I'm not sure as DP specs for DP_SINK_COUNT says:

The Sink device shall add one more if it has a local Rendering
Function.

and eDP spec do not redefine or alter this, so I guess is more safe
also read for eDP too.


> 
> 
> -DK
> 
> > +
> >  	return true;
> >  }
> >  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915: Cache sink_count for eDP

[Dhinakaran Pandiyan]

On Mon, 2018-10-08 at 17:35 -0700, Souza, Jose wrote:
> On Mon, 2018-10-08 at 17:19 -0700, Dhinakaran Pandiyan wrote:
> > On Fri, 2018-10-05 at 16:35 -0700, José Roberto de Souza wrote:
> > > For eDP panels all the DPCD and EDID data is cached when
> > > initializing
> > > the eDP connector so in futher detection it do not call
> > > intel_dp_detect_dpcd() for eDP.
> > > The problem is on the first short pulse interruption it calls
> > > intel_dp_get_dpcd() for eDP and DP and it will read and set the
> > > sink
> > > count, causing a mismatch between old sink count and the new one
> > > triggering a full detection without needed.
> > > 
> > > 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 | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > b/drivers/gpu/drm/i915/intel_dp.c
> > > index 19f0c3f59cbe..4a1c31ec9065 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -3926,6 +3926,7 @@ intel_edp_init_dpcd(struct intel_dp
> > > *intel_dp)
> > >  {
> > >  	struct drm_i915_private *dev_priv =
> > >  		to_i915(dp_to_dig_port(intel_dp)->base.base.dev);
> > > +	u8 val;
> > >  
> > >  	/* this function is meant to be called only once */
> > >  	WARN_ON(intel_dp->dpcd[DP_DPCD_REV] != 0);
> > > @@ -3997,6 +3998,10 @@ intel_edp_init_dpcd(struct intel_dp
> > > *intel_dp)
> > >  
> > >  	intel_dp_set_common_rates(intel_dp);
> > >  
> > > +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_SINK_COUNT, &val) <=
> > > 0)
> > > +		return false;
> > > +	intel_dp->sink_count = DP_GET_SINK_COUNT(val);
> > 
> > Is this even relevant for eDPs? Seems unnecessary to read or
> > compare
> > sink count for eDP. I'd suggest skipping DP_SINK_COUNT checks for
> > eDP.
> 
> I'm not sure as DP specs for DP_SINK_COUNT says:
> 
> The Sink device shall add one more if it has a local Rendering
> Function.
> 
> and eDP spec do not redefine or alter this, so I guess is more safe
> also read for eDP too.
> 

We already special case eDP in several places, for example, don't
update link rates from the short pulse handler etc. And also don't
support hotplug, I don't see a point.

-DK


> 
> > 
> > 
> > -DK
> > 
> > > +
> > >  	return true;
> > >  }
> > >  

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

Re: [PATCH 3/4] drm/i915: Cache sink_count for eDP

[Ville Syrjälä]

On Mon, Oct 08, 2018 at 05:54:17PM -0700, Dhinakaran Pandiyan wrote:
> On Mon, 2018-10-08 at 17:35 -0700, Souza, Jose wrote:
> > On Mon, 2018-10-08 at 17:19 -0700, Dhinakaran Pandiyan wrote:
> > > On Fri, 2018-10-05 at 16:35 -0700, José Roberto de Souza wrote:
> > > > For eDP panels all the DPCD and EDID data is cached when
> > > > initializing
> > > > the eDP connector so in futher detection it do not call
> > > > intel_dp_detect_dpcd() for eDP.
> > > > The problem is on the first short pulse interruption it calls
> > > > intel_dp_get_dpcd() for eDP and DP and it will read and set the
> > > > sink
> > > > count, causing a mismatch between old sink count and the new one
> > > > triggering a full detection without needed.
> > > > 
> > > > 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 | 5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > index 19f0c3f59cbe..4a1c31ec9065 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -3926,6 +3926,7 @@ intel_edp_init_dpcd(struct intel_dp
> > > > *intel_dp)
> > > >  {
> > > >  	struct drm_i915_private *dev_priv =
> > > >  		to_i915(dp_to_dig_port(intel_dp)->base.base.dev);
> > > > +	u8 val;
> > > >  
> > > >  	/* this function is meant to be called only once */
> > > >  	WARN_ON(intel_dp->dpcd[DP_DPCD_REV] != 0);
> > > > @@ -3997,6 +3998,10 @@ intel_edp_init_dpcd(struct intel_dp
> > > > *intel_dp)
> > > >  
> > > >  	intel_dp_set_common_rates(intel_dp);
> > > >  
> > > > +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_SINK_COUNT, &val) <=
> > > > 0)
> > > > +		return false;
> > > > +	intel_dp->sink_count = DP_GET_SINK_COUNT(val);
> > > 
> > > Is this even relevant for eDPs? Seems unnecessary to read or
> > > compare
> > > sink count for eDP. I'd suggest skipping DP_SINK_COUNT checks for
> > > eDP.
> > 
> > I'm not sure as DP specs for DP_SINK_COUNT says:
> > 
> > The Sink device shall add one more if it has a local Rendering
> > Function.
> > 
> > and eDP spec do not redefine or alter this, so I guess is more safe
> > also read for eDP too.
> > 
> 
> We already special case eDP in several places, for example, don't
> update link rates from the short pulse handler etc. And also don't
> support hotplug, I don't see a point.

IIRC some conformance test or something required that we read this.
I guess what we could do is still read it but just not update
intel_dp->sink_count. We already seem to have a special case which
ignores a zero sink_count on eDP. Might as well extend that a bit
I suppose.

In general I think special cases are bad, so IMO we should try
hard not add more unless really necessary. In this case it seems
the special case is warranted. Unfortunately commit 1034ce707b57
("drm/i915: Fixing eDP detection on certain platforms") failed to add
a comment explaining why. I'd appreciate if someone could add that
comment now so that we don't forget this in the future.

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

Re: [PATCH 3/4] drm/i915: Cache sink_count for eDP

[Souza, Jose]

On Tue, 2018-10-09 at 16:27 +0300, Ville Syrjälä wrote:
> On Mon, Oct 08, 2018 at 05:54:17PM -0700, Dhinakaran Pandiyan wrote:
> > On Mon, 2018-10-08 at 17:35 -0700, Souza, Jose wrote:
> > > On Mon, 2018-10-08 at 17:19 -0700, Dhinakaran Pandiyan wrote:
> > > > On Fri, 2018-10-05 at 16:35 -0700, José Roberto de Souza wrote:
> > > > > For eDP panels all the DPCD and EDID data is cached when
> > > > > initializing
> > > > > the eDP connector so in futher detection it do not call
> > > > > intel_dp_detect_dpcd() for eDP.
> > > > > The problem is on the first short pulse interruption it calls
> > > > > intel_dp_get_dpcd() for eDP and DP and it will read and set
> > > > > the
> > > > > sink
> > > > > count, causing a mismatch between old sink count and the new
> > > > > one
> > > > > triggering a full detection without needed.
> > > > > 
> > > > > 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 | 5 +++++
> > > > >  1 file changed, 5 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > > index 19f0c3f59cbe..4a1c31ec9065 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > @@ -3926,6 +3926,7 @@ intel_edp_init_dpcd(struct intel_dp
> > > > > *intel_dp)
> > > > >  {
> > > > >  	struct drm_i915_private *dev_priv =
> > > > >  		to_i915(dp_to_dig_port(intel_dp)-
> > > > > >base.base.dev);
> > > > > +	u8 val;
> > > > >  
> > > > >  	/* this function is meant to be called only once */
> > > > >  	WARN_ON(intel_dp->dpcd[DP_DPCD_REV] != 0);
> > > > > @@ -3997,6 +3998,10 @@ intel_edp_init_dpcd(struct intel_dp
> > > > > *intel_dp)
> > > > >  
> > > > >  	intel_dp_set_common_rates(intel_dp);
> > > > >  
> > > > > +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_SINK_COUNT,
> > > > > &val) <=
> > > > > 0)
> > > > > +		return false;
> > > > > +	intel_dp->sink_count = DP_GET_SINK_COUNT(val);
> > > > 
> > > > Is this even relevant for eDPs? Seems unnecessary to read or
> > > > compare
> > > > sink count for eDP. I'd suggest skipping DP_SINK_COUNT checks
> > > > for
> > > > eDP.
> > > 
> > > I'm not sure as DP specs for DP_SINK_COUNT says:
> > > 
> > > The Sink device shall add one more if it has a local Rendering
> > > Function.
> > > 
> > > and eDP spec do not redefine or alter this, so I guess is more
> > > safe
> > > also read for eDP too.
> > > 
> > 
> > We already special case eDP in several places, for example, don't
> > update link rates from the short pulse handler etc. And also don't
> > support hotplug, I don't see a point.
> 
> IIRC some conformance test or something required that we read this.
> I guess what we could do is still read it but just not update
> intel_dp->sink_count. We already seem to have a special case which
> ignores a zero sink_count on eDP. Might as well extend that a bit
> I suppose.

Okay, I will skip the comparison for eDP.

> 
> In general I think special cases are bad, so IMO we should try
> hard not add more unless really necessary. In this case it seems
> the special case is warranted. Unfortunately commit 1034ce707b57
> ("drm/i915: Fixing eDP detection on certain platforms") failed to add
> a comment explaining why. I'd appreciate if someone could add that
> comment now so that we don't forget this in the future.

Sure, I will add this comment.

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

[PATCH 4/4] 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 4a1c31ec9065..f89802bdf525 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4363,6 +4363,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 43190c6e9ef2..e2076ec9d965 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 74090fffea23..63c00c802fa7 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -1155,3 +1155,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.0

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

✗ Fi.CI.SPARSE: warning for series starting with [1/4] drm/i915/psr: Always wait for idle state when disabling PSR

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915/psr: Always wait for idle state when disabling PSR
URL   : https://patchwork.freedesktop.org/series/50654/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Sparse version: v0.5.2
Commit: drm/i915/psr: Always wait for idle state when disabling PSR
Okay!

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

Commit: drm/i915: Cache sink_count for eDP
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 [1/4] drm/i915/psr: Always wait for idle state when disabling PSR

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915/psr: Always wait for idle state when disabling PSR
URL   : https://patchwork.freedesktop.org/series/50654/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4945 -> Patchwork_10386 =

== Summary - SUCCESS ==

  No regressions found.

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

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@pm_rpm@module-reload:
      fi-glk-j4005:       NOTRUN -> DMESG-WARN (fdo#107726)

    
    ==== Possible fixes ====

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

    
  fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
  fdo#107726 https://bugs.freedesktop.org/show_bug.cgi?id=107726


== Participating hosts (44 -> 39) ==

  Additional (1): fi-glk-j4005 
  Missing    (6): fi-ilk-m540 fi-bxt-dsi fi-byt-squawks fi-icl-u2 fi-bsw-cyan fi-ctg-p8600 


== Build changes ==

    * Linux: CI_DRM_4945 -> Patchwork_10386

  CI_DRM_4945: d9b2bbbdba15cadca76ffd3ff0476e71222d671b @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4671: b121f7d42c260ae3a050c3f440d1c11f7cff7d1a @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10386: a80ef2c1e92629cd69081e088ded4a58b609b5f3 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

a80ef2c1e926 drm/i915: Check PSR errors instead of retrain while PSR is enabled
52e9bd524423 drm/i915: Cache sink_count for eDP
39febff6f09c drm/i915: Disable PSR when a PSR aux error happen
ba05b8343469 drm/i915/psr: Always wait for idle state when disabling PSR

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10386/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 [1/4] drm/i915/psr: Always wait for idle state when disabling PSR

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915/psr: Always wait for idle state when disabling PSR
URL   : https://patchwork.freedesktop.org/series/50654/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4945_full -> Patchwork_10386_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_10386_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_10386_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_10386_full:

  === IGT changes ===

    ==== Warnings ====

    igt@pm_rc6_residency@rc6-accuracy:
      shard-snb:          PASS -> SKIP

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_exec_big:
      shard-hsw:          PASS -> TIMEOUT (fdo#107937)

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

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

    igt@gem_softpin@noreloc-s3:
      shard-skl:          PASS -> INCOMPLETE (fdo#107773, fdo#104108)

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

    igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-c:
      shard-apl:          NOTRUN -> DMESG-WARN (fdo#107956) +3

    igt@kms_ccs@pipe-b-crc-sprite-planes-basic:
      shard-apl:          NOTRUN -> FAIL (fdo#105458, fdo#106510)

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

    igt@kms_cursor_crc@cursor-256x256-random:
      shard-skl:          NOTRUN -> FAIL (fdo#103232)

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

    igt@kms_flip@flip-vs-fences-interruptible:
      shard-snb:          NOTRUN -> INCOMPLETE (fdo#105411)

    igt@kms_flip@plain-flip-ts-check-interruptible:
      shard-skl:          PASS -> FAIL (fdo#100368)

    igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-shrfb-draw-mmap-gtt:
      shard-glk:          PASS -> DMESG-FAIL (fdo#103167, fdo#106538)

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

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

    igt@kms_plane@pixel-format-pipe-c-planes:
      shard-skl:          NOTRUN -> DMESG-WARN (fdo#106885)

    {igt@kms_plane_alpha_blend@pipe-a-alpha-basic}:
      shard-skl:          NOTRUN -> FAIL (fdo#108145) +1

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

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

    {igt@kms_plane_alpha_blend@pipe-b-alpha-7efc}:
      shard-apl:          NOTRUN -> FAIL (fdo#108146) +1

    {igt@kms_plane_alpha_blend@pipe-b-alpha-transparant-fb}:
      shard-apl:          NOTRUN -> FAIL (fdo#108145) +1

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

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

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

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

    
    ==== Possible fixes ====

    igt@kms_ccs@pipe-a-crc-primary-rotation-180:
      shard-skl:          FAIL -> PASS

    igt@kms_color@pipe-b-legacy-gamma:
      shard-apl:          FAIL (fdo#104782) -> PASS

    igt@kms_cursor_crc@cursor-256x256-dpms:
      shard-glk:          FAIL (fdo#103232) -> PASS +2

    igt@kms_cursor_crc@cursor-64x21-random:
      shard-apl:          FAIL (fdo#103232) -> PASS

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-mmap-wc:
      shard-apl:          FAIL (fdo#103167) -> PASS

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

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

    igt@perf_pmu@busy-accuracy-98-rcs0:
      shard-snb:          INCOMPLETE (fdo#105411) -> SKIP

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

  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  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#103232 https://bugs.freedesktop.org/show_bug.cgi?id=103232
  fdo#104108 https://bugs.freedesktop.org/show_bug.cgi?id=104108
  fdo#104782 https://bugs.freedesktop.org/show_bug.cgi?id=104782
  fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
  fdo#105458 https://bugs.freedesktop.org/show_bug.cgi?id=105458
  fdo#105763 https://bugs.freedesktop.org/show_bug.cgi?id=105763
  fdo#106510 https://bugs.freedesktop.org/show_bug.cgi?id=106510
  fdo#106538 https://bugs.freedesktop.org/show_bug.cgi?id=106538
  fdo#106641 https://bugs.freedesktop.org/show_bug.cgi?id=106641
  fdo#106885 https://bugs.freedesktop.org/show_bug.cgi?id=106885
  fdo#107773 https://bugs.freedesktop.org/show_bug.cgi?id=107773
  fdo#107937 https://bugs.freedesktop.org/show_bug.cgi?id=107937
  fdo#107956 https://bugs.freedesktop.org/show_bug.cgi?id=107956
  fdo#108039 https://bugs.freedesktop.org/show_bug.cgi?id=108039
  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


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

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4945 -> Patchwork_10386

  CI_DRM_4945: d9b2bbbdba15cadca76ffd3ff0476e71222d671b @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4671: b121f7d42c260ae3a050c3f440d1c11f7cff7d1a @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10386: a80ef2c1e92629cd69081e088ded4a58b609b5f3 @ 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_10386/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/i915/psr: Always wait for idle state when disabling PSR

[Dhinakaran Pandiyan]

On Fri, 2018-10-05 at 16:35 -0700, 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.
Agreed.

> 
> 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 | 43 +++++++++++++++---------------
> --
>  1 file changed, 20 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c
> b/drivers/gpu/drm/i915/intel_psr.c
> index 423cdf84059c..cd9a60d1efa1 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -661,40 +661,37 @@ 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;
> -
> -		if (dev_priv->psr.psr2_enabled) {
> -			psr_status = EDP_PSR2_STATUS;
> -			psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
> -
> +		if (dev_priv->psr.psr2_enabled)
>  			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_READ(EDP_PSR2_CTL) &
> ~EDP_PSR2_ENABLE);

Is there a way to reuse psr_exit() and move rest of the stuff to the
caller? We won't not need disable_source() if that can be done?



> +		else
>  			I915_WRITE(EDP_PSR_CTL,
>  				   I915_READ(EDP_PSR_CTL) &
> ~EDP_PSR_ENABLE);
> -		}
> -
> -		/* 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");
> -
> -		dev_priv->psr.active = false;
>  	} 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);
>  	}
> +
> +	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");
> +
> +	dev_priv->psr.active = false;
>  }
>  
>  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 1/4] drm/i915/psr: Always wait for idle state when disabling PSR

[Souza, Jose]

On Mon, 2018-10-08 at 15:43 -0700, Dhinakaran Pandiyan wrote:
> On Fri, 2018-10-05 at 16:35 -0700, 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.
> Agreed.
> 
> > 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 | 43 +++++++++++++++-------------
> > --
> > --
> >  1 file changed, 20 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 423cdf84059c..cd9a60d1efa1 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -661,40 +661,37 @@ 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;
> > -
> > -		if (dev_priv->psr.psr2_enabled) {
> > -			psr_status = EDP_PSR2_STATUS;
> > -			psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
> > -
> > +		if (dev_priv->psr.psr2_enabled)
> >  			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_READ(EDP_PSR2_CTL) &
> > ~EDP_PSR2_ENABLE);
> 
> Is there a way to reuse psr_exit() and move rest of the stuff to the
> caller? We won't not need disable_source() if that can be done?

We could move to the caller and reuse psr_exit(), I will do that in
another patch.

> 
> 
> 
> > +		else
> >  			I915_WRITE(EDP_PSR_CTL,
> >  				   I915_READ(EDP_PSR_CTL) &
> > ~EDP_PSR_ENABLE);
> > -		}
> > -
> > -		/* 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");
> > -
> > -		dev_priv->psr.active = false;
> >  	} 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);
> >  	}
> > +
> > +	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");
> > +
> > +	dev_priv->psr.active = false;
> >  }
> >  
> >  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