Some comments down below:
On Wed, 2024-06-26 at 16:48 +0800, Wayne Lin wrote:
> [Why]
> During resume, observe that we receive CSN event before we start
> topology
> probing. Handling CSN at this moment based on uncertain topology is
> unnecessary.
>
> [How]
> Add checking condition in drm_dp_mst_handle_up_req() to skip handling
> CSN
> if the topology is yet to be probed.
>
> Cc: Lyude Paul <lyude@xxxxxxxxxx>
> Cc: Harry Wentland <hwentlan@xxxxxxx>
> Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
> Cc: Imre Deak <imre.deak@xxxxxxxxx>
> Cc: Daniel Vetter <daniel@xxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Wayne Lin <Wayne.Lin@xxxxxxx>
> ---
> drivers/gpu/drm/display/drm_dp_mst_topology.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> index 68831f4e502a..fc2ceae61db2 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -4069,6 +4069,7 @@ static int drm_dp_mst_handle_up_req(struct
> drm_dp_mst_topology_mgr *mgr)
> if (up_req->msg.req_type == DP_CONNECTION_STATUS_NOTIFY) {
> const struct drm_dp_connection_status_notify
> *conn_stat =
> &up_req->msg.u.conn_stat;
> + bool handle_csn;
>
> drm_dbg_kms(mgr->dev, "Got CSN: pn: %d ldps:%d ddps:
> %d mcs: %d ip: %d pdt: %d\n",
> conn_stat->port_number,
> @@ -4077,6 +4078,16 @@ static int drm_dp_mst_handle_up_req(struct
> drm_dp_mst_topology_mgr *mgr)
> conn_stat->message_capability_status,
> conn_stat->input_port,
> conn_stat->peer_device_type);
> +
> + mutex_lock(&mgr->probe_lock);
> + handle_csn = mgr->mst_primary->link_address_sent;
> + mutex_unlock(&mgr->probe_lock);
> +
> + if (!handle_csn) {
> + drm_dbg_kms(mgr->dev, "Got CSN before finish
> topology probing. Skip it.");
> + kfree(up_req);
> + goto out;
> + }
Hm. I think you're definitely on the right track here with not handling
CSNs immediately after resume. My one question though is whether
dropping the event entirely here is a good idea? In theory, we could
receive a CSN at any time during the probe - including receiving a CSN
for a connector that we've already probed in the initial post-resume
process, which could result in us missing CSNs coming out of resume and
still having an outdated topology layout.
I'm not totally sure about the solution I'm going to suggest but it
seems like it would certainly be worth trying: what if we added a flag
to drm_dp_mst_topology_mgr called something like "csn_during_resume"
and simply set it to true in response to getting a CSN before we've
finished reprobing? Then we at the end of the reprobe, we can simply
restart the reprobing process if csn_during_resume gets set - which
should still ensure we're up to date with reality.
> } else if (up_req->msg.req_type ==
> DP_RESOURCE_STATUS_NOTIFY) {
> const struct drm_dp_resource_status_notify *res_stat
> =
> &up_req->msg.u.resource_stat;
--
Cheers,
Lyude Paul (she/her)
Software Engineer at Red Hat
