On Wed, Apr 09, 2014 at 06:58:41PM +0100, Chris Wilson wrote: > On Wed, Apr 09, 2014 at 10:43:47AM -0700, Ben Widawsky wrote: > > On Wed, Apr 09, 2014 at 08:03:39AM +0100, Chris Wilson wrote: > > > We neither report any unfinished operations during releasing GEM objects > > > associated with the file, and even if we did, it is bad form to report > > > -EINTR from a close(). > > > > > > The root cause of the bug that first showed itself during close is that > > > we do not do proper live tracking of vma and contexts under full-ppgtt, > > > but this is useful piece of defensive programming enforcing our > > > userspace API contract. > > > > > > Cc: Ben Widawsky <[email protected]> > > > Signed-off-by: Chris Wilson <[email protected]> > > > --- > > > drivers/gpu/drm/i915/i915_dma.c | 9 +++++++++ > > > 1 file changed, 9 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_dma.c > > > b/drivers/gpu/drm/i915/i915_dma.c > > > index 24dd55a16436..d67ca8051e07 100644 > > > --- a/drivers/gpu/drm/i915/i915_dma.c > > > +++ b/drivers/gpu/drm/i915/i915_dma.c > > > @@ -1937,9 +1937,18 @@ void i915_driver_lastclose(struct drm_device * dev) > > > > > > void i915_driver_preclose(struct drm_device * dev, struct drm_file > > > *file_priv) > > > { > > > + struct drm_i915_private *dev_priv = to_i915(dev); > > > + bool was_interruptible; > > > + > > > mutex_lock(&dev->struct_mutex); > > > + was_interruptible = dev_priv->mm.interruptible; > > > + WARN_ON(!was_interruptible); > > > + dev_priv->mm.interruptible = false; > > > + > > > i915_gem_context_close(dev, file_priv); > > > i915_gem_release(dev, file_priv); > > > + > > > + dev_priv->mm.interruptible = was_interruptible; > > > mutex_unlock(&dev->struct_mutex); > > > } > > > > > > > I guess you missed: > > [email protected] > > Oops, I did. > > > True in my case, I should have put the read of > > 'dev_priv->mm.interruptible' within the lock. > > > > I don't think we need to protect gem_release. > > My argument is that I want to protect the entire preclose() as it cannot > be allowed to fail, i.e. all future bugs. > -Chris >
Reviewed-by: Ben Widawsky <[email protected]> (I haven't actually tested this patch, but it's similar enough to my patch that I think it could probably get a Tested-by too) -- Ben Widawsky, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
