[PATCH v2] Fix the possible watermark miswriting for skl+
Ville Syrjälä
ville.syrjala at linux.intel.com
Wed Nov 14 11:09:27 UTC 2018
On Wed, Nov 14, 2018 at 08:19:26AM +0000, Lisovskiy, Stanislav wrote:
> 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:
Probably due to skl_ddb_add_affected_pipes(), which looks safe enough
to me.
Not sure why it sets dirty_pipes though. Either the ddb allocations
change or they don't, so I don't really see why we need to mark
every pipe as dirty here. They will be marked naturally by
skl_ddb_add_affected_planes() anyway.
So looks to me we can just drop the dirty_pipes frobbing from
skl_ddb_add_affected_pipes().
>
> [ 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 at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> --
> Best Regards,
>
> Lisovskiy Stanislav
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel
More information about the dri-devel
mailing list