On Thu, Sep 25, 2014 at 10:36 AM, Paulo Zanoni <przanoni@xxxxxxxxx> wrote:
2014-09-24 19:16 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>:
> 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@xxxxxxxx>
> Cc: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
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@xxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
