On Tue, Oct 23, 2018 at 07:12:49PM -0400, Lyude Paul wrote:
> 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 moves the code for checking whether or not the
> VCPI allocations in the atomic state are valid into the new
> ->atomic_check() hook for private objects, so we can ensure that we only
> check the final VCPI allocation that would be incurred by the new state
> and not the mid-check VCPI allocations. This prevents us from failing
> checks which end up allocating VCPI slots to ports before freeing any of
> the VCPI slots allocated by ports which are about to be disabled.
>
> Also: update the documentation and make it more obvious that these
> /must/ be called by /all/ atomic drivers supporting MST.
>
> Signed-off-by: Lyude Paul <ly...@redhat.com>
> Cc: Daniel Vetter <daniel.vet...@ffwll.ch>
lgtm, but I'll wait with detailed review until we have the "where should
we atomic_check this" question sorted out.
-Daniel
> ---
> drivers/gpu/drm/drm_dp_mst_topology.c | 172 +++++++++++++++++++++-----
> drivers/gpu/drm/i915/intel_dp_mst.c | 31 ++---
> include/drm/drm_dp_mst_helper.h | 10 +-
> 3 files changed, 167 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 8c3cfac437f4..adb4298570cc 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;
>
> - topology_state->avail_slots -= req_slots;
> - DRM_DEBUG_KMS("vcpi slots avail=%d", topology_state->avail_slots);
> + 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;
> + }
> +
> + 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,13 +3210,45 @@ 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 int drm_dp_mst_atomic_check(struct drm_private_obj *obj,
> + struct drm_private_state *state)
> +{
> + struct drm_dp_mst_topology_state *mst_state =
> + to_dp_mst_topology_state(state);
> + struct drm_dp_mst_topology_mgr *mgr = mst_state->mgr;
> + struct drm_dp_vcpi_allocation *pos;
> + int avail_slots = 63;
> +
> + list_for_each_entry(pos, &mst_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;
> +}
> +
> static const struct drm_private_state_funcs mst_state_funcs = {
> .atomic_duplicate_state = drm_dp_mst_duplicate_state,
> .atomic_destroy_state = drm_dp_mst_destroy_state,
> + .atomic_check = drm_dp_mst_atomic_check,
> };
>
> /**
> @@ -3213,9 +3327,7 @@ 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,
> 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 59f005b419cf..4c0805e56335 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,7 +630,7 @@ 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);
>
> --
> 2.17.2
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
