On Tue, Nov 06, 2018 at 08:21:14PM -0500, 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 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 v2:
> - Use kmemdup() for duplicating MST state - danvet
> - Move port validation out of duplicate state callback - danvet
> - Handle looping through MST topology states in
> drm_dp_mst_atomic_check() so the driver doesn't have to do it
> - Fix documentation in drm_dp_atomic_find_vcpi_slots()
> - Move the atomic check for each individual topology state into it's
> own function, reduces indenting
> - Don't consider "stale" MST ports when calculating the bandwidth
> requirements. This is needed because originally we relied on the
> state duplication functions to prune any stale ports from the new
> state, which would prevent us from incorrectly considering their
> bandwidth requirements alongside legitimate new payloads.
> - Add function references in drm_dp_atomic_release_vcpi_slots() - danvet
> - Annotate atomic VCPI and atomic check functions with __must_check
> - danvet
>
> 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 | 222 ++++++++++++++++++++++----
> drivers/gpu/drm/i915/intel_display.c | 4 +
> drivers/gpu/drm/i915/intel_dp_mst.c | 31 ++--
> include/drm/drm_dp_mst_helper.h | 23 ++-
> 4 files changed, 225 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 8c3cfac437f4..74823afb262e 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -2614,21 +2614,34 @@ 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
> *
> - * RETURNS:
> - * Total slots in the atomic state assigned for this port or error
> + * 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 &drm_encoder_helper_funcs.atomic_check() callback to change the
> + * current VCPI allocation for the new state. The allocations are not checked
> + * against the bandwidth restraints of @mgr until the driver calls
> + * drm_dp_mst_atomic_check().
> + *
> + * See also:
> + * drm_dp_atomic_release_vcpi_slots()
> + * drm_dp_mst_atomic_check()
> + *
> + * Returns:
> + * 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 +2650,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);
>
> - topology_state->avail_slots -= req_slots;
> - DRM_DEBUG_KMS("vcpi slots avail=%d", topology_state->avail_slots);
> + 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 +2692,50 @@ 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
> + * @port: The port to release the VCPI slots from
> *
> - * RETURNS:
> - * 0 if @slots were added back to &drm_dp_mst_topology_state->avail_slots or
> - * negative error code
> + * 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 &drm_connector_helper_funcs.atomic_check() callback 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()
> + * drm_dp_mst_atomic_check()
> + *
> + * Returns:
> + * 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!
> + */
This case here has me a bit worried. I thought with your changes we'll
always have the port pointer still around, to be able to clean things up
properly. If this is not the case then we'd potentially leak the vcpi
allocation for an unplugged port.
Otoh if the driver releases the allocation for a port twice it's probably
mixed up it's own book-keeping. We should WARN about that case.
This is kinda fallout from your change to not prune allocations on
duplicate, and I think will further clean up how this is handled.
So with the comment removed and replaced by a WARNING(1,"no vcpi
allocation found for port %p\n", port) or something like that this should
be solid.
> return 0;
> }
> EXPORT_SYMBOL(drm_dp_atomic_release_vcpi_slots);
> @@ -3112,15 +3164,34 @@ 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_vcpi_allocation *pos, *vcpi;
>
> - state = kmemdup(obj->state, sizeof(*state), GFP_KERNEL);
> + state = kmemdup(old_state, sizeof(*state), GFP_KERNEL);
> if (!state)
> return NULL;
>
> __drm_atomic_helper_private_obj_duplicate_state(obj, &state->base);
>
> + INIT_LIST_HEAD(&state->vcpis);
> +
> + list_for_each_entry(pos, &old_state->vcpis, next) {
> + vcpi = kmemdup(pos, sizeof(*vcpi), GFP_KERNEL);
> + if (!vcpi)
> + goto fail_alloc;
> +
> + list_add(&vcpi->next, &state->vcpis);
> + }
> +
> 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 +3199,99 @@ 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 = {
> +static inline int
> +drm_dp_mst_atomic_check_topology_state(struct drm_dp_mst_topology_mgr *mgr,
> + struct drm_dp_mst_topology_state *mst_state)
> +{
> + struct drm_dp_vcpi_allocation *vcpi;
> + struct drm_dp_mst_port *port;
> + int avail_slots = 63, ret;
> +
> + list_for_each_entry(vcpi, &mst_state->vcpis, next) {
> + /* Ports that no longer exist shouldn't be counted towards the
> + * bandwidth requirements for the state, so skip them
> + */
Scenario:
- 2 mst sinks on a single host side port.
- 1st connector is active, 2nd connector is off.
- 1st connector is hot-unplugged, but userspace hasn't realized this yet,
so not yet come around to do the disabling modeset.
- At the same time userspace decides to enable the 2nd output, which
forces a modeset and pulls the topology state in.
- This function here decides to only look at the vcpi allocation for the
2nd connector because the first one is gone already.
- Unfortunately lighting up both connectors at the same time isn't
possible, because too many vcpi. But since userspace didn't ask for a
modeset on the now unplugged first connector.
-> Modeset goes boom because we enable too many vcpi.
But with the change in how we release vcpi allocations there's no need to
ignore disconnected stuff here, and we should be able to check vcpi limits
by just checking all of them.
I didn't see anything else serious, so with the above two things resolved
this has my Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
One bikeshed below, because I need to prove I scrolled to the end :-)
> + port = drm_dp_get_validated_port_ref(mgr, vcpi->port);
> + if (!port) {
> + DRM_DEBUG_ATOMIC("[MST PORT:%p] is stale, skipping checks\n",
> + vcpi->port);
> + continue;
> + }
> +
> + DRM_DEBUG_ATOMIC("[MST PORT:%p] requires %d vcpi slots\n",
> + vcpi->port, vcpi->vcpi);
> +
> + avail_slots -= vcpi->vcpi;
> + if (avail_slots < 0) {
> + DRM_DEBUG_ATOMIC("[MST PORT:%p] not enough vcpi slots in mst_state %p (avail=%d)\n",
> + vcpi->port, mst_state,
> + avail_slots + vcpi->vcpi);
> + ret = -ENOSPC;
> + goto port_fail;
> + }
> +
> + drm_dp_put_port(port);
> + }
> + DRM_DEBUG_ATOMIC("[MST MGR:%p] mst_state %p vcpi avail=%d used=%d\n",
> + mgr, mst_state, avail_slots,
> + 63 - avail_slots);
> +
> + return 0;
> +port_fail:
> + drm_dp_put_port(port);
> + return ret;
> +}
> +
> +/**
> + * 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
> + * &drm_mode_config_funcs.atomic_check() callback.
> + *
> + * See also:
> + * drm_dp_atomic_find_vcpi_slots()
> + * drm_dp_atomic_release_vcpi_slots()
> + *
> + * Returns:
> + *
> + * 0 if the new state is valid, negative error code otherwise.
> + */
> +int drm_dp_mst_atomic_check(struct drm_atomic_state *state)
> +{
> + struct drm_dp_mst_topology_mgr *mgr;
> + struct drm_dp_mst_topology_state *mst_state;
> + int i, ret = 0;
> +
> + for_each_new_mst_mgr_in_state(state, mgr, mst_state, i) {
> + ret = drm_dp_mst_atomic_check_topology_state(mgr, mst_state);
> + if (ret)
> + break;
> + }
> +
> + return ret;
> +}
> +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 +3369,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 bbf8ca21a7a2..8749cf61a6ac 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12553,6 +12553,10 @@ static int intel_atomic_check(struct drm_device *dev,
> "[modeset]" : "[fastset]");
> }
>
> + ret = drm_dp_mst_atomic_check(state);
> + if (ret)
> + return ret;
Complete bikeshed, so feel free to ignore: I'd stuff this into
intel_modeset_checks(). Feel free to ignore.
Cheers, Daniel
> +
> 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..08b67948b8f9 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;
> };
>
> @@ -619,14 +625,17 @@ void drm_dp_mst_topology_mgr_suspend(struct drm_dp_mst_topology_mgr *mgr);
> int drm_dp_mst_topology_mgr_resume(struct drm_dp_mst_topology_mgr *mgr);
> struct drm_dp_mst_topology_state *drm_atomic_get_mst_topology_state(struct drm_atomic_state *state,
> struct drm_dp_mst_topology_mgr *mgr);
> -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);
> -int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,
> - struct drm_dp_mst_topology_mgr *mgr,
> - int slots);
> +int __must_check
> +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);
> +int __must_check
> +drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state *state,
> + struct drm_dp_mst_topology_mgr *mgr,
> + 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 __must_check drm_dp_mst_atomic_check(struct drm_atomic_state *state);
>
> extern const struct drm_private_state_funcs drm_dp_mst_topology_state_funcs;
>
> --
> 2.19.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
