| Message ID | 20200117060350.26358-2-Wayne.Lin@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Take SST-only branch device into account | expand |
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@linux.intel.com> > Cc: Harry Wentland <hwentlan@amd.com> > Cc: Lyude Paul <lyude@redhat.com> > Signed-off-by: Wayne Lin <Wayne.Lin@amd.com> > Reviewed-by: Lyude Paul <lyude@redhat.com> > --- > 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); > } >
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); }
[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@linux.intel.com> Cc: Harry Wentland <hwentlan@amd.com> Cc: Lyude Paul <lyude@redhat.com> Signed-off-by: Wayne Lin <Wayne.Lin@amd.com> Reviewed-by: Lyude Paul <lyude@redhat.com> --- drivers/gpu/drm/drm_dp_mst_topology.c | 140 +++++++++++++++----------- 1 file changed, 80 insertions(+), 60 deletions(-)