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

[Tarun Vyas]

On Tue, Jun 19, 2018 at 02:59:54PM -0700, Tarun Vyas wrote:
> On Tue, Jun 19, 2018 at 02:54:07PM -0700, Dhinakaran Pandiyan wrote:
> > On Tue, 2018-06-19 at 14:27 -0700, Dhinakaran Pandiyan wrote:
> > > 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 
> > > > ---
> > > >  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_EVASI
> > > > ON
> > > > _TIME_US);
> > > >     max = vblank_start - 1;
> > > >  
> > > > -   local_irq_disable();
> > > > -
> > > >     if (min <= 0 || max <= 0)
> > > >     return;
> > > >  
> > > >     if (WARN_ON(drm_crtc_vblank_get(>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.
> > > 
> Right, if we are OK with some duplication (of psr_wait_for_idle) inside 
> intel_psr.c, then we can duplicate the PSR2 vs. PSR check that's being done 
> in psr_wait_for_idle and then just wait without grabbing any locks, so 
> essentially a lockless version of psr_wait_for_idle()
> > Correct me if this was already considered, why not wait until the
> > scanline counter starts moving? I see we have a 
> > intel_wait_for_pipe_scanline_moving(crtc) that's used when the
> > pipe is enabled.
> > 
> > -DK
> 
> Didn't consider this before, but, pipe_scanline_is_moving waits for a minimum 
> of 5 msec. Are we OK with a min wait of 5 msec inside pipe_update_start ? 
> Heuristically, waiting for PSR idle has almost always returned within < 2 
> msec. Occasionally it takes upto 1 full frame.
As per some preliminary measurements
Approach 1:
Wait *unconditionally* (so no need to check for PSR enabled/disabled 
and hence no locks) for PSR_STATUS to IDLE out. 
This takes ~7msec when PSR is active and ~2 usec when PSR is inactive/disabled.

Approach 2:
Use intel_wait_for_pipe_scanline_moving to wait for PIPEDSL to start 
moving after PSR exit. Currently, this ends up waiting for a minimum of 5 msec 
but I changed this to accept a caller defined value for the delay.
After the above changes, this approach takes ~7msec when PSR is active and 
~25-40 usec with PSR disabled, b/c we still need to check for at least 10+ usec 
and see if PIPEDSL moved, if it did, we wait for longer, otherwise we move on.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Dhinakaran Pandiyan]

On Tue, 2018-06-19 at 14:59 -0700, Tarun Vyas wrote:
> On Tue, Jun 19, 2018 at 02:54:07PM -0700, Dhinakaran Pandiyan wrote:
> > 
> > On Tue, 2018-06-19 at 14:27 -0700, Dhinakaran Pandiyan wrote:
> > > 
> > > 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 
> > > > ---
> > > >  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_E
> > > > VASI
> > > > ON
> > > > _TIME_US);
> > > >     max = vblank_start - 1;
> > > >  
> > > > -   local_irq_disable();
> > > > -
> > > >     if (min <= 0 || max <= 0)
> > > >     return;
> > > >  
> > > >     if (WARN_ON(drm_crtc_vblank_get(>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.
> > > 
> Right, if we are OK with some duplication (of psr_wait_for_idle)
> inside intel_psr.c, then we can duplicate the PSR2 vs. PSR check
> that's being done in psr_wait_for_idle and then just wait without
> grabbing any locks, so essentially a lockless version of
> psr_wait_for_idle()

Yeah, you can extract the wait into psr_wait_for_idle_locked() 

> > 
> > Correct me if this was already considered, why not wait until the
> > scanline counter starts moving? I see we have a 
> > intel_wait_for_pipe_scanline_moving(crtc) that's used when the
> > pipe is enabled.
> > 
> > -DK
> Didn't consider this before, but, pipe_scanline_is_moving waits for a
> minimum of 5 msec. Are we OK with a min wait of 5 msec inside
> pipe_update_start ? Heuristically, waiting for PSR idle has almost
> always returned within < 2 msec. Occasionally it takes upto 1 full
> frame.

We should be able to change that function.
 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tarun Vyas]

On Tue, Jun 19, 2018 at 02:54:07PM -0700, Dhinakaran Pandiyan wrote:
> On Tue, 2018-06-19 at 14:27 -0700, Dhinakaran Pandiyan wrote:
> > 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 
> > > ---
> > >  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_EVASI
> > > ON
> > > _TIME_US);
> > >   max = vblank_start - 1;
> > >  
> > > - local_irq_disable();
> > > -
> > >   if (min <= 0 || max <= 0)
> > >   return;
> > >  
> > >   if (WARN_ON(drm_crtc_vblank_get(>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.
> > 
Right, if we are OK with some duplication (of psr_wait_for_idle) inside 
intel_psr.c, then we can duplicate the PSR2 vs. PSR check that's being done in 
psr_wait_for_idle and then just wait without grabbing any locks, so essentially 
a lockless version of psr_wait_for_idle()
> Correct me if this was already considered, why not wait until the
> scanline counter starts moving? I see we have a 
>   intel_wait_for_pipe_scanline_moving(crtc) that's used when the
> pipe is enabled.
> 
> -DK

Didn't consider this before, but, pipe_scanline_is_moving waits for a minimum 
of 5 msec. Are we OK with a min wait of 5 msec inside pipe_update_start ? 
Heuristically, waiting for PSR idle has almost always returned within < 2 msec. 
Occasionally it takes upto 1 full frame. 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Dhinakaran Pandiyan]

On Tue, 2018-06-19 at 14:27 -0700, Dhinakaran Pandiyan wrote:
> 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 
> > ---
> >  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_EVASI
> > ON
> > _TIME_US);
> >     max = vblank_start - 1;
> >  
> > -   local_irq_disable();
> > -
> >     if (min <= 0 || max <= 0)
> >     return;
> >  
> >     if (WARN_ON(drm_crtc_vblank_get(>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.
> 
Correct me if this was already considered, why not wait until the
scanline counter starts moving? I see we have a 
intel_wait_for_pipe_scanline_moving(crtc) that's used when the
pipe is enabled.

-DK
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Dhinakaran Pandiyan]

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

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

[Tarun Vyas]

On Mon, May 14, 2018 at 10:16:38PM +0100, Chris Wilson wrote:
> Quoting Tarun Vyas (2018-05-14 21:49:22)
> > 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 
> > ---
> >  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(>base)))
> > return;
> >  
> > +   if(new_crtc_state->has_psr && dev_priv->psr.active)
> > +   psr_wait_for_idle(dev_priv);
> > +
> > +   local_irq_disable();
> 
> Pop quiz, does intel_pipe_update_finish() unconditionally assume it is
> called with irqs disabled?
> -Chris
Unless local_irq_disable() fails, intel_pipe_update_end() should always get 
called with irqs disabled, from what it looks like to me.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[kbuild test robot]

Hi Tarun,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on v4.17-rc5 next-20180514]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Tarun-Vyas/drm-i915-Wait-for-PSR-exit-before-checking-for-vblank-evasion/20180515-103355
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: i386-randconfig-x000-201819 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/gpu/drm/i915/intel_sprite.c: In function 'intel_pipe_update_start':
>> drivers/gpu/drm/i915/intel_sprite.c:117:3: error: implicit declaration of 
>> function 'psr_wait_for_idle'; did you mean 'apic_wait_icr_idle'? 
>> [-Werror=implicit-function-declaration]
  psr_wait_for_idle(dev_priv);
  ^
  apic_wait_icr_idle
   cc1: some warnings being treated as errors

vim +117 drivers/gpu/drm/i915/intel_sprite.c

76  
77  /**
78   * intel_pipe_update_start() - start update of a set of display 
registers
79   * @new_crtc_state: the new crtc state
80   *
81   * Mark the start of an update to pipe registers that should be updated
82   * atomically regarding vblank. If the next vblank will happens within
83   * the next 100 us, this function waits until the vblank passes.
84   *
85   * After a successful call to this function, interrupts will be disabled
86   * until a subsequent call to intel_pipe_update_end(). That is done to
87   * avoid random delays.
88   */
89  void intel_pipe_update_start(const struct intel_crtc_state 
*new_crtc_state)
90  {
91  struct intel_crtc *crtc = 
to_intel_crtc(new_crtc_state->base.crtc);
92  struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
93  const struct drm_display_mode *adjusted_mode = 
_crtc_state->base.adjusted_mode;
94  long timeout = msecs_to_jiffies_timeout(1);
95  int scanline, min, max, vblank_start;
96  wait_queue_head_t *wq = drm_crtc_vblank_waitqueue(>base);
97  bool need_vlv_dsi_wa = (IS_VALLEYVIEW(dev_priv) || 
IS_CHERRYVIEW(dev_priv)) &&
98  intel_crtc_has_type(new_crtc_state, INTEL_OUTPUT_DSI);
99  DEFINE_WAIT(wait);
   100  
   101  vblank_start = adjusted_mode->crtc_vblank_start;
   102  if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE)
   103  vblank_start = DIV_ROUND_UP(vblank_start, 2);
   104  
   105  /* FIXME needs to be calibrated sensibly */
   106  min = vblank_start - intel_usecs_to_scanlines(adjusted_mode,
   107
VBLANK_EVASION_TIME_US);
   108  max = vblank_start - 1;
   109  
   110  if (min <= 0 || max <= 0)
   111  return;
   112  
   113  if (WARN_ON(drm_crtc_vblank_get(>base)))
   114  return;
   115  
   116  if(new_crtc_state->has_psr && dev_priv->psr.active)
 > 117  psr_wait_for_idle(dev_priv);
   118  
   119  local_irq_disable();
   120  
   121  crtc->debug.min_vbl = min;
   122  crtc->debug.max_vbl = max;
   123  trace_i915_pipe_update_start(crtc);
   124  
   125  for (;;) {
   126  /*
   127   * prepare_to_wait() has a memory barrier, which 
guarantees
   128   * other CPUs can see the task state update by the time 
we
   129   * read the scanline.
   130   */
   131  prepare_to_wait(wq, , TASK_UNINTERRUPTIBLE);
   132  
   133  scanline = intel_get_crtc_scanline(crtc);
   134  if (scanline < min || scanline > max)
   135  break;
   136  
   137  if (!timeout) {
   138  DRM_ERROR("Potential atomic update failure on 
pipe %c\n",
   139pipe_name(crtc->pipe));
   140  break;
   141  }
   142  
   143  local_irq_enable();
   144  
   145  timeout = schedule_timeout(timeout);
   146  
   147  local_irq_disable();
   148  }
   149  
   150  finish_wait(wq, );
   151  
   152  drm_crtc_vblank_put(>base);
   153  
   154  /*
   155   * On VLV/CHV DSI the scanline counter would appear to
   156   * increment approx. 1/3 of a scanline before start of vblank.
   157   * The registers still get latched at start of vblank however.
   158   * This means we must not write any registers on the first
   159   * line of vblank (since not the whole 

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

[Chris Wilson]

Quoting Tarun Vyas (2018-05-14 21:49:22)
> 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 
> ---
>  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(>base)))
> return;
>  
> +   if(new_crtc_state->has_psr && dev_priv->psr.active)
> +   psr_wait_for_idle(dev_priv);
> +
> +   local_irq_disable();

Pop quiz, does intel_pipe_update_finish() unconditionally assume it is
called with irqs disabled?
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tarun Vyas]

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 
---
 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(>base)))
return;
 
+   if(new_crtc_state->has_psr && dev_priv->psr.active)
+   psr_wait_for_idle(dev_priv);
+
+   local_irq_disable();
+
crtc->debug.min_vbl = min;
crtc->debug.max_vbl = max;
trace_i915_pipe_update_start(crtc);
-- 
2.13.5

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx