Nouveau - Nov 2018 - [PATCH v3 2/5] drm/dp_mst: Start tracking per-port VCPI allocations

On Wed, 2018-11-07 at 16:23 -0500, Lyude Paul wrote:
> On Wed, 2018-11-07 at 21:59 +0100, Daniel Vetter wrote:
> > On Tue, Nov 06, 2018 at 08:21:14PM -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 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 | 222
++++++++++++++++++++++----
> > >  drivers/gpu/drm/i915/intel_display.c  |   4 +
> > >  drivers/gpu/drm/i915/intel_dp_mst.c   |  31 ++--
> > >  include/drm/drm_dp_mst_helper.h       |  23 ++-
> > >  4 files changed, 225 insertions(+), 55 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > index 8c3cfac437f4..74823afb262e 100644
> > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > @@ -2614,21 +2614,34 @@ 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. The allocations
are not
> > > checked
> > > + * against the bandwidth restraints of @mgr until the driver
calls
> > > + * drm_dp_mst_atomic_check().
> > > + *
> > > + * 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))
> > > @@ -2637,20 +2650,41 @@ 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;
> > > +	/* 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;
> > > +			break;
> > > +		}
> > >  	}
> > > +	if (!vcpi)
> > > +		prev_slots = 0;
> > > +
> > > +	req_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
> > >  
> > > -	topology_state->avail_slots -= req_slots;
> > > -	DRM_DEBUG_KMS("vcpi slots avail=%d",
topology_state->avail_slots);
> > > +	DRM_DEBUG_KMS("[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 = port;
> > > +		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);
> > >  
> > > @@ -2658,32 +2692,50 @@
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).
> > > + *
> > > + * It is OK to call this even if @port has been removed from the
> > > system,
> > > in
> > > + * which case it will just amount to a no-op.
> > > + *
> > > + * 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 *tmp, *pos;
> > >  
> > >  	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_safe(pos, tmp,
&topology_state->vcpis, next) {
> > > +		if (pos->port == port) {
> > > +			list_del(&pos->next);
> > > +			DRM_DEBUG_KMS("[MST PORT:%p] vcpi %d -> 0\n",
> > > +				      port, pos->vcpi);
> > >  
> > > +			kfree(pos);
> > > +			return 0;
> > > +		}
> > > +	}
> > > +
> > > +	/* If no allocation was found, all that means is that the port
was
> > > +	 * destroyed since the last atomic commit. That's OK!
> > > +	 */
> > 
> > This case here has me a bit worried. I thought with your changes
we'll
> > always have the port pointer still around, to be able to clean things
up
> > properly. If this is not the case then we'd potentially leak the
vcpi
> > allocation for an unplugged port.
> > 
> > Otoh if the driver releases the allocation for a port twice it's
probably
> > mixed up it's own book-keeping. We should WARN about that case.
> > 
> > This is kinda fallout from your change to not prune allocations on
> > duplicate, and I think will further clean up how this is handled.
> > 
> > So with the comment removed and replaced by a WARNING(1,"no vcpi
> > allocation found for port %p\n", port) or something like that
this should
> > be solid.
> 
> Hm; that comment is leftover from the fallout of moving the port
> verification
> logic out of the state duplicate functions, but I don't think this
would
> nessecarily be a bookkeeping error. If we're going to support fallback
> link retraining, we'll need to make sure that every driver's
> drm_connector->atomic_check() function is idempotent so that if the
> atomic helpers have to pull in more CRTCs to perform modesets on then
> userspace originally added, the second check on the DRM connector
> doesn't cause any issues. Remember: if we're trying to retrain an
MST
> topology at a lower link rate then before, that means changing the PBN
> divisor which in turn means that the only valid actions that can be
> taken are recalculating every active VCPI allocation at once in order to
> prevent state inconsistencies, or disabling all of the displays (since
> going from >0 VCPI to 0 VCPI should always be safe).
> 
> We /could/ just leave this up to the driver to do the bookkeeping on
> this, but that seems rather redundant. We could add extra bookkeeping in
> the atomic check though so that instead of just deleting the structs for
> each VCPI allocation when they go to 0, we instead just set them to 0
> then free them somewhere else.
jfyi; I think it would go something like this:

drm_dp_atomic_release_vcpi_slots():

for each vcpi:
  if vcpi.port == port:
    vcpi.vcpi = 0
    return 0;

drm_error("No previous VCPI allocation found! Help!!!\n");
return -EINVAL;

Then we'd need to add something to prune the VCPI allocations in the state,
which the driver would call after all atomic checks/mode validations have been
performed, including drm_dp_mst_atomic_check()

drm_dp_mst_atomic_check_finish():
  for each vcpi:
    if vcpi.vcpi == 0:
      del vcpi

Then, that way we've made sure that multiple VCPI frees in the same check
are
OK, but freeing VCPI that never existed actually throws
errors
> 
> > 
> > >  	return 0;
> > >  }
> > >  EXPORT_SYMBOL(drm_dp_atomic_release_vcpi_slots);
> > > @@ -3112,15 +3164,34 @@ 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);
> > > +
> > > +	list_for_each_entry(pos, &old_state->vcpis, next) {
> > > +		vcpi = kmemdup(pos, sizeof(*vcpi), GFP_KERNEL);
> > > +		if (!vcpi)
> > > +			goto fail_alloc;
> > > +
> > > +		list_add(&vcpi->next, &state->vcpis);
> > > +	}
> > > +
> > >  	return &state->base;
> > > +
> > > +fail_alloc:
> > > +	list_for_each_entry_safe(pos, vcpi, &state->vcpis, next)
> > > +		kfree(pos);
> > > +	kfree(state);
> > > +
> > > +	return NULL;
> > >  }
> > >  
> > >  static void drm_dp_mst_destroy_state(struct drm_private_obj
*obj,
> > > @@ -3128,14 +3199,99 @@ 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)
> > > +		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;
> > > +
> > > +	list_for_each_entry(vcpi, &mst_state->vcpis, next) {
> > > +		/* Ports that no longer exist shouldn't be counted towards
the
> > > +		 * bandwidth requirements for the state, so skip them
> > > +		 */
> > 
> > Scenario:
> > - 2 mst sinks on a single host side port.
> > - 1st connector is active, 2nd connector is off.
> > - 1st connector is hot-unplugged, but userspace hasn't realized
this yet,
> >   so not yet come around to do the disabling modeset.
> > - At the same time userspace decides to enable the 2nd output, which
> >   forces a modeset and pulls the topology state in.
> > - This function here decides to only look at the vcpi allocation for
the
> >   2nd connector because the first one is gone already.
> > - Unfortunately lighting up both connectors at the same time isn't
> >   possible, because too many vcpi. But since userspace didn't ask
for a
> >   modeset on the now unplugged first connector.
> > -> Modeset goes boom because we enable too many vcpi.
> > 
> > But with the change in how we release vcpi allocations there's no
need to
> > ignore disconnected stuff here, and we should be able to check vcpi
limits
> > by just checking all of them.
> > 
> > I didn't see anything else serious, so with the above two things
resolved
> > this has my Reviewed-by: Daniel Vetter <daniel.vetter at
ffwll.ch>
> > 
> > One bikeshed below, because I need to prove I scrolled to the end :-)
> > 
> > > +		port = drm_dp_get_validated_port_ref(mgr, vcpi->port);
> > > +		if (!port) {
> > > +			DRM_DEBUG_ATOMIC("[MST PORT:%p] is stale, skipping
> > > checks\n",
> > > +					 vcpi->port);
> > > +			continue;
> > > +		}
> > > +
> > > +		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
> > > @@ -3213,13 +3369,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 bbf8ca21a7a2..8749cf61a6ac 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -12553,6 +12553,10 @@ static int intel_atomic_check(struct
drm_device
> > > *dev,
> > >  				       "[modeset]" : "[fastset]");
> > >  	}
> > >  
> > > +	ret = drm_dp_mst_atomic_check(state);
> > > +	if (ret)
> > > +		return ret;
> > 
> > Complete bikeshed, so feel free to ignore: I'd stuff this into
> > intel_modeset_checks(). Feel free to ignore.
> > 
> > Cheers, Daniel
> > 
> > > +
> > >  	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 8b71d64ebd9d..aaf904738b78 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > @@ -114,28 +114,31 @@ static int intel_dp_mst_atomic_check(struct
> > > drm_connector *connector,
> > >  	struct drm_connector_state *old_conn_state;
> > >  	struct drm_crtc *old_crtc;
> > >  	struct drm_crtc_state *crtc_state;
> > > -	int slots, ret = 0;
> > > +	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;
> > > +	int ret = 0;
> > >  
> > >  	old_conn_state = drm_atomic_get_old_connector_state(state,
connector);
> > >  	old_crtc = old_conn_state->crtc;
> > >  	if (!old_crtc)
> > >  		return ret;
> > >  
> > > -	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;
> > > +	/* Free VCPI, since compute_config() won't be run */
> > > +	if (!new_conn_state->crtc) {
> > > +		crtc_state = drm_atomic_get_new_crtc_state(state, old_crtc);
> > >  
> > > -		old_encoder = old_conn_state->best_encoder;
> > > -		mgr = &enc_to_mst(old_encoder)->primary->dp.mst_mgr;
> > > -
> > > -		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;
> > > +		ret = drm_dp_atomic_release_vcpi_slots(state, mgr, port);
> > > +		if (ret) {
> > > +			DRM_DEBUG_KMS("failed releasing vcpi slots: %d\n",
> > > +				      ret);
> > > +			return ret;
> > > +		}
> > > +		to_intel_crtc_state(crtc_state)->dp_m_n.tu = 0;
> > >  	}
> > > +
> > >  	return ret;
> > >  }
> > >  
> > > diff --git a/include/drm/drm_dp_mst_helper.h
> > > b/include/drm/drm_dp_mst_helper.h
> > > index 3faceb66f5cb..08b67948b8f9 100644
> > > --- a/include/drm/drm_dp_mst_helper.h
> > > +++ b/include/drm/drm_dp_mst_helper.h
> > > @@ -406,9 +406,15 @@ 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 {
> > > +	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;
> > >  	struct drm_dp_mst_topology_mgr *mgr;
> > >  };
> > >  
> > > @@ -619,14 +625,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
> > > 
-- 
Cheers,
	Lyude Paul

On Wed, Nov 07, 2018 at 04:39:57PM -0500, Lyude Paul
wrote:
> On Wed, 2018-11-07 at 16:23 -0500, Lyude Paul wrote:
> > On Wed, 2018-11-07 at 21:59 +0100, Daniel Vetter wrote:
> > > On Tue, Nov 06, 2018 at 08:21:14PM -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 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 | 222
++++++++++++++++++++++----
> > > >  drivers/gpu/drm/i915/intel_display.c  |   4 +
> > > >  drivers/gpu/drm/i915/intel_dp_mst.c   |  31 ++--
> > > >  include/drm/drm_dp_mst_helper.h       |  23 ++-
> > > >  4 files changed, 225 insertions(+), 55 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > index 8c3cfac437f4..74823afb262e 100644
> > > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > @@ -2614,21 +2614,34 @@ 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. The
allocations are not
> > > > checked
> > > > + * against the bandwidth restraints of @mgr until the
driver calls
> > > > + * drm_dp_mst_atomic_check().
> > > > + *
> > > > + * 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))
> > > > @@ -2637,20 +2650,41 @@ 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;
> > > > +	/* 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;
> > > > +			break;
> > > > +		}
> > > >  	}
> > > > +	if (!vcpi)
> > > > +		prev_slots = 0;
> > > > +
> > > > +	req_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
> > > >  
> > > > -	topology_state->avail_slots -= req_slots;
> > > > -	DRM_DEBUG_KMS("vcpi slots avail=%d",
topology_state->avail_slots);
> > > > +	DRM_DEBUG_KMS("[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 = port;
> > > > +		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);
> > > >  
> > > > @@ -2658,32 +2692,50 @@
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).
> > > > + *
> > > > + * It is OK to call this even if @port has been removed
from the
> > > > system,
> > > > in
> > > > + * which case it will just amount to a no-op.
> > > > + *
> > > > + * 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 *tmp, *pos;
> > > >  
> > > >  	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_safe(pos, tmp,
&topology_state->vcpis, next) {
> > > > +		if (pos->port == port) {
> > > > +			list_del(&pos->next);
> > > > +			DRM_DEBUG_KMS("[MST PORT:%p] vcpi %d ->
0\n",
> > > > +				      port, pos->vcpi);
> > > >  
> > > > +			kfree(pos);
> > > > +			return 0;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	/* If no allocation was found, all that means is that the
port was
> > > > +	 * destroyed since the last atomic commit. That's OK!
> > > > +	 */
> > > 
> > > This case here has me a bit worried. I thought with your changes
we'll
> > > always have the port pointer still around, to be able to clean
things up
> > > properly. If this is not the case then we'd potentially leak
the vcpi
> > > allocation for an unplugged port.
> > > 
> > > Otoh if the driver releases the allocation for a port twice
it's probably
> > > mixed up it's own book-keeping. We should WARN about that
case.
> > > 
> > > This is kinda fallout from your change to not prune allocations
on
> > > duplicate, and I think will further clean up how this is handled.
> > > 
> > > So with the comment removed and replaced by a WARNING(1,"no
vcpi
> > > allocation found for port %p\n", port) or something like
that this should
> > > be solid.
> > 
> > Hm; that comment is leftover from the fallout of moving the port
> > verification
> > logic out of the state duplicate functions, but I don't think this
would
> > nessecarily be a bookkeeping error. If we're going to support
fallback
> > link retraining, we'll need to make sure that every driver's
> > drm_connector->atomic_check() function is idempotent so that if the
> > atomic helpers have to pull in more CRTCs to perform modesets on then
> > userspace originally added, the second check on the DRM connector
> > doesn't cause any issues. Remember: if we're trying to retrain
an MST
> > topology at a lower link rate then before, that means changing the PBN
> > divisor which in turn means that the only valid actions that can be
> > taken are recalculating every active VCPI allocation at once in order
to
> > prevent state inconsistencies, or disabling all of the displays (since
> > going from >0 VCPI to 0 VCPI should always be safe).
> > 
> > We /could/ just leave this up to the driver to do the bookkeeping on
> > this, but that seems rather redundant. We could add extra bookkeeping
in
> > the atomic check though so that instead of just deleting the structs
for
> > each VCPI allocation when they go to 0, we instead just set them to 0
> > then free them somewhere else.
> 
> jfyi; I think it would go something like this:
> 
> drm_dp_atomic_release_vcpi_slots():
> 
> for each vcpi:
>   if vcpi.port == port:
>     vcpi.vcpi = 0
>     return 0;
> 
> drm_error("No previous VCPI allocation found! Help!!!\n");
> return -EINVAL;
> 
> Then we'd need to add something to prune the VCPI allocations in the
state,
> which the driver would call after all atomic checks/mode validations have
been
> performed, including drm_dp_mst_atomic_check()
> 
> drm_dp_mst_atomic_check_finish():
>   for each vcpi:
>     if vcpi.vcpi == 0:
>       del vcpi
> 
> Then, that way we've made sure that multiple VCPI frees in the same
check are
> OK, but freeing VCPI that never existed actually throws errors
Not sure that's needed at all. The way link training works is we just ask
userspace to do the heavy lifting for us, by issuing a new modeset. If
that means mode pruning, we also ask userspace to do the heavy lifting for
us, again by issuing a new modeset. Kernel never needs to retrain if that
means recomputing state stuff or pruning modes or other fun problems. It
doesn't even need to retrain if that means shutting down a bunch of links,
it can just punt that to userspace and let it retrain at a convenient
time.

Of course that retrain might then result in a modeset, which might fail if
the connector is entirely gone by then.

Otoh I might just be talking nonsense right now, it's a bit late :-)

Cheers, Daniel

> > 
> > > 
> > > >  	return 0;
> > > >  }
> > > >  EXPORT_SYMBOL(drm_dp_atomic_release_vcpi_slots);
> > > > @@ -3112,15 +3164,34 @@ 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);
> > > > +
> > > > +	list_for_each_entry(pos, &old_state->vcpis, next) {
> > > > +		vcpi = kmemdup(pos, sizeof(*vcpi), GFP_KERNEL);
> > > > +		if (!vcpi)
> > > > +			goto fail_alloc;
> > > > +
> > > > +		list_add(&vcpi->next, &state->vcpis);
> > > > +	}
> > > > +
> > > >  	return &state->base;
> > > > +
> > > > +fail_alloc:
> > > > +	list_for_each_entry_safe(pos, vcpi, &state->vcpis,
next)
> > > > +		kfree(pos);
> > > > +	kfree(state);
> > > > +
> > > > +	return NULL;
> > > >  }
> > > >  
> > > >  static void drm_dp_mst_destroy_state(struct drm_private_obj
*obj,
> > > > @@ -3128,14 +3199,99 @@ 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)
> > > > +		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;
> > > > +
> > > > +	list_for_each_entry(vcpi, &mst_state->vcpis, next)
{
> > > > +		/* Ports that no longer exist shouldn't be counted
towards the
> > > > +		 * bandwidth requirements for the state, so skip them
> > > > +		 */
> > > 
> > > Scenario:
> > > - 2 mst sinks on a single host side port.
> > > - 1st connector is active, 2nd connector is off.
> > > - 1st connector is hot-unplugged, but userspace hasn't
realized this yet,
> > >   so not yet come around to do the disabling modeset.
> > > - At the same time userspace decides to enable the 2nd output,
which
> > >   forces a modeset and pulls the topology state in.
> > > - This function here decides to only look at the vcpi allocation
for the
> > >   2nd connector because the first one is gone already.
> > > - Unfortunately lighting up both connectors at the same time
isn't
> > >   possible, because too many vcpi. But since userspace didn't
ask for a
> > >   modeset on the now unplugged first connector.
> > > -> Modeset goes boom because we enable too many vcpi.
> > > 
> > > But with the change in how we release vcpi allocations
there's no need to
> > > ignore disconnected stuff here, and we should be able to check
vcpi limits
> > > by just checking all of them.
> > > 
> > > I didn't see anything else serious, so with the above two
things resolved
> > > this has my Reviewed-by: Daniel Vetter <daniel.vetter at
ffwll.ch>
> > > 
> > > One bikeshed below, because I need to prove I scrolled to the end
:-)
> > > 
> > > > +		port = drm_dp_get_validated_port_ref(mgr, vcpi->port);
> > > > +		if (!port) {
> > > > +			DRM_DEBUG_ATOMIC("[MST PORT:%p] is stale, skipping
> > > > checks\n",
> > > > +					 vcpi->port);
> > > > +			continue;
> > > > +		}
> > > > +
> > > > +		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
> > > > @@ -3213,13 +3369,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 bbf8ca21a7a2..8749cf61a6ac 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -12553,6 +12553,10 @@ static int
intel_atomic_check(struct drm_device
> > > > *dev,
> > > >  				       "[modeset]" : "[fastset]");
> > > >  	}
> > > >  
> > > > +	ret = drm_dp_mst_atomic_check(state);
> > > > +	if (ret)
> > > > +		return ret;
> > > 
> > > Complete bikeshed, so feel free to ignore: I'd stuff this
into
> > > intel_modeset_checks(). Feel free to ignore.
> > > 
> > > Cheers, Daniel
> > > 
> > > > +
> > > >  	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 8b71d64ebd9d..aaf904738b78 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > > @@ -114,28 +114,31 @@ static int
intel_dp_mst_atomic_check(struct
> > > > drm_connector *connector,
> > > >  	struct drm_connector_state *old_conn_state;
> > > >  	struct drm_crtc *old_crtc;
> > > >  	struct drm_crtc_state *crtc_state;
> > > > -	int slots, ret = 0;
> > > > +	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;
> > > > +	int ret = 0;
> > > >  
> > > >  	old_conn_state = drm_atomic_get_old_connector_state(state,
connector);
> > > >  	old_crtc = old_conn_state->crtc;
> > > >  	if (!old_crtc)
> > > >  		return ret;
> > > >  
> > > > -	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;
> > > > +	/* Free VCPI, since compute_config() won't be run */
> > > > +	if (!new_conn_state->crtc) {
> > > > +		crtc_state = drm_atomic_get_new_crtc_state(state,
old_crtc);
> > > >  
> > > > -		old_encoder = old_conn_state->best_encoder;
> > > > -		mgr =
&enc_to_mst(old_encoder)->primary->dp.mst_mgr;
> > > > -
> > > > -		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;
> > > > +		ret = drm_dp_atomic_release_vcpi_slots(state, mgr, port);
> > > > +		if (ret) {
> > > > +			DRM_DEBUG_KMS("failed releasing vcpi slots:
%d\n",
> > > > +				      ret);
> > > > +			return ret;
> > > > +		}
> > > > +		to_intel_crtc_state(crtc_state)->dp_m_n.tu = 0;
> > > >  	}
> > > > +
> > > >  	return ret;
> > > >  }
> > > >  
> > > > diff --git a/include/drm/drm_dp_mst_helper.h
> > > > b/include/drm/drm_dp_mst_helper.h
> > > > index 3faceb66f5cb..08b67948b8f9 100644
> > > > --- a/include/drm/drm_dp_mst_helper.h
> > > > +++ b/include/drm/drm_dp_mst_helper.h
> > > > @@ -406,9 +406,15 @@ 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 {
> > > > +	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;
> > > >  	struct drm_dp_mst_topology_mgr *mgr;
> > > >  };
> > > >  
> > > > @@ -619,14 +625,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
> > > > 
> -- 
> Cheers,
> 	Lyude Paul
> 
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

On Wed, 2018-11-07 at 22:59 +0100, Daniel Vetter wrote:
> On Wed, Nov 07, 2018 at 04:39:57PM -0500, Lyude Paul wrote:
> > On Wed, 2018-11-07 at 16:23 -0500, Lyude Paul wrote:
> > > On Wed, 2018-11-07 at 21:59 +0100, Daniel Vetter wrote:
> > > > On Tue, Nov 06, 2018 at 08:21:14PM -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 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 | 222
++++++++++++++++++++++-
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_display.c  |   4 +
> > > > >  drivers/gpu/drm/i915/intel_dp_mst.c   |  31 ++--
> > > > >  include/drm/drm_dp_mst_helper.h       |  23 ++-
> > > > >  4 files changed, 225 insertions(+), 55 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > > index 8c3cfac437f4..74823afb262e 100644
> > > > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > > > @@ -2614,21 +2614,34 @@ 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. The
allocations are
> > > > > not
> > > > > checked
> > > > > + * against the bandwidth restraints of @mgr until the
driver calls
> > > > > + * drm_dp_mst_atomic_check().
> > > > > + *
> > > > > + * 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))
> > > > > @@ -2637,20 +2650,41 @@ 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;
> > > > > +	/* 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;
> > > > > +			break;
> > > > > +		}
> > > > >  	}
> > > > > +	if (!vcpi)
> > > > > +		prev_slots = 0;
> > > > > +
> > > > > +	req_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
> > > > >  
> > > > > -	topology_state->avail_slots -= req_slots;
> > > > > -	DRM_DEBUG_KMS("vcpi slots avail=%d",
topology_state-
> > > > > >avail_slots);
> > > > > +	DRM_DEBUG_KMS("[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 = port;
> > > > > +		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);
> > > > >  
> > > > > @@ -2658,32 +2692,50 @@
> > > > > 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).
> > > > > + *
> > > > > + * It is OK to call this even if @port has been
removed from the
> > > > > system,
> > > > > in
> > > > > + * which case it will just amount to a no-op.
> > > > > + *
> > > > > + * 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 *tmp, *pos;
> > > > >  
> > > > >  	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_safe(pos, tmp,
&topology_state->vcpis,
> > > > > next) {
> > > > > +		if (pos->port == port) {
> > > > > +			list_del(&pos->next);
> > > > > +			DRM_DEBUG_KMS("[MST PORT:%p] vcpi %d ->
0\n",
> > > > > +				      port, pos->vcpi);
> > > > >  
> > > > > +			kfree(pos);
> > > > > +			return 0;
> > > > > +		}
> > > > > +	}
> > > > > +
> > > > > +	/* If no allocation was found, all that means is that
the port
> > > > > was
> > > > > +	 * destroyed since the last atomic commit. That's
OK!
> > > > > +	 */
> > > > 
> > > > This case here has me a bit worried. I thought with your
changes we'll
> > > > always have the port pointer still around, to be able to
clean things
> > > > up
> > > > properly. If this is not the case then we'd potentially
leak the vcpi
> > > > allocation for an unplugged port.
> > > > 
> > > > Otoh if the driver releases the allocation for a port twice
it's
> > > > probably
> > > > mixed up it's own book-keeping. We should WARN about
that case.
> > > > 
> > > > This is kinda fallout from your change to not prune
allocations on
> > > > duplicate, and I think will further clean up how this is
handled.
> > > > 
> > > > So with the comment removed and replaced by a
WARNING(1,"no vcpi
> > > > allocation found for port %p\n", port) or something
like that this
> > > > should
> > > > be solid.
> > > 
> > > Hm; that comment is leftover from the fallout of moving the port
> > > verification
> > > logic out of the state duplicate functions, but I don't think
this would
> > > nessecarily be a bookkeeping error. If we're going to support
fallback
> > > link retraining, we'll need to make sure that every
driver's
> > > drm_connector->atomic_check() function is idempotent so that
if the
> > > atomic helpers have to pull in more CRTCs to perform modesets on
then
> > > userspace originally added, the second check on the DRM connector
> > > doesn't cause any issues. Remember: if we're trying to
retrain an MST
> > > topology at a lower link rate then before, that means changing
the PBN
> > > divisor which in turn means that the only valid actions that can
be
> > > taken are recalculating every active VCPI allocation at once in
order to
> > > prevent state inconsistencies, or disabling all of the displays
(since
> > > going from >0 VCPI to 0 VCPI should always be safe).
> > > 
> > > We /could/ just leave this up to the driver to do the bookkeeping
on
> > > this, but that seems rather redundant. We could add extra
bookkeeping in
> > > the atomic check though so that instead of just deleting the
structs for
> > > each VCPI allocation when they go to 0, we instead just set them
to 0
> > > then free them somewhere else.
> > 
> > jfyi; I think it would go something like this:
> > 
> > drm_dp_atomic_release_vcpi_slots():
> > 
> > for each vcpi:
> >   if vcpi.port == port:
> >     vcpi.vcpi = 0
> >     return 0;
> > 
> > drm_error("No previous VCPI allocation found! Help!!!\n");
> > return -EINVAL;
> > 
> > Then we'd need to add something to prune the VCPI allocations in
the
> > state,
> > which the driver would call after all atomic checks/mode validations
have
> > been
> > performed, including drm_dp_mst_atomic_check()
> > 
> > drm_dp_mst_atomic_check_finish():
> >   for each vcpi:
> >     if vcpi.vcpi == 0:
> >       del vcpi
> > 
> > Then, that way we've made sure that multiple VCPI frees in the
same check
> > are
> > OK, but freeing VCPI that never existed actually throws errors
> 
> Not sure that's needed at all. The way link training works is we just
ask
> userspace to do the heavy lifting for us, by issuing a new modeset. If
> that means mode pruning, we also ask userspace to do the heavy lifting for
> us, again by issuing a new modeset. Kernel never needs to retrain if that
> means recomputing state stuff or pruning modes or other fun problems. It
> doesn't even need to retrain if that means shutting down a bunch of
links,
> it can just punt that to userspace and let it retrain at a convenient
> time.
> 
> Of course that retrain might then result in a modeset, which might fail if
> the connector is entirely gone by then.
> 
> Otoh I might just be talking nonsense right now, it's a bit late :-)
hehe-it's fine. Either way, I'll try to explain it a little better to
make
this easier to understand for tommorrow.

So: the VCPI allocations are all calculated based on the topology's PBN
divisor, which itself is calculated based on the link rate and bandwidth. For
a topology running at 5.4 GBit/s x 4 lanes, the pbn_divisor would be 40. For
2.7GBit/s x 4 lanes, it would be 20. The higher the PBN divisor, the more VCPI
tokens are needed for each active display, and vice versa.

So: let's say we have a topology like this:

DP-1 @ 5.4 GBit/s w/ pbn_div == 40:
 |- DP-4: 1920x1080 @ 60Hz bpp=24, VCPI = 14
 |- DP-5: 1920x1080 @ 60Hz bpp=24, VCPI = 14

As a note: because DP-4 and DP-5 share the same DP link since they're on
the same topology, I'm assuming that LINK_STATUS_BAD is set at once for
all of them.

If DP-4 and DP-5's connection status suddenly went LINK_STATUS_BAD, we'd
need to recalculate everything to fit in 2.7 GBit/s with a pbn_div of
20. This is the important part: remember that since everything's sharing
the same link, that means they're also sharing the same PBN divisor.
Which means that if userspace were to try to fix this by changing one
display at a time, say:

DP-1 @ 2.7 GBit/s w/ pbn_div == 20: (Invalid state)
 |- DP-4: 1920x1080 @ 60Hz bpp=24, VCPI=28
 |- DP-5: 1920x1080 @ 60Hz bpp=24, VCPI=14

This wouldn't work at all: the 14 VCPI slots that DP-5 has now
correspond to half the amount of real-world bandwidth then they did
previously. This would be broken, and additionally the MST spec mandates
that in the event that the all payloads need to be recalculated on link
rate changes before programming new payloads.. So, that means we're only
left with a few ways to fix this. Change DP-4 and DP-5 at the same time:

DP-1 @ 2.7 GBit/s w/ pbn_div == 20: (Link good, valid state)
 |- DP-4: 1920x1080 @ 60Hz bpp=24, VCPI=28
 |- DP-5: 1920x1080 @ 60Hz bpp=24, VCPI=28

Or disable each connector. This can be done all at once, or one at a
time since we're not programming new payloads:

DP-1 @ 5.4 GBit/s w/ pbn_div == 40: (Link bad, valid state)
 |- DP-4: 1920x1080 @ 60Hz bpp=24, VCPI = 14
 |- DP-5: <disabled>

DP-1 @ 2.7 GBit/s w/ pbn_div == 20: (Link good, valid state)
 |- DP-4: <disabled>
 |- DP-5: <disabled>

What this means for atomic userspace is that while it is a bit
complicated, we do actually need to add restrictions on what modesets it
tries to do on topologies that need fallback retraining to prevent it
from setting an invalid state like the one shown above. Part of this
means that if userspace tries to do a modeset on one connector on a
topology, we have to force a modeset on all of the other connectors on
the topology to ensure that they still fit within the VCPI restrictions
after having their allocations recalculated. /THIS/ is where having
everything that the atomic MST helpers do be idempotent comes in handy
as it would solve a situation like this:

(assuming the current setup is identical to the diagrams I listed above)

Atomic state containing:
 - mgr w/ pbn_div == 40 (link @ 5.4 GBit/s)
 - DP-4: 1920x1080 @ 60Hz bpp=24, VCPI=14
   - LINK_STATUS bad -> good (requires modeset to apply new VCPI of 28)

DP-4 encoder.atomic_check() -> OK
DP-4.atomic_check() -> OK

mgr.atomic_check() -> nono wait a second userspace!!
 - DP-4 can't be changed without changing DP-5, so:
   state.add_connector(DP-5)
   DP-5.mode_changed = true
   need_recheck = true

state.recheck():
 # Rerun checks now
 DP-4 encoder.atomic_check() -> OK
 DP-4.atomic_check() -> OK
 DP-5 encoder.atomic_check() -> OK
 DP-5.atomic_check() -> OK
 mgr.atomic_check() -> OK

As you can see: DP-4 needed to get checked a second time, which means
it's VCPI would be freed twice in the same state, but nothing would
actually be leaking! That's pretty much the whole usecase for something
like this, so that drivers don't have to track where they are in their
atomic check() since that's a lot more error prone to have rewritten
once for each MST driver.
> 
> Cheers, Daniel
> 
> 
> > > > >  	return 0;
> > > > >  }
> > > > >  EXPORT_SYMBOL(drm_dp_atomic_release_vcpi_slots);
> > > > > @@ -3112,15 +3164,34 @@ 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);
> > > > > +
> > > > > +	list_for_each_entry(pos, &old_state->vcpis,
next) {
> > > > > +		vcpi = kmemdup(pos, sizeof(*vcpi), GFP_KERNEL);
> > > > > +		if (!vcpi)
> > > > > +			goto fail_alloc;
> > > > > +
> > > > > +		list_add(&vcpi->next, &state->vcpis);
> > > > > +	}
> > > > > +
> > > > >  	return &state->base;
> > > > > +
> > > > > +fail_alloc:
> > > > > +	list_for_each_entry_safe(pos, vcpi,
&state->vcpis, next)
> > > > > +		kfree(pos);
> > > > > +	kfree(state);
> > > > > +
> > > > > +	return NULL;
> > > > >  }
> > > > >  
> > > > >  static void drm_dp_mst_destroy_state(struct
drm_private_obj *obj,
> > > > > @@ -3128,14 +3199,99 @@ 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)
> > > > > +		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;
> > > > > +
> > > > > +	list_for_each_entry(vcpi, &mst_state->vcpis,
next) {
> > > > > +		/* Ports that no longer exist shouldn't be
counted
> > > > > towards the
> > > > > +		 * bandwidth requirements for the state, so skip
them
> > > > > +		 */
> > > > 
> > > > Scenario:
> > > > - 2 mst sinks on a single host side port.
> > > > - 1st connector is active, 2nd connector is off.
> > > > - 1st connector is hot-unplugged, but userspace hasn't
realized this
> > > > yet,
> > > >   so not yet come around to do the disabling modeset.
> > > > - At the same time userspace decides to enable the 2nd
output, which
> > > >   forces a modeset and pulls the topology state in.
> > > > - This function here decides to only look at the vcpi
allocation for
> > > > the
> > > >   2nd connector because the first one is gone already.
> > > > - Unfortunately lighting up both connectors at the same time
isn't
> > > >   possible, because too many vcpi. But since userspace
didn't ask for
> > > > a
> > > >   modeset on the now unplugged first connector.
> > > > -> Modeset goes boom because we enable too many vcpi.
> > > > 
> > > > But with the change in how we release vcpi allocations
there's no need
> > > > to
> > > > ignore disconnected stuff here, and we should be able to
check vcpi
> > > > limits
> > > > by just checking all of them.
> > > > 
> > > > I didn't see anything else serious, so with the above
two things
> > > > resolved
> > > > this has my Reviewed-by: Daniel Vetter <daniel.vetter at
ffwll.ch>
> > > > 
> > > > One bikeshed below, because I need to prove I scrolled to
the end :-)
> > > > 
> > > > > +		port = drm_dp_get_validated_port_ref(mgr,
vcpi->port);
> > > > > +		if (!port) {
> > > > > +			DRM_DEBUG_ATOMIC("[MST PORT:%p] is stale,
> > > > > skipping
> > > > > checks\n",
> > > > > +					 vcpi->port);
> > > > > +			continue;
> > > > > +		}
> > > > > +
> > > > > +		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
> > > > > @@ -3213,13 +3369,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 bbf8ca21a7a2..8749cf61a6ac 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > > @@ -12553,6 +12553,10 @@ static int
intel_atomic_check(struct
> > > > > drm_device
> > > > > *dev,
> > > > >  				       "[modeset]" :
"[fastset]");
> > > > >  	}
> > > > >  
> > > > > +	ret = drm_dp_mst_atomic_check(state);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > 
> > > > Complete bikeshed, so feel free to ignore: I'd stuff
this into
> > > > intel_modeset_checks(). Feel free to ignore.
> > > > 
> > > > Cheers, Daniel
> > > > 
> > > > > +
> > > > >  	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 8b71d64ebd9d..aaf904738b78 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > > > @@ -114,28 +114,31 @@ static int
intel_dp_mst_atomic_check(struct
> > > > > drm_connector *connector,
> > > > >  	struct drm_connector_state *old_conn_state;
> > > > >  	struct drm_crtc *old_crtc;
> > > > >  	struct drm_crtc_state *crtc_state;
> > > > > -	int slots, ret = 0;
> > > > > +	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;
> > > > > +	int ret = 0;
> > > > >  
> > > > >  	old_conn_state =
drm_atomic_get_old_connector_state(state,
> > > > > connector);
> > > > >  	old_crtc = old_conn_state->crtc;
> > > > >  	if (!old_crtc)
> > > > >  		return ret;
> > > > >  
> > > > > -	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;
> > > > > +	/* Free VCPI, since compute_config() won't be run
*/
> > > > > +	if (!new_conn_state->crtc) {
> > > > > +		crtc_state = drm_atomic_get_new_crtc_state(state,
> > > > > old_crtc);
> > > > >  
> > > > > -		old_encoder = old_conn_state->best_encoder;
> > > > > -		mgr =
&enc_to_mst(old_encoder)->primary->dp.mst_mgr;
> > > > > -
> > > > > -		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;
> > > > > +		ret = drm_dp_atomic_release_vcpi_slots(state, mgr,
> > > > > port);
> > > > > +		if (ret) {
> > > > > +			DRM_DEBUG_KMS("failed releasing vcpi slots:
> > > > > %d\n",
> > > > > +				      ret);
> > > > > +			return ret;
> > > > > +		}
> > > > > +		to_intel_crtc_state(crtc_state)->dp_m_n.tu = 0;
> > > > >  	}
> > > > > +
> > > > >  	return ret;
> > > > >  }
> > > > >  
> > > > > diff --git a/include/drm/drm_dp_mst_helper.h
> > > > > b/include/drm/drm_dp_mst_helper.h
> > > > > index 3faceb66f5cb..08b67948b8f9 100644
> > > > > --- a/include/drm/drm_dp_mst_helper.h
> > > > > +++ b/include/drm/drm_dp_mst_helper.h
> > > > > @@ -406,9 +406,15 @@ 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 {
> > > > > +	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;
> > > > >  	struct drm_dp_mst_topology_mgr *mgr;
> > > > >  };
> > > > >  
> > > > > @@ -619,14 +625,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
> > > > > 
> > -- 
> > Cheers,
> > 	Lyude Paul
> > 
-- 
Cheers,
	Lyude Paul