Hi! So, there was a pretty bad regression with modesetting on nouveau that got caught by one of my coworkers which could lead to screens connected to docks not coming up at all on laptops like the ThinkPad P50. Note there isn't a public bug open for this, but I'll be happy to open one if you would like.
Note as well that both of these patches are already upstream: "drm/nouveau: Only release VCPI slots on mode changes" is already in master, and "drm/nouveau: Only recalculate PBN/VCPI on mode/connector changes" is currently waiting to get pulled into the kernel from drm-fixes. So you guys shouldn't need to carry these patches around downstream for very long.
Lyude Paul (2): drm/nouveau: Only release VCPI slots on mode changes drm/nouveau: Only recalculate PBN/VCPI on mode/connector changes
drivers/gpu/drm/nouveau/dispnv50/disp.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-)
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 Signed-off-by: Dave Airlie airlied@redhat.com Link: https://patchwork.freedesktop.org/patch/msgid/20190801220216.15323-1-lyude@r... --- 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 4b1650f51955..847b7866137d 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c @@ -775,7 +775,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);
On Fri, Aug 16, 2019 at 4:25 PM 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 Signed-off-by: Dave Airlie airlied@redhat.com Link: https://patchwork.freedesktop.org/patch/msgid/20190801220216.15323-1-lyude@r...
drivers/gpu/drm/nouveau/dispnv50/disp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
This patch was included in 5.2.7 stable upstream.
Justin
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c index 4b1650f51955..847b7866137d 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c @@ -775,7 +775,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 _______________________________________________ kernel mailing list -- kernel@lists.fedoraproject.org To unsubscribe send an email to kernel-leave@lists.fedoraproject.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/kernel@lists.fedoraproject.org
I -thought- I had fixed this entirely, but it looks like that I didn't test this thoroughly enough as we apparently still make one big mistake with nv50_msto_atomic_check() - we don't handle the following scenario:
* CRTC #1 has n VCPI allocated to it, is attached to connector DP-4 which is attached to encoder #1. enabled=y active=n * CRTC #1 is changed from DP-4 to DP-5, causing: * DP-4 crtc=#1→NULL (VCPI n→0) * DP-5 crtc=NULL→#1 * CRTC #1 steals encoder #1 back from DP-4 and gives it to DP-5 * CRTC #1 maintains the same mode as before, just with a different connector * mode_changed=n connectors_changed=y (we _SHOULD_ do VCPI 0→n here, but don't)
Once the above scenario is repeated once, we'll attempt freeing VCPI from the connector that we didn't allocate due to the connectors changing, but the mode staying the same. Sigh.
Since nv50_msto_atomic_check() has broken a few times now, let's rethink things a bit to be more careful: limit both VCPI/PBN allocations to mode_changed || connectors_changed, since neither VCPI or PBN should ever need to change outside of routing and mode changes.
Changes since v1: * Fix accidental reversal of clock and bpp arguments in drm_dp_calc_pbn_mode() - William Lewis
Signed-off-by: Lyude Paul lyude@redhat.com Reported-by: Bohdan Milar bmilar@redhat.com Tested-by: Bohdan Milar bmilar@redhat.com Fixes: 232c9eec417a ("drm/nouveau: Use atomic VCPI helpers for MST") References: 412e85b60531 ("drm/nouveau: Only release VCPI slots on mode changes") 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: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Karol Herbst karolherbst@gmail.com Cc: Ilia Mirkin imirkin@alum.mit.edu Cc: stable@vger.kernel.org # v5.1+ Acked-by: Ben Skeggs bskeggs@redhat.com Signed-off-by: Dave Airlie airlied@redhat.com Link: https://patchwork.freedesktop.org/patch/msgid/20190809005307.18391-1-lyude@r... --- drivers/gpu/drm/nouveau/dispnv50/disp.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c index 847b7866137d..bdaf5ffd2504 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c @@ -766,16 +766,20 @@ nv50_msto_atomic_check(struct drm_encoder *encoder, struct nv50_head_atom *asyh = nv50_head_atom(crtc_state); int slots;
- /* When restoring duplicated states, we need to make sure that the - * bw remains the same and avoid recalculating it, as the connector's - * bpc may have changed after the state was duplicated - */ - if (!state->duplicated) - asyh->dp.pbn = - drm_dp_calc_pbn_mode(crtc_state->adjusted_mode.clock, - connector->display_info.bpc * 3); + if (crtc_state->mode_changed || crtc_state->connectors_changed) { + /* + * When restoring duplicated states, we need to make sure that + * the bw remains the same and avoid recalculating it, as the + * connector's bpc may have changed after the state was + * duplicated + */ + if (!state->duplicated) { + const int bpp = connector->display_info.bpc * 3; + const int clock = crtc_state->adjusted_mode.clock; + + asyh->dp.pbn = drm_dp_calc_pbn_mode(clock, bpp); + }
- if (crtc_state->mode_changed) { slots = drm_dp_atomic_find_vcpi_slots(state, &mstm->mgr, mstc->port, asyh->dp.pbn);
On Fri, Aug 16, 2019 at 4:25 PM Lyude Paul lyude@redhat.com wrote:
I -thought- I had fixed this entirely, but it looks like that I didn't test this thoroughly enough as we apparently still make one big mistake with nv50_msto_atomic_check() - we don't handle the following scenario:
- CRTC #1 has n VCPI allocated to it, is attached to connector DP-4 which is attached to encoder #1. enabled=y active=n
- CRTC #1 is changed from DP-4 to DP-5, causing:
- DP-4 crtc=#1→NULL (VCPI n→0)
- DP-5 crtc=NULL→#1
- CRTC #1 steals encoder #1 back from DP-4 and gives it to DP-5
- CRTC #1 maintains the same mode as before, just with a different connector
- mode_changed=n connectors_changed=y (we _SHOULD_ do VCPI 0→n here, but don't)
Once the above scenario is repeated once, we'll attempt freeing VCPI from the connector that we didn't allocate due to the connectors changing, but the mode staying the same. Sigh.
Since nv50_msto_atomic_check() has broken a few times now, let's rethink things a bit to be more careful: limit both VCPI/PBN allocations to mode_changed || connectors_changed, since neither VCPI or PBN should ever need to change outside of routing and mode changes.
Changes since v1:
- Fix accidental reversal of clock and bpp arguments in drm_dp_calc_pbn_mode() - William Lewis
Signed-off-by: Lyude Paul lyude@redhat.com Reported-by: Bohdan Milar bmilar@redhat.com Tested-by: Bohdan Milar bmilar@redhat.com Fixes: 232c9eec417a ("drm/nouveau: Use atomic VCPI helpers for MST") References: 412e85b60531 ("drm/nouveau: Only release VCPI slots on mode changes") 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: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Karol Herbst karolherbst@gmail.com Cc: Ilia Mirkin imirkin@alum.mit.edu Cc: stable@vger.kernel.org # v5.1+ Acked-by: Ben Skeggs bskeggs@redhat.com Signed-off-by: Dave Airlie airlied@redhat.com Link: https://patchwork.freedesktop.org/patch/msgid/20190809005307.18391-1-lyude@r...
drivers/gpu/drm/nouveau/dispnv50/disp.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-)
Applied, should make all 5.2.10 and newer builds.
Thanks, Justin
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c index 847b7866137d..bdaf5ffd2504 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c @@ -766,16 +766,20 @@ nv50_msto_atomic_check(struct drm_encoder *encoder, struct nv50_head_atom *asyh = nv50_head_atom(crtc_state); int slots;
/* When restoring duplicated states, we need to make sure that the
* bw remains the same and avoid recalculating it, as the connector's
* bpc may have changed after the state was duplicated
*/
if (!state->duplicated)
asyh->dp.pbn =
drm_dp_calc_pbn_mode(crtc_state->adjusted_mode.clock,
connector->display_info.bpc * 3);
if (crtc_state->mode_changed || crtc_state->connectors_changed) {
/*
* When restoring duplicated states, we need to make sure that
* the bw remains the same and avoid recalculating it, as the
* connector's bpc may have changed after the state was
* duplicated
*/
if (!state->duplicated) {
const int bpp = connector->display_info.bpc * 3;
const int clock = crtc_state->adjusted_mode.clock;
asyh->dp.pbn = drm_dp_calc_pbn_mode(clock, bpp);
}
if (crtc_state->mode_changed) { slots = drm_dp_atomic_find_vcpi_slots(state, &mstm->mgr, mstc->port, asyh->dp.pbn);
-- 2.21.0 _______________________________________________ kernel mailing list -- kernel@lists.fedoraproject.org To unsubscribe send an email to kernel-leave@lists.fedoraproject.org Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/kernel@lists.fedoraproject.org
kernel@lists.fedoraproject.org