| Message ID | 20211129134735.628712-7-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Remove short term pins from execbuf. | expand |
On Mon, 29 Nov 2021 at 13:57, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote: > > In the next commit, we don't evict when refcount = 0. > > igt_vm_isolation() continuously tries to pin/unpin at same address, > but also calls put() on the object, which means the object may not > be unpinned in time. > > Instead of this, re-use the same object over and over, so they can > be unbound as required. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Is this something to be worried about in the real world, outside of the selftests? > --- > .../drm/i915/gem/selftests/i915_gem_context.c | 54 +++++++++++-------- > 1 file changed, 32 insertions(+), 22 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c > index b32f7fed2d9c..3fc595b57cf4 100644 > --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c > @@ -1481,10 +1481,10 @@ static int check_scratch(struct i915_address_space *vm, u64 offset) > > static int write_to_scratch(struct i915_gem_context *ctx, > struct intel_engine_cs *engine, > + struct drm_i915_gem_object *obj, > u64 offset, u32 value) > { > struct drm_i915_private *i915 = ctx->i915; > - struct drm_i915_gem_object *obj; > struct i915_address_space *vm; > struct i915_request *rq; > struct i915_vma *vma; > @@ -1497,15 +1497,9 @@ static int write_to_scratch(struct i915_gem_context *ctx, > if (err) > return err; > > - obj = i915_gem_object_create_internal(i915, PAGE_SIZE); > - if (IS_ERR(obj)) > - return PTR_ERR(obj); > - > cmd = i915_gem_object_pin_map_unlocked(obj, I915_MAP_WB); > - if (IS_ERR(cmd)) { > - err = PTR_ERR(cmd); > - goto out; > - } > + if (IS_ERR(cmd)) > + return PTR_ERR(cmd); > > *cmd++ = MI_STORE_DWORD_IMM_GEN4; > if (GRAPHICS_VER(i915) >= 8) { > @@ -1569,17 +1563,19 @@ static int write_to_scratch(struct i915_gem_context *ctx, > i915_vma_unpin(vma); > out_vm: > i915_vm_put(vm); > -out: > - i915_gem_object_put(obj); > + > + if (!err) > + err = i915_gem_object_wait(obj, 0, MAX_SCHEDULE_TIMEOUT); > + > return err; > } > > static int read_from_scratch(struct i915_gem_context *ctx, > struct intel_engine_cs *engine, > + struct drm_i915_gem_object *obj, > u64 offset, u32 *value) > { > struct drm_i915_private *i915 = ctx->i915; > - struct drm_i915_gem_object *obj; > struct i915_address_space *vm; > const u32 result = 0x100; > struct i915_request *rq; > @@ -1594,10 +1590,6 @@ static int read_from_scratch(struct i915_gem_context *ctx, > if (err) > return err; > > - obj = i915_gem_object_create_internal(i915, PAGE_SIZE); > - if (IS_ERR(obj)) > - return PTR_ERR(obj); > - > if (GRAPHICS_VER(i915) >= 8) { > const u32 GPR0 = engine->mmio_base + 0x600; > > @@ -1615,7 +1607,7 @@ static int read_from_scratch(struct i915_gem_context *ctx, > cmd = i915_gem_object_pin_map_unlocked(obj, I915_MAP_WB); > if (IS_ERR(cmd)) { > err = PTR_ERR(cmd); > - goto out; > + goto err_unpin; > } > > memset(cmd, POISON_INUSE, PAGE_SIZE); > @@ -1651,7 +1643,7 @@ static int read_from_scratch(struct i915_gem_context *ctx, > cmd = i915_gem_object_pin_map_unlocked(obj, I915_MAP_WB); > if (IS_ERR(cmd)) { > err = PTR_ERR(cmd); > - goto out; > + goto err_unpin; > } > > memset(cmd, POISON_INUSE, PAGE_SIZE); > @@ -1722,8 +1714,10 @@ static int read_from_scratch(struct i915_gem_context *ctx, > i915_vma_unpin(vma); > out_vm: > i915_vm_put(vm); > -out: > - i915_gem_object_put(obj); > + > + if (!err) > + err = i915_gem_object_wait(obj, 0, MAX_SCHEDULE_TIMEOUT); > + > return err; > } > > @@ -1765,6 +1759,7 @@ static int igt_vm_isolation(void *arg) > u64 vm_total; > u32 expected; > int err; > + struct drm_i915_gem_object *obj_a, *obj_b; Nit: Christmas tree-ish > > if (GRAPHICS_VER(i915) < 7) > return 0; > @@ -1810,6 +1805,18 @@ static int igt_vm_isolation(void *arg) > vm_total = ctx_a->vm->total; > GEM_BUG_ON(ctx_b->vm->total != vm_total); > > + obj_a = i915_gem_object_create_internal(i915, PAGE_SIZE); > + if (IS_ERR(obj_a)) { > + err = PTR_ERR(obj_a); > + goto out_file; > + } > + > + obj_b = i915_gem_object_create_internal(i915, PAGE_SIZE); > + if (IS_ERR(obj_b)) { > + err = PTR_ERR(obj_b); > + goto put_a; > + } > + > count = 0; > num_engines = 0; > for_each_uabi_engine(engine, i915) { > @@ -1832,10 +1839,10 @@ static int igt_vm_isolation(void *arg) > I915_GTT_PAGE_SIZE, vm_total, > sizeof(u32), alignof_dword); > > - err = write_to_scratch(ctx_a, engine, > + err = write_to_scratch(ctx_a, engine, obj_a, > offset, 0xdeadbeef); > if (err == 0) > - err = read_from_scratch(ctx_b, engine, > + err = read_from_scratch(ctx_b, engine, obj_b, > offset, &value); > if (err) > goto out_file; goto put_b; below also? Otherwise, Reviewed-by: Matthew Auld <matthew.auld@intel.com> > @@ -1858,6 +1865,9 @@ static int igt_vm_isolation(void *arg) > pr_info("Checked %lu scratch offsets across %lu engines\n", > count, num_engines); > > + i915_gem_object_put(obj_b); > +put_a: > + i915_gem_object_put(obj_a); > out_file: > if (igt_live_test_end(&t)) > err = -EIO; > -- > 2.34.0 >
On 07-12-2021 11:44, Matthew Auld wrote: > On Mon, 29 Nov 2021 at 13:57, Maarten Lankhorst > <maarten.lankhorst@linux.intel.com> wrote: >> In the next commit, we don't evict when refcount = 0. >> >> igt_vm_isolation() continuously tries to pin/unpin at same address, >> but also calls put() on the object, which means the object may not >> be unpinned in time. >> >> Instead of this, re-use the same object over and over, so they can >> be unbound as required. >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Is this something to be worried about in the real world, outside of > the selftests? I don't think userspace could hit it because the race is small, it would need to free an object, then immediately try to softpin a new object in the same place. It could be fixed, but it would require a massive rework of eviction. It could eventually be done, but requires fixing the entire vm locking. I don't think userspace will hit it, except if it tried deliberately. If it does turn out to be a problem, a workaround would be only calling i915_gem_evict_vm() without locks, so it can call drain_freed_objects as needed. This requires some surgery to make execbuf handle the case where we may drop all locks when evicting. >> --- >> .../drm/i915/gem/selftests/i915_gem_context.c | 54 +++++++++++-------- >> 1 file changed, 32 insertions(+), 22 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c >> index b32f7fed2d9c..3fc595b57cf4 100644 >> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c >> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c >> @@ -1481,10 +1481,10 @@ static int check_scratch(struct i915_address_space *vm, u64 offset) >> >> static int write_to_scratch(struct i915_gem_context *ctx, >> struct intel_engine_cs *engine, >> + struct drm_i915_gem_object *obj, >> u64 offset, u32 value) >> { >> struct drm_i915_private *i915 = ctx->i915; >> - struct drm_i915_gem_object *obj; >> struct i915_address_space *vm; >> struct i915_request *rq; >> struct i915_vma *vma; >> @@ -1497,15 +1497,9 @@ static int write_to_scratch(struct i915_gem_context *ctx, >> if (err) >> return err; >> >> - obj = i915_gem_object_create_internal(i915, PAGE_SIZE); >> - if (IS_ERR(obj)) >> - return PTR_ERR(obj); >> - >> cmd = i915_gem_object_pin_map_unlocked(obj, I915_MAP_WB); >> - if (IS_ERR(cmd)) { >> - err = PTR_ERR(cmd); >> - goto out; >> - } >> + if (IS_ERR(cmd)) >> + return PTR_ERR(cmd); >> >> *cmd++ = MI_STORE_DWORD_IMM_GEN4; >> if (GRAPHICS_VER(i915) >= 8) { >> @@ -1569,17 +1563,19 @@ static int write_to_scratch(struct i915_gem_context *ctx, >> i915_vma_unpin(vma); >> out_vm: >> i915_vm_put(vm); >> -out: >> - i915_gem_object_put(obj); >> + >> + if (!err) >> + err = i915_gem_object_wait(obj, 0, MAX_SCHEDULE_TIMEOUT); >> + >> return err; >> } >> >> static int read_from_scratch(struct i915_gem_context *ctx, >> struct intel_engine_cs *engine, >> + struct drm_i915_gem_object *obj, >> u64 offset, u32 *value) >> { >> struct drm_i915_private *i915 = ctx->i915; >> - struct drm_i915_gem_object *obj; >> struct i915_address_space *vm; >> const u32 result = 0x100; >> struct i915_request *rq; >> @@ -1594,10 +1590,6 @@ static int read_from_scratch(struct i915_gem_context *ctx, >> if (err) >> return err; >> >> - obj = i915_gem_object_create_internal(i915, PAGE_SIZE); >> - if (IS_ERR(obj)) >> - return PTR_ERR(obj); >> - >> if (GRAPHICS_VER(i915) >= 8) { >> const u32 GPR0 = engine->mmio_base + 0x600; >> >> @@ -1615,7 +1607,7 @@ static int read_from_scratch(struct i915_gem_context *ctx, >> cmd = i915_gem_object_pin_map_unlocked(obj, I915_MAP_WB); >> if (IS_ERR(cmd)) { >> err = PTR_ERR(cmd); >> - goto out; >> + goto err_unpin; >> } >> >> memset(cmd, POISON_INUSE, PAGE_SIZE); >> @@ -1651,7 +1643,7 @@ static int read_from_scratch(struct i915_gem_context *ctx, >> cmd = i915_gem_object_pin_map_unlocked(obj, I915_MAP_WB); >> if (IS_ERR(cmd)) { >> err = PTR_ERR(cmd); >> - goto out; >> + goto err_unpin; >> } >> >> memset(cmd, POISON_INUSE, PAGE_SIZE); >> @@ -1722,8 +1714,10 @@ static int read_from_scratch(struct i915_gem_context *ctx, >> i915_vma_unpin(vma); >> out_vm: >> i915_vm_put(vm); >> -out: >> - i915_gem_object_put(obj); >> + >> + if (!err) >> + err = i915_gem_object_wait(obj, 0, MAX_SCHEDULE_TIMEOUT); >> + >> return err; >> } >> >> @@ -1765,6 +1759,7 @@ static int igt_vm_isolation(void *arg) >> u64 vm_total; >> u32 expected; >> int err; >> + struct drm_i915_gem_object *obj_a, *obj_b; > Nit: Christmas tree-ish > >> if (GRAPHICS_VER(i915) < 7) >> return 0; >> @@ -1810,6 +1805,18 @@ static int igt_vm_isolation(void *arg) >> vm_total = ctx_a->vm->total; >> GEM_BUG_ON(ctx_b->vm->total != vm_total); >> >> + obj_a = i915_gem_object_create_internal(i915, PAGE_SIZE); >> + if (IS_ERR(obj_a)) { >> + err = PTR_ERR(obj_a); >> + goto out_file; >> + } >> + >> + obj_b = i915_gem_object_create_internal(i915, PAGE_SIZE); >> + if (IS_ERR(obj_b)) { >> + err = PTR_ERR(obj_b); >> + goto put_a; >> + } >> + >> count = 0; >> num_engines = 0; >> for_each_uabi_engine(engine, i915) { >> @@ -1832,10 +1839,10 @@ static int igt_vm_isolation(void *arg) >> I915_GTT_PAGE_SIZE, vm_total, >> sizeof(u32), alignof_dword); >> >> - err = write_to_scratch(ctx_a, engine, >> + err = write_to_scratch(ctx_a, engine, obj_a, >> offset, 0xdeadbeef); >> if (err == 0) >> - err = read_from_scratch(ctx_b, engine, >> + err = read_from_scratch(ctx_b, engine, obj_b, >> offset, &value); >> if (err) >> goto out_file; > goto put_b; below also? > > Otherwise, > Reviewed-by: Matthew Auld <matthew.auld@intel.com> Thanks, will fixup both! ~Maarten
diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c index b32f7fed2d9c..3fc595b57cf4 100644 --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c @@ -1481,10 +1481,10 @@ static int check_scratch(struct i915_address_space *vm, u64 offset) static int write_to_scratch(struct i915_gem_context *ctx, struct intel_engine_cs *engine, + struct drm_i915_gem_object *obj, u64 offset, u32 value) { struct drm_i915_private *i915 = ctx->i915; - struct drm_i915_gem_object *obj; struct i915_address_space *vm; struct i915_request *rq; struct i915_vma *vma; @@ -1497,15 +1497,9 @@ static int write_to_scratch(struct i915_gem_context *ctx, if (err) return err; - obj = i915_gem_object_create_internal(i915, PAGE_SIZE); - if (IS_ERR(obj)) - return PTR_ERR(obj); - cmd = i915_gem_object_pin_map_unlocked(obj, I915_MAP_WB); - if (IS_ERR(cmd)) { - err = PTR_ERR(cmd); - goto out; - } + if (IS_ERR(cmd)) + return PTR_ERR(cmd); *cmd++ = MI_STORE_DWORD_IMM_GEN4; if (GRAPHICS_VER(i915) >= 8) { @@ -1569,17 +1563,19 @@ static int write_to_scratch(struct i915_gem_context *ctx, i915_vma_unpin(vma); out_vm: i915_vm_put(vm); -out: - i915_gem_object_put(obj); + + if (!err) + err = i915_gem_object_wait(obj, 0, MAX_SCHEDULE_TIMEOUT); + return err; } static int read_from_scratch(struct i915_gem_context *ctx, struct intel_engine_cs *engine, + struct drm_i915_gem_object *obj, u64 offset, u32 *value) { struct drm_i915_private *i915 = ctx->i915; - struct drm_i915_gem_object *obj; struct i915_address_space *vm; const u32 result = 0x100; struct i915_request *rq; @@ -1594,10 +1590,6 @@ static int read_from_scratch(struct i915_gem_context *ctx, if (err) return err; - obj = i915_gem_object_create_internal(i915, PAGE_SIZE); - if (IS_ERR(obj)) - return PTR_ERR(obj); - if (GRAPHICS_VER(i915) >= 8) { const u32 GPR0 = engine->mmio_base + 0x600; @@ -1615,7 +1607,7 @@ static int read_from_scratch(struct i915_gem_context *ctx, cmd = i915_gem_object_pin_map_unlocked(obj, I915_MAP_WB); if (IS_ERR(cmd)) { err = PTR_ERR(cmd); - goto out; + goto err_unpin; } memset(cmd, POISON_INUSE, PAGE_SIZE); @@ -1651,7 +1643,7 @@ static int read_from_scratch(struct i915_gem_context *ctx, cmd = i915_gem_object_pin_map_unlocked(obj, I915_MAP_WB); if (IS_ERR(cmd)) { err = PTR_ERR(cmd); - goto out; + goto err_unpin; } memset(cmd, POISON_INUSE, PAGE_SIZE); @@ -1722,8 +1714,10 @@ static int read_from_scratch(struct i915_gem_context *ctx, i915_vma_unpin(vma); out_vm: i915_vm_put(vm); -out: - i915_gem_object_put(obj); + + if (!err) + err = i915_gem_object_wait(obj, 0, MAX_SCHEDULE_TIMEOUT); + return err; } @@ -1765,6 +1759,7 @@ static int igt_vm_isolation(void *arg) u64 vm_total; u32 expected; int err; + struct drm_i915_gem_object *obj_a, *obj_b; if (GRAPHICS_VER(i915) < 7) return 0; @@ -1810,6 +1805,18 @@ static int igt_vm_isolation(void *arg) vm_total = ctx_a->vm->total; GEM_BUG_ON(ctx_b->vm->total != vm_total); + obj_a = i915_gem_object_create_internal(i915, PAGE_SIZE); + if (IS_ERR(obj_a)) { + err = PTR_ERR(obj_a); + goto out_file; + } + + obj_b = i915_gem_object_create_internal(i915, PAGE_SIZE); + if (IS_ERR(obj_b)) { + err = PTR_ERR(obj_b); + goto put_a; + } + count = 0; num_engines = 0; for_each_uabi_engine(engine, i915) { @@ -1832,10 +1839,10 @@ static int igt_vm_isolation(void *arg) I915_GTT_PAGE_SIZE, vm_total, sizeof(u32), alignof_dword); - err = write_to_scratch(ctx_a, engine, + err = write_to_scratch(ctx_a, engine, obj_a, offset, 0xdeadbeef); if (err == 0) - err = read_from_scratch(ctx_b, engine, + err = read_from_scratch(ctx_b, engine, obj_b, offset, &value); if (err) goto out_file; @@ -1858,6 +1865,9 @@ static int igt_vm_isolation(void *arg) pr_info("Checked %lu scratch offsets across %lu engines\n", count, num_engines); + i915_gem_object_put(obj_b); +put_a: + i915_gem_object_put(obj_a); out_file: if (igt_live_test_end(&t)) err = -EIO;
In the next commit, we don't evict when refcount = 0. igt_vm_isolation() continuously tries to pin/unpin at same address, but also calls put() on the object, which means the object may not be unpinned in time. Instead of this, re-use the same object over and over, so they can be unbound as required. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- .../drm/i915/gem/selftests/i915_gem_context.c | 54 +++++++++++-------- 1 file changed, 32 insertions(+), 22 deletions(-)