Re: [PATCH 1/3] drm/atomic-helper: Fix leak in disable_all

[Daniel Vetter <daniel@ffwll.ch>]

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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel