[Intel-gfx] [PATCH 08/51] drm/i915: Update alloc_request to return the allocated request

[Daniel Vetter]

On Fri, Feb 27, 2015 at 12:34:29PM +0000, John Harrison wrote:
> On 25/02/2015 21:08, Daniel Vetter wrote:
> >On Fri, Feb 13, 2015 at 12:21:29PM +0000, Chris Wilson wrote:
> >>On Fri, Feb 13, 2015 at 11:48:17AM +0000, John.C.Harrison at Intel.com wrote:
> >>>From: John Harrison <John.C.Harrison at Intel.com>
> >>>
> >>>The alloc_request() function does not actually return the newly allocated
> >>>request. Instead, it must be pulled from ring->outstanding_lazy_request. This
> >>>patch fixes this so that code can create a request and start using it knowing
> >>>exactly which request it actually owns.
> >>Why do we have different functions in the first place?
> >There seems to be a bit a layer fumble going on with the lrc alloc request
> >also pinning the lrc context. We could pull that out and then share the
> >function again since there's indeed no reason no to. At least afaics.
> >
> >Also we should probably assign the ctx (if there is any) right in the
> >request alloc function so that these two bits are always tied together.
> >-Daniel
> 
> See later patch 'set context in request creation...'. That moves the ctx
> assignment out of _i915_add_request() and into alloc_request() for the
> legacy code path. Thus bringing the legacy and lrc versions back into
> alignment. As for the pinning, I am leaving that exactly as is as I don't
> really know the ins and outs of it. One of the execlist experts might be
> able to comment as to whether that is the right place for it or not.
> 
> Also, I am currently working on the conversion to struct fence. As part of
> that, the request allocation changes quite a bit. Rather than have two
> clones of the code that have to be independently maintained, I have a patch
> to unify the common portion. We then have i915_gem_request_alloc() that all
> the driver calls instead of the indirected function pointer. That then does
> all the common work and chains on to the legacy/execlist specific helper at
> the end (which currently only sets the ringbuffer field for legacy mode and
> also does the LRC pinning for execlist mode).

That sounds exactly like what I'd want to see here. So no need for
additional frobbing in the mean time imo.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch