On Tue, 2018-11-13 at 16:40 +0000, Chris Wilson wrote:
> Quoting Stanislav Lisovskiy (2018-11-13 14:31:38)
> > Currently whenever we attempt to recalculate
> > watermarks, we assign dirty_pipes to zero,
> > then compare current wm results to the recalculated
> > one and if they changed we set correspondent dirty_pipes
> > bit again.
> > This can lead to situation, when we are clearing dirty_pipes,
> > same wm results twice in a row and not setting dirty_pipes
> > => so that watermarks are not actually updated, which then might
> > lead to fifo underruns, crc mismatch and other issues.
> >
> > Instead, whenever we detect that wm results are changed,
> > need to set correspondent dirty_pipes bit and clear it
> > only once the change is written, but not clear it everytime
> > we attempt to recalculate those in skl_compute_wm.
>
> Ok, but are not dirty_pipes being recomputed for each commit wrt to
> the
> current HW state. Should dirty_pipes not be 0 at the start of
> compute_wm
> naturally due to it not being used before in the atomic commit
> sequence?
> -Chris
We discussed this yesterday with Ville, as I understand the whole
drm_atomic_state is allocated each time the commit starts and it is
already naturally 0. So anyway assigning here dirty_pipes to 0 looked
redundant at least to me and Ville.
However the problem I mean was that if skl_compute_wm is invoked
twice and getting the same wm results, then we are not going to set
correspondent dirty_pipes bit and because it was zeroed in the
beginning of skl_compute_wm, we are not going to update the watermarks
at all, even though we should as it had changed.
During our discussion with Ville, we agreed that theoretically this
shouldn't happen because skl_compute_wm(called via
compute_global_watermarks hook) should be called only once per commit
in intel_atomic_check and then results are written, when crtc is
updated and this potentially can be caught using some WARN_ON.
What I did is I added a WARN_ON here in skl_compute_wm and removed
assigning dirty_pipes to zero(so that we can see that it might happen
that correspondent bit is already set, but we are getting here again):
@@ -5441,9 +5441,6 @@ skl_compute_wm(struct drm_atomic_state *state)
bool changed = false;
int ret, i;
- /* Clear all dirty flags */
- results->dirty_pipes = 0;
-
@@ -5471,6 +5471,8 @@ skl_compute_wm(struct drm_atomic_state *state)
ret = skl_update_pipe_wm(cstate, old_pipe_wm, pipe_wm,
&results->ddb, &changed);
if (ret)
return ret;
+ WARN_ON(!changed && (results->dirty_pipes &
drm_crtc_mask(crtc)));
+
if (changed)
results->dirty_pipes |= drm_crtc_mask(crtc);
If skl_compute_wm is called only once we should never get it, as
we would otherwise never have correpondent crtc bit already set here
(and we would already had zeroed dirty_pipes here as we do now, the
update would be lost).
However this WARN really catches the issue:
[ 35.681909] ------------[ cut here ]------------
[ 35.681917] WARN_ON(!changed && (results->dirty_pipes &
drm_crtc_mask(crtc)))
[ 35.681998] WARNING: CPU: 3 PID: 1499 at
drivers/gpu/drm/i915/intel_pm.c:5474 skl_compute_wm+0x709/0x800 [i915]
[ 35.682002] Modules linked in: cdc_ether usbnet snd_hda_codec_hdmi
snd_hda_codec_realtek snd_hda_codec_generic r8152 mii snd_hda_intel
snd_hda_codec x86_pkg_temp_thermal snd_hwdep coretemp snd_hda_core
snd_pcm pl2303 usbserial btusb btrtl btbcm btintel bluetooth
ecdh_generic mei_me mei dm_crypt crct10dif_pclmul crc32_pclmul
ghash_clmulni_intel i915 prime_numbers i2c_hid pinctrl_sunrisepoint
pinctrl_intel
[ 35.682044] CPU: 3 PID: 1499 Comm: Xorg Tainted:
G W 4.20.0-rc1-CI-CI_DRM_5076+ #123
[ 35.682047] Hardware name: LENOVO 81A8/LNVNB161216, BIOS 5SCN34WW
09/01/2017
[ 35.682093] RIP: 0010:skl_compute_wm+0x709/0x800 [i915]
[ 35.682097] Code: c1 e8 11 e9 40 fe ff ff d3 e0 41 85 86 98 04 00 00
0f 84 85 fe ff ff 48 c7 c6 10 48 1c a0 48 c7 c7 2c 13 1b a0 e8 07 49 03
e1 <0f> 0b 41 8b 86 98 04 00 00 e9 4f fe ff ff 44 89 ca e9 a5 f9 ff ff
[ 35.682101] RSP: 0018:ffffc90000c53a08 EFLAGS: 00010282
[ 35.682106] RAX: 0000000000000000 RBX: 00000000007784bf RCX:
0000000000000006
[ 35.682109] RDX: 0000000000000006 RSI: ffffffff8212886a RDI:
ffffffff820d6d57
[ 35.682112] RBP: ffff88029ef40000 R08: 000000001d5e3b8b R09:
0000000000000000
[ 35.682116] R10: ffff88029be2ea54 R11: 0000000000000000 R12:
ffff880299db6fc8
[ 35.682119] R13: ffff88029be2e678 R14: ffff88026000c138 R15:
0000000000000001
[ 35.682123] FS: 00007f3bbca90600(0000) GS:ffff8802b9d80000(0000)
knlGS:0000000000000000
[ 35.682126] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 35.682130] CR2: 00007f838a7b1158 CR3: 000000026cbe2003 CR4:
00000000003606e0
[ 35.682133] Call Trace:
[ 35.682148] ? __mutex_unlock_slowpath+0x46/0x2b0
[ 35.682215] intel_atomic_check+0x4d6/0x1230 [i915]
[ 35.682233] drm_atomic_check_only+0x477/0x710
[ 35.682245] drm_atomic_commit+0xe/0x50
[ 35.682253] drm_atomic_helper_set_config+0x7b/0x90
[ 35.682260] drm_mode_setcrtc+0x195/0x6b0
[ 35.682289] ? drm_mode_getcrtc+0x180/0x180
[ 35.682295] drm_ioctl_kernel+0x81/0xf0
[ 35.682305] drm_ioctl+0x2de/0x390
[ 35.682312] ? drm_mode_getcrtc+0x180/0x180
[ 35.682327] ? lock_acquire+0xa6/0x1c0
[ 35.682336] do_vfs_ioctl+0xa0/0x6e0
[ 35.682345] ? __fget+0xfc/0x1e0
[ 35.682353] ksys_ioctl+0x35/0x60
[ 35.682361] __x64_sys_ioctl+0x11/0x20
[ 35.682369] do_syscall_64+0x55/0x190
[ 35.682376] entry_SYSCALL_64_after_hwframe+0x49/0xbe
[ 35.682380] RIP: 0033:0x7f3bb9ea85d7
[ 35.682385] Code: b3 66 90 48 8b 05 b1 48 2d 00 64 c7 00 26 00 00 00
48 c7 c0 ff ff ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 b8 10 00 00 00 0f
05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 81 48 2d 00 f7 d8 64 89 01 48
[ 35.682388] RSP: 002b:00007fff41c6e8d8 EFLAGS: 00000246 ORIG_RAX:
0000000000000010
[ 35.682393] RAX: ffffffffffffffda RBX: 000055fca1377680 RCX:
00007f3bb9ea85d7
[ 35.682396] RDX: 00007fff41c6e9c0 RSI: 00000000c06864a2 RDI:
000000000000000d
[ 35.682400] RBP: 00007fff41c6e9c0 R08: 0000000000000001 R09:
00007f3bbc9f4000
[ 35.682403] R10: 0000000000000001 R11: 0000000000000246 R12:
00000000c06864a2
[ 35.682406] R13: 000000000000000d R14: 000000000000000a R15:
000055fca137fa50
[ 35.682424] irq event stamp: 847224
[ 35.682429] hardirqs last enabled at (847223): [<ffffffff810fba24>]
vprintk_emit+0x124/0x320
[ 35.682434] hardirqs last disabled at (847224): [<ffffffff810019a0>]
trace_hardirqs_off_thunk+0x1a/0x1c
[ 35.682438] softirqs last enabled at (847182): [<ffffffff81c0033a>]
__do_softirq+0x33a/0x4b9
[ 35.682444] softirqs last disabled at (847175): [<ffffffff8108def9>]
irq_exit+0xa9/0xc0
[ 35.682484] WARNING: CPU: 3 PID: 1499 at
drivers/gpu/drm/i915/intel_pm.c:5474 skl_compute_wm+0x709/0x800 [i915]
[ 35.682487] ---[ end trace 7b4397aa7cd7d405 ]---
Which means that we are losing some watermark updates, in case if we
assign dirty_pipes to zero and then not setting it back.
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Best Regards,
Lisovskiy Stanislav
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
