From: Sean Paul <sean@poorly.run>
To: Lyude Paul <lyude@redhat.com>
Cc: dri-devel@lists.freedesktop.org, nouveau@lists.freedesktop.org,
amd-gfx@lists.freedesktop.org, "Juston Li" <juston.li@intel.com>,
"Imre Deak" <imre.deak@intel.com>,
"Ville Syrjälä" <ville.syrjala@linux.intel.com>,
"Harry Wentland" <hwentlan@amd.com>,
"Daniel Vetter" <daniel.vetter@ffwll.ch>,
"Maarten Lankhorst" <maarten.lankhorst@linux.intel.com>,
"Maxime Ripard" <mripard@kernel.org>,
"Sean Paul" <sean@poorly.run>, "David Airlie" <airlied@linux.ie>,
"Daniel Vetter" <daniel@ffwll.ch>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 19/27] drm/dp_mst: Handle UP requests asynchronously
Date: Wed, 25 Sep 2019 15:46:26 -0400 [thread overview]
Message-ID: <20190925194626.GK218215@art_vandelay> (raw)
In-Reply-To: <20190903204645.25487-20-lyude@redhat.com>
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 <juston.li@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Harry Wentland <hwentlan@amd.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Lyude Paul <lyude@redhat.com>
Looks really good!
Reviewed-by: Sean Paul <sean@poorly.run>
> ---
> 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(&mgr->lock);
> + flush_work(&mgr->up_req_work);
> flush_work(&mgr->work);
> flush_work(&mgr->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 = &up_req->msg;
> + struct drm_dp_sideband_msg_hdr *hdr = &up_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, &msg->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(&mgr->up_req_lock);
> + up_req = list_first_entry_or_null(&mgr->up_req_list,
> + struct drm_dp_pending_up_req,
> + next);
> + if (up_req)
> + list_del(&up_req->next);
> + mutex_unlock(&mgr->up_req_lock);
> +
> + if (!up_req)
> + break;
> +
> + drm_dp_mst_process_up_req(mgr, up_req);
> + kfree(up_req);
> + }
> +}
> +
> static int drm_dp_mst_handle_up_req(struct drm_dp_mst_topology_mgr *mgr)
> {
> - struct drm_dp_sideband_msg_req_body msg;
> struct drm_dp_sideband_msg_hdr *hdr = &mgr->up_req_recv.initial_hdr;
> - struct drm_dp_mst_branch *mstb = NULL;
> - const u8 *guid;
> + struct drm_dp_pending_up_req *up_req;
> bool seqno;
>
> if (!drm_dp_get_one_sb_msg(mgr, true))
> @@ -3295,56 +3360,53 @@ static int drm_dp_mst_handle_up_req(struct drm_dp_mst_topology_mgr *mgr)
> if (!mgr->up_req_recv.have_eomt)
> return 0;
>
> - if (!hdr->broadcast) {
> - 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);
> - goto out;
> - }
> + up_req = kzalloc(sizeof(*up_req), GFP_KERNEL);
> + if (!up_req) {
> + DRM_ERROR("Not enough memory to process MST up req\n");
> + return -ENOMEM;
> }
> + INIT_LIST_HEAD(&up_req->next);
>
> seqno = hdr->seqno;
> - drm_dp_sideband_parse_req(&mgr->up_req_recv, &msg);
> + drm_dp_sideband_parse_req(&mgr->up_req_recv, &up_req->msg);
>
> - 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;
> - else
> + if (up_req->msg.req_type != DP_CONNECTION_STATUS_NOTIFY &&
> + up_req->msg.req_type != DP_RESOURCE_STATUS_NOTIFY) {
> + DRM_DEBUG_KMS("Received unknown up req type, ignoring: %x\n",
> + up_req->msg.req_type);
> + kfree(up_req);
> goto out;
> -
> - drm_dp_send_up_ack_reply(mgr, mgr->mst_primary, msg.req_type, seqno,
> - false);
> -
> - if (!mstb) {
> - mstb = drm_dp_get_mst_branch_device_by_guid(mgr, guid);
> - if (!mstb) {
> - DRM_DEBUG_KMS("Got MST reply from unknown device %d\n",
> - hdr->lct);
> - goto out;
> - }
> }
>
> - if (msg.req_type == DP_CONNECTION_STATUS_NOTIFY) {
> - drm_dp_mst_handle_conn_stat(mstb, &msg.u.conn_stat);
> + drm_dp_send_up_ack_reply(mgr, mgr->mst_primary, up_req->msg.req_type,
> + seqno, false);
> +
> + 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;
>
> DRM_DEBUG_KMS("Got CSN: pn: %d ldps:%d ddps: %d mcs: %d ip: %d pdt: %d\n",
> - msg.u.conn_stat.port_number,
> - msg.u.conn_stat.legacy_device_plug_status,
> - msg.u.conn_stat.displayport_device_plug_status,
> - msg.u.conn_stat.message_capability_status,
> - msg.u.conn_stat.input_port,
> - msg.u.conn_stat.peer_device_type);
> + conn_stat->port_number,
> + conn_stat->legacy_device_plug_status,
> + conn_stat->displayport_device_plug_status,
> + conn_stat->message_capability_status,
> + conn_stat->input_port,
> + conn_stat->peer_device_type);
> + } 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;
>
> - drm_kms_helper_hotplug_event(mgr->dev);
> - } else if (msg.req_type == DP_RESOURCE_STATUS_NOTIFY) {
> DRM_DEBUG_KMS("Got RSN: pn: %d avail_pbn %d\n",
> - msg.u.resource_stat.port_number,
> - msg.u.resource_stat.available_pbn);
> + res_stat->port_number,
> + res_stat->available_pbn);
> }
>
> - drm_dp_mst_topology_put_mstb(mstb);
> + up_req->hdr = *hdr;
> + mutex_lock(&mgr->up_req_lock);
> + list_add_tail(&up_req->next, &mgr->up_req_list);
> + mutex_unlock(&mgr->up_req_lock);
> + queue_work(system_long_wq, &mgr->up_req_work);
> +
> out:
> memset(&mgr->up_req_recv, 0, sizeof(struct drm_dp_sideband_msg_rx));
> return 0;
> @@ -4320,12 +4382,15 @@ int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,
> mutex_init(&mgr->qlock);
> mutex_init(&mgr->payload_lock);
> mutex_init(&mgr->delayed_destroy_lock);
> + mutex_init(&mgr->up_req_lock);
> INIT_LIST_HEAD(&mgr->tx_msg_downq);
> INIT_LIST_HEAD(&mgr->destroy_port_list);
> INIT_LIST_HEAD(&mgr->destroy_branch_device_list);
> + INIT_LIST_HEAD(&mgr->up_req_list);
> INIT_WORK(&mgr->work, drm_dp_mst_link_probe_work);
> INIT_WORK(&mgr->tx_work, drm_dp_tx_work);
> INIT_WORK(&mgr->delayed_destroy_work, drm_dp_delayed_destroy_work);
> + INIT_WORK(&mgr->up_req_work, drm_dp_mst_up_req_work);
> init_waitqueue_head(&mgr->tx_waitq);
> mgr->dev = dev;
> mgr->aux = aux;
> @@ -4382,6 +4447,7 @@ void drm_dp_mst_topology_mgr_destroy(struct drm_dp_mst_topology_mgr *mgr)
> mutex_destroy(&mgr->payload_lock);
> mutex_destroy(&mgr->qlock);
> mutex_destroy(&mgr->lock);
> + mutex_destroy(&mgr->up_req_lock);
> }
> EXPORT_SYMBOL(drm_dp_mst_topology_mgr_destroy);
>
> diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
> index 8ba2a01324bb..7d80c38ee00e 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -597,6 +597,22 @@ struct drm_dp_mst_topology_mgr {
> * devices, needed to avoid locking inversion.
> */
> struct work_struct delayed_destroy_work;
> +
> + /**
> + * @up_req_list: List of pending up requests from the topology that
> + * need to be processed, in chronological order.
> + */
> + struct list_head up_req_list;
> + /**
> + * @up_req_lock: Protects @up_req_list
> + */
> + struct mutex up_req_lock;
> + /**
> + * @up_req_work: Work item to process up requests received from the
> + * topology. Needed to avoid blocking hotplug handling and sideband
> + * transmissions.
> + */
> + struct work_struct up_req_work;
> };
>
> int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,
> --
> 2.21.0
>
--
Sean Paul, Software Engineer, Google / Chromium OS
next prev parent reply other threads:[~2019-09-25 19:46 UTC|newest]
Thread overview: 62+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-03 20:45 [PATCH v2 00/27] DP MST Refactors + debugging tools + suspend/resume reprobing Lyude Paul
2019-09-03 20:45 ` [PATCH v2 01/27] drm/dp_mst: Move link address dumping into a function Lyude Paul
2019-09-25 17:45 ` Sean Paul
2019-09-03 20:45 ` [PATCH v2 02/27] drm/dp_mst: Get rid of list clear in destroy_connector_work Lyude Paul
2019-09-25 17:45 ` Sean Paul
2019-09-03 20:45 ` [PATCH v2 03/27] drm/dp_mst: Destroy MSTBs asynchronously Lyude Paul
2019-09-25 18:16 ` Sean Paul
2019-09-25 20:08 ` Lyude Paul
[not found] ` <e9a2638663549d86779002e3616fc2e89f9c7028.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2019-09-27 13:31 ` Sean Paul
2019-10-08 9:45 ` Daniel Vetter
[not found] ` <20190903204645.25487-1-lyude-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2019-09-03 20:45 ` [PATCH v2 04/27] drm/dp_mst: Move test_calc_pbn_mode() into an actual selftest Lyude Paul
2019-09-25 18:17 ` Sean Paul
2019-09-03 20:45 ` [PATCH v2 05/27] drm/print: Add drm_err_printer() Lyude Paul
2019-09-03 20:45 ` [PATCH v2 07/27] drm/dp_mst: Add sideband down request tracing + selftests Lyude Paul
2019-09-10 9:01 ` Jani Nikula
2019-09-03 20:45 ` [PATCH v2 08/27] drm/dp_mst: Remove PDT teardown in drm_dp_destroy_port() and refactor Lyude Paul
2019-09-25 19:00 ` Sean Paul
2019-09-03 20:45 ` [PATCH v2 09/27] drm/dp_mst: Refactor drm_dp_send_enum_path_resources Lyude Paul
2019-09-03 20:45 ` [PATCH v2 10/27] drm/dp_mst: Remove huge conditional in drm_dp_mst_handle_up_req() Lyude Paul
2019-09-03 20:45 ` [PATCH v2 11/27] drm/dp_mst: Constify guid in drm_dp_get_mst_branch_by_guid() Lyude Paul
[not found] ` <20190903204645.25487-12-lyude-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2019-09-03 21:41 ` Dave Airlie
2019-09-03 20:45 ` [PATCH v2 12/27] drm/dp_mst: Refactor drm_dp_mst_handle_up_req() Lyude Paul
2019-09-03 20:45 ` [PATCH v2 14/27] drm/dp_mst: Destroy topology_mgr mutexes Lyude Paul
2019-09-25 19:14 ` Sean Paul
2019-09-03 20:45 ` [PATCH v2 15/27] drm/dp_mst: Cleanup drm_dp_send_link_address() a bit Lyude Paul
2019-09-03 21:42 ` Dave Airlie
2019-09-03 20:45 ` [PATCH v2 17/27] drm/dp_mst: Rename drm_dp_add_port and drm_dp_update_port Lyude Paul
2019-09-25 19:30 ` Sean Paul
2019-09-03 20:46 ` [PATCH v2 26/27] drm/dp_mst: Also print unhashed pointers for malloc/topology references Lyude Paul
2019-09-27 14:25 ` Sean Paul
2019-10-09 19:40 ` Lyude Paul
2019-09-03 20:45 ` [PATCH v2 06/27] drm/dp_mst: Combine redundant cases in drm_dp_encode_sideband_req() Lyude Paul
2019-09-03 21:35 ` Dave Airlie
2019-09-03 20:45 ` [PATCH v2 13/27] drm/dp_mst: Refactor drm_dp_mst_handle_down_rep() Lyude Paul
2019-09-03 20:45 ` [PATCH v2 16/27] drm/dp_mst: Refactor pdt setup/teardown, add more locking Lyude Paul
2019-09-25 19:27 ` Sean Paul
2019-09-25 21:00 ` Lyude Paul
[not found] ` <581dc77ab09314ac8d7c4cd7dc3efee5d4663a97.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2019-09-27 13:30 ` Sean Paul
2019-09-03 20:45 ` [PATCH v2 18/27] drm/dp_mst: Remove lies in {up,down}_rep_recv documentation Lyude Paul
2019-09-25 19:32 ` Sean Paul
2019-09-03 20:45 ` [PATCH v2 19/27] drm/dp_mst: Handle UP requests asynchronously Lyude Paul
2019-09-25 19:46 ` Sean Paul [this message]
2019-09-03 20:45 ` [PATCH v2 20/27] drm/dp_mst: Protect drm_dp_mst_port members with connection_mutex Lyude Paul
2019-09-25 20:00 ` Sean Paul
2019-09-25 21:01 ` Lyude Paul
2019-09-03 20:45 ` [PATCH v2 21/27] drm/dp_mst: Don't forget to update port->input in drm_dp_mst_handle_conn_stat() Lyude Paul
2019-09-25 20:03 ` Sean Paul
2019-09-03 20:46 ` [PATCH v2 22/27] drm/nouveau: Don't grab runtime PM refs for HPD IRQs Lyude Paul
2019-09-25 20:06 ` Sean Paul
2019-09-03 20:46 ` [PATCH v2 23/27] drm/amdgpu: Iterate through DRM connectors correctly Lyude Paul
[not found] ` <20190903204645.25487-24-lyude-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2019-09-13 20:45 ` Alex Deucher
[not found] ` <CADnq5_NvhO751ihc64KmkxR_dOWNn5XJ4bJc95CMUEkGkw1Y2Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2019-09-27 13:48 ` Alex Deucher
2019-09-03 20:46 ` [PATCH v2 24/27] drm/amdgpu/dm: Resume short HPD IRQs before resuming MST topology Lyude Paul
2019-09-13 20:46 ` Alex Deucher
2019-09-25 20:08 ` Sean Paul
[not found] ` <20190903204645.25487-25-lyude-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2019-09-25 21:52 ` [PATCH v4] " Lyude Paul
2019-09-27 13:29 ` Alex Deucher
2019-09-03 20:46 ` [PATCH v2 25/27] drm/dp_mst: Add basic topology reprobing when resuming Lyude Paul
2019-09-27 13:52 ` Sean Paul
2019-10-09 19:06 ` Lyude Paul
2019-09-03 20:46 ` [PATCH v2 27/27] drm/dp_mst: Add topology ref history tracking for debugging Lyude Paul
2019-09-27 14:51 ` Sean Paul
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190925194626.GK218215@art_vandelay \
--to=sean@poorly.run \
--cc=airlied@linux.ie \
--cc=amd-gfx@lists.freedesktop.org \
--cc=daniel.vetter@ffwll.ch \
--cc=daniel@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=hwentlan@amd.com \
--cc=imre.deak@intel.com \
--cc=juston.li@intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=lyude@redhat.com \
--cc=maarten.lankhorst@linux.intel.com \
--cc=mripard@kernel.org \
--cc=nouveau@lists.freedesktop.org \
--cc=ville.syrjala@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox