| Message ID | 20191206083937.9411-1-Wayne.Lin@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/dp_mst: clear time slots for ports invalid | expand |
Hi,
[This is an automated email]
This commit has been processed because it contains a -stable tag.
The stable tag indicates that it's relevant for the following trees: all
The bot has tested the following trees: v5.4.2, v5.3.15, v4.19.88, v4.14.158, v4.9.206, v4.4.206.
v5.4.2: Failed to apply! Possible dependencies:
14692a3637d4 ("drm/dp_mst: Add probe_lock")
37dfdc55ffeb ("drm/dp_mst: Cleanup drm_dp_send_link_address() a bit")
3f9b3f02dda5 ("drm/dp_mst: Protect drm_dp_mst_port members with locking")
50094b5dcd32 ("drm/dp_mst: Destroy topology_mgr mutexes")
5950f0b797fc ("drm/dp_mst: Move link address dumping into a function")
60f9ae9d0d3d ("drm/dp_mst: Remove huge conditional in drm_dp_mst_handle_up_req()")
7cb12d48314e ("drm/dp_mst: Destroy MSTBs asynchronously")
9408cc94eb04 ("drm/dp_mst: Handle UP requests asynchronously")
a29d881875fc ("drm/dp_mst: Refactor drm_dp_mst_handle_up_req()")
c485e2c97dae ("drm/dp_mst: Refactor pdt setup/teardown, add more locking")
caf81ec6cd72 ("drm: Destroy the correct mutex name in drm_dp_mst_topology_mgr_destroy")
dad7d84f8835 ("drm/dp_mst: Don't forget to update port->input in drm_dp_mst_handle_conn_stat()")
e2839ff692c6 ("drm/dp_mst: Rename drm_dp_add_port and drm_dp_update_port")
v5.3.15: Failed to apply! Possible dependencies:
14692a3637d4 ("drm/dp_mst: Add probe_lock")
37dfdc55ffeb ("drm/dp_mst: Cleanup drm_dp_send_link_address() a bit")
3f9b3f02dda5 ("drm/dp_mst: Protect drm_dp_mst_port members with locking")
50094b5dcd32 ("drm/dp_mst: Destroy topology_mgr mutexes")
562836a269e3 ("drm/dp_mst: Enable registration of AUX devices for MST ports")
5950f0b797fc ("drm/dp_mst: Move link address dumping into a function")
60f9ae9d0d3d ("drm/dp_mst: Remove huge conditional in drm_dp_mst_handle_up_req()")
7cb12d48314e ("drm/dp_mst: Destroy MSTBs asynchronously")
9408cc94eb04 ("drm/dp_mst: Handle UP requests asynchronously")
a29d881875fc ("drm/dp_mst: Refactor drm_dp_mst_handle_up_req()")
c485e2c97dae ("drm/dp_mst: Refactor pdt setup/teardown, add more locking")
caf81ec6cd72 ("drm: Destroy the correct mutex name in drm_dp_mst_topology_mgr_destroy")
dad7d84f8835 ("drm/dp_mst: Don't forget to update port->input in drm_dp_mst_handle_conn_stat()")
e2839ff692c6 ("drm/dp_mst: Rename drm_dp_add_port and drm_dp_update_port")
v4.19.88: Failed to apply! Possible dependencies:
1e55a53a28d3 ("drm: Trivial comment grammar cleanups")
706246c761dd ("drm/dp_mst: Refactor drm_dp_update_payload_part1()")
72fdb40c1a4b ("drm: extract drm_atomic_uapi.c")
7f4de521001f ("drm/atomic: Add __drm_atomic_helper_plane_reset")
a5ec8332d428 ("drm: Add per-plane pixel blend mode property")
c485e2c97dae ("drm/dp_mst: Refactor pdt setup/teardown, add more locking")
d0757afd00d7 ("drm/dp_mst: Rename drm_dp_mst_get_validated_(port|mstb)_ref and friends")
d86552efe10a ("drm/atomic: trim driver interface/docs")
dad7d84f8835 ("drm/dp_mst: Don't forget to update port->input in drm_dp_mst_handle_conn_stat()")
de9f8eea5a44 ("drm/atomic_helper: Stop modesets on unregistered connectors harder")
ebcc0e6b5091 ("drm/dp_mst: Introduce new refcounting scheme for mstbs and ports")
fc63668656bd ("drm/dp_mst: Remove bogus conditional in drm_dp_update_payload_part1()")
v4.14.158: Failed to apply! Possible dependencies:
0bb9c2b27f5e ("drm/dp/mst: Sideband message transaction to power up/down nodes")
163bcc2c74a2 ("drm/atomic: Move drm_crtc_commit to drm_crtc_state, v4.")
179c02fe90a4 ("drm/tve200: Add new driver for TVE200")
1e55a53a28d3 ("drm: Trivial comment grammar cleanups")
21a01abbe32a ("drm/atomic: Fix freeing connector/plane state too early by tracking commits, v3.")
22a07038c0ea ("drm: NULL pointer dereference [null-pointer-deref] (CWE 476) problem")
24557865c8b1 ("drm: Add Content Protection property")
2ed077e467ee ("drm: Add drm_object lease infrastructure [v5]")
34ca26a98ad6 ("drm/atomic_helper: Allow DPMS On<->Off changes for unregistered connectors")
66660d4cf21b ("drm: add connector info/property for non-desktop displays [v2]")
6d544fd6f4e1 ("drm/doc: Put all driver docs into a separate chapter")
706246c761dd ("drm/dp_mst: Refactor drm_dp_update_payload_part1()")
72fdb40c1a4b ("drm: extract drm_atomic_uapi.c")
8d70f395e6cb ("drm: Add support for a panel-orientation connector property, v6")
935774cd71fe ("drm: Add writeback connector type")
c485e2c97dae ("drm/dp_mst: Refactor pdt setup/teardown, add more locking")
c76f0f7cb546 ("drm: Begin an API for in-kernel clients")
d0757afd00d7 ("drm/dp_mst: Rename drm_dp_mst_get_validated_(port|mstb)_ref and friends")
dad7d84f8835 ("drm/dp_mst: Don't forget to update port->input in drm_dp_mst_handle_conn_stat()")
de9f8eea5a44 ("drm/atomic_helper: Stop modesets on unregistered connectors harder")
e96550956fbc ("drm/atomic_helper: Disallow new modesets on unregistered connectors")
ebcc0e6b5091 ("drm/dp_mst: Introduce new refcounting scheme for mstbs and ports")
fc63668656bd ("drm/dp_mst: Remove bogus conditional in drm_dp_update_payload_part1()")
v4.9.206: Failed to apply! Possible dependencies:
0bb9c2b27f5e ("drm/dp/mst: Sideband message transaction to power up/down nodes")
1cec20f0ea0e ("dma-buf: Restart reservation_object_wait_timeout_rcu() after writes")
3f3353b7e121 ("drm/dp: Introduce MST topology state to track available link bandwidth")
6806cdf9aa1c ("drm/kms-helpers: Use recommened kerneldoc for struct member refs")
78010cd9736e ("dma-buf/fence: add an lockdep_assert_held()")
9498c19b3f53 ("drm: Move tile group code into drm_connector.c")
9a83a71ac0d5 ("drm/fences: add DOC: for explicit fencing")
beaf5af48034 ("drm/fence: add out-fences support")
c485e2c97dae ("drm/dp_mst: Refactor pdt setup/teardown, add more locking")
d0757afd00d7 ("drm/dp_mst: Rename drm_dp_mst_get_validated_(port|mstb)_ref and friends")
d807ed1c55fb ("drm: atomic: Clarify documentation around drm_atomic_crtc_needs_modeset")
dad7d84f8835 ("drm/dp_mst: Don't forget to update port->input in drm_dp_mst_handle_conn_stat()")
ea0dd85a75f1 ("drm/doc: use preferred struct reference in kernel-doc")
edb1ed1ab7d3 ("drm/dp: Add DP MST helpers to atomically find and release vcpi slots")
f54d1867005c ("dma-buf: Rename struct fence to dma_fence")
fedf54132d24 ("dma-buf: Restart reservation_object_get_fences_rcu() after writes")
v4.4.206: Failed to apply! Possible dependencies:
081e9c0f8b5f ("drm/atomic: kerneldoc for drm_atomic_crtc_needs_modeset")
092d01dae09a ("drm: Reorganize helper vtables and their docs")
0bb9c2b27f5e ("drm/dp/mst: Sideband message transaction to power up/down nodes")
132d49d728f3 ("drm/dp-mst: Missing kernel doc")
1eb83451ba55 ("drm: Pass the user drm_mode_fb_cmd2 as const to .fb_create()")
286dbb8d5d80 ("drm/atomic: Rename async parameter to nonblocking.")
373701b1fc7d ("drm: fix potential dangling else problems in for_each_ macros")
3f3353b7e121 ("drm/dp: Introduce MST topology state to track available link bandwidth")
441388a8a73f ("drm/mst: Don't ignore the MST PBN self-test result")
4c5b7f3ae53b ("drm/atomic: export drm_atomic_helper_wait_for_fences()")
5fff80bbdb6b ("drm/atomic: Allow for holes in connector state, v2.")
6806cdf9aa1c ("drm/kms-helpers: Use recommened kerneldoc for struct member refs")
69a0f89c0641 ("drm/dp/mst: constify drm_dp_mst_topology_cbs structures")
9953f41799bd ("drm: Kerneldoc for drm_mode_config_funcs")
9f2a7950e77a ("drm/atomic-helper: nonblocking commit support")
a095caa7f5ec ("drm/atomic-helper: roll out commit synchronization")
b516a9efb7af ("drm: Move LEAVE/ENTER_ATOMIC_MODESET to fbdev helpers")
be9174a482b9 ("drm/atomic-helper: use for_each_*_in_state more")
c240906d3665 ("drm/atomic-helper: Export framebuffer_changed()")
c485e2c97dae ("drm/dp_mst: Refactor pdt setup/teardown, add more locking")
d0757afd00d7 ("drm/dp_mst: Rename drm_dp_mst_get_validated_(port|mstb)_ref and friends")
d807ed1c55fb ("drm: atomic: Clarify documentation around drm_atomic_crtc_needs_modeset")
dad7d84f8835 ("drm/dp_mst: Don't forget to update port->input in drm_dp_mst_handle_conn_stat()")
edb1ed1ab7d3 ("drm/dp: Add DP MST helpers to atomically find and release vcpi slots")
ef8f9bea1368 ("dp/mst: add SDP stream support")
NOTE: The patch will not be queued to stable trees until it is upstream.
How should we proceed with this patch?
[AMD Official Use Only - Internal Distribution Only] Pinged. Hi, can someone help to review please. Thanks a lot. Regards, Wayne
Hi! I will try to review this patch today, must have gotten lost in the noise On Fri, 2019-12-20 at 01:46 +0000, Lin, Wayne wrote: > [AMD Official Use Only - Internal Distribution Only] > > Pinged. > Hi, can someone help to review please. > > Thanks a lot. > > Regards, > Wayne > > ________________________________________ > From: Wayne Lin <Wayne.Lin@amd.com> > Sent: Friday, December 6, 2019 16:39 > To: dri-devel@lists.freedesktop.org; amd-gfx@lists.freedesktop.org > Cc: Kazlauskas, Nicholas; Wentland, Harry; Zuo, Jerry; lyude@redhat.com; > stable@vger.kernel.org; Lin, Wayne > Subject: [PATCH] drm/dp_mst: clear time slots for ports invalid > > [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@amd.com> > Cc: stable@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_validat > ed(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_va > lidated); > + } > + } > + } > + } > + > if (port->connector) > drm_modeset_unlock(&mgr->base.lock); > else if (create_connector) > -- > 2.17.1 >
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://01.org/linuxgraphics/gfx-docs/drm/gpu/drm-kms-helpers.html#refcount-relationships-in-a-topology 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? 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@amd.com> > Cc: stable@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)
> -----Original Message----- > From: Lyude Paul <lyude@redhat.com> > Sent: Saturday, December 21, 2019 8:12 AM > To: Lin, Wayne <Wayne.Lin@amd.com>; dri-devel@lists.freedesktop.org; > amd-gfx@lists.freedesktop.org > Cc: Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Wentland, Harry > <Harry.Wentland@amd.com>; Zuo, Jerry <Jerry.Zuo@amd.com>; > stable@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@amd.com> > > Cc: stable@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
On Wed, 2019-12-25 at 06:45 +0000, Lin, Wayne wrote: > > -----Original Message----- > > From: Lyude Paul <lyude@redhat.com> > > Sent: Saturday, December 21, 2019 8:12 AM > > To: Lin, Wayne <Wayne.Lin@amd.com>; dri-devel@lists.freedesktop.org; > > amd-gfx@lists.freedesktop.org > > Cc: Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Wentland, Harry > > <Harry.Wentland@amd.com>; Zuo, Jerry <Jerry.Zuo@amd.com>; > > stable@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? 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@amd.com> > > > Cc: stable@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@redhat.com> > > > + 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
[AMD Public Use] > -----原始郵件----- > 寄件者: Lyude Paul <lyude@redhat.com> > 寄件日期: Saturday, January 4, 2020 7:34 AM > 收件者: Lin, Wayne <Wayne.Lin@amd.com>; dri- > devel@lists.freedesktop.org; amd-gfx@lists.freedesktop.org > 副本: Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Wentland, > Harry <Harry.Wentland@amd.com>; Zuo, Jerry <Jerry.Zuo@amd.com>; > stable@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@redhat.com> > > > Sent: Saturday, December 21, 2019 8:12 AM > > > To: Lin, Wayne <Wayne.Lin@amd.com>; dri-devel@lists.freedesktop.org; > > > amd-gfx@lists.freedesktop.org > > > Cc: Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Wentland, > > > Harry <Harry.Wentland@amd.com>; Zuo, Jerry <Jerry.Zuo@amd.com>; > > > stable@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@amd.com> > > > > Cc: stable@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@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
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_validated); + } + } + } + } + if (port->connector) drm_modeset_unlock(&mgr->base.lock); else if (create_connector)
[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@amd.com> Cc: stable@vger.kernel.org --- drivers/gpu/drm/drm_dp_mst_topology.c | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-)