| Message ID | 20191120134113.3777499-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/gem: Reduce flush_ggtt() from a wait-for-idle to a mere barrier flush | expand |
On 20/11/2019 13:41, Chris Wilson wrote: > 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. > > 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 Bugzilla: ? This test gets permanently stuck on some platforms? Regards, Tvrtko > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > drivers/gpu/drm/i915/i915_gem_evict.c | 26 +++++++++++++------------- > 1 file changed, 13 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c > index 7e62c310290f..78ca56c06a3c 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" > @@ -38,8 +38,11 @@ I915_SELFTEST_DECLARE(static struct igt_evict_ctl { > bool fail_if_busy:1; > } igt_evict_ctl;) > > -static int ggtt_flush(struct intel_gt *gt) > +static void 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,11 @@ 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) > + intel_engine_flush_barriers(engine); > + > + cond_resched(); > } > > static bool > @@ -197,11 +204,7 @@ i915_gem_evict_something(struct i915_address_space *vm, > if (I915_SELFTEST_ONLY(igt_evict_ctl.fail_if_busy)) > return -EBUSY; > > - ret = ggtt_flush(vm->gt); > - if (ret) > - return ret; > - > - cond_resched(); > + ggtt_flush(vm->gt); > > flags |= PIN_NONBLOCK; > goto search_again; > @@ -371,11 +374,8 @@ int i915_gem_evict_vm(struct i915_address_space *vm) > * pin themselves inside the global GTT and performing the > * switch otherwise is ineffective. > */ > - if (i915_is_ggtt(vm)) { > - ret = ggtt_flush(vm->gt); > - if (ret) > - return ret; > - } > + if (i915_is_ggtt(vm)) > + ggtt_flush(vm->gt); > > INIT_LIST_HEAD(&eviction_list); > list_for_each_entry(vma, &vm->bound_list, vm_link) { >
Quoting Tvrtko Ursulin (2019-11-20 15:58:49) > > On 20/11/2019 13:41, Chris Wilson wrote: > > 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. > > > > 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 > > Bugzilla: ? It's been in CI since before the w/e (the test itself is much, much older), I guess it hasn't been vetted yet as no bug has been filed. > This test gets permanently stuck on some platforms? All !full-ppgtt platforms. -Chris
On 20/11/2019 16:02, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2019-11-20 15:58:49) >> >> On 20/11/2019 13:41, Chris Wilson wrote: >>> 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. >>> >>> 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 >> >> Bugzilla: ? > > It's been in CI since before the w/e (the test itself is much, much > older), I guess it hasn't been vetted yet as no bug has been filed. > >> This test gets permanently stuck on some platforms? > > All !full-ppgtt platforms. How it will cope with actual ggtt pressure? Wait for presumably there for a reason and now it will only retire what's already done and send an idle pulse down the engines. Regards, Tvrtko
Quoting Tvrtko Ursulin (2019-11-20 16:14:40) > > On 20/11/2019 16:02, Chris Wilson wrote: > > Quoting Tvrtko Ursulin (2019-11-20 15:58:49) > >> > >> On 20/11/2019 13:41, Chris Wilson wrote: > >>> 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. > >>> > >>> 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 > >> > >> Bugzilla: ? > > > > It's been in CI since before the w/e (the test itself is much, much > > older), I guess it hasn't been vetted yet as no bug has been filed. > > > >> This test gets permanently stuck on some platforms? > > > > All !full-ppgtt platforms. > > How it will cope with actual ggtt pressure? Wait for presumably there > for a reason and now it will only retire what's already done and send an > idle pulse down the engines. Same it did previously... I've vacillated between using a flush and a wait. Originally, it was meant to just be a flush as we would wait on individual objects. But now context pinning requires waiting on barriers. Hmm, actually that would be a simple way of obtaining the previous behaviour when required. -Chris
Quoting Chris Wilson (2019-11-20 13:41:13) > 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. > > 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@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> I think we might as well just revert 7936a22dd466 and take another look at how to repeat the waits; I'm optimistic that with commit 1683d24c1470fb47716bd3ccd4e06547eb0ce0ed Author: Chris Wilson <chris@chris-wilson.co.uk> Date: Tue Nov 19 16:25:58 2019 +0000 drm/i915/gt: Move new timelines to the end of active_list the problem (e.g. igt/live_late_gt_pm) has mostly evaporated. -Chris
diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c index 7e62c310290f..78ca56c06a3c 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" @@ -38,8 +38,11 @@ I915_SELFTEST_DECLARE(static struct igt_evict_ctl { bool fail_if_busy:1; } igt_evict_ctl;) -static int ggtt_flush(struct intel_gt *gt) +static void 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,11 @@ 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) + intel_engine_flush_barriers(engine); + + cond_resched(); } static bool @@ -197,11 +204,7 @@ i915_gem_evict_something(struct i915_address_space *vm, if (I915_SELFTEST_ONLY(igt_evict_ctl.fail_if_busy)) return -EBUSY; - ret = ggtt_flush(vm->gt); - if (ret) - return ret; - - cond_resched(); + ggtt_flush(vm->gt); flags |= PIN_NONBLOCK; goto search_again; @@ -371,11 +374,8 @@ int i915_gem_evict_vm(struct i915_address_space *vm) * pin themselves inside the global GTT and performing the * switch otherwise is ineffective. */ - if (i915_is_ggtt(vm)) { - ret = ggtt_flush(vm->gt); - if (ret) - return ret; - } + if (i915_is_ggtt(vm)) + ggtt_flush(vm->gt); INIT_LIST_HEAD(&eviction_list); list_for_each_entry(vma, &vm->bound_list, vm_link) {
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. 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@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> --- drivers/gpu/drm/i915/i915_gem_evict.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-)