Re: [PATCH 1/2] drm/i915: Modify psr_wait_for_idle to be reused.

[Tarun Vyas <tarun.vyas@intel.com>]

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