From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Widawsky Subject: Re: [PATCH] drm/i915: Prevent signals from interrupting close() Date: Wed, 9 Apr 2014 10:43:47 -0700 Message-ID: <20140409174346.GA2704@intel.com> References: <1397027019-16634-1-git-send-email-chris@chris-wilson.co.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id E36306EB6A for ; Wed, 9 Apr 2014 10:46:50 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1397027019-16634-1-git-send-email-chris@chris-wilson.co.uk> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Chris Wilson Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org 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 > Signed-off-by: Chris Wilson > --- > 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: 1396905423-19453-1-git-send-email-benjamin.widawsky@intel.com 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. -- Ben Widawsky, Intel Open Source Technology Center