* [PATCH v2 0/2] Fix observed mst problems with StarTech hub @ 2021-06-16 3:54 Wayne Lin 2021-06-16 3:55 ` [PATCH v2 1/2] drm/dp_mst: Do not set proposed vcpi directly Wayne Lin 2021-06-16 3:55 ` [PATCH v2 2/2] drm/dp_mst: Avoid to mess up payload table by ports in stale topology Wayne Lin 0 siblings, 2 replies; 6+ messages in thread From: Wayne Lin @ 2021-06-16 3:54 UTC (permalink / raw) To: dri-devel; +Cc: jerry.zuo, aurabindo.pillai, Wayne Lin, Nicholas.Kazlauskas Use Startech 1to3 DP hub to do some mst hotplug tests and find some light up issues. 1. ACT polling timeout: Which is due to we didn't update DPCD payload table but still try to send ACT and polling for "ACT Handled" bit 2. Not all monitors light up: Due to we wrongly set unavailable VCP ID for new streams Changes since v1: * Add appropriate tags: Fixes & Cc * Change debug macro to use drm_dbg_kms() instead Wayne Lin (2): drm/dp_mst: Do not set proposed vcpi directly drm/dp_mst: Avoid to mess up payload table by ports in stale topology drivers/gpu/drm/drm_dp_mst_topology.c | 65 ++++++++++++++++----------- 1 file changed, 39 insertions(+), 26 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 1/2] drm/dp_mst: Do not set proposed vcpi directly 2021-06-16 3:54 [PATCH v2 0/2] Fix observed mst problems with StarTech hub Wayne Lin @ 2021-06-16 3:55 ` Wayne Lin 2021-06-16 19:53 ` Harry Wentland 2021-06-16 3:55 ` [PATCH v2 2/2] drm/dp_mst: Avoid to mess up payload table by ports in stale topology Wayne Lin 1 sibling, 1 reply; 6+ messages in thread From: Wayne Lin @ 2021-06-16 3:55 UTC (permalink / raw) To: dri-devel Cc: jerry.zuo, aurabindo.pillai, stable, Thomas Zimmermann, Wayne Lin, Nicholas.Kazlauskas [Why] When we receive CSN message to notify one port is disconnected, we will implicitly set its corresponding num_slots to 0. Later on, we will eventually call drm_dp_update_payload_part1() to arrange down streams. In drm_dp_update_payload_part1(), we iterate over all proposed_vcpis[] to do the update. Not specific to a target sink only. For example, if we light up 2 monitors, Monitor_A and Monitor_B, and then we unplug Monitor_B. Later on, when we call drm_dp_update_payload_part1() to try to update payload for Monitor_A, we'll also implicitly clean payload for Monitor_B at the same time. And finally, when we try to call drm_dp_update_payload_part1() to clean payload for Monitor_B, we will do nothing at this time since payload for Monitor_B has been cleaned up previously. For StarTech 1to3 DP hub, it seems like if we didn't update DPCD payload ID table then polling for "ACT Handled"(BIT_1 of DPCD 002C0h) will fail and this polling will last for 3 seconds. Therefore, guess the best way is we don't set the proposed_vcpi[] diretly. Let user of these herlper functions to set the proposed_vcpi directly. [How] 1. Revert commit 7617e9621bf2 ("drm/dp_mst: clear time slots for ports invalid") 2. Tackle the issue in previous commit by skipping those trasient proposed VCPIs. These stale VCPIs shoulde be explicitly cleared by user later on. Changes since v1: * Change debug macro to use drm_dbg_kms() instead * Amend the commit message to add Fixed & Cc tags Signed-off-by: Wayne Lin <Wayne.Lin@amd.com> Fixes: 7617e9621bf2 ("drm/dp_mst: clear time slots for ports invalid") Cc: Lyude Paul <lyude@redhat.com> Cc: Wayne Lin <Wayne.Lin@amd.com> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Maxime Ripard <mripard@kernel.org> Cc: Thomas Zimmermann <tzimmermann@suse.de> Cc: dri-devel@lists.freedesktop.org Cc: <stable@vger.kernel.org> # v5.5+ --- drivers/gpu/drm/drm_dp_mst_topology.c | 36 ++++++++------------------- 1 file changed, 10 insertions(+), 26 deletions(-) diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 32b7f8983b94..b41b837db66d 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -2501,7 +2501,7 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb, { struct drm_dp_mst_topology_mgr *mgr = mstb->mgr; struct drm_dp_mst_port *port; - int old_ddps, old_input, ret, i; + int old_ddps, ret; u8 new_pdt; bool new_mcs; bool dowork = false, create_connector = false; @@ -2533,7 +2533,6 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb, } old_ddps = port->ddps; - old_input = port->input; port->input = conn_stat->input_port; port->ldps = conn_stat->legacy_device_plug_status; port->ddps = conn_stat->displayport_device_plug_status; @@ -2555,28 +2554,6 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb, dowork = false; } - if (!old_input && old_ddps != port->ddps && !port->ddps) { - for (i = 0; i < mgr->max_payloads; i++) { - struct drm_dp_vcpi *vcpi = mgr->proposed_vcpis[i]; - struct drm_dp_mst_port *port_validated; - - if (!vcpi) - continue; - - port_validated = - container_of(vcpi, struct drm_dp_mst_port, vcpi); - port_validated = - drm_dp_mst_topology_get_port_validated(mgr, port_validated); - if (!port_validated) { - mutex_lock(&mgr->payload_lock); - vcpi->num_slots = 0; - mutex_unlock(&mgr->payload_lock); - } else { - drm_dp_mst_topology_put_port(port_validated); - } - } - } - if (port->connector) drm_modeset_unlock(&mgr->base.lock); else if (create_connector) @@ -3410,8 +3387,15 @@ int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr) port = drm_dp_mst_topology_get_port_validated( mgr, port); if (!port) { - mutex_unlock(&mgr->payload_lock); - return -EINVAL; + if (vcpi->num_slots == payload->num_slots) { + cur_slots += vcpi->num_slots; + payload->start_slot = req_payload.start_slot; + continue; + } else { + drm_dbg_kms("Fail:set payload to invalid sink"); + mutex_unlock(&mgr->payload_lock); + return -EINVAL; + } } put_port = true; } -- 2.17.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] drm/dp_mst: Do not set proposed vcpi directly 2021-06-16 3:55 ` [PATCH v2 1/2] drm/dp_mst: Do not set proposed vcpi directly Wayne Lin @ 2021-06-16 19:53 ` Harry Wentland 2021-06-17 8:47 ` Lin, Wayne 0 siblings, 1 reply; 6+ messages in thread From: Harry Wentland @ 2021-06-16 19:53 UTC (permalink / raw) To: Wayne Lin, dri-devel Cc: jerry.zuo, aurabindo.pillai, stable, Thomas Zimmermann, Nicholas.Kazlauskas On 2021-06-15 11:55 p.m., Wayne Lin wrote: > [Why] > When we receive CSN message to notify one port is disconnected, we will > implicitly set its corresponding num_slots to 0. Later on, we will > eventually call drm_dp_update_payload_part1() to arrange down streams. > > In drm_dp_update_payload_part1(), we iterate over all proposed_vcpis[] > to do the update. Not specific to a target sink only. For example, if we > light up 2 monitors, Monitor_A and Monitor_B, and then we unplug > Monitor_B. Later on, when we call drm_dp_update_payload_part1() to try > to update payload for Monitor_A, we'll also implicitly clean payload for > Monitor_B at the same time. And finally, when we try to call > drm_dp_update_payload_part1() to clean payload for Monitor_B, we will do > nothing at this time since payload for Monitor_B has been cleaned up > previously. > > For StarTech 1to3 DP hub, it seems like if we didn't update DPCD payload > ID table then polling for "ACT Handled"(BIT_1 of DPCD 002C0h) will fail > and this polling will last for 3 seconds. > > Therefore, guess the best way is we don't set the proposed_vcpi[] > diretly. Let user of these herlper functions to set the proposed_vcpi > directly. > > [How] > 1. Revert commit 7617e9621bf2 ("drm/dp_mst: clear time slots for ports > invalid") > 2. Tackle the issue in previous commit by skipping those trasient > proposed VCPIs. These stale VCPIs shoulde be explicitly cleared by > user later on. > > Changes since v1: > * Change debug macro to use drm_dbg_kms() instead > * Amend the commit message to add Fixed & Cc tags > > Signed-off-by: Wayne Lin <Wayne.Lin@amd.com> > Fixes: 7617e9621bf2 ("drm/dp_mst: clear time slots for ports invalid") > Cc: Lyude Paul <lyude@redhat.com> > Cc: Wayne Lin <Wayne.Lin@amd.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Maxime Ripard <mripard@kernel.org> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Cc: dri-devel@lists.freedesktop.org > Cc: <stable@vger.kernel.org> # v5.5+ > --- > drivers/gpu/drm/drm_dp_mst_topology.c | 36 ++++++++------------------- > 1 file changed, 10 insertions(+), 26 deletions(-) > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c > index 32b7f8983b94..b41b837db66d 100644 > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > @@ -2501,7 +2501,7 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb, > { > struct drm_dp_mst_topology_mgr *mgr = mstb->mgr; > struct drm_dp_mst_port *port; > - int old_ddps, old_input, ret, i; > + int old_ddps, ret; > u8 new_pdt; > bool new_mcs; > bool dowork = false, create_connector = false; > @@ -2533,7 +2533,6 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb, > } > > old_ddps = port->ddps; > - old_input = port->input; > port->input = conn_stat->input_port; > port->ldps = conn_stat->legacy_device_plug_status; > port->ddps = conn_stat->displayport_device_plug_status; > @@ -2555,28 +2554,6 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb, > dowork = false; > } > > - if (!old_input && old_ddps != port->ddps && !port->ddps) { > - for (i = 0; i < mgr->max_payloads; i++) { > - struct drm_dp_vcpi *vcpi = mgr->proposed_vcpis[i]; > - struct drm_dp_mst_port *port_validated; > - > - if (!vcpi) > - continue; > - > - port_validated = > - container_of(vcpi, struct drm_dp_mst_port, vcpi); > - port_validated = > - drm_dp_mst_topology_get_port_validated(mgr, port_validated); > - if (!port_validated) { > - mutex_lock(&mgr->payload_lock); > - vcpi->num_slots = 0; > - mutex_unlock(&mgr->payload_lock); > - } else { > - drm_dp_mst_topology_put_port(port_validated); > - } > - } > - } > - > if (port->connector) > drm_modeset_unlock(&mgr->base.lock); > else if (create_connector) > @@ -3410,8 +3387,15 @@ int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr) > port = drm_dp_mst_topology_get_port_validated( > mgr, port); > if (!port) { > - mutex_unlock(&mgr->payload_lock); > - return -EINVAL; > + if (vcpi->num_slots == payload->num_slots) { > + cur_slots += vcpi->num_slots; > + payload->start_slot = req_payload.start_slot; > + continue; > + } else { > + drm_dbg_kms("Fail:set payload to invalid sink"); drm_dbg_kms takes a drm_device as first parameter. Harry > + mutex_unlock(&mgr->payload_lock); > + return -EINVAL; > + } > } > put_port = true; > } > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] drm/dp_mst: Do not set proposed vcpi directly 2021-06-16 19:53 ` Harry Wentland @ 2021-06-17 8:47 ` Lin, Wayne 0 siblings, 0 replies; 6+ messages in thread From: Lin, Wayne @ 2021-06-17 8:47 UTC (permalink / raw) To: Wentland, Harry, dri-devel@lists.freedesktop.org Cc: Zuo, Jerry, Pillai, Aurabindo, stable@vger.kernel.org, Thomas Zimmermann, Kazlauskas, Nicholas [AMD Official Use Only] ________________________________________ > From: Wentland, Harry <Harry.Wentland@amd.com> > Sent: Thursday, June 17, 2021 03:53 > To: Lin, Wayne; dri-devel@lists.freedesktop.org > Cc: lyude@redhat.com; Kazlauskas, Nicholas; Zuo, Jerry; Pillai, Aurabindo; Maarten Lankhorst; Maxime Ripard; Thomas Zimmermann; stable@vger.kernel.org > Subject: Re: [PATCH v2 1/2] drm/dp_mst: Do not set proposed vcpi directly > > > > On 2021-06-15 11:55 p.m., Wayne Lin wrote: > > [Why] > > When we receive CSN message to notify one port is disconnected, we will > > implicitly set its corresponding num_slots to 0. Later on, we will > > eventually call drm_dp_update_payload_part1() to arrange down streams. > > > > In drm_dp_update_payload_part1(), we iterate over all proposed_vcpis[] > > to do the update. Not specific to a target sink only. For example, if we > > light up 2 monitors, Monitor_A and Monitor_B, and then we unplug > > Monitor_B. Later on, when we call drm_dp_update_payload_part1() to try > > to update payload for Monitor_A, we'll also implicitly clean payload for > > Monitor_B at the same time. And finally, when we try to call > > drm_dp_update_payload_part1() to clean payload for Monitor_B, we will do > > nothing at this time since payload for Monitor_B has been cleaned up > > previously. > > > > For StarTech 1to3 DP hub, it seems like if we didn't update DPCD payload > > ID table then polling for "ACT Handled"(BIT_1 of DPCD 002C0h) will fail > > and this polling will last for 3 seconds. > > > > Therefore, guess the best way is we don't set the proposed_vcpi[] > > diretly. Let user of these herlper functions to set the proposed_vcpi > > directly. > > > > [How] > > 1. Revert commit 7617e9621bf2 ("drm/dp_mst: clear time slots for ports > > invalid") > > 2. Tackle the issue in previous commit by skipping those trasient > > proposed VCPIs. These stale VCPIs shoulde be explicitly cleared by > > user later on. > > > > Changes since v1: > > * Change debug macro to use drm_dbg_kms() instead > > * Amend the commit message to add Fixed & Cc tags > > > > Signed-off-by: Wayne Lin <Wayne.Lin@amd.com> > > Fixes: 7617e9621bf2 ("drm/dp_mst: clear time slots for ports invalid") > > Cc: Lyude Paul <lyude@redhat.com> > > Cc: Wayne Lin <Wayne.Lin@amd.com> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Cc: Maxime Ripard <mripard@kernel.org> > > Cc: Thomas Zimmermann <tzimmermann@suse.de> > > Cc: dri-devel@lists.freedesktop.org > > Cc: <stable@vger.kernel.org> # v5.5+ > > --- > > drivers/gpu/drm/drm_dp_mst_topology.c | 36 ++++++++------------------- > > 1 file changed, 10 insertions(+), 26 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c > > index 32b7f8983b94..b41b837db66d 100644 > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > > @@ -2501,7 +2501,7 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb, > > { > > struct drm_dp_mst_topology_mgr *mgr = mstb->mgr; > > struct drm_dp_mst_port *port; > > - int old_ddps, old_input, ret, i; > > + int old_ddps, ret; > > u8 new_pdt; > > bool new_mcs; > > bool dowork = false, create_connector = false; > > @@ -2533,7 +2533,6 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb, > > } > > > > old_ddps = port->ddps; > > - old_input = port->input; > > port->input = conn_stat->input_port; > > port->ldps = conn_stat->legacy_device_plug_status; > > port->ddps = conn_stat->displayport_device_plug_status; > > @@ -2555,28 +2554,6 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb, > > dowork = false; > > } > > > > - if (!old_input && old_ddps != port->ddps && !port->ddps) { > > - for (i = 0; i < mgr->max_payloads; i++) { > > - struct drm_dp_vcpi *vcpi = mgr->proposed_vcpis[i]; > > - struct drm_dp_mst_port *port_validated; > > - > > - if (!vcpi) > > - continue; > > - > > - port_validated = > > - container_of(vcpi, struct drm_dp_mst_port, vcpi); > > - port_validated = > > - drm_dp_mst_topology_get_port_validated(mgr, port_validated); > > - if (!port_validated) { > > - mutex_lock(&mgr->payload_lock); > > - vcpi->num_slots = 0; > > - mutex_unlock(&mgr->payload_lock); > > - } else { > > - drm_dp_mst_topology_put_port(port_validated); > > - } > > - } > > - } > > - > > if (port->connector) > > drm_modeset_unlock(&mgr->base.lock); > > else if (create_connector) > > @@ -3410,8 +3387,15 @@ int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr) > > port = drm_dp_mst_topology_get_port_validated( > > mgr, port); > > if (!port) { > > - mutex_unlock(&mgr->payload_lock); > > - return -EINVAL; > > + if (vcpi->num_slots == payload->num_slots) { > > + cur_slots += vcpi->num_slots; > > + payload->start_slot = req_payload.start_slot; > > + continue; > > + } else { > > + drm_dbg_kms("Fail:set payload to invalid sink"); > > drm_dbg_kms takes a drm_device as first parameter. > > Harry Thanks Harry. Should be addressed by another patch provided by José. > > > + mutex_unlock(&mgr->payload_lock); > > + return -EINVAL; > > + } > > } > > put_port = true; > > } > > -- Regards, Wayne ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] drm/dp_mst: Avoid to mess up payload table by ports in stale topology 2021-06-16 3:54 [PATCH v2 0/2] Fix observed mst problems with StarTech hub Wayne Lin 2021-06-16 3:55 ` [PATCH v2 1/2] drm/dp_mst: Do not set proposed vcpi directly Wayne Lin @ 2021-06-16 3:55 ` Wayne Lin 2021-06-16 19:26 ` Souza, Jose 1 sibling, 1 reply; 6+ messages in thread From: Wayne Lin @ 2021-06-16 3:55 UTC (permalink / raw) To: dri-devel Cc: stable, jerry.zuo, aurabindo.pillai, Wayne Lin, Nicholas.Kazlauskas [Why] After unplug/hotplug hub from the system, userspace might start to clear stale payloads gradually. If we call drm_dp_mst_deallocate_vcpi() to release stale VCPI of those ports which are not relating to current topology, we have chane to wrongly clear active payload table entry for current topology. E.g. We have allocated VCPI 1 in current payload table and we call drm_dp_mst_deallocate_vcpi() to clean VCPI 1 in stale topology. In drm_dp_mst_deallocate_vcpi(), it will call drm_dp_mst_put_payload_id() tp put VCPI 1 and which means ID 1 is available again. Thereafter, if we want to allocate a new payload stream, it will find ID 1 is available by drm_dp_mst_assign_payload_id(). However, ID 1 is being used [How] Check target sink is relating to current topology or not before doing any payload table update. Searching upward to find the target sink's relevant root branch device. If the found root branch device is not the same root of current topology, don't update payload table. Changes since v1: * Change debug macro to use drm_dbg_kms() instead * Amend the commit message to add Cc tag. Signed-off-by: Wayne Lin <Wayne.Lin@amd.com> Cc: stable@vger.kernel.org --- drivers/gpu/drm/drm_dp_mst_topology.c | 29 +++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index b41b837db66d..9ac148efd9e4 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -94,6 +94,9 @@ static int drm_dp_mst_register_i2c_bus(struct drm_dp_mst_port *port); static void drm_dp_mst_unregister_i2c_bus(struct drm_dp_mst_port *port); static void drm_dp_mst_kick_tx(struct drm_dp_mst_topology_mgr *mgr); +static bool drm_dp_mst_port_downstream_of_branch(struct drm_dp_mst_port *port, + struct drm_dp_mst_branch *branch); + #define DBG_PREFIX "[dp_mst]" #define DP_STR(x) [DP_ ## x] = #x @@ -3366,6 +3369,7 @@ int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr) struct drm_dp_mst_port *port; int i, j; int cur_slots = 1; + bool skip; mutex_lock(&mgr->payload_lock); for (i = 0; i < mgr->max_payloads; i++) { @@ -3380,6 +3384,14 @@ int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr) port = container_of(vcpi, struct drm_dp_mst_port, vcpi); + mutex_lock(&mgr->lock); + skip = !drm_dp_mst_port_downstream_of_branch(port, mgr->mst_primary); + mutex_unlock(&mgr->lock); + + if (skip) { + drm_dbg_kms("Virtual channel %d is not in current topology\n", i); + continue; + } /* Validated ports don't matter if we're releasing * VCPI */ @@ -3479,6 +3491,7 @@ int drm_dp_update_payload_part2(struct drm_dp_mst_topology_mgr *mgr) struct drm_dp_mst_port *port; int i; int ret = 0; + bool skip; mutex_lock(&mgr->payload_lock); for (i = 0; i < mgr->max_payloads; i++) { @@ -3488,6 +3501,13 @@ int drm_dp_update_payload_part2(struct drm_dp_mst_topology_mgr *mgr) port = container_of(mgr->proposed_vcpis[i], struct drm_dp_mst_port, vcpi); + mutex_lock(&mgr->lock); + skip = !drm_dp_mst_port_downstream_of_branch(port, mgr->mst_primary); + mutex_unlock(&mgr->lock); + + if (skip) + continue; + drm_dbg_kms(mgr->dev, "payload %d %d\n", i, mgr->payloads[i].payload_state); if (mgr->payloads[i].payload_state == DP_PAYLOAD_LOCAL) { ret = drm_dp_create_payload_step2(mgr, port, mgr->proposed_vcpis[i]->vcpi, &mgr->payloads[i]); @@ -4574,9 +4594,18 @@ 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) { + bool skip; + if (!port->vcpi.vcpi) return; + mutex_lock(&mgr->lock); + skip = !drm_dp_mst_port_downstream_of_branch(port, mgr->mst_primary); + mutex_unlock(&mgr->lock); + + if (skip) + return; + drm_dp_mst_put_payload_id(mgr, port->vcpi.vcpi); port->vcpi.num_slots = 0; port->vcpi.pbn = 0; -- 2.17.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] drm/dp_mst: Avoid to mess up payload table by ports in stale topology 2021-06-16 3:55 ` [PATCH v2 2/2] drm/dp_mst: Avoid to mess up payload table by ports in stale topology Wayne Lin @ 2021-06-16 19:26 ` Souza, Jose 0 siblings, 0 replies; 6+ messages in thread From: Souza, Jose @ 2021-06-16 19:26 UTC (permalink / raw) To: dri-devel@lists.freedesktop.org, Wayne.Lin@amd.com, lyude@redhat.com Cc: jerry.zuo@amd.com, aurabindo.pillai@amd.com, Nicholas.Kazlauskas@amd.com, stable@vger.kernel.org On Wed, 2021-06-16 at 11:55 +0800, Wayne Lin wrote: > [Why] > After unplug/hotplug hub from the system, userspace might start to > clear stale payloads gradually. If we call drm_dp_mst_deallocate_vcpi() > to release stale VCPI of those ports which are not relating to current > topology, we have chane to wrongly clear active payload table entry for > current topology. > > E.g. > We have allocated VCPI 1 in current payload table and we call > drm_dp_mst_deallocate_vcpi() to clean VCPI 1 in stale topology. In > drm_dp_mst_deallocate_vcpi(), it will call drm_dp_mst_put_payload_id() > tp put VCPI 1 and which means ID 1 is available again. Thereafter, if we > want to allocate a new payload stream, it will find ID 1 is available by > drm_dp_mst_assign_payload_id(). However, ID 1 is being used > > [How] > Check target sink is relating to current topology or not before doing > any payload table update. > Searching upward to find the target sink's relevant root branch device. > If the found root branch device is not the same root of current > topology, don't update payload table. > > Changes since v1: > * Change debug macro to use drm_dbg_kms() instead > * Amend the commit message to add Cc tag. > > Signed-off-by: Wayne Lin <Wayne.Lin@amd.com> > Cc: stable@vger.kernel.org > --- > drivers/gpu/drm/drm_dp_mst_topology.c | 29 +++++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c > index b41b837db66d..9ac148efd9e4 100644 > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > @@ -94,6 +94,9 @@ static int drm_dp_mst_register_i2c_bus(struct drm_dp_mst_port *port); > static void drm_dp_mst_unregister_i2c_bus(struct drm_dp_mst_port *port); > static void drm_dp_mst_kick_tx(struct drm_dp_mst_topology_mgr *mgr); > > +static bool drm_dp_mst_port_downstream_of_branch(struct drm_dp_mst_port *port, > + struct drm_dp_mst_branch *branch); > + > #define DBG_PREFIX "[dp_mst]" > > #define DP_STR(x) [DP_ ## x] = #x > @@ -3366,6 +3369,7 @@ int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr) > struct drm_dp_mst_port *port; > int i, j; > int cur_slots = 1; > + bool skip; > > mutex_lock(&mgr->payload_lock); > for (i = 0; i < mgr->max_payloads; i++) { > @@ -3380,6 +3384,14 @@ int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr) > port = container_of(vcpi, struct drm_dp_mst_port, > vcpi); > > + mutex_lock(&mgr->lock); > + skip = !drm_dp_mst_port_downstream_of_branch(port, mgr->mst_primary); > + mutex_unlock(&mgr->lock); > + > + if (skip) { > + drm_dbg_kms("Virtual channel %d is not in current topology\n", i); Missing dev/drm parameter and breaking the build. > + continue; > + } > /* Validated ports don't matter if we're releasing > * VCPI > */ > @@ -3479,6 +3491,7 @@ int drm_dp_update_payload_part2(struct drm_dp_mst_topology_mgr *mgr) > struct drm_dp_mst_port *port; > int i; > int ret = 0; > + bool skip; > > mutex_lock(&mgr->payload_lock); > for (i = 0; i < mgr->max_payloads; i++) { > @@ -3488,6 +3501,13 @@ int drm_dp_update_payload_part2(struct drm_dp_mst_topology_mgr *mgr) > > port = container_of(mgr->proposed_vcpis[i], struct drm_dp_mst_port, vcpi); > > + mutex_lock(&mgr->lock); > + skip = !drm_dp_mst_port_downstream_of_branch(port, mgr->mst_primary); > + mutex_unlock(&mgr->lock); > + > + if (skip) > + continue; > + > drm_dbg_kms(mgr->dev, "payload %d %d\n", i, mgr->payloads[i].payload_state); > if (mgr->payloads[i].payload_state == DP_PAYLOAD_LOCAL) { > ret = drm_dp_create_payload_step2(mgr, port, mgr->proposed_vcpis[i]->vcpi, &mgr->payloads[i]); > @@ -4574,9 +4594,18 @@ 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) > { > + bool skip; > + > if (!port->vcpi.vcpi) > return; > > + mutex_lock(&mgr->lock); > + skip = !drm_dp_mst_port_downstream_of_branch(port, mgr->mst_primary); > + mutex_unlock(&mgr->lock); > + > + if (skip) > + return; > + > drm_dp_mst_put_payload_id(mgr, port->vcpi.vcpi); > port->vcpi.num_slots = 0; > port->vcpi.pbn = 0; ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-06-17 8:47 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-06-16 3:54 [PATCH v2 0/2] Fix observed mst problems with StarTech hub Wayne Lin 2021-06-16 3:55 ` [PATCH v2 1/2] drm/dp_mst: Do not set proposed vcpi directly Wayne Lin 2021-06-16 19:53 ` Harry Wentland 2021-06-17 8:47 ` Lin, Wayne 2021-06-16 3:55 ` [PATCH v2 2/2] drm/dp_mst: Avoid to mess up payload table by ports in stale topology Wayne Lin 2021-06-16 19:26 ` Souza, Jose
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox