On Tue, Sep 03, 2019 at 04:45:57PM -0400, Lyude Paul wrote:
> Once upon a time, hotplugging devices on MST branches actually worked in
> DRM. Now, it only works in amdgpu (likely because of how it's hotplug
> handlers are implemented). On both i915 and nouveau, hotplug
> notifications from MST branches are noticed - but trying to respond to
> them causes messaging timeouts and causes the whole topology state to go
> out of sync with reality, usually resulting in the user needing to
> replug the entire topology in hopes that it actually fixes things.
>
> The reason for this is because the way we currently handle UP requests
> in MST is completely bogus. drm_dp_mst_handle_up_req() is called from
> drm_dp_mst_hpd_irq(), which is usually called from the driver's hotplug
> handler. Because we handle sending the hotplug event from this function,
> we actually cause the driver's hotplug handler (and in turn, all
> sideband transactions) to block on
> drm_device->mode_config.connection_mutex. This makes it impossible to
> send any sideband messages from the driver's connector probing
> functions, resulting in the aforementioned sideband message timeout.
>
> There's even more problems with this beyond breaking hotplugging on MST
> branch devices. It also makes it almost impossible to protect
> drm_dp_mst_port struct members under a lock because we then have to
> worry about dealing with all of the lock dependency issues that ensue.
>
> So, let's finally actually fix this issue by handling the processing of
> up requests asyncronously. This way we can send sideband messages from
> most contexts without having to deal with getting blocked if we hold
> connection_mutex. This also fixes MST branch device hotplugging on i915,
> finally!
>
> Cc: Juston Li
> Cc: Imre Deak
> Cc: Ville Syrjälä
> Cc: Harry Wentland
> Cc: Daniel Vetter
> Signed-off-by: Lyude Paul
Looks really good!
Reviewed-by: Sean Paul
> ---
> drivers/gpu/drm/drm_dp_mst_topology.c | 146 +++---
> include/drm/drm_dp_mst_helper.h | 16 +++
> 2 files changed, 122 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index cfaf9eb7ace9..5101eeab4485 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -46,6 +46,12 @@
> * protocol. The helpers contain a topology manager and bandwidth manager.
> * The helpers encapsulate the sending and received of sideband msgs.
> */
> +struct drm_dp_pending_up_req {
> + struct drm_dp_sideband_msg_hdr hdr;
> + struct drm_dp_sideband_msg_req_body msg;
> + struct list_head next;
> +};
> +
> static bool dump_dp_payload_table(struct drm_dp_mst_topology_mgr *mgr,
> char *buf);
>
> @@ -3109,6 +3115,7 @@ void drm_dp_mst_topology_mgr_suspend(struct
> drm_dp_mst_topology_mgr *mgr)
> drm_dp_dpcd_writeb(mgr->aux, DP_MSTM_CTRL,
> DP_MST_EN | DP_UPSTREAM_IS_SRC);
> mutex_unlock(>lock);
> + flush_work(>up_req_work);
> flush_work(>work);
> flush_work(>delayed_destroy_work);
> }
> @@ -3281,12 +3288,70 @@ static int drm_dp_mst_handle_down_rep(struct
> drm_dp_mst_topology_mgr *mgr)
> return 0;
> }
>
> +static inline void
> +drm_dp_mst_process_up_req(struct drm_dp_mst_topology_mgr *mgr,
> + struct drm_dp_pending_up_req *up_req)
> +{
> + struct drm_dp_mst_branch *mstb = NULL;
> + struct drm_dp_sideband_msg_req_body *msg = _req->msg;
> + struct drm_dp_sideband_msg_hdr *hdr = _req->hdr;
> +
> + if (hdr->broadcast) {
> + const u8 *guid = NULL;
> +
> + if (msg->req_type == DP_CONNECTION_STATUS_NOTIFY)
> + guid = msg->u.conn_stat.guid;
> + else if (msg->req_type == DP_RESOURCE_STATUS_NOTIFY)
> + guid = msg->u.resource_stat.guid;
> +
> + mstb = drm_dp_get_mst_branch_device_by_guid(mgr, guid);
> + } else {
> + mstb = drm_dp_get_mst_branch_device(mgr, hdr->lct, hdr->rad);
> + }
> +
> + if (!mstb) {
> + DRM_DEBUG_KMS("Got MST reply from unknown device %d\n",
> + hdr->lct);
> + return;
> + }
> +
> + /* TODO: Add missing handler for DP_RESOURCE_STATUS_NOTIFY events */
> + if (msg->req_type == DP_CONNECTION_STATUS_NOTIFY) {
> + drm_dp_mst_handle_conn_stat(mstb, >u.conn_stat);
> + drm_kms_helper_hotplug_event(mgr->dev);
> + }
> +
> + drm_dp_mst_topology_put_mstb(mstb);
> +}
> +
> +static void drm_dp_mst_up_req_work(struct work_struct *work)
> +{
> + struct drm_dp_mst_topology_mgr *mgr =
> + container_of(work, struct drm_dp_mst_topology_mgr,
> + up_req_work);
> + struct drm_dp_pending_up_req *up_req;
> +
> + while (true) {
> + mutex_lock(>up_req_lock);
> + up_req =