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

[Rodrigo Vivi]

On Thu, Sep 25, 2014 at 10:36 AM, Paulo Zanoni <przanoni at gmail.com> wrote:

> 2014-09-24 19:16 GMT-03:00 Rodrigo Vivi <rodrigo.vivi at 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 at ffwll.ch>
> > Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi at 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...



> 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.


>
> That said, the current patch is already an improvement, so:
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni at 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 at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> --
> Paulo Zanoni
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20140925/ac81dbba/attachment.html>