[Intel-gfx] [PATCH 1/3] drm/atomic-helper: Fix leak in disable_all
Daniel Vetter
daniel at ffwll.ch
Sat Jul 15 11:03:15 UTC 2017
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
More information about the Intel-gfx
mailing list