[Intel-gfx] [PATCH v3 21/22] drm/atomic: Introduce drm_atomic_helper_duplicate_commited_state()

Daniel Vetter daniel at ffwll.ch
Fri Jul 7 12:03:38 UTC 2017


On Thu, Jul 06, 2017 at 11:24:41PM +0300, ville.syrjala at linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> 
> For i915 GPU reset handling we'll want to be able to duplicate the state
> that was last commited to the hardware. For that purpose let's start to
> track the commited state for each object and provide a way to duplicate
> the commmited state into a new drm_atomic_state. The locking for
> .commited_state must to be provided by the driver.
> 
> drm_atomic_helper_duplicate_commited_state() duplicates the state
> to both old_state and new_state. For the purposes of i915 GPU reset we
> would only need one of them, but we actually need two top level states;
> one for disabling everything (which would need the duplicated state to
> be old_state), and another to reenable everything (which would need the
> duplicated state to be new_state). So to make it less comples I figured
> I'd just always duplicate both. Might want to rethink this if for no
> other reason that reducing the chances of memory allocation failure.
> Due to the double state duplication we need
> drm_atomic_helper_clean_commited_state() to clean up the duplicated
> old_state since that's not handled by the normal drm_atomic_state
> cleanup code.
> 
> TODO: do we want this in the helper, or maybe it should be just in i915?
> 
> v2: s/commited/committed/ everywhere (checkpatch)
>     Handle state duplication errors better
> v3: Even more care in dealing with memory allocation errors
>     Handle private objs too
>     Deal with the potential ordering issues between swap_state()
>     and hw_done() by keeping track of which state was swapped in
>     last
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>

I still don't get why we need to duplicate the committed state for gpu
reset. When I said I'm not against adding all that complexity long-term I
meant when we actually really need it. Imo g4x gpu reset isn't a good
justification for that, reworking the atomic world for that seems
massively out of proportion.

Why exactly can't we do this simpler? I still don't get that part. Is
there really no solution that doesn't break atomic's current assumption
that commits are fully ordered on a given crtc?
-Daniel

