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@xxxxxxxxxxxxxxxxxx>
> > > Signed-off-by: Tarun Vyas <tarun.vyas@xxxxxxxxx>
> > > ---
> > > 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;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
