From bf9d3f5e40d566ac9a54e5e8522cfa0edd66eb07 Mon Sep 17 00:00:00 2001
From: Lyude Paul <lyude@redhat.com>
Date: Wed, 13 Apr 2022 13:41:05 -0400
Subject: [PATCH] WIP: drm/dp_mst: Drop all ports from topology on CSNs before
queueing link address work
TODO: Finish writing commit message by adding refs
We want to start cutting down on all of the places that we use port
validation, so that ports may be removed from the topology as quickly as
possible to minimize the number of errors we run into as a result of being
out of sync with the current topology status. Let's do this with CSNs by
moving some code around so that we only queue link address probing work at
the end of handling all CSNs - allowing us to make sure we drop as many
topology references as we can beforehand.
This is also intended to replace the workaround that was added fix to some
of the payload leaks that were observed before $COMMIT, where we would
attempt to determine if the port was still connected to the topology before
updating payloads using drm_dp_mst_port_downstream_of_branch. This wasn't a
particularly good solution, since one of the points of still having port
and mstb validation is to avoid sending messages to newly disconnected
branches wherever possible - thus the required use of
drm_dp_mst_port_downstream_of_branch would indicate something may be wrong
with said validation.
It seems like it may have just been races and luck that made
drm_dp_mst_port_downstream_of_branch work however, as while I was trying to
figure out the true cause of this issue when removing the legacy MST code -
I discovered an important excerpt in section 2.14.2.3.3.6 of the DP 2.0
specs:
"BAD_PARAM - This reply is transmitted when a Message Transaction parameter
is in error; for example, the next port number is invalid or /no device is
connected/ to the port associated with the port number."
Sure enough - removing the calls to drm_dp_mst_port_downstream_of_branch()
and instead checking the ->ddps field of the parent port to see whether we
should release a given payload or not seems to totally fix the issue. This
does actually make sense to me, as it seems the implication is that given a
topology where an MSTB is removed, the payload for the MST parent's port
will be released automatically if that port is also marked as disconnected.
However, if there's another parent in the chain after that which is
connected - payloads must be released there with an ALLOCATE_PAYLOAD
message.
Signed-off-by: Lyude Paul <lyude@redhat.com>
---
drivers/gpu/drm/display/drm_dp_mst_topology.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index ead32aa23468d..5d1efad191a1c 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -2509,7 +2509,7 @@ drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb,
return ret;
}
-static void
+static int
drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
struct drm_dp_connection_status_notify *conn_stat)
{
@@ -2522,7 +2522,7 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
port = drm_dp_get_port(mstb, conn_stat->port_number);
if (!port)
- return;
+ return 0;
if (port->connector) {
if (!port->input && conn_stat->input_port) {
@@ -2575,8 +2575,7 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
out:
drm_dp_mst_topology_put_port(port);
- if (dowork)
- queue_work(system_long_wq, &mstb->mgr->work);
+ return dowork;
}
static struct drm_dp_mst_branch *drm_dp_get_mst_branch_device(struct drm_dp_mst_topology_mgr *mgr,
@@ -4073,7 +4072,7 @@ drm_dp_mst_process_up_req(struct drm_dp_mst_topology_mgr *mgr,
struct drm_dp_mst_branch *mstb = NULL;
struct drm_dp_sideband_msg_req_body *msg = &up_req->msg;
struct drm_dp_sideband_msg_hdr *hdr = &up_req->hdr;
- bool hotplug = false;
+ bool hotplug = false, dowork = false;
if (hdr->broadcast) {
const u8 *guid = NULL;
@@ -4096,11 +4095,14 @@ drm_dp_mst_process_up_req(struct drm_dp_mst_topology_mgr *mgr,
/* TODO: Add missing handler for DP_RESOURCE_STATUS_NOTIFY events */
if (msg->req_type == DP_CONNECTION_STATUS_NOTIFY) {
- drm_dp_mst_handle_conn_stat(mstb, &msg->u.conn_stat);
+ dowork = drm_dp_mst_handle_conn_stat(mstb, &msg->u.conn_stat);
hotplug = true;
}
drm_dp_mst_topology_put_mstb(mstb);
+
+ if (dowork)
+ queue_work(system_long_wq, &mgr->work);
return hotplug;
}
--
GitLab