| Message ID | 20200108084416.6296-3-Wayne.Lin@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Take SST-only branch device into account | expand |
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.syrjala@linux.intel.com> > Cc: Harry Wentland <hwentlan@amd.com> > Cc: Lyude Paul <lyude@redhat.com> > Signed-off-by: Wayne Lin <Wayne.Lin@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 <lyude@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); > } >
[AMD Public Use] > -----Original Message----- > From: Lyude Paul <lyude@redhat.com> > Sent: Wednesday, January 15, 2020 5:19 AM > To: Lin, Wayne <Wayne.Lin@amd.com>; dri-devel@lists.freedesktop.org; > amd-gfx@lists.freedesktop.org > Cc: Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Wentland, Harry > <Harry.Wentland@amd.com>; Zuo, Jerry <Jerry.Zuo@amd.com>; Ville Syrjälä > <ville.syrjala@linux.intel.com>; Wentland, Harry <Harry.Wentland@amd.com> > Subject: Re: [PATCH 2/2] drm/dp_mst: Handle SST-only branch device case > > 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.syrjala@linux.intel.com> > > Cc: Harry Wentland <hwentlan@amd.com> > > Cc: Lyude Paul <lyude@redhat.com> > > Signed-off-by: Wayne Lin <Wayne.Lin@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 <lyude@redhat.com> Thanks for your time! I will fix it in the new version. Thanks. > > > > } > > > > 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 -- Best regards, Wayne Lin
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); + } } 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); }
[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.syrjala@linux.intel.com> Cc: Harry Wentland <hwentlan@amd.com> Cc: Lyude Paul <lyude@redhat.com> Signed-off-by: Wayne Lin <Wayne.Lin@amd.com> --- drivers/gpu/drm/drm_dp_mst_topology.c | 131 +++++++++++++------------- 1 file changed, 68 insertions(+), 63 deletions(-)