| Message ID | 20180622090129.3271-1-tarun.vyas@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.18-rc1 next-20180622]
[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/20180622-175933
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-randconfig-x011-201824 (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=x86_64
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:121:6: error: implicit declaration of function 'intel_psr_wait_for_idle'; did you mean 'i915_gem_wait_for_idle'? [-Werror=implicit-function-declaration]
if (intel_psr_wait_for_idle(dev_priv))
^~~~~~~~~~~~~~~~~~~~~~~
i915_gem_wait_for_idle
cc1: some warnings being treated as errors
vim +121 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 /*
117 * Wait for psr to idle out after enabling the VBL interrupts
118 * VBL interrupts will start the PSR exit and prevent a PSR
119 * re-entry as well.
120 */
> 121 if (intel_psr_wait_for_idle(dev_priv))
122 DRM_ERROR("PSR idle timed out, atomic update may fail\n");
123
124 local_irq_disable();
125
126 crtc->debug.min_vbl = min;
127 crtc->debug.max_vbl = max;
128 trace_i915_pipe_update_start(crtc);
129
130 for (;;) {
131 /*
132 * prepare_to_wait() has a memory barrier, which guarantees
133 * other CPUs can see the task state update by the time we
134 * read the scanline.
135 */
136 prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE);
137
138 scanline = intel_get_crtc_scanline(crtc);
139 if (scanline < min || scanline > max)
140 break;
141
142 if (!timeout) {
143 DRM_ERROR("Potential atomic update failure on pipe %c\n",
144 pipe_name(crtc->pipe));
145 break;
146 }
147
148 local_irq_enable();
149
150 timeout = schedule_timeout(timeout);
151
152 local_irq_disable();
153 }
154
155 finish_wait(wq, &wait);
156
157 drm_crtc_vblank_put(&crtc->base);
158
159 /*
160 * On VLV/CHV DSI the scanline counter would appear to
161 * increment approx. 1/3 of a scanline before start of vblank.
162 * The registers still get latched at start of vblank however.
163 * This means we must not write any registers on the first
164 * line of vblank (since not the whole line is actually in
165 * vblank). And unfortunately we can't use the interrupt to
166 * wait here since it will fire too soon. We could use the
167 * frame start interrupt instead since it will fire after the
168 * critical scanline, but that would require more changes
169 * in the interrupt code. So for now we'll just do the nasty
170 * thing and poll for the bad scanline to pass us by.
171 *
172 * FIXME figure out if BXT+ DSI suffers from this as well
173 */
174 while (need_vlv_dsi_wa && scanline == vblank_start)
175 scanline = intel_get_crtc_scanline(crtc);
176
177 crtc->debug.scanline_start = scanline;
178 crtc->debug.start_vbl_time = ktime_get();
179 crtc->debug.start_vbl_count = intel_crtc_get_vblank_counter(crtc);
180
181 trace_i915_pipe_update_vblank_evaded(crtc);
182 }
183
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Tarun,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on v4.18-rc1 next-20180622]
[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/20180622-175933
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-randconfig-x009-201824 (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=x86_64
All warnings (new ones prefixed by >>):
In file included from include/linux/kernel.h:10:0,
from include/linux/list.h:9,
from include/linux/agp_backend.h:33,
from include/drm/drmP.h:35,
from drivers/gpu//drm/i915/intel_sprite.c:32:
drivers/gpu//drm/i915/intel_sprite.c: In function 'intel_pipe_update_start':
drivers/gpu//drm/i915/intel_sprite.c:121:6: error: implicit declaration of function 'intel_psr_wait_for_idle'; did you mean 'i915_gem_wait_for_idle'? [-Werror=implicit-function-declaration]
if (intel_psr_wait_for_idle(dev_priv))
^
include/linux/compiler.h:58:30: note: in definition of macro '__trace_if'
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^~~~
>> drivers/gpu//drm/i915/intel_sprite.c:121:2: note: in expansion of macro 'if'
if (intel_psr_wait_for_idle(dev_priv))
^~
cc1: some warnings being treated as errors
vim +/if +121 drivers/gpu//drm/i915/intel_sprite.c
> 32 #include <drm/drmP.h>
33 #include <drm/drm_atomic_helper.h>
34 #include <drm/drm_crtc.h>
35 #include <drm/drm_fourcc.h>
36 #include <drm/drm_rect.h>
37 #include <drm/drm_atomic.h>
38 #include <drm/drm_plane_helper.h>
39 #include "intel_drv.h"
40 #include "intel_frontbuffer.h"
41 #include <drm/i915_drm.h>
42 #include "i915_drv.h"
43
44 bool intel_format_is_yuv(u32 format)
45 {
46 switch (format) {
47 case DRM_FORMAT_YUYV:
48 case DRM_FORMAT_UYVY:
49 case DRM_FORMAT_VYUY:
50 case DRM_FORMAT_YVYU:
51 case DRM_FORMAT_NV12:
52 return true;
53 default:
54 return false;
55 }
56 }
57
58 int intel_usecs_to_scanlines(const struct drm_display_mode *adjusted_mode,
59 int usecs)
60 {
61 /* paranoia */
62 if (!adjusted_mode->crtc_htotal)
63 return 1;
64
65 return DIV_ROUND_UP(usecs * adjusted_mode->crtc_clock,
66 1000 * adjusted_mode->crtc_htotal);
67 }
68
69 /* FIXME: We should instead only take spinlocks once for the entire update
70 * instead of once per mmio. */
71 #if IS_ENABLED(CONFIG_PROVE_LOCKING)
72 #define VBLANK_EVASION_TIME_US 250
73 #else
74 #define VBLANK_EVASION_TIME_US 100
75 #endif
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 /*
117 * Wait for psr to idle out after enabling the VBL interrupts
118 * VBL interrupts will start the PSR exit and prevent a PSR
119 * re-entry as well.
120 */
> 121 if (intel_psr_wait_for_idle(dev_priv))
122 DRM_ERROR("PSR idle timed out, atomic update may fail\n");
123
124 local_irq_disable();
125
126 crtc->debug.min_vbl = min;
127 crtc->debug.max_vbl = max;
128 trace_i915_pipe_update_start(crtc);
129
130 for (;;) {
131 /*
132 * prepare_to_wait() has a memory barrier, which guarantees
133 * other CPUs can see the task state update by the time we
134 * read the scanline.
135 */
136 prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE);
137
138 scanline = intel_get_crtc_scanline(crtc);
139 if (scanline < min || scanline > max)
140 break;
141
142 if (!timeout) {
143 DRM_ERROR("Potential atomic update failure on pipe %c\n",
144 pipe_name(crtc->pipe));
145 break;
146 }
147
148 local_irq_enable();
149
150 timeout = schedule_timeout(timeout);
151
152 local_irq_disable();
153 }
154
155 finish_wait(wq, &wait);
156
157 drm_crtc_vblank_put(&crtc->base);
158
159 /*
160 * On VLV/CHV DSI the scanline counter would appear to
161 * increment approx. 1/3 of a scanline before start of vblank.
162 * The registers still get latched at start of vblank however.
163 * This means we must not write any registers on the first
164 * line of vblank (since not the whole line is actually in
165 * vblank). And unfortunately we can't use the interrupt to
166 * wait here since it will fire too soon. We could use the
167 * frame start interrupt instead since it will fire after the
168 * critical scanline, but that would require more changes
169 * in the interrupt code. So for now we'll just do the nasty
170 * thing and poll for the bad scanline to pass us by.
171 *
172 * FIXME figure out if BXT+ DSI suffers from this as well
173 */
174 while (need_vlv_dsi_wa && scanline == vblank_start)
175 scanline = intel_get_crtc_scanline(crtc);
176
177 crtc->debug.scanline_start = scanline;
178 crtc->debug.start_vbl_time = ktime_get();
179 crtc->debug.start_vbl_count = intel_crtc_get_vblank_counter(crtc);
180
181 trace_i915_pipe_update_vblank_evaded(crtc);
182 }
183
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index 344c0e709b19..3a4ad6fed246 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -107,14 +107,22 @@ 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; + /* + * Wait for psr to idle out after enabling the VBL interrupts + * VBL interrupts will start the PSR exit and prevent a PSR + * re-entry as well. + */ + if (intel_psr_wait_for_idle(dev_priv)) + DRM_ERROR("PSR idle timed out, atomic update may fail\n"); + + 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 disabled, we incur a ~1-2 usec penalty) before reading the PIPEDSL, b/c if we haven't fully exited PSR, then checking for vblank evasion isn't actually applicable. v4: Comment explaining psr_wait after enabling VBL interrupts (DK) Signed-off-by: Tarun Vyas <tarun.vyas@intel.com> --- drivers/gpu/drm/i915/intel_sprite.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)