Re: [PATCH v2 2/4] drm/dp_mst: Start tracking per-port VCPI allocations

[Daniel Vetter <daniel-/w4YWyX8dFk@public.gmane.org>]

On Mon, Oct 29, 2018 at 03:24:29PM +0100, Daniel Vetter wrote:
> On Fri, Oct 26, 2018 at 04:35:47PM -0400, 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 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@redhat.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/drm_dp_mst_topology.c | 190 +++++++++++++++++++++-----
> >  drivers/gpu/drm/i915/intel_display.c  |   8 ++
> >  drivers/gpu/drm/i915/intel_dp_mst.c   |  31 +++--
> >  include/drm/drm_dp_mst_helper.h       |  11 +-
> >  4 files changed, 192 insertions(+), 48 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index 8c3cfac437f4..dcfab7536914 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -2614,21 +2614,33 @@ 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
> >   *
> > + * 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 atomic_check() handlers to change the current VCPI allocation for
> 
> Maybe do a nice kerneldoc reference to the right atomic_check here.
> 
> > + * the new state. After the ->atomic_check() hooks of the driver and all other
> 
> This will upset the kerneldoc parser I think.
> 
> > + * mode objects in the state have been called, DRM will check the final VCPI
> > + * allocations to ensure that they will fit into the available bandwidth on
> > + * the topology.
> > + *
> > + * See also: drm_dp_atomic_release_vcpi_slots()
> 
> Also need to reference drm_dp_mst_atomic_check() here and that drivers
> must call it or nothing happens.
> > + *
> >   * RETURNS:
> > - * Total slots in the atomic state assigned for this port or error
> > + * 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 +2649,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;
> 
> For robustness should we warn here, since drivers forgetting to release
> vcpi slots is kinda a bug? Or do we need to have this to make life easier
> for driver writers?
> 
> > +
> > +	req_slots = DIV_ROUND_UP(pbn, mgr->pbn_div);
> > +
> > +	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;
> > +		}
> >  
> > -	topology_state->avail_slots -= req_slots;
> > -	DRM_DEBUG_KMS("vcpi slots avail=%d", topology_state->avail_slots);
> > +		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 +2691,46 @@ 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
> > + *
> > + * 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 connector's atomic_check() handler 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()
> 
> Same comments as with the _find_vcpi_slots() function.
> 
> >   *
> >   * RETURNS:
> > - * 0 if @slots were added back to &drm_dp_mst_topology_state->avail_slots or
> > - * negative error code
> > + * 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!
> > +	 */
> >  	return 0;
> >  }
> >  EXPORT_SYMBOL(drm_dp_atomic_release_vcpi_slots);
> > @@ -3112,15 +3159,50 @@ 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_mst_topology_mgr *mgr = old_state->mgr;
> > +	struct drm_dp_mst_port *port;
> > +	struct drm_dp_vcpi_allocation *pos, *vcpi;
> >  
> > -	state = kmemdup(obj->state, sizeof(*state), GFP_KERNEL);
> > +	state = kzalloc(sizeof(*state), GFP_KERNEL);
> >  	if (!state)
> >  		return NULL;
> >  
> >  	__drm_atomic_helper_private_obj_duplicate_state(obj, &state->base);
> >  
> > +	state->mgr = mgr;
> > +	INIT_LIST_HEAD(&state->vcpis);
> > +
> > +	/* Copy over the VCPI allocations for ports that still exist */
> > +	list_for_each_entry(pos, &old_state->vcpis, next) {
> > +		port = drm_dp_get_validated_port_ref(mgr, pos->port);
> > +		if (!port) {
> > +			DRM_DEBUG_ATOMIC("[MST PORT:%p] is gone, vcpi %d -> 0\n",
> > +					 pos->port, pos->vcpi);
> > +			continue;
> > +		}
> 
> Hm, in general we have 0 validation in the duplicate_state functions, Just
> dump copying, with minimal pointer/refcount updates.
> > +
> > +		vcpi = kzalloc(sizeof(*vcpi), GFP_KERNEL);
> 
> kmemdump + updating the list. Just in case we ever add more stuff here.
> Probably needs an explicit memset to avoid list debugging.
> 
> At least that's the style we use everywhere else.
> 
> > +		if (!vcpi) {
> > +			drm_dp_put_port(port);
> > +			goto fail_alloc;
> > +		}
> > +
> > +		vcpi->port = port;
> > +		vcpi->vcpi = pos->vcpi;
> > +		list_add(&vcpi->next, &state->vcpis);
> > +		drm_dp_put_port(port);
> > +	}
> > +
> >  	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 +3210,60 @@ 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;
> > +
> WARN_ON(!list_empty());
> 
> I think this not being empty on mgr teardown would be a driver bug. All
> the ports/branch devices should be gone by now, and also all the modeset
> state should have been torn down, using drm_atomic_helper_shutdown() or
> friends.

Lyude pointed out on irc that I was not yet sufficiently coffee'd up when
making this comment: The destroy_state callback needs to clean up without
complaining ofc! I mixed it up somehow with drm_mode_config_cleanup,
where the WARN_ON would have been a good idea.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau