[PATCH v2] drm/dp_mst: Clear all payload id tables downstream when initializing

[PATCH v2] drm/dp_mst: Clear all payload id tables downstream when initializing

[Lyude Paul]

From: Sean Paul <seanpaul@chromium.org>

It seems that on certain MST hubs, namely the CableMatters USB-C 2x DP
hub, using the DP_PAYLOAD_ALLOCATE_SET and DP_PAYLOAD_TABLE_UPDATE_STATUS
register ranges to clear any pre-existing payload allocations on the hub isn't
always enough to reset things if the source device has been reset unexpectedly.

Or at least, that's the current running theory. The precise behavior appears to
be that when the source device gets reset unexpectedly, the hub begins reporting
an available_pbn value of 0 for all of its ports. This is a bit inconsistent
with the our theory, since this seems to happen even if previously set PBN
allocations should have resulted in a non-zero available_pbn value. So, it's
possible that something else may be going on here.

Strangely though, sending a CLEAR_PAYLOAD_ID_TABLE broadcast request when
initializing the MST topology seems to bring things into working order and make
available_pbn work again. Since this is a pretty safe solution, let's go ahead
and implement it.

Changes since v1:
* Change indenting on drm_dp_send_clear_payload_id_table() prototype
* Remove some braces in drm_dp_send_clear_payload_id_table()
* Reorganize some variable declarations in drm_dp_send_clear_payload_id_table()
* Don't forget to handle DP_CLEAR_PAYLOAD_ID_TABLE in
  drm_dp_sideband_parse_reply()
* Move drm_dp_send_clear_payload_id_table() call into
  drm_dp_mst_link_probe_work(), since we can't send sideband messages
  while under lock in drm_dp_mst_topology_mgr_set_mst()
* Change commit message

Change-Id: I2c763e8dae3844eca76033a41f264080052fbbfc
Signed-off-by: Sean Paul <seanpaul@chromium.org>
Signed-off-by: Lyude Paul <lyude@redhat.com>
---

A heads up to anyone looking at this patch: it's quite possible this
won't be the final solution that we go with. Me and Sean would like to
do a bit more investigation to try to figure out what exactly is
happening here before we go ahead and push it, and hopefully figure out
why available_pbn is being set to 0 instead of some other leftover
non-zero allocation.

 drivers/gpu/drm/drm_dp_mst_topology.c | 63 +++++++++++++++++++++++++--
 include/drm/drm_dp_mst_helper.h       | 16 +++++--
 2 files changed, 72 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 82add736e17d..969e43b7eb4c 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -64,6 +64,11 @@ static int drm_dp_send_dpcd_write(struct drm_dp_mst_topology_mgr *mgr,
 
 static void drm_dp_send_link_address(struct drm_dp_mst_topology_mgr *mgr,
 				     struct drm_dp_mst_branch *mstb);
+
+static void
+drm_dp_send_clear_payload_id_table(struct drm_dp_mst_topology_mgr *mgr,
+				   struct drm_dp_mst_branch *mstb);
+
 static int drm_dp_send_enum_path_resources(struct drm_dp_mst_topology_mgr *mgr,
 					   struct drm_dp_mst_branch *mstb,
 					   struct drm_dp_mst_port *port);
@@ -657,6 +662,8 @@ static bool drm_dp_sideband_parse_reply(struct drm_dp_sideband_msg_rx *raw,
 	case DP_POWER_DOWN_PHY:
 	case DP_POWER_UP_PHY:
 		return drm_dp_sideband_parse_power_updown_phy_ack(raw, msg);
+	case DP_CLEAR_PAYLOAD_ID_TABLE:
+		return true; /* since there's nothing to parse */
 	default:
 		DRM_ERROR("Got unknown reply 0x%02x (%s)\n", msg->req_type,
 			  drm_dp_mst_req_type_str(msg->req_type));
@@ -755,6 +762,15 @@ static int build_link_address(struct drm_dp_sideband_msg_tx *msg)
 	return 0;
 }
 
+static int build_clear_payload_id_table(struct drm_dp_sideband_msg_tx *msg)
+{
+	struct drm_dp_sideband_msg_req_body req;
+
+	req.req_type = DP_CLEAR_PAYLOAD_ID_TABLE;
+	drm_dp_encode_sideband_req(&req, msg);
+	return 0;
+}
+
 static int build_enum_path_resources(struct drm_dp_sideband_msg_tx *msg, int port_num)
 {
 	struct drm_dp_sideband_msg_req_body req;
@@ -1877,8 +1893,12 @@ static void drm_dp_mst_link_probe_work(struct work_struct *work)
 	struct drm_dp_mst_topology_mgr *mgr = container_of(work, struct drm_dp_mst_topology_mgr, work);
 	struct drm_dp_mst_branch *mstb;
 	int ret;
+	bool clear_payload_id_table;
 
 	mutex_lock(&mgr->lock);
+	clear_payload_id_table = !mgr->payload_id_table_cleared;
+	mgr->payload_id_table_cleared = true;
+
 	mstb = mgr->mst_primary;
 	if (mstb) {
 		ret = drm_dp_mst_topology_try_get_mstb(mstb);
@@ -1886,10 +1906,24 @@ static void drm_dp_mst_link_probe_work(struct work_struct *work)
 			mstb = NULL;
 	}
 	mutex_unlock(&mgr->lock);
-	if (mstb) {
-		drm_dp_check_and_send_link_address(mgr, mstb);
-		drm_dp_mst_topology_put_mstb(mstb);
+	if (!mstb)
+		return;
+
+	/*
+	 * Certain branch devices seem to incorrectly report an available_pbn
+	 * of 0 on downstream sinks, even after clearing the
+	 * DP_PAYLOAD_ALLOCATE_* registers in
+	 * drm_dp_mst_topology_mgr_set_mst(). Namely, the CableMatters USB-C
+	 * 2x DP hub. Sending a CLEAR_PAYLOAD_ID_TABLE message seems to make
+	 * things work again.
+	 */
+	if (clear_payload_id_table) {
+		DRM_DEBUG_KMS("Clearing payload ID table\n");
+		drm_dp_send_clear_payload_id_table(mgr, mstb);
 	}
+
+	drm_dp_check_and_send_link_address(mgr, mstb);
+	drm_dp_mst_topology_put_mstb(mstb);
 }
 
 static bool drm_dp_validate_guid(struct drm_dp_mst_topology_mgr *mgr,
@@ -2156,6 +2190,28 @@ static void drm_dp_send_link_address(struct drm_dp_mst_topology_mgr *mgr,
 	kfree(txmsg);
 }
 
+void drm_dp_send_clear_payload_id_table(struct drm_dp_mst_topology_mgr *mgr,
+					struct drm_dp_mst_branch *mstb)
+{
+	struct drm_dp_sideband_msg_tx *txmsg;
+	int len, ret;
+
+	txmsg = kzalloc(sizeof(*txmsg), GFP_KERNEL);
+	if (!txmsg)
+		return;
+
+	txmsg->dst = mstb;
+	len = build_clear_payload_id_table(txmsg);
+
+	drm_dp_queue_down_tx(mgr, txmsg);
+
+	ret = drm_dp_mst_wait_tx_reply(mstb, txmsg);
+	if (ret > 0 && txmsg->reply.reply_type == DP_SIDEBAND_REPLY_NAK)
+		DRM_DEBUG_KMS("clear payload table id nak received\n");
+
+	kfree(txmsg);
+}
+
 static int drm_dp_send_enum_path_resources(struct drm_dp_mst_topology_mgr *mgr,
 					   struct drm_dp_mst_branch *mstb,
 					   struct drm_dp_mst_port *port)
@@ -2756,6 +2812,7 @@ int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool ms
 		mgr->payload_mask = 0;
 		set_bit(0, &mgr->payload_mask);
 		mgr->vcpi_mask = 0;
+		mgr->payload_id_table_cleared = false;
 	}
 
 out_unlock:
diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
index 2ba6253ea6d3..ee4093c1bba3 100644
--- a/include/drm/drm_dp_mst_helper.h
+++ b/include/drm/drm_dp_mst_helper.h
@@ -494,15 +494,23 @@ struct drm_dp_mst_topology_mgr {
 	struct drm_dp_sideband_msg_rx up_req_recv;
 
 	/**
-	 * @lock: protects mst state, primary, dpcd.
+	 * @lock: protects @mst_state, @mst_primary, @dpcd, and
+	 * @payload_id_table_cleared.
 	 */
 	struct mutex lock;
 
 	/**
-	 * @mst_state: If this manager is enabled for an MST capable port. False
-	 * if no MST sink/branch devices is connected.
+	 * @mst_state: If this manager is enabled for an MST capable port.
+	 * False if no MST sink/branch devices is connected.
 	 */
-	bool mst_state;
+	bool mst_state : 1;
+
+	/**
+	 * @payload_id_table_cleared: Whether or not we've cleared the payload
+	 * ID table for @mst_primary. Protected by @lock.
+	 */
+	bool payload_id_table_cleared : 1;
+
 	/**
 	 * @mst_primary: Pointer to the primary/first branch device.
 	 */
-- 
2.21.0

Re: [PATCH v2] drm/dp_mst: Clear all payload id tables downstream when initializing

[Sean Paul]

On Wed, Aug 28, 2019 at 08:09:44PM -0400, Lyude Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
> 
> It seems that on certain MST hubs, namely the CableMatters USB-C 2x DP
> hub, using the DP_PAYLOAD_ALLOCATE_SET and DP_PAYLOAD_TABLE_UPDATE_STATUS
> register ranges to clear any pre-existing payload allocations on the hub isn't
> always enough to reset things if the source device has been reset unexpectedly.
> 
> Or at least, that's the current running theory. The precise behavior appears to
> be that when the source device gets reset unexpectedly, the hub begins reporting
> an available_pbn value of 0 for all of its ports. This is a bit inconsistent
> with the our theory, since this seems to happen even if previously set PBN
> allocations should have resulted in a non-zero available_pbn value. So, it's
> possible that something else may be going on here.
> 
> Strangely though, sending a CLEAR_PAYLOAD_ID_TABLE broadcast request when
> initializing the MST topology seems to bring things into working order and make
> available_pbn work again. Since this is a pretty safe solution, let's go ahead
> and implement it.
> 
> Changes since v1:
> * Change indenting on drm_dp_send_clear_payload_id_table() prototype
> * Remove some braces in drm_dp_send_clear_payload_id_table()
> * Reorganize some variable declarations in drm_dp_send_clear_payload_id_table()
> * Don't forget to handle DP_CLEAR_PAYLOAD_ID_TABLE in
>   drm_dp_sideband_parse_reply()
> * Move drm_dp_send_clear_payload_id_table() call into
>   drm_dp_mst_link_probe_work(), since we can't send sideband messages
>   while under lock in drm_dp_mst_topology_mgr_set_mst()
> * Change commit message
> 
> Change-Id: I2c763e8dae3844eca76033a41f264080052fbbfc
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> Signed-off-by: Lyude Paul <lyude@redhat.com>

Pushed to drm-misc-next without that nasty Change-Id and with danvet's IRC
Acked-by to appease almighty dim.

Thanks, Lyude, for polishing this up :-)

Sean

> ---
> 
> A heads up to anyone looking at this patch: it's quite possible this
> won't be the final solution that we go with. Me and Sean would like to
> do a bit more investigation to try to figure out what exactly is
> happening here before we go ahead and push it, and hopefully figure out
> why available_pbn is being set to 0 instead of some other leftover
> non-zero allocation.
> 
>  drivers/gpu/drm/drm_dp_mst_topology.c | 63 +++++++++++++++++++++++++--
>  include/drm/drm_dp_mst_helper.h       | 16 +++++--
>  2 files changed, 72 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 82add736e17d..969e43b7eb4c 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -64,6 +64,11 @@ static int drm_dp_send_dpcd_write(struct drm_dp_mst_topology_mgr *mgr,
>  
>  static void drm_dp_send_link_address(struct drm_dp_mst_topology_mgr *mgr,
>  				     struct drm_dp_mst_branch *mstb);
> +
> +static void
> +drm_dp_send_clear_payload_id_table(struct drm_dp_mst_topology_mgr *mgr,
> +				   struct drm_dp_mst_branch *mstb);
> +
>  static int drm_dp_send_enum_path_resources(struct drm_dp_mst_topology_mgr *mgr,
>  					   struct drm_dp_mst_branch *mstb,
>  					   struct drm_dp_mst_port *port);
> @@ -657,6 +662,8 @@ static bool drm_dp_sideband_parse_reply(struct drm_dp_sideband_msg_rx *raw,
>  	case DP_POWER_DOWN_PHY:
>  	case DP_POWER_UP_PHY:
>  		return drm_dp_sideband_parse_power_updown_phy_ack(raw, msg);
> +	case DP_CLEAR_PAYLOAD_ID_TABLE:
> +		return true; /* since there's nothing to parse */
>  	default:
>  		DRM_ERROR("Got unknown reply 0x%02x (%s)\n", msg->req_type,
>  			  drm_dp_mst_req_type_str(msg->req_type));
> @@ -755,6 +762,15 @@ static int build_link_address(struct drm_dp_sideband_msg_tx *msg)
>  	return 0;
>  }
>  
> +static int build_clear_payload_id_table(struct drm_dp_sideband_msg_tx *msg)
> +{
> +	struct drm_dp_sideband_msg_req_body req;
> +
> +	req.req_type = DP_CLEAR_PAYLOAD_ID_TABLE;
> +	drm_dp_encode_sideband_req(&req, msg);
> +	return 0;
> +}
> +
>  static int build_enum_path_resources(struct drm_dp_sideband_msg_tx *msg, int port_num)
>  {
>  	struct drm_dp_sideband_msg_req_body req;
> @@ -1877,8 +1893,12 @@ static void drm_dp_mst_link_probe_work(struct work_struct *work)
>  	struct drm_dp_mst_topology_mgr *mgr = container_of(work, struct drm_dp_mst_topology_mgr, work);
>  	struct drm_dp_mst_branch *mstb;
>  	int ret;
> +	bool clear_payload_id_table;
>  
>  	mutex_lock(&mgr->lock);
> +	clear_payload_id_table = !mgr->payload_id_table_cleared;
> +	mgr->payload_id_table_cleared = true;
> +
>  	mstb = mgr->mst_primary;
>  	if (mstb) {
>  		ret = drm_dp_mst_topology_try_get_mstb(mstb);
> @@ -1886,10 +1906,24 @@ static void drm_dp_mst_link_probe_work(struct work_struct *work)
>  			mstb = NULL;
>  	}
>  	mutex_unlock(&mgr->lock);
> -	if (mstb) {
> -		drm_dp_check_and_send_link_address(mgr, mstb);
> -		drm_dp_mst_topology_put_mstb(mstb);
> +	if (!mstb)
> +		return;
> +
> +	/*
> +	 * Certain branch devices seem to incorrectly report an available_pbn
> +	 * of 0 on downstream sinks, even after clearing the
> +	 * DP_PAYLOAD_ALLOCATE_* registers in
> +	 * drm_dp_mst_topology_mgr_set_mst(). Namely, the CableMatters USB-C
> +	 * 2x DP hub. Sending a CLEAR_PAYLOAD_ID_TABLE message seems to make
> +	 * things work again.
> +	 */
> +	if (clear_payload_id_table) {
> +		DRM_DEBUG_KMS("Clearing payload ID table\n");
> +		drm_dp_send_clear_payload_id_table(mgr, mstb);
>  	}
> +
> +	drm_dp_check_and_send_link_address(mgr, mstb);
> +	drm_dp_mst_topology_put_mstb(mstb);
>  }
>  
>  static bool drm_dp_validate_guid(struct drm_dp_mst_topology_mgr *mgr,
> @@ -2156,6 +2190,28 @@ static void drm_dp_send_link_address(struct drm_dp_mst_topology_mgr *mgr,
>  	kfree(txmsg);
>  }
>  
> +void drm_dp_send_clear_payload_id_table(struct drm_dp_mst_topology_mgr *mgr,
> +					struct drm_dp_mst_branch *mstb)
> +{
> +	struct drm_dp_sideband_msg_tx *txmsg;
> +	int len, ret;
> +
> +	txmsg = kzalloc(sizeof(*txmsg), GFP_KERNEL);
> +	if (!txmsg)
> +		return;
> +
> +	txmsg->dst = mstb;
> +	len = build_clear_payload_id_table(txmsg);
> +
> +	drm_dp_queue_down_tx(mgr, txmsg);
> +
> +	ret = drm_dp_mst_wait_tx_reply(mstb, txmsg);
> +	if (ret > 0 && txmsg->reply.reply_type == DP_SIDEBAND_REPLY_NAK)
> +		DRM_DEBUG_KMS("clear payload table id nak received\n");
> +
> +	kfree(txmsg);
> +}
> +
>  static int drm_dp_send_enum_path_resources(struct drm_dp_mst_topology_mgr *mgr,
>  					   struct drm_dp_mst_branch *mstb,
>  					   struct drm_dp_mst_port *port)
> @@ -2756,6 +2812,7 @@ int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool ms
>  		mgr->payload_mask = 0;
>  		set_bit(0, &mgr->payload_mask);
>  		mgr->vcpi_mask = 0;
> +		mgr->payload_id_table_cleared = false;
>  	}
>  
>  out_unlock:
> diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
> index 2ba6253ea6d3..ee4093c1bba3 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -494,15 +494,23 @@ struct drm_dp_mst_topology_mgr {
>  	struct drm_dp_sideband_msg_rx up_req_recv;
>  
>  	/**
> -	 * @lock: protects mst state, primary, dpcd.
> +	 * @lock: protects @mst_state, @mst_primary, @dpcd, and
> +	 * @payload_id_table_cleared.
>  	 */
>  	struct mutex lock;
>  
>  	/**
> -	 * @mst_state: If this manager is enabled for an MST capable port. False
> -	 * if no MST sink/branch devices is connected.
> +	 * @mst_state: If this manager is enabled for an MST capable port.
> +	 * False if no MST sink/branch devices is connected.
>  	 */
> -	bool mst_state;
> +	bool mst_state : 1;
> +
> +	/**
> +	 * @payload_id_table_cleared: Whether or not we've cleared the payload
> +	 * ID table for @mst_primary. Protected by @lock.
> +	 */
> +	bool payload_id_table_cleared : 1;
> +
>  	/**
>  	 * @mst_primary: Pointer to the primary/first branch device.
>  	 */
> -- 
> 2.21.0
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS