Skip to content

Commit

Permalink
drm/dp_mst: Refactor pdt setup/teardown, add more locking
Browse files Browse the repository at this point in the history 
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 &mgr->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 <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>
Reviewed-by: Sean Paul <sean@poorly.run>
Signed-off-by: Lyude Paul <lyude@redhat.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20191022023641.8026-4-lyude@redhat.com
  • Loading branch information
Lyude Paul committed Oct 24, 2019
1 parent d29333c commit c485e2c
Show file tree
Hide file tree
Showing 2 changed files with 110 additions and 77 deletions.
181 changes: 106 additions & 75 deletions drivers/gpu/drm/drm_dp_mst_topology.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1486,24 +1486,6 @@ drm_dp_mst_topology_put_mstb(struct drm_dp_mst_branch *mstb)
kref_put(&mstb->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(&port->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 =
Expand Down Expand Up @@ -1713,38 +1695,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)
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(&port->aux);
break;
case DP_PEER_DEVICE_MST_BRANCHING:
mutex_lock(&mgr->lock);
drm_dp_mst_topology_put_mstb(port->mstb);
port->mstb = NULL;
mutex_unlock(&mgr->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(&port->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(&mgr->lock);
port->mstb = mstb;
mstb->mgr = port->mgr;
mstb->port_parent = port;

send_link = true;
}
/*
* Make sure this port's memory allocation stays
* around until its child MSTB releases it
*/
drm_dp_mst_get_port_malloc(port);
mutex_unlock(&mgr->lock);

/* And make sure we send a link address for this */
ret = 1;
break;
}
return send_link;

out:
if (ret < 0)
port->pdt = DP_PEER_DEVICE_NONE;
return ret;
}

/**
Expand Down Expand Up @@ -1881,10 +1904,9 @@ 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;
bool ret;
bool created = false;
int old_pdt = 0;
int old_ddps = 0;

port = drm_dp_get_port(mstb, port_msg->port_number);
Expand All @@ -1896,7 +1918,7 @@ drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb,
kref_init(&port->malloc_kref);
port->parent = mstb;
port->port_num = port_msg->port_number;
port->mgr = mstb->mgr;
port->mgr = mgr;
port->aux.name = "DPMST";
port->aux.dev = dev->dev;
port->aux.is_remote = true;
Expand All @@ -1909,11 +1931,9 @@ drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb,

created = true;
} else {
old_pdt = port->pdt;
old_ddps = port->ddps;
}

port->pdt = port_msg->peer_device_type;
port->input = port_msg->input_port;
port->mcs = port_msg->mcs;
port->ddps = port_msg->ddps;
Expand All @@ -1925,78 +1945,85 @@ drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb,
/* manage mstb port lists with mgr lock - take a reference
for this list */
if (created) {
mutex_lock(&mstb->mgr->lock);
mutex_lock(&mgr->lock);
drm_dp_mst_topology_get_port(port);
list_add(&port->next, &mstb->ports);
mutex_unlock(&mstb->mgr->lock);
mutex_unlock(&mgr->lock);
}

if (old_ddps != port->ddps) {
if (port->ddps) {
if (!port->input) {
drm_dp_send_enum_path_resources(mstb->mgr,
mstb, port);
drm_dp_send_enum_path_resources(mgr, mstb,
port);
}
} else {
port->available_pbn = 0;
}
}

if (old_pdt != port->pdt && !port->input) {
drm_dp_port_teardown_pdt(port, old_pdt);

ret = drm_dp_port_setup_pdt(port);
if (ret == true)
drm_dp_send_link_address(mstb->mgr, port->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);
} else if (ret < 0) {
DRM_ERROR("Failed to change PDT on port %p: %d\n",
port, ret);
goto fail;
}
}

if (created && !port->input) {
char proppath[255];

build_mst_prop_path(mstb, port->port_num, proppath,
sizeof(proppath));
port->connector = (*mstb->mgr->cbs->add_connector)(mstb->mgr,
port,
proppath);
if (!port->connector) {
/* remove it from the port list */
mutex_lock(&mstb->mgr->lock);
list_del(&port->next);
mutex_unlock(&mstb->mgr->lock);
/* drop port list reference */
drm_dp_mst_topology_put_port(port);
goto out;
}
port->connector = (*mgr->cbs->add_connector)(mgr, port,
proppath);
if (!port->connector)
goto fail;

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,
&port->aux.ddc);
drm_connector_set_tile_property(port->connector);
}
(*mstb->mgr->cbs->register_connector)(port->connector);

(*mgr->cbs->register_connector)(port->connector);
}

out:
/* put reference to this port */
drm_dp_mst_topology_put_port(port);
return;

fail:
/* Remove it from the port list */
mutex_lock(&mgr->lock);
list_del(&port->next);
mutex_unlock(&mgr->lock);

/* Drop the port list reference */
drm_dp_mst_topology_put_port(port);
/* And now drop our reference */
drm_dp_mst_topology_put_port(port);
}

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_pdt;
int old_ddps;
bool dowork = false;

port = drm_dp_get_port(mstb, conn_stat->port_number);
if (!port)
return;

old_ddps = port->ddps;
old_pdt = port->pdt;
port->pdt = conn_stat->peer_device_type;
port->mcs = conn_stat->message_capability_status;
port->ldps = conn_stat->legacy_device_plug_status;
port->ddps = conn_stat->displayport_device_plug_status;
Expand All @@ -2008,11 +2035,17 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
port->available_pbn = 0;
}
}
if (old_pdt != port->pdt && !port->input) {
drm_dp_port_teardown_pdt(port, old_pdt);

if (drm_dp_port_setup_pdt(port))
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;
}
}

drm_dp_mst_topology_put_port(port);
Expand Down Expand Up @@ -3975,9 +4008,7 @@ drm_dp_delayed_destroy_port(struct drm_dp_mst_port *port)
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;

drm_dp_port_set_pdt(port, DP_PEER_DEVICE_NONE);
drm_dp_mst_put_port_malloc(port);
}

Expand Down
6 changes: 4 additions & 2 deletions include/drm/drm_dp_mst_helper.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,10 @@ struct drm_dp_vcpi {
* @num_sdp_stream_sinks: Number of stream sinks
* @available_pbn: Available bandwidth for this port.
* @next: link to next port on this branch device
* @mstb: branch device attach below this port
* @aux: i2c aux transport to talk to device connected to this port.
* @mstb: branch device on this port, protected by
* &drm_dp_mst_topology_mgr.lock
* @aux: i2c aux transport to talk to device connected to this port, protected
* by &drm_dp_mst_topology_mgr.lock
* @parent: branch device parent of this port
* @vcpi: Virtual Channel Payload info for this port.
* @connector: DRM connector this port is connected to.
Expand Down

0 comments on commit c485e2c

Please sign in to comment.