On Sat, Jul 15, 2017 at 11:10:07AM +0100, Chris Wilson wrote: > Quoting Daniel Vetter (2017-07-14 23:46:54) > > @@ -2902,6 +2907,8 @@ int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state, > > struct drm_connector_state *new_conn_state; > > struct drm_crtc *crtc; > > struct drm_crtc_state *new_crtc_state; > > + struct drm_device *dev = state->dev; > > + int ret; > > > > state->acquire_ctx = ctx; > > > > @@ -2914,7 +2921,10 @@ int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state, > > for_each_new_connector_in_state(state, connector, new_conn_state, i) > > state->connectors[i].old_state = connector->state; > > > > - return drm_atomic_commit(state); > > + ret = drm_atomic_commit(state); > > + drm_atomic_clean_old_fb(dev, ~0U, ret); > > I have no idea what the rules should be here. Or how it should interact > with error. Should we just try the "drm: Track framebuffer references at > the point of assignment" approach to simplify the rules (at least from > my perspective)? The problem with that patch is sorting out the state > duplication done in a couple of drivers and figuring out if they are > transferring ownership or not. Yeah this is a decent mess. I think a first incremental step would be to move the ->old_fb and ->fb refcount updating into drm_atomic_commit - clean_old_fb is a superbly misnamed function, what it really does is update the legacy framebuffer pointer refcounts. The kernel-doc it has also just serves to increase the confusion :-/ The only problem is that for an drm_atomic_commit in one of the legacy callbacks we must _not_ do that, because the core already takes care fo that. But having a drm_atomic_commit_legacy for that would be a lot cleaner I think. Once we have that I guess we could try and figure out what to do with the refcounting of the legacy pointers at large. Meanwhile the rules are: - If you do an atomic commit, you must call clean_old_fb. Everywhere. We got away in a few cases because we've made nice symmetric commits (like suspend/resume) or commits where we know the fb doesn't get updated. - Exception: When called from ->plane_update/disable, ->set_config or ->page_flip, the core does it for you. It's a mess, but I'd like to decouple fixing that mess from fixing the unload bug we have. I'll try to type up the drm_atomic_commit/_legacy patch next week. -Daniel -- 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
