From: Lyude Paul <lyude@redhat.com> To: dri-devel@lists.freedesktop.org, nouveau@lists.freedesktop.org, intel-gfx@lists.freedesktop.org, amd-gfx@lists.freedesktop.org Cc: Daniel Vetter <daniel@ffwll.ch>, Dave Airlie <airlied@gmail.com>, Harry Wentland <harry.wentland@amd.com>, Jerry Zuo <Jerry.Zuo@amd.com>, Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, Maxime Ripard <maxime.ripard@bootlin.com>, Sean Paul <sean@poorly.run>, David Airlie <airlied@linux.ie>, linux-kernel@vger.kernel.org Subject: [WIP PATCH 05/15] drm/dp_mst: Fix payload deallocation on hotplugs using malloc refs Date: Thu, 13 Dec 2018 20:25:34 -0500 [thread overview] Message-ID: <20181214012604.13746-6-lyude@redhat.com> (raw) In-Reply-To: <20181214012604.13746-1-lyude@redhat.com> Up until now, freeing payloads on remote MST hubs that just had ports removed has almost never worked because we've been relying on port validation in order to stop us from accessing ports that have already been freed from memory, but ports which need their payloads released due to being removed will never be a valid part of the topology after they've been removed. Since we've introduced malloc refs, we can replace all of the validation logic in payload helpers which are used for deallocation with some well-placed malloc krefs. This ensures that regardless of whether or not the ports are still valid and in the topology, any port which has an allocated payload will remain allocated in memory until it's payloads have been removed - finally allowing us to actually release said payloads correctly. Signed-off-by: Lyude Paul <lyude@redhat.com> --- drivers/gpu/drm/drm_dp_mst_topology.c | 54 +++++++++++++++------------ 1 file changed, 30 insertions(+), 24 deletions(-) diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index ae9d019af9f2..93f08bfd2ab3 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -1989,10 +1989,6 @@ static int drm_dp_payload_send_msg(struct drm_dp_mst_topology_mgr *mgr, u8 sinks[DRM_DP_MAX_SDP_STREAMS]; int i; - port = drm_dp_mst_topology_get_port_validated(mgr, port); - if (!port) - return -EINVAL; - port_num = port->port_num; mstb = drm_dp_mst_topology_get_mstb_validated(mgr, port->parent); if (!mstb) { @@ -2000,10 +1996,8 @@ static int drm_dp_payload_send_msg(struct drm_dp_mst_topology_mgr *mgr, port->parent, &port_num); - if (!mstb) { - drm_dp_mst_topology_put_port(port); + if (!mstb) return -EINVAL; - } } txmsg = kzalloc(sizeof(*txmsg), GFP_KERNEL); @@ -2032,7 +2026,6 @@ static int drm_dp_payload_send_msg(struct drm_dp_mst_topology_mgr *mgr, kfree(txmsg); fail_put: drm_dp_mst_topology_put_mstb(mstb); - drm_dp_mst_topology_put_port(port); return ret; } @@ -2137,15 +2130,16 @@ static int drm_dp_destroy_payload_step2(struct drm_dp_mst_topology_mgr *mgr, */ int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr) { - int i, j; - int cur_slots = 1; struct drm_dp_payload req_payload; struct drm_dp_mst_port *port; + int i, j; + int cur_slots = 1; mutex_lock(&mgr->payload_lock); for (i = 0; i < mgr->max_payloads; i++) { struct drm_dp_vcpi *vcpi = mgr->proposed_vcpis[i]; struct drm_dp_payload *payload = &mgr->payloads[i]; + bool put_port = false; /* solve the current payloads - compare to the hw ones - update the hw view */ @@ -2153,12 +2147,20 @@ int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr) if (vcpi) { port = container_of(vcpi, struct drm_dp_mst_port, vcpi); - port = drm_dp_mst_topology_get_port_validated(mgr, - port); - if (!port) { - mutex_unlock(&mgr->payload_lock); - return -EINVAL; + + /* Validated ports don't matter if we're releasing + * VCPI + */ + if (vcpi->num_slots) { + port = drm_dp_mst_topology_get_port_validated( + mgr, port); + if (!port) { + mutex_unlock(&mgr->payload_lock); + return -EINVAL; + } + put_port = true; } + req_payload.num_slots = vcpi->num_slots; req_payload.vcpi = vcpi->vcpi; } else { @@ -2190,7 +2192,7 @@ int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr) } cur_slots += req_payload.num_slots; - if (port) + if (put_port) drm_dp_mst_topology_put_port(port); } @@ -3005,6 +3007,8 @@ bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, DRM_DEBUG_KMS("initing vcpi for pbn=%d slots=%d\n", pbn, port->vcpi.num_slots); + /* Keep port allocated until it's payload has been removed */ + drm_dp_mst_get_port_malloc(port); drm_dp_mst_topology_put_port(port); return true; out: @@ -3034,11 +3038,12 @@ EXPORT_SYMBOL(drm_dp_mst_get_vcpi_slots); */ void drm_dp_mst_reset_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port) { - port = drm_dp_mst_topology_get_port_validated(mgr, port); - if (!port) - return; + /* + * A port with VCPI will remain allocated until it's VCPI is + * released, no verified ref needed + */ + port->vcpi.num_slots = 0; - drm_dp_mst_topology_put_port(port); } EXPORT_SYMBOL(drm_dp_mst_reset_vcpi_slots); @@ -3050,16 +3055,17 @@ EXPORT_SYMBOL(drm_dp_mst_reset_vcpi_slots); void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port) { - port = drm_dp_mst_topology_get_port_validated(mgr, port); - if (!port) - return; + /* + * A port with VCPI will remain allocated until it's VCPI is + * released, no verified ref needed + */ drm_dp_mst_put_payload_id(mgr, port->vcpi.vcpi); port->vcpi.num_slots = 0; port->vcpi.pbn = 0; port->vcpi.aligned_pbn = 0; port->vcpi.vcpi = 0; - drm_dp_mst_topology_put_port(port); + drm_dp_mst_put_port_malloc(port); } EXPORT_SYMBOL(drm_dp_mst_deallocate_vcpi); -- 2.19.2
WARNING: multiple messages have this Message-ID (diff)
From: Lyude Paul <lyude-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> To: dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org, nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org, intel-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org Cc: Sean Paul <sean-p7yTbzM4H96eqtR555YLDQ@public.gmane.org>, Maxime Ripard <maxime.ripard-LDxbnhwyfcJBDgjK7y7TUQ@public.gmane.org>, Dave Airlie <airlied-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>, Maarten Lankhorst <maarten.lankhorst-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, David Airlie <airlied-cv59FeDIM0c@public.gmane.org>, Jerry Zuo <Jerry.Zuo-5C7GfCeVMHo@public.gmane.org>, Daniel Vetter <daniel-/w4YWyX8dFk@public.gmane.org>, Harry Wentland <harry.wentland-5C7GfCeVMHo@public.gmane.org> Subject: [WIP PATCH 05/15] drm/dp_mst: Fix payload deallocation on hotplugs using malloc refs Date: Thu, 13 Dec 2018 20:25:34 -0500 [thread overview] Message-ID: <20181214012604.13746-6-lyude@redhat.com> (raw) In-Reply-To: <20181214012604.13746-1-lyude-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> Up until now, freeing payloads on remote MST hubs that just had ports removed has almost never worked because we've been relying on port validation in order to stop us from accessing ports that have already been freed from memory, but ports which need their payloads released due to being removed will never be a valid part of the topology after they've been removed. Since we've introduced malloc refs, we can replace all of the validation logic in payload helpers which are used for deallocation with some well-placed malloc krefs. This ensures that regardless of whether or not the ports are still valid and in the topology, any port which has an allocated payload will remain allocated in memory until it's payloads have been removed - finally allowing us to actually release said payloads correctly. Signed-off-by: Lyude Paul <lyude@redhat.com> --- drivers/gpu/drm/drm_dp_mst_topology.c | 54 +++++++++++++++------------ 1 file changed, 30 insertions(+), 24 deletions(-) diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index ae9d019af9f2..93f08bfd2ab3 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -1989,10 +1989,6 @@ static int drm_dp_payload_send_msg(struct drm_dp_mst_topology_mgr *mgr, u8 sinks[DRM_DP_MAX_SDP_STREAMS]; int i; - port = drm_dp_mst_topology_get_port_validated(mgr, port); - if (!port) - return -EINVAL; - port_num = port->port_num; mstb = drm_dp_mst_topology_get_mstb_validated(mgr, port->parent); if (!mstb) { @@ -2000,10 +1996,8 @@ static int drm_dp_payload_send_msg(struct drm_dp_mst_topology_mgr *mgr, port->parent, &port_num); - if (!mstb) { - drm_dp_mst_topology_put_port(port); + if (!mstb) return -EINVAL; - } } txmsg = kzalloc(sizeof(*txmsg), GFP_KERNEL); @@ -2032,7 +2026,6 @@ static int drm_dp_payload_send_msg(struct drm_dp_mst_topology_mgr *mgr, kfree(txmsg); fail_put: drm_dp_mst_topology_put_mstb(mstb); - drm_dp_mst_topology_put_port(port); return ret; } @@ -2137,15 +2130,16 @@ static int drm_dp_destroy_payload_step2(struct drm_dp_mst_topology_mgr *mgr, */ int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr) { - int i, j; - int cur_slots = 1; struct drm_dp_payload req_payload; struct drm_dp_mst_port *port; + int i, j; + int cur_slots = 1; mutex_lock(&mgr->payload_lock); for (i = 0; i < mgr->max_payloads; i++) { struct drm_dp_vcpi *vcpi = mgr->proposed_vcpis[i]; struct drm_dp_payload *payload = &mgr->payloads[i]; + bool put_port = false; /* solve the current payloads - compare to the hw ones - update the hw view */ @@ -2153,12 +2147,20 @@ int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr) if (vcpi) { port = container_of(vcpi, struct drm_dp_mst_port, vcpi); - port = drm_dp_mst_topology_get_port_validated(mgr, - port); - if (!port) { - mutex_unlock(&mgr->payload_lock); - return -EINVAL; + + /* Validated ports don't matter if we're releasing + * VCPI + */ + if (vcpi->num_slots) { + port = drm_dp_mst_topology_get_port_validated( + mgr, port); + if (!port) { + mutex_unlock(&mgr->payload_lock); + return -EINVAL; + } + put_port = true; } + req_payload.num_slots = vcpi->num_slots; req_payload.vcpi = vcpi->vcpi; } else { @@ -2190,7 +2192,7 @@ int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr) } cur_slots += req_payload.num_slots; - if (port) + if (put_port) drm_dp_mst_topology_put_port(port); } @@ -3005,6 +3007,8 @@ bool drm_dp_mst_allocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, DRM_DEBUG_KMS("initing vcpi for pbn=%d slots=%d\n", pbn, port->vcpi.num_slots); + /* Keep port allocated until it's payload has been removed */ + drm_dp_mst_get_port_malloc(port); drm_dp_mst_topology_put_port(port); return true; out: @@ -3034,11 +3038,12 @@ EXPORT_SYMBOL(drm_dp_mst_get_vcpi_slots); */ void drm_dp_mst_reset_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port) { - port = drm_dp_mst_topology_get_port_validated(mgr, port); - if (!port) - return; + /* + * A port with VCPI will remain allocated until it's VCPI is + * released, no verified ref needed + */ + port->vcpi.num_slots = 0; - drm_dp_mst_topology_put_port(port); } EXPORT_SYMBOL(drm_dp_mst_reset_vcpi_slots); @@ -3050,16 +3055,17 @@ EXPORT_SYMBOL(drm_dp_mst_reset_vcpi_slots); void drm_dp_mst_deallocate_vcpi(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_port *port) { - port = drm_dp_mst_topology_get_port_validated(mgr, port); - if (!port) - return; + /* + * A port with VCPI will remain allocated until it's VCPI is + * released, no verified ref needed + */ drm_dp_mst_put_payload_id(mgr, port->vcpi.vcpi); port->vcpi.num_slots = 0; port->vcpi.pbn = 0; port->vcpi.aligned_pbn = 0; port->vcpi.vcpi = 0; - drm_dp_mst_topology_put_port(port); + drm_dp_mst_put_port_malloc(port); } EXPORT_SYMBOL(drm_dp_mst_deallocate_vcpi); -- 2.19.2 _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
next prev parent reply other threads:[~2018-12-14 1:26 UTC|newest] Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-12-14 1:25 [WIP PATCH 00/15] MST refcounting/atomic helpers cleanup Lyude Paul 2018-12-14 1:25 ` [WIP PATCH 01/15] drm/dp_mst: Remove bogus conditional in drm_dp_update_payload_part1() Lyude Paul 2018-12-14 8:42 ` Daniel Vetter 2018-12-17 20:09 ` Wentland, Harry 2018-12-17 20:09 ` Wentland, Harry 2018-12-14 1:25 ` [WIP PATCH 02/15] drm/dp_mst: Refactor drm_dp_update_payload_part1() Lyude Paul 2018-12-14 8:47 ` Daniel Vetter 2018-12-14 8:47 ` Daniel Vetter 2018-12-17 20:27 ` Wentland, Harry 2018-12-17 20:27 ` Wentland, Harry 2018-12-14 1:25 ` [WIP PATCH 03/15] drm/dp_mst: Introduce new refcounting scheme for mstbs and ports Lyude Paul 2018-12-14 1:25 ` Lyude Paul 2018-12-14 9:29 ` Daniel Vetter 2018-12-14 9:29 ` Daniel Vetter 2018-12-18 21:27 ` Lyude Paul 2018-12-18 21:27 ` Lyude Paul 2018-12-19 12:48 ` Daniel Vetter 2018-12-19 12:48 ` Daniel Vetter 2018-12-19 18:36 ` Lyude Paul 2018-12-14 1:25 ` [WIP PATCH 04/15] drm/dp_mst: Stop releasing VCPI when removing ports from topology Lyude Paul 2018-12-14 9:40 ` Daniel Vetter 2018-12-14 1:25 ` Lyude Paul [this message] 2018-12-14 1:25 ` [WIP PATCH 05/15] drm/dp_mst: Fix payload deallocation on hotplugs using malloc refs Lyude Paul 2018-12-14 9:38 ` Daniel Vetter 2018-12-14 9:38 ` Daniel Vetter 2018-12-18 22:02 ` Lyude Paul 2018-12-18 22:02 ` Lyude Paul 2018-12-14 1:25 ` [WIP PATCH 06/15] drm/i915: Keep malloc references to MST ports Lyude Paul 2018-12-14 1:25 ` Lyude Paul 2018-12-14 9:32 ` Daniel Vetter 2018-12-18 21:52 ` Lyude Paul 2018-12-18 21:52 ` Lyude Paul 2018-12-19 13:20 ` Daniel Vetter 2018-12-14 1:25 ` [WIP PATCH 07/15] drm/nouveau: Remove bogus cleanup in nv50_mstm_add_connector() Lyude Paul 2018-12-14 1:25 ` [WIP PATCH 08/15] drm/nouveau: Remove unnecessary VCPI checks in nv50_msto_cleanup() Lyude Paul 2018-12-14 1:25 ` [WIP PATCH 09/15] drm/nouveau: Fix potential use-after-frees for MSTCs Lyude Paul 2018-12-14 1:25 ` Lyude Paul 2018-12-14 1:25 ` [WIP PATCH 10/15] drm/nouveau: Stop unsetting mstc->port, use malloc refs Lyude Paul 2018-12-14 1:25 ` Lyude Paul 2018-12-14 1:25 ` [WIP PATCH 11/15] drm/nouveau: Grab payload lock in nv50_msto_payload() Lyude Paul 2018-12-14 1:25 ` Lyude Paul 2018-12-14 1:25 ` [WIP PATCH 12/15] drm/dp_mst: Add some atomic state iterator macros Lyude Paul 2018-12-14 1:25 ` Lyude Paul 2018-12-14 1:25 ` [WIP PATCH 13/15] drm/dp_mst: Start tracking per-port VCPI allocations Lyude Paul 2018-12-14 1:25 ` Lyude Paul 2018-12-14 16:37 ` Daniel Vetter 2018-12-14 16:37 ` Daniel Vetter 2018-12-14 1:25 ` [WIP PATCH 14/15] drm/dp_mst: Check payload count in drm_dp_mst_atomic_check() Lyude Paul 2018-12-14 1:25 ` [WIP PATCH 15/15] drm/nouveau: Use atomic VCPI helpers for MST Lyude Paul 2018-12-14 1:38 ` ✗ Fi.CI.CHECKPATCH: warning for MST refcounting/atomic helpers cleanup Patchwork 2018-12-14 1:42 ` ✗ Fi.CI.SPARSE: " Patchwork 2018-12-14 2:01 ` ✗ Fi.CI.BAT: failure " Patchwork
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20181214012604.13746-6-lyude@redhat.com \ --to=lyude@redhat.com \ --cc=Jerry.Zuo@amd.com \ --cc=airlied@gmail.com \ --cc=airlied@linux.ie \ --cc=amd-gfx@lists.freedesktop.org \ --cc=daniel@ffwll.ch \ --cc=dri-devel@lists.freedesktop.org \ --cc=harry.wentland@amd.com \ --cc=intel-gfx@lists.freedesktop.org \ --cc=linux-kernel@vger.kernel.org \ --cc=maarten.lankhorst@linux.intel.com \ --cc=maxime.ripard@bootlin.com \ --cc=nouveau@lists.freedesktop.org \ --cc=sean@poorly.run \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.