Lyude Paul
2019-Oct-22 02:35 UTC
[Nouveau] [PATCH v5 01/14] drm/dp_mst: Destroy MSTBs asynchronously
When reprobing an MST topology during resume, we have to account for the
fact that while we were suspended it's possible that mstbs may have been
removed from any ports in the topology. Since iterating downwards in the
topology requires that we hold &mgr->lock, destroying MSTBs from this
context would result in attempting to lock &mgr->lock a second time and
deadlocking.
So, fix this by first moving destruction of MSTBs into
destroy_connector_work, then rename destroy_connector_work and friends
to reflect that they now destroy both ports and mstbs.
Note that even though this means that MSTBs will still be accessible for
a short period of time between their removal from the topology and
delayed destruction, we are still protected against referencing a MSTB
with a refcount of 0 since we use kref_get_unless_zero() in most places.
Changes since v1:
* s/destroy_connector_list/destroy_port_list/
s/connector_destroy_lock/delayed_destroy_lock/
s/connector_destroy_work/delayed_destroy_work/
s/drm_dp_finish_destroy_branch_device/drm_dp_delayed_destroy_mstb/
s/drm_dp_finish_destroy_port/drm_dp_delayed_destroy_port/
- danvet
* Use two loops in drm_dp_delayed_destroy_work() - danvet
* Better explain why we need to do this - danvet
* Use cancel_work_sync() instead of flush_work() - flush_work() doesn't
account for work requeing
Cc: Juston Li <juston.li at intel.com>
Cc: Imre Deak <imre.deak at intel.com>
Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
Cc: Harry Wentland <hwentlan at amd.com>
Cc: Daniel Vetter <daniel at ffwll.ch>
Signed-off-by: Lyude Paul <lyude at redhat.com>
Reviewed-by: Sean Paul <sean at poorly.run>
---
drivers/gpu/drm/drm_dp_mst_topology.c | 164 +++++++++++++++++---------
include/drm/drm_dp_mst_helper.h | 26 ++--
2 files changed, 128 insertions(+), 62 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
b/drivers/gpu/drm/drm_dp_mst_topology.c
index 9cccc5e63309..66ff226d8c86 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -1398,34 +1398,17 @@ static void drm_dp_destroy_mst_branch_device(struct kref
*kref)
struct drm_dp_mst_branch *mstb container_of(kref, struct drm_dp_mst_branch,
topology_kref);
struct drm_dp_mst_topology_mgr *mgr = mstb->mgr;
- struct drm_dp_mst_port *port, *tmp;
- bool wake_tx = false;
- mutex_lock(&mgr->lock);
- list_for_each_entry_safe(port, tmp, &mstb->ports, next) {
- list_del(&port->next);
- drm_dp_mst_topology_put_port(port);
- }
- mutex_unlock(&mgr->lock);
-
- /* drop any tx slots msg */
- mutex_lock(&mstb->mgr->qlock);
- if (mstb->tx_slots[0]) {
- mstb->tx_slots[0]->state = DRM_DP_SIDEBAND_TX_TIMEOUT;
- mstb->tx_slots[0] = NULL;
- wake_tx = true;
- }
- if (mstb->tx_slots[1]) {
- mstb->tx_slots[1]->state = DRM_DP_SIDEBAND_TX_TIMEOUT;
- mstb->tx_slots[1] = NULL;
- wake_tx = true;
- }
- mutex_unlock(&mstb->mgr->qlock);
+ INIT_LIST_HEAD(&mstb->destroy_next);
- if (wake_tx)
- wake_up_all(&mstb->mgr->tx_waitq);
-
- drm_dp_mst_put_mstb_malloc(mstb);
+ /*
+ * This can get called under mgr->mutex, so we need to perform the
+ * actual destruction of the mstb in another worker
+ */
+ mutex_lock(&mgr->delayed_destroy_lock);
+ list_add(&mstb->destroy_next, &mgr->destroy_branch_device_list);
+ mutex_unlock(&mgr->delayed_destroy_lock);
+ schedule_work(&mgr->delayed_destroy_work);
}
/**
@@ -1540,10 +1523,10 @@ static void drm_dp_destroy_port(struct kref *kref)
* we might be holding the mode_config.mutex
* from an EDID retrieval */
- mutex_lock(&mgr->destroy_connector_lock);
- list_add(&port->next, &mgr->destroy_connector_list);
- mutex_unlock(&mgr->destroy_connector_lock);
- schedule_work(&mgr->destroy_connector_work);
+ mutex_lock(&mgr->delayed_destroy_lock);
+ list_add(&port->next, &mgr->destroy_port_list);
+ mutex_unlock(&mgr->delayed_destroy_lock);
+ schedule_work(&mgr->delayed_destroy_work);
return;
}
/* no need to clean up vcpi
@@ -3085,7 +3068,7 @@ void drm_dp_mst_topology_mgr_suspend(struct
drm_dp_mst_topology_mgr *mgr)
DP_MST_EN | DP_UPSTREAM_IS_SRC);
mutex_unlock(&mgr->lock);
flush_work(&mgr->work);
- flush_work(&mgr->destroy_connector_work);
+ flush_work(&mgr->delayed_destroy_work);
}
EXPORT_SYMBOL(drm_dp_mst_topology_mgr_suspend);
@@ -3995,34 +3978,104 @@ static void drm_dp_tx_work(struct work_struct *work)
mutex_unlock(&mgr->qlock);
}
-static void drm_dp_destroy_connector_work(struct work_struct *work)
+static inline void
+drm_dp_delayed_destroy_port(struct drm_dp_mst_port *port)
{
- struct drm_dp_mst_topology_mgr *mgr = container_of(work, struct
drm_dp_mst_topology_mgr, destroy_connector_work);
- struct drm_dp_mst_port *port;
- bool send_hotplug = false;
+ port->mgr->cbs->destroy_connector(port->mgr, port->connector);
+
+ drm_dp_port_teardown_pdt(port, port->pdt);
+ port->pdt = DP_PEER_DEVICE_NONE;
+
+ drm_dp_mst_put_port_malloc(port);
+}
+
+static inline void
+drm_dp_delayed_destroy_mstb(struct drm_dp_mst_branch *mstb)
+{
+ struct drm_dp_mst_topology_mgr *mgr = mstb->mgr;
+ struct drm_dp_mst_port *port, *tmp;
+ bool wake_tx = false;
+
+ mutex_lock(&mgr->lock);
+ list_for_each_entry_safe(port, tmp, &mstb->ports, next) {
+ list_del(&port->next);
+ drm_dp_mst_topology_put_port(port);
+ }
+ mutex_unlock(&mgr->lock);
+
+ /* drop any tx slots msg */
+ mutex_lock(&mstb->mgr->qlock);
+ if (mstb->tx_slots[0]) {
+ mstb->tx_slots[0]->state = DRM_DP_SIDEBAND_TX_TIMEOUT;
+ mstb->tx_slots[0] = NULL;
+ wake_tx = true;
+ }
+ if (mstb->tx_slots[1]) {
+ mstb->tx_slots[1]->state = DRM_DP_SIDEBAND_TX_TIMEOUT;
+ mstb->tx_slots[1] = NULL;
+ wake_tx = true;
+ }
+ mutex_unlock(&mstb->mgr->qlock);
+
+ if (wake_tx)
+ wake_up_all(&mstb->mgr->tx_waitq);
+
+ drm_dp_mst_put_mstb_malloc(mstb);
+}
+
+static void drm_dp_delayed_destroy_work(struct work_struct *work)
+{
+ struct drm_dp_mst_topology_mgr *mgr + container_of(work, struct
drm_dp_mst_topology_mgr,
+ delayed_destroy_work);
+ bool send_hotplug = false, go_again;
+
/*
* Not a regular list traverse as we have to drop the destroy
- * connector lock before destroying the connector, to avoid AB->BA
+ * connector lock before destroying the mstb/port, to avoid AB->BA
* ordering between this lock and the config mutex.
*/
- for (;;) {
- mutex_lock(&mgr->destroy_connector_lock);
- port = list_first_entry_or_null(&mgr->destroy_connector_list, struct
drm_dp_mst_port, next);
- if (!port) {
- mutex_unlock(&mgr->destroy_connector_lock);
- break;
+ do {
+ go_again = false;
+
+ for (;;) {
+ struct drm_dp_mst_branch *mstb;
+
+ mutex_lock(&mgr->delayed_destroy_lock);
+ mstb = list_first_entry_or_null(&mgr->destroy_branch_device_list,
+ struct drm_dp_mst_branch,
+ destroy_next);
+ if (mstb)
+ list_del(&mstb->destroy_next);
+ mutex_unlock(&mgr->delayed_destroy_lock);
+
+ if (!mstb)
+ break;
+
+ drm_dp_delayed_destroy_mstb(mstb);
+ go_again = true;
}
- list_del(&port->next);
- mutex_unlock(&mgr->destroy_connector_lock);
- mgr->cbs->destroy_connector(mgr, port->connector);
+ for (;;) {
+ struct drm_dp_mst_port *port;
- drm_dp_port_teardown_pdt(port, port->pdt);
- port->pdt = DP_PEER_DEVICE_NONE;
+ mutex_lock(&mgr->delayed_destroy_lock);
+ port = list_first_entry_or_null(&mgr->destroy_port_list,
+ struct drm_dp_mst_port,
+ next);
+ if (port)
+ list_del(&port->next);
+ mutex_unlock(&mgr->delayed_destroy_lock);
+
+ if (!port)
+ break;
+
+ drm_dp_delayed_destroy_port(port);
+ send_hotplug = true;
+ go_again = true;
+ }
+ } while (go_again);
- drm_dp_mst_put_port_malloc(port);
- send_hotplug = true;
- }
if (send_hotplug)
drm_kms_helper_hotplug_event(mgr->dev);
}
@@ -4209,12 +4262,13 @@ int drm_dp_mst_topology_mgr_init(struct
drm_dp_mst_topology_mgr *mgr,
mutex_init(&mgr->lock);
mutex_init(&mgr->qlock);
mutex_init(&mgr->payload_lock);
- mutex_init(&mgr->destroy_connector_lock);
+ mutex_init(&mgr->delayed_destroy_lock);
INIT_LIST_HEAD(&mgr->tx_msg_downq);
- INIT_LIST_HEAD(&mgr->destroy_connector_list);
+ INIT_LIST_HEAD(&mgr->destroy_port_list);
+ INIT_LIST_HEAD(&mgr->destroy_branch_device_list);
INIT_WORK(&mgr->work, drm_dp_mst_link_probe_work);
INIT_WORK(&mgr->tx_work, drm_dp_tx_work);
- INIT_WORK(&mgr->destroy_connector_work, drm_dp_destroy_connector_work);
+ INIT_WORK(&mgr->delayed_destroy_work, drm_dp_delayed_destroy_work);
init_waitqueue_head(&mgr->tx_waitq);
mgr->dev = dev;
mgr->aux = aux;
@@ -4255,7 +4309,7 @@ void drm_dp_mst_topology_mgr_destroy(struct
drm_dp_mst_topology_mgr *mgr)
{
drm_dp_mst_topology_mgr_set_mst(mgr, false);
flush_work(&mgr->work);
- flush_work(&mgr->destroy_connector_work);
+ cancel_work_sync(&mgr->delayed_destroy_work);
mutex_lock(&mgr->payload_lock);
kfree(mgr->payloads);
mgr->payloads = NULL;
@@ -4267,7 +4321,7 @@ void drm_dp_mst_topology_mgr_destroy(struct
drm_dp_mst_topology_mgr *mgr)
drm_atomic_private_obj_fini(&mgr->base);
mgr->funcs = NULL;
- mutex_destroy(&mgr->destroy_connector_lock);
+ mutex_destroy(&mgr->delayed_destroy_lock);
mutex_destroy(&mgr->payload_lock);
mutex_destroy(&mgr->qlock);
mutex_destroy(&mgr->lock);
diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
index 4a25e0577ae0..b2160c366fb7 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -143,6 +143,12 @@ struct drm_dp_mst_branch {
*/
struct kref malloc_kref;
+ /**
+ * @destroy_next: linked-list entry used by
+ * drm_dp_delayed_destroy_work()
+ */
+ struct list_head destroy_next;
+
u8 rad[8];
u8 lct;
int num_ports;
@@ -571,18 +577,24 @@ struct drm_dp_mst_topology_mgr {
struct work_struct tx_work;
/**
- * @destroy_connector_list: List of to be destroyed connectors.
+ * @destroy_port_list: List of to be destroyed connectors.
+ */
+ struct list_head destroy_port_list;
+ /**
+ * @destroy_branch_device_list: List of to be destroyed branch
+ * devices.
*/
- struct list_head destroy_connector_list;
+ struct list_head destroy_branch_device_list;
/**
- * @destroy_connector_lock: Protects @connector_list.
+ * @delayed_destroy_lock: Protects @destroy_port_list and
+ * @destroy_branch_device_list.
*/
- struct mutex destroy_connector_lock;
+ struct mutex delayed_destroy_lock;
/**
- * @destroy_connector_work: Work item to destroy connectors. Needed to
- * avoid locking inversion.
+ * @delayed_destroy_work: Work item to destroy MST port and branch
+ * devices, needed to avoid locking inversion.
*/
- struct work_struct destroy_connector_work;
+ struct work_struct delayed_destroy_work;
};
int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,
--
2.21.0
Lyude Paul
2019-Oct-22 02:35 UTC
[Nouveau] [PATCH v5 02/14] drm/dp_mst: Remove PDT teardown in drm_dp_destroy_port() and refactor
This will allow us to add some locking for port->* members, in
particular the PDT and ->connector, which can't be done from
drm_dp_destroy_port() since we don't know what locks the caller might be
holding.
Note that we already do this in delayed_destroy_work (renamed from
destroy_connector_work in this patch) for ports, we're just making it so
mstbs are also destroyed in this worker.
Changes since v2:
* Clarify commit message
Changes since v4:
* Clarify commit message more
Cc: Juston Li <juston.li at intel.com>
Cc: Imre Deak <imre.deak at intel.com>
Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
Cc: Harry Wentland <hwentlan at amd.com>
Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
Reviewed-by: Sean Paul <sean at poorly.run>
Signed-off-by: Lyude Paul <lyude at redhat.com>
---
drivers/gpu/drm/drm_dp_mst_topology.c | 40 +++++++++++----------------
1 file changed, 16 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
b/drivers/gpu/drm/drm_dp_mst_topology.c
index 66ff226d8c86..204d0c832c65 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -1510,31 +1510,22 @@ static void drm_dp_destroy_port(struct kref *kref)
container_of(kref, struct drm_dp_mst_port, topology_kref);
struct drm_dp_mst_topology_mgr *mgr = port->mgr;
- if (!port->input) {
- kfree(port->cached_edid);
+ /* There's nothing that needs locking to destroy an input port yet */
+ if (port->input) {
+ drm_dp_mst_put_port_malloc(port);
+ return;
+ }
- /*
- * The only time we don't have a connector
- * on an output port is if the connector init
- * fails.
- */
- if (port->connector) {
- /* we can't destroy the connector here, as
- * we might be holding the mode_config.mutex
- * from an EDID retrieval */
+ kfree(port->cached_edid);
- mutex_lock(&mgr->delayed_destroy_lock);
- list_add(&port->next, &mgr->destroy_port_list);
- mutex_unlock(&mgr->delayed_destroy_lock);
- schedule_work(&mgr->delayed_destroy_work);
- return;
- }
- /* no need to clean up vcpi
- * as if we have no connector we never setup a vcpi */
- drm_dp_port_teardown_pdt(port, port->pdt);
- port->pdt = DP_PEER_DEVICE_NONE;
- }
- drm_dp_mst_put_port_malloc(port);
+ /*
+ * we can't destroy the connector here, as we might be holding the
+ * mode_config.mutex from an EDID retrieval
+ */
+ mutex_lock(&mgr->delayed_destroy_lock);
+ list_add(&port->next, &mgr->destroy_port_list);
+ mutex_unlock(&mgr->delayed_destroy_lock);
+ schedule_work(&mgr->delayed_destroy_work);
}
/**
@@ -3981,7 +3972,8 @@ static void drm_dp_tx_work(struct work_struct *work)
static inline void
drm_dp_delayed_destroy_port(struct drm_dp_mst_port *port)
{
- port->mgr->cbs->destroy_connector(port->mgr, port->connector);
+ if (port->connector)
+ port->mgr->cbs->destroy_connector(port->mgr, port->connector);
drm_dp_port_teardown_pdt(port, port->pdt);
port->pdt = DP_PEER_DEVICE_NONE;
--
2.21.0
Lyude Paul
2019-Oct-22 02:35 UTC
[Nouveau] [PATCH v5 03/14] drm/dp_mst: Refactor pdt setup/teardown, add more locking
Since we're going to be implementing suspend/resume reprobing very soon,
we need to make sure we are extra careful to ensure that our locking
actually protects the topology state where we expect it to. Turns out
this isn't the case with drm_dp_port_setup_pdt() and
drm_dp_port_teardown_pdt(), both of which change port->mstb without
grabbing &mgr->lock.
Additionally, since most callers of these functions are just using it to
teardown the port's previous PDT and setup a new one we can simplify
things a bit and combine drm_dp_port_setup_pdt() and
drm_dp_port_teardown_pdt() into a single function:
drm_dp_port_set_pdt(). This function also handles actually ensuring that
we grab the correct locks when we need to modify port->mstb.
Cc: Juston Li <juston.li at intel.com>
Cc: Imre Deak <imre.deak at intel.com>
Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
Cc: Harry Wentland <hwentlan at amd.com>
Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
Signed-off-by: Lyude Paul <lyude at redhat.com>
Reviewed-by: Sean Paul <sean at poorly.run>
---
drivers/gpu/drm/drm_dp_mst_topology.c | 181 +++++++++++++++-----------
include/drm/drm_dp_mst_helper.h | 6 +-
2 files changed, 110 insertions(+), 77 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
b/drivers/gpu/drm/drm_dp_mst_topology.c
index 204d0c832c65..3f16c0cb094b 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -1486,24 +1486,6 @@ drm_dp_mst_topology_put_mstb(struct drm_dp_mst_branch
*mstb)
kref_put(&mstb->topology_kref, drm_dp_destroy_mst_branch_device);
}
-static void drm_dp_port_teardown_pdt(struct drm_dp_mst_port *port, int old_pdt)
-{
- struct drm_dp_mst_branch *mstb;
-
- switch (old_pdt) {
- case DP_PEER_DEVICE_DP_LEGACY_CONV:
- case DP_PEER_DEVICE_SST_SINK:
- /* remove i2c over sideband */
- drm_dp_mst_unregister_i2c_bus(&port->aux);
- break;
- case DP_PEER_DEVICE_MST_BRANCHING:
- mstb = port->mstb;
- port->mstb = NULL;
- drm_dp_mst_topology_put_mstb(mstb);
- break;
- }
-}
-
static void drm_dp_destroy_port(struct kref *kref)
{
struct drm_dp_mst_port *port @@ -1713,38 +1695,79 @@ static u8
drm_dp_calculate_rad(struct drm_dp_mst_port *port,
return parent_lct + 1;
}
-/*
- * return sends link address for new mstb
- */
-static bool drm_dp_port_setup_pdt(struct drm_dp_mst_port *port)
+static int drm_dp_port_set_pdt(struct drm_dp_mst_port *port, u8 new_pdt)
{
- int ret;
- u8 rad[6], lct;
- bool send_link = false;
+ struct drm_dp_mst_topology_mgr *mgr = port->mgr;
+ struct drm_dp_mst_branch *mstb;
+ u8 rad[8], lct;
+ int ret = 0;
+
+ if (port->pdt == new_pdt)
+ return 0;
+
+ /* Teardown the old pdt, if there is one */
+ switch (port->pdt) {
+ case DP_PEER_DEVICE_DP_LEGACY_CONV:
+ case DP_PEER_DEVICE_SST_SINK:
+ /*
+ * If the new PDT would also have an i2c bus, don't bother
+ * with reregistering it
+ */
+ if (new_pdt == DP_PEER_DEVICE_DP_LEGACY_CONV ||
+ new_pdt == DP_PEER_DEVICE_SST_SINK) {
+ port->pdt = new_pdt;
+ return 0;
+ }
+
+ /* remove i2c over sideband */
+ drm_dp_mst_unregister_i2c_bus(&port->aux);
+ break;
+ case DP_PEER_DEVICE_MST_BRANCHING:
+ mutex_lock(&mgr->lock);
+ drm_dp_mst_topology_put_mstb(port->mstb);
+ port->mstb = NULL;
+ mutex_unlock(&mgr->lock);
+ break;
+ }
+
+ port->pdt = new_pdt;
switch (port->pdt) {
case DP_PEER_DEVICE_DP_LEGACY_CONV:
case DP_PEER_DEVICE_SST_SINK:
/* add i2c over sideband */
ret = drm_dp_mst_register_i2c_bus(&port->aux);
break;
+
case DP_PEER_DEVICE_MST_BRANCHING:
lct = drm_dp_calculate_rad(port, rad);
+ mstb = drm_dp_add_mst_branch_device(lct, rad);
+ if (!mstb) {
+ ret = -ENOMEM;
+ DRM_ERROR("Failed to create MSTB for port %p", port);
+ goto out;
+ }
- port->mstb = drm_dp_add_mst_branch_device(lct, rad);
- if (port->mstb) {
- port->mstb->mgr = port->mgr;
- port->mstb->port_parent = port;
- /*
- * Make sure this port's memory allocation stays
- * around until its child MSTB releases it
- */
- drm_dp_mst_get_port_malloc(port);
+ mutex_lock(&mgr->lock);
+ port->mstb = mstb;
+ mstb->mgr = port->mgr;
+ mstb->port_parent = port;
- send_link = true;
- }
+ /*
+ * Make sure this port's memory allocation stays
+ * around until its child MSTB releases it
+ */
+ drm_dp_mst_get_port_malloc(port);
+ mutex_unlock(&mgr->lock);
+
+ /* And make sure we send a link address for this */
+ ret = 1;
break;
}
- return send_link;
+
+out:
+ if (ret < 0)
+ port->pdt = DP_PEER_DEVICE_NONE;
+ return ret;
}
/**
@@ -1881,10 +1904,9 @@ drm_dp_mst_handle_link_address_port(struct
drm_dp_mst_branch *mstb,
struct drm_device *dev,
struct drm_dp_link_addr_reply_port *port_msg)
{
+ struct drm_dp_mst_topology_mgr *mgr = mstb->mgr;
struct drm_dp_mst_port *port;
- bool ret;
bool created = false;
- int old_pdt = 0;
int old_ddps = 0;
port = drm_dp_get_port(mstb, port_msg->port_number);
@@ -1896,7 +1918,7 @@ drm_dp_mst_handle_link_address_port(struct
drm_dp_mst_branch *mstb,
kref_init(&port->malloc_kref);
port->parent = mstb;
port->port_num = port_msg->port_number;
- port->mgr = mstb->mgr;
+ port->mgr = mgr;
port->aux.name = "DPMST";
port->aux.dev = dev->dev;
port->aux.is_remote = true;
@@ -1909,11 +1931,9 @@ drm_dp_mst_handle_link_address_port(struct
drm_dp_mst_branch *mstb,
created = true;
} else {
- old_pdt = port->pdt;
old_ddps = port->ddps;
}
- port->pdt = port_msg->peer_device_type;
port->input = port_msg->input_port;
port->mcs = port_msg->mcs;
port->ddps = port_msg->ddps;
@@ -1925,29 +1945,33 @@ drm_dp_mst_handle_link_address_port(struct
drm_dp_mst_branch *mstb,
/* manage mstb port lists with mgr lock - take a reference
for this list */
if (created) {
- mutex_lock(&mstb->mgr->lock);
+ mutex_lock(&mgr->lock);
drm_dp_mst_topology_get_port(port);
list_add(&port->next, &mstb->ports);
- mutex_unlock(&mstb->mgr->lock);
+ mutex_unlock(&mgr->lock);
}
if (old_ddps != port->ddps) {
if (port->ddps) {
if (!port->input) {
- drm_dp_send_enum_path_resources(mstb->mgr,
- mstb, port);
+ drm_dp_send_enum_path_resources(mgr, mstb,
+ port);
}
} else {
port->available_pbn = 0;
}
}
- if (old_pdt != port->pdt && !port->input) {
- drm_dp_port_teardown_pdt(port, old_pdt);
-
- ret = drm_dp_port_setup_pdt(port);
- if (ret == true)
- drm_dp_send_link_address(mstb->mgr, port->mstb);
+ if (!port->input) {
+ int ret = drm_dp_port_set_pdt(port,
+ port_msg->peer_device_type);
+ if (ret == 1) {
+ drm_dp_send_link_address(mgr, port->mstb);
+ } else if (ret < 0) {
+ DRM_ERROR("Failed to change PDT on port %p: %d\n",
+ port, ret);
+ goto fail;
+ }
}
if (created && !port->input) {
@@ -1955,18 +1979,11 @@ drm_dp_mst_handle_link_address_port(struct
drm_dp_mst_branch *mstb,
build_mst_prop_path(mstb, port->port_num, proppath,
sizeof(proppath));
- port->connector = (*mstb->mgr->cbs->add_connector)(mstb->mgr,
- port,
- proppath);
- if (!port->connector) {
- /* remove it from the port list */
- mutex_lock(&mstb->mgr->lock);
- list_del(&port->next);
- mutex_unlock(&mstb->mgr->lock);
- /* drop port list reference */
- drm_dp_mst_topology_put_port(port);
- goto out;
- }
+ port->connector = (*mgr->cbs->add_connector)(mgr, port,
+ proppath);
+ if (!port->connector)
+ goto fail;
+
if ((port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV ||
port->pdt == DP_PEER_DEVICE_SST_SINK) &&
port->port_num >= DP_MST_LOGICAL_PORT_0) {
@@ -1974,12 +1991,24 @@ drm_dp_mst_handle_link_address_port(struct
drm_dp_mst_branch *mstb,
&port->aux.ddc);
drm_connector_set_tile_property(port->connector);
}
- (*mstb->mgr->cbs->register_connector)(port->connector);
+
+ (*mgr->cbs->register_connector)(port->connector);
}
-out:
/* put reference to this port */
drm_dp_mst_topology_put_port(port);
+ return;
+
+fail:
+ /* Remove it from the port list */
+ mutex_lock(&mgr->lock);
+ list_del(&port->next);
+ mutex_unlock(&mgr->lock);
+
+ /* Drop the port list reference */
+ drm_dp_mst_topology_put_port(port);
+ /* And now drop our reference */
+ drm_dp_mst_topology_put_port(port);
}
static void
@@ -1987,16 +2016,14 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch
*mstb,
struct drm_dp_connection_status_notify *conn_stat)
{
struct drm_dp_mst_port *port;
- int old_pdt;
int old_ddps;
bool dowork = false;
+
port = drm_dp_get_port(mstb, conn_stat->port_number);
if (!port)
return;
old_ddps = port->ddps;
- old_pdt = port->pdt;
- port->pdt = conn_stat->peer_device_type;
port->mcs = conn_stat->message_capability_status;
port->ldps = conn_stat->legacy_device_plug_status;
port->ddps = conn_stat->displayport_device_plug_status;
@@ -2008,11 +2035,17 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch
*mstb,
port->available_pbn = 0;
}
}
- if (old_pdt != port->pdt && !port->input) {
- drm_dp_port_teardown_pdt(port, old_pdt);
- if (drm_dp_port_setup_pdt(port))
+ if (!port->input) {
+ int ret = drm_dp_port_set_pdt(port,
+ conn_stat->peer_device_type);
+ if (ret == 1) {
dowork = true;
+ } else if (ret < 0) {
+ DRM_ERROR("Failed to change PDT for port %p: %d\n",
+ port, ret);
+ dowork = false;
+ }
}
drm_dp_mst_topology_put_port(port);
@@ -3975,9 +4008,7 @@ drm_dp_delayed_destroy_port(struct drm_dp_mst_port *port)
if (port->connector)
port->mgr->cbs->destroy_connector(port->mgr, port->connector);
- drm_dp_port_teardown_pdt(port, port->pdt);
- port->pdt = DP_PEER_DEVICE_NONE;
-
+ drm_dp_port_set_pdt(port, DP_PEER_DEVICE_NONE);
drm_dp_mst_put_port_malloc(port);
}
diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
index b2160c366fb7..8ba2a01324bb 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -55,8 +55,10 @@ struct drm_dp_vcpi {
* @num_sdp_stream_sinks: Number of stream sinks
* @available_pbn: Available bandwidth for this port.
* @next: link to next port on this branch device
- * @mstb: branch device attach below this port
- * @aux: i2c aux transport to talk to device connected to this port.
+ * @mstb: branch device on this port, protected by
+ * &drm_dp_mst_topology_mgr.lock
+ * @aux: i2c aux transport to talk to device connected to this port, protected
+ * by &drm_dp_mst_topology_mgr.lock
* @parent: branch device parent of this port
* @vcpi: Virtual Channel Payload info for this port.
* @connector: DRM connector this port is connected to.
--
2.21.0
Lyude Paul
2019-Oct-22 02:35 UTC
[Nouveau] [PATCH v5 04/14] drm/dp_mst: Handle UP requests asynchronously
Once upon a time, hotplugging devices on MST branches actually worked in
DRM. Now, it only works in amdgpu (likely because of how it's hotplug
handlers are implemented). On both i915 and nouveau, hotplug
notifications from MST branches are noticed - but trying to respond to
them causes messaging timeouts and causes the whole topology state to go
out of sync with reality, usually resulting in the user needing to
replug the entire topology in hopes that it actually fixes things.
The reason for this is because the way we currently handle UP requests
in MST is completely bogus. drm_dp_mst_handle_up_req() is called from
drm_dp_mst_hpd_irq(), which is usually called from the driver's hotplug
handler. Because we handle sending the hotplug event from this function,
we actually cause the driver's hotplug handler (and in turn, all
sideband transactions) to block on
drm_device->mode_config.connection_mutex. This makes it impossible to
send any sideband messages from the driver's connector probing
functions, resulting in the aforementioned sideband message timeout.
There's even more problems with this beyond breaking hotplugging on MST
branch devices. It also makes it almost impossible to protect
drm_dp_mst_port struct members under a lock because we then have to
worry about dealing with all of the lock dependency issues that ensue.
So, let's finally actually fix this issue by handling the processing of
up requests asyncronously. This way we can send sideband messages from
most contexts without having to deal with getting blocked if we hold
connection_mutex. This also fixes MST branch device hotplugging on i915,
finally!
Cc: Juston Li <juston.li at intel.com>
Cc: Imre Deak <imre.deak at intel.com>
Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
Cc: Harry Wentland <hwentlan at amd.com>
Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
Signed-off-by: Lyude Paul <lyude at redhat.com>
Reviewed-by: Sean Paul <sean at poorly.run>
---
drivers/gpu/drm/drm_dp_mst_topology.c | 146 +++++++++++++++++++-------
include/drm/drm_dp_mst_helper.h | 16 +++
2 files changed, 122 insertions(+), 40 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
b/drivers/gpu/drm/drm_dp_mst_topology.c
index 3f16c0cb094b..08c316a727df 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -45,6 +45,12 @@
* protocol. The helpers contain a topology manager and bandwidth manager.
* The helpers encapsulate the sending and received of sideband msgs.
*/
+struct drm_dp_pending_up_req {
+ struct drm_dp_sideband_msg_hdr hdr;
+ struct drm_dp_sideband_msg_req_body msg;
+ struct list_head next;
+};
+
static bool dump_dp_payload_table(struct drm_dp_mst_topology_mgr *mgr,
char *buf);
@@ -3091,6 +3097,7 @@ void drm_dp_mst_topology_mgr_suspend(struct
drm_dp_mst_topology_mgr *mgr)
drm_dp_dpcd_writeb(mgr->aux, DP_MSTM_CTRL,
DP_MST_EN | DP_UPSTREAM_IS_SRC);
mutex_unlock(&mgr->lock);
+ flush_work(&mgr->up_req_work);
flush_work(&mgr->work);
flush_work(&mgr->delayed_destroy_work);
}
@@ -3263,12 +3270,70 @@ static int drm_dp_mst_handle_down_rep(struct
drm_dp_mst_topology_mgr *mgr)
return 0;
}
+static inline void
+drm_dp_mst_process_up_req(struct drm_dp_mst_topology_mgr *mgr,
+ struct drm_dp_pending_up_req *up_req)
+{
+ 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;
+
+ if (hdr->broadcast) {
+ const u8 *guid = NULL;
+
+ if (msg->req_type == DP_CONNECTION_STATUS_NOTIFY)
+ guid = msg->u.conn_stat.guid;
+ else if (msg->req_type == DP_RESOURCE_STATUS_NOTIFY)
+ guid = msg->u.resource_stat.guid;
+
+ mstb = drm_dp_get_mst_branch_device_by_guid(mgr, guid);
+ } else {
+ mstb = drm_dp_get_mst_branch_device(mgr, hdr->lct, hdr->rad);
+ }
+
+ if (!mstb) {
+ DRM_DEBUG_KMS("Got MST reply from unknown device %d\n",
+ hdr->lct);
+ return;
+ }
+
+ /* 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);
+ drm_kms_helper_hotplug_event(mgr->dev);
+ }
+
+ drm_dp_mst_topology_put_mstb(mstb);
+}
+
+static void drm_dp_mst_up_req_work(struct work_struct *work)
+{
+ struct drm_dp_mst_topology_mgr *mgr + container_of(work, struct
drm_dp_mst_topology_mgr,
+ up_req_work);
+ struct drm_dp_pending_up_req *up_req;
+
+ while (true) {
+ mutex_lock(&mgr->up_req_lock);
+ up_req = list_first_entry_or_null(&mgr->up_req_list,
+ struct drm_dp_pending_up_req,
+ next);
+ if (up_req)
+ list_del(&up_req->next);
+ mutex_unlock(&mgr->up_req_lock);
+
+ if (!up_req)
+ break;
+
+ drm_dp_mst_process_up_req(mgr, up_req);
+ kfree(up_req);
+ }
+}
+
static int drm_dp_mst_handle_up_req(struct drm_dp_mst_topology_mgr *mgr)
{
- struct drm_dp_sideband_msg_req_body msg;
struct drm_dp_sideband_msg_hdr *hdr = &mgr->up_req_recv.initial_hdr;
- struct drm_dp_mst_branch *mstb = NULL;
- const u8 *guid;
+ struct drm_dp_pending_up_req *up_req;
bool seqno;
if (!drm_dp_get_one_sb_msg(mgr, true))
@@ -3277,56 +3342,53 @@ static int drm_dp_mst_handle_up_req(struct
drm_dp_mst_topology_mgr *mgr)
if (!mgr->up_req_recv.have_eomt)
return 0;
- if (!hdr->broadcast) {
- mstb = drm_dp_get_mst_branch_device(mgr, hdr->lct, hdr->rad);
- if (!mstb) {
- DRM_DEBUG_KMS("Got MST reply from unknown device %d\n",
- hdr->lct);
- goto out;
- }
+ up_req = kzalloc(sizeof(*up_req), GFP_KERNEL);
+ if (!up_req) {
+ DRM_ERROR("Not enough memory to process MST up req\n");
+ return -ENOMEM;
}
+ INIT_LIST_HEAD(&up_req->next);
seqno = hdr->seqno;
- drm_dp_sideband_parse_req(&mgr->up_req_recv, &msg);
+ drm_dp_sideband_parse_req(&mgr->up_req_recv, &up_req->msg);
- if (msg.req_type == DP_CONNECTION_STATUS_NOTIFY)
- guid = msg.u.conn_stat.guid;
- else if (msg.req_type == DP_RESOURCE_STATUS_NOTIFY)
- guid = msg.u.resource_stat.guid;
- else
+ if (up_req->msg.req_type != DP_CONNECTION_STATUS_NOTIFY &&
+ up_req->msg.req_type != DP_RESOURCE_STATUS_NOTIFY) {
+ DRM_DEBUG_KMS("Received unknown up req type, ignoring: %x\n",
+ up_req->msg.req_type);
+ kfree(up_req);
goto out;
-
- drm_dp_send_up_ack_reply(mgr, mgr->mst_primary, msg.req_type, seqno,
- false);
-
- if (!mstb) {
- mstb = drm_dp_get_mst_branch_device_by_guid(mgr, guid);
- if (!mstb) {
- DRM_DEBUG_KMS("Got MST reply from unknown device %d\n",
- hdr->lct);
- goto out;
- }
}
- if (msg.req_type == DP_CONNECTION_STATUS_NOTIFY) {
- drm_dp_mst_handle_conn_stat(mstb, &msg.u.conn_stat);
+ drm_dp_send_up_ack_reply(mgr, mgr->mst_primary, up_req->msg.req_type,
+ seqno, false);
+
+ if (up_req->msg.req_type == DP_CONNECTION_STATUS_NOTIFY) {
+ const struct drm_dp_connection_status_notify *conn_stat +
&up_req->msg.u.conn_stat;
DRM_DEBUG_KMS("Got CSN: pn: %d ldps:%d ddps: %d mcs: %d ip: %d pdt:
%d\n",
- msg.u.conn_stat.port_number,
- msg.u.conn_stat.legacy_device_plug_status,
- msg.u.conn_stat.displayport_device_plug_status,
- msg.u.conn_stat.message_capability_status,
- msg.u.conn_stat.input_port,
- msg.u.conn_stat.peer_device_type);
+ conn_stat->port_number,
+ conn_stat->legacy_device_plug_status,
+ conn_stat->displayport_device_plug_status,
+ conn_stat->message_capability_status,
+ conn_stat->input_port,
+ conn_stat->peer_device_type);
+ } else if (up_req->msg.req_type == DP_RESOURCE_STATUS_NOTIFY) {
+ const struct drm_dp_resource_status_notify *res_stat +
&up_req->msg.u.resource_stat;
- drm_kms_helper_hotplug_event(mgr->dev);
- } else if (msg.req_type == DP_RESOURCE_STATUS_NOTIFY) {
DRM_DEBUG_KMS("Got RSN: pn: %d avail_pbn %d\n",
- msg.u.resource_stat.port_number,
- msg.u.resource_stat.available_pbn);
+ res_stat->port_number,
+ res_stat->available_pbn);
}
- drm_dp_mst_topology_put_mstb(mstb);
+ up_req->hdr = *hdr;
+ mutex_lock(&mgr->up_req_lock);
+ list_add_tail(&up_req->next, &mgr->up_req_list);
+ mutex_unlock(&mgr->up_req_lock);
+ queue_work(system_long_wq, &mgr->up_req_work);
+
out:
memset(&mgr->up_req_recv, 0, sizeof(struct drm_dp_sideband_msg_rx));
return 0;
@@ -4286,12 +4348,15 @@ int drm_dp_mst_topology_mgr_init(struct
drm_dp_mst_topology_mgr *mgr,
mutex_init(&mgr->qlock);
mutex_init(&mgr->payload_lock);
mutex_init(&mgr->delayed_destroy_lock);
+ mutex_init(&mgr->up_req_lock);
INIT_LIST_HEAD(&mgr->tx_msg_downq);
INIT_LIST_HEAD(&mgr->destroy_port_list);
INIT_LIST_HEAD(&mgr->destroy_branch_device_list);
+ INIT_LIST_HEAD(&mgr->up_req_list);
INIT_WORK(&mgr->work, drm_dp_mst_link_probe_work);
INIT_WORK(&mgr->tx_work, drm_dp_tx_work);
INIT_WORK(&mgr->delayed_destroy_work, drm_dp_delayed_destroy_work);
+ INIT_WORK(&mgr->up_req_work, drm_dp_mst_up_req_work);
init_waitqueue_head(&mgr->tx_waitq);
mgr->dev = dev;
mgr->aux = aux;
@@ -4348,6 +4413,7 @@ void drm_dp_mst_topology_mgr_destroy(struct
drm_dp_mst_topology_mgr *mgr)
mutex_destroy(&mgr->payload_lock);
mutex_destroy(&mgr->qlock);
mutex_destroy(&mgr->lock);
+ mutex_destroy(&mgr->up_req_lock);
}
EXPORT_SYMBOL(drm_dp_mst_topology_mgr_destroy);
diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
index 8ba2a01324bb..7d80c38ee00e 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -597,6 +597,22 @@ struct drm_dp_mst_topology_mgr {
* devices, needed to avoid locking inversion.
*/
struct work_struct delayed_destroy_work;
+
+ /**
+ * @up_req_list: List of pending up requests from the topology that
+ * need to be processed, in chronological order.
+ */
+ struct list_head up_req_list;
+ /**
+ * @up_req_lock: Protects @up_req_list
+ */
+ struct mutex up_req_lock;
+ /**
+ * @up_req_work: Work item to process up requests received from the
+ * topology. Needed to avoid blocking hotplug handling and sideband
+ * transmissions.
+ */
+ struct work_struct up_req_work;
};
int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,
--
2.21.0
Currently, MST lacks locking in a lot of places that really should have
some sort of locking. Hotplugging and link address code paths are some
of the offenders here, as there is actually nothing preventing us from
running a link address probe while at the same time handling a
connection status update request - something that's likely always been
possible but never seen in the wild because hotplugging has been broken
for ages now (with the exception of amdgpu, for reasons I don't think
are worth digging into very far).
Note: I'm going to start using the term "in-memory topology
layout" here
to refer to drm_dp_mst_port->mstb and drm_dp_mst_branch->ports.
Locking in these places is a little tougher then it looks though.
Generally we protect anything having to do with the in-memory topology
layout under &mgr->lock. But this becomes nearly impossible to do from
the context of link address probes due to the fact that &mgr->lock is
usually grabbed under random various modesetting locks, meaning that
there's no way we can just invert the &mgr->lock order and keep it
locked throughout the whole process of updating the topology.
Luckily there are only two workers which can modify the in-memory
topology layout: drm_dp_mst_up_req_work() and
drm_dp_mst_link_probe_work(), meaning as long as we prevent these two
workers from traveling the topology layout in parallel with the intent
of updating it we don't need to worry about grabbing &mgr->lock in
these
workers for reads. We only need to grab &mgr->lock in these workers for
writes, so that readers outside these two workers are still protected
from the topology layout changing beneath them.
So, add the new &mgr->probe_lock and use it in both
drm_dp_mst_link_probe_work() and drm_dp_mst_up_req_work(). Additionally,
add some more detailed explanations for how this locking is intended to
work to drm_dp_mst_port->mstb and drm_dp_mst_branch->ports.
Signed-off-by: Lyude Paul <lyude at redhat.com>
Cc: Juston Li <juston.li at intel.com>
Cc: Imre Deak <imre.deak at intel.com>
Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
Cc: Harry Wentland <hwentlan at amd.com>
Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
Cc: Sean Paul <sean at poorly.run>
---
drivers/gpu/drm/drm_dp_mst_topology.c | 28 ++++++++++++++---------
include/drm/drm_dp_mst_helper.h | 32 +++++++++++++++++++++++----
2 files changed, 46 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
b/drivers/gpu/drm/drm_dp_mst_topology.c
index 08c316a727df..11d842f0bff5 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -2147,37 +2147,40 @@ static void drm_dp_check_and_send_link_address(struct
drm_dp_mst_topology_mgr *m
struct drm_dp_mst_branch *mstb)
{
struct drm_dp_mst_port *port;
- struct drm_dp_mst_branch *mstb_child;
+
if (!mstb->link_address_sent)
drm_dp_send_link_address(mgr, mstb);
list_for_each_entry(port, &mstb->ports, next) {
- if (port->input)
- continue;
+ struct drm_dp_mst_branch *mstb_child = NULL;
- if (!port->ddps)
+ if (port->input || !port->ddps)
continue;
if (!port->available_pbn)
drm_dp_send_enum_path_resources(mgr, mstb, port);
- if (port->mstb) {
+ if (port->mstb)
mstb_child = drm_dp_mst_topology_get_mstb_validated(
mgr, port->mstb);
- if (mstb_child) {
- drm_dp_check_and_send_link_address(mgr, mstb_child);
- drm_dp_mst_topology_put_mstb(mstb_child);
- }
+
+ if (mstb_child) {
+ drm_dp_check_and_send_link_address(mgr, mstb_child);
+ drm_dp_mst_topology_put_mstb(mstb_child);
}
}
}
static void drm_dp_mst_link_probe_work(struct work_struct *work)
{
- struct drm_dp_mst_topology_mgr *mgr = container_of(work, struct
drm_dp_mst_topology_mgr, work);
+ struct drm_dp_mst_topology_mgr *mgr + container_of(work, struct
drm_dp_mst_topology_mgr, work);
+ struct drm_device *dev = mgr->dev;
struct drm_dp_mst_branch *mstb;
int ret;
+ mutex_lock(&mgr->probe_lock);
+
mutex_lock(&mgr->lock);
mstb = mgr->mst_primary;
if (mstb) {
@@ -2190,6 +2193,7 @@ static void drm_dp_mst_link_probe_work(struct work_struct
*work)
drm_dp_check_and_send_link_address(mgr, mstb);
drm_dp_mst_topology_put_mstb(mstb);
}
+ mutex_unlock(&mgr->probe_lock);
}
static bool drm_dp_validate_guid(struct drm_dp_mst_topology_mgr *mgr,
@@ -3313,6 +3317,7 @@ static void drm_dp_mst_up_req_work(struct work_struct
*work)
up_req_work);
struct drm_dp_pending_up_req *up_req;
+ mutex_lock(&mgr->probe_lock);
while (true) {
mutex_lock(&mgr->up_req_lock);
up_req = list_first_entry_or_null(&mgr->up_req_list,
@@ -3328,6 +3333,7 @@ static void drm_dp_mst_up_req_work(struct work_struct
*work)
drm_dp_mst_process_up_req(mgr, up_req);
kfree(up_req);
}
+ mutex_unlock(&mgr->probe_lock);
}
static int drm_dp_mst_handle_up_req(struct drm_dp_mst_topology_mgr *mgr)
@@ -4349,6 +4355,7 @@ int drm_dp_mst_topology_mgr_init(struct
drm_dp_mst_topology_mgr *mgr,
mutex_init(&mgr->payload_lock);
mutex_init(&mgr->delayed_destroy_lock);
mutex_init(&mgr->up_req_lock);
+ mutex_init(&mgr->probe_lock);
INIT_LIST_HEAD(&mgr->tx_msg_downq);
INIT_LIST_HEAD(&mgr->destroy_port_list);
INIT_LIST_HEAD(&mgr->destroy_branch_device_list);
@@ -4414,6 +4421,7 @@ void drm_dp_mst_topology_mgr_destroy(struct
drm_dp_mst_topology_mgr *mgr)
mutex_destroy(&mgr->qlock);
mutex_destroy(&mgr->lock);
mutex_destroy(&mgr->up_req_lock);
+ mutex_destroy(&mgr->probe_lock);
}
EXPORT_SYMBOL(drm_dp_mst_topology_mgr_destroy);
diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
index 7d80c38ee00e..bccb5514e0ef 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -55,8 +55,6 @@ struct drm_dp_vcpi {
* @num_sdp_stream_sinks: Number of stream sinks
* @available_pbn: Available bandwidth for this port.
* @next: link to next port on this branch device
- * @mstb: branch device on this port, protected by
- * &drm_dp_mst_topology_mgr.lock
* @aux: i2c aux transport to talk to device connected to this port, protected
* by &drm_dp_mst_topology_mgr.lock
* @parent: branch device parent of this port
@@ -92,7 +90,17 @@ struct drm_dp_mst_port {
u8 num_sdp_stream_sinks;
uint16_t available_pbn;
struct list_head next;
- struct drm_dp_mst_branch *mstb; /* pointer to an mstb if this port has one */
+ /**
+ * @mstb: the branch device connected to this port, if there is one.
+ * This should be considered protected for reading by
+ * &drm_dp_mst_topology_mgr.lock. There are two exceptions to this:
+ * &drm_dp_mst_topology_mgr.up_req_work and
+ * &drm_dp_mst_topology_mgr.work, which do not grab
+ * &drm_dp_mst_topology_mgr.lock during reads but are the only
+ * updaters of this list and are protected from writing concurrently
+ * by &drm_dp_mst_topology_mgr.probe_lock.
+ */
+ struct drm_dp_mst_branch *mstb;
struct drm_dp_aux aux; /* i2c bus for this port? */
struct drm_dp_mst_branch *parent;
@@ -118,7 +126,6 @@ struct drm_dp_mst_port {
* @lct: Link count total to talk to this branch device.
* @num_ports: number of ports on the branch.
* @msg_slots: one bit per transmitted msg slot.
- * @ports: linked list of ports on this branch.
* @port_parent: pointer to the port parent, NULL if toplevel.
* @mgr: topology manager for this branch device.
* @tx_slots: transmission slots for this device.
@@ -156,6 +163,16 @@ struct drm_dp_mst_branch {
int num_ports;
int msg_slots;
+ /**
+ * @ports: the list of ports on this branch device. This should be
+ * considered protected for reading by &drm_dp_mst_topology_mgr.lock.
+ * There are two exceptions to this:
+ * &drm_dp_mst_topology_mgr.up_req_work and
+ * &drm_dp_mst_topology_mgr.work, which do not grab
+ * &drm_dp_mst_topology_mgr.lock during reads but are the only
+ * updaters of this list and are protected from updating the list
+ * concurrently by @drm_dp_mst_topology_mgr.probe_lock
+ */
struct list_head ports;
/* list of tx ops queue for this port */
@@ -502,6 +519,13 @@ struct drm_dp_mst_topology_mgr {
*/
struct mutex lock;
+ /**
+ * @probe_lock: Prevents @work and @up_req_work, the only writers of
+ * &drm_dp_mst_port.mstb and &drm_dp_mst_branch.ports, from racing
+ * while they update the topology.
+ */
+ struct mutex probe_lock;
+
/**
* @mst_state: If this manager is enabled for an MST capable port. False
* if no MST sink/branch devices is connected.
--
2.21.0
Lyude Paul
2019-Oct-22 02:36 UTC
[Nouveau] [PATCH v5 07/14] drm/dp_mst: Don't forget to update port->input in drm_dp_mst_handle_conn_stat()
This probably hasn't caused any problems up until now since it's
probably nearly impossible to encounter this in the wild, however if we
were to receive a connection status notification from the MST hub after
resume while we're in the middle of reprobing the link addresses for a
topology then there's a much larger chance that a port could have
changed from being an output port to input port (or vice versa). If we
forget to update this bit of information, we'll potentially ignore a
valid PDT change on a downstream port because we think it's an input
port.
So, make sure we read the input_port field in connection status
notifications in drm_dp_mst_handle_conn_stat() to prevent this from
happening once we've implemented suspend/resume reprobing.
Cc: Juston Li <juston.li at intel.com>
Cc: Imre Deak <imre.deak at intel.com>
Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
Cc: Harry Wentland <hwentlan at amd.com>
Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
Signed-off-by: Lyude Paul <lyude at redhat.com>
Reviewed-by: Sean Paul <sean at poorly.run>
---
drivers/gpu/drm/drm_dp_mst_topology.c | 52 +++++++++++++++++++--------
1 file changed, 38 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
b/drivers/gpu/drm/drm_dp_mst_topology.c
index 7bf4db91ff90..c8e218b902ae 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -2079,18 +2079,40 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch
*mstb,
{
struct drm_dp_mst_topology_mgr *mgr = mstb->mgr;
struct drm_dp_mst_port *port;
- int old_ddps;
- bool dowork = false;
+ int old_ddps, ret;
+ u8 new_pdt;
+ bool dowork = false, create_connector = false;
port = drm_dp_get_port(mstb, conn_stat->port_number);
if (!port)
return;
- /* Locking is only needed if the port's exposed to userspace */
- if (port->connector)
+ if (port->connector) {
+ if (!port->input && conn_stat->input_port) {
+ /*
+ * We can't remove a connector from an already exposed
+ * port, so just throw the port out and make sure we
+ * reprobe the link address of it's parent MSTB
+ */
+ drm_dp_mst_topology_unlink_port(mgr, port);
+ mstb->link_address_sent = false;
+ dowork = true;
+ goto out;
+ }
+
+ /*
+ * Locking is only needed if the port's exposed to userspace
+ */
drm_modeset_lock(&mgr->base.lock, NULL);
+ } else if (port->input && !conn_stat->input_port) {
+ create_connector = true;
+ /* Reprobe link address so we get num_sdp_streams */
+ mstb->link_address_sent = false;
+ dowork = true;
+ }
old_ddps = port->ddps;
+ port->input = conn_stat->input_port;
port->mcs = conn_stat->message_capability_status;
port->ldps = conn_stat->legacy_device_plug_status;
port->ddps = conn_stat->displayport_device_plug_status;
@@ -2103,21 +2125,23 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch
*mstb,
}
}
- if (!port->input) {
- int ret = drm_dp_port_set_pdt(port,
- conn_stat->peer_device_type);
- if (ret == 1) {
- dowork = true;
- } else if (ret < 0) {
- DRM_ERROR("Failed to change PDT for port %p: %d\n",
- port, ret);
- dowork = false;
- }
+ new_pdt = port->input ? DP_PEER_DEVICE_NONE :
conn_stat->peer_device_type;
+
+ ret = drm_dp_port_set_pdt(port, new_pdt);
+ if (ret == 1) {
+ dowork = true;
+ } else if (ret < 0) {
+ DRM_ERROR("Failed to change PDT for port %p: %d\n",
+ port, ret);
+ dowork = false;
}
if (port->connector)
drm_modeset_unlock(&mgr->base.lock);
+ else if (create_connector)
+ drm_dp_mst_port_add_connector(mstb, port);
+out:
drm_dp_mst_topology_put_port(port);
if (dowork)
queue_work(system_long_wq, &mstb->mgr->work);
--
2.21.0
Lyude Paul
2019-Oct-22 02:36 UTC
[Nouveau] [PATCH v5 08/14] drm/dp_mst: Lessen indenting in drm_dp_mst_topology_mgr_resume()
Does what it says on the tin.
Cc: Juston Li <juston.li at intel.com>
Cc: Imre Deak <imre.deak at intel.com>
Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
Cc: Harry Wentland <hwentlan at amd.com>
Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
Reviewed-by: Sean Paul <sean at poorly.run>
Signed-off-by: Lyude Paul <lyude at redhat.com>
---
drivers/gpu/drm/drm_dp_mst_topology.c | 59 +++++++++++++--------------
1 file changed, 29 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
b/drivers/gpu/drm/drm_dp_mst_topology.c
index c8e218b902ae..d486d15aa002 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -3212,45 +3212,44 @@ EXPORT_SYMBOL(drm_dp_mst_topology_mgr_suspend);
*/
int drm_dp_mst_topology_mgr_resume(struct drm_dp_mst_topology_mgr *mgr)
{
- int ret = 0;
+ int ret;
+ u8 guid[16];
mutex_lock(&mgr->lock);
+ if (!mgr->mst_primary)
+ goto out_fail;
- if (mgr->mst_primary) {
- int sret;
- u8 guid[16];
+ ret = drm_dp_dpcd_read(mgr->aux, DP_DPCD_REV, mgr->dpcd,
+ DP_RECEIVER_CAP_SIZE);
+ if (ret != DP_RECEIVER_CAP_SIZE) {
+ DRM_DEBUG_KMS("dpcd read failed - undocked during suspend?\n");
+ goto out_fail;
+ }
- sret = drm_dp_dpcd_read(mgr->aux, DP_DPCD_REV, mgr->dpcd,
DP_RECEIVER_CAP_SIZE);
- if (sret != DP_RECEIVER_CAP_SIZE) {
- DRM_DEBUG_KMS("dpcd read failed - undocked during suspend?\n");
- ret = -1;
- goto out_unlock;
- }
+ ret = drm_dp_dpcd_writeb(mgr->aux, DP_MSTM_CTRL,
+ DP_MST_EN |
+ DP_UP_REQ_EN |
+ DP_UPSTREAM_IS_SRC);
+ if (ret < 0) {
+ DRM_DEBUG_KMS("mst write failed - undocked during suspend?\n");
+ goto out_fail;
+ }
- ret = drm_dp_dpcd_writeb(mgr->aux, DP_MSTM_CTRL,
- DP_MST_EN | DP_UP_REQ_EN | DP_UPSTREAM_IS_SRC);
- if (ret < 0) {
- DRM_DEBUG_KMS("mst write failed - undocked during suspend?\n");
- ret = -1;
- goto out_unlock;
- }
+ /* Some hubs forget their guids after they resume */
+ ret = drm_dp_dpcd_read(mgr->aux, DP_GUID, guid, 16);
+ if (ret != 16) {
+ DRM_DEBUG_KMS("dpcd read failed - undocked during suspend?\n");
+ goto out_fail;
+ }
+ drm_dp_check_mstb_guid(mgr->mst_primary, guid);
- /* Some hubs forget their guids after they resume */
- sret = drm_dp_dpcd_read(mgr->aux, DP_GUID, guid, 16);
- if (sret != 16) {
- DRM_DEBUG_KMS("dpcd read failed - undocked during suspend?\n");
- ret = -1;
- goto out_unlock;
- }
- drm_dp_check_mstb_guid(mgr->mst_primary, guid);
+ mutex_unlock(&mgr->lock);
- ret = 0;
- } else
- ret = -1;
+ return 0;
-out_unlock:
+out_fail:
mutex_unlock(&mgr->lock);
- return ret;
+ return -1;
}
EXPORT_SYMBOL(drm_dp_mst_topology_mgr_resume);
--
2.21.0
Lyude Paul
2019-Oct-22 02:36 UTC
[Nouveau] [PATCH v5 09/14] drm/nouveau: Don't grab runtime PM refs for HPD IRQs
In order for suspend/resume reprobing to work, we need to be able to
perform sideband communications during suspend/resume, along with
runtime PM suspend/resume. In order to do so, we also need to make sure
that nouveau doesn't bother grabbing a runtime PM reference to do so,
since otherwise we'll start deadlocking runtime PM again.
Note that we weren't able to do this before, because of the DP MST
helpers processing UP requests from topologies in the same context as
drm_dp_mst_hpd_irq() which would have caused us to open ourselves up to
receiving hotplug events and deadlocking with runtime suspend/resume.
Now that those requests are handled asynchronously, this change should
be completely safe.
Cc: Juston Li <juston.li at intel.com>
Cc: Imre Deak <imre.deak at intel.com>
Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
Cc: Harry Wentland <hwentlan at amd.com>
Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
Reviewed-by: Sean Paul <sean at poorly.run>
Signed-off-by: Lyude Paul <lyude at redhat.com>
---
drivers/gpu/drm/nouveau/nouveau_connector.c | 33 +++++++++++----------
1 file changed, 17 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c
b/drivers/gpu/drm/nouveau/nouveau_connector.c
index 3a5db17bc5c7..5b413588b823 100644
--- a/drivers/gpu/drm/nouveau/nouveau_connector.c
+++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
@@ -1130,6 +1130,16 @@ nouveau_connector_hotplug(struct nvif_notify *notify)
const char *name = connector->name;
struct nouveau_encoder *nv_encoder;
int ret;
+ bool plugged = (rep->mask != NVIF_NOTIFY_CONN_V0_UNPLUG);
+
+ if (rep->mask & NVIF_NOTIFY_CONN_V0_IRQ) {
+ NV_DEBUG(drm, "service %s\n", name);
+ drm_dp_cec_irq(&nv_connector->aux);
+ if ((nv_encoder = find_encoder(connector, DCB_OUTPUT_DP)))
+ nv50_mstm_service(nv_encoder->dp.mstm);
+
+ return NVIF_NOTIFY_KEEP;
+ }
ret = pm_runtime_get(drm->dev->dev);
if (ret == 0) {
@@ -1150,25 +1160,16 @@ nouveau_connector_hotplug(struct nvif_notify *notify)
return NVIF_NOTIFY_DROP;
}
- if (rep->mask & NVIF_NOTIFY_CONN_V0_IRQ) {
- NV_DEBUG(drm, "service %s\n", name);
- drm_dp_cec_irq(&nv_connector->aux);
- if ((nv_encoder = find_encoder(connector, DCB_OUTPUT_DP)))
- nv50_mstm_service(nv_encoder->dp.mstm);
- } else {
- bool plugged = (rep->mask != NVIF_NOTIFY_CONN_V0_UNPLUG);
-
+ if (!plugged)
+ drm_dp_cec_unset_edid(&nv_connector->aux);
+ NV_DEBUG(drm, "%splugged %s\n", plugged ? "" :
"un", name);
+ if ((nv_encoder = find_encoder(connector, DCB_OUTPUT_DP))) {
if (!plugged)
- drm_dp_cec_unset_edid(&nv_connector->aux);
- NV_DEBUG(drm, "%splugged %s\n", plugged ? "" :
"un", name);
- if ((nv_encoder = find_encoder(connector, DCB_OUTPUT_DP))) {
- if (!plugged)
- nv50_mstm_remove(nv_encoder->dp.mstm);
- }
-
- drm_helper_hpd_irq_event(connector->dev);
+ nv50_mstm_remove(nv_encoder->dp.mstm);
}
+ drm_helper_hpd_irq_event(connector->dev);
+
pm_runtime_mark_last_busy(drm->dev->dev);
pm_runtime_put_autosuspend(drm->dev->dev);
return NVIF_NOTIFY_KEEP;
--
2.21.0
Lyude Paul
2019-Oct-22 02:36 UTC
[Nouveau] [PATCH v5 10/14] drm/nouveau: Resume hotplug interrupts earlier
Currently, we enable hotplug detection only after we re-enable the
display. However, this is too late if we're planning on sending sideband
messages during the resume process - which we'll need to do in order to
reprobe the topology on resume.
So, enable hotplug events before reinitializing the display.
Cc: Juston Li <juston.li at intel.com>
Cc: Imre Deak <imre.deak at intel.com>
Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
Cc: Harry Wentland <hwentlan at amd.com>
Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
Cc: Sean Paul <sean at poorly.run>
Signed-off-by: Lyude Paul <lyude at redhat.com>
---
drivers/gpu/drm/nouveau/nouveau_display.c | 19 +++++++++++--------
1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c
b/drivers/gpu/drm/nouveau/nouveau_display.c
index 6f038511a03a..53f9bceaf17a 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.c
+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
@@ -407,6 +407,17 @@ nouveau_display_init(struct drm_device *dev, bool resume,
bool runtime)
struct drm_connector_list_iter conn_iter;
int ret;
+ /*
+ * Enable hotplug interrupts (done as early as possible, since we need
+ * them for MST)
+ */
+ drm_connector_list_iter_begin(dev, &conn_iter);
+ nouveau_for_each_non_mst_connector_iter(connector, &conn_iter) {
+ struct nouveau_connector *conn = nouveau_connector(connector);
+ nvif_notify_get(&conn->hpd);
+ }
+ drm_connector_list_iter_end(&conn_iter);
+
ret = disp->init(dev, resume, runtime);
if (ret)
return ret;
@@ -416,14 +427,6 @@ nouveau_display_init(struct drm_device *dev, bool resume,
bool runtime)
*/
drm_kms_helper_poll_enable(dev);
- /* enable hotplug interrupts */
- drm_connector_list_iter_begin(dev, &conn_iter);
- nouveau_for_each_non_mst_connector_iter(connector, &conn_iter) {
- struct nouveau_connector *conn = nouveau_connector(connector);
- nvif_notify_get(&conn->hpd);
- }
- drm_connector_list_iter_end(&conn_iter);
-
return ret;
}
--
2.21.0
Lyude Paul
2019-Oct-22 02:36 UTC
[Nouveau] [PATCH v5 14/14] drm/dp_mst: Add topology ref history tracking for debugging
For very subtle mistakes with topology refs, it can be rather difficult
to trace them down with the debugging info that we already have. I had
one such issue recently while trying to implement suspend/resume
reprobing for MST, and ended up coming up with this.
Inspired by Chris Wilson's wakeref tracking for i915, this adds a very
similar feature to the DP MST helpers, which allows for partial tracking
of topology refs for both ports and branch devices. This is a lot less
advanced then wakeref tracking: we merely keep a count of all of the
spots where a topology ref has been grabbed or dropped, then dump out
that history in chronological order when a port or branch device's
topology refcount reaches 0. So far, I've found this incredibly useful
for debugging topology refcount errors.
Since this has the potential to be somewhat slow and loud, we add an
expert kernel config option to enable or disable this feature,
CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS.
Changes since v1:
* Don't forget to destroy topology_ref_history_lock
Changes since v4:
* Correct order of kref_put()/topology_ref_history_unlock - we can't
unlock the history after kref_put() since the memory might have been
freed by that point
* Don't print message on allocation error failures, the kernel already
does this for us
Cc: Juston Li <juston.li at intel.com>
Cc: Imre Deak <imre.deak at intel.com>
Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
Cc: Harry Wentland <hwentlan at amd.com>
Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
Reviewed-by: Sean Paul <sean at poorly.run>
Signed-off-by: Lyude Paul <lyude at redhat.com>
---
drivers/gpu/drm/Kconfig | 14 ++
drivers/gpu/drm/drm_dp_mst_topology.c | 241 ++++++++++++++++++++++++--
include/drm/drm_dp_mst_helper.h | 45 +++++
3 files changed, 290 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 36357a36a281..f3f5910743d4 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -93,6 +93,20 @@ config DRM_KMS_FB_HELPER
help
FBDEV helpers for KMS drivers.
+config DRM_DEBUG_DP_MST_TOPOLOGY_REFS
+ bool "Enable refcount backtrace history in the DP MST
helpers"
+ select STACKDEPOT
+ depends on DRM_KMS_HELPER
+ depends on DEBUG_KERNEL
+ depends on EXPERT
+ help
+ Enables debug tracing for topology refs in DRM's DP MST helpers.
A
+ history of each topology reference/dereference will be printed to the
+ kernel log once a port or branch device's topology refcount
reaches 0.
+
+ This has the potential to use a lot of memory and print some very
+ large kernel messages. If in doubt, say "N".
+
config DRM_FBDEV_EMULATION
bool "Enable legacy fbdev support for your modesetting driver"
depends on DRM
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
b/drivers/gpu/drm/drm_dp_mst_topology.c
index 428160270482..cedfa281a22e 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -28,6 +28,13 @@
#include <linux/sched.h>
#include <linux/seq_file.h>
+#if IS_ENABLED(CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS)
+#include <linux/stackdepot.h>
+#include <linux/sort.h>
+#include <linux/timekeeping.h>
+#include <linux/math64.h>
+#endif
+
#include <drm/drm_atomic.h>
#include <drm/drm_atomic_helper.h>
#include <drm/drm_dp_mst_helper.h>
@@ -1399,12 +1406,187 @@ drm_dp_mst_put_port_malloc(struct drm_dp_mst_port
*port)
}
EXPORT_SYMBOL(drm_dp_mst_put_port_malloc);
+#if IS_ENABLED(CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS)
+
+#define STACK_DEPTH 8
+
+static noinline void
+__topology_ref_save(struct drm_dp_mst_topology_mgr *mgr,
+ struct drm_dp_mst_topology_ref_history *history,
+ enum drm_dp_mst_topology_ref_type type)
+{
+ struct drm_dp_mst_topology_ref_entry *entry = NULL;
+ depot_stack_handle_t backtrace;
+ ulong stack_entries[STACK_DEPTH];
+ uint n;
+ int i;
+
+ n = stack_trace_save(stack_entries, ARRAY_SIZE(stack_entries), 1);
+ backtrace = stack_depot_save(stack_entries, n, GFP_KERNEL);
+ if (!backtrace)
+ return;
+
+ /* Try to find an existing entry for this backtrace */
+ for (i = 0; i < history->len; i++) {
+ if (history->entries[i].backtrace == backtrace) {
+ entry = &history->entries[i];
+ break;
+ }
+ }
+
+ /* Otherwise add one */
+ if (!entry) {
+ struct drm_dp_mst_topology_ref_entry *new;
+ int new_len = history->len + 1;
+
+ new = krealloc(history->entries, sizeof(*new) * new_len,
+ GFP_KERNEL);
+ if (!new)
+ return;
+
+ entry = &new[history->len];
+ history->len = new_len;
+ history->entries = new;
+
+ entry->backtrace = backtrace;
+ entry->type = type;
+ entry->count = 0;
+ }
+ entry->count++;
+ entry->ts_nsec = ktime_get_ns();
+
+ return;
+}
+
+static int
+topology_ref_history_cmp(const void *a, const void *b)
+{
+ const struct drm_dp_mst_topology_ref_entry *entry_a = a, *entry_b = b;
+
+ if (entry_a->ts_nsec > entry_b->ts_nsec)
+ return 1;
+ else if (entry_a->ts_nsec < entry_b->ts_nsec)
+ return -1;
+ else
+ return 0;
+}
+
+static inline const char *
+topology_ref_type_to_str(enum drm_dp_mst_topology_ref_type type)
+{
+ if (type == DRM_DP_MST_TOPOLOGY_REF_GET)
+ return "get";
+ else
+ return "put";
+}
+
+static void
+__dump_topology_ref_history(struct drm_dp_mst_topology_ref_history *history,
+ void *ptr, const char *type_str)
+{
+ struct drm_printer p = drm_debug_printer(DBG_PREFIX);
+ char *buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
+ int i;
+
+ if (!buf)
+ return;
+
+ if (!history->len)
+ goto out;
+
+ /* First, sort the list so that it goes from oldest to newest
+ * reference entry
+ */
+ sort(history->entries, history->len, sizeof(*history->entries),
+ topology_ref_history_cmp, NULL);
+
+ drm_printf(&p,
+ "%s (%p/%px) topology count reached 0, dumping history:\n",
+ type_str, ptr, ptr);
+
+ for (i = 0; i < history->len; i++) {
+ const struct drm_dp_mst_topology_ref_entry *entry +
&history->entries[i];
+ ulong *entries;
+ uint nr_entries;
+ u64 ts_nsec = entry->ts_nsec;
+ u64 rem_nsec = do_div(ts_nsec, 1000000000);
+
+ nr_entries = stack_depot_fetch(entry->backtrace, &entries);
+ stack_trace_snprint(buf, PAGE_SIZE, entries, nr_entries, 4);
+
+ drm_printf(&p, " %d %ss (last at %5llu.%06llu):\n%s",
+ entry->count,
+ topology_ref_type_to_str(entry->type),
+ ts_nsec, rem_nsec / 1000, buf);
+ }
+
+ /* Now free the history, since this is the only time we expose it */
+ kfree(history->entries);
+out:
+ kfree(buf);
+}
+
+static __always_inline void
+drm_dp_mst_dump_mstb_topology_history(struct drm_dp_mst_branch *mstb)
+{
+ __dump_topology_ref_history(&mstb->topology_ref_history, mstb,
+ "MSTB");
+}
+
+static __always_inline void
+drm_dp_mst_dump_port_topology_history(struct drm_dp_mst_port *port)
+{
+ __dump_topology_ref_history(&port->topology_ref_history, port,
+ "Port");
+}
+
+static __always_inline void
+save_mstb_topology_ref(struct drm_dp_mst_branch *mstb,
+ enum drm_dp_mst_topology_ref_type type)
+{
+ __topology_ref_save(mstb->mgr, &mstb->topology_ref_history, type);
+}
+
+static __always_inline void
+save_port_topology_ref(struct drm_dp_mst_port *port,
+ enum drm_dp_mst_topology_ref_type type)
+{
+ __topology_ref_save(port->mgr, &port->topology_ref_history, type);
+}
+
+static inline void
+topology_ref_history_lock(struct drm_dp_mst_topology_mgr *mgr)
+{
+ mutex_lock(&mgr->topology_ref_history_lock);
+}
+
+static inline void
+topology_ref_history_unlock(struct drm_dp_mst_topology_mgr *mgr)
+{
+ mutex_unlock(&mgr->topology_ref_history_lock);
+}
+#else
+static inline void
+topology_ref_history_lock(struct drm_dp_mst_topology_mgr *mgr) {}
+static inline void
+topology_ref_history_unlock(struct drm_dp_mst_topology_mgr *mgr) {}
+static inline void
+drm_dp_mst_dump_mstb_topology_history(struct drm_dp_mst_branch *mstb) {}
+static inline void
+drm_dp_mst_dump_port_topology_history(struct drm_dp_mst_port *port) {}
+#define save_mstb_topology_ref(mstb, type)
+#define save_port_topology_ref(port, type)
+#endif
+
static void drm_dp_destroy_mst_branch_device(struct kref *kref)
{
struct drm_dp_mst_branch *mstb container_of(kref, struct drm_dp_mst_branch,
topology_kref);
struct drm_dp_mst_topology_mgr *mgr = mstb->mgr;
+ drm_dp_mst_dump_mstb_topology_history(mstb);
+
INIT_LIST_HEAD(&mstb->destroy_next);
/*
@@ -1442,11 +1624,17 @@ static void drm_dp_destroy_mst_branch_device(struct kref
*kref)
static int __must_check
drm_dp_mst_topology_try_get_mstb(struct drm_dp_mst_branch *mstb)
{
- int ret = kref_get_unless_zero(&mstb->topology_kref);
+ int ret;
- if (ret)
- DRM_DEBUG("mstb %p (%d)\n", mstb,
- kref_read(&mstb->topology_kref));
+ topology_ref_history_lock(mstb->mgr);
+ ret = kref_get_unless_zero(&mstb->topology_kref);
+ if (ret) {
+ DRM_DEBUG("mstb %p (%d)\n",
+ mstb, kref_read(&mstb->topology_kref));
+ save_mstb_topology_ref(mstb, DRM_DP_MST_TOPOLOGY_REF_GET);
+ }
+
+ topology_ref_history_unlock(mstb->mgr);
return ret;
}
@@ -1467,9 +1655,14 @@ drm_dp_mst_topology_try_get_mstb(struct drm_dp_mst_branch
*mstb)
*/
static void drm_dp_mst_topology_get_mstb(struct drm_dp_mst_branch *mstb)
{
+ topology_ref_history_lock(mstb->mgr);
+
+ save_mstb_topology_ref(mstb, DRM_DP_MST_TOPOLOGY_REF_GET);
WARN_ON(kref_read(&mstb->topology_kref) == 0);
kref_get(&mstb->topology_kref);
DRM_DEBUG("mstb %p (%d)\n", mstb,
kref_read(&mstb->topology_kref));
+
+ topology_ref_history_unlock(mstb->mgr);
}
/**
@@ -1487,8 +1680,13 @@ static void drm_dp_mst_topology_get_mstb(struct
drm_dp_mst_branch *mstb)
static void
drm_dp_mst_topology_put_mstb(struct drm_dp_mst_branch *mstb)
{
+ topology_ref_history_lock(mstb->mgr);
+
DRM_DEBUG("mstb %p (%d)\n",
mstb, kref_read(&mstb->topology_kref) - 1);
+ save_mstb_topology_ref(mstb, DRM_DP_MST_TOPOLOGY_REF_PUT);
+
+ topology_ref_history_unlock(mstb->mgr);
kref_put(&mstb->topology_kref, drm_dp_destroy_mst_branch_device);
}
@@ -1498,6 +1696,8 @@ static void drm_dp_destroy_port(struct kref *kref)
container_of(kref, struct drm_dp_mst_port, topology_kref);
struct drm_dp_mst_topology_mgr *mgr = port->mgr;
+ drm_dp_mst_dump_port_topology_history(port);
+
/* There's nothing that needs locking to destroy an input port yet */
if (port->input) {
drm_dp_mst_put_port_malloc(port);
@@ -1541,12 +1741,17 @@ static void drm_dp_destroy_port(struct kref *kref)
static int __must_check
drm_dp_mst_topology_try_get_port(struct drm_dp_mst_port *port)
{
- int ret = kref_get_unless_zero(&port->topology_kref);
+ int ret;
- if (ret)
- DRM_DEBUG("port %p (%d)\n", port,
- kref_read(&port->topology_kref));
+ topology_ref_history_lock(port->mgr);
+ ret = kref_get_unless_zero(&port->topology_kref);
+ if (ret) {
+ DRM_DEBUG("port %p (%d)\n",
+ port, kref_read(&port->topology_kref));
+ save_port_topology_ref(port, DRM_DP_MST_TOPOLOGY_REF_GET);
+ }
+ topology_ref_history_unlock(port->mgr);
return ret;
}
@@ -1565,9 +1770,14 @@ drm_dp_mst_topology_try_get_port(struct drm_dp_mst_port
*port)
*/
static void drm_dp_mst_topology_get_port(struct drm_dp_mst_port *port)
{
+ topology_ref_history_lock(port->mgr);
+
WARN_ON(kref_read(&port->topology_kref) == 0);
kref_get(&port->topology_kref);
DRM_DEBUG("port %p (%d)\n", port,
kref_read(&port->topology_kref));
+ save_port_topology_ref(port, DRM_DP_MST_TOPOLOGY_REF_GET);
+
+ topology_ref_history_unlock(port->mgr);
}
/**
@@ -1583,8 +1793,13 @@ static void drm_dp_mst_topology_get_port(struct
drm_dp_mst_port *port)
*/
static void drm_dp_mst_topology_put_port(struct drm_dp_mst_port *port)
{
- DRM_DEBUG("port %p (%d)\n",
- port, kref_read(&port->topology_kref) - 1);
+ topology_ref_history_lock(port->mgr);
+
+ DRM_DEBUG("port %p/%px (%d)\n",
+ port, port, kref_read(&port->topology_kref) - 1);
+ save_port_topology_ref(port, DRM_DP_MST_TOPOLOGY_REF_PUT);
+
+ topology_ref_history_unlock(port->mgr);
kref_put(&port->topology_kref, drm_dp_destroy_port);
}
@@ -4577,6 +4792,9 @@ int drm_dp_mst_topology_mgr_init(struct
drm_dp_mst_topology_mgr *mgr,
mutex_init(&mgr->delayed_destroy_lock);
mutex_init(&mgr->up_req_lock);
mutex_init(&mgr->probe_lock);
+#if IS_ENABLED(CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS)
+ mutex_init(&mgr->topology_ref_history_lock);
+#endif
INIT_LIST_HEAD(&mgr->tx_msg_downq);
INIT_LIST_HEAD(&mgr->destroy_port_list);
INIT_LIST_HEAD(&mgr->destroy_branch_device_list);
@@ -4643,6 +4861,9 @@ void drm_dp_mst_topology_mgr_destroy(struct
drm_dp_mst_topology_mgr *mgr)
mutex_destroy(&mgr->lock);
mutex_destroy(&mgr->up_req_lock);
mutex_destroy(&mgr->probe_lock);
+#if IS_ENABLED(CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS)
+ mutex_destroy(&mgr->topology_ref_history_lock);
+#endif
}
EXPORT_SYMBOL(drm_dp_mst_topology_mgr_destroy);
diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
index 144027e27464..d5fc90b30487 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -26,6 +26,26 @@
#include <drm/drm_dp_helper.h>
#include <drm/drm_atomic.h>
+#if IS_ENABLED(CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS)
+#include <linux/stackdepot.h>
+#include <linux/timekeeping.h>
+
+enum drm_dp_mst_topology_ref_type {
+ DRM_DP_MST_TOPOLOGY_REF_GET,
+ DRM_DP_MST_TOPOLOGY_REF_PUT,
+};
+
+struct drm_dp_mst_topology_ref_history {
+ struct drm_dp_mst_topology_ref_entry {
+ enum drm_dp_mst_topology_ref_type type;
+ int count;
+ ktime_t ts_nsec;
+ depot_stack_handle_t backtrace;
+ } *entries;
+ int len;
+};
+#endif /* IS_ENABLED(CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS) */
+
struct drm_dp_mst_branch;
/**
@@ -89,6 +109,14 @@ struct drm_dp_mst_port {
*/
struct kref malloc_kref;
+#if IS_ENABLED(CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS)
+ /**
+ * @topology_ref_history: A history of each topology
+ * reference/dereference. See CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS.
+ */
+ struct drm_dp_mst_topology_ref_history topology_ref_history;
+#endif
+
u8 port_num;
bool input;
bool mcs;
@@ -162,6 +190,14 @@ struct drm_dp_mst_branch {
*/
struct kref malloc_kref;
+#if IS_ENABLED(CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS)
+ /**
+ * @topology_ref_history: A history of each topology
+ * reference/dereference. See CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS.
+ */
+ struct drm_dp_mst_topology_ref_history topology_ref_history;
+#endif
+
/**
* @destroy_next: linked-list entry used by
* drm_dp_delayed_destroy_work()
@@ -647,6 +683,15 @@ struct drm_dp_mst_topology_mgr {
* transmissions.
*/
struct work_struct up_req_work;
+
+#if IS_ENABLED(CONFIG_DRM_DEBUG_DP_MST_TOPOLOGY_REFS)
+ /**
+ * @topology_ref_history_lock: protects
+ * &drm_dp_mst_port.topology_ref_history and
+ * &drm_dp_mst_branch.topology_ref_history.
+ */
+ struct mutex topology_ref_history_lock;
+#endif
};
int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,
--
2.21.0
Lyude Paul
2019-Oct-22 02:39 UTC
[Nouveau] [PATCH v5 00/14] DP MST Refactors + debugging tools + suspend/resume reprobing
This is the final portion of the large series for adding MST
suspend/resume reprobing that I've been working on for quite a while
now. In addition, I:
* Refactored and cleaned up any code I ended up digging through in the
process of understanding how some parts of these helpers worked.
* Added some debugging tools along the way that I ended up needing to
figure out some issues in my own code
Note that there's still one important part of this process missing
that's not included in this patch series: EDID reprobing, which I
believe Stanislav Lisovskiy from Intel is currently working on. The main
purpose of this series is to fix the issue of the in-memory topology
state (e.g. connectors connected to an MST hub, branch devices, etc.)
going out of sync if a topology connected to a connector is swapped out
with a different topology while the system is resumed, or while the
device housing said connector is in runtime suspend.
As well, the debugging tools that are added in this include:
* A limited debugging utility for dumping the list of topology
references on an MST port or branch connector whose topology reference
count has reached 0
<sideband message dumping has been merged :)>
Lyude Paul (14):
drm/dp_mst: Destroy MSTBs asynchronously
drm/dp_mst: Remove PDT teardown in drm_dp_destroy_port() and refactor
drm/dp_mst: Refactor pdt setup/teardown, add more locking
drm/dp_mst: Handle UP requests asynchronously
drm/dp_mst: Add probe_lock
drm/dp_mst: Protect drm_dp_mst_port members with locking
drm/dp_mst: Don't forget to update port->input in
drm_dp_mst_handle_conn_stat()
drm/dp_mst: Lessen indenting in drm_dp_mst_topology_mgr_resume()
drm/nouveau: Don't grab runtime PM refs for HPD IRQs
drm/nouveau: Resume hotplug interrupts earlier
drm/amdgpu: Iterate through DRM connectors correctly
drm/amdgpu/dm: Resume short HPD IRQs before resuming MST topology
drm/dp_mst: Add basic topology reprobing when resuming
drm/dp_mst: Add topology ref history tracking for debugging
drivers/gpu/drm/Kconfig | 14 +
.../gpu/drm/amd/amdgpu/amdgpu_connectors.c | 13 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 20 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 5 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_encoders.c | 40 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c | 5 +-
drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 34 +-
drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 34 +-
drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 40 +-
drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 34 +-
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 41 +-
.../drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c | 10 +-
.../display/amdgpu_dm/amdgpu_dm_mst_types.c | 28 +-
drivers/gpu/drm/drm_dp_mst_topology.c | 1185 +++++++++++++----
drivers/gpu/drm/i915/display/intel_dp.c | 3 +-
drivers/gpu/drm/i915/display/intel_dp_mst.c | 28 +-
drivers/gpu/drm/nouveau/dispnv50/disp.c | 38 +-
drivers/gpu/drm/nouveau/nouveau_connector.c | 33 +-
drivers/gpu/drm/nouveau/nouveau_display.c | 19 +-
drivers/gpu/drm/radeon/radeon_dp_mst.c | 24 +-
include/drm/drm_dp_mst_helper.h | 160 ++-
21 files changed, 1329 insertions(+), 479 deletions(-)
--
2.21.0
Lyude Paul
2019-Oct-22 02:40 UTC
[Nouveau] [PATCH v5 06/14] drm/dp_mst: Protect drm_dp_mst_port members with locking
This is a complicated one. Essentially, there's currently a problem in the
MST
core that hasn't really caused any issues that we're aware of (emphasis
on "that
we're aware of"): locking.
When we go through and probe the link addresses and path resources in a
topology, we hold no locks when updating ports with said information. The
members I'm referring to in particular are:
- ldps
- ddps
- mcs
- pdt
- dpcd_rev
- num_sdp_streams
- num_sdp_stream_sinks
- available_pbn
- input
- connector
Now that we're handling UP requests asynchronously and will be using some of
the struct members mentioned above in atomic modesetting in the future for
features such as PBN validation, this is going to become a lot more important.
As well, the next few commits that prepare us for and introduce suspend/resume
reprobing will also need clear locking in order to prevent from additional
racing hilarities that we never could have hit in the past.
So, let's solve this issue by using &mgr->base.lock, the modesetting
lock which currently only protects &mgr->base.state. This works
perfectly because it allows us to avoid blocking connection_mutex
unnecessarily, and we can grab this in connector detection paths since
it's a ww mutex. We start by having drm_dp_mst_handle_up_req() hold this
when updating ports. For drm_dp_mst_handle_link_address_port() things
are a bit more complicated. As I've learned the hard way, we can grab
&mgr->lock.base for everything except for port->connector. See, our
normal driver probing paths end up generating this rather obvious
lockdep chain:
&drm->mode_config.mutex
-> crtc_ww_class_mutex/crtc_ww_class_acquire
-> &connector->mutex
However, sysfs grabs &drm->mode_config.mutex in order to protect itself
from connector state changing under it. Because this entails grabbing
kn->count, e.g. the lock that the kernel provides for protecting sysfs
contexts, we end up grabbing kn->count followed by
&drm->mode_config.mutex. This ends up creating an extremely rude chain:
&kn->count
-> &drm->mode_config.mutex
-> crtc_ww_class_mutex/crtc_ww_class_acquire
-> &connector->mutex
I mean, look at that thing! It's just evil!!! This gross thing ends up
making any calls to drm_connector_register()/drm_connector_unregister()
impossible when holding any kind of modesetting lock. This is annoying
because ideally, we always want to ensure that
drm_dp_mst_port->connector never changes when doing an atomic commit or
check that would affect the atomic topology state so that it can
reliably and easily be used from future DRM DP MST helpers to assist
with tasks such as scanning through the current VCPI allocations and
adding connectors which need to have their allocations updated in
response to a bandwidth change or the like.
Being able to hold &mgr->base.lock throughout the entire link probe
process would have been _great_, since we could prevent userspace from
ever seeing any states in-between individual port changes and as a
result likely end up with a much faster probe and more consistent
results from said probes. But without some rework of how we handle
connector probing in sysfs it's not at all currently possible. In the
future, maybe we can try using the sysfs locks to protect updates to
connector probing state and fix this mess.
So for now, to protect everything other than port->connector under
&mgr->base.lock and ensure that we still have the guarantee that atomic
check/commit contexts will never see port->connector change we use a
silly trick. See: port->connector only needs to change in order to
ensure that input ports (see the MST spec) never have a ghost connector
associated with them. But, there's nothing stopping us from simply
throwing the entire port out and creating a new one in order to maintain
that requirement while still keeping port->connector consistent across
the lifetime of the port in atomic check/commit contexts. For all
intended purposes this works fine, as we validate ports in any contexts
we care about before using them and as such will end up reporting the
connector as disconnected until it's port's destruction finalizes. So,
we just do that in cases where we detect port->input has transitioned
from true->false. We don't need to worry about the other direction,
since a port without a connector isn't visible to userspace and as such
doesn't need to be protected by &mgr->base.lock until we finish
registering a connector for it.
For updating members of drm_dp_mst_port other than port->connector, we
simply grab &mgr->base.lock in drm_dp_mst_link_probe_work() for already
registered ports, update said members and drop the lock before
potentially registering a connector and probing the link address of it's
children.
Finally, we modify drm_dp_mst_detect_port() to take a modesetting lock
acquisition context in order to acquire &mgr->base.lock under
&connection_mutex and convert all it's users over to using the
.detect_ctx probe hooks.
With that, we finally have well defined locking.
Changes since v4:
* Get rid of port->mutex, stop using connection_mutex and just use our own
modesetting lock - mgr->base.lock. Also, add a probe_lock that comes
before this patch.
* Just throw out ports that get changed from an output to an input, and
replace them with new ports. This lets us ensure that modesetting
contexts never see port->connector go from having a connector to being
NULL.
* Write an extremely detailed explanation of what problems this is
trying to fix, since there's a _lot_ of context here and I honestly
forgot some of it myself a couple times.
* Don't grab mgr->lock when reading port->mstb in
drm_dp_mst_handle_link_address_port(). It's not needed.
Cc: Juston Li <juston.li at intel.com>
Cc: Imre Deak <imre.deak at intel.com>
Cc: Ville Syrj?l? <ville.syrjala at linux.intel.com>
Cc: Harry Wentland <hwentlan at amd.com>
Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
Cc: Sean Paul <sean at poorly.run>
Signed-off-by: Lyude Paul <lyude at redhat.com>
---
.../display/amdgpu_dm/amdgpu_dm_mst_types.c | 28 +--
drivers/gpu/drm/drm_dp_mst_topology.c | 230 ++++++++++++------
drivers/gpu/drm/i915/display/intel_dp_mst.c | 28 ++-
drivers/gpu/drm/nouveau/dispnv50/disp.c | 32 +--
drivers/gpu/drm/radeon/radeon_dp_mst.c | 24 +-
include/drm/drm_dp_mst_helper.h | 38 ++-
6 files changed, 240 insertions(+), 140 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
index 5ec14efd4d8c..f8214acf70b3 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
@@ -123,21 +123,6 @@ static ssize_t dm_dp_aux_transfer(struct drm_dp_aux *aux,
return result;
}
-static enum drm_connector_status
-dm_dp_mst_detect(struct drm_connector *connector, bool force)
-{
- struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
- struct amdgpu_dm_connector *master = aconnector->mst_port;
-
- enum drm_connector_status status - drm_dp_mst_detect_port(
- connector,
- &master->mst_mgr,
- aconnector->port);
-
- return status;
-}
-
static void
dm_dp_mst_connector_destroy(struct drm_connector *connector)
{
@@ -177,7 +162,6 @@ amdgpu_dm_mst_connector_early_unregister(struct
drm_connector *connector)
}
static const struct drm_connector_funcs dm_dp_mst_connector_funcs = {
- .detect = dm_dp_mst_detect,
.fill_modes = drm_helper_probe_single_connector_modes,
.destroy = dm_dp_mst_connector_destroy,
.reset = amdgpu_dm_connector_funcs_reset,
@@ -252,10 +236,22 @@ static struct drm_encoder *dm_mst_best_encoder(struct
drm_connector *connector)
return &amdgpu_dm_connector->mst_encoder->base;
}
+static int
+dm_dp_mst_detect(struct drm_connector *connector,
+ struct drm_modeset_acquire_ctx *ctx, bool force)
+{
+ struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
+ struct amdgpu_dm_connector *master = aconnector->mst_port;
+
+ return drm_dp_mst_detect_port(connector, ctx, &master->mst_mgr,
+ aconnector->port);
+}
+
static const struct drm_connector_helper_funcs dm_dp_mst_connector_helper_funcs
= {
.get_modes = dm_dp_mst_get_modes,
.mode_valid = amdgpu_dm_connector_mode_valid,
.best_encoder = dm_mst_best_encoder,
+ .detect_ctx = dm_dp_mst_detect,
};
static void amdgpu_dm_encoder_destroy(struct drm_encoder *encoder)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
b/drivers/gpu/drm/drm_dp_mst_topology.c
index 11d842f0bff5..7bf4db91ff90 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -1905,6 +1905,78 @@ void drm_dp_mst_connector_early_unregister(struct
drm_connector *connector,
}
EXPORT_SYMBOL(drm_dp_mst_connector_early_unregister);
+static void
+drm_dp_mst_port_add_connector(struct drm_dp_mst_branch *mstb,
+ struct drm_dp_mst_port *port)
+{
+ struct drm_dp_mst_topology_mgr *mgr = port->mgr;
+ char proppath[255];
+ int ret;
+
+ build_mst_prop_path(mstb, port->port_num, proppath, sizeof(proppath));
+ port->connector = mgr->cbs->add_connector(mgr, port, proppath);
+ if (!port->connector) {
+ ret = -ENOMEM;
+ goto error;
+ }
+
+ if ((port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV ||
+ port->pdt == DP_PEER_DEVICE_SST_SINK) &&
+ port->port_num >= DP_MST_LOGICAL_PORT_0) {
+ port->cached_edid = drm_get_edid(port->connector,
+ &port->aux.ddc);
+ drm_connector_set_tile_property(port->connector);
+ }
+
+ mgr->cbs->register_connector(port->connector);
+ return;
+
+error:
+ DRM_ERROR("Failed to create connector for port %p: %d\n", port,
ret);
+}
+
+/*
+ * Drop a topology reference, and unlink the port from the in-memory topology
+ * layout
+ */
+static void
+drm_dp_mst_topology_unlink_port(struct drm_dp_mst_topology_mgr *mgr,
+ struct drm_dp_mst_port *port)
+{
+ mutex_lock(&mgr->lock);
+ list_del(&port->next);
+ mutex_unlock(&mgr->lock);
+ drm_dp_mst_topology_put_port(port);
+}
+
+static struct drm_dp_mst_port *
+drm_dp_mst_add_port(struct drm_device *dev,
+ struct drm_dp_mst_topology_mgr *mgr,
+ struct drm_dp_mst_branch *mstb, u8 port_number)
+{
+ struct drm_dp_mst_port *port = kzalloc(sizeof(*port), GFP_KERNEL);
+
+ if (!port)
+ return NULL;
+
+ kref_init(&port->topology_kref);
+ kref_init(&port->malloc_kref);
+ port->parent = mstb;
+ port->port_num = port_number;
+ port->mgr = mgr;
+ port->aux.name = "DPMST";
+ port->aux.dev = dev->dev;
+ port->aux.is_remote = true;
+
+ /*
+ * Make sure the memory allocation for our parent branch stays
+ * around until our own memory allocation is released
+ */
+ drm_dp_mst_get_mstb_malloc(mstb);
+
+ return port;
+}
+
static void
drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb,
struct drm_device *dev,
@@ -1912,35 +1984,40 @@ drm_dp_mst_handle_link_address_port(struct
drm_dp_mst_branch *mstb,
{
struct drm_dp_mst_topology_mgr *mgr = mstb->mgr;
struct drm_dp_mst_port *port;
- bool created = false;
- int old_ddps = 0;
+ int old_ddps = 0, ret;
+ u8 new_pdt = DP_PEER_DEVICE_NONE;
+ bool created = false, send_link_addr = false;
port = drm_dp_get_port(mstb, port_msg->port_number);
if (!port) {
- port = kzalloc(sizeof(*port), GFP_KERNEL);
+ port = drm_dp_mst_add_port(dev, mgr, mstb,
+ port_msg->port_number);
if (!port)
return;
- kref_init(&port->topology_kref);
- kref_init(&port->malloc_kref);
- port->parent = mstb;
- port->port_num = port_msg->port_number;
- port->mgr = mgr;
- port->aux.name = "DPMST";
- port->aux.dev = dev->dev;
- port->aux.is_remote = true;
-
- /*
- * Make sure the memory allocation for our parent branch stays
- * around until our own memory allocation is released
+ created = true;
+ } else if (port_msg->input_port && !port->input &&
port->connector) {
+ /* Destroying the connector is impossible in this context, so
+ * replace the port with a new one
*/
- drm_dp_mst_get_mstb_malloc(mstb);
+ drm_dp_mst_topology_unlink_port(mgr, port);
+ drm_dp_mst_topology_put_port(port);
+ port = drm_dp_mst_add_port(dev, mgr, mstb,
+ port_msg->port_number);
+ if (!port)
+ return;
created = true;
} else {
+ /* Locking is only needed when the port has a connector
+ * exposed to userspace
+ */
+ drm_modeset_lock(&mgr->base.lock, NULL);
old_ddps = port->ddps;
}
port->input = port_msg->input_port;
+ if (!port->input)
+ new_pdt = port_msg->peer_device_type;
port->mcs = port_msg->mcs;
port->ddps = port_msg->ddps;
port->ldps = port_msg->legacy_device_plug_status;
@@ -1968,59 +2045,39 @@ drm_dp_mst_handle_link_address_port(struct
drm_dp_mst_branch *mstb,
}
}
- if (!port->input) {
- int ret = drm_dp_port_set_pdt(port,
- port_msg->peer_device_type);
- if (ret == 1) {
- drm_dp_send_link_address(mgr, port->mstb);
- } else if (ret < 0) {
- DRM_ERROR("Failed to change PDT on port %p: %d\n",
- port, ret);
- goto fail;
- }
+ ret = drm_dp_port_set_pdt(port, new_pdt);
+ if (ret == 1) {
+ send_link_addr = true;
+ } else if (ret < 0) {
+ DRM_ERROR("Failed to change PDT on port %p: %d\n",
+ port, ret);
+ goto fail;
}
- if (created && !port->input) {
- char proppath[255];
-
- build_mst_prop_path(mstb, port->port_num, proppath,
- sizeof(proppath));
- port->connector = (*mgr->cbs->add_connector)(mgr, port,
- proppath);
- if (!port->connector)
- goto fail;
-
- if ((port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV ||
- port->pdt == DP_PEER_DEVICE_SST_SINK) &&
- port->port_num >= DP_MST_LOGICAL_PORT_0) {
- port->cached_edid = drm_get_edid(port->connector,
- &port->aux.ddc);
- drm_connector_set_tile_property(port->connector);
- }
+ if (!created)
+ drm_modeset_unlock(&mgr->base.lock);
+ else if (!port->connector && !port->input)
+ drm_dp_mst_port_add_connector(mstb, port);
- (*mgr->cbs->register_connector)(port->connector);
- }
+ if (send_link_addr && port->mstb)
+ drm_dp_send_link_address(mgr, port->mstb);
/* put reference to this port */
drm_dp_mst_topology_put_port(port);
return;
fail:
- /* Remove it from the port list */
- mutex_lock(&mgr->lock);
- list_del(&port->next);
- mutex_unlock(&mgr->lock);
-
- /* Drop the port list reference */
- drm_dp_mst_topology_put_port(port);
- /* And now drop our reference */
+ drm_dp_mst_topology_unlink_port(mgr, port);
drm_dp_mst_topology_put_port(port);
+ if (!created)
+ drm_modeset_unlock(&mgr->base.lock);
}
static void
drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
struct drm_dp_connection_status_notify *conn_stat)
{
+ struct drm_dp_mst_topology_mgr *mgr = mstb->mgr;
struct drm_dp_mst_port *port;
int old_ddps;
bool dowork = false;
@@ -2029,6 +2086,10 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch
*mstb,
if (!port)
return;
+ /* Locking is only needed if the port's exposed to userspace */
+ if (port->connector)
+ drm_modeset_lock(&mgr->base.lock, NULL);
+
old_ddps = port->ddps;
port->mcs = conn_stat->message_capability_status;
port->ldps = conn_stat->legacy_device_plug_status;
@@ -2054,10 +2115,12 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch
*mstb,
}
}
+ if (port->connector)
+ drm_modeset_unlock(&mgr->base.lock);
+
drm_dp_mst_topology_put_port(port);
if (dowork)
queue_work(system_long_wq, &mstb->mgr->work);
-
}
static struct drm_dp_mst_branch *drm_dp_get_mst_branch_device(struct
drm_dp_mst_topology_mgr *mgr,
@@ -2157,8 +2220,11 @@ static void drm_dp_check_and_send_link_address(struct
drm_dp_mst_topology_mgr *m
if (port->input || !port->ddps)
continue;
- if (!port->available_pbn)
+ if (!port->available_pbn) {
+ drm_modeset_lock(&mgr->base.lock, NULL);
drm_dp_send_enum_path_resources(mgr, mstb, port);
+ drm_modeset_unlock(&mgr->base.lock);
+ }
if (port->mstb)
mstb_child = drm_dp_mst_topology_get_mstb_validated(
@@ -2189,11 +2255,16 @@ static void drm_dp_mst_link_probe_work(struct
work_struct *work)
mstb = NULL;
}
mutex_unlock(&mgr->lock);
- if (mstb) {
- drm_dp_check_and_send_link_address(mgr, mstb);
- drm_dp_mst_topology_put_mstb(mstb);
+ if (!mstb) {
+ mutex_unlock(&mgr->probe_lock);
+ return;
}
+
+ drm_dp_check_and_send_link_address(mgr, mstb);
+ drm_dp_mst_topology_put_mstb(mstb);
+
mutex_unlock(&mgr->probe_lock);
+ drm_kms_helper_hotplug_event(dev);
}
static bool drm_dp_validate_guid(struct drm_dp_mst_topology_mgr *mgr,
@@ -2478,8 +2549,6 @@ static void drm_dp_send_link_address(struct
drm_dp_mst_topology_mgr *mgr,
drm_dp_mst_handle_link_address_port(mstb, mgr->dev,
&reply->ports[i]);
- drm_kms_helper_hotplug_event(mgr->dev);
-
out:
if (ret <= 0)
mstb->link_address_sent = false;
@@ -3274,13 +3343,14 @@ static int drm_dp_mst_handle_down_rep(struct
drm_dp_mst_topology_mgr *mgr)
return 0;
}
-static inline void
+static inline bool
drm_dp_mst_process_up_req(struct drm_dp_mst_topology_mgr *mgr,
struct drm_dp_pending_up_req *up_req)
{
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;
if (hdr->broadcast) {
const u8 *guid = NULL;
@@ -3298,16 +3368,17 @@ drm_dp_mst_process_up_req(struct drm_dp_mst_topology_mgr
*mgr,
if (!mstb) {
DRM_DEBUG_KMS("Got MST reply from unknown device %d\n",
hdr->lct);
- return;
+ return false;
}
/* 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);
- drm_kms_helper_hotplug_event(mgr->dev);
+ hotplug = true;
}
drm_dp_mst_topology_put_mstb(mstb);
+ return hotplug;
}
static void drm_dp_mst_up_req_work(struct work_struct *work)
@@ -3316,6 +3387,7 @@ static void drm_dp_mst_up_req_work(struct work_struct
*work)
container_of(work, struct drm_dp_mst_topology_mgr,
up_req_work);
struct drm_dp_pending_up_req *up_req;
+ bool send_hotplug = false;
mutex_lock(&mgr->probe_lock);
while (true) {
@@ -3330,10 +3402,13 @@ static void drm_dp_mst_up_req_work(struct work_struct
*work)
if (!up_req)
break;
- drm_dp_mst_process_up_req(mgr, up_req);
+ send_hotplug |= drm_dp_mst_process_up_req(mgr, up_req);
kfree(up_req);
}
mutex_unlock(&mgr->probe_lock);
+
+ if (send_hotplug)
+ drm_kms_helper_hotplug_event(mgr->dev);
}
static int drm_dp_mst_handle_up_req(struct drm_dp_mst_topology_mgr *mgr)
@@ -3441,22 +3516,31 @@ EXPORT_SYMBOL(drm_dp_mst_hpd_irq);
/**
* drm_dp_mst_detect_port() - get connection status for an MST port
* @connector: DRM connector for this port
+ * @ctx: The acquisition context to use for grabbing locks
* @mgr: manager for this port
- * @port: unverified pointer to a port
+ * @port: pointer to a port
*
- * This returns the current connection state for a port. It validates the
- * port pointer still exists so the caller doesn't require a reference
+ * This returns the current connection state for a port.
*/
-enum drm_connector_status drm_dp_mst_detect_port(struct drm_connector
*connector,
- struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port)
+int
+drm_dp_mst_detect_port(struct drm_connector *connector,
+ struct drm_modeset_acquire_ctx *ctx,
+ struct drm_dp_mst_topology_mgr *mgr,
+ struct drm_dp_mst_port *port)
{
- enum drm_connector_status status = connector_status_disconnected;
+ int ret;
/* we need to search for the port in the mgr in case it's gone */
port = drm_dp_mst_topology_get_port_validated(mgr, port);
if (!port)
return connector_status_disconnected;
+ ret = drm_modeset_lock(&mgr->base.lock, ctx);
+ if (ret)
+ goto out;
+
+ ret = connector_status_disconnected;
+
if (!port->ddps)
goto out;
@@ -3466,7 +3550,7 @@ enum drm_connector_status drm_dp_mst_detect_port(struct
drm_connector *connector
break;
case DP_PEER_DEVICE_SST_SINK:
- status = connector_status_connected;
+ ret = connector_status_connected;
/* for logical ports - cache the EDID */
if (port->port_num >= 8 && !port->cached_edid) {
port->cached_edid = drm_get_edid(connector, &port->aux.ddc);
@@ -3474,12 +3558,12 @@ enum drm_connector_status drm_dp_mst_detect_port(struct
drm_connector *connector
break;
case DP_PEER_DEVICE_DP_LEGACY_CONV:
if (port->ldps)
- status = connector_status_connected;
+ ret = connector_status_connected;
break;
}
out:
drm_dp_mst_topology_put_port(port);
- return status;
+ return ret;
}
EXPORT_SYMBOL(drm_dp_mst_detect_port);
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index bbcab27644dc..a9962846a503 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -391,20 +391,7 @@ static int intel_dp_mst_get_ddc_modes(struct drm_connector
*connector)
return ret;
}
-static enum drm_connector_status
-intel_dp_mst_detect(struct drm_connector *connector, bool force)
-{
- struct intel_connector *intel_connector = to_intel_connector(connector);
- struct intel_dp *intel_dp = intel_connector->mst_port;
-
- if (drm_connector_is_unregistered(connector))
- return connector_status_disconnected;
- return drm_dp_mst_detect_port(connector, &intel_dp->mst_mgr,
- intel_connector->port);
-}
-
static const struct drm_connector_funcs intel_dp_mst_connector_funcs = {
- .detect = intel_dp_mst_detect,
.fill_modes = drm_helper_probe_single_connector_modes,
.atomic_get_property = intel_digital_connector_atomic_get_property,
.atomic_set_property = intel_digital_connector_atomic_set_property,
@@ -465,11 +452,26 @@ static struct drm_encoder
*intel_mst_atomic_best_encoder(struct drm_connector *c
return &intel_dp->mst_encoders[crtc->pipe]->base.base;
}
+static int
+intel_dp_mst_detect(struct drm_connector *connector,
+ struct drm_modeset_acquire_ctx *ctx, bool force)
+{
+ struct intel_connector *intel_connector = to_intel_connector(connector);
+ struct intel_dp *intel_dp = intel_connector->mst_port;
+
+ if (drm_connector_is_unregistered(connector))
+ return connector_status_disconnected;
+
+ return drm_dp_mst_detect_port(connector, ctx, &intel_dp->mst_mgr,
+ intel_connector->port);
+}
+
static const struct drm_connector_helper_funcs
intel_dp_mst_connector_helper_funcs = {
.get_modes = intel_dp_mst_get_modes,
.mode_valid = intel_dp_mst_mode_valid,
.atomic_best_encoder = intel_mst_atomic_best_encoder,
.atomic_check = intel_dp_mst_atomic_check,
+ .detect_ctx = intel_dp_mst_detect,
};
static void intel_dp_mst_encoder_destroy(struct drm_encoder *encoder)
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c
b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index a13924ae1992..a9d6aa110cfd 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -986,20 +986,11 @@ nv50_mstc_atomic_check(struct drm_connector *connector,
return drm_dp_atomic_release_vcpi_slots(state, mgr, mstc->port);
}
-static const struct drm_connector_helper_funcs
-nv50_mstc_help = {
- .get_modes = nv50_mstc_get_modes,
- .mode_valid = nv50_mstc_mode_valid,
- .best_encoder = nv50_mstc_best_encoder,
- .atomic_best_encoder = nv50_mstc_atomic_best_encoder,
- .atomic_check = nv50_mstc_atomic_check,
-};
-
-static enum drm_connector_status
-nv50_mstc_detect(struct drm_connector *connector, bool force)
+static int
+nv50_mstc_detect(struct drm_connector *connector,
+ struct drm_modeset_acquire_ctx *ctx, bool force)
{
struct nv50_mstc *mstc = nv50_mstc(connector);
- enum drm_connector_status conn_status;
int ret;
if (drm_connector_is_unregistered(connector))
@@ -1009,14 +1000,24 @@ nv50_mstc_detect(struct drm_connector *connector, bool
force)
if (ret < 0 && ret != -EACCES)
return connector_status_disconnected;
- conn_status = drm_dp_mst_detect_port(connector, mstc->port->mgr,
- mstc->port);
+ ret = drm_dp_mst_detect_port(connector, ctx, mstc->port->mgr,
+ mstc->port);
pm_runtime_mark_last_busy(connector->dev->dev);
pm_runtime_put_autosuspend(connector->dev->dev);
- return conn_status;
+ return ret;
}
+static const struct drm_connector_helper_funcs
+nv50_mstc_help = {
+ .get_modes = nv50_mstc_get_modes,
+ .mode_valid = nv50_mstc_mode_valid,
+ .best_encoder = nv50_mstc_best_encoder,
+ .atomic_best_encoder = nv50_mstc_atomic_best_encoder,
+ .atomic_check = nv50_mstc_atomic_check,
+ .detect_ctx = nv50_mstc_detect,
+};
+
static void
nv50_mstc_destroy(struct drm_connector *connector)
{
@@ -1031,7 +1032,6 @@ nv50_mstc_destroy(struct drm_connector *connector)
static const struct drm_connector_funcs
nv50_mstc = {
.reset = nouveau_conn_reset,
- .detect = nv50_mstc_detect,
.fill_modes = drm_helper_probe_single_connector_modes,
.destroy = nv50_mstc_destroy,
.atomic_duplicate_state = nouveau_conn_atomic_duplicate_state,
diff --git a/drivers/gpu/drm/radeon/radeon_dp_mst.c
b/drivers/gpu/drm/radeon/radeon_dp_mst.c
index 2994f07fbad9..ee28f5b3785e 100644
--- a/drivers/gpu/drm/radeon/radeon_dp_mst.c
+++ b/drivers/gpu/drm/radeon/radeon_dp_mst.c
@@ -233,21 +233,26 @@ drm_encoder *radeon_mst_best_encoder(struct drm_connector
*connector)
return &radeon_connector->mst_encoder->base;
}
+static int
+radeon_dp_mst_detect(struct drm_connector *connector,
+ struct drm_modeset_acquire_ctx *ctx,
+ bool force)
+{
+ struct radeon_connector *radeon_connector + to_radeon_connector(connector);
+ struct radeon_connector *master = radeon_connector->mst_port;
+
+ return drm_dp_mst_detect_port(connector, ctx, &master->mst_mgr,
+ radeon_connector->port);
+}
+
static const struct drm_connector_helper_funcs
radeon_dp_mst_connector_helper_funcs = {
.get_modes = radeon_dp_mst_get_modes,
.mode_valid = radeon_dp_mst_mode_valid,
.best_encoder = radeon_mst_best_encoder,
+ .detect_ctx = radeon_dp_mst_detect,
};
-static enum drm_connector_status
-radeon_dp_mst_detect(struct drm_connector *connector, bool force)
-{
- struct radeon_connector *radeon_connector = to_radeon_connector(connector);
- struct radeon_connector *master = radeon_connector->mst_port;
-
- return drm_dp_mst_detect_port(connector, &master->mst_mgr,
radeon_connector->port);
-}
-
static void
radeon_dp_mst_connector_destroy(struct drm_connector *connector)
{
@@ -262,7 +267,6 @@ radeon_dp_mst_connector_destroy(struct drm_connector
*connector)
static const struct drm_connector_funcs radeon_dp_mst_connector_funcs = {
.dpms = drm_helper_connector_dpms,
- .detect = radeon_dp_mst_detect,
.fill_modes = drm_helper_probe_single_connector_modes,
.destroy = radeon_dp_mst_connector_destroy,
};
diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
index bccb5514e0ef..fd142db42cb0 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -45,21 +45,31 @@ struct drm_dp_vcpi {
/**
* struct drm_dp_mst_port - MST port
* @port_num: port number
- * @input: if this port is an input port.
- * @mcs: message capability status - DP 1.2 spec.
- * @ddps: DisplayPort Device Plug Status - DP 1.2
- * @pdt: Peer Device Type
- * @ldps: Legacy Device Plug Status
- * @dpcd_rev: DPCD revision of device on this port
- * @num_sdp_streams: Number of simultaneous streams
- * @num_sdp_stream_sinks: Number of stream sinks
- * @available_pbn: Available bandwidth for this port.
+ * @input: if this port is an input port. Protected by
+ * &drm_dp_mst_topology_mgr.base.lock.
+ * @mcs: message capability status - DP 1.2 spec. Protected by
+ * &drm_dp_mst_topology_mgr.base.lock.
+ * @ddps: DisplayPort Device Plug Status - DP 1.2. Protected by
+ * &drm_dp_mst_topology_mgr.base.lock.
+ * @pdt: Peer Device Type. Protected by
+ * &drm_dp_mst_topology_mgr.base.lock.
+ * @ldps: Legacy Device Plug Status. Protected by
+ * &drm_dp_mst_topology_mgr.base.lock.
+ * @dpcd_rev: DPCD revision of device on this port. Protected by
+ * &drm_dp_mst_topology_mgr.base.lock.
+ * @num_sdp_streams: Number of simultaneous streams. Protected by
+ * &drm_dp_mst_topology_mgr.base.lock.
+ * @num_sdp_stream_sinks: Number of stream sinks. Protected by
+ * &drm_dp_mst_topology_mgr.base.lock.
+ * @available_pbn: Available bandwidth for this port. Protected by
+ * &drm_dp_mst_topology_mgr.base.lock.
* @next: link to next port on this branch device
* @aux: i2c aux transport to talk to device connected to this port, protected
- * by &drm_dp_mst_topology_mgr.lock
+ * by &drm_dp_mst_topology_mgr.base.lock.
* @parent: branch device parent of this port
* @vcpi: Virtual Channel Payload info for this port.
- * @connector: DRM connector this port is connected to.
+ * @connector: DRM connector this port is connected to. Protected by
+ * &drm_dp_mst_topology_mgr.base.lock.
* @mgr: topology manager this port lives under.
*
* This structure represents an MST port endpoint on a device somewhere
@@ -653,7 +663,11 @@ int drm_dp_mst_topology_mgr_set_mst(struct
drm_dp_mst_topology_mgr *mgr, bool ms
int drm_dp_mst_hpd_irq(struct drm_dp_mst_topology_mgr *mgr, u8 *esi, bool
*handled);
-enum drm_connector_status drm_dp_mst_detect_port(struct drm_connector
*connector, struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port);
+int
+drm_dp_mst_detect_port(struct drm_connector *connector,
+ struct drm_modeset_acquire_ctx *ctx,
+ struct drm_dp_mst_topology_mgr *mgr,
+ struct drm_dp_mst_port *port);
bool drm_dp_mst_port_has_audio(struct drm_dp_mst_topology_mgr *mgr,
struct drm_dp_mst_port *port);
--
2.21.0
Lyude Paul
2019-Oct-22 02:41 UTC
[Nouveau] [PATCH v5 11/14] drm/amdgpu: Iterate through DRM connectors correctly
Currently, every single piece of code in amdgpu that loops through
connectors does it incorrectly and doesn't use the proper list iteration
helpers, drm_connector_list_iter_begin() and
drm_connector_list_iter_end(). Yeesh.
So, do that.
Cc: Juston Li <juston.li at intel.com>
Cc: Imre Deak <imre.deak at intel.com>
Cc: Ville Syrj?l? <ville.syrjala at linux.intel.com>
Cc: Harry Wentland <hwentlan at amd.com>
Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
Signed-off-by: Lyude Paul <lyude at redhat.com>
Reviewed-by: Alex Deucher <alexander.deucher at amd.com>
---
.../gpu/drm/amd/amdgpu/amdgpu_connectors.c | 13 +++++-
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 20 +++++++---
drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 5 ++-
drivers/gpu/drm/amd/amdgpu/amdgpu_encoders.c | 40 +++++++++++++------
drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c | 5 ++-
drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 34 ++++++++++++----
drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 34 ++++++++++++----
drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 40 ++++++++++++++-----
drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 34 ++++++++++++----
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 33 ++++++++-------
.../drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c | 10 ++++-
11 files changed, 195 insertions(+), 73 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
index d8729285f731..a62cbc8199de 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
@@ -1019,8 +1019,12 @@ amdgpu_connector_dvi_detect(struct drm_connector
*connector, bool force)
*/
if (amdgpu_connector->shared_ddc && (ret ==
connector_status_connected)) {
struct drm_connector *list_connector;
+ struct drm_connector_list_iter iter;
struct amdgpu_connector *list_amdgpu_connector;
- list_for_each_entry(list_connector,
&dev->mode_config.connector_list, head) {
+
+ drm_connector_list_iter_begin(dev, &iter);
+ drm_for_each_connector_iter(list_connector,
+ &iter) {
if (connector == list_connector)
continue;
list_amdgpu_connector = to_amdgpu_connector(list_connector);
@@ -1037,6 +1041,7 @@ amdgpu_connector_dvi_detect(struct drm_connector
*connector, bool force)
}
}
}
+ drm_connector_list_iter_end(&iter);
}
}
}
@@ -1494,6 +1499,7 @@ amdgpu_connector_add(struct amdgpu_device *adev,
{
struct drm_device *dev = adev->ddev;
struct drm_connector *connector;
+ struct drm_connector_list_iter iter;
struct amdgpu_connector *amdgpu_connector;
struct amdgpu_connector_atom_dig *amdgpu_dig_connector;
struct drm_encoder *encoder;
@@ -1508,10 +1514,12 @@ amdgpu_connector_add(struct amdgpu_device *adev,
return;
/* see if we already added it */
- list_for_each_entry(connector, &dev->mode_config.connector_list, head)
{
+ drm_connector_list_iter_begin(dev, &iter);
+ drm_for_each_connector_iter(connector, &iter) {
amdgpu_connector = to_amdgpu_connector(connector);
if (amdgpu_connector->connector_id == connector_id) {
amdgpu_connector->devices |= supported_device;
+ drm_connector_list_iter_end(&iter);
return;
}
if (amdgpu_connector->ddc_bus && i2c_bus->valid) {
@@ -1526,6 +1534,7 @@ amdgpu_connector_add(struct amdgpu_device *adev,
}
}
}
+ drm_connector_list_iter_end(&iter);
/* check if it's a dp bridge */
list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) {
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 5a1939dbd4e3..cff16f554f2e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -3007,6 +3007,7 @@ int amdgpu_device_suspend(struct drm_device *dev, bool
suspend, bool fbcon)
struct amdgpu_device *adev;
struct drm_crtc *crtc;
struct drm_connector *connector;
+ struct drm_connector_list_iter iter;
int r;
if (dev == NULL || dev->dev_private == NULL) {
@@ -3029,9 +3030,11 @@ int amdgpu_device_suspend(struct drm_device *dev, bool
suspend, bool fbcon)
if (!amdgpu_device_has_dc_support(adev)) {
/* turn off display hw */
drm_modeset_lock_all(dev);
- list_for_each_entry(connector, &dev->mode_config.connector_list, head)
{
- drm_helper_connector_dpms(connector, DRM_MODE_DPMS_OFF);
- }
+ drm_connector_list_iter_begin(dev, &iter);
+ drm_for_each_connector_iter(connector, &iter)
+ drm_helper_connector_dpms(connector,
+ DRM_MODE_DPMS_OFF);
+ drm_connector_list_iter_end(&iter);
drm_modeset_unlock_all(dev);
/* unpin the front buffers and cursors */
list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
@@ -3110,6 +3113,7 @@ int amdgpu_device_suspend(struct drm_device *dev, bool
suspend, bool fbcon)
int amdgpu_device_resume(struct drm_device *dev, bool resume, bool fbcon)
{
struct drm_connector *connector;
+ struct drm_connector_list_iter iter;
struct amdgpu_device *adev = dev->dev_private;
struct drm_crtc *crtc;
int r = 0;
@@ -3180,9 +3184,13 @@ int amdgpu_device_resume(struct drm_device *dev, bool
resume, bool fbcon)
/* turn on display hw */
drm_modeset_lock_all(dev);
- list_for_each_entry(connector, &dev->mode_config.connector_list,
head) {
- drm_helper_connector_dpms(connector, DRM_MODE_DPMS_ON);
- }
+
+ drm_connector_list_iter_begin(dev, &iter);
+ drm_for_each_connector_iter(connector, &iter)
+ drm_helper_connector_dpms(connector,
+ DRM_MODE_DPMS_ON);
+ drm_connector_list_iter_end(&iter);
+
drm_modeset_unlock_all(dev);
}
amdgpu_fbdev_set_suspend(adev, 0);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index 1d4aaa9580f4..d2dd59a95e8a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -370,11 +370,13 @@ void amdgpu_display_print_display_setup(struct drm_device
*dev)
struct amdgpu_connector *amdgpu_connector;
struct drm_encoder *encoder;
struct amdgpu_encoder *amdgpu_encoder;
+ struct drm_connector_list_iter iter;
uint32_t devices;
int i = 0;
+ drm_connector_list_iter_begin(dev, &iter);
DRM_INFO("AMDGPU Display Connectors\n");
- list_for_each_entry(connector, &dev->mode_config.connector_list, head)
{
+ drm_for_each_connector_iter(connector, &iter) {
amdgpu_connector = to_amdgpu_connector(connector);
DRM_INFO("Connector %d:\n", i);
DRM_INFO(" %s\n", connector->name);
@@ -438,6 +440,7 @@ void amdgpu_display_print_display_setup(struct drm_device
*dev)
}
i++;
}
+ drm_connector_list_iter_end(&iter);
}
/**
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_encoders.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_encoders.c
index 571a6dfb473e..61fcf247a638 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_encoders.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_encoders.c
@@ -37,12 +37,14 @@ amdgpu_link_encoder_connector(struct drm_device *dev)
{
struct amdgpu_device *adev = dev->dev_private;
struct drm_connector *connector;
+ struct drm_connector_list_iter iter;
struct amdgpu_connector *amdgpu_connector;
struct drm_encoder *encoder;
struct amdgpu_encoder *amdgpu_encoder;
+ drm_connector_list_iter_begin(dev, &iter);
/* walk the list and link encoders to connectors */
- list_for_each_entry(connector, &dev->mode_config.connector_list, head)
{
+ drm_for_each_connector_iter(connector, &iter) {
amdgpu_connector = to_amdgpu_connector(connector);
list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) {
amdgpu_encoder = to_amdgpu_encoder(encoder);
@@ -55,6 +57,7 @@ amdgpu_link_encoder_connector(struct drm_device *dev)
}
}
}
+ drm_connector_list_iter_end(&iter);
}
void amdgpu_encoder_set_active_device(struct drm_encoder *encoder)
@@ -62,8 +65,10 @@ void amdgpu_encoder_set_active_device(struct drm_encoder
*encoder)
struct drm_device *dev = encoder->dev;
struct amdgpu_encoder *amdgpu_encoder = to_amdgpu_encoder(encoder);
struct drm_connector *connector;
+ struct drm_connector_list_iter iter;
- list_for_each_entry(connector, &dev->mode_config.connector_list, head)
{
+ drm_connector_list_iter_begin(dev, &iter);
+ drm_for_each_connector_iter(connector, &iter) {
if (connector->encoder == encoder) {
struct amdgpu_connector *amdgpu_connector = to_amdgpu_connector(connector);
amdgpu_encoder->active_device = amdgpu_encoder->devices &
amdgpu_connector->devices;
@@ -72,6 +77,7 @@ void amdgpu_encoder_set_active_device(struct drm_encoder
*encoder)
amdgpu_connector->devices, encoder->encoder_type);
}
}
+ drm_connector_list_iter_end(&iter);
}
struct drm_connector *
@@ -79,15 +85,20 @@ amdgpu_get_connector_for_encoder(struct drm_encoder
*encoder)
{
struct drm_device *dev = encoder->dev;
struct amdgpu_encoder *amdgpu_encoder = to_amdgpu_encoder(encoder);
- struct drm_connector *connector;
+ struct drm_connector *connector, *found = NULL;
+ struct drm_connector_list_iter iter;
struct amdgpu_connector *amdgpu_connector;
- list_for_each_entry(connector, &dev->mode_config.connector_list, head)
{
+ drm_connector_list_iter_begin(dev, &iter);
+ drm_for_each_connector_iter(connector, &iter) {
amdgpu_connector = to_amdgpu_connector(connector);
- if (amdgpu_encoder->active_device & amdgpu_connector->devices)
- return connector;
+ if (amdgpu_encoder->active_device & amdgpu_connector->devices) {
+ found = connector;
+ break;
+ }
}
- return NULL;
+ drm_connector_list_iter_end(&iter);
+ return found;
}
struct drm_connector *
@@ -95,15 +106,20 @@ amdgpu_get_connector_for_encoder_init(struct drm_encoder
*encoder)
{
struct drm_device *dev = encoder->dev;
struct amdgpu_encoder *amdgpu_encoder = to_amdgpu_encoder(encoder);
- struct drm_connector *connector;
+ struct drm_connector *connector, *found = NULL;
+ struct drm_connector_list_iter iter;
struct amdgpu_connector *amdgpu_connector;
- list_for_each_entry(connector, &dev->mode_config.connector_list, head)
{
+ drm_connector_list_iter_begin(dev, &iter);
+ drm_for_each_connector_iter(connector, &iter) {
amdgpu_connector = to_amdgpu_connector(connector);
- if (amdgpu_encoder->devices & amdgpu_connector->devices)
- return connector;
+ if (amdgpu_encoder->devices & amdgpu_connector->devices) {
+ found = connector;
+ break;
+ }
}
- return NULL;
+ drm_connector_list_iter_end(&iter);
+ return found;
}
struct drm_encoder *amdgpu_get_external_encoder(struct drm_encoder *encoder)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
index 2a3f5ec298db..977e121204e6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
@@ -87,10 +87,13 @@ static void amdgpu_hotplug_work_func(struct work_struct
*work)
struct drm_device *dev = adev->ddev;
struct drm_mode_config *mode_config = &dev->mode_config;
struct drm_connector *connector;
+ struct drm_connector_list_iter iter;
mutex_lock(&mode_config->mutex);
- list_for_each_entry(connector, &mode_config->connector_list, head)
+ drm_connector_list_iter_begin(dev, &iter);
+ drm_for_each_connector_iter(connector, &iter)
amdgpu_connector_hotplug(connector);
+ drm_connector_list_iter_end(&iter);
mutex_unlock(&mode_config->mutex);
/* Just fire off a uevent and let userspace tell us what to do */
drm_helper_hpd_irq_event(dev);
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
index 645550e7caf5..be82871ac3bd 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
@@ -330,9 +330,11 @@ static void dce_v10_0_hpd_init(struct amdgpu_device *adev)
{
struct drm_device *dev = adev->ddev;
struct drm_connector *connector;
+ struct drm_connector_list_iter iter;
u32 tmp;
- list_for_each_entry(connector, &dev->mode_config.connector_list, head)
{
+ drm_connector_list_iter_begin(dev, &iter);
+ drm_for_each_connector_iter(connector, &iter) {
struct amdgpu_connector *amdgpu_connector = to_amdgpu_connector(connector);
if (amdgpu_connector->hpd.hpd >= adev->mode_info.num_hpd)
@@ -368,6 +370,7 @@ static void dce_v10_0_hpd_init(struct amdgpu_device *adev)
amdgpu_irq_get(adev, &adev->hpd_irq,
amdgpu_connector->hpd.hpd);
}
+ drm_connector_list_iter_end(&iter);
}
/**
@@ -382,9 +385,11 @@ static void dce_v10_0_hpd_fini(struct amdgpu_device *adev)
{
struct drm_device *dev = adev->ddev;
struct drm_connector *connector;
+ struct drm_connector_list_iter iter;
u32 tmp;
- list_for_each_entry(connector, &dev->mode_config.connector_list, head)
{
+ drm_connector_list_iter_begin(dev, &iter);
+ drm_for_each_connector_iter(connector, &iter) {
struct amdgpu_connector *amdgpu_connector = to_amdgpu_connector(connector);
if (amdgpu_connector->hpd.hpd >= adev->mode_info.num_hpd)
@@ -397,6 +402,7 @@ static void dce_v10_0_hpd_fini(struct amdgpu_device *adev)
amdgpu_irq_put(adev, &adev->hpd_irq,
amdgpu_connector->hpd.hpd);
}
+ drm_connector_list_iter_end(&iter);
}
static u32 dce_v10_0_hpd_get_gpio_reg(struct amdgpu_device *adev)
@@ -1219,10 +1225,12 @@ static void dce_v10_0_afmt_audio_select_pin(struct
drm_encoder *encoder)
static void dce_v10_0_audio_write_latency_fields(struct drm_encoder *encoder,
struct drm_display_mode *mode)
{
- struct amdgpu_device *adev = encoder->dev->dev_private;
+ struct drm_device *dev = encoder->dev;
+ struct amdgpu_device *adev = dev->dev_private;
struct amdgpu_encoder *amdgpu_encoder = to_amdgpu_encoder(encoder);
struct amdgpu_encoder_atom_dig *dig = amdgpu_encoder->enc_priv;
struct drm_connector *connector;
+ struct drm_connector_list_iter iter;
struct amdgpu_connector *amdgpu_connector = NULL;
u32 tmp;
int interlace = 0;
@@ -1230,12 +1238,14 @@ static void dce_v10_0_audio_write_latency_fields(struct
drm_encoder *encoder,
if (!dig || !dig->afmt || !dig->afmt->pin)
return;
- list_for_each_entry(connector,
&encoder->dev->mode_config.connector_list, head) {
+ drm_connector_list_iter_begin(dev, &iter);
+ drm_for_each_connector_iter(connector, &iter) {
if (connector->encoder == encoder) {
amdgpu_connector = to_amdgpu_connector(connector);
break;
}
}
+ drm_connector_list_iter_end(&iter);
if (!amdgpu_connector) {
DRM_ERROR("Couldn't find encoder's connector\n");
@@ -1261,10 +1271,12 @@ static void dce_v10_0_audio_write_latency_fields(struct
drm_encoder *encoder,
static void dce_v10_0_audio_write_speaker_allocation(struct drm_encoder
*encoder)
{
- struct amdgpu_device *adev = encoder->dev->dev_private;
+ struct drm_device *dev = encoder->dev;
+ struct amdgpu_device *adev = dev->dev_private;
struct amdgpu_encoder *amdgpu_encoder = to_amdgpu_encoder(encoder);
struct amdgpu_encoder_atom_dig *dig = amdgpu_encoder->enc_priv;
struct drm_connector *connector;
+ struct drm_connector_list_iter iter;
struct amdgpu_connector *amdgpu_connector = NULL;
u32 tmp;
u8 *sadb = NULL;
@@ -1273,12 +1285,14 @@ static void
dce_v10_0_audio_write_speaker_allocation(struct drm_encoder *encoder
if (!dig || !dig->afmt || !dig->afmt->pin)
return;
- list_for_each_entry(connector,
&encoder->dev->mode_config.connector_list, head) {
+ drm_connector_list_iter_begin(dev, &iter);
+ drm_for_each_connector_iter(connector, &iter) {
if (connector->encoder == encoder) {
amdgpu_connector = to_amdgpu_connector(connector);
break;
}
}
+ drm_connector_list_iter_end(&iter);
if (!amdgpu_connector) {
DRM_ERROR("Couldn't find encoder's connector\n");
@@ -1313,10 +1327,12 @@ static void
dce_v10_0_audio_write_speaker_allocation(struct drm_encoder *encoder
static void dce_v10_0_audio_write_sad_regs(struct drm_encoder *encoder)
{
- struct amdgpu_device *adev = encoder->dev->dev_private;
+ struct drm_device *dev = encoder->dev;
+ struct amdgpu_device *adev = dev->dev_private;
struct amdgpu_encoder *amdgpu_encoder = to_amdgpu_encoder(encoder);
struct amdgpu_encoder_atom_dig *dig = amdgpu_encoder->enc_priv;
struct drm_connector *connector;
+ struct drm_connector_list_iter iter;
struct amdgpu_connector *amdgpu_connector = NULL;
struct cea_sad *sads;
int i, sad_count;
@@ -1339,12 +1355,14 @@ static void dce_v10_0_audio_write_sad_regs(struct
drm_encoder *encoder)
if (!dig || !dig->afmt || !dig->afmt->pin)
return;
- list_for_each_entry(connector,
&encoder->dev->mode_config.connector_list, head) {
+ drm_connector_list_iter_begin(dev, &iter);
+ drm_for_each_connector_iter(connector, &iter) {
if (connector->encoder == encoder) {
amdgpu_connector = to_amdgpu_connector(connector);
break;
}
}
+ drm_connector_list_iter_end(&iter);
if (!amdgpu_connector) {
DRM_ERROR("Couldn't find encoder's connector\n");
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
index d9f470632b2c..bde48775cf1b 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
@@ -348,9 +348,11 @@ static void dce_v11_0_hpd_init(struct amdgpu_device *adev)
{
struct drm_device *dev = adev->ddev;
struct drm_connector *connector;
+ struct drm_connector_list_iter iter;
u32 tmp;
- list_for_each_entry(connector, &dev->mode_config.connector_list, head)
{
+ drm_connector_list_iter_begin(dev, &iter);
+ drm_for_each_connector_iter(connector, &iter) {
struct amdgpu_connector *amdgpu_connector = to_amdgpu_connector(connector);
if (amdgpu_connector->hpd.hpd >= adev->mode_info.num_hpd)
@@ -385,6 +387,7 @@ static void dce_v11_0_hpd_init(struct amdgpu_device *adev)
dce_v11_0_hpd_set_polarity(adev, amdgpu_connector->hpd.hpd);
amdgpu_irq_get(adev, &adev->hpd_irq, amdgpu_connector->hpd.hpd);
}
+ drm_connector_list_iter_end(&iter);
}
/**
@@ -399,9 +402,11 @@ static void dce_v11_0_hpd_fini(struct amdgpu_device *adev)
{
struct drm_device *dev = adev->ddev;
struct drm_connector *connector;
+ struct drm_connector_list_iter iter;
u32 tmp;
- list_for_each_entry(connector, &dev->mode_config.connector_list, head)
{
+ drm_connector_list_iter_begin(dev, &iter);
+ drm_for_each_connector_iter(connector, &iter) {
struct amdgpu_connector *amdgpu_connector = to_amdgpu_connector(connector);
if (amdgpu_connector->hpd.hpd >= adev->mode_info.num_hpd)
@@ -413,6 +418,7 @@ static void dce_v11_0_hpd_fini(struct amdgpu_device *adev)
amdgpu_irq_put(adev, &adev->hpd_irq, amdgpu_connector->hpd.hpd);
}
+ drm_connector_list_iter_end(&iter);
}
static u32 dce_v11_0_hpd_get_gpio_reg(struct amdgpu_device *adev)
@@ -1245,10 +1251,12 @@ static void dce_v11_0_afmt_audio_select_pin(struct
drm_encoder *encoder)
static void dce_v11_0_audio_write_latency_fields(struct drm_encoder *encoder,
struct drm_display_mode *mode)
{
- struct amdgpu_device *adev = encoder->dev->dev_private;
+ struct drm_device *dev = encoder->dev;
+ struct amdgpu_device *adev = dev->dev_private;
struct amdgpu_encoder *amdgpu_encoder = to_amdgpu_encoder(encoder);
struct amdgpu_encoder_atom_dig *dig = amdgpu_encoder->enc_priv;
struct drm_connector *connector;
+ struct drm_connector_list_iter iter;
struct amdgpu_connector *amdgpu_connector = NULL;
u32 tmp;
int interlace = 0;
@@ -1256,12 +1264,14 @@ static void dce_v11_0_audio_write_latency_fields(struct
drm_encoder *encoder,
if (!dig || !dig->afmt || !dig->afmt->pin)
return;
- list_for_each_entry(connector,
&encoder->dev->mode_config.connector_list, head) {
+ drm_connector_list_iter_begin(dev, &iter);
+ drm_for_each_connector_iter(connector, &iter) {
if (connector->encoder == encoder) {
amdgpu_connector = to_amdgpu_connector(connector);
break;
}
}
+ drm_connector_list_iter_end(&iter);
if (!amdgpu_connector) {
DRM_ERROR("Couldn't find encoder's connector\n");
@@ -1287,10 +1297,12 @@ static void dce_v11_0_audio_write_latency_fields(struct
drm_encoder *encoder,
static void dce_v11_0_audio_write_speaker_allocation(struct drm_encoder
*encoder)
{
- struct amdgpu_device *adev = encoder->dev->dev_private;
+ struct drm_device *dev = encoder->dev;
+ struct amdgpu_device *adev = dev->dev_private;
struct amdgpu_encoder *amdgpu_encoder = to_amdgpu_encoder(encoder);
struct amdgpu_encoder_atom_dig *dig = amdgpu_encoder->enc_priv;
struct drm_connector *connector;
+ struct drm_connector_list_iter iter;
struct amdgpu_connector *amdgpu_connector = NULL;
u32 tmp;
u8 *sadb = NULL;
@@ -1299,12 +1311,14 @@ static void
dce_v11_0_audio_write_speaker_allocation(struct drm_encoder *encoder
if (!dig || !dig->afmt || !dig->afmt->pin)
return;
- list_for_each_entry(connector,
&encoder->dev->mode_config.connector_list, head) {
+ drm_connector_list_iter_begin(dev, &iter);
+ drm_for_each_connector_iter(connector, &iter) {
if (connector->encoder == encoder) {
amdgpu_connector = to_amdgpu_connector(connector);
break;
}
}
+ drm_connector_list_iter_end(&iter);
if (!amdgpu_connector) {
DRM_ERROR("Couldn't find encoder's connector\n");
@@ -1339,10 +1353,12 @@ static void
dce_v11_0_audio_write_speaker_allocation(struct drm_encoder *encoder
static void dce_v11_0_audio_write_sad_regs(struct drm_encoder *encoder)
{
- struct amdgpu_device *adev = encoder->dev->dev_private;
+ struct drm_device *dev = encoder->dev;
+ struct amdgpu_device *adev = dev->dev_private;
struct amdgpu_encoder *amdgpu_encoder = to_amdgpu_encoder(encoder);
struct amdgpu_encoder_atom_dig *dig = amdgpu_encoder->enc_priv;
struct drm_connector *connector;
+ struct drm_connector_list_iter iter;
struct amdgpu_connector *amdgpu_connector = NULL;
struct cea_sad *sads;
int i, sad_count;
@@ -1365,12 +1381,14 @@ static void dce_v11_0_audio_write_sad_regs(struct
drm_encoder *encoder)
if (!dig || !dig->afmt || !dig->afmt->pin)
return;
- list_for_each_entry(connector,
&encoder->dev->mode_config.connector_list, head) {
+ drm_connector_list_iter_begin(dev, &iter);
+ drm_for_each_connector_iter(connector, &iter) {
if (connector->encoder == encoder) {
amdgpu_connector = to_amdgpu_connector(connector);
break;
}
}
+ drm_connector_list_iter_end(&iter);
if (!amdgpu_connector) {
DRM_ERROR("Couldn't find encoder's connector\n");
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
index 3eb2e7429269..65f61de931d7 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
@@ -281,9 +281,11 @@ static void dce_v6_0_hpd_init(struct amdgpu_device *adev)
{
struct drm_device *dev = adev->ddev;
struct drm_connector *connector;
+ struct drm_connector_list_iter iter;
u32 tmp;
- list_for_each_entry(connector, &dev->mode_config.connector_list, head)
{
+ drm_connector_list_iter_begin(dev, &iter);
+ drm_for_each_connector_iter(connector, &iter) {
struct amdgpu_connector *amdgpu_connector = to_amdgpu_connector(connector);
if (amdgpu_connector->hpd.hpd >= adev->mode_info.num_hpd)
@@ -309,7 +311,7 @@ static void dce_v6_0_hpd_init(struct amdgpu_device *adev)
dce_v6_0_hpd_set_polarity(adev, amdgpu_connector->hpd.hpd);
amdgpu_irq_get(adev, &adev->hpd_irq, amdgpu_connector->hpd.hpd);
}
-
+ drm_connector_list_iter_end(&iter);
}
/**
@@ -324,9 +326,11 @@ static void dce_v6_0_hpd_fini(struct amdgpu_device *adev)
{
struct drm_device *dev = adev->ddev;
struct drm_connector *connector;
+ struct drm_connector_list_iter iter;
u32 tmp;
- list_for_each_entry(connector, &dev->mode_config.connector_list, head)
{
+ drm_connector_list_iter_begin(dev, &iter);
+ drm_for_each_connector_iter(connector, &iter) {
struct amdgpu_connector *amdgpu_connector = to_amdgpu_connector(connector);
if (amdgpu_connector->hpd.hpd >= adev->mode_info.num_hpd)
@@ -338,6 +342,7 @@ static void dce_v6_0_hpd_fini(struct amdgpu_device *adev)
amdgpu_irq_put(adev, &adev->hpd_irq, amdgpu_connector->hpd.hpd);
}
+ drm_connector_list_iter_end(&iter);
}
static u32 dce_v6_0_hpd_get_gpio_reg(struct amdgpu_device *adev)
@@ -1124,20 +1129,24 @@ static void dce_v6_0_audio_select_pin(struct drm_encoder
*encoder)
static void dce_v6_0_audio_write_latency_fields(struct drm_encoder *encoder,
struct drm_display_mode *mode)
{
- struct amdgpu_device *adev = encoder->dev->dev_private;
+ struct drm_device *dev = encoder->dev;
+ struct amdgpu_device *adev = dev->dev_private;
struct amdgpu_encoder *amdgpu_encoder = to_amdgpu_encoder(encoder);
struct amdgpu_encoder_atom_dig *dig = amdgpu_encoder->enc_priv;
struct drm_connector *connector;
+ struct drm_connector_list_iter iter;
struct amdgpu_connector *amdgpu_connector = NULL;
int interlace = 0;
u32 tmp;
- list_for_each_entry(connector,
&encoder->dev->mode_config.connector_list, head) {
+ drm_connector_list_iter_begin(dev, &iter);
+ drm_for_each_connector_iter(connector, &iter) {
if (connector->encoder == encoder) {
amdgpu_connector = to_amdgpu_connector(connector);
break;
}
}
+ drm_connector_list_iter_end(&iter);
if (!amdgpu_connector) {
DRM_ERROR("Couldn't find encoder's connector\n");
@@ -1164,21 +1173,25 @@ static void dce_v6_0_audio_write_latency_fields(struct
drm_encoder *encoder,
static void dce_v6_0_audio_write_speaker_allocation(struct drm_encoder
*encoder)
{
- struct amdgpu_device *adev = encoder->dev->dev_private;
+ struct drm_device *dev = encoder->dev;
+ struct amdgpu_device *adev = dev->dev_private;
struct amdgpu_encoder *amdgpu_encoder = to_amdgpu_encoder(encoder);
struct amdgpu_encoder_atom_dig *dig = amdgpu_encoder->enc_priv;
struct drm_connector *connector;
+ struct drm_connector_list_iter iter;
struct amdgpu_connector *amdgpu_connector = NULL;
u8 *sadb = NULL;
int sad_count;
u32 tmp;
- list_for_each_entry(connector,
&encoder->dev->mode_config.connector_list, head) {
+ drm_connector_list_iter_begin(dev, &iter);
+ drm_for_each_connector_iter(connector, &iter) {
if (connector->encoder == encoder) {
amdgpu_connector = to_amdgpu_connector(connector);
break;
}
}
+ drm_connector_list_iter_end(&iter);
if (!amdgpu_connector) {
DRM_ERROR("Couldn't find encoder's connector\n");
@@ -1221,10 +1234,12 @@ static void
dce_v6_0_audio_write_speaker_allocation(struct drm_encoder *encoder)
static void dce_v6_0_audio_write_sad_regs(struct drm_encoder *encoder)
{
- struct amdgpu_device *adev = encoder->dev->dev_private;
+ struct drm_device *dev = encoder->dev;
+ struct amdgpu_device *adev = dev->dev_private;
struct amdgpu_encoder *amdgpu_encoder = to_amdgpu_encoder(encoder);
struct amdgpu_encoder_atom_dig *dig = amdgpu_encoder->enc_priv;
struct drm_connector *connector;
+ struct drm_connector_list_iter iter;
struct amdgpu_connector *amdgpu_connector = NULL;
struct cea_sad *sads;
int i, sad_count;
@@ -1244,12 +1259,14 @@ static void dce_v6_0_audio_write_sad_regs(struct
drm_encoder *encoder)
{ ixAZALIA_F0_CODEC_PIN_CONTROL_AUDIO_DESCRIPTOR13,
HDMI_AUDIO_CODING_TYPE_WMA_PRO },
};
- list_for_each_entry(connector,
&encoder->dev->mode_config.connector_list, head) {
+ drm_connector_list_iter_begin(dev, &iter);
+ drm_for_each_connector_iter(connector, &iter) {
if (connector->encoder == encoder) {
amdgpu_connector = to_amdgpu_connector(connector);
break;
}
}
+ drm_connector_list_iter_end(&iter);
if (!amdgpu_connector) {
DRM_ERROR("Couldn't find encoder's connector\n");
@@ -1632,6 +1649,7 @@ static void dce_v6_0_afmt_setmode(struct drm_encoder
*encoder,
struct amdgpu_encoder *amdgpu_encoder = to_amdgpu_encoder(encoder);
struct amdgpu_encoder_atom_dig *dig = amdgpu_encoder->enc_priv;
struct drm_connector *connector;
+ struct drm_connector_list_iter iter;
struct amdgpu_connector *amdgpu_connector = NULL;
int em = amdgpu_atombios_encoder_get_encoder_mode(encoder);
int bpc = 8;
@@ -1639,12 +1657,14 @@ static void dce_v6_0_afmt_setmode(struct drm_encoder
*encoder,
if (!dig || !dig->afmt)
return;
- list_for_each_entry(connector,
&encoder->dev->mode_config.connector_list, head) {
+ drm_connector_list_iter_begin(dev, &iter);
+ drm_for_each_connector_iter(connector, &iter) {
if (connector->encoder == encoder) {
amdgpu_connector = to_amdgpu_connector(connector);
break;
}
}
+ drm_connector_list_iter_end(&iter);
if (!amdgpu_connector) {
DRM_ERROR("Couldn't find encoder's connector\n");
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
index a16c5e9e610e..e5f50882a51d 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
@@ -275,9 +275,11 @@ static void dce_v8_0_hpd_init(struct amdgpu_device *adev)
{
struct drm_device *dev = adev->ddev;
struct drm_connector *connector;
+ struct drm_connector_list_iter iter;
u32 tmp;
- list_for_each_entry(connector, &dev->mode_config.connector_list, head)
{
+ drm_connector_list_iter_begin(dev, &iter);
+ drm_for_each_connector_iter(connector, &iter) {
struct amdgpu_connector *amdgpu_connector = to_amdgpu_connector(connector);
if (amdgpu_connector->hpd.hpd >= adev->mode_info.num_hpd)
@@ -303,6 +305,7 @@ static void dce_v8_0_hpd_init(struct amdgpu_device *adev)
dce_v8_0_hpd_set_polarity(adev, amdgpu_connector->hpd.hpd);
amdgpu_irq_get(adev, &adev->hpd_irq, amdgpu_connector->hpd.hpd);
}
+ drm_connector_list_iter_end(&iter);
}
/**
@@ -317,9 +320,11 @@ static void dce_v8_0_hpd_fini(struct amdgpu_device *adev)
{
struct drm_device *dev = adev->ddev;
struct drm_connector *connector;
+ struct drm_connector_list_iter iter;
u32 tmp;
- list_for_each_entry(connector, &dev->mode_config.connector_list, head)
{
+ drm_connector_list_iter_begin(dev, &iter);
+ drm_for_each_connector_iter(connector, &iter) {
struct amdgpu_connector *amdgpu_connector = to_amdgpu_connector(connector);
if (amdgpu_connector->hpd.hpd >= adev->mode_info.num_hpd)
@@ -331,6 +336,7 @@ static void dce_v8_0_hpd_fini(struct amdgpu_device *adev)
amdgpu_irq_put(adev, &adev->hpd_irq, amdgpu_connector->hpd.hpd);
}
+ drm_connector_list_iter_end(&iter);
}
static u32 dce_v8_0_hpd_get_gpio_reg(struct amdgpu_device *adev)
@@ -1157,10 +1163,12 @@ static void dce_v8_0_afmt_audio_select_pin(struct
drm_encoder *encoder)
static void dce_v8_0_audio_write_latency_fields(struct drm_encoder *encoder,
struct drm_display_mode *mode)
{
- struct amdgpu_device *adev = encoder->dev->dev_private;
+ struct drm_device *dev = encoder->dev;
+ struct amdgpu_device *adev = dev->dev_private;
struct amdgpu_encoder *amdgpu_encoder = to_amdgpu_encoder(encoder);
struct amdgpu_encoder_atom_dig *dig = amdgpu_encoder->enc_priv;
struct drm_connector *connector;
+ struct drm_connector_list_iter iter;
struct amdgpu_connector *amdgpu_connector = NULL;
u32 tmp = 0, offset;
@@ -1169,12 +1177,14 @@ static void dce_v8_0_audio_write_latency_fields(struct
drm_encoder *encoder,
offset = dig->afmt->pin->offset;
- list_for_each_entry(connector,
&encoder->dev->mode_config.connector_list, head) {
+ drm_connector_list_iter_begin(dev, &iter);
+ drm_for_each_connector_iter(connector, &iter) {
if (connector->encoder == encoder) {
amdgpu_connector = to_amdgpu_connector(connector);
break;
}
}
+ drm_connector_list_iter_end(&iter);
if (!amdgpu_connector) {
DRM_ERROR("Couldn't find encoder's connector\n");
@@ -1214,10 +1224,12 @@ static void dce_v8_0_audio_write_latency_fields(struct
drm_encoder *encoder,
static void dce_v8_0_audio_write_speaker_allocation(struct drm_encoder
*encoder)
{
- struct amdgpu_device *adev = encoder->dev->dev_private;
+ struct drm_device *dev = encoder->dev;
+ struct amdgpu_device *adev = dev->dev_private;
struct amdgpu_encoder *amdgpu_encoder = to_amdgpu_encoder(encoder);
struct amdgpu_encoder_atom_dig *dig = amdgpu_encoder->enc_priv;
struct drm_connector *connector;
+ struct drm_connector_list_iter iter;
struct amdgpu_connector *amdgpu_connector = NULL;
u32 offset, tmp;
u8 *sadb = NULL;
@@ -1228,12 +1240,14 @@ static void
dce_v8_0_audio_write_speaker_allocation(struct drm_encoder *encoder)
offset = dig->afmt->pin->offset;
- list_for_each_entry(connector,
&encoder->dev->mode_config.connector_list, head) {
+ drm_connector_list_iter_begin(dev, &iter);
+ drm_for_each_connector_iter(connector, &iter) {
if (connector->encoder == encoder) {
amdgpu_connector = to_amdgpu_connector(connector);
break;
}
}
+ drm_connector_list_iter_end(&iter);
if (!amdgpu_connector) {
DRM_ERROR("Couldn't find encoder's connector\n");
@@ -1263,11 +1277,13 @@ static void
dce_v8_0_audio_write_speaker_allocation(struct drm_encoder *encoder)
static void dce_v8_0_audio_write_sad_regs(struct drm_encoder *encoder)
{
- struct amdgpu_device *adev = encoder->dev->dev_private;
+ struct drm_device *dev = encoder->dev;
+ struct amdgpu_device *adev = dev->dev_private;
struct amdgpu_encoder *amdgpu_encoder = to_amdgpu_encoder(encoder);
struct amdgpu_encoder_atom_dig *dig = amdgpu_encoder->enc_priv;
u32 offset;
struct drm_connector *connector;
+ struct drm_connector_list_iter iter;
struct amdgpu_connector *amdgpu_connector = NULL;
struct cea_sad *sads;
int i, sad_count;
@@ -1292,12 +1308,14 @@ static void dce_v8_0_audio_write_sad_regs(struct
drm_encoder *encoder)
offset = dig->afmt->pin->offset;
- list_for_each_entry(connector,
&encoder->dev->mode_config.connector_list, head) {
+ drm_connector_list_iter_begin(dev, &iter);
+ drm_for_each_connector_iter(connector, &iter) {
if (connector->encoder == encoder) {
amdgpu_connector = to_amdgpu_connector(connector);
break;
}
}
+ drm_connector_list_iter_end(&iter);
if (!amdgpu_connector) {
DRM_ERROR("Couldn't find encoder's connector\n");
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index c67d3c41db19..887bc1d5d9e2 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -897,27 +897,29 @@ static int detect_mst_link_for_all_connectors(struct
drm_device *dev)
{
struct amdgpu_dm_connector *aconnector;
struct drm_connector *connector;
+ struct drm_connector_list_iter iter;
int ret = 0;
- drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
-
- list_for_each_entry(connector, &dev->mode_config.connector_list, head)
{
+ drm_connector_list_iter_begin(dev, &iter);
+ drm_for_each_connector_iter(connector, &iter) {
aconnector = to_amdgpu_dm_connector(connector);
if (aconnector->dc_link->type == dc_connection_mst_branch &&
aconnector->mst_mgr.aux) {
DRM_DEBUG_DRIVER("DM_MST: starting TM on aconnector: %p [id:
%d]\n",
- aconnector, aconnector->base.base.id);
+ aconnector,
+ aconnector->base.base.id);
ret = drm_dp_mst_topology_mgr_set_mst(&aconnector->mst_mgr, true);
if (ret < 0) {
DRM_ERROR("DM_MST: Failed to start MST\n");
- ((struct dc_link *)aconnector->dc_link)->type = dc_connection_single;
- return ret;
- }
+ aconnector->dc_link->type + dc_connection_single;
+ break;
}
+ }
}
+ drm_connector_list_iter_end(&iter);
- drm_modeset_unlock(&dev->mode_config.connection_mutex);
return ret;
}
@@ -955,14 +957,13 @@ static void s3_handle_mst(struct drm_device *dev, bool
suspend)
{
struct amdgpu_dm_connector *aconnector;
struct drm_connector *connector;
+ struct drm_connector_list_iter iter;
struct drm_dp_mst_topology_mgr *mgr;
int ret;
bool need_hotplug = false;
- drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
-
- list_for_each_entry(connector, &dev->mode_config.connector_list,
- head) {
+ drm_connector_list_iter_begin(dev, &iter);
+ drm_for_each_connector_iter(connector, &iter) {
aconnector = to_amdgpu_dm_connector(connector);
if (aconnector->dc_link->type != dc_connection_mst_branch ||
aconnector->mst_port)
@@ -980,8 +981,7 @@ static void s3_handle_mst(struct drm_device *dev, bool
suspend)
}
}
}
-
- drm_modeset_unlock(&dev->mode_config.connection_mutex);
+ drm_connector_list_iter_end(&iter);
if (need_hotplug)
drm_kms_helper_hotplug_event(dev);
@@ -1163,6 +1163,7 @@ static int dm_resume(void *handle)
struct amdgpu_display_manager *dm = &adev->dm;
struct amdgpu_dm_connector *aconnector;
struct drm_connector *connector;
+ struct drm_connector_list_iter iter;
struct drm_crtc *crtc;
struct drm_crtc_state *new_crtc_state;
struct dm_crtc_state *dm_new_crtc_state;
@@ -1195,7 +1196,8 @@ static int dm_resume(void *handle)
amdgpu_dm_irq_resume_early(adev);
/* Do detection*/
- list_for_each_entry(connector, &ddev->mode_config.connector_list, head)
{
+ drm_connector_list_iter_begin(ddev, &iter);
+ drm_for_each_connector_iter(connector, &iter) {
aconnector = to_amdgpu_dm_connector(connector);
/*
@@ -1223,6 +1225,7 @@ static int dm_resume(void *handle)
amdgpu_dm_update_connector_after_detect(aconnector);
mutex_unlock(&aconnector->hpd_lock);
}
+ drm_connector_list_iter_end(&iter);
/* Force mode set in atomic commit */
for_each_new_crtc_in_state(dm->cached_state, crtc, new_crtc_state, i)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
index fa5d503d379c..64445c4cc4c2 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
@@ -732,8 +732,10 @@ void amdgpu_dm_hpd_init(struct amdgpu_device *adev)
{
struct drm_device *dev = adev->ddev;
struct drm_connector *connector;
+ struct drm_connector_list_iter iter;
- list_for_each_entry(connector, &dev->mode_config.connector_list, head)
{
+ drm_connector_list_iter_begin(dev, &iter);
+ drm_for_each_connector_iter(connector, &iter) {
struct amdgpu_dm_connector *amdgpu_dm_connector
to_amdgpu_dm_connector(connector);
@@ -751,6 +753,7 @@ void amdgpu_dm_hpd_init(struct amdgpu_device *adev)
true);
}
}
+ drm_connector_list_iter_end(&iter);
}
/**
@@ -765,8 +768,10 @@ void amdgpu_dm_hpd_fini(struct amdgpu_device *adev)
{
struct drm_device *dev = adev->ddev;
struct drm_connector *connector;
+ struct drm_connector_list_iter iter;
- list_for_each_entry(connector, &dev->mode_config.connector_list, head)
{
+ drm_connector_list_iter_begin(dev, &iter);
+ drm_for_each_connector_iter(connector, &iter) {
struct amdgpu_dm_connector *amdgpu_dm_connector
to_amdgpu_dm_connector(connector);
const struct dc_link *dc_link = amdgpu_dm_connector->dc_link;
@@ -779,4 +784,5 @@ void amdgpu_dm_hpd_fini(struct amdgpu_device *adev)
false);
}
}
+ drm_connector_list_iter_end(&iter);
}
--
2.21.0
Lyude Paul
2019-Oct-22 02:41 UTC
[Nouveau] [PATCH v5 12/14] drm/amdgpu/dm: Resume short HPD IRQs before resuming MST topology
Since we're going to be reprobing the entire topology state on resume
now using sideband transactions, we need to ensure that we actually have
short HPD irqs enabled before calling drm_dp_mst_topology_mgr_resume().
So, do that.
Changes since v3:
* Fix typo in comments
Cc: Juston Li <juston.li at intel.com>
Cc: Imre Deak <imre.deak at intel.com>
Cc: Ville Syrj?l? <ville.syrjala at linux.intel.com>
Cc: Harry Wentland <hwentlan at amd.com>
Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
Signed-off-by: Lyude Paul <lyude at redhat.com>
Acked-by: Alex Deucher <alexander.deucher at amd.com>
---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 887bc1d5d9e2..8f67d301ad81 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -1186,15 +1186,15 @@ static int dm_resume(void *handle)
/* program HPD filter */
dc_resume(dm->dc);
- /* On resume we need to rewrite the MSTM control bits to enamble MST*/
- s3_handle_mst(ddev, false);
-
/*
* early enable HPD Rx IRQ, should be done before set mode as short
* pulse interrupts are used for MST
*/
amdgpu_dm_irq_resume_early(adev);
+ /* On resume we need to rewrite the MSTM control bits to enable MST*/
+ s3_handle_mst(ddev, false);
+
/* Do detection*/
drm_connector_list_iter_begin(ddev, &iter);
drm_for_each_connector_iter(connector, &iter) {
--
2.21.0
Lyude Paul
2019-Oct-22 02:42 UTC
[Nouveau] [PATCH v5 13/14] drm/dp_mst: Add basic topology reprobing when resuming
Finally! For a very long time, our MST helpers have had one very
annoying issue: They don't know how to reprobe the topology state when
coming out of suspend. This means that if a user has a machine connected
to an MST topology and decides to suspend their machine, we lose all
topology changes that happened during that period. That can be a big
problem if the machine was connected to a different topology on the same
port before resuming, as we won't bother reprobing any of the ports and
likely cause the user's monitors not to come back up as expected.
So, we start fixing this by teaching our MST helpers how to reprobe the
link addresses of each connected topology when resuming. As it turns
out, the behavior that we want here is identical to the behavior we want
when initially probing a newly connected MST topology, with a couple of
important differences:
- We need to be more careful about handling the potential races between
events from the MST hub that could change the topology state as we're
performing the link address reprobe
- We need to be more careful about handling unlikely state changes on
ports - such as an input port turning into an output port, something
that would be far more likely to happen in situations like the MST hub
we're connected to being changed while we're suspend
Both of which have been solved by previous commits. That leaves one
requirement:
- We need to prune any MST ports in our in-memory topology state that
were present when suspending, but have not appeared in the post-resume
link address response from their parent branch device
Which we can now handle in this commit by modifying
drm_dp_send_link_address(). We then introduce suspend/resume reprobing
by introducing drm_dp_mst_topology_mgr_invalidate_mstb(), which we call
in drm_dp_mst_topology_mgr_suspend() to traverse the in-memory topology
state to indicate that each mstb needs it's link address resent and PBN
resources reprobed.
On resume, we start back up &mgr->work and have it reprobe the topology
in the same way we would on a hotplug, removing any leftover ports that
no longer appear in the topology state.
Changes since v4:
* Split indenting changes in drm_dp_mst_topology_mgr_resume() into a
separate patch
* Only fire hotplugs when something has actually changed after a link
address probe
* Don't try to change port->connector at all on ports, just throw out
ports that need their connectors removed to make things easier.
Cc: Juston Li <juston.li at intel.com>
Cc: Imre Deak <imre.deak at intel.com>
Cc: Ville Syrj?l? <ville.syrjala at linux.intel.com>
Cc: Harry Wentland <hwentlan at amd.com>
Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
Reviewed-by: Sean Paul <sean at poorly.run>
Signed-off-by: Lyude Paul <lyude at redhat.com>
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +-
drivers/gpu/drm/drm_dp_mst_topology.c | 182 ++++++++++++++----
drivers/gpu/drm/i915/display/intel_dp.c | 3 +-
drivers/gpu/drm/nouveau/dispnv50/disp.c | 6 +-
include/drm/drm_dp_mst_helper.h | 3 +-
5 files changed, 156 insertions(+), 40 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 8f67d301ad81..6c34f640f419 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -974,7 +974,7 @@ static void s3_handle_mst(struct drm_device *dev, bool
suspend)
if (suspend) {
drm_dp_mst_topology_mgr_suspend(mgr);
} else {
- ret = drm_dp_mst_topology_mgr_resume(mgr);
+ ret = drm_dp_mst_topology_mgr_resume(mgr, true);
if (ret < 0) {
drm_dp_mst_topology_mgr_set_mst(mgr, false);
need_hotplug = true;
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
b/drivers/gpu/drm/drm_dp_mst_topology.c
index d486d15aa002..428160270482 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -67,8 +67,8 @@ static int drm_dp_send_dpcd_write(struct
drm_dp_mst_topology_mgr *mgr,
struct drm_dp_mst_port *port,
int offset, int size, u8 *bytes);
-static void drm_dp_send_link_address(struct drm_dp_mst_topology_mgr *mgr,
- struct drm_dp_mst_branch *mstb);
+static int drm_dp_send_link_address(struct drm_dp_mst_topology_mgr *mgr,
+ struct drm_dp_mst_branch *mstb);
static int drm_dp_send_enum_path_resources(struct drm_dp_mst_topology_mgr *mgr,
struct drm_dp_mst_branch *mstb,
struct drm_dp_mst_port *port);
@@ -1977,7 +1977,7 @@ drm_dp_mst_add_port(struct drm_device *dev,
return port;
}
-static void
+static int
drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb,
struct drm_device *dev,
struct drm_dp_link_addr_reply_port *port_msg)
@@ -1986,33 +1986,45 @@ drm_dp_mst_handle_link_address_port(struct
drm_dp_mst_branch *mstb,
struct drm_dp_mst_port *port;
int old_ddps = 0, ret;
u8 new_pdt = DP_PEER_DEVICE_NONE;
- bool created = false, send_link_addr = false;
+ bool created = false, send_link_addr = false, changed = false;
port = drm_dp_get_port(mstb, port_msg->port_number);
if (!port) {
port = drm_dp_mst_add_port(dev, mgr, mstb,
port_msg->port_number);
if (!port)
- return;
+ return -ENOMEM;
created = true;
- } else if (port_msg->input_port && !port->input &&
port->connector) {
- /* Destroying the connector is impossible in this context, so
- * replace the port with a new one
+ changed = true;
+ } else if (!port->input && port_msg->input_port &&
port->connector) {
+ /* Since port->connector can't be changed here, we create a
+ * new port if input_port changes from 0 to 1
*/
drm_dp_mst_topology_unlink_port(mgr, port);
drm_dp_mst_topology_put_port(port);
-
port = drm_dp_mst_add_port(dev, mgr, mstb,
port_msg->port_number);
if (!port)
- return;
+ return -ENOMEM;
+ changed = true;
created = true;
- } else {
- /* Locking is only needed when the port has a connector
- * exposed to userspace
+ } else if (port->input && !port_msg->input_port) {
+ changed = true;
+ } else if (port->connector) {
+ /* We're updating a port that's exposed to userspace, so do it
+ * under lock
*/
drm_modeset_lock(&mgr->base.lock, NULL);
+
old_ddps = port->ddps;
+ changed = port->ddps != port_msg->ddps ||
+ (port->ddps &&
+ (port->ldps != port_msg->legacy_device_plug_status ||
+ port->dpcd_rev != port_msg->dpcd_revision ||
+ port->mcs != port_msg->mcs ||
+ port->pdt != port_msg->peer_device_type ||
+ port->num_sdp_stream_sinks !+ port_msg->num_sdp_stream_sinks));
}
port->input = port_msg->input_port;
@@ -2054,23 +2066,38 @@ drm_dp_mst_handle_link_address_port(struct
drm_dp_mst_branch *mstb,
goto fail;
}
- if (!created)
+ /*
+ * If this port wasn't just created, then we're reprobing because
+ * we're coming out of suspend. In this case, always resend the link
+ * address if there's an MSTB on this port
+ */
+ if (!created && port->pdt == DP_PEER_DEVICE_MST_BRANCHING)
+ send_link_addr = true;
+
+ if (port->connector)
drm_modeset_unlock(&mgr->base.lock);
- else if (!port->connector && !port->input)
+ else if (!port->input)
drm_dp_mst_port_add_connector(mstb, port);
- if (send_link_addr && port->mstb)
- drm_dp_send_link_address(mgr, port->mstb);
+ if (send_link_addr && port->mstb) {
+ ret = drm_dp_send_link_address(mgr, port->mstb);
+ if (ret == 1) /* MSTB below us changed */
+ changed = true;
+ else if (ret < 0)
+ goto fail_put;
+ }
/* put reference to this port */
drm_dp_mst_topology_put_port(port);
- return;
+ return changed;
fail:
drm_dp_mst_topology_unlink_port(mgr, port);
- drm_dp_mst_topology_put_port(port);
- if (!created)
+ if (port->connector)
drm_modeset_unlock(&mgr->base.lock);
+fail_put:
+ drm_dp_mst_topology_put_port(port);
+ return ret;
}
static void
@@ -2230,13 +2257,20 @@ drm_dp_get_mst_branch_device_by_guid(struct
drm_dp_mst_topology_mgr *mgr,
return mstb;
}
-static void drm_dp_check_and_send_link_address(struct drm_dp_mst_topology_mgr
*mgr,
+static int drm_dp_check_and_send_link_address(struct drm_dp_mst_topology_mgr
*mgr,
struct drm_dp_mst_branch *mstb)
{
struct drm_dp_mst_port *port;
+ int ret;
+ bool changed = false;
- if (!mstb->link_address_sent)
- drm_dp_send_link_address(mgr, mstb);
+ if (!mstb->link_address_sent) {
+ ret = drm_dp_send_link_address(mgr, mstb);
+ if (ret == 1)
+ changed = true;
+ else if (ret < 0)
+ return ret;
+ }
list_for_each_entry(port, &mstb->ports, next) {
struct drm_dp_mst_branch *mstb_child = NULL;
@@ -2248,6 +2282,7 @@ static void drm_dp_check_and_send_link_address(struct
drm_dp_mst_topology_mgr *m
drm_modeset_lock(&mgr->base.lock, NULL);
drm_dp_send_enum_path_resources(mgr, mstb, port);
drm_modeset_unlock(&mgr->base.lock);
+ changed = true;
}
if (port->mstb)
@@ -2255,10 +2290,17 @@ static void drm_dp_check_and_send_link_address(struct
drm_dp_mst_topology_mgr *m
mgr, port->mstb);
if (mstb_child) {
- drm_dp_check_and_send_link_address(mgr, mstb_child);
+ ret = drm_dp_check_and_send_link_address(mgr,
+ mstb_child);
drm_dp_mst_topology_put_mstb(mstb_child);
+ if (ret == 1)
+ changed = true;
+ else if (ret < 0)
+ return ret;
}
}
+
+ return changed;
}
static void drm_dp_mst_link_probe_work(struct work_struct *work)
@@ -2284,11 +2326,12 @@ static void drm_dp_mst_link_probe_work(struct
work_struct *work)
return;
}
- drm_dp_check_and_send_link_address(mgr, mstb);
+ ret = drm_dp_check_and_send_link_address(mgr, mstb);
drm_dp_mst_topology_put_mstb(mstb);
mutex_unlock(&mgr->probe_lock);
- drm_kms_helper_hotplug_event(dev);
+ if (ret)
+ drm_kms_helper_hotplug_event(dev);
}
static bool drm_dp_validate_guid(struct drm_dp_mst_topology_mgr *mgr,
@@ -2534,16 +2577,18 @@ drm_dp_dump_link_address(struct
drm_dp_link_address_ack_reply *reply)
}
}
-static void drm_dp_send_link_address(struct drm_dp_mst_topology_mgr *mgr,
+static int drm_dp_send_link_address(struct drm_dp_mst_topology_mgr *mgr,
struct drm_dp_mst_branch *mstb)
{
struct drm_dp_sideband_msg_tx *txmsg;
struct drm_dp_link_address_ack_reply *reply;
- int i, len, ret;
+ struct drm_dp_mst_port *port, *tmp;
+ int i, len, ret, port_mask = 0;
+ bool changed = false;
txmsg = kzalloc(sizeof(*txmsg), GFP_KERNEL);
if (!txmsg)
- return;
+ return -ENOMEM;
txmsg->dst = mstb;
len = build_link_address(txmsg);
@@ -2569,14 +2614,39 @@ static void drm_dp_send_link_address(struct
drm_dp_mst_topology_mgr *mgr,
drm_dp_check_mstb_guid(mstb, reply->guid);
- for (i = 0; i < reply->nports; i++)
- drm_dp_mst_handle_link_address_port(mstb, mgr->dev,
- &reply->ports[i]);
+ for (i = 0; i < reply->nports; i++) {
+ port_mask |= BIT(reply->ports[i].port_number);
+ ret = drm_dp_mst_handle_link_address_port(mstb, mgr->dev,
+ &reply->ports[i]);
+ if (ret == 1)
+ changed = true;
+ else if (ret < 0)
+ goto out;
+ }
+
+ /* Prune any ports that are currently a part of mstb in our in-memory
+ * topology, but were not seen in this link address. Usually this
+ * means that they were removed while the topology was out of sync,
+ * e.g. during suspend/resume
+ */
+ mutex_lock(&mgr->lock);
+ list_for_each_entry_safe(port, tmp, &mstb->ports, next) {
+ if (port_mask & BIT(port->port_num))
+ continue;
+
+ DRM_DEBUG_KMS("port %d was not in link address, removing\n",
+ port->port_num);
+ list_del(&port->next);
+ drm_dp_mst_topology_put_port(port);
+ changed = true;
+ }
+ mutex_unlock(&mgr->lock);
out:
if (ret <= 0)
mstb->link_address_sent = false;
kfree(txmsg);
+ return ret < 0 ? ret : changed;
}
static int
@@ -3181,6 +3251,23 @@ int drm_dp_mst_topology_mgr_set_mst(struct
drm_dp_mst_topology_mgr *mgr, bool ms
}
EXPORT_SYMBOL(drm_dp_mst_topology_mgr_set_mst);
+static void
+drm_dp_mst_topology_mgr_invalidate_mstb(struct drm_dp_mst_branch *mstb)
+{
+ struct drm_dp_mst_port *port;
+
+ /* The link address will need to be re-sent on resume */
+ mstb->link_address_sent = false;
+
+ list_for_each_entry(port, &mstb->ports, next) {
+ /* The PBN for each port will also need to be re-probed */
+ port->available_pbn = 0;
+
+ if (port->mstb)
+ drm_dp_mst_topology_mgr_invalidate_mstb(port->mstb);
+ }
+}
+
/**
* drm_dp_mst_topology_mgr_suspend() - suspend the MST manager
* @mgr: manager to suspend
@@ -3197,20 +3284,36 @@ void drm_dp_mst_topology_mgr_suspend(struct
drm_dp_mst_topology_mgr *mgr)
flush_work(&mgr->up_req_work);
flush_work(&mgr->work);
flush_work(&mgr->delayed_destroy_work);
+
+ mutex_lock(&mgr->lock);
+ if (mgr->mst_state && mgr->mst_primary)
+ drm_dp_mst_topology_mgr_invalidate_mstb(mgr->mst_primary);
+ mutex_unlock(&mgr->lock);
}
EXPORT_SYMBOL(drm_dp_mst_topology_mgr_suspend);
/**
* drm_dp_mst_topology_mgr_resume() - resume the MST manager
* @mgr: manager to resume
+ * @sync: whether or not to perform topology reprobing synchronously
*
* This will fetch DPCD and see if the device is still there,
* if it is, it will rewrite the MSTM control bits, and return.
*
- * if the device fails this returns -1, and the driver should do
+ * If the device fails this returns -1, and the driver should do
* a full MST reprobe, in case we were undocked.
+ *
+ * During system resume (where it is assumed that the driver will be calling
+ * drm_atomic_helper_resume()) this function should be called beforehand with
+ * @sync set to true. In contexts like runtime resume where the driver is not
+ * expected to be calling drm_atomic_helper_resume(), this function should be
+ * called with @sync set to false in order to avoid deadlocking.
+ *
+ * Returns: -1 if the MST topology was removed while we were suspended, 0
+ * otherwise.
*/
-int drm_dp_mst_topology_mgr_resume(struct drm_dp_mst_topology_mgr *mgr)
+int drm_dp_mst_topology_mgr_resume(struct drm_dp_mst_topology_mgr *mgr,
+ bool sync)
{
int ret;
u8 guid[16];
@@ -3243,8 +3346,19 @@ int drm_dp_mst_topology_mgr_resume(struct
drm_dp_mst_topology_mgr *mgr)
}
drm_dp_check_mstb_guid(mgr->mst_primary, guid);
+ /*
+ * For the final step of resuming the topology, we need to bring the
+ * state of our in-memory topology back into sync with reality. So,
+ * restart the probing process as if we're probing a new hub
+ */
+ queue_work(system_long_wq, &mgr->work);
mutex_unlock(&mgr->lock);
+ if (sync) {
+ DRM_DEBUG_KMS("Waiting for link probe work to finish re-syncing
topology...\n");
+ flush_work(&mgr->work);
+ }
+
return 0;
out_fail:
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
b/drivers/gpu/drm/i915/display/intel_dp.c
index 5eeafa45831a..403b593a3eb4 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -7625,7 +7625,8 @@ void intel_dp_mst_resume(struct drm_i915_private
*dev_priv)
if (!intel_dp->can_mst)
continue;
- ret = drm_dp_mst_topology_mgr_resume(&intel_dp->mst_mgr);
+ ret = drm_dp_mst_topology_mgr_resume(&intel_dp->mst_mgr,
+ true);
if (ret) {
intel_dp->is_mst = false;
drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c
b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index a9d6aa110cfd..549486f1d937 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -1309,14 +1309,14 @@ nv50_mstm_fini(struct nv50_mstm *mstm)
}
static void
-nv50_mstm_init(struct nv50_mstm *mstm)
+nv50_mstm_init(struct nv50_mstm *mstm, bool runtime)
{
int ret;
if (!mstm || !mstm->mgr.mst_state)
return;
- ret = drm_dp_mst_topology_mgr_resume(&mstm->mgr);
+ ret = drm_dp_mst_topology_mgr_resume(&mstm->mgr, !runtime);
if (ret == -1) {
drm_dp_mst_topology_mgr_set_mst(&mstm->mgr, false);
drm_kms_helper_hotplug_event(mstm->mgr.dev);
@@ -2263,7 +2263,7 @@ nv50_display_init(struct drm_device *dev, bool resume,
bool runtime)
if (encoder->encoder_type != DRM_MODE_ENCODER_DPMST) {
struct nouveau_encoder *nv_encoder nouveau_encoder(encoder);
- nv50_mstm_init(nv_encoder->dp.mstm);
+ nv50_mstm_init(nv_encoder->dp.mstm, runtime);
}
}
diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
index fd142db42cb0..144027e27464 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -706,7 +706,8 @@ void drm_dp_mst_dump_topology(struct seq_file *m,
void drm_dp_mst_topology_mgr_suspend(struct drm_dp_mst_topology_mgr *mgr);
int __must_check
-drm_dp_mst_topology_mgr_resume(struct drm_dp_mst_topology_mgr *mgr);
+drm_dp_mst_topology_mgr_resume(struct drm_dp_mst_topology_mgr *mgr,
+ bool sync);
ssize_t drm_dp_mst_dpcd_read(struct drm_dp_aux *aux,
unsigned int offset, void *buffer, size_t size);
--
2.21.0
On Mon, Oct 21, 2019 at 10:36:00PM -0400, Lyude Paul wrote:> Currently, MST lacks locking in a lot of places that really should have > some sort of locking. Hotplugging and link address code paths are some > of the offenders here, as there is actually nothing preventing us from > running a link address probe while at the same time handling a > connection status update request - something that's likely always been > possible but never seen in the wild because hotplugging has been broken > for ages now (with the exception of amdgpu, for reasons I don't think > are worth digging into very far). > > Note: I'm going to start using the term "in-memory topology layout" here > to refer to drm_dp_mst_port->mstb and drm_dp_mst_branch->ports. > > Locking in these places is a little tougher then it looks though. > Generally we protect anything having to do with the in-memory topology > layout under &mgr->lock. But this becomes nearly impossible to do from > the context of link address probes due to the fact that &mgr->lock is > usually grabbed under random various modesetting locks, meaning that > there's no way we can just invert the &mgr->lock order and keep it > locked throughout the whole process of updating the topology. > > Luckily there are only two workers which can modify the in-memory > topology layout: drm_dp_mst_up_req_work() and > drm_dp_mst_link_probe_work(), meaning as long as we prevent these two > workers from traveling the topology layout in parallel with the intent > of updating it we don't need to worry about grabbing &mgr->lock in these > workers for reads. We only need to grab &mgr->lock in these workers for > writes, so that readers outside these two workers are still protected > from the topology layout changing beneath them. > > So, add the new &mgr->probe_lock and use it in both > drm_dp_mst_link_probe_work() and drm_dp_mst_up_req_work(). Additionally, > add some more detailed explanations for how this locking is intended to > work to drm_dp_mst_port->mstb and drm_dp_mst_branch->ports.This seems like a good solution to me, thanks for working this through! Any chance we could add a WARN_ON(!mutex_is_locked(&mgr->probe_lock)); somewhere centrally called by all paths modifying the in-memory topology layout? drm_dp_add_port perhaps? That way we have a fallback in case something else starts mucking with the topology. Other than that, Reviewed-by: Sean Paul <sean at poorly.run>> > Signed-off-by: Lyude Paul <lyude at redhat.com> > Cc: Juston Li <juston.li at intel.com> > Cc: Imre Deak <imre.deak at intel.com> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com> > Cc: Harry Wentland <hwentlan at amd.com> > Cc: Daniel Vetter <daniel.vetter at ffwll.ch> > Cc: Sean Paul <sean at poorly.run> > --- > drivers/gpu/drm/drm_dp_mst_topology.c | 28 ++++++++++++++--------- > include/drm/drm_dp_mst_helper.h | 32 +++++++++++++++++++++++---- > 2 files changed, 46 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c > index 08c316a727df..11d842f0bff5 100644 > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > @@ -2147,37 +2147,40 @@ static void drm_dp_check_and_send_link_address(struct drm_dp_mst_topology_mgr *m > struct drm_dp_mst_branch *mstb) > { > struct drm_dp_mst_port *port; > - struct drm_dp_mst_branch *mstb_child; > + > if (!mstb->link_address_sent) > drm_dp_send_link_address(mgr, mstb); > > list_for_each_entry(port, &mstb->ports, next) { > - if (port->input) > - continue; > + struct drm_dp_mst_branch *mstb_child = NULL; > > - if (!port->ddps) > + if (port->input || !port->ddps) > continue; > > if (!port->available_pbn) > drm_dp_send_enum_path_resources(mgr, mstb, port); > > - if (port->mstb) { > + if (port->mstb) > mstb_child = drm_dp_mst_topology_get_mstb_validated( > mgr, port->mstb); > - if (mstb_child) { > - drm_dp_check_and_send_link_address(mgr, mstb_child); > - drm_dp_mst_topology_put_mstb(mstb_child); > - } > + > + if (mstb_child) { > + drm_dp_check_and_send_link_address(mgr, mstb_child); > + drm_dp_mst_topology_put_mstb(mstb_child); > } > } > } > > static void drm_dp_mst_link_probe_work(struct work_struct *work) > { > - struct drm_dp_mst_topology_mgr *mgr = container_of(work, struct drm_dp_mst_topology_mgr, work); > + struct drm_dp_mst_topology_mgr *mgr > + container_of(work, struct drm_dp_mst_topology_mgr, work); > + struct drm_device *dev = mgr->dev; > struct drm_dp_mst_branch *mstb; > int ret; > > + mutex_lock(&mgr->probe_lock); > + > mutex_lock(&mgr->lock); > mstb = mgr->mst_primary; > if (mstb) { > @@ -2190,6 +2193,7 @@ static void drm_dp_mst_link_probe_work(struct work_struct *work) > drm_dp_check_and_send_link_address(mgr, mstb); > drm_dp_mst_topology_put_mstb(mstb); > } > + mutex_unlock(&mgr->probe_lock); > } > > static bool drm_dp_validate_guid(struct drm_dp_mst_topology_mgr *mgr, > @@ -3313,6 +3317,7 @@ static void drm_dp_mst_up_req_work(struct work_struct *work) > up_req_work); > struct drm_dp_pending_up_req *up_req; > > + mutex_lock(&mgr->probe_lock); > while (true) { > mutex_lock(&mgr->up_req_lock); > up_req = list_first_entry_or_null(&mgr->up_req_list, > @@ -3328,6 +3333,7 @@ static void drm_dp_mst_up_req_work(struct work_struct *work) > drm_dp_mst_process_up_req(mgr, up_req); > kfree(up_req); > } > + mutex_unlock(&mgr->probe_lock); > } > > static int drm_dp_mst_handle_up_req(struct drm_dp_mst_topology_mgr *mgr) > @@ -4349,6 +4355,7 @@ int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr, > mutex_init(&mgr->payload_lock); > mutex_init(&mgr->delayed_destroy_lock); > mutex_init(&mgr->up_req_lock); > + mutex_init(&mgr->probe_lock); > INIT_LIST_HEAD(&mgr->tx_msg_downq); > INIT_LIST_HEAD(&mgr->destroy_port_list); > INIT_LIST_HEAD(&mgr->destroy_branch_device_list); > @@ -4414,6 +4421,7 @@ void drm_dp_mst_topology_mgr_destroy(struct drm_dp_mst_topology_mgr *mgr) > mutex_destroy(&mgr->qlock); > mutex_destroy(&mgr->lock); > mutex_destroy(&mgr->up_req_lock); > + mutex_destroy(&mgr->probe_lock); > } > EXPORT_SYMBOL(drm_dp_mst_topology_mgr_destroy); > > diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h > index 7d80c38ee00e..bccb5514e0ef 100644 > --- a/include/drm/drm_dp_mst_helper.h > +++ b/include/drm/drm_dp_mst_helper.h > @@ -55,8 +55,6 @@ struct drm_dp_vcpi { > * @num_sdp_stream_sinks: Number of stream sinks > * @available_pbn: Available bandwidth for this port. > * @next: link to next port on this branch device > - * @mstb: branch device on this port, protected by > - * &drm_dp_mst_topology_mgr.lock > * @aux: i2c aux transport to talk to device connected to this port, protected > * by &drm_dp_mst_topology_mgr.lock > * @parent: branch device parent of this port > @@ -92,7 +90,17 @@ struct drm_dp_mst_port { > u8 num_sdp_stream_sinks; > uint16_t available_pbn; > struct list_head next; > - struct drm_dp_mst_branch *mstb; /* pointer to an mstb if this port has one */ > + /** > + * @mstb: the branch device connected to this port, if there is one. > + * This should be considered protected for reading by > + * &drm_dp_mst_topology_mgr.lock. There are two exceptions to this: > + * &drm_dp_mst_topology_mgr.up_req_work and > + * &drm_dp_mst_topology_mgr.work, which do not grab > + * &drm_dp_mst_topology_mgr.lock during reads but are the only > + * updaters of this list and are protected from writing concurrently > + * by &drm_dp_mst_topology_mgr.probe_lock. > + */ > + struct drm_dp_mst_branch *mstb; > struct drm_dp_aux aux; /* i2c bus for this port? */ > struct drm_dp_mst_branch *parent; > > @@ -118,7 +126,6 @@ struct drm_dp_mst_port { > * @lct: Link count total to talk to this branch device. > * @num_ports: number of ports on the branch. > * @msg_slots: one bit per transmitted msg slot. > - * @ports: linked list of ports on this branch. > * @port_parent: pointer to the port parent, NULL if toplevel. > * @mgr: topology manager for this branch device. > * @tx_slots: transmission slots for this device. > @@ -156,6 +163,16 @@ struct drm_dp_mst_branch { > int num_ports; > > int msg_slots; > + /** > + * @ports: the list of ports on this branch device. This should be > + * considered protected for reading by &drm_dp_mst_topology_mgr.lock. > + * There are two exceptions to this: > + * &drm_dp_mst_topology_mgr.up_req_work and > + * &drm_dp_mst_topology_mgr.work, which do not grab > + * &drm_dp_mst_topology_mgr.lock during reads but are the only > + * updaters of this list and are protected from updating the list > + * concurrently by @drm_dp_mst_topology_mgr.probe_lock > + */ > struct list_head ports; > > /* list of tx ops queue for this port */ > @@ -502,6 +519,13 @@ struct drm_dp_mst_topology_mgr { > */ > struct mutex lock; > > + /** > + * @probe_lock: Prevents @work and @up_req_work, the only writers of > + * &drm_dp_mst_port.mstb and &drm_dp_mst_branch.ports, from racing > + * while they update the topology. > + */ > + struct mutex probe_lock; > + > /** > * @mst_state: If this manager is enabled for an MST capable port. False > * if no MST sink/branch devices is connected. > -- > 2.21.0 >-- Sean Paul, Software Engineer, Google / Chromium OS
Sean Paul
2019-Oct-22 20:08 UTC
[Nouveau] [PATCH v5 06/14] drm/dp_mst: Protect drm_dp_mst_port members with locking
On Mon, Oct 21, 2019 at 10:36:01PM -0400, Lyude Paul wrote:> This is a complicated one. Essentially, there's currently a problem in the MST > core that hasn't really caused any issues that we're aware of (emphasis on "that > we're aware of"): locking. > > When we go through and probe the link addresses and path resources in a > topology, we hold no locks when updating ports with said information. The > members I'm referring to in particular are: > > - ldps > - ddps > - mcs > - pdt > - dpcd_rev > - num_sdp_streams > - num_sdp_stream_sinks > - available_pbn > - input > - connector > > Now that we're handling UP requests asynchronously and will be using some of > the struct members mentioned above in atomic modesetting in the future for > features such as PBN validation, this is going to become a lot more important. > As well, the next few commits that prepare us for and introduce suspend/resume > reprobing will also need clear locking in order to prevent from additional > racing hilarities that we never could have hit in the past. > > So, let's solve this issue by using &mgr->base.lock, the modesetting > lock which currently only protects &mgr->base.state. This works > perfectly because it allows us to avoid blocking connection_mutex > unnecessarily, and we can grab this in connector detection paths since > it's a ww mutex. We start by having drm_dp_mst_handle_up_req() hold this > when updating ports. For drm_dp_mst_handle_link_address_port() things > are a bit more complicated. As I've learned the hard way, we can grab > &mgr->lock.base for everything except for port->connector. See, our > normal driver probing paths end up generating this rather obvious > lockdep chain: > > &drm->mode_config.mutex > -> crtc_ww_class_mutex/crtc_ww_class_acquire > -> &connector->mutex > > However, sysfs grabs &drm->mode_config.mutex in order to protect itself > from connector state changing under it. Because this entails grabbing > kn->count, e.g. the lock that the kernel provides for protecting sysfs > contexts, we end up grabbing kn->count followed by > &drm->mode_config.mutex. This ends up creating an extremely rude chain: > > &kn->count > -> &drm->mode_config.mutex > -> crtc_ww_class_mutex/crtc_ww_class_acquire > -> &connector->mutex > > I mean, look at that thing! It's just evil!!! This gross thing ends up > making any calls to drm_connector_register()/drm_connector_unregister() > impossible when holding any kind of modesetting lock. This is annoying > because ideally, we always want to ensure that > drm_dp_mst_port->connector never changes when doing an atomic commit or > check that would affect the atomic topology state so that it can > reliably and easily be used from future DRM DP MST helpers to assist > with tasks such as scanning through the current VCPI allocations and > adding connectors which need to have their allocations updated in > response to a bandwidth change or the like. > > Being able to hold &mgr->base.lock throughout the entire link probe > process would have been _great_, since we could prevent userspace from > ever seeing any states in-between individual port changes and as a > result likely end up with a much faster probe and more consistent > results from said probes. But without some rework of how we handle > connector probing in sysfs it's not at all currently possible. In the > future, maybe we can try using the sysfs locks to protect updates to > connector probing state and fix this mess. > > So for now, to protect everything other than port->connector under > &mgr->base.lock and ensure that we still have the guarantee that atomic > check/commit contexts will never see port->connector change we use a > silly trick. See: port->connector only needs to change in order to > ensure that input ports (see the MST spec) never have a ghost connector > associated with them. But, there's nothing stopping us from simply > throwing the entire port out and creating a new one in order to maintain > that requirement while still keeping port->connector consistent across > the lifetime of the port in atomic check/commit contexts. For all > intended purposes this works fine, as we validate ports in any contexts > we care about before using them and as such will end up reporting the > connector as disconnected until it's port's destruction finalizes. So, > we just do that in cases where we detect port->input has transitioned > from true->false. We don't need to worry about the other direction, > since a port without a connector isn't visible to userspace and as such > doesn't need to be protected by &mgr->base.lock until we finish > registering a connector for it. > > For updating members of drm_dp_mst_port other than port->connector, we > simply grab &mgr->base.lock in drm_dp_mst_link_probe_work() for already > registered ports, update said members and drop the lock before > potentially registering a connector and probing the link address of it's > children. > > Finally, we modify drm_dp_mst_detect_port() to take a modesetting lock > acquisition context in order to acquire &mgr->base.lock under > &connection_mutex and convert all it's users over to using the > .detect_ctx probe hooks. > > With that, we finally have well defined locking. > > Changes since v4: > * Get rid of port->mutex, stop using connection_mutex and just use our own > modesetting lock - mgr->base.lock. Also, add a probe_lock that comes > before this patch. > * Just throw out ports that get changed from an output to an input, and > replace them with new ports. This lets us ensure that modesetting > contexts never see port->connector go from having a connector to being > NULL. > * Write an extremely detailed explanation of what problems this is > trying to fix, since there's a _lot_ of context here and I honestly > forgot some of it myself a couple times. > * Don't grab mgr->lock when reading port->mstb in > drm_dp_mst_handle_link_address_port(). It's not needed. > > Cc: Juston Li <juston.li at intel.com> > Cc: Imre Deak <imre.deak at intel.com> > Cc: Ville Syrj?l? <ville.syrjala at linux.intel.com> > Cc: Harry Wentland <hwentlan at amd.com> > Cc: Daniel Vetter <daniel.vetter at ffwll.ch> > Cc: Sean Paul <sean at poorly.run> > Signed-off-by: Lyude Paul <lyude at redhat.com>Overall makes sense to me. Thanks for the comprehensive commit message and comments, they definitely help :) Just one nit below, Reviewed-by: Sean Paul <sean at poorly.run>> --- > .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 28 +-- > drivers/gpu/drm/drm_dp_mst_topology.c | 230 ++++++++++++------ > drivers/gpu/drm/i915/display/intel_dp_mst.c | 28 ++- > drivers/gpu/drm/nouveau/dispnv50/disp.c | 32 +-- > drivers/gpu/drm/radeon/radeon_dp_mst.c | 24 +- > include/drm/drm_dp_mst_helper.h | 38 ++- > 6 files changed, 240 insertions(+), 140 deletions(-) >/snip> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c > index 11d842f0bff5..7bf4db91ff90 100644 > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c/snip> @@ -1912,35 +1984,40 @@ drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb, > { > struct drm_dp_mst_topology_mgr *mgr = mstb->mgr; > struct drm_dp_mst_port *port; > - bool created = false; > - int old_ddps = 0; > + int old_ddps = 0, ret; > + u8 new_pdt = DP_PEER_DEVICE_NONE; > + bool created = false, send_link_addr = false; > > port = drm_dp_get_port(mstb, port_msg->port_number); > if (!port) { > - port = kzalloc(sizeof(*port), GFP_KERNEL); > + port = drm_dp_mst_add_port(dev, mgr, mstb, > + port_msg->port_number); > if (!port) > return; > - kref_init(&port->topology_kref); > - kref_init(&port->malloc_kref); > - port->parent = mstb; > - port->port_num = port_msg->port_number; > - port->mgr = mgr; > - port->aux.name = "DPMST"; > - port->aux.dev = dev->dev; > - port->aux.is_remote = true; > - > - /* > - * Make sure the memory allocation for our parent branch stays > - * around until our own memory allocation is released > + created = true; > + } else if (port_msg->input_port && !port->input && port->connector) { > + /* Destroying the connector is impossible in this context, so > + * replace the port with a new one > */ > - drm_dp_mst_get_mstb_malloc(mstb); > + drm_dp_mst_topology_unlink_port(mgr, port); > + drm_dp_mst_topology_put_port(port); > > + port = drm_dp_mst_add_port(dev, mgr, mstb, > + port_msg->port_number); > + if (!port) > + return; > created = true; > } else { > + /* Locking is only needed when the port has a connector > + * exposed to userspace > + */ > + drm_modeset_lock(&mgr->base.lock, NULL);Random musing: It's kind of unfortunate that we don't have a void varient of drm_modeset_lock for when there's no acquire_ctx since we end up with a mix of drm_modeset_lock calls with and without return checking. /snip> @@ -3441,22 +3516,31 @@ EXPORT_SYMBOL(drm_dp_mst_hpd_irq); > /** > * drm_dp_mst_detect_port() - get connection status for an MST port > * @connector: DRM connector for this port > + * @ctx: The acquisition context to use for grabbing locks > * @mgr: manager for this port > - * @port: unverified pointer to a port > + * @port: pointer to a port > * > - * This returns the current connection state for a port. It validates the > - * port pointer still exists so the caller doesn't require a reference > + * This returns the current connection state for a port."On error, this returns -errno" /snip> -- > 2.21.0 >-- Sean Paul, Software Engineer, Google / Chromium OS
Sean Paul
2019-Oct-22 20:14 UTC
[Nouveau] [PATCH v5 07/14] drm/dp_mst: Don't forget to update port->input in drm_dp_mst_handle_conn_stat()
On Mon, Oct 21, 2019 at 10:36:02PM -0400, Lyude Paul wrote:> This probably hasn't caused any problems up until now since it's > probably nearly impossible to encounter this in the wild, however if we > were to receive a connection status notification from the MST hub after > resume while we're in the middle of reprobing the link addresses for a > topology then there's a much larger chance that a port could have > changed from being an output port to input port (or vice versa). If we > forget to update this bit of information, we'll potentially ignore a > valid PDT change on a downstream port because we think it's an input > port. > > So, make sure we read the input_port field in connection status > notifications in drm_dp_mst_handle_conn_stat() to prevent this from > happening once we've implemented suspend/resume reprobing. > > Cc: Juston Li <juston.li at intel.com> > Cc: Imre Deak <imre.deak at intel.com> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com> > Cc: Harry Wentland <hwentlan at amd.com> > Cc: Daniel Vetter <daniel.vetter at ffwll.ch> > Signed-off-by: Lyude Paul <lyude at redhat.com> > Reviewed-by: Sean Paul <sean at poorly.run> > --- > drivers/gpu/drm/drm_dp_mst_topology.c | 52 +++++++++++++++++++-------- > 1 file changed, 38 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c > index 7bf4db91ff90..c8e218b902ae 100644 > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > @@ -2079,18 +2079,40 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb, > { > struct drm_dp_mst_topology_mgr *mgr = mstb->mgr; > struct drm_dp_mst_port *port; > - int old_ddps; > - bool dowork = false; > + int old_ddps, ret; > + u8 new_pdt; > + bool dowork = false, create_connector = false; > > port = drm_dp_get_port(mstb, conn_stat->port_number); > if (!port) > return; > > - /* Locking is only needed if the port's exposed to userspace */ > - if (port->connector) > + if (port->connector) { > + if (!port->input && conn_stat->input_port) { > + /* > + * We can't remove a connector from an already exposed > + * port, so just throw the port out and make sure we > + * reprobe the link address of it's parent MSTB > + */ > + drm_dp_mst_topology_unlink_port(mgr, port); > + mstb->link_address_sent = false; > + dowork = true; > + goto out; > + } > + > + /* > + * Locking is only needed if the port's exposed to userspace > + */optional nit: this will still fit on one line> drm_modeset_lock(&mgr->base.lock, NULL); > + } else if (port->input && !conn_stat->input_port) { > + create_connector = true; > + /* Reprobe link address so we get num_sdp_streams */ > + mstb->link_address_sent = false; > + dowork = true; > + } > > old_ddps = port->ddps; > + port->input = conn_stat->input_port; > port->mcs = conn_stat->message_capability_status; > port->ldps = conn_stat->legacy_device_plug_status; > port->ddps = conn_stat->displayport_device_plug_status; > @@ -2103,21 +2125,23 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb, > } > } > > - if (!port->input) { > - int ret = drm_dp_port_set_pdt(port, > - conn_stat->peer_device_type); > - if (ret == 1) { > - dowork = true; > - } else if (ret < 0) { > - DRM_ERROR("Failed to change PDT for port %p: %d\n", > - port, ret); > - dowork = false; > - } > + new_pdt = port->input ? DP_PEER_DEVICE_NONE : conn_stat->peer_device_type; > + > + ret = drm_dp_port_set_pdt(port, new_pdt); > + if (ret == 1) { > + dowork = true; > + } else if (ret < 0) { > + DRM_ERROR("Failed to change PDT for port %p: %d\n", > + port, ret); > + dowork = false; > } > > if (port->connector) > drm_modeset_unlock(&mgr->base.lock); > + else if (create_connector) > + drm_dp_mst_port_add_connector(mstb, port); > > +out: > drm_dp_mst_topology_put_port(port); > if (dowork) > queue_work(system_long_wq, &mstb->mgr->work); > -- > 2.21.0 >-- Sean Paul, Software Engineer, Google / Chromium OS
Possibly Parallel Threads
- [PATCH v2 00/27] DP MST Refactors + debugging tools + suspend/resume reprobing
- [PATCH v2 16/27] drm/dp_mst: Refactor pdt setup/teardown, add more locking
- [PATCH v2 00/16] MST refcounting/atomic helpers cleanup
- [PATCH v2 20/27] drm/dp_mst: Protect drm_dp_mst_port members with connection_mutex
- [PATCH v3 00/16] MST refcounting/atomic helpers cleanup