Message-ID: <1d67c05ace278fd0f7de31a560ac5ebafdb04eca.camel@redhat.com>
Date:   Thu, 05 Mar 2020 14:44:43 -0500
From:   Lyude Paul <lyude@...hat.com>
To:     Ville Syrjälä <ville.syrjala@...ux.intel.com>
Cc:     dri-devel@...ts.freedesktop.org, amd-gfx@...ts.freedesktop.org,
        nouveau@...ts.freedesktop.org, Sean Paul <seanpaul@...gle.com>,
        David Airlie <airlied@...ux.ie>, linux-kernel@...r.kernel.org,
        Hans de Goede <hdegoede@...hat.com>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        Alex Deucher <alexander.deucher@....com>,
        Mikita Lipski <mikita.lipski@....com>
Subject: Re: [PATCH 2/3] drm/dp_mst: Don't show connectors as connected
 before probing available PBN

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@...hat.com>
> > > > > Fixes: cd82d82cbc04 ("drm/dp_mst: Add branch bandwidth validation to
> > > > > MST
> > > > > atomic check")
> > > > > Cc: Mikita Lipski <mikita.lipski@....com>
> > > > > Cc: Alex Deucher <alexander.deucher@....com>
> > > > > Cc: Sean Paul <seanpaul@...gle.com>
> > > > > Cc: Hans de Goede <hdegoede@...hat.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@...ts.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

Powered by blists - more mailing lists