Re: [Intel-gfx] [PATCH v7 1/3] drm/i915: Get active pending request for given context

[Chris Wilson <chris@chris-wilson.co.uk>]

Quoting Ankit Navik (2020-03-16 13:29:49)
> This patch gives us the active pending request count which is yet
> to be submitted to the GPU.
> 
> V2:
>  * Change 64-bit to atomic for request count. (Tvrtko Ursulin)
> 
> V3:
>  * Remove mutex for request count.
>  * Rebase.
>  * Fixes hitting underflow for predictive request. (Tvrtko Ursulin)
> 
> V4:
>  * Rebase.
> 
> V5:
>  * Rebase.
> 
> V6:
>  * Rebase.
> 
> V7:
>  * Rebase.
>  * Add GEM_BUG_ON for req_cnt.
> 
> Cc: Vipin Anand <vipin.anand@intel.com>
> Signed-off-by: Ankit Navik <ankit.p.navik@intel.com>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_context.c       | 1 +
>  drivers/gpu/drm/i915/gem/i915_gem_context_types.h | 5 +++++
>  drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c    | 2 ++
>  drivers/gpu/drm/i915/gt/intel_lrc.c               | 9 +++++++++
>  4 files changed, 17 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index 026999b34abd..d0ff999429ff 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -879,6 +879,7 @@ i915_gem_create_context(struct drm_i915_private *i915, unsigned int flags)
>         }
>  
>         trace_i915_context_create(ctx);
> +       atomic_set(&ctx->req_cnt, 0);
>  
>         return ctx;
>  }
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
> index 28760bd03265..a9ba13f8865e 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
> @@ -171,6 +171,11 @@ struct i915_gem_context {
>          */
>         struct radix_tree_root handles_vma;
>  
> +       /** req_cnt: tracks the pending commands, based on which we decide to
> +        * go for low/medium/high load configuration of the GPU.
> +        */
> +       atomic_t req_cnt;
> +
>         /**
>          * @name: arbitrary name, used for user debug
>          *
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> index d3f4f28e9468..f90c968f95cd 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> @@ -2565,6 +2565,8 @@ i915_gem_do_execbuffer(struct drm_device *dev,
>         if (batch->private)
>                 intel_engine_pool_mark_active(batch->private, eb.request);
>  
> +       atomic_inc(&eb.gem_context->req_cnt);
> +
>         trace_i915_request_queue(eb.request, eb.batch_flags);
>         err = eb_submit(&eb, batch);
>  err_request:
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index 112531b29f59..ccfebebb0071 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -2143,6 +2143,8 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>                         }
>  
>                         if (__i915_request_submit(rq)) {
> +                               struct i915_gem_context *ctx;
> +
>                                 if (!merge) {
>                                         *port = execlists_schedule_in(last, port - execlists->pending);
>                                         port++;
> @@ -2158,6 +2160,13 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>  
>                                 submit = true;
>                                 last = rq;
> +
> +                               ctx = rcu_dereference_protected(
> +                                       rq->context->gem_context, true);
> +
> +                               GEM_BUG_ON(atomic_read(&ctx->req_cnt));
> +                               if (atomic_read(&ctx->req_cnt) > 0)
> +                                       atomic_dec(&ctx->req_cnt);

This is wrong on so many levels. The GEM context is an opaque pointer
here, and often not available. The rcu_dereference_protected is woeful.
There is not even a 1:1 relationship between execbuf and requests -- you
should have recognised that the moment you "handled" the bug.

Please do look at the other metrics we have time and time again pointed
you towards.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx