drm/dp_mst: Clear MSG_RDY flag before sending new message

PATCH Superseded Wayne Lin 2 participants 5 messages Apr 27, 2023 – May 22, 2023

Subsystems (8)

Stable branch DRM drivers DRM misc Intel i915 +4 more subsystems

lore

Share & tools

lore

Wayne Lin

author

2023-04-27 07:29 UTC #0 lore raw

PATCH v2 1/1 drm/dp_mst: Clear MSG_RDY flag before sending new message

[Why]
The sequence for collecting down_reply from source perspective should be:

Request_n->repeat (get partial reply of Request_n->clear message ready flag to ack DPRX that the message is received) till all partial replies for Request_n are received->new Request_n+1.

Now there is chance that drm_dp_mst_hpd_irq() will fire new down request in the tx queue when the down reply is incomplete. Source is restricted to generate interveleaved message transactions so we should avoid it.

Also, while assembling partial reply packets, reading out DPCD DOWN_REP Sideband MSG buffer + clearing DOWN_REP_MSG_RDY flag should be wrapped up as a complete operation for reading out a reply packet. Kicking off a new request before clearing DOWN_REP_MSG_RDY flag might be risky. e.g. If the reply of the new request has overwritten the DPRX DOWN_REP Sideband MSG buffer before source writing one to clear DOWN_REP_MSG_RDY flag, source then unintentionally flushes the reply for the new request. Should handle the up request in the same way.

[How]
Separete drm_dp_mst_hpd_irq() into 2 steps. After acking the MST IRQ event, driver calls drm_dp_mst_hpd_irq_step2() and might trigger drm_dp_mst_kick_tx() only when there is no on going message transaction.

Changes since v1:
* Reworked on review comments received
-> Adjust the fix to let driver explicitly kick off new down request when mst irq event is handled and acked
-> Adjust the commit message

Signed-off-by Wayne Lin <[email protected]>

Cc [email protected]

---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  8 ++---
 drivers/gpu/drm/display/drm_dp_mst_topology.c | 35 ++++++++++++++++---
 drivers/gpu/drm/i915/display/intel_dp.c       |  5 ++-
 drivers/gpu/drm/nouveau/dispnv50/disp.c       |  5 ++-
 include/drm/display/drm_dp_mst_helper.h       |  4 +--
 5 files changed, 45 insertions(+), 12 deletions(-)

drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +4 -4

index 1ad67c2a697e..48bdcb2ee9b1 100644
@@ -3259,10 +3259,9 @@ static void dm_handle_mst_sideband_msg(struct amdgpu_dm_connector *aconnector)
 		DRM_DEBUG_DRIVER("ESI %02x %02x %02x\n", esi[0], esi[1], esi[2]);
 		/* handle HPD short pulse irq */
 		if (aconnector->mst_mgr.mst_state)
-			drm_dp_mst_hpd_irq(
-				&aconnector->mst_mgr,
-				esi,
-				&new_irq_handled);
+			drm_dp_mst_hpd_irq_step1(&aconnector->mst_mgr,
+						 esi,
+						 &new_irq_handled);
 
 		if (new_irq_handled) {
 			/* ACK at DPCD to notify down stream */

@@ -3281,6 +3280,7 @@ static void dm_handle_mst_sideband_msg(struct amdgpu_dm_connector *aconnector)
 					break;
 			}
 
+			drm_dp_mst_hpd_irq_step2(&aconnector->mst_mgr);
 			/* check if there is new irq to be handled */
 			dret = drm_dp_dpcd_read(
 				&aconnector->dm_dp_aux.aux,

drivers/gpu/drm/display/drm_dp_mst_topology.c +31 -4

index 70df29fe92db..2e0a38a6509c 100644
@@ -4045,7 +4045,7 @@ static int drm_dp_mst_handle_up_req(struct drm_dp_mst_topology_mgr *mgr)
 }
 
 /**
- * drm_dp_mst_hpd_irq() - MST hotplug IRQ notify
+ * drm_dp_mst_hpd_irq_step1() - MST hotplug IRQ notify
  * @mgr: manager to notify irq for.
  * @esi: 4 bytes from SINK_COUNT_ESI
  * @handled: whether the hpd interrupt was consumed or not

@@ -4055,7 +4055,7 @@ static int drm_dp_mst_handle_up_req(struct drm_dp_mst_topology_mgr *mgr)
  * topology manager will process the sideband messages received as a result
  * of this.
  */
-int drm_dp_mst_hpd_irq(struct drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handled)
+int drm_dp_mst_hpd_irq_step1(struct drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handled)
 {
 	int ret = 0;
 	int sc;

@@ -4077,11 +4077,38 @@ int drm_dp_mst_hpd_irq(struct drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl
 		*handled = true;
 	}
 
-	drm_dp_mst_kick_tx(mgr);
 	return ret;
 }
-EXPORT_SYMBOL(drm_dp_mst_hpd_irq);
+EXPORT_SYMBOL(drm_dp_mst_hpd_irq_step1);
+
+/**
+ * drm_dp_mst_hpd_irq_step2() - MST hotplug IRQ 2nd part handling
+ * @mgr: manager to notify irq for.
+ *
+ * This should be called from the driver when mst irq event is handled
+ * and acked. Note that new down request should only be sent when
+ * previous message transaction is done. Source is not supposed to generate
+ * interleaved message transactions.
+ */
+void drm_dp_mst_hpd_irq_step2(struct drm_dp_mst_topology_mgr *mgr)
+{
+	struct drm_dp_sideband_msg_tx *txmsg;
+	bool skip = false;
 
+	mutex_lock(&mgr->qlock);
+	txmsg = list_first_entry_or_null(&mgr->tx_msg_downq,
+					 struct drm_dp_sideband_msg_tx, next);
+	/* If last transaction is not completed yet*/
+	if (!txmsg ||
+	    txmsg->state == DRM_DP_SIDEBAND_TX_START_SEND ||
+	    txmsg->state == DRM_DP_SIDEBAND_TX_SENT)
+		skip = true;
+	mutex_unlock(&mgr->qlock);
+
+	if (!skip)
+		drm_dp_mst_kick_tx(mgr);
+}
+EXPORT_SYMBOL(drm_dp_mst_hpd_irq_step2);
 /**
  * drm_dp_mst_detect_port() - get connection status for an MST port
  * @connector: DRM connector for this port

drivers/gpu/drm/i915/display/intel_dp.c +4 -1

index 75070eb07d4b..9a9a5aec9534 100644
@@ -3803,7 +3803,7 @@ intel_dp_mst_hpd_irq(struct intel_dp *intel_dp, u8 *esi, u8 *ack)
 {
 	bool handled = false;
 
-	drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled);
+	drm_dp_mst_hpd_irq_step1(&intel_dp->mst_mgr, esi, &handled);
 	if (handled)
 		ack[1] |= esi[1] & (DP_DOWN_REP_MSG_RDY | DP_UP_REQ_MSG_RDY);

@@ -3880,6 +3880,9 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
 
 		if (!intel_dp_ack_sink_irq_esi(intel_dp, ack))
 			drm_dbg_kms(&i915->drm, "Failed to ack ESI\n");
+
+		if (ack[1] & (DP_DOWN_REP_MSG_RDY | DP_UP_REQ_MSG_RDY))
+			drm_dp_mst_hpd_irq_step2(&intel_dp->mst_mgr);
 	}
 
 	return link_ok;

drivers/gpu/drm/nouveau/dispnv50/disp.c +4 -1

index ed9d374147b8..00c36fcc8afd 100644
@@ -1332,12 +1332,15 @@ nv50_mstm_service(struct nouveau_drm *drm,
 			break;
 		}
 
-		drm_dp_mst_hpd_irq(&mstm->mgr, esi, &handled);
+		drm_dp_mst_hpd_irq_step1(&mstm->mgr, esi, &handled);
 		if (!handled)
 			break;
 
 		rc = drm_dp_dpcd_write(aux, DP_SINK_COUNT_ESI + 1, &esi[1],
 				       3);
+
+		drm_dp_mst_hpd_irq_step2(&mstm->mgr);
+
 		if (rc != 3) {
 			ret = false;
 			break;

include/drm/display/drm_dp_mst_helper.h +2 -2

index 32c764fb9cb5..6c08ba765d5a 100644
@@ -815,8 +815,8 @@ void drm_dp_mst_topology_mgr_destroy(struct drm_dp_mst_topology_mgr *mgr);
 bool drm_dp_read_mst_cap(struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_CAP_SIZE]);
 int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool mst_state);
 
-int drm_dp_mst_hpd_irq(struct drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handled);
-
+int drm_dp_mst_hpd_irq_step1(struct drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handled);
+void drm_dp_mst_hpd_irq_step2(struct drm_dp_mst_topology_mgr *mgr);
 
 int
 drm_dp_mst_detect_port(struct drm_connector *connector,

Signature · 1 line

2.37.3

Lin, Wayne

author

2023-05-08 09:48 UTC #1 lore raw

[Public]

Hi Lyude and Jani,

Could you help to review please? Thanks for your time!

Regards,
Wayne Lin

Quoted 230 lines

-----Original Message-----
From: Wayne Lin <[email protected]>
Sent: Thursday, April 27, 2023 3:29 PM
To: [email protected]; [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]; Wentland, Harry <[email protected]>; Zuo, Jerry <[email protected]>; Lin, Wayne <[email protected]>; [email protected]
Subject: [PATCH v2] drm/dp_mst: Clear MSG_RDY flag before sending new message
[Why]
The sequence for collecting down_reply from source perspective should be:
Request_n->repeat (get partial reply of Request_n->clear message ready flag to ack DPRX that the message is received) till all partial replies for Request_n are received->new Request_n+1.
Now there is chance that drm_dp_mst_hpd_irq() will fire new down request in the tx queue when the down reply is incomplete. Source is restricted to generate interveleaved message transactions so we should avoid it.
Also, while assembling partial reply packets, reading out DPCD DOWN_REP Sideband MSG buffer + clearing DOWN_REP_MSG_RDY flag should be wrapped up as a complete operation for reading out a reply packet. Kicking off a new request before clearing DOWN_REP_MSG_RDY flag might be risky. e.g. If the reply of the new request has overwritten the DPRX DOWN_REP Sideband MSG buffer before source writing one to clear DOWN_REP_MSG_RDY flag, source then unintentionally flushes the reply for the new request. Should handle the up request in the same way.
[How]
Separete drm_dp_mst_hpd_irq() into 2 steps. After acking the MST IRQ event, driver calls drm_dp_mst_hpd_irq_step2() and might trigger drm_dp_mst_kick_tx() only when there is no on going message transaction.
Changes since v1:
* Reworked on review comments received
-> Adjust the fix to let driver explicitly kick off new down request when mst irq event is handled and acked
-> Adjust the commit message
Signed-off-by Wayne Lin <[email protected]>
Cc [email protected]
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  8 ++---
drivers/gpu/drm/display/drm_dp_mst_topology.c | 35
++++++++++++++++---
 drivers/gpu/drm/i915/display/intel_dp.c       |  5 ++-
 drivers/gpu/drm/nouveau/dispnv50/disp.c       |  5 ++-
 include/drm/display/drm_dp_mst_helper.h       |  4 +--
 5 files changed, 45 insertions(+), 12 deletions(-)
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +3 -4
index 1ad67c2a697e..48bdcb2ee9b1 100644
@@ -3259,10 +3259,9 @@ static void dm_handle_mst_sideband_msg(struct
 amdgpu_dm_connector *aconnector)
 		DRM_DEBUG_DRIVER("ESI %02x %02x %02x\n", esi[0], esi[1],
 esi[2]);
 		/* handle HPD short pulse irq */
 		if (aconnector->mst_mgr.mst_state)
-			drm_dp_mst_hpd_irq(
-				&aconnector->mst_mgr,
-				esi,
-				&new_irq_handled);
+			drm_dp_mst_hpd_irq_step1(&aconnector-
 >mst_mgr,
+						 esi,
+						 &new_irq_handled);
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +1 -0
 		if (new_irq_handled) {
 			/* ACK at DPCD to notify down stream */ @@ -3281,6
+3280,7 @@ static void dm_handle_mst_sideband_msg(struct
amdgpu_dm_connector *aconnector)
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +1 -0
 					break;
 			}
 
+			drm_dp_mst_hpd_irq_step2(&aconnector-
Quoted 1 line
mst_mgr);
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +0 -0
 			/* check if there is new irq to be handled */
 			dret = drm_dp_dpcd_read(
 				&aconnector->dm_dp_aux.aux,
drivers/gpu/drm/display/drm_dp_mst_topology.c +2 -1
index 70df29fe92db..2e0a38a6509c 100644
@@ -4045,7 +4045,7 @@ static int drm_dp_mst_handle_up_req(struct
 drm_dp_mst_topology_mgr *mgr)  }
 
 /**
- * drm_dp_mst_hpd_irq() - MST hotplug IRQ notify
+ * drm_dp_mst_hpd_irq_step1() - MST hotplug IRQ notify
  * @mgr: manager to notify irq for.
  * @esi: 4 bytes from SINK_COUNT_ESI
  * @handled: whether the hpd interrupt was consumed or not @@ -4055,7
+4055,7 @@ static int drm_dp_mst_handle_up_req(struct
drm_dp_mst_topology_mgr *mgr)
* topology manager will process the sideband messages received as a result
* of this.
*/
-int drm_dp_mst_hpd_irq(struct drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handled)
+int drm_dp_mst_hpd_irq_step1(struct drm_dp_mst_topology_mgr *mgr, u8
drivers/gpu/drm/display/drm_dp_mst_topology.c +1 -0
+*esi, bool *handled)
 {
 	int ret = 0;
 	int sc;
@@ -4077,11 +4077,38 @@ int drm_dp_mst_hpd_irq(struct
drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl
drivers/gpu/drm/display/drm_dp_mst_topology.c +13 -2
 		*handled = true;
 	}
 
-	drm_dp_mst_kick_tx(mgr);
 	return ret;
 }
-EXPORT_SYMBOL(drm_dp_mst_hpd_irq);
+EXPORT_SYMBOL(drm_dp_mst_hpd_irq_step1);
+
+/**
+ * drm_dp_mst_hpd_irq_step2() - MST hotplug IRQ 2nd part handling
+ * @mgr: manager to notify irq for.
+ *
+ * This should be called from the driver when mst irq event is handled
+ * and acked. Note that new down request should only be sent when
+ * previous message transaction is done. Source is not supposed to
+generate
+ * interleaved message transactions.
+ */
+void drm_dp_mst_hpd_irq_step2(struct drm_dp_mst_topology_mgr *mgr)
{
drivers/gpu/drm/display/drm_dp_mst_topology.c +5 -0
+	struct drm_dp_sideband_msg_tx *txmsg;
+	bool skip = false;
 
+	mutex_lock(&mgr->qlock);
+	txmsg = list_first_entry_or_null(&mgr->tx_msg_downq,
+					 struct drm_dp_sideband_msg_tx,
next);
drivers/gpu/drm/display/drm_dp_mst_topology.c +11 -0
+	/* If last transaction is not completed yet*/
+	if (!txmsg ||
+	    txmsg->state == DRM_DP_SIDEBAND_TX_START_SEND ||
+	    txmsg->state == DRM_DP_SIDEBAND_TX_SENT)
+		skip = true;
+	mutex_unlock(&mgr->qlock);
+
+	if (!skip)
+		drm_dp_mst_kick_tx(mgr);
+}
+EXPORT_SYMBOL(drm_dp_mst_hpd_irq_step2);
 /**
  * drm_dp_mst_detect_port() - get connection status for an MST port
  * @connector: DRM connector for this port diff --git
a/drivers/gpu/drm/i915/display/intel_dp.c
b/drivers/gpu/drm/i915/display/intel_dp.c
index 75070eb07d4b..9a9a5aec9534 100644
drivers/gpu/drm/i915/display/intel_dp.c +4 -1
@@ -3803,7 +3803,7 @@ intel_dp_mst_hpd_irq(struct intel_dp *intel_dp,
 u8 *esi, u8 *ack)  {
 	bool handled = false;
 
-	drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled);
+	drm_dp_mst_hpd_irq_step1(&intel_dp->mst_mgr, esi, &handled);
 	if (handled)
 		ack[1] |= esi[1] & (DP_DOWN_REP_MSG_RDY |
 DP_UP_REQ_MSG_RDY);
@@ -3880,6 +3880,9 @@ intel_dp_check_mst_status(struct intel_dp
 *intel_dp)
 
 		if (!intel_dp_ack_sink_irq_esi(intel_dp, ack))
 			drm_dbg_kms(&i915->drm, "Failed to ack ESI\n");
+
+		if (ack[1] & (DP_DOWN_REP_MSG_RDY |
 DP_UP_REQ_MSG_RDY))
+			drm_dp_mst_hpd_irq_step2(&intel_dp->mst_mgr);
 	}
drivers/gpu/drm/i915/display/intel_dp.c +0 -0
 	return link_ok;
drivers/gpu/drm/nouveau/dispnv50/disp.c +4 -1
index ed9d374147b8..00c36fcc8afd 100644
@@ -1332,12 +1332,15 @@ nv50_mstm_service(struct nouveau_drm *drm,
 			break;
 		}
 
-		drm_dp_mst_hpd_irq(&mstm->mgr, esi, &handled);
+		drm_dp_mst_hpd_irq_step1(&mstm->mgr, esi, &handled);
 		if (!handled)
 			break;
 
 		rc = drm_dp_dpcd_write(aux, DP_SINK_COUNT_ESI + 1,
 &esi[1],
 				       3);
+
+		drm_dp_mst_hpd_irq_step2(&mstm->mgr);
+
 		if (rc != 3) {
 			ret = false;
drivers/gpu/drm/nouveau/dispnv50/disp.c +0 -0
 			break;
include/drm/display/drm_dp_mst_helper.h +0 -0
index 32c764fb9cb5..6c08ba765d5a 100644
@@ -815,8 +815,8 @@ void drm_dp_mst_topology_mgr_destroy(struct
drm_dp_mst_topology_mgr *mgr); bool drm_dp_read_mst_cap(struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_CAP_SIZE]); int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool mst_state);
-int drm_dp_mst_hpd_irq(struct drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handled);
include/drm/display/drm_dp_mst_helper.h +1 -1
-
+int drm_dp_mst_hpd_irq_step1(struct drm_dp_mst_topology_mgr *mgr,
u8
include/drm/display/drm_dp_mst_helper.h +2 -1
+*esi, bool *handled); void drm_dp_mst_hpd_irq_step2(struct
+drm_dp_mst_topology_mgr *mgr);
 
 int
 drm_dp_mst_detect_port(struct drm_connector *connector,
--
2.37.3

Lin, Wayne

author

2023-05-16 09:29 UTC #2 lore raw

[Public]

Hi,

Ping again for code review. Much appreciated!

Regards,
Wayne

Quoted 259 lines

-----Original Message-----
From: Lin, Wayne
Sent: Monday, May 8, 2023 5:49 PM
To: [email protected]; [email protected]; dri- [email protected]; [email protected]
Cc: [email protected]; [email protected]; Wentland, Harry <[email protected]>; Zuo, Jerry <[email protected]>; [email protected]
Subject: RE: [PATCH v2] drm/dp_mst: Clear MSG_RDY flag before sending new message
[Public]
Hi Lyude and Jani,
Could you help to review please? Thanks for your time!
Regards,
Wayne Lin
Quoted 7 lines
-----Original Message-----
From: Wayne Lin <[email protected]>
Sent: Thursday, April 27, 2023 3:29 PM
To: [email protected]; [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]; Wentland, Harry <[email protected]>; Zuo, Jerry <[email protected]>; Lin,
Wayne
Quoted 8 lines
<[email protected]>; [email protected]
Subject: [PATCH v2] drm/dp_mst: Clear MSG_RDY flag before sending new message
[Why]
The sequence for collecting down_reply from source perspective should be:
Quoted 7 lines
Request_n->repeat (get partial reply of Request_n->clear message ready flag to ack DPRX that the message is received) till all partial replies for Request_n are received->new Request_n+1.
Now there is chance that drm_dp_mst_hpd_irq() will fire new down request in the tx queue when the down reply is incomplete. Source is restricted to generate interveleaved message transactions so we should
avoid it.
Quoted 3 lines
Also, while assembling partial reply packets, reading out DPCD DOWN_REP Sideband MSG buffer + clearing DOWN_REP_MSG_RDY flag
should
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c static void
Quoted 35 lines
be wrapped up as a complete operation for reading out a reply packet. Kicking off a new request before clearing DOWN_REP_MSG_RDY flag might be risky. e.g. If the reply of the new request has overwritten the DPRX DOWN_REP Sideband MSG buffer before source writing one to clear DOWN_REP_MSG_RDY flag, source then unintentionally flushes the reply for the new request. Should handle the up request in the same way.
[How]
Separete drm_dp_mst_hpd_irq() into 2 steps. After acking the MST IRQ event, driver calls drm_dp_mst_hpd_irq_step2() and might trigger drm_dp_mst_kick_tx() only when there is no on going message transaction.
Changes since v1:
* Reworked on review comments received
-> Adjust the fix to let driver explicitly kick off new down request when mst irq event is handled and acked
-> Adjust the commit message
Signed-off-by Wayne Lin <[email protected]>
Cc [email protected]
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  8 ++---
drivers/gpu/drm/display/drm_dp_mst_topology.c | 35
++++++++++++++++---
 drivers/gpu/drm/i915/display/intel_dp.c       |  5 ++-
 drivers/gpu/drm/nouveau/dispnv50/disp.c       |  5 ++-
 include/drm/display/drm_dp_mst_helper.h       |  4 +--
 5 files changed, 45 insertions(+), 12 deletions(-)
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +0 -0
index 1ad67c2a697e..48bdcb2ee9b1 100644
@@ -3259,10 +3259,9 @@ static void
dm_handle_mst_sideband_msg(struct
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c static void · line 3260
Quoted 2 lines
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +0 -0
 amdgpu_dm_connector *aconnector)
 		DRM_DEBUG_DRIVER("ESI %02x %02x %02x\n", esi[0], esi[1],
esi[2]);
Quoted 38 lines
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +3 -4
 		/* handle HPD short pulse irq */
 		if (aconnector->mst_mgr.mst_state)
-			drm_dp_mst_hpd_irq(
-				&aconnector->mst_mgr,
-				esi,
-				&new_irq_handled);
+			drm_dp_mst_hpd_irq_step1(&aconnector-
 >mst_mgr,
+						 esi,
+						 &new_irq_handled);
 
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +1 -0
 		if (new_irq_handled) {
 			/* ACK at DPCD to notify down stream */ @@ -3281,6
+3280,7 @@ static void dm_handle_mst_sideband_msg(struct
amdgpu_dm_connector *aconnector)
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +1 -0
 					break;
 			}
 
+			drm_dp_mst_hpd_irq_step2(&aconnector-
Quoted 1 line
mst_mgr);
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +0 -0
 			/* check if there is new irq to be handled */
 			dret = drm_dp_dpcd_read(
 				&aconnector->dm_dp_aux.aux,
drivers/gpu/drm/display/drm_dp_mst_topology.c +2 -1
index 70df29fe92db..2e0a38a6509c 100644
@@ -4045,7 +4045,7 @@ static int drm_dp_mst_handle_up_req(struct
 drm_dp_mst_topology_mgr *mgr)  }
 
 /**
- * drm_dp_mst_hpd_irq() - MST hotplug IRQ notify
+ * drm_dp_mst_hpd_irq_step1() - MST hotplug IRQ notify
  * @mgr: manager to notify irq for.
  * @esi: 4 bytes from SINK_COUNT_ESI
  * @handled: whether the hpd interrupt was consumed or not @@ -4055,7
+4055,7 @@ static int drm_dp_mst_handle_up_req(struct
Quoted 2 lines
drm_dp_mst_topology_mgr *mgr)
* topology manager will process the sideband messages received as a
result
Quoted 3 lines
* of this.
*/
-int drm_dp_mst_hpd_irq(struct drm_dp_mst_topology_mgr *mgr, u8
*esi,
Quoted 20 lines
bool *handled)
+int drm_dp_mst_hpd_irq_step1(struct drm_dp_mst_topology_mgr *mgr, u8
drivers/gpu/drm/display/drm_dp_mst_topology.c +1 -0
+*esi, bool *handled)
 {
 	int ret = 0;
 	int sc;
@@ -4077,11 +4077,38 @@ int drm_dp_mst_hpd_irq(struct
drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl
drivers/gpu/drm/display/drm_dp_mst_topology.c +4 -2
 		*handled = true;
 	}
 
-	drm_dp_mst_kick_tx(mgr);
 	return ret;
 }
-EXPORT_SYMBOL(drm_dp_mst_hpd_irq);
+EXPORT_SYMBOL(drm_dp_mst_hpd_irq_step1);
+
+/**
+ * drm_dp_mst_hpd_irq_step2() - MST hotplug IRQ 2nd part handling
drivers/gpu/drm/display/drm_dp_mst_topology.c int drm_dp_mst_hpd_irq(struct drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl
Quoted 10 lines
drivers/gpu/drm/display/drm_dp_mst_topology.c +10 -0
+ * @mgr: manager to notify irq for.
+ *
+ * This should be called from the driver when mst irq event is
+handled
+ * and acked. Note that new down request should only be sent when
+ * previous message transaction is done. Source is not supposed to
+generate
+ * interleaved message transactions.
+ */
+void drm_dp_mst_hpd_irq_step2(struct drm_dp_mst_topology_mgr
*mgr)
Quoted 28 lines
{
drivers/gpu/drm/display/drm_dp_mst_topology.c +5 -0
+	struct drm_dp_sideband_msg_tx *txmsg;
+	bool skip = false;
 
+	mutex_lock(&mgr->qlock);
+	txmsg = list_first_entry_or_null(&mgr->tx_msg_downq,
+					 struct drm_dp_sideband_msg_tx,
next);
drivers/gpu/drm/display/drm_dp_mst_topology.c +11 -0
+	/* If last transaction is not completed yet*/
+	if (!txmsg ||
+	    txmsg->state == DRM_DP_SIDEBAND_TX_START_SEND ||
+	    txmsg->state == DRM_DP_SIDEBAND_TX_SENT)
+		skip = true;
+	mutex_unlock(&mgr->qlock);
+
+	if (!skip)
+		drm_dp_mst_kick_tx(mgr);
+}
+EXPORT_SYMBOL(drm_dp_mst_hpd_irq_step2);
 /**
  * drm_dp_mst_detect_port() - get connection status for an MST port
  * @connector: DRM connector for this port diff --git
a/drivers/gpu/drm/i915/display/intel_dp.c
b/drivers/gpu/drm/i915/display/intel_dp.c
index 75070eb07d4b..9a9a5aec9534 100644
drivers/gpu/drm/i915/display/intel_dp.c intel_dp_mst_hpd_irq(struct intel_dp *intel_dp, · line 3808
Quoted 7 lines
drivers/gpu/drm/i915/display/intel_dp.c +1 -1
@@ -3803,7 +3803,7 @@ intel_dp_mst_hpd_irq(struct intel_dp *intel_dp,
 u8 *esi, u8 *ack)  {
 	bool handled = false;
 
-	drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled);
+	drm_dp_mst_hpd_irq_step1(&intel_dp->mst_mgr, esi, &handled);
 	if (handled)
 		ack[1] |= esi[1] & (DP_DOWN_REP_MSG_RDY |
DP_UP_REQ_MSG_RDY);
Quoted 19 lines
drivers/gpu/drm/i915/display/intel_dp.c +3 -0
@@ -3880,6 +3880,9 @@ intel_dp_check_mst_status(struct intel_dp
 *intel_dp)
 
 		if (!intel_dp_ack_sink_irq_esi(intel_dp, ack))
 			drm_dbg_kms(&i915->drm, "Failed to ack ESI\n");
+
+		if (ack[1] & (DP_DOWN_REP_MSG_RDY |
 DP_UP_REQ_MSG_RDY))
+			drm_dp_mst_hpd_irq_step2(&intel_dp->mst_mgr);
 	}
drivers/gpu/drm/i915/display/intel_dp.c +0 -0
 	return link_ok;
drivers/gpu/drm/nouveau/dispnv50/disp.c +0 -0
index ed9d374147b8..00c36fcc8afd 100644
drivers/gpu/drm/nouveau/dispnv50/disp.c nv50_mstm_service(struct nouveau_drm *drm, · line 1339
Quoted 9 lines
drivers/gpu/drm/nouveau/dispnv50/disp.c +1 -1
@@ -1332,12 +1332,15 @@ nv50_mstm_service(struct nouveau_drm *drm,
 			break;
 		}
 
-		drm_dp_mst_hpd_irq(&mstm->mgr, esi, &handled);
+		drm_dp_mst_hpd_irq_step1(&mstm->mgr, esi, &handled);
 		if (!handled)
 			break;
 
 		rc = drm_dp_dpcd_write(aux, DP_SINK_COUNT_ESI + 1,
&esi[1],
Quoted 13 lines
drivers/gpu/drm/nouveau/dispnv50/disp.c +3 -0
 				       3);
+
+		drm_dp_mst_hpd_irq_step2(&mstm->mgr);
+
 		if (rc != 3) {
 			ret = false;
 			break;
include/drm/display/drm_dp_mst_helper.h +0 -0
index 32c764fb9cb5..6c08ba765d5a 100644
@@ -815,8 +815,8 @@ void drm_dp_mst_topology_mgr_destroy(struct
Quoted 3 lines
drm_dp_mst_topology_mgr *mgr); bool drm_dp_read_mst_cap(struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_CAP_SIZE]); int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr
*mgr,
Quoted 3 lines
bool mst_state);
-int drm_dp_mst_hpd_irq(struct drm_dp_mst_topology_mgr *mgr, u8
*esi,
Quoted 11 lines
bool *handled);
include/drm/display/drm_dp_mst_helper.h +1 -1
-
+int drm_dp_mst_hpd_irq_step1(struct drm_dp_mst_topology_mgr *mgr,
u8
include/drm/display/drm_dp_mst_helper.h +2 -1
+*esi, bool *handled); void drm_dp_mst_hpd_irq_step2(struct
+drm_dp_mst_topology_mgr *mgr);
 
 int
 drm_dp_mst_detect_port(struct drm_connector *connector,
--
2.37.3

Jani Nikula

2023-05-16 10:34 UTC #3 lore raw

In reply to Wayne Lin

Quoted 83 lines · Wayne Lin

[Why]
The sequence for collecting down_reply from source perspective should be:
Request_n->repeat (get partial reply of Request_n->clear message ready flag to ack DPRX that the message is received) till all partial replies for Request_n are received->new Request_n+1.
Now there is chance that drm_dp_mst_hpd_irq() will fire new down request in the tx queue when the down reply is incomplete. Source is restricted to generate interveleaved message transactions so we should avoid it.
Also, while assembling partial reply packets, reading out DPCD DOWN_REP Sideband MSG buffer + clearing DOWN_REP_MSG_RDY flag should be wrapped up as a complete operation for reading out a reply packet. Kicking off a new request before clearing DOWN_REP_MSG_RDY flag might be risky. e.g. If the reply of the new request has overwritten the DPRX DOWN_REP Sideband MSG buffer before source writing one to clear DOWN_REP_MSG_RDY flag, source then unintentionally flushes the reply for the new request. Should handle the up request in the same way.
[How]
Separete drm_dp_mst_hpd_irq() into 2 steps. After acking the MST IRQ event, driver calls drm_dp_mst_hpd_irq_step2() and might trigger drm_dp_mst_kick_tx() only when there is no on going message transaction.
Changes since v1:
* Reworked on review comments received
-> Adjust the fix to let driver explicitly kick off new down request when mst irq event is handled and acked
-> Adjust the commit message
Signed-off-by Wayne Lin <[email protected]>
Cc [email protected]
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  8 ++---
 drivers/gpu/drm/display/drm_dp_mst_topology.c | 35 ++++++++++++++++---
 drivers/gpu/drm/i915/display/intel_dp.c       |  5 ++-
 drivers/gpu/drm/nouveau/dispnv50/disp.c       |  5 ++-
 include/drm/display/drm_dp_mst_helper.h       |  4 +--
 5 files changed, 45 insertions(+), 12 deletions(-)
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +4 -4
index 1ad67c2a697e..48bdcb2ee9b1 100644
@@ -3259,10 +3259,9 @@ static void dm_handle_mst_sideband_msg(struct amdgpu_dm_connector *aconnector)
 		DRM_DEBUG_DRIVER("ESI %02x %02x %02x\n", esi[0], esi[1], esi[2]);
 		/* handle HPD short pulse irq */
 		if (aconnector->mst_mgr.mst_state)
-			drm_dp_mst_hpd_irq(
-				&aconnector->mst_mgr,
-				esi,
-				&new_irq_handled);
+			drm_dp_mst_hpd_irq_step1(&aconnector->mst_mgr,
+						 esi,
+						 &new_irq_handled);
 
 		if (new_irq_handled) {
 			/* ACK at DPCD to notify down stream */
@@ -3281,6 +3280,7 @@ static void dm_handle_mst_sideband_msg(struct amdgpu_dm_connector *aconnector)
 					break;
 			}
 
+			drm_dp_mst_hpd_irq_step2(&aconnector->mst_mgr);
 			/* check if there is new irq to be handled */
 			dret = drm_dp_dpcd_read(
 				&aconnector->dm_dp_aux.aux,
drivers/gpu/drm/display/drm_dp_mst_topology.c +1 -1
index 70df29fe92db..2e0a38a6509c 100644
@@ -4045,7 +4045,7 @@ static int drm_dp_mst_handle_up_req(struct drm_dp_mst_topology_mgr *mgr)
 }
 
 /**
- * drm_dp_mst_hpd_irq() - MST hotplug IRQ notify
+ * drm_dp_mst_hpd_irq_step1() - MST hotplug IRQ notify
  * @mgr: manager to notify irq for.
  * @esi: 4 bytes from SINK_COUNT_ESI
  * @handled: whether the hpd interrupt was consumed or not

drivers/gpu/drm/display/drm_dp_mst_topology.c static int drm_dp_mst_handle_up_req(struct drm_dp_mst_topology_mgr *mgr) · line 4058

Quoted 5 lines

drivers/gpu/drm/display/drm_dp_mst_topology.c +1 -1

@@ -4055,7 +4055,7 @@ static int drm_dp_mst_handle_up_req(struct drm_dp_mst_topology_mgr *mgr)
  * topology manager will process the sideband messages received as a result
  * of this.
  */
