From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Widawsky Subject: Re: [PATCH] drm/i915: Prevent signals from interrupting close() Date: Thu, 10 Apr 2014 23:44:34 -0700 Message-ID: <20140411064434.GG7387@bwidawsk.net> References: <1397027019-16634-1-git-send-email-chris@chris-wilson.co.uk> <20140409174346.GA2704@intel.com> <20140409175841.GD14192@nuc-i3427.alporthouse.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail.bwidawsk.net (bwidawsk.net [166.78.191.112]) by gabe.freedesktop.org (Postfix) with ESMTP id 6F9336E352 for ; Thu, 10 Apr 2014 23:44:48 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20140409175841.GD14192@nuc-i3427.alporthouse.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Chris Wilson , Ben Widawsky , intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org 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 > > > 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 > > 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 (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