There has been a TODO waiting for quite a long time in
drm_dp_mst_topology.c:
/* We cannot rely on port->vcpi.num_slots to update
* topology_state->avail_slots as the port may not exist if the parent
* branch device was unplugged. This should be fixed by tracking
* per-port slot allocation in drm_dp_mst_topology_state instead of
* depending on the caller to tell us how many slots to release.
*/
That's not the only reason we should fix this: forcing the driver to
track the VCPI allocations throughout a state's atomic check is
error prone, because it means that extra care has to be taken with the
order that drm_dp_atomic_find_vcpi_slots() and
drm_dp_atomic_release_vcpi_slots() are called in in order to ensure
idempotency. Currently the only driver actually using these helpers,
i915, doesn't even do this correctly: multiple ->best_encoder() checks
with i915's current implementation would not be idempotent and would
over-allocate VCPI slots, something I learned trying to implement
fallback retraining in MST.
So: simplify this whole mess, and teach drm_dp_atomic_find_vcpi_slots()
and drm_dp_atomic_release_vcpi_slots() to track the VCPI allocations for
each port. This allows us to ensure idempotency without having to rely
on the driver as much. Additionally: the driver doesn't need to do any
kind of VCPI slot tracking anymore if it doesn't need it for it's own
internal state.
Additionally; this adds a new drm_dp_mst_atomic_check() helper which
must be used by atomic drivers to perform validity checks for the new
VCPI allocations incurred by a state.
Also: update the documentation and make it more obvious that these
/must/ be called by /all/ atomic drivers supporting MST.
Changes since v1:
- Don't use the now-removed ->atomic_check() for private objects hook,
just give drivers a function to call themselves
Signed-off-by: Lyude Paul <lyude@xxxxxxxxxx>
Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
---
drivers/gpu/drm/drm_dp_mst_topology.c | 190 +++++++++++++++++++++-----
drivers/gpu/drm/i915/intel_display.c | 8 ++
drivers/gpu/drm/i915/intel_dp_mst.c | 31 +++--
include/drm/drm_dp_mst_helper.h | 11 +-
4 files changed, 192 insertions(+), 48 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 8c3cfac437f4..dcfab7536914 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -2614,21 +2614,33 @@ static int drm_dp_init_vcpi(struct drm_dp_mst_topology_mgr *mgr,
}
/**
- * drm_dp_atomic_find_vcpi_slots() - Find and add vcpi slots to the state
+ * drm_dp_atomic_find_vcpi_slots() - Find and add VCPI slots to the state
* @state: global atomic state
* @mgr: MST topology manager for the port
* @port: port to find vcpi slots for
* @pbn: bandwidth required for the mode in PBN
*
+ * Allocates VCPI slots to @port, replacing any previous VCPI allocations it
+ * may have had. Any atomic drivers which support MST must call this function
+ * in their atomic_check() handlers to change the current VCPI allocation for
+ * the new state. After the ->atomic_check() hooks of the driver and all other
+ * mode objects in the state have been called, DRM will check the final VCPI
+ * allocations to ensure that they will fit into the available bandwidth on
+ * the topology.
+ *
+ * See also: drm_dp_atomic_release_vcpi_slots()
+ *
* RETURNS:
- * Total slots in the atomic state assigned for this port or error
+ * Total slots in the atomic state assigned for this port, or a negative error
+ * code if the port no longer exists
*/
int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state,
struct drm_dp_mst_topology_mgr *mgr,
struct drm_dp_mst_port *port, int pbn)
{
struct drm_dp_mst_topology_state *topology_state;
- int req_slots;
+ struct drm_dp_vcpi_allocation *pos, *vcpi = NULL;
+ int prev_slots, req_slots, ret;
topology_state = drm_atomic_get_mst_topology_state(state, mgr);
if (IS_ERR(topology_state))
@@ -2637,20 +2649,41 @@ int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state,
port = drm_dp_get_validated_port_ref(mgr, port);
if (port == NULL)
return -EINVAL;
- req_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
- DRM_DEBUG_KMS("vcpi slots req=%d, avail=%d\n",
- req_slots, topology_state->avail_slots);
- if (req_slots > topology_state->avail_slots) {
- drm_dp_put_port(port);
- return -ENOSPC;
+ /* Find the current allocation for this port, if any */
+ list_for_each_entry(pos, &topology_state->vcpis, next) {
+ if (pos->port == port) {
+ vcpi = pos;
+ prev_slots = vcpi->vcpi;
+ break;
+ }
}
+ if (!vcpi)
+ prev_slots = 0;
+
+ req_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
+
+ DRM_DEBUG_KMS("[CONNECTOR:%d:%s] [MST PORT:%p] vcpi %d -> %d\n",
+ port->connector->base.id, port->connector->name, port,
+ prev_slots, req_slots);
+
+ /* Add the new allocation to the state */
+ if (!vcpi) {
+ vcpi = kzalloc(sizeof(*vcpi), GFP_KERNEL);
+ if (!vcpi) {
+ ret = -ENOMEM;
+ goto out;
+ }
- topology_state->avail_slots -= req_slots;
- DRM_DEBUG_KMS("vcpi slots avail=%d", topology_state->avail_slots);
+ vcpi->port = port;
+ list_add(&vcpi->next, &topology_state->vcpis);
+ }
+ vcpi->vcpi = req_slots;
+ ret = req_slots;
+out:
drm_dp_put_port(port);
- return req_slots;
+ return ret;
}
EXPORT_SYMBOL(drm_dp_atomic_find_vcpi_slots);
@@ -2658,32 +2691,46 @@ EXPORT_SYMBOL(drm_dp_atomic_find_vcpi_slots);
* drm_dp_atomic_release_vcpi_slots() - Release allocated vcpi slots
* @state: global atomic state
* @mgr: MST topology manager for the port
- * @slots: number of vcpi slots to release
+ *
+ * Releases any VCPI slots that have been allocated to a port in the atomic
+ * state. Any atomic drivers which support MST must call this function in
+ * their connector's atomic_check() handler when the connector will no longer
+ * have VCPI allocated (e.g. because it's CRTC was removed).
+ *
+ * It is OK to call this even if @port has been removed from the system, in
+ * which case it will just amount to a no-op.
+ *
+ * See also: drm_dp_atomic_find_vcpi_slots()
*
* RETURNS:
- * 0 if @slots were added back to &drm_dp_mst_topology_state->avail_slots or
- * negative error code
+ * 0 if all slots for this port were added back to
+ * &drm_dp_mst_topology_state->avail_slots or negative error code
*/
int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,
struct drm_dp_mst_topology_mgr *mgr,
- int slots)
+ struct drm_dp_mst_port *port)
{
struct drm_dp_mst_topology_state *topology_state;
+ struct drm_dp_vcpi_allocation *tmp, *pos;
topology_state = drm_atomic_get_mst_topology_state(state, mgr);
if (IS_ERR(topology_state))
return PTR_ERR(topology_state);
- /* We cannot rely on port->vcpi.num_slots to update
- * topology_state->avail_slots as the port may not exist if the parent
- * branch device was unplugged. This should be fixed by tracking
- * per-port slot allocation in drm_dp_mst_topology_state instead of
- * depending on the caller to tell us how many slots to release.
- */
- topology_state->avail_slots += slots;
- DRM_DEBUG_KMS("vcpi slots released=%d, avail=%d\n",
- slots, topology_state->avail_slots);
+ list_for_each_entry_safe(pos, tmp, &topology_state->vcpis, next) {
+ if (pos->port == port) {
+ list_del(&pos->next);
+ DRM_DEBUG_KMS("[MST PORT:%p] vcpi %d -> 0\n",
+ port, pos->vcpi);
+ kfree(pos);
+ return 0;
+ }
+ }
+
+ /* If no allocation was found, all that means is that the port was
+ * destroyed since the last atomic commit. That's OK!
+ */
return 0;
}
EXPORT_SYMBOL(drm_dp_atomic_release_vcpi_slots);
@@ -3112,15 +3159,50 @@ static void drm_dp_destroy_connector_work(struct work_struct *work)
static struct drm_private_state *
drm_dp_mst_duplicate_state(struct drm_private_obj *obj)
{
- struct drm_dp_mst_topology_state *state;
+ struct drm_dp_mst_topology_state *state, *old_state =
+ to_dp_mst_topology_state(obj->state);
+ struct drm_dp_mst_topology_mgr *mgr = old_state->mgr;
+ struct drm_dp_mst_port *port;
+ struct drm_dp_vcpi_allocation *pos, *vcpi;
- state = kmemdup(obj->state, sizeof(*state), GFP_KERNEL);
+ state = kzalloc(sizeof(*state), GFP_KERNEL);
if (!state)
return NULL;
__drm_atomic_helper_private_obj_duplicate_state(obj, &state->base);
+ state->mgr = mgr;
+ INIT_LIST_HEAD(&state->vcpis);
+
+ /* Copy over the VCPI allocations for ports that still exist */
+ list_for_each_entry(pos, &old_state->vcpis, next) {
+ port = drm_dp_get_validated_port_ref(mgr, pos->port);
+ if (!port) {
+ DRM_DEBUG_ATOMIC("[MST PORT:%p] is gone, vcpi %d -> 0\n",
+ pos->port, pos->vcpi);
+ continue;
+ }
+
+ vcpi = kzalloc(sizeof(*vcpi), GFP_KERNEL);
+ if (!vcpi) {
+ drm_dp_put_port(port);
+ goto fail_alloc;
+ }
+
+ vcpi->port = port;
+ vcpi->vcpi = pos->vcpi;
+ list_add(&vcpi->next, &state->vcpis);
+ drm_dp_put_port(port);
+ }
+
return &state->base;
+
+fail_alloc:
+ list_for_each_entry_safe(pos, vcpi, &state->vcpis, next)
+ kfree(pos);
+ kfree(state);
+
+ return NULL;
}
static void drm_dp_mst_destroy_state(struct drm_private_obj *obj,
@@ -3128,14 +3210,60 @@ static void drm_dp_mst_destroy_state(struct drm_private_obj *obj,
{
struct drm_dp_mst_topology_state *mst_state =
to_dp_mst_topology_state(state);
+ struct drm_dp_vcpi_allocation *pos, *tmp;
+
+ list_for_each_entry_safe(pos, tmp, &mst_state->vcpis, next)
+ kfree(pos);
kfree(mst_state);
}
-static const struct drm_private_state_funcs mst_state_funcs = {
+/**
+ * drm_dp_mst_atomic_check - Check that the new state of an MST topology in an
+ * atomic update is valid
+ * @state: Pointer to the new &struct drm_dp_mst_topology_state
+ *
+ * Checks the given topology state for an atomic update to ensure that it's
+ * valid. This includes checking whether there's enough bandwidth to support
+ * the new VCPI allocations in the atomic update.
+ *
+ * Any atomic drivers supporting DP MST must make sure to call this after
+ * checking the rest of their state in their ->atomic_check() callback.
+ *
+ * Returns:
+ *
+ * 0 if the new state is valid, negative error code otherwise.
+ */
+int drm_dp_mst_atomic_check(struct drm_dp_mst_topology_state *state)
+{
+ struct drm_dp_mst_topology_mgr *mgr = state->mgr;
+ struct drm_dp_vcpi_allocation *pos;
+ int avail_slots = 63;
+
+ list_for_each_entry(pos, &state->vcpis, next) {
+ DRM_DEBUG_ATOMIC("[MST PORT:%p] requires %d vcpi slots\n",
+ pos->port, pos->vcpi);
+
+ avail_slots -= pos->vcpi;
+ if (avail_slots < 0) {
+ DRM_DEBUG_ATOMIC("[MST PORT:%p] not enough vcpi slots in state %p (avail=%d)\n",
+ pos->port, state,
+ avail_slots + pos->vcpi);
+ return -ENOSPC;
+ }
+ }
+ DRM_DEBUG_ATOMIC("[MST MGR:%p] state %p vcpi avail=%d used=%d\n",
+ mgr, state, avail_slots, 63 - avail_slots);
+
+ return 0;
+}
+EXPORT_SYMBOL(drm_dp_mst_atomic_check);
+
+const struct drm_private_state_funcs drm_dp_mst_topology_state_funcs = {
.atomic_duplicate_state = drm_dp_mst_duplicate_state,
.atomic_destroy_state = drm_dp_mst_destroy_state,
};
+EXPORT_SYMBOL(drm_dp_mst_topology_state_funcs);
/**
* drm_atomic_get_mst_topology_state: get MST topology state
@@ -3213,13 +3341,11 @@ int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,
return -ENOMEM;
mst_state->mgr = mgr;
-
- /* max. time slots - one slot for MTP header */
- mst_state->avail_slots = 63;
+ INIT_LIST_HEAD(&mst_state->vcpis);
drm_atomic_private_obj_init(&mgr->base,
&mst_state->base,
- &mst_state_funcs);
+ &drm_dp_mst_topology_state_funcs);
return 0;
}
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index fe045abb6472..f66af1465686 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12484,6 +12484,8 @@ static int intel_atomic_check(struct drm_device *dev,
struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
struct drm_crtc *crtc;
struct drm_crtc_state *old_crtc_state, *crtc_state;
+ struct drm_dp_mst_topology_mgr *mgr;
+ struct drm_dp_mst_topology_state *mst_state;
int ret, i;
bool any_ms = false;
@@ -12534,6 +12536,12 @@ static int intel_atomic_check(struct drm_device *dev,
"[modeset]" : "[fastset]");
}
+ for_each_new_mst_mgr_in_state(state, mgr, mst_state, i) {
+ ret = drm_dp_mst_atomic_check(mst_state);
+ if (ret)
+ return ret;
+ }
+
if (any_ms) {
ret = intel_modeset_checks(state);
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 8b71d64ebd9d..aaf904738b78 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -114,28 +114,31 @@ static int intel_dp_mst_atomic_check(struct drm_connector *connector,
struct drm_connector_state *old_conn_state;
struct drm_crtc *old_crtc;
struct drm_crtc_state *crtc_state;
- int slots, ret = 0;
+ struct intel_connector *intel_connector =
+ to_intel_connector(connector);
+ struct drm_dp_mst_topology_mgr *mgr =
+ &intel_connector->mst_port->mst_mgr;
+ struct drm_dp_mst_port *port = intel_connector->port;
+ int ret = 0;
old_conn_state = drm_atomic_get_old_connector_state(state, connector);
old_crtc = old_conn_state->crtc;
if (!old_crtc)
return ret;
- crtc_state = drm_atomic_get_new_crtc_state(state, old_crtc);
- slots = to_intel_crtc_state(crtc_state)->dp_m_n.tu;
- if (drm_atomic_crtc_needs_modeset(crtc_state) && slots > 0) {
- struct drm_dp_mst_topology_mgr *mgr;
- struct drm_encoder *old_encoder;
+ /* Free VCPI, since compute_config() won't be run */
+ if (!new_conn_state->crtc) {
+ crtc_state = drm_atomic_get_new_crtc_state(state, old_crtc);
- old_encoder = old_conn_state->best_encoder;
- mgr = &enc_to_mst(old_encoder)->primary->dp.mst_mgr;
-
- ret = drm_dp_atomic_release_vcpi_slots(state, mgr, slots);
- if (ret)
- DRM_DEBUG_KMS("failed releasing %d vcpi slots:%d\n", slots, ret);
- else
- to_intel_crtc_state(crtc_state)->dp_m_n.tu = 0;
+ ret = drm_dp_atomic_release_vcpi_slots(state, mgr, port);
+ if (ret) {
+ DRM_DEBUG_KMS("failed releasing vcpi slots: %d\n",
+ ret);
+ return ret;
+ }
+ to_intel_crtc_state(crtc_state)->dp_m_n.tu = 0;
}
+
return ret;
}
diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
index 3faceb66f5cb..0aa7d3658013 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -406,9 +406,15 @@ struct drm_dp_payload {
#define to_dp_mst_topology_state(x) container_of(x, struct drm_dp_mst_topology_state, base)
+struct drm_dp_vcpi_allocation {
+ struct drm_dp_mst_port *port;
+ int vcpi;
+ struct list_head next;
+};
+
struct drm_dp_mst_topology_state {
struct drm_private_state base;
- int avail_slots;
+ struct list_head vcpis;
struct drm_dp_mst_topology_mgr *mgr;
};
@@ -624,9 +630,10 @@ int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state,
struct drm_dp_mst_port *port, int pbn);
int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,
struct drm_dp_mst_topology_mgr *mgr,
- int slots);
+ struct drm_dp_mst_port *port);
int drm_dp_send_power_updown_phy(struct drm_dp_mst_topology_mgr *mgr,
struct drm_dp_mst_port *port, bool power_up);
+int drm_dp_mst_atomic_check(struct drm_dp_mst_topology_state *state);
extern const struct drm_private_state_funcs drm_dp_mst_topology_state_funcs;
--
2.17.2
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
