Re: [PATCH] drm/i915: Make sure PSR is ready for been re-enabled.

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Sep 25, 2014 at 10:50:51AM -0700, Rodrigo Vivi wrote:
> On Thu, Sep 25, 2014 at 10:36 AM, Paulo Zanoni <przanoni@gmail.com> wrote:
> 
> > 2014-09-24 19:16 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> > > Let's make sure PSR is propperly disabled before to re-enabled it.
> > >
> > > According to Spec, after disabled PSR CTL, the Idle state might occur
> > > up to 24ms, that is one full frame time (1/refresh rate),
> > > plus SRD exit training time (max of 6ms),
> > > plus SRD aux channel handshake (max of 1.5ms).
> > >
> > > So if something went wrong PSR will be disabled until next full
> > > enable/disable setup.
> > >
> > > v2: The 24ms above takes in account 16ms for refresh rate on 60Hz mode.
> > However
> > > on low frequency modes this can take longer. So let's use 50ms for
> > safeness.
> > >
> > > v3: Move wait out of psr.lock critical area.
> > >
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >
> > Again, since we already waited for 100ms while queueing
> > intel_edp_psr_work, an additional 50ms is basically useless.
> 
> 
> Agree. But it doesn't hurt it is just a timeout to give more time in case
> psr is still on transition.
> what is unlike I know...

Yeah, looks good enough for now imo, patch merged.

> > I'd
> > really like this function to just have an I915_READ instead of yet
> > another wait, so any necessary wait-time-tuning would be part of the
> > schedule_delayed_work(dev_priv->psr.work) call.
> >
> 
> Uhm. that was my first idea actually. I was willing to kill the delayed
> work at all and use just this read scheme.
> However this didn't worked.... It seems hardware doesn't like when we have
> to much sequential on-off psr switches.
> 
> So 100ms is enough to avoid this high frequency on-off and avoid sloweness
> and other issues.
> 
> The 50ms extra is just to be on the safe side checking if we can reaable it
> or give a bit more time for it instead of just wait until next full
> enable/disable sequence.

Is there a way to have a less massive psr entry/exit knob? Maybe one that
just does a one-shot upload?

If that doesn't work then I think the timeout is totally ok - if we need
to upload a few frames anyway to keep hw happy then a short delay won't
make things worse really. Hopfully single-shot upload for pageflips still
work.

Cheers, Daniel


> 
> 
> >
> > That said, the current patch is already an improvement, so:
> > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >
> 
> Thank you very much.
> 
> 
> >
> > But I'd prefer the solution I proposed :)
> >
> 
> me too. :)
> 
> 
> >
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c | 11 +++++++++++
> > >  1 file changed, 11 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > > index a119b9b..b8699b0 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -2208,6 +2208,17 @@ static void intel_edp_psr_work(struct work_struct
> > *work)
> > >                 container_of(work, typeof(*dev_priv), psr.work.work);
> > >         struct intel_dp *intel_dp = dev_priv->psr.enabled;
> > >
> > > +       /* We have to make sure PSR is ready for re-enable
> > > +        * otherwise it keeps disabled until next full enable/disable
> > cycle.
> > > +        * PSR might take some time to get fully disabled
> > > +        * and be ready for re-enable.
> > > +        */
> > > +       if (wait_for((I915_READ(EDP_PSR_STATUS_CTL(dev_priv->dev)) &
> > > +                     EDP_PSR_STATUS_STATE_MASK) == 0, 50)) {
> > > +               DRM_ERROR("Timed out waiting for PSR Idle for
> > re-enable\n");
> > > +               return;
> > > +       }
> > > +
> > >         mutex_lock(&dev_priv->psr.lock);
> > >         intel_dp = dev_priv->psr.enabled;
> > >
> > > --
> > > 1.9.3
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> >
> >
> > --
> > Paulo Zanoni
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> 
> 
> 
> -- 
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch