Quoting Chris Wilson (2017-11-10 22:09:53)
> 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.
>
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> ---
> 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 | 19 ++++++++++++++-----
> drivers/gpu/drm/i915/intel_ringbuffer.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 ----
> 8 files changed, 20 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index a2e5a54128c1..bf4995f93357 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -5001,7 +5001,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..1c9a92e4213a 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,25 @@ i915_gem_request_alloc(struct intel_engine_cs *engine,
> */
> req->head = req->ring->emit;
>
> + ret = engine->request_alloc(req);
> + if (ret)
> + goto err_ctx;
> +
> /* Check that we didn't interrupt ourselves with a new request */
> GEM_BUG_ON(req->timeline->seqno != req->fence.seqno);
> return req;
>
> err_ctx:
> /* 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));
>
> + if (!list_empty(&req->active_list)) {
> + __i915_add_request(req, false);
> + return ERR_PTR(ret);
> + }
This was written pre-context unification, where i915_switch_context()
was using vma tracking (i.e. coupling into the req->active_list).
However, i915_switch_context() is still touching global state so this is
not safe anymore. To be safe, I think the right approach is to check to
see if the request is non-empty and if so always add the request, only
cancelling the empty request.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
