[PATCH 0/2] Take SST-only branch device into account

[PATCH 0/2] Take SST-only branch device into account

[Wayne Lin]

Noticed this while testing 4 ports MST hub from StarTech.com.
While plugging in and display a MST monitor(Dell U2417H), change the MST
feature to off from OSD. Monitor then can't display anymore.

After analyzing, found out that the CSN reports the specific port from
Device with MST Branching Unit (Message_Capability_Status=1 &&
Peer_Device_Type=010) to SST-only Branch Device ((Message_Capability_Status=0 &&
Peer_Device_Type=010). Current code expects peer device as MST branch
device and can handle sideband message once the peer device type is
declared as Peer_Device_Type=010. But for SST-only Branch Device, its
Peer_Device_Type is 010 and can't handle sideband message. For this case,
we should follow the same handling procedure as the case in
DP_PEER_DEVICE_DP_LEGACY_CONV & DP_PEER_DEVICE_SST_SINK.

This series is trying to add the SST-only branch device case in and remain
the original processing logic in current code.

Wayne Lin (2):
  drm/dp_mst: Add a function to determine the mst end device
  drm/dp_mst: Handle SST-only branch device case

 drivers/gpu/drm/drm_dp_mst_topology.c | 147 +++++++++++++++-----------
 1 file changed, 84 insertions(+), 63 deletions(-)

-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/2] drm/dp_mst: Add a function to determine the mst end device

[Wayne Lin]

[Why]
For later usage convenience, add the function
drm_dp_mst_is_dp_mst_end_device() to decide whether a peer device
connected to a DFP is mst end device. Which also indicates if the peer
device is capable of handling message or not.

Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index eebf325d7f48..8f54b241db08 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -1918,6 +1918,22 @@ static u8 drm_dp_calculate_rad(struct drm_dp_mst_port *port,
 	return parent_lct + 1;
 }
 
+static bool drm_dp_mst_is_dp_mst_end_device(u8 pdt, bool mcs)
+{
+	switch (pdt) {
+	case DP_PEER_DEVICE_DP_LEGACY_CONV:
+	case DP_PEER_DEVICE_SST_SINK:
+		return true;
+	case DP_PEER_DEVICE_MST_BRANCHING:
+		/* For sst branch device */
+		if (!mcs)
+			return true;
+
+		return false;
+	}
+	return true;
+}
+
 static int drm_dp_port_set_pdt(struct drm_dp_mst_port *port, u8 new_pdt)
 {
 	struct drm_dp_mst_topology_mgr *mgr = port->mgr;
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/dp_mst: Add a function to determine the mst end device

[Lyude Paul]

This patch series looks awesome so far, thank you for the great work! This
patch looks great, I think we should just squash it into the next patch though
since we don't use this function until then.

On Wed, 2020-01-08 at 16:44 +0800, Wayne Lin wrote:
> [Why]
> For later usage convenience, add the function
> drm_dp_mst_is_dp_mst_end_device() to decide whether a peer device
> connected to a DFP is mst end device. Which also indicates if the peer
> device is capable of handling message or not.
> 
> Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index eebf325d7f48..8f54b241db08 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -1918,6 +1918,22 @@ static u8 drm_dp_calculate_rad(struct drm_dp_mst_port
> *port,
>  	return parent_lct + 1;
>  }
>  
> +static bool drm_dp_mst_is_dp_mst_end_device(u8 pdt, bool mcs)
> +{
> +	switch (pdt) {
> +	case DP_PEER_DEVICE_DP_LEGACY_CONV:
> +	case DP_PEER_DEVICE_SST_SINK:
> +		return true;
> +	case DP_PEER_DEVICE_MST_BRANCHING:
> +		/* For sst branch device */
> +		if (!mcs)
> +			return true;
> +
> +		return false;
> +	}
> +	return true;
> +}
> +
>  static int drm_dp_port_set_pdt(struct drm_dp_mst_port *port, u8 new_pdt)
>  {
>  	struct drm_dp_mst_topology_mgr *mgr = port->mgr;
-- 
Cheers,
	Lyude Paul

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [PATCH 1/2] drm/dp_mst: Add a function to determine the mst end device

[Lin, Wayne]

[AMD Public Use]



> -----Original Message-----
> From: Lyude Paul <lyude@redhat.com>
> Sent: Wednesday, January 15, 2020 5:16 AM
> To: Lin, Wayne <Wayne.Lin@amd.com>; dri-devel@lists.freedesktop.org;
> amd-gfx@lists.freedesktop.org
> Cc: Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Wentland, Harry
> <Harry.Wentland@amd.com>; Zuo, Jerry <Jerry.Zuo@amd.com>
> Subject: Re: [PATCH 1/2] drm/dp_mst: Add a function to determine the mst end
> device
> 
> This patch series looks awesome so far, thank you for the great work! This
> patch looks great, I think we should just squash it into the next patch though
> since we don't use this function until then.
> 
Thanks for your time.
I will squash it in the new version.

> On Wed, 2020-01-08 at 16:44 +0800, Wayne Lin wrote:
> > [Why]
> > For later usage convenience, add the function
> > drm_dp_mst_is_dp_mst_end_device() to decide whether a peer device
> > connected to a DFP is mst end device. Which also indicates if the peer
> > device is capable of handling message or not.
> >
> > Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
> > ---
> >  drivers/gpu/drm/drm_dp_mst_topology.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index eebf325d7f48..8f54b241db08 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -1918,6 +1918,22 @@ static u8 drm_dp_calculate_rad(struct
> > drm_dp_mst_port *port,
> >  	return parent_lct + 1;
> >  }
> >
> > +static bool drm_dp_mst_is_dp_mst_end_device(u8 pdt, bool mcs) {
> > +	switch (pdt) {
> > +	case DP_PEER_DEVICE_DP_LEGACY_CONV:
> > +	case DP_PEER_DEVICE_SST_SINK:
> > +		return true;
> > +	case DP_PEER_DEVICE_MST_BRANCHING:
> > +		/* For sst branch device */
> > +		if (!mcs)
> > +			return true;
> > +
> > +		return false;
> > +	}
> > +	return true;
> > +}
> > +
> >  static int drm_dp_port_set_pdt(struct drm_dp_mst_port *port, u8
> > new_pdt)  {
> >  	struct drm_dp_mst_topology_mgr *mgr = port->mgr;
> --
> Cheers,
> 	Lyude Paul
--
Best regards,
Wayne Lin
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] drm/dp_mst: Handle SST-only branch device case

[Wayne Lin]

[Why]
While handling LINK_ADDRESS reply, current code expects a peer device
can handle sideband message once the peer device type is reported as
DP_PEER_DEVICE_MST_BRANCHING. However, when the connected device is
a SST branch case, it can't handle the sideband message(MST_CAP=0 in
DPCD 00021h).

Current code will try to send LINK_ADDRESS to SST branch device and end
up with message timeout and monitor can't display normally. As the
result of that, we should take SST branch device into account.

[How]
According to DP 1.4 spec, we can use Peer_Device_Type as
DP_PEER_DEVICE_MST_BRANCHING and Message_Capability_Status as 0 to
indicate peer device as a SST-only branch device.

Fix following:
- Take SST-only branch device case into account in
drm_dp_port_set_pdt() and add a new parameter 'new_mcs'. Take sst branch
device case as the same case as DP_PEER_DEVICE_DP_LEGACY_CONV and
DP_PEER_DEVICE_SST_SINK. All original handling logics remain.
- Take SST-only branch device case into account in
drm_dp_mst_port_add_connector().
- Fix some parts in drm_dp_mst_handle_link_address_port() to have SST
branch device case into consideration.
- Fix the arguments of drm_dp_port_set_pdt() in
drm_dp_mst_handle_conn_stat().
- Have SST branch device also report
connector_status_connected when the ddps is true
in drm_dp_mst_detect_port()
- Fix the arguments of drm_dp_port_set_pdt() in
drm_dp_delayed_destroy_port()

Fixes: c485e2c97dae ("drm/dp_mst: Refactor pdt setup/teardown, add more locking")
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Harry Wentland <hwentlan@amd.com>
Cc: Lyude Paul <lyude@redhat.com>
Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 131 +++++++++++++-------------
 1 file changed, 68 insertions(+), 63 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 8f54b241db08..4395d5cc0645 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -1934,73 +1934,74 @@ static bool drm_dp_mst_is_dp_mst_end_device(u8 pdt, bool mcs)
 	return true;
 }
 
