| Message ID | 20170308141257.12119-13-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. -Chris
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. -Daniel
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
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index b1e9027a4f80..1e511d17684e 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1423,17 +1423,14 @@ static void i915_driver_lastclose(struct drm_device *dev) vga_switcheroo_process_delayed_switch(); } -static void i915_driver_preclose(struct drm_device *dev, struct drm_file *file) +static void i915_driver_postclose(struct drm_device *dev, struct drm_file *file) { + struct drm_i915_file_private *file_priv = file->driver_priv; + mutex_lock(&dev->struct_mutex); i915_gem_context_close(dev, file); i915_gem_release(dev, file); mutex_unlock(&dev->struct_mutex); -} - -static void i915_driver_postclose(struct drm_device *dev, struct drm_file *file) -{ - struct drm_i915_file_private *file_priv = file->driver_priv; kfree(file_priv); } @@ -2638,7 +2635,6 @@ static struct drm_driver driver = { .release = i915_driver_release, .open = i915_driver_open, .lastclose = i915_driver_lastclose, - .preclose = i915_driver_preclose, .postclose = i915_driver_postclose, .set_busid = drm_pci_set_busid,
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. In the past this was more relevant when drivers still had to track and clean up pending drm events, but that's all done by the core now. Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/i915/i915_drv.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)