[PATCH] drm/dp_mst: Clear MSG_RDY flag before sending new message

[PATCH] drm/dp_mst: Clear MSG_RDY flag before sending new message

[Wayne Lin]

[Why & How]
The sequence for collecting down_reply/up_request 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.

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

In drm_dp_mst_hpd_irq(), we don't clear MSG_RDY flag before caliing
drm_dp_mst_kick_tx(). Fix that.

Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
Cc: stable@vger.kernel.org
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  2 ++
 drivers/gpu/drm/display/drm_dp_mst_topology.c | 22 +++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_dp.c       |  3 +++
 drivers/gpu/drm/nouveau/dispnv50/disp.c       |  2 ++
 4 files changed, 29 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 77277d90b6e2..5313a5656598 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3166,6 +3166,8 @@ static void dm_handle_mst_sideband_msg(struct amdgpu_dm_connector *aconnector)
 			for (retry = 0; retry < 3; retry++) {
 				uint8_t wret;
 
+				/* MSG_RDY ack is done in drm*/
+				esi[1] &= ~(DP_DOWN_REP_MSG_RDY | DP_UP_REQ_MSG_RDY);
 				wret = drm_dp_dpcd_write(
 					&aconnector->dm_dp_aux.aux,
 					dpcd_addr + 1,
diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index 51a46689cda7..02aad713c67c 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -4054,6 +4054,9 @@ int drm_dp_mst_hpd_irq(struct drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl
 {
 	int ret = 0;
 	int sc;
+	const int tosend = 1;
+	int retries = 0;
+	u8 buf = 0;
 	*handled = false;
 	sc = DP_GET_SINK_COUNT(esi[0]);
 
@@ -4072,6 +4075,25 @@ int drm_dp_mst_hpd_irq(struct drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl
 		*handled = true;
 	}
 
+	if (*handled) {
+		buf = esi[1] & (DP_DOWN_REP_MSG_RDY | DP_UP_REQ_MSG_RDY);
+		do {
+			ret = drm_dp_dpcd_write(mgr->aux,
+						DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0,
+						&buf,
+						tosend);
+
+			if (ret == tosend)
+				break;
+
+			retries++;
+		} while (retries < 5);
+
+		if (ret != tosend)
+			drm_dbg_kms(mgr->dev, "failed to write dpcd 0x%x\n",
+				    DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0);
+	}
+
 	drm_dp_mst_kick_tx(mgr);
 	return ret;
 }
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index bf80f296a8fd..abec3de38b66 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -3939,6 +3939,9 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
 		if (!memchr_inv(ack, 0, sizeof(ack)))
 			break;
 
+		/* MSG_RDY ack is done in drm*/
+		ack[1] &= ~(DP_DOWN_REP_MSG_RDY | DP_UP_REQ_MSG_RDY);
+
 		if (!intel_dp_ack_sink_irq_esi(intel_dp, ack))
 			drm_dbg_kms(&i915->drm, "Failed to ack ESI\n");
 	}
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index edcb2529b402..e905987104ed 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -1336,6 +1336,8 @@ nv50_mstm_service(struct nouveau_drm *drm,
 		if (!handled)
 			break;
 
+		/* MSG_RDY ack is done in drm*/
+		esi[1] &= ~(DP_DOWN_REP_MSG_RDY | DP_UP_REQ_MSG_RDY);
 		rc = drm_dp_dpcd_write(aux, DP_SINK_COUNT_ESI + 1, &esi[1],
 				       3);
 		if (rc != 3) {
-- 
2.37.3

[PATCH] drm/dp_mst: Clear MSG_RDY flag before sending new message

[Wayne Lin]

[Why & How]
The sequence for collecting down_reply/up_request 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.

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

In drm_dp_mst_hpd_irq(), we don't clear MSG_RDY flag before caliing
drm_dp_mst_kick_tx(). Fix that.

Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
Cc: stable@vger.kernel.org
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  2 ++
 drivers/gpu/drm/display/drm_dp_mst_topology.c | 22 +++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_dp.c       |  3 +++
 drivers/gpu/drm/nouveau/dispnv50/disp.c       |  2 ++
 4 files changed, 29 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 77277d90b6e2..5313a5656598 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3166,6 +3166,8 @@ static void dm_handle_mst_sideband_msg(struct amdgpu_dm_connector *aconnector)
 			for (retry = 0; retry < 3; retry++) {
 				uint8_t wret;
 
+				/* MSG_RDY ack is done in drm*/
+				esi[1] &= ~(DP_DOWN_REP_MSG_RDY | DP_UP_REQ_MSG_RDY);
 				wret = drm_dp_dpcd_write(
 					&aconnector->dm_dp_aux.aux,
 					dpcd_addr + 1,
diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index 51a46689cda7..02aad713c67c 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -4054,6 +4054,9 @@ int drm_dp_mst_hpd_irq(struct drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl
 {
 	int ret = 0;
 	int sc;
+	const int tosend = 1;
+	int retries = 0;
+	u8 buf = 0;
 	*handled = false;
 	sc = DP_GET_SINK_COUNT(esi[0]);
 
@@ -4072,6 +4075,25 @@ int drm_dp_mst_hpd_irq(struct drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl
 		*handled = true;
 	}
 
+	if (*handled) {
+		buf = esi[1] & (DP_DOWN_REP_MSG_RDY | DP_UP_REQ_MSG_RDY);
+		do {
+			ret = drm_dp_dpcd_write(mgr->aux,
+						DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0,
+						&buf,
+						tosend);
+
+			if (ret == tosend)
+				break;
+
+			retries++;
+		} while (retries < 5);
+
+		if (ret != tosend)
+			drm_dbg_kms(mgr->dev, "failed to write dpcd 0x%x\n",
+				    DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0);
+	}
+
 	drm_dp_mst_kick_tx(mgr);
 	return ret;
 }
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index bf80f296a8fd..abec3de38b66 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -3939,6 +3939,9 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
 		if (!memchr_inv(ack, 0, sizeof(ack)))
 			break;
 
+		/* MSG_RDY ack is done in drm*/
+		ack[1] &= ~(DP_DOWN_REP_MSG_RDY | DP_UP_REQ_MSG_RDY);
+
 		if (!intel_dp_ack_sink_irq_esi(intel_dp, ack))
 			drm_dbg_kms(&i915->drm, "Failed to ack ESI\n");
 	}
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index edcb2529b402..e905987104ed 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -1336,6 +1336,8 @@ nv50_mstm_service(struct nouveau_drm *drm,
 		if (!handled)
 			break;
 
+		/* MSG_RDY ack is done in drm*/
+		esi[1] &= ~(DP_DOWN_REP_MSG_RDY | DP_UP_REQ_MSG_RDY);
 		rc = drm_dp_dpcd_write(aux, DP_SINK_COUNT_ESI + 1, &esi[1],
 				       3);
 		if (rc != 3) {
-- 
2.37.3

[PATCH] drm/dp_mst: Clear MSG_RDY flag before sending new message

[Wayne Lin]

[Why & How]
The sequence for collecting down_reply/up_request 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.

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

In drm_dp_mst_hpd_irq(), we don't clear MSG_RDY flag before caliing
drm_dp_mst_kick_tx(). Fix that.

Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
Cc: stable@vger.kernel.org
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  2 ++
 drivers/gpu/drm/display/drm_dp_mst_topology.c | 22 +++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_dp.c       |  3 +++
 drivers/gpu/drm/nouveau/dispnv50/disp.c       |  2 ++
 4 files changed, 29 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 77277d90b6e2..5313a5656598 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3166,6 +3166,8 @@ static void dm_handle_mst_sideband_msg(struct amdgpu_dm_connector *aconnector)
 			for (retry = 0; retry < 3; retry++) {
 				uint8_t wret;
 
+				/* MSG_RDY ack is done in drm*/
+				esi[1] &= ~(DP_DOWN_REP_MSG_RDY | DP_UP_REQ_MSG_RDY);
 				wret = drm_dp_dpcd_write(
 					&aconnector->dm_dp_aux.aux,
 					dpcd_addr + 1,
diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index 51a46689cda7..02aad713c67c 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -4054,6 +4054,9 @@ int drm_dp_mst_hpd_irq(struct drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl
 {
 	int ret = 0;
 	int sc;
+	const int tosend = 1;
+	int retries = 0;
+	u8 buf = 0;
 	*handled = false;
 	sc = DP_GET_SINK_COUNT(esi[0]);
 
@@ -4072,6 +4075,25 @@ int drm_dp_mst_hpd_irq(struct drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl
 		*handled = true;
 	}
 
+	if (*handled) {
+		buf = esi[1] & (DP_DOWN_REP_MSG_RDY | DP_UP_REQ_MSG_RDY);
+		do {
+			ret = drm_dp_dpcd_write(mgr->aux,
+						DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0,
+						&buf,
+						tosend);
+
+			if (ret == tosend)
+				break;
+
+			retries++;
+		} while (retries < 5);
+
+		if (ret != tosend)
+			drm_dbg_kms(mgr->dev, "failed to write dpcd 0x%x\n",
+				    DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0);
+	}
+
 	drm_dp_mst_kick_tx(mgr);
 	return ret;
 }
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index bf80f296a8fd..abec3de38b66 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -3939,6 +3939,9 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
 		if (!memchr_inv(ack, 0, sizeof(ack)))
 			break;
 
+		/* MSG_RDY ack is done in drm*/
+		ack[1] &= ~(DP_DOWN_REP_MSG_RDY | DP_UP_REQ_MSG_RDY);
+
 		if (!intel_dp_ack_sink_irq_esi(intel_dp, ack))
 			drm_dbg_kms(&i915->drm, "Failed to ack ESI\n");
 	}
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index edcb2529b402..e905987104ed 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -1336,6 +1336,8 @@ nv50_mstm_service(struct nouveau_drm *drm,
 		if (!handled)
 			break;
 
+		/* MSG_RDY ack is done in drm*/
+		esi[1] &= ~(DP_DOWN_REP_MSG_RDY | DP_UP_REQ_MSG_RDY);
 		rc = drm_dp_dpcd_write(aux, DP_SINK_COUNT_ESI + 1, &esi[1],
 				       3);
 		if (rc != 3) {
-- 
2.37.3

[Intel-gfx] [PATCH] drm/dp_mst: Clear MSG_RDY flag before sending new message

[Wayne Lin]

[Why & How]
The sequence for collecting down_reply/up_request 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.

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

In drm_dp_mst_hpd_irq(), we don't clear MSG_RDY flag before caliing
drm_dp_mst_kick_tx(). Fix that.

Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
Cc: stable@vger.kernel.org
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  2 ++
 drivers/gpu/drm/display/drm_dp_mst_topology.c | 22 +++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_dp.c       |  3 +++
 drivers/gpu/drm/nouveau/dispnv50/disp.c       |  2 ++
 4 files changed, 29 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 77277d90b6e2..5313a5656598 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3166,6 +3166,8 @@ static void dm_handle_mst_sideband_msg(struct amdgpu_dm_connector *aconnector)
 			for (retry = 0; retry < 3; retry++) {
 				uint8_t wret;
 
+				/* MSG_RDY ack is done in drm*/
+				esi[1] &= ~(DP_DOWN_REP_MSG_RDY | DP_UP_REQ_MSG_RDY);
 				wret = drm_dp_dpcd_write(
 					&aconnector->dm_dp_aux.aux,
 					dpcd_addr + 1,
diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index 51a46689cda7..02aad713c67c 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -4054,6 +4054,9 @@ int drm_dp_mst_hpd_irq(struct drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl
 {
 	int ret = 0;
 	int sc;
+	const int tosend = 1;
+	int retries = 0;
+	u8 buf = 0;
 	*handled = false;
 	sc = DP_GET_SINK_COUNT(esi[0]);
 
@@ -4072,6 +4075,25 @@ int drm_dp_mst_hpd_irq(struct drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl
 		*handled = true;
 	}
 
+	if (*handled) {
+		buf = esi[1] & (DP_DOWN_REP_MSG_RDY | DP_UP_REQ_MSG_RDY);
+		do {
+			ret = drm_dp_dpcd_write(mgr->aux,
+						DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0,
+						&buf,
+						tosend);
+
+			if (ret == tosend)
+				break;
+
+			retries++;
+		} while (retries < 5);
+
+		if (ret != tosend)
+			drm_dbg_kms(mgr->dev, "failed to write dpcd 0x%x\n",
+				    DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0);
+	}
+
 	drm_dp_mst_kick_tx(mgr);
 	return ret;
 }
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index bf80f296a8fd..abec3de38b66 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -3939,6 +3939,9 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
 		if (!memchr_inv(ack, 0, sizeof(ack)))
 			break;
 
+		/* MSG_RDY ack is done in drm*/
+		ack[1] &= ~(DP_DOWN_REP_MSG_RDY | DP_UP_REQ_MSG_RDY);
+
 		if (!intel_dp_ack_sink_irq_esi(intel_dp, ack))
 			drm_dbg_kms(&i915->drm, "Failed to ack ESI\n");
 	}
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index edcb2529b402..e905987104ed 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -1336,6 +1336,8 @@ nv50_mstm_service(struct nouveau_drm *drm,
 		if (!handled)
 			break;
 
+		/* MSG_RDY ack is done in drm*/
+		esi[1] &= ~(DP_DOWN_REP_MSG_RDY | DP_UP_REQ_MSG_RDY);
 		rc = drm_dp_dpcd_write(aux, DP_SINK_COUNT_ESI + 1, &esi[1],
 				       3);
 		if (rc != 3) {
-- 
2.37.3

[Intel-gfx] ✗ Fi.CI.BAT: failure for drm/dp_mst: Clear MSG_RDY flag before sending new message

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 3474 bytes --]

== Series Details ==

Series: drm/dp_mst: Clear MSG_RDY flag before sending new message
URL   : https://patchwork.freedesktop.org/series/116623/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_13023 -> Patchwork_116623v1
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_116623v1 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_116623v1, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v1/index.html

Participating hosts (38 -> 36)
------------------------------

  Missing    (2): fi-kbl-soraka fi-snb-2520m 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_116623v1:

### IGT changes ###

#### Possible regressions ####

  * igt@gem_exec_suspend@basic-s0@smem:
    - fi-cfl-8700k:       [PASS][1] -> [ABORT][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13023/fi-cfl-8700k/igt@gem_exec_suspend@basic-s0@smem.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v1/fi-cfl-8700k/igt@gem_exec_suspend@basic-s0@smem.html

  
Known issues
------------

  Here are the changes found in Patchwork_116623v1 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live@gt_pm:
    - bat-rpls-2:         [PASS][3] -> [DMESG-FAIL][4] ([i915#4258])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13023/bat-rpls-2/igt@i915_selftest@live@gt_pm.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v1/bat-rpls-2/igt@i915_selftest@live@gt_pm.html

  * igt@i915_selftest@live@requests:
    - bat-rpls-1:         [PASS][5] -> [ABORT][6] ([i915#7911] / [i915#7982])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13023/bat-rpls-1/igt@i915_selftest@live@requests.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v1/bat-rpls-1/igt@i915_selftest@live@requests.html

  * igt@i915_selftest@live@reset:
    - bat-rpls-2:         [PASS][7] -> [ABORT][8] ([i915#4983] / [i915#7913])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13023/bat-rpls-2/igt@i915_selftest@live@reset.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v1/bat-rpls-2/igt@i915_selftest@live@reset.html

  
  [i915#4258]: https://gitlab.freedesktop.org/drm/intel/issues/4258
  [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
  [i915#7911]: https://gitlab.freedesktop.org/drm/intel/issues/7911
  [i915#7913]: https://gitlab.freedesktop.org/drm/intel/issues/7913
  [i915#7982]: https://gitlab.freedesktop.org/drm/intel/issues/7982


Build changes
-------------

  * Linux: CI_DRM_13023 -> Patchwork_116623v1

  CI-20190529: 20190529
  CI_DRM_13023: 0c6c8e560ac78d000179e195e1280cc3479ebd58 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7258: ad2eb276eda849b7a7985229009a816c7608186c @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_116623v1: 0c6c8e560ac78d000179e195e1280cc3479ebd58 @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

9c6a56132f18 drm/dp_mst: Clear MSG_RDY flag before sending new message

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v1/index.html

[-- Attachment #2: Type: text/html, Size: 4171 bytes --]

Re: [PATCH] drm/dp_mst: Clear MSG_RDY flag before sending new message

[Jani Nikula]

On Tue, 18 Apr 2023, Wayne Lin <Wayne.Lin@amd.com> wrote:
> [Why & How]
> The sequence for collecting down_reply/up_request 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.
>
> 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 ack 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.
>
> In drm_dp_mst_hpd_irq(), we don't clear MSG_RDY flag before caliing
> drm_dp_mst_kick_tx(). Fix that.
>
> Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
> Cc: stable@vger.kernel.org
> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  2 ++
>  drivers/gpu/drm/display/drm_dp_mst_topology.c | 22 +++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_dp.c       |  3 +++
>  drivers/gpu/drm/nouveau/dispnv50/disp.c       |  2 ++
>  4 files changed, 29 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 77277d90b6e2..5313a5656598 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -3166,6 +3166,8 @@ static void dm_handle_mst_sideband_msg(struct amdgpu_dm_connector *aconnector)
>  			for (retry = 0; retry < 3; retry++) {
>  				uint8_t wret;
>  
> +				/* MSG_RDY ack is done in drm*/
> +				esi[1] &= ~(DP_DOWN_REP_MSG_RDY | DP_UP_REQ_MSG_RDY);

Why do the masking within the retry loop?

>  				wret = drm_dp_dpcd_write(
>  					&aconnector->dm_dp_aux.aux,
>  					dpcd_addr + 1,
> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> index 51a46689cda7..02aad713c67c 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -4054,6 +4054,9 @@ int drm_dp_mst_hpd_irq(struct drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl
>  {
>  	int ret = 0;
>  	int sc;
> +	const int tosend = 1;
> +	int retries = 0;
> +	u8 buf = 0;

All of these should be in tighter scope.

>  	*handled = false;
>  	sc = DP_GET_SINK_COUNT(esi[0]);
>  
> @@ -4072,6 +4075,25 @@ int drm_dp_mst_hpd_irq(struct drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl
>  		*handled = true;
>  	}
>  
> +	if (*handled) {

That should check for DP_DOWN_REP_MSG_RDY and DP_UP_REQ_MSG_RDY only,
right? If those are not set, we didn't do anything with them, and should
not ack.

> +		buf = esi[1] & (DP_DOWN_REP_MSG_RDY | DP_UP_REQ_MSG_RDY);
> +		do {
> +			ret = drm_dp_dpcd_write(mgr->aux,
> +						DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0,
> +						&buf,
> +						tosend);

We should probably have a helper function to do the acking, similar to
intel_dp_ack_sink_irq_esi(), which could be used both by this function
and the drivers.

> +
> +			if (ret == tosend)
> +				break;
> +
> +			retries++;
> +		} while (retries < 5);

Please don't use a do-while when a for loop is sufficient.

	for (tries = 0; tries < 5; tries++)

and it's obvious at a glance how many times at most this runs. Not so
with a do-while where you count *re-tries*. Again, would be nice to
abstract this away in a helper function.

> +
> +		if (ret != tosend)
> +			drm_dbg_kms(mgr->dev, "failed to write dpcd 0x%x\n",
> +				    DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0);
> +	}
> +
>  	drm_dp_mst_kick_tx(mgr);
>  	return ret;
>  }
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index bf80f296a8fd..abec3de38b66 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -3939,6 +3939,9 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
>  		if (!memchr_inv(ack, 0, sizeof(ack)))
>  			break;
>  
> +		/* MSG_RDY ack is done in drm*/
> +		ack[1] &= ~(DP_DOWN_REP_MSG_RDY | DP_UP_REQ_MSG_RDY);

Above we check if there's anything to ack and bail out, and now this
clears the bits but writes them anyway.

I think the handled parameter was problematic before, but now it's even
more convoluted. What does it indicate? It used to mean you need to ack
if it's set, but now it's something different. This function is getting
very difficult to use correctly.

BR,
Jani.



> +
>  		if (!intel_dp_ack_sink_irq_esi(intel_dp, ack))
>  			drm_dbg_kms(&i915->drm, "Failed to ack ESI\n");
>  	}
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> index edcb2529b402..e905987104ed 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> @@ -1336,6 +1336,8 @@ nv50_mstm_service(struct nouveau_drm *drm,
>  		if (!handled)
>  			break;
>  
> +		/* MSG_RDY ack is done in drm*/
> +		esi[1] &= ~(DP_DOWN_REP_MSG_RDY | DP_UP_REQ_MSG_RDY);
>  		rc = drm_dp_dpcd_write(aux, DP_SINK_COUNT_ESI + 1, &esi[1],
>  				       3);

Same here, this acks even if it's already been acked.

>  		if (rc != 3) {

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH] drm/dp_mst: Clear MSG_RDY flag before sending new message

[Jani Nikula]

On Tue, 18 Apr 2023, Wayne Lin <Wayne.Lin@amd.com> wrote:
> [Why & How]
> The sequence for collecting down_reply/up_request 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.
>
> 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 ack 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.
>
> In drm_dp_mst_hpd_irq(), we don't clear MSG_RDY flag before caliing
> drm_dp_mst_kick_tx(). Fix that.
>
> Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
> Cc: stable@vger.kernel.org
> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  2 ++
>  drivers/gpu/drm/display/drm_dp_mst_topology.c | 22 +++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_dp.c       |  3 +++
>  drivers/gpu/drm/nouveau/dispnv50/disp.c       |  2 ++
>  4 files changed, 29 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 77277d90b6e2..5313a5656598 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -3166,6 +3166,8 @@ static void dm_handle_mst_sideband_msg(struct amdgpu_dm_connector *aconnector)
>  			for (retry = 0; retry < 3; retry++) {
>  				uint8_t wret;
>  
> +				/* MSG_RDY ack is done in drm*/
> +				esi[1] &= ~(DP_DOWN_REP_MSG_RDY | DP_UP_REQ_MSG_RDY);

Why do the masking within the retry loop?

>  				wret = drm_dp_dpcd_write(
>  					&aconnector->dm_dp_aux.aux,
>  					dpcd_addr + 1,
> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> index 51a46689cda7..02aad713c67c 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -4054,6 +4054,9 @@ int drm_dp_mst_hpd_irq(struct drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl
>  {
>  	int ret = 0;
>  	int sc;
> +	const int tosend = 1;
> +	int retries = 0;
> +	u8 buf = 0;

All of these should be in tighter scope.

>  	*handled = false;
>  	sc = DP_GET_SINK_COUNT(esi[0]);
>  
> @@ -4072,6 +4075,25 @@ int drm_dp_mst_hpd_irq(struct drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl
>  		*handled = true;
>  	}
>  
> +	if (*handled) {

That should check for DP_DOWN_REP_MSG_RDY and DP_UP_REQ_MSG_RDY only,
right? If those are not set, we didn't do anything with them, and should
not ack.

> +		buf = esi[1] & (DP_DOWN_REP_MSG_RDY | DP_UP_REQ_MSG_RDY);
> +		do {
> +			ret = drm_dp_dpcd_write(mgr->aux,
> +						DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0,
> +						&buf,
> +						tosend);

We should probably have a helper function to do the acking, similar to
intel_dp_ack_sink_irq_esi(), which could be used both by this function
and the drivers.

> +
> +			if (ret == tosend)
> +				break;
> +
> +			retries++;
> +		} while (retries < 5);

Please don't use a do-while when a for loop is sufficient.

	for (tries = 0; tries < 5; tries++)

and it's obvious at a glance how many times at most this runs. Not so
with a do-while where you count *re-tries*. Again, would be nice to
abstract this away in a helper function.

> +
> +		if (ret != tosend)
> +			drm_dbg_kms(mgr->dev, "failed to write dpcd 0x%x\n",
> +				    DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0);
> +	}
> +
>  	drm_dp_mst_kick_tx(mgr);
>  	return ret;
>  }
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index bf80f296a8fd..abec3de38b66 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -3939,6 +3939,9 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
>  		if (!memchr_inv(ack, 0, sizeof(ack)))
>  			break;
>  
> +		/* MSG_RDY ack is done in drm*/
> +		ack[1] &= ~(DP_DOWN_REP_MSG_RDY | DP_UP_REQ_MSG_RDY);

Above we check if there's anything to ack and bail out, and now this
clears the bits but writes them anyway.

I think the handled parameter was problematic before, but now it's even
more convoluted. What does it indicate? It used to mean you need to ack
if it's set, but now it's something different. This function is getting
very difficult to use correctly.

BR,
Jani.



> +
>  		if (!intel_dp_ack_sink_irq_esi(intel_dp, ack))
>  			drm_dbg_kms(&i915->drm, "Failed to ack ESI\n");
>  	}
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> index edcb2529b402..e905987104ed 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> @@ -1336,6 +1336,8 @@ nv50_mstm_service(struct nouveau_drm *drm,
>  		if (!handled)
>  			break;
>  
> +		/* MSG_RDY ack is done in drm*/
> +		esi[1] &= ~(DP_DOWN_REP_MSG_RDY | DP_UP_REQ_MSG_RDY);
>  		rc = drm_dp_dpcd_write(aux, DP_SINK_COUNT_ESI + 1, &esi[1],
>  				       3);

Same here, this acks even if it's already been acked.

>  		if (rc != 3) {

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH] drm/dp_mst: Clear MSG_RDY flag before sending new message

[Jani Nikula]

On Tue, 18 Apr 2023, Wayne Lin <Wayne.Lin@amd.com> wrote:
> [Why & How]
> The sequence for collecting down_reply/up_request 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.
>
> 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 ack 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.
>
> In drm_dp_mst_hpd_irq(), we don't clear MSG_RDY flag before caliing
> drm_dp_mst_kick_tx(). Fix that.
>
> Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
> Cc: stable@vger.kernel.org
> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  2 ++
>  drivers/gpu/drm/display/drm_dp_mst_topology.c | 22 +++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_dp.c       |  3 +++
>  drivers/gpu/drm/nouveau/dispnv50/disp.c       |  2 ++
>  4 files changed, 29 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 77277d90b6e2..5313a5656598 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -3166,6 +3166,8 @@ static void dm_handle_mst_sideband_msg(struct amdgpu_dm_connector *aconnector)
>  			for (retry = 0; retry < 3; retry++) {
>  				uint8_t wret;
>  
> +				/* MSG_RDY ack is done in drm*/
> +				esi[1] &= ~(DP_DOWN_REP_MSG_RDY | DP_UP_REQ_MSG_RDY);

Why do the masking within the retry loop?

>  				wret = drm_dp_dpcd_write(
>  					&aconnector->dm_dp_aux.aux,
>  					dpcd_addr + 1,
> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> index 51a46689cda7..02aad713c67c 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -4054,6 +4054,9 @@ int drm_dp_mst_hpd_irq(struct drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl
>  {
>  	int ret = 0;
>  	int sc;
> +	const int tosend = 1;
> +	int retries = 0;
> +	u8 buf = 0;

All of these should be in tighter scope.

>  	*handled = false;
>  	sc = DP_GET_SINK_COUNT(esi[0]);
>  
> @@ -4072,6 +4075,25 @@ int drm_dp_mst_hpd_irq(struct drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl
>  		*handled = true;
>  	}
>  
> +	if (*handled) {

That should check for DP_DOWN_REP_MSG_RDY and DP_UP_REQ_MSG_RDY only,
right? If those are not set, we didn't do anything with them, and should
not ack.

> +		buf = esi[1] & (DP_DOWN_REP_MSG_RDY | DP_UP_REQ_MSG_RDY);
> +		do {
> +			ret = drm_dp_dpcd_write(mgr->aux,
> +						DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0,
> +						&buf,
> +						tosend);

We should probably have a helper function to do the acking, similar to
intel_dp_ack_sink_irq_esi(), which could be used both by this function
and the drivers.

> +
> +			if (ret == tosend)
> +				break;
> +
> +			retries++;
> +		} while (retries < 5);

Please don't use a do-while when a for loop is sufficient.

	for (tries = 0; tries < 5; tries++)

and it's obvious at a glance how many times at most this runs. Not so
with a do-while where you count *re-tries*. Again, would be nice to
abstract this away in a helper function.

> +
> +		if (ret != tosend)
> +			drm_dbg_kms(mgr->dev, "failed to write dpcd 0x%x\n",
> +				    DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0);
> +	}
> +
>  	drm_dp_mst_kick_tx(mgr);
>  	return ret;
>  }
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index bf80f296a8fd..abec3de38b66 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -3939,6 +3939,9 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
>  		if (!memchr_inv(ack, 0, sizeof(ack)))
>  			break;
>  
> +		/* MSG_RDY ack is done in drm*/
> +		ack[1] &= ~(DP_DOWN_REP_MSG_RDY | DP_UP_REQ_MSG_RDY);

Above we check if there's anything to ack and bail out, and now this
clears the bits but writes them anyway.

I think the handled parameter was problematic before, but now it's even
more convoluted. What does it indicate? It used to mean you need to ack
if it's set, but now it's something different. This function is getting
very difficult to use correctly.

BR,
Jani.



> +
>  		if (!intel_dp_ack_sink_irq_esi(intel_dp, ack))
>  			drm_dbg_kms(&i915->drm, "Failed to ack ESI\n");
>  	}
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> index edcb2529b402..e905987104ed 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> @@ -1336,6 +1336,8 @@ nv50_mstm_service(struct nouveau_drm *drm,
>  		if (!handled)
>  			break;
>  
> +		/* MSG_RDY ack is done in drm*/
> +		esi[1] &= ~(DP_DOWN_REP_MSG_RDY | DP_UP_REQ_MSG_RDY);
>  		rc = drm_dp_dpcd_write(aux, DP_SINK_COUNT_ESI + 1, &esi[1],
>  				       3);

Same here, this acks even if it's already been acked.

>  		if (rc != 3) {

-- 
Jani Nikula, Intel Open Source Graphics Center

RE: [PATCH] drm/dp_mst: Clear MSG_RDY flag before sending new message

[Lin, Wayne]

[Public]

Hi Jani Nikula,

Appreciate your time and feedback! Will adjust the patch.
Some comments inline.

> -----Original Message-----
> From: Jani Nikula <jani.nikula@intel.com>
> Sent: Tuesday, April 18, 2023 4:53 PM
> To: Lin, Wayne <Wayne.Lin@amd.com>; dri-devel@lists.freedesktop.org;
> amd-gfx@lists.freedesktop.org
> Cc: lyude@redhat.com; imre.deak@intel.com; ville.syrjala@linux.intel.com;
> Wentland, Harry <Harry.Wentland@amd.com>; Zuo, Jerry
> <Jerry.Zuo@amd.com>; Lin, Wayne <Wayne.Lin@amd.com>;
> stable@vger.kernel.org
> Subject: Re: [PATCH] drm/dp_mst: Clear MSG_RDY flag before sending new
> message
> 
> On Tue, 18 Apr 2023, Wayne Lin <Wayne.Lin@amd.com> wrote:
> > [Why & How]
> > The sequence for collecting down_reply/up_request 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.
> >
> > 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 ack 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.
> >
> > In drm_dp_mst_hpd_irq(), we don't clear MSG_RDY flag before caliing
> > drm_dp_mst_kick_tx(). Fix that.
> >
> > Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
> > Cc: stable@vger.kernel.org
> > ---
> >  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  2 ++
> > drivers/gpu/drm/display/drm_dp_mst_topology.c | 22
> +++++++++++++++++++
> >  drivers/gpu/drm/i915/display/intel_dp.c       |  3 +++
> >  drivers/gpu/drm/nouveau/dispnv50/disp.c       |  2 ++
> >  4 files changed, 29 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > index 77277d90b6e2..5313a5656598 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -3166,6 +3166,8 @@ static void dm_handle_mst_sideband_msg(struct
> amdgpu_dm_connector *aconnector)
> >  			for (retry = 0; retry < 3; retry++) {
> >  				uint8_t wret;
> >
> > +				/* MSG_RDY ack is done in drm*/
> > +				esi[1] &= ~(DP_DOWN_REP_MSG_RDY |
> DP_UP_REQ_MSG_RDY);
> 
> Why do the masking within the retry loop?
> 
> >  				wret = drm_dp_dpcd_write(
> >  					&aconnector->dm_dp_aux.aux,
> >  					dpcd_addr + 1,
> > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > index 51a46689cda7..02aad713c67c 100644
> > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > @@ -4054,6 +4054,9 @@ int drm_dp_mst_hpd_irq(struct
> > drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl  {
> >  	int ret = 0;
> >  	int sc;
> > +	const int tosend = 1;
> > +	int retries = 0;
> > +	u8 buf = 0;
> 
> All of these should be in tighter scope.
> 
> >  	*handled = false;
> >  	sc = DP_GET_SINK_COUNT(esi[0]);
> >
> > @@ -4072,6 +4075,25 @@ int drm_dp_mst_hpd_irq(struct
> drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl
> >  		*handled = true;
> >  	}
> >
> > +	if (*handled) {
> 
> That should check for DP_DOWN_REP_MSG_RDY and
> DP_UP_REQ_MSG_RDY only, right? If those are not set, we didn't do
> anything with them, and should not ack.

Right. I was thinking the sink count change will accompany the CSN
up request message. I'll change it to be more clear. Thanks.
> 
> > +		buf = esi[1] & (DP_DOWN_REP_MSG_RDY |
> DP_UP_REQ_MSG_RDY);
> > +		do {
> > +			ret = drm_dp_dpcd_write(mgr->aux,
> > +
> 	DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0,
> > +						&buf,
> > +						tosend);
> 
> We should probably have a helper function to do the acking, similar to
> intel_dp_ack_sink_irq_esi(), which could be used both by this function and
> the drivers.
> 
> > +
> > +			if (ret == tosend)
> > +				break;
> > +
> > +			retries++;
> > +		} while (retries < 5);
> 
> Please don't use a do-while when a for loop is sufficient.
> 
> 	for (tries = 0; tries < 5; tries++)
> 
> and it's obvious at a glance how many times at most this runs. Not so with a
> do-while where you count *re-tries*. Again, would be nice to abstract this
> away in a helper function.
> 
> > +
> > +		if (ret != tosend)
> > +			drm_dbg_kms(mgr->dev, "failed to write dpcd
> 0x%x\n",
> > +				    DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0);
> > +	}
> > +
> >  	drm_dp_mst_kick_tx(mgr);
> >  	return ret;
> >  }
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index bf80f296a8fd..abec3de38b66 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -3939,6 +3939,9 @@ intel_dp_check_mst_status(struct intel_dp
> *intel_dp)
> >  		if (!memchr_inv(ack, 0, sizeof(ack)))
> >  			break;
> >
> > +		/* MSG_RDY ack is done in drm*/
> > +		ack[1] &= ~(DP_DOWN_REP_MSG_RDY |
> DP_UP_REQ_MSG_RDY);
> 
> Above we check if there's anything to ack and bail out, and now this clears
> the bits but writes them anyway.
> 
> I think the handled parameter was problematic before, but now it's even
> more convoluted. What does it indicate? It used to mean you need to ack if
> it's set, but now it's something different. This function is getting very difficult
> to use correctly.

My plan was to ack message events within drm_dp_mst_hpd_irq() since the
events are handled there. There are still CP_IRQ and LINK_STATUS_CHANGED
events above get handled in intel_dp_check_mst_status(), so I intended to
mask DP_DOWN_REP_MSG_RDY/DP_UP_REQ_MSG_RDY, and ack
CP_IRQ/LINK_STATUS_CHANGED here.
> 
> BR,
> Jani.
> 
> 
> 
> > +
> >  		if (!intel_dp_ack_sink_irq_esi(intel_dp, ack))
> >  			drm_dbg_kms(&i915->drm, "Failed to ack ESI\n");
> >  	}
> > diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > index edcb2529b402..e905987104ed 100644
> > --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > @@ -1336,6 +1336,8 @@ nv50_mstm_service(struct nouveau_drm *drm,
> >  		if (!handled)
> >  			break;
> >
> > +		/* MSG_RDY ack is done in drm*/
> > +		esi[1] &= ~(DP_DOWN_REP_MSG_RDY |
> DP_UP_REQ_MSG_RDY);
> >  		rc = drm_dp_dpcd_write(aux, DP_SINK_COUNT_ESI + 1,
> &esi[1],
> >  				       3);
> 
> Same here, this acks even if it's already been acked.
> 
> >  		if (rc != 3) {
> 
> --
> Jani Nikula, Intel Open Source Graphics Center

--
Regards,
Wayne Lin

RE: [PATCH] drm/dp_mst: Clear MSG_RDY flag before sending new message

[Lin, Wayne]

[Public]

Hi Jani Nikula,

Appreciate your time and feedback! Will adjust the patch.
Some comments inline.

> -----Original Message-----
> From: Jani Nikula <jani.nikula@intel.com>
> Sent: Tuesday, April 18, 2023 4:53 PM
> To: Lin, Wayne <Wayne.Lin@amd.com>; dri-devel@lists.freedesktop.org;
> amd-gfx@lists.freedesktop.org
> Cc: lyude@redhat.com; imre.deak@intel.com; ville.syrjala@linux.intel.com;
> Wentland, Harry <Harry.Wentland@amd.com>; Zuo, Jerry
> <Jerry.Zuo@amd.com>; Lin, Wayne <Wayne.Lin@amd.com>;
> stable@vger.kernel.org
> Subject: Re: [PATCH] drm/dp_mst: Clear MSG_RDY flag before sending new
> message
> 
> On Tue, 18 Apr 2023, Wayne Lin <Wayne.Lin@amd.com> wrote:
> > [Why & How]
> > The sequence for collecting down_reply/up_request 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.
> >
> > 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 ack 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.
> >
> > In drm_dp_mst_hpd_irq(), we don't clear MSG_RDY flag before caliing
> > drm_dp_mst_kick_tx(). Fix that.
> >
> > Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
> > Cc: stable@vger.kernel.org
> > ---
> >  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  2 ++
> > drivers/gpu/drm/display/drm_dp_mst_topology.c | 22
> +++++++++++++++++++
> >  drivers/gpu/drm/i915/display/intel_dp.c       |  3 +++
> >  drivers/gpu/drm/nouveau/dispnv50/disp.c       |  2 ++
> >  4 files changed, 29 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > index 77277d90b6e2..5313a5656598 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -3166,6 +3166,8 @@ static void dm_handle_mst_sideband_msg(struct
> amdgpu_dm_connector *aconnector)
> >  			for (retry = 0; retry < 3; retry++) {
> >  				uint8_t wret;
> >
> > +				/* MSG_RDY ack is done in drm*/
> > +				esi[1] &= ~(DP_DOWN_REP_MSG_RDY |
> DP_UP_REQ_MSG_RDY);
> 
> Why do the masking within the retry loop?
> 
> >  				wret = drm_dp_dpcd_write(
> >  					&aconnector->dm_dp_aux.aux,
> >  					dpcd_addr + 1,
> > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > index 51a46689cda7..02aad713c67c 100644
> > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > @@ -4054,6 +4054,9 @@ int drm_dp_mst_hpd_irq(struct
> > drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl  {
> >  	int ret = 0;
> >  	int sc;
> > +	const int tosend = 1;
> > +	int retries = 0;
> > +	u8 buf = 0;
> 
> All of these should be in tighter scope.
> 
> >  	*handled = false;
> >  	sc = DP_GET_SINK_COUNT(esi[0]);
> >
> > @@ -4072,6 +4075,25 @@ int drm_dp_mst_hpd_irq(struct
> drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl
> >  		*handled = true;
> >  	}
> >
> > +	if (*handled) {
> 
> That should check for DP_DOWN_REP_MSG_RDY and
> DP_UP_REQ_MSG_RDY only, right? If those are not set, we didn't do
> anything with them, and should not ack.

Right. I was thinking the sink count change will accompany the CSN
up request message. I'll change it to be more clear. Thanks.
> 
> > +		buf = esi[1] & (DP_DOWN_REP_MSG_RDY |
> DP_UP_REQ_MSG_RDY);
> > +		do {
> > +			ret = drm_dp_dpcd_write(mgr->aux,
> > +
> 	DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0,
> > +						&buf,
> > +						tosend);
> 
> We should probably have a helper function to do the acking, similar to
> intel_dp_ack_sink_irq_esi(), which could be used both by this function and
> the drivers.
> 
> > +
> > +			if (ret == tosend)
> > +				break;
> > +
> > +			retries++;
> > +		} while (retries < 5);
> 
> Please don't use a do-while when a for loop is sufficient.
> 
> 	for (tries = 0; tries < 5; tries++)
> 
> and it's obvious at a glance how many times at most this runs. Not so with a
> do-while where you count *re-tries*. Again, would be nice to abstract this
> away in a helper function.
> 
> > +
> > +		if (ret != tosend)
> > +			drm_dbg_kms(mgr->dev, "failed to write dpcd
> 0x%x\n",
> > +				    DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0);
> > +	}
> > +
> >  	drm_dp_mst_kick_tx(mgr);
> >  	return ret;
> >  }
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index bf80f296a8fd..abec3de38b66 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -3939,6 +3939,9 @@ intel_dp_check_mst_status(struct intel_dp
> *intel_dp)
> >  		if (!memchr_inv(ack, 0, sizeof(ack)))
> >  			break;
> >
> > +		/* MSG_RDY ack is done in drm*/
> > +		ack[1] &= ~(DP_DOWN_REP_MSG_RDY |
> DP_UP_REQ_MSG_RDY);
> 
> Above we check if there's anything to ack and bail out, and now this clears
> the bits but writes them anyway.
> 
> I think the handled parameter was problematic before, but now it's even
> more convoluted. What does it indicate? It used to mean you need to ack if
> it's set, but now it's something different. This function is getting very difficult
> to use correctly.

My plan was to ack message events within drm_dp_mst_hpd_irq() since the
events are handled there. There are still CP_IRQ and LINK_STATUS_CHANGED
events above get handled in intel_dp_check_mst_status(), so I intended to
mask DP_DOWN_REP_MSG_RDY/DP_UP_REQ_MSG_RDY, and ack
CP_IRQ/LINK_STATUS_CHANGED here.
> 
> BR,
> Jani.
> 
> 
> 
> > +
> >  		if (!intel_dp_ack_sink_irq_esi(intel_dp, ack))
> >  			drm_dbg_kms(&i915->drm, "Failed to ack ESI\n");
> >  	}
> > diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > index edcb2529b402..e905987104ed 100644
> > --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > @@ -1336,6 +1336,8 @@ nv50_mstm_service(struct nouveau_drm *drm,
> >  		if (!handled)
> >  			break;
> >
> > +		/* MSG_RDY ack is done in drm*/
> > +		esi[1] &= ~(DP_DOWN_REP_MSG_RDY |
> DP_UP_REQ_MSG_RDY);
> >  		rc = drm_dp_dpcd_write(aux, DP_SINK_COUNT_ESI + 1,
> &esi[1],
> >  				       3);
> 
> Same here, this acks even if it's already been acked.
> 
> >  		if (rc != 3) {
> 
> --
> Jani Nikula, Intel Open Source Graphics Center

--
Regards,
Wayne Lin

RE: [PATCH] drm/dp_mst: Clear MSG_RDY flag before sending new message

[Lin, Wayne]

[Public]

Hi Jani Nikula,

Appreciate your time and feedback! Will adjust the patch.
Some comments inline.

> -----Original Message-----
> From: Jani Nikula <jani.nikula@intel.com>
> Sent: Tuesday, April 18, 2023 4:53 PM
> To: Lin, Wayne <Wayne.Lin@amd.com>; dri-devel@lists.freedesktop.org;
> amd-gfx@lists.freedesktop.org
> Cc: lyude@redhat.com; imre.deak@intel.com; ville.syrjala@linux.intel.com;
> Wentland, Harry <Harry.Wentland@amd.com>; Zuo, Jerry
> <Jerry.Zuo@amd.com>; Lin, Wayne <Wayne.Lin@amd.com>;
> stable@vger.kernel.org
> Subject: Re: [PATCH] drm/dp_mst: Clear MSG_RDY flag before sending new
> message
> 
> On Tue, 18 Apr 2023, Wayne Lin <Wayne.Lin@amd.com> wrote:
> > [Why & How]
> > The sequence for collecting down_reply/up_request 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.
> >
> > 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 ack 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.
> >
> > In drm_dp_mst_hpd_irq(), we don't clear MSG_RDY flag before caliing
> > drm_dp_mst_kick_tx(). Fix that.
> >
> > Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
> > Cc: stable@vger.kernel.org
> > ---
> >  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  2 ++
> > drivers/gpu/drm/display/drm_dp_mst_topology.c | 22
> +++++++++++++++++++
> >  drivers/gpu/drm/i915/display/intel_dp.c       |  3 +++
> >  drivers/gpu/drm/nouveau/dispnv50/disp.c       |  2 ++
> >  4 files changed, 29 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > index 77277d90b6e2..5313a5656598 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -3166,6 +3166,8 @@ static void dm_handle_mst_sideband_msg(struct
> amdgpu_dm_connector *aconnector)
> >  			for (retry = 0; retry < 3; retry++) {
> >  				uint8_t wret;
> >
> > +				/* MSG_RDY ack is done in drm*/
> > +				esi[1] &= ~(DP_DOWN_REP_MSG_RDY |
> DP_UP_REQ_MSG_RDY);
> 
> Why do the masking within the retry loop?
> 
> >  				wret = drm_dp_dpcd_write(
> >  					&aconnector->dm_dp_aux.aux,
> >  					dpcd_addr + 1,
> > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > index 51a46689cda7..02aad713c67c 100644
> > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > @@ -4054,6 +4054,9 @@ int drm_dp_mst_hpd_irq(struct
> > drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl  {
> >  	int ret = 0;
> >  	int sc;
> > +	const int tosend = 1;
> > +	int retries = 0;
> > +	u8 buf = 0;
> 
> All of these should be in tighter scope.
> 
> >  	*handled = false;
> >  	sc = DP_GET_SINK_COUNT(esi[0]);
> >
> > @@ -4072,6 +4075,25 @@ int drm_dp_mst_hpd_irq(struct
> drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl
> >  		*handled = true;
> >  	}
> >
> > +	if (*handled) {
> 
> That should check for DP_DOWN_REP_MSG_RDY and
> DP_UP_REQ_MSG_RDY only, right? If those are not set, we didn't do
> anything with them, and should not ack.

Right. I was thinking the sink count change will accompany the CSN
up request message. I'll change it to be more clear. Thanks.
> 
> > +		buf = esi[1] & (DP_DOWN_REP_MSG_RDY |
> DP_UP_REQ_MSG_RDY);
> > +		do {
> > +			ret = drm_dp_dpcd_write(mgr->aux,
> > +
> 	DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0,
> > +						&buf,
> > +						tosend);
> 
> We should probably have a helper function to do the acking, similar to
> intel_dp_ack_sink_irq_esi(), which could be used both by this function and
> the drivers.
> 
> > +
> > +			if (ret == tosend)
> > +				break;
> > +
> > +			retries++;
> > +		} while (retries < 5);
> 
> Please don't use a do-while when a for loop is sufficient.
> 
> 	for (tries = 0; tries < 5; tries++)
> 
> and it's obvious at a glance how many times at most this runs. Not so with a
> do-while where you count *re-tries*. Again, would be nice to abstract this
> away in a helper function.
> 
> > +
> > +		if (ret != tosend)
> > +			drm_dbg_kms(mgr->dev, "failed to write dpcd
> 0x%x\n",
> > +				    DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0);
> > +	}
> > +
> >  	drm_dp_mst_kick_tx(mgr);
> >  	return ret;
> >  }
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index bf80f296a8fd..abec3de38b66 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -3939,6 +3939,9 @@ intel_dp_check_mst_status(struct intel_dp
> *intel_dp)
> >  		if (!memchr_inv(ack, 0, sizeof(ack)))
> >  			break;
> >
> > +		/* MSG_RDY ack is done in drm*/
> > +		ack[1] &= ~(DP_DOWN_REP_MSG_RDY |
> DP_UP_REQ_MSG_RDY);
> 
> Above we check if there's anything to ack and bail out, and now this clears
> the bits but writes them anyway.
> 
> I think the handled parameter was problematic before, but now it's even
> more convoluted. What does it indicate? It used to mean you need to ack if
> it's set, but now it's something different. This function is getting very difficult
> to use correctly.

My plan was to ack message events within drm_dp_mst_hpd_irq() since the
events are handled there. There are still CP_IRQ and LINK_STATUS_CHANGED
events above get handled in intel_dp_check_mst_status(), so I intended to
mask DP_DOWN_REP_MSG_RDY/DP_UP_REQ_MSG_RDY, and ack
CP_IRQ/LINK_STATUS_CHANGED here.
> 
> BR,
> Jani.
> 
> 
> 
> > +
> >  		if (!intel_dp_ack_sink_irq_esi(intel_dp, ack))
> >  			drm_dbg_kms(&i915->drm, "Failed to ack ESI\n");
> >  	}
> > diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > index edcb2529b402..e905987104ed 100644
> > --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > @@ -1336,6 +1336,8 @@ nv50_mstm_service(struct nouveau_drm *drm,
> >  		if (!handled)
> >  			break;
> >
> > +		/* MSG_RDY ack is done in drm*/
> > +		esi[1] &= ~(DP_DOWN_REP_MSG_RDY |
> DP_UP_REQ_MSG_RDY);
> >  		rc = drm_dp_dpcd_write(aux, DP_SINK_COUNT_ESI + 1,
> &esi[1],
> >  				       3);
> 
> Same here, this acks even if it's already been acked.
> 
> >  		if (rc != 3) {
> 
> --
> Jani Nikula, Intel Open Source Graphics Center

--
Regards,
Wayne Lin

[Intel-gfx] ✓ Fi.CI.BAT: success for drm/dp_mst: Clear MSG_RDY flag before sending new message (rev2)

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 8921 bytes --]

== Series Details ==

Series: drm/dp_mst: Clear MSG_RDY flag before sending new message (rev2)
URL   : https://patchwork.freedesktop.org/series/116623/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_13025 -> Patchwork_116623v2
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v2/index.html

Participating hosts (37 -> 37)
------------------------------

  Additional (1): bat-adlm-1 
  Missing    (1): fi-snb-2520m 

Known issues
------------

  Here are the changes found in Patchwork_116623v2 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@debugfs_test@basic-hwmon:
    - bat-adlm-1:         NOTRUN -> [SKIP][1] ([i915#7456])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v2/bat-adlm-1/igt@debugfs_test@basic-hwmon.html

  * igt@fbdev@eof:
    - bat-adlm-1:         NOTRUN -> [SKIP][2] ([i915#2582]) +4 similar issues
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v2/bat-adlm-1/igt@fbdev@eof.html

  * igt@gem_exec_suspend@basic-s0@smem:
    - bat-rpls-2:         NOTRUN -> [ABORT][3] ([i915#6687])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v2/bat-rpls-2/igt@gem_exec_suspend@basic-s0@smem.html

  * igt@gem_lmem_swapping@parallel-random-engines:
    - bat-adlm-1:         NOTRUN -> [SKIP][4] ([i915#4613]) +3 similar issues
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v2/bat-adlm-1/igt@gem_lmem_swapping@parallel-random-engines.html

  * igt@gem_tiled_pread_basic:
    - bat-adlm-1:         NOTRUN -> [SKIP][5] ([i915#3282])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v2/bat-adlm-1/igt@gem_tiled_pread_basic.html

  * igt@i915_pm_backlight@basic-brightness:
    - bat-adlm-1:         NOTRUN -> [SKIP][6] ([i915#7561])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v2/bat-adlm-1/igt@i915_pm_backlight@basic-brightness.html

  * igt@i915_selftest@live@mman:
    - bat-rpls-2:         [PASS][7] -> [TIMEOUT][8] ([i915#6794])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13025/bat-rpls-2/igt@i915_selftest@live@mman.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v2/bat-rpls-2/igt@i915_selftest@live@mman.html

  * igt@kms_chamelium_frames@dp-crc-fast:
    - bat-adlm-1:         NOTRUN -> [SKIP][9] ([i915#7828]) +8 similar issues
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v2/bat-adlm-1/igt@kms_chamelium_frames@dp-crc-fast.html

  * igt@kms_chamelium_hpd@common-hpd-after-suspend:
    - bat-jsl-1:          NOTRUN -> [SKIP][10] ([i915#7828])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v2/bat-jsl-1/igt@kms_chamelium_hpd@common-hpd-after-suspend.html
    - bat-rpls-1:         NOTRUN -> [SKIP][11] ([i915#7828])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v2/bat-rpls-1/igt@kms_chamelium_hpd@common-hpd-after-suspend.html

  * igt@kms_cursor_legacy@basic-flip-after-cursor-varying-size:
    - bat-adlm-1:         NOTRUN -> [SKIP][12] ([i915#1845]) +15 similar issues
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v2/bat-adlm-1/igt@kms_cursor_legacy@basic-flip-after-cursor-varying-size.html

  * igt@kms_flip@basic-flip-vs-wf_vblank@c-hdmi-a2:
    - fi-bsw-n3050:       [PASS][13] -> [FAIL][14] ([i915#2122])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13025/fi-bsw-n3050/igt@kms_flip@basic-flip-vs-wf_vblank@c-hdmi-a2.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v2/fi-bsw-n3050/igt@kms_flip@basic-flip-vs-wf_vblank@c-hdmi-a2.html

  * igt@kms_flip@basic-plain-flip:
    - bat-adlm-1:         NOTRUN -> [SKIP][15] ([i915#3637]) +3 similar issues
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v2/bat-adlm-1/igt@kms_flip@basic-plain-flip.html

  * igt@kms_force_connector_basic@force-load-detect:
    - bat-adlm-1:         NOTRUN -> [SKIP][16] ([fdo#109285])
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v2/bat-adlm-1/igt@kms_force_connector_basic@force-load-detect.html

  * igt@kms_frontbuffer_tracking@basic:
    - bat-adlm-1:         NOTRUN -> [SKIP][17] ([i915#1849])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v2/bat-adlm-1/igt@kms_frontbuffer_tracking@basic.html

  * igt@kms_pipe_crc_basic@suspend-read-crc:
    - bat-rpls-1:         NOTRUN -> [SKIP][18] ([i915#1845])
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v2/bat-rpls-1/igt@kms_pipe_crc_basic@suspend-read-crc.html

  * igt@kms_psr@cursor_plane_move:
    - bat-adlm-1:         NOTRUN -> [SKIP][19] ([i915#1072]) +3 similar issues
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v2/bat-adlm-1/igt@kms_psr@cursor_plane_move.html

  * igt@kms_setmode@basic-clone-single-crtc:
    - bat-adlm-1:         NOTRUN -> [SKIP][20] ([i915#3555])
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v2/bat-adlm-1/igt@kms_setmode@basic-clone-single-crtc.html

  * igt@prime_vgem@basic-fence-flip:
    - bat-adlm-1:         NOTRUN -> [SKIP][21] ([i915#1845] / [i915#3708])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v2/bat-adlm-1/igt@prime_vgem@basic-fence-flip.html

  * igt@prime_vgem@basic-write:
    - bat-adlm-1:         NOTRUN -> [SKIP][22] ([i915#3708]) +3 similar issues
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v2/bat-adlm-1/igt@prime_vgem@basic-write.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@gt_heartbeat:
    - fi-apl-guc:         [DMESG-FAIL][23] ([i915#5334]) -> [PASS][24]
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13025/fi-apl-guc/igt@i915_selftest@live@gt_heartbeat.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v2/fi-apl-guc/igt@i915_selftest@live@gt_heartbeat.html

  * igt@i915_selftest@live@migrate:
    - bat-dg2-11:         [DMESG-WARN][25] ([i915#7699]) -> [PASS][26]
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13025/bat-dg2-11/igt@i915_selftest@live@migrate.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v2/bat-dg2-11/igt@i915_selftest@live@migrate.html

  * igt@i915_selftest@live@reset:
    - bat-rpls-1:         [ABORT][27] ([i915#4983]) -> [PASS][28]
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13025/bat-rpls-1/igt@i915_selftest@live@reset.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v2/bat-rpls-1/igt@i915_selftest@live@reset.html

  * igt@i915_suspend@basic-s3-without-i915:
    - bat-rpls-2:         [ABORT][29] ([i915#7978]) -> [PASS][30]
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13025/bat-rpls-2/igt@i915_suspend@basic-s3-without-i915.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v2/bat-rpls-2/igt@i915_suspend@basic-s3-without-i915.html

  
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#1845]: https://gitlab.freedesktop.org/drm/intel/issues/1845
  [i915#1849]: https://gitlab.freedesktop.org/drm/intel/issues/1849
  [i915#2122]: https://gitlab.freedesktop.org/drm/intel/issues/2122
  [i915#2582]: https://gitlab.freedesktop.org/drm/intel/issues/2582
  [i915#3282]: https://gitlab.freedesktop.org/drm/intel/issues/3282
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3637]: https://gitlab.freedesktop.org/drm/intel/issues/3637
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
  [i915#5334]: https://gitlab.freedesktop.org/drm/intel/issues/5334
  [i915#6687]: https://gitlab.freedesktop.org/drm/intel/issues/6687
  [i915#6794]: https://gitlab.freedesktop.org/drm/intel/issues/6794
  [i915#7456]: https://gitlab.freedesktop.org/drm/intel/issues/7456
  [i915#7561]: https://gitlab.freedesktop.org/drm/intel/issues/7561
  [i915#7699]: https://gitlab.freedesktop.org/drm/intel/issues/7699
  [i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828
  [i915#7978]: https://gitlab.freedesktop.org/drm/intel/issues/7978


Build changes
-------------

  * Linux: CI_DRM_13025 -> Patchwork_116623v2

  CI-20190529: 20190529
  CI_DRM_13025: aedb908bbfc13d3d66f2715709f26f02283aef5a @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7258: ad2eb276eda849b7a7985229009a816c7608186c @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_116623v2: aedb908bbfc13d3d66f2715709f26f02283aef5a @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

becb01b3e5ec drm/dp_mst: Clear MSG_RDY flag before sending new message

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v2/index.html

[-- Attachment #2: Type: text/html, Size: 10474 bytes --]

RE: [PATCH] drm/dp_mst: Clear MSG_RDY flag before sending new message

[Jani Nikula]

On Tue, 18 Apr 2023, "Lin, Wayne" <Wayne.Lin@amd.com> wrote:
> [Public]
>
> Hi Jani Nikula,
>
> Appreciate your time and feedback! Will adjust the patch.
> Some comments inline.
>
>> -----Original Message-----
>> From: Jani Nikula <jani.nikula@intel.com>
>> Sent: Tuesday, April 18, 2023 4:53 PM
>> To: Lin, Wayne <Wayne.Lin@amd.com>; dri-devel@lists.freedesktop.org;
>> amd-gfx@lists.freedesktop.org
>> Cc: lyude@redhat.com; imre.deak@intel.com; ville.syrjala@linux.intel.com;
>> Wentland, Harry <Harry.Wentland@amd.com>; Zuo, Jerry
>> <Jerry.Zuo@amd.com>; Lin, Wayne <Wayne.Lin@amd.com>;
>> stable@vger.kernel.org
>> Subject: Re: [PATCH] drm/dp_mst: Clear MSG_RDY flag before sending new
>> message
>> 
>> On Tue, 18 Apr 2023, Wayne Lin <Wayne.Lin@amd.com> wrote:
>> > [Why & How]
>> > The sequence for collecting down_reply/up_request 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.
>> >
>> > 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 ack 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.
>> >
>> > In drm_dp_mst_hpd_irq(), we don't clear MSG_RDY flag before caliing
>> > drm_dp_mst_kick_tx(). Fix that.
>> >
>> > Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
>> > Cc: stable@vger.kernel.org
>> > ---
>> >  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  2 ++
>> > drivers/gpu/drm/display/drm_dp_mst_topology.c | 22
>> +++++++++++++++++++
>> >  drivers/gpu/drm/i915/display/intel_dp.c       |  3 +++
>> >  drivers/gpu/drm/nouveau/dispnv50/disp.c       |  2 ++
>> >  4 files changed, 29 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> > index 77277d90b6e2..5313a5656598 100644
>> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> > @@ -3166,6 +3166,8 @@ static void dm_handle_mst_sideband_msg(struct
>> amdgpu_dm_connector *aconnector)
>> >  			for (retry = 0; retry < 3; retry++) {
>> >  				uint8_t wret;
>> >
>> > +				/* MSG_RDY ack is done in drm*/
>> > +				esi[1] &= ~(DP_DOWN_REP_MSG_RDY |
>> DP_UP_REQ_MSG_RDY);
>> 
>> Why do the masking within the retry loop?
>> 
>> >  				wret = drm_dp_dpcd_write(
>> >  					&aconnector->dm_dp_aux.aux,
>> >  					dpcd_addr + 1,
>> > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c
>> > b/drivers/gpu/drm/display/drm_dp_mst_topology.c
>> > index 51a46689cda7..02aad713c67c 100644
>> > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
>> > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
>> > @@ -4054,6 +4054,9 @@ int drm_dp_mst_hpd_irq(struct
>> > drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl  {
>> >  	int ret = 0;
>> >  	int sc;
>> > +	const int tosend = 1;
>> > +	int retries = 0;
>> > +	u8 buf = 0;
>> 
>> All of these should be in tighter scope.
>> 
>> >  	*handled = false;
>> >  	sc = DP_GET_SINK_COUNT(esi[0]);
>> >
>> > @@ -4072,6 +4075,25 @@ int drm_dp_mst_hpd_irq(struct
>> drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl
>> >  		*handled = true;
>> >  	}
>> >
>> > +	if (*handled) {
>> 
>> That should check for DP_DOWN_REP_MSG_RDY and
>> DP_UP_REQ_MSG_RDY only, right? If those are not set, we didn't do
>> anything with them, and should not ack.
>
> Right. I was thinking the sink count change will accompany the CSN
> up request message. I'll change it to be more clear. Thanks.
>> 
>> > +		buf = esi[1] & (DP_DOWN_REP_MSG_RDY |
>> DP_UP_REQ_MSG_RDY);
>> > +		do {
>> > +			ret = drm_dp_dpcd_write(mgr->aux,
>> > +
>> 	DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0,
>> > +						&buf,
>> > +						tosend);
>> 
>> We should probably have a helper function to do the acking, similar to
>> intel_dp_ack_sink_irq_esi(), which could be used both by this function and
>> the drivers.
>> 
>> > +
>> > +			if (ret == tosend)
>> > +				break;
>> > +
>> > +			retries++;
>> > +		} while (retries < 5);
>> 
>> Please don't use a do-while when a for loop is sufficient.
>> 
>> 	for (tries = 0; tries < 5; tries++)
>> 
>> and it's obvious at a glance how many times at most this runs. Not so with a
>> do-while where you count *re-tries*. Again, would be nice to abstract this
>> away in a helper function.
>> 
>> > +
>> > +		if (ret != tosend)
>> > +			drm_dbg_kms(mgr->dev, "failed to write dpcd
>> 0x%x\n",
>> > +				    DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0);
>> > +	}
>> > +
>> >  	drm_dp_mst_kick_tx(mgr);
>> >  	return ret;
>> >  }
>> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
>> > b/drivers/gpu/drm/i915/display/intel_dp.c
>> > index bf80f296a8fd..abec3de38b66 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> > @@ -3939,6 +3939,9 @@ intel_dp_check_mst_status(struct intel_dp
>> *intel_dp)
>> >  		if (!memchr_inv(ack, 0, sizeof(ack)))
>> >  			break;
>> >
>> > +		/* MSG_RDY ack is done in drm*/
>> > +		ack[1] &= ~(DP_DOWN_REP_MSG_RDY |
>> DP_UP_REQ_MSG_RDY);
>> 
>> Above we check if there's anything to ack and bail out, and now this clears
>> the bits but writes them anyway.
>> 
>> I think the handled parameter was problematic before, but now it's even
>> more convoluted. What does it indicate? It used to mean you need to ack if
>> it's set, but now it's something different. This function is getting very difficult
>> to use correctly.
>
> My plan was to ack message events within drm_dp_mst_hpd_irq() since the
> events are handled there. There are still CP_IRQ and LINK_STATUS_CHANGED
> events above get handled in intel_dp_check_mst_status(), so I intended to
> mask DP_DOWN_REP_MSG_RDY/DP_UP_REQ_MSG_RDY, and ack
> CP_IRQ/LINK_STATUS_CHANGED here.

I get it, but if DP_DOWN_REP_MSG_RDY or DP_UP_REQ_MSG_RDY were the only
events to ack, and they were already acked in drm_dp_mst_hpd_irq(), we
should not do an extra "nop" ack.

The caller of drm_dp_mst_hpd_irq() needs to be able to conveniently
figure out what to ack, and what to not ack. And without duplicating the
logic within drm_dp_mst_hpd_irq().

BR,
Jani.



>> 
>> BR,
>> Jani.
>> 
>> 
>> 
>> > +
>> >  		if (!intel_dp_ack_sink_irq_esi(intel_dp, ack))
>> >  			drm_dbg_kms(&i915->drm, "Failed to ack ESI\n");
>> >  	}
>> > diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c
>> > b/drivers/gpu/drm/nouveau/dispnv50/disp.c
>> > index edcb2529b402..e905987104ed 100644
>> > --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
>> > +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
>> > @@ -1336,6 +1336,8 @@ nv50_mstm_service(struct nouveau_drm *drm,
>> >  		if (!handled)
>> >  			break;
>> >
>> > +		/* MSG_RDY ack is done in drm*/
>> > +		esi[1] &= ~(DP_DOWN_REP_MSG_RDY |
>> DP_UP_REQ_MSG_RDY);
>> >  		rc = drm_dp_dpcd_write(aux, DP_SINK_COUNT_ESI + 1,
>> &esi[1],
>> >  				       3);
>> 
>> Same here, this acks even if it's already been acked.
>> 
>> >  		if (rc != 3) {
>> 
>> --
>> Jani Nikula, Intel Open Source Graphics Center
>
> --
> Regards,
> Wayne Lin

-- 
Jani Nikula, Intel Open Source Graphics Center

RE: [PATCH] drm/dp_mst: Clear MSG_RDY flag before sending new message

[Jani Nikula]

On Tue, 18 Apr 2023, "Lin, Wayne" <Wayne.Lin@amd.com> wrote:
> [Public]
>
> Hi Jani Nikula,
>
> Appreciate your time and feedback! Will adjust the patch.
> Some comments inline.
>
>> -----Original Message-----
>> From: Jani Nikula <jani.nikula@intel.com>
>> Sent: Tuesday, April 18, 2023 4:53 PM
>> To: Lin, Wayne <Wayne.Lin@amd.com>; dri-devel@lists.freedesktop.org;
>> amd-gfx@lists.freedesktop.org
>> Cc: lyude@redhat.com; imre.deak@intel.com; ville.syrjala@linux.intel.com;
>> Wentland, Harry <Harry.Wentland@amd.com>; Zuo, Jerry
>> <Jerry.Zuo@amd.com>; Lin, Wayne <Wayne.Lin@amd.com>;
>> stable@vger.kernel.org
>> Subject: Re: [PATCH] drm/dp_mst: Clear MSG_RDY flag before sending new
>> message
>> 
>> On Tue, 18 Apr 2023, Wayne Lin <Wayne.Lin@amd.com> wrote:
>> > [Why & How]
>> > The sequence for collecting down_reply/up_request 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.
>> >
>> > 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 ack 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.
>> >
>> > In drm_dp_mst_hpd_irq(), we don't clear MSG_RDY flag before caliing
>> > drm_dp_mst_kick_tx(). Fix that.
>> >
>> > Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
>> > Cc: stable@vger.kernel.org
>> > ---
>> >  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  2 ++
>> > drivers/gpu/drm/display/drm_dp_mst_topology.c | 22
>> +++++++++++++++++++
>> >  drivers/gpu/drm/i915/display/intel_dp.c       |  3 +++
>> >  drivers/gpu/drm/nouveau/dispnv50/disp.c       |  2 ++
>> >  4 files changed, 29 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> > index 77277d90b6e2..5313a5656598 100644
>> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> > @@ -3166,6 +3166,8 @@ static void dm_handle_mst_sideband_msg(struct
>> amdgpu_dm_connector *aconnector)
>> >  			for (retry = 0; retry < 3; retry++) {
>> >  				uint8_t wret;
>> >
>> > +				/* MSG_RDY ack is done in drm*/
>> > +				esi[1] &= ~(DP_DOWN_REP_MSG_RDY |
>> DP_UP_REQ_MSG_RDY);
>> 
>> Why do the masking within the retry loop?
>> 
>> >  				wret = drm_dp_dpcd_write(
>> >  					&aconnector->dm_dp_aux.aux,
>> >  					dpcd_addr + 1,
>> > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c
>> > b/drivers/gpu/drm/display/drm_dp_mst_topology.c
>> > index 51a46689cda7..02aad713c67c 100644
>> > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
>> > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
>> > @@ -4054,6 +4054,9 @@ int drm_dp_mst_hpd_irq(struct
>> > drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl  {
>> >  	int ret = 0;
>> >  	int sc;
>> > +	const int tosend = 1;
>> > +	int retries = 0;
>> > +	u8 buf = 0;
>> 
>> All of these should be in tighter scope.
>> 
>> >  	*handled = false;
>> >  	sc = DP_GET_SINK_COUNT(esi[0]);
>> >
>> > @@ -4072,6 +4075,25 @@ int drm_dp_mst_hpd_irq(struct
>> drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl
>> >  		*handled = true;
>> >  	}
>> >
>> > +	if (*handled) {
>> 
>> That should check for DP_DOWN_REP_MSG_RDY and
>> DP_UP_REQ_MSG_RDY only, right? If those are not set, we didn't do
>> anything with them, and should not ack.
>
> Right. I was thinking the sink count change will accompany the CSN
> up request message. I'll change it to be more clear. Thanks.
>> 
>> > +		buf = esi[1] & (DP_DOWN_REP_MSG_RDY |
>> DP_UP_REQ_MSG_RDY);
>> > +		do {
>> > +			ret = drm_dp_dpcd_write(mgr->aux,
>> > +
>> 	DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0,
>> > +						&buf,
>> > +						tosend);
>> 
>> We should probably have a helper function to do the acking, similar to
>> intel_dp_ack_sink_irq_esi(), which could be used both by this function and
>> the drivers.
>> 
>> > +
>> > +			if (ret == tosend)
>> > +				break;
>> > +
>> > +			retries++;
>> > +		} while (retries < 5);
>> 
>> Please don't use a do-while when a for loop is sufficient.
>> 
>> 	for (tries = 0; tries < 5; tries++)
>> 
>> and it's obvious at a glance how many times at most this runs. Not so with a
>> do-while where you count *re-tries*. Again, would be nice to abstract this
>> away in a helper function.
>> 
>> > +
>> > +		if (ret != tosend)
>> > +			drm_dbg_kms(mgr->dev, "failed to write dpcd
>> 0x%x\n",
>> > +				    DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0);
>> > +	}
>> > +
>> >  	drm_dp_mst_kick_tx(mgr);
>> >  	return ret;
>> >  }
>> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
>> > b/drivers/gpu/drm/i915/display/intel_dp.c
>> > index bf80f296a8fd..abec3de38b66 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> > @@ -3939,6 +3939,9 @@ intel_dp_check_mst_status(struct intel_dp
>> *intel_dp)
>> >  		if (!memchr_inv(ack, 0, sizeof(ack)))
>> >  			break;
>> >
>> > +		/* MSG_RDY ack is done in drm*/
>> > +		ack[1] &= ~(DP_DOWN_REP_MSG_RDY |
>> DP_UP_REQ_MSG_RDY);
>> 
>> Above we check if there's anything to ack and bail out, and now this clears
>> the bits but writes them anyway.
>> 
>> I think the handled parameter was problematic before, but now it's even
>> more convoluted. What does it indicate? It used to mean you need to ack if
>> it's set, but now it's something different. This function is getting very difficult
>> to use correctly.
>
> My plan was to ack message events within drm_dp_mst_hpd_irq() since the
> events are handled there. There are still CP_IRQ and LINK_STATUS_CHANGED
> events above get handled in intel_dp_check_mst_status(), so I intended to
> mask DP_DOWN_REP_MSG_RDY/DP_UP_REQ_MSG_RDY, and ack
> CP_IRQ/LINK_STATUS_CHANGED here.

I get it, but if DP_DOWN_REP_MSG_RDY or DP_UP_REQ_MSG_RDY were the only
events to ack, and they were already acked in drm_dp_mst_hpd_irq(), we
should not do an extra "nop" ack.

The caller of drm_dp_mst_hpd_irq() needs to be able to conveniently
figure out what to ack, and what to not ack. And without duplicating the
logic within drm_dp_mst_hpd_irq().

BR,
Jani.



>> 
>> BR,
>> Jani.
>> 
>> 
>> 
>> > +
>> >  		if (!intel_dp_ack_sink_irq_esi(intel_dp, ack))
>> >  			drm_dbg_kms(&i915->drm, "Failed to ack ESI\n");
>> >  	}
>> > diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c
>> > b/drivers/gpu/drm/nouveau/dispnv50/disp.c
>> > index edcb2529b402..e905987104ed 100644
>> > --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
>> > +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
>> > @@ -1336,6 +1336,8 @@ nv50_mstm_service(struct nouveau_drm *drm,
>> >  		if (!handled)
>> >  			break;
>> >
>> > +		/* MSG_RDY ack is done in drm*/
>> > +		esi[1] &= ~(DP_DOWN_REP_MSG_RDY |
>> DP_UP_REQ_MSG_RDY);
>> >  		rc = drm_dp_dpcd_write(aux, DP_SINK_COUNT_ESI + 1,
>> &esi[1],
>> >  				       3);
>> 
>> Same here, this acks even if it's already been acked.
>> 
>> >  		if (rc != 3) {
>> 
>> --
>> Jani Nikula, Intel Open Source Graphics Center
>
> --
> Regards,
> Wayne Lin

-- 
Jani Nikula, Intel Open Source Graphics Center

RE: [PATCH] drm/dp_mst: Clear MSG_RDY flag before sending new message

[Jani Nikula]

On Tue, 18 Apr 2023, "Lin, Wayne" <Wayne.Lin@amd.com> wrote:
> [Public]
>
> Hi Jani Nikula,
>
> Appreciate your time and feedback! Will adjust the patch.
> Some comments inline.
>
>> -----Original Message-----
>> From: Jani Nikula <jani.nikula@intel.com>
>> Sent: Tuesday, April 18, 2023 4:53 PM
>> To: Lin, Wayne <Wayne.Lin@amd.com>; dri-devel@lists.freedesktop.org;
>> amd-gfx@lists.freedesktop.org
>> Cc: lyude@redhat.com; imre.deak@intel.com; ville.syrjala@linux.intel.com;
>> Wentland, Harry <Harry.Wentland@amd.com>; Zuo, Jerry
>> <Jerry.Zuo@amd.com>; Lin, Wayne <Wayne.Lin@amd.com>;
>> stable@vger.kernel.org
>> Subject: Re: [PATCH] drm/dp_mst: Clear MSG_RDY flag before sending new
>> message
>> 
>> On Tue, 18 Apr 2023, Wayne Lin <Wayne.Lin@amd.com> wrote:
>> > [Why & How]
>> > The sequence for collecting down_reply/up_request 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.
>> >
>> > 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 ack 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.
>> >
>> > In drm_dp_mst_hpd_irq(), we don't clear MSG_RDY flag before caliing
>> > drm_dp_mst_kick_tx(). Fix that.
>> >
>> > Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
>> > Cc: stable@vger.kernel.org
>> > ---
>> >  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  2 ++
>> > drivers/gpu/drm/display/drm_dp_mst_topology.c | 22
>> +++++++++++++++++++
>> >  drivers/gpu/drm/i915/display/intel_dp.c       |  3 +++
>> >  drivers/gpu/drm/nouveau/dispnv50/disp.c       |  2 ++
>> >  4 files changed, 29 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> > index 77277d90b6e2..5313a5656598 100644
>> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> > @@ -3166,6 +3166,8 @@ static void dm_handle_mst_sideband_msg(struct
>> amdgpu_dm_connector *aconnector)
>> >  			for (retry = 0; retry < 3; retry++) {
>> >  				uint8_t wret;
>> >
>> > +				/* MSG_RDY ack is done in drm*/
>> > +				esi[1] &= ~(DP_DOWN_REP_MSG_RDY |
>> DP_UP_REQ_MSG_RDY);
>> 
>> Why do the masking within the retry loop?
>> 
>> >  				wret = drm_dp_dpcd_write(
>> >  					&aconnector->dm_dp_aux.aux,
>> >  					dpcd_addr + 1,
>> > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c
>> > b/drivers/gpu/drm/display/drm_dp_mst_topology.c
>> > index 51a46689cda7..02aad713c67c 100644
>> > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
>> > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
>> > @@ -4054,6 +4054,9 @@ int drm_dp_mst_hpd_irq(struct
>> > drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl  {
>> >  	int ret = 0;
>> >  	int sc;
>> > +	const int tosend = 1;
>> > +	int retries = 0;
>> > +	u8 buf = 0;
>> 
>> All of these should be in tighter scope.
>> 
>> >  	*handled = false;
>> >  	sc = DP_GET_SINK_COUNT(esi[0]);
>> >
>> > @@ -4072,6 +4075,25 @@ int drm_dp_mst_hpd_irq(struct
>> drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl
>> >  		*handled = true;
>> >  	}
>> >
>> > +	if (*handled) {
>> 
>> That should check for DP_DOWN_REP_MSG_RDY and
>> DP_UP_REQ_MSG_RDY only, right? If those are not set, we didn't do
>> anything with them, and should not ack.
>
> Right. I was thinking the sink count change will accompany the CSN
> up request message. I'll change it to be more clear. Thanks.
>> 
>> > +		buf = esi[1] & (DP_DOWN_REP_MSG_RDY |
>> DP_UP_REQ_MSG_RDY);
>> > +		do {
>> > +			ret = drm_dp_dpcd_write(mgr->aux,
>> > +
>> 	DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0,
>> > +						&buf,
>> > +						tosend);
>> 
>> We should probably have a helper function to do the acking, similar to
>> intel_dp_ack_sink_irq_esi(), which could be used both by this function and
>> the drivers.
>> 
>> > +
>> > +			if (ret == tosend)
>> > +				break;
>> > +
>> > +			retries++;
>> > +		} while (retries < 5);
>> 
>> Please don't use a do-while when a for loop is sufficient.
>> 
>> 	for (tries = 0; tries < 5; tries++)
>> 
>> and it's obvious at a glance how many times at most this runs. Not so with a
>> do-while where you count *re-tries*. Again, would be nice to abstract this
>> away in a helper function.
>> 
>> > +
>> > +		if (ret != tosend)
>> > +			drm_dbg_kms(mgr->dev, "failed to write dpcd
>> 0x%x\n",
>> > +				    DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0);
>> > +	}
>> > +
>> >  	drm_dp_mst_kick_tx(mgr);
>> >  	return ret;
>> >  }
>> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
>> > b/drivers/gpu/drm/i915/display/intel_dp.c
>> > index bf80f296a8fd..abec3de38b66 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> > @@ -3939,6 +3939,9 @@ intel_dp_check_mst_status(struct intel_dp
>> *intel_dp)
>> >  		if (!memchr_inv(ack, 0, sizeof(ack)))
>> >  			break;
>> >
>> > +		/* MSG_RDY ack is done in drm*/
>> > +		ack[1] &= ~(DP_DOWN_REP_MSG_RDY |
>> DP_UP_REQ_MSG_RDY);
>> 
>> Above we check if there's anything to ack and bail out, and now this clears
>> the bits but writes them anyway.
>> 
>> I think the handled parameter was problematic before, but now it's even
>> more convoluted. What does it indicate? It used to mean you need to ack if
>> it's set, but now it's something different. This function is getting very difficult
>> to use correctly.
>
> My plan was to ack message events within drm_dp_mst_hpd_irq() since the
> events are handled there. There are still CP_IRQ and LINK_STATUS_CHANGED
> events above get handled in intel_dp_check_mst_status(), so I intended to
> mask DP_DOWN_REP_MSG_RDY/DP_UP_REQ_MSG_RDY, and ack
> CP_IRQ/LINK_STATUS_CHANGED here.

I get it, but if DP_DOWN_REP_MSG_RDY or DP_UP_REQ_MSG_RDY were the only
events to ack, and they were already acked in drm_dp_mst_hpd_irq(), we
should not do an extra "nop" ack.

The caller of drm_dp_mst_hpd_irq() needs to be able to conveniently
figure out what to ack, and what to not ack. And without duplicating the
logic within drm_dp_mst_hpd_irq().

BR,
Jani.



>> 
>> BR,
>> Jani.
>> 
>> 
>> 
>> > +
>> >  		if (!intel_dp_ack_sink_irq_esi(intel_dp, ack))
>> >  			drm_dbg_kms(&i915->drm, "Failed to ack ESI\n");
>> >  	}
>> > diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c
>> > b/drivers/gpu/drm/nouveau/dispnv50/disp.c
>> > index edcb2529b402..e905987104ed 100644
>> > --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
>> > +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
>> > @@ -1336,6 +1336,8 @@ nv50_mstm_service(struct nouveau_drm *drm,
>> >  		if (!handled)
>> >  			break;
>> >
>> > +		/* MSG_RDY ack is done in drm*/
>> > +		esi[1] &= ~(DP_DOWN_REP_MSG_RDY |
>> DP_UP_REQ_MSG_RDY);
>> >  		rc = drm_dp_dpcd_write(aux, DP_SINK_COUNT_ESI + 1,
>> &esi[1],
>> >  				       3);
>> 
>> Same here, this acks even if it's already been acked.
>> 
>> >  		if (rc != 3) {
>> 
>> --
>> Jani Nikula, Intel Open Source Graphics Center
>
> --
> Regards,
> Wayne Lin

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH] drm/dp_mst: Clear MSG_RDY flag before sending new message

[Ville Syrjälä]

On Tue, Apr 18, 2023 at 02:09:05PM +0800, Wayne Lin wrote:
> [Why & How]
> The sequence for collecting down_reply/up_request 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.
> 
> 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 ack 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.
> 
> In drm_dp_mst_hpd_irq(), we don't clear MSG_RDY flag before caliing
> drm_dp_mst_kick_tx(). Fix that.
> 
> Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
> Cc: stable@vger.kernel.org
> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  2 ++
>  drivers/gpu/drm/display/drm_dp_mst_topology.c | 22 +++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_dp.c       |  3 +++
>  drivers/gpu/drm/nouveau/dispnv50/disp.c       |  2 ++
>  4 files changed, 29 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 77277d90b6e2..5313a5656598 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -3166,6 +3166,8 @@ static void dm_handle_mst_sideband_msg(struct amdgpu_dm_connector *aconnector)
>  			for (retry = 0; retry < 3; retry++) {
>  				uint8_t wret;
>  
> +				/* MSG_RDY ack is done in drm*/
> +				esi[1] &= ~(DP_DOWN_REP_MSG_RDY | DP_UP_REQ_MSG_RDY);
>  				wret = drm_dp_dpcd_write(
>  					&aconnector->dm_dp_aux.aux,
>  					dpcd_addr + 1,
> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> index 51a46689cda7..02aad713c67c 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -4054,6 +4054,9 @@ int drm_dp_mst_hpd_irq(struct drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl
>  {
>  	int ret = 0;
>  	int sc;
> +	const int tosend = 1;
> +	int retries = 0;
> +	u8 buf = 0;
>  	*handled = false;
>  	sc = DP_GET_SINK_COUNT(esi[0]);
>  
> @@ -4072,6 +4075,25 @@ int drm_dp_mst_hpd_irq(struct drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl
>  		*handled = true;
>  	}
>  
> +	if (*handled) {
> +		buf = esi[1] & (DP_DOWN_REP_MSG_RDY | DP_UP_REQ_MSG_RDY);
> +		do {
> +			ret = drm_dp_dpcd_write(mgr->aux,
> +						DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0,
> +						&buf,
> +						tosend);
> +
> +			if (ret == tosend)
> +				break;
> +
> +			retries++;
> +		} while (retries < 5);

What's with this magic retry loop?

Not sure I like the whole thing though. Splitting the irq ack
semi-randomly between driver vs. multiple helpers doesn't feel
great to me.

As a whole the HPD_IRQ handling is a total mess atm. At some point
I was trying to sketch something a bit better for it. The approach
I was thinking was something along the lines of:

 u8 vector[...];
 drm_dp_read_irq_vector(vector);
 ... handle all irqs/etc., calling suitable helpers as needed
 drm_dp_clear_irq_vector(vector);

And I was also thinking that this drm_dp_*_irq_vector() stuff
would always use the ESI layout, converting as needed from/to
the old layout for pre-1.2 (or whatever the cutoff was) devices.
That way drivers would just need the one codepath.

> +
> +		if (ret != tosend)
> +			drm_dbg_kms(mgr->dev, "failed to write dpcd 0x%x\n",
> +				    DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0);
> +	}
> +
>  	drm_dp_mst_kick_tx(mgr);
>  	return ret;
>  }
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index bf80f296a8fd..abec3de38b66 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -3939,6 +3939,9 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
>  		if (!memchr_inv(ack, 0, sizeof(ack)))
>  			break;
>  
> +		/* MSG_RDY ack is done in drm*/
> +		ack[1] &= ~(DP_DOWN_REP_MSG_RDY | DP_UP_REQ_MSG_RDY);
> +
>  		if (!intel_dp_ack_sink_irq_esi(intel_dp, ack))
>  			drm_dbg_kms(&i915->drm, "Failed to ack ESI\n");
>  	}
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> index edcb2529b402..e905987104ed 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> @@ -1336,6 +1336,8 @@ nv50_mstm_service(struct nouveau_drm *drm,
>  		if (!handled)
>  			break;
>  
> +		/* MSG_RDY ack is done in drm*/
> +		esi[1] &= ~(DP_DOWN_REP_MSG_RDY | DP_UP_REQ_MSG_RDY);
>  		rc = drm_dp_dpcd_write(aux, DP_SINK_COUNT_ESI + 1, &esi[1],
>  				       3);
>  		if (rc != 3) {
> -- 
> 2.37.3

-- 
Ville Syrjälä
Intel

Re: [PATCH] drm/dp_mst: Clear MSG_RDY flag before sending new message

[Ville Syrjälä]

On Tue, Apr 18, 2023 at 02:09:05PM +0800, Wayne Lin wrote:
> [Why & How]
> The sequence for collecting down_reply/up_request 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.
> 
> 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 ack 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.
> 
> In drm_dp_mst_hpd_irq(), we don't clear MSG_RDY flag before caliing
> drm_dp_mst_kick_tx(). Fix that.
> 
> Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
> Cc: stable@vger.kernel.org
> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  2 ++
>  drivers/gpu/drm/display/drm_dp_mst_topology.c | 22 +++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_dp.c       |  3 +++
>  drivers/gpu/drm/nouveau/dispnv50/disp.c       |  2 ++
>  4 files changed, 29 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 77277d90b6e2..5313a5656598 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -3166,6 +3166,8 @@ static void dm_handle_mst_sideband_msg(struct amdgpu_dm_connector *aconnector)
>  			for (retry = 0; retry < 3; retry++) {
>  				uint8_t wret;
>  
> +				/* MSG_RDY ack is done in drm*/
> +				esi[1] &= ~(DP_DOWN_REP_MSG_RDY | DP_UP_REQ_MSG_RDY);
>  				wret = drm_dp_dpcd_write(
>  					&aconnector->dm_dp_aux.aux,
>  					dpcd_addr + 1,
> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> index 51a46689cda7..02aad713c67c 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -4054,6 +4054,9 @@ int drm_dp_mst_hpd_irq(struct drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl
>  {
>  	int ret = 0;
>  	int sc;
> +	const int tosend = 1;
> +	int retries = 0;
> +	u8 buf = 0;
>  	*handled = false;
>  	sc = DP_GET_SINK_COUNT(esi[0]);
>  
> @@ -4072,6 +4075,25 @@ int drm_dp_mst_hpd_irq(struct drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl
>  		*handled = true;
>  	}
>  
> +	if (*handled) {
> +		buf = esi[1] & (DP_DOWN_REP_MSG_RDY | DP_UP_REQ_MSG_RDY);
> +		do {
> +			ret = drm_dp_dpcd_write(mgr->aux,
> +						DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0,
> +						&buf,
> +						tosend);
> +
> +			if (ret == tosend)
> +				break;
> +
> +			retries++;
> +		} while (retries < 5);

What's with this magic retry loop?

Not sure I like the whole thing though. Splitting the irq ack
semi-randomly between driver vs. multiple helpers doesn't feel
great to me.

As a whole the HPD_IRQ handling is a total mess atm. At some point
I was trying to sketch something a bit better for it. The approach
I was thinking was something along the lines of:

 u8 vector[...];
 drm_dp_read_irq_vector(vector);
 ... handle all irqs/etc., calling suitable helpers as needed
 drm_dp_clear_irq_vector(vector);

And I was also thinking that this drm_dp_*_irq_vector() stuff
would always use the ESI layout, converting as needed from/to
the old layout for pre-1.2 (or whatever the cutoff was) devices.
That way drivers would just need the one codepath.

> +
> +		if (ret != tosend)
> +			drm_dbg_kms(mgr->dev, "failed to write dpcd 0x%x\n",
> +				    DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0);
> +	}
> +
>  	drm_dp_mst_kick_tx(mgr);
>  	return ret;
>  }
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index bf80f296a8fd..abec3de38b66 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -3939,6 +3939,9 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
>  		if (!memchr_inv(ack, 0, sizeof(ack)))
>  			break;
>  
> +		/* MSG_RDY ack is done in drm*/
> +		ack[1] &= ~(DP_DOWN_REP_MSG_RDY | DP_UP_REQ_MSG_RDY);
> +
>  		if (!intel_dp_ack_sink_irq_esi(intel_dp, ack))
>  			drm_dbg_kms(&i915->drm, "Failed to ack ESI\n");
>  	}
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> index edcb2529b402..e905987104ed 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> @@ -1336,6 +1336,8 @@ nv50_mstm_service(struct nouveau_drm *drm,
>  		if (!handled)
>  			break;
>  
> +		/* MSG_RDY ack is done in drm*/
> +		esi[1] &= ~(DP_DOWN_REP_MSG_RDY | DP_UP_REQ_MSG_RDY);
>  		rc = drm_dp_dpcd_write(aux, DP_SINK_COUNT_ESI + 1, &esi[1],
>  				       3);
>  		if (rc != 3) {
> -- 
> 2.37.3

-- 
Ville Syrjälä
Intel

Re: [PATCH] drm/dp_mst: Clear MSG_RDY flag before sending new message

[Ville Syrjälä]

On Tue, Apr 18, 2023 at 02:09:05PM +0800, Wayne Lin wrote:
> [Why & How]
> The sequence for collecting down_reply/up_request 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.
> 
> 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 ack 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.
> 
> In drm_dp_mst_hpd_irq(), we don't clear MSG_RDY flag before caliing
> drm_dp_mst_kick_tx(). Fix that.
> 
> Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
> Cc: stable@vger.kernel.org
> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  2 ++
>  drivers/gpu/drm/display/drm_dp_mst_topology.c | 22 +++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_dp.c       |  3 +++
>  drivers/gpu/drm/nouveau/dispnv50/disp.c       |  2 ++
>  4 files changed, 29 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 77277d90b6e2..5313a5656598 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -3166,6 +3166,8 @@ static void dm_handle_mst_sideband_msg(struct amdgpu_dm_connector *aconnector)
>  			for (retry = 0; retry < 3; retry++) {
>  				uint8_t wret;
>  
> +				/* MSG_RDY ack is done in drm*/
> +				esi[1] &= ~(DP_DOWN_REP_MSG_RDY | DP_UP_REQ_MSG_RDY);
>  				wret = drm_dp_dpcd_write(
>  					&aconnector->dm_dp_aux.aux,
>  					dpcd_addr + 1,
> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> index 51a46689cda7..02aad713c67c 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -4054,6 +4054,9 @@ int drm_dp_mst_hpd_irq(struct drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl
>  {
>  	int ret = 0;
>  	int sc;
> +	const int tosend = 1;
> +	int retries = 0;
> +	u8 buf = 0;
>  	*handled = false;
>  	sc = DP_GET_SINK_COUNT(esi[0]);
>  
> @@ -4072,6 +4075,25 @@ int drm_dp_mst_hpd_irq(struct drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl
>  		*handled = true;
>  	}
>  
> +	if (*handled) {
> +		buf = esi[1] & (DP_DOWN_REP_MSG_RDY | DP_UP_REQ_MSG_RDY);
> +		do {
> +			ret = drm_dp_dpcd_write(mgr->aux,
> +						DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0,
> +						&buf,
> +						tosend);
> +
> +			if (ret == tosend)
> +				break;
> +
> +			retries++;
> +		} while (retries < 5);

What's with this magic retry loop?

Not sure I like the whole thing though. Splitting the irq ack
semi-randomly between driver vs. multiple helpers doesn't feel
great to me.

As a whole the HPD_IRQ handling is a total mess atm. At some point
I was trying to sketch something a bit better for it. The approach
I was thinking was something along the lines of:

 u8 vector[...];
 drm_dp_read_irq_vector(vector);
 ... handle all irqs/etc., calling suitable helpers as needed
 drm_dp_clear_irq_vector(vector);

And I was also thinking that this drm_dp_*_irq_vector() stuff
would always use the ESI layout, converting as needed from/to
the old layout for pre-1.2 (or whatever the cutoff was) devices.
That way drivers would just need the one codepath.

> +
> +		if (ret != tosend)
> +			drm_dbg_kms(mgr->dev, "failed to write dpcd 0x%x\n",
> +				    DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0);
> +	}
> +
>  	drm_dp_mst_kick_tx(mgr);
>  	return ret;
>  }
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index bf80f296a8fd..abec3de38b66 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -3939,6 +3939,9 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
>  		if (!memchr_inv(ack, 0, sizeof(ack)))
>  			break;
>  
> +		/* MSG_RDY ack is done in drm*/
> +		ack[1] &= ~(DP_DOWN_REP_MSG_RDY | DP_UP_REQ_MSG_RDY);
> +
>  		if (!intel_dp_ack_sink_irq_esi(intel_dp, ack))
>  			drm_dbg_kms(&i915->drm, "Failed to ack ESI\n");
>  	}
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> index edcb2529b402..e905987104ed 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> @@ -1336,6 +1336,8 @@ nv50_mstm_service(struct nouveau_drm *drm,
>  		if (!handled)
>  			break;
>  
> +		/* MSG_RDY ack is done in drm*/
> +		esi[1] &= ~(DP_DOWN_REP_MSG_RDY | DP_UP_REQ_MSG_RDY);
>  		rc = drm_dp_dpcd_write(aux, DP_SINK_COUNT_ESI + 1, &esi[1],
>  				       3);
>  		if (rc != 3) {
> -- 
> 2.37.3

-- 
Ville Syrjälä
Intel

[Intel-gfx] ✓ Fi.CI.IGT: success for drm/dp_mst: Clear MSG_RDY flag before sending new message (rev2)

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 12547 bytes --]

== Series Details ==

Series: drm/dp_mst: Clear MSG_RDY flag before sending new message (rev2)
URL   : https://patchwork.freedesktop.org/series/116623/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_13025_full -> Patchwork_116623v2_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

Participating hosts (7 -> 7)
------------------------------

  No changes in participating hosts

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_116623v2_full:

### IGT changes ###

#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@kms_plane_lowres@tiling-y@pipe-a-hdmi-a-2:
    - {shard-rkl}:        [PASS][1] -> [ABORT][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13025/shard-rkl-3/igt@kms_plane_lowres@tiling-y@pipe-a-hdmi-a-2.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v2/shard-rkl-6/igt@kms_plane_lowres@tiling-y@pipe-a-hdmi-a-2.html

  
Known issues
------------

  Here are the changes found in Patchwork_116623v2_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_fair@basic-pace-solo@rcs0:
    - shard-apl:          [PASS][3] -> [FAIL][4] ([i915#2842])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13025/shard-apl3/igt@gem_exec_fair@basic-pace-solo@rcs0.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v2/shard-apl1/igt@gem_exec_fair@basic-pace-solo@rcs0.html

  * igt@i915_pm_rps@reset:
    - shard-snb:          [PASS][5] -> [DMESG-FAIL][6] ([i915#8319])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13025/shard-snb7/igt@i915_pm_rps@reset.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v2/shard-snb7/igt@i915_pm_rps@reset.html

  * igt@kms_cursor_legacy@2x-long-flip-vs-cursor-atomic:
    - shard-glk:          [PASS][7] -> [FAIL][8] ([i915#72])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13025/shard-glk2/igt@kms_cursor_legacy@2x-long-flip-vs-cursor-atomic.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v2/shard-glk4/igt@kms_cursor_legacy@2x-long-flip-vs-cursor-atomic.html

  * igt@kms_plane_scaling@planes-downscale-factor-0-5-unity-scaling@pipe-b-vga-1:
    - shard-snb:          NOTRUN -> [SKIP][9] ([fdo#109271]) +40 similar issues
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v2/shard-snb2/igt@kms_plane_scaling@planes-downscale-factor-0-5-unity-scaling@pipe-b-vga-1.html

  * igt@kms_setmode@basic@pipe-a-hdmi-a-1:
    - shard-snb:          NOTRUN -> [FAIL][10] ([i915#5465]) +1 similar issue
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v2/shard-snb1/igt@kms_setmode@basic@pipe-a-hdmi-a-1.html

  
#### Possible fixes ####

  * igt@drm_fdinfo@most-busy-idle-check-all@rcs0:
    - {shard-rkl}:        [FAIL][11] ([i915#7742]) -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13025/shard-rkl-3/igt@drm_fdinfo@most-busy-idle-check-all@rcs0.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v2/shard-rkl-7/igt@drm_fdinfo@most-busy-idle-check-all@rcs0.html

  * igt@gem_ctx_exec@basic-nohangcheck:
    - {shard-tglu}:       [FAIL][13] ([i915#6268]) -> [PASS][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13025/shard-tglu-10/igt@gem_ctx_exec@basic-nohangcheck.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v2/shard-tglu-10/igt@gem_ctx_exec@basic-nohangcheck.html

  * igt@gem_exec_fair@basic-deadline:
    - shard-glk:          [FAIL][15] ([i915#2846]) -> [PASS][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13025/shard-glk3/igt@gem_exec_fair@basic-deadline.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v2/shard-glk4/igt@gem_exec_fair@basic-deadline.html

  * igt@gem_exec_fair@basic-none@vcs0:
    - {shard-rkl}:        [FAIL][17] ([i915#2842]) -> [PASS][18]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13025/shard-rkl-4/igt@gem_exec_fair@basic-none@vcs0.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v2/shard-rkl-6/igt@gem_exec_fair@basic-none@vcs0.html

  * igt@gem_exec_fair@basic-pace-share@rcs0:
    - shard-apl:          [FAIL][19] ([i915#2842]) -> [PASS][20]
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13025/shard-apl6/igt@gem_exec_fair@basic-pace-share@rcs0.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v2/shard-apl4/igt@gem_exec_fair@basic-pace-share@rcs0.html

  * igt@i915_pm_rpm@dpms-mode-unset-non-lpsp:
    - {shard-dg1}:        [SKIP][21] ([i915#1397]) -> [PASS][22] +1 similar issue
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13025/shard-dg1-14/igt@i915_pm_rpm@dpms-mode-unset-non-lpsp.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v2/shard-dg1-18/igt@i915_pm_rpm@dpms-mode-unset-non-lpsp.html

  * igt@i915_pm_rpm@modeset-non-lpsp-stress:
    - {shard-rkl}:        [SKIP][23] ([i915#1397]) -> [PASS][24]
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13025/shard-rkl-7/igt@i915_pm_rpm@modeset-non-lpsp-stress.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v2/shard-rkl-4/igt@i915_pm_rpm@modeset-non-lpsp-stress.html

  * igt@kms_async_flips@alternate-sync-async-flip@pipe-b-dp-1:
    - shard-apl:          [FAIL][25] ([i915#2521]) -> [PASS][26]
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13025/shard-apl7/igt@kms_async_flips@alternate-sync-async-flip@pipe-b-dp-1.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v2/shard-apl4/igt@kms_async_flips@alternate-sync-async-flip@pipe-b-dp-1.html

  * igt@kms_big_fb@y-tiled-max-hw-stride-64bpp-rotate-180-hflip-async-flip:
    - {shard-rkl}:        [FAIL][27] ([i915#3743]) -> [PASS][28]
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13025/shard-rkl-7/igt@kms_big_fb@y-tiled-max-hw-stride-64bpp-rotate-180-hflip-async-flip.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v2/shard-rkl-4/igt@kms_big_fb@y-tiled-max-hw-stride-64bpp-rotate-180-hflip-async-flip.html

  * igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions:
    - shard-apl:          [FAIL][29] ([i915#2346]) -> [PASS][30]
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13025/shard-apl7/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v2/shard-apl4/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions.html

  * igt@kms_cursor_legacy@single-move@pipe-b:
    - {shard-rkl}:        [INCOMPLETE][31] ([i915#8011]) -> [PASS][32]
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13025/shard-rkl-7/igt@kms_cursor_legacy@single-move@pipe-b.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v2/shard-rkl-3/igt@kms_cursor_legacy@single-move@pipe-b.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
  [fdo#109302]: https://bugs.freedesktop.org/show_bug.cgi?id=109302
  [fdo#109303]: https://bugs.freedesktop.org/show_bug.cgi?id=109303
  [fdo#110189]: https://bugs.freedesktop.org/show_bug.cgi?id=110189
  [fdo#111068]: https://bugs.freedesktop.org/show_bug.cgi?id=111068
  [fdo#111825]: https://bugs.freedesktop.org/show_bug.cgi?id=111825
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#1397]: https://gitlab.freedesktop.org/drm/intel/issues/1397
  [i915#1937]: https://gitlab.freedesktop.org/drm/intel/issues/1937
  [i915#2346]: https://gitlab.freedesktop.org/drm/intel/issues/2346
  [i915#2521]: https://gitlab.freedesktop.org/drm/intel/issues/2521
  [i915#2527]: https://gitlab.freedesktop.org/drm/intel/issues/2527
  [i915#2575]: https://gitlab.freedesktop.org/drm/intel/issues/2575
  [i915#2587]: https://gitlab.freedesktop.org/drm/intel/issues/2587
  [i915#2672]: https://gitlab.freedesktop.org/drm/intel/issues/2672
  [i915#2705]: https://gitlab.freedesktop.org/drm/intel/issues/2705
  [i915#2842]: https://gitlab.freedesktop.org/drm/intel/issues/2842
  [i915#2846]: https://gitlab.freedesktop.org/drm/intel/issues/2846
  [i915#3281]: https://gitlab.freedesktop.org/drm/intel/issues/3281
  [i915#3282]: https://gitlab.freedesktop.org/drm/intel/issues/3282
  [i915#3297]: https://gitlab.freedesktop.org/drm/intel/issues/3297
  [i915#3299]: https://gitlab.freedesktop.org/drm/intel/issues/3299
  [i915#3361]: https://gitlab.freedesktop.org/drm/intel/issues/3361
  [i915#3458]: https://gitlab.freedesktop.org/drm/intel/issues/3458
  [i915#3528]: https://gitlab.freedesktop.org/drm/intel/issues/3528
  [i915#3539]: https://gitlab.freedesktop.org/drm/intel/issues/3539
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3638]: https://gitlab.freedesktop.org/drm/intel/issues/3638
  [i915#3689]: https://gitlab.freedesktop.org/drm/intel/issues/3689
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#3743]: https://gitlab.freedesktop.org/drm/intel/issues/3743
  [i915#3886]: https://gitlab.freedesktop.org/drm/intel/issues/3886
  [i915#3955]: https://gitlab.freedesktop.org/drm/intel/issues/3955
  [i915#4070]: https://gitlab.freedesktop.org/drm/intel/issues/4070
  [i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
  [i915#4078]: https://gitlab.freedesktop.org/drm/intel/issues/4078
  [i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
  [i915#4215]: https://gitlab.freedesktop.org/drm/intel/issues/4215
  [i915#4270]: https://gitlab.freedesktop.org/drm/intel/issues/4270
  [i915#4538]: https://gitlab.freedesktop.org/drm/intel/issues/4538
  [i915#4579]: https://gitlab.freedesktop.org/drm/intel/issues/4579
  [i915#4812]: https://gitlab.freedesktop.org/drm/intel/issues/4812
  [i915#4816]: https://gitlab.freedesktop.org/drm/intel/issues/4816
  [i915#4833]: https://gitlab.freedesktop.org/drm/intel/issues/4833
  [i915#4852]: https://gitlab.freedesktop.org/drm/intel/issues/4852
  [i915#4880]: https://gitlab.freedesktop.org/drm/intel/issues/4880
  [i915#5176]: https://gitlab.freedesktop.org/drm/intel/issues/5176
  [i915#5235]: https://gitlab.freedesktop.org/drm/intel/issues/5235
  [i915#5286]: https://gitlab.freedesktop.org/drm/intel/issues/5286
  [i915#5461]: https://gitlab.freedesktop.org/drm/intel/issues/5461
  [i915#5465]: https://gitlab.freedesktop.org/drm/intel/issues/5465
  [i915#5563]: https://gitlab.freedesktop.org/drm/intel/issues/5563
  [i915#6095]: https://gitlab.freedesktop.org/drm/intel/issues/6095
  [i915#6245]: https://gitlab.freedesktop.org/drm/intel/issues/6245
  [i915#6268]: https://gitlab.freedesktop.org/drm/intel/issues/6268
  [i915#6524]: https://gitlab.freedesktop.org/drm/intel/issues/6524
  [i915#658]: https://gitlab.freedesktop.org/drm/intel/issues/658
  [i915#6946]: https://gitlab.freedesktop.org/drm/intel/issues/6946
  [i915#72]: https://gitlab.freedesktop.org/drm/intel/issues/72
  [i915#7697]: https://gitlab.freedesktop.org/drm/intel/issues/7697
  [i915#7701]: https://gitlab.freedesktop.org/drm/intel/issues/7701
  [i915#7711]: https://gitlab.freedesktop.org/drm/intel/issues/7711
  [i915#7742]: https://gitlab.freedesktop.org/drm/intel/issues/7742
  [i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828
  [i915#8011]: https://gitlab.freedesktop.org/drm/intel/issues/8011
  [i915#8228]: https://gitlab.freedesktop.org/drm/intel/issues/8228
  [i915#8311]: https://gitlab.freedesktop.org/drm/intel/issues/8311
  [i915#8319]: https://gitlab.freedesktop.org/drm/intel/issues/8319


Build changes
-------------

  * Linux: CI_DRM_13025 -> Patchwork_116623v2

  CI-20190529: 20190529
  CI_DRM_13025: aedb908bbfc13d3d66f2715709f26f02283aef5a @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7258: ad2eb276eda849b7a7985229009a816c7608186c @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_116623v2: aedb908bbfc13d3d66f2715709f26f02283aef5a @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116623v2/index.html

[-- Attachment #2: Type: text/html, Size: 9905 bytes --]

RE: [PATCH] drm/dp_mst: Clear MSG_RDY flag before sending new message

[Lin, Wayne]

[Public]

Much appreciated, Ville and Jani!

To tackle this MST message ack event now, probably I could just pull out the 
drm_dp_mst_kick_tx() out of drm_dp_mst_hpd_irq() and make it the second 
step function to handle mst hpd irq? Would like to know your thoughts : )

Again, thanks for your time!

Regards,
Wayne Lin

> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Sent: Tuesday, April 18, 2023 10:01 PM
> To: Lin, Wayne <Wayne.Lin@amd.com>
> Cc: dri-devel@lists.freedesktop.org; amd-gfx@lists.freedesktop.org;
> lyude@redhat.com; imre.deak@intel.com; jani.nikula@intel.com; Wentland,
> Harry <Harry.Wentland@amd.com>; Zuo, Jerry <Jerry.Zuo@amd.com>;
> stable@vger.kernel.org
> Subject: Re: [PATCH] drm/dp_mst: Clear MSG_RDY flag before sending new
> message
> 
> On Tue, Apr 18, 2023 at 02:09:05PM +0800, Wayne Lin wrote:
> > [Why & How]
> > The sequence for collecting down_reply/up_request 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.
> >
> > 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 ack 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.
> >
> > In drm_dp_mst_hpd_irq(), we don't clear MSG_RDY flag before caliing
> > drm_dp_mst_kick_tx(). Fix that.
> >
> > Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
> > Cc: stable@vger.kernel.org
> > ---
> >  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  2 ++
> > drivers/gpu/drm/display/drm_dp_mst_topology.c | 22
> +++++++++++++++++++
> >  drivers/gpu/drm/i915/display/intel_dp.c       |  3 +++
> >  drivers/gpu/drm/nouveau/dispnv50/disp.c       |  2 ++
> >  4 files changed, 29 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > index 77277d90b6e2..5313a5656598 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -3166,6 +3166,8 @@ static void dm_handle_mst_sideband_msg(struct
> amdgpu_dm_connector *aconnector)
> >  			for (retry = 0; retry < 3; retry++) {
> >  				uint8_t wret;
> >
> > +				/* MSG_RDY ack is done in drm*/
> > +				esi[1] &= ~(DP_DOWN_REP_MSG_RDY |
> DP_UP_REQ_MSG_RDY);
> >  				wret = drm_dp_dpcd_write(
> >  					&aconnector->dm_dp_aux.aux,
> >  					dpcd_addr + 1,
> > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > index 51a46689cda7..02aad713c67c 100644
> > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > @@ -4054,6 +4054,9 @@ int drm_dp_mst_hpd_irq(struct
> > drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl  {
> >  	int ret = 0;
> >  	int sc;
> > +	const int tosend = 1;
> > +	int retries = 0;
> > +	u8 buf = 0;
> >  	*handled = false;
> >  	sc = DP_GET_SINK_COUNT(esi[0]);
> >
> > @@ -4072,6 +4075,25 @@ int drm_dp_mst_hpd_irq(struct
> drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl
> >  		*handled = true;
> >  	}
> >
> > +	if (*handled) {
> > +		buf = esi[1] & (DP_DOWN_REP_MSG_RDY |
> DP_UP_REQ_MSG_RDY);
> > +		do {
> > +			ret = drm_dp_dpcd_write(mgr->aux,
> > +
> 	DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0,
> > +						&buf,
> > +						tosend);
> > +
> > +			if (ret == tosend)
> > +				break;
> > +
> > +			retries++;
> > +		} while (retries < 5);
> 
> What's with this magic retry loop?
> 
> Not sure I like the whole thing though. Splitting the irq ack semi-randomly
> between driver vs. multiple helpers doesn't feel great to me.
> 
> As a whole the HPD_IRQ handling is a total mess atm. At some point I was
> trying to sketch something a bit better for it. The approach I was thinking was
> something along the lines of:
> 
>  u8 vector[...];
>  drm_dp_read_irq_vector(vector);
>  ... handle all irqs/etc., calling suitable helpers as needed
> drm_dp_clear_irq_vector(vector);
> 
> And I was also thinking that this drm_dp_*_irq_vector() stuff would always
> use the ESI layout, converting as needed from/to the old layout for pre-1.2
> (or whatever the cutoff was) devices.
> That way drivers would just need the one codepath.
> 
> > +
> > +		if (ret != tosend)
> > +			drm_dbg_kms(mgr->dev, "failed to write dpcd
> 0x%x\n",
> > +				    DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0);
> > +	}
> > +
> >  	drm_dp_mst_kick_tx(mgr);
> >  	return ret;
> >  }
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index bf80f296a8fd..abec3de38b66 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -3939,6 +3939,9 @@ intel_dp_check_mst_status(struct intel_dp
> *intel_dp)
> >  		if (!memchr_inv(ack, 0, sizeof(ack)))
> >  			break;
> >
> > +		/* MSG_RDY ack is done in drm*/
> > +		ack[1] &= ~(DP_DOWN_REP_MSG_RDY |
> DP_UP_REQ_MSG_RDY);
> > +
> >  		if (!intel_dp_ack_sink_irq_esi(intel_dp, ack))
> >  			drm_dbg_kms(&i915->drm, "Failed to ack ESI\n");
> >  	}
> > diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > index edcb2529b402..e905987104ed 100644
> > --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > @@ -1336,6 +1336,8 @@ nv50_mstm_service(struct nouveau_drm *drm,
> >  		if (!handled)
> >  			break;
> >
> > +		/* MSG_RDY ack is done in drm*/
> > +		esi[1] &= ~(DP_DOWN_REP_MSG_RDY |
> DP_UP_REQ_MSG_RDY);
> >  		rc = drm_dp_dpcd_write(aux, DP_SINK_COUNT_ESI + 1,
> &esi[1],
> >  				       3);
> >  		if (rc != 3) {
> > --
> > 2.37.3
> 
> --
> Ville Syrjälä
> Intel

RE: [PATCH] drm/dp_mst: Clear MSG_RDY flag before sending new message

[Lin, Wayne]

[Public]

Much appreciated, Ville and Jani!

To tackle this MST message ack event now, probably I could just pull out the 
drm_dp_mst_kick_tx() out of drm_dp_mst_hpd_irq() and make it the second 
step function to handle mst hpd irq? Would like to know your thoughts : )

Again, thanks for your time!

Regards,
Wayne Lin

> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Sent: Tuesday, April 18, 2023 10:01 PM
> To: Lin, Wayne <Wayne.Lin@amd.com>
> Cc: dri-devel@lists.freedesktop.org; amd-gfx@lists.freedesktop.org;
> lyude@redhat.com; imre.deak@intel.com; jani.nikula@intel.com; Wentland,
> Harry <Harry.Wentland@amd.com>; Zuo, Jerry <Jerry.Zuo@amd.com>;
> stable@vger.kernel.org
> Subject: Re: [PATCH] drm/dp_mst: Clear MSG_RDY flag before sending new
> message
> 
> On Tue, Apr 18, 2023 at 02:09:05PM +0800, Wayne Lin wrote:
> > [Why & How]
> > The sequence for collecting down_reply/up_request 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.
> >
> > 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 ack 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.
> >
> > In drm_dp_mst_hpd_irq(), we don't clear MSG_RDY flag before caliing
> > drm_dp_mst_kick_tx(). Fix that.
> >
> > Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
> > Cc: stable@vger.kernel.org
> > ---
> >  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  2 ++
> > drivers/gpu/drm/display/drm_dp_mst_topology.c | 22
> +++++++++++++++++++
> >  drivers/gpu/drm/i915/display/intel_dp.c       |  3 +++
> >  drivers/gpu/drm/nouveau/dispnv50/disp.c       |  2 ++
> >  4 files changed, 29 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > index 77277d90b6e2..5313a5656598 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -3166,6 +3166,8 @@ static void dm_handle_mst_sideband_msg(struct
> amdgpu_dm_connector *aconnector)
> >  			for (retry = 0; retry < 3; retry++) {
> >  				uint8_t wret;
> >
> > +				/* MSG_RDY ack is done in drm*/
> > +				esi[1] &= ~(DP_DOWN_REP_MSG_RDY |
> DP_UP_REQ_MSG_RDY);
> >  				wret = drm_dp_dpcd_write(
> >  					&aconnector->dm_dp_aux.aux,
> >  					dpcd_addr + 1,
> > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > index 51a46689cda7..02aad713c67c 100644
> > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > @@ -4054,6 +4054,9 @@ int drm_dp_mst_hpd_irq(struct
> > drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl  {
> >  	int ret = 0;
> >  	int sc;
> > +	const int tosend = 1;
> > +	int retries = 0;
> > +	u8 buf = 0;
> >  	*handled = false;
> >  	sc = DP_GET_SINK_COUNT(esi[0]);
> >
> > @@ -4072,6 +4075,25 @@ int drm_dp_mst_hpd_irq(struct
> drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl
> >  		*handled = true;
> >  	}
> >
> > +	if (*handled) {
> > +		buf = esi[1] & (DP_DOWN_REP_MSG_RDY |
> DP_UP_REQ_MSG_RDY);
> > +		do {
> > +			ret = drm_dp_dpcd_write(mgr->aux,
> > +
> 	DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0,
> > +						&buf,
> > +						tosend);
> > +
> > +			if (ret == tosend)
> > +				break;
> > +
> > +			retries++;
> > +		} while (retries < 5);
> 
> What's with this magic retry loop?
> 
> Not sure I like the whole thing though. Splitting the irq ack semi-randomly
> between driver vs. multiple helpers doesn't feel great to me.
> 
> As a whole the HPD_IRQ handling is a total mess atm. At some point I was
> trying to sketch something a bit better for it. The approach I was thinking was
> something along the lines of:
> 
>  u8 vector[...];
>  drm_dp_read_irq_vector(vector);
>  ... handle all irqs/etc., calling suitable helpers as needed
> drm_dp_clear_irq_vector(vector);
> 
> And I was also thinking that this drm_dp_*_irq_vector() stuff would always
> use the ESI layout, converting as needed from/to the old layout for pre-1.2
> (or whatever the cutoff was) devices.
> That way drivers would just need the one codepath.
> 
> > +
> > +		if (ret != tosend)
> > +			drm_dbg_kms(mgr->dev, "failed to write dpcd
> 0x%x\n",
> > +				    DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0);
> > +	}
> > +
> >  	drm_dp_mst_kick_tx(mgr);
> >  	return ret;
> >  }
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index bf80f296a8fd..abec3de38b66 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -3939,6 +3939,9 @@ intel_dp_check_mst_status(struct intel_dp
> *intel_dp)
> >  		if (!memchr_inv(ack, 0, sizeof(ack)))
> >  			break;
> >
> > +		/* MSG_RDY ack is done in drm*/
> > +		ack[1] &= ~(DP_DOWN_REP_MSG_RDY |
> DP_UP_REQ_MSG_RDY);
> > +
> >  		if (!intel_dp_ack_sink_irq_esi(intel_dp, ack))
> >  			drm_dbg_kms(&i915->drm, "Failed to ack ESI\n");
> >  	}
> > diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > index edcb2529b402..e905987104ed 100644
> > --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > @@ -1336,6 +1336,8 @@ nv50_mstm_service(struct nouveau_drm *drm,
> >  		if (!handled)
> >  			break;
> >
> > +		/* MSG_RDY ack is done in drm*/
> > +		esi[1] &= ~(DP_DOWN_REP_MSG_RDY |
> DP_UP_REQ_MSG_RDY);
> >  		rc = drm_dp_dpcd_write(aux, DP_SINK_COUNT_ESI + 1,
> &esi[1],
> >  				       3);
> >  		if (rc != 3) {
> > --
> > 2.37.3
> 
> --
> Ville Syrjälä
> Intel

RE: [PATCH] drm/dp_mst: Clear MSG_RDY flag before sending new message

[Lin, Wayne]

[Public]

Much appreciated, Ville and Jani!

To tackle this MST message ack event now, probably I could just pull out the 
drm_dp_mst_kick_tx() out of drm_dp_mst_hpd_irq() and make it the second 
step function to handle mst hpd irq? Would like to know your thoughts : )

Again, thanks for your time!

Regards,
Wayne Lin

> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Sent: Tuesday, April 18, 2023 10:01 PM
> To: Lin, Wayne <Wayne.Lin@amd.com>
> Cc: dri-devel@lists.freedesktop.org; amd-gfx@lists.freedesktop.org;
> lyude@redhat.com; imre.deak@intel.com; jani.nikula@intel.com; Wentland,
> Harry <Harry.Wentland@amd.com>; Zuo, Jerry <Jerry.Zuo@amd.com>;
> stable@vger.kernel.org
> Subject: Re: [PATCH] drm/dp_mst: Clear MSG_RDY flag before sending new
> message
> 
> On Tue, Apr 18, 2023 at 02:09:05PM +0800, Wayne Lin wrote:
> > [Why & How]
> > The sequence for collecting down_reply/up_request 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.
> >
> > 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 ack 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.
> >
> > In drm_dp_mst_hpd_irq(), we don't clear MSG_RDY flag before caliing
> > drm_dp_mst_kick_tx(). Fix that.
> >
> > Signed-off-by: Wayne Lin <Wayne.Lin@amd.com>
> > Cc: stable@vger.kernel.org
> > ---
> >  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  2 ++
> > drivers/gpu/drm/display/drm_dp_mst_topology.c | 22
> +++++++++++++++++++
> >  drivers/gpu/drm/i915/display/intel_dp.c       |  3 +++
> >  drivers/gpu/drm/nouveau/dispnv50/disp.c       |  2 ++
> >  4 files changed, 29 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > index 77277d90b6e2..5313a5656598 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -3166,6 +3166,8 @@ static void dm_handle_mst_sideband_msg(struct
> amdgpu_dm_connector *aconnector)
> >  			for (retry = 0; retry < 3; retry++) {
> >  				uint8_t wret;
> >
> > +				/* MSG_RDY ack is done in drm*/
> > +				esi[1] &= ~(DP_DOWN_REP_MSG_RDY |
> DP_UP_REQ_MSG_RDY);
> >  				wret = drm_dp_dpcd_write(
> >  					&aconnector->dm_dp_aux.aux,
> >  					dpcd_addr + 1,
> > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > index 51a46689cda7..02aad713c67c 100644
> > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > @@ -4054,6 +4054,9 @@ int drm_dp_mst_hpd_irq(struct
> > drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl  {
> >  	int ret = 0;
> >  	int sc;
> > +	const int tosend = 1;
> > +	int retries = 0;
> > +	u8 buf = 0;
> >  	*handled = false;
> >  	sc = DP_GET_SINK_COUNT(esi[0]);
> >
> > @@ -4072,6 +4075,25 @@ int drm_dp_mst_hpd_irq(struct
> drm_dp_mst_topology_mgr *mgr, u8 *esi, bool *handl
> >  		*handled = true;
> >  	}
> >
> > +	if (*handled) {
> > +		buf = esi[1] & (DP_DOWN_REP_MSG_RDY |
> DP_UP_REQ_MSG_RDY);
> > +		do {
> > +			ret = drm_dp_dpcd_write(mgr->aux,
> > +
> 	DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0,
> > +						&buf,
> > +						tosend);
> > +
> > +			if (ret == tosend)
> > +				break;
> > +
> > +			retries++;
> > +		} while (retries < 5);
> 
> What's with this magic retry loop?
> 
> Not sure I like the whole thing though. Splitting the irq ack semi-randomly
> between driver vs. multiple helpers doesn't feel great to me.
> 
> As a whole the HPD_IRQ handling is a total mess atm. At some point I was
> trying to sketch something a bit better for it. The approach I was thinking was
> something along the lines of:
> 
>  u8 vector[...];
>  drm_dp_read_irq_vector(vector);
>  ... handle all irqs/etc., calling suitable helpers as needed
> drm_dp_clear_irq_vector(vector);
> 
> And I was also thinking that this drm_dp_*_irq_vector() stuff would always
> use the ESI layout, converting as needed from/to the old layout for pre-1.2
> (or whatever the cutoff was) devices.
> That way drivers would just need the one codepath.
> 
> > +
> > +		if (ret != tosend)
> > +			drm_dbg_kms(mgr->dev, "failed to write dpcd
> 0x%x\n",
> > +				    DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0);
> > +	}
> > +
> >  	drm_dp_mst_kick_tx(mgr);
> >  	return ret;
> >  }
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index bf80f296a8fd..abec3de38b66 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -3939,6 +3939,9 @@ intel_dp_check_mst_status(struct intel_dp
> *intel_dp)
> >  		if (!memchr_inv(ack, 0, sizeof(ack)))
> >  			break;
> >
> > +		/* MSG_RDY ack is done in drm*/
> > +		ack[1] &= ~(DP_DOWN_REP_MSG_RDY |
> DP_UP_REQ_MSG_RDY);
> > +
> >  		if (!intel_dp_ack_sink_irq_esi(intel_dp, ack))
> >  			drm_dbg_kms(&i915->drm, "Failed to ack ESI\n");
> >  	}
> > diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > index edcb2529b402..e905987104ed 100644
> > --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> > @@ -1336,6 +1336,8 @@ nv50_mstm_service(struct nouveau_drm *drm,
> >  		if (!handled)
> >  			break;
> >
> > +		/* MSG_RDY ack is done in drm*/
> > +		esi[1] &= ~(DP_DOWN_REP_MSG_RDY |
> DP_UP_REQ_MSG_RDY);
> >  		rc = drm_dp_dpcd_write(aux, DP_SINK_COUNT_ESI + 1,
> &esi[1],
> >  				       3);
> >  		if (rc != 3) {
> > --
> > 2.37.3
> 
> --
> Ville Syrjälä
> Intel