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

[Paulo Zanoni]

2014-09-17 14:23 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).
>
> 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.

Well, the patch looks correct, but it doesn't seem to take into
consideration the fact that we already waited for 100ms before
triggering psr.work. Also, we do the wait that you added with psr.lock
locked, so we could be blocking user-space from doing other stuff for
the whole 50ms, and that's an eternity and a half.

So maybe we should tune the schedule_delayed_work() call at
intel_edp_psr_flush() based on the calculation you did above (or just
keep the 100ms, since it seems to be above the timeout for any modes
bigger than 11Hz). And then when we're inside the work function, we
should just I915_READ(EDP_PSR_STATUS_CTL) - instead of doing
wait_for() -, and in case PSR is not idle yet, there's a huge
probability that waiting for more 50ms won't really help. We could
also try to reschedule psr.work to be triggered again in the future in
case the bits we want are not ready, but by doing this we also risk
rescheduling psr.work forever.

More bikeshed on the timeout thing: can't we try discover the exact
amount of time we need to sleep based on the refresh rate? We could
try to look at the mode structure...

tl;dr: if you remove the wait_for() call and keep just the I915_READ,
I can give a R-B tag, but other patches could be acceptable too.


>
> So if something went wrong PSR will be disabled until next full
> enable/disable setup.
>
> 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>
> ---
>  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 2f0eee5..2e8c544 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1885,6 +1885,17 @@ static void intel_edp_psr_do_enable(struct intel_dp *intel_dp)
>         WARN_ON(dev_priv->psr.active);
>         lockdep_assert_held(&dev_priv->psr.lock);
>
> +       /* 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)) &
> +                     EDP_PSR_STATUS_STATE_MASK) == 0, 50)) {
> +               DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n");
> +               return;
> +       }
> +
>         /* Enable/Re-enable PSR on the host */
>         intel_edp_psr_enable_source(intel_dp);
>
> --
> 1.9.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni