Nouveau - Nov 2018 - [PATCH v6 3/6] drm/dp_mst: Start tracking per-port VCPI allocations

On Mon, 2018-11-26 at 22:22 +0100, Daniel Vetter wrote:
> On Mon, Nov 26, 2018 at 10:04:21PM +0100, Daniel Vetter wrote:
> > On Thu, Nov 15, 2018 at 07:50:05PM -0500, Lyude Paul wrote:
> > > There has been a TODO waiting for quite a long time in
> > > drm_dp_mst_topology.c:
> > > 
> > > 	/* We cannot rely on port->vcpi.num_slots to update
> > > 	 * topology_state->avail_slots as the port may not exist if
the parent
> > > 	 * branch device was unplugged. This should be fixed by tracking
> > > 	 * per-port slot allocation in drm_dp_mst_topology_state instead
of
> > > 	 * depending on the caller to tell us how many slots to release.
> > > 	 */
> > > 
> > > That's not the only reason we should fix this: forcing the
driver to
> > > track the VCPI allocations throughout a state's atomic check
is
> > > error prone, because it means that extra care has to be taken
with the
> > > order that drm_dp_atomic_find_vcpi_slots() and
> > > drm_dp_atomic_release_vcpi_slots() are called in in order to
ensure
> > > idempotency. Currently the only driver actually using these
helpers,
> > > i915, doesn't even do this correctly: multiple
->best_encoder() checks
> > > with i915's current implementation would not be idempotent
and would
> > > over-allocate VCPI slots, something I learned trying to implement
> > > fallback retraining in MST.
> > > 
> > > So: simplify this whole mess, and teach
drm_dp_atomic_find_vcpi_slots()
> > > and drm_dp_atomic_release_vcpi_slots() to track the VCPI
allocations for
> > > each port. This allows us to ensure idempotency without having to
rely
> > > on the driver as much. Additionally: the driver doesn't need
to do any
> > > kind of VCPI slot tracking anymore if it doesn't need it for
it's own
> > > internal state.
> > > 
> > > Additionally; this adds a new drm_dp_mst_atomic_check() helper
which
> > > must be used by atomic drivers to perform validity checks for the
new
> > > VCPI allocations incurred by a state.
> > > 
> > > Also: update the documentation and make it more obvious that
these
> > > /must/ be called by /all/ atomic drivers supporting MST.
> > > 
> > > Changes since v6:
> > >  - Keep a kref to all of the ports we have allocations on. This
required
> > >    a good bit of changing to when we call
drm_dp_find_vcpi_slots(),
> > >    mainly that we need to ensure that we only redo VCPI
allocations on
> > >    actual mode or CRTC changes, not crtc_state->active
changes.
> > >    Additionally, we no longer take the registration of the DRM
connector
> > >    for each port into account because so long as we have a kref
to the
> > >    port in the new or previous atomic state, the connector will
stay
> > >    registered.
> > 
> > I write an entire pile of small nitpits (still included most of them
> > below), until I realized this here wont work. Delaying the call to
destroy
> > the connector (well, unregister it really) wreaks the design we've
come up
> > with thus far, resulting in most of my comments here.
> > 
> > Instead, all we need to do is delay the kfree(port) at the bottom of
> > drm_dp_destroy_port(). The vcpi allocation structure _only_ needs the
> > pointer value to stay valid, as a lookup key. It doesn't care at
all about
> > anything actually stored in there. So the only thing we need to delay
is
> > the kfree. I think the simplest way to achieve this is to add a 2nd
kref
> > (port->kfree_ref or something like that), with on reference held by
the
> > port itself (released in drm_dp_destroy_port), and the other one held
> > as-needed by the vcpi allocation lists.
> > 
> > I think if we go with this design instead of retrofitting a semantic
> > change of the port lifetime itself, all the complications I complain
about
> > below should disappear.
> 
> In the above I meant drm_dp_destroy_port or drm_dp_destroy_connector_work.
> 
> Aside: I think creating a kref for the final kfree would also solve a
> bunch of other issues in a much neater way: In
> 
> commit f038c5b99fc1332f558b495d136d4f433ee65caa
> Author: Lyude Paul <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
:)
> -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,
> > >  								    struct
> > > 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

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
> > -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,
> > > >  								    struct
> > > > 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
> 
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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