Re: [PATCH v2] drm/i915: Wait for PSR exit before checking for vblank evasion

[Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>]

On Mon, 2018-05-14 at 13:49 -0700, Tarun Vyas wrote:
> The PIPEDSL freezes on PSR entry and if PSR hasn't fully exited, then
> the pipe_update_start call schedules itself out to check back later.
> 
> On ChromeOS-4.4 kernel, which is fairly up-to-date w.r.t drm/i915 but
> lags w.r.t core kernel code, hot plugging an external display
> triggers
> tons of "potential atomic update errors" in the dmesg, on *pipe A*. A
> closer analysis reveals that we try to read the scanline 3 times and
> eventually timeout, b/c PSR hasn't exited fully leading to a PIPEDSL
> stuck @ 1599. This issue is not seen on upstream kernels, b/c for
> *some*
> reason we loop inside intel_pipe_update start for ~2+ msec which in
> this
> case is more than enough to exit PSR fully, hence an *unstuck*
> PIPEDSL
> counter, hence no error. On the other hand, the ChromeOS kernel
> spends
> ~1.1 msec looping inside intel_pipe_update_start and hence errors out
> b/c the source is still in PSR.
> 
> Regardless, we should wait for PSR exit (if PSR is supported and
> active
> on the current pipe) before reading the PIPEDSL, b/c if we haven't
> fully exited PSR, then checking for vblank evasion isn't actually
> applicable.
> 
> This scenario applies to a configuration with an additional pipe,
> as of now
> 
> Signed-off-by: Tarun Vyas <tarun.vyas@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_sprite.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> b/drivers/gpu/drm/i915/intel_sprite.c
> index ee23613f9fd4..481d310e5c3b 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -107,14 +107,17 @@ void intel_pipe_update_start(const struct
> intel_crtc_state *new_crtc_state)
>  						      VBLANK_EVASION
> _TIME_US);
>  	max = vblank_start - 1;
>  
> -	local_irq_disable();
> -
>  	if (min <= 0 || max <= 0)
>  		return;
>  
>  	if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
>  		return;
>  
> +	if(new_crtc_state->has_psr && dev_priv->psr.active)
> +		psr_wait_for_idle(dev_priv);

How about just waiting for PSR_STATUS to idle without grabbing any
locks or checking whether PSR is active?

Status should be idle if PSR was disabled or on it's way to becoming
idle if it was enabled. Even if PSR did get enabled while we are in
pipe_update_start(), it will not be active as long as VBIs are enabled.



> +
> +	local_irq_disable();
> +
>  	crtc->debug.min_vbl = min;
>  	crtc->debug.max_vbl = max;
>  	trace_i915_pipe_update_start(crtc);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx