From patchwork Fri Oct 26 20:35:47 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Lyude Paul X-Patchwork-Id: 10657913 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 8FBAC13BF for ; Fri, 26 Oct 2018 20:36:36 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 839BB2C84A for ; Fri, 26 Oct 2018 20:36:36 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 77D7F2C89B; Fri, 26 Oct 2018 20:36:36 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.2 required=2.0 tests=BAYES_00,MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED autolearn=unavailable version=3.3.1 Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id AC4402C84A for ; Fri, 26 Oct 2018 20:36:35 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 05DF36E574; Fri, 26 Oct 2018 20:36:04 +0000 (UTC) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by gabe.freedesktop.org (Postfix) with ESMTPS id 3242C6E555; Fri, 26 Oct 2018 20:35:57 +0000 (UTC) Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B9082C0528C4; Fri, 26 Oct 2018 20:35:56 +0000 (UTC) Received: from malachite.bss.redhat.com (dhcp-10-20-1-11.bss.redhat.com [10.20.1.11]) by smtp.corp.redhat.com (Postfix) with ESMTP id 090C41948E; Fri, 26 Oct 2018 20:35:55 +0000 (UTC) From: Lyude Paul To: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, nouveau@lists.freedesktop.org Subject: [PATCH v2 2/4] drm/dp_mst: Start tracking per-port VCPI allocations Date: Fri, 26 Oct 2018 16:35:47 -0400 Message-Id: <20181026203549.1796-3-lyude@redhat.com> In-Reply-To: <20181026203549.1796-1-lyude@redhat.com> References: <20181026203549.1796-1-lyude@redhat.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Fri, 26 Oct 2018 20:35:56 +0000 (UTC) X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Daniel Vetter MIME-Version: 1.0 Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Virus-Scanned: ClamAV using ClamSMTP 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 Cc: Daniel Vetter Reviewed-by: Daniel Vetter --- 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;