AMD's patch series for adding DSC support to the MST helpers unfortunately introduced a few regressions into the kernel that I didn't get around to fixing until just now. I would have reverted the changes earlier, but seeing as that would have reverted all of amd's DSC support + everything that was done on top of that I realllllly wanted to avoid doing that.
Anyway, this should fix everything as far as I can tell. Note that I don't have any DSC displays locally yet, so if someone from AMD could sanity check this I would appreciate it ♥.
Cc: Mikita Lipski mikita.lipski@amd.com Cc: Alex Deucher alexander.deucher@amd.com Cc: Sean Paul seanpaul@google.com Cc: Hans de Goede hdegoede@redhat.com
Lyude Paul (3): drm/dp_mst: Rename drm_dp_mst_is_dp_mst_end_device() to be less redundant drm/dp_mst: Don't show connectors as connected before probing available PBN drm/dp_mst: Rewrite and fix bandwidth limit checks
drivers/gpu/drm/drm_dp_mst_topology.c | 124 ++++++++++++++++++++------ 1 file changed, 96 insertions(+), 28 deletions(-)
It's already prefixed by dp_mst, so we don't really need to repeat ourselves here. One of the changes I should have picked up originally when reviewing MST DSC support.
There should be no functional changes here
Cc: Mikita Lipski mikita.lipski@amd.com Cc: Alex Deucher alexander.deucher@amd.com Cc: Sean Paul seanpaul@google.com Cc: Hans de Goede hdegoede@redhat.com Signed-off-by: Lyude Paul lyude@redhat.com --- drivers/gpu/drm/drm_dp_mst_topology.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 61e7beada832..207eef08d12c 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -1937,7 +1937,7 @@ static u8 drm_dp_calculate_rad(struct drm_dp_mst_port *port, return parent_lct + 1; }
-static bool drm_dp_mst_is_dp_mst_end_device(u8 pdt, bool mcs) +static bool drm_dp_mst_is_end_device(u8 pdt, bool mcs) { switch (pdt) { case DP_PEER_DEVICE_DP_LEGACY_CONV: @@ -1967,13 +1967,13 @@ drm_dp_port_set_pdt(struct drm_dp_mst_port *port, u8 new_pdt,
/* Teardown the old pdt, if there is one */ if (port->pdt != DP_PEER_DEVICE_NONE) { - if (drm_dp_mst_is_dp_mst_end_device(port->pdt, port->mcs)) { + if (drm_dp_mst_is_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)) { + drm_dp_mst_is_end_device(new_pdt, new_mcs)) { port->pdt = new_pdt; port->mcs = new_mcs; return 0; @@ -1993,7 +1993,7 @@ drm_dp_port_set_pdt(struct drm_dp_mst_port *port, u8 new_pdt, port->mcs = new_mcs;
if (port->pdt != DP_PEER_DEVICE_NONE) { - if (drm_dp_mst_is_dp_mst_end_device(port->pdt, port->mcs)) { + if (drm_dp_mst_is_end_device(port->pdt, port->mcs)) { /* add i2c over sideband */ ret = drm_dp_mst_register_i2c_bus(&port->aux); } else { @@ -2169,7 +2169,7 @@ drm_dp_mst_port_add_connector(struct drm_dp_mst_branch *mstb, }
if (port->pdt != DP_PEER_DEVICE_NONE && - drm_dp_mst_is_dp_mst_end_device(port->pdt, port->mcs)) { + drm_dp_mst_is_end_device(port->pdt, port->mcs)) { port->cached_edid = drm_get_edid(port->connector, &port->aux.ddc); drm_connector_set_tile_property(port->connector);
It's next to impossible for us to do connector probing on topologies without occasionally racing with userspace, since creating a connector itself causes a hotplug event which we have to send before probing the available PBN of a connector. Even if we didn't have this hotplug event sent, there's still always a chance that userspace started probing connectors before we finished probing the topology.
This can be a problem when validating a new MST state since the connector will be shown as connected briefly, but without any available PBN - causing any atomic state which would enable said connector to fail with -ENOSPC. So, let's simply workaround this by telling userspace new MST connectors are disconnected until we've finished probing their PBN. Since we always send a hotplug event at the end of the link address probing process, userspace will still know to reprobe the connector when we're ready.
Signed-off-by: Lyude Paul lyude@redhat.com Fixes: cd82d82cbc04 ("drm/dp_mst: Add branch bandwidth validation to MST atomic check") Cc: Mikita Lipski mikita.lipski@amd.com Cc: Alex Deucher alexander.deucher@amd.com Cc: Sean Paul seanpaul@google.com Cc: Hans de Goede hdegoede@redhat.com --- drivers/gpu/drm/drm_dp_mst_topology.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 207eef08d12c..7b0ff0cff954 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -4033,6 +4033,19 @@ drm_dp_mst_detect_port(struct drm_connector *connector, ret = connector_status_connected; break; } + + /* We don't want to tell userspace the port is actually plugged into + * anything until we've finished probing it's available_pbn, otherwise + * userspace will see racy atomic check failures + * + * Since we always send a hotplug at the end of probing topology + * state, we can just let userspace reprobe this connector later. + */ + if (ret == connector_status_connected && !port->available_pbn) { + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] not ready yet (PBN not probed)\n", + connector->base.id, connector->name); + ret = connector_status_disconnected; + } out: drm_dp_mst_topology_put_port(port); return ret;
On Wed, Mar 04, 2020 at 05:36:12PM -0500, Lyude Paul wrote:
It's next to impossible for us to do connector probing on topologies without occasionally racing with userspace, since creating a connector itself causes a hotplug event which we have to send before probing the available PBN of a connector. Even if we didn't have this hotplug event sent, there's still always a chance that userspace started probing connectors before we finished probing the topology.
This can be a problem when validating a new MST state since the connector will be shown as connected briefly, but without any available PBN - causing any atomic state which would enable said connector to fail with -ENOSPC. So, let's simply workaround this by telling userspace new MST connectors are disconnected until we've finished probing their PBN. Since we always send a hotplug event at the end of the link address probing process, userspace will still know to reprobe the connector when we're ready.
Signed-off-by: Lyude Paul lyude@redhat.com Fixes: cd82d82cbc04 ("drm/dp_mst: Add branch bandwidth validation to MST atomic check") Cc: Mikita Lipski mikita.lipski@amd.com Cc: Alex Deucher alexander.deucher@amd.com Cc: Sean Paul seanpaul@google.com Cc: Hans de Goede hdegoede@redhat.com
drivers/gpu/drm/drm_dp_mst_topology.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 207eef08d12c..7b0ff0cff954 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -4033,6 +4033,19 @@ drm_dp_mst_detect_port(struct drm_connector *connector, ret = connector_status_connected; break; }
- /* We don't want to tell userspace the port is actually plugged into
* anything until we've finished probing it's available_pbn, otherwise
"its"
Why is the connector even registered before we've finished the probe?
* userspace will see racy atomic check failures** Since we always send a hotplug at the end of probing topology* state, we can just let userspace reprobe this connector later.*/- if (ret == connector_status_connected && !port->available_pbn) {
DRM_DEBUG_KMS("[CONNECTOR:%d:%s] not ready yet (PBN not probed)\n",connector->base.id, connector->name);ret = connector_status_disconnected;- }
out: drm_dp_mst_topology_put_port(port); return ret; -- 2.24.1
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, 2020-03-05 at 15:11 +0200, Ville Syrjälä wrote:
On Wed, Mar 04, 2020 at 05:36:12PM -0500, Lyude Paul wrote:
It's next to impossible for us to do connector probing on topologies without occasionally racing with userspace, since creating a connector itself causes a hotplug event which we have to send before probing the available PBN of a connector. Even if we didn't have this hotplug event sent, there's still always a chance that userspace started probing connectors before we finished probing the topology.
This can be a problem when validating a new MST state since the connector will be shown as connected briefly, but without any available PBN - causing any atomic state which would enable said connector to fail with -ENOSPC. So, let's simply workaround this by telling userspace new MST connectors are disconnected until we've finished probing their PBN. Since we always send a hotplug event at the end of the link address probing process, userspace will still know to reprobe the connector when we're ready.
Signed-off-by: Lyude Paul lyude@redhat.com Fixes: cd82d82cbc04 ("drm/dp_mst: Add branch bandwidth validation to MST atomic check") Cc: Mikita Lipski mikita.lipski@amd.com Cc: Alex Deucher alexander.deucher@amd.com Cc: Sean Paul seanpaul@google.com Cc: Hans de Goede hdegoede@redhat.com
drivers/gpu/drm/drm_dp_mst_topology.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 207eef08d12c..7b0ff0cff954 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -4033,6 +4033,19 @@ drm_dp_mst_detect_port(struct drm_connector *connector, ret = connector_status_connected; break; }
- /* We don't want to tell userspace the port is actually plugged into
* anything until we've finished probing it's available_pbn, otherwise"its"
Why is the connector even registered before we've finished the probe?
Oops, I'm not sure how I did this by accident but the explanation I gave in the commit message was uh, completely wrong. I must have forgotten that I made sure we didn't expose connectors before probing their PBN back when I started my MST cleanup....
So: despite what I said before it's not actually when new connectors are created, it's when downstream hotplugs happen which means that the conenctor's always going to be there before we probe the available_pbn. I did just notice though that we send a hotplug on connection status notifications even before we've finished the PBN probe, so I might be able to improve on that as well. We still definitely want to report the connector as disconnected before we have the available PBN though, in case another probe was already going before we got the connection status notification.
I'll make sure to fixup the explanation in the commit message on the next respin
* userspace will see racy atomic check failures** Since we always send a hotplug at the end of probing topology* state, we can just let userspace reprobe this connector later.*/- if (ret == connector_status_connected && !port->available_pbn) {
DRM_DEBUG_KMS("[CONNECTOR:%d:%s] not ready yet (PBN notprobed)\n",
connector->base.id, connector->name);ret = connector_status_disconnected;- }
out: drm_dp_mst_topology_put_port(port); return ret; -- 2.24.1
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Mar 05, 2020 at 01:13:36PM -0500, Lyude Paul wrote:
On Thu, 2020-03-05 at 15:11 +0200, Ville Syrjälä wrote:
On Wed, Mar 04, 2020 at 05:36:12PM -0500, Lyude Paul wrote:
It's next to impossible for us to do connector probing on topologies without occasionally racing with userspace, since creating a connector itself causes a hotplug event which we have to send before probing the available PBN of a connector. Even if we didn't have this hotplug event sent, there's still always a chance that userspace started probing connectors before we finished probing the topology.
This can be a problem when validating a new MST state since the connector will be shown as connected briefly, but without any available PBN - causing any atomic state which would enable said connector to fail with -ENOSPC. So, let's simply workaround this by telling userspace new MST connectors are disconnected until we've finished probing their PBN. Since we always send a hotplug event at the end of the link address probing process, userspace will still know to reprobe the connector when we're ready.
Signed-off-by: Lyude Paul lyude@redhat.com Fixes: cd82d82cbc04 ("drm/dp_mst: Add branch bandwidth validation to MST atomic check") Cc: Mikita Lipski mikita.lipski@amd.com Cc: Alex Deucher alexander.deucher@amd.com Cc: Sean Paul seanpaul@google.com Cc: Hans de Goede hdegoede@redhat.com
drivers/gpu/drm/drm_dp_mst_topology.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 207eef08d12c..7b0ff0cff954 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -4033,6 +4033,19 @@ drm_dp_mst_detect_port(struct drm_connector *connector, ret = connector_status_connected; break; }
- /* We don't want to tell userspace the port is actually plugged into
* anything until we've finished probing it's available_pbn, otherwise"its"
Why is the connector even registered before we've finished the probe?
Oops, I'm not sure how I did this by accident but the explanation I gave in the commit message was uh, completely wrong. I must have forgotten that I made sure we didn't expose connectors before probing their PBN back when I started my MST cleanup....
So: despite what I said before it's not actually when new connectors are created, it's when downstream hotplugs happen which means that the conenctor's always going to be there before we probe the available_pbn.
Not sure I understand. You're saying this is going to change for already existing connectors when something else gets plugged in, and either we zero it out during the probe or it always was zero to begin with for whatever reason?
I did just notice though that we send a hotplug on connection status notifications even before we've finished the PBN probe, so I might be able to improve on that as well. We still definitely want to report the connector as disconnected before we have the available PBN though, in case another probe was already going before we got the connection status notification.
I'll make sure to fixup the explanation in the commit message on the next respin
* userspace will see racy atomic check failures** Since we always send a hotplug at the end of probing topology* state, we can just let userspace reprobe this connector later.*/- if (ret == connector_status_connected && !port->available_pbn) {
DRM_DEBUG_KMS("[CONNECTOR:%d:%s] not ready yet (PBN notprobed)\n",
connector->base.id, connector->name);ret = connector_status_disconnected;- }
out: drm_dp_mst_topology_put_port(port); return ret; -- 2.24.1
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Cheers, Lyude Paul (she/her) Associate Software Engineer at Red Hat
On Thu, 2020-03-05 at 20:29 +0200, Ville Syrjälä wrote:
On Thu, Mar 05, 2020 at 01:13:36PM -0500, Lyude Paul wrote:
On Thu, 2020-03-05 at 15:11 +0200, Ville Syrjälä wrote:
On Wed, Mar 04, 2020 at 05:36:12PM -0500, Lyude Paul wrote:
It's next to impossible for us to do connector probing on topologies without occasionally racing with userspace, since creating a connector itself causes a hotplug event which we have to send before probing the available PBN of a connector. Even if we didn't have this hotplug event sent, there's still always a chance that userspace started probing connectors before we finished probing the topology.
This can be a problem when validating a new MST state since the connector will be shown as connected briefly, but without any available PBN - causing any atomic state which would enable said connector to fail with -ENOSPC. So, let's simply workaround this by telling userspace new MST connectors are disconnected until we've finished probing their PBN. Since we always send a hotplug event at the end of the link address probing process, userspace will still know to reprobe the connector when we're ready.
Signed-off-by: Lyude Paul lyude@redhat.com Fixes: cd82d82cbc04 ("drm/dp_mst: Add branch bandwidth validation to MST atomic check") Cc: Mikita Lipski mikita.lipski@amd.com Cc: Alex Deucher alexander.deucher@amd.com Cc: Sean Paul seanpaul@google.com Cc: Hans de Goede hdegoede@redhat.com
drivers/gpu/drm/drm_dp_mst_topology.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 207eef08d12c..7b0ff0cff954 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -4033,6 +4033,19 @@ drm_dp_mst_detect_port(struct drm_connector *connector, ret = connector_status_connected; break; }
- /* We don't want to tell userspace the port is actually
plugged into
* anything until we've finished probing it's available_pbn,otherwise
"its"
Why is the connector even registered before we've finished the probe?
Oops, I'm not sure how I did this by accident but the explanation I gave in the commit message was uh, completely wrong. I must have forgotten that I made sure we didn't expose connectors before probing their PBN back when I started my MST cleanup....
So: despite what I said before it's not actually when new connectors are created, it's when downstream hotplugs happen which means that the conenctor's always going to be there before we probe the available_pbn.
Not sure I understand. You're saying this is going to change for already existing connectors when something else gets plugged in, and either we zero it out during the probe or it always was zero to begin with for whatever reason?
So: you just made me realize that I'm not actually sure whether there's any point to us clearing port->available_pbn here since the available_pbn (at least the value that we cache on initial link address probing for bandwidth constraint checking) shouldn't actually change on a port just because of a hotplug. I bet this is probably causing more problems on it's own as well, since reprobing the available_pbn might actually give us a value that reflects allocations on other ports that are already in place.
So: I think what I'm going to do instead is make it so that we never clear port->available_pbn; mainly to make things less complicated during suspend/resume, since we want to make sure there's always some sort of PBN value populated even during the middle of reprobing the link address on resume. That way we don't have to pretend that it's ever disconnected either. Will send a respin in a bit.
I did just notice though that we send a hotplug on connection status notifications even before we've finished the PBN probe, so I might be able to improve on that as well. We still definitely want to report the connector as disconnected before we have the available PBN though, in case another probe was already going before we got the connection status notification.
I'll make sure to fixup the explanation in the commit message on the next respin
* userspace will see racy atomic check failures** Since we always send a hotplug at the end of probingtopology
* state, we can just let userspace reprobe this connectorlater.
*/- if (ret == connector_status_connected && !port->available_pbn)
{
DRM_DEBUG_KMS("[CONNECTOR:%d:%s] not ready yet (PBNnot probed)\n",
connector->base.id, connector->name);ret = connector_status_disconnected;- }
out: drm_dp_mst_topology_put_port(port); return ret; -- 2.24.1
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Cheers, Lyude Paul (she/her) Associate Software Engineer at Red Hat
On Thu, 2020-03-05 at 13:52 -0500, Lyude Paul wrote:
On Thu, 2020-03-05 at 20:29 +0200, Ville Syrjälä wrote:
On Thu, Mar 05, 2020 at 01:13:36PM -0500, Lyude Paul wrote:
On Thu, 2020-03-05 at 15:11 +0200, Ville Syrjälä wrote:
On Wed, Mar 04, 2020 at 05:36:12PM -0500, Lyude Paul wrote:
It's next to impossible for us to do connector probing on topologies without occasionally racing with userspace, since creating a connector itself causes a hotplug event which we have to send before probing the available PBN of a connector. Even if we didn't have this hotplug event sent, there's still always a chance that userspace started probing connectors before we finished probing the topology.
This can be a problem when validating a new MST state since the connector will be shown as connected briefly, but without any available PBN - causing any atomic state which would enable said connector to fail with -ENOSPC. So, let's simply workaround this by telling userspace new MST connectors are disconnected until we've finished probing their PBN. Since we always send a hotplug event at the end of the link address probing process, userspace will still know to reprobe the connector when we're ready.
Signed-off-by: Lyude Paul lyude@redhat.com Fixes: cd82d82cbc04 ("drm/dp_mst: Add branch bandwidth validation to MST atomic check") Cc: Mikita Lipski mikita.lipski@amd.com Cc: Alex Deucher alexander.deucher@amd.com Cc: Sean Paul seanpaul@google.com Cc: Hans de Goede hdegoede@redhat.com
drivers/gpu/drm/drm_dp_mst_topology.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 207eef08d12c..7b0ff0cff954 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -4033,6 +4033,19 @@ drm_dp_mst_detect_port(struct drm_connector *connector, ret = connector_status_connected; break; }
- /* We don't want to tell userspace the port is actually
plugged into
* anything until we've finished probing it's available_pbn,otherwise
"its"
Why is the connector even registered before we've finished the probe?
Oops, I'm not sure how I did this by accident but the explanation I gave in the commit message was uh, completely wrong. I must have forgotten that I made sure we didn't expose connectors before probing their PBN back when I started my MST cleanup....
So: despite what I said before it's not actually when new connectors are created, it's when downstream hotplugs happen which means that the conenctor's always going to be there before we probe the available_pbn.
Not sure I understand. You're saying this is going to change for already existing connectors when something else gets plugged in, and either we zero it out during the probe or it always was zero to begin with for whatever reason?
So: you just made me realize that I'm not actually sure whether there's any point to us clearing port->available_pbn here since the available_pbn (at least the value that we cache on initial link address probing for bandwidth constraint checking) shouldn't actually change on a port just because of a hotplug. I bet this is probably causing more problems on it's own as well, since reprobing the available_pbn might actually give us a value that reflects allocations on other ports that are already in place.
So: I think what I'm going to do instead is make it so that we never clear port->available_pbn; mainly to make things less complicated during suspend/resume, since we want to make sure there's always some sort of PBN value populated even during the middle of reprobing the link address on resume. That way we don't have to pretend that it's ever disconnected either. Will send a respin in a bit.
Wait, nope, I believe I am the fool here - _supposedly_ available bw is supposed to reflect the smallest link rate that occurs in a patch to a branch device. I think, me and sean paul are looking at this a bit more closely. I think I might need to do some more playing around with my hubs to make sure this value is actually what we think it is because unfortunately the spec is pretty vague on this from what I can tell :(
I did just notice though that we send a hotplug on connection status notifications even before we've finished the PBN probe, so I might be able to improve on that as well. We still definitely want to report the connector as disconnected before we have the available PBN though, in case another probe was already going before we got the connection status notification.
I'll make sure to fixup the explanation in the commit message on the next respin
* userspace will see racy atomic check failures** Since we always send a hotplug at the end of probingtopology
* state, we can just let userspace reprobe this connectorlater.
*/- if (ret == connector_status_connected && !port-
available_pbn)
{
DRM_DEBUG_KMS("[CONNECTOR:%d:%s] not ready yet (PBNnot probed)\n",
connector->base.id, connector->name);ret = connector_status_disconnected;- }
out: drm_dp_mst_topology_put_port(port); return ret; -- 2.24.1
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Cheers, Lyude Paul (she/her) Associate Software Engineer at Red Hat
On Thu, 2020-03-05 at 20:29 +0200, Ville Syrjälä wrote:
On Thu, Mar 05, 2020 at 01:13:36PM -0500, Lyude Paul wrote:
On Thu, 2020-03-05 at 15:11 +0200, Ville Syrjälä wrote:
On Wed, Mar 04, 2020 at 05:36:12PM -0500, Lyude Paul wrote:
It's next to impossible for us to do connector probing on topologies without occasionally racing with userspace, since creating a connector itself causes a hotplug event which we have to send before probing the available PBN of a connector. Even if we didn't have this hotplug event sent, there's still always a chance that userspace started probing connectors before we finished probing the topology.
This can be a problem when validating a new MST state since the connector will be shown as connected briefly, but without any available PBN - causing any atomic state which would enable said connector to fail with -ENOSPC. So, let's simply workaround this by telling userspace new MST connectors are disconnected until we've finished probing their PBN. Since we always send a hotplug event at the end of the link address probing process, userspace will still know to reprobe the connector when we're ready.
Signed-off-by: Lyude Paul lyude@redhat.com Fixes: cd82d82cbc04 ("drm/dp_mst: Add branch bandwidth validation to MST atomic check") Cc: Mikita Lipski mikita.lipski@amd.com Cc: Alex Deucher alexander.deucher@amd.com Cc: Sean Paul seanpaul@google.com Cc: Hans de Goede hdegoede@redhat.com
drivers/gpu/drm/drm_dp_mst_topology.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 207eef08d12c..7b0ff0cff954 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -4033,6 +4033,19 @@ drm_dp_mst_detect_port(struct drm_connector *connector, ret = connector_status_connected; break; }
- /* We don't want to tell userspace the port is actually
plugged into
* anything until we've finished probing it's available_pbn,otherwise
"its"
Why is the connector even registered before we've finished the probe?
Oops, I'm not sure how I did this by accident but the explanation I gave in the commit message was uh, completely wrong. I must have forgotten that I made sure we didn't expose connectors before probing their PBN back when I started my MST cleanup....
So: despite what I said before it's not actually when new connectors are created, it's when downstream hotplugs happen which means that the conenctor's always going to be there before we probe the available_pbn.
Not sure I understand. You're saying this is going to change for already existing connectors when something else gets plugged in, and either we zero it out during the probe or it always was zero to begin with for whatever reason?
ok-me and Sean Paul did some playing around with available_pbn and full_pbn (I'll get into this one in a moment), and I also played around with a couple of different hubs and have a much better understanding of how this should work now.
So: first off tl;dr available_pbn is absolutely not what we want in basically any scenario, we actually want to use the full_pbn field that we get when sending ENUM_PATH_RESOURCES. Second, full_pbn represents the _smallest_ bandwidth limitation encountered in the path between the root MSTB and each connected sink. Remember that there's technically a DisplayPort link trained between each branch device going down the topology, so that bandwidth limitation basically equates to "what is the lowest trained link rate that exists down the path to this port?". This also means that full_pbn will potentially change every time a new connector is plugged in, as some hubs will be clever and optimize the link rate they decide to use. Likewise, since there's not going to be anything trained on a disconnected port (e.g. ddps=0) there's no point in keeping full_pbn around for disconnected ports, since otherwise we might let userspace see a connected port with a stale full_pbn value.
So-IMHO the behavior of not letting connectors show as connected until we also have their full_pbn probed should definitely be the right solution here. Especially if we want to eventually start pruning modes based on full_pbn at some point in the future.
I did just notice though that we send a hotplug on connection status notifications even before we've finished the PBN probe, so I might be able to improve on that as well. We still definitely want to report the connector as disconnected before we have the available PBN though, in case another probe was already going before we got the connection status notification.
I'll make sure to fixup the explanation in the commit message on the next respin
* userspace will see racy atomic check failures** Since we always send a hotplug at the end of probingtopology
* state, we can just let userspace reprobe this connectorlater.
*/- if (ret == connector_status_connected && !port->available_pbn)
{
DRM_DEBUG_KMS("[CONNECTOR:%d:%s] not ready yet (PBNnot probed)\n",
connector->base.id, connector->name);ret = connector_status_disconnected;- }
out: drm_dp_mst_topology_put_port(port); return ret; -- 2.24.1
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Cheers, Lyude Paul (she/her) Associate Software Engineer at Red Hat
final correction: I probably could actually get rid of this patch and do this a bit more nicely by just making sure that we reprobe path resources for connectors while under drm_dp_mst_topology_mgr->base.lock on CSNs, instead of punting it off to the link address work like we seem to be doing. So, going to try doing that instead.
On Fri, 2020-03-06 at 15:03 -0500, Lyude Paul wrote:
On Thu, 2020-03-05 at 20:29 +0200, Ville Syrjälä wrote:
On Thu, Mar 05, 2020 at 01:13:36PM -0500, Lyude Paul wrote:
On Thu, 2020-03-05 at 15:11 +0200, Ville Syrjälä wrote:
On Wed, Mar 04, 2020 at 05:36:12PM -0500, Lyude Paul wrote:
It's next to impossible for us to do connector probing on topologies without occasionally racing with userspace, since creating a connector itself causes a hotplug event which we have to send before probing the available PBN of a connector. Even if we didn't have this hotplug event sent, there's still always a chance that userspace started probing connectors before we finished probing the topology.
This can be a problem when validating a new MST state since the connector will be shown as connected briefly, but without any available PBN - causing any atomic state which would enable said connector to fail with -ENOSPC. So, let's simply workaround this by telling userspace new MST connectors are disconnected until we've finished probing their PBN. Since we always send a hotplug event at the end of the link address probing process, userspace will still know to reprobe the connector when we're ready.
Signed-off-by: Lyude Paul lyude@redhat.com Fixes: cd82d82cbc04 ("drm/dp_mst: Add branch bandwidth validation to MST atomic check") Cc: Mikita Lipski mikita.lipski@amd.com Cc: Alex Deucher alexander.deucher@amd.com Cc: Sean Paul seanpaul@google.com Cc: Hans de Goede hdegoede@redhat.com
drivers/gpu/drm/drm_dp_mst_topology.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 207eef08d12c..7b0ff0cff954 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -4033,6 +4033,19 @@ drm_dp_mst_detect_port(struct drm_connector *connector, ret = connector_status_connected; break; }
- /* We don't want to tell userspace the port is actually
plugged into
* anything until we've finished probing it's available_pbn,otherwise
"its"
Why is the connector even registered before we've finished the probe?
Oops, I'm not sure how I did this by accident but the explanation I gave in the commit message was uh, completely wrong. I must have forgotten that I made sure we didn't expose connectors before probing their PBN back when I started my MST cleanup....
So: despite what I said before it's not actually when new connectors are created, it's when downstream hotplugs happen which means that the conenctor's always going to be there before we probe the available_pbn.
Not sure I understand. You're saying this is going to change for already existing connectors when something else gets plugged in, and either we zero it out during the probe or it always was zero to begin with for whatever reason?
ok-me and Sean Paul did some playing around with available_pbn and full_pbn (I'll get into this one in a moment), and I also played around with a couple of different hubs and have a much better understanding of how this should work now.
So: first off tl;dr available_pbn is absolutely not what we want in basically any scenario, we actually want to use the full_pbn field that we get when sending ENUM_PATH_RESOURCES. Second, full_pbn represents the _smallest_ bandwidth limitation encountered in the path between the root MSTB and each connected sink. Remember that there's technically a DisplayPort link trained between each branch device going down the topology, so that bandwidth limitation basically equates to "what is the lowest trained link rate that exists down the path to this port?". This also means that full_pbn will potentially change every time a new connector is plugged in, as some hubs will be clever and optimize the link rate they decide to use. Likewise, since there's not going to be anything trained on a disconnected port (e.g. ddps=0) there's no point in keeping full_pbn around for disconnected ports, since otherwise we might let userspace see a connected port with a stale full_pbn value.
So-IMHO the behavior of not letting connectors show as connected until we also have their full_pbn probed should definitely be the right solution here. Especially if we want to eventually start pruning modes based on full_pbn at some point in the future.
I did just notice though that we send a hotplug on connection status notifications even before we've finished the PBN probe, so I might be able to improve on that as well. We still definitely want to report the connector as disconnected before we have the available PBN though, in case another probe was already going before we got the connection status notification.
I'll make sure to fixup the explanation in the commit message on the next respin
* userspace will see racy atomic check failures** Since we always send a hotplug at the end of probingtopology
* state, we can just let userspace reprobe this connectorlater.
*/- if (ret == connector_status_connected && !port-
available_pbn)
{
DRM_DEBUG_KMS("[CONNECTOR:%d:%s] not ready yet (PBNnot probed)\n",
connector->base.id, connector->name);ret = connector_status_disconnected;- }
out: drm_dp_mst_topology_put_port(port); return ret; -- 2.24.1
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Cheers, Lyude Paul (she/her) Associate Software Engineer at Red Hat
Sigh, this is mostly my fault for not giving commit cd82d82cbc04 ("drm/dp_mst: Add branch bandwidth validation to MST atomic check") enough scrutiny during review. The way we're checking bandwidth limitations here is mostly wrong.
First things first, we need to follow the locking conventions for MST. Whenever traversing downwards (upwards is always safe) in the topology, we need to hold &mgr->lock to prevent the topology from changing under us. We don't currently do that when performing bandwidth limit checks.
Next we need to figure out the actual PBN limit for the primary MSTB. Here we actually want to use the highest available_pbn value we can find on each level of the topology, then make sure that the combined sum of allocated PBN on each port of the branch device doesn't exceed that amount. Currently, we just loop through each level of the topology and use the last non-zero PBN we find.
Once we've done that, we then want to traverse down each branch device we find in the topology with at least one downstream port that has PBN allocated in our atomic state, and repeat the whole process on each level of the topology as we travel down. While doing this, we need to take care to avoid attempting to traverse down end devices. We don't currently do this, although I'm not actually sure whether or not this broke anything before.
Since there's a bit too many issues here to try to fix one by one, and the drm_dp_mst_atomic_check_bw_limit() code is not entirely clear on all of these pain points anyway, let's just take the easy way out and rewrite the whole function. Additionally, we also add a kernel warning if we find that any ports we performed bandwidth limit checks on didn't actually have available_pbn populated - as this is always a bug in the MST helpers.
This should fix regressions seen on nouveau, i915 and amdgpu where we erroneously reject atomic states that should fit within bandwidth limitations.
Signed-off-by: Lyude Paul lyude@redhat.com Fixes: cd82d82cbc04 ("drm/dp_mst: Add branch bandwidth validation to MST atomic check") Cc: Mikita Lipski mikita.lipski@amd.com Cc: Alex Deucher alexander.deucher@amd.com Cc: Sean Paul seanpaul@google.com Cc: Hans de Goede hdegoede@redhat.com --- drivers/gpu/drm/drm_dp_mst_topology.c | 101 ++++++++++++++++++++------ 1 file changed, 78 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index 7b0ff0cff954..87dc7c92d339 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -4853,41 +4853,90 @@ static bool drm_dp_mst_port_downstream_of_branch(struct drm_dp_mst_port *port, return false; }
-static inline -int drm_dp_mst_atomic_check_bw_limit(struct drm_dp_mst_branch *branch, - struct drm_dp_mst_topology_state *mst_state) +static int +drm_dp_mst_atomic_check_bw_limit(struct drm_dp_mst_branch *branch, + struct drm_dp_mst_topology_state *mst_state) { struct drm_dp_mst_port *port; struct drm_dp_vcpi_allocation *vcpi; - int pbn_limit = 0, pbn_used = 0; + int pbn_limit = 0, pbn_used = 0, ret;
- list_for_each_entry(port, &branch->ports, next) { - if (port->mstb) - if (drm_dp_mst_atomic_check_bw_limit(port->mstb, mst_state)) - return -ENOSPC; + if (branch->port_parent) + DRM_DEBUG_ATOMIC("[MSTB:%p] [MST PORT:%p] checking [MSTB:%p]\n", + branch->port_parent->parent, + branch->port_parent, branch); + else + DRM_DEBUG_ATOMIC("Checking [MSTB:%p]\n", branch);
- if (port->available_pbn > 0) + list_for_each_entry(port, &branch->ports, next) { + /* Since each port shares a link, the highest PBN we find + * should be assumed to be the limit for this branch device + */ + if (pbn_limit < port->available_pbn) pbn_limit = port->available_pbn; - } - DRM_DEBUG_ATOMIC("[MST BRANCH:%p] branch has %d PBN available\n", - branch, pbn_limit);
- list_for_each_entry(vcpi, &mst_state->vcpis, next) { - if (!vcpi->pbn) + if (port->pdt == DP_PEER_DEVICE_NONE) continue;
- if (drm_dp_mst_port_downstream_of_branch(vcpi->port, branch)) - pbn_used += vcpi->pbn; + if (drm_dp_mst_is_end_device(port->pdt, port->mcs)) { + list_for_each_entry(vcpi, &mst_state->vcpis, next) { + if (vcpi->port != port) + continue; + if (!vcpi->pbn) + break; + + /* This should never happen, as it means we + * tried to set a mode before querying the + * available_pbn + */ + if (WARN_ON(!port->available_pbn)) + return -EINVAL; + + if (vcpi->pbn > port->available_pbn) { + DRM_DEBUG_ATOMIC("[MSTB:%p] [MST PORT:%p] %d exceeds available PBN of %d\n", + branch, port, + vcpi->pbn, + port->available_pbn); + return -ENOSPC; + } + + DRM_DEBUG_ATOMIC("[MSTB:%p] [MST PORT:%p] using %d PBN\n", + branch, port, vcpi->pbn); + pbn_used += vcpi->pbn; + break; + } + } else { + list_for_each_entry(vcpi, &mst_state->vcpis, next) { + if (!vcpi->pbn || + !drm_dp_mst_port_downstream_of_branch(vcpi->port, + port->mstb)) + continue; + + ret = drm_dp_mst_atomic_check_bw_limit(port->mstb, + mst_state); + if (ret < 0) + return ret; + + pbn_used += ret; + break; + } + } } - DRM_DEBUG_ATOMIC("[MST BRANCH:%p] branch used %d PBN\n", - branch, pbn_used); + if (!pbn_used) + return 0; + + DRM_DEBUG_ATOMIC("[MSTB:%p] has total available PBN of %d\n", + branch, pbn_limit);
if (pbn_used > pbn_limit) { - DRM_DEBUG_ATOMIC("[MST BRANCH:%p] No available bandwidth\n", - branch); + DRM_DEBUG_ATOMIC("[MSTB:%p] Not enough bandwidth (need: %d)\n", + branch, pbn_used); return -ENOSPC; } - return 0; + + DRM_DEBUG_ATOMIC("[MSTB:%p] using %d PBN\n", branch, pbn_used); + + return pbn_used; }
static inline int @@ -5085,9 +5134,15 @@ int drm_dp_mst_atomic_check(struct drm_atomic_state *state) ret = drm_dp_mst_atomic_check_vcpi_alloc_limit(mgr, mst_state); if (ret) break; - ret = drm_dp_mst_atomic_check_bw_limit(mgr->mst_primary, mst_state); - if (ret) + + mutex_lock(&mgr->lock); + ret = drm_dp_mst_atomic_check_bw_limit(mgr->mst_primary, + mst_state); + mutex_unlock(&mgr->lock); + if (ret < 0) break; + else + ret = 0; }
return ret;
Hi Lyude,
On 3/4/20 11:36 PM, Lyude Paul wrote:
AMD's patch series for adding DSC support to the MST helpers unfortunately introduced a few regressions into the kernel that I didn't get around to fixing until just now. I would have reverted the changes earlier, but seeing as that would have reverted all of amd's DSC support
- everything that was done on top of that I realllllly wanted to avoid
doing that.
Anyway, this should fix everything as far as I can tell. Note that I don't have any DSC displays locally yet, so if someone from AMD could sanity check this I would appreciate it ♥.
Thank you for trying to fix this, unfortunately for me this is not fixed, with this series. My setup:
5.6-rc4 + your 3 patches (+ some unrelated patches outside of drm)
-Lenovo x1 7th gen + Lenovo TB3 dock gen 2 + 2 external 1920x1080@60 monitors connected to the 2 HDMI interfaces on the dock -System booted with the LID closed, so that the firmware/BIOS has already initialized both monitors when the kernel boots
This should be fairly easy to reproduce on a similar setup, other users are seeing similar problems when connecting more then 1 monitor to DP-MST docks, see e.g. :
https://bugzilla.redhat.com/show_bug.cgi?id=1809681 https://bugzilla.redhat.com/show_bug.cgi?id=1810070
Let me know if you want me to collect some drm.debug logs, I guess if you do, you want me to use drm.debug=0x114 ?
Regards,
Hans
Cc: Mikita Lipski mikita.lipski@amd.com Cc: Alex Deucher alexander.deucher@amd.com Cc: Sean Paul seanpaul@google.com Cc: Hans de Goede hdegoede@redhat.com
Lyude Paul (3): drm/dp_mst: Rename drm_dp_mst_is_dp_mst_end_device() to be less redundant drm/dp_mst: Don't show connectors as connected before probing available PBN drm/dp_mst: Rewrite and fix bandwidth limit checks
drivers/gpu/drm/drm_dp_mst_topology.c | 124 ++++++++++++++++++++------ 1 file changed, 96 insertions(+), 28 deletions(-)
dri-devel@lists.freedesktop.org
-
Hans de Goede -
Lyude Paul -
Ville Syrjälä