[PATCH 1/2] drm/dp_mst: Return whether drm_dp_remove_payload_part1() removed the payload

[PATCH 1/2] drm/dp_mst: Return whether drm_dp_remove_payload_part1() removed the payload

[Xiao Lu]

drm_dp_remove_payload_part1() doesn't send the removal of a payload to the
branch devices if the port of the payload is not in the topology anymore.
The caller can't tell, so it generates the ACT and waits for it afterwards,
polling DPCD 0x2c0 for up to 3 seconds, although the payload table of the
branch devices wasn't changed and the ACT will never be handled.

Return whether the removal was sent, so that drivers can skip the ACT in
the other case. The existing callers ignore the return value and are not
affected.

Signed-off-by: Xiao Lu <xiaolu.xie@intel.com>
---
 drivers/gpu/drm/display/drm_dp_mst_topology.c | 10 +++++++++-
 include/drm/display/drm_dp_mst_helper.h       |  2 +-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index 7ce9e2127..363cd5237 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -3369,8 +3369,14 @@ EXPORT_SYMBOL(drm_dp_add_payload_part1);
  * Removes a payload along the virtual channel if it was successfully allocated.
  * After calling this, the driver should set HW to generate ACT and then switch to new
  * payload allocation state.
+ *
+ * Returns:
+ * %true if the port of the payload is in the topology, so the removal was
+ * sent to the branch devices and the driver should generate the ACT and wait
+ * for it. %false if the port is not in the topology anymore, so nothing was
+ * sent to the branch devices and there is no ACT to generate or wait for.
  */
-void drm_dp_remove_payload_part1(struct drm_dp_mst_topology_mgr *mgr,
+bool drm_dp_remove_payload_part1(struct drm_dp_mst_topology_mgr *mgr,
 				 struct drm_dp_mst_topology_state *mst_state,
 				 struct drm_dp_mst_atomic_payload *payload)
 {
@@ -3388,6 +3394,8 @@ void drm_dp_remove_payload_part1(struct drm_dp_mst_topology_mgr *mgr,
 			    payload->vcpi);
 
 	payload->payload_allocation_status = DRM_DP_MST_PAYLOAD_ALLOCATION_LOCAL;
+
+	return send_remove;
 }
 EXPORT_SYMBOL(drm_dp_remove_payload_part1);
 
diff --git a/include/drm/display/drm_dp_mst_helper.h b/include/drm/display/drm_dp_mst_helper.h
index 27658bfb5..d5a3bbb28 100644
--- a/include/drm/display/drm_dp_mst_helper.h
+++ b/include/drm/display/drm_dp_mst_helper.h
@@ -885,7 +885,7 @@ int drm_dp_add_payload_part1(struct drm_dp_mst_topology_mgr *mgr,
 			     struct drm_dp_mst_atomic_payload *payload);
 int drm_dp_add_payload_part2(struct drm_dp_mst_topology_mgr *mgr,
 			     struct drm_dp_mst_atomic_payload *payload);
-void drm_dp_remove_payload_part1(struct drm_dp_mst_topology_mgr *mgr,
+bool drm_dp_remove_payload_part1(struct drm_dp_mst_topology_mgr *mgr,
 				 struct drm_dp_mst_topology_state *mst_state,
 				 struct drm_dp_mst_atomic_payload *payload);
 void drm_dp_remove_payload_part2(struct drm_dp_mst_topology_mgr *mgr,
-- 
2.43.0

Re: [PATCH 1/2] drm/dp_mst: Return whether drm_dp_remove_payload_part1() removed the payload

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/dp_mst: drm_dp_remove_payload_part1() returns true when no removal is sent
--

commit c8db41e1060632039b0824f7c27d83352d6e3bba
Author: Xiao Lu <xiaolu.xie@intel.com>

drm/dp_mst: Return whether drm_dp_remove_payload_part1() removed the payload

This commit changes drm_dp_remove_payload_part1() to return a boolean
indicating whether the payload removal was sent to the branch devices so
drivers can skip generating and waiting for an ACT when the port is no
longer in the topology.

> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> index b7fce0bf8acc5..82c9c53b3fa96 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -3375,8 +3375,14 @@ EXPORT_SYMBOL(drm_dp_add_payload_part1);
>   * Removes a payload along the virtual channel if it was successfully allocated.
>   * After calling this, the driver should set HW to generate ACT and then switch to new
>   * payload allocation state.
> + *
> + * Returns:
> + * %true if the port of the payload is in the topology, so the removal was
> + * sent to the branch devices and the driver should generate the ACT and wait
> + * for it. %false if the port is not in the topology anymore, so nothing was
> + * sent to the branch devices and there is no ACT to generate or wait for.
>   */
> -void drm_dp_remove_payload_part1(struct drm_dp_mst_topology_mgr *mgr,
> +bool drm_dp_remove_payload_part1(struct drm_dp_mst_topology_mgr *mgr,
>  				 struct drm_dp_mst_topology_state *mst_state,
>  				 struct drm_dp_mst_atomic_payload *payload)
>  {
> @@ -3394,6 +3400,8 @@ void drm_dp_remove_payload_part1(struct drm_dp_mst_topology_mgr *mgr,
>  			    payload->vcpi);
>  
>  	payload->payload_allocation_status = DRM_DP_MST_PAYLOAD_ALLOCATION_LOCAL;
> +
> +	return send_remove;

[Severity: Medium]
Can drm_dp_remove_payload_part1() return true here even when no removal was
sent to the branch devices or written to the DFP DPCD payload table?

In drivers/gpu/drm/display/drm_dp_mst_topology.c, drm_dp_add_payload_part1()
initializes payload->payload_allocation_status to
DRM_DP_MST_PAYLOAD_ALLOCATION_LOCAL before validating the port and writing
the DFP payload table:

	payload->payload_allocation_status = DRM_DP_MST_PAYLOAD_ALLOCATION_LOCAL;

	/* Allocate payload to immediate downstream facing port */
	port = drm_dp_mst_topology_get_port_validated(mgr, payload->port);
	if (!port) {
		drm_dbg_kms(mgr->dev,
			    "VCPI %d for port %p not in topology, not creating a payload to remote\n",
			    payload->vcpi, payload->port);
		return -EIO;
	}

	ret = drm_dp_create_payload_at_dfp(mgr, payload);
	if (ret < 0) {
		drm_dbg_kms(mgr->dev, "Failed to create MST payload for port %p: %d\n",
			    payload->port, ret);
		goto put_port;
	}

If drm_dp_mst_topology_get_port_validated() returns NULL because a
downstream port or sub-branch was unlinked while mgr->mst_primary remained
connected, or if drm_dp_create_payload_at_dfp() fails,
payload->payload_allocation_status stays DRM_DP_MST_PAYLOAD_ALLOCATION_LOCAL
and DPCD 0x1c0-0x1c2 is not updated.

When the stream is subsequently disabled, drm_dp_remove_payload_part1()
computes send_remove via:

	send_remove = drm_dp_mst_port_downstream_of_branch(payload->port, mgr->mst_primary);

Because drm_dp_mst_port_downstream_of_branch() only walks port->parent and
port->parent->port_parent (which remain set until malloc_kref release),
send_remove evaluates to true as long as mgr->mst_primary has not been
replaced.

It then calls drm_dp_destroy_payload_at_remote_and_dfp():

	if (payload->payload_allocation_status == DRM_DP_MST_PAYLOAD_ALLOCATION_REMOTE) {
		drm_dp_payload_send_msg(mgr, payload->port, payload->vcpi, 0);
		payload->payload_allocation_status = DRM_DP_MST_PAYLOAD_ALLOCATION_DFP;
	}

	if (payload->payload_allocation_status == DRM_DP_MST_PAYLOAD_ALLOCATION_DFP)
		drm_dp_dpcd_write_payload(mgr->aux, payload->vcpi, payload->vc_start_slot, 0);

When payload->payload_allocation_status is
DRM_DP_MST_PAYLOAD_ALLOCATION_LOCAL (or DRM_DP_MST_PAYLOAD_ALLOCATION_NONE),
drm_dp_destroy_payload_at_remote_and_dfp() does nothing at all.

Returning send_remove unconditionally in that state will tell callers to
generate an ACT and poll DPCD 0x2c0 for up to 3 seconds even though the DFP
payload table was never modified. Should the return value also check whether
payload->payload_allocation_status was DFP or REMOTE (or whether the DFP
payload table was actually updated)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031153550.444676-1-xiaolu.xie@intel.com?part=1