On Fri, Jul 07, 2017 at 02:03:38PM +0200, Daniel Vetter wrote:
> 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.
Well, I still don't see what's so "massive" about a couple of extra state
pointers hanging around.
Also while doing that state duplication stuff, my old idea of
splitting the crtc disable and enable phases into separate atomic
commits popped up again in my head. For that being able to duplicate
arbitrary states would seem like a nice thing to have. So the
refactoring I had to do can have other uses.
> 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?
>From the point of view of the old and new states it doesn't actually
break that. The commits done from the reset path are essentially
invisible to the normal modeset operation.
The one alternative proposed idea of allowing gem and kms sides go
out of whack scares me a bit. I think that might land us in more
trouble when I finally get around to making the video overlay a
drm_plane.
And I think trying to keep the GPU reset paths as similar as possible
between all the platforms would be a nice thing. Just whacking
everything on the head with a hammer on one platform but not on
another one seems to me like extra variation in behaviour that we
don't necessarily want.
But like I said, if someone can come up with a better solution I
probably wouldn't object too much. It's not going to be coming from me
though since I have plenty of other things to do and vacation time is
coming up very soon. So unless someone else comes up with something nice
soon I think we should just go with my solution because a) it's already
available, and b) works quite decently from what I can see.
> -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
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
