* [PATCH 1/4] drm/i915/psr: Always wait for idle state when disabling PSR @ 2018-10-05 23:35 José Roberto de Souza 2018-10-05 23:35 ` [PATCH 2/4] drm/i915: Disable PSR when a PSR aux error happen José Roberto de Souza ` (6 more replies) 0 siblings, 7 replies; 18+ messages in thread From: José Roberto de Souza @ 2018-10-05 23:35 UTC (permalink / raw) To: intel-gfx; +Cc: Dhinakaran Pandiyan, Rodrigo Vivi 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/4] drm/i915: Disable PSR when a PSR aux error happen 2018-10-05 23:35 [PATCH 1/4] drm/i915/psr: Always wait for idle state when disabling PSR José Roberto de Souza @ 2018-10-05 23:35 ` José Roberto de Souza 2018-10-09 0:14 ` Dhinakaran Pandiyan 2018-10-05 23:35 ` [PATCH 3/4] drm/i915: Cache sink_count for eDP José Roberto de Souza ` (5 subsequent siblings) 6 siblings, 1 reply; 18+ messages in thread From: José Roberto de Souza @ 2018-10-05 23:35 UTC (permalink / raw) To: intel-gfx; +Cc: Dhinakaran Pandiyan 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 2/4] drm/i915: Disable PSR when a PSR aux error happen 2018-10-05 23:35 ` [PATCH 2/4] drm/i915: Disable PSR when a PSR aux error happen José Roberto de Souza @ 2018-10-09 0:14 ` Dhinakaran Pandiyan 2018-10-09 0:30 ` Souza, Jose 0 siblings, 1 reply; 18+ messages in thread From: Dhinakaran Pandiyan @ 2018-10-09 0:14 UTC (permalink / raw) To: José Roberto de Souza, intel-gfx 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/4] drm/i915: Disable PSR when a PSR aux error happen 2018-10-09 0:14 ` Dhinakaran Pandiyan @ 2018-10-09 0:30 ` Souza, Jose 2018-10-09 0:49 ` Dhinakaran Pandiyan 0 siblings, 1 reply; 18+ messages in thread From: Souza, Jose @ 2018-10-09 0:30 UTC (permalink / raw) To: intel-gfx, Pandiyan, Dhinakaran 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/4] drm/i915: Disable PSR when a PSR aux error happen 2018-10-09 0:30 ` Souza, Jose @ 2018-10-09 0:49 ` Dhinakaran Pandiyan 2018-10-09 0:57 ` Souza, Jose 0 siblings, 1 reply; 18+ messages in thread From: Dhinakaran Pandiyan @ 2018-10-09 0:49 UTC (permalink / raw) To: Souza, Jose, intel-gfx 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/4] drm/i915: Disable PSR when a PSR aux error happen 2018-10-09 0:49 ` Dhinakaran Pandiyan @ 2018-10-09 0:57 ` Souza, Jose 0 siblings, 0 replies; 18+ messages in thread From: Souza, Jose @ 2018-10-09 0:57 UTC (permalink / raw) To: intel-gfx, Pandiyan, Dhinakaran 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 3/4] drm/i915: Cache sink_count for eDP 2018-10-05 23:35 [PATCH 1/4] drm/i915/psr: Always wait for idle state when disabling PSR José Roberto de Souza 2018-10-05 23:35 ` [PATCH 2/4] drm/i915: Disable PSR when a PSR aux error happen José Roberto de Souza @ 2018-10-05 23:35 ` José Roberto de Souza 2018-10-09 0:19 ` Dhinakaran Pandiyan 2018-10-05 23:35 ` [PATCH 4/4] drm/i915: Check PSR errors instead of retrain while PSR is enabled José Roberto de Souza ` (4 subsequent siblings) 6 siblings, 1 reply; 18+ messages in thread From: José Roberto de Souza @ 2018-10-05 23:35 UTC (permalink / raw) To: intel-gfx; +Cc: Dhinakaran Pandiyan 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 3/4] drm/i915: Cache sink_count for eDP 2018-10-05 23:35 ` [PATCH 3/4] drm/i915: Cache sink_count for eDP José Roberto de Souza @ 2018-10-09 0:19 ` Dhinakaran Pandiyan 2018-10-09 0:35 ` Souza, Jose 0 siblings, 1 reply; 18+ messages in thread From: Dhinakaran Pandiyan @ 2018-10-09 0:19 UTC (permalink / raw) To: José Roberto de Souza, intel-gfx 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/4] drm/i915: Cache sink_count for eDP 2018-10-09 0:19 ` Dhinakaran Pandiyan @ 2018-10-09 0:35 ` Souza, Jose 2018-10-09 0:54 ` Dhinakaran Pandiyan 0 siblings, 1 reply; 18+ messages in thread From: Souza, Jose @ 2018-10-09 0:35 UTC (permalink / raw) To: intel-gfx, Pandiyan, Dhinakaran 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/4] drm/i915: Cache sink_count for eDP 2018-10-09 0:35 ` Souza, Jose @ 2018-10-09 0:54 ` Dhinakaran Pandiyan 2018-10-09 13:27 ` Ville Syrjälä 0 siblings, 1 reply; 18+ messages in thread From: Dhinakaran Pandiyan @ 2018-10-09 0:54 UTC (permalink / raw) To: Souza, Jose, intel-gfx 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/4] drm/i915: Cache sink_count for eDP 2018-10-09 0:54 ` Dhinakaran Pandiyan @ 2018-10-09 13:27 ` Ville Syrjälä 2018-10-10 1:09 ` Souza, Jose 0 siblings, 1 reply; 18+ messages in thread From: Ville Syrjälä @ 2018-10-09 13:27 UTC (permalink / raw) To: Dhinakaran Pandiyan; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/4] drm/i915: Cache sink_count for eDP 2018-10-09 13:27 ` Ville Syrjälä @ 2018-10-10 1:09 ` Souza, Jose 0 siblings, 0 replies; 18+ messages in thread From: Souza, Jose @ 2018-10-10 1:09 UTC (permalink / raw) To: ville.syrjala, Pandiyan, Dhinakaran; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 4/4] drm/i915: Check PSR errors instead of retrain while PSR is enabled 2018-10-05 23:35 [PATCH 1/4] drm/i915/psr: Always wait for idle state when disabling PSR José Roberto de Souza 2018-10-05 23:35 ` [PATCH 2/4] drm/i915: Disable PSR when a PSR aux error happen José Roberto de Souza 2018-10-05 23:35 ` [PATCH 3/4] drm/i915: Cache sink_count for eDP José Roberto de Souza @ 2018-10-05 23:35 ` José Roberto de Souza 2018-10-06 0:50 ` ✗ Fi.CI.SPARSE: warning for series starting with [1/4] drm/i915/psr: Always wait for idle state when disabling PSR Patchwork ` (3 subsequent siblings) 6 siblings, 0 replies; 18+ messages in thread From: José Roberto de Souza @ 2018-10-05 23:35 UTC (permalink / raw) To: intel-gfx; +Cc: Dhinakaran Pandiyan 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* ✗ Fi.CI.SPARSE: warning for series starting with [1/4] drm/i915/psr: Always wait for idle state when disabling PSR 2018-10-05 23:35 [PATCH 1/4] drm/i915/psr: Always wait for idle state when disabling PSR José Roberto de Souza ` (2 preceding siblings ...) 2018-10-05 23:35 ` [PATCH 4/4] drm/i915: Check PSR errors instead of retrain while PSR is enabled José Roberto de Souza @ 2018-10-06 0:50 ` Patchwork 2018-10-06 1:10 ` ✓ Fi.CI.BAT: success " Patchwork ` (2 subsequent siblings) 6 siblings, 0 replies; 18+ messages in thread From: Patchwork @ 2018-10-06 0:50 UTC (permalink / raw) To: José Roberto de Souza; +Cc: intel-gfx == 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* ✓ Fi.CI.BAT: success for series starting with [1/4] drm/i915/psr: Always wait for idle state when disabling PSR 2018-10-05 23:35 [PATCH 1/4] drm/i915/psr: Always wait for idle state when disabling PSR José Roberto de Souza ` (3 preceding siblings ...) 2018-10-06 0:50 ` ✗ Fi.CI.SPARSE: warning for series starting with [1/4] drm/i915/psr: Always wait for idle state when disabling PSR Patchwork @ 2018-10-06 1:10 ` Patchwork 2018-10-06 8:52 ` ✓ Fi.CI.IGT: " Patchwork 2018-10-08 22:43 ` [PATCH 1/4] " Dhinakaran Pandiyan 6 siblings, 0 replies; 18+ messages in thread From: Patchwork @ 2018-10-06 1:10 UTC (permalink / raw) To: José Roberto de Souza; +Cc: intel-gfx == 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* ✓ Fi.CI.IGT: success for series starting with [1/4] drm/i915/psr: Always wait for idle state when disabling PSR 2018-10-05 23:35 [PATCH 1/4] drm/i915/psr: Always wait for idle state when disabling PSR José Roberto de Souza ` (4 preceding siblings ...) 2018-10-06 1:10 ` ✓ Fi.CI.BAT: success " Patchwork @ 2018-10-06 8:52 ` Patchwork 2018-10-08 22:43 ` [PATCH 1/4] " Dhinakaran Pandiyan 6 siblings, 0 replies; 18+ messages in thread From: Patchwork @ 2018-10-06 8:52 UTC (permalink / raw) To: José Roberto de Souza; +Cc: intel-gfx == 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/4] drm/i915/psr: Always wait for idle state when disabling PSR 2018-10-05 23:35 [PATCH 1/4] drm/i915/psr: Always wait for idle state when disabling PSR José Roberto de Souza ` (5 preceding siblings ...) 2018-10-06 8:52 ` ✓ Fi.CI.IGT: " Patchwork @ 2018-10-08 22:43 ` Dhinakaran Pandiyan 2018-10-10 1:12 ` Souza, Jose 6 siblings, 1 reply; 18+ messages in thread From: Dhinakaran Pandiyan @ 2018-10-08 22:43 UTC (permalink / raw) To: José Roberto de Souza, intel-gfx, Ville Syrjälä Cc: Rodrigo Vivi 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/4] drm/i915/psr: Always wait for idle state when disabling PSR 2018-10-08 22:43 ` [PATCH 1/4] " Dhinakaran Pandiyan @ 2018-10-10 1:12 ` Souza, Jose 0 siblings, 0 replies; 18+ messages in thread From: Souza, Jose @ 2018-10-10 1:12 UTC (permalink / raw) To: ville.syrjala, intel-gfx, Pandiyan, Dhinakaran; +Cc: Vivi, Rodrigo 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2018-10-10 1:13 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-10-05 23:35 [PATCH 1/4] drm/i915/psr: Always wait for idle state when disabling PSR José Roberto de Souza 2018-10-05 23:35 ` [PATCH 2/4] drm/i915: Disable PSR when a PSR aux error happen José Roberto de Souza 2018-10-09 0:14 ` Dhinakaran Pandiyan 2018-10-09 0:30 ` Souza, Jose 2018-10-09 0:49 ` Dhinakaran Pandiyan 2018-10-09 0:57 ` Souza, Jose 2018-10-05 23:35 ` [PATCH 3/4] drm/i915: Cache sink_count for eDP José Roberto de Souza 2018-10-09 0:19 ` Dhinakaran Pandiyan 2018-10-09 0:35 ` Souza, Jose 2018-10-09 0:54 ` Dhinakaran Pandiyan 2018-10-09 13:27 ` Ville Syrjälä 2018-10-10 1:09 ` Souza, Jose 2018-10-05 23:35 ` [PATCH 4/4] drm/i915: Check PSR errors instead of retrain while PSR is enabled José Roberto de Souza 2018-10-06 0:50 ` ✗ Fi.CI.SPARSE: warning for series starting with [1/4] drm/i915/psr: Always wait for idle state when disabling PSR Patchwork 2018-10-06 1:10 ` ✓ Fi.CI.BAT: success " Patchwork 2018-10-06 8:52 ` ✓ Fi.CI.IGT: " Patchwork 2018-10-08 22:43 ` [PATCH 1/4] " Dhinakaran Pandiyan 2018-10-10 1:12 ` Souza, Jose
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.