| Message ID | 20190801220216.15323-1-lyude@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/nouveau: Only release VCPI slots on mode changes | expand |
On Fri, 2 Aug 2019 at 08:02, Lyude Paul <lyude@redhat.com> wrote: > > Looks like a regression got introduced into nv50_mstc_atomic_check() > that somehow didn't get found until now. If userspace changes > crtc_state->active to false but leaves the CRTC enabled, we end up > calling drm_dp_atomic_find_vcpi_slots() using the PBN calculated in > asyh->dp.pbn. However, if the display is inactive we end up calculating > a PBN of 0, which inadvertently causes us to have an allocation of 0. > From there, if userspace then disables the CRTC afterwards we end up > accidentally attempting to free the VCPI twice: > > WARNING: CPU: 0 PID: 1484 at drivers/gpu/drm/drm_dp_mst_topology.c:3336 > drm_dp_atomic_release_vcpi_slots+0x87/0xb0 [drm_kms_helper] > RIP: 0010:drm_dp_atomic_release_vcpi_slots+0x87/0xb0 [drm_kms_helper] > Call Trace: > drm_atomic_helper_check_modeset+0x3f3/0xa60 [drm_kms_helper] > ? drm_atomic_check_only+0x43/0x780 [drm] > drm_atomic_helper_check+0x15/0x90 [drm_kms_helper] > nv50_disp_atomic_check+0x83/0x1d0 [nouveau] > drm_atomic_check_only+0x54d/0x780 [drm] > ? drm_atomic_set_crtc_for_connector+0xec/0x100 [drm] > drm_atomic_commit+0x13/0x50 [drm] > drm_atomic_helper_set_config+0x81/0x90 [drm_kms_helper] > drm_mode_setcrtc+0x194/0x6a0 [drm] > ? vprintk_emit+0x16a/0x230 > ? drm_ioctl+0x163/0x390 [drm] > ? drm_mode_getcrtc+0x180/0x180 [drm] > drm_ioctl_kernel+0xaa/0xf0 [drm] > drm_ioctl+0x208/0x390 [drm] > ? drm_mode_getcrtc+0x180/0x180 [drm] > nouveau_drm_ioctl+0x63/0xb0 [nouveau] > do_vfs_ioctl+0x405/0x660 > ? recalc_sigpending+0x17/0x50 > ? _copy_from_user+0x37/0x60 > ksys_ioctl+0x5e/0x90 > ? exit_to_usermode_loop+0x92/0xe0 > __x64_sys_ioctl+0x16/0x20 > do_syscall_64+0x59/0x190 > entry_SYSCALL_64_after_hwframe+0x44/0xa9 > WARNING: CPU: 0 PID: 1484 at drivers/gpu/drm/drm_dp_mst_topology.c:3336 > drm_dp_atomic_release_vcpi_slots+0x87/0xb0 [drm_kms_helper] > ---[ end trace 4c395c0c51b1f88d ]--- > [drm:drm_dp_atomic_release_vcpi_slots [drm_kms_helper]] *ERROR* no VCPI for > [MST PORT:00000000e288eb7d] found in mst state 000000008e642070 > > So, fix this by doing what we probably should have done from the start: only > call drm_dp_atomic_find_vcpi_slots() when crtc_state->mode_changed is set, so > that VCPI allocations remain for as long as the CRTC is enabled. > > Signed-off-by: Lyude Paul <lyude@redhat.com> > Fixes: 232c9eec417a ("drm/nouveau: Use atomic VCPI helpers for MST") > Cc: Lyude Paul <lyude@redhat.com> > Cc: Ben Skeggs <bskeggs@redhat.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: David Airlie <airlied@redhat.com> > Cc: Jerry Zuo <Jerry.Zuo@amd.com> > Cc: Harry Wentland <harry.wentland@amd.com> > Cc: Juston Li <juston.li@intel.com> > Cc: Karol Herbst <karolherbst@gmail.com> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Ilia Mirkin <imirkin@alum.mit.edu> > Cc: <stable@vger.kernel.org> # v5.1+ Acked-by: Ben Skeggs <bskeggs@redhat.com> > --- > drivers/gpu/drm/nouveau/dispnv50/disp.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c > index 8497768f1b41..126703816794 100644 > --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c > +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c > @@ -780,7 +780,7 @@ nv50_msto_atomic_check(struct drm_encoder *encoder, > drm_dp_calc_pbn_mode(crtc_state->adjusted_mode.clock, > connector->display_info.bpc * 3); > > - if (drm_atomic_crtc_needs_modeset(crtc_state)) { > + if (crtc_state->mode_changed) { > slots = drm_dp_atomic_find_vcpi_slots(state, &mstm->mgr, > mstc->port, > asyh->dp.pbn); > -- > 2.21.0 > > _______________________________________________ > Nouveau mailing list > Nouveau@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/nouveau
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c index 8497768f1b41..126703816794 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c @@ -780,7 +780,7 @@ nv50_msto_atomic_check(struct drm_encoder *encoder, drm_dp_calc_pbn_mode(crtc_state->adjusted_mode.clock, connector->display_info.bpc * 3); - if (drm_atomic_crtc_needs_modeset(crtc_state)) { + if (crtc_state->mode_changed) { slots = drm_dp_atomic_find_vcpi_slots(state, &mstm->mgr, mstc->port, asyh->dp.pbn);
Looks like a regression got introduced into nv50_mstc_atomic_check() that somehow didn't get found until now. If userspace changes crtc_state->active to false but leaves the CRTC enabled, we end up calling drm_dp_atomic_find_vcpi_slots() using the PBN calculated in asyh->dp.pbn. However, if the display is inactive we end up calculating a PBN of 0, which inadvertently causes us to have an allocation of 0. From there, if userspace then disables the CRTC afterwards we end up accidentally attempting to free the VCPI twice: WARNING: CPU: 0 PID: 1484 at drivers/gpu/drm/drm_dp_mst_topology.c:3336 drm_dp_atomic_release_vcpi_slots+0x87/0xb0 [drm_kms_helper] RIP: 0010:drm_dp_atomic_release_vcpi_slots+0x87/0xb0 [drm_kms_helper] Call Trace: drm_atomic_helper_check_modeset+0x3f3/0xa60 [drm_kms_helper] ? drm_atomic_check_only+0x43/0x780 [drm] drm_atomic_helper_check+0x15/0x90 [drm_kms_helper] nv50_disp_atomic_check+0x83/0x1d0 [nouveau] drm_atomic_check_only+0x54d/0x780 [drm] ? drm_atomic_set_crtc_for_connector+0xec/0x100 [drm] drm_atomic_commit+0x13/0x50 [drm] drm_atomic_helper_set_config+0x81/0x90 [drm_kms_helper] drm_mode_setcrtc+0x194/0x6a0 [drm] ? vprintk_emit+0x16a/0x230 ? drm_ioctl+0x163/0x390 [drm] ? drm_mode_getcrtc+0x180/0x180 [drm] drm_ioctl_kernel+0xaa/0xf0 [drm] drm_ioctl+0x208/0x390 [drm] ? drm_mode_getcrtc+0x180/0x180 [drm] nouveau_drm_ioctl+0x63/0xb0 [nouveau] do_vfs_ioctl+0x405/0x660 ? recalc_sigpending+0x17/0x50 ? _copy_from_user+0x37/0x60 ksys_ioctl+0x5e/0x90 ? exit_to_usermode_loop+0x92/0xe0 __x64_sys_ioctl+0x16/0x20 do_syscall_64+0x59/0x190 entry_SYSCALL_64_after_hwframe+0x44/0xa9 WARNING: CPU: 0 PID: 1484 at drivers/gpu/drm/drm_dp_mst_topology.c:3336 drm_dp_atomic_release_vcpi_slots+0x87/0xb0 [drm_kms_helper] ---[ end trace 4c395c0c51b1f88d ]--- [drm:drm_dp_atomic_release_vcpi_slots [drm_kms_helper]] *ERROR* no VCPI for [MST PORT:00000000e288eb7d] found in mst state 000000008e642070 So, fix this by doing what we probably should have done from the start: only call drm_dp_atomic_find_vcpi_slots() when crtc_state->mode_changed is set, so that VCPI allocations remain for as long as the CRTC is enabled. Signed-off-by: Lyude Paul <lyude@redhat.com> Fixes: 232c9eec417a ("drm/nouveau: Use atomic VCPI helpers for MST") Cc: Lyude Paul <lyude@redhat.com> Cc: Ben Skeggs <bskeggs@redhat.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: David Airlie <airlied@redhat.com> Cc: Jerry Zuo <Jerry.Zuo@amd.com> Cc: Harry Wentland <harry.wentland@amd.com> Cc: Juston Li <juston.li@intel.com> Cc: Karol Herbst <karolherbst@gmail.com> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Cc: Ilia Mirkin <imirkin@alum.mit.edu> Cc: <stable@vger.kernel.org> # v5.1+ --- drivers/gpu/drm/nouveau/dispnv50/disp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)