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

John Harrison
2015-02-27T12:34:32Z

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