Nouveau - Mar 2020 - [PATCH 2/3] drm/dp_mst: Don't show connectors as connected before probing available PBN

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 at redhat.com>
> > > Fixes: cd82d82cbc04 ("drm/dp_mst: Add branch bandwidth
validation to MST
> > > atomic check")
> > > Cc: Mikita Lipski <mikita.lipski at amd.com>
> > > Cc: Alex Deucher <alexander.deucher at amd.com>
> > > Cc: Sean Paul <seanpaul at google.com>
> > > Cc: Hans de Goede <hdegoede at 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 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 at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> -- 
> Cheers,
> 	Lyude Paul (she/her)
> 	Associate Software Engineer at Red Hat
-- 
Ville Syrj?l?
Intel

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 at redhat.com>
> > > > Fixes: cd82d82cbc04 ("drm/dp_mst: Add branch bandwidth
validation to
> > > > MST
> > > > atomic check")
> > > > Cc: Mikita Lipski <mikita.lipski at amd.com>
> > > > Cc: Alex Deucher <alexander.deucher at amd.com>
> > > > Cc: Sean Paul <seanpaul at google.com>
> > > > Cc: Hans de Goede <hdegoede at 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 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 at lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > -- 
> > Cheers,
> > 	Lyude Paul (she/her)
> > 	Associate Software Engineer at Red Hat
-- 
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 at redhat.com>
> > > > > Fixes: cd82d82cbc04 ("drm/dp_mst: Add branch
bandwidth validation to
> > > > > MST
> > > > > atomic check")
> > > > > Cc: Mikita Lipski <mikita.lipski at amd.com>
> > > > > Cc: Alex Deucher <alexander.deucher at amd.com>
> > > > > Cc: Sean Paul <seanpaul at google.com>
> > > > > Cc: Hans de Goede <hdegoede at 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
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 at lists.freedesktop.org
> > > > >
https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > -- 
> > > Cheers,
> > > 	Lyude Paul (she/her)
> > > 	Associate Software Engineer at Red Hat
-- 
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 at redhat.com>
> > > > Fixes: cd82d82cbc04 ("drm/dp_mst: Add branch bandwidth
validation to
> > > > MST
> > > > atomic check")
> > > > Cc: Mikita Lipski <mikita.lipski at amd.com>
> > > > Cc: Alex Deucher <alexander.deucher at amd.com>
> > > > Cc: Sean Paul <seanpaul at google.com>
> > > > Cc: Hans de Goede <hdegoede at 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 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 at lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > -- 
> > Cheers,
> > 	Lyude Paul (she/her)
> > 	Associate Software Engineer at Red Hat
-- 
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 at redhat.com>
> > > > > Fixes: cd82d82cbc04 ("drm/dp_mst: Add branch
bandwidth validation to
> > > > > MST
> > > > > atomic check")
> > > > > Cc: Mikita Lipski <mikita.lipski at amd.com>
> > > > > Cc: Alex Deucher <alexander.deucher at amd.com>
> > > > > Cc: Sean Paul <seanpaul at google.com>
> > > > > Cc: Hans de Goede <hdegoede at 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
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 at lists.freedesktop.org
> > > > >
https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > -- 
> > > Cheers,
> > > 	Lyude Paul (she/her)
> > > 	Associate Software Engineer at Red Hat
-- 
Cheers,
	Lyude Paul (she/her)
	Associate Software Engineer at Red Hat