| Message ID | 1453500449-9224-4-git-send-email-harry.wentland@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 23 January 2016 at 08:07, Harry Wentland <harry.wentland@amd.com> wrote: > From: Mykola Lysenko <Mykola.Lysenko@amd.com> > > 1. Get edid for all connected MST displays, not only on logical ports, > in the same thread as MST topology detection is done: > There are displays that have branches inside w/o logical ports. > So in case another SST display connected downstream system can > end-up in situation when 3 DOWN requests sent: two for > ‘remote i2c read’ and one for ‘enum path resources’, making slots full. > > 2. Call notification callback in one place in the end of topology discovery/update: > This is done to reduce number of events sent to userspace in case complex > topology discovery is going, adding multiple number of connectors; > > 3. Remove notification callback call from short pulse interrupt processing function: > This is done in order not to block interrupt processing function, in case any > MST request will be made from it. Notification will be send from topology > discovery/update work item. I've had to pull this out, as I did some more indepth testing with i915 and a Dell 30" and this broke things. The main thing it broke is setting the tiling property that userspace needs to see those dual-panel monitors as one. you should be able to see xrandr --props if the tile property is correct if you test. I'm also not sure about some other bits in this patch, so I probably need to dig a bit deeper into it. Dave. > > Signed-off-by: Mykola Lysenko <Mykola.Lysenko@amd.com> > Reviewed-by: Harry Wentland <Harry.Wentland@amd.com> > Acked-by: Alex Deucher <alexander.deucher@amd.com> > --- > drivers/gpu/drm/drm_dp_mst_topology.c | 37 ++++++++++++++++++----------------- > 1 file changed, 19 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c > index 041597b7a7c6..052c20ca35ee 100644 > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > @@ -1130,13 +1130,11 @@ static void drm_dp_add_port(struct drm_dp_mst_branch *mstb, > drm_dp_put_port(port); > goto out; > } > - if (port->port_num >= DP_MST_LOGICAL_PORT_0) { > - port->cached_edid = drm_get_edid(port->connector, &port->aux.ddc); > - drm_mode_connector_set_tile_property(port->connector); > - } > + > + drm_mode_connector_set_tile_property(port->connector); > + > (*mstb->mgr->cbs->register_connector)(port->connector); > } > - > out: > /* put reference to this port */ > drm_dp_put_port(port); > @@ -1161,9 +1159,9 @@ static void drm_dp_update_port(struct drm_dp_mst_branch *mstb, > port->ddps = conn_stat->displayport_device_plug_status; > > if (old_ddps != port->ddps) { > + dowork = true; > if (port->ddps) { > drm_dp_check_port_guid(mstb, port); > - dowork = true; > } else { > port->guid_valid = false; > port->available_pbn = 0; > @@ -1271,8 +1269,13 @@ static void drm_dp_check_and_send_link_address(struct drm_dp_mst_topology_mgr *m > if (port->input) > continue; > > - if (!port->ddps) > + if (!port->ddps) { > + if (port->cached_edid) { > + kfree(port->cached_edid); > + port->cached_edid = NULL; > + } > continue; > + } > > if (!port->available_pbn) > drm_dp_send_enum_path_resources(mgr, mstb, port); > @@ -1283,6 +1286,12 @@ static void drm_dp_check_and_send_link_address(struct drm_dp_mst_topology_mgr *m > drm_dp_check_and_send_link_address(mgr, mstb_child); > drm_dp_put_mst_branch_device(mstb_child); > } > + } else if (port->pdt == DP_PEER_DEVICE_SST_SINK || > + port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV) { > + if (!port->cached_edid) { > + port->cached_edid = > + drm_get_edid(port->connector, &port->aux.ddc); > + } > } > } > } > @@ -1302,6 +1311,8 @@ static void drm_dp_mst_link_probe_work(struct work_struct *work) > drm_dp_check_and_send_link_address(mgr, mstb); > drm_dp_put_mst_branch_device(mstb); > } > + > + (*mgr->cbs->hotplug)(mgr); > } > > static bool drm_dp_validate_guid(struct drm_dp_mst_topology_mgr *mgr, > @@ -1558,7 +1569,6 @@ static void drm_dp_send_link_address(struct drm_dp_mst_topology_mgr *mgr, > for (i = 0; i < txmsg->reply.u.link_addr.nports; i++) { > drm_dp_add_port(mstb, mgr->dev, &txmsg->reply.u.link_addr.ports[i]); > } > - (*mgr->cbs->hotplug)(mgr); > } > } else { > mstb->link_address_sent = false; > @@ -2232,8 +2242,6 @@ static int drm_dp_mst_handle_up_req(struct drm_dp_mst_topology_mgr *mgr) > > drm_dp_update_port(mstb, &msg.u.conn_stat); > DRM_DEBUG_KMS("Got CSN: pn: %d ldps:%d ddps: %d mcs: %d ip: %d pdt: %d\n", msg.u.conn_stat.port_number, msg.u.conn_stat.legacy_device_plug_status, msg.u.conn_stat.displayport_device_plug_status, msg.u.conn_stat.message_capability_status, msg.u.conn_stat.input_port, msg.u.conn_stat.peer_device_type); > - (*mgr->cbs->hotplug)(mgr); > - > } else if (msg.req_type == DP_RESOURCE_STATUS_NOTIFY) { > drm_dp_send_up_ack_reply(mgr, mgr->mst_primary, msg.req_type, seqno, false); > if (!mstb) > @@ -2320,10 +2328,6 @@ enum drm_connector_status drm_dp_mst_detect_port(struct drm_connector *connector > > case DP_PEER_DEVICE_SST_SINK: > status = connector_status_connected; > - /* for logical ports - cache the EDID */ > - if (port->port_num >= 8 && !port->cached_edid) { > - port->cached_edid = drm_get_edid(connector, &port->aux.ddc); > - } > break; > case DP_PEER_DEVICE_DP_LEGACY_CONV: > if (port->ldps) > @@ -2378,10 +2382,7 @@ struct edid *drm_dp_mst_get_edid(struct drm_connector *connector, struct drm_dp_ > > if (port->cached_edid) > edid = drm_edid_duplicate(port->cached_edid); > - else { > - edid = drm_get_edid(connector, &port->aux.ddc); > - drm_mode_connector_set_tile_property(connector); > - } > + > port->has_audio = drm_detect_monitor_audio(edid); > drm_dp_put_port(port); > return edid; > -- > 2.1.4 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Dave, it seems to be missed call to drm_mode_connector_set_tile_property(port->connector); here: > + } else if (port->pdt == DP_PEER_DEVICE_SST_SINK || > + port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV) { > + if (!port->cached_edid) { > + port->cached_edid = > + drm_get_edid(port->connector, &port->aux.ddc); drm_mode_connector_set_tile_property(port->connector); > + } I will test with tile display to see if it solves the issue. Thanks, Mykola -----Original Message----- From: Dave Airlie [mailto:airlied@gmail.com] Sent: Wednesday, February 17, 2016 1:46 PM To: Wentland, Harry <Harry.Wentland@amd.com> Cc: dri-devel <dri-devel@lists.freedesktop.org>; Lysenko, Mykola <Mykola.Lysenko@amd.com> Subject: Re: [PATCH 3/5] drm/dp/mst: change MST detection scheme On 23 January 2016 at 08:07, Harry Wentland <harry.wentland@amd.com> wrote: > From: Mykola Lysenko <Mykola.Lysenko@amd.com> > > 1. Get edid for all connected MST displays, not only on logical ports, > in the same thread as MST topology detection is done: > There are displays that have branches inside w/o logical ports. > So in case another SST display connected downstream system can > end-up in situation when 3 DOWN requests sent: two for > ‘remote i2c read’ and one for ‘enum path resources’, making slots full. > > 2. Call notification callback in one place in the end of topology discovery/update: > This is done to reduce number of events sent to userspace in case complex > topology discovery is going, adding multiple number of connectors; > > 3. Remove notification callback call from short pulse interrupt processing function: > This is done in order not to block interrupt processing function, in case any > MST request will be made from it. Notification will be send from topology > discovery/update work item. I've had to pull this out, as I did some more indepth testing with i915 and a Dell 30" and this broke things. The main thing it broke is setting the tiling property that userspace needs to see those dual-panel monitors as one. you should be able to see xrandr --props if the tile property is correct if you test. I'm also not sure about some other bits in this patch, so I probably need to dig a bit deeper into it. Dave. > > Signed-off-by: Mykola Lysenko <Mykola.Lysenko@amd.com> > Reviewed-by: Harry Wentland <Harry.Wentland@amd.com> > Acked-by: Alex Deucher <alexander.deucher@amd.com> > --- > drivers/gpu/drm/drm_dp_mst_topology.c | 37 ++++++++++++++++++----------------- > 1 file changed, 19 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c > index 041597b7a7c6..052c20ca35ee 100644 > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > @@ -1130,13 +1130,11 @@ static void drm_dp_add_port(struct drm_dp_mst_branch *mstb, > drm_dp_put_port(port); > goto out; > } > - if (port->port_num >= DP_MST_LOGICAL_PORT_0) { > - port->cached_edid = drm_get_edid(port->connector, &port->aux.ddc); > - drm_mode_connector_set_tile_property(port->connector); > - } > + > + drm_mode_connector_set_tile_property(port->connector); > + > (*mstb->mgr->cbs->register_connector)(port->connector); > } > - > out: > /* put reference to this port */ > drm_dp_put_port(port); > @@ -1161,9 +1159,9 @@ static void drm_dp_update_port(struct drm_dp_mst_branch *mstb, > port->ddps = conn_stat->displayport_device_plug_status; > > if (old_ddps != port->ddps) { > + dowork = true; > if (port->ddps) { > drm_dp_check_port_guid(mstb, port); > - dowork = true; > } else { > port->guid_valid = false; > port->available_pbn = 0; > @@ -1271,8 +1269,13 @@ static void drm_dp_check_and_send_link_address(struct drm_dp_mst_topology_mgr *m > if (port->input) > continue; > > - if (!port->ddps) > + if (!port->ddps) { > + if (port->cached_edid) { > + kfree(port->cached_edid); > + port->cached_edid = NULL; > + } > continue; > + } > > if (!port->available_pbn) > drm_dp_send_enum_path_resources(mgr, mstb, port); > @@ -1283,6 +1286,12 @@ static void drm_dp_check_and_send_link_address(struct drm_dp_mst_topology_mgr *m > drm_dp_check_and_send_link_address(mgr, mstb_child); > drm_dp_put_mst_branch_device(mstb_child); > } > + } else if (port->pdt == DP_PEER_DEVICE_SST_SINK || > + port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV) { > + if (!port->cached_edid) { > + port->cached_edid = > + drm_get_edid(port->connector, &port->aux.ddc); > + } > } > } > } > @@ -1302,6 +1311,8 @@ static void drm_dp_mst_link_probe_work(struct work_struct *work) > drm_dp_check_and_send_link_address(mgr, mstb); > drm_dp_put_mst_branch_device(mstb); > } > + > + (*mgr->cbs->hotplug)(mgr); > } > > static bool drm_dp_validate_guid(struct drm_dp_mst_topology_mgr *mgr, > @@ -1558,7 +1569,6 @@ static void drm_dp_send_link_address(struct drm_dp_mst_topology_mgr *mgr, > for (i = 0; i < txmsg->reply.u.link_addr.nports; i++) { > drm_dp_add_port(mstb, mgr->dev, &txmsg->reply.u.link_addr.ports[i]); > } > - (*mgr->cbs->hotplug)(mgr); > } > } else { > mstb->link_address_sent = false; > @@ -2232,8 +2242,6 @@ static int drm_dp_mst_handle_up_req(struct drm_dp_mst_topology_mgr *mgr) > > drm_dp_update_port(mstb, &msg.u.conn_stat); > DRM_DEBUG_KMS("Got CSN: pn: %d ldps:%d ddps: %d mcs: %d ip: %d pdt: %d\n", msg.u.conn_stat.port_number, msg.u.conn_stat.legacy_device_plug_status, msg.u.conn_stat.displayport_device_plug_status, msg.u.conn_stat.message_capability_status, msg.u.conn_stat.input_port, msg.u.conn_stat.peer_device_type); > - (*mgr->cbs->hotplug)(mgr); > - > } else if (msg.req_type == DP_RESOURCE_STATUS_NOTIFY) { > drm_dp_send_up_ack_reply(mgr, mgr->mst_primary, msg.req_type, seqno, false); > if (!mstb) > @@ -2320,10 +2328,6 @@ enum drm_connector_status drm_dp_mst_detect_port(struct drm_connector *connector > > case DP_PEER_DEVICE_SST_SINK: > status = connector_status_connected; > - /* for logical ports - cache the EDID */ > - if (port->port_num >= 8 && !port->cached_edid) { > - port->cached_edid = drm_get_edid(connector, &port->aux.ddc); > - } > break; > case DP_PEER_DEVICE_DP_LEGACY_CONV: > if (port->ldps) > @@ -2378,10 +2382,7 @@ struct edid *drm_dp_mst_get_edid(struct drm_connector *connector, struct drm_dp_ > > if (port->cached_edid) > edid = drm_edid_duplicate(port->cached_edid); > - else { > - edid = drm_get_edid(connector, &port->aux.ddc); > - drm_mode_connector_set_tile_property(connector); > - } > + > port->has_audio = drm_detect_monitor_audio(edid); > drm_dp_put_port(port); > return edid; > -- > 2.1.4 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Dave, Attached patch should fix the issue. I have one question though: should we add drm_mode_connector_set_tile_property to the drm_get_displayid function, so it will be updated in case of regular DP displays as well? As an example of such usage could be 5k displays with two tiles 2560x2880 that could not be driven on one MST connector because of bandwidth limitations, and are driven in SST mode. Thanks, Mykola -----Original Message----- From: Lysenko, Mykola Sent: Thursday, February 18, 2016 12:53 PM To: 'Dave Airlie' <airlied@gmail.com>; Wentland, Harry <Harry.Wentland@amd.com> Cc: dri-devel <dri-devel@lists.freedesktop.org> Subject: RE: [PATCH 3/5] drm/dp/mst: change MST detection scheme Hi Dave, it seems to be missed call to drm_mode_connector_set_tile_property(port->connector); here: > + } else if (port->pdt == DP_PEER_DEVICE_SST_SINK || > + port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV) { > + if (!port->cached_edid) { > + port->cached_edid = > + drm_get_edid(port->connector, > + &port->aux.ddc); drm_mode_connector_set_tile_property(port->connector); > + } I will test with tile display to see if it solves the issue. Thanks, Mykola -----Original Message----- From: Dave Airlie [mailto:airlied@gmail.com] Sent: Wednesday, February 17, 2016 1:46 PM To: Wentland, Harry <Harry.Wentland@amd.com> Cc: dri-devel <dri-devel@lists.freedesktop.org>; Lysenko, Mykola <Mykola.Lysenko@amd.com> Subject: Re: [PATCH 3/5] drm/dp/mst: change MST detection scheme On 23 January 2016 at 08:07, Harry Wentland <harry.wentland@amd.com> wrote: > From: Mykola Lysenko <Mykola.Lysenko@amd.com> > > 1. Get edid for all connected MST displays, not only on logical ports, > in the same thread as MST topology detection is done: > There are displays that have branches inside w/o logical ports. > So in case another SST display connected downstream system can > end-up in situation when 3 DOWN requests sent: two for > ‘remote i2c read’ and one for ‘enum path resources’, making slots full. > > 2. Call notification callback in one place in the end of topology discovery/update: > This is done to reduce number of events sent to userspace in case complex > topology discovery is going, adding multiple number of > connectors; > > 3. Remove notification callback call from short pulse interrupt processing function: > This is done in order not to block interrupt processing function, in case any > MST request will be made from it. Notification will be send from topology > discovery/update work item. I've had to pull this out, as I did some more indepth testing with i915 and a Dell 30" and this broke things. The main thing it broke is setting the tiling property that userspace needs to see those dual-panel monitors as one. you should be able to see xrandr --props if the tile property is correct if you test. I'm also not sure about some other bits in this patch, so I probably need to dig a bit deeper into it. Dave. > > Signed-off-by: Mykola Lysenko <Mykola.Lysenko@amd.com> > Reviewed-by: Harry Wentland <Harry.Wentland@amd.com> > Acked-by: Alex Deucher <alexander.deucher@amd.com> > --- > drivers/gpu/drm/drm_dp_mst_topology.c | 37 > ++++++++++++++++++----------------- > 1 file changed, 19 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c > b/drivers/gpu/drm/drm_dp_mst_topology.c > index 041597b7a7c6..052c20ca35ee 100644 > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > @@ -1130,13 +1130,11 @@ static void drm_dp_add_port(struct drm_dp_mst_branch *mstb, > drm_dp_put_port(port); > goto out; > } > - if (port->port_num >= DP_MST_LOGICAL_PORT_0) { > - port->cached_edid = drm_get_edid(port->connector, &port->aux.ddc); > - drm_mode_connector_set_tile_property(port->connector); > - } > + > + drm_mode_connector_set_tile_property(port->connector); > + > (*mstb->mgr->cbs->register_connector)(port->connector); > } > - > out: > /* put reference to this port */ > drm_dp_put_port(port); > @@ -1161,9 +1159,9 @@ static void drm_dp_update_port(struct drm_dp_mst_branch *mstb, > port->ddps = conn_stat->displayport_device_plug_status; > > if (old_ddps != port->ddps) { > + dowork = true; > if (port->ddps) { > drm_dp_check_port_guid(mstb, port); > - dowork = true; > } else { > port->guid_valid = false; > port->available_pbn = 0; @@ -1271,8 +1269,13 > @@ static void drm_dp_check_and_send_link_address(struct drm_dp_mst_topology_mgr *m > if (port->input) > continue; > > - if (!port->ddps) > + if (!port->ddps) { > + if (port->cached_edid) { > + kfree(port->cached_edid); > + port->cached_edid = NULL; > + } > continue; > + } > > if (!port->available_pbn) > drm_dp_send_enum_path_resources(mgr, mstb, > port); @@ -1283,6 +1286,12 @@ static void drm_dp_check_and_send_link_address(struct drm_dp_mst_topology_mgr *m > drm_dp_check_and_send_link_address(mgr, mstb_child); > drm_dp_put_mst_branch_device(mstb_child); > } > + } else if (port->pdt == DP_PEER_DEVICE_SST_SINK || > + port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV) { > + if (!port->cached_edid) { > + port->cached_edid = > + drm_get_edid(port->connector, &port->aux.ddc); > + } > } > } > } > @@ -1302,6 +1311,8 @@ static void drm_dp_mst_link_probe_work(struct work_struct *work) > drm_dp_check_and_send_link_address(mgr, mstb); > drm_dp_put_mst_branch_device(mstb); > } > + > + (*mgr->cbs->hotplug)(mgr); > } > > static bool drm_dp_validate_guid(struct drm_dp_mst_topology_mgr *mgr, > @@ -1558,7 +1569,6 @@ static void drm_dp_send_link_address(struct drm_dp_mst_topology_mgr *mgr, > for (i = 0; i < txmsg->reply.u.link_addr.nports; i++) { > drm_dp_add_port(mstb, mgr->dev, &txmsg->reply.u.link_addr.ports[i]); > } > - (*mgr->cbs->hotplug)(mgr); > } > } else { > mstb->link_address_sent = false; @@ -2232,8 +2242,6 @@ > static int drm_dp_mst_handle_up_req(struct drm_dp_mst_topology_mgr > *mgr) > > drm_dp_update_port(mstb, &msg.u.conn_stat); > DRM_DEBUG_KMS("Got CSN: pn: %d ldps:%d ddps: %d mcs: %d ip: %d pdt: %d\n", msg.u.conn_stat.port_number, msg.u.conn_stat.legacy_device_plug_status, msg.u.conn_stat.displayport_device_plug_status, msg.u.conn_stat.message_capability_status, msg.u.conn_stat.input_port, msg.u.conn_stat.peer_device_type); > - (*mgr->cbs->hotplug)(mgr); > - > } else if (msg.req_type == DP_RESOURCE_STATUS_NOTIFY) { > drm_dp_send_up_ack_reply(mgr, mgr->mst_primary, msg.req_type, seqno, false); > if (!mstb) > @@ -2320,10 +2328,6 @@ enum drm_connector_status > drm_dp_mst_detect_port(struct drm_connector *connector > > case DP_PEER_DEVICE_SST_SINK: > status = connector_status_connected; > - /* for logical ports - cache the EDID */ > - if (port->port_num >= 8 && !port->cached_edid) { > - port->cached_edid = drm_get_edid(connector, &port->aux.ddc); > - } > break; > case DP_PEER_DEVICE_DP_LEGACY_CONV: > if (port->ldps) > @@ -2378,10 +2382,7 @@ struct edid *drm_dp_mst_get_edid(struct > drm_connector *connector, struct drm_dp_ > > if (port->cached_edid) > edid = drm_edid_duplicate(port->cached_edid); > - else { > - edid = drm_get_edid(connector, &port->aux.ddc); > - drm_mode_connector_set_tile_property(connector); > - } > + > port->has_audio = drm_detect_monitor_audio(edid); > drm_dp_put_port(port); > return edid; > -- > 2.1.4 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Dave, Have you had a chance to see if Mykola's latest patch addresses the issue you observed with tiled MST display ? Thanks. -----Original Message----- From: Lysenko, Mykola Sent: Monday, February 22, 2016 10:09 PM To: Dave Airlie; Wentland, Harry Cc: dri-devel Subject: RE: [PATCH 3/5] drm/dp/mst: change MST detection scheme Hi Dave, Attached patch should fix the issue. I have one question though: should we add drm_mode_connector_set_tile_property to the drm_get_displayid function, so it will be updated in case of regular DP displays as well? As an example of such usage could be 5k displays with two tiles 2560x2880 that could not be driven on one MST connector because of bandwidth limitations, and are driven in SST mode. Thanks, Mykola -----Original Message----- From: Lysenko, Mykola Sent: Thursday, February 18, 2016 12:53 PM To: 'Dave Airlie' <airlied@gmail.com>; Wentland, Harry <Harry.Wentland@amd.com> Cc: dri-devel <dri-devel@lists.freedesktop.org> Subject: RE: [PATCH 3/5] drm/dp/mst: change MST detection scheme Hi Dave, it seems to be missed call to drm_mode_connector_set_tile_property(port->connector); here: > + } else if (port->pdt == DP_PEER_DEVICE_SST_SINK || > + port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV) { > + if (!port->cached_edid) { > + port->cached_edid = > + drm_get_edid(port->connector, > + &port->aux.ddc); drm_mode_connector_set_tile_property(port->connector); > + } I will test with tile display to see if it solves the issue. Thanks, Mykola -----Original Message----- From: Dave Airlie [mailto:airlied@gmail.com] Sent: Wednesday, February 17, 2016 1:46 PM To: Wentland, Harry <Harry.Wentland@amd.com> Cc: dri-devel <dri-devel@lists.freedesktop.org>; Lysenko, Mykola <Mykola.Lysenko@amd.com> Subject: Re: [PATCH 3/5] drm/dp/mst: change MST detection scheme On 23 January 2016 at 08:07, Harry Wentland <harry.wentland@amd.com> wrote: > From: Mykola Lysenko <Mykola.Lysenko@amd.com> > > 1. Get edid for all connected MST displays, not only on logical ports, > in the same thread as MST topology detection is done: > There are displays that have branches inside w/o logical ports. > So in case another SST display connected downstream system can > end-up in situation when 3 DOWN requests sent: two for > ‘remote i2c read’ and one for ‘enum path resources’, making slots full. > > 2. Call notification callback in one place in the end of topology discovery/update: > This is done to reduce number of events sent to userspace in case complex > topology discovery is going, adding multiple number of > connectors; > > 3. Remove notification callback call from short pulse interrupt processing function: > This is done in order not to block interrupt processing function, in case any > MST request will be made from it. Notification will be send from topology > discovery/update work item. I've had to pull this out, as I did some more indepth testing with i915 and a Dell 30" and this broke things. The main thing it broke is setting the tiling property that userspace needs to see those dual-panel monitors as one. you should be able to see xrandr --props if the tile property is correct if you test. I'm also not sure about some other bits in this patch, so I probably need to dig a bit deeper into it. Dave. > > Signed-off-by: Mykola Lysenko <Mykola.Lysenko@amd.com> > Reviewed-by: Harry Wentland <Harry.Wentland@amd.com> > Acked-by: Alex Deucher <alexander.deucher@amd.com> > --- > drivers/gpu/drm/drm_dp_mst_topology.c | 37 > ++++++++++++++++++----------------- > 1 file changed, 19 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c > b/drivers/gpu/drm/drm_dp_mst_topology.c > index 041597b7a7c6..052c20ca35ee 100644 > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > @@ -1130,13 +1130,11 @@ static void drm_dp_add_port(struct drm_dp_mst_branch *mstb, > drm_dp_put_port(port); > goto out; > } > - if (port->port_num >= DP_MST_LOGICAL_PORT_0) { > - port->cached_edid = drm_get_edid(port->connector, &port->aux.ddc); > - drm_mode_connector_set_tile_property(port->connector); > - } > + > + drm_mode_connector_set_tile_property(port->connector); > + > (*mstb->mgr->cbs->register_connector)(port->connector); > } > - > out: > /* put reference to this port */ > drm_dp_put_port(port); > @@ -1161,9 +1159,9 @@ static void drm_dp_update_port(struct drm_dp_mst_branch *mstb, > port->ddps = conn_stat->displayport_device_plug_status; > > if (old_ddps != port->ddps) { > + dowork = true; > if (port->ddps) { > drm_dp_check_port_guid(mstb, port); > - dowork = true; > } else { > port->guid_valid = false; > port->available_pbn = 0; @@ -1271,8 +1269,13 > @@ static void drm_dp_check_and_send_link_address(struct drm_dp_mst_topology_mgr *m > if (port->input) > continue; > > - if (!port->ddps) > + if (!port->ddps) { > + if (port->cached_edid) { > + kfree(port->cached_edid); > + port->cached_edid = NULL; > + } > continue; > + } > > if (!port->available_pbn) > drm_dp_send_enum_path_resources(mgr, mstb, > port); @@ -1283,6 +1286,12 @@ static void drm_dp_check_and_send_link_address(struct drm_dp_mst_topology_mgr *m > drm_dp_check_and_send_link_address(mgr, mstb_child); > drm_dp_put_mst_branch_device(mstb_child); > } > + } else if (port->pdt == DP_PEER_DEVICE_SST_SINK || > + port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV) { > + if (!port->cached_edid) { > + port->cached_edid = > + drm_get_edid(port->connector, &port->aux.ddc); > + } > } > } > } > @@ -1302,6 +1311,8 @@ static void drm_dp_mst_link_probe_work(struct work_struct *work) > drm_dp_check_and_send_link_address(mgr, mstb); > drm_dp_put_mst_branch_device(mstb); > } > + > + (*mgr->cbs->hotplug)(mgr); > } > > static bool drm_dp_validate_guid(struct drm_dp_mst_topology_mgr *mgr, > @@ -1558,7 +1569,6 @@ static void drm_dp_send_link_address(struct drm_dp_mst_topology_mgr *mgr, > for (i = 0; i < txmsg->reply.u.link_addr.nports; i++) { > drm_dp_add_port(mstb, mgr->dev, &txmsg->reply.u.link_addr.ports[i]); > } > - (*mgr->cbs->hotplug)(mgr); > } > } else { > mstb->link_address_sent = false; @@ -2232,8 +2242,6 @@ > static int drm_dp_mst_handle_up_req(struct drm_dp_mst_topology_mgr > *mgr) > > drm_dp_update_port(mstb, &msg.u.conn_stat); > DRM_DEBUG_KMS("Got CSN: pn: %d ldps:%d ddps: %d mcs: %d ip: %d pdt: %d\n", msg.u.conn_stat.port_number, msg.u.conn_stat.legacy_device_plug_status, msg.u.conn_stat.displayport_device_plug_status, msg.u.conn_stat.message_capability_status, msg.u.conn_stat.input_port, msg.u.conn_stat.peer_device_type); > - (*mgr->cbs->hotplug)(mgr); > - > } else if (msg.req_type == DP_RESOURCE_STATUS_NOTIFY) { > drm_dp_send_up_ack_reply(mgr, mgr->mst_primary, msg.req_type, seqno, false); > if (!mstb) > @@ -2320,10 +2328,6 @@ enum drm_connector_status > drm_dp_mst_detect_port(struct drm_connector *connector > > case DP_PEER_DEVICE_SST_SINK: > status = connector_status_connected; > - /* for logical ports - cache the EDID */ > - if (port->port_num >= 8 && !port->cached_edid) { > - port->cached_edid = drm_get_edid(connector, &port->aux.ddc); > - } > break; > case DP_PEER_DEVICE_DP_LEGACY_CONV: > if (port->ldps) > @@ -2378,10 +2382,7 @@ struct edid *drm_dp_mst_get_edid(struct > drm_connector *connector, struct drm_dp_ > > if (port->cached_edid) > edid = drm_edid_duplicate(port->cached_edid); > - else { > - edid = drm_get_edid(connector, &port->aux.ddc); > - drm_mode_connector_set_tile_property(connector); > - } > + > port->has_audio = drm_detect_monitor_audio(edid); > drm_dp_put_port(port); > return edid; > -- > 2.1.4 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
Excuse me for top posting. So I finally got back to this patch, still don't like it. Apart from the doing 3 things in once which is just annoying, current userspace for tiled monitors relies on the behaviour that the cached edids are retrieved before the connector is show to userspace. This is so that for tiled monitors both connectors are shown with tiling properties so userspace can pick the correct monitor size. Now I realise this isn't great, since at least for the two cable 5K monitors you won't ever plug them in at once. So we should probably discuss more what userspace should do in the presence of a tiled monitor where it can only see one tile, If we go ahead with something like this, things will look kinda ugly as you plug in a 4k monitor and it'll try and set a mode on the first tile when it gets the hotplug (or it could be racing and notice the connectors before the hotplug). So it then sets the 1920x1080 mode on one connector and gets another hotplug to set the tiled mode up. Dave. On 13 May 2016 at 08:41, Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com> wrote: > Hi Dave, > > Have you had a chance to see if Mykola's latest patch addresses the issue you observed with tiled MST display ? > > Thanks. > -----Original Message----- > From: Lysenko, Mykola > Sent: Monday, February 22, 2016 10:09 PM > To: Dave Airlie; Wentland, Harry > Cc: dri-devel > Subject: RE: [PATCH 3/5] drm/dp/mst: change MST detection scheme > > Hi Dave, > > Attached patch should fix the issue. > > I have one question though: should we add drm_mode_connector_set_tile_property to the drm_get_displayid function, so it will be updated in case of regular DP displays as well? > > As an example of such usage could be 5k displays with two tiles 2560x2880 that could not be driven on one MST connector because of bandwidth limitations, and are driven in SST mode. > > Thanks, > Mykola > > -----Original Message----- > From: Lysenko, Mykola > Sent: Thursday, February 18, 2016 12:53 PM > To: 'Dave Airlie' <airlied@gmail.com>; Wentland, Harry <Harry.Wentland@amd.com> > Cc: dri-devel <dri-devel@lists.freedesktop.org> > Subject: RE: [PATCH 3/5] drm/dp/mst: change MST detection scheme > > Hi Dave, > > it seems to be missed call to > > drm_mode_connector_set_tile_property(port->connector); > > here: > >> + } else if (port->pdt == DP_PEER_DEVICE_SST_SINK || >> + port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV) { >> + if (!port->cached_edid) { >> + port->cached_edid = >> + drm_get_edid(port->connector, >> + &port->aux.ddc); > drm_mode_connector_set_tile_property(port->connector); >> + } > > I will test with tile display to see if it solves the issue. > > Thanks, > Mykola > > > -----Original Message----- > From: Dave Airlie [mailto:airlied@gmail.com] > Sent: Wednesday, February 17, 2016 1:46 PM > To: Wentland, Harry <Harry.Wentland@amd.com> > Cc: dri-devel <dri-devel@lists.freedesktop.org>; Lysenko, Mykola <Mykola.Lysenko@amd.com> > Subject: Re: [PATCH 3/5] drm/dp/mst: change MST detection scheme > > On 23 January 2016 at 08:07, Harry Wentland <harry.wentland@amd.com> wrote: >> From: Mykola Lysenko <Mykola.Lysenko@amd.com> >> >> 1. Get edid for all connected MST displays, not only on logical ports, >> in the same thread as MST topology detection is done: >> There are displays that have branches inside w/o logical ports. >> So in case another SST display connected downstream system can >> end-up in situation when 3 DOWN requests sent: two for >> ‘remote i2c read’ and one for ‘enum path resources’, making slots full. >> >> 2. Call notification callback in one place in the end of topology discovery/update: >> This is done to reduce number of events sent to userspace in case complex >> topology discovery is going, adding multiple number of >> connectors; >> >> 3. Remove notification callback call from short pulse interrupt processing function: >> This is done in order not to block interrupt processing function, in case any >> MST request will be made from it. Notification will be send from topology >> discovery/update work item. > > I've had to pull this out, as I did some more indepth testing with > i915 and a Dell 30" > and this broke things. > > The main thing it broke is setting the tiling property that userspace needs to see those dual-panel monitors as one. > > you should be able to see xrandr --props if the tile property is correct if you test. > > I'm also not sure about some other bits in this patch, so I probably need to dig a bit deeper into it. > > Dave. > >> >> Signed-off-by: Mykola Lysenko <Mykola.Lysenko@amd.com> >> Reviewed-by: Harry Wentland <Harry.Wentland@amd.com> >> Acked-by: Alex Deucher <alexander.deucher@amd.com> >> --- >> drivers/gpu/drm/drm_dp_mst_topology.c | 37 >> ++++++++++++++++++----------------- >> 1 file changed, 19 insertions(+), 18 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c >> b/drivers/gpu/drm/drm_dp_mst_topology.c >> index 041597b7a7c6..052c20ca35ee 100644 >> --- a/drivers/gpu/drm/drm_dp_mst_topology.c >> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c >> @@ -1130,13 +1130,11 @@ static void drm_dp_add_port(struct drm_dp_mst_branch *mstb, >> drm_dp_put_port(port); >> goto out; >> } >> - if (port->port_num >= DP_MST_LOGICAL_PORT_0) { >> - port->cached_edid = drm_get_edid(port->connector, &port->aux.ddc); >> - drm_mode_connector_set_tile_property(port->connector); >> - } >> + >> + drm_mode_connector_set_tile_property(port->connector); >> + >> (*mstb->mgr->cbs->register_connector)(port->connector); >> } >> - >> out: >> /* put reference to this port */ >> drm_dp_put_port(port); >> @@ -1161,9 +1159,9 @@ static void drm_dp_update_port(struct drm_dp_mst_branch *mstb, >> port->ddps = conn_stat->displayport_device_plug_status; >> >> if (old_ddps != port->ddps) { >> + dowork = true; >> if (port->ddps) { >> drm_dp_check_port_guid(mstb, port); >> - dowork = true; >> } else { >> port->guid_valid = false; >> port->available_pbn = 0; @@ -1271,8 +1269,13 >> @@ static void drm_dp_check_and_send_link_address(struct drm_dp_mst_topology_mgr *m >> if (port->input) >> continue; >> >> - if (!port->ddps) >> + if (!port->ddps) { >> + if (port->cached_edid) { >> + kfree(port->cached_edid); >> + port->cached_edid = NULL; >> + } >> continue; >> + } >> >> if (!port->available_pbn) >> drm_dp_send_enum_path_resources(mgr, mstb, >> port); @@ -1283,6 +1286,12 @@ static void drm_dp_check_and_send_link_address(struct drm_dp_mst_topology_mgr *m >> drm_dp_check_and_send_link_address(mgr, mstb_child); >> drm_dp_put_mst_branch_device(mstb_child); >> } >> + } else if (port->pdt == DP_PEER_DEVICE_SST_SINK || >> + port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV) { >> + if (!port->cached_edid) { >> + port->cached_edid = >> + drm_get_edid(port->connector, &port->aux.ddc); >> + } >> } >> } >> } >> @@ -1302,6 +1311,8 @@ static void drm_dp_mst_link_probe_work(struct work_struct *work) >> drm_dp_check_and_send_link_address(mgr, mstb); >> drm_dp_put_mst_branch_device(mstb); >> } >> + >> + (*mgr->cbs->hotplug)(mgr); >> } >> >> static bool drm_dp_validate_guid(struct drm_dp_mst_topology_mgr *mgr, >> @@ -1558,7 +1569,6 @@ static void drm_dp_send_link_address(struct drm_dp_mst_topology_mgr *mgr, >> for (i = 0; i < txmsg->reply.u.link_addr.nports; i++) { >> drm_dp_add_port(mstb, mgr->dev, &txmsg->reply.u.link_addr.ports[i]); >> } >> - (*mgr->cbs->hotplug)(mgr); >> } >> } else { >> mstb->link_address_sent = false; @@ -2232,8 +2242,6 @@ >> static int drm_dp_mst_handle_up_req(struct drm_dp_mst_topology_mgr >> *mgr) >> >> drm_dp_update_port(mstb, &msg.u.conn_stat); >> DRM_DEBUG_KMS("Got CSN: pn: %d ldps:%d ddps: %d mcs: %d ip: %d pdt: %d\n", msg.u.conn_stat.port_number, msg.u.conn_stat.legacy_device_plug_status, msg.u.conn_stat.displayport_device_plug_status, msg.u.conn_stat.message_capability_status, msg.u.conn_stat.input_port, msg.u.conn_stat.peer_device_type); >> - (*mgr->cbs->hotplug)(mgr); >> - >> } else if (msg.req_type == DP_RESOURCE_STATUS_NOTIFY) { >> drm_dp_send_up_ack_reply(mgr, mgr->mst_primary, msg.req_type, seqno, false); >> if (!mstb) >> @@ -2320,10 +2328,6 @@ enum drm_connector_status >> drm_dp_mst_detect_port(struct drm_connector *connector >> >> case DP_PEER_DEVICE_SST_SINK: >> status = connector_status_connected; >> - /* for logical ports - cache the EDID */ >> - if (port->port_num >= 8 && !port->cached_edid) { >> - port->cached_edid = drm_get_edid(connector, &port->aux.ddc); >> - } >> break; >> case DP_PEER_DEVICE_DP_LEGACY_CONV: >> if (port->ldps) >> @@ -2378,10 +2382,7 @@ struct edid *drm_dp_mst_get_edid(struct >> drm_connector *connector, struct drm_dp_ >> >> if (port->cached_edid) >> edid = drm_edid_duplicate(port->cached_edid); >> - else { >> - edid = drm_get_edid(connector, &port->aux.ddc); >> - drm_mode_connector_set_tile_property(connector); >> - } >> + >> port->has_audio = drm_detect_monitor_audio(edid); >> drm_dp_put_port(port); >> return edid; >> -- >> 2.1.4 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/dri-devel
On 15 June 2016 at 13:49, Dave Airlie <airlied@gmail.com> wrote: > Excuse me for top posting. > > So I finally got back to this patch, still don't like it. > > Apart from the doing 3 things in once which is just annoying, current > userspace for tiled monitors > relies on the behaviour that the cached edids are retrieved before the > connector is show to userspace. > > This is so that for tiled monitors both connectors are shown with > tiling properties so userspace can pick the correct monitor size. > > Now I realise this isn't great, since at least for the two cable 5K > monitors you won't ever plug them in at once. > > So we should probably discuss more what userspace should do in the > presence of a tiled monitor where it can only see one tile, > > If we go ahead with something like this, things will look kinda ugly > as you plug in a 4k monitor and it'll try and set a mode > on the first tile when it gets the hotplug (or it could be racing and > notice the connectors before the hotplug). So it then > sets the 1920x1080 mode on one connector and gets another hotplug to > set the tiled mode up. Though I think the fix is to leave the cached edid test in add port, And maybe the rest of the patch is okay then. Dave.
Hi, On środa, 15 czerwca 2016 14:14:01 CET wrote: > On 15 June 2016 at 13:49, Dave Airlie <airlied at gmail.com> wrote: > > Excuse me for top posting. > > > > So I finally got back to this patch, still don't like it. > > > > Apart from the doing 3 things in once which is just annoying, current > > userspace for tiled monitors > > relies on the behaviour that the cached edids are retrieved before the > > connector is show to userspace. > > > > This is so that for tiled monitors both connectors are shown with > > tiling properties so userspace can pick the correct monitor size. > > > > Now I realise this isn't great, since at least for the two cable 5K > > monitors you won't ever plug them in at once. > > > > So we should probably discuss more what userspace should do in the > > presence of a tiled monitor where it can only see one tile, > > > > If we go ahead with something like this, things will look kinda ugly > > as you plug in a 4k monitor and it'll try and set a mode > > on the first tile when it gets the hotplug (or it could be racing and > > notice the connectors before the hotplug). So it then > > sets the 1920x1080 mode on one connector and gets another hotplug to > > set the tiled mode up. > > Though I think the fix is to leave the cached edid test in add port, > > And maybe the rest of the patch is okay then. > > Dave. Is this patch still being considered for pulling in? Recently I've been fighting with a HP UltraSlim docking station with a MST hub inside in order to get it working with the amdgpu DAL/DC code. I couldn't get it to work until I stumbled upon this patch. Applying it solved the issue instantly. What I have found during debugging is that the MST hub would return SST sink port numbers < 8 which resulted in no EDID being retrieved and in turn the status of the outputs was 'disconnected'. Regards Chris
On Wed, Jan 25, 2017 at 12:16 PM, Krzysztof Nowicki <krzysztof.a.nowicki@gmail.com> wrote: > Hi, > > On środa, 15 czerwca 2016 14:14:01 CET wrote: >> On 15 June 2016 at 13:49, Dave Airlie <airlied at gmail.com> wrote: >> > Excuse me for top posting. >> > >> > So I finally got back to this patch, still don't like it. >> > >> > Apart from the doing 3 things in once which is just annoying, current >> > userspace for tiled monitors >> > relies on the behaviour that the cached edids are retrieved before the >> > connector is show to userspace. >> > >> > This is so that for tiled monitors both connectors are shown with >> > tiling properties so userspace can pick the correct monitor size. >> > >> > Now I realise this isn't great, since at least for the two cable 5K >> > monitors you won't ever plug them in at once. >> > >> > So we should probably discuss more what userspace should do in the >> > presence of a tiled monitor where it can only see one tile, >> > >> > If we go ahead with something like this, things will look kinda ugly >> > as you plug in a 4k monitor and it'll try and set a mode >> > on the first tile when it gets the hotplug (or it could be racing and >> > notice the connectors before the hotplug). So it then >> > sets the 1920x1080 mode on one connector and gets another hotplug to >> > set the tiled mode up. >> >> Though I think the fix is to leave the cached edid test in add port, >> >> And maybe the rest of the patch is okay then. >> >> Dave. > > Is this patch still being considered for pulling in? > > Recently I've been fighting with a HP UltraSlim docking station with a MST hub > inside in order to get it working with the amdgpu DAL/DC code. I couldn't get > it to work until I stumbled upon this patch. Applying it solved the issue > instantly. > > What I have found during debugging is that the MST hub would return SST sink > port numbers < 8 which resulted in no EDID being retrieved and in turn the > status of the outputs was 'disconnected'. Harry, Any chance you could re-spin this against something more recent and re-send? Alex
You mean rebase dal/dc on drm-next with these patches? I'll see if I find some time today or tomorrow to do that. We'll probably need that anyways for our atomic rework. Do you still rebase amd-staging on drm-next? https://cgit.freedesktop.org/~agd5f/linux/log/?h=amd-staging-drm-next looks a few weeks old now. Harry On 2017-01-25 12:24 PM, Alex Deucher wrote: > On Wed, Jan 25, 2017 at 12:16 PM, Krzysztof Nowicki > <krzysztof.a.nowicki@gmail.com> wrote: >> Hi, >> >> On środa, 15 czerwca 2016 14:14:01 CET wrote: >>> On 15 June 2016 at 13:49, Dave Airlie <airlied at gmail.com> wrote: >>>> Excuse me for top posting. >>>> >>>> So I finally got back to this patch, still don't like it. >>>> >>>> Apart from the doing 3 things in once which is just annoying, current >>>> userspace for tiled monitors >>>> relies on the behaviour that the cached edids are retrieved before the >>>> connector is show to userspace. >>>> >>>> This is so that for tiled monitors both connectors are shown with >>>> tiling properties so userspace can pick the correct monitor size. >>>> >>>> Now I realise this isn't great, since at least for the two cable 5K >>>> monitors you won't ever plug them in at once. >>>> >>>> So we should probably discuss more what userspace should do in the >>>> presence of a tiled monitor where it can only see one tile, >>>> >>>> If we go ahead with something like this, things will look kinda ugly >>>> as you plug in a 4k monitor and it'll try and set a mode >>>> on the first tile when it gets the hotplug (or it could be racing and >>>> notice the connectors before the hotplug). So it then >>>> sets the 1920x1080 mode on one connector and gets another hotplug to >>>> set the tiled mode up. >>> Though I think the fix is to leave the cached edid test in add port, >>> >>> And maybe the rest of the patch is okay then. >>> >>> Dave. >> Is this patch still being considered for pulling in? >> >> Recently I've been fighting with a HP UltraSlim docking station with a MST hub >> inside in order to get it working with the amdgpu DAL/DC code. I couldn't get >> it to work until I stumbled upon this patch. Applying it solved the issue >> instantly. >> >> What I have found during debugging is that the MST hub would return SST sink >> port numbers < 8 which resulted in no EDID being retrieved and in turn the >> status of the outputs was 'disconnected'. > Harry, > > Any chance you could re-spin this against something more recent and re-send? > > Alex
On Wed, Jan 25, 2017 at 1:32 PM, Harry Wentland <harry.wentland@amd.com> wrote: > You mean rebase dal/dc on drm-next with these patches? This is a core MST fix, so it would be good to get upstream for all drivers. > > I'll see if I find some time today or tomorrow to do that. We'll > probably need that anyways for our atomic rework. > > Do you still rebase amd-staging on drm-next? > https://cgit.freedesktop.org/~agd5f/linux/log/?h=amd-staging-drm-next > looks a few weeks old now. I haven't had a chance to rebase that branch in quite a while. Alex > > Harry > > On 2017-01-25 12:24 PM, Alex Deucher wrote: >> On Wed, Jan 25, 2017 at 12:16 PM, Krzysztof Nowicki >> <krzysztof.a.nowicki@gmail.com> wrote: >>> Hi, >>> >>> On środa, 15 czerwca 2016 14:14:01 CET wrote: >>>> On 15 June 2016 at 13:49, Dave Airlie <airlied at gmail.com> wrote: >>>>> Excuse me for top posting. >>>>> >>>>> So I finally got back to this patch, still don't like it. >>>>> >>>>> Apart from the doing 3 things in once which is just annoying, current >>>>> userspace for tiled monitors >>>>> relies on the behaviour that the cached edids are retrieved before the >>>>> connector is show to userspace. >>>>> >>>>> This is so that for tiled monitors both connectors are shown with >>>>> tiling properties so userspace can pick the correct monitor size. >>>>> >>>>> Now I realise this isn't great, since at least for the two cable 5K >>>>> monitors you won't ever plug them in at once. >>>>> >>>>> So we should probably discuss more what userspace should do in the >>>>> presence of a tiled monitor where it can only see one tile, >>>>> >>>>> If we go ahead with something like this, things will look kinda ugly >>>>> as you plug in a 4k monitor and it'll try and set a mode >>>>> on the first tile when it gets the hotplug (or it could be racing and >>>>> notice the connectors before the hotplug). So it then >>>>> sets the 1920x1080 mode on one connector and gets another hotplug to >>>>> set the tiled mode up. >>>> Though I think the fix is to leave the cached edid test in add port, >>>> >>>> And maybe the rest of the patch is okay then. >>>> >>>> Dave. >>> Is this patch still being considered for pulling in? >>> >>> Recently I've been fighting with a HP UltraSlim docking station with a MST hub >>> inside in order to get it working with the amdgpu DAL/DC code. I couldn't get >>> it to work until I stumbled upon this patch. Applying it solved the issue >>> instantly. >>> >>> What I have found during debugging is that the MST hub would return SST sink >>> port numbers < 8 which resulted in no EDID being retrieved and in turn the >>> status of the outputs was 'disconnected'. >> Harry, >> >> Any chance you could re-spin this against something more recent and re-send? >> >> Alex >
On 2017-01-25 05:46 PM, Alex Deucher wrote: > On Wed, Jan 25, 2017 at 1:32 PM, Harry Wentland <harry.wentland@amd.com> wrote: >> You mean rebase dal/dc on drm-next with these patches? > > This is a core MST fix, so it would be good to get upstream for all drivers. > Ignore my previous message. Somehow I though this was something else. It didn't make it upstream because it broke tiled display. We'll need to find some time to take a second look. It's definitely still on our list. Harry >> >> I'll see if I find some time today or tomorrow to do that. We'll >> probably need that anyways for our atomic rework. >> >> Do you still rebase amd-staging on drm-next? >> https://cgit.freedesktop.org/~agd5f/linux/log/?h=amd-staging-drm-next >> looks a few weeks old now. > > I haven't had a chance to rebase that branch in quite a while. > > Alex > >> >> Harry >> >> On 2017-01-25 12:24 PM, Alex Deucher wrote: >>> On Wed, Jan 25, 2017 at 12:16 PM, Krzysztof Nowicki >>> <krzysztof.a.nowicki@gmail.com> wrote: >>>> Hi, >>>> >>>> On środa, 15 czerwca 2016 14:14:01 CET wrote: >>>>> On 15 June 2016 at 13:49, Dave Airlie <airlied at gmail.com> wrote: >>>>>> Excuse me for top posting. >>>>>> >>>>>> So I finally got back to this patch, still don't like it. >>>>>> >>>>>> Apart from the doing 3 things in once which is just annoying, current >>>>>> userspace for tiled monitors >>>>>> relies on the behaviour that the cached edids are retrieved before the >>>>>> connector is show to userspace. >>>>>> >>>>>> This is so that for tiled monitors both connectors are shown with >>>>>> tiling properties so userspace can pick the correct monitor size. >>>>>> >>>>>> Now I realise this isn't great, since at least for the two cable 5K >>>>>> monitors you won't ever plug them in at once. >>>>>> >>>>>> So we should probably discuss more what userspace should do in the >>>>>> presence of a tiled monitor where it can only see one tile, >>>>>> >>>>>> If we go ahead with something like this, things will look kinda ugly >>>>>> as you plug in a 4k monitor and it'll try and set a mode >>>>>> on the first tile when it gets the hotplug (or it could be racing and >>>>>> notice the connectors before the hotplug). So it then >>>>>> sets the 1920x1080 mode on one connector and gets another hotplug to >>>>>> set the tiled mode up. >>>>> Though I think the fix is to leave the cached edid test in add port, >>>>> >>>>> And maybe the rest of the patch is okay then. >>>>> >>>>> Dave. >>>> Is this patch still being considered for pulling in? >>>> >>>> Recently I've been fighting with a HP UltraSlim docking station with a MST hub >>>> inside in order to get it working with the amdgpu DAL/DC code. I couldn't get >>>> it to work until I stumbled upon this patch. Applying it solved the issue >>>> instantly. >>>> >>>> What I have found during debugging is that the MST hub would return SST sink >>>> port numbers < 8 which resulted in no EDID being retrieved and in turn the >>>> status of the outputs was 'disconnected'. >>> Harry, >>> >>> Any chance you could re-spin this against something more recent and re-send? >>> >>> Alex >>
On środa, 25 stycznia 2017 12:24:35 CET Alex Deucher wrote: > On Wed, Jan 25, 2017 at 12:16 PM, Krzysztof Nowicki > > <krzysztof.a.nowicki@gmail.com> wrote: > > Hi, > > > > > > Is this patch still being considered for pulling in? > > > > Recently I've been fighting with a HP UltraSlim docking station with a MST > > hub inside in order to get it working with the amdgpu DAL/DC code. I > > couldn't get it to work until I stumbled upon this patch. Applying it > > solved the issue instantly. > > > > What I have found during debugging is that the MST hub would return SST > > sink port numbers < 8 which resulted in no EDID being retrieved and in > > turn the status of the outputs was 'disconnected'. > > Harry, > > Any chance you could re-spin this against something more recent and re-send? > > Alex Hi, I have actually investigated this problem further and noticed that it's a problem in the amdgpu DAL/DC driver, which makes an incorrect assumption about the presence of a cached EDID. The fact that it started to work was only a side-effect of this patch. I have submitted a patch to amd-gfx with what I believe is the proper fix. Regards Chris
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 041597b7a7c6..052c20ca35ee 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -1130,13 +1130,11 @@ static void drm_dp_add_port(struct drm_dp_mst_branch *mstb, drm_dp_put_port(port); goto out; } - if (port->port_num >= DP_MST_LOGICAL_PORT_0) { - port->cached_edid = drm_get_edid(port->connector, &port->aux.ddc); - drm_mode_connector_set_tile_property(port->connector); - } + + drm_mode_connector_set_tile_property(port->connector); + (*mstb->mgr->cbs->register_connector)(port->connector); } - out: /* put reference to this port */ drm_dp_put_port(port); @@ -1161,9 +1159,9 @@ static void drm_dp_update_port(struct drm_dp_mst_branch *mstb, port->ddps = conn_stat->displayport_device_plug_status; if (old_ddps != port->ddps) { + dowork = true; if (port->ddps) { drm_dp_check_port_guid(mstb, port); - dowork = true; } else { port->guid_valid = false; port->available_pbn = 0; @@ -1271,8 +1269,13 @@ static void drm_dp_check_and_send_link_address(struct drm_dp_mst_topology_mgr *m if (port->input) continue; - if (!port->ddps) + if (!port->ddps) { + if (port->cached_edid) { + kfree(port->cached_edid); + port->cached_edid = NULL; + } continue; + } if (!port->available_pbn) drm_dp_send_enum_path_resources(mgr, mstb, port); @@ -1283,6 +1286,12 @@ static void drm_dp_check_and_send_link_address(struct drm_dp_mst_topology_mgr *m drm_dp_check_and_send_link_address(mgr, mstb_child); drm_dp_put_mst_branch_device(mstb_child); } + } else if (port->pdt == DP_PEER_DEVICE_SST_SINK || + port->pdt == DP_PEER_DEVICE_DP_LEGACY_CONV) { + if (!port->cached_edid) { + port->cached_edid = + drm_get_edid(port->connector, &port->aux.ddc); + } } } } @@ -1302,6 +1311,8 @@ static void drm_dp_mst_link_probe_work(struct work_struct *work) drm_dp_check_and_send_link_address(mgr, mstb); drm_dp_put_mst_branch_device(mstb); } + + (*mgr->cbs->hotplug)(mgr); } static bool drm_dp_validate_guid(struct drm_dp_mst_topology_mgr *mgr, @@ -1558,7 +1569,6 @@ static void drm_dp_send_link_address(struct drm_dp_mst_topology_mgr *mgr, for (i = 0; i < txmsg->reply.u.link_addr.nports; i++) { drm_dp_add_port(mstb, mgr->dev, &txmsg->reply.u.link_addr.ports[i]); } - (*mgr->cbs->hotplug)(mgr); } } else { mstb->link_address_sent = false; @@ -2232,8 +2242,6 @@ static int drm_dp_mst_handle_up_req(struct drm_dp_mst_topology_mgr *mgr) drm_dp_update_port(mstb, &msg.u.conn_stat); DRM_DEBUG_KMS("Got CSN: pn: %d ldps:%d ddps: %d mcs: %d ip: %d pdt: %d\n", msg.u.conn_stat.port_number, msg.u.conn_stat.legacy_device_plug_status, msg.u.conn_stat.displayport_device_plug_status, msg.u.conn_stat.message_capability_status, msg.u.conn_stat.input_port, msg.u.conn_stat.peer_device_type); - (*mgr->cbs->hotplug)(mgr); - } else if (msg.req_type == DP_RESOURCE_STATUS_NOTIFY) { drm_dp_send_up_ack_reply(mgr, mgr->mst_primary, msg.req_type, seqno, false); if (!mstb) @@ -2320,10 +2328,6 @@ enum drm_connector_status drm_dp_mst_detect_port(struct drm_connector *connector case DP_PEER_DEVICE_SST_SINK: status = connector_status_connected; - /* for logical ports - cache the EDID */ - if (port->port_num >= 8 && !port->cached_edid) { - port->cached_edid = drm_get_edid(connector, &port->aux.ddc); - } break; case DP_PEER_DEVICE_DP_LEGACY_CONV: if (port->ldps) @@ -2378,10 +2382,7 @@ struct edid *drm_dp_mst_get_edid(struct drm_connector *connector, struct drm_dp_ if (port->cached_edid) edid = drm_edid_duplicate(port->cached_edid); - else { - edid = drm_get_edid(connector, &port->aux.ddc); - drm_mode_connector_set_tile_property(connector); - } + port->has_audio = drm_detect_monitor_audio(edid); drm_dp_put_port(port); return edid;