Re: [PATCH 1/1] drm/dp_mst: Kill the second sideband tx slot, save the world
Hi [This is an automated email] This commit has been processed because it contains a "Fixes:" tag fixing commit: ad7f8a1f9ced ("drm/helper: add Displayport multi-stream helper (v0.6)"). The bot has tested the following trees: v5.6.7, v5.4.35, v4.19.118, v4.14.177, v4.9.220, v4.4.220. v5.6.7: Failed to apply! Possible dependencies: 1cfff5f01563 ("drm/dp_mst: Convert drm_dp_mst_topology_mgr.is_waiting_for_dwn_reply to bitfield") v5.4.35: Failed to apply! Possible dependencies: 14692a3637d4 ("drm/dp_mst: Add probe_lock") 2f015ec6eab6 ("drm/dp_mst: Add sideband down request tracing + selftests") 37dfdc55ffeb ("drm/dp_mst: Cleanup drm_dp_send_link_address() a bit") 50094b5dcd32 ("drm/dp_mst: Destroy topology_mgr mutexes") 5950f0b797fc ("drm/dp_mst: Move link address dumping into a function") 5a64967a2f3b ("drm/dp_mst: Have DP_Tx send one msg at a time") 60f9ae9d0d3d ("drm/dp_mst: Remove huge conditional in drm_dp_mst_handle_up_req()") 7cb12d48314e ("drm/dp_mst: Destroy MSTBs asynchronously") 7cbce45d6243 ("drm/dp_mst: Move test_calc_pbn_mode() into an actual selftest") 8b1e589d138c ("drm/dp_mst: Refactor drm_dp_mst_handle_down_rep()") 9408cc94eb04 ("drm/dp_mst: Handle UP requests asynchronously") a29d881875fc ("drm/dp_mst: Refactor drm_dp_mst_handle_up_req()") caf81ec6cd72 ("drm: Destroy the correct mutex name in drm_dp_mst_topology_mgr_destroy") e2839ff692c6 ("drm/dp_mst: Rename drm_dp_add_port and drm_dp_update_port") v4.19.118: Failed to apply! Possible dependencies: 16bff572cc66 ("drm/dp-mst-helper: Remove hotplug callback") 19b85cfabf5c ("drm/bochs: move remaining fb bits to kms") 2f015ec6eab6 ("drm/dp_mst: Add sideband down request tracing + selftests") 2f69deb1d9a1 ("drm/arcpgu: prepare for drmP.h removal from drm_modeset_helper.h") 48b442238250 ("drm/bochs: fix DRM_FORMAT_* handling for big endian machines.") 562836a269e3 ("drm/dp_mst: Enable registration of AUX devices for MST ports") 580fc13f3ee4 ("drm/dp: drmP.h include removal") 5a64967a2f3b ("drm/dp_mst: Have DP_Tx send one msg at a time") 6579c39594ae ("drm/bochs: atomic: switch planes to atomic, wire up helpers.") 6abb49402a79 ("drm/bridge: cdns: prepare for drmP.h removal from drm_modeset_helper.h") 6c76c0eb031f ("drm/bridge: ti-sn65dsi86: Fixup register names") 7780eb9ce80f ("bochs: convert to drm_dev_register") 86351de023dd ("drm/bochs: support changing byteorder at mode set time") a095f15c00e2 ("drm/bridge: add support for sn65dsi86 bridge driver") b814ec6d4535 ("drm/bridge: ti-sn65dsi86: Implement AUX channel") df2052cc9221 ("bochs: convert to drm_fb_helper_fbdev_setup/teardown") f38b7cca6d0e ("drm/bridge: tc358764: Add DSI to LVDS bridge driver") fcd70cd36b9b ("drm: Split out drm_probe_helper.h") fe1f664a3609 ("drm/arc: do not rely on drmP.h from drm_gem_cma_helper.h") v4.14.177: Failed to apply! Possible dependencies: 1b0c0f9dc5ca ("drm/amdgpu: move userptr BOs to CPU domain during CS v2") 1ed3d2567c80 ("drm/amdgpu: keep the MMU lock until the update ends v4") 2f015ec6eab6 ("drm/dp_mst: Add sideband down request tracing + selftests") 3fe89771cb0a ("drm/amdgpu: stop reserving the BO in the MMU callback v3") 4562236b3bc0 ("drm/amd/dc: Add dc display driver (v2)") 562836a269e3 ("drm/dp_mst: Enable registration of AUX devices for MST ports") 580fc13f3ee4 ("drm/dp: drmP.h include removal") 5a64967a2f3b ("drm/dp_mst: Have DP_Tx send one msg at a time") 60de1c1740f3 ("drm/amdgpu: use a rw_semaphore for MMU notifiers") 9a18999640fa ("drm/amdgpu: move MMU notifier related defines to amdgpu_mn.h") 9cca0b8e5df0 ("drm/amdgpu: move amdgpu_cs_sysvm_access_required into find_mapping") a216ab09955d ("drm/amdgpu: fix userptr put_page handling") b72cf4fca2bb ("drm/amdgpu: move taking mmap_sem into get_user_pages v2") ca666a3c298f ("drm/amdgpu: stop using BO status for user pages") fcd70cd36b9b ("drm: Split out drm_probe_helper.h") v4.9.220: Failed to apply! Possible dependencies: 178e32c224d2 ("drm/atomic: Remove pointless private object NULL state check") 1cec20f0ea0e ("dma-buf: Restart reservation_object_wait_timeout_rcu() after writes") 2f015ec6eab6 ("drm/dp_mst: Add sideband down request tracing + selftests") 3941dae15ed9 ("drm_dp_aux_dev: switch to read_iter/write_iter") 3f3353b7e121 ("drm/dp: Introduce MST topology state to track available link bandwidth") 562836a269e3 ("drm/dp_mst: Enable registration of AUX devices for MST ports") 580fc13f3ee4 ("drm/dp: drmP.h include removal") 5a64967a2f3b ("drm/dp_mst: Have DP_Tx send one msg at a time") 6806cdf9aa1c ("drm/kms-helpers: Use recommened kerneldoc for struct member refs") 78010cd9736e ("dma-buf/fence: add an lockdep_assert_held()") 9498c19b3f53 ("drm: Move tile group code into drm_connector.c") 9a83a71ac0d5 ("drm/fences: add DOC: for
[PATCH 1/1] drm/dp_mst: Kill the second sideband tx slot, save the world
While we support using both tx slots for sideband transmissions, it appears that DisplayPort devices in the field didn't end up doing a very good job of supporting it. From section 5.2.1 of the DP 2.0 specification: There are MST Sink/Branch devices in the field that do not handle interleaved message transactions. To facilitate message transaction handling by downstream devices, an MST Source device shall generate message transactions in an atomic manner (i.e., the MST Source device shall not concurrently interleave multiple message transactions). Therefore, an MST Source device shall clear the Message_Sequence_No value in the Sideband_MSG_Header to 0. This might come as a bit of a surprise since the vast majority of hubs will support using both tx slots even if they don't support interleaved message transactions, and we've also been using both tx slots since MST was introduced into the kernel. However, there is one device we've had trouble getting working consistently with MST for so long that we actually assumed it was just broken: the infamous Dell P2415Qb. Previously this monitor would appear to work sometimes, but in most situations would end up timing out LINK_ADDRESS messages almost at random until you power cycled the whole display. After reading section 5.2.1 in the DP 2.0 spec, some closer investigation into this infamous display revealed it was only ever timing out on sideband messages in the second TX slot. Sure enough, avoiding the second TX slot has suddenly made this monitor function perfectly for the first time in five years. And since they explicitly mention this in the specification, I doubt this is the only monitor out there with this issue. This might even explain explain the seemingly harmless garbage sideband responses we would occasionally see with MST hubs! So - rewrite our sideband TX handlers to only support one TX slot. In order to simplify our sideband handling now that we don't support transmitting to multiple MSTBs at once, we also move all state tracking for down replies from mstbs to the topology manager. Signed-off-by: Lyude Paul Fixes: ad7f8a1f9ced ("drm/helper: add Displayport multi-stream helper (v0.6)") Cc: Sean Paul Cc: "Lin, Wayne" Cc: # v3.17+ Reviewed-by: Sean Paul Link: https://patchwork.freedesktop.org/patch/msgid/20200424181308.770749-1-ly...@redhat.com --- drivers/gpu/drm/drm_dp_mst_topology.c | 122 +++--- include/drm/drm_dp_mst_helper.h | 18 +--- 2 files changed, 33 insertions(+), 107 deletions(-) diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 9d89ebf3a749..ed6faaf4bbf3 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -1203,16 +1203,8 @@ static int drm_dp_mst_wait_tx_reply(struct drm_dp_mst_branch *mstb, /* remove from q */ if (txmsg->state == DRM_DP_SIDEBAND_TX_QUEUED || - txmsg->state == DRM_DP_SIDEBAND_TX_START_SEND) { + txmsg->state == DRM_DP_SIDEBAND_TX_START_SEND) list_del(>next); - } - - if (txmsg->state == DRM_DP_SIDEBAND_TX_START_SEND || - txmsg->state == DRM_DP_SIDEBAND_TX_SENT) { - mstb->tx_slots[txmsg->seqno] = NULL; - } - mgr->is_waiting_for_dwn_reply = false; - } out: if (unlikely(ret == -EIO) && drm_debug_enabled(DRM_UT_DP)) { @@ -2691,22 +2683,6 @@ static int set_hdr_from_dst_qlock(struct drm_dp_sideband_msg_hdr *hdr, struct drm_dp_mst_branch *mstb = txmsg->dst; u8 req_type; - /* both msg slots are full */ - if (txmsg->seqno == -1) { - if (mstb->tx_slots[0] && mstb->tx_slots[1]) { - DRM_DEBUG_KMS("%s: failed to find slot\n", __func__); - return -EAGAIN; - } - if (mstb->tx_slots[0] == NULL && mstb->tx_slots[1] == NULL) { - txmsg->seqno = mstb->last_seqno; - mstb->last_seqno ^= 1; - } else if (mstb->tx_slots[0] == NULL) - txmsg->seqno = 0; - else - txmsg->seqno = 1; - mstb->tx_slots[txmsg->seqno] = txmsg; - } - req_type = txmsg->msg[0] & 0x7f; if (req_type == DP_CONNECTION_STATUS_NOTIFY || req_type == DP_RESOURCE_STATUS_NOTIFY) @@ -2718,7 +2694,7 @@ static int set_hdr_from_dst_qlock(struct drm_dp_sideband_msg_hdr *hdr, hdr->lcr = mstb->lct - 1; if (mstb->lct > 1) memcpy(hdr->rad, mstb->rad, mstb->lct / 2); - hdr->seqno = txmsg->seqno; + return 0; } /* @@ -2733,15 +2709,15 @@ static int process_single_tx_qlock(struct drm_dp_mst_topology_mgr *mgr, int len, space, idx, tosend; int ret; + if (txmsg->state == DRM_DP_SIDEBAND_TX_SENT) +