[Intel-gfx] [PATCH] drm/i915: Suppress spurious EIO when moving away from the GPU domain

[Daniel Vetter]

On Wed, May 1, 2013 at 3:01 PM, Chris Wilson <chris at chris-wilson.co.uk> wrote:
> On Wed, May 01, 2013 at 02:40:43PM +0200, Daniel Vetter wrote:
>> On Wed, May 1, 2013 at 1:06 PM, Chris Wilson <chris at chris-wilson.co.uk> wrote:
>> > On Wed, May 01, 2013 at 12:38:27PM +0200, Daniel Vetter wrote:
>> >> On Wed, May 1, 2013 at 12:25 PM, Chris Wilson <chris at chris-wilson.co.uk> wrote:
>> >> > If reset fails, the GPU is declared wedged. This ideally should never
>> >> > happen, but very rarely it does. After the GPU is declared wedged, we
>> >> > must allow userspace to continue to use its mapping of bo in order to
>> >> > recover its data (and in some cases in order for memory management to
>> >> > continue unabated). Obviously after the GPU is wedged, no bo are
>> >> > currently accessed by the GPU and so we can complete any waits or domain
>> >> > transitions away from the GPU. Currently, we fail this essential task
>> >> > and instead report EIO and send a SIGBUS to the affected process -
>> >> > causing major loss of data (by killing X or compiz).
>> >> >
>> >> > References: https://bugs.freedesktop.org/show_bug.cgi?id=63921
>> >> > References: https://bugs.freedesktop.org/show_bug.cgi?id=64073
>> >> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>> >>
>> >> So I've read again through the reset code and I still don't see how
>> >> wait_rendering can ever gives us -EIO once the gpu is dead. So all the
>> >> -EIO eating after wait_rendering looks really suspicious to me.
>> >
>> > That's always been a layer of defense against the driver. Alternatively,
>> > throw we can throw a warn into the wait as we shouldn't enter there with
>> > a seqno and a wedged GPU.
>>
>> Yeah, that sounds like a good idea. We really shouldn't ever have an
>> oustanding request while the gpu is wedged (ignoring dangerous changes
>> to the wedged state through debugfs).
>
> Except it is only true for a locked wait. :(

Hm, indeed ... we need to convert any -EIO into a -ERESTARTSYS in (at
least nonblocking) waits. We'd get that by simply dropping all the
check_wedge calls from the wait functions. That would leave us with a
check_wedge in throttle and intel_ring_begin, which I think are the
two places we really want that.

>> >> Now the other thing is i915_gem_object_wait_
>> >> rendering, that thing loves to throw an -EIO at us. And on a quick
>> >> check your patch misses the one in set_domain_ioctl. We probably need
>> >> to do the same with sw_finish_ioctl. So what about a
>> >> i915_mutex_lock_interruptible_no_EIO or similar to explictily annotate
>> >> the few places we don't want to hear about a dead gpu?
>> >
>> > In fact, it turns out that we hardly ever want
>> > i915_mutex_lock_interruptible to return -EIO. Long ago, the idea was
>> > that EIO was only ever returned when we tried to issue a command on the
>> > GPU whilst it was terminally wedged - in order to preserve data
>> > integrity.
>>
>> Don't we need to re-add a check in execbuf then? Otherwise I guess
>> userspace has no idea ever that something is amiss ...
>
> It will catch the EIO as soon as it tries to emit the command, but
> adding the explicit check will save a ton of work in reserving buffers.

Right, I've forgotten about the check in intel_ring_begin.

>> Otherwise I think this approach would also make sense, and feels more
>> future-proof. Applications that want real robustness simply need to
>> query the kernel through the new interface whether anything ugly has
>> happened to them. And if we want more synchronous feedback we can
>> always improve wait/set_domain_ioctl to check whether the given bo
>> suffered through a little calamity.
>
> The only explicit point we have to remember is to preserve the
> throttle() semantics of reporting EIO when wedged.

I think plugging the leak in execbuf is still good to avoid queueing
up tons of crap (since we really don't want to do that). But I think
with the -EIO check in intel_ring_begin we are covered.

>> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> > index 2bd8d7a..f243e32 100644
>> > --- a/drivers/gpu/drm/i915/i915_gem.c
>> > +++ b/drivers/gpu/drm/i915/i915_gem.c
>> > @@ -97,7 +97,7 @@ i915_gem_wait_for_error(struct i915_gpu_error *error)
>> >
>> >         /* GPU is already declared terminally dead, give up. */
>> >         if (i915_terminally_wedged(error))
>> > -               return -EIO;
>> > +               return 0;
>> >
>> >         /*
>> >          * Only wait 10 seconds for the gpu reset to complete to avoid hanging
>> > @@ -109,13 +109,12 @@ i915_gem_wait_for_error(struct i915_gpu_error *error)
>> >                                                10*HZ);
>> >         if (ret == 0) {
>> >                 DRM_ERROR("Timed out waiting for the gpu reset to complete\n");
>> > -               return -EIO;
>> > -       } else if (ret < 0) {
>> > -               return ret;
>> > -       }
>> > +               atomic_set(&error->reset_counter, I915_WEDGED);
>> > +       } else if (ret > 0)
>> > +               ret = 0;
>>
>> Less convinced about this hunk here. I think the right approach would
>> be to simply kill the timeout. Unlucky users can always get out of
>> here with a signal, and it's imo more robust if we have all relevant
>> timeouts for the reset process in the reset work. Separate patch
>> though. Also we now have quite good coverage of all the reset handling
>> in igt, so I don't think we need to go overboard in defending against
>> our own logic screw-ups ... Reset really should work nowadays.
>
> Apart from #63291 said otherwise (iirc)... I think several layers of
> paranoia over handling GPU and driver hangs is justified.

The atomic_set(WEDGED) is imo very dangerous, since it'll wreak havoc
with our accounting. But in general I really prefer an angry user with
a stuck-process backtrace than trying to paper over our own
shortcomings with hacks. And at least for the gpu hang vs.
pageflip/set_base we should now be fairly well-covered with tests. And
I haven't yet seen a report indicating that there's another issue
looming ...

I guess an obnoxious WARN with a return 0; would also work for the
time out case. But I prefer to just ditch the timeout.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch