Re: [PATCH v2 25/27] drm/dp_mst: Add basic topology reprobing when resuming
On Fri, 2019-09-27 at 09:52 -0400, Sean Paul wrote:
> On Tue, Sep 03, 2019 at 04:46:03PM -0400, Lyude Paul wrote:
> > Finally! For a very long time, our MST helpers have had one very
> > annoying issue: They don't know how to reprobe the topology state when
> > coming out of suspend. This means that if a user has a machine connected
> > to an MST topology and decides to suspend their machine, we lose all
> > topology changes that happened during that period. That can be a big
> > problem if the machine was connected to a different topology on the same
> > port before resuming, as we won't bother reprobing any of the ports and
> > likely cause the user's monitors not to come back up as expected.
> >
> > So, we start fixing this by teaching our MST helpers how to reprobe the
> > link addresses of each connected topology when resuming. As it turns
> > out, the behavior that we want here is identical to the behavior we want
> > when initially probing a newly connected MST topology, with a couple of
> > important differences:
> >
> > - We need to be more careful about handling the potential races between
> > events from the MST hub that could change the topology state as we're
> > performing the link address reprobe
> > - We need to be more careful about handling unlikely state changes on
> > ports - such as an input port turning into an output port, something
> > that would be far more likely to happen in situations like the MST hub
> > we're connected to being changed while we're suspend
> >
> > Both of which have been solved by previous commits. That leaves one
> > requirement:
> >
> > - We need to prune any MST ports in our in-memory topology state that
> > were present when suspending, but have not appeared in the post-resume
> > link address response from their parent branch device
> >
> > Which we can now handle in this commit by modifying
> > drm_dp_send_link_address(). We then introduce suspend/resume reprobing
> > by introducing drm_dp_mst_topology_mgr_invalidate_mstb(), which we call
> > in drm_dp_mst_topology_mgr_suspend() to traverse the in-memory topology
> > state to indicate that each mstb needs it's link address resent and PBN
> > resources reprobed.
> >
> > On resume, we start back up >work and have it reprobe the topology
> > in the same way we would on a hotplug, removing any leftover ports that
> > no longer appear in the topology state.
> >
> > Cc: Juston Li
> > Cc: Imre Deak
> > Cc: Ville Syrjälä
> > Cc: Harry Wentland
> > Cc: Daniel Vetter
> > Signed-off-by: Lyude Paul
> > ---
> > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +-
> > drivers/gpu/drm/drm_dp_mst_topology.c | 138 +-
> > drivers/gpu/drm/i915/display/intel_dp.c | 3 +-
> > drivers/gpu/drm/nouveau/dispnv50/disp.c | 6 +-
> > include/drm/drm_dp_mst_helper.h | 3 +-
> > 5 files changed, 112 insertions(+), 40 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > index 4d3c8bff77da..27ee3e045b86 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -973,7 +973,7 @@ static void s3_handle_mst(struct drm_device *dev, bool
> > suspend)
> > if (suspend) {
> > drm_dp_mst_topology_mgr_suspend(mgr);
> > } else {
> > - ret = drm_dp_mst_topology_mgr_resume(mgr);
> > + ret = drm_dp_mst_topology_mgr_resume(mgr, true);
> > if (ret < 0) {
> > drm_dp_mst_topology_mgr_set_mst(mgr, false);
> > need_hotplug = true;
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index e407aba1fbd2..2fe24e366925 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -2020,6 +2020,14 @@ drm_dp_mst_handle_link_address_port(struct
> > drm_dp_mst_branch *mstb,
> > goto fail_unlock;
> > }
> >
> > + /*
> > +* If this port wasn't just created, then we're reprobing because
> > +* we're coming out of suspend. In this case, always resend the link
> > +* address if there's an MSTB on this port
> > +*/
> > + if (!created && port->pdt == DP_PEER_DEVICE_MST_BRANCHING)
> > + send_link_addr = true;
> > +
> > if (send_link_addr) {
> > mutex_lock(>lock);
> > if (port->mstb) {
> > @@ -2530,7 +2538,8 @@ static void drm_dp_send_link_address(struct
> > drm_dp_mst_topology_mgr *mgr,
> > {
> > struct drm_dp_sideband_msg_tx *txmsg;
> > struct drm_dp_link_address_ack_reply *reply;
> > - int i, len, ret;
> > + struct drm_dp_mst_port *port, *tmp;
> > + int i, len, ret, port_mask = 0;
> >
> > txmsg = kzalloc(sizeof(*txmsg), GFP_KERNEL);
> > if (!txmsg)
> > @@ -2560,9 +2569,28
Re: [PATCH v2 25/27] drm/dp_mst: Add basic topology reprobing when resuming
On Tue, Sep 03, 2019 at 04:46:03PM -0400, Lyude Paul wrote:
> Finally! For a very long time, our MST helpers have had one very
> annoying issue: They don't know how to reprobe the topology state when
> coming out of suspend. This means that if a user has a machine connected
> to an MST topology and decides to suspend their machine, we lose all
> topology changes that happened during that period. That can be a big
> problem if the machine was connected to a different topology on the same
> port before resuming, as we won't bother reprobing any of the ports and
> likely cause the user's monitors not to come back up as expected.
>
> So, we start fixing this by teaching our MST helpers how to reprobe the
> link addresses of each connected topology when resuming. As it turns
> out, the behavior that we want here is identical to the behavior we want
> when initially probing a newly connected MST topology, with a couple of
> important differences:
>
> - We need to be more careful about handling the potential races between
> events from the MST hub that could change the topology state as we're
> performing the link address reprobe
> - We need to be more careful about handling unlikely state changes on
> ports - such as an input port turning into an output port, something
> that would be far more likely to happen in situations like the MST hub
> we're connected to being changed while we're suspend
>
> Both of which have been solved by previous commits. That leaves one
> requirement:
>
> - We need to prune any MST ports in our in-memory topology state that
> were present when suspending, but have not appeared in the post-resume
> link address response from their parent branch device
>
> Which we can now handle in this commit by modifying
> drm_dp_send_link_address(). We then introduce suspend/resume reprobing
> by introducing drm_dp_mst_topology_mgr_invalidate_mstb(), which we call
> in drm_dp_mst_topology_mgr_suspend() to traverse the in-memory topology
> state to indicate that each mstb needs it's link address resent and PBN
> resources reprobed.
>
> On resume, we start back up >work and have it reprobe the topology
> in the same way we would on a hotplug, removing any leftover ports that
> no longer appear in the topology state.
>
> Cc: Juston Li
> Cc: Imre Deak
> Cc: Ville Syrjälä
> Cc: Harry Wentland
> Cc: Daniel Vetter
> Signed-off-by: Lyude Paul
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +-
> drivers/gpu/drm/drm_dp_mst_topology.c | 138 +-
> drivers/gpu/drm/i915/display/intel_dp.c | 3 +-
> drivers/gpu/drm/nouveau/dispnv50/disp.c | 6 +-
> include/drm/drm_dp_mst_helper.h | 3 +-
> 5 files changed, 112 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 4d3c8bff77da..27ee3e045b86 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -973,7 +973,7 @@ static void s3_handle_mst(struct drm_device *dev, bool
> suspend)
> if (suspend) {
> drm_dp_mst_topology_mgr_suspend(mgr);
> } else {
> - ret = drm_dp_mst_topology_mgr_resume(mgr);
> + ret = drm_dp_mst_topology_mgr_resume(mgr, true);
> if (ret < 0) {
> drm_dp_mst_topology_mgr_set_mst(mgr, false);
> need_hotplug = true;
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index e407aba1fbd2..2fe24e366925 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -2020,6 +2020,14 @@ drm_dp_mst_handle_link_address_port(struct
> drm_dp_mst_branch *mstb,
> goto fail_unlock;
> }
>
> + /*
> + * If this port wasn't just created, then we're reprobing because
> + * we're coming out of suspend. In this case, always resend the link
> + * address if there's an MSTB on this port
> + */
> + if (!created && port->pdt == DP_PEER_DEVICE_MST_BRANCHING)
> + send_link_addr = true;
> +
> if (send_link_addr) {
> mutex_lock(>lock);
> if (port->mstb) {
> @@ -2530,7 +2538,8 @@ static void drm_dp_send_link_address(struct
> drm_dp_mst_topology_mgr *mgr,
> {
> struct drm_dp_sideband_msg_tx *txmsg;
> struct drm_dp_link_address_ack_reply *reply;
> - int i, len, ret;
> + struct drm_dp_mst_port *port, *tmp;
> + int i, len, ret, port_mask = 0;
>
> txmsg = kzalloc(sizeof(*txmsg), GFP_KERNEL);
> if (!txmsg)
> @@ -2560,9 +2569,28 @@ static void drm_dp_send_link_address(struct
> drm_dp_mst_topology_mgr *mgr,
>
> drm_dp_check_mstb_guid(mstb, reply->guid);
>
> - for (i = 0; i < reply->nports; i++)
> + for (i =
[PATCH v2 25/27] drm/dp_mst: Add basic topology reprobing when resuming
Finally! For a very long time, our MST helpers have had one very
annoying issue: They don't know how to reprobe the topology state when
coming out of suspend. This means that if a user has a machine connected
to an MST topology and decides to suspend their machine, we lose all
topology changes that happened during that period. That can be a big
problem if the machine was connected to a different topology on the same
port before resuming, as we won't bother reprobing any of the ports and
likely cause the user's monitors not to come back up as expected.
So, we start fixing this by teaching our MST helpers how to reprobe the
link addresses of each connected topology when resuming. As it turns
out, the behavior that we want here is identical to the behavior we want
when initially probing a newly connected MST topology, with a couple of
important differences:
- We need to be more careful about handling the potential races between
events from the MST hub that could change the topology state as we're
performing the link address reprobe
- We need to be more careful about handling unlikely state changes on
ports - such as an input port turning into an output port, something
that would be far more likely to happen in situations like the MST hub
we're connected to being changed while we're suspend
Both of which have been solved by previous commits. That leaves one
requirement:
- We need to prune any MST ports in our in-memory topology state that
were present when suspending, but have not appeared in the post-resume
link address response from their parent branch device
Which we can now handle in this commit by modifying
drm_dp_send_link_address(). We then introduce suspend/resume reprobing
by introducing drm_dp_mst_topology_mgr_invalidate_mstb(), which we call
in drm_dp_mst_topology_mgr_suspend() to traverse the in-memory topology
state to indicate that each mstb needs it's link address resent and PBN
resources reprobed.
On resume, we start back up >work and have it reprobe the topology
in the same way we would on a hotplug, removing any leftover ports that
no longer appear in the topology state.
Cc: Juston Li
Cc: Imre Deak
Cc: Ville Syrjälä
Cc: Harry Wentland
Cc: Daniel Vetter
Signed-off-by: Lyude Paul
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +-
drivers/gpu/drm/drm_dp_mst_topology.c | 138 +-
drivers/gpu/drm/i915/display/intel_dp.c | 3 +-
drivers/gpu/drm/nouveau/dispnv50/disp.c | 6 +-
include/drm/drm_dp_mst_helper.h | 3 +-
5 files changed, 112 insertions(+), 40 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 4d3c8bff77da..27ee3e045b86 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -973,7 +973,7 @@ static void s3_handle_mst(struct drm_device *dev, bool
suspend)
if (suspend) {
drm_dp_mst_topology_mgr_suspend(mgr);
} else {
- ret = drm_dp_mst_topology_mgr_resume(mgr);
+ ret = drm_dp_mst_topology_mgr_resume(mgr, true);
if (ret < 0) {
drm_dp_mst_topology_mgr_set_mst(mgr, false);
need_hotplug = true;
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
b/drivers/gpu/drm/drm_dp_mst_topology.c
index e407aba1fbd2..2fe24e366925 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -2020,6 +2020,14 @@ drm_dp_mst_handle_link_address_port(struct
drm_dp_mst_branch *mstb,
goto fail_unlock;
}
+ /*
+* If this port wasn't just created, then we're reprobing because
+* we're coming out of suspend. In this case, always resend the link
+* address if there's an MSTB on this port
+*/
+ if (!created && port->pdt == DP_PEER_DEVICE_MST_BRANCHING)
+ send_link_addr = true;
+
if (send_link_addr) {
mutex_lock(>lock);
if (port->mstb) {
@@ -2530,7 +2538,8 @@ static void drm_dp_send_link_address(struct
drm_dp_mst_topology_mgr *mgr,
{
struct drm_dp_sideband_msg_tx *txmsg;
struct drm_dp_link_address_ack_reply *reply;
- int i, len, ret;
+ struct drm_dp_mst_port *port, *tmp;
+ int i, len, ret, port_mask = 0;
txmsg = kzalloc(sizeof(*txmsg), GFP_KERNEL);
if (!txmsg)
@@ -2560,9 +2569,28 @@ static void drm_dp_send_link_address(struct
drm_dp_mst_topology_mgr *mgr,
drm_dp_check_mstb_guid(mstb, reply->guid);
- for (i = 0; i < reply->nports; i++)
+ for (i = 0; i < reply->nports; i++) {
+ port_mask |= BIT(reply->ports[i].port_number);
drm_dp_mst_handle_link_address_port(mstb, mgr->dev,
