回覆: [PATCH] drm/dp_mst: clear time slots for ports invalid
Lin, Wayne
Wayne.Lin at amd.com
Mon Jan 6 08:17:35 UTC 2020
[AMD Public Use]
> -----原始郵件-----
> 寄件者: Lyude Paul <lyude at redhat.com>
> 寄件日期: Saturday, January 4, 2020 7:34 AM
> 收件者: Lin, Wayne <Wayne.Lin at amd.com>; dri-
> devel at lists.freedesktop.org; amd-gfx at lists.freedesktop.org
> 副本: 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
> 主旨: 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 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%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 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) {
>
> 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 at redhat.com>
>
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
More information about the amd-gfx
mailing list