-static int drm_dp_port_set_pdt(struct drm_dp_mst_port *port, u8 new_pdt)
+static int
+drm_dp_port_set_pdt(struct drm_dp_mst_port *port, u8 new_pdt,
+		    bool new_mcs)
 {
 	struct drm_dp_mst_topology_mgr *mgr = port->mgr;
 	struct drm_dp_mst_branch *mstb;
 	u8 rad[8], lct;
 	int ret = 0;
 
-	if (port->pdt == new_pdt)
+	if (port->pdt == new_pdt && port->mcs == new_mcs)
 		return 0;
 
 	/* Teardown the old pdt, if there is one */
-	switch (port->pdt) {
-	case DP_PEER_DEVICE_DP_LEGACY_CONV:
-	case DP_PEER_DEVICE_SST_SINK:
-		/*
-		 * If the new PDT would also have an i2c bus, don't bother
-		 * with reregistering it
-		 */
-		if (new_pdt == DP_PEER_DEVICE_DP_LEGACY_CONV ||
-		    new_pdt == DP_PEER_DEVICE_SST_SINK) {
-			port->pdt = new_pdt;
-			return 0;
-		}
+	if (port->pdt != DP_PEER_DEVICE_NONE) {
+		if (drm_dp_mst_is_dp_mst_end_device(port->pdt, port->mcs)) {
+			/*
+			 * If the new PDT would also have an i2c bus,
+			 * don't bother with reregistering it
+			 */
+			if (new_pdt != DP_PEER_DEVICE_NONE &&
+			    drm_dp_mst_is_dp_mst_end_device(new_pdt, new_mcs)) {
+				port->pdt = new_pdt;
+				port->mcs = new_mcs;
+				return 0;
+			}
 
-		/* remove i2c over sideband */
-		drm_dp_mst_unregister_i2c_bus(&port->aux);
-		break;
-	case DP_PEER_DEVICE_MST_BRANCHING:
-		mutex_lock(&mgr->lock);
-		drm_dp_mst_topology_put_mstb(port->mstb);
-		port->mstb = NULL;
-		mutex_unlock(&mgr->lock);
-		break;
+			/* remove i2c over sideband */
+			drm_dp_mst_unregister_i2c_bus(&port->aux);
+		} else {
+			mutex_lock(&mgr->lock);
+			drm_dp_mst_topology_put_mstb(port->mstb);
+			port->mstb = NULL;
+			mutex_unlock(&mgr->lock);
+		}
 	}
 
 	port->pdt = new_pdt;
-	switch (port->pdt) {
-	case DP_PEER_DEVICE_DP_LEGACY_CONV:
-	case DP_PEER_DEVICE_SST_SINK:
-		/* add i2c over sideband */
-		ret = drm_dp_mst_register_i2c_bus(&port->aux);
-		break;
+	port->mcs = new_mcs;
 
-	case DP_PEER_DEVICE_MST_BRANCHING:
-		lct = drm_dp_calculate_rad(port, rad);
-		mstb = drm_dp_add_mst_branch_device(lct, rad);
-		if (!mstb) {
-			ret = -ENOMEM;
-			DRM_ERROR("Failed to create MSTB for port %p", port);
-			goto out;
-		}
+	if (port->pdt != DP_PEER_DEVICE_NONE) {
+		if (drm_dp_mst_is_dp_mst_end_device(port->pdt, port->mcs)) {
+			/* add i2c over sideband */
+			ret = drm_dp_mst_register_i2c_bus(&port->aux);
+		} else {
+			lct = drm_dp_calculate_rad(port, rad);
+			mstb = drm_dp_add_mst_branch_device(lct, rad);
+			if (!mstb) {
+				ret = -ENOMEM;
+				DRM_ERROR("Failed to create MSTB for port %p",
+					  port);
+				goto out;
+			}
 
-		mutex_lock(&mgr->lock);
-		port->mstb = mstb;
-		mstb->mgr = port->mgr;
-		mstb->port_parent = port;
+			mutex_lock(&mgr->lock);
+			port->mstb = mstb;
+			mstb->mgr = port->mgr;
+			mstb->port_parent = port;
 
-		/*
-		 * Make sure this port's memory allocation stays
-		 * around until its child MSTB releases it
-		 */
-		drm_dp_mst_get_port_malloc(port);
-		mutex_unlock(&mgr->lock);
+			/*
+			 * Make sure this port's memory allocation stays
+			 * around until its child MSTB releases it
+			 */
+			drm_dp_mst_get_port_malloc(port);
+			mutex_unlock(&mgr->lock);
 
-		/* And make sure we send a link address for this */
-		ret = 1;
-		break;
+			/* And make sure we send a link address for this */
+			ret = 1;
+		}
 	}
 
 out:
@@ -2153,12 +2154,12 @@ drm_dp_mst_port_add_connector(struct drm_dp_mst_branch *mstb,
 		goto error;
 	}
 
-	if ((port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV ||
-	     port->pdt == DP_PEER_DEVICE_SST_SINK) &&
-	    port->port_num >= DP_MST_LOGICAL_PORT_0) {
-		port->cached_edid = drm_get_edid(port->connector,
-						 &port->aux.ddc);
-		drm_connector_set_tile_property(port->connector);
+	if (port->pdt != DP_PEER_DEVICE_NONE) {
+		if (drm_dp_mst_is_dp_mst_end_device(port->pdt, port->mcs)) {
+			port->cached_edid = drm_get_edid(port->connector,
+							 &port->aux.ddc);
+			drm_connector_set_tile_property(port->connector);
+		}
 	}
 
 	mgr->cbs->register_connector(port->connector);
@@ -2223,6 +2224,7 @@ drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb,
 	struct drm_dp_mst_port *port;
 	int old_ddps = 0, ret;
 	u8 new_pdt = DP_PEER_DEVICE_NONE;
+	bool new_mcs = 0;
 	bool created = false, send_link_addr = false, changed = false;
 
 	port = drm_dp_get_port(mstb, port_msg->port_number);
@@ -2267,7 +2269,7 @@ drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb,
 	port->input = port_msg->input_port;
 	if (!port->input)
 		new_pdt = port_msg->peer_device_type;
-	port->mcs = port_msg->mcs;
+	new_mcs = port_msg->mcs;
 	port->ddps = port_msg->ddps;
 	port->ldps = port_msg->legacy_device_plug_status;
 	port->dpcd_rev = port_msg->dpcd_revision;
@@ -2295,7 +2297,7 @@ drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb,
 		}
 	}
 
-	ret = drm_dp_port_set_pdt(port, new_pdt);
+	ret = drm_dp_port_set_pdt(port, new_pdt, new_mcs);
 	if (ret == 1) {
 		send_link_addr = true;
 	} else if (ret < 0) {
@@ -2309,7 +2311,8 @@ drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb,
 	 * we're coming out of suspend. In this case, always resend the link
 	 * address if there's an MSTB on this port
 	 */
-	if (!created && port->pdt == DP_PEER_DEVICE_MST_BRANCHING)
+	if (!created && port->pdt == DP_PEER_DEVICE_MST_BRANCHING &&
+	    port->mcs)
 		send_link_addr = true;
 
 	if (port->connector)
@@ -2346,6 +2349,7 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
 	struct drm_dp_mst_port *port;
 	int old_ddps, old_input, ret, i;
 	u8 new_pdt;
+	bool new_mcs;
 	bool dowork = false, create_connector = false;
 
 	port = drm_dp_get_port(mstb, conn_stat->port_number);
@@ -2377,7 +2381,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->mcs = conn_stat->message_capability_status;
 	port->ldps = conn_stat->legacy_device_plug_status;
 	port->ddps = conn_stat->displayport_device_plug_status;
 
@@ -2390,8 +2393,8 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
 	}
 
 	new_pdt = port->input ? DP_PEER_DEVICE_NONE : conn_stat->peer_device_type;
-
-	ret = drm_dp_port_set_pdt(port, new_pdt);
+	new_mcs = conn_stat->message_capability_status;
+	ret = drm_dp_port_set_pdt(port, new_pdt, new_mcs);
 	if (ret == 1) {
 		dowork = true;
 	} else if (ret < 0) {
@@ -3958,6 +3961,8 @@ drm_dp_mst_detect_port(struct drm_connector *connector,
 	switch (port->pdt) {
 	case DP_PEER_DEVICE_NONE:
 	case DP_PEER_DEVICE_MST_BRANCHING:
+		if (!port->mcs)
+			ret = connector_status_connected;
 		break;
 
 	case DP_PEER_DEVICE_SST_SINK:
@@ -4597,7 +4602,7 @@ drm_dp_delayed_destroy_port(struct drm_dp_mst_port *port)
 	if (port->connector)
 		port->mgr->cbs->destroy_connector(port->mgr, port->connector);
 
-	drm_dp_port_set_pdt(port, DP_PEER_DEVICE_NONE);
+	drm_dp_port_set_pdt(port, DP_PEER_DEVICE_NONE, port->mcs);
 	drm_dp_mst_put_port_malloc(port);
 }
 
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/dp_mst: Handle SST-only branch device case

[Lyude Paul]

On Wed, 2020-01-08 at 16:44 +0800, Wayne Lin wrote:
> [Why]
> While handling LINK_ADDRESS reply, current code expects a peer device
> can handle sideband message once the peer device type is reported as
> DP_PEER_DEVICE_MST_BRANCHING. However, when the connected device is
> a SST branch case, it can't handle the sideband message(MST_CAP=0 in
> DPCD 00021h).
> 
> Current code will try to send LINK_ADDRESS to SST branch device and end
> up with message timeout and monitor can't display normally. As the
> result of that, we should take SST branch device into account.
> 
> [How]
> According to DP 1.4 spec, we can use Peer_Device_Type as
> DP_PEER_DEVICE_MST_BRANCHING and Message_Capability_Status as 0 to
> indicate peer device as a SST-only branch device.
> 
> Fix following:
> - Take SST-only branch device case into account in
> drm_dp_port_set_pdt() and add a new parameter 'new_mcs'. Take sst branch
> device case as the same case as DP_PEER_DEVICE_DP_LEGACY_CONV and
> DP_PEER_DEVICE_SST_SINK. All original handling logics remain.
> - Take SST-only branch device case into account in
> drm_dp_mst_port_add_connector().
> - Fix some parts in drm_dp_mst_handle_link_address_port() to have SST
> branch device case into consideration.
> - Fix the arguments of drm_dp_port_set_pdt() in
> drm_dp_mst_handle_conn_stat().
> - Have SST branch device also report
> connector_status_connected when the ddps is true
> in drm_dp_mst_detect_port()
> - Fix the arguments of drm_dp_port_set_pdt() in
> drm_dp_delayed_destroy_port()
> 
> Fixes: c485e2c97dae ("drm/dp_mst: Refactor pdt setup/teardown, add more
> locking")
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Harry Wentland <hwentlan@amd.com>
> Cc: Lyude Paul <lyude@redhat.com>
> Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 131 +++++++++++++-------------
>  1 file changed, 68 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 8f54b241db08..4395d5cc0645 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -1934,73 +1934,74 @@ static bool drm_dp_mst_is_dp_mst_end_device(u8 pdt,
> bool mcs)
>  	return true;
>  }
>  
> -static int drm_dp_port_set_pdt(struct drm_dp_mst_port *port, u8 new_pdt)
> +static int
> +drm_dp_port_set_pdt(struct drm_dp_mst_port *port, u8 new_pdt,
> +		    bool new_mcs)
>  {
>  	struct drm_dp_mst_topology_mgr *mgr = port->mgr;
>  	struct drm_dp_mst_branch *mstb;
>  	u8 rad[8], lct;
>  	int ret = 0;
>  
> -	if (port->pdt == new_pdt)
> +	if (port->pdt == new_pdt && port->mcs == new_mcs)
>  		return 0;
>  
>  	/* Teardown the old pdt, if there is one */
> -	switch (port->pdt) {
> -	case DP_PEER_DEVICE_DP_LEGACY_CONV:
> -	case DP_PEER_DEVICE_SST_SINK:
> -		/*
> -		 * If the new PDT would also have an i2c bus, don't bother
> -		 * with reregistering it
> -		 */
> -		if (new_pdt == DP_PEER_DEVICE_DP_LEGACY_CONV ||
> -		    new_pdt == DP_PEER_DEVICE_SST_SINK) {
> -			port->pdt = new_pdt;
> -			return 0;
> -		}
> +	if (port->pdt != DP_PEER_DEVICE_NONE) {
> +		if (drm_dp_mst_is_dp_mst_end_device(port->pdt, port->mcs)) {
> +			/*
> +			 * If the new PDT would also have an i2c bus,
> +			 * don't bother with reregistering it
> +			 */
> +			if (new_pdt != DP_PEER_DEVICE_NONE &&
> +			    drm_dp_mst_is_dp_mst_end_device(new_pdt, new_mcs))
> {
> +				port->pdt = new_pdt;
> +				port->mcs = new_mcs;
> +				return 0;
> +			}
>  
> -		/* remove i2c over sideband */
> -		drm_dp_mst_unregister_i2c_bus(&port->aux);
> -		break;
> -	case DP_PEER_DEVICE_MST_BRANCHING:
> -		mutex_lock(&mgr->lock);
> -		drm_dp_mst_topology_put_mstb(port->mstb);
> -		port->mstb = NULL;
> -		mutex_unlock(&mgr->lock);
> -		break;
> +			/* remove i2c over sideband */
> +			drm_dp_mst_unregister_i2c_bus(&port->aux);
> +		} else {
> +			mutex_lock(&mgr->lock);
> +			drm_dp_mst_topology_put_mstb(port->mstb);
> +			port->mstb = NULL;
> +			mutex_unlock(&mgr->lock);
> +		}
>  	}
>  
>  	port->pdt = new_pdt;
> -	switch (port->pdt) {
> -	case DP_PEER_DEVICE_DP_LEGACY_CONV:
> -	case DP_PEER_DEVICE_SST_SINK:
> -		/* add i2c over sideband */
> -		ret = drm_dp_mst_register_i2c_bus(&port->aux);
> -		break;
> +	port->mcs = new_mcs;
>  
> -	case DP_PEER_DEVICE_MST_BRANCHING:
> -		lct = drm_dp_calculate_rad(port, rad);
> -		mstb = drm_dp_add_mst_branch_device(lct, rad);
> -		if (!mstb) {
> -			ret = -ENOMEM;
> -			DRM_ERROR("Failed to create MSTB for port %p", port);
> -			goto out;
> -		}
> +	if (port->pdt != DP_PEER_DEVICE_NONE) {
> +		if (drm_dp_mst_is_dp_mst_end_device(port->pdt, port->mcs)) {
> +			/* add i2c over sideband */
> +			ret = drm_dp_mst_register_i2c_bus(&port->aux);
> +		} else {
> +			lct = drm_dp_calculate_rad(port, rad);
> +			mstb = drm_dp_add_mst_branch_device(lct, rad);
> +			if (!mstb) {
> +				ret = -ENOMEM;
> +				DRM_ERROR("Failed to create MSTB for port %p",
> +					  port);
> +				goto out;
> +			}
>  
> -		mutex_lock(&mgr->lock);
> -		port->mstb = mstb;
> -		mstb->mgr = port->mgr;
> -		mstb->port_parent = port;
> +			mutex_lock(&mgr->lock);
> +			port->mstb = mstb;
> +			mstb->mgr = port->mgr;
> +			mstb->port_parent = port;
>  
> -		/*
> -		 * Make sure this port's memory allocation stays
> -		 * around until its child MSTB releases it
> -		 */
> -		drm_dp_mst_get_port_malloc(port);
> -		mutex_unlock(&mgr->lock);
> +			/*
> +			 * Make sure this port's memory allocation stays
> +			 * around until its child MSTB releases it
> +			 */
> +			drm_dp_mst_get_port_malloc(port);
> +			mutex_unlock(&mgr->lock);
>  
> -		/* And make sure we send a link address for this */
> -		ret = 1;
> -		break;
> +			/* And make sure we send a link address for this */
> +			ret = 1;
> +		}
>  	}
>  
>  out:
> @@ -2153,12 +2154,12 @@ drm_dp_mst_port_add_connector(struct
> drm_dp_mst_branch *mstb,
>  		goto error;
>  	}
>  
> -	if ((port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV ||
> -	     port->pdt == DP_PEER_DEVICE_SST_SINK) &&
> -	    port->port_num >= DP_MST_LOGICAL_PORT_0) {
> -		port->cached_edid = drm_get_edid(port->connector,
> -						 &port->aux.ddc);
> -		drm_connector_set_tile_property(port->connector);
> +	if (port->pdt != DP_PEER_DEVICE_NONE) {
> +		if (drm_dp_mst_is_dp_mst_end_device(port->pdt, port->mcs)) {
> +			port->cached_edid = drm_get_edid(port->connector,
> +							 &port->aux.ddc);
> +			drm_connector_set_tile_property(port->connector);
> +		}

I'd combine these two if statements here into one, otherwise this looks great.
Thank you for all of the great fixes recently :)

Reviewed-by: Lyude Paul <lyude@redhat.com>


>  	}
>  
>  	mgr->cbs->register_connector(port->connector);
> @@ -2223,6 +2224,7 @@ drm_dp_mst_handle_link_address_port(struct
> drm_dp_mst_branch *mstb,
>  	struct drm_dp_mst_port *port;
>  	int old_ddps = 0, ret;
>  	u8 new_pdt = DP_PEER_DEVICE_NONE;
> +	bool new_mcs = 0;
>  	bool created = false, send_link_addr = false, changed = false;
>  
>  	port = drm_dp_get_port(mstb, port_msg->port_number);
> @@ -2267,7 +2269,7 @@ drm_dp_mst_handle_link_address_port(struct
> drm_dp_mst_branch *mstb,
>  	port->input = port_msg->input_port;
>  	if (!port->input)
>  		new_pdt = port_msg->peer_device_type;
> -	port->mcs = port_msg->mcs;
> +	new_mcs = port_msg->mcs;
>  	port->ddps = port_msg->ddps;
>  	port->ldps = port_msg->legacy_device_plug_status;
>  	port->dpcd_rev = port_msg->dpcd_revision;
> @@ -2295,7 +2297,7 @@ drm_dp_mst_handle_link_address_port(struct
> drm_dp_mst_branch *mstb,
>  		}
>  	}
>  
> -	ret = drm_dp_port_set_pdt(port, new_pdt);
> +	ret = drm_dp_port_set_pdt(port, new_pdt, new_mcs);
>  	if (ret == 1) {
>  		send_link_addr = true;
>  	} else if (ret < 0) {
> @@ -2309,7 +2311,8 @@ drm_dp_mst_handle_link_address_port(struct
> drm_dp_mst_branch *mstb,
>  	 * we're coming out of suspend. In this case, always resend the link
>  	 * address if there's an MSTB on this port
>  	 */
> -	if (!created && port->pdt == DP_PEER_DEVICE_MST_BRANCHING)
> +	if (!created && port->pdt == DP_PEER_DEVICE_MST_BRANCHING &&
> +	    port->mcs)
>  		send_link_addr = true;
>  
>  	if (port->connector)
> @@ -2346,6 +2349,7 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch
> *mstb,
>  	struct drm_dp_mst_port *port;
>  	int old_ddps, old_input, ret, i;
>  	u8 new_pdt;
> +	bool new_mcs;
>  	bool dowork = false, create_connector = false;
>  
>  	port = drm_dp_get_port(mstb, conn_stat->port_number);
> @@ -2377,7 +2381,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->mcs = conn_stat->message_capability_status;
>  	port->ldps = conn_stat->legacy_device_plug_status;
>  	port->ddps = conn_stat->displayport_device_plug_status;
>  
> @@ -2390,8 +2393,8 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch
> *mstb,
>  	}
>  
>  	new_pdt = port->input ? DP_PEER_DEVICE_NONE : conn_stat-
> >peer_device_type;
> -
> -	ret = drm_dp_port_set_pdt(port, new_pdt);
> +	new_mcs = conn_stat->message_capability_status;
> +	ret = drm_dp_port_set_pdt(port, new_pdt, new_mcs);
>  	if (ret == 1) {
>  		dowork = true;
>  	} else if (ret < 0) {
> @@ -3958,6 +3961,8 @@ drm_dp_mst_detect_port(struct drm_connector
> *connector,
>  	switch (port->pdt) {
>  	case DP_PEER_DEVICE_NONE:
>  	case DP_PEER_DEVICE_MST_BRANCHING:
> +		if (!port->mcs)
> +			ret = connector_status_connected;
>  		break;
>  
>  	case DP_PEER_DEVICE_SST_SINK:
> @@ -4597,7 +4602,7 @@ drm_dp_delayed_destroy_port(struct drm_dp_mst_port
> *port)
>  	if (port->connector)
>  		port->mgr->cbs->destroy_connector(port->mgr, port->connector);
>  
> -	drm_dp_port_set_pdt(port, DP_PEER_DEVICE_NONE);
> +	drm_dp_port_set_pdt(port, DP_PEER_DEVICE_NONE, port->mcs);
>  	drm_dp_mst_put_port_malloc(port);
>  }
>  
-- 
Cheers,
	Lyude Paul

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [PATCH 2/2] drm/dp_mst: Handle SST-only branch device case

[Lin, Wayne]

[AMD Public Use]



> -----Original Message-----
> From: Lyude Paul <lyude@redhat.com>
> Sent: Wednesday, January 15, 2020 5:19 AM
> To: Lin, Wayne <Wayne.Lin@amd.com>; dri-devel@lists.freedesktop.org;
> amd-gfx@lists.freedesktop.org
> Cc: Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Wentland, Harry
> <Harry.Wentland@amd.com>; Zuo, Jerry <Jerry.Zuo@amd.com>; Ville Syrjälä
> <ville.syrjala@linux.intel.com>; Wentland, Harry <Harry.Wentland@amd.com>
> Subject: Re: [PATCH 2/2] drm/dp_mst: Handle SST-only branch device case
> 
> On Wed, 2020-01-08 at 16:44 +0800, Wayne Lin wrote:
> > [Why]
> > While handling LINK_ADDRESS reply, current code expects a peer device
> > can handle sideband message once the peer device type is reported as
> > DP_PEER_DEVICE_MST_BRANCHING. However, when the connected device
> is a
> > SST branch case, it can't handle the sideband message(MST_CAP=0 in
> > DPCD 00021h).
> >
> > Current code will try to send LINK_ADDRESS to SST branch device and
> > end up with message timeout and monitor can't display normally. As the
> > result of that, we should take SST branch device into account.
> >
> > [How]
> > According to DP 1.4 spec, we can use Peer_Device_Type as
> > DP_PEER_DEVICE_MST_BRANCHING and Message_Capability_Status as 0 to
> > indicate peer device as a SST-only branch device.
> >
> > Fix following:
> > - Take SST-only branch device case into account in
> > drm_dp_port_set_pdt() and add a new parameter 'new_mcs'. Take sst
> > branch device case as the same case as
> DP_PEER_DEVICE_DP_LEGACY_CONV
> > and DP_PEER_DEVICE_SST_SINK. All original handling logics remain.
> > - Take SST-only branch device case into account in
> > drm_dp_mst_port_add_connector().
> > - Fix some parts in drm_dp_mst_handle_link_address_port() to have SST
> > branch device case into consideration.
> > - Fix the arguments of drm_dp_port_set_pdt() in
> > drm_dp_mst_handle_conn_stat().
> > - Have SST branch device also report
> > connector_status_connected when the ddps is true in
> > drm_dp_mst_detect_port()
> > - Fix the arguments of drm_dp_port_set_pdt() in
> > drm_dp_delayed_destroy_port()
> >
> > Fixes: c485e2c97dae ("drm/dp_mst: Refactor pdt setup/teardown, add
> > more
> > locking")
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Harry Wentland <hwentlan@amd.com>
> > Cc: Lyude Paul <lyude@redhat.com>
> > Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
> > ---
> >  drivers/gpu/drm/drm_dp_mst_topology.c | 131
> > +++++++++++++-------------
> >  1 file changed, 68 insertions(+), 63 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index 8f54b241db08..4395d5cc0645 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -1934,73 +1934,74 @@ static bool
> drm_dp_mst_is_dp_mst_end_device(u8
> > pdt, bool mcs)
> >  	return true;
> >  }
> >
> > -static int drm_dp_port_set_pdt(struct drm_dp_mst_port *port, u8
> > new_pdt)
> > +static int
> > +drm_dp_port_set_pdt(struct drm_dp_mst_port *port, u8 new_pdt,
> > +		    bool new_mcs)
> >  {
> >  	struct drm_dp_mst_topology_mgr *mgr = port->mgr;
> >  	struct drm_dp_mst_branch *mstb;
> >  	u8 rad[8], lct;
> >  	int ret = 0;
> >
> > -	if (port->pdt == new_pdt)
> > +	if (port->pdt == new_pdt && port->mcs == new_mcs)
> >  		return 0;
> >
> >  	/* Teardown the old pdt, if there is one */
> > -	switch (port->pdt) {
> > -	case DP_PEER_DEVICE_DP_LEGACY_CONV:
> > -	case DP_PEER_DEVICE_SST_SINK:
> > -		/*
> > -		 * If the new PDT would also have an i2c bus, don't bother
> > -		 * with reregistering it
> > -		 */
> > -		if (new_pdt == DP_PEER_DEVICE_DP_LEGACY_CONV ||
> > -		    new_pdt == DP_PEER_DEVICE_SST_SINK) {
> > -			port->pdt = new_pdt;
> > -			return 0;
> > -		}
> > +	if (port->pdt != DP_PEER_DEVICE_NONE) {
> > +		if (drm_dp_mst_is_dp_mst_end_device(port->pdt, port->mcs)) {
> > +			/*
> > +			 * If the new PDT would also have an i2c bus,
> > +			 * don't bother with reregistering it
> > +			 */
> > +			if (new_pdt != DP_PEER_DEVICE_NONE &&
> > +			    drm_dp_mst_is_dp_mst_end_device(new_pdt, new_mcs))
> > {
> > +				port->pdt = new_pdt;
> > +				port->mcs = new_mcs;
> > +				return 0;
> > +			}
> >
> > -		/* remove i2c over sideband */
> > -		drm_dp_mst_unregister_i2c_bus(&port->aux);
> > -		break;
> > -	case DP_PEER_DEVICE_MST_BRANCHING:
> > -		mutex_lock(&mgr->lock);
> > -		drm_dp_mst_topology_put_mstb(port->mstb);
> > -		port->mstb = NULL;
> > -		mutex_unlock(&mgr->lock);
> > -		break;
> > +			/* remove i2c over sideband */
> > +			drm_dp_mst_unregister_i2c_bus(&port->aux);
> > +		} else {
> > +			mutex_lock(&mgr->lock);
> > +			drm_dp_mst_topology_put_mstb(port->mstb);
> > +			port->mstb = NULL;
> > +			mutex_unlock(&mgr->lock);
> > +		}
> >  	}
> >
> >  	port->pdt = new_pdt;
> > -	switch (port->pdt) {
> > -	case DP_PEER_DEVICE_DP_LEGACY_CONV:
> > -	case DP_PEER_DEVICE_SST_SINK:
> > -		/* add i2c over sideband */
> > -		ret = drm_dp_mst_register_i2c_bus(&port->aux);
> > -		break;
> > +	port->mcs = new_mcs;
> >
> > -	case DP_PEER_DEVICE_MST_BRANCHING:
> > -		lct = drm_dp_calculate_rad(port, rad);
> > -		mstb = drm_dp_add_mst_branch_device(lct, rad);
> > -		if (!mstb) {
> > -			ret = -ENOMEM;
> > -			DRM_ERROR("Failed to create MSTB for port %p", port);
> > -			goto out;
> > -		}
> > +	if (port->pdt != DP_PEER_DEVICE_NONE) {
> > +		if (drm_dp_mst_is_dp_mst_end_device(port->pdt, port->mcs)) {
> > +			/* add i2c over sideband */
> > +			ret = drm_dp_mst_register_i2c_bus(&port->aux);
> > +		} else {
> > +			lct = drm_dp_calculate_rad(port, rad);
> > +			mstb = drm_dp_add_mst_branch_device(lct, rad);
> > +			if (!mstb) {
> > +				ret = -ENOMEM;
> > +				DRM_ERROR("Failed to create MSTB for port %p",
> > +					  port);
> > +				goto out;
> > +			}
> >
> > -		mutex_lock(&mgr->lock);
> > -		port->mstb = mstb;
> > -		mstb->mgr = port->mgr;
> > -		mstb->port_parent = port;
> > +			mutex_lock(&mgr->lock);
> > +			port->mstb = mstb;
> > +			mstb->mgr = port->mgr;
> > +			mstb->port_parent = port;
> >
> > -		/*
> > -		 * Make sure this port's memory allocation stays
> > -		 * around until its child MSTB releases it
> > -		 */
> > -		drm_dp_mst_get_port_malloc(port);
> > -		mutex_unlock(&mgr->lock);
> > +			/*
> > +			 * Make sure this port's memory allocation stays
> > +			 * around until its child MSTB releases it
> > +			 */
> > +			drm_dp_mst_get_port_malloc(port);
> > +			mutex_unlock(&mgr->lock);
> >
> > -		/* And make sure we send a link address for this */
> > -		ret = 1;
> > -		break;
> > +			/* And make sure we send a link address for this */
> > +			ret = 1;
> > +		}
> >  	}
> >
> >  out:
> > @@ -2153,12 +2154,12 @@ drm_dp_mst_port_add_connector(struct
> > drm_dp_mst_branch *mstb,
> >  		goto error;
> >  	}
> >
> > -	if ((port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV ||
> > -	     port->pdt == DP_PEER_DEVICE_SST_SINK) &&
> > -	    port->port_num >= DP_MST_LOGICAL_PORT_0) {
> > -		port->cached_edid = drm_get_edid(port->connector,
> > -						 &port->aux.ddc);
> > -		drm_connector_set_tile_property(port->connector);
> > +	if (port->pdt != DP_PEER_DEVICE_NONE) {
> > +		if (drm_dp_mst_is_dp_mst_end_device(port->pdt, port->mcs)) {
> > +			port->cached_edid = drm_get_edid(port->connector,
> > +							 &port->aux.ddc);
> > +			drm_connector_set_tile_property(port->connector);
> > +		}
> 
> I'd combine these two if statements here into one, otherwise this looks great.
> Thank you for all of the great fixes recently :)
> 
> Reviewed-by: Lyude Paul <lyude@redhat.com>
Thanks for your time!
I will fix it in the new version.
Thanks.
> 
> 
> >  	}
> >
> >  	mgr->cbs->register_connector(port->connector);
> > @@ -2223,6 +2224,7 @@ drm_dp_mst_handle_link_address_port(struct
> > drm_dp_mst_branch *mstb,
> >  	struct drm_dp_mst_port *port;
> >  	int old_ddps = 0, ret;
> >  	u8 new_pdt = DP_PEER_DEVICE_NONE;
> > +	bool new_mcs = 0;
> >  	bool created = false, send_link_addr = false, changed = false;
> >
> >  	port = drm_dp_get_port(mstb, port_msg->port_number); @@ -2267,7
> > +2269,7 @@ drm_dp_mst_handle_link_address_port(struct
> > drm_dp_mst_branch *mstb,
> >  	port->input = port_msg->input_port;
> >  	if (!port->input)
> >  		new_pdt = port_msg->peer_device_type;
> > -	port->mcs = port_msg->mcs;
> > +	new_mcs = port_msg->mcs;
> >  	port->ddps = port_msg->ddps;
> >  	port->ldps = port_msg->legacy_device_plug_status;
> >  	port->dpcd_rev = port_msg->dpcd_revision; @@ -2295,7 +2297,7 @@
> > drm_dp_mst_handle_link_address_port(struct
> > drm_dp_mst_branch *mstb,
> >  		}
> >  	}
> >
> > -	ret = drm_dp_port_set_pdt(port, new_pdt);
> > +	ret = drm_dp_port_set_pdt(port, new_pdt, new_mcs);
> >  	if (ret == 1) {
> >  		send_link_addr = true;
> >  	} else if (ret < 0) {
> > @@ -2309,7 +2311,8 @@ drm_dp_mst_handle_link_address_port(struct
> > drm_dp_mst_branch *mstb,
> >  	 * we're coming out of suspend. In this case, always resend the link
> >  	 * address if there's an MSTB on this port
> >  	 */
> > -	if (!created && port->pdt == DP_PEER_DEVICE_MST_BRANCHING)
> > +	if (!created && port->pdt == DP_PEER_DEVICE_MST_BRANCHING &&
> > +	    port->mcs)
> >  		send_link_addr = true;
> >
> >  	if (port->connector)
> > @@ -2346,6 +2349,7 @@ drm_dp_mst_handle_conn_stat(struct
> > drm_dp_mst_branch *mstb,
> >  	struct drm_dp_mst_port *port;
> >  	int old_ddps, old_input, ret, i;
> >  	u8 new_pdt;
> > +	bool new_mcs;
> >  	bool dowork = false, create_connector = false;
> >
> >  	port = drm_dp_get_port(mstb, conn_stat->port_number); @@ -2377,7
> > +2381,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->mcs = conn_stat->message_capability_status;
> >  	port->ldps = conn_stat->legacy_device_plug_status;
> >  	port->ddps = conn_stat->displayport_device_plug_status;
> >
> > @@ -2390,8 +2393,8 @@ drm_dp_mst_handle_conn_stat(struct
> > drm_dp_mst_branch *mstb,
> >  	}
> >
> >  	new_pdt = port->input ? DP_PEER_DEVICE_NONE : conn_stat-
> > >peer_device_type;
> > -
> > -	ret = drm_dp_port_set_pdt(port, new_pdt);
> > +	new_mcs = conn_stat->message_capability_status;
> > +	ret = drm_dp_port_set_pdt(port, new_pdt, new_mcs);
> >  	if (ret == 1) {
> >  		dowork = true;
> >  	} else if (ret < 0) {
> > @@ -3958,6 +3961,8 @@ drm_dp_mst_detect_port(struct drm_connector
> > *connector,
> >  	switch (port->pdt) {
> >  	case DP_PEER_DEVICE_NONE:
> >  	case DP_PEER_DEVICE_MST_BRANCHING:
> > +		if (!port->mcs)
> > +			ret = connector_status_connected;
> >  		break;
> >
> >  	case DP_PEER_DEVICE_SST_SINK:
> > @@ -4597,7 +4602,7 @@ drm_dp_delayed_destroy_port(struct
> > drm_dp_mst_port
> > *port)
> >  	if (port->connector)
> >  		port->mgr->cbs->destroy_connector(port->mgr, port->connector);
> >
> > -	drm_dp_port_set_pdt(port, DP_PEER_DEVICE_NONE);
> > +	drm_dp_port_set_pdt(port, DP_PEER_DEVICE_NONE, port->mcs);
> >  	drm_dp_mst_put_port_malloc(port);
> >  }
> >
> --
> Cheers,
> 	Lyude Paul
--
Best regards,
Wayne Lin
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel