[PATCH 0/2] Fix observed mst problems with StarTech hub

[PATCH 0/2] Fix observed mst problems with StarTech hub

[Wayne Lin]

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

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

[PATCH 1/2] drm/dp_mst: Do not set proposed vcpi directly

[Wayne Lin]

[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.

Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
---
 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 8f5a008501d9..5fc261014a20 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -2497,7 +2497,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;
@@ -2529,7 +2529,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;
@@ -2552,28 +2551,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)
@@ -3404,8 +3381,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_DEBUG_KMS("Fail:set payload to invalid sink");
+						mutex_unlock(&mgr->payload_lock);
+						return -EINVAL;
+					}
 				}
 				put_port = true;
 			}
-- 
2.17.1

[PATCH 2/2] drm/dp_mst: Avoid to mess up payload table by ports in stale topology

[Wayne Lin]

[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.

Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
---
 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 5fc261014a20..3d988d54df89 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
@@ -3360,6 +3363,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++) {
@@ -3374,6 +3378,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_DEBUG_KMS("Virtual channel %d is not in current topology\n", i);
+				continue;
+			}
 			/* Validated ports don't matter if we're releasing
 			 * VCPI
 			 */
@@ -3473,6 +3485,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++) {
@@ -3482,6 +3495,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_DEBUG_KMS("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]);
@@ -4562,9 +4582,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

Re: [PATCH 2/2] drm/dp_mst: Avoid to mess up payload table by ports in stale topology

[Lyude Paul]

On Fri, 2021-05-28 at 21: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.
> 
> Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
> ---
>  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 5fc261014a20..3d988d54df89 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
> @@ -3360,6 +3363,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++) {
> @@ -3374,6 +3378,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_DEBUG_KMS("Virtual channel %d is not in
> current topology\n", i);

sorry I totally missed this on my first look - but this is the wrong debugging
macro (drm_dbg_kms() should be used) and as well, this patch doesn't actually
apply because it needs to be rebased against drm-tip. Could you fix this,
rebase the patches, and send a new version along with the fixes tags I
mentioned earlier? You can generate them using the dim tool here:

https://drm.pages.freedesktop.org/maintainer-tools/committer-drm-intel.html#labeling-fixes-before-pushing

> +                               continue;
> +                       }
>                         /* Validated ports don't matter if we're releasing
>                          * VCPI
>                          */
> @@ -3473,6 +3485,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++) {
> @@ -3482,6 +3495,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_DEBUG_KMS("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]);
> @@ -4562,9 +4582,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;

-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat

Re: [PATCH 2/2] drm/dp_mst: Avoid to mess up payload table by ports in stale topology

[Lin, Wayne]

[Public]

________________________________________
> From: Lyude Paul <lyude@redhat.com>
> Sent: Wednesday, June 16, 2021 03:44
> To: Lin, Wayne; dri-devel@lists.freedesktop.org
> Cc: Kazlauskas, Nicholas; Wentland, Harry; Zuo, Jerry; Pillai, Aurabindo
> Subject: Re: [PATCH 2/2] drm/dp_mst: Avoid to mess up payload table by ports in stale topology
>
> On Fri, 2021-05-28 at 21: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.
> >
> > Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
> > ---
> >  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 5fc261014a20..3d988d54df89 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
> > @@ -3360,6 +3363,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++) {
> > @@ -3374,6 +3378,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_DEBUG_KMS("Virtual channel %d is not in
> > current topology\n", i);
>
> sorry I totally missed this on my first look - but this is the wrong debugging
> macro (drm_dbg_kms() should be used) and as well, this patch doesn't actually
> apply because it needs to be rebased against drm-tip. Could you fix this,
> rebase the patches, and send a new version along with the fixes tags I
> mentioned earlier? You can generate them using the dim tool here:
>
> https://drm.pages.freedesktop.org/maintainer-tools/committer-drm-intel.html#labeling-fixes-before-pushing
>

Thanks for your time! Will do.

> > +                               continue;
> > +                       }
> >                         /* Validated ports don't matter if we're releasing
> >                          * VCPI
> >                          */
> > @@ -3473,6 +3485,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++) {
> > @@ -3482,6 +3495,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_DEBUG_KMS("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]);
> > @@ -4562,9 +4582,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;
>
> --
> Cheers,
>  Lyude Paul (she/her)
>  Software Engineer at Red Hat
--
Regards,
Wayne Lin

Re: [PATCH 0/2] Fix observed mst problems with StarTech hub

[Lyude Paul]

haha. turns out it actually was a good thing I was busy with work today,
because I ended up testing some backports and running into the exact same MST
bug these patches appear to fix. How convienent :)

anyway-I looked over this and this looks good to me (and IMO, I like these
fixes more then the workarounds they replace!). The one thing we do need to
fix here though is this appears to definitely fix a regression, so we need to
make sure we actually bisect the issue that this patch is fixing so we can add
the appropriate Fixes: and Cc: tags so that these fixes get backported to
earlier stable kernel versions.

I definitely need this fix in asap though for my own work, so I am going to
see if I can start bisecting this. If I manage to figure out what's breaking
it before my workday ends today I'll just add my R-b and push this upstream,
otherwise I'll probably just push this first thing on monday. If you see this
message beforethen and know what kernel version introduced this issue, feel
free to respond ;)

On Fri, 2021-05-28 at 21:55 +0800, Wayne Lin wrote:
> 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
> 
> 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(-)
> 

-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat

Re: [PATCH 0/2] Fix observed mst problems with StarTech hub

[Lin, Wayne]

[Public]

Thanks Lyude for the review!

For the 1st patch, it's trying to fix the patch
7617e9621bf2 ("drm/dp_mst: clear time slots for ports invalid").

As for the 2nd one, it's my first time to test on this hub and I not
yet know the exact regression point. I'm also not quite sure if this
regression is caused by driver or just behavior changes from upper layer.
I was thinking that this patch is just an enhancement for our mst
helper functions and should be applicable for older kernel versions.

Thanks for your time!

Regards,
Wayne
________________________________________
> From: Lyude Paul <lyude@redhat.com>
> Sent: Saturday, June 12, 2021 06:43
> To: Lin, Wayne; dri-devel@lists.freedesktop.org
> Cc: Kazlauskas, Nicholas; Wentland, Harry; Zuo, Jerry; Pillai, Aurabindo
> Subject: Re: [PATCH 0/2] Fix observed mst problems with StarTech hub
>
> haha. turns out it actually was a good thing I was busy with work today,
> because I ended up testing some backports and running into the exact same MST
> bug these patches appear to fix. How convienent :)
>
> anyway-I looked over this and this looks good to me (and IMO, I like these
> fixes more then the workarounds they replace!). The one thing we do need to
> fix here though is this appears to definitely fix a regression, so we need to
> make sure we actually bisect the issue that this patch is fixing so we can add
> the appropriate Fixes: and Cc: tags so that these fixes get backported to
> earlier stable kernel versions.
>
> I definitely need this fix in asap though for my own work, so I am going to
> see if I can start bisecting this. If I manage to figure out what's breaking
> it before my workday ends today I'll just add my R-b and push this upstream,
> otherwise I'll probably just push this first thing on monday. If you see this
> message beforethen and know what kernel version introduced this issue, feel
> free to respond ;)
>
> On Fri, 2021-05-28 at 21:55 +0800, Wayne Lin wrote:
> > 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
> >
> > 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(-)
> >
>
> --
> Cheers,
>  Lyude Paul (she/her)
>  Software Engineer at Red Hat