Re: [PATCH] drm/i915: Ignore -EIO from __i915_wait_request() during mmio flip

From: Daniel Vetter <daniel@ffwll.ch>
To: Chris Wilson <chris@chris-wilson.co.uk>,
	Daniel Vetter <daniel@ffwll.ch>,
	ville.syrjala@linux.intel.com, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915: Ignore -EIO from __i915_wait_request() during mmio flip
Date: Tue, 16 Jun 2015 18:21:53 +0200	[thread overview]
Message-ID: <20150616162153.GA23637@phenom.ffwll.local> (raw)
In-Reply-To: <20150616121033.GW28462@nuc-i3427.alporthouse.com>

On Tue, Jun 16, 2015 at 01:10:33PM +0100, Chris Wilson wrote:
> On Mon, Jun 15, 2015 at 06:34:51PM +0200, Daniel Vetter wrote:
> > On Thu, Jun 11, 2015 at 09:01:08PM +0100, Chris Wilson wrote:
> > > On Thu, Jun 11, 2015 at 07:14:28PM +0300, ville.syrjala@linux.intel.com wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > When the GPU gets reset __i915_wait_request() returns -EIO to the
> > > > mmio flip worker. Currently we WARN whenever we get anything other
> > > > than 0. Ignore the -EIO too since it's a perfectly normal thing
> > > > to get during a GPU reset.
> > > 
> > > Nak. I consider it is a bug in __i915_wait_request(). I am discussing
> > > with Thomas Elf how to fix this wrt the next generation of individual
> > > ring resets.
> > 
> > We should only get an -EIO if the gpu is truly gone, but an -EAGAIN when
> > the reset is ongoing. Neither is currently handled. For lockless users we
> > probably want a version of wait_request which just dtrt (of waiting for
> > the reset handler to complete without trying to grab the mutex and then
> > returning). Or some other means of retrying.
> > 
> > Returning -EIO from the low-level wait function still seems appropriate,
> > but callers need to eat/handle it appropriately. WARN_ON isn't it here
> > ofc.
> 
> Bleh, a few years ago you decided not to take the EIO handling along the
> call paths that don't care.
> 
> I disagree. There are two classes of callers, those that care about
> EIO/EAGAIN and those that simply want to know when the GPU is no longer
> processing that request. That latter class is still popping up in
> bugzilla with frozen displays. For the former, we actually only care
> about backoff if we are holding the mutex - and that is only required
> for EAGAIN. The only user that cares about EIO is throttle().

Hm, right now the design is that for non-interruptible designs we indeed
return -EIO or -EAGAIN, but the reset handler will fix up outstanding
flips. So I guess removing the WARN_ON here is indeed the right thing to
do. We should probably change this once we have atomic (where the wait
doesn't need a lock really, at least for async commits which is what
matters here) and loop until completion.

I'm still vary of eating -EIO in general since it's so hard to test all
this for correctness. Maybe we need a __check_wedge which can return -EIO
and a check_wedge which eats it. And then decide once for where to put
special checks, probably just execbuf and throttle.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx