Pushed to drm-misc-fixes, thanks! On Fri, 2020-01-17 at 14:03 +0800, Wayne Lin wrote: > [Why] > While handling LINK_ADDRESS reply, current code expects a peer device > can handle sideband message once the peer device type is reported as > DP_PEER_DEVICE_MST_BRANCHING. However, when the connected device is > a SST branch case, it can't handle the sideband message(MST_CAP=0 in > DPCD 00021h). > > Current code will try to send LINK_ADDRESS to SST branch device and end > up with message timeout and monitor can't display normally. As the > result of that, we should take SST branch device into account. > > [How] > According to DP 1.4 spec, we can use Peer_Device_Type as > DP_PEER_DEVICE_MST_BRANCHING and Message_Capability_Status as 0 to > indicate peer device as a SST-only branch device. > > Fix following: > - Add the function drm_dp_mst_is_dp_mst_end_device() to decide whether a > peer device connected to a DFP is mst end device. Which also indicates > if the peer device is capable of handling message or not. > - Take SST-only branch device case into account in > drm_dp_port_set_pdt() and add a new parameter 'new_mcs'. Take sst branch > device case as the same case as DP_PEER_DEVICE_DP_LEGACY_CONV and > DP_PEER_DEVICE_SST_SINK. All original handling logics remain. > - Take SST-only branch device case into account in > drm_dp_mst_port_add_connector(). > - Fix some parts in drm_dp_mst_handle_link_address_port() to have SST > branch device case into consideration. > - Fix the arguments of drm_dp_port_set_pdt() in > drm_dp_mst_handle_conn_stat(). > - Have SST branch device also report > connector_status_connected when the ddps is true > in drm_dp_mst_detect_port() > - Fix the arguments of drm_dp_port_set_pdt() in > drm_dp_delayed_destroy_port() > > Changes since v1:(https://patchwork.kernel.org/patch/11323079/) > * Squash previous patch into one patch and merge the commit message here. > * Combine the if statements mentioned in comments > > Fixes: c485e2c97dae ("drm/dp_mst: Refactor pdt setup/teardown, add more > locking") > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > Cc: Harry Wentland <hwentlan@xxxxxxx> > Cc: Lyude Paul <lyude@xxxxxxxxxx> > Signed-off-by: Wayne Lin <Wayne.Lin@xxxxxxx> > Reviewed-by: Lyude Paul <lyude@xxxxxxxxxx> > --- > drivers/gpu/drm/drm_dp_mst_topology.c | 140 +++++++++++++++----------- > 1 file changed, 80 insertions(+), 60 deletions(-) > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c > b/drivers/gpu/drm/drm_dp_mst_topology.c > index 79691c553182..c40a9bd63cfb 100644 > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > @@ -1914,73 +1914,90 @@ static u8 drm_dp_calculate_rad(struct > drm_dp_mst_port *port, > return parent_lct + 1; > } > > -static int drm_dp_port_set_pdt(struct drm_dp_mst_port *port, u8 new_pdt) > +static bool drm_dp_mst_is_dp_mst_end_device(u8 pdt, bool mcs) > +{ > + switch (pdt) { > + case DP_PEER_DEVICE_DP_LEGACY_CONV: > + case DP_PEER_DEVICE_SST_SINK: > + return true; > + case DP_PEER_DEVICE_MST_BRANCHING: > + /* For sst branch device */ > + if (!mcs) > + return true; > + > + return false; > + } > + return true; > +} > + > +static int > +drm_dp_port_set_pdt(struct drm_dp_mst_port *port, u8 new_pdt, > + bool new_mcs) > { > 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) > + if (port->pdt == new_pdt && port->mcs == new_mcs) > 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; > - } > + if (port->pdt != DP_PEER_DEVICE_NONE) { > + if (drm_dp_mst_is_dp_mst_end_device(port->pdt, port->mcs)) { > + /* > + * If the new PDT would also have an i2c bus, > + * don't bother with reregistering it > + */ > + if (new_pdt != DP_PEER_DEVICE_NONE && > + drm_dp_mst_is_dp_mst_end_device(new_pdt, new_mcs)) > { > + port->pdt = new_pdt; > + port->mcs = new_mcs; > + 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; > + /* remove i2c over sideband */ > + drm_dp_mst_unregister_i2c_bus(&port->aux); > + } else { > + mutex_lock(&mgr->lock); > + drm_dp_mst_topology_put_mstb(port->mstb); > + port->mstb = NULL; > + mutex_unlock(&mgr->lock); > + } > } > > 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; > + port->mcs = new_mcs; > > - 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; > - } > + if (port->pdt != DP_PEER_DEVICE_NONE) { > + if (drm_dp_mst_is_dp_mst_end_device(port->pdt, port->mcs)) { > + /* add i2c over sideband */ > + ret = drm_dp_mst_register_i2c_bus(&port->aux); > + } else { > + 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; > + } > > - mutex_lock(&mgr->lock); > - port->mstb = mstb; > - mstb->mgr = port->mgr; > - mstb->port_parent = port; > + mutex_lock(&mgr->lock); > + port->mstb = mstb; > + mstb->mgr = port->mgr; > + 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_unlock(&mgr->lock); > + /* > + * 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; > + /* And make sure we send a link address for this */ > + ret = 1; > + } > } > > out: > @@ -2133,9 +2150,8 @@ drm_dp_mst_port_add_connector(struct drm_dp_mst_branch > *mstb, > 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) { > + if (port->pdt != DP_PEER_DEVICE_NONE && > + drm_dp_mst_is_dp_mst_end_device(port->pdt, port->mcs)) { > port->cached_edid = drm_get_edid(port->connector, > &port->aux.ddc); > drm_connector_set_tile_property(port->connector); > @@ -2203,6 +2219,7 @@ drm_dp_mst_handle_link_address_port(struct > drm_dp_mst_branch *mstb, > struct drm_dp_mst_port *port; > int old_ddps = 0, ret; > u8 new_pdt = DP_PEER_DEVICE_NONE; > + bool new_mcs = 0; > bool created = false, send_link_addr = false, changed = false; > > port = drm_dp_get_port(mstb, port_msg->port_number); > @@ -2247,7 +2264,7 @@ drm_dp_mst_handle_link_address_port(struct > drm_dp_mst_branch *mstb, > port->input = port_msg->input_port; > if (!port->input) > new_pdt = port_msg->peer_device_type; > - port->mcs = port_msg->mcs; > + new_mcs = port_msg->mcs; > port->ddps = port_msg->ddps; > port->ldps = port_msg->legacy_device_plug_status; > port->dpcd_rev = port_msg->dpcd_revision; > @@ -2275,7 +2292,7 @@ drm_dp_mst_handle_link_address_port(struct > drm_dp_mst_branch *mstb, > } > } > > - ret = drm_dp_port_set_pdt(port, new_pdt); > + ret = drm_dp_port_set_pdt(port, new_pdt, new_mcs); > if (ret == 1) { > send_link_addr = true; > } else if (ret < 0) { > @@ -2289,7 +2306,8 @@ drm_dp_mst_handle_link_address_port(struct > drm_dp_mst_branch *mstb, > * we're coming out of suspend. In this case, always resend the link > * address if there's an MSTB on this port > */ > - if (!created && port->pdt == DP_PEER_DEVICE_MST_BRANCHING) > + if (!created && port->pdt == DP_PEER_DEVICE_MST_BRANCHING && > + port->mcs) > send_link_addr = true; > > if (port->connector) > @@ -2326,6 +2344,7 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch > *mstb, > struct drm_dp_mst_port *port; > int old_ddps, old_input, ret, i; > u8 new_pdt; > + bool new_mcs; > bool dowork = false, create_connector = false; > > port = drm_dp_get_port(mstb, conn_stat->port_number); > @@ -2357,7 +2376,6 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch > *mstb, > old_ddps = port->ddps; > old_input = port->input; > 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; > > @@ -2370,8 +2388,8 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch > *mstb, > } > > new_pdt = port->input ? DP_PEER_DEVICE_NONE : conn_stat- > >peer_device_type; > - > - ret = drm_dp_port_set_pdt(port, new_pdt); > + new_mcs = conn_stat->message_capability_status; > + ret = drm_dp_port_set_pdt(port, new_pdt, new_mcs); > if (ret == 1) { > dowork = true; > } else if (ret < 0) { > @@ -3938,6 +3956,8 @@ drm_dp_mst_detect_port(struct drm_connector > *connector, > switch (port->pdt) { > case DP_PEER_DEVICE_NONE: > case DP_PEER_DEVICE_MST_BRANCHING: > + if (!port->mcs) > + ret = connector_status_connected; > break; > > case DP_PEER_DEVICE_SST_SINK: > @@ -4572,7 +4592,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_set_pdt(port, DP_PEER_DEVICE_NONE); > + drm_dp_port_set_pdt(port, DP_PEER_DEVICE_NONE, port->mcs); > drm_dp_mst_put_port_malloc(port); > } > -- Cheers, Lyude Paul _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
