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

From: Daniel Vetter <daniel@ffwll.ch>
To: Chris Wilson <chris@chris-wilson.co.uk>,
	Daniel Vetter <daniel@ffwll.ch>,
	John.C.Harrison@Intel.com, Intel-GFX@Lists.FreeDesktop.Org
Subject: Re: [PATCH 48/55] drm/i915: Add *_ring_begin() to request allocation
Date: Wed, 17 Jun 2015 16:54:42 +0200	[thread overview]
Message-ID: <20150617145442.GA23637@phenom.ffwll.local> (raw)
In-Reply-To: <20150617142708.GJ24012@nuc-i3427.alporthouse.com>

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@Intel.com wrote:
> > > From: John Harrison <John.C.Harrison@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@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.
-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