[PATCH v2 1/1] drm/dp_mst: Handle SST-only branch device case
Lyude Paul
lyude at redhat.com
Sat Jan 18 00:18:11 UTC 2020
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 at linux.intel.com>
> Cc: Harry Wentland <hwentlan at amd.com>
> Cc: Lyude Paul <lyude at redhat.com>
> Signed-off-by: Wayne Lin <Wayne.Lin at amd.com>
> Reviewed-by: Lyude Paul <lyude at 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);
> }
>
--
Cheers,
Lyude Paul
More information about the dri-devel
mailing list