| Message ID | 20180514204922.82073-1-tarun.vyas@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Tarun Vyas (2018-05-14 21:49:20) > intel_pipe_update_start also needs to wait for PSR to idle > out. Need some minor modifications in psr_wait_for_idle in > order to reuse it. > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com> > --- > drivers/gpu/drm/i915/intel_psr.c | 29 ++++++++++++++++++----------- > 1 file changed, 18 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > index db27f2faa1de..40aafc0f4513 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -889,11 +889,15 @@ static bool psr_wait_for_idle(struct drm_i915_private *dev_priv) > i915_reg_t reg; > u32 mask; > int err; > + bool wait = false; > + > + mutex_lock(&dev_priv->psr.lock); > > intel_dp = dev_priv->psr.enabled; > if (!intel_dp) > - return false; > + goto unlock; > > + wait = true; > if (HAS_DDI(dev_priv)) { > if (dev_priv->psr.psr2_enabled) { > reg = EDP_PSR2_STATUS; > @@ -911,15 +915,18 @@ static bool psr_wait_for_idle(struct drm_i915_private *dev_priv) > mask = VLV_EDP_PSR_IN_TRANS; > } > > +unlock: > mutex_unlock(&dev_priv->psr.lock); > > - err = intel_wait_for_register(dev_priv, reg, mask, 0, 50); > - if (err) > - DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n"); > + if(wait) { > + err = intel_wait_for_register(dev_priv, reg, mask, 0, 50); > + if (err) { > + DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n"); > + wait = false; > + } > + } > > - /* After the unlocked wait, verify that PSR is still wanted! */ > - mutex_lock(&dev_priv->psr.lock); > - return err == 0 && dev_priv->psr.enabled; > + return wait; > } > > static void intel_psr_work(struct work_struct *work) > @@ -927,7 +934,6 @@ static void intel_psr_work(struct work_struct *work) > struct drm_i915_private *dev_priv = > container_of(work, typeof(*dev_priv), psr.work.work); > > - mutex_lock(&dev_priv->psr.lock); > > /* > * We have to make sure PSR is ready for re-enable > @@ -936,14 +942,15 @@ static void intel_psr_work(struct work_struct *work) > * and be ready for re-enable. > */ > if (!psr_wait_for_idle(dev_priv)) > - goto unlock; > + return; > > - /* > + /* After the unlocked wait, verify that PSR is still wanted! > * The delayed work can race with an invalidate hence we need to > * recheck. Since psr_flush first clears this and then reschedules we > * won't ever miss a flush when bailing out here. > */ > - if (dev_priv->psr.busy_frontbuffer_bits) > + mutex_lock(&dev_priv->psr.lock); > + if (dev_priv->psr.enabled && dev_priv->psr.busy_frontbuffer_bits) > goto unlock; I'm not sold on the locking dropping here, doing so inside the wait is bad enough. (And do we need to there anyway?) Since you need to introduce intel_psr_wait_for_idle() anyway, how about void intel_psr_wait_for_idle(...) { mutex_lock(&i915->psr.lock); psr_wait_for_idle(); mutex_unlock(&i915->psr.lock); } -Chris
On Mon, May 14, 2018 at 10:15:19PM +0100, Chris Wilson wrote: > Quoting Tarun Vyas (2018-05-14 21:49:20) > > intel_pipe_update_start also needs to wait for PSR to idle > > out. Need some minor modifications in psr_wait_for_idle in > > order to reuse it. > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com> > > --- > > drivers/gpu/drm/i915/intel_psr.c | 29 ++++++++++++++++++----------- > > 1 file changed, 18 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > > index db27f2faa1de..40aafc0f4513 100644 > > --- a/drivers/gpu/drm/i915/intel_psr.c > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > @@ -889,11 +889,15 @@ static bool psr_wait_for_idle(struct drm_i915_private *dev_priv) > > i915_reg_t reg; > > u32 mask; > > int err; > > + bool wait = false; > > + > > + mutex_lock(&dev_priv->psr.lock); > > > > intel_dp = dev_priv->psr.enabled; > > if (!intel_dp) > > - return false; > > + goto unlock; > > > > + wait = true; > > if (HAS_DDI(dev_priv)) { > > if (dev_priv->psr.psr2_enabled) { > > reg = EDP_PSR2_STATUS; > > @@ -911,15 +915,18 @@ static bool psr_wait_for_idle(struct drm_i915_private *dev_priv) > > mask = VLV_EDP_PSR_IN_TRANS; > > } > > > > +unlock: > > mutex_unlock(&dev_priv->psr.lock); > > > > - err = intel_wait_for_register(dev_priv, reg, mask, 0, 50); > > - if (err) > > - DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n"); > > + if(wait) { > > + err = intel_wait_for_register(dev_priv, reg, mask, 0, 50); > > + if (err) { > > + DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n"); > > + wait = false; > > + } > > + } > > > > - /* After the unlocked wait, verify that PSR is still wanted! */ > > - mutex_lock(&dev_priv->psr.lock); > > - return err == 0 && dev_priv->psr.enabled; > > + return wait; I wanted to avoid taking this additional lock b/c all we need inside intel_pipe_update_start is for PSR to go idle. So can we retain moving it to intel_psr_work ? > > } > > > > static void intel_psr_work(struct work_struct *work) > > @@ -927,7 +934,6 @@ static void intel_psr_work(struct work_struct *work) > > struct drm_i915_private *dev_priv = > > container_of(work, typeof(*dev_priv), psr.work.work); > > > > - mutex_lock(&dev_priv->psr.lock); > > > > /* > > * We have to make sure PSR is ready for re-enable > > @@ -936,14 +942,15 @@ static void intel_psr_work(struct work_struct *work) > > * and be ready for re-enable. > > */ > > if (!psr_wait_for_idle(dev_priv)) > > - goto unlock; > > + return; > > > > - /* > > + /* After the unlocked wait, verify that PSR is still wanted! > > * The delayed work can race with an invalidate hence we need to > > * recheck. Since psr_flush first clears this and then reschedules we > > * won't ever miss a flush when bailing out here. > > */ > > - if (dev_priv->psr.busy_frontbuffer_bits) > > + mutex_lock(&dev_priv->psr.lock); > > + if (dev_priv->psr.enabled && dev_priv->psr.busy_frontbuffer_bits) > > goto unlock; > > I'm not sold on the locking dropping here, doing so inside the wait is > bad enough. (And do we need to there anyway?) > Thanks for the comments Chris. In that case, as suggested by Rodrigo, can we assert that the lock is held, inside psr_wait_for_idle() ? > Since you need to introduce intel_psr_wait_for_idle() anyway, how about > > void intel_psr_wait_for_idle(...) > { > mutex_lock(&i915->psr.lock); > psr_wait_for_idle(); > mutex_unlock(&i915->psr.lock); > } > -Chris >> /* After the unlocked wait, verify that PSR is still wanted! */ >> mutex_lock(&dev_priv->psr.lock); >> return err == 0 && dev_priv->psr.enabled;
On Mon, May 14, 2018 at 03:00:15PM -0700, Tarun Vyas wrote: > On Mon, May 14, 2018 at 10:15:19PM +0100, Chris Wilson wrote: > > Quoting Tarun Vyas (2018-05-14 21:49:20) > > > intel_pipe_update_start also needs to wait for PSR to idle > > > out. Need some minor modifications in psr_wait_for_idle in > > > order to reuse it. > > > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_psr.c | 29 ++++++++++++++++++----------- > > > 1 file changed, 18 insertions(+), 11 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > > > index db27f2faa1de..40aafc0f4513 100644 > > > --- a/drivers/gpu/drm/i915/intel_psr.c > > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > > @@ -889,11 +889,15 @@ static bool psr_wait_for_idle(struct drm_i915_private *dev_priv) > > > i915_reg_t reg; > > > u32 mask; > > > int err; > > > + bool wait = false; > > > + > > > + mutex_lock(&dev_priv->psr.lock); > > > > > > intel_dp = dev_priv->psr.enabled; > > > if (!intel_dp) > > > - return false; > > > + goto unlock; > > > > > > + wait = true; > > > if (HAS_DDI(dev_priv)) { > > > if (dev_priv->psr.psr2_enabled) { > > > reg = EDP_PSR2_STATUS; > > > @@ -911,15 +915,18 @@ static bool psr_wait_for_idle(struct drm_i915_private *dev_priv) > > > mask = VLV_EDP_PSR_IN_TRANS; > > > } > > > > > > +unlock: > > > mutex_unlock(&dev_priv->psr.lock); > > > > > > - err = intel_wait_for_register(dev_priv, reg, mask, 0, 50); > > > - if (err) > > > - DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n"); > > > + if(wait) { > > > + err = intel_wait_for_register(dev_priv, reg, mask, 0, 50); > > > + if (err) { > > > + DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n"); > > > + wait = false; > > > + } > > > + } > > > > > > - /* After the unlocked wait, verify that PSR is still wanted! */ > > > - mutex_lock(&dev_priv->psr.lock); > > > - return err == 0 && dev_priv->psr.enabled; > > > + return wait; > I wanted to avoid taking this additional lock b/c all we need inside intel_pipe_update_start is for PSR to go idle. So can we retain moving it to intel_psr_work ? > > > } > > > > > > static void intel_psr_work(struct work_struct *work) > > > @@ -927,7 +934,6 @@ static void intel_psr_work(struct work_struct *work) > > > struct drm_i915_private *dev_priv = > > > container_of(work, typeof(*dev_priv), psr.work.work); > > > > > > - mutex_lock(&dev_priv->psr.lock); > > > > > > /* > > > * We have to make sure PSR is ready for re-enable > > > @@ -936,14 +942,15 @@ static void intel_psr_work(struct work_struct *work) > > > * and be ready for re-enable. > > > */ > > > if (!psr_wait_for_idle(dev_priv)) > > > - goto unlock; > > > + return; > > > > > > - /* > > > + /* After the unlocked wait, verify that PSR is still wanted! > > > * The delayed work can race with an invalidate hence we need to > > > * recheck. Since psr_flush first clears this and then reschedules we > > > * won't ever miss a flush when bailing out here. > > > */ > > > - if (dev_priv->psr.busy_frontbuffer_bits) > > > + mutex_lock(&dev_priv->psr.lock); > > > + if (dev_priv->psr.enabled && dev_priv->psr.busy_frontbuffer_bits) > > > goto unlock; > > > > I'm not sold on the locking dropping here, doing so inside the wait is > > bad enough. (And do we need to there anyway?) > > Per the commit message in "daeb725e drm/i915/psr: Chase psr.enabled only under the psr.lock", the wait_for_register is done after dropping the locks so that we don't block the modeset path. > > Since you need to introduce intel_psr_wait_for_idle() anyway, how about > > > > void intel_psr_wait_for_idle(...) > > { > > mutex_lock(&i915->psr.lock); > > psr_wait_for_idle(); > > mutex_unlock(&i915->psr.lock); On a second thought, I need to wait for PSR idle inside intel_pipe_update_start. Now, intel_psr_disable/intel_psr_enable will *not race* inside intel_pipe_update_start(), so we do not need any psr.locks, at all. The locking dropping inside psr_wait_for_idle makes it difficult to move the common wait code, so I was thinking if I can do something like: void intel_psr_wait_for_idle(...) { if (dev_priv->psr.psr2_enabled) { reg = EDP_PSR2_STATUS; mask = EDP_PSR2_STATUS_STATE_MASK; } else { reg = EDP_PSR_STATUS; mask = EDP_PSR_STATUS_STATE_MASK; } intel_wait_for_register(dev_priv, reg, mask, 0, 25); } which is essentially a lockless version of psr_wait_for_idle(). So, there is code duplication here, but I can't help it. Any thoughts ? > > } > > -Chris > >> /* After the unlocked wait, verify that PSR is still wanted! */ > >> mutex_lock(&dev_priv->psr.lock); > >> return err == 0 && dev_priv->psr.enabled;
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index db27f2faa1de..40aafc0f4513 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -889,11 +889,15 @@ static bool psr_wait_for_idle(struct drm_i915_private *dev_priv) i915_reg_t reg; u32 mask; int err; + bool wait = false; + + mutex_lock(&dev_priv->psr.lock); intel_dp = dev_priv->psr.enabled; if (!intel_dp) - return false; + goto unlock; + wait = true; if (HAS_DDI(dev_priv)) { if (dev_priv->psr.psr2_enabled) { reg = EDP_PSR2_STATUS; @@ -911,15 +915,18 @@ static bool psr_wait_for_idle(struct drm_i915_private *dev_priv) mask = VLV_EDP_PSR_IN_TRANS; } +unlock: mutex_unlock(&dev_priv->psr.lock); - err = intel_wait_for_register(dev_priv, reg, mask, 0, 50); - if (err) - DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n"); + if(wait) { + err = intel_wait_for_register(dev_priv, reg, mask, 0, 50); + if (err) { + DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n"); + wait = false; + } + } - /* After the unlocked wait, verify that PSR is still wanted! */ - mutex_lock(&dev_priv->psr.lock); - return err == 0 && dev_priv->psr.enabled; + return wait; } static void intel_psr_work(struct work_struct *work) @@ -927,7 +934,6 @@ static void intel_psr_work(struct work_struct *work) struct drm_i915_private *dev_priv = container_of(work, typeof(*dev_priv), psr.work.work); - mutex_lock(&dev_priv->psr.lock); /* * We have to make sure PSR is ready for re-enable @@ -936,14 +942,15 @@ static void intel_psr_work(struct work_struct *work) * and be ready for re-enable. */ if (!psr_wait_for_idle(dev_priv)) - goto unlock; + return; - /* + /* After the unlocked wait, verify that PSR is still wanted! * The delayed work can race with an invalidate hence we need to * recheck. Since psr_flush first clears this and then reschedules we * won't ever miss a flush when bailing out here. */ - if (dev_priv->psr.busy_frontbuffer_bits) + mutex_lock(&dev_priv->psr.lock); + if (dev_priv->psr.enabled && dev_priv->psr.busy_frontbuffer_bits) goto unlock; intel_psr_activate(dev_priv->psr.enabled);
intel_pipe_update_start also needs to wait for PSR to idle out. Need some minor modifications in psr_wait_for_idle in order to reuse it. Cc: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Tarun Vyas <tarun.vyas@intel.com> --- drivers/gpu/drm/i915/intel_psr.c | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-)