<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Sep 25, 2014 at 10:36 AM, Paulo Zanoni <span dir="ltr"><<a href="mailto:przanoni@gmail.com" target="_blank">przanoni@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">2014-09-24 19:16 GMT-03:00 Rodrigo Vivi <<a href="mailto:rodrigo.vivi@intel.com">rodrigo.vivi@intel.com</a>>:<br>
<span class="">> Let's make sure PSR is propperly disabled before to re-enabled it.<br>
><br>
> According to Spec, after disabled PSR CTL, the Idle state might occur<br>
> up to 24ms, that is one full frame time (1/refresh rate),<br>
> plus SRD exit training time (max of 6ms),<br>
> plus SRD aux channel handshake (max of 1.5ms).<br>
><br>
> So if something went wrong PSR will be disabled until next full<br>
> enable/disable setup.<br>
><br>
> v2: The 24ms above takes in account 16ms for refresh rate on 60Hz mode. However<br>
> on low frequency modes this can take longer. So let's use 50ms for safeness.<br>
><br>
> v3: Move wait out of psr.lock critical area.<br>
><br>
> Cc: Daniel Vetter <<a href="mailto:daniel.vetter@ffwll.ch">daniel.vetter@ffwll.ch</a>><br>
> Cc: Paulo Zanoni <<a href="mailto:paulo.r.zanoni@intel.com">paulo.r.zanoni@intel.com</a>><br>
> Signed-off-by: Rodrigo Vivi <<a href="mailto:rodrigo.vivi@intel.com">rodrigo.vivi@intel.com</a>><br>
<br>
</span>Again, since we already waited for 100ms while queueing<br>
intel_edp_psr_work, an additional 50ms is basically useless. </blockquote><div><br></div><div>Agree. But it doesn't hurt it is just a timeout to give more time in case psr is still on transition.</div><div>what is unlike I know...</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I'd<br>
really like this function to just have an I915_READ instead of yet<br>
another wait, so any necessary wait-time-tuning would be part of the<br>
schedule_delayed_work(dev_priv->psr.work) call.<br></blockquote><div><br></div><div>Uhm. that was my first idea actually. I was willing to kill the delayed work at all and use just this read scheme.</div><div>However this didn't worked.... It seems hardware doesn't like when we have to much sequential on-off psr switches.</div><div><br></div><div>So 100ms is enough to avoid this high frequency on-off and avoid sloweness and other issues.</div><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
That said, the current patch is already an improvement, so:<br>
Reviewed-by: Paulo Zanoni <<a href="mailto:paulo.r.zanoni@intel.com">paulo.r.zanoni@intel.com</a>><br></blockquote><div><br></div><div>Thank you very much.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
But I'd prefer the solution I proposed :)<br></blockquote><div><br></div><div>me too. :)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><br>
> ---<br>
>  drivers/gpu/drm/i915/intel_dp.c | 11 +++++++++++<br>
>  1 file changed, 11 insertions(+)<br>
><br>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c<br>
> index a119b9b..b8699b0 100644<br>
> --- a/drivers/gpu/drm/i915/intel_dp.c<br>
> +++ b/drivers/gpu/drm/i915/intel_dp.c<br>
> @@ -2208,6 +2208,17 @@ static void intel_edp_psr_work(struct work_struct *work)<br>
>                 container_of(work, typeof(*dev_priv), psr.work.work);<br>
>         struct intel_dp *intel_dp = dev_priv->psr.enabled;<br>
><br>
> +       /* We have to make sure PSR is ready for re-enable<br>
> +        * otherwise it keeps disabled until next full enable/disable cycle.<br>
> +        * PSR might take some time to get fully disabled<br>
> +        * and be ready for re-enable.<br>
> +        */<br>
> +       if (wait_for((I915_READ(EDP_PSR_STATUS_CTL(dev_priv->dev)) &<br>
> +                     EDP_PSR_STATUS_STATE_MASK) == 0, 50)) {<br>
> +               DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n");<br>
> +               return;<br>
> +       }<br>
> +<br>
>         mutex_lock(&dev_priv->psr.lock);<br>
>         intel_dp = dev_priv->psr.enabled;<br>
><br>
> --<br>
> 1.9.3<br>
><br>
> _______________________________________________<br>
> Intel-gfx mailing list<br>
> <a href="mailto:Intel-gfx@lists.freedesktop.org">Intel-gfx@lists.freedesktop.org</a><br>
> <a href="http://lists.freedesktop.org/mailman/listinfo/intel-gfx" target="_blank">http://lists.freedesktop.org/mailman/listinfo/intel-gfx</a><br>
<br>
<br>
<br>
</div></div><span class="HOEnZb"><font color="#888888">--<br>
Paulo Zanoni<br>
</font></span><div class="HOEnZb"><div class="h5">_______________________________________________<br>
Intel-gfx mailing list<br>
<a href="mailto:Intel-gfx@lists.freedesktop.org">Intel-gfx@lists.freedesktop.org</a><br>
<a href="http://lists.freedesktop.org/mailman/listinfo/intel-gfx" target="_blank">http://lists.freedesktop.org/mailman/listinfo/intel-gfx</a><br>
</div></div></blockquote></div><br><br clear="all"><div><br></div>-- <br><div>Rodrigo Vivi</div><div>Blog: <a href="http://blog.vivi.eng.br" target="_blank">http://blog.vivi.eng.br</a></div><div> </div>
</div></div>