[PATCH] drm/dp_mst: clear time slots for ports invalid

[Lin, Wayne]

> -----Original Message-----
> From: Lyude Paul <lyude at redhat.com>
> Sent: Saturday, December 21, 2019 8:12 AM
> To: Lin, Wayne <Wayne.Lin at amd.com>; dri-devel at lists.freedesktop.org;
> amd-gfx at lists.freedesktop.org
> Cc: Kazlauskas, Nicholas <Nicholas.Kazlauskas at amd.com>; Wentland, Harry
> <Harry.Wentland at amd.com>; Zuo, Jerry <Jerry.Zuo at amd.com>;
> stable at vger.kernel.org
> Subject: Re: [PATCH] drm/dp_mst: clear time slots for ports invalid
> 
> Mhh-I think I understand the problem you're trying to solve here but I think this
> solution might be a bit overkill. When I did the rework of topology references
> for ports, I made it so that we can guarantee memory access to a port without
> it needing to be a valid part of the topology. As well, all parents of the port are
> guaranteed to be accessible for as long as the child is. Take a look at:
> 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2F01.org%
> 2Flinuxgraphics%2Fgfx-docs%2Fdrm%2Fgpu%2Fdrm-kms-helpers.html%23refco
> unt-relationships-in-a-topology&data=02%7C01%7Cwayne.lin%40amd.co
> m%7C722655b546c049dc081908d785aa6758%7C3dd8961fe4884e608e11a82d
> 994e183d%7C0%7C0%7C637124839257213115&sdata=Ctha3ja8kleeFOp
> PpA7EwDV1is81RAMsjqd1P6463ak%3D&reserved=0
> 
> It's also worth noting that because of this there's a lot of
> get_port_validated()/put_port_validated() calls in the MST helpers that are
> now bogus and need to be removed once I get a chance. For new code we
> should limit the use of topology references to sections of code where we need
> a guarantee that resources on the port/branch (such as a drm connector, dp
> aux port, etc.) won't go away for as long as we need to use them.
> 
> Do you think we could change this patch so instead of removing it from the
> proposed payloads on the CONNECTION_STATUS_NOTIFY, we keep the port's
> memory allocation around until it's been removed from the proposed payloads
> table and clean it up there on the next payload update?
> 
Really appreciate for your time and comments in detail.

In this patch, I wanted to just set the proposed_vcpi->num_slots to 0 for those
ports which are no longer in the topology due to there is no need to allocate time
slots for these port. And expect those vcpi will be updated during next update of 
payload ID table by drm_dp_update_payload_part1(). 

I tried to use drm_dp_mst_topology_get_port_validated() as a helper to 
decide whether a port is in the topology or not. Use this function to iterate over
all ports that all proposed_vcpi[] drive to. If one port is not in the topology, set the
num_slots of the proposed_vcpi for this port to 0. With num_slots as 0, these 
proposed_vcpi will be clean up in next payload table update by 
drm_dp_update_payload_part1(). If a port is still in the topology, then release
the reference count which was acquired previously from
drm_dp_mst_topology_get_port_validated() and do nothing.

I didn't mean to kill invalid ports on receiving CONNECTION_STATUS_NOTIFY.
Sorry if I misuse or misunderstand something here?

> On Fri, 2019-12-06 at 16:39 +0800, Wayne Lin wrote:
> > [Why]
> > When change the connection status in a MST topology, mst device which
> > detect the event will send out CONNECTION_STATUS_NOTIFY messgae.
> >
> > e.g. src-mst-mst-sst => src-mst (unplug) mst-sst
> >
> > Currently, under the above case of unplugging device, ports which have
> > been allocated payloads and are no longer in the topology still occupy
> > time slots and recorded in proposed_vcpi[] of topology manager.
> >
> > If we don't clean up the proposed_vcpi[], when code flow goes to try
> > to update payload table by calling drm_dp_update_payload_part1(), we
> > will fail at checking port validation due to there are ports with
> > proposed time slots but no longer in the mst topology. As the result
> > of that, we will also stop updating the DPCD payload table of down stream
> port.
> >
> > [How]
> > While handling the CONNECTION_STATUS_NOTIFY message, add a detection
> > to see if the event indicates that a device is unplugged to an output port.
> > If the detection is true, then iterrate over all proposed_vcpi[] to
> > see whether a port of the proposed_vcpi[] is still in the topology or
> > not. If the port is invalid, set its num_slots to 0.
> >
> > Thereafter, when try to update payload table by calling
> > drm_dp_update_payload_part1(), we can successfully update the DPCD
> > payload table of down stream port and clear the proposed_vcpi[] to NULL.
> >
> > Signed-off-by: Wayne Lin <Wayne.Lin at amd.com>
> > Cc: stable at vger.kernel.org
> > ---
> >  drivers/gpu/drm/drm_dp_mst_topology.c | 24
> +++++++++++++++++++++++-
> >  1 file changed, 23 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index 5306c47dc820..2e236b6275c4 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -2318,7 +2318,7 @@ drm_dp_mst_handle_conn_stat(struct
> > drm_dp_mst_branch *mstb,  {
> >  	struct drm_dp_mst_topology_mgr *mgr = mstb->mgr;
> >  	struct drm_dp_mst_port *port;
> > -	int old_ddps, ret;
> > +	int old_ddps, old_input, ret, i;
> >  	u8 new_pdt;
> >  	bool dowork = false, create_connector = false;
> >
> > @@ -2349,6 +2349,7 @@ drm_dp_mst_handle_conn_stat(struct
> > drm_dp_mst_branch *mstb,
> >  	}
> >
> >  	old_ddps = port->ddps;
> > +	old_input = port->input;
> >  	port->input = conn_stat->input_port;
> >  	port->mcs = conn_stat->message_capability_status;
> >  	port->ldps = conn_stat->legacy_device_plug_status;
> > @@ -2373,6 +2374,27 @@ drm_dp_mst_handle_conn_stat(struct
> > drm_dp_mst_branch *mstb,
> >  		dowork = false;
> >  	}
> >
> > +	if (!old_input && old_ddps != port->ddps && !port->ddps) {
> > +		for (i = 0; i < mgr->max_payloads; i++) {
> > +			struct drm_dp_vcpi *vcpi = mgr->proposed_vcpis[i];
> > +			struct drm_dp_mst_port *port_validated;
> > +
> > +			if (vcpi) {
> > +				port_validated =
> > +					container_of(vcpi, struct
> > drm_dp_mst_port, vcpi);
> > +				port_validated =
> > +					drm_dp_mst_topology_get_port_validated
> > (mgr, port_validated);
> > +				if (!port_validated) {
> > +					mutex_lock(&mgr->payload_lock);
> > +					vcpi->num_slots = 0;
> > +					mutex_unlock(&mgr->payload_lock);
> > +				} else {
> > +					drm_dp_mst_topology_put_port(port_vali
> > dated);
> > +				}
> > +			}
> > +		}
> > +	}
> > +
> >  	if (port->connector)
> >  		drm_modeset_unlock(&mgr->base.lock);
> >  	else if (create_connector)
> --
> Cheers,
> 	Lyude Paul
--
Best regards,
Wayne Lin