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

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

 



On Mon, 29 Nov 2021 at 13:57, Maarten Lankhorst
<maarten.lankhorst@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>

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@xxxxxxxxx>

> @@ -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
>



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux