[Intel-gfx] [PATCH v6 3/6] drm/dp_mst: Start tracking per-port VCPI allocations
Daniel Vetter
daniel at ffwll.ch
Thu Nov 29 09:21:13 UTC 2018
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 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 <lyude at redhat.com>
> > > > > > > > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> > > > > > > > ---
> > > > > > > > drivers/gpu/drm/drm_dp_mst_topology.c | 294
> > > > > > > > +++++++++++++++++++++++---
> > > > > > > > drivers/gpu/drm/i915/intel_display.c | 4 +
> > > > > > > > drivers/gpu/drm/i915/intel_dp_mst.c | 64 +++---
> > > > > > > > include/drm/drm_dp_mst_helper.h | 24 ++-
> > > > > > > > 4 files changed, 319 insertions(+), 67 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > > > > > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > > > > > index 00fbe7a2699d..183d0e832ced 100644
> > > > > > > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > > > > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > > > > > @@ -2617,21 +2617,42 @@ static int drm_dp_init_vcpi(struct
> > > > > > > > drm_dp_mst_topology_mgr *mgr,
> > > > > > > > }
> > > > > > > >
> > > > > > > > /**
> > > > > > > > - * drm_dp_atomic_find_vcpi_slots() - Find and add vcpi slots to
> > > > > > > > the
> > > > > > > > state
> > > > > > > > + * drm_dp_atomic_find_vcpi_slots() - Find and add VCPI slots to
> > > > > > > > the
> > > > > > > > state
> > > > > > > > * @state: global atomic state
> > > > > > > > * @mgr: MST topology manager for the port
> > > > > > > > * @port: port to find vcpi slots for
> > > > > > > > * @pbn: bandwidth required for the mode in PBN
> > > > > > > > *
> > > > > > > > - * RETURNS:
> > > > > > > > - * Total slots in the atomic state assigned for this port or
> > > > > > > > error
> > > > > > > > + * Allocates VCPI slots to @port, replacing any previous VCPI
> > > > > > > > allocations it
> > > > > > > > + * may have had. Any atomic drivers which support MST must call
> > > > > > > > this
> > > > > > > > function
> > > > > > > > + * in their &drm_encoder_helper_funcs.atomic_check() callback
> > > > > > > > to
> > > > > > > > change
> > > > > > > > the
> > > > > > > > + * current VCPI allocation for the new state, but only when
> > > > > > > > + * &drm_crtc_state.mode_changed or
> > > > > > > > &drm_crtc_state.connectors_changed
> > > > > > > > is set
> > > > > > > > + * to ensure compatibility with userspace applications that
> > > > > > > > still
> > > > > > > > use
> > > > > > > > the
> > > > > > > > + * legacy modesetting UAPI.
> > > > > > > > + *
> > > > > > > > + * Allocations set by this function are not checked against the
> > > > > > > > bandwidth
> > > > > > > > + * restraints of @mgr until the driver calls
> > > > > > > > drm_dp_mst_atomic_check().
> > > > > > > > + *
> > > > > > > > + * Additionally, it is OK to call this function multiple times
> > > > > > > > on
> > > > > > > > the
> > > > > > > > same
> > > > > > > > + * @port as needed. It is not OK however, to call this function
> > > > > > > > and
> > > > > > > > + * drm_dp_atomic_release_vcpi_slots() in the same atomic check
> > > > > > > > phase.
> > > > > > > > + *
> > > > > > > > + * See also:
> > > > > > > > + * drm_dp_atomic_release_vcpi_slots()
> > > > > > > > + * drm_dp_mst_atomic_check()
> > > > > > > > + *
> > > > > > > > + * Returns:
> > > > > > > > + * Total slots in the atomic state assigned for this port, or a
> > > > > > > > negative error
> > > > > > > > + * code if the port no longer exists
> > > > > > > > */
> > > > > > > > int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state
> > > > > > > > *state,
> > > > > > > > struct drm_dp_mst_topology_mgr *mgr,
> > > > > > > > struct drm_dp_mst_port *port, int
> > > > > > > > pbn)
> > > > > > > > {
> > > > > > > > struct drm_dp_mst_topology_state *topology_state;
> > > > > > > > - int req_slots;
> > > > > > > > + struct drm_dp_vcpi_allocation *pos, *vcpi = NULL;
> > > > > > > > + int prev_slots, req_slots, ret;
> > > > > > > >
> > > > > > > > topology_state = drm_atomic_get_mst_topology_state(state,
> > > > > > > > mgr);
> > > > > > > > if (IS_ERR(topology_state))
> > > > > > > > @@ -2640,20 +2661,60 @@ int drm_dp_atomic_find_vcpi_slots(struct
> > > > > > > > drm_atomic_state *state,
> > > > > > > > port = drm_dp_get_validated_port_ref(mgr, port);
> > > > > > > > if (port == NULL)
> > > > > > > > return -EINVAL;
> > > > > > > > - req_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
> > > > > > > > - DRM_DEBUG_KMS("vcpi slots req=%d, avail=%d\n",
> > > > > > > > - req_slots, topology_state->avail_slots);
> > > > > > > >
> > > > > > > > - if (req_slots > topology_state->avail_slots) {
> > > > > > > > - drm_dp_put_port(port);
> > > > > > > > - return -ENOSPC;
> > > > > > > > + topology_state->vcpi_allocated = true;
> > > > > > > > +
> > > > > > > > + /* Find the current allocation for this port, if any */
> > > > > > > > + list_for_each_entry(pos, &topology_state->vcpis, next) {
> > > > > > > > + if (pos->port == port) {
> > > > > > > > + vcpi = pos;
> > > > > > > > + prev_slots = vcpi->vcpi;
> > > > > > > > +
> > > > > > > > + /*
> > > > > > > > + * This should never happen, unless the driver
> > > > > > > > tries
> > > > > > > > + * releasing and allocating the same VCPI
> > > > > > > > allocation,
> > > > > > > > + * which is an error
> > > > > > > > + */
> > > > > > > > + if (WARN_ON(!prev_slots)) {
> > > > > > > > + DRM_ERROR("cannot allocate and release
> > > > > > > > VCPI on
> > > > > > > > [MST PORT:%p] in the same state\n",
> > > > > > > > + port);
> > > > > > > > + return -EINVAL;
> > > > > > > > + }
> > > > > > >
> > > > > > > Allowing atomic changes to output state (like disabling it on one
> > > > > > > crtc,
> > > > > > > and then enabling on the other, thus both releasing and re-
> > > > > > > allocating
> > > > > > > the
> > > > > > > vcpi in one go) is kinda the point of atomic. So definitely can't
> > > > > > > EINVAL
> > > > > > > here.
> > > > > > >
> > > > > > > Sounds like we need more testcases in igt?
> > > > > > >
> > > > > > > > +
> > > > > > > > + break;
> > > > > > > > + }
> > > > > > > > }
> > > > > > > > + if (!vcpi)
> > > > > > > > + prev_slots = 0;
> > > > > > > >
> > > > > > > > - topology_state->avail_slots -= req_slots;
> > > > > > > > - DRM_DEBUG_KMS("vcpi slots avail=%d", topology_state-
> > > > > > > > > avail_slots);
> > > > > > > > + req_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
> > > > > > > > +
> > > > > > > > + DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] [MST PORT:%p] VCPI %d ->
> > > > > > > > %d\n",
> > > > > > > > + port->connector->base.id, port->connector-
> > > > > > > > > name,
> > > > > > > > + port, prev_slots, req_slots);
> > > > > > > > +
> > > > > > > > + /* Add the new allocation to the state */
> > > > > > > > + if (!vcpi) {
> > > > > > > > + vcpi = kzalloc(sizeof(*vcpi), GFP_KERNEL);
> > > > > > > > + if (!vcpi) {
> > > > > > > > + ret = -ENOMEM;
> > > > > > > > + goto out;
> > > > > > > > + }
> > > > > > > >
> > > > > > > > + vcpi->port = drm_dp_get_validated_port_ref(mgr, port);
> > > > > > > > + if (!vcpi->port) {
> > > > > > > > + ret = -EINVAL;
> > > > > > > > + kfree(vcpi);
> > > > > > > > + goto out;
> > > > > > > > + }
> > > > > > > > + list_add(&vcpi->next, &topology_state->vcpis);
> > > > > > > > + }
> > > > > > > > + vcpi->vcpi = req_slots;
> > > > > > > > +
> > > > > > > > + ret = req_slots;
> > > > > > > > +out:
> > > > > > > > drm_dp_put_port(port);
> > > > > > > > - return req_slots;
> > > > > > > > + return ret;
> > > > > > > > }
> > > > > > > > EXPORT_SYMBOL(drm_dp_atomic_find_vcpi_slots);
> > > > > > > >
> > > > > > > > @@ -2661,31 +2722,66 @@
> > > > > > > > EXPORT_SYMBOL(drm_dp_atomic_find_vcpi_slots);
> > > > > > > > * drm_dp_atomic_release_vcpi_slots() - Release allocated vcpi
> > > > > > > > slots
> > > > > > > > * @state: global atomic state
> > > > > > > > * @mgr: MST topology manager for the port
> > > > > > > > - * @slots: number of vcpi slots to release
> > > > > > > > + * @port: The port to release the VCPI slots from
> > > > > > > > *
> > > > > > > > - * RETURNS:
> > > > > > > > - * 0 if @slots were added back to &drm_dp_mst_topology_state-
> > > > > > > > > avail_slots or
> > > > > > > > - * negative error code
> > > > > > > > + * Releases any VCPI slots that have been allocated to a port
> > > > > > > > in
> > > > > > > > the
> > > > > > > > atomic
> > > > > > > > + * state. Any atomic drivers which support MST must call this
> > > > > > > > function
> > > > > > > > in
> > > > > > > > + * their &drm_connector_helper_funcs.atomic_check() callback
> > > > > > > > when
> > > > > > > > the
> > > > > > > > + * connector will no longer have VCPI allocated (e.g. because
> > > > > > > > it's
> > > > > > > > CRTC
> > > > > > > > was
> > > > > > > > + * removed) when it had VCPI allocated in the previous atomic
> > > > > > > > state.
> > > > > > > > + *
> > > > > > > > + * It is OK to call this even if @port has been removed from
> > > > > > > > the
> > > > > > > > system.
> > > > > > > > + * Additionally, it is OK to call this function multiple times
> > > > > > > > on
> > > > > > > > the
> > > > > > > > same
> > > > > > > > + * @port as needed. It is not OK however, to call this function
> > > > > > > > and
> > > > > > > > + * drm_dp_atomic_find_vcpi_slots() on the same @port in a
> > > > > > > > single
> > > > > > > > atomic
> > > > > > > > check
> > > > > > > > + * phase.
> > > > > > > > + *
> > > > > > > > + * See also:
> > > > > > > > + * drm_dp_atomic_find_vcpi_slots()
> > > > > > > > + * drm_dp_mst_atomic_check()
> > > > > > > > + *
> > > > > > > > + * Returns:
> > > > > > > > + * 0 if all slots for this port were added back to
> > > > > > > > + * &drm_dp_mst_topology_state.avail_slots or negative error
> > > > > > > > code
> > > > > > > > */
> > > > > > > > int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state
> > > > > > > > *state,
> > > > > > > > struct drm_dp_mst_topology_mgr
> > > > > > > > *mgr,
> > > > > > > > - int slots)
> > > > > > > > + struct drm_dp_mst_port *port)
> > > > > > > > {
> > > > > > > > struct drm_dp_mst_topology_state *topology_state;
> > > > > > > > + struct drm_dp_vcpi_allocation *pos;
> > > > > > > > + bool found = false;
> > > > > > > >
> > > > > > > > topology_state = drm_atomic_get_mst_topology_state(state,
> > > > > > > > mgr);
> > > > > > > > if (IS_ERR(topology_state))
> > > > > > > > return PTR_ERR(topology_state);
> > > > > > > >
> > > > > > > > - /* 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.
> > > > > > > > - */
> > > > > > > > - topology_state->avail_slots += slots;
> > > > > > > > - DRM_DEBUG_KMS("vcpi slots released=%d, avail=%d\n",
> > > > > > > > - slots, topology_state->avail_slots);
> > > > > > > > + list_for_each_entry(pos, &topology_state->vcpis, next) {
> > > > > > > > + if (pos->port == port) {
> > > > > > > > + found = true;
> > > > > > > > + break;
> > > > > > > > + }
> > > > > > > > + }
> > > > > > > > + if (WARN_ON(!found)) {
> > > > > > > > + DRM_ERROR("no VCPI for [MST PORT:%p] found in mst
> > > > > > > > state %p\n",
> > > > > > > > + port, &topology_state->base);
> > > > > > > > + return -EINVAL;
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > + DRM_DEBUG_ATOMIC("[MST PORT:%p] VCPI %d -> 0\n", port, pos-
> > > > > > > > > vcpi);
> > > > > > > > + if (pos->vcpi) {
> > > > > > > > + /*
> > > > > > > > + * This should be safe-as the previous state will
> > > > > > > > still hold a
> > > > > > > > + * reference to this port until we swap states. If
> > > > > > > > it's not,
> > > > > > > > + * the driver has a bookkeeping error
> > > > > > > > + */
> > > > > > > > + if (WARN_ON(drm_dp_put_port(port))) {
> > > > > > >
> > > > > > > The usual way to do this (instead of adding a return value to the
> > > > > > > normal
> > > > > > > _put function) is something like
> > > > > > >
> > > > > > > kref_put(port, function_that_with_just_a_WARNING_DRM_ERROR)
> > > > > > >
> > > > > > > avoids the need for patch 2.
> > > > > > >
> > > > > > > > + DRM_ERROR("cannot allocate and release VCPI on
> > > > > > > > [MST
> > > > > > > > PORT:%p] in the same state\n",
> > > > > > > > + port);
> > > > > > > > + return -EINVAL;
> > > > > > > > + }
> > > > > > > > + pos->vcpi = 0;
> > > > > > > > + }
> > > > > > > >
> > > > > > > > return 0;
> > > > > > > > }
> > > > > > > > @@ -3115,15 +3211,45 @@ static void
> > > > > > > > drm_dp_destroy_connector_work(struct
> > > > > > > > work_struct *work)
> > > > > > > > static struct drm_private_state *
> > > > > > > > drm_dp_mst_duplicate_state(struct drm_private_obj *obj)
> > > > > > > > {
> > > > > > > > - struct drm_dp_mst_topology_state *state;
> > > > > > > > + struct drm_dp_mst_topology_state *state, *old_state =
> > > > > > > > + to_dp_mst_topology_state(obj->state);
> > > > > > > > + struct drm_dp_vcpi_allocation *pos, *vcpi;
> > > > > > > >
> > > > > > > > - state = kmemdup(obj->state, sizeof(*state), GFP_KERNEL);
> > > > > > > > + state = kmemdup(old_state, sizeof(*state), GFP_KERNEL);
> > > > > > > > if (!state)
> > > > > > > > return NULL;
> > > > > > > >
> > > > > > > > __drm_atomic_helper_private_obj_duplicate_state(obj, &state-
> > > > > > > > > base);
> > > > > > > >
> > > > > > > > + INIT_LIST_HEAD(&state->vcpis);
> > > > > > > > + state->vcpi_allocated = false;
> > > > > > > > +
> > > > > > > > + list_for_each_entry(pos, &old_state->vcpis, next) {
> > > > > > > > + /* Prune leftover freed VCPI allocations */
> > > > > > > > + if (!pos->vcpi)
> > > > > > > > + continue;
> > > > > > > > +
> > > > > > > > + vcpi = kmemdup(pos, sizeof(*vcpi), GFP_KERNEL);
> > > > > > > > + if (!vcpi)
> > > > > > > > + goto fail;
> > > > > > > > +
> > > > > > > > + /* Take a plain old kref here and don't validate the
> > > > > > > > port, as
> > > > > > > > + * it's topology might not even exist anymore
> > > > > > > > + */
> > > > > > > > + kref_get(&vcpi->port->kref);
> > > > > > > > + list_add(&vcpi->next, &state->vcpis);
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > return &state->base;
> > > > > > > > +
> > > > > > > > +fail:
> > > > > > > > + list_for_each_entry_safe(pos, vcpi, &state->vcpis, next) {
> > > > > > > > + drm_dp_put_port(pos->port);
> > > > > > > > + kfree(pos);
> > > > > > > > + }
> > > > > > > > + kfree(state);
> > > > > > > > +
> > > > > > > > + return NULL;
> > > > > > > > }
> > > > > > > >
> > > > > > > > static void drm_dp_mst_destroy_state(struct drm_private_obj
> > > > > > > > *obj,
> > > > > > > > @@ -3131,14 +3257,116 @@ static void
> > > > > > > > drm_dp_mst_destroy_state(struct
> > > > > > > > drm_private_obj *obj,
> > > > > > > > {
> > > > > > > > struct drm_dp_mst_topology_state *mst_state =
> > > > > > > > to_dp_mst_topology_state(state);
> > > > > > > > + struct drm_dp_vcpi_allocation *pos, *tmp;
> > > > > > > > +
> > > > > > > > + list_for_each_entry_safe(pos, tmp, &mst_state->vcpis, next) {
> > > > > > > > + /* We only keep references to ports with non-zero
> > > > > > > > VCPIs */
> > > > > > > > + if (pos->vcpi)
> > > > > > > > + drm_dp_put_port(pos->port);
> > > > > > > > + kfree(pos);
> > > > > > > > + }
> > > > > > > >
> > > > > > > > kfree(mst_state);
> > > > > > > > }
> > > > > > > >
> > > > > > > > -static const struct drm_private_state_funcs mst_state_funcs = {
> > > > > > > > +static inline int
> > > > > > > > +drm_dp_mst_atomic_check_topology_state(struct
> > > > > > > > drm_dp_mst_topology_mgr
> > > > > > > > *mgr,
> > > > > > > > + struct
> > > > > > > > drm_dp_mst_topology_state
> > > > > > > > *mst_state)
> > > > > > > > +{
> > > > > > > > + struct drm_dp_vcpi_allocation *vcpi;
> > > > > > > > + struct drm_dp_mst_port *port;
> > > > > > > > + int avail_slots = 63, ret;
> > > > > > > > +
> > > > > > > > + /* There's no possible scenario where releasing VCPI or
> > > > > > > > keeping it the
> > > > > > > > + * same would make the state invalid
> > > > > > > > + */
> > > > > > > > + if (!mst_state->vcpi_allocated) {
> > > > > > >
> > > > > > > Do we need this? If not I'm always in favour of removing code if
> > > > > > > it's
> > > > > > > not
> > > > > > > needed :-)
> > > > > > >
> > > > > > > > + DRM_DEBUG_ATOMIC("[MST MGR:%p] state %p allocates no
> > > > > > > > VCPI,
> > > > > > > > check skipped\n",
> > > > > > > > + mgr, &mst_state->base);
> > > > > > > > + return 0;
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > + list_for_each_entry(vcpi, &mst_state->vcpis, next) {
> > > > > > > > + /* Releasing VCPI is always OK-even if the port is
> > > > > > > > gone */
> > > > > > > > + if (!vcpi->vcpi) {
> > > > > > > > + DRM_DEBUG_ATOMIC("[MST PORT:%p] releases all
> > > > > > > > VCPI
> > > > > > > > slots\n",
> > > > > > > > + vcpi->port);
> > > > > > > > + continue;
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > + port = drm_dp_get_validated_port_ref(mgr, vcpi->port);
> > > > > > > > + if (!port) {
> > > > > > > > + DRM_DEBUG_ATOMIC("[MST PORT:%p] is gone but
> > > > > > > > still has
> > > > > > > > VCPI, cannot add new VCPI\n",
> > > > > > > > + vcpi->port);
> > > > > > > > + return -EINVAL;
> > > > > > > > + }
> > > > > > >
> > > > > > > For find_vcpi we've already done this. For unplugged vcpi, we
> > > > > > > can't do
> > > > > > > this here I think - otherwise if you enable 2 mst outputs on the
> > > > > > > same
> > > > > > > branch, and unplug both, old non-atomic userspace can never ever
> > > > > > > again
> > > > > > > disable them because it can't disable both. The check (to skip
> > > > > > > already
> > > > > > > deallocated ports) above isn't enough to handle that.
> > > > > > >
> > > > > > > > +
> > > > > > > > + DRM_DEBUG_ATOMIC("[MST PORT:%p] requires %d vcpi
> > > > > > > > slots\n",
> > > > > > > > + vcpi->port, vcpi->vcpi);
> > > > > > > > +
> > > > > > > > + avail_slots -= vcpi->vcpi;
> > > > > > > > + if (avail_slots < 0) {
> > > > > > > > + DRM_DEBUG_ATOMIC("[MST PORT:%p] not enough
> > > > > > > > VCPI slots
> > > > > > > > in mst state %p (avail=%d)\n",
> > > > > > > > + vcpi->port, mst_state,
> > > > > > > > + avail_slots + vcpi->vcpi);
> > > > > > > > + ret = -ENOSPC;
> > > > > > > > + goto port_fail;
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > + drm_dp_put_port(port);
> > > > > > > > + }
> > > > > > > > + DRM_DEBUG_ATOMIC("[MST MGR:%p] mst state %p VCPI avail=%d
> > > > > > > > used=%d\n",
> > > > > > > > + mgr, mst_state, avail_slots,
> > > > > > > > + 63 - avail_slots);
> > > > > > > > +
> > > > > > > > + return 0;
> > > > > > > > +port_fail:
> > > > > > > > + drm_dp_put_port(port);
> > > > > > > > + return ret;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +/**
> > > > > > > > + * drm_dp_mst_atomic_check - Check that the new state of an MST
> > > > > > > > topology in an
> > > > > > > > + * atomic update is valid
> > > > > > > > + * @state: Pointer to the new &struct drm_dp_mst_topology_state
> > > > > > > > + *
> > > > > > > > + * Checks the given topology state for an atomic update to
> > > > > > > > ensure
> > > > > > > > that
> > > > > > > > it's
> > > > > > > > + * valid. This includes checking whether there's enough
> > > > > > > > bandwidth
> > > > > > > > to
> > > > > > > > support
> > > > > > > > + * the new VCPI allocations in the atomic update.
> > > > > > > > + *
> > > > > > > > + * Any atomic drivers supporting DP MST must make sure to call
> > > > > > > > this
> > > > > > > > after
> > > > > > > > + * checking the rest of their state in their
> > > > > > > > + * &drm_mode_config_funcs.atomic_check() callback.
> > > > > > > > + *
> > > > > > > > + * See also:
> > > > > > > > + * drm_dp_atomic_find_vcpi_slots()
> > > > > > > > + * drm_dp_atomic_release_vcpi_slots()
> > > > > > > > + *
> > > > > > > > + * Returns:
> > > > > > > > + *
> > > > > > > > + * 0 if the new state is valid, negative error code otherwise.
> > > > > > > > + */
> > > > > > > > +int drm_dp_mst_atomic_check(struct drm_atomic_state *state)
> > > > > > > > +{
> > > > > > > > + struct drm_dp_mst_topology_mgr *mgr;
> > > > > > > > + struct drm_dp_mst_topology_state *mst_state;
> > > > > > > > + int i, ret = 0;
> > > > > > > > +
> > > > > > > > + for_each_new_mst_mgr_in_state(state, mgr, mst_state, i) {
> > > > > > > > + ret = drm_dp_mst_atomic_check_topology_state(mgr,
> > > > > > > > mst_state);
> > > > > > > > + if (ret)
> > > > > > > > + break;
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > + return ret;
> > > > > > > > +}
> > > > > > > > +EXPORT_SYMBOL(drm_dp_mst_atomic_check);
> > > > > > > > +
> > > > > > > > +const struct drm_private_state_funcs
> > > > > > > > drm_dp_mst_topology_state_funcs =
> > > > > > > > {
> > > > > > > > .atomic_duplicate_state = drm_dp_mst_duplicate_state,
> > > > > > > > .atomic_destroy_state = drm_dp_mst_destroy_state,
> > > > > > > > };
> > > > > > > > +EXPORT_SYMBOL(drm_dp_mst_topology_state_funcs);
> > > > > > > >
> > > > > > > > /**
> > > > > > > > * drm_atomic_get_mst_topology_state: get MST topology state
> > > > > > > > @@ -3216,13 +3444,11 @@ int drm_dp_mst_topology_mgr_init(struct
> > > > > > > > drm_dp_mst_topology_mgr *mgr,
> > > > > > > > return -ENOMEM;
> > > > > > > >
> > > > > > > > mst_state->mgr = mgr;
> > > > > > > > -
> > > > > > > > - /* max. time slots - one slot for MTP header */
> > > > > > > > - mst_state->avail_slots = 63;
> > > > > > > > + INIT_LIST_HEAD(&mst_state->vcpis);
> > > > > > > >
> > > > > > > > drm_atomic_private_obj_init(&mgr->base,
> > > > > > > > &mst_state->base,
> > > > > > > > - &mst_state_funcs);
> > > > > > > > + &drm_dp_mst_topology_state_funcs);
> > > > > > > >
> > > > > > > > return 0;
> > > > > > > > }
> > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > > > > > > b/drivers/gpu/drm/i915/intel_display.c
> > > > > > > > index 132e978227fb..8e85e04a5948 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > > > > > @@ -12592,6 +12592,10 @@ static int intel_atomic_check(struct
> > > > > > > > drm_device
> > > > > > > > *dev,
> > > > > > > > "[modeset]" : "[fastset]");
> > > > > > > > }
> > > > > > > >
> > > > > > > > + ret = drm_dp_mst_atomic_check(state);
> > > > > > > > + if (ret)
> > > > > > > > + return ret;
> > > > > > > > +
> > > > > > > > if (any_ms) {
> > > > > > > > ret = intel_modeset_checks(state);
> > > > > > > >
> > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c
> > > > > > > > b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > > > > > > index 4de247ddf05f..be2d1ac7d87d 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > > > > > > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > > > > > > @@ -41,8 +41,13 @@ static bool
> > > > > > > > intel_dp_mst_compute_config(struct
> > > > > > > > intel_encoder *encoder,
> > > > > > > > struct drm_connector *connector = conn_state->connector;
> > > > > > > > void *port = to_intel_connector(connector)->port;
> > > > > > > > struct drm_atomic_state *state = pipe_config->base.state;
> > > > > > > > + struct drm_crtc *crtc = pipe_config->base.crtc;
> > > > > > > > + struct drm_crtc_state *old_crtc_state =
> > > > > > > > + drm_atomic_get_old_crtc_state(state, crtc);
> > > > > > > > + struct drm_crtc_state *new_crtc_state = &pipe_config->base;
> > > > > > > > int bpp;
> > > > > > > > - int lane_count, slots = 0;
> > > > > > > > + int lane_count, slots =
> > > > > > > > + to_intel_crtc_state(old_crtc_state)->dp_m_n.tu;
> > > > > > > > const struct drm_display_mode *adjusted_mode = &pipe_config-
> > > > > > > > > base.adjusted_mode;
> > > > > > > > int mst_pbn;
> > > > > > > > bool constant_n = drm_dp_has_quirk(&intel_dp->desc,
> > > > > > > > @@ -77,8 +82,12 @@ static bool
> > > > > > > > intel_dp_mst_compute_config(struct
> > > > > > > > intel_encoder *encoder,
> > > > > > > > mst_pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock,
> > > > > > > > bpp);
> > > > > > > > pipe_config->pbn = mst_pbn;
> > > > > > > >
> > > > > > > > - /* Zombie connectors can't have VCPI slots */
> > > > > > > > - if (!drm_connector_is_unregistered(connector)) {
> > > > > > > > + /* Only change VCPI allocation on actual mode changes, to
> > > > > > > > prevent us
> > > > > > > > + * from trying to allocate VCPI to ports that no longer exist
> > > > > > > > when we
> > > > > > > > + * may just be trying to disable DPMS on them
> > > > > > > > + */
> > > > > > > > + if (new_crtc_state->mode_changed ||
> > > > > > > > + new_crtc_state->connectors_changed) {
> > > > > > >
> > > > > > > Why exactly is this needed? We only call compute_config on the
> > > > > > > enocoder
> > > > > > > when enabling an output. Not when disabling one. Similar for the
> > > > > > > atomic
> > > > > > > helpers and the various encoder callbacks (you have the same
> > > > > > > change in
> > > > > > > the
> > > > > > > nouveau code). Did this actually blow up while testing?
> > > > > > >
> > > > > > > > slots = drm_dp_atomic_find_vcpi_slots(state,
> > > > > > > > &intel_dp-
> > > > > > > > > mst_mgr,
> > > > > > > > port,
> > > > > > > > @@ -107,36 +116,39 @@ static bool
> > > > > > > > intel_dp_mst_compute_config(struct
> > > > > > > > intel_encoder *encoder,
> > > > > > > > return true;
> > > > > > > > }
> > > > > > > >
> > > > > > > > -static int intel_dp_mst_atomic_check(struct drm_connector
> > > > > > > > *connector,
> > > > > > > > - struct drm_connector_state *new_conn_state)
> > > > > > > > +static int
> > > > > > > > +intel_dp_mst_atomic_check(struct drm_connector *connector,
> > > > > > > > + struct drm_connector_state *new_conn_state)
> > > > > > > > {
> > > > > > > > + struct intel_connector *intel_connector =
> > > > > > > > + to_intel_connector(connector);
> > > > > > > > + struct drm_dp_mst_topology_mgr *mgr =
> > > > > > > > + &intel_connector->mst_port->mst_mgr;
> > > > > > > > + struct drm_dp_mst_port *port = intel_connector->port;
> > > > > > > > struct drm_atomic_state *state = new_conn_state->state;
> > > > > > > > - struct drm_connector_state *old_conn_state;
> > > > > > > > - struct drm_crtc *old_crtc;
> > > > > > > > - struct drm_crtc_state *crtc_state;
> > > > > > > > - int slots, ret = 0;
> > > > > > > > + struct drm_connector_state *old_conn_state =
> > > > > > > > + drm_atomic_get_old_connector_state(state, connector);
> > > > > > > > + struct drm_crtc *old_crtc = old_conn_state->crtc,
> > > > > > > > + *new_crtc = new_conn_state->crtc;
> > > > > > > > + struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> > > > > > > >
> > > > > > > > - old_conn_state = drm_atomic_get_old_connector_state(state,
> > > > > > > > connector);
> > > > > > > > - old_crtc = old_conn_state->crtc;
> > > > > > >
> > > > > > > Bikeshed: Reworking all the above hides the real changes quite a
> > > > > > > bit.
> > > > > > >
> > > > > > > > if (!old_crtc)
> > > > > > > > - return ret;
> > > > > > > > + return 0;
> > > > > > > >
> > > > > > > > - crtc_state = drm_atomic_get_new_crtc_state(state, old_crtc);
> > > > > > > > - slots = to_intel_crtc_state(crtc_state)->dp_m_n.tu;
> > > > > > > > - if (drm_atomic_crtc_needs_modeset(crtc_state) && slots > 0) {
> > > > > > > > - struct drm_dp_mst_topology_mgr *mgr;
> > > > > > > > - struct drm_encoder *old_encoder;
> > > > > > > > + old_crtc_state = drm_atomic_get_old_crtc_state(state,
> > > > > > > > old_crtc);
> > > > > > > > + if (!old_crtc_state ||
> > > > > > >
> > > > > > > !old_crtc_state would be a framework bug. Just remove that check.
> > > > > > >
> > > > > > > > + !to_intel_crtc_state(old_crtc_state)->dp_m_n.tu)
> > > > > > > > + return 0;
> > > > > > >
> > > > > > > Hm, why do we need this? Having a DP port previously enabled with
> > > > > > > a 0
> > > > > > > tu
> > > > > > > sounds like a pretty big driver bug.
> > > > > > > > - old_encoder = old_conn_state->best_encoder;
> > > > > > > > - mgr = &enc_to_mst(old_encoder)->primary->dp.mst_mgr;
> > > > > > > > + /* If we switched CRTCs, clear the previous one's allocation
> > > > > > > > */
> > > > > > > > + new_crtc_state = drm_atomic_get_new_crtc_state(state,
> > > > > > > > old_crtc);
> > > > > > > > + if (new_crtc_state->connectors_changed && !new_crtc_state-
> > > > > > > > > enable)
> > > > > > > > + to_intel_crtc_state(new_crtc_state)->dp_m_n.tu = 0;
> > > > > > >
> > > > > > > No idea why we need to clear this here? Shouldn't we just clear
> > > > > > > this
> > > > > > > unconditionally if we release the allocation.
> > > > > > >
> > > > > > > Also naming convention, I'd call this old_crtc_new_state, and
> > > > > > > maybe
> > > > > > > old_crtc_old_state. To make it clear these are the old/new states
> > > > > > > for
> > > > > > > old_crtc. Instead of old/new state for the new crtc.
> > > > > > >
> > > > > > > > - ret = drm_dp_atomic_release_vcpi_slots(state, mgr,
> > > > > > > > slots);
> > > > > > > > - if (ret)
> > > > > > > > - DRM_DEBUG_KMS("failed releasing %d vcpi
> > > > > > > > slots:%d\n",
> > > > > > > > slots, ret);
> > > > > > > > - else
> > > > > > > > - to_intel_crtc_state(crtc_state)->dp_m_n.tu =
> > > > > > > > 0;
> > > > > > > > - }
> > > > > > > > - return ret;
> > > > > > > > + if (new_crtc)
> > > > > > > > + return 0;
> > > > > > >
> > > > > > > Not sure why we shouldn't release this? If we handle this in
> > > > > > > helpers,
> > > > > > > it
> > > > > > > would remove piles of special casing code here I think. Plus
> > > > > > > remove
> > > > > > > special code in the hlpers.
> > > > > > >
> > > > > > >
> > > > > > > > +
> > > > > > > > + return drm_dp_atomic_release_vcpi_slots(state, mgr, port);
> > > > > > > > }
> > > > > > > >
> > > > > > > > static void intel_mst_disable_dp(struct intel_encoder *encoder,
> > > > > > > > diff --git a/include/drm/drm_dp_mst_helper.h
> > > > > > > > b/include/drm/drm_dp_mst_helper.h
> > > > > > > > index 3faceb66f5cb..2392e12b6bce 100644
> > > > > > > > --- a/include/drm/drm_dp_mst_helper.h
> > > > > > > > +++ b/include/drm/drm_dp_mst_helper.h
> > > > > > > > @@ -406,9 +406,16 @@ struct drm_dp_payload {
> > > > > > > >
> > > > > > > > #define to_dp_mst_topology_state(x) container_of(x, struct
> > > > > > > > drm_dp_mst_topology_state, base)
> > > > > > > >
> > > > > > > > +struct drm_dp_vcpi_allocation {
> > > > > > >
> > > > > > > I think a comment here that this pointer holds a full reference
> > > > > > > (even
> > > > > > > past
> > > > > > > when the connector is unplugged) would be good. Otherwise the loop
> > > > > > > in
> > > > > > > release_vcpi would be buggy.
> > > > > > >
> > > > > > > > + struct drm_dp_mst_port *port;
> > > > > > > > + int vcpi;
> > > > > > > > + struct list_head next;
> > > > > > > > +};
> > > > > > > > +
> > > > > > > > struct drm_dp_mst_topology_state {
> > > > > > > > struct drm_private_state base;
> > > > > > > > - int avail_slots;
> > > > > > > > + struct list_head vcpis;
> > > > > > > > + u8 vcpi_allocated;
> > > > > > > > struct drm_dp_mst_topology_mgr *mgr;
> > > > > > > > };
> > > > > > > >
> > > > > > > > @@ -619,14 +626,17 @@ void
> > > > > > > > drm_dp_mst_topology_mgr_suspend(struct
> > > > > > > > drm_dp_mst_topology_mgr *mgr);
> > > > > > > > int drm_dp_mst_topology_mgr_resume(struct
> > > > > > > > drm_dp_mst_topology_mgr
> > > > > > > > *mgr);
> > > > > > > > struct drm_dp_mst_topology_state
> > > > > > > > *drm_atomic_get_mst_topology_state(struct drm_atomic_state
> > > > > > > > *state,
> > > > > > > > st
> > > > > > > > ruct
> > > > > > > > drm_dp_mst_topology_mgr *mgr);
> > > > > > > > -int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state
> > > > > > > > *state,
> > > > > > > > - struct drm_dp_mst_topology_mgr *mgr,
> > > > > > > > - struct drm_dp_mst_port *port, int
> > > > > > > > pbn);
> > > > > > > > -int drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state
> > > > > > > > *state,
> > > > > > > > - struct drm_dp_mst_topology_mgr
> > > > > > > > *mgr,
> > > > > > > > - int slots);
> > > > > > > > +int __must_check
> > > > > > > > +drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state,
> > > > > > > > + struct drm_dp_mst_topology_mgr *mgr,
> > > > > > > > + struct drm_dp_mst_port *port, int pbn);
> > > > > > > > +int __must_check
> > > > > > > > +drm_dp_atomic_release_vcpi_slots(struct drm_atomic_state
> > > > > > > > *state,
> > > > > > > > + struct drm_dp_mst_topology_mgr *mgr,
> > > > > > > > + struct drm_dp_mst_port *port);
> > > > > > > > int drm_dp_send_power_updown_phy(struct drm_dp_mst_topology_mgr
> > > > > > > > *mgr,
> > > > > > > > struct drm_dp_mst_port *port, bool
> > > > > > > > power_up);
> > > > > > > > +int __must_check drm_dp_mst_atomic_check(struct
> > > > > > > > drm_atomic_state
> > > > > > > > *state);
> > > > > > > >
> > > > > > > > extern const struct drm_private_state_funcs
> > > > > > > > drm_dp_mst_topology_state_funcs;
> > > > > > > >
> > > > > > > > --
> > > > > > > > 2.19.1
> > > > > > > >
> > > > > > >
> > > > > > > --
> > > > > > > Daniel Vetter
> > > > > > > Software Engineer, Intel Corporation
> > > > > > > http://blog.ffwll.ch
> > > > > --
> > > > > Cheers,
> > > > > Lyude Paul
> > > > >
> > > --
> > > Cheers,
> > > Lyude Paul
> > >
> --
> Cheers,
> Lyude Paul
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the Intel-gfx
mailing list