Re: [Nouveau] [PATCH v2 20/27] drm/dp_mst: Protect drm_dp_mst_port members with connection_mutex
On Wed, 2019-09-25 at 16:00 -0400, Sean Paul wrote:
> On Tue, Sep 03, 2019 at 04:45:58PM -0400, Lyude Paul wrote:
> > Yes-you read that right. Currently there is literally no locking in
> > place for any of the drm_dp_mst_port struct members that can be modified
> > in response to a link address response, or a connection status response.
> > Which literally means if we're unlucky enough to have any sort of
> > hotplugging event happen before we're finished with reprobing link
> > addresses, we'll race and the contents of said struct members becomes
> > undefined. Fun!
> >
> > So, finally add some simple locking protections to our MST helpers by
> > protecting any drm_dp_mst_port members which can be changed by link
> > address responses or connection status notifications under
> > drm_device->mode_config.connection_mutex.
> >
> > Cc: Juston Li
> > Cc: Imre Deak
> > Cc: Ville Syrjälä
> > Cc: Harry Wentland
> > Cc: Daniel Vetter
> > Signed-off-by: Lyude Paul
> > ---
> > drivers/gpu/drm/drm_dp_mst_topology.c | 144 +++---
> > include/drm/drm_dp_mst_helper.h | 39 +--
> > 2 files changed, 133 insertions(+), 50 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index 5101eeab4485..259634c5d6dc 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -1354,6 +1354,7 @@ static void drm_dp_free_mst_port(struct kref *kref)
> > container_of(kref, struct drm_dp_mst_port, malloc_kref);
> >
> > drm_dp_mst_put_mstb_malloc(port->parent);
> > + mutex_destroy(>lock);
> > kfree(port);
> > }
> >
> > @@ -1906,6 +1907,36 @@ void drm_dp_mst_connector_early_unregister(struct
> > drm_connector *connector,
> > }
> > EXPORT_SYMBOL(drm_dp_mst_connector_early_unregister);
> >
> > +static void
> > +drm_dp_mst_port_add_connector(struct drm_dp_mst_branch *mstb,
> > + struct drm_dp_mst_port *port)
> > +{
> > + struct drm_dp_mst_topology_mgr *mgr = port->mgr;
> > + char proppath[255];
> > + int ret;
> > +
> > + build_mst_prop_path(mstb, port->port_num, proppath, sizeof(proppath));
> > + port->connector = mgr->cbs->add_connector(mgr, port, proppath);
> > + if (!port->connector) {
> > + ret = -ENOMEM;
> > + goto error;
> > + }
> > +
> > + if ((port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV ||
> > +port->pdt == DP_PEER_DEVICE_SST_SINK) &&
> > + port->port_num >= DP_MST_LOGICAL_PORT_0) {
> > + port->cached_edid = drm_get_edid(port->connector,
> > +>aux.ddc);
> > + drm_connector_set_tile_property(port->connector);
> > + }
> > +
> > + mgr->cbs->register_connector(port->connector);
> > + return;
> > +
> > +error:
> > + DRM_ERROR("Failed to create connector for port %p: %d\n", port, ret);
> > +}
> > +
> > static void
> > drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb,
> > struct drm_device *dev,
> > @@ -1913,8 +1944,12 @@ drm_dp_mst_handle_link_address_port(struct
> > drm_dp_mst_branch *mstb,
> > {
> > struct drm_dp_mst_topology_mgr *mgr = mstb->mgr;
> > struct drm_dp_mst_port *port;
> > - bool created = false;
> > - int old_ddps = 0;
> > + struct drm_dp_mst_branch *child_mstb = NULL;
> > + struct drm_connector *connector_to_destroy = NULL;
> > + int old_ddps = 0, ret;
> > + u8 new_pdt = DP_PEER_DEVICE_NONE;
> > + bool created = false, send_link_addr = false,
> > +create_connector = false;
> >
> > port = drm_dp_get_port(mstb, port_msg->port_number);
> > if (!port) {
> > @@ -1923,6 +1958,7 @@ drm_dp_mst_handle_link_address_port(struct
> > drm_dp_mst_branch *mstb,
> > return;
> > kref_init(>topology_kref);
> > kref_init(>malloc_kref);
> > + mutex_init(>lock);
> > port->parent = mstb;
> > port->port_num = port_msg->port_number;
> > port->mgr = mgr;
> > @@ -1937,11 +1973,17 @@ drm_dp_mst_handle_link_address_port(struct
> > drm_dp_mst_branch *mstb,
> > drm_dp_mst_get_mstb_malloc(mstb);
> >
> > created = true;
> > - } else {
> > - old_ddps = port->ddps;
> > }
> >
> > + mutex_lock(>lock);
> > + drm_modeset_lock(>mode_config.connection_mutex, NULL);
> > +
> > + if (!created)
> > + old_ddps = port->ddps;
> > +
> > port->input = port_msg->input_port;
> > + if (!port->input)
> > + new_pdt = port_msg->peer_device_type;
> > port->mcs = port_msg->mcs;
> > port->ddps = port_msg->ddps;
> > port->ldps = port_msg->legacy_device_plug_status;
> > @@ -1969,44 +2011,58 @@ drm_dp_mst_handle_link_address_port(struct
> > drm_dp_mst_branch *mstb,
> > }
> > }
> >
> > - if (!port->input) {
> > - int ret = drm_dp_port_set_pdt(port,
> > -
Re: [Nouveau] [PATCH v2 16/27] drm/dp_mst: Refactor pdt setup/teardown, add more locking
On Wed, 2019-09-25 at 15:27 -0400, Sean Paul wrote:
> On Tue, Sep 03, 2019 at 04:45:54PM -0400, Lyude Paul wrote:
> > Since we're going to be implementing suspend/resume reprobing very soon,
> > we need to make sure we are extra careful to ensure that our locking
> > actually protects the topology state where we expect it to. Turns out
> > this isn't the case with drm_dp_port_setup_pdt() and
> > drm_dp_port_teardown_pdt(), both of which change port->mstb without
> > grabbing >lock.
> >
> > Additionally, since most callers of these functions are just using it to
> > teardown the port's previous PDT and setup a new one we can simplify
> > things a bit and combine drm_dp_port_setup_pdt() and
> > drm_dp_port_teardown_pdt() into a single function:
> > drm_dp_port_set_pdt(). This function also handles actually ensuring that
> > we grab the correct locks when we need to modify port->mstb.
> >
> > Cc: Juston Li
> > Cc: Imre Deak
> > Cc: Ville Syrjälä
> > Cc: Harry Wentland
> > Cc: Daniel Vetter
> > Signed-off-by: Lyude Paul
> > ---
> > drivers/gpu/drm/drm_dp_mst_topology.c | 181 +++---
> > include/drm/drm_dp_mst_helper.h | 6 +-
> > 2 files changed, 110 insertions(+), 77 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index d1610434a0cb..9944ef2ce885 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -1487,24 +1487,6 @@ drm_dp_mst_topology_put_mstb(struct
> > drm_dp_mst_branch *mstb)
> > kref_put(>topology_kref, drm_dp_destroy_mst_branch_device);
> > }
> >
> > -static void drm_dp_port_teardown_pdt(struct drm_dp_mst_port *port, int
> > old_pdt)
> > -{
> > - struct drm_dp_mst_branch *mstb;
> > -
> > - switch (old_pdt) {
> > - case DP_PEER_DEVICE_DP_LEGACY_CONV:
> > - case DP_PEER_DEVICE_SST_SINK:
> > - /* remove i2c over sideband */
> > - drm_dp_mst_unregister_i2c_bus(>aux);
> > - break;
> > - case DP_PEER_DEVICE_MST_BRANCHING:
> > - mstb = port->mstb;
> > - port->mstb = NULL;
> > - drm_dp_mst_topology_put_mstb(mstb);
> > - break;
> > - }
> > -}
> > -
> > static void drm_dp_destroy_port(struct kref *kref)
> > {
> > struct drm_dp_mst_port *port =
> > @@ -1714,38 +1696,79 @@ static u8 drm_dp_calculate_rad(struct
> > drm_dp_mst_port *port,
> > return parent_lct + 1;
> > }
> >
> > -/*
> > - * return sends link address for new mstb
> > - */
> > -static bool drm_dp_port_setup_pdt(struct drm_dp_mst_port *port)
> > +static int drm_dp_port_set_pdt(struct drm_dp_mst_port *port, u8 new_pdt)
> > {
> > - int ret;
> > - u8 rad[6], lct;
> > - bool send_link = false;
> > + struct drm_dp_mst_topology_mgr *mgr = port->mgr;
> > + struct drm_dp_mst_branch *mstb;
> > + u8 rad[8], lct;
> > + int ret = 0;
> > +
> > + if (port->pdt == new_pdt)
>
> Shouldn't we also ensure that access to port->pdt is also locked?
>
It's specifically port->mstb that needs to be protected under lock. We don't
use port->pdt for traversing the topology at all, so keeping it under
connection_mutex is sufficient.
> Sean
>
> > + return 0;
> > +
> > + /* Teardown the old pdt, if there is one */
> > + switch (port->pdt) {
> > + case DP_PEER_DEVICE_DP_LEGACY_CONV:
> > + case DP_PEER_DEVICE_SST_SINK:
> > + /*
> > +* If the new PDT would also have an i2c bus, don't bother
> > +* with reregistering it
> > +*/
> > + if (new_pdt == DP_PEER_DEVICE_DP_LEGACY_CONV ||
> > + new_pdt == DP_PEER_DEVICE_SST_SINK) {
> > + port->pdt = new_pdt;
> > + return 0;
> > + }
> > +
> > + /* remove i2c over sideband */
> > + drm_dp_mst_unregister_i2c_bus(>aux);
> > + break;
> > + case DP_PEER_DEVICE_MST_BRANCHING:
> > + mutex_lock(>lock);
> > + drm_dp_mst_topology_put_mstb(port->mstb);
> > + port->mstb = NULL;
> > + mutex_unlock(>lock);
> > + break;
> > + }
> > +
> > + port->pdt = new_pdt;
> > switch (port->pdt) {
> > case DP_PEER_DEVICE_DP_LEGACY_CONV:
> > case DP_PEER_DEVICE_SST_SINK:
> > /* add i2c over sideband */
> > ret = drm_dp_mst_register_i2c_bus(>aux);
> > break;
> > +
> > case DP_PEER_DEVICE_MST_BRANCHING:
> > lct = drm_dp_calculate_rad(port, rad);
> > + mstb = drm_dp_add_mst_branch_device(lct, rad);
> > + if (!mstb) {
> > + ret = -ENOMEM;
> > + DRM_ERROR("Failed to create MSTB for port %p", port);
> > + goto out;
> > + }
> >
> > - port->mstb = drm_dp_add_mst_branch_device(lct, rad);
> > - if (port->mstb) {
> > - port->mstb->mgr = port->mgr;
> > - port->mstb->port_parent =
Re: [Nouveau] [PATCH v2 03/27] drm/dp_mst: Destroy MSTBs asynchronously
On Wed, 2019-09-25 at 14:16 -0400, Sean Paul wrote:
> On Tue, Sep 03, 2019 at 04:45:41PM -0400, Lyude Paul wrote:
> > When reprobing an MST topology during resume, we have to account for the
> > fact that while we were suspended it's possible that mstbs may have been
> > removed from any ports in the topology. Since iterating downwards in the
> > topology requires that we hold >lock, destroying MSTBs from this
> > context would result in attempting to lock >lock a second time and
> > deadlocking.
> >
> > So, fix this by first moving destruction of MSTBs into
> > destroy_connector_work, then rename destroy_connector_work and friends
> > to reflect that they now destroy both ports and mstbs.
> >
> > Changes since v1:
> > * s/destroy_connector_list/destroy_port_list/
> > s/connector_destroy_lock/delayed_destroy_lock/
> > s/connector_destroy_work/delayed_destroy_work/
> > s/drm_dp_finish_destroy_branch_device/drm_dp_delayed_destroy_mstb/
> > s/drm_dp_finish_destroy_port/drm_dp_delayed_destroy_port/
> > - danvet
> > * Use two loops in drm_dp_delayed_destroy_work() - danvet
> > * Better explain why we need to do this - danvet
> > * Use cancel_work_sync() instead of flush_work() - flush_work() doesn't
> > account for work requeing
> >
> > Cc: Juston Li
> > Cc: Imre Deak
> > Cc: Ville Syrjälä
> > Cc: Harry Wentland
> > Cc: Daniel Vetter
> > Signed-off-by: Lyude Paul
>
> Took me a while to grok this, and I'm still not 100% confident my mental
> model
> is correct, so please bear with me while I ask silly questions :)
>
> Now that the destroy is delayed, and the port remains in the topology, is it
> possible we will underflow the topology kref by calling put_mstb multiple
> times?
> It looks like that would result in a WARN from refcount.c, and wouldn't call
> the
> destroy function multiple times, so that's nice :)
>
> Similarly, is there any defense against calling get_mstb() between destroy()
> and
> the delayed destroy worker running?
>
Good question! There's only one instance where we unconditionally grab an MSTB
ref, drm_dp_mst_topology_mgr_set_mst(), and in that location we're guaranteed
to be the only one with access to that mstb until we drop >lock.
Everywhere else we use drm_dp_mst_topology_try_get_mstb(), which uses
kref_get_unless_zero() to protect against that kind of situation (and forces
the caller to check with __must_check)
> Sean
>
> > ---
> > drivers/gpu/drm/drm_dp_mst_topology.c | 162 +-
> > include/drm/drm_dp_mst_helper.h | 26 +++--
> > 2 files changed, 127 insertions(+), 61 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index 3054ec622506..738f260d4b15 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -1113,34 +1113,17 @@ static void
> > drm_dp_destroy_mst_branch_device(struct kref *kref)
> > struct drm_dp_mst_branch *mstb =
> > container_of(kref, struct drm_dp_mst_branch, topology_kref);
> > struct drm_dp_mst_topology_mgr *mgr = mstb->mgr;
> > - struct drm_dp_mst_port *port, *tmp;
> > - bool wake_tx = false;
> >
> > - mutex_lock(>lock);
> > - list_for_each_entry_safe(port, tmp, >ports, next) {
> > - list_del(>next);
> > - drm_dp_mst_topology_put_port(port);
> > - }
> > - mutex_unlock(>lock);
> > -
> > - /* drop any tx slots msg */
> > - mutex_lock(>mgr->qlock);
> > - if (mstb->tx_slots[0]) {
> > - mstb->tx_slots[0]->state = DRM_DP_SIDEBAND_TX_TIMEOUT;
> > - mstb->tx_slots[0] = NULL;
> > - wake_tx = true;
> > - }
> > - if (mstb->tx_slots[1]) {
> > - mstb->tx_slots[1]->state = DRM_DP_SIDEBAND_TX_TIMEOUT;
> > - mstb->tx_slots[1] = NULL;
> > - wake_tx = true;
> > - }
> > - mutex_unlock(>mgr->qlock);
> > + INIT_LIST_HEAD(>destroy_next);
> >
> > - if (wake_tx)
> > - wake_up_all(>mgr->tx_waitq);
> > -
> > - drm_dp_mst_put_mstb_malloc(mstb);
> > + /*
> > +* This can get called under mgr->mutex, so we need to perform the
> > +* actual destruction of the mstb in another worker
> > +*/
> > + mutex_lock(>delayed_destroy_lock);
> > + list_add(>destroy_next, >destroy_branch_device_list);
> > + mutex_unlock(>delayed_destroy_lock);
> > + schedule_work(>delayed_destroy_work);
> > }
> >
> > /**
> > @@ -1255,10 +1238,10 @@ static void drm_dp_destroy_port(struct kref *kref)
> > * we might be holding the mode_config.mutex
> > * from an EDID retrieval */
> >
> > - mutex_lock(>destroy_connector_lock);
> > - list_add(>next, >destroy_connector_list);
> > - mutex_unlock(>destroy_connector_lock);
> > - schedule_work(>destroy_connector_work);
> > + mutex_lock(>delayed_destroy_lock);
> > + list_add(>next,
Re: [Nouveau] [PATCH v2 22/27] drm/nouveau: Don't grab runtime PM refs for HPD IRQs
On Tue, Sep 03, 2019 at 04:46:00PM -0400, Lyude Paul wrote:
> In order for suspend/resume reprobing to work, we need to be able to
> perform sideband communications during suspend/resume, along with
> runtime PM suspend/resume. In order to do so, we also need to make sure
> that nouveau doesn't bother grabbing a runtime PM reference to do so,
> since otherwise we'll start deadlocking runtime PM again.
>
> Note that we weren't able to do this before, because of the DP MST
> helpers processing UP requests from topologies in the same context as
> drm_dp_mst_hpd_irq() which would have caused us to open ourselves up to
> receiving hotplug events and deadlocking with runtime suspend/resume.
> Now that those requests are handled asynchronously, this change should
> be completely safe.
>
> Cc: Juston Li
> Cc: Imre Deak
> Cc: Ville Syrjälä
> Cc: Harry Wentland
> Cc: Daniel Vetter
> Signed-off-by: Lyude Paul
Seems reasonable to me, but would feel better if a nouveau person confirmed
Reviewed-by: Sean Paul
> ---
> drivers/gpu/drm/nouveau/nouveau_connector.c | 33 +++--
> 1 file changed, 17 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c
> b/drivers/gpu/drm/nouveau/nouveau_connector.c
> index 56871d34e3fb..f276918d3f3b 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_connector.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
> @@ -1131,6 +1131,16 @@ nouveau_connector_hotplug(struct nvif_notify *notify)
> const char *name = connector->name;
> struct nouveau_encoder *nv_encoder;
> int ret;
> + bool plugged = (rep->mask != NVIF_NOTIFY_CONN_V0_UNPLUG);
> +
> + if (rep->mask & NVIF_NOTIFY_CONN_V0_IRQ) {
> + NV_DEBUG(drm, "service %s\n", name);
> + drm_dp_cec_irq(_connector->aux);
> + if ((nv_encoder = find_encoder(connector, DCB_OUTPUT_DP)))
> + nv50_mstm_service(nv_encoder->dp.mstm);
> +
> + return NVIF_NOTIFY_KEEP;
> + }
>
> ret = pm_runtime_get(drm->dev->dev);
> if (ret == 0) {
> @@ -1151,25 +1161,16 @@ nouveau_connector_hotplug(struct nvif_notify *notify)
> return NVIF_NOTIFY_DROP;
> }
>
> - if (rep->mask & NVIF_NOTIFY_CONN_V0_IRQ) {
> - NV_DEBUG(drm, "service %s\n", name);
> - drm_dp_cec_irq(_connector->aux);
> - if ((nv_encoder = find_encoder(connector, DCB_OUTPUT_DP)))
> - nv50_mstm_service(nv_encoder->dp.mstm);
> - } else {
> - bool plugged = (rep->mask != NVIF_NOTIFY_CONN_V0_UNPLUG);
> -
> + if (!plugged)
> + drm_dp_cec_unset_edid(_connector->aux);
> + NV_DEBUG(drm, "%splugged %s\n", plugged ? "" : "un", name);
> + if ((nv_encoder = find_encoder(connector, DCB_OUTPUT_DP))) {
> if (!plugged)
> - drm_dp_cec_unset_edid(_connector->aux);
> - NV_DEBUG(drm, "%splugged %s\n", plugged ? "" : "un", name);
> - if ((nv_encoder = find_encoder(connector, DCB_OUTPUT_DP))) {
> - if (!plugged)
> - nv50_mstm_remove(nv_encoder->dp.mstm);
> - }
> -
> - drm_helper_hpd_irq_event(connector->dev);
> + nv50_mstm_remove(nv_encoder->dp.mstm);
> }
>
> + drm_helper_hpd_irq_event(connector->dev);
> +
> pm_runtime_mark_last_busy(drm->dev->dev);
> pm_runtime_put_autosuspend(drm->dev->dev);
> return NVIF_NOTIFY_KEEP;
> --
> 2.21.0
>
--
Sean Paul, Software Engineer, Google / Chromium OS
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH v2 21/27] drm/dp_mst: Don't forget to update port->input in drm_dp_mst_handle_conn_stat()
On Tue, Sep 03, 2019 at 04:45:59PM -0400, Lyude Paul wrote:
> This probably hasn't caused any problems up until now since it's
> probably nearly impossible to encounter this in the wild, however if we
> were to receive a connection status notification from the MST hub after
> resume while we're in the middle of reprobing the link addresses for a
> topology then there's a much larger chance that a port could have
> changed from being an output port to input port (or vice versa). If we
> forget to update this bit of information, we'll potentially ignore a
> valid PDT change on a downstream port because we think it's an input
> port.
>
> So, make sure we read the input_port field in connection status
> notifications in drm_dp_mst_handle_conn_stat() to prevent this from
> happening once we've implemented suspend/resume reprobing.
>
> Cc: Juston Li
> Cc: Imre Deak
> Cc: Ville Syrjälä
> Cc: Harry Wentland
> Cc: Daniel Vetter
> Signed-off-by: Lyude Paul
Nice catch! Same comment here re: port->mutex, but we can sort that out on the
other thread
Reviewed-by: Sean Paul
> ---
> drivers/gpu/drm/drm_dp_mst_topology.c | 51 +++
> 1 file changed, 37 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 259634c5d6dc..e407aba1fbd2 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -2078,18 +2078,23 @@ static void
> drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
> struct drm_dp_connection_status_notify *conn_stat)
> {
> - struct drm_device *dev = mstb->mgr->dev;
> + struct drm_dp_mst_topology_mgr *mgr = mstb->mgr;
> + struct drm_device *dev = mgr->dev;
> struct drm_dp_mst_port *port;
> - int old_ddps;
> - bool dowork = false;
> + struct drm_connector *connector_to_destroy = NULL;
> + int old_ddps, ret;
> + u8 new_pdt;
> + bool dowork = false, create_connector = false;
>
> port = drm_dp_get_port(mstb, conn_stat->port_number);
> if (!port)
> return;
>
> + mutex_lock(>lock);
> drm_modeset_lock(>mode_config.connection_mutex, NULL);
>
> old_ddps = port->ddps;
> + port->input = conn_stat->input_port;
> port->mcs = conn_stat->message_capability_status;
> port->ldps = conn_stat->legacy_device_plug_status;
> port->ddps = conn_stat->displayport_device_plug_status;
> @@ -2102,23 +2107,41 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch
> *mstb,
> }
> }
>
> - if (!port->input) {
> - int ret = drm_dp_port_set_pdt(port,
> - conn_stat->peer_device_type);
> - if (ret == 1) {
> - dowork = true;
> - } else if (ret < 0) {
> - DRM_ERROR("Failed to change PDT for port %p: %d\n",
> - port, ret);
> - dowork = false;
> - }
> + new_pdt = port->input ? DP_PEER_DEVICE_NONE :
> conn_stat->peer_device_type;
> +
> + ret = drm_dp_port_set_pdt(port, new_pdt);
> + if (ret == 1) {
> + dowork = true;
> + } else if (ret < 0) {
> + DRM_ERROR("Failed to change PDT for port %p: %d\n",
> + port, ret);
> + dowork = false;
> + }
> +
> + /*
> + * We unset port->connector before dropping connection_mutex so that
> + * there's no chance any of the atomic MST helpers can accidentally
> + * associate a to-be-destroyed connector with a port.
> + */
> + if (port->connector && port->input) {
> + connector_to_destroy = port->connector;
> + port->connector = NULL;
> + } else if (!port->connector && !port->input) {
> + create_connector = true;
> }
>
> drm_modeset_unlock(>mode_config.connection_mutex);
> +
> + if (connector_to_destroy)
> + mgr->cbs->destroy_connector(mgr, connector_to_destroy);
> + else if (create_connector)
> + drm_dp_mst_port_add_connector(mstb, port);
> +
> + mutex_unlock(>lock);
> +
> drm_dp_mst_topology_put_port(port);
> if (dowork)
> queue_work(system_long_wq, >mgr->work);
> -
> }
>
> static struct drm_dp_mst_branch *drm_dp_get_mst_branch_device(struct
> drm_dp_mst_topology_mgr *mgr,
> --
> 2.21.0
>
--
Sean Paul, Software Engineer, Google / Chromium OS
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH v2 20/27] drm/dp_mst: Protect drm_dp_mst_port members with connection_mutex
On Tue, Sep 03, 2019 at 04:45:58PM -0400, Lyude Paul wrote:
> Yes-you read that right. Currently there is literally no locking in
> place for any of the drm_dp_mst_port struct members that can be modified
> in response to a link address response, or a connection status response.
> Which literally means if we're unlucky enough to have any sort of
> hotplugging event happen before we're finished with reprobing link
> addresses, we'll race and the contents of said struct members becomes
> undefined. Fun!
>
> So, finally add some simple locking protections to our MST helpers by
> protecting any drm_dp_mst_port members which can be changed by link
> address responses or connection status notifications under
> drm_device->mode_config.connection_mutex.
>
> Cc: Juston Li
> Cc: Imre Deak
> Cc: Ville Syrjälä
> Cc: Harry Wentland
> Cc: Daniel Vetter
> Signed-off-by: Lyude Paul
> ---
> drivers/gpu/drm/drm_dp_mst_topology.c | 144 +++---
> include/drm/drm_dp_mst_helper.h | 39 +--
> 2 files changed, 133 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 5101eeab4485..259634c5d6dc 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -1354,6 +1354,7 @@ static void drm_dp_free_mst_port(struct kref *kref)
> container_of(kref, struct drm_dp_mst_port, malloc_kref);
>
> drm_dp_mst_put_mstb_malloc(port->parent);
> + mutex_destroy(>lock);
> kfree(port);
> }
>
> @@ -1906,6 +1907,36 @@ void drm_dp_mst_connector_early_unregister(struct
> drm_connector *connector,
> }
> EXPORT_SYMBOL(drm_dp_mst_connector_early_unregister);
>
> +static void
> +drm_dp_mst_port_add_connector(struct drm_dp_mst_branch *mstb,
> + struct drm_dp_mst_port *port)
> +{
> + struct drm_dp_mst_topology_mgr *mgr = port->mgr;
> + char proppath[255];
> + int ret;
> +
> + build_mst_prop_path(mstb, port->port_num, proppath, sizeof(proppath));
> + port->connector = mgr->cbs->add_connector(mgr, port, proppath);
> + if (!port->connector) {
> + ret = -ENOMEM;
> + goto error;
> + }
> +
> + if ((port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV ||
> + port->pdt == DP_PEER_DEVICE_SST_SINK) &&
> + port->port_num >= DP_MST_LOGICAL_PORT_0) {
> + port->cached_edid = drm_get_edid(port->connector,
> + >aux.ddc);
> + drm_connector_set_tile_property(port->connector);
> + }
> +
> + mgr->cbs->register_connector(port->connector);
> + return;
> +
> +error:
> + DRM_ERROR("Failed to create connector for port %p: %d\n", port, ret);
> +}
> +
> static void
> drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb,
> struct drm_device *dev,
> @@ -1913,8 +1944,12 @@ drm_dp_mst_handle_link_address_port(struct
> drm_dp_mst_branch *mstb,
> {
> struct drm_dp_mst_topology_mgr *mgr = mstb->mgr;
> struct drm_dp_mst_port *port;
> - bool created = false;
> - int old_ddps = 0;
> + struct drm_dp_mst_branch *child_mstb = NULL;
> + struct drm_connector *connector_to_destroy = NULL;
> + int old_ddps = 0, ret;
> + u8 new_pdt = DP_PEER_DEVICE_NONE;
> + bool created = false, send_link_addr = false,
> + create_connector = false;
>
> port = drm_dp_get_port(mstb, port_msg->port_number);
> if (!port) {
> @@ -1923,6 +1958,7 @@ drm_dp_mst_handle_link_address_port(struct
> drm_dp_mst_branch *mstb,
> return;
> kref_init(>topology_kref);
> kref_init(>malloc_kref);
> + mutex_init(>lock);
> port->parent = mstb;
> port->port_num = port_msg->port_number;
> port->mgr = mgr;
> @@ -1937,11 +1973,17 @@ drm_dp_mst_handle_link_address_port(struct
> drm_dp_mst_branch *mstb,
> drm_dp_mst_get_mstb_malloc(mstb);
>
> created = true;
> - } else {
> - old_ddps = port->ddps;
> }
>
> + mutex_lock(>lock);
> + drm_modeset_lock(>mode_config.connection_mutex, NULL);
> +
> + if (!created)
> + old_ddps = port->ddps;
> +
> port->input = port_msg->input_port;
> + if (!port->input)
> + new_pdt = port_msg->peer_device_type;
> port->mcs = port_msg->mcs;
> port->ddps = port_msg->ddps;
> port->ldps = port_msg->legacy_device_plug_status;
> @@ -1969,44 +2011,58 @@ drm_dp_mst_handle_link_address_port(struct
> drm_dp_mst_branch *mstb,
> }
> }
>
> - if (!port->input) {
> - int ret = drm_dp_port_set_pdt(port,
> - port_msg->peer_device_type);
> - if (ret == 1) {
> - drm_dp_send_link_address(mgr, port->mstb);
> - }
Re: [Nouveau] [PATCH v2 19/27] drm/dp_mst: Handle UP requests asynchronously
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 =
Re: [Nouveau] [PATCH v2 18/27] drm/dp_mst: Remove lies in {up, down}_rep_recv documentation
On Tue, Sep 03, 2019 at 04:45:56PM -0400, Lyude Paul wrote:
> These are most certainly accessed from far more than the mgr work. In
> fact, up_req_recv is -only- ever accessed from outside the mgr work.
>
> Cc: Juston Li
> Cc: Imre Deak
> Cc: Ville Syrjälä
> Cc: Harry Wentland
> Cc: Daniel Vetter
> Signed-off-by: Lyude Paul
Reviewed-by: Sean Paul
> ---
> include/drm/drm_dp_mst_helper.h | 8 ++--
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/include/drm/drm_dp_mst_helper.h b/include/drm/drm_dp_mst_helper.h
> index f253ee43e9d9..8ba2a01324bb 100644
> --- a/include/drm/drm_dp_mst_helper.h
> +++ b/include/drm/drm_dp_mst_helper.h
> @@ -489,15 +489,11 @@ struct drm_dp_mst_topology_mgr {
> int conn_base_id;
>
> /**
> - * @down_rep_recv: Message receiver state for down replies. This and
> - * @up_req_recv are only ever access from the work item, which is
> - * serialised.
> + * @down_rep_recv: Message receiver state for down replies.
>*/
> struct drm_dp_sideband_msg_rx down_rep_recv;
> /**
> - * @up_req_recv: Message receiver state for up requests. This and
> - * @down_rep_recv are only ever access from the work item, which is
> - * serialised.
> + * @up_req_recv: Message receiver state for up requests.
>*/
> struct drm_dp_sideband_msg_rx up_req_recv;
>
> --
> 2.21.0
>
--
Sean Paul, Software Engineer, Google / Chromium OS
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH v2 17/27] drm/dp_mst: Rename drm_dp_add_port and drm_dp_update_port
On Tue, Sep 03, 2019 at 04:45:55PM -0400, Lyude Paul wrote:
> The names for these functions are rather confusing. drm_dp_add_port()
> sounds like a function that would simply create a port and add it to a
> topology, and do nothing more. Similarly, drm_dp_update_port() would be
> assumed to be the function that should be used to update port
> information after initial creation.
>
> While those assumptions are currently correct in how these functions are
> used, a quick glance at drm_dp_add_port() reveals that drm_dp_add_port()
> can also update the information on a port, and seems explicitly designed
> to do so. This can be explained pretty simply by the fact that there's
> more situations that would involve updating the port information based
> on a link address response as opposed to a connection status
> notification than the driver's initial topology probe. Case in point:
> reprobing link addresses after suspend/resume.
>
> Since we're about to start using drm_dp_add_port() differently for
> suspend/resume reprobing, let's rename both functions to clarify what
> they actually do.
>
> Cc: Juston Li
> Cc: Imre Deak
> Cc: Ville Syrjälä
> Cc: Harry Wentland
> Cc: Daniel Vetter
> Signed-off-by: Lyude Paul
Reviewed-by: Sean Paul
> ---
> drivers/gpu/drm/drm_dp_mst_topology.c | 17 ++---
> 1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 9944ef2ce885..cfaf9eb7ace9 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -1900,9 +1900,10 @@ void drm_dp_mst_connector_early_unregister(struct
> drm_connector *connector,
> }
> EXPORT_SYMBOL(drm_dp_mst_connector_early_unregister);
>
> -static void drm_dp_add_port(struct drm_dp_mst_branch *mstb,
> - struct drm_device *dev,
> - struct drm_dp_link_addr_reply_port *port_msg)
> +static void
> +drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb,
> + struct drm_device *dev,
> + struct drm_dp_link_addr_reply_port
> *port_msg)
> {
> struct drm_dp_mst_topology_mgr *mgr = mstb->mgr;
> struct drm_dp_mst_port *port;
> @@ -2011,8 +2012,9 @@ static void drm_dp_add_port(struct drm_dp_mst_branch
> *mstb,
> drm_dp_mst_topology_put_port(port);
> }
>
> -static void drm_dp_update_port(struct drm_dp_mst_branch *mstb,
> -struct drm_dp_connection_status_notify
> *conn_stat)
> +static void
> +drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
> + struct drm_dp_connection_status_notify *conn_stat)
> {
> struct drm_dp_mst_port *port;
> int old_ddps;
> @@ -2464,7 +2466,8 @@ static void drm_dp_send_link_address(struct
> drm_dp_mst_topology_mgr *mgr,
> drm_dp_check_mstb_guid(mstb, reply->guid);
>
> for (i = 0; i < reply->nports; i++)
> - drm_dp_add_port(mstb, mgr->dev, >ports[i]);
> + drm_dp_mst_handle_link_address_port(mstb, mgr->dev,
> + >ports[i]);
>
> drm_kms_helper_hotplug_event(mgr->dev);
>
> @@ -3324,7 +3327,7 @@ static int drm_dp_mst_handle_up_req(struct
> drm_dp_mst_topology_mgr *mgr)
> }
>
> if (msg.req_type == DP_CONNECTION_STATUS_NOTIFY) {
> - drm_dp_update_port(mstb, _stat);
> + drm_dp_mst_handle_conn_stat(mstb, _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,
> --
> 2.21.0
>
--
Sean Paul, Software Engineer, Google / Chromium OS
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH v2 16/27] drm/dp_mst: Refactor pdt setup/teardown, add more locking
On Tue, Sep 03, 2019 at 04:45:54PM -0400, Lyude Paul wrote:
> Since we're going to be implementing suspend/resume reprobing very soon,
> we need to make sure we are extra careful to ensure that our locking
> actually protects the topology state where we expect it to. Turns out
> this isn't the case with drm_dp_port_setup_pdt() and
> drm_dp_port_teardown_pdt(), both of which change port->mstb without
> grabbing >lock.
>
> Additionally, since most callers of these functions are just using it to
> teardown the port's previous PDT and setup a new one we can simplify
> things a bit and combine drm_dp_port_setup_pdt() and
> drm_dp_port_teardown_pdt() into a single function:
> drm_dp_port_set_pdt(). This function also handles actually ensuring that
> we grab the correct locks when we need to modify port->mstb.
>
> Cc: Juston Li
> Cc: Imre Deak
> Cc: Ville Syrjälä
> Cc: Harry Wentland
> Cc: Daniel Vetter
> Signed-off-by: Lyude Paul
> ---
> drivers/gpu/drm/drm_dp_mst_topology.c | 181 +++---
> include/drm/drm_dp_mst_helper.h | 6 +-
> 2 files changed, 110 insertions(+), 77 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index d1610434a0cb..9944ef2ce885 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -1487,24 +1487,6 @@ drm_dp_mst_topology_put_mstb(struct drm_dp_mst_branch
> *mstb)
> kref_put(>topology_kref, drm_dp_destroy_mst_branch_device);
> }
>
> -static void drm_dp_port_teardown_pdt(struct drm_dp_mst_port *port, int
> old_pdt)
> -{
> - struct drm_dp_mst_branch *mstb;
> -
> - switch (old_pdt) {
> - case DP_PEER_DEVICE_DP_LEGACY_CONV:
> - case DP_PEER_DEVICE_SST_SINK:
> - /* remove i2c over sideband */
> - drm_dp_mst_unregister_i2c_bus(>aux);
> - break;
> - case DP_PEER_DEVICE_MST_BRANCHING:
> - mstb = port->mstb;
> - port->mstb = NULL;
> - drm_dp_mst_topology_put_mstb(mstb);
> - break;
> - }
> -}
> -
> static void drm_dp_destroy_port(struct kref *kref)
> {
> struct drm_dp_mst_port *port =
> @@ -1714,38 +1696,79 @@ static u8 drm_dp_calculate_rad(struct drm_dp_mst_port
> *port,
> return parent_lct + 1;
> }
>
> -/*
> - * return sends link address for new mstb
> - */
> -static bool drm_dp_port_setup_pdt(struct drm_dp_mst_port *port)
> +static int drm_dp_port_set_pdt(struct drm_dp_mst_port *port, u8 new_pdt)
> {
> - int ret;
> - u8 rad[6], lct;
> - bool send_link = false;
> + struct drm_dp_mst_topology_mgr *mgr = port->mgr;
> + struct drm_dp_mst_branch *mstb;
> + u8 rad[8], lct;
> + int ret = 0;
> +
> + if (port->pdt == new_pdt)
Shouldn't we also ensure that access to port->pdt is also locked?
Sean
> + return 0;
> +
> + /* Teardown the old pdt, if there is one */
> + switch (port->pdt) {
> + case DP_PEER_DEVICE_DP_LEGACY_CONV:
> + case DP_PEER_DEVICE_SST_SINK:
> + /*
> + * If the new PDT would also have an i2c bus, don't bother
> + * with reregistering it
> + */
> + if (new_pdt == DP_PEER_DEVICE_DP_LEGACY_CONV ||
> + new_pdt == DP_PEER_DEVICE_SST_SINK) {
> + port->pdt = new_pdt;
> + return 0;
> + }
> +
> + /* remove i2c over sideband */
> + drm_dp_mst_unregister_i2c_bus(>aux);
> + break;
> + case DP_PEER_DEVICE_MST_BRANCHING:
> + mutex_lock(>lock);
> + drm_dp_mst_topology_put_mstb(port->mstb);
> + port->mstb = NULL;
> + mutex_unlock(>lock);
> + break;
> + }
> +
> + port->pdt = new_pdt;
> switch (port->pdt) {
> case DP_PEER_DEVICE_DP_LEGACY_CONV:
> case DP_PEER_DEVICE_SST_SINK:
> /* add i2c over sideband */
> ret = drm_dp_mst_register_i2c_bus(>aux);
> break;
> +
> case DP_PEER_DEVICE_MST_BRANCHING:
> lct = drm_dp_calculate_rad(port, rad);
> + mstb = drm_dp_add_mst_branch_device(lct, rad);
> + if (!mstb) {
> + ret = -ENOMEM;
> + DRM_ERROR("Failed to create MSTB for port %p", port);
> + goto out;
> + }
>
> - port->mstb = drm_dp_add_mst_branch_device(lct, rad);
> - if (port->mstb) {
> - port->mstb->mgr = port->mgr;
> - port->mstb->port_parent = port;
> - /*
> - * Make sure this port's memory allocation stays
> - * around until its child MSTB releases it
> - */
> - drm_dp_mst_get_port_malloc(port);
> + mutex_lock(>lock);
> + port->mstb = mstb;
> + mstb->mgr =
Re: [Nouveau] [PATCH v2 14/27] drm/dp_mst: Destroy topology_mgr mutexes
On Tue, Sep 03, 2019 at 04:45:52PM -0400, Lyude Paul wrote: > Turns out we've been forgetting for a while now to actually destroy any > of the mutexes that we create in drm_dp_mst_topology_mgr. So, let's do > that. > > Cc: Juston Li > Cc: Imre Deak > Cc: Ville Syrjälä > Cc: Harry Wentland > Cc: Daniel Vetter > Signed-off-by: Lyude Paul Cleanup is overrated :) Reviewed-by: Sean Paul > --- > drivers/gpu/drm/drm_dp_mst_topology.c | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c > b/drivers/gpu/drm/drm_dp_mst_topology.c > index 74161f442584..2f88cc173500 100644 > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > @@ -4339,6 +4339,11 @@ void drm_dp_mst_topology_mgr_destroy(struct > drm_dp_mst_topology_mgr *mgr) > mgr->aux = NULL; > drm_atomic_private_obj_fini(>base); > mgr->funcs = NULL; > + > + mutex_destroy(>delayed_destroy_lock); > + mutex_destroy(>payload_lock); > + mutex_destroy(>qlock); > + mutex_destroy(>lock); > } > EXPORT_SYMBOL(drm_dp_mst_topology_mgr_destroy); > > -- > 2.21.0 > -- Sean Paul, Software Engineer, Google / Chromium OS ___ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH v2 08/27] drm/dp_mst: Remove PDT teardown in drm_dp_destroy_port() and refactor
On Tue, Sep 03, 2019 at 04:45:46PM -0400, Lyude Paul wrote:
> This will allow us to add some locking for port->* members, in
> particular the PDT and ->connector, which can't be done from
> drm_dp_destroy_port() since we don't know what locks the caller might be
> holding.
Might be nice to mention that this is already done in the delayed destroy worker
so readers don't need to go looking for it. Perhaps update this when you apply
the patch.
>
> Changes since v2:
> * Clarify commit message
>
> Cc: Juston Li
> Cc: Imre Deak
> Cc: Ville Syrjälä
> Cc: Harry Wentland
> Cc: Daniel Vetter
> Signed-off-by: Lyude Paul
Reviewed-by: Sean Paul
> ---
> drivers/gpu/drm/drm_dp_mst_topology.c | 40 +++
> 1 file changed, 16 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index f5f1d8b50fb6..af3189df28aa 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -1511,31 +1511,22 @@ static void drm_dp_destroy_port(struct kref *kref)
> container_of(kref, struct drm_dp_mst_port, topology_kref);
> struct drm_dp_mst_topology_mgr *mgr = port->mgr;
>
> - if (!port->input) {
> - kfree(port->cached_edid);
> + /* There's nothing that needs locking to destroy an input port yet */
> + if (port->input) {
> + drm_dp_mst_put_port_malloc(port);
> + return;
> + }
>
> - /*
> - * The only time we don't have a connector
> - * on an output port is if the connector init
> - * fails.
> - */
> - if (port->connector) {
> - /* we can't destroy the connector here, as
> - * we might be holding the mode_config.mutex
> - * from an EDID retrieval */
> + kfree(port->cached_edid);
>
> - mutex_lock(>delayed_destroy_lock);
> - list_add(>next, >destroy_port_list);
> - mutex_unlock(>delayed_destroy_lock);
> - schedule_work(>delayed_destroy_work);
> - return;
> - }
> - /* no need to clean up vcpi
> - * as if we have no connector we never setup a vcpi */
> - drm_dp_port_teardown_pdt(port, port->pdt);
> - port->pdt = DP_PEER_DEVICE_NONE;
> - }
> - drm_dp_mst_put_port_malloc(port);
> + /*
> + * we can't destroy the connector here, as we might be holding the
> + * mode_config.mutex from an EDID retrieval
> + */
> + mutex_lock(>delayed_destroy_lock);
> + list_add(>next, >destroy_port_list);
> + mutex_unlock(>delayed_destroy_lock);
> + schedule_work(>delayed_destroy_work);
> }
>
> /**
> @@ -3998,7 +3989,8 @@ static void drm_dp_tx_work(struct work_struct *work)
> static inline void
> drm_dp_delayed_destroy_port(struct drm_dp_mst_port *port)
> {
> - port->mgr->cbs->destroy_connector(port->mgr, port->connector);
> + if (port->connector)
> + port->mgr->cbs->destroy_connector(port->mgr, port->connector);
>
> drm_dp_port_teardown_pdt(port, port->pdt);
> port->pdt = DP_PEER_DEVICE_NONE;
> --
> 2.21.0
>
--
Sean Paul, Software Engineer, Google / Chromium OS
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH v2 04/27] drm/dp_mst: Move test_calc_pbn_mode() into an actual selftest
On Tue, Sep 03, 2019 at 04:45:42PM -0400, Lyude Paul wrote:
> Yes, apparently we've been testing this for every single driver load for
> quite a long time now. At least that means our PBN calculation is solid!
>
> Anyway, introduce self tests for MST and move this into there.
>
> Cc: Juston Li
> Cc: Imre Deak
> Cc: Ville Syrjälä
> Cc: Harry Wentland
> Reviewed-by: Daniel Vetter
> Signed-off-by: Lyude Paul
Reviewed-by: Sean Paul
> ---
> drivers/gpu/drm/drm_dp_mst_topology.c | 27 ---
> drivers/gpu/drm/selftests/Makefile| 2 +-
> .../gpu/drm/selftests/drm_modeset_selftests.h | 1 +
> .../drm/selftests/test-drm_dp_mst_helper.c| 34 +++
> .../drm/selftests/test-drm_modeset_common.h | 1 +
> 5 files changed, 37 insertions(+), 28 deletions(-)
> create mode 100644 drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 738f260d4b15..6f7f449ca12b 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -47,7 +47,6 @@
> */
> static bool dump_dp_payload_table(struct drm_dp_mst_topology_mgr *mgr,
> char *buf);
> -static int test_calc_pbn_mode(void);
>
> static void drm_dp_mst_topology_put_port(struct drm_dp_mst_port *port);
>
> @@ -3561,30 +3560,6 @@ int drm_dp_calc_pbn_mode(int clock, int bpp)
> }
> EXPORT_SYMBOL(drm_dp_calc_pbn_mode);
>
> -static int test_calc_pbn_mode(void)
> -{
> - int ret;
> - ret = drm_dp_calc_pbn_mode(154000, 30);
> - if (ret != 689) {
> - DRM_ERROR("PBN calculation test failed - clock %d, bpp %d,
> expected PBN %d, actual PBN %d.\n",
> - 154000, 30, 689, ret);
> - return -EINVAL;
> - }
> - ret = drm_dp_calc_pbn_mode(234000, 30);
> - if (ret != 1047) {
> - DRM_ERROR("PBN calculation test failed - clock %d, bpp %d,
> expected PBN %d, actual PBN %d.\n",
> - 234000, 30, 1047, ret);
> - return -EINVAL;
> - }
> - ret = drm_dp_calc_pbn_mode(297000, 24);
> - if (ret != 1063) {
> - DRM_ERROR("PBN calculation test failed - clock %d, bpp %d,
> expected PBN %d, actual PBN %d.\n",
> - 297000, 24, 1063, ret);
> - return -EINVAL;
> - }
> - return 0;
> -}
> -
> /* we want to kick the TX after we've ack the up/down IRQs. */
> static void drm_dp_mst_kick_tx(struct drm_dp_mst_topology_mgr *mgr)
> {
> @@ -4033,8 +4008,6 @@ int drm_dp_mst_topology_mgr_init(struct
> drm_dp_mst_topology_mgr *mgr,
> if (!mgr->proposed_vcpis)
> return -ENOMEM;
> set_bit(0, >payload_mask);
> - if (test_calc_pbn_mode() < 0)
> - DRM_ERROR("MST PBN self-test failed\n");
>
> mst_state = kzalloc(sizeof(*mst_state), GFP_KERNEL);
> if (mst_state == NULL)
> diff --git a/drivers/gpu/drm/selftests/Makefile
> b/drivers/gpu/drm/selftests/Makefile
> index aae88f8a016c..d2137342b371 100644
> --- a/drivers/gpu/drm/selftests/Makefile
> +++ b/drivers/gpu/drm/selftests/Makefile
> @@ -1,6 +1,6 @@
> # SPDX-License-Identifier: GPL-2.0-only
> test-drm_modeset-y := test-drm_modeset_common.o test-drm_plane_helper.o \
>test-drm_format.o test-drm_framebuffer.o \
> - test-drm_damage_helper.o
> + test-drm_damage_helper.o test-drm_dp_mst_helper.o
>
> obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o test-drm_modeset.o
> test-drm_cmdline_parser.o
> diff --git a/drivers/gpu/drm/selftests/drm_modeset_selftests.h
> b/drivers/gpu/drm/selftests/drm_modeset_selftests.h
> index 464753746013..dec3ee3ec96f 100644
> --- a/drivers/gpu/drm/selftests/drm_modeset_selftests.h
> +++ b/drivers/gpu/drm/selftests/drm_modeset_selftests.h
> @@ -32,3 +32,4 @@ selftest(damage_iter_damage_one_intersect,
> igt_damage_iter_damage_one_intersect)
> selftest(damage_iter_damage_one_outside, igt_damage_iter_damage_one_outside)
> selftest(damage_iter_damage_src_moved, igt_damage_iter_damage_src_moved)
> selftest(damage_iter_damage_not_visible, igt_damage_iter_damage_not_visible)
> +selftest(dp_mst_calc_pbn_mode, igt_dp_mst_calc_pbn_mode)
> diff --git a/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c
> b/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c
> new file mode 100644
> index ..9baa5171988d
> --- /dev/null
> +++ b/drivers/gpu/drm/selftests/test-drm_dp_mst_helper.c
> @@ -0,0 +1,34 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Test cases for for the DRM DP MST helpers
> + */
> +
> +#include
> +#include
> +
> +#include "test-drm_modeset_common.h"
> +
> +int igt_dp_mst_calc_pbn_mode(void *ignored)
> +{
> + int pbn, i;
> + const struct {
> + int rate;
> + int bpp;
> + int expected;
> + } test_params[] = {
> + {
Re: [Nouveau] [PATCH v2 03/27] drm/dp_mst: Destroy MSTBs asynchronously
On Tue, Sep 03, 2019 at 04:45:41PM -0400, Lyude Paul wrote:
> When reprobing an MST topology during resume, we have to account for the
> fact that while we were suspended it's possible that mstbs may have been
> removed from any ports in the topology. Since iterating downwards in the
> topology requires that we hold >lock, destroying MSTBs from this
> context would result in attempting to lock >lock a second time and
> deadlocking.
>
> So, fix this by first moving destruction of MSTBs into
> destroy_connector_work, then rename destroy_connector_work and friends
> to reflect that they now destroy both ports and mstbs.
>
> Changes since v1:
> * s/destroy_connector_list/destroy_port_list/
> s/connector_destroy_lock/delayed_destroy_lock/
> s/connector_destroy_work/delayed_destroy_work/
> s/drm_dp_finish_destroy_branch_device/drm_dp_delayed_destroy_mstb/
> s/drm_dp_finish_destroy_port/drm_dp_delayed_destroy_port/
> - danvet
> * Use two loops in drm_dp_delayed_destroy_work() - danvet
> * Better explain why we need to do this - danvet
> * Use cancel_work_sync() instead of flush_work() - flush_work() doesn't
> account for work requeing
>
> Cc: Juston Li
> Cc: Imre Deak
> Cc: Ville Syrjälä
> Cc: Harry Wentland
> Cc: Daniel Vetter
> Signed-off-by: Lyude Paul
Took me a while to grok this, and I'm still not 100% confident my mental model
is correct, so please bear with me while I ask silly questions :)
Now that the destroy is delayed, and the port remains in the topology, is it
possible we will underflow the topology kref by calling put_mstb multiple times?
It looks like that would result in a WARN from refcount.c, and wouldn't call the
destroy function multiple times, so that's nice :)
Similarly, is there any defense against calling get_mstb() between destroy() and
the delayed destroy worker running?
Sean
> ---
> drivers/gpu/drm/drm_dp_mst_topology.c | 162 +-
> include/drm/drm_dp_mst_helper.h | 26 +++--
> 2 files changed, 127 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 3054ec622506..738f260d4b15 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -1113,34 +1113,17 @@ static void drm_dp_destroy_mst_branch_device(struct
> kref *kref)
> struct drm_dp_mst_branch *mstb =
> container_of(kref, struct drm_dp_mst_branch, topology_kref);
> struct drm_dp_mst_topology_mgr *mgr = mstb->mgr;
> - struct drm_dp_mst_port *port, *tmp;
> - bool wake_tx = false;
>
> - mutex_lock(>lock);
> - list_for_each_entry_safe(port, tmp, >ports, next) {
> - list_del(>next);
> - drm_dp_mst_topology_put_port(port);
> - }
> - mutex_unlock(>lock);
> -
> - /* drop any tx slots msg */
> - mutex_lock(>mgr->qlock);
> - if (mstb->tx_slots[0]) {
> - mstb->tx_slots[0]->state = DRM_DP_SIDEBAND_TX_TIMEOUT;
> - mstb->tx_slots[0] = NULL;
> - wake_tx = true;
> - }
> - if (mstb->tx_slots[1]) {
> - mstb->tx_slots[1]->state = DRM_DP_SIDEBAND_TX_TIMEOUT;
> - mstb->tx_slots[1] = NULL;
> - wake_tx = true;
> - }
> - mutex_unlock(>mgr->qlock);
> + INIT_LIST_HEAD(>destroy_next);
>
> - if (wake_tx)
> - wake_up_all(>mgr->tx_waitq);
> -
> - drm_dp_mst_put_mstb_malloc(mstb);
> + /*
> + * This can get called under mgr->mutex, so we need to perform the
> + * actual destruction of the mstb in another worker
> + */
> + mutex_lock(>delayed_destroy_lock);
> + list_add(>destroy_next, >destroy_branch_device_list);
> + mutex_unlock(>delayed_destroy_lock);
> + schedule_work(>delayed_destroy_work);
> }
>
> /**
> @@ -1255,10 +1238,10 @@ static void drm_dp_destroy_port(struct kref *kref)
>* we might be holding the mode_config.mutex
>* from an EDID retrieval */
>
> - mutex_lock(>destroy_connector_lock);
> - list_add(>next, >destroy_connector_list);
> - mutex_unlock(>destroy_connector_lock);
> - schedule_work(>destroy_connector_work);
> + mutex_lock(>delayed_destroy_lock);
> + list_add(>next, >destroy_port_list);
> + mutex_unlock(>delayed_destroy_lock);
> + schedule_work(>delayed_destroy_work);
> return;
> }
> /* no need to clean up vcpi
> @@ -2792,7 +2775,7 @@ void drm_dp_mst_topology_mgr_suspend(struct
> drm_dp_mst_topology_mgr *mgr)
> DP_MST_EN | DP_UPSTREAM_IS_SRC);
> mutex_unlock(>lock);
> flush_work(>work);
> - flush_work(>destroy_connector_work);
> + flush_work(>delayed_destroy_work);
> }
> EXPORT_SYMBOL(drm_dp_mst_topology_mgr_suspend);
>
Re: [Nouveau] [PATCH v2 02/27] drm/dp_mst: Get rid of list clear in destroy_connector_work
On Tue, Sep 03, 2019 at 04:45:40PM -0400, Lyude Paul wrote: > This seems to be some leftover detritus from before the port/mstb kref > cleanup and doesn't do anything anymore, so get rid of it. > > Cc: Juston Li > Cc: Imre Deak > Cc: Ville Syrjälä > Cc: Harry Wentland > Reviewed-by: Daniel Vetter > Signed-off-by: Lyude Paul Reviewed-by: Sean Paul > --- > drivers/gpu/drm/drm_dp_mst_topology.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c > b/drivers/gpu/drm/drm_dp_mst_topology.c > index 36db66a0ddb1..3054ec622506 100644 > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > @@ -3760,8 +3760,6 @@ static void drm_dp_destroy_connector_work(struct > work_struct *work) > list_del(>next); > mutex_unlock(>destroy_connector_lock); > > - INIT_LIST_HEAD(>next); > - > mgr->cbs->destroy_connector(mgr, port->connector); > > drm_dp_port_teardown_pdt(port, port->pdt); > -- > 2.21.0 > -- Sean Paul, Software Engineer, Google / Chromium OS ___ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH v2 01/27] drm/dp_mst: Move link address dumping into a function
On Tue, Sep 03, 2019 at 04:45:39PM -0400, Lyude Paul wrote:
> Makes things easier to read.
>
> Cc: Juston Li
> Cc: Imre Deak
> Cc: Ville Syrjälä
> Cc: Harry Wentland
> Reviewed-by: Daniel Vetter
> Signed-off-by: Lyude Paul
Reviewed-by: Sean Paul
> ---
> drivers/gpu/drm/drm_dp_mst_topology.c | 35 ++-
> 1 file changed, 23 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 82add736e17d..36db66a0ddb1 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -2103,6 +2103,28 @@ static void drm_dp_queue_down_tx(struct
> drm_dp_mst_topology_mgr *mgr,
> mutex_unlock(>qlock);
> }
>
> +static void
> +drm_dp_dump_link_address(struct drm_dp_link_address_ack_reply *reply)
> +{
> + struct drm_dp_link_addr_reply_port *port_reply;
> + int i;
> +
> + for (i = 0; i < reply->nports; i++) {
> + port_reply = >ports[i];
> + DRM_DEBUG_KMS("port %d: input %d, pdt: %d, pn: %d, dpcd_rev:
> %02x, mcs: %d, ddps: %d, ldps %d, sdp %d/%d\n",
> + i,
> + port_reply->input_port,
> + port_reply->peer_device_type,
> + port_reply->port_number,
> + port_reply->dpcd_revision,
> + port_reply->mcs,
> + port_reply->ddps,
> + port_reply->legacy_device_plug_status,
> + port_reply->num_sdp_streams,
> + port_reply->num_sdp_stream_sinks);
> + }
> +}
> +
> static void drm_dp_send_link_address(struct drm_dp_mst_topology_mgr *mgr,
>struct drm_dp_mst_branch *mstb)
> {
> @@ -2128,18 +2150,7 @@ static void drm_dp_send_link_address(struct
> drm_dp_mst_topology_mgr *mgr,
> DRM_DEBUG_KMS("link address nak received\n");
> } else {
> DRM_DEBUG_KMS("link address reply: %d\n",
> txmsg->reply.u.link_addr.nports);
> - for (i = 0; i < txmsg->reply.u.link_addr.nports; i++) {
> - DRM_DEBUG_KMS("port %d: input %d, pdt: %d, pn:
> %d, dpcd_rev: %02x, mcs: %d, ddps: %d, ldps %d, sdp %d/%d\n", i,
> -
> txmsg->reply.u.link_addr.ports[i].input_port,
> -
> txmsg->reply.u.link_addr.ports[i].peer_device_type,
> -
> txmsg->reply.u.link_addr.ports[i].port_number,
> -
> txmsg->reply.u.link_addr.ports[i].dpcd_revision,
> -txmsg->reply.u.link_addr.ports[i].mcs,
> -txmsg->reply.u.link_addr.ports[i].ddps,
> -
> txmsg->reply.u.link_addr.ports[i].legacy_device_plug_status,
> -
> txmsg->reply.u.link_addr.ports[i].num_sdp_streams,
> -
> txmsg->reply.u.link_addr.ports[i].num_sdp_stream_sinks);
> - }
> + drm_dp_dump_link_address(>reply.u.link_addr);
>
> drm_dp_check_mstb_guid(mstb,
> txmsg->reply.u.link_addr.guid);
>
> --
> 2.21.0
>
--
Sean Paul, Software Engineer, Google / Chromium OS
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
