[AMD Public Use] > -----原始郵件----- > 寄件者: Lyude Paul <lyude@xxxxxxxxxx> > 寄件日期: Saturday, January 4, 2020 7:34 AM > 收件者: Lin, Wayne <Wayne.Lin@xxxxxxx>; dri- > devel@xxxxxxxxxxxxxxxxxxxxx; amd-gfx@xxxxxxxxxxxxxxxxxxxxx > 副本: Kazlauskas, Nicholas <Nicholas.Kazlauskas@xxxxxxx>; Wentland, > Harry <Harry.Wentland@xxxxxxx>; Zuo, Jerry <Jerry.Zuo@xxxxxxx>; > stable@xxxxxxxxxxxxxxx > 主旨: Re: [PATCH] drm/dp_mst: clear time slots for ports invalid > > On Wed, 2019-12-25 at 06:45 +0000, Lin, Wayne wrote: > > > -----Original Message----- > > > From: Lyude Paul <lyude@xxxxxxxxxx> > > > Sent: Saturday, December 21, 2019 8:12 AM > > > To: Lin, Wayne <Wayne.Lin@xxxxxxx>; dri-devel@xxxxxxxxxxxxxxxxxxxxx; > > > amd-gfx@xxxxxxxxxxxxxxxxxxxxx > > > Cc: Kazlauskas, Nicholas <Nicholas.Kazlauskas@xxxxxxx>; Wentland, > > > Harry <Harry.Wentland@xxxxxxx>; Zuo, Jerry <Jerry.Zuo@xxxxxxx>; > > > stable@xxxxxxxxxxxxxxx > > > 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%23refc > > > o > > > unt-relationships-in-a- > topology&data=02%7C01%7Cwayne.lin%40amd.c > > > o > 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? > > Ahh, it seems I made the mistake here then because from your explanation > you're using the API exactly as intended :). All of this has me wondering if > some day we should try to get rid of the payload tracking we have and move > it into atomic. But, that's a problem for another day. > > Anyway-one small change below: > > > > > > 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@xxxxxxx> > > > > Cc: stable@xxxxxxxxxxxxxxx > > > > --- > > > > 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) { > > Let's invert this conditional to reduce the indenting here a bit if (!vcpi) > continue; > > With that change this is: > > Reviewed-by: Lyude Paul <lyude@xxxxxxxxxx> > Appreciate for your time. I will do that later. Thanks! > > > > + 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 > -- > Cheers, > Lyude Paul -- Best regards, Wayne Lin
