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 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@xxxxxxxxxx>
> > > > > > > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > > > > > > ---
> > > > > > > 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
