[Intel-gfx] [PATCH 2/2] drm/i915: Automatic i915_switch_context for legacy

Chris Wilson chris at chris-wilson.co.uk
Fri Nov 10 22:37:59 UTC 2017


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 at 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           | 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


More information about the Intel-gfx mailing list