Nouveau - Nov 2018 - [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 <lyude at redhat.com>
> > > 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
> > definitely
> > the right solution to go with. It'll end up decomplicating a lot
of this
> > :)
>
> Since it looks like we'll use the kfree_ref quite a bit, wrappers for
that
> would be useful. Still not sure what to call those, best I could come up
> with is dp_port_malloc_get/put. Plus a comment explaining what it does.
> It's going to be a bit of wtf-is-this no matter what :-/
> -Daniel
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
>
> > > -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

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 <lyude at redhat.com>
> > > > 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
> > > definitely
> > > the right solution to go with. It'll end up decomplicating a
lot of this
> > > :)
> >
> > Since it looks like we'll use the kfree_ref quite a bit, wrappers
for that
> > would be useful. Still not sure what to call those, best I could come
up
> > with is dp_port_malloc_get/put. Plus a comment explaining what it
does.
> > It's going to be a bit of wtf-is-this no matter what :-/
> > -Daniel
> 
> 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 :-/
-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
> 
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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
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 <lyude at redhat.com>
> > > > > 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
> > > > definitely
> > > > the right solution to go with. It'll end up
decomplicating a lot of
> > > > this
> > > > :)
> > > 
> > > Since it looks like we'll use the kfree_ref quite a bit,
wrappers for
> > > that
> > > would be useful. Still not sure what to call those, best I could
come up
> > > with is dp_port_malloc_get/put. Plus a comment explaining what it
does.
> > > It's going to be a bit of wtf-is-this no matter what :-/
> > > -Daniel
> > 
> > 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 :-/
Well if we exposed get/put functions for this publically, I'd probably just
call them drm_dp_get_port() and drm_dp_put_port() since it wouldn't make
sense
to have differentiated naming if we're only exposing one of the krefs
publically (e.g. the malloc kref).

Also: I've been thinking about shortening some of the names of the MST
helper
functions for a while. I don't think we need to say drm_dp_mst_topology_mgr
everywhere :)
> -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

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 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