Re: [Intel-gfx] [PATCH v2 06/16] drm/i915: Ensure gem_contexts selftests work with unbind changes.

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

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