[Intel-gfx] [PATCH v6 3/6] drm/dp_mst: Start tracking per-port VCPI allocations
There has been a TODO waiting for quite a long time in drm_dp_mst_topology.c: /* We cannot rely on port->vcpi.num_slots to update * topology_state->avail_slots as the port may not exist if the parent * branch device was unplugged. This should be fixed by tracking * per-port slot allocation in drm_dp_mst_topology_state instead of * depending on the caller to tell us how many slots to release. */ That's not the only reason we should fix this: forcing the driver to track the VCPI allocations throughout a state's atomic check is error prone, because it means that extra care has to be taken with the order that drm_dp_atomic_find_vcpi_slots() and drm_dp_atomic_release_vcpi_slots() are called in in order to ensure idempotency. Currently the only driver actually using these helpers, i915, doesn't even do this correctly: multiple ->best_encoder() checks with i915's current implementation would not be idempotent and would over-allocate VCPI slots, something I learned trying to implement fallback retraining in MST. So: simplify this whole mess, and teach drm_dp_atomic_find_vcpi_slots() and drm_dp_atomic_release_vcpi_slots() to track the VCPI allocations for each port. This allows us to ensure idempotency without having to rely on the driver as much. Additionally: the driver doesn't need to do any kind of VCPI slot tracking anymore if it doesn't need it for it's own internal state. Additionally; this adds a new drm_dp_mst_atomic_check() helper which must be used by atomic drivers to perform validity checks for the new VCPI allocations incurred by a state. Also: update the documentation and make it more obvious that these /must/ be called by /all/ atomic drivers supporting MST. Changes since v6: - Keep a kref to all of the ports we have allocations on. This required a good bit of changing to when we call drm_dp_find_vcpi_slots(), mainly that we need to ensure that we only redo VCPI allocations on actual mode or CRTC changes, not crtc_state->active changes. Additionally, we no longer take the registration of the DRM connector for each port into account because so long as we have a kref to the port in the new or previous atomic state, the connector will stay registered. - Use the small changes to drm_dp_put_port() to add even more error checking to make misusage of the helpers more obvious. I added this after having to chase down various use-after-free conditions that started popping up from the new helpers so no one else has to troubleshoot that. - Move some accidental DRM_DEBUG_KMS() calls to DRM_DEBUG_ATOMIC() - Update documentation again, note that find/release() should both not be called on the same port in a single atomic check phase (but multiple calls to one or the other is OK) Changes since v4: - Don't skip the atomic checks for VCPI allocations if no new VCPI allocations happen in a state. This makes the next change I'm about to list here a lot easier to implement. - Don't ignore VCPI allocations on destroyed ports, instead ensure that when ports are destroyed and still have VCPI allocations in the topology state, the only state changes allowed are releasing said ports' VCPI. This prevents a state with a mix of VCPI allocations from destroyed ports, and allocations from valid ports. Changes since v3: - Don't release VCPI allocations in the topology state immediately in drm_dp_atomic_release_vcpi_slots(), instead mark them as 0 and skip over them in drm_dp_mst_duplicate_state(). This makes it so drm_dp_atomic_release_vcpi_slots() is still idempotent while also throwing warnings if the driver messes up it's book keeping and tries to release VCPI slots on a port that doesn't have any pre-existing VCPI allocation - danvet - Change mst_state/state in some debugging messages to "mst state" Changes since v2: - Use kmemdup() for duplicating MST state - danvet - Move port validation out of duplicate state callback - danvet - Handle looping through MST topology states in drm_dp_mst_atomic_check() so the driver doesn't have to do it - Fix documentation in drm_dp_atomic_find_vcpi_slots() - Move the atomic check for each individual topology state into it's own function, reduces indenting - Don't consider "stale" MST ports when calculating the bandwidth requirements. This is needed because originally we relied on the state duplication functions to prune any stale ports from the new state, which would prevent us from incorrectly considering their bandwidth requirements alongside legitimate new payloads. - Add function references in drm_dp_atomic_release_vcpi_slots() - danvet - Annotate atomic VCPI and atomic check functions with __must_check - danvet Changes since v1: - Don't use the now-removed ->atomic_check() for private objects hook, just give drivers a function to call themselves Signed-off-by: Lyude Paul Cc: Daniel Vetter --- drivers/gpu/dr
Re: [Intel-gfx] [PATCH v6 3/6] drm/dp_mst: Start tracking per-port VCPI allocations
On Thu, Nov 15, 2018 at 07:50:05PM -0500, Lyude Paul wrote: > There has been a TODO waiting for quite a long time in > drm_dp_mst_topology.c: > > /* We cannot rely on port->vcpi.num_slots to update >* topology_state->avail_slots as the port may not exist if the parent >* branch device was unplugged. This should be fixed by tracking >* per-port slot allocation in drm_dp_mst_topology_state instead of >* depending on the caller to tell us how many slots to release. >*/ > > That's not the only reason we should fix this: forcing the driver to > track the VCPI allocations throughout a state's atomic check is > error prone, because it means that extra care has to be taken with the > order that drm_dp_atomic_find_vcpi_slots() and > drm_dp_atomic_release_vcpi_slots() are called in in order to ensure > idempotency. Currently the only driver actually using these helpers, > i915, doesn't even do this correctly: multiple ->best_encoder() checks > with i915's current implementation would not be idempotent and would > over-allocate VCPI slots, something I learned trying to implement > fallback retraining in MST. > > So: simplify this whole mess, and teach drm_dp_atomic_find_vcpi_slots() > and drm_dp_atomic_release_vcpi_slots() to track the VCPI allocations for > each port. This allows us to ensure idempotency without having to rely > on the driver as much. Additionally: the driver doesn't need to do any > kind of VCPI slot tracking anymore if it doesn't need it for it's own > internal state. > > Additionally; this adds a new drm_dp_mst_atomic_check() helper which > must be used by atomic drivers to perform validity checks for the new > VCPI allocations incurred by a state. > > Also: update the documentation and make it more obvious that these > /must/ be called by /all/ atomic drivers supporting MST. > > Changes since v6: > - Keep a kref to all of the ports we have allocations on. This required >a good bit of changing to when we call drm_dp_find_vcpi_slots(), >mainly that we need to ensure that we only redo VCPI allocations on >actual mode or CRTC changes, not crtc_state->active changes. >Additionally, we no longer take the registration of the DRM connector >for each port into account because so long as we have a kref to the >port in the new or previous atomic state, the connector will stay >registered. I write an entire pile of small nitpits (still included most of them below), until I realized this here wont work. Delaying the call to destroy the connector (well, unregister it really) wreaks the design we've come up with thus far, resulting in most of my comments here. Instead, all we need to do is delay the kfree(port) at the bottom of drm_dp_destroy_port(). The vcpi allocation structure _only_ needs the pointer value to stay valid, as a lookup key. It doesn't care at all about anything actually stored in there. So the only thing we need to delay is the kfree. I think the simplest way to achieve this is to add a 2nd kref (port->kfree_ref or something like that), with on reference held by the port itself (released in drm_dp_destroy_port), and the other one held as-needed by the vcpi allocation lists. I think if we go with this design instead of retrofitting a semantic change of the port lifetime itself, all the complications I complain about below should disappear. Piles of comments below. Cheers, Daniel > - Use the small changes to drm_dp_put_port() to add even more error >checking to make misusage of the helpers more obvious. I added this >after having to chase down various use-after-free conditions that >started popping up from the new helpers so no one else has to >troubleshoot that. > - Move some accidental DRM_DEBUG_KMS() calls to DRM_DEBUG_ATOMIC() > - Update documentation again, note that find/release() should both not be >called on the same port in a single atomic check phase (but multiple >calls to one or the other is OK) > > Changes since v4: > - Don't skip the atomic checks for VCPI allocations if no new VCPI >allocations happen in a state. This makes the next change I'm about >to list here a lot easier to implement. > - Don't ignore VCPI allocations on destroyed ports, instead ensure that >when ports are destroyed and still have VCPI allocations in the >topology state, the only state changes allowed are releasing said >ports' VCPI. This prevents a state with a mix of VCPI allocations >from destroyed ports, and allocations from valid ports. > > Changes since v3: > - Don't release VCPI allocations in the topology state immediately in >drm_dp_atomic_release_vcpi_slots(), instead mark them as 0 and skip >over them in drm_dp_mst_duplicate_state(). This makes it so >drm_dp_atomic_release_vcpi_slots() is still idempotent while also >throwing warnings if the driver messes up it's book keeping and tries >to release VCPI slots on a port that doesn't have a
Re: [Intel-gfx] [PATCH v6 3/6] drm/dp_mst: Start tracking per-port VCPI allocations
On Mon, Nov 26, 2018 at 10:04:21PM +0100, Daniel Vetter wrote: > On Thu, Nov 15, 2018 at 07:50:05PM -0500, Lyude Paul wrote: > > There has been a TODO waiting for quite a long time in > > drm_dp_mst_topology.c: > > > > /* We cannot rely on port->vcpi.num_slots to update > > * topology_state->avail_slots as the port may not exist if the parent > > * branch device was unplugged. This should be fixed by tracking > > * per-port slot allocation in drm_dp_mst_topology_state instead of > > * depending on the caller to tell us how many slots to release. > > */ > > > > That's not the only reason we should fix this: forcing the driver to > > track the VCPI allocations throughout a state's atomic check is > > error prone, because it means that extra care has to be taken with the > > order that drm_dp_atomic_find_vcpi_slots() and > > drm_dp_atomic_release_vcpi_slots() are called in in order to ensure > > idempotency. Currently the only driver actually using these helpers, > > i915, doesn't even do this correctly: multiple ->best_encoder() checks > > with i915's current implementation would not be idempotent and would > > over-allocate VCPI slots, something I learned trying to implement > > fallback retraining in MST. > > > > So: simplify this whole mess, and teach drm_dp_atomic_find_vcpi_slots() > > and drm_dp_atomic_release_vcpi_slots() to track the VCPI allocations for > > each port. This allows us to ensure idempotency without having to rely > > on the driver as much. Additionally: the driver doesn't need to do any > > kind of VCPI slot tracking anymore if it doesn't need it for it's own > > internal state. > > > > Additionally; this adds a new drm_dp_mst_atomic_check() helper which > > must be used by atomic drivers to perform validity checks for the new > > VCPI allocations incurred by a state. > > > > Also: update the documentation and make it more obvious that these > > /must/ be called by /all/ atomic drivers supporting MST. > > > > Changes since v6: > > - Keep a kref to all of the ports we have allocations on. This required > >a good bit of changing to when we call drm_dp_find_vcpi_slots(), > >mainly that we need to ensure that we only redo VCPI allocations on > >actual mode or CRTC changes, not crtc_state->active changes. > >Additionally, we no longer take the registration of the DRM connector > >for each port into account because so long as we have a kref to the > >port in the new or previous atomic state, the connector will stay > >registered. > > I write an entire pile of small nitpits (still included most of them > below), until I realized this here wont work. Delaying the call to destroy > the connector (well, unregister it really) wreaks the design we've come up > with thus far, resulting in most of my comments here. > > Instead, all we need to do is delay the kfree(port) at the bottom of > drm_dp_destroy_port(). The vcpi allocation structure _only_ needs the > pointer value to stay valid, as a lookup key. It doesn't care at all about > anything actually stored in there. So the only thing we need to delay is > the kfree. I think the simplest way to achieve this is to add a 2nd kref > (port->kfree_ref or something like that), with on reference held by the > port itself (released in drm_dp_destroy_port), and the other one held > as-needed by the vcpi allocation lists. > > I think if we go with this design instead of retrofitting a semantic > change of the port lifetime itself, all the complications I complain about > below should disappear. In the above I meant drm_dp_destroy_port or drm_dp_destroy_connector_work. Aside: I think creating a kref for the final kfree would also solve a bunch of other issues in a much neater way: In commit f038c5b99fc1332f558b495d136d4f433ee65caa Author: Lyude Paul Date: Tue Nov 13 17:46:14 2018 -0500 drm/dp_mst: Skip validating ports during destruction, just ref we could use that kfree reference to make sure the port pointer is alive. This of course means that drm_dp_update_payload_part1() would also need to use the kfree reference for the vcpi allocations. And probably everywhere else (e.g. in your nouveau series for payload information). That would give us a very clear separation overall between "port can still be used because it's not yet unplugged" vs. "port only hangs around because a bunch of vcpi allocations and other payload things upstream of the port might still need the port lookup key to free resources". Instead of again sprinkling complicated conditions and magic tricks all over the code to figure out whethe we should allow the get_validate_port to succeed or not, or the modeset to succeed or not. -Daniel > > Piles of comments below. > > Cheers, Daniel > > > - Use the small changes to drm_dp_put_port() to add even more error > >checking to make misusage of the helpers more obvious. I added this > >after having to chase down various use-after-free conditions that > >
Re: [Intel-gfx] [PATCH v6 3/6] drm/dp_mst: Start tracking per-port VCPI allocations
On Mon, 2018-11-26 at 22:04 +0100, Daniel Vetter wrote: > On Thu, Nov 15, 2018 at 07:50:05PM -0500, Lyude Paul wrote: > > There has been a TODO waiting for quite a long time in > > drm_dp_mst_topology.c: > > > > /* We cannot rely on port->vcpi.num_slots to update > > * topology_state->avail_slots as the port may not exist if the parent > > * branch device was unplugged. This should be fixed by tracking > > * per-port slot allocation in drm_dp_mst_topology_state instead of > > * depending on the caller to tell us how many slots to release. > > */ > > > > That's not the only reason we should fix this: forcing the driver to > > track the VCPI allocations throughout a state's atomic check is > > error prone, because it means that extra care has to be taken with the > > order that drm_dp_atomic_find_vcpi_slots() and > > drm_dp_atomic_release_vcpi_slots() are called in in order to ensure > > idempotency. Currently the only driver actually using these helpers, > > i915, doesn't even do this correctly: multiple ->best_encoder() checks > > with i915's current implementation would not be idempotent and would > > over-allocate VCPI slots, something I learned trying to implement > > fallback retraining in MST. > > > > So: simplify this whole mess, and teach drm_dp_atomic_find_vcpi_slots() > > and drm_dp_atomic_release_vcpi_slots() to track the VCPI allocations for > > each port. This allows us to ensure idempotency without having to rely > > on the driver as much. Additionally: the driver doesn't need to do any > > kind of VCPI slot tracking anymore if it doesn't need it for it's own > > internal state. > > > > Additionally; this adds a new drm_dp_mst_atomic_check() helper which > > must be used by atomic drivers to perform validity checks for the new > > VCPI allocations incurred by a state. > > > > Also: update the documentation and make it more obvious that these > > /must/ be called by /all/ atomic drivers supporting MST. > > > > Changes since v6: > > - Keep a kref to all of the ports we have allocations on. This required > >a good bit of changing to when we call drm_dp_find_vcpi_slots(), > >mainly that we need to ensure that we only redo VCPI allocations on > >actual mode or CRTC changes, not crtc_state->active changes. > >Additionally, we no longer take the registration of the DRM connector > >for each port into account because so long as we have a kref to the > >port in the new or previous atomic state, the connector will stay > >registered. > > I write an entire pile of small nitpits (still included most of them > below), until I realized this here wont work. Delaying the call to destroy > the connector (well, unregister it really) wreaks the design we've come up > with thus far, resulting in most of my comments here. > > Instead, all we need to do is delay the kfree(port) at the bottom of > drm_dp_destroy_port(). The vcpi allocation structure _only_ needs the > pointer value to stay valid, as a lookup key. It doesn't care at all about > anything actually stored in there. So the only thing we need to delay is > the kfree. I think the simplest way to achieve this is to add a 2nd kref > (port->kfree_ref or something like that), with on reference held by the > port itself (released in drm_dp_destroy_port), and the other one held > as-needed by the vcpi allocation lists. > > I think if we go with this design instead of retrofitting a semantic > change of the port lifetime itself, all the complications I complain about > below should disappear. > > Piles of comments below. > > Cheers, Daniel > > > - Use the small changes to drm_dp_put_port() to add even more error > >checking to make misusage of the helpers more obvious. I added this > >after having to chase down various use-after-free conditions that > >started popping up from the new helpers so no one else has to > >troubleshoot that. > > - Move some accidental DRM_DEBUG_KMS() calls to DRM_DEBUG_ATOMIC() > > - Update documentation again, note that find/release() should both not be > >called on the same port in a single atomic check phase (but multiple > >calls to one or the other is OK) > > > > Changes since v4: > > - Don't skip the atomic checks for VCPI allocations if no new VCPI > >allocations happen in a state. This makes the next change I'm about > >to list here a lot easier to implement. > > - Don't ignore VCPI allocations on destroyed ports, instead ensure that > >when ports are destroyed and still have VCPI allocations in the > >topology state, the only state changes allowed are releasing said > >ports' VCPI. This prevents a state with a mix of VCPI allocations > >from destroyed ports, and allocations from valid ports. > > > > Changes since v3: > > - Don't release VCPI allocations in the topology state immediately in > >drm_dp_atomic_release_vcpi_slots(), instead mark them as 0 and skip > >over them in drm_dp_mst_duplicate_
Re: [Intel-gfx] [PATCH v6 3/6] drm/dp_mst: Start tracking per-port VCPI allocations
On Mon, Nov 26, 2018 at 04:36:46PM -0500, Lyude Paul wrote: > On Mon, 2018-11-26 at 22:04 +0100, Daniel Vetter wrote: > > On Thu, Nov 15, 2018 at 07:50:05PM -0500, Lyude Paul wrote: > > > There has been a TODO waiting for quite a long time in > > > drm_dp_mst_topology.c: > > > > > > /* We cannot rely on port->vcpi.num_slots to update > > >* topology_state->avail_slots as the port may not exist if the parent > > >* branch device was unplugged. This should be fixed by tracking > > >* per-port slot allocation in drm_dp_mst_topology_state instead of > > >* depending on the caller to tell us how many slots to release. > > >*/ > > > > > > That's not the only reason we should fix this: forcing the driver to > > > track the VCPI allocations throughout a state's atomic check is > > > error prone, because it means that extra care has to be taken with the > > > order that drm_dp_atomic_find_vcpi_slots() and > > > drm_dp_atomic_release_vcpi_slots() are called in in order to ensure > > > idempotency. Currently the only driver actually using these helpers, > > > i915, doesn't even do this correctly: multiple ->best_encoder() checks > > > with i915's current implementation would not be idempotent and would > > > over-allocate VCPI slots, something I learned trying to implement > > > fallback retraining in MST. > > > > > > So: simplify this whole mess, and teach drm_dp_atomic_find_vcpi_slots() > > > and drm_dp_atomic_release_vcpi_slots() to track the VCPI allocations for > > > each port. This allows us to ensure idempotency without having to rely > > > on the driver as much. Additionally: the driver doesn't need to do any > > > kind of VCPI slot tracking anymore if it doesn't need it for it's own > > > internal state. > > > > > > Additionally; this adds a new drm_dp_mst_atomic_check() helper which > > > must be used by atomic drivers to perform validity checks for the new > > > VCPI allocations incurred by a state. > > > > > > Also: update the documentation and make it more obvious that these > > > /must/ be called by /all/ atomic drivers supporting MST. > > > > > > Changes since v6: > > > - Keep a kref to all of the ports we have allocations on. This required > > >a good bit of changing to when we call drm_dp_find_vcpi_slots(), > > >mainly that we need to ensure that we only redo VCPI allocations on > > >actual mode or CRTC changes, not crtc_state->active changes. > > >Additionally, we no longer take the registration of the DRM connector > > >for each port into account because so long as we have a kref to the > > >port in the new or previous atomic state, the connector will stay > > >registered. > > > > I write an entire pile of small nitpits (still included most of them > > below), until I realized this here wont work. Delaying the call to destroy > > the connector (well, unregister it really) wreaks the design we've come up > > with thus far, resulting in most of my comments here. > > > > Instead, all we need to do is delay the kfree(port) at the bottom of > > drm_dp_destroy_port(). The vcpi allocation structure _only_ needs the > > pointer value to stay valid, as a lookup key. It doesn't care at all about > > anything actually stored in there. So the only thing we need to delay is > > the kfree. I think the simplest way to achieve this is to add a 2nd kref > > (port->kfree_ref or something like that), with on reference held by the > > port itself (released in drm_dp_destroy_port), and the other one held > > as-needed by the vcpi allocation lists. > > > > I think if we go with this design instead of retrofitting a semantic > > change of the port lifetime itself, all the complications I complain about > > below should disappear. > > > > Piles of comments below. > > > > Cheers, Daniel > > > > > - Use the small changes to drm_dp_put_port() to add even more error > > >checking to make misusage of the helpers more obvious. I added this > > >after having to chase down various use-after-free conditions that > > >started popping up from the new helpers so no one else has to > > >troubleshoot that. > > > - Move some accidental DRM_DEBUG_KMS() calls to DRM_DEBUG_ATOMIC() > > > - Update documentation again, note that find/release() should both not be > > >called on the same port in a single atomic check phase (but multiple > > >calls to one or the other is OK) > > > > > > Changes since v4: > > > - Don't skip the atomic checks for VCPI allocations if no new VCPI > > >allocations happen in a state. This makes the next change I'm about > > >to list here a lot easier to implement. > > > - Don't ignore VCPI allocations on destroyed ports, instead ensure that > > >when ports are destroyed and still have VCPI allocations in the > > >topology state, the only state changes allowed are releasing said > > >ports' VCPI. This prevents a state with a mix of VCPI allocations > > >from destroyed ports, and allocations from valid p
Re: [Intel-gfx] [PATCH v6 3/6] drm/dp_mst: Start tracking per-port VCPI allocations
On Mon, 2018-11-26 at 22:22 +0100, Daniel Vetter wrote: > On Mon, Nov 26, 2018 at 10:04:21PM +0100, Daniel Vetter wrote: > > On Thu, Nov 15, 2018 at 07:50:05PM -0500, Lyude Paul wrote: > > > There has been a TODO waiting for quite a long time in > > > drm_dp_mst_topology.c: > > > > > > /* We cannot rely on port->vcpi.num_slots to update > > >* topology_state->avail_slots as the port may not exist if the parent > > >* branch device was unplugged. This should be fixed by tracking > > >* per-port slot allocation in drm_dp_mst_topology_state instead of > > >* depending on the caller to tell us how many slots to release. > > >*/ > > > > > > That's not the only reason we should fix this: forcing the driver to > > > track the VCPI allocations throughout a state's atomic check is > > > error prone, because it means that extra care has to be taken with the > > > order that drm_dp_atomic_find_vcpi_slots() and > > > drm_dp_atomic_release_vcpi_slots() are called in in order to ensure > > > idempotency. Currently the only driver actually using these helpers, > > > i915, doesn't even do this correctly: multiple ->best_encoder() checks > > > with i915's current implementation would not be idempotent and would > > > over-allocate VCPI slots, something I learned trying to implement > > > fallback retraining in MST. > > > > > > So: simplify this whole mess, and teach drm_dp_atomic_find_vcpi_slots() > > > and drm_dp_atomic_release_vcpi_slots() to track the VCPI allocations for > > > each port. This allows us to ensure idempotency without having to rely > > > on the driver as much. Additionally: the driver doesn't need to do any > > > kind of VCPI slot tracking anymore if it doesn't need it for it's own > > > internal state. > > > > > > Additionally; this adds a new drm_dp_mst_atomic_check() helper which > > > must be used by atomic drivers to perform validity checks for the new > > > VCPI allocations incurred by a state. > > > > > > Also: update the documentation and make it more obvious that these > > > /must/ be called by /all/ atomic drivers supporting MST. > > > > > > Changes since v6: > > > - Keep a kref to all of the ports we have allocations on. This required > > >a good bit of changing to when we call drm_dp_find_vcpi_slots(), > > >mainly that we need to ensure that we only redo VCPI allocations on > > >actual mode or CRTC changes, not crtc_state->active changes. > > >Additionally, we no longer take the registration of the DRM connector > > >for each port into account because so long as we have a kref to the > > >port in the new or previous atomic state, the connector will stay > > >registered. > > > > I write an entire pile of small nitpits (still included most of them > > below), until I realized this here wont work. Delaying the call to destroy > > the connector (well, unregister it really) wreaks the design we've come up > > with thus far, resulting in most of my comments here. > > > > Instead, all we need to do is delay the kfree(port) at the bottom of > > drm_dp_destroy_port(). The vcpi allocation structure _only_ needs the > > pointer value to stay valid, as a lookup key. It doesn't care at all about > > anything actually stored in there. So the only thing we need to delay is > > the kfree. I think the simplest way to achieve this is to add a 2nd kref > > (port->kfree_ref or something like that), with on reference held by the > > port itself (released in drm_dp_destroy_port), and the other one held > > as-needed by the vcpi allocation lists. > > > > I think if we go with this design instead of retrofitting a semantic > > change of the port lifetime itself, all the complications I complain about > > below should disappear. > > In the above I meant drm_dp_destroy_port or drm_dp_destroy_connector_work. > > Aside: I think creating a kref for the final kfree would also solve a > bunch of other issues in a much neater way: In > > commit f038c5b99fc1332f558b495d136d4f433ee65caa > Author: Lyude Paul > Date: Tue Nov 13 17:46:14 2018 -0500 > > drm/dp_mst: Skip validating ports during destruction, just ref > > we could use that kfree reference to make sure the port pointer is alive. > This of course means that drm_dp_update_payload_part1() would also need to > use the kfree reference for the vcpi allocations. And probably everywhere > else (e.g. in your nouveau series for payload information). > > That would give us a very clear separation overall between "port can still > be used because it's not yet unplugged" vs. "port only hangs around > because a bunch of vcpi allocations and other payload things upstream of > the port might still need the port lookup key to free resources". Instead > of again sprinkling complicated conditions and magic tricks all over the > code to figure out whethe we should allow the get_validate_port to succeed > or not, or the modeset to succeed or not. mhm, I've been experimenting with this and I think I agree this is de
Re: [Intel-gfx] [PATCH v6 3/6] drm/dp_mst: Start tracking per-port VCPI allocations
On Tue, Nov 27, 2018 at 12:48:59PM -0500, Lyude Paul wrote: > On Mon, 2018-11-26 at 22:22 +0100, Daniel Vetter wrote: > > On Mon, Nov 26, 2018 at 10:04:21PM +0100, Daniel Vetter wrote: > > > On Thu, Nov 15, 2018 at 07:50:05PM -0500, Lyude Paul wrote: > > > > There has been a TODO waiting for quite a long time in > > > > drm_dp_mst_topology.c: > > > > > > > > /* We cannot rely on port->vcpi.num_slots to update > > > > * topology_state->avail_slots as the port may not exist if the > > > > parent > > > > * branch device was unplugged. This should be fixed by tracking > > > > * per-port slot allocation in drm_dp_mst_topology_state > > > > instead of > > > > * depending on the caller to tell us how many slots to release. > > > > */ > > > > > > > > That's not the only reason we should fix this: forcing the driver to > > > > track the VCPI allocations throughout a state's atomic check is > > > > error prone, because it means that extra care has to be taken with the > > > > order that drm_dp_atomic_find_vcpi_slots() and > > > > drm_dp_atomic_release_vcpi_slots() are called in in order to ensure > > > > idempotency. Currently the only driver actually using these helpers, > > > > i915, doesn't even do this correctly: multiple ->best_encoder() checks > > > > with i915's current implementation would not be idempotent and would > > > > over-allocate VCPI slots, something I learned trying to implement > > > > fallback retraining in MST. > > > > > > > > So: simplify this whole mess, and teach drm_dp_atomic_find_vcpi_slots() > > > > and drm_dp_atomic_release_vcpi_slots() to track the VCPI allocations for > > > > each port. This allows us to ensure idempotency without having to rely > > > > on the driver as much. Additionally: the driver doesn't need to do any > > > > kind of VCPI slot tracking anymore if it doesn't need it for it's own > > > > internal state. > > > > > > > > Additionally; this adds a new drm_dp_mst_atomic_check() helper which > > > > must be used by atomic drivers to perform validity checks for the new > > > > VCPI allocations incurred by a state. > > > > > > > > Also: update the documentation and make it more obvious that these > > > > /must/ be called by /all/ atomic drivers supporting MST. > > > > > > > > Changes since v6: > > > > - Keep a kref to all of the ports we have allocations on. This required > > > >a good bit of changing to when we call drm_dp_find_vcpi_slots(), > > > >mainly that we need to ensure that we only redo VCPI allocations on > > > >actual mode or CRTC changes, not crtc_state->active changes. > > > >Additionally, we no longer take the registration of the DRM connector > > > >for each port into account because so long as we have a kref to the > > > >port in the new or previous atomic state, the connector will stay > > > >registered. > > > > > > I write an entire pile of small nitpits (still included most of them > > > below), until I realized this here wont work. Delaying the call to destroy > > > the connector (well, unregister it really) wreaks the design we've come up > > > with thus far, resulting in most of my comments here. > > > > > > Instead, all we need to do is delay the kfree(port) at the bottom of > > > drm_dp_destroy_port(). The vcpi allocation structure _only_ needs the > > > pointer value to stay valid, as a lookup key. It doesn't care at all about > > > anything actually stored in there. So the only thing we need to delay is > > > the kfree. I think the simplest way to achieve this is to add a 2nd kref > > > (port->kfree_ref or something like that), with on reference held by the > > > port itself (released in drm_dp_destroy_port), and the other one held > > > as-needed by the vcpi allocation lists. > > > > > > I think if we go with this design instead of retrofitting a semantic > > > change of the port lifetime itself, all the complications I complain about > > > below should disappear. > > > > In the above I meant drm_dp_destroy_port or drm_dp_destroy_connector_work. > > > > Aside: I think creating a kref for the final kfree would also solve a > > bunch of other issues in a much neater way: In > > > > commit f038c5b99fc1332f558b495d136d4f433ee65caa > > Author: Lyude Paul > > Date: Tue Nov 13 17:46:14 2018 -0500 > > > > drm/dp_mst: Skip validating ports during destruction, just ref > > > > we could use that kfree reference to make sure the port pointer is alive. > > This of course means that drm_dp_update_payload_part1() would also need to > > use the kfree reference for the vcpi allocations. And probably everywhere > > else (e.g. in your nouveau series for payload information). > > > > That would give us a very clear separation overall between "port can still > > be used because it's not yet unplugged" vs. "port only hangs around > > because a bunch of vcpi allocations and other payload things upstream of > > the port might still need the port lookup key to free
Re: [Intel-gfx] [PATCH v6 3/6] drm/dp_mst: Start tracking per-port VCPI allocations
On Tue, 2018-11-27 at 20:44 +0100, Daniel Vetter wrote: > On Tue, Nov 27, 2018 at 12:48:59PM -0500, Lyude Paul wrote: > > On Mon, 2018-11-26 at 22:22 +0100, Daniel Vetter wrote: > > > On Mon, Nov 26, 2018 at 10:04:21PM +0100, Daniel Vetter wrote: > > > > On Thu, Nov 15, 2018 at 07:50:05PM -0500, Lyude Paul wrote: > > > > > There has been a TODO waiting for quite a long time in > > > > > drm_dp_mst_topology.c: > > > > > > > > > > /* We cannot rely on port->vcpi.num_slots to update > > > > >* topology_state->avail_slots as the port may not exist if > > > > > the parent > > > > >* branch device was unplugged. This should be fixed by > > > > > tracking > > > > >* per-port slot allocation in drm_dp_mst_topology_state > > > > > instead of > > > > >* depending on the caller to tell us how many slots to > > > > > release. > > > > >*/ > > > > > > > > > > That's not the only reason we should fix this: forcing the driver to > > > > > track the VCPI allocations throughout a state's atomic check is > > > > > error prone, because it means that extra care has to be taken with > > > > > the > > > > > order that drm_dp_atomic_find_vcpi_slots() and > > > > > drm_dp_atomic_release_vcpi_slots() are called in in order to ensure > > > > > idempotency. Currently the only driver actually using these helpers, > > > > > i915, doesn't even do this correctly: multiple ->best_encoder() > > > > > checks > > > > > with i915's current implementation would not be idempotent and would > > > > > over-allocate VCPI slots, something I learned trying to implement > > > > > fallback retraining in MST. > > > > > > > > > > So: simplify this whole mess, and teach > > > > > drm_dp_atomic_find_vcpi_slots() > > > > > and drm_dp_atomic_release_vcpi_slots() to track the VCPI allocations > > > > > for > > > > > each port. This allows us to ensure idempotency without having to > > > > > rely > > > > > on the driver as much. Additionally: the driver doesn't need to do > > > > > any > > > > > kind of VCPI slot tracking anymore if it doesn't need it for it's > > > > > own > > > > > internal state. > > > > > > > > > > Additionally; this adds a new drm_dp_mst_atomic_check() helper which > > > > > must be used by atomic drivers to perform validity checks for the > > > > > new > > > > > VCPI allocations incurred by a state. > > > > > > > > > > Also: update the documentation and make it more obvious that these > > > > > /must/ be called by /all/ atomic drivers supporting MST. > > > > > > > > > > Changes since v6: > > > > > - Keep a kref to all of the ports we have allocations on. This > > > > > required > > > > >a good bit of changing to when we call drm_dp_find_vcpi_slots(), > > > > >mainly that we need to ensure that we only redo VCPI allocations > > > > > on > > > > >actual mode or CRTC changes, not crtc_state->active changes. > > > > >Additionally, we no longer take the registration of the DRM > > > > > connector > > > > >for each port into account because so long as we have a kref to > > > > > the > > > > >port in the new or previous atomic state, the connector will stay > > > > >registered. > > > > > > > > I write an entire pile of small nitpits (still included most of them > > > > below), until I realized this here wont work. Delaying the call to > > > > destroy > > > > the connector (well, unregister it really) wreaks the design we've > > > > come up > > > > with thus far, resulting in most of my comments here. > > > > > > > > Instead, all we need to do is delay the kfree(port) at the bottom of > > > > drm_dp_destroy_port(). The vcpi allocation structure _only_ needs the > > > > pointer value to stay valid, as a lookup key. It doesn't care at all > > > > about > > > > anything actually stored in there. So the only thing we need to delay > > > > is > > > > the kfree. I think the simplest way to achieve this is to add a 2nd > > > > kref > > > > (port->kfree_ref or something like that), with on reference held by > > > > the > > > > port itself (released in drm_dp_destroy_port), and the other one held > > > > as-needed by the vcpi allocation lists. > > > > > > > > I think if we go with this design instead of retrofitting a semantic > > > > change of the port lifetime itself, all the complications I complain > > > > about > > > > below should disappear. > > > > > > In the above I meant drm_dp_destroy_port or > > > drm_dp_destroy_connector_work. > > > > > > Aside: I think creating a kref for the final kfree would also solve a > > > bunch of other issues in a much neater way: In > > > > > > commit f038c5b99fc1332f558b495d136d4f433ee65caa > > > Author: Lyude Paul > > > Date: Tue Nov 13 17:46:14 2018 -0500 > > > > > > drm/dp_mst: Skip validating ports during destruction, just ref > > > > > > we could use that kfree reference to make sure the port pointer is > > > alive. > > > This of course means that drm_dp_update_payload_part1() would also need > > > to > > > use the kfree referen
Re: [Intel-gfx] [PATCH v6 3/6] drm/dp_mst: Start tracking per-port VCPI allocations
On Tue, Nov 27, 2018 at 08:44:14PM -0500, Lyude Paul wrote: > On Tue, 2018-11-27 at 20:44 +0100, Daniel Vetter wrote: > > On Tue, Nov 27, 2018 at 12:48:59PM -0500, Lyude Paul wrote: > > > On Mon, 2018-11-26 at 22:22 +0100, Daniel Vetter wrote: > > > > On Mon, Nov 26, 2018 at 10:04:21PM +0100, Daniel Vetter wrote: > > > > > On Thu, Nov 15, 2018 at 07:50:05PM -0500, Lyude Paul wrote: > > > > > > There has been a TODO waiting for quite a long time in > > > > > > drm_dp_mst_topology.c: > > > > > > > > > > > > /* We cannot rely on port->vcpi.num_slots to update > > > > > > * topology_state->avail_slots as the port may not exist if > > > > > > the parent > > > > > > * branch device was unplugged. This should be fixed by > > > > > > tracking > > > > > > * per-port slot allocation in drm_dp_mst_topology_state > > > > > > instead of > > > > > > * depending on the caller to tell us how many slots to > > > > > > release. > > > > > > */ > > > > > > > > > > > > That's not the only reason we should fix this: forcing the driver to > > > > > > track the VCPI allocations throughout a state's atomic check is > > > > > > error prone, because it means that extra care has to be taken with > > > > > > the > > > > > > order that drm_dp_atomic_find_vcpi_slots() and > > > > > > drm_dp_atomic_release_vcpi_slots() are called in in order to ensure > > > > > > idempotency. Currently the only driver actually using these helpers, > > > > > > i915, doesn't even do this correctly: multiple ->best_encoder() > > > > > > checks > > > > > > with i915's current implementation would not be idempotent and would > > > > > > over-allocate VCPI slots, something I learned trying to implement > > > > > > fallback retraining in MST. > > > > > > > > > > > > So: simplify this whole mess, and teach > > > > > > drm_dp_atomic_find_vcpi_slots() > > > > > > and drm_dp_atomic_release_vcpi_slots() to track the VCPI allocations > > > > > > for > > > > > > each port. This allows us to ensure idempotency without having to > > > > > > rely > > > > > > on the driver as much. Additionally: the driver doesn't need to do > > > > > > any > > > > > > kind of VCPI slot tracking anymore if it doesn't need it for it's > > > > > > own > > > > > > internal state. > > > > > > > > > > > > Additionally; this adds a new drm_dp_mst_atomic_check() helper which > > > > > > must be used by atomic drivers to perform validity checks for the > > > > > > new > > > > > > VCPI allocations incurred by a state. > > > > > > > > > > > > Also: update the documentation and make it more obvious that these > > > > > > /must/ be called by /all/ atomic drivers supporting MST. > > > > > > > > > > > > Changes since v6: > > > > > > - Keep a kref to all of the ports we have allocations on. This > > > > > > required > > > > > >a good bit of changing to when we call drm_dp_find_vcpi_slots(), > > > > > >mainly that we need to ensure that we only redo VCPI allocations > > > > > > on > > > > > >actual mode or CRTC changes, not crtc_state->active changes. > > > > > >Additionally, we no longer take the registration of the DRM > > > > > > connector > > > > > >for each port into account because so long as we have a kref to > > > > > > the > > > > > >port in the new or previous atomic state, the connector will stay > > > > > >registered. > > > > > > > > > > I write an entire pile of small nitpits (still included most of them > > > > > below), until I realized this here wont work. Delaying the call to > > > > > destroy > > > > > the connector (well, unregister it really) wreaks the design we've > > > > > come up > > > > > with thus far, resulting in most of my comments here. > > > > > > > > > > Instead, all we need to do is delay the kfree(port) at the bottom of > > > > > drm_dp_destroy_port(). The vcpi allocation structure _only_ needs the > > > > > pointer value to stay valid, as a lookup key. It doesn't care at all > > > > > about > > > > > anything actually stored in there. So the only thing we need to delay > > > > > is > > > > > the kfree. I think the simplest way to achieve this is to add a 2nd > > > > > kref > > > > > (port->kfree_ref or something like that), with on reference held by > > > > > the > > > > > port itself (released in drm_dp_destroy_port), and the other one held > > > > > as-needed by the vcpi allocation lists. > > > > > > > > > > I think if we go with this design instead of retrofitting a semantic > > > > > change of the port lifetime itself, all the complications I complain > > > > > about > > > > > below should disappear. > > > > > > > > In the above I meant drm_dp_destroy_port or > > > > drm_dp_destroy_connector_work. > > > > > > > > Aside: I think creating a kref for the final kfree would also solve a > > > > bunch of other issues in a much neater way: In > > > > > > > > commit f038c5b99fc1332f558b495d136d4f433ee65caa > > > > Author: Lyude Paul > > > > Date: Tue Nov 13 17:46:14 2018 -0500 > > > > > > > > drm/dp_mst: Skip
Re: [Intel-gfx] [PATCH v6 3/6] drm/dp_mst: Start tracking per-port VCPI allocations
On Wed, 2018-11-28 at 09:17 +0100, Daniel Vetter wrote: > On Tue, Nov 27, 2018 at 08:44:14PM -0500, Lyude Paul wrote: > > On Tue, 2018-11-27 at 20:44 +0100, Daniel Vetter wrote: > > > On Tue, Nov 27, 2018 at 12:48:59PM -0500, Lyude Paul wrote: > > > > On Mon, 2018-11-26 at 22:22 +0100, Daniel Vetter wrote: > > > > > On Mon, Nov 26, 2018 at 10:04:21PM +0100, Daniel Vetter wrote: > > > > > > On Thu, Nov 15, 2018 at 07:50:05PM -0500, Lyude Paul wrote: > > > > > > > There has been a TODO waiting for quite a long time in > > > > > > > drm_dp_mst_topology.c: > > > > > > > > > > > > > > /* We cannot rely on port->vcpi.num_slots to update > > > > > > >* topology_state->avail_slots as the port may not exist if > > > > > > > the parent > > > > > > >* branch device was unplugged. This should be fixed by > > > > > > > tracking > > > > > > >* per-port slot allocation in drm_dp_mst_topology_state > > > > > > > instead of > > > > > > >* depending on the caller to tell us how many slots to > > > > > > > release. > > > > > > >*/ > > > > > > > > > > > > > > That's not the only reason we should fix this: forcing the > > > > > > > driver to > > > > > > > track the VCPI allocations throughout a state's atomic check is > > > > > > > error prone, because it means that extra care has to be taken > > > > > > > with > > > > > > > the > > > > > > > order that drm_dp_atomic_find_vcpi_slots() and > > > > > > > drm_dp_atomic_release_vcpi_slots() are called in in order to > > > > > > > ensure > > > > > > > idempotency. Currently the only driver actually using these > > > > > > > helpers, > > > > > > > i915, doesn't even do this correctly: multiple ->best_encoder() > > > > > > > checks > > > > > > > with i915's current implementation would not be idempotent and > > > > > > > would > > > > > > > over-allocate VCPI slots, something I learned trying to > > > > > > > implement > > > > > > > fallback retraining in MST. > > > > > > > > > > > > > > So: simplify this whole mess, and teach > > > > > > > drm_dp_atomic_find_vcpi_slots() > > > > > > > and drm_dp_atomic_release_vcpi_slots() to track the VCPI > > > > > > > allocations > > > > > > > for > > > > > > > each port. This allows us to ensure idempotency without having > > > > > > > to > > > > > > > rely > > > > > > > on the driver as much. Additionally: the driver doesn't need to > > > > > > > do > > > > > > > any > > > > > > > kind of VCPI slot tracking anymore if it doesn't need it for > > > > > > > it's > > > > > > > own > > > > > > > internal state. > > > > > > > > > > > > > > Additionally; this adds a new drm_dp_mst_atomic_check() helper > > > > > > > which > > > > > > > must be used by atomic drivers to perform validity checks for > > > > > > > the > > > > > > > new > > > > > > > VCPI allocations incurred by a state. > > > > > > > > > > > > > > Also: update the documentation and make it more obvious that > > > > > > > these > > > > > > > /must/ be called by /all/ atomic drivers supporting MST. > > > > > > > > > > > > > > Changes since v6: > > > > > > > - Keep a kref to all of the ports we have allocations on. This > > > > > > > required > > > > > > >a good bit of changing to when we call > > > > > > > drm_dp_find_vcpi_slots(), > > > > > > >mainly that we need to ensure that we only redo VCPI > > > > > > > allocations > > > > > > > on > > > > > > >actual mode or CRTC changes, not crtc_state->active changes. > > > > > > >Additionally, we no longer take the registration of the DRM > > > > > > > connector > > > > > > >for each port into account because so long as we have a kref > > > > > > > to > > > > > > > the > > > > > > >port in the new or previous atomic state, the connector will > > > > > > > stay > > > > > > >registered. > > > > > > > > > > > > I write an entire pile of small nitpits (still included most of > > > > > > them > > > > > > below), until I realized this here wont work. Delaying the call to > > > > > > destroy > > > > > > the connector (well, unregister it really) wreaks the design we've > > > > > > come up > > > > > > with thus far, resulting in most of my comments here. > > > > > > > > > > > > Instead, all we need to do is delay the kfree(port) at the bottom > > > > > > of > > > > > > drm_dp_destroy_port(). The vcpi allocation structure _only_ needs > > > > > > the > > > > > > pointer value to stay valid, as a lookup key. It doesn't care at > > > > > > all > > > > > > about > > > > > > anything actually stored in there. So the only thing we need to > > > > > > delay > > > > > > is > > > > > > the kfree. I think the simplest way to achieve this is to add a > > > > > > 2nd > > > > > > kref > > > > > > (port->kfree_ref or something like that), with on reference held > > > > > > by > > > > > > the > > > > > > port itself (released in drm_dp_destroy_port), and the other one > > > > > > held > > > > > > as-needed by the vcpi allocation lists. > > > > > > > > > > > > I think if we go with this design instead of retrof
Re: [Intel-gfx] [PATCH v6 3/6] drm/dp_mst: Start tracking per-port VCPI allocations
On Wed, 2018-11-28 at 09:17 +0100, Daniel Vetter wrote: > On Tue, Nov 27, 2018 at 08:44:14PM -0500, Lyude Paul wrote: > > On Tue, 2018-11-27 at 20:44 +0100, Daniel Vetter wrote: > > > > We could do this the other way around so it looks like this maybe > > > > struct kref; /* manages kfree() */ > > struct topology_kref; /* corresonds to lifetime in topology */ > > > > Then only expose functions for the normal kref to drivers, so that any > > possible confusion is still limited to drm_dp_mst_topology.c > > I like this bikeshed a lot. Since we need a bunch of the plain > kref_get/put internally, probably still good to have a wrapper. For that > the port_malloc_get/put() still sounds good to me - port_kfree_get/put > sounds confusing, since it's not the kfree we're refcounting, but the > memory allocation. > > Another option would be to add _topology to the public get/put functions, > but that makes for a fairly long function name :-/ bleh. Looked at not using a malloc() prefix in the naming but it's definitely still more confusing without one. I really do question if we really want the canonical naming prefix for the MST helpers to be drm_dp_mst_topology. It would be very nice to have this freed so we could do something like this: drm_dp_mst_topology_get_mstb() /* &topology_kref */ drm_dp_mst_topology_put_mstb() /* &topology_kref */ drm_dp_mst_get_mstb_malloc() /* &kref */ drm_dp_mst_put_mstb_malloc() /* &kref */ drm_dp_mst_topology_get_port() /* &topology_kref */ drm_dp_mst_topology_put_port() /* &topology_kref */ drm_dp_mst_get_port_malloc() /* &kref */ drm_dp_mst_put_port_malloc() /* &kref */ Additionally, I had an epiphany and figured out a seriously dead-simple rule that answers "what ref to use where" in the MST helper code: Let's start with a topology like this: |- mst_primary |- port #1 |- port #2 |- port #3 |- mstb #1 |- port #4 |- mstb #2 |- port #5 |- port #6 |- mstb #3 |- port #7 |- mstb #4 |- port #8 |- port #9 If mstb #1 was removed from the topology, ports 4-9 and mstbs 2-4 must lose their topology references. Simply put: each port holds a topology reference to it's mstb (if there is one), and each mstb holds a topology reference to it's ports. Now, let's allocate some payloads on the topology: |- mst_primary |- port #1 |- port #2 |- port #3 |- mstb #1 |- port #4 |- mstb #2 (has payload) |- port #5 |- port #6 |- mstb #3 |- port #7 |- mstb #4 (has payload) |- port #8 |- port #9 So, now if we remove mstb #1 any payloads under it need to be freed. Since mstb #4's and #2's parents no longer exist in the topology however, we must go back up the tree to find our last living relative. So: mstb #4 → port #7 → mstb #3 → port #6 → mstb #1 → port #3 for payload 1 mstb #4 → port #4 → mstb #1 → port #3 for payload 2 Going off this we can come up with the rule for malloc refs: each port holds a malloc ref to it's parent mstb, and each mstb holds a malloc ref to it's parent port (if there is one). So, it's just the reverse of the topology ref rule. Now just add refs for payloads and other stuff, and we're good. Hooray! Now I can also use this in any docs I write too :) > -Daniel > > > > > > -Daniel > > > > > > > > > > > Piles of comments below. > > > > > > > > > > > > Cheers, Daniel > > > > > > > > > > > > > - Use the small changes to drm_dp_put_port() to add even more > > > > > > > error > > > > > > >checking to make misusage of the helpers more obvious. I > > > > > > > added > > > > > > > this > > > > > > >after having to chase down various use-after-free conditions > > > > > > > that > > > > > > >started popping up from the new helpers so no one else has to > > > > > > >troubleshoot that. > > > > > > > - Move some accidental DRM_DEBUG_KMS() calls to > > > > > > > DRM_DEBUG_ATOMIC() > > > > > > > - Update documentation again, note that find/release() should > > > > > > > both > > > > > > > not > > > > > > > be > > > > > > >called on the same port in a single atomic check phase (but > > > > > > > multiple > > > > > > >calls to one or the other is OK) > > > > > > > > > > > > > > Changes since v4: > > > > > > > - Don't skip the atomic checks for VCPI allocations if no new > > > > > > > VCPI > > > > > > >allocations happen in a state. This makes the next change I'm > > > > > > > about > > > > > > >to list here a lot easier to implement. > > > > > > > - Don't ignore VCPI allocations on destroyed ports, instead > > > > > > > ensure > > > > > > > that > > > > > > >when ports are destroyed and still have VCPI allocations in > > > > > > > the > > > > > > >topology state, the only state changes allowed are
Re: [Intel-gfx] [PATCH v6 3/6] drm/dp_mst: Start tracking per-port VCPI allocations
On Wed, Nov 28, 2018 at 08:14:13PM -0500, Lyude Paul wrote: > > On Wed, 2018-11-28 at 09:17 +0100, Daniel Vetter wrote: > > On Tue, Nov 27, 2018 at 08:44:14PM -0500, Lyude Paul wrote: > > > On Tue, 2018-11-27 at 20:44 +0100, Daniel Vetter wrote: > > > > > > We could do this the other way around so it looks like this maybe > > > > > > struct kref; /* manages kfree() */ > > > struct topology_kref; /* corresonds to lifetime in topology */ > > > > > > Then only expose functions for the normal kref to drivers, so that any > > > possible confusion is still limited to drm_dp_mst_topology.c > > > > I like this bikeshed a lot. Since we need a bunch of the plain > > kref_get/put internally, probably still good to have a wrapper. For that > > the port_malloc_get/put() still sounds good to me - port_kfree_get/put > > sounds confusing, since it's not the kfree we're refcounting, but the > > memory allocation. > > > > Another option would be to add _topology to the public get/put functions, > > but that makes for a fairly long function name :-/ > > bleh. Looked at not using a malloc() prefix in the naming but it's > definitely still more confusing without one. > > I really do question if we really want the canonical naming prefix for the MST > helpers to be drm_dp_mst_topology. It would be very nice to have this freed so > we could do something like this: > > drm_dp_mst_topology_get_mstb() /* &topology_kref */ > drm_dp_mst_topology_put_mstb() /* &topology_kref */ > drm_dp_mst_get_mstb_malloc() /* &kref */ > drm_dp_mst_put_mstb_malloc() /* &kref */ > > drm_dp_mst_topology_get_port() /* &topology_kref */ > drm_dp_mst_topology_put_port() /* &topology_kref */ > drm_dp_mst_get_port_malloc() /* &kref */ > drm_dp_mst_put_port_malloc() /* &kref */ Ack on this bikeshed, looks good to me. > Additionally, I had an epiphany and figured out a seriously dead-simple > rule that answers "what ref to use where" in the MST helper code: > > Let's start with a topology like this: > > |- mst_primary > |- port #1 > |- port #2 > |- port #3 > |- mstb #1 > |- port #4 > |- mstb #2 > |- port #5 > |- port #6 > |- mstb #3 >|- port #7 > |- mstb #4 >|- port #8 >|- port #9 > > If mstb #1 was removed from the topology, ports 4-9 and mstbs 2-4 must > lose their topology references. Simply put: each port holds a topology > reference to it's mstb (if there is one), and each mstb holds a topology > reference to it's ports. > > Now, let's allocate some payloads on the topology: > > |- mst_primary > |- port #1 > |- port #2 > |- port #3 > |- mstb #1 > |- port #4 > |- mstb #2 (has payload) > |- port #5 > |- port #6 > |- mstb #3 >|- port #7 > |- mstb #4 (has payload) >|- port #8 >|- port #9 > > So, now if we remove mstb #1 any payloads under it need to be freed. > Since mstb #4's and #2's parents no longer exist in the topology > however, we must go back up the tree to find our last living relative. > > So: > > mstb #4 → port #7 → mstb #3 → port #6 → mstb #1 → port #3 for payload 1 > mstb #4 → port #4 → mstb #1 → port #3 for payload 2 > > Going off this we can come up with the rule for malloc refs: each port > holds a malloc ref to it's parent mstb, and each mstb holds a malloc ref > to it's parent port (if there is one). So, it's just the reverse of the > topology ref rule. Now just add refs for payloads and other stuff, and > we're good. > > Hooray! Now I can also use this in any docs I write too :) This flew right over my head (but coffee isn't working yet I guess), so definitely good to put this into the kerneldoc comments :-) -Daniel > > > -Daniel > > > > > > > > -Daniel > > > > > > > > > > > > > Piles of comments below. > > > > > > > > > > > > > > Cheers, Daniel > > > > > > > > > > > > > > > - Use the small changes to drm_dp_put_port() to add even more > > > > > > > > error > > > > > > > >checking to make misusage of the helpers more obvious. I > > > > > > > > added > > > > > > > > this > > > > > > > >after having to chase down various use-after-free conditions > > > > > > > > that > > > > > > > >started popping up from the new helpers so no one else has to > > > > > > > >troubleshoot that. > > > > > > > > - Move some accidental DRM_DEBUG_KMS() calls to > > > > > > > > DRM_DEBUG_ATOMIC() > > > > > > > > - Update documentation again, note that find/release() should > > > > > > > > both > > > > > > > > not > > > > > > > > be > > > > > > > >called on the same port in a single atomic check phase (but > > > > > > > > multiple > > > > > > > >calls to one or the other is OK) > > > > > > > > > > > > > > > > Changes since v4: > > > > > > > > - Don't skip the atomic checks for VCPI allocations if n
