On Thu, Jul 06, 2017 at 11:24:41PM +0300, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
