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