> ---
>  drivers/gpu/drm/drm_atomic.c        |   1 +
>  drivers/gpu/drm/drm_atomic_helper.c | 231 ++++++++++++++++++++++++++++++++++++
>  include/drm/drm_atomic.h            |   4 +
>  include/drm/drm_atomic_helper.h     |   8 ++
>  include/drm/drm_connector.h         |  11 ++
>  include/drm/drm_crtc.h              |  11 ++
>  include/drm/drm_plane.h             |  11 ++
>  7 files changed, 277 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 56925b93f598..e1578d50d66f 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1020,6 +1020,7 @@ drm_atomic_private_obj_init(struct drm_private_obj *obj,
>  	memset(obj, 0, sizeof(*obj));
>  
>  	obj->state = state;
> +	obj->committed_state = state;
>  	obj->funcs = funcs;
>  }
>  EXPORT_SYMBOL(drm_atomic_private_obj_init);
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index f0887f231fb8..c3d02f12cd5d 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1815,6 +1815,11 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
>  
> +static bool state_seqno_after(unsigned int a, unsigned int b)
> +{
> +	return (int)(b - a) < 0;
> +}
> +
>  /**
>   * drm_atomic_helper_commit_hw_done - setup possible nonblocking commit
>   * @old_state: atomic state object with old state structures
> @@ -1833,11 +1838,39 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
>  void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state)
>  {
>  	struct drm_crtc *crtc;
> +	struct drm_plane *plane;
> +	struct drm_connector *connector;
> +	struct drm_private_obj *obj;
>  	struct drm_crtc_state *new_crtc_state;
> +	struct drm_plane_state *new_plane_state;
> +	struct drm_connector_state *new_connector_state;
> +	struct drm_private_state *new_obj_state;
>  	struct drm_crtc_commit *commit;
>  	int i;
> +	static DEFINE_SPINLOCK(committed_state_lock);
> +
> +	spin_lock(&committed_state_lock);
> +
> +	for_each_new_plane_in_state(old_state, plane, new_plane_state, i) {
> +		if (plane->committed_state &&
> +		    state_seqno_after(new_plane_state->seqno,
> +				      plane->committed_state->seqno))
> +			plane->committed_state = new_plane_state;
> +	}
> +
> +	for_each_new_connector_in_state(old_state, connector, new_connector_state, i) {
> +		if (connector->committed_state &&
> +		    state_seqno_after(new_connector_state->seqno,
> +				      connector->committed_state->seqno))
> +			connector->committed_state = new_connector_state;
> +	}
>  
>  	for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
> +		if (crtc->committed_state &&
> +		    state_seqno_after(new_crtc_state->seqno,
> +				      crtc->committed_state->seqno))
> +			crtc->committed_state = new_crtc_state;
> +
>  		commit = old_state->crtcs[i].commit;
>  		if (!commit)
>  			continue;
> @@ -1846,6 +1879,15 @@ void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state)
>  		WARN_ON(new_crtc_state->event);
>  		complete_all(&commit->hw_done);
>  	}
> +
> +	for_each_new_private_obj_in_state(old_state, obj, new_obj_state, i) {
> +		if (obj->committed_state &&
> +		    state_seqno_after(new_obj_state->seqno,
> +				      obj->committed_state->seqno))
> +			obj->committed_state = new_obj_state;
> +	}
> +
> +	spin_unlock(&committed_state_lock);
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_commit_hw_done);
>  
> @@ -2296,6 +2338,7 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>  
>  		old_conn_state->state = state;
>  		new_conn_state->state = NULL;
> +		new_conn_state->seqno = ++connector->state_seqno;
>  
>  		__drm_atomic_state_connector(state, i)->state = old_conn_state;
>  		connector->state = new_conn_state;
> @@ -2309,6 +2352,7 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>  
>  		state->crtcs[i].state = old_crtc_state;
>  		crtc->state = new_crtc_state;
> +		new_crtc_state->seqno = ++crtc->state_seqno;
>  
>  		if (state->crtcs[i].commit) {
>  			spin_lock(&crtc->commit_lock);
> @@ -2325,6 +2369,7 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>  
>  		old_plane_state->state = state;
>  		new_plane_state->state = NULL;
> +		new_plane_state->seqno = ++plane->state_seqno;
>  
>  		state->planes[i].state = old_plane_state;
>  		plane->state = new_plane_state;
> @@ -2335,6 +2380,7 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
>  
>  		old_obj_state->state = state;
>  		new_obj_state->state = NULL;
> +		new_obj_state->seqno = ++obj->state_seqno;
>  
>  		__drm_atomic_state_private_obj(state, i)->state = old_obj_state;
>  		obj->state = new_obj_state;
> @@ -3582,6 +3628,7 @@ __drm_atomic_helper_connector_reset(struct drm_connector *connector,
>  		conn_state->connector = connector;
>  
>  	connector->state = conn_state;
> +	connector->committed_state = conn_state;
>  }
>  EXPORT_SYMBOL(__drm_atomic_helper_connector_reset);
>  
> @@ -3740,6 +3787,189 @@ drm_atomic_helper_duplicate_state(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_duplicate_state);
>  
> +struct drm_atomic_state *
> +drm_atomic_helper_duplicate_committed_state(struct drm_device *dev)
> +{
> +	struct drm_atomic_state *state;
> +	struct drm_connector_list_iter conn_iter;
> +	struct drm_connector *conn;
> +	struct drm_plane *plane;
> +	struct drm_crtc *crtc;
> +	int err = 0;
> +
> +	state = drm_atomic_state_alloc(dev);
> +	if (!state)
> +		return ERR_PTR(-ENOMEM);
> +
> +	drm_for_each_plane(plane, dev) {
> +		struct drm_plane_state *old_state, *new_state;
> +		int i = drm_plane_index(plane);
> +
> +		old_state = plane->funcs->atomic_duplicate_state(plane, plane->committed_state);
> +		if (!old_state) {
> +			err = -ENOMEM;
> +			goto free;
> +		}
> +		new_state = plane->funcs->atomic_duplicate_state(plane, plane->committed_state);
> +		if (!new_state) {
> +			plane->funcs->atomic_destroy_state(plane, old_state);
> +			err = -ENOMEM;
> +			goto free;
> +		}
> +
> +		state->planes[i].state = new_state;
> +		state->planes[i].old_state = old_state;
> +		state->planes[i].new_state = new_state;
> +		state->planes[i].ptr = plane;
> +
> +		old_state->state = state;
> +	}
> +
> +	drm_for_each_crtc(crtc, dev) {
> +		struct drm_crtc_state *old_state, *new_state;
> +		int i = drm_crtc_index(crtc);
> +
> +		old_state = crtc->funcs->atomic_duplicate_state(crtc, crtc->committed_state);
> +		if (!old_state) {
> +			err = -ENOMEM;
> +			goto free;
> +		}
> +		new_state = crtc->funcs->atomic_duplicate_state(crtc, crtc->committed_state);
> +		if (!new_state) {
> +			crtc->funcs->atomic_destroy_state(crtc, old_state);
> +			err = -ENOMEM;
> +			goto free;
> +		}
> +
> +		state->crtcs[i].state = new_state;
> +		state->crtcs[i].old_state = old_state;
> +		state->crtcs[i].new_state = new_state;
> +		state->crtcs[i].ptr = crtc;
> +
> +		old_state->state = state;
> +	}
> +
> +	drm_connector_list_iter_begin(dev, &conn_iter);
> +	drm_for_each_connector_iter(conn, &conn_iter) {
> +		struct drm_connector_state *old_state, *new_state;
> +		struct __drm_connectors_state *c;
> +		int i = drm_connector_index(conn);
> +
> +		err = drm_dynarray_reserve(&state->connectors, i);
> +		if (err)
> +			break;
> +
> +		old_state = conn->funcs->atomic_duplicate_state(conn, conn->committed_state);
> +		if (!old_state) {
> +			err = -ENOMEM;
> +			break;
> +		}
> +		new_state = conn->funcs->atomic_duplicate_state(conn, conn->committed_state);
> +		if (!new_state) {
> +			conn->funcs->atomic_destroy_state(conn, old_state);
> +			err = -ENOMEM;
> +			break;
> +		}
> +
> +		state->num_connector = state->connectors.num_elems;
> +
> +		c = __drm_atomic_state_connector(state, i);
> +
> +		c->state = new_state;
> +		c->old_state = old_state;
> +		c->new_state = new_state;
> +		c->ptr = drm_connector_get(conn);
> +
> +		old_state->state = state;
> +	}
> +	drm_connector_list_iter_end(&conn_iter);
> +
> +free:
> +	if (err < 0) {
> +		drm_atomic_helper_clean_committed_state(state);
> +		drm_atomic_state_put(state);
> +		state = ERR_PTR(err);
> +	}
> +
> +	return state;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_duplicate_committed_state);
> +
> +int drm_atomic_helper_duplicate_private_obj_committed_state(struct drm_atomic_state *state,
> +							   struct drm_private_obj *obj)
> +{
> +	struct drm_private_state *old_state, *new_state;
> +	struct __drm_private_objs_state *p;
> +	int ret, i = state->num_private_objs;
> +
> +	ret = drm_dynarray_reserve(&state->private_objs, i);
> +	if (ret)
> +		return ret;
> +
> +	old_state = obj->funcs->atomic_duplicate_state(obj, obj->committed_state);
> +	if (!old_state)
> +		return -ENOMEM;
> +
> +	new_state = obj->funcs->atomic_duplicate_state(obj, obj->committed_state);
> +	if (!new_state) {
> +		obj->funcs->atomic_destroy_state(obj, obj->committed_state);
> +		return -ENOMEM;
> +	}
> +
> +	state->num_private_objs = state->private_objs.num_elems;
> +
> +	p = __drm_atomic_state_private_obj(state, i);
> +
> +	p->state = new_state;
> +	p->old_state = old_state;
> +	p->new_state = new_state;
> +	p->ptr = obj;
> +
> +	old_state->state = state;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_duplicate_private_obj_committed_state);
> +
> +void
> +drm_atomic_helper_clean_committed_state(struct drm_atomic_state *state)
> +{
> +	struct drm_plane *plane;
> +	struct drm_crtc *crtc;
> +	struct drm_connector *conn;
> +	struct drm_plane_state *plane_state;
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_connector_state *conn_state;
> +	struct drm_private_obj *obj;
> +	struct drm_private_state *obj_state;
> +	int i;
> +
> +	/* restore the correct state->state */
> +	for_each_new_plane_in_state(state, plane, plane_state, i) {
> +		plane->funcs->atomic_destroy_state(plane, state->planes[i].old_state);
> +		state->planes[i].old_state = NULL;
> +	}
> +	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> +		crtc->funcs->atomic_destroy_state(crtc, state->crtcs[i].old_state);
> +		state->crtcs[i].old_state = NULL;
> +	}
> +	for_each_new_connector_in_state(state, conn, conn_state, i) {
> +		struct __drm_connectors_state *c =
> +			__drm_atomic_state_connector(state, i);
> +
> +		conn->funcs->atomic_destroy_state(conn, c->old_state);
> +		c->old_state = NULL;
> +	}
> +	for_each_new_private_obj_in_state(state, obj, obj_state, i) {
> +		struct __drm_private_objs_state *p =
> +			__drm_atomic_state_private_obj(state, i);
> +
> +		obj->funcs->atomic_destroy_state(obj, p->old_state);
> +		p->old_state = NULL;
> +	}
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_clean_committed_state);
> +
>  /**
>   * __drm_atomic_helper_connector_destroy_state - release connector state
>   * @state: connector state object to release
> @@ -3856,6 +4086,7 @@ EXPORT_SYMBOL(drm_atomic_helper_legacy_gamma_set);
>   * __drm_atomic_helper_private_duplicate_state - copy atomic private state
>   * @obj: CRTC object
>   * @state: new private object state
> + * @old_state: old private object state
>   *
>   * Copies atomic state from a private objects's current state and resets inferred values.
>   * This is useful for drivers that subclass the private state.
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 0e6c54b3e0af..9ff4fb46d500 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -194,12 +194,16 @@ struct drm_private_state_funcs {
>  
>  struct drm_private_obj {
>  	struct drm_private_state *state;
> +	struct drm_private_state *committed_state;
>  
>  	const struct drm_private_state_funcs *funcs;
> +
> +	unsigned int state_seqno;
>  };
>  
>  struct drm_private_state {
>  	struct drm_atomic_state *state;
> +	unsigned int seqno;
>  };
>  
>  struct __drm_private_objs_state {
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index b799687a4b09..3a609bbb5818 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -185,6 +185,14 @@ drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector,
>  struct drm_atomic_state *
>  drm_atomic_helper_duplicate_state(struct drm_device *dev,
>  				  struct drm_modeset_acquire_ctx *ctx);
> +struct drm_atomic_state *
> +drm_atomic_helper_duplicate_committed_state(struct drm_device *dev);
> +struct drm_private_obj;
> +int
> +drm_atomic_helper_duplicate_private_obj_committed_state(struct drm_atomic_state *state,
> +							struct drm_private_obj *obj);
> +void
> +drm_atomic_helper_clean_committed_state(struct drm_atomic_state *state);
>  void
>  __drm_atomic_helper_connector_destroy_state(struct drm_connector_state *state);
>  void drm_atomic_helper_connector_destroy_state(struct drm_connector *connector,
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 8f26166f78b4..a38ba05db4fa 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -306,6 +306,8 @@ struct drm_tv_connector_state {
>  struct drm_connector_state {
>  	struct drm_connector *connector;
>  
> +	unsigned int seqno;
> +
>  	/**
>  	 * @crtc: CRTC to connect connector to, NULL if disabled.
>  	 *
> @@ -707,6 +709,8 @@ struct drm_connector {
>  
>  	char *name;
>  
> +	unsigned int state_seqno;
> +
>  	/**
>  	 * @mutex: Lock for general connector state, but currently only protects
>  	 * @registered. Most of the connector state is still protected by
> @@ -868,6 +872,13 @@ struct drm_connector {
>  	 */
>  	struct drm_connector_state *state;
>  
> +	/**
> +	 * @committed_state:
> +	 *
> +	 * Current committed atomic state for this connector.
> +	 */
> +	struct drm_connector_state *committed_state;
> +
>  	/* DisplayID bits */
>  	bool has_tile;
>  	struct drm_tile_group *tile_group;
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 8bfbc54660ab..2a1897d76c71 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -108,6 +108,8 @@ struct drm_plane_helper_funcs;
>  struct drm_crtc_state {
>  	struct drm_crtc *crtc;
>  
> +	unsigned int seqno;
> +
>  	bool enable;
>  	bool active;
>  
> @@ -750,6 +752,8 @@ struct drm_crtc {
>  
>  	char *name;
>  
> +	unsigned int state_seqno;
> +
>  	/**
>  	 * @mutex:
>  	 *
> @@ -816,6 +820,13 @@ struct drm_crtc {
>  	struct drm_crtc_state *state;
>  
>  	/**
> +	 * @committed_state:
> +	 *
> +	 * Current committed atomic state for this CRTC.
> +	 */
> +	struct drm_crtc_state *committed_state;
> +
> +	/**
>  	 * @commit_list:
>  	 *
>  	 * List of &drm_crtc_commit structures tracking pending commits.
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 08ad4b58adbe..ff3495705e63 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -59,6 +59,8 @@ struct drm_modeset_acquire_ctx;
>  struct drm_plane_state {
>  	struct drm_plane *plane;
>  
> +	unsigned int seqno;
> +
>  	/**
>  	 * @crtc:
>  	 *
> @@ -470,6 +472,8 @@ struct drm_plane {
>  
>  	char *name;
>  
> +	unsigned int state_seqno;
> +
>  	/**
>  	 * @mutex:
>  	 *
> @@ -522,6 +526,13 @@ struct drm_plane {
>  	 */
>  	struct drm_plane_state *state;
>  
> +	/**
> +	 * @committed_state:
> +	 *
> +	 * Current committed atomic state for this plane.
> +	 */
> +	struct drm_plane_state *committed_state;
> +
>  	struct drm_property *zpos_property;
>  	struct drm_property *rotation_property;
>  };
> -- 
> 2.13.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch


More information about the Intel-gfx mailing list