On Wed, 2020-01-08 at 16:44 +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: > - 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() > > Fixes: c485e2c97dae ("drm/dp_mst: Refactor pdt setup/teardown, add more > locking") > Cc: Ville Syrjälä <ville.syrj...@linux.intel.com> > Cc: Harry Wentland <hwent...@amd.com> > Cc: Lyude Paul <ly...@redhat.com> > Signed-off-by: Wayne Lin <wayne....@amd.com> > --- > drivers/gpu/drm/drm_dp_mst_topology.c | 131 +++++++++++++------------- > 1 file changed, 68 insertions(+), 63 deletions(-) > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c > b/drivers/gpu/drm/drm_dp_mst_topology.c > index 8f54b241db08..4395d5cc0645 100644 > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > @@ -1934,73 +1934,74 @@ static bool drm_dp_mst_is_dp_mst_end_device(u8 pdt, > bool mcs) > return true; > } > > -static int drm_dp_port_set_pdt(struct drm_dp_mst_port *port, u8 new_pdt) > +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: > @@ -2153,12 +2154,12 @@ 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) { > - port->cached_edid = drm_get_edid(port->connector, > - &port->aux.ddc); > - drm_connector_set_tile_property(port->connector); > + if (port->pdt != DP_PEER_DEVICE_NONE) { > + if (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); > + }
I'd combine these two if statements here into one, otherwise this looks great. Thank you for all of the great fixes recently :) Reviewed-by: Lyude Paul <ly...@redhat.com> > } > > mgr->cbs->register_connector(port->connector); > @@ -2223,6 +2224,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); > @@ -2267,7 +2269,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; > @@ -2295,7 +2297,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) { > @@ -2309,7 +2311,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) > @@ -2346,6 +2349,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); > @@ -2377,7 +2381,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; > > @@ -2390,8 +2393,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) { > @@ -3958,6 +3961,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: > @@ -4597,7 +4602,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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel