Re: [PATCH 12/24] drm/i915: Merge pre/postclose hooks

From: Daniel Vetter <daniel@ffwll.ch>
To: Chris Wilson <chris@chris-wilson.co.uk>,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	DRI Development <dri-devel@lists.freedesktop.org>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH 12/24] drm/i915: Merge pre/postclose hooks
Date: Tue, 14 Mar 2017 14:38:12 +0100	[thread overview]
Message-ID: <20170314133812.sudjfpqwewv7klgq@phenom.ffwll.local> (raw)
In-Reply-To: <20170308154513.kmg5i2vv4adpi3xn@phenom.ffwll.local>

On Wed, Mar 08, 2017 at 04:45:13PM +0100, Daniel Vetter wrote:
> On Wed, Mar 08, 2017 at 03:07:48PM +0000, Chris Wilson wrote:
> > On Wed, Mar 08, 2017 at 03:12:45PM +0100, Daniel Vetter wrote:
> > > There's really not a reason afaics that we can't just clean up
> > > everything at the end, in the terminal postclose hook: Since this is
> > > closing a file descriptor we know no one else can have a reference or
> > > a thread doing something with that drm_file except the close code.
> > > Ordering shouldn't matter, as long as we don't kfree before we clean
> > > stuff up.
> > 
> > My gut feeling was that preclose is the one we want to keep, as that is
> > closer to the onion unwind - during open, we do the core drm ops first
> > (e.g. drm_gem_open) before the backend, so during close we should do the
> > driver before the core.
> > 
> > Maybe completely wrong ofc.
> 
> I wasn't sure whether drivers might have some random pointers to fpriv
> that they might chase when we tear down gem objects and stuff like that.
> And if you look at what's before postclose, we still have onion
> unwrapping: All the things before it (fb cleanup, gem cleanup, master
> dropping) is stuff that userspace adds to the file _after_ it's fully
> opened.
> 
> Well it's not entirely clear, because gem destroys the idr before
> postclose, which might or might not trip up someone.
> 
> But afaik all the other destructive cleanup is done after postclose.
> 
> So maybe that's the split we want in drm_close?
> 
> - First release all runtime resources acquired after ->open. Not
>   destructive cleanup, fpriv should keep working like right after ->open
>   has returned. These functions would all have _release in their names.
> - call the driver's ->postclose.
> - All the destructive cleanup. Those functions would all have _destroy in
>   their names.
> 
> Since there's not really a real issue right now I'd prefer if we postpone
> this cleanup though, and first get all the driver patches merged. Just in
> case I missed something ...
> 
> I kinda want to do an s/postclose/close/ using cocci anyway, so this isn't
> the last series on this topic.

And merged with Chris' irc r-b.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx