On Fri, Jul 7, 2017 at 3:21 PM, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote: > 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. I fully realize that you're unhappy with how atomic ended up getting merged and that you think it's a grave mistake. And maybe it is, maybe it isn't, I have no idea. But right now we have _nothing_ asking for that reorg afaik, and using gen4 reset to justify it is in my opinion simply not solid engineering practice. Maybe we need this in the future, and then we can add it, but not before. Refactoring stuff to prettify the architecture isn't really useful work. >> 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. You insert something into a fully ordered queue. That does break the entire concept and needs a pile of locks and stuff to make it work. Yes it's doable, but it's a redesign with all the implications of subtle breakage all over. While we do have open bugs for the current design. Rewriting the world to fix a bug needs seriously better justification imo. > 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. We've run perfectly fine with this idea for years. > 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. I guess I'll have to retype the old thing in the new world, but it really shouldn't be more than the quick draft I've laid down in the old thread. This here is imo no-go with all the core changes, and even just done within i915 I think it's highly dubious that it provides a real benefit, since defacto it means we'll have to abandon the atomic helpers entirely. -Daniel > >> -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 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx