* [PATCH] drm/i915: Prevent signals from interrupting close()
@ 2014-04-09 7:03 Chris Wilson
2014-04-09 14:49 ` Daniel Vetter
2014-04-09 17:43 ` Ben Widawsky
0 siblings, 2 replies; 8+ messages in thread
From: Chris Wilson @ 2014-04-09 7:03 UTC (permalink / raw
To: intel-gfx; +Cc: Ben Widawsky
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 <benjamin.widawsky@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
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);
}
--
1.9.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: Prevent signals from interrupting close()
2014-04-09 7:03 [PATCH] drm/i915: Prevent signals from interrupting close() Chris Wilson
@ 2014-04-09 14:49 ` Daniel Vetter
2014-04-09 15:03 ` Chris Wilson
2014-04-09 17:43 ` Ben Widawsky
1 sibling, 1 reply; 8+ messages in thread
From: Daniel Vetter @ 2014-04-09 14:49 UTC (permalink / raw
To: Chris Wilson; +Cc: intel-gfx, Ben Widawsky
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 <benjamin.widawsky@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
I'd really prefer something annoying and loud like we've done when nuking
the deferred free list in
commit 1488fc08c1706288616c602416654fd38c773deb
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date: Tue Apr 24 15:47:31 2012 +0100
drm/i915: Remove the deferred-free list
where we've added a WARN_ON in gem_free_object if any unbind was failing
due to interrupts. This patch here disables that imo useful safety check.
-Daniel
> ---
> 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);
> }
>
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: Prevent signals from interrupting close()
2014-04-09 14:49 ` Daniel Vetter
@ 2014-04-09 15:03 ` Chris Wilson
2014-04-09 16:50 ` Daniel Vetter
0 siblings, 1 reply; 8+ messages in thread
From: Chris Wilson @ 2014-04-09 15:03 UTC (permalink / raw
To: Daniel Vetter; +Cc: intel-gfx, Ben Widawsky
On Wed, Apr 09, 2014 at 04:49:02PM +0200, Daniel Vetter 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 <benjamin.widawsky@intel.com>
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>
> I'd really prefer something annoying and loud like we've done when nuking
> the deferred free list in
>
> commit 1488fc08c1706288616c602416654fd38c773deb
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date: Tue Apr 24 15:47:31 2012 +0100
>
> drm/i915: Remove the deferred-free list
>
> where we've added a WARN_ON in gem_free_object if any unbind was failing
> due to interrupts. This patch here disables that imo useful safety check.
It doesn't classify as a safety check if kernel/userspace explodes. The
trick would be to somehow get the error code back. And in this case we
cannot accept any error anyway. So yes, I would like a nice big warning
to catch bugs, but that is icing on the cake compared to preventing bugs
from destroying data.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: Prevent signals from interrupting close()
2014-04-09 15:03 ` Chris Wilson
@ 2014-04-09 16:50 ` Daniel Vetter
2014-04-09 16:57 ` Chris Wilson
0 siblings, 1 reply; 8+ messages in thread
From: Daniel Vetter @ 2014-04-09 16:50 UTC (permalink / raw
To: Chris Wilson, Daniel Vetter, intel-gfx, Ben Widawsky
On Wed, Apr 09, 2014 at 04:03:23PM +0100, Chris Wilson wrote:
> On Wed, Apr 09, 2014 at 04:49:02PM +0200, Daniel Vetter 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 <benjamin.widawsky@intel.com>
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >
> > I'd really prefer something annoying and loud like we've done when nuking
> > the deferred free list in
> >
> > commit 1488fc08c1706288616c602416654fd38c773deb
> > Author: Chris Wilson <chris@chris-wilson.co.uk>
> > Date: Tue Apr 24 15:47:31 2012 +0100
> >
> > drm/i915: Remove the deferred-free list
> >
> > where we've added a WARN_ON in gem_free_object if any unbind was failing
> > due to interrupts. This patch here disables that imo useful safety check.
>
> It doesn't classify as a safety check if kernel/userspace explodes. The
> trick would be to somehow get the error code back. And in this case we
> cannot accept any error anyway. So yes, I would like a nice big warning
> to catch bugs, but that is icing on the cake compared to preventing bugs
> from destroying data.
Hm ... double-checking: This is just for full ppgtt, right? I think in
that case I prefer if people working on it just carry this around locally.
Otherwise I need to freak out ;-)
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: Prevent signals from interrupting close()
2014-04-09 16:50 ` Daniel Vetter
@ 2014-04-09 16:57 ` Chris Wilson
0 siblings, 0 replies; 8+ messages in thread
From: Chris Wilson @ 2014-04-09 16:57 UTC (permalink / raw
To: Daniel Vetter; +Cc: intel-gfx, Ben Widawsky
On Wed, Apr 09, 2014 at 06:50:03PM +0200, Daniel Vetter wrote:
> On Wed, Apr 09, 2014 at 04:03:23PM +0100, Chris Wilson wrote:
> > On Wed, Apr 09, 2014 at 04:49:02PM +0200, Daniel Vetter 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 <benjamin.widawsky@intel.com>
> > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > >
> > > I'd really prefer something annoying and loud like we've done when nuking
> > > the deferred free list in
> > >
> > > commit 1488fc08c1706288616c602416654fd38c773deb
> > > Author: Chris Wilson <chris@chris-wilson.co.uk>
> > > Date: Tue Apr 24 15:47:31 2012 +0100
> > >
> > > drm/i915: Remove the deferred-free list
> > >
> > > where we've added a WARN_ON in gem_free_object if any unbind was failing
> > > due to interrupts. This patch here disables that imo useful safety check.
> >
> > It doesn't classify as a safety check if kernel/userspace explodes. The
> > trick would be to somehow get the error code back. And in this case we
> > cannot accept any error anyway. So yes, I would like a nice big warning
> > to catch bugs, but that is icing on the cake compared to preventing bugs
> > from destroying data.
>
> Hm ... double-checking: This is just for full ppgtt, right? I think in
> that case I prefer if people working on it just carry this around locally.
> Otherwise I need to freak out ;-)
The current bug under discussion is full-ppgtt (well, hopefully). I'd
prefer it if we could prevent driver bugs leaking out into userspace...
Pretty much everywhere we could hit an error but can't report one is
simmering trouble. (And to continue playing the fiddle, and when the
driver does explode, I expect to be able to continue using all the
memory management and modesetting intefaces.)
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: Prevent signals from interrupting close()
2014-04-09 7:03 [PATCH] drm/i915: Prevent signals from interrupting close() Chris Wilson
2014-04-09 14:49 ` Daniel Vetter
@ 2014-04-09 17:43 ` Ben Widawsky
2014-04-09 17:58 ` Chris Wilson
1 sibling, 1 reply; 8+ messages in thread
From: Ben Widawsky @ 2014-04-09 17:43 UTC (permalink / raw
To: Chris Wilson; +Cc: intel-gfx
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 <benjamin.widawsky@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: Prevent signals from interrupting close()
2014-04-09 17:43 ` Ben Widawsky
@ 2014-04-09 17:58 ` Chris Wilson
2014-04-11 6:44 ` Ben Widawsky
0 siblings, 1 reply; 8+ messages in thread
From: Chris Wilson @ 2014-04-09 17:58 UTC (permalink / raw
To: Ben Widawsky; +Cc: intel-gfx
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 <benjamin.widawsky@intel.com>
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> > 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
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/i915: Prevent signals from interrupting close()
2014-04-09 17:58 ` Chris Wilson
@ 2014-04-11 6:44 ` Ben Widawsky
0 siblings, 0 replies; 8+ messages in thread
From: Ben Widawsky @ 2014-04-11 6:44 UTC (permalink / raw
To: Chris Wilson, Ben Widawsky, intel-gfx
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 <benjamin.widawsky@intel.com>
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > ---
> > > 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 <ben@bwidawsk.net>
(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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-04-11 6:44 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-04-09 7:03 [PATCH] drm/i915: Prevent signals from interrupting close() Chris Wilson
2014-04-09 14:49 ` Daniel Vetter
2014-04-09 15:03 ` Chris Wilson
2014-04-09 16:50 ` Daniel Vetter
2014-04-09 16:57 ` Chris Wilson
2014-04-09 17:43 ` Ben Widawsky
2014-04-09 17:58 ` Chris Wilson
2014-04-11 6:44 ` Ben Widawsky
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.