Re: [Intel-gfx] [PATCH 1/3] drm/i915/gem: Use chained reloc batches

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 01/05/2020 11:18, Chris Wilson wrote:
> The ring is a precious resource: we anticipate to only use a few hundred
> bytes for a request, and only try to reserve that before we start. If we
> go beyond our guess in building the request, then instead of waiting at
> the start of execbuf before we hold any locks or other resources, we
> may trigger a wait inside a critical region. One example is in using gpu
> relocations, where currently we emit a new MI_BB_START from the ring
> every time we overflow a page of relocation entries. However, instead of
> insert the command into the precious ring, we can chain the next page of
> relocation entries as MI_BB_START from the end of the previous.
> 
> v2: Delay the emit_bb_start until after all the chained vma
> synchronisation is complete. Since the buffer pool batches are idle, this
> _should_ be a no-op, but one day we may some fancy async GPU bindings
> for new vma!
> 
> Testcase: igt/gem_exec_reloc/basic-many-active
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   .../gpu/drm/i915/gem/i915_gem_execbuffer.c    | 130 +++++++++++++++---
>   1 file changed, 111 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> index 414859fa2673..293bf06b65b2 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> @@ -271,6 +271,7 @@ struct i915_execbuffer {
>   		struct i915_request *rq;
>   		u32 *rq_cmd;
>   		unsigned int rq_size;
> +		struct i915_vma *rq_vma;
>   	} reloc_cache;
>   
>   	u64 invalid_flags; /** Set of execobj.flags that are invalid */
> @@ -975,20 +976,111 @@ static inline struct i915_ggtt *cache_to_ggtt(struct reloc_cache *cache)
>   	return &i915->ggtt;
>   }
>   
> +static int reloc_gpu_chain(struct reloc_cache *cache)
> +{
> +	struct intel_gt_buffer_pool_node *pool;
> +	struct i915_request *rq = cache->rq;
> +	struct i915_vma *batch;
> +	u32 *cmd;
> +	int err;
> +
> +	pool = intel_gt_get_buffer_pool(rq->engine->gt, PAGE_SIZE);
> +	if (IS_ERR(pool))
> +		return PTR_ERR(pool);
> +
> +	batch = i915_vma_instance(pool->obj, rq->context->vm, NULL);
> +	if (IS_ERR(batch)) {
> +		err = PTR_ERR(batch);
> +		goto out_pool;
> +	}
> +
> +	err = i915_vma_pin(batch, 0, 0, PIN_USER | PIN_NONBLOCK);
> +	if (err)
> +		goto out_pool;
> +
> +	cmd = cache->rq_cmd + cache->rq_size;
> +	*cmd++ = MI_ARB_CHECK;
> +	if (cache->gen >= 8) {
> +		*cmd++ = MI_BATCH_BUFFER_START_GEN8;
> +		*cmd++ = lower_32_bits(batch->node.start);
> +		*cmd++ = upper_32_bits(batch->node.start);
> +	} else {
> +		*cmd++ = MI_BATCH_BUFFER_START;
> +		*cmd++ = lower_32_bits(batch->node.start);
> +	}
> +	i915_gem_object_flush_map(cache->rq_vma->obj);
> +	i915_gem_object_unpin_map(cache->rq_vma->obj);
> +	cache->rq_vma = NULL;
> +
> +	err = intel_gt_buffer_pool_mark_active(pool, rq);
> +	if (err == 0) {
> +		i915_vma_lock(batch);
> +		err = i915_request_await_object(rq, batch->obj, false);
> +		if (err == 0)
> +			err = i915_vma_move_to_active(batch, rq, 0);
> +		i915_vma_unlock(batch);
> +	}
> +	i915_vma_unpin(batch);
> +	if (err)
> +		goto out_pool;
> +
> +	cmd = i915_gem_object_pin_map(pool->obj,

batch->obj maybe to be consistent in this block? Few lines above you get 
to it via batch.

> +				      cache->has_llc ?
> +				      I915_MAP_FORCE_WB :
> +				      I915_MAP_FORCE_WC);
> +	if (IS_ERR(cmd)) {
> +		err = PTR_ERR(cmd);
> +		goto out_pool;
> +	}
> +
> +	/* Return with batch mapping (cmd) still pinned */
> +	cache->rq_cmd = cmd;
> +	cache->rq_size = 0;
> +	cache->rq_vma = batch;
> +
> +out_pool:
> +	intel_gt_buffer_pool_put(pool);
> +	return err;
> +}
> +
> +static unsigned int reloc_bb_flags(const struct reloc_cache *cache)
> +{
> +	return cache->gen > 5 ? 0 : I915_DISPATCH_SECURE;
> +}
> +
>   static void reloc_gpu_flush(struct reloc_cache *cache)
>   {
> -	struct drm_i915_gem_object *obj = cache->rq->batch->obj;
> +	struct i915_request *rq;
> +	int err;
>   
> -	GEM_BUG_ON(cache->rq_size >= obj->base.size / sizeof(u32));
> -	cache->rq_cmd[cache->rq_size] = MI_BATCH_BUFFER_END;
> +	rq = fetch_and_zero(&cache->rq);
> +	if (!rq)
> +		return;
>   
> -	__i915_gem_object_flush_map(obj, 0, sizeof(u32) * (cache->rq_size + 1));
> -	i915_gem_object_unpin_map(obj);
> +	if (cache->rq_vma) {
> +		struct drm_i915_gem_object *obj = cache->rq_vma->obj;
>   
> -	intel_gt_chipset_flush(cache->rq->engine->gt);
> +		GEM_BUG_ON(cache->rq_size >= obj->base.size / sizeof(u32));
> +		cache->rq_cmd[cache->rq_size++] = MI_BATCH_BUFFER_END;
>   
> -	i915_request_add(cache->rq);
> -	cache->rq = NULL;
> +		__i915_gem_object_flush_map(obj,
> +					    0, sizeof(u32) * cache->rq_size);
> +		i915_gem_object_unpin_map(obj);
> +	}
> +
> +	err = 0;
> +	if (rq->engine->emit_init_breadcrumb)
> +		err = rq->engine->emit_init_breadcrumb(rq);
> +	if (!err)
> +		err = rq->engine->emit_bb_start(rq,
> +						rq->batch->node.start,
> +						PAGE_SIZE,
> +						reloc_bb_flags(cache));
> +	if (err)
> +		i915_request_set_error_once(rq, err);

Will this error propagate and fail the execbuf?

> +
> +	intel_gt_chipset_flush(rq->engine->gt);
> +	i915_request_add(rq);
>   }
>   
>   static void reloc_cache_reset(struct reloc_cache *cache)
> @@ -1237,12 +1329,6 @@ static int __reloc_gpu_alloc(struct i915_execbuffer *eb,
>   	if (err)
>   		goto err_request;
>   
> -	err = eb->engine->emit_bb_start(rq,
> -					batch->node.start, PAGE_SIZE,
> -					cache->gen > 5 ? 0 : I915_DISPATCH_SECURE);
> -	if (err)
> -		goto skip_request;
> -
>   	i915_vma_lock(batch);
>   	err = i915_request_await_object(rq, batch->obj, false);
>   	if (err == 0)
> @@ -1257,6 +1343,7 @@ static int __reloc_gpu_alloc(struct i915_execbuffer *eb,
>   	cache->rq = rq;
>   	cache->rq_cmd = cmd;
>   	cache->rq_size = 0;
> +	cache->rq_vma = batch;
>   
>   	/* Return with batch mapping (cmd) still pinned */
>   	goto out_pool;
> @@ -1280,13 +1367,9 @@ static u32 *reloc_gpu(struct i915_execbuffer *eb,
>   {
>   	struct reloc_cache *cache = &eb->reloc_cache;
>   	u32 *cmd;
> -
> -	if (cache->rq_size > PAGE_SIZE/sizeof(u32) - (len + 1))
> -		reloc_gpu_flush(cache);
> +	int err;
>   
>   	if (unlikely(!cache->rq)) {
> -		int err;
> -
>   		if (!intel_engine_can_store_dword(eb->engine))
>   			return ERR_PTR(-ENODEV);
>   
> @@ -1295,6 +1378,15 @@ static u32 *reloc_gpu(struct i915_execbuffer *eb,
>   			return ERR_PTR(err);
>   	}
>   
> +	if (unlikely(cache->rq_size + len > PAGE_SIZE / sizeof(u32) - 4)) {

4 dwords for the chain, ok.

> +		err = reloc_gpu_chain(cache);
> +		if (unlikely(err)) {
> +			i915_request_set_error_once(cache->rq, err);
> +			return ERR_PTR(err);
> +		}
> +	}
> +
> +	GEM_BUG_ON(cache->rq_size + len >= PAGE_SIZE  / sizeof(u32));
>   	cmd = cache->rq_cmd + cache->rq_size;
>   	cache->rq_size += len;
>   
> 

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx