From patchwork Tue Oct 23 23:12:49 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Lyude Paul X-Patchwork-Id: 10653885 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 A104A14BB for ; Tue, 23 Oct 2018 23:13:45 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 8BE052862A for ; Tue, 23 Oct 2018 23:13:45 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 7F93C2893B; Tue, 23 Oct 2018 23:13:45 +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=ham 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 B601E2862A for ; Tue, 23 Oct 2018 23:13:44 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 987C06E108; Tue, 23 Oct 2018 23:13:10 +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 D8B046E0F5; Tue, 23 Oct 2018 23:13:08 +0000 (UTC) Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 7A4F05F74A; Tue, 23 Oct 2018 23:13:08 +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 D78D060C60; Tue, 23 Oct 2018 23:13:07 +0000 (UTC) From: Lyude Paul To: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, nouveau@lists.freedesktop.org Subject: [PATCH 4/6] drm/dp_mst: Start tracking per-port VCPI allocations Date: Tue, 23 Oct 2018 19:12:49 -0400 Message-Id: <20181023231251.16883-5-lyude@redhat.com> In-Reply-To: <20181023231251.16883-1-lyude@redhat.com> References: <20181023231251.16883-1-lyude@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Tue, 23 Oct 2018 23:13:08 +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 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 Cc: Daniel Vetter --- 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);