Re: [PATCH] drm/i915/gem: Reduce ggtt_flush() from a wait-for-idle to a mere barrier flush

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Quoting Chris Wilson (2019-11-20 16:42:46)
> Since we use barriers, we need only explicitly flush those barriers to
> ensure tha we can reclaim the available ggtt for ourselves. The barrier
> flush was implicit inside the intel_gt_wait_for_idle() -- except because
> we use i915_gem_evict from inside an active timeline during execbuf, we
> could easily end up waiting upon ourselves.
> 
> v2: Wait on the barriers to ensure that any context unpinning that can
> be done, will be done.
> 
> Fixes: 7936a22dd466 ("drm/i915/gt: Wait for new requests in intel_gt_retire_requests()")
> Fixes: a46bfdc83fee ("drm/i915/gt: Wait for new requests in intel_gt_retire_requests()")
> Testcase: igt/gem_exec_reloc/basic-range
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> ---
>  .../gpu/drm/i915/gt/intel_engine_heartbeat.c  |  8 +++-
>  .../gpu/drm/i915/gt/intel_engine_heartbeat.h  |  4 +-
>  drivers/gpu/drm/i915/gt/selftest_context.c    | 38 ++++---------------
>  .../drm/i915/gt/selftest_engine_heartbeat.c   |  7 +++-
>  drivers/gpu/drm/i915/i915_gem_evict.c         | 26 ++++++++++++-
>  5 files changed, 48 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
> index c91fd4e4af29..0173720af05a 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
> @@ -212,10 +212,14 @@ int intel_engine_pulse(struct intel_engine_cs *engine)
>         return err;
>  }
>  
> -int intel_engine_flush_barriers(struct intel_engine_cs *engine)
> +int intel_engine_flush_barriers(struct intel_engine_cs *engine,
> +                               struct i915_request **out)
>  {
>         struct i915_request *rq;
>  
> +       if (out)
> +               *out = NULL;
> +
>         if (llist_empty(&engine->barrier_tasks))
>                 return 0;
>  
> @@ -224,6 +228,8 @@ int intel_engine_flush_barriers(struct intel_engine_cs *engine)
>                 return PTR_ERR(rq);
>  
>         idle_pulse(engine, rq);
> +       if (out)
> +               *out = i915_request_get(rq);
>         i915_request_add(rq);
>  
>         return 0;
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.h b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.h
> index a7b8c0f9e005..17e973d86f5c 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.h
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.h
> @@ -7,6 +7,7 @@
>  #ifndef INTEL_ENGINE_HEARTBEAT_H
>  #define INTEL_ENGINE_HEARTBEAT_H
>  
> +struct i915_request;
>  struct intel_engine_cs;
>  
>  void intel_engine_init_heartbeat(struct intel_engine_cs *engine);
> @@ -18,6 +19,7 @@ void intel_engine_park_heartbeat(struct intel_engine_cs *engine);
>  void intel_engine_unpark_heartbeat(struct intel_engine_cs *engine);
>  
>  int intel_engine_pulse(struct intel_engine_cs *engine);
> -int intel_engine_flush_barriers(struct intel_engine_cs *engine);
> +int intel_engine_flush_barriers(struct intel_engine_cs *engine,
> +                               struct i915_request **barrier);
>  
>  #endif /* INTEL_ENGINE_HEARTBEAT_H */
> diff --git a/drivers/gpu/drm/i915/gt/selftest_context.c b/drivers/gpu/drm/i915/gt/selftest_context.c
> index 3586af636304..0c0f130802fb 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_context.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_context.c
> @@ -41,33 +41,6 @@ static int request_sync(struct i915_request *rq)
>         return err;
>  }
>  
> -static int context_sync(struct intel_context *ce)
> -{
> -       struct intel_timeline *tl = ce->timeline;
> -       int err = 0;
> -
> -       mutex_lock(&tl->mutex);
> -       do {
> -               struct dma_fence *fence;
> -               long timeout;
> -
> -               fence = i915_active_fence_get(&tl->last_request);
> -               if (!fence)
> -                       break;
> -
> -               timeout = dma_fence_wait_timeout(fence, false, HZ / 10);
> -               if (timeout < 0)
> -                       err = timeout;
> -               else
> -                       i915_request_retire_upto(to_request(fence));
> -
> -               dma_fence_put(fence);
> -       } while (!err);
> -       mutex_unlock(&tl->mutex);
> -
> -       return err;
> -}
> -
>  static int __live_context_size(struct intel_engine_cs *engine,
>                                struct i915_gem_context *fixme)
>  {
> @@ -202,6 +175,7 @@ static int __live_active_context(struct intel_engine_cs *engine,
>                                  struct i915_gem_context *fixme)
>  {
>         unsigned long saved_heartbeat;
> +       struct i915_request *barrier;
>         struct intel_context *ce;
>         int pass;
>         int err;
> @@ -269,17 +243,21 @@ static int __live_active_context(struct intel_engine_cs *engine,
>         }
>  
>         /* Now make sure our idle-barriers are flushed */
> -       err = intel_engine_flush_barriers(engine);
> +       err = intel_engine_flush_barriers(engine, &barrier);
>         if (err)
>                 goto err;
>  
> -       err = context_sync(engine->kernel_context);
> -       if (err)
> +       if (i915_request_wait(barrier, 0, HZ / 5) < 0) {
> +               i915_request_put(barrier);
> +               err = -ETIME;
>                 goto err;
> +       }
> +       i915_request_put(barrier);
>  
>         if (!i915_active_is_idle(&ce->active)) {
>                 pr_err("context is still active!");
>                 err = -EINVAL;
> +               goto err;
>         }
>  
>         if (intel_engine_pm_is_awake(engine)) {
> diff --git a/drivers/gpu/drm/i915/gt/selftest_engine_heartbeat.c b/drivers/gpu/drm/i915/gt/selftest_engine_heartbeat.c
> index f665a0e23c61..0bd9afc20ef3 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_engine_heartbeat.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_engine_heartbeat.c
> @@ -115,6 +115,11 @@ static int __live_idle_pulse(struct intel_engine_cs *engine,
>         return err;
>  }
>  
> +static int wrap_engine_flush_barriers(struct intel_engine_cs *engine)
> +{
> +       return intel_engine_flush_barriers(engine, NULL);
> +}
> +
>  static int live_idle_flush(void *arg)
>  {
>         struct intel_gt *gt = arg;
> @@ -126,7 +131,7 @@ static int live_idle_flush(void *arg)
>  
>         for_each_engine(engine, gt, id) {
>                 intel_engine_pm_get(engine);
> -               err = __live_idle_pulse(engine, intel_engine_flush_barriers);
> +               err = __live_idle_pulse(engine, wrap_engine_flush_barriers);
>                 intel_engine_pm_put(engine);
>                 if (err)
>                         break;
> diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
> index 7e62c310290f..91daf87f491e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_evict.c
> +++ b/drivers/gpu/drm/i915/i915_gem_evict.c
> @@ -28,7 +28,7 @@
>  
>  #include <drm/i915_drm.h>
>  
> -#include "gem/i915_gem_context.h"
> +#include "gt/intel_engine_heartbeat.h"
>  #include "gt/intel_gt_requests.h"
>  
>  #include "i915_drv.h"
> @@ -40,6 +40,9 @@ I915_SELFTEST_DECLARE(static struct igt_evict_ctl {
>  
>  static int ggtt_flush(struct intel_gt *gt)
>  {
> +       struct intel_engine_cs *engine;
> +       enum intel_engine_id id;
> +
>         /*
>          * Not everything in the GGTT is tracked via vma (otherwise we
>          * could evict as required with minimal stalling) so we are forced
> @@ -47,7 +50,26 @@ static int ggtt_flush(struct intel_gt *gt)
>          * the hopes that we can then remove contexts and the like only
>          * bound by their active reference.
>          */
> -       return intel_gt_wait_for_idle(gt, MAX_SCHEDULE_TIMEOUT);
> +       intel_gt_retire_requests(gt);
> +       for_each_engine(engine, gt, id) {
> +               struct i915_request *barrier;
> +               long err;
> +
> +               /* A barrier will unpin anything that is ready to be unpinned */
> +               err = intel_engine_flush_barriers(engine, &barrier);
> +               if (err)
> +                       return err;
> +
> +               err = i915_request_wait(barrier,
> +                                       I915_WAIT_INTERRUPTIBLE,
> +                                       MAX_SCHEDULE_TIMEOUT);
> +               i915_request_put(barrier);
> +               if (err)
> +                       return err;

It's still weaker than it was before, I can keep papering over it. :|
Long term plan is pipelined evictions. Oh boy.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux