[Intel-gfx] [PATCH v3 2/5] drm/dp_mst: Start tracking per-port VCPI allocations

Lyude Paul lyude at redhat.com
Wed Nov 7 21:39:57 UTC 2018


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



More information about the Intel-gfx mailing list