[Intel-gfx] [PATCH 07/51] drm/i915: Early alloc request in execbuff

[John Harrison]

On 25/02/2015 22:22, Daniel Vetter wrote:
> On Fri, Feb 13, 2015 at 11:48:16AM +0000, John.C.Harrison at Intel.com wrote:
>> From: John Harrison <John.C.Harrison at Intel.com>
>>
>> Start of explicit request management in the execbuffer code path. This patch
>> adds a call to allocate a request structure before all the actual hardware work
>> is done. Thus guaranteeing that all that work is tagged by a known request. At
>> present, nothing further is done with the request, the rest comes later in the
>> series.
>>
>> The only noticable change is that failure to get a request (e.g. due to lack of
>> memory) will be caught earlier in the sequence. It now occurs right at the start
>> before any un-undoable work has been done.
>>
>> For: VIZ-5115
>> Signed-off-by: John Harrison <John.C.Harrison at Intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_gem_execbuffer.c |   13 ++++++++++---
>>   1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> index ca85803..61471e9 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> @@ -1356,7 +1356,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>>   	const u32 ctx_id = i915_execbuffer2_get_context_id(*args);
>>   	u32 dispatch_flags;
>>   	int ret;
>> -	bool need_relocs;
>> +	bool need_relocs, batch_pinned = false;
>>   
>>   	if (!i915_gem_check_execbuffer(args))
>>   		return -EINVAL;
>> @@ -1525,10 +1525,16 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>>   		if (ret)
>>   			goto err;
>>   
>> +		batch_pinned = true;
>>   		params->batch_obj_vm_offset = i915_gem_obj_ggtt_offset(batch_obj);
>>   	} else
>>   		params->batch_obj_vm_offset = i915_gem_obj_offset(batch_obj, vm);
>>   
>> +	/* Allocate a request for this batch buffer nice and early. */
>> +	ret = dev_priv->gt.alloc_request(ring, ctx);
>> +	if (ret)
>> +		goto err;
>> +
>>   	/*
>>   	 * Save assorted stuff away to pass through to *_submission().
>>   	 * NB: This data should be 'persistent' and not local as it will
>> @@ -1544,15 +1550,16 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>>   
>>   	ret = dev_priv->gt.do_execbuf(params, args, &eb->vmas);
>>   
>> +err:
>>   	/*
>>   	 * FIXME: We crucially rely upon the active tracking for the (ppgtt)
>>   	 * batch vma for correctness. For less ugly and less fragility this
>>   	 * needs to be adjusted to also track the ggtt batch vma properly as
>>   	 * active.
>>   	 */
>> -	if (dispatch_flags & I915_DISPATCH_SECURE)
>> +	if (batch_pinned)
>>   		i915_gem_object_ggtt_unpin(batch_obj);
>> -err:
>> +
>>   	/* the request owns the ref now */
>>   	i915_gem_context_unreference(ctx);
>>   	eb_destroy(eb);
> This hunk here looks wrong, or maybe the context changed sufficiently
> already (but I can't find that in previous patches). Why do we need to
> change the pinning for the ggtt batch pin hack when allocating the request
> earlier?
> -Daniel

It doesn't change the behaviour. It is just coping with the extra error 
path. If the alloc request call fails, we need to jump past the 
do_execbuf() call but not past the batch unpin. Hence the 'err:' tag is 
moved upwards. That means it is now possible to get to the batch unpin 
test from an error that occurred before the pin call actually happened. 
Hence it is no longer safe to just test the dispatch flag. Instead an 
explicit 'did this get pinned yet' boolean is required.