Re: [PATCH 3/8] drm/i915: Automatic i915_switch_context for legacy

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> During request construction, after pinning the context we know whether
> or not we have to emit a context switch. So move this common operation
> from every caller into i915_gem_request_alloc() itself.
>
> v2: Always submit the request if we emitted some commands during request
> construction, as typically it also involves changes in global state.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem.c                   |  2 +-
>  drivers/gpu/drm/i915/i915_gem_context.c           |  7 +------
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c        |  8 --------
>  drivers/gpu/drm/i915/i915_gem_request.c           | 18 +++++++++++++-----
>  drivers/gpu/drm/i915/i915_perf.c                  |  3 +--
>  drivers/gpu/drm/i915/intel_ringbuffer.c           |  4 ++++
>  drivers/gpu/drm/i915/selftests/huge_pages.c       |  4 ----
>  drivers/gpu/drm/i915/selftests/i915_gem_context.c |  4 ----
>  drivers/gpu/drm/i915/selftests/i915_gem_request.c | 10 ----------
>  drivers/gpu/drm/i915/selftests/intel_hangcheck.c  |  4 ----
>  10 files changed, 20 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index a7979b74ce21..d885cf0d2943 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -5043,7 +5043,7 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
>  			goto out_ctx;
>  		}
>  
> -		err = i915_switch_context(rq);
> +		err = 0;
>  		if (engine->init_context)
>  			err = engine->init_context(rq);
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 2db040695035..c1efbaf02bf2 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -842,8 +842,7 @@ int i915_switch_context(struct drm_i915_gem_request *req)
>  	struct intel_engine_cs *engine = req->engine;
>  
>  	lockdep_assert_held(&req->i915->drm.struct_mutex);
> -	if (i915_modparams.enable_execlists)
> -		return 0;
> +	GEM_BUG_ON(i915_modparams.enable_execlists);
>  
>  	if (!req->ctx->engine[engine->id].state) {
>  		struct i915_gem_context *to = req->ctx;
> @@ -899,7 +898,6 @@ int i915_gem_switch_to_kernel_context(struct drm_i915_private *dev_priv)
>  
>  	for_each_engine(engine, dev_priv, id) {
>  		struct drm_i915_gem_request *req;
> -		int ret;
>  
>  		if (engine_has_idle_kernel_context(engine))
>  			continue;
> @@ -922,10 +920,7 @@ int i915_gem_switch_to_kernel_context(struct drm_i915_private *dev_priv)
>  								 GFP_KERNEL);
>  		}
>  
> -		ret = i915_switch_context(req);
>  		i915_add_request(req);
> -		if (ret)
> -			return ret;
>  	}
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 435ed95df144..85c7e8afe26e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1115,10 +1115,6 @@ static int __reloc_gpu_alloc(struct i915_execbuffer *eb,
>  	if (err)
>  		goto err_request;
>  
> -	err = i915_switch_context(rq);
> -	if (err)
> -		goto err_request;
> -
>  	err = eb->engine->emit_bb_start(rq,
>  					batch->node.start, PAGE_SIZE,
>  					cache->gen > 5 ? 0 : I915_DISPATCH_SECURE);
> @@ -1965,10 +1961,6 @@ static int eb_submit(struct i915_execbuffer *eb)
>  	if (err)
>  		return err;
>  
> -	err = i915_switch_context(eb->request);
> -	if (err)
> -		return err;
> -
>  	if (eb->args->flags & I915_EXEC_GEN7_SOL_RESET) {
>  		err = i915_reset_gen7_sol_offsets(eb->request);
>  		if (err)
> diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
> index e0d6221022a8..445495f9893c 100644
> --- a/drivers/gpu/drm/i915/i915_gem_request.c
> +++ b/drivers/gpu/drm/i915/i915_gem_request.c
> @@ -624,6 +624,10 @@ i915_gem_request_alloc(struct intel_engine_cs *engine,
>  	if (ret)
>  		goto err_unpin;
>  
> +	ret = intel_ring_wait_for_space(ring, MIN_SPACE_FOR_ADD_REQUEST);
> +	if (ret)
> +		goto err_unreserve;
> +
>  	/* Move the oldest request to the slab-cache (if not in use!) */
>  	req = list_first_entry_or_null(&engine->timeline->requests,
>  				       typeof(*req), link);
> @@ -703,10 +707,6 @@ i915_gem_request_alloc(struct intel_engine_cs *engine,
>  	req->reserved_space = MIN_SPACE_FOR_ADD_REQUEST;
>  	GEM_BUG_ON(req->reserved_space < engine->emit_breadcrumb_sz);
>  
> -	ret = engine->request_alloc(req);
> -	if (ret)
> -		goto err_ctx;
> -
>  	/* Record the position of the start of the request so that
>  	 * should we detect the updated seqno part-way through the
>  	 * GPU processing the request, we never over-estimate the
> @@ -714,16 +714,24 @@ i915_gem_request_alloc(struct intel_engine_cs *engine,
>  	 */
>  	req->head = req->ring->emit;
>  
> +	ret = engine->request_alloc(req);
> +	if (ret)
> +		goto err_ctx;
> +

Ok as we don't emit anything with request alloc you can switch the
order.
>  	/* Check that we didn't interrupt ourselves with a new request */
>  	GEM_BUG_ON(req->timeline->seqno != req->fence.seqno);
>  	return req;
>  
>  err_ctx:
> +	if (req->ring->emit != req->head) {
> +		__i915_add_request(req, false);
> +		return ERR_PTR(ret);

..so why this check. What could emit and why the early return?


And also more in generally, we do drop some error return patchs
due to removing switch_context(). Is there a risk of ignoring
and not propagating some error code we did actually catch
at i915_switch_context()?

-Mika


> +	}
> +
>  	/* Make sure we didn't add ourselves to external state before freeing */
>  	GEM_BUG_ON(!list_empty(&req->active_list));
>  	GEM_BUG_ON(!list_empty(&req->priotree.signalers_list));
>  	GEM_BUG_ON(!list_empty(&req->priotree.waiters_list));
> -
>  	kmem_cache_free(dev_priv->requests, req);
>  err_unreserve:
>  	unreserve_engine(engine);
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index 00be015e01df..d8952ff8e6b7 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -1726,10 +1726,9 @@ static int gen8_switch_to_updated_kernel_context(struct drm_i915_private *dev_pr
>  							 GFP_KERNEL);
>  	}
>  
> -	ret = i915_switch_context(req);
>  	i915_add_request(req);
>  
> -	return ret;
> +	return 0;
>  }
>  
>  /*
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 12e734b29463..be98868115bf 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1592,6 +1592,10 @@ static int ring_request_alloc(struct drm_i915_gem_request *request)
>  	if (ret)
>  		return ret;
>  
> +	ret = i915_switch_context(request);
> +	if (ret)
> +		return ret;
> +
>  	request->reserved_space -= LEGACY_REQUEST_SIZE;
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/i915/selftests/huge_pages.c b/drivers/gpu/drm/i915/selftests/huge_pages.c
> index 01af540b6ef9..eb142bd4c9e1 100644
> --- a/drivers/gpu/drm/i915/selftests/huge_pages.c
> +++ b/drivers/gpu/drm/i915/selftests/huge_pages.c
> @@ -993,10 +993,6 @@ static int gpu_write(struct i915_vma *vma,
>  	if (err)
>  		goto err_request;
>  
> -	err = i915_switch_context(rq);
> -	if (err)
> -		goto err_request;
> -
>  	err = rq->engine->emit_bb_start(rq,
>  					batch->node.start, batch->node.size,
>  					flags);
> diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_context.c b/drivers/gpu/drm/i915/selftests/i915_gem_context.c
> index def5052862ae..61fcfa2c4dfd 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/selftests/i915_gem_context.c
> @@ -162,10 +162,6 @@ static int gpu_fill(struct drm_i915_gem_object *obj,
>  	if (err)
>  		goto err_request;
>  
> -	err = i915_switch_context(rq);
> -	if (err)
> -		goto err_request;
> -
>  	flags = 0;
>  	if (INTEL_GEN(vm->i915) <= 5)
>  		flags |= I915_DISPATCH_SECURE;
> diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_request.c b/drivers/gpu/drm/i915/selftests/i915_gem_request.c
> index a999161e8db1..9a35ebd5c876 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_gem_request.c
> +++ b/drivers/gpu/drm/i915/selftests/i915_gem_request.c
> @@ -463,10 +463,6 @@ empty_request(struct intel_engine_cs *engine,
>  	if (err)
>  		goto out_request;
>  
> -	err = i915_switch_context(request);
> -	if (err)
> -		goto out_request;
> -
>  	err = engine->emit_bb_start(request,
>  				    batch->node.start,
>  				    batch->node.size,
> @@ -678,9 +674,6 @@ static int live_all_engines(void *arg)
>  		err = engine->emit_flush(request[id], EMIT_INVALIDATE);
>  		GEM_BUG_ON(err);
>  
> -		err = i915_switch_context(request[id]);
> -		GEM_BUG_ON(err);
> -
>  		err = engine->emit_bb_start(request[id],
>  					    batch->node.start,
>  					    batch->node.size,
> @@ -800,9 +793,6 @@ static int live_sequential_engines(void *arg)
>  		err = engine->emit_flush(request[id], EMIT_INVALIDATE);
>  		GEM_BUG_ON(err);
>  
> -		err = i915_switch_context(request[id]);
> -		GEM_BUG_ON(err);
> -
>  		err = engine->emit_bb_start(request[id],
>  					    batch->node.start,
>  					    batch->node.size,
> diff --git a/drivers/gpu/drm/i915/selftests/intel_hangcheck.c b/drivers/gpu/drm/i915/selftests/intel_hangcheck.c
> index 71ce06680d66..cafe39e2e0f7 100644
> --- a/drivers/gpu/drm/i915/selftests/intel_hangcheck.c
> +++ b/drivers/gpu/drm/i915/selftests/intel_hangcheck.c
> @@ -118,10 +118,6 @@ static int emit_recurse_batch(struct hang *h,
>  	if (err)
>  		goto unpin_hws;
>  
> -	err = i915_switch_context(rq);
> -	if (err)
> -		goto unpin_hws;
> -
>  	i915_vma_move_to_active(vma, rq, 0);
>  	if (!i915_gem_object_has_active_reference(vma->obj)) {
>  		i915_gem_object_get(vma->obj);
> -- 
> 2.15.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx