Re: [PATCH v5] drm/dp_mst: Clear MSG_RDY flag before sending new message
Alright, managed to figure out my MST woes! Just tested with nouveau and I see
no regressions :)
Reviewed-by: Lyude Paul
On Fri, 2023-06-09 at 18:49 +0800, Wayne Lin wrote:
> [Why]
> The sequence for collecting down_reply from source perspective should
> be:
>
> Request_n->repeat (get partial reply of Request_n->clear message ready
> flag to ack DPRX that the message is received) till all partial
> replies for Request_n are received->new Request_n+1.
>
> Now there is chance that drm_dp_mst_hpd_irq() will fire new down
> request in the tx queue when the down reply is incomplete. Source is
> restricted to generate interveleaved message transactions so we should
> avoid it.
>
> Also, while assembling partial reply packets, reading out DPCD DOWN_REP
> Sideband MSG buffer + clearing DOWN_REP_MSG_RDY flag should be
> wrapped up as a complete operation for reading out a reply packet.
> Kicking off a new request before clearing DOWN_REP_MSG_RDY flag might
> be risky. e.g. If the reply of the new request has overwritten the
> DPRX DOWN_REP Sideband MSG buffer before source writing one to clear
> DOWN_REP_MSG_RDY flag, source then unintentionally flushes the reply
> for the new request. Should handle the up request in the same way.
>
> [How]
> Separete drm_dp_mst_hpd_irq() into 2 steps. After acking the MST IRQ
> event, driver calls drm_dp_mst_hpd_irq_send_new_request() and might
> trigger drm_dp_mst_kick_tx() only when there is no on going message
> transaction.
>
> Changes since v1:
> * Reworked on review comments received
> -> Adjust the fix to let driver explicitly kick off new down request
> when mst irq event is handled and acked
> -> Adjust the commit message
>
> Changes since v2:
> * Adjust the commit message
> * Adjust the naming of the divided 2 functions and add a new input
> parameter "ack".
> * Adjust code flow as per review comments.
>
> Changes since v3:
> * Update the function description of drm_dp_mst_hpd_irq_handle_event
>
> Changes since v4:
> * Change ack of drm_dp_mst_hpd_irq_handle_event() to be an array align
> the size of esi[]
>
> Signed-off-by: Wayne Lin
> Cc: sta...@vger.kernel.org
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 32 +--
> drivers/gpu/drm/display/drm_dp_mst_topology.c | 54 ---
> drivers/gpu/drm/i915/display/intel_dp.c | 7 +--
> drivers/gpu/drm/nouveau/dispnv50/disp.c | 12 +++--
> include/drm/display/drm_dp_mst_helper.h | 7 ++-
> 5 files changed, 81 insertions(+), 31 deletions(-)
>
> 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 d5cec03eaa8d..ec629b4037e4 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -3263,6 +3263,7 @@ static void dm_handle_mst_sideband_msg(struct
> amdgpu_dm_connector *aconnector)
>
> while (dret == dpcd_bytes_to_read &&
> process_count < max_process_count) {
> + u8 ack[DP_PSR_ERROR_STATUS - DP_SINK_COUNT_ESI] = {};
> u8 retry;
> dret = 0;
>
> @@ -3271,28 +3272,29 @@ static void dm_handle_mst_sideband_msg(struct
> amdgpu_dm_connector *aconnector)
> DRM_DEBUG_DRIVER("ESI %02x %02x %02x\n", esi[0], esi[1],
> esi[2]);
> /* handle HPD short pulse irq */
> if (aconnector->mst_mgr.mst_state)
> - drm_dp_mst_hpd_irq(
> - >mst_mgr,
> - esi,
> - _irq_handled);
> + drm_dp_mst_hpd_irq_handle_event(>mst_mgr,
> + esi,
> + ack,
> + _irq_handled);
>
> if (new_irq_handled) {
> /* ACK at DPCD to notify down stream */
> - const int ack_dpcd_bytes_to_write =
> - dpcd_bytes_to_read - 1;
> -
> for (retry = 0; retry < 3; retry++) {
> - u8 wret;
> -
> - wret = drm_dp_dpcd_write(
> - >dm_dp_aux.aux,
> - dpcd_addr + 1,
> - [1],
> - ack_dpcd_bytes_to_write);
> - if (wret == ack_dpcd_bytes_to_write)
> + ssize_t wret;
> +
> + wret =
> drm_dp_dpcd_writeb(>dm_dp_aux.aux,
> + dpcd_addr + 1,
> + ack[1]);
> + if (wret == 1)
> break;
> }
>
> + if (retry == 3) {
> +
RE: [PATCH v5] drm/dp_mst: Clear MSG_RDY flag before sending new message
[AMD Official Use Only - General]
Noted. Thanks, Lyude!
Regards,
Wayne Lin
> -Original Message-
> From: Lyude Paul
> Sent: Tuesday, June 13, 2023 6:34 AM
> To: Lin, Wayne ; dri-devel@lists.freedesktop.org;
> amd-...@lists.freedesktop.org
> Cc: ville.syrj...@linux.intel.com; jani.nik...@intel.com; imre.d...@intel.com;
> Wentland, Harry ; Zuo, Jerry
> ; sta...@vger.kernel.org
> Subject: Re: [PATCH v5] drm/dp_mst: Clear MSG_RDY flag before sending new
> message
>
> FWIW: Should have a response to this very soon, figured out the cause of my
> MST issues so I should be able to test this very soon
>
> On Fri, 2023-06-09 at 18:49 +0800, Wayne Lin wrote:
> > [Why]
> > The sequence for collecting down_reply from source perspective should
> > be:
> >
> > Request_n->repeat (get partial reply of Request_n->clear message ready
> > flag to ack DPRX that the message is received) till all partial
> > replies for Request_n are received->new Request_n+1.
> >
> > Now there is chance that drm_dp_mst_hpd_irq() will fire new down
> > request in the tx queue when the down reply is incomplete. Source is
> > restricted to generate interveleaved message transactions so we should
> > avoid it.
> >
> > Also, while assembling partial reply packets, reading out DPCD
> > DOWN_REP Sideband MSG buffer + clearing DOWN_REP_MSG_RDY flag
> should
> > be wrapped up as a complete operation for reading out a reply packet.
> > Kicking off a new request before clearing DOWN_REP_MSG_RDY flag might
> > be risky. e.g. If the reply of the new request has overwritten the
> > DPRX DOWN_REP Sideband MSG buffer before source writing one to clear
> > DOWN_REP_MSG_RDY flag, source then unintentionally flushes the reply
> > for the new request. Should handle the up request in the same way.
> >
> > [How]
> > Separete drm_dp_mst_hpd_irq() into 2 steps. After acking the MST IRQ
> > event, driver calls drm_dp_mst_hpd_irq_send_new_request() and might
> > trigger drm_dp_mst_kick_tx() only when there is no on going message
> > transaction.
> >
> > Changes since v1:
> > * Reworked on review comments received
> > -> Adjust the fix to let driver explicitly kick off new down request
> > when mst irq event is handled and acked
> > -> Adjust the commit message
> >
> > Changes since v2:
> > * Adjust the commit message
> > * Adjust the naming of the divided 2 functions and add a new input
> > parameter "ack".
> > * Adjust code flow as per review comments.
> >
> > Changes since v3:
> > * Update the function description of drm_dp_mst_hpd_irq_handle_event
> >
> > Changes since v4:
> > * Change ack of drm_dp_mst_hpd_irq_handle_event() to be an array align
> > the size of esi[]
> >
> > Signed-off-by: Wayne Lin
> > Cc: sta...@vger.kernel.org
> > ---
> > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 32 +--
> > drivers/gpu/drm/display/drm_dp_mst_topology.c | 54
> ---
> > drivers/gpu/drm/i915/display/intel_dp.c | 7 +--
> > drivers/gpu/drm/nouveau/dispnv50/disp.c | 12 +++--
> > include/drm/display/drm_dp_mst_helper.h | 7 ++-
> > 5 files changed, 81 insertions(+), 31 deletions(-)
> >
> > 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 d5cec03eaa8d..ec629b4037e4 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -3263,6 +3263,7 @@ static void dm_handle_mst_sideband_msg(struct
> > amdgpu_dm_connector *aconnector)
> >
> > while (dret == dpcd_bytes_to_read &&
> > process_count < max_process_count) {
> > + u8 ack[DP_PSR_ERROR_STATUS - DP_SINK_COUNT_ESI] = {};
> > u8 retry;
> > dret = 0;
> >
> > @@ -3271,28 +3272,29 @@ static void
> dm_handle_mst_sideband_msg(struct amdgpu_dm_connector *aconnector)
> > DRM_DEBUG_DRIVER("ESI %02x %02x %02x\n", esi[0],
> esi[1], esi[2]);
> > /* handle HPD short pulse irq */
> > if (aconnector->mst_mgr.mst_state)
> > - drm_dp_mst_hpd_irq(
> > - >mst_mgr,
> > - esi,
> > - _irq_handled);
> > + drm_dp_mst_hpd_irq_handle_event(
> >mst_mgr,
> > + esi,
> > +
Re: [PATCH v5] drm/dp_mst: Clear MSG_RDY flag before sending new message
FWIW: Should have a response to this very soon, figured out the cause of my
MST issues so I should be able to test this very soon
On Fri, 2023-06-09 at 18:49 +0800, Wayne Lin wrote:
> [Why]
> The sequence for collecting down_reply from source perspective should
> be:
>
> Request_n->repeat (get partial reply of Request_n->clear message ready
> flag to ack DPRX that the message is received) till all partial
> replies for Request_n are received->new Request_n+1.
>
> Now there is chance that drm_dp_mst_hpd_irq() will fire new down
> request in the tx queue when the down reply is incomplete. Source is
> restricted to generate interveleaved message transactions so we should
> avoid it.
>
> Also, while assembling partial reply packets, reading out DPCD DOWN_REP
> Sideband MSG buffer + clearing DOWN_REP_MSG_RDY flag should be
> wrapped up as a complete operation for reading out a reply packet.
> Kicking off a new request before clearing DOWN_REP_MSG_RDY flag might
> be risky. e.g. If the reply of the new request has overwritten the
> DPRX DOWN_REP Sideband MSG buffer before source writing one to clear
> DOWN_REP_MSG_RDY flag, source then unintentionally flushes the reply
> for the new request. Should handle the up request in the same way.
>
> [How]
> Separete drm_dp_mst_hpd_irq() into 2 steps. After acking the MST IRQ
> event, driver calls drm_dp_mst_hpd_irq_send_new_request() and might
> trigger drm_dp_mst_kick_tx() only when there is no on going message
> transaction.
>
> Changes since v1:
> * Reworked on review comments received
> -> Adjust the fix to let driver explicitly kick off new down request
> when mst irq event is handled and acked
> -> Adjust the commit message
>
> Changes since v2:
> * Adjust the commit message
> * Adjust the naming of the divided 2 functions and add a new input
> parameter "ack".
> * Adjust code flow as per review comments.
>
> Changes since v3:
> * Update the function description of drm_dp_mst_hpd_irq_handle_event
>
> Changes since v4:
> * Change ack of drm_dp_mst_hpd_irq_handle_event() to be an array align
> the size of esi[]
>
> Signed-off-by: Wayne Lin
> Cc: sta...@vger.kernel.org
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 32 +--
> drivers/gpu/drm/display/drm_dp_mst_topology.c | 54 ---
> drivers/gpu/drm/i915/display/intel_dp.c | 7 +--
> drivers/gpu/drm/nouveau/dispnv50/disp.c | 12 +++--
> include/drm/display/drm_dp_mst_helper.h | 7 ++-
> 5 files changed, 81 insertions(+), 31 deletions(-)
>
> 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 d5cec03eaa8d..ec629b4037e4 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -3263,6 +3263,7 @@ static void dm_handle_mst_sideband_msg(struct
> amdgpu_dm_connector *aconnector)
>
> while (dret == dpcd_bytes_to_read &&
> process_count < max_process_count) {
> + u8 ack[DP_PSR_ERROR_STATUS - DP_SINK_COUNT_ESI] = {};
> u8 retry;
> dret = 0;
>
> @@ -3271,28 +3272,29 @@ static void dm_handle_mst_sideband_msg(struct
> amdgpu_dm_connector *aconnector)
> DRM_DEBUG_DRIVER("ESI %02x %02x %02x\n", esi[0], esi[1],
> esi[2]);
> /* handle HPD short pulse irq */
> if (aconnector->mst_mgr.mst_state)
> - drm_dp_mst_hpd_irq(
> - >mst_mgr,
> - esi,
> - _irq_handled);
> + drm_dp_mst_hpd_irq_handle_event(>mst_mgr,
> + esi,
> + ack,
> + _irq_handled);
>
> if (new_irq_handled) {
> /* ACK at DPCD to notify down stream */
> - const int ack_dpcd_bytes_to_write =
> - dpcd_bytes_to_read - 1;
> -
> for (retry = 0; retry < 3; retry++) {
> - u8 wret;
> -
> - wret = drm_dp_dpcd_write(
> - >dm_dp_aux.aux,
> - dpcd_addr + 1,
> - [1],
> - ack_dpcd_bytes_to_write);
> - if (wret == ack_dpcd_bytes_to_write)
> + ssize_t wret;
> +
> + wret =
> drm_dp_dpcd_writeb(>dm_dp_aux.aux,
> + dpcd_addr + 1,
> + ack[1]);
> + if (wret == 1)
> break;
> }
>
> + if (retry == 3) {
> +
Re: [PATCH v5] drm/dp_mst: Clear MSG_RDY flag before sending new message
On Fri, 09 Jun 2023, Wayne Lin wrote:
> [Why]
> The sequence for collecting down_reply from source perspective should
> be:
>
> Request_n->repeat (get partial reply of Request_n->clear message ready
> flag to ack DPRX that the message is received) till all partial
> replies for Request_n are received->new Request_n+1.
>
> Now there is chance that drm_dp_mst_hpd_irq() will fire new down
> request in the tx queue when the down reply is incomplete. Source is
> restricted to generate interveleaved message transactions so we should
> avoid it.
>
> Also, while assembling partial reply packets, reading out DPCD DOWN_REP
> Sideband MSG buffer + clearing DOWN_REP_MSG_RDY flag should be
> wrapped up as a complete operation for reading out a reply packet.
> Kicking off a new request before clearing DOWN_REP_MSG_RDY flag might
> be risky. e.g. If the reply of the new request has overwritten the
> DPRX DOWN_REP Sideband MSG buffer before source writing one to clear
> DOWN_REP_MSG_RDY flag, source then unintentionally flushes the reply
> for the new request. Should handle the up request in the same way.
>
> [How]
> Separete drm_dp_mst_hpd_irq() into 2 steps. After acking the MST IRQ
> event, driver calls drm_dp_mst_hpd_irq_send_new_request() and might
> trigger drm_dp_mst_kick_tx() only when there is no on going message
> transaction.
>
> Changes since v1:
> * Reworked on review comments received
> -> Adjust the fix to let driver explicitly kick off new down request
> when mst irq event is handled and acked
> -> Adjust the commit message
>
> Changes since v2:
> * Adjust the commit message
> * Adjust the naming of the divided 2 functions and add a new input
> parameter "ack".
> * Adjust code flow as per review comments.
>
> Changes since v3:
> * Update the function description of drm_dp_mst_hpd_irq_handle_event
>
> Changes since v4:
> * Change ack of drm_dp_mst_hpd_irq_handle_event() to be an array align
> the size of esi[]
I don't have the time for detailed review right now, but assuming
someone does that, and Intel CI passes (I bounced this to
intel-...@lists.freedesktop.org which should get it going),
Acked-by: Jani Nikula
>
> Signed-off-by: Wayne Lin
> Cc: sta...@vger.kernel.org
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 32 +--
> drivers/gpu/drm/display/drm_dp_mst_topology.c | 54 ---
> drivers/gpu/drm/i915/display/intel_dp.c | 7 +--
> drivers/gpu/drm/nouveau/dispnv50/disp.c | 12 +++--
> include/drm/display/drm_dp_mst_helper.h | 7 ++-
> 5 files changed, 81 insertions(+), 31 deletions(-)
>
> 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 d5cec03eaa8d..ec629b4037e4 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -3263,6 +3263,7 @@ static void dm_handle_mst_sideband_msg(struct
> amdgpu_dm_connector *aconnector)
>
> while (dret == dpcd_bytes_to_read &&
> process_count < max_process_count) {
> + u8 ack[DP_PSR_ERROR_STATUS - DP_SINK_COUNT_ESI] = {};
> u8 retry;
> dret = 0;
>
> @@ -3271,28 +3272,29 @@ static void dm_handle_mst_sideband_msg(struct
> amdgpu_dm_connector *aconnector)
> DRM_DEBUG_DRIVER("ESI %02x %02x %02x\n", esi[0], esi[1],
> esi[2]);
> /* handle HPD short pulse irq */
> if (aconnector->mst_mgr.mst_state)
> - drm_dp_mst_hpd_irq(
> - >mst_mgr,
> - esi,
> - _irq_handled);
> + drm_dp_mst_hpd_irq_handle_event(>mst_mgr,
> + esi,
> + ack,
> + _irq_handled);
>
> if (new_irq_handled) {
> /* ACK at DPCD to notify down stream */
> - const int ack_dpcd_bytes_to_write =
> - dpcd_bytes_to_read - 1;
> -
> for (retry = 0; retry < 3; retry++) {
> - u8 wret;
> -
> - wret = drm_dp_dpcd_write(
> - >dm_dp_aux.aux,
> - dpcd_addr + 1,
> - [1],
> - ack_dpcd_bytes_to_write);
> - if (wret == ack_dpcd_bytes_to_write)
> + ssize_t wret;
> +
> + wret =
> drm_dp_dpcd_writeb(>dm_dp_aux.aux,
> + dpcd_addr + 1,
> + ack[1]);
> + if (wret == 1)
> break;
>
[PATCH v5] drm/dp_mst: Clear MSG_RDY flag before sending new message
[Why]
The sequence for collecting down_reply from source perspective should
be:
Request_n->repeat (get partial reply of Request_n->clear message ready
flag to ack DPRX that the message is received) till all partial
replies for Request_n are received->new Request_n+1.
Now there is chance that drm_dp_mst_hpd_irq() will fire new down
request in the tx queue when the down reply is incomplete. Source is
restricted to generate interveleaved message transactions so we should
avoid it.
Also, while assembling partial reply packets, reading out DPCD DOWN_REP
Sideband MSG buffer + clearing DOWN_REP_MSG_RDY flag should be
wrapped up as a complete operation for reading out a reply packet.
Kicking off a new request before clearing DOWN_REP_MSG_RDY flag might
be risky. e.g. If the reply of the new request has overwritten the
DPRX DOWN_REP Sideband MSG buffer before source writing one to clear
DOWN_REP_MSG_RDY flag, source then unintentionally flushes the reply
for the new request. Should handle the up request in the same way.
[How]
Separete drm_dp_mst_hpd_irq() into 2 steps. After acking the MST IRQ
event, driver calls drm_dp_mst_hpd_irq_send_new_request() and might
trigger drm_dp_mst_kick_tx() only when there is no on going message
transaction.
Changes since v1:
* Reworked on review comments received
-> Adjust the fix to let driver explicitly kick off new down request
when mst irq event is handled and acked
-> Adjust the commit message
Changes since v2:
* Adjust the commit message
* Adjust the naming of the divided 2 functions and add a new input
parameter "ack".
* Adjust code flow as per review comments.
Changes since v3:
* Update the function description of drm_dp_mst_hpd_irq_handle_event
Changes since v4:
* Change ack of drm_dp_mst_hpd_irq_handle_event() to be an array align
the size of esi[]
Signed-off-by: Wayne Lin
Cc: sta...@vger.kernel.org
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 32 +--
drivers/gpu/drm/display/drm_dp_mst_topology.c | 54 ---
drivers/gpu/drm/i915/display/intel_dp.c | 7 +--
drivers/gpu/drm/nouveau/dispnv50/disp.c | 12 +++--
include/drm/display/drm_dp_mst_helper.h | 7 ++-
5 files changed, 81 insertions(+), 31 deletions(-)
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 d5cec03eaa8d..ec629b4037e4 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3263,6 +3263,7 @@ static void dm_handle_mst_sideband_msg(struct
amdgpu_dm_connector *aconnector)
while (dret == dpcd_bytes_to_read &&
process_count < max_process_count) {
+ u8 ack[DP_PSR_ERROR_STATUS - DP_SINK_COUNT_ESI] = {};
u8 retry;
dret = 0;
@@ -3271,28 +3272,29 @@ static void dm_handle_mst_sideband_msg(struct
amdgpu_dm_connector *aconnector)
DRM_DEBUG_DRIVER("ESI %02x %02x %02x\n", esi[0], esi[1],
esi[2]);
/* handle HPD short pulse irq */
if (aconnector->mst_mgr.mst_state)
- drm_dp_mst_hpd_irq(
- >mst_mgr,
- esi,
- _irq_handled);
+ drm_dp_mst_hpd_irq_handle_event(>mst_mgr,
+ esi,
+ ack,
+ _irq_handled);
if (new_irq_handled) {
/* ACK at DPCD to notify down stream */
- const int ack_dpcd_bytes_to_write =
- dpcd_bytes_to_read - 1;
-
for (retry = 0; retry < 3; retry++) {
- u8 wret;
-
- wret = drm_dp_dpcd_write(
- >dm_dp_aux.aux,
- dpcd_addr + 1,
- [1],
- ack_dpcd_bytes_to_write);
- if (wret == ack_dpcd_bytes_to_write)
+ ssize_t wret;
+
+ wret =
drm_dp_dpcd_writeb(>dm_dp_aux.aux,
+ dpcd_addr + 1,
+ ack[1]);
+ if (wret == 1)
break;
}
+ if (retry == 3) {
+ DRM_ERROR("Failed to ack MST event.\n");
+ return;
+ }
+
+
drm_dp_mst_hpd_irq_send_new_request(>mst_mgr);
/* check if there is new irq to be handled */
dret = drm_dp_dpcd_read(