-int drm_dp_mst_hpd_irq(struct drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handled)
+int drm_dp_mst_hpd_irq_step1(struct drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handled)

If you're changing the signature of the function, I'd make esi "const u8 *esi", and add a separate "u8 *ack" that you have to provide, where this function would |= the flags to ack. It would be useful at least in i915.

As to naming, _step1 and _step2 are pretty vague.

Quoted 13 lines

drivers/gpu/drm/display/drm_dp_mst_topology.c +2 -2

 {
 	int ret = 0;
 	int sc;

@@ -4077,11 +4077,38 @@ int drm_dp_mst_hpd_irq(struct drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl
 		*handled = true;
 	}
 
-	drm_dp_mst_kick_tx(mgr);
 	return ret;
 }
-EXPORT_SYMBOL(drm_dp_mst_hpd_irq);
+EXPORT_SYMBOL(drm_dp_mst_hpd_irq_step1);
+

drivers/gpu/drm/display/drm_dp_mst_topology.c int drm_dp_mst_hpd_irq(struct drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl · line 4093

Quoted 10 lines

drivers/gpu/drm/display/drm_dp_mst_topology.c +10 -0

+/**
+ * drm_dp_mst_hpd_irq_step2() - MST hotplug IRQ 2nd part handling
+ * @mgr: manager to notify irq for.
+ *
+ * This should be called from the driver when mst irq event is handled
+ * and acked. Note that new down request should only be sent when
+ * previous message transaction is done. Source is not supposed to generate
+ * interleaved message transactions.
+ */
+void drm_dp_mst_hpd_irq_step2(struct drm_dp_mst_topology_mgr *mgr)

_done, _finish, _complete?

Quoted 5 lines

drivers/gpu/drm/display/drm_dp_mst_topology.c +4 -0

+{
+	struct drm_dp_sideband_msg_tx *txmsg;
+	bool skip = false;
 
+	mutex_lock(&mgr->qlock);

drivers/gpu/drm/display/drm_dp_mst_topology.c int drm_dp_mst_hpd_irq(struct drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl · line 4108

Quoted 10 lines

drivers/gpu/drm/display/drm_dp_mst_topology.c +10 -0

+	txmsg = list_first_entry_or_null(&mgr->tx_msg_downq,
+					 struct drm_dp_sideband_msg_tx, next);
+	/* If last transaction is not completed yet*/
+	if (!txmsg ||
+	    txmsg->state == DRM_DP_SIDEBAND_TX_START_SEND ||
+	    txmsg->state == DRM_DP_SIDEBAND_TX_SENT)
+		skip = true;
+	mutex_unlock(&mgr->qlock);
+
+	if (!skip)

Please avoid negatives like this. You could have bool kick = true instead.

Quoted 20 lines

drivers/gpu/drm/display/drm_dp_mst_topology.c +3 -0

+		drm_dp_mst_kick_tx(mgr);
+}
+EXPORT_SYMBOL(drm_dp_mst_hpd_irq_step2);
 /**
  * drm_dp_mst_detect_port() - get connection status for an MST port
  * @connector: DRM connector for this port

drivers/gpu/drm/i915/display/intel_dp.c +1 -1

index 75070eb07d4b..9a9a5aec9534 100644
@@ -3803,7 +3803,7 @@ intel_dp_mst_hpd_irq(struct intel_dp *intel_dp, u8 *esi, u8 *ack)
 {
 	bool handled = false;
 
-	drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled);
+	drm_dp_mst_hpd_irq_step1(&intel_dp->mst_mgr, esi, &handled);
 	if (handled)
 		ack[1] |= esi[1] & (DP_DOWN_REP_MSG_RDY | DP_UP_REQ_MSG_RDY);

drivers/gpu/drm/i915/display/intel_dp.c intel_dp_check_mst_status(struct intel_dp *intel_dp) · line 3885

Quoted 6 lines

drivers/gpu/drm/i915/display/intel_dp.c +3 -0

@@ -3880,6 +3880,9 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
 
 		if (!intel_dp_ack_sink_irq_esi(intel_dp, ack))
 			drm_dbg_kms(&i915->drm, "Failed to ack ESI\n");
+
+		if (ack[1] & (DP_DOWN_REP_MSG_RDY | DP_UP_REQ_MSG_RDY))
+			drm_dp_mst_hpd_irq_step2(&intel_dp->mst_mgr);

I'm getting confused about the division of responsibilities between the two functions to be called, and the caller. Why does i915 do things differently from nouveau and amd wrt this?

Quoted 11 lines

drivers/gpu/drm/i915/display/intel_dp.c +0 -0
 	}
 
 	return link_ok;
drivers/gpu/drm/nouveau/dispnv50/disp.c +0 -0
index ed9d374147b8..00c36fcc8afd 100644
@@ -1332,12 +1332,15 @@ nv50_mstm_service(struct nouveau_drm *drm,
 			break;
 		}
 

drivers/gpu/drm/nouveau/dispnv50/disp.c nv50_mstm_service(struct nouveau_drm *drm, · line 1343

Quoted 10 lines

drivers/gpu/drm/nouveau/dispnv50/disp.c +4 -1

-		drm_dp_mst_hpd_irq(&mstm->mgr, esi, &handled);
+		drm_dp_mst_hpd_irq_step1(&mstm->mgr, esi, &handled);
 		if (!handled)
 			break;
 
 		rc = drm_dp_dpcd_write(aux, DP_SINK_COUNT_ESI + 1, &esi[1],
 				       3);
+
+		drm_dp_mst_hpd_irq_step2(&mstm->mgr);
+

Don't you think the return value should be checked first?

Quoted 18 lines

drivers/gpu/drm/nouveau/dispnv50/disp.c +0 -0

 		if (rc != 3) {
 			ret = false;
 			break;

include/drm/display/drm_dp_mst_helper.h +2 -2

index 32c764fb9cb5..6c08ba765d5a 100644
@@ -815,8 +815,8 @@ void drm_dp_mst_topology_mgr_destroy(struct drm_dp_mst_topology_mgr *mgr);
 bool drm_dp_read_mst_cap(struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_CAP_SIZE]);
 int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool mst_state);
 
-int drm_dp_mst_hpd_irq(struct drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handled);
-
+int drm_dp_mst_hpd_irq_step1(struct drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handled);
+void drm_dp_mst_hpd_irq_step2(struct drm_dp_mst_topology_mgr *mgr);
 
 int
 drm_dp_mst_detect_port(struct drm_connector *connector,

Signature · 1 line

Jani Nikula, Intel Open Source Graphics Center

Lin, Wayne

author

2023-05-22 07:11 UTC #4 lore raw

[Public]

Hi Jani,
Thanks for your time!
Comments inline.

Quoted 113 lines

-----Original Message-----
From: Jani Nikula <[email protected]>
Sent: Tuesday, May 16, 2023 6:34 PM
To: Lin, Wayne <[email protected]>; [email protected]; [email protected]
Cc: [email protected]; [email protected]; [email protected]; Wentland, Harry <[email protected]>; Zuo, Jerry <[email protected]>; Lin, Wayne <[email protected]>; [email protected]
Subject: Re: [PATCH v2] drm/dp_mst: Clear MSG_RDY flag before sending new message
Quoted 8 lines · Wayne Lin
[Why]
The sequence for collecting down_reply from source perspective should be:
Request_n->repeat (get partial reply of Request_n->clear message ready flag to ack DPRX that the message is received) till all partial replies for Request_n are received->new Request_n+1.
Quoted 7 lines
Now there is chance that drm_dp_mst_hpd_irq() will fire new down request in the tx queue when the down reply is incomplete. Source is restricted to generate interveleaved message transactions so we should avoid it.
Also, while assembling partial reply packets, reading out DPCD DOWN_REP Sideband MSG buffer + clearing DOWN_REP_MSG_RDY flag
should
Quoted 18 lines
be wrapped up as a complete operation for reading out a reply packet. Kicking off a new request before clearing DOWN_REP_MSG_RDY flag might be risky. e.g. If the reply of the new request has overwritten the DPRX DOWN_REP Sideband MSG buffer before source writing one to clear DOWN_REP_MSG_RDY flag, source then unintentionally flushes the reply for the new request. Should handle the up request in the same way.
[How]
Separete drm_dp_mst_hpd_irq() into 2 steps. After acking the MST IRQ event, driver calls drm_dp_mst_hpd_irq_step2() and might trigger drm_dp_mst_kick_tx() only when there is no on going message transaction.
Changes since v1:
* Reworked on review comments received
-> Adjust the fix to let driver explicitly kick off new down request when mst irq event is handled and acked
-> Adjust the commit message
Quoted 5 lines
Signed-off-by Wayne Lin <[email protected]>
Cc [email protected]
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  8 ++---
drivers/gpu/drm/display/drm_dp_mst_topology.c | 35
++++++++++++++++---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c static void
Quoted 11 lines
 drivers/gpu/drm/i915/display/intel_dp.c       |  5 ++-
 drivers/gpu/drm/nouveau/dispnv50/disp.c       |  5 ++-
 include/drm/display/drm_dp_mst_helper.h       |  4 +--
 5 files changed, 45 insertions(+), 12 deletions(-)
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +0 -0
index 1ad67c2a697e..48bdcb2ee9b1 100644
@@ -3259,10 +3259,9 @@ static void
dm_handle_mst_sideband_msg(struct amdgpu_dm_connector *aconnector)
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c static void · line 3259
Quoted 1 line
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +0 -0
            DRM_DEBUG_DRIVER("ESI %02x %02x %02x\n", esi[0],
esi[1], esi[2]);
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c static void dm_handle_mst_sideband_msg(struct amdgpu_dm_connector *aconnector)
Quoted 12 lines
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +3 -4
            /* handle HPD short pulse irq */
            if (aconnector->mst_mgr.mst_state)
-                   drm_dp_mst_hpd_irq(
-                           &aconnector->mst_mgr,
-                           esi,
-                           &new_irq_handled);
+                   drm_dp_mst_hpd_irq_step1(&aconnector->mst_mgr,
+                                            esi,
+                                            &new_irq_handled);
 
            if (new_irq_handled) {
                    /* ACK at DPCD to notify down stream */ @@ -
3281,6 +3280,7 @@
Quoted 1 line
static void dm_handle_mst_sideband_msg(struct amdgpu_dm_connector
*aconnector)
Quoted 14 lines
                                    break;
                    }
+ drm_dp_mst_hpd_irq_step2(&aconnector-
mst_mgr);
                    /* check if there is new irq to be handled */
                    dret = drm_dp_dpcd_read(
                            &aconnector->dm_dp_aux.aux,
drivers/gpu/drm/display/drm_dp_mst_topology.c +0 -0
index 70df29fe92db..2e0a38a6509c 100644
drivers/gpu/drm/display/drm_dp_mst_topology.c static int drm_dp_mst_handle_up_req(struct
Quoted 9 lines
drivers/gpu/drm/display/drm_dp_mst_topology.c +2 -1
@@ -4045,7 +4045,7 @@ static int drm_dp_mst_handle_up_req(struct
 drm_dp_mst_topology_mgr *mgr)  }
 
 /**
- * drm_dp_mst_hpd_irq() - MST hotplug IRQ notify
+ * drm_dp_mst_hpd_irq_step1() - MST hotplug IRQ notify
  * @mgr: manager to notify irq for.
  * @esi: 4 bytes from SINK_COUNT_ESI
  * @handled: whether the hpd interrupt was consumed or not @@ -4055,7
+4055,7 @@ static int drm_dp_mst_handle_up_req(struct
drm_dp_mst_topology_mgr *mgr)
Quoted 6 lines
* topology manager will process the sideband messages received as a result
* of this.
*/
-int drm_dp_mst_hpd_irq(struct drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handled)
+int drm_dp_mst_hpd_irq_step1(struct drm_dp_mst_topology_mgr *mgr,
u8

Quoted 5 lines

Quoted 1 line
+*esi, bool *handled)
If you're changing the signature of the function, I'd make esi "const u8 *esi", and add a separate "u8 *ack" that you have to provide, where this function would |= the flags to ack. It would be useful at least in i915.

Will adjust. Thanks.

Quoted 2 lines

As to naming, _step1 and _step2 are pretty vague.

Was trying to align the naming method we used for payload allocation/de-allocation. Anyway, I'll adjust the naming here.

Quoted 29 lines

drivers/gpu/drm/display/drm_dp_mst_topology.c int drm_dp_mst_hpd_irq(struct
Quoted 5 lines
 {
    int ret = 0;
    int sc;
drivers/gpu/drm/display/drm_dp_mst_topology.c +0 -0
@@ -4077,11 +4077,38 @@ int drm_dp_mst_hpd_irq(struct
drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl
Quoted 11 lines
drivers/gpu/drm/display/drm_dp_mst_topology.c +4 -2
            *handled = true;
    }
 
-   drm_dp_mst_kick_tx(mgr);
    return ret;
 }
-EXPORT_SYMBOL(drm_dp_mst_hpd_irq);
+EXPORT_SYMBOL(drm_dp_mst_hpd_irq_step1);
+
+/**
+ * drm_dp_mst_hpd_irq_step2() - MST hotplug IRQ 2nd part handling
drivers/gpu/drm/display/drm_dp_mst_topology.c int drm_dp_mst_hpd_irq(struct drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl
Quoted 10 lines
drivers/gpu/drm/display/drm_dp_mst_topology.c +10 -0
+ * @mgr: manager to notify irq for.
+ *
+ * This should be called from the driver when mst irq event is
+handled
+ * and acked. Note that new down request should only be sent when
+ * previous message transaction is done. Source is not supposed to
+generate
+ * interleaved message transactions.
+ */
+void drm_dp_mst_hpd_irq_step2(struct drm_dp_mst_topology_mgr
*mgr)

Quoted 1 line

_done, _finish, _complete?

Will use "complete". Thanks.

Quoted 18 lines

drivers/gpu/drm/display/drm_dp_mst_topology.c int drm_dp_mst_hpd_irq(struct drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl
Quoted 8 lines
drivers/gpu/drm/display/drm_dp_mst_topology.c +6 -0
+{
+   struct drm_dp_sideband_msg_tx *txmsg;
+   bool skip = false;
 
+   mutex_lock(&mgr->qlock);
+   txmsg = list_first_entry_or_null(&mgr->tx_msg_downq,
+                                    struct drm_dp_sideband_msg_tx,
next);
drivers/gpu/drm/display/drm_dp_mst_topology.c int drm_dp_mst_hpd_irq(struct drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl · line 4108
Quoted 8 lines
drivers/gpu/drm/display/drm_dp_mst_topology.c +8 -0
+   /* If last transaction is not completed yet*/
+   if (!txmsg ||
+       txmsg->state == DRM_DP_SIDEBAND_TX_START_SEND ||
+       txmsg->state == DRM_DP_SIDEBAND_TX_SENT)
+           skip = true;
+   mutex_unlock(&mgr->qlock);
+
+   if (!skip)

Quoted 1 line

Please avoid negatives like this. You could have bool kick = true instead.

Thanks. Will modify it.

Quoted 30 lines

Quoted 13 lines
drivers/gpu/drm/display/drm_dp_mst_topology.c +3 -0
+           drm_dp_mst_kick_tx(mgr);
+}
+EXPORT_SYMBOL(drm_dp_mst_hpd_irq_step2);
 /**
  * drm_dp_mst_detect_port() - get connection status for an MST port
  * @connector: DRM connector for this port diff --git
a/drivers/gpu/drm/i915/display/intel_dp.c
b/drivers/gpu/drm/i915/display/intel_dp.c
index 75070eb07d4b..9a9a5aec9534 100644
drivers/gpu/drm/i915/display/intel_dp.c intel_dp_mst_hpd_irq(struct intel_dp *intel_dp, · line 3808
Quoted 7 lines
drivers/gpu/drm/i915/display/intel_dp.c +1 -1
@@ -3803,7 +3803,7 @@ intel_dp_mst_hpd_irq(struct intel_dp *intel_dp,
 u8 *esi, u8 *ack)  {
    bool handled = false;
 
-   drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled);
+   drm_dp_mst_hpd_irq_step1(&intel_dp->mst_mgr, esi, &handled);
    if (handled)
            ack[1] |= esi[1] & (DP_DOWN_REP_MSG_RDY |
DP_UP_REQ_MSG_RDY);
drivers/gpu/drm/i915/display/intel_dp.c intel_dp_check_mst_status(struct intel_dp · line 3885
Quoted 8 lines
drivers/gpu/drm/i915/display/intel_dp.c +2 -0
@@ -3880,6 +3880,9 @@ intel_dp_check_mst_status(struct intel_dp
 *intel_dp)
 
            if (!intel_dp_ack_sink_irq_esi(intel_dp, ack))
                    drm_dbg_kms(&i915->drm, "Failed to ack ESI\n");
+
+           if (ack[1] & (DP_DOWN_REP_MSG_RDY |
DP_UP_REQ_MSG_RDY))

Quoted 5 lines

drivers/gpu/drm/i915/display/intel_dp.c intel_dp_check_mst_status(struct intel_dp *intel_dp) · line 3885
Quoted 1 line
drivers/gpu/drm/i915/display/intel_dp.c +1 -0
+                   drm_dp_mst_hpd_irq_step2(&intel_dp->mst_mgr);
I'm getting confused about the division of responsibilities between the two functions to be called, and the caller. Why does i915 do things differently from nouveau and amd wrt this?

The main idea is trying to ack the irq before sending a new request. We used to send a new request before acking the irq event which will cause message interleaving and that's not recommended by DP spec.

amd and nouveau only handle mst up/down message irq events while calling drm_dp_mst_hpd_irq() but i915 also tries to handle content protection irq while handling mst events. That's why it's a bit different between amd/nouveau and i915.

Quoted 19 lines

drivers/gpu/drm/nouveau/dispnv50/disp.c nv50_mstm_service(struct nouveau_drm

Quoted 10 lines

drivers/gpu/drm/i915/display/intel_dp.c +0 -0
    }
 
    return link_ok;
drivers/gpu/drm/nouveau/dispnv50/disp.c +0 -0
index ed9d374147b8..00c36fcc8afd 100644
@@ -1332,12 +1332,15 @@ nv50_mstm_service(struct nouveau_drm

*drm,

Quoted 8 lines

drivers/gpu/drm/nouveau/dispnv50/disp.c +1 -1

                    break;
            }
 
-           drm_dp_mst_hpd_irq(&mstm->mgr, esi, &handled);
+           drm_dp_mst_hpd_irq_step1(&mstm->mgr, esi, &handled);
            if (!handled)
                    break;

Quoted 8 lines

drivers/gpu/drm/nouveau/dispnv50/disp.c nv50_mstm_service(struct nouveau_drm · line 1339
Quoted 1 line
drivers/gpu/drm/nouveau/dispnv50/disp.c +0 -0
            rc = drm_dp_dpcd_write(aux, DP_SINK_COUNT_ESI + 1,
&esi[1],
drivers/gpu/drm/nouveau/dispnv50/disp.c nv50_mstm_service(struct nouveau_drm *drm, · line 1343
Quoted 4 lines
drivers/gpu/drm/nouveau/dispnv50/disp.c +3 -0
                                   3);
+
+           drm_dp_mst_hpd_irq_step2(&mstm->mgr);
+
Don't you think the return value should be checked first?

Sorry I missed it. Will adjust. Thanks!

Quoted 21 lines

Quoted 10 lines
drivers/gpu/drm/nouveau/dispnv50/disp.c +0 -0
            if (rc != 3) {
                    ret = false;
                    break;
include/drm/display/drm_dp_mst_helper.h +0 -0
index 32c764fb9cb5..6c08ba765d5a 100644
@@ -815,8 +815,8 @@ void drm_dp_mst_topology_mgr_destroy(struct
Quoted 3 lines
drm_dp_mst_topology_mgr *mgr); bool drm_dp_read_mst_cap(struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_CAP_SIZE]); int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr
*mgr,
include/drm/display/drm_dp_mst_helper.h
Quoted 6 lines
bool mst_state);
-int drm_dp_mst_hpd_irq(struct drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handled);
include/drm/display/drm_dp_mst_helper.h +1 -1
-
+int drm_dp_mst_hpd_irq_step1(struct drm_dp_mst_topology_mgr *mgr,
u8

Quoted 8 lines

include/drm/display/drm_dp_mst_helper.h void drm_dp_mst_topology_mgr_destroy(struct drm_dp_mst_topology_mgr *mgr); · line 822
Quoted 5 lines
include/drm/display/drm_dp_mst_helper.h +2 -0
+*esi, bool *handled); void drm_dp_mst_hpd_irq_step2(struct
+drm_dp_mst_topology_mgr *mgr);
 
 int
 drm_dp_mst_detect_port(struct drm_connector *connector,
--
Jani Nikula, Intel Open Source Graphics Center

--
Regards,
Wayne Lin