[Intel-gfx] [PATCH] drm/i915: Fix EIO/wedged handling in gem fault handler

[Chris Wilson]

On Thu, Sep 04, 2014 at 08:17:42AM +0100, Chris Wilson wrote:
> On Thu, Sep 04, 2014 at 09:11:23AM +0200, Daniel Vetter wrote:
> > In
> > 
> > commit 1f83fee08d625f8d0130f9fe5ef7b17c2e022f3c
> > Author: Daniel Vetter <daniel.vetter at ffwll.ch>
> > Date:   Thu Nov 15 17:17:22 2012 +0100
> > 
> >     drm/i915: clear up wedged transitions
> > 
> > I've accidentally inverted the EIO/wedged handling in the fault
> > handler: We want to return the EIO as a SIGBUS only if it's not
> > because of the gpu having died, to prevent userspace from unduly
> > dying.
> > 
> > In my defence the comment right above is completely misleading, so fix
> > both.
> > 
> > v2: Drop the WARN_ON, it's not actually a bug to e.g. receive an -EIO
> > when swap-in fails.
> 
> You mean drop the WARN_ON and the if...
>  
> > Reported-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: stable at vger.kernel.org
> > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 6c685708a516..247ad18b8c3e 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -1596,9 +1596,12 @@ unlock:
> >  out:
> >  	switch (ret) {
> >  	case -EIO:
> > -		/* If this -EIO is due to a gpu hang, give the reset code a
> > -		 * chance to clean up the mess. Otherwise return the proper
> > -		 * SIGBUS. */
> > +		/*
> > +		 * We eat errors when the gpu is terminally wedged to avoid
> > +		 * userspace unduly crashing (gl has no provisions for mmaps to
> > +		 * fail). But any other -EIO isn't ours (e.g. swap in failure)
> > +		 * and so needs to be reported.
> > +		 */
> >  		if (i915_terminally_wedged(&dev_priv->gpu_error)) {
> 
> Because you didn't actually make the change you set out to do :)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 377d619827bb..b11fbe9332d5 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1541,23 +1541,21 @@ unlock:
        mutex_unlock(&dev->struct_mutex);
 out:
        switch (ret) {
-       case -EIO:
-               /* If this -EIO is due to a gpu hang, give the reset code a
-                * chance to clean up the mess. Otherwise return the proper
-                * SIGBUS. */
-               if (!i915_terminally_wedged(&dev_priv->gpu_error)) {
-                       ret = VM_FAULT_SIGBUS;
-                       break;
-               }
+       case 0:
+               /*
+                * Success. You may proceed.
+                */
        case -EAGAIN:
                /*
                 * EAGAIN means the gpu is hung and we'll wait for the error
                 * handler to reset everything when re-faulting in
                 * i915_mutex_lock_interruptible.
                 */
-       case 0:
        case -ERESTARTSYS:
        case -EINTR:
+               /* Signal interruptus. Return to userspace and await
+                * further instructions.
+                */
        case -EBUSY:
                /*
                 * EBUSY is ok: this just means that another thread
@@ -1568,6 +1566,20 @@ out:
        case -ENOMEM:
                ret = VM_FAULT_OOM;
                break;
+       case -EIO:
+               /* The driver should never report an EIO due to GPU hangs,
+                * all requests shold have been reset and the object should
+                * be idle. There should be no impediment for us to mmap the
+                * object in those cases. It would be nice if we could throw
+                * a warning in such cases, but...
+                *
+                * EIO can also arise from a failure to swap in pages,
+                * for example. Here, we must report SIGBUS as there is
+                * no recovery for memory corruption.
+                *
+                * And since we can have a wedged GPU and swap failure,
+                * having a WARN here could be misleading.
+                */
        case -ENOSPC:
        case -EFAULT:
                ret = VM_FAULT_SIGBUS;


-- 
Chris Wilson, Intel Open Source Technology Centre