This fixes the extra issues I discovered upstream after the introduction of my rework of the atomic VCPI helpers that occur during suspend/resume.
This time around, we use a slightly different but much less complicated approach for fixing said issues.
Cc: Daniel Vetter daniel@ffwll.ch
Lyude Paul (4): drm/dp_mst: Fix unbalanced malloc ref in drm_dp_mst_deallocate_vcpi() drm/dp_mst: Remove port validation in drm_dp_atomic_find_vcpi_slots() drm/atomic: Add drm_atomic_state->duplicated drm/nouveau: Move PBN and VCPI allocation into nv50_head_atom
drivers/gpu/drm/drm_atomic_helper.c | 10 ++++- drivers/gpu/drm/drm_dp_mst_topology.c | 51 +++++++++++++++++-------- drivers/gpu/drm/i915/intel_dp_mst.c | 17 +++------ drivers/gpu/drm/nouveau/dispnv50/atom.h | 6 +++ drivers/gpu/drm/nouveau/dispnv50/disp.c | 31 ++++++++------- drivers/gpu/drm/nouveau/dispnv50/head.c | 1 + include/drm/drm_atomic.h | 9 +++++ 7 files changed, 85 insertions(+), 40 deletions(-)
In drm_dp_mst_deallocate_vcpi(), we currently unconditionally call drm_dp_mst_put_port_malloc() on the port that's passed to us, even if we never successfully allocated VCPI to it. This is contrary to what we do in drm_dp_mst_allocate_vcpi(), where we only call drm_dp_mst_get_port_malloc() on the passed port if we successfully allocated VCPI to it.
As a result, if drm_dp_mst_allocate_vcpi() fails during a modeset and another successive modeset calls drm_dp_mst_deallocate_vcpi() we will end up dropping someone else's malloc reference to the port. Example:
[ 962.309260] ================================================================== [ 962.309290] BUG: KASAN: use-after-free in drm_dp_mst_put_port_malloc+0x72/0x180 [drm_kms_helper] [ 962.309296] Read of size 4 at addr ffff888416c30004 by task kworker/0:1H/500
[ 962.309308] CPU: 0 PID: 500 Comm: kworker/0:1H Tainted: G W O 5.0.0-rc2Lyude-Test+ #1 [ 962.309313] Hardware name: LENOVO 20L8S2N800/20L8S2N800, BIOS N22ET35W (1.12 ) 04/09/2018 [ 962.309428] Workqueue: events_highpri intel_atomic_cleanup_work [i915] [ 962.309434] Call Trace: [ 962.309452] dump_stack+0xad/0x150 [ 962.309462] ? dump_stack_print_info.cold.0+0x1b/0x1b [ 962.309472] ? kmsg_dump_rewind_nolock+0xd9/0xd9 [ 962.309504] ? drm_dp_mst_put_port_malloc+0x72/0x180 [drm_kms_helper] [ 962.309515] print_address_description+0x6c/0x23c [ 962.309542] ? drm_dp_mst_put_port_malloc+0x72/0x180 [drm_kms_helper] [ 962.309568] ? drm_dp_mst_put_port_malloc+0x72/0x180 [drm_kms_helper] [ 962.309577] kasan_report.cold.3+0x1a/0x32 [ 962.309605] ? drm_dp_mst_put_port_malloc+0x72/0x180 [drm_kms_helper] [ 962.309631] drm_dp_mst_put_port_malloc+0x72/0x180 [drm_kms_helper] [ 962.309658] ? drm_dp_mst_put_mstb_malloc+0x180/0x180 [drm_kms_helper] [ 962.309687] drm_dp_mst_destroy_state+0xcd/0x120 [drm_kms_helper] [ 962.309745] drm_atomic_state_default_clear+0x6ee/0xcc0 [drm] [ 962.309864] intel_atomic_state_clear+0xe/0x80 [i915] [ 962.309928] __drm_atomic_state_free+0x35/0xd0 [drm] [ 962.310044] intel_atomic_cleanup_work+0x56/0x70 [i915] [ 962.310057] process_one_work+0x884/0x1400 [ 962.310067] ? drain_workqueue+0x5a0/0x5a0 [ 962.310075] ? __schedule+0x87f/0x1e80 [ 962.310086] ? __sched_text_start+0x8/0x8 [ 962.310095] ? run_rebalance_domains+0x400/0x400 [ 962.310110] ? deref_stack_reg+0xb4/0x120 [ 962.310117] ? __read_once_size_nocheck.constprop.7+0x10/0x10 [ 962.310124] ? worker_enter_idle+0x47f/0x6a0 [ 962.310134] ? schedule+0xd7/0x2e0 [ 962.310141] ? __schedule+0x1e80/0x1e80 [ 962.310148] ? _raw_spin_lock_irq+0x9f/0x130 [ 962.310155] ? _raw_write_unlock_irqrestore+0x110/0x110 [ 962.310164] worker_thread+0x196/0x11e0 [ 962.310175] ? set_load_weight+0x2e0/0x2e0 [ 962.310181] ? __switch_to_asm+0x34/0x70 [ 962.310187] ? __switch_to_asm+0x40/0x70 [ 962.310194] ? process_one_work+0x1400/0x1400 [ 962.310199] ? __switch_to_asm+0x40/0x70 [ 962.310205] ? __switch_to_asm+0x34/0x70 [ 962.310211] ? __switch_to_asm+0x34/0x70 [ 962.310216] ? __switch_to_asm+0x40/0x70 [ 962.310221] ? __switch_to_asm+0x34/0x70 [ 962.310226] ? __switch_to_asm+0x40/0x70 [ 962.310231] ? __switch_to_asm+0x34/0x70 [ 962.310236] ? __switch_to_asm+0x40/0x70 [ 962.310242] ? syscall_return_via_sysret+0xf/0x7f [ 962.310248] ? __switch_to_asm+0x34/0x70 [ 962.310253] ? __switch_to_asm+0x40/0x70 [ 962.310258] ? __switch_to_asm+0x34/0x70 [ 962.310263] ? __switch_to_asm+0x40/0x70 [ 962.310268] ? __switch_to_asm+0x34/0x70 [ 962.310273] ? __switch_to_asm+0x40/0x70 [ 962.310281] ? __schedule+0x87f/0x1e80 [ 962.310292] ? __sched_text_start+0x8/0x8 [ 962.310300] ? save_stack+0x8c/0xb0 [ 962.310308] ? __kasan_kmalloc.constprop.6+0xc6/0xd0 [ 962.310313] ? kthread+0x98/0x3a0 [ 962.310318] ? ret_from_fork+0x35/0x40 [ 962.310334] ? __wake_up_common+0x178/0x6f0 [ 962.310343] ? _raw_spin_lock_irqsave+0xa4/0x140 [ 962.310349] ? __lock_text_start+0x8/0x8 [ 962.310355] ? _raw_write_lock_irqsave+0x70/0x130 [ 962.310360] ? __lock_text_start+0x8/0x8 [ 962.310371] ? process_one_work+0x1400/0x1400 [ 962.310376] kthread+0x2e2/0x3a0 [ 962.310383] ? kthread_create_on_node+0xc0/0xc0 [ 962.310389] ret_from_fork+0x35/0x40
[ 962.310401] Allocated by task 1462: [ 962.310410] __kasan_kmalloc.constprop.6+0xc6/0xd0 [ 962.310437] drm_dp_add_port+0xd60/0x1960 [drm_kms_helper] [ 962.310464] drm_dp_send_link_address+0x4b0/0x770 [drm_kms_helper] [ 962.310491] drm_dp_check_and_send_link_address+0x197/0x1f0 [drm_kms_helper] [ 962.310515] drm_dp_mst_link_probe_work+0x2b6/0x330 [drm_kms_helper] [ 962.310522] process_one_work+0x884/0x1400 [ 962.310529] worker_thread+0x196/0x11e0 [ 962.310533] kthread+0x2e2/0x3a0 [ 962.310538] ret_from_fork+0x35/0x40
[ 962.310543] Freed by task 500: [ 962.310550] __kasan_slab_free+0x133/0x180 [ 962.310555] kfree+0x92/0x1a0 [ 962.310581] drm_dp_mst_put_port_malloc+0x14d/0x180 [drm_kms_helper] [ 962.310693] intel_connector_destroy+0xb2/0xe0 [i915] [ 962.310747] drm_mode_object_put.part.0+0x12b/0x1a0 [drm] [ 962.310802] drm_atomic_state_default_clear+0x1f2/0xcc0 [drm] [ 962.310916] intel_atomic_state_clear+0xe/0x80 [i915] [ 962.310972] __drm_atomic_state_free+0x35/0xd0 [drm] [ 962.311083] intel_atomic_cleanup_work+0x56/0x70 [i915] [ 962.311092] process_one_work+0x884/0x1400 [ 962.311098] worker_thread+0x196/0x11e0 [ 962.311103] kthread+0x2e2/0x3a0 [ 962.311108] ret_from_fork+0x35/0x40
[ 962.311116] The buggy address belongs to the object at ffff888416c30000 which belongs to the cache kmalloc-2k of size 2048 [ 962.311122] The buggy address is located 4 bytes inside of 2048-byte region [ffff888416c30000, ffff888416c30800) [ 962.311124] The buggy address belongs to the page: [ 962.311132] page:ffffea00105b0c00 count:1 mapcount:0 mapping:ffff88841d003040 index:0x0 compound_mapcount: 0 [ 962.311142] flags: 0x8000000000010200(slab|head) [ 962.311152] raw: 8000000000010200 dead000000000100 dead000000000200 ffff88841d003040 [ 962.311159] raw: 0000000000000000 00000000000f000f 00000001ffffffff 0000000000000000 [ 962.311162] page dumped because: kasan: bad access detected
So, bail early if drm_dp_mst_deallocate_vcpi() is called on a port with no VCPI allocation. Additionally, clean up the surrounding kerneldoc while we're at it since the port is assumed to be kept around because the DRM driver is expected to hold a malloc reference to it, not just us.
Changes since v1: * Doc changes - danvet
Signed-off-by: Lyude Paul lyude@redhat.com Fixes: eceae1472467 ("drm/dp_mst: Start tracking per-port VCPI allocations") Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_dp_mst_topology.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index b1c63e9cdf8a..abb0ea8ba9d9 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -3311,15 +3311,16 @@ EXPORT_SYMBOL(drm_dp_mst_reset_vcpi_slots); /** * drm_dp_mst_deallocate_vcpi() - deallocate a VCPI * @mgr: manager for this port - * @port: unverified port to deallocate vcpi for + * @port: port to deallocate vcpi for + * + * This can be called unconditionally, regardless of whether + * drm_dp_mst_allocate_vcpi() succeeded or not. */ void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port) { - /* - * A port with VCPI will remain allocated until it's VCPI is - * released, no verified ref needed - */ + if (!port->vcpi.vcpi) + return;
drm_dp_mst_put_payload_id(mgr, port->vcpi.vcpi); port->vcpi.num_slots = 0;
Since we now have an easy way of refcounting drm_dp_mst_port structs and safely accessing their contents, there isn't any good reason to keep validating ports here. It doesn't prevent us from performing modesets on branch devices that have been removed either, and we already disallow enabling new displays on unregistered connectors in update_connector_routing() in drm_atomic_check_modeset(). All it does is cause us to have to make weird special exceptions in our atomic modesetting code. So, get rid of it entirely.
Signed-off-by: Lyude Paul lyude@redhat.com Fixes: eceae1472467 ("drm/dp_mst: Start tracking per-port VCPI allocations") Cc: Daniel Vetter daniel@ffwll.ch --- drivers/gpu/drm/drm_dp_mst_topology.c | 12 ++---------- drivers/gpu/drm/i915/intel_dp_mst.c | 17 ++++++----------- drivers/gpu/drm/nouveau/dispnv50/disp.c | 3 +-- 3 files changed, 9 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index abb0ea8ba9d9..4325e1518286 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -3117,10 +3117,6 @@ int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state, if (IS_ERR(topology_state)) return PTR_ERR(topology_state);
- port = drm_dp_mst_topology_get_port_validated(mgr, port); - if (port == NULL) - return -EINVAL; - /* Find the current allocation for this port, if any */ list_for_each_entry(pos, &topology_state->vcpis, next) { if (pos->port == port) { @@ -3153,10 +3149,8 @@ int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state, /* Add the new allocation to the state */ if (!vcpi) { vcpi = kzalloc(sizeof(*vcpi), GFP_KERNEL); - if (!vcpi) { - ret = -ENOMEM; - goto out; - } + if (!vcpi) + return -ENOMEM;
drm_dp_mst_get_port_malloc(port); vcpi->port = port; @@ -3165,8 +3159,6 @@ int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state, vcpi->vcpi = req_slots;
ret = req_slots; -out: - drm_dp_mst_topology_put_port(port); return ret; } EXPORT_SYMBOL(drm_dp_atomic_find_vcpi_slots); diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c index cdb83d294cdd..fb67cd931117 100644 --- a/drivers/gpu/drm/i915/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/intel_dp_mst.c @@ -80,17 +80,12 @@ static int intel_dp_mst_compute_config(struct intel_encoder *encoder, mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, bpp); pipe_config->pbn = mst_pbn;
- /* Zombie connectors can't have VCPI slots */ - if (!drm_connector_is_unregistered(connector)) { - slots = drm_dp_atomic_find_vcpi_slots(state, - &intel_dp->mst_mgr, - port, - mst_pbn); - if (slots < 0) { - DRM_DEBUG_KMS("failed finding vcpi slots:%d\n", - slots); - return slots; - } + slots = drm_dp_atomic_find_vcpi_slots(state, &intel_dp->mst_mgr, port, + mst_pbn); + if (slots < 0) { + DRM_DEBUG_KMS("failed finding vcpi slots:%d\n", + slots); + return slots; }
intel_link_compute_m_n(bpp, lane_count, diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c index 2e8a5fd9b262..60d858c2f2ce 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c @@ -771,8 +771,7 @@ nv50_msto_atomic_check(struct drm_encoder *encoder, mstc->pbn = drm_dp_calc_pbn_mode(crtc_state->adjusted_mode.clock, bpp);
- if (drm_atomic_crtc_needs_modeset(crtc_state) && - !drm_connector_is_unregistered(connector)) { + if (drm_atomic_crtc_needs_modeset(crtc_state)) { slots = drm_dp_atomic_find_vcpi_slots(state, &mstm->mgr, mstc->port, mstc->pbn); if (slots < 0)
On Thu, Jan 31, 2019 at 08:14:49PM -0500, Lyude Paul wrote:
Since we now have an easy way of refcounting drm_dp_mst_port structs and safely accessing their contents, there isn't any good reason to keep validating ports here. It doesn't prevent us from performing modesets on branch devices that have been removed either, and we already disallow enabling new displays on unregistered connectors in update_connector_routing() in drm_atomic_check_modeset(). All it does is cause us to have to make weird special exceptions in our atomic modesetting code. So, get rid of it entirely.
Signed-off-by: Lyude Paul lyude@redhat.com Fixes: eceae1472467 ("drm/dp_mst: Start tracking per-port VCPI allocations") Cc: Daniel Vetter daniel@ffwll.ch
drivers/gpu/drm/drm_dp_mst_topology.c | 12 ++---------- drivers/gpu/drm/i915/intel_dp_mst.c | 17 ++++++----------- drivers/gpu/drm/nouveau/dispnv50/disp.c | 3 +-- 3 files changed, 9 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index abb0ea8ba9d9..4325e1518286 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -3117,10 +3117,6 @@ int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state, if (IS_ERR(topology_state)) return PTR_ERR(topology_state);
- port = drm_dp_mst_topology_get_port_validated(mgr, port);
- if (port == NULL)
return -EINVAL;- /* Find the current allocation for this port, if any */ list_for_each_entry(pos, &topology_state->vcpis, next) { if (pos->port == port) {
Also noticed that the WARN_ON() return -EINVAL; here gets fixed with this patch.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
@@ -3153,10 +3149,8 @@ int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state, /* Add the new allocation to the state */ if (!vcpi) { vcpi = kzalloc(sizeof(*vcpi), GFP_KERNEL);
if (!vcpi) {ret = -ENOMEM;goto out;}
if (!vcpi)return -ENOMEM;drm_dp_mst_get_port_malloc(port); vcpi->port = port;
@@ -3165,8 +3159,6 @@ int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state, vcpi->vcpi = req_slots;
ret = req_slots; -out:
- drm_dp_mst_topology_put_port(port); return ret;
} EXPORT_SYMBOL(drm_dp_atomic_find_vcpi_slots); diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c index cdb83d294cdd..fb67cd931117 100644 --- a/drivers/gpu/drm/i915/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/intel_dp_mst.c @@ -80,17 +80,12 @@ static int intel_dp_mst_compute_config(struct intel_encoder *encoder, mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, bpp); pipe_config->pbn = mst_pbn;
- /* Zombie connectors can't have VCPI slots */
- if (!drm_connector_is_unregistered(connector)) {
slots = drm_dp_atomic_find_vcpi_slots(state,&intel_dp->mst_mgr,port,mst_pbn);if (slots < 0) {DRM_DEBUG_KMS("failed finding vcpi slots:%d\n",slots);return slots;}
slots = drm_dp_atomic_find_vcpi_slots(state, &intel_dp->mst_mgr, port,
mst_pbn);if (slots < 0) {
DRM_DEBUG_KMS("failed finding vcpi slots:%d\n",slots);return slots;}
intel_link_compute_m_n(bpp, lane_count,
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c index 2e8a5fd9b262..60d858c2f2ce 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c @@ -771,8 +771,7 @@ nv50_msto_atomic_check(struct drm_encoder *encoder, mstc->pbn = drm_dp_calc_pbn_mode(crtc_state->adjusted_mode.clock, bpp);
- if (drm_atomic_crtc_needs_modeset(crtc_state) &&
!drm_connector_is_unregistered(connector)) {
- if (drm_atomic_crtc_needs_modeset(crtc_state)) { slots = drm_dp_atomic_find_vcpi_slots(state, &mstm->mgr, mstc->port, mstc->pbn); if (slots < 0)
-- 2.20.1
Since
commit 39b50c603878 ("drm/atomic_helper: Stop modesets on unregistered connectors harder")
We've been failing atomic checks if they try to enable new displays on unregistered connectors. This is fine except for the one situation that breaks atomic assumptions: suspend/resume. If a connector is unregistered before we attempt to restore the atomic state, something we end up failing the atomic check that happens when trying to restore the state during resume.
Normally this would be OK: we try our best to make sure that the atomic state pre-suspend can be restored post-suspend, but failures at that point usually don't cause problems. That is of course, until we introduced the new atomic MST VCPI helpers:
[drm:drm_atomic_helper_check_modeset [drm_kms_helper]] [CRTC:65:pipe B] active changed [drm:drm_atomic_helper_check_modeset [drm_kms_helper]] Updating routing for [CONNECTOR:123:DP-5] [drm:drm_atomic_helper_check_modeset [drm_kms_helper]] Disabling [CONNECTOR:123:DP-5] [drm:drm_atomic_get_private_obj_state [drm]] Added new private object 0000000025844636 state 000000009fd2899a to 000000003a13d7b8 WARNING: CPU: 6 PID: 1070 at drivers/gpu/drm/drm_dp_mst_topology.c:3153 drm_dp_atomic_release_vcpi_slots+0xb9/0x200 [drm_kms_helper] Modules linked in: fuse vfat fat snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic joydev iTCO_wdt i915(O) wmi_bmof intel_rapl btusb btrtl x86_pkg_temp_thermal btbcm btintel coretemp i2c_algo_bit drm_kms_helper(O) crc32_pclmul snd_hda_intel syscopyarea sysfillrect snd_hda_codec sysimgblt snd_hda_core bluetooth fb_sys_fops snd_pcm pcspkr drm(O) psmouse snd_timer mei_me ecdh_generic i2c_i801 mei i2c_core ucsi_acpi typec_ucsi typec wmi thinkpad_acpi ledtrig_audio snd soundcore tpm_tis rfkill tpm_tis_core video tpm acpi_pad pcc_cpufreq uas usb_storage crc32c_intel nvme serio_raw xhci_pci nvme_core xhci_hcd CPU: 6 PID: 1070 Comm: gnome-shell Tainted: G W O 5.0.0-rc2Lyude-Test+ #1 Hardware name: LENOVO 20L8S2N800/20L8S2N800, BIOS N22ET35W (1.12 ) 04/09/2018 RIP: 0010:drm_dp_atomic_release_vcpi_slots+0xb9/0x200 [drm_kms_helper] Code: 00 4c 39 6d f0 74 49 48 8d 7b 10 48 89 f9 48 c1 e9 03 42 80 3c 21 00 0f 85 d2 00 00 00 48 8b 6b 10 48 8d 5d f0 49 39 ee 75 c5 <0f> 0b 48 c7 c7 c0 78 b3 a0 48 89 c2 4c 89 ee e8 03 6c aa ff b8 ea RSP: 0018:ffff88841235f268 EFLAGS: 00010246 RAX: ffff88841bf12ab0 RBX: ffff88841bf12aa8 RCX: 1ffff110837e2557 RDX: dffffc0000000000 RSI: 0000000000000000 RDI: ffffed108246bde0 RBP: ffff88841bf12ab8 R08: ffffed1083db3c93 R09: ffffed1083db3c92 R10: ffffed1083db3c92 R11: ffff88841ed9e497 R12: ffff888419555d80 R13: ffff8883bc499100 R14: ffff88841bf12ab8 R15: 0000000000000000 FS: 00007f16fbd4cd00(0000) GS:ffff88841ed80000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00007f1687c9f000 CR3: 00000003ba3cc003 CR4: 00000000003606e0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call Trace: drm_atomic_helper_check_modeset+0xf21/0x2f50 [drm_kms_helper] ? drm_atomic_helper_commit_modeset_enables+0xa90/0xa90 [drm_kms_helper] ? __printk_safe_exit+0x10/0x10 ? save_stack+0x8c/0xb0 ? vprintk_func+0x96/0x1bf ? __printk_safe_exit+0x10/0x10 intel_atomic_check+0x234/0x4750 [i915] ? printk+0x9f/0xc5 ? kmsg_dump_rewind_nolock+0xd9/0xd9 ? _raw_spin_lock_irqsave+0xa4/0x140 ? drm_atomic_check_only+0xb1/0x28b0 [drm] ? drm_dbg+0x186/0x1b0 [drm] ? drm_dev_dbg+0x200/0x200 [drm] ? intel_link_compute_m_n+0xb0/0xb0 [i915] ? drm_mode_put_tile_group+0x20/0x20 [drm] ? skl_plane_format_mod_supported+0x17f/0x1b0 [i915] ? drm_plane_check_pixel_format+0x14a/0x310 [drm] drm_atomic_check_only+0x13c4/0x28b0 [drm] ? drm_state_info+0x220/0x220 [drm] ? drm_atomic_helper_disable_plane+0x1d0/0x1d0 [drm_kms_helper] ? pick_single_encoder_for_connector+0xe0/0xe0 [drm_kms_helper] ? kasan_unpoison_shadow+0x35/0x40 drm_atomic_commit+0x3b/0x100 [drm] drm_atomic_helper_set_config+0xd5/0x100 [drm_kms_helper] drm_mode_setcrtc+0x636/0x1660 [drm] ? vprintk_func+0x96/0x1bf ? drm_dev_dbg+0x200/0x200 [drm] ? drm_mode_getcrtc+0x790/0x790 [drm] ? printk+0x9f/0xc5 ? mutex_unlock+0x1d/0x40 ? drm_mode_addfb2+0x2e9/0x3a0 [drm] ? rcu_sync_dtor+0x2e0/0x2e0 ? drm_dbg+0x186/0x1b0 [drm] ? set_page_dirty+0x271/0x4d0 drm_ioctl_kernel+0x203/0x290 [drm] ? drm_mode_getcrtc+0x790/0x790 [drm] ? drm_setversion+0x7f0/0x7f0 [drm] ? __switch_to_asm+0x34/0x70 ? __switch_to_asm+0x34/0x70 drm_ioctl+0x445/0x950 [drm] ? drm_mode_getcrtc+0x790/0x790 [drm] ? drm_getunique+0x220/0x220 [drm] ? expand_files.part.10+0x920/0x920 do_vfs_ioctl+0x1a1/0x13d0 ? ioctl_preallocate+0x2b0/0x2b0 ? __fget_light+0x2d6/0x390 ? schedule+0xd7/0x2e0 ? fget_raw+0x10/0x10 ? apic_timer_interrupt+0xa/0x20 ? apic_timer_interrupt+0xa/0x20 ? rcu_cleanup_dead_rnp+0x2c0/0x2c0 ksys_ioctl+0x60/0x90 __x64_sys_ioctl+0x6f/0xb0 do_syscall_64+0x136/0x440 ? syscall_return_slowpath+0x2d0/0x2d0 ? do_page_fault+0x89/0x330 ? __do_page_fault+0x9c0/0x9c0 ? prepare_exit_to_usermode+0x188/0x200 ? perf_trace_sys_enter+0x1090/0x1090 ? __x64_sys_sigaltstack+0x280/0x280 ? __put_user_4+0x1c/0x30 entry_SYSCALL_64_after_hwframe+0x44/0xa9 RIP: 0033:0x7f16ff89a09b Code: 0f 1e fa 48 8b 05 ed bd 0c 00 64 c7 00 26 00 00 00 48 c7 c0 ff ff ff ff c3 66 0f 1f 44 00 00 f3 0f 1e fa b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d bd bd 0c 00 f7 d8 64 89 01 48 RSP: 002b:00007fff001232b8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010 RAX: ffffffffffffffda RBX: 00007fff001232f0 RCX: 00007f16ff89a09b RDX: 00007fff001232f0 RSI: 00000000c06864a2 RDI: 000000000000000b RBP: 00007fff001232f0 R08: 0000000000000000 R09: 000055a79d484460 R10: 000055a79d44e770 R11: 0000000000000246 R12: 00000000c06864a2 R13: 000000000000000b R14: 0000000000000000 R15: 000055a79d44e770 WARNING: CPU: 6 PID: 1070 at drivers/gpu/drm/drm_dp_mst_topology.c:3153 drm_dp_atomic_release_vcpi_slots+0xb9/0x200 [drm_kms_helper] ---[ end trace d536c05c13c83be2 ]--- [drm:drm_dp_atomic_release_vcpi_slots [drm_kms_helper]] *ERROR* no VCPI for [MST PORT:00000000f9e2b143] found in mst state 000000009fd2899a
This appears to be happening because we destroy the VCPI allocations when disabling all connected displays while suspending, and those VCPI allocations don't get restored on resume due to failing to restore the atomic state.
So, fix this by introducing the suspending option to drm_atomic_helper_duplicate_state() and use that to indicate in the atomic state that it's being used for suspending or resuming the system, and thus needs to be fixed up by the driver. We can then use the new state->duplicated hook to tell update_connector_routing() in drm_atomic_check_modeset() to allow for modesets on unregistered connectors, which allows us to restore atomic states that contain MST topologies that were removed after the state was duplicated and thus: mostly fixing suspend and resume. This just leaves some issues that were introduced with nouveau, that will be addressed next.
Changes since v2: * Remove the changes in this patch to the VCPI helpers, they aren't needed anymore Changes since v1: * Rename suspend_or_resume to duplicated
Signed-off-by: Lyude Paul lyude@redhat.com Fixes: eceae1472467 ("drm/dp_mst: Start tracking per-port VCPI allocations") Cc: Daniel Vetter daniel@ffwll.ch --- drivers/gpu/drm/drm_atomic_helper.c | 10 +++++++++- drivers/gpu/drm/drm_dp_mst_topology.c | 28 +++++++++++++++++++++++++++ include/drm/drm_atomic.h | 9 +++++++++ 3 files changed, 46 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 6fe2303fccd9..f578bf1fe164 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -332,8 +332,15 @@ update_connector_routing(struct drm_atomic_state *state, * about is ensuring that userspace can't do anything but shut off the * display on a connector that was destroyed after its been notified, * not before. + * + * Additionally, we also want to ignore connector registration when + * we're trying to restore an atomic state during system resume since + * there's a chance the connector may have been destroyed during the + * process, but it's better to ignore that then cause + * drm_atomic_helper_resume() to fail. */ - if (drm_connector_is_unregistered(connector) && crtc_state->active) { + if (!state->duplicated && drm_connector_is_unregistered(connector) && + crtc_state->active) { DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] is not registered\n", connector->base.id, connector->name); return -EINVAL; @@ -3180,6 +3187,7 @@ drm_atomic_helper_duplicate_state(struct drm_device *dev, return ERR_PTR(-ENOMEM);
state->acquire_ctx = ctx; + state->duplicated = true;
drm_for_each_crtc(crtc, dev) { struct drm_crtc_state *crtc_state; diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 4325e1518286..ea1540ea67af 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -3097,6 +3097,10 @@ static int drm_dp_init_vcpi(struct drm_dp_mst_topology_mgr *mgr, * @port as needed. It is not OK however, to call this function and * drm_dp_atomic_release_vcpi_slots() in the same atomic check phase. * + * When &drm_atomic_state.duplicated is set to %true%, this function will not + * perform any error checking and will instead simply return the previously + * recorded VCPI allocations. + * * See also: * drm_dp_atomic_release_vcpi_slots() * drm_dp_mst_atomic_check() @@ -3123,6 +3127,19 @@ int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state, vcpi = pos; prev_slots = vcpi->vcpi;
+ /* + * When resuming, we just want to restore the previous + * VCPI without doing error checking + */ + if (state->duplicated) { + DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] [MST PORT:%p] restoring VCPI of %d\n", + port->connector->base.id, + port->connector->name, + port, prev_slots); + + return prev_slots; + } + /* * This should never happen, unless the driver tries * releasing and allocating the same VCPI allocation, @@ -3181,6 +3198,11 @@ EXPORT_SYMBOL(drm_dp_atomic_find_vcpi_slots); * drm_dp_atomic_find_vcpi_slots() on the same @port in a single atomic check * phase. * + * When &drm_atomic_state.duplicated is set, this function will not + * modify the VCPI allocations in &drm_dp_mst_topology_state.vcpis, so that + * those VCPI allocations may be restored as-is from the duplicated state. In + * this scenario, this function will always return 0. + * * See also: * drm_dp_atomic_find_vcpi_slots() * drm_dp_mst_atomic_check() @@ -3197,6 +3219,12 @@ int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state, struct drm_dp_vcpi_allocation *pos; bool found = false;
+ /* During suspend, just keep our VCPI allocations as-is in the atomic + * state so they can be restored as-is at resume + */ + if (state->duplicated) + return 0; + topology_state = drm_atomic_get_mst_topology_state(state, mgr); if (IS_ERR(topology_state)) return PTR_ERR(topology_state); diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 811b4a92568f..961c792fa98b 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -329,6 +329,15 @@ struct drm_atomic_state { bool allow_modeset : 1; bool legacy_cursor_update : 1; bool async_update : 1; + /** + * @duplicated: + * + * Indicates whether or not this atomic state was duplicated using + * drm_atomic_helper_duplicate_state(). Drivers and atomic helpers + * should use this to fixup normal inconsistencies in duplicated + * states. + */ + bool duplicated : 1; struct __drm_planes_state *planes; struct __drm_crtcs_state *crtcs; int num_connector;
On Thu, Jan 31, 2019 at 08:14:50PM -0500, Lyude Paul wrote:
Since
commit 39b50c603878 ("drm/atomic_helper: Stop modesets on unregistered connectors harder")
We've been failing atomic checks if they try to enable new displays on unregistered connectors. This is fine except for the one situation that breaks atomic assumptions: suspend/resume. If a connector is unregistered before we attempt to restore the atomic state, something we end up failing the atomic check that happens when trying to restore the state during resume.
Normally this would be OK: we try our best to make sure that the atomic state pre-suspend can be restored post-suspend, but failures at that point usually don't cause problems. That is of course, until we introduced the new atomic MST VCPI helpers:
[drm:drm_atomic_helper_check_modeset [drm_kms_helper]] [CRTC:65:pipe B] active changed [drm:drm_atomic_helper_check_modeset [drm_kms_helper]] Updating routing for [CONNECTOR:123:DP-5] [drm:drm_atomic_helper_check_modeset [drm_kms_helper]] Disabling [CONNECTOR:123:DP-5] [drm:drm_atomic_get_private_obj_state [drm]] Added new private object 0000000025844636 state 000000009fd2899a to 000000003a13d7b8 WARNING: CPU: 6 PID: 1070 at drivers/gpu/drm/drm_dp_mst_topology.c:3153 drm_dp_atomic_release_vcpi_slots+0xb9/0x200 [drm_kms_helper] Modules linked in: fuse vfat fat snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic joydev iTCO_wdt i915(O) wmi_bmof intel_rapl btusb btrtl x86_pkg_temp_thermal btbcm btintel coretemp i2c_algo_bit drm_kms_helper(O) crc32_pclmul snd_hda_intel syscopyarea sysfillrect snd_hda_codec sysimgblt snd_hda_core bluetooth fb_sys_fops snd_pcm pcspkr drm(O) psmouse snd_timer mei_me ecdh_generic i2c_i801 mei i2c_core ucsi_acpi typec_ucsi typec wmi thinkpad_acpi ledtrig_audio snd soundcore tpm_tis rfkill tpm_tis_core video tpm acpi_pad pcc_cpufreq uas usb_storage crc32c_intel nvme serio_raw xhci_pci nvme_core xhci_hcd CPU: 6 PID: 1070 Comm: gnome-shell Tainted: G W O 5.0.0-rc2Lyude-Test+ #1 Hardware name: LENOVO 20L8S2N800/20L8S2N800, BIOS N22ET35W (1.12 ) 04/09/2018 RIP: 0010:drm_dp_atomic_release_vcpi_slots+0xb9/0x200 [drm_kms_helper] Code: 00 4c 39 6d f0 74 49 48 8d 7b 10 48 89 f9 48 c1 e9 03 42 80 3c 21 00 0f 85 d2 00 00 00 48 8b 6b 10 48 8d 5d f0 49 39 ee 75 c5 <0f> 0b 48 c7 c7 c0 78 b3 a0 48 89 c2 4c 89 ee e8 03 6c aa ff b8 ea RSP: 0018:ffff88841235f268 EFLAGS: 00010246 RAX: ffff88841bf12ab0 RBX: ffff88841bf12aa8 RCX: 1ffff110837e2557 RDX: dffffc0000000000 RSI: 0000000000000000 RDI: ffffed108246bde0 RBP: ffff88841bf12ab8 R08: ffffed1083db3c93 R09: ffffed1083db3c92 R10: ffffed1083db3c92 R11: ffff88841ed9e497 R12: ffff888419555d80 R13: ffff8883bc499100 R14: ffff88841bf12ab8 R15: 0000000000000000 FS: 00007f16fbd4cd00(0000) GS:ffff88841ed80000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00007f1687c9f000 CR3: 00000003ba3cc003 CR4: 00000000003606e0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call Trace: drm_atomic_helper_check_modeset+0xf21/0x2f50 [drm_kms_helper] ? drm_atomic_helper_commit_modeset_enables+0xa90/0xa90 [drm_kms_helper] ? __printk_safe_exit+0x10/0x10 ? save_stack+0x8c/0xb0 ? vprintk_func+0x96/0x1bf ? __printk_safe_exit+0x10/0x10 intel_atomic_check+0x234/0x4750 [i915] ? printk+0x9f/0xc5 ? kmsg_dump_rewind_nolock+0xd9/0xd9 ? _raw_spin_lock_irqsave+0xa4/0x140 ? drm_atomic_check_only+0xb1/0x28b0 [drm] ? drm_dbg+0x186/0x1b0 [drm] ? drm_dev_dbg+0x200/0x200 [drm] ? intel_link_compute_m_n+0xb0/0xb0 [i915] ? drm_mode_put_tile_group+0x20/0x20 [drm] ? skl_plane_format_mod_supported+0x17f/0x1b0 [i915] ? drm_plane_check_pixel_format+0x14a/0x310 [drm] drm_atomic_check_only+0x13c4/0x28b0 [drm] ? drm_state_info+0x220/0x220 [drm] ? drm_atomic_helper_disable_plane+0x1d0/0x1d0 [drm_kms_helper] ? pick_single_encoder_for_connector+0xe0/0xe0 [drm_kms_helper] ? kasan_unpoison_shadow+0x35/0x40 drm_atomic_commit+0x3b/0x100 [drm] drm_atomic_helper_set_config+0xd5/0x100 [drm_kms_helper] drm_mode_setcrtc+0x636/0x1660 [drm] ? vprintk_func+0x96/0x1bf ? drm_dev_dbg+0x200/0x200 [drm] ? drm_mode_getcrtc+0x790/0x790 [drm] ? printk+0x9f/0xc5 ? mutex_unlock+0x1d/0x40 ? drm_mode_addfb2+0x2e9/0x3a0 [drm] ? rcu_sync_dtor+0x2e0/0x2e0 ? drm_dbg+0x186/0x1b0 [drm] ? set_page_dirty+0x271/0x4d0 drm_ioctl_kernel+0x203/0x290 [drm] ? drm_mode_getcrtc+0x790/0x790 [drm] ? drm_setversion+0x7f0/0x7f0 [drm] ? __switch_to_asm+0x34/0x70 ? __switch_to_asm+0x34/0x70 drm_ioctl+0x445/0x950 [drm] ? drm_mode_getcrtc+0x790/0x790 [drm] ? drm_getunique+0x220/0x220 [drm] ? expand_files.part.10+0x920/0x920 do_vfs_ioctl+0x1a1/0x13d0 ? ioctl_preallocate+0x2b0/0x2b0 ? __fget_light+0x2d6/0x390 ? schedule+0xd7/0x2e0 ? fget_raw+0x10/0x10 ? apic_timer_interrupt+0xa/0x20 ? apic_timer_interrupt+0xa/0x20 ? rcu_cleanup_dead_rnp+0x2c0/0x2c0 ksys_ioctl+0x60/0x90 __x64_sys_ioctl+0x6f/0xb0 do_syscall_64+0x136/0x440 ? syscall_return_slowpath+0x2d0/0x2d0 ? do_page_fault+0x89/0x330 ? __do_page_fault+0x9c0/0x9c0 ? prepare_exit_to_usermode+0x188/0x200 ? perf_trace_sys_enter+0x1090/0x1090 ? __x64_sys_sigaltstack+0x280/0x280 ? __put_user_4+0x1c/0x30 entry_SYSCALL_64_after_hwframe+0x44/0xa9 RIP: 0033:0x7f16ff89a09b Code: 0f 1e fa 48 8b 05 ed bd 0c 00 64 c7 00 26 00 00 00 48 c7 c0 ff ff ff ff c3 66 0f 1f 44 00 00 f3 0f 1e fa b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d bd bd 0c 00 f7 d8 64 89 01 48 RSP: 002b:00007fff001232b8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010 RAX: ffffffffffffffda RBX: 00007fff001232f0 RCX: 00007f16ff89a09b RDX: 00007fff001232f0 RSI: 00000000c06864a2 RDI: 000000000000000b RBP: 00007fff001232f0 R08: 0000000000000000 R09: 000055a79d484460 R10: 000055a79d44e770 R11: 0000000000000246 R12: 00000000c06864a2 R13: 000000000000000b R14: 0000000000000000 R15: 000055a79d44e770 WARNING: CPU: 6 PID: 1070 at drivers/gpu/drm/drm_dp_mst_topology.c:3153 drm_dp_atomic_release_vcpi_slots+0xb9/0x200 [drm_kms_helper] ---[ end trace d536c05c13c83be2 ]--- [drm:drm_dp_atomic_release_vcpi_slots [drm_kms_helper]] *ERROR* no VCPI for [MST PORT:00000000f9e2b143] found in mst state 000000009fd2899a
This appears to be happening because we destroy the VCPI allocations when disabling all connected displays while suspending, and those VCPI allocations don't get restored on resume due to failing to restore the atomic state.
So, fix this by introducing the suspending option to drm_atomic_helper_duplicate_state() and use that to indicate in the atomic state that it's being used for suspending or resuming the system, and thus needs to be fixed up by the driver. We can then use the new state->duplicated hook to tell update_connector_routing() in drm_atomic_check_modeset() to allow for modesets on unregistered connectors, which allows us to restore atomic states that contain MST topologies that were removed after the state was duplicated and thus: mostly fixing suspend and resume. This just leaves some issues that were introduced with nouveau, that will be addressed next.
Changes since v2:
- Remove the changes in this patch to the VCPI helpers, they aren't needed anymore
Changes since v1:
- Rename suspend_or_resume to duplicated
Signed-off-by: Lyude Paul lyude@redhat.com Fixes: eceae1472467 ("drm/dp_mst: Start tracking per-port VCPI allocations") Cc: Daniel Vetter daniel@ffwll.ch
drivers/gpu/drm/drm_atomic_helper.c | 10 +++++++++- drivers/gpu/drm/drm_dp_mst_topology.c | 28 +++++++++++++++++++++++++++ include/drm/drm_atomic.h | 9 +++++++++ 3 files changed, 46 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 6fe2303fccd9..f578bf1fe164 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -332,8 +332,15 @@ update_connector_routing(struct drm_atomic_state *state, * about is ensuring that userspace can't do anything but shut off the * display on a connector that was destroyed after its been notified, * not before.
** Additionally, we also want to ignore connector registration when* we're trying to restore an atomic state during system resume since* there's a chance the connector may have been destroyed during the* process, but it's better to ignore that then cause* drm_atomic_helper_resume() to fail.
- if (drm_connector_is_unregistered(connector) && crtc_state->active) {
- if (!state->duplicated && drm_connector_is_unregistered(connector) &&
crtc_state->active) {@@ -3180,6 +3187,7 @@ drm_atomic_helper_duplicate_state(struct drm_device *dev, return ERR_PTR(-ENOMEM);
state->acquire_ctx = ctx;
state->duplicated = true;
drm_for_each_crtc(crtc, dev) { struct drm_crtc_state *crtc_state;
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 4325e1518286..ea1540ea67af 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -3097,6 +3097,10 @@ static int drm_dp_init_vcpi(struct drm_dp_mst_topology_mgr *mgr,
- @port as needed. It is not OK however, to call this function and
- drm_dp_atomic_release_vcpi_slots() in the same atomic check phase.
- When &drm_atomic_state.duplicated is set to %true%, this function will not
- perform any error checking and will instead simply return the previously
- recorded VCPI allocations.
- See also:
- drm_dp_atomic_release_vcpi_slots()
- drm_dp_mst_atomic_check()
@@ -3123,6 +3127,19 @@ int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state, vcpi = pos; prev_slots = vcpi->vcpi;
/** When resuming, we just want to restore the previous* VCPI without doing error checking*/if (state->duplicated) {DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] [MST PORT:%p] restoring VCPI of %d\n",port->connector->base.id,port->connector->name,port, prev_slots);return prev_slots;}/* * This should never happen, unless the driver tries * releasing and allocating the same VCPI allocation,@@ -3181,6 +3198,11 @@ EXPORT_SYMBOL(drm_dp_atomic_find_vcpi_slots);
- drm_dp_atomic_find_vcpi_slots() on the same @port in a single atomic check
- phase.
- When &drm_atomic_state.duplicated is set, this function will not
- modify the VCPI allocations in &drm_dp_mst_topology_state.vcpis, so that
- those VCPI allocations may be restored as-is from the duplicated state. In
- this scenario, this function will always return 0.
- See also:
- drm_dp_atomic_find_vcpi_slots()
- drm_dp_mst_atomic_check()
@@ -3197,6 +3219,12 @@ int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state, struct drm_dp_vcpi_allocation *pos; bool found = false;
- /* During suspend, just keep our VCPI allocations as-is in the atomic
* state so they can be restored as-is at resume*/- if (state->duplicated)
return 0;- topology_state = drm_atomic_get_mst_topology_state(state, mgr); if (IS_ERR(topology_state)) return PTR_ERR(topology_state);
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 811b4a92568f..961c792fa98b 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -329,6 +329,15 @@ struct drm_atomic_state { bool allow_modeset : 1; bool legacy_cursor_update : 1; bool async_update : 1;
- /**
* @duplicated:** Indicates whether or not this atomic state was duplicated using* drm_atomic_helper_duplicate_state(). Drivers and atomic helpers* should use this to fixup normal inconsistencies in duplicated
s/normal/expected/ maybe? Not too sure about the nuances of English here. Also double space right afterwards.
With or without the bikeshed (but pls remove the double space because ocd):
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
Cheers, Daniel
* states.*/- bool duplicated : 1; struct __drm_planes_state *planes; struct __drm_crtcs_state *crtcs; int num_connector;
-- 2.20.1
Important! below
On Fri, 2019-02-01 at 18:57 +0100, Daniel Vetter wrote:
On Thu, Jan 31, 2019 at 08:14:50PM -0500, Lyude Paul wrote:
Since
commit 39b50c603878 ("drm/atomic_helper: Stop modesets on unregistered connectors harder")
We've been failing atomic checks if they try to enable new displays on unregistered connectors. This is fine except for the one situation that breaks atomic assumptions: suspend/resume. If a connector is unregistered before we attempt to restore the atomic state, something we end up failing the atomic check that happens when trying to restore the state during resume.
Normally this would be OK: we try our best to make sure that the atomic state pre-suspend can be restored post-suspend, but failures at that point usually don't cause problems. That is of course, until we introduced the new atomic MST VCPI helpers:
[drm:drm_atomic_helper_check_modeset [drm_kms_helper]] [CRTC:65:pipe B] active changed [drm:drm_atomic_helper_check_modeset [drm_kms_helper]] Updating routing for [CONNECTOR:123:DP-5] [drm:drm_atomic_helper_check_modeset [drm_kms_helper]] Disabling [CONNECTOR:123:DP-5] [drm:drm_atomic_get_private_obj_state [drm]] Added new private object 0000000025844636 state 000000009fd2899a to 000000003a13d7b8 WARNING: CPU: 6 PID: 1070 at drivers/gpu/drm/drm_dp_mst_topology.c:3153 drm_dp_atomic_release_vcpi_slots+0xb9/0x200 [drm_kms_helper] Modules linked in: fuse vfat fat snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic joydev iTCO_wdt i915(O) wmi_bmof intel_rapl btusb btrtl x86_pkg_temp_thermal btbcm btintel coretemp i2c_algo_bit drm_kms_helper(O) crc32_pclmul snd_hda_intel syscopyarea sysfillrect snd_hda_codec sysimgblt snd_hda_core bluetooth fb_sys_fops snd_pcm pcspkr drm(O) psmouse snd_timer mei_me ecdh_generic i2c_i801 mei i2c_core ucsi_acpi typec_ucsi typec wmi thinkpad_acpi ledtrig_audio snd soundcore tpm_tis rfkill tpm_tis_core video tpm acpi_pad pcc_cpufreq uas usb_storage crc32c_intel nvme serio_raw xhci_pci nvme_core xhci_hcd CPU: 6 PID: 1070 Comm: gnome-shell Tainted: G W O 5.0.0- rc2Lyude-Test+ #1 Hardware name: LENOVO 20L8S2N800/20L8S2N800, BIOS N22ET35W (1.12 ) 04/09/2018 RIP: 0010:drm_dp_atomic_release_vcpi_slots+0xb9/0x200 [drm_kms_helper] Code: 00 4c 39 6d f0 74 49 48 8d 7b 10 48 89 f9 48 c1 e9 03 42 80 3c 21 00 0f 85 d2 00 00 00 48 8b 6b 10 48 8d 5d f0 49 39 ee 75 c5 <0f> 0b 48 c7 c7 c0 78 b3 a0 48 89 c2 4c 89 ee e8 03 6c aa ff b8 ea RSP: 0018:ffff88841235f268 EFLAGS: 00010246 RAX: ffff88841bf12ab0 RBX: ffff88841bf12aa8 RCX: 1ffff110837e2557 RDX: dffffc0000000000 RSI: 0000000000000000 RDI: ffffed108246bde0 RBP: ffff88841bf12ab8 R08: ffffed1083db3c93 R09: ffffed1083db3c92 R10: ffffed1083db3c92 R11: ffff88841ed9e497 R12: ffff888419555d80 R13: ffff8883bc499100 R14: ffff88841bf12ab8 R15: 0000000000000000 FS: 00007f16fbd4cd00(0000) GS:ffff88841ed80000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00007f1687c9f000 CR3: 00000003ba3cc003 CR4: 00000000003606e0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call Trace: drm_atomic_helper_check_modeset+0xf21/0x2f50 [drm_kms_helper] ? drm_atomic_helper_commit_modeset_enables+0xa90/0xa90 [drm_kms_helper] ? __printk_safe_exit+0x10/0x10 ? save_stack+0x8c/0xb0 ? vprintk_func+0x96/0x1bf ? __printk_safe_exit+0x10/0x10 intel_atomic_check+0x234/0x4750 [i915] ? printk+0x9f/0xc5 ? kmsg_dump_rewind_nolock+0xd9/0xd9 ? _raw_spin_lock_irqsave+0xa4/0x140 ? drm_atomic_check_only+0xb1/0x28b0 [drm] ? drm_dbg+0x186/0x1b0 [drm] ? drm_dev_dbg+0x200/0x200 [drm] ? intel_link_compute_m_n+0xb0/0xb0 [i915] ? drm_mode_put_tile_group+0x20/0x20 [drm] ? skl_plane_format_mod_supported+0x17f/0x1b0 [i915] ? drm_plane_check_pixel_format+0x14a/0x310 [drm] drm_atomic_check_only+0x13c4/0x28b0 [drm] ? drm_state_info+0x220/0x220 [drm] ? drm_atomic_helper_disable_plane+0x1d0/0x1d0 [drm_kms_helper] ? pick_single_encoder_for_connector+0xe0/0xe0 [drm_kms_helper] ? kasan_unpoison_shadow+0x35/0x40 drm_atomic_commit+0x3b/0x100 [drm] drm_atomic_helper_set_config+0xd5/0x100 [drm_kms_helper] drm_mode_setcrtc+0x636/0x1660 [drm] ? vprintk_func+0x96/0x1bf ? drm_dev_dbg+0x200/0x200 [drm] ? drm_mode_getcrtc+0x790/0x790 [drm] ? printk+0x9f/0xc5 ? mutex_unlock+0x1d/0x40 ? drm_mode_addfb2+0x2e9/0x3a0 [drm] ? rcu_sync_dtor+0x2e0/0x2e0 ? drm_dbg+0x186/0x1b0 [drm] ? set_page_dirty+0x271/0x4d0 drm_ioctl_kernel+0x203/0x290 [drm] ? drm_mode_getcrtc+0x790/0x790 [drm] ? drm_setversion+0x7f0/0x7f0 [drm] ? __switch_to_asm+0x34/0x70 ? __switch_to_asm+0x34/0x70 drm_ioctl+0x445/0x950 [drm] ? drm_mode_getcrtc+0x790/0x790 [drm] ? drm_getunique+0x220/0x220 [drm] ? expand_files.part.10+0x920/0x920 do_vfs_ioctl+0x1a1/0x13d0 ? ioctl_preallocate+0x2b0/0x2b0 ? __fget_light+0x2d6/0x390 ? schedule+0xd7/0x2e0 ? fget_raw+0x10/0x10 ? apic_timer_interrupt+0xa/0x20 ? apic_timer_interrupt+0xa/0x20 ? rcu_cleanup_dead_rnp+0x2c0/0x2c0 ksys_ioctl+0x60/0x90 __x64_sys_ioctl+0x6f/0xb0 do_syscall_64+0x136/0x440 ? syscall_return_slowpath+0x2d0/0x2d0 ? do_page_fault+0x89/0x330 ? __do_page_fault+0x9c0/0x9c0 ? prepare_exit_to_usermode+0x188/0x200 ? perf_trace_sys_enter+0x1090/0x1090 ? __x64_sys_sigaltstack+0x280/0x280 ? __put_user_4+0x1c/0x30 entry_SYSCALL_64_after_hwframe+0x44/0xa9 RIP: 0033:0x7f16ff89a09b Code: 0f 1e fa 48 8b 05 ed bd 0c 00 64 c7 00 26 00 00 00 48 c7 c0 ff ff ff ff c3 66 0f 1f 44 00 00 f3 0f 1e fa b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d bd bd 0c 00 f7 d8 64 89 01 48 RSP: 002b:00007fff001232b8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010 RAX: ffffffffffffffda RBX: 00007fff001232f0 RCX: 00007f16ff89a09b RDX: 00007fff001232f0 RSI: 00000000c06864a2 RDI: 000000000000000b RBP: 00007fff001232f0 R08: 0000000000000000 R09: 000055a79d484460 R10: 000055a79d44e770 R11: 0000000000000246 R12: 00000000c06864a2 R13: 000000000000000b R14: 0000000000000000 R15: 000055a79d44e770 WARNING: CPU: 6 PID: 1070 at drivers/gpu/drm/drm_dp_mst_topology.c:3153 drm_dp_atomic_release_vcpi_slots+0xb9/0x200 [drm_kms_helper] ---[ end trace d536c05c13c83be2 ]--- [drm:drm_dp_atomic_release_vcpi_slots [drm_kms_helper]] *ERROR* no VCPI for [MST PORT:00000000f9e2b143] found in mst state 000000009fd2899a
This appears to be happening because we destroy the VCPI allocations when disabling all connected displays while suspending, and those VCPI allocations don't get restored on resume due to failing to restore the atomic state.
So, fix this by introducing the suspending option to drm_atomic_helper_duplicate_state() and use that to indicate in the atomic state that it's being used for suspending or resuming the system, and thus needs to be fixed up by the driver. We can then use the new state->duplicated hook to tell update_connector_routing() in drm_atomic_check_modeset() to allow for modesets on unregistered connectors, which allows us to restore atomic states that contain MST topologies that were removed after the state was duplicated and thus: mostly fixing suspend and resume. This just leaves some issues that were introduced with nouveau, that will be addressed next.
Changes since v2:
- Remove the changes in this patch to the VCPI helpers, they aren't needed anymore
Changes since v1:
- Rename suspend_or_resume to duplicated
Signed-off-by: Lyude Paul lyude@redhat.com Fixes: eceae1472467 ("drm/dp_mst: Start tracking per-port VCPI allocations") Cc: Daniel Vetter daniel@ffwll.ch
drivers/gpu/drm/drm_atomic_helper.c | 10 +++++++++- drivers/gpu/drm/drm_dp_mst_topology.c | 28 +++++++++++++++++++++++++++ include/drm/drm_atomic.h | 9 +++++++++ 3 files changed, 46 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 6fe2303fccd9..f578bf1fe164 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -332,8 +332,15 @@ update_connector_routing(struct drm_atomic_state *state, * about is ensuring that userspace can't do anything but shut off the * display on a connector that was destroyed after its been notified, * not before.
** Additionally, we also want to ignore connector registration when* we're trying to restore an atomic state during system resume since* there's a chance the connector may have been destroyed during the* process, but it's better to ignore that then cause* drm_atomic_helper_resume() to fail.
- if (drm_connector_is_unregistered(connector) && crtc_state->active) {
- if (!state->duplicated && drm_connector_is_unregistered(connector) &&
crtc_state->active) {@@ -3180,6 +3187,7 @@ drm_atomic_helper_duplicate_state(struct drm_device *dev, return ERR_PTR(-ENOMEM);
state->acquire_ctx = ctx;
state->duplicated = true;
drm_for_each_crtc(crtc, dev) { struct drm_crtc_state *crtc_state;
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 4325e1518286..ea1540ea67af 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -3097,6 +3097,10 @@ static int drm_dp_init_vcpi(struct drm_dp_mst_topology_mgr *mgr,
- @port as needed. It is not OK however, to call this function and
- drm_dp_atomic_release_vcpi_slots() in the same atomic check phase.
- When &drm_atomic_state.duplicated is set to %true%, this function will
not
- perform any error checking and will instead simply return the
previously
- recorded VCPI allocations.
- See also:
- drm_dp_atomic_release_vcpi_slots()
- drm_dp_mst_atomic_check()
@@ -3123,6 +3127,19 @@ int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state, vcpi = pos; prev_slots = vcpi->vcpi;
/** When resuming, we just want to restore the previous* VCPI without doing error checking*/if (state->duplicated) {DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] [MSTPORT:%p] restoring VCPI of %d\n",
port->connector->base.id,port->connector->name,port, prev_slots);return prev_slots;}
Gah, hold on-part of the point of this was that we didn't need to put special casing for ->duplicated into the VCPI helpers but it looks like I left that chunk in by accident, whoops.
So, the hunk above this comment ^ (more below)
/* * This should never happen, unless the driver tries * releasing and allocating the same VCPI allocation,@@ -3181,6 +3198,11 @@ EXPORT_SYMBOL(drm_dp_atomic_find_vcpi_slots);
- drm_dp_atomic_find_vcpi_slots() on the same @port in a single atomic
check
- phase.
- When &drm_atomic_state.duplicated is set, this function will not
- modify the VCPI allocations in &drm_dp_mst_topology_state.vcpis, so
that
- those VCPI allocations may be restored as-is from the duplicated
state. In
- this scenario, this function will always return 0.
- See also:
- drm_dp_atomic_find_vcpi_slots()
- drm_dp_mst_atomic_check()
@@ -3197,6 +3219,12 @@ int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state, struct drm_dp_vcpi_allocation *pos; bool found = false;
- /* During suspend, just keep our VCPI allocations as-is in the atomic
* state so they can be restored as-is at resume*/- if (state->duplicated)
return 0;
And the hunk above this comment aren't needed or supposed to be here anymore. Does your R-B still count?
topology_state = drm_atomic_get_mst_topology_state(state, mgr); if (IS_ERR(topology_state)) return PTR_ERR(topology_state); diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 811b4a92568f..961c792fa98b 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -329,6 +329,15 @@ struct drm_atomic_state { bool allow_modeset : 1; bool legacy_cursor_update : 1; bool async_update : 1;
- /**
* @duplicated:** Indicates whether or not this atomic state was duplicated using* drm_atomic_helper_duplicate_state(). Drivers and atomic helpers* should use this to fixup normal inconsistencies in duplicateds/normal/expected/ maybe? Not too sure about the nuances of English here. Also double space right afterwards.
With or without the bikeshed (but pls remove the double space because ocd):
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
Cheers, Daniel
* states.*/- bool duplicated : 1; struct __drm_planes_state *planes; struct __drm_crtcs_state *crtcs; int num_connector;
-- 2.20.1
On Fri, Feb 01, 2019 at 05:41:58PM -0500, Lyude Paul wrote:
Important! below
On Fri, 2019-02-01 at 18:57 +0100, Daniel Vetter wrote:
On Thu, Jan 31, 2019 at 08:14:50PM -0500, Lyude Paul wrote:
Since
commit 39b50c603878 ("drm/atomic_helper: Stop modesets on unregistered connectors harder")
We've been failing atomic checks if they try to enable new displays on unregistered connectors. This is fine except for the one situation that breaks atomic assumptions: suspend/resume. If a connector is unregistered before we attempt to restore the atomic state, something we end up failing the atomic check that happens when trying to restore the state during resume.
Normally this would be OK: we try our best to make sure that the atomic state pre-suspend can be restored post-suspend, but failures at that point usually don't cause problems. That is of course, until we introduced the new atomic MST VCPI helpers:
[drm:drm_atomic_helper_check_modeset [drm_kms_helper]] [CRTC:65:pipe B] active changed [drm:drm_atomic_helper_check_modeset [drm_kms_helper]] Updating routing for [CONNECTOR:123:DP-5] [drm:drm_atomic_helper_check_modeset [drm_kms_helper]] Disabling [CONNECTOR:123:DP-5] [drm:drm_atomic_get_private_obj_state [drm]] Added new private object 0000000025844636 state 000000009fd2899a to 000000003a13d7b8 WARNING: CPU: 6 PID: 1070 at drivers/gpu/drm/drm_dp_mst_topology.c:3153 drm_dp_atomic_release_vcpi_slots+0xb9/0x200 [drm_kms_helper] Modules linked in: fuse vfat fat snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic joydev iTCO_wdt i915(O) wmi_bmof intel_rapl btusb btrtl x86_pkg_temp_thermal btbcm btintel coretemp i2c_algo_bit drm_kms_helper(O) crc32_pclmul snd_hda_intel syscopyarea sysfillrect snd_hda_codec sysimgblt snd_hda_core bluetooth fb_sys_fops snd_pcm pcspkr drm(O) psmouse snd_timer mei_me ecdh_generic i2c_i801 mei i2c_core ucsi_acpi typec_ucsi typec wmi thinkpad_acpi ledtrig_audio snd soundcore tpm_tis rfkill tpm_tis_core video tpm acpi_pad pcc_cpufreq uas usb_storage crc32c_intel nvme serio_raw xhci_pci nvme_core xhci_hcd CPU: 6 PID: 1070 Comm: gnome-shell Tainted: G W O 5.0.0- rc2Lyude-Test+ #1 Hardware name: LENOVO 20L8S2N800/20L8S2N800, BIOS N22ET35W (1.12 ) 04/09/2018 RIP: 0010:drm_dp_atomic_release_vcpi_slots+0xb9/0x200 [drm_kms_helper] Code: 00 4c 39 6d f0 74 49 48 8d 7b 10 48 89 f9 48 c1 e9 03 42 80 3c 21 00 0f 85 d2 00 00 00 48 8b 6b 10 48 8d 5d f0 49 39 ee 75 c5 <0f> 0b 48 c7 c7 c0 78 b3 a0 48 89 c2 4c 89 ee e8 03 6c aa ff b8 ea RSP: 0018:ffff88841235f268 EFLAGS: 00010246 RAX: ffff88841bf12ab0 RBX: ffff88841bf12aa8 RCX: 1ffff110837e2557 RDX: dffffc0000000000 RSI: 0000000000000000 RDI: ffffed108246bde0 RBP: ffff88841bf12ab8 R08: ffffed1083db3c93 R09: ffffed1083db3c92 R10: ffffed1083db3c92 R11: ffff88841ed9e497 R12: ffff888419555d80 R13: ffff8883bc499100 R14: ffff88841bf12ab8 R15: 0000000000000000 FS: 00007f16fbd4cd00(0000) GS:ffff88841ed80000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00007f1687c9f000 CR3: 00000003ba3cc003 CR4: 00000000003606e0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call Trace: drm_atomic_helper_check_modeset+0xf21/0x2f50 [drm_kms_helper] ? drm_atomic_helper_commit_modeset_enables+0xa90/0xa90 [drm_kms_helper] ? __printk_safe_exit+0x10/0x10 ? save_stack+0x8c/0xb0 ? vprintk_func+0x96/0x1bf ? __printk_safe_exit+0x10/0x10 intel_atomic_check+0x234/0x4750 [i915] ? printk+0x9f/0xc5 ? kmsg_dump_rewind_nolock+0xd9/0xd9 ? _raw_spin_lock_irqsave+0xa4/0x140 ? drm_atomic_check_only+0xb1/0x28b0 [drm] ? drm_dbg+0x186/0x1b0 [drm] ? drm_dev_dbg+0x200/0x200 [drm] ? intel_link_compute_m_n+0xb0/0xb0 [i915] ? drm_mode_put_tile_group+0x20/0x20 [drm] ? skl_plane_format_mod_supported+0x17f/0x1b0 [i915] ? drm_plane_check_pixel_format+0x14a/0x310 [drm] drm_atomic_check_only+0x13c4/0x28b0 [drm] ? drm_state_info+0x220/0x220 [drm] ? drm_atomic_helper_disable_plane+0x1d0/0x1d0 [drm_kms_helper] ? pick_single_encoder_for_connector+0xe0/0xe0 [drm_kms_helper] ? kasan_unpoison_shadow+0x35/0x40 drm_atomic_commit+0x3b/0x100 [drm] drm_atomic_helper_set_config+0xd5/0x100 [drm_kms_helper] drm_mode_setcrtc+0x636/0x1660 [drm] ? vprintk_func+0x96/0x1bf ? drm_dev_dbg+0x200/0x200 [drm] ? drm_mode_getcrtc+0x790/0x790 [drm] ? printk+0x9f/0xc5 ? mutex_unlock+0x1d/0x40 ? drm_mode_addfb2+0x2e9/0x3a0 [drm] ? rcu_sync_dtor+0x2e0/0x2e0 ? drm_dbg+0x186/0x1b0 [drm] ? set_page_dirty+0x271/0x4d0 drm_ioctl_kernel+0x203/0x290 [drm] ? drm_mode_getcrtc+0x790/0x790 [drm] ? drm_setversion+0x7f0/0x7f0 [drm] ? __switch_to_asm+0x34/0x70 ? __switch_to_asm+0x34/0x70 drm_ioctl+0x445/0x950 [drm] ? drm_mode_getcrtc+0x790/0x790 [drm] ? drm_getunique+0x220/0x220 [drm] ? expand_files.part.10+0x920/0x920 do_vfs_ioctl+0x1a1/0x13d0 ? ioctl_preallocate+0x2b0/0x2b0 ? __fget_light+0x2d6/0x390 ? schedule+0xd7/0x2e0 ? fget_raw+0x10/0x10 ? apic_timer_interrupt+0xa/0x20 ? apic_timer_interrupt+0xa/0x20 ? rcu_cleanup_dead_rnp+0x2c0/0x2c0 ksys_ioctl+0x60/0x90 __x64_sys_ioctl+0x6f/0xb0 do_syscall_64+0x136/0x440 ? syscall_return_slowpath+0x2d0/0x2d0 ? do_page_fault+0x89/0x330 ? __do_page_fault+0x9c0/0x9c0 ? prepare_exit_to_usermode+0x188/0x200 ? perf_trace_sys_enter+0x1090/0x1090 ? __x64_sys_sigaltstack+0x280/0x280 ? __put_user_4+0x1c/0x30 entry_SYSCALL_64_after_hwframe+0x44/0xa9 RIP: 0033:0x7f16ff89a09b Code: 0f 1e fa 48 8b 05 ed bd 0c 00 64 c7 00 26 00 00 00 48 c7 c0 ff ff ff ff c3 66 0f 1f 44 00 00 f3 0f 1e fa b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d bd bd 0c 00 f7 d8 64 89 01 48 RSP: 002b:00007fff001232b8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010 RAX: ffffffffffffffda RBX: 00007fff001232f0 RCX: 00007f16ff89a09b RDX: 00007fff001232f0 RSI: 00000000c06864a2 RDI: 000000000000000b RBP: 00007fff001232f0 R08: 0000000000000000 R09: 000055a79d484460 R10: 000055a79d44e770 R11: 0000000000000246 R12: 00000000c06864a2 R13: 000000000000000b R14: 0000000000000000 R15: 000055a79d44e770 WARNING: CPU: 6 PID: 1070 at drivers/gpu/drm/drm_dp_mst_topology.c:3153 drm_dp_atomic_release_vcpi_slots+0xb9/0x200 [drm_kms_helper] ---[ end trace d536c05c13c83be2 ]--- [drm:drm_dp_atomic_release_vcpi_slots [drm_kms_helper]] *ERROR* no VCPI for [MST PORT:00000000f9e2b143] found in mst state 000000009fd2899a
This appears to be happening because we destroy the VCPI allocations when disabling all connected displays while suspending, and those VCPI allocations don't get restored on resume due to failing to restore the atomic state.
So, fix this by introducing the suspending option to drm_atomic_helper_duplicate_state() and use that to indicate in the atomic state that it's being used for suspending or resuming the system, and thus needs to be fixed up by the driver. We can then use the new state->duplicated hook to tell update_connector_routing() in drm_atomic_check_modeset() to allow for modesets on unregistered connectors, which allows us to restore atomic states that contain MST topologies that were removed after the state was duplicated and thus: mostly fixing suspend and resume. This just leaves some issues that were introduced with nouveau, that will be addressed next.
Changes since v2:
- Remove the changes in this patch to the VCPI helpers, they aren't needed anymore
Changes since v1:
- Rename suspend_or_resume to duplicated
Signed-off-by: Lyude Paul lyude@redhat.com Fixes: eceae1472467 ("drm/dp_mst: Start tracking per-port VCPI allocations") Cc: Daniel Vetter daniel@ffwll.ch
drivers/gpu/drm/drm_atomic_helper.c | 10 +++++++++- drivers/gpu/drm/drm_dp_mst_topology.c | 28 +++++++++++++++++++++++++++ include/drm/drm_atomic.h | 9 +++++++++ 3 files changed, 46 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 6fe2303fccd9..f578bf1fe164 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -332,8 +332,15 @@ update_connector_routing(struct drm_atomic_state *state, * about is ensuring that userspace can't do anything but shut off the * display on a connector that was destroyed after its been notified, * not before.
** Additionally, we also want to ignore connector registration when* we're trying to restore an atomic state during system resume since* there's a chance the connector may have been destroyed during the* process, but it's better to ignore that then cause* drm_atomic_helper_resume() to fail.
- if (drm_connector_is_unregistered(connector) && crtc_state->active) {
- if (!state->duplicated && drm_connector_is_unregistered(connector) &&
crtc_state->active) {@@ -3180,6 +3187,7 @@ drm_atomic_helper_duplicate_state(struct drm_device *dev, return ERR_PTR(-ENOMEM);
state->acquire_ctx = ctx;
state->duplicated = true;
drm_for_each_crtc(crtc, dev) { struct drm_crtc_state *crtc_state;
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 4325e1518286..ea1540ea67af 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -3097,6 +3097,10 @@ static int drm_dp_init_vcpi(struct drm_dp_mst_topology_mgr *mgr,
- @port as needed. It is not OK however, to call this function and
- drm_dp_atomic_release_vcpi_slots() in the same atomic check phase.
- When &drm_atomic_state.duplicated is set to %true%, this function will
not
- perform any error checking and will instead simply return the
previously
- recorded VCPI allocations.
- See also:
- drm_dp_atomic_release_vcpi_slots()
- drm_dp_mst_atomic_check()
@@ -3123,6 +3127,19 @@ int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state, vcpi = pos; prev_slots = vcpi->vcpi;
/** When resuming, we just want to restore the previous* VCPI without doing error checking*/if (state->duplicated) {DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] [MSTPORT:%p] restoring VCPI of %d\n",
port->connector->base.id,port->connector->name,port, prev_slots);return prev_slots;}Gah, hold on-part of the point of this was that we didn't need to put special casing for ->duplicated into the VCPI helpers but it looks like I left that chunk in by accident, whoops.
So, the hunk above this comment ^ (more below)
/* * This should never happen, unless the driver tries * releasing and allocating the same VCPI allocation,@@ -3181,6 +3198,11 @@ EXPORT_SYMBOL(drm_dp_atomic_find_vcpi_slots);
- drm_dp_atomic_find_vcpi_slots() on the same @port in a single atomic
check
- phase.
- When &drm_atomic_state.duplicated is set, this function will not
- modify the VCPI allocations in &drm_dp_mst_topology_state.vcpis, so
that
- those VCPI allocations may be restored as-is from the duplicated
state. In
- this scenario, this function will always return 0.
- See also:
- drm_dp_atomic_find_vcpi_slots()
- drm_dp_mst_atomic_check()
@@ -3197,6 +3219,12 @@ int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state, struct drm_dp_vcpi_allocation *pos; bool found = false;
- /* During suspend, just keep our VCPI allocations as-is in the atomic
* state so they can be restored as-is at resume*/- if (state->duplicated)
return 0;And the hunk above this comment aren't needed or supposed to be here anymore. Does your R-B still count?
Huh, I convinced myself that we still need these (and that the goal of this series was to get the is_unregistered() checks out of the drivers). Can you pls resend with that twist (in-reply-to here or whatever) so that intel CI can check this, and I get some time to ponder where exactly I've gone off the rails with my thinking?
Thanks, Daniel
topology_state = drm_atomic_get_mst_topology_state(state, mgr); if (IS_ERR(topology_state)) return PTR_ERR(topology_state); diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 811b4a92568f..961c792fa98b 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -329,6 +329,15 @@ struct drm_atomic_state { bool allow_modeset : 1; bool legacy_cursor_update : 1; bool async_update : 1;
- /**
* @duplicated:** Indicates whether or not this atomic state was duplicated using* drm_atomic_helper_duplicate_state(). Drivers and atomic helpers* should use this to fixup normal inconsistencies in duplicateds/normal/expected/ maybe? Not too sure about the nuances of English here. Also double space right afterwards.
With or without the bikeshed (but pls remove the double space because ocd):
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
Cheers, Daniel
* states.*/- bool duplicated : 1; struct __drm_planes_state *planes; struct __drm_crtcs_state *crtcs; int num_connector;
-- 2.20.1
-- Cheers, Lyude Paul
Atomic checks should never modify anything outside of the state that they're passed in. Unfortunately this appears to be exactly what we're doing in nv50_msto_atomic_check() where we update mstc->pbn every time the function is called. This hasn't caused any bugs yet, but it needs to be fixed in order to ensure that when committing an artificially duplicated state (like during system resume), that we reuse the PBN of that state to perform VCPI allocations and don't recalculate a different value from the drm connector's reported bpc.
Also, move the VCPI slot allocations while we're at it as well. With this, removing a topology in suspend while using nouveau no longer causes the new atomic VCPI helpers to complain.
Signed-off-by: Lyude Paul lyude@redhat.com Fixes: eceae1472467 ("drm/dp_mst: Start tracking per-port VCPI allocations") Cc: Daniel Vetter daniel@ffwll.ch --- drivers/gpu/drm/nouveau/dispnv50/atom.h | 6 ++++++ drivers/gpu/drm/nouveau/dispnv50/disp.c | 28 +++++++++++++++---------- drivers/gpu/drm/nouveau/dispnv50/head.c | 1 + 3 files changed, 24 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/dispnv50/atom.h b/drivers/gpu/drm/nouveau/dispnv50/atom.h index a194990d2b0d..b5fae5ab3fa8 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/atom.h +++ b/drivers/gpu/drm/nouveau/dispnv50/atom.h @@ -116,6 +116,12 @@ struct nv50_head_atom { u8 depth:4; } or;
+ /* Currently only used for MST */ + struct { + int pbn; + u8 tu:6; + } dp; + union nv50_head_atom_mask { struct { bool olut:1; diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c index 60d858c2f2ce..e8bb35f6d015 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c @@ -659,8 +659,6 @@ struct nv50_mstc {
struct drm_display_mode *native; struct edid *edid; - - int pbn; };
struct nv50_msto { @@ -765,17 +763,26 @@ nv50_msto_atomic_check(struct drm_encoder *encoder, struct drm_connector *connector = conn_state->connector; struct nv50_mstc *mstc = nv50_mstc(connector); struct nv50_mstm *mstm = mstc->mstm; - int bpp = connector->display_info.bpc * 3; + struct nv50_head_atom *asyh = nv50_head_atom(crtc_state); int slots;
- mstc->pbn = drm_dp_calc_pbn_mode(crtc_state->adjusted_mode.clock, - bpp); + /* 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 (drm_atomic_crtc_needs_modeset(crtc_state)) { slots = drm_dp_atomic_find_vcpi_slots(state, &mstm->mgr, - mstc->port, mstc->pbn); + mstc->port, + asyh->dp.pbn); if (slots < 0) return slots; + + asyh->dp.tu = slots; }
return nv50_outp_atomic_check_view(encoder, crtc_state, conn_state, @@ -786,13 +793,13 @@ static void nv50_msto_enable(struct drm_encoder *encoder) { struct nv50_head *head = nv50_head(encoder->crtc); + struct nv50_head_atom *armh = nv50_head_atom(head->base.base.state); struct nv50_msto *msto = nv50_msto(encoder); struct nv50_mstc *mstc = NULL; struct nv50_mstm *mstm = NULL; struct drm_connector *connector; struct drm_connector_list_iter conn_iter; u8 proto, depth; - int slots; bool r;
drm_connector_list_iter_begin(encoder->dev, &conn_iter); @@ -808,8 +815,8 @@ nv50_msto_enable(struct drm_encoder *encoder) if (WARN_ON(!mstc)) return;
- slots = drm_dp_find_vcpi_slots(&mstm->mgr, mstc->pbn); - r = drm_dp_mst_allocate_vcpi(&mstm->mgr, mstc->port, mstc->pbn, slots); + r = drm_dp_mst_allocate_vcpi(&mstm->mgr, mstc->port, armh->dp.pbn, + armh->dp.tu); WARN_ON(!r);
if (!mstm->links++) @@ -827,8 +834,7 @@ nv50_msto_enable(struct drm_encoder *encoder) default: depth = 0x6; break; }
- mstm->outp->update(mstm->outp, head->base.index, - nv50_head_atom(head->base.base.state), proto, depth); + mstm->outp->update(mstm->outp, head->base.index, armh, proto, depth);
msto->head = head; msto->mstc = mstc; diff --git a/drivers/gpu/drm/nouveau/dispnv50/head.c b/drivers/gpu/drm/nouveau/dispnv50/head.c index ac97ebce5b35..2e7a0c347ddb 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/head.c +++ b/drivers/gpu/drm/nouveau/dispnv50/head.c @@ -413,6 +413,7 @@ nv50_head_atomic_duplicate_state(struct drm_crtc *crtc) asyh->ovly = armh->ovly; asyh->dither = armh->dither; asyh->procamp = armh->procamp; + asyh->dp = armh->dp; asyh->clr.mask = 0; asyh->set.mask = 0; return &asyh->state;
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Lyude Paul