| Message ID | 20180514204922.82073-3-tarun.vyas@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <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); > + > + local_irq_disable(); Pop quiz, does intel_pipe_update_finish() unconditionally assume it is called with irqs disabled? -Chris
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 = &new_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(&crtc->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(&crtc->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, &wait, 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",
139 pipe_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, &wait);
151
152 drm_crtc_vblank_put(&crtc->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 line is actually in
160 * vblank). And unfortunately we can't use the interrupt to
161 * wait here since it will fire too soon. We could use the
162 * frame start interrupt instead since it will fire after the
163 * critical scanline, but that would require more changes
164 * in the interrupt code. So for now we'll just do the nasty
165 * thing and poll for the bad scanline to pass us by.
166 *
167 * FIXME figure out if BXT+ DSI suffers from this as well
168 */
169 while (need_vlv_dsi_wa && scanline == vblank_start)
170 scanline = intel_get_crtc_scanline(crtc);
171
172 crtc->debug.scanline_start = scanline;
173 crtc->debug.start_vbl_time = ktime_get();
174 crtc->debug.start_vbl_count = intel_crtc_get_vblank_counter(crtc);
175
176 trace_i915_pipe_update_vblank_evaded(crtc);
177 }
178
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
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 <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); > > + > > + 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.
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);
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 <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_EVASI > > ON > > _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. > 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
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 <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_EVASI > > > ON > > > _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. > > 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.
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 <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_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(&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. > > > > 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.
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 <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_EVASI > > > > ON > > > > _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. > > > > 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.
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); + + local_irq_disable(); + crtc->debug.min_vbl = min; crtc->debug.max_vbl = max; trace_i915_pipe_update_start(crtc);
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(-)