[Intel-gfx] [PATCH 48/55] drm/i915: Add *_ring_begin() to request allocation

[Chris Wilson]

On Wed, Jun 17, 2015 at 04:54:42PM +0200, Daniel Vetter wrote:
> On Wed, Jun 17, 2015 at 03:27:08PM +0100, Chris Wilson wrote:
> > On Wed, Jun 17, 2015 at 03:31:59PM +0200, Daniel Vetter wrote:
> > > On Fri, May 29, 2015 at 05:44:09PM +0100, John.C.Harrison at Intel.com wrote:
> > > > From: John Harrison <John.C.Harrison at Intel.com>
> > > > 
> > > > Now that the *_ring_begin() functions no longer call the request allocation
> > > > code, it is finally safe for the request allocation code to call *_ring_begin().
> > > > This is important to guarantee that the space reserved for the subsequent
> > > > i915_add_request() call does actually get reserved.
> > > > 
> > > > v2: Renamed functions according to review feedback (Tomas Elf).
> > > > 
> > > > For: VIZ-5115
> > > > Signed-off-by: John Harrison <John.C.Harrison at Intel.com>
> > > 
> > > Still has my question open from the previos round:
> > > 
> > > http://mid.gmane.org/20150323091030.GL1349@phenom.ffwll.local
> > > 
> > > Note that this isn't all that unlikely with GuC mode since there the
> > > ringbuffer is substantially smaller (due to firmware limitations) than
> > > what we allocate ourselves right now.
> > 
> > Looking at this patch, I am still fundamentally opposed to reserving
> > space for the request. Detecting a request that wraps and cancelling
> > that request (after the appropriate WARN for the overlow) is trivial and
> > such a rare case (as it is a programming error) that it should only be
> > handled in the slow path.
> 
> I thought the entire point here that we don't have request half-committed
> because the final request ringcmds didn't fit in. And that does require
> that we reserve a bit of space for that postamble.
> 
> I guess if it's too much (atm it's super-pessimistic due to ilk) we can
> make per-platform reservation limits to be really minimal.
> 
> Maybe we could go towards a rollback model longterm of rewingind the
> ringbuffer. But if there's no clear need I'd like to avoid that
> complexity.

Even if you didn't like the rollback model which helps handling the
partial state from context switches and what not, if you run out of
ringspace you can set the GPU as wedged. Issuing a request that fills
the entire ringbuffer is a programming bug that needs to be caught very
early in development.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre