Re: [PATCH v2] drm/i915: recheck ringspace after wait+retire

From: Daniel Vetter <daniel@ffwll.ch>
To: Dave Gordon <david.s.gordon@intel.com>
Cc: "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH v2] drm/i915: recheck ringspace after wait+retire
Date: Thu, 18 Dec 2014 21:19:57 +0100	[thread overview]
Message-ID: <20141218201957.GR2711@phenom.ffwll.local> (raw)
In-Reply-To: <5493184E.3090203@intel.com>

On Thu, Dec 18, 2014 at 06:09:18PM +0000, Dave Gordon wrote:
> On 18/12/14 17:18, Chris Wilson wrote:
> > On Thu, Dec 18, 2014 at 05:03:41PM +0000, Dave Gordon wrote:
> >> In {intel,logical}_ring_wait_request(), we try to find a request
> >> whose completion will release the amount of ring space required.
> >> If we find such a request, we wait for it, and then retire it, in
> >> the expectation that we will now have at least the required amount
> >> of free space in the ring. But it's good to check that this has
> >> actually happened, so we can back out with a meaningful error
> >> code if something unexpected has happened, such as wait_request
> >> returning early.
> > 
> > It's pretty pointless. It's a programming bug, if anything it should
> > WARN if it happens.
> > -Chris
> 
> I don't regard preventing the propagation of errors as pointless,
> whether or not it's a programming bug. If this case arose, then without
> this check we would go ahead and trample over whatever was next in the
> ring. Quite likely TAIL would overtake HEAD, and then all sorts of
> bizarre things would happen some time later.
> 
> In particular, this may be able to catch bugs introduced by /future
> features/, such as TDR, scheduling, and preemption, all of which are in
> preparation now and all of which have the potential to disrupt the
> predictable flow of requests. So it's good to be resilient to upcoming
> changes and make it as easy as possible to catch bugs at the earliest
> opportunity.
> 
> This is also a step towards the deduplication of the ringbuffer and LRC
> paths by making sure that they are as similar as possible, and thus
> simple to merge at some later opportunity.

Overwriting the TAIL is a fairly benign disaster as far as gpus are
concerned, the gpu tends to survive those. I concur with Chris that a
simple WARN_ON for paranoia is good enough. And I don't think we need the
comments, imo the code is clear enough as-is.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx