* [PATCH v3 1/4] drm/dp_mst: Fix unbalanced malloc ref in drm_dp_mst_deallocate_vcpi()
2019-02-02 0:20 [PATCH v3 0/4] drm/dp_mst: Fix regressions from new atomic VCPI helpers Lyude Paul
@ 2019-02-02 0:20 ` Lyude Paul
2019-02-02 0:20 ` [PATCH v3 2/4] drm/dp_mst: Remove port validation in drm_dp_atomic_find_vcpi_slots() Lyude Paul
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Lyude Paul @ 2019-02-02 0:20 UTC (permalink / raw)
To: dri-devel, nouveau
Cc: Daniel Vetter, Maarten Lankhorst, Maxime Ripard, Sean Paul,
David Airlie, Daniel Vetter, linux-kernel
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;
--
2.20.1
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH v3 2/4] drm/dp_mst: Remove port validation in drm_dp_atomic_find_vcpi_slots()
2019-02-02 0:20 [PATCH v3 0/4] drm/dp_mst: Fix regressions from new atomic VCPI helpers Lyude Paul
2019-02-02 0:20 ` [PATCH v3 1/4] drm/dp_mst: Fix unbalanced malloc ref in drm_dp_mst_deallocate_vcpi() Lyude Paul
@ 2019-02-02 0:20 ` Lyude Paul
2019-02-02 0:20 ` [PATCH v3 3/4] drm/atomic: Add drm_atomic_state->duplicated Lyude Paul
2019-02-02 0:20 ` [PATCH v3 4/4] drm/nouveau: Move PBN and VCPI allocation into nv50_head_atom Lyude Paul
3 siblings, 0 replies; 7+ messages in thread
From: Lyude Paul @ 2019-02-02 0:20 UTC (permalink / raw)
To: dri-devel, nouveau
Cc: Daniel Vetter, Maarten Lankhorst, Maxime Ripard, Sean Paul,
David Airlie, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
Ben Skeggs, Ilia Mirkin, linux-kernel, intel-gfx
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")
Reviewed-by: 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)
--
2.20.1
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH v3 3/4] drm/atomic: Add drm_atomic_state->duplicated
2019-02-02 0:20 [PATCH v3 0/4] drm/dp_mst: Fix regressions from new atomic VCPI helpers Lyude Paul
2019-02-02 0:20 ` [PATCH v3 1/4] drm/dp_mst: Fix unbalanced malloc ref in drm_dp_mst_deallocate_vcpi() Lyude Paul
2019-02-02 0:20 ` [PATCH v3 2/4] drm/dp_mst: Remove port validation in drm_dp_atomic_find_vcpi_slots() Lyude Paul
@ 2019-02-02 0:20 ` Lyude Paul
2019-02-04 10:04 ` Daniel Vetter
2019-02-02 0:20 ` [PATCH v3 4/4] drm/nouveau: Move PBN and VCPI allocation into nv50_head_atom Lyude Paul
3 siblings, 1 reply; 7+ messages in thread
From: Lyude Paul @ 2019-02-02 0:20 UTC (permalink / raw)
To: dri-devel, nouveau
Cc: Daniel Vetter, Maarten Lankhorst, Maxime Ripard, Sean Paul,
David Airlie, linux-kernel
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 v3:
* Remove ->duplicated hunks that I left in the VCPI helpers by accident.
These don't need to be here, that was the supposed to be the purpose
of the last revision
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 | 9 +++++++++
include/drm/drm_atomic.h | 9 +++++++++
3 files changed, 27 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..61bba5bd1551 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()
@@ -3181,6 +3185,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()
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;
--
2.20.1
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v3 3/4] drm/atomic: Add drm_atomic_state->duplicated
2019-02-02 0:20 ` [PATCH v3 3/4] drm/atomic: Add drm_atomic_state->duplicated Lyude Paul
@ 2019-02-04 10:04 ` Daniel Vetter
0 siblings, 0 replies; 7+ messages in thread
From: Daniel Vetter @ 2019-02-04 10:04 UTC (permalink / raw)
To: Lyude Paul
Cc: dri-devel, nouveau, Daniel Vetter, Maarten Lankhorst,
Maxime Ripard, Sean Paul, David Airlie, linux-kernel
On Fri, Feb 01, 2019 at 07:20:03PM -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 v3:
> * Remove ->duplicated hunks that I left in the VCPI helpers by accident.
> These don't need to be here, that was the supposed to be the purpose
> of the last revision
> 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 | 9 +++++++++
> include/drm/drm_atomic.h | 9 +++++++++
> 3 files changed, 27 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) {
This now is the only is_unregistered() check in the entire atomic ioctl
path I could find, makes sense that we only have to check for
state->duplicated here. Plus all the validated topology references are
gone too.
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 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..61bba5bd1551 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()
> @@ -3181,6 +3185,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()
> 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;
> --
> 2.20.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v3 4/4] drm/nouveau: Move PBN and VCPI allocation into nv50_head_atom
2019-02-02 0:20 [PATCH v3 0/4] drm/dp_mst: Fix regressions from new atomic VCPI helpers Lyude Paul
` (2 preceding siblings ...)
2019-02-02 0:20 ` [PATCH v3 3/4] drm/atomic: Add drm_atomic_state->duplicated Lyude Paul
@ 2019-02-02 0:20 ` Lyude Paul
2019-02-05 1:37 ` [Nouveau] " Ben Skeggs
3 siblings, 1 reply; 7+ messages in thread
From: Lyude Paul @ 2019-02-02 0:20 UTC (permalink / raw)
To: dri-devel, nouveau
Cc: Daniel Vetter, Ben Skeggs, David Airlie, Sean Paul, Ilia Mirkin,
linux-kernel
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;
--
2.20.1
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Nouveau] [PATCH v3 4/4] drm/nouveau: Move PBN and VCPI allocation into nv50_head_atom
2019-02-02 0:20 ` [PATCH v3 4/4] drm/nouveau: Move PBN and VCPI allocation into nv50_head_atom Lyude Paul
@ 2019-02-05 1:37 ` Ben Skeggs
0 siblings, 0 replies; 7+ messages in thread
From: Ben Skeggs @ 2019-02-05 1:37 UTC (permalink / raw)
To: Lyude Paul
Cc: dri-devel, nouveau, David Airlie, linux-kernel, Sean Paul,
Ben Skeggs, Daniel Vetter
On Sat, 2 Feb 2019 at 10:20, Lyude Paul <lyude@redhat.com> wrote:
>
> 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>
Reviewed-by: Ben Skeggs <bskeggs@redhat.com>
> ---
> 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;
> --
> 2.20.1
>
> _______________________________________________
> Nouveau mailing list
> Nouveau@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau
^ permalink raw reply [flat|nested] 7+ messages in thread