Re: [Intel-gfx] [PATCH] drm/i915/gt: Use i915_vm_put on ppgtt_create error paths

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

 



On Mon, 26 Sept 2022 at 16:34, Matthew Auld <matthew.auld@xxxxxxxxx> wrote:
>
> From: Chris Wilson <chris.p.wilson@xxxxxxxxx>
>
> Now that the scratch page and page directories have a reference back to
> the i915_address_space, we cannot do an immediate free of the ppgtt upon
> error as those buffer objects will perform a later i915_vm_put in their
> deferred frees.
>
> The downside is that by replacing the onion unwind along the error
> paths, the ppgtt cleanup must handle a partially constructed vm. This
> includes ensuring that the vm->cleanup is set prior to the error path.
>
> Fixes: https://gitlab.freedesktop.org/drm/intel/-/issues/6900
> Signed-off-by: Chris Wilson <chris.p.wilson@xxxxxxxxx>
> Fixes: 4d8151ae5329 ("drm/i915: Don't free shared locks while shared")
> Cc: Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx>
> Cc: Matthew Auld <matthew.auld@xxxxxxxxx>
> Cc: <stable@xxxxxxxxxxxxxxx> # v5.14+

Tested locally on dg2 (with small-bar), and no longer goes down in
flames it seems,
Reviewed-by: Matthew Auld <matthew.auld@xxxxxxxxx>

> Signed-off-by: Matthew Auld <matthew.auld@xxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/gt/gen6_ppgtt.c | 16 ++++----
>  drivers/gpu/drm/i915/gt/gen8_ppgtt.c | 58 ++++++++++++++--------------
>  drivers/gpu/drm/i915/gt/intel_gtt.c  |  3 ++
>  3 files changed, 41 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> index 1bb766c79dcb..5aaacc53fa4c 100644
> --- a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> +++ b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> @@ -247,6 +247,7 @@ static int gen6_ppgtt_init_scratch(struct gen6_ppgtt *ppgtt)
>         i915_gem_object_put(vm->scratch[1]);
>  err_scratch0:
>         i915_gem_object_put(vm->scratch[0]);
> +       vm->scratch[0] = NULL;
>         return ret;
>  }
>
> @@ -268,9 +269,10 @@ static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
>         gen6_ppgtt_free_pd(ppgtt);
>         free_scratch(vm);
>
> -       mutex_destroy(&ppgtt->flush);
> +       if (ppgtt->base.pd)
> +               free_pd(&ppgtt->base.vm, ppgtt->base.pd);
>
> -       free_pd(&ppgtt->base.vm, ppgtt->base.pd);
> +       mutex_destroy(&ppgtt->flush);
>  }
>
>  static void pd_vma_bind(struct i915_address_space *vm,
> @@ -449,19 +451,17 @@ struct i915_ppgtt *gen6_ppgtt_create(struct intel_gt *gt)
>
>         err = gen6_ppgtt_init_scratch(ppgtt);
>         if (err)
> -               goto err_free;
> +               goto err_put;
>
>         ppgtt->base.pd = gen6_alloc_top_pd(ppgtt);
>         if (IS_ERR(ppgtt->base.pd)) {
>                 err = PTR_ERR(ppgtt->base.pd);
> -               goto err_scratch;
> +               goto err_put;
>         }
>
>         return &ppgtt->base;
>
> -err_scratch:
> -       free_scratch(&ppgtt->base.vm);
> -err_free:
> -       kfree(ppgtt);
> +err_put:
> +       i915_vm_put(&ppgtt->base.vm);
>         return ERR_PTR(err);
>  }
> diff --git a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> index c7bd5d71b03e..2128b7a72a25 100644
> --- a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> +++ b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> @@ -196,7 +196,10 @@ static void gen8_ppgtt_cleanup(struct i915_address_space *vm)
>         if (intel_vgpu_active(vm->i915))
>                 gen8_ppgtt_notify_vgt(ppgtt, false);
>
> -       __gen8_ppgtt_cleanup(vm, ppgtt->pd, gen8_pd_top_count(vm), vm->top);
> +       if (ppgtt->pd)
> +               __gen8_ppgtt_cleanup(vm, ppgtt->pd,
> +                                    gen8_pd_top_count(vm), vm->top);
> +
>         free_scratch(vm);
>  }
>
> @@ -803,8 +806,10 @@ static int gen8_init_scratch(struct i915_address_space *vm)
>                 struct drm_i915_gem_object *obj;
>
>                 obj = vm->alloc_pt_dma(vm, I915_GTT_PAGE_SIZE_4K);
> -               if (IS_ERR(obj))
> +               if (IS_ERR(obj)) {
> +                       ret = PTR_ERR(obj);
>                         goto free_scratch;
> +               }
>
>                 ret = map_pt_dma(vm, obj);
>                 if (ret) {
> @@ -823,7 +828,8 @@ static int gen8_init_scratch(struct i915_address_space *vm)
>  free_scratch:
>         while (i--)
>                 i915_gem_object_put(vm->scratch[i]);
> -       return -ENOMEM;
> +       vm->scratch[0] = NULL;
> +       return ret;
>  }
>
>  static int gen8_preallocate_top_level_pdp(struct i915_ppgtt *ppgtt)
> @@ -901,6 +907,7 @@ gen8_alloc_top_pd(struct i915_address_space *vm)
>  struct i915_ppgtt *gen8_ppgtt_create(struct intel_gt *gt,
>                                      unsigned long lmem_pt_obj_flags)
>  {
> +       struct i915_page_directory *pd;
>         struct i915_ppgtt *ppgtt;
>         int err;
>
> @@ -946,21 +953,7 @@ struct i915_ppgtt *gen8_ppgtt_create(struct intel_gt *gt,
>                 ppgtt->vm.alloc_scratch_dma = alloc_pt_dma;
>         }
>
> -       err = gen8_init_scratch(&ppgtt->vm);
> -       if (err)
> -               goto err_free;
> -
> -       ppgtt->pd = gen8_alloc_top_pd(&ppgtt->vm);
> -       if (IS_ERR(ppgtt->pd)) {
> -               err = PTR_ERR(ppgtt->pd);
> -               goto err_free_scratch;
> -       }
> -
> -       if (!i915_vm_is_4lvl(&ppgtt->vm)) {
> -               err = gen8_preallocate_top_level_pdp(ppgtt);
> -               if (err)
> -                       goto err_free_pd;
> -       }
> +       ppgtt->vm.pte_encode = gen8_pte_encode;
>
>         ppgtt->vm.bind_async_flags = I915_VMA_LOCAL_BIND;
>         ppgtt->vm.insert_entries = gen8_ppgtt_insert;
> @@ -971,22 +964,31 @@ struct i915_ppgtt *gen8_ppgtt_create(struct intel_gt *gt,
>         ppgtt->vm.allocate_va_range = gen8_ppgtt_alloc;
>         ppgtt->vm.clear_range = gen8_ppgtt_clear;
>         ppgtt->vm.foreach = gen8_ppgtt_foreach;
> +       ppgtt->vm.cleanup = gen8_ppgtt_cleanup;
>
> -       ppgtt->vm.pte_encode = gen8_pte_encode;
> +       err = gen8_init_scratch(&ppgtt->vm);
> +       if (err)
> +               goto err_put;
> +
> +       pd = gen8_alloc_top_pd(&ppgtt->vm);
> +       if (IS_ERR(pd)) {
> +               err = PTR_ERR(pd);
> +               goto err_put;
> +       }
> +       ppgtt->pd = pd;
> +
> +       if (!i915_vm_is_4lvl(&ppgtt->vm)) {
> +               err = gen8_preallocate_top_level_pdp(ppgtt);
> +               if (err)
> +                       goto err_put;
> +       }
>
>         if (intel_vgpu_active(gt->i915))
>                 gen8_ppgtt_notify_vgt(ppgtt, true);
>
> -       ppgtt->vm.cleanup = gen8_ppgtt_cleanup;
> -
>         return ppgtt;
>
> -err_free_pd:
> -       __gen8_ppgtt_cleanup(&ppgtt->vm, ppgtt->pd,
> -                            gen8_pd_top_count(&ppgtt->vm), ppgtt->vm.top);
> -err_free_scratch:
> -       free_scratch(&ppgtt->vm);
> -err_free:
> -       kfree(ppgtt);
> +err_put:
> +       i915_vm_put(&ppgtt->vm);
>         return ERR_PTR(err);
>  }
> diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.c b/drivers/gpu/drm/i915/gt/intel_gtt.c
> index b67831833c9a..2eaeba14319e 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gtt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gtt.c
> @@ -405,6 +405,9 @@ void free_scratch(struct i915_address_space *vm)
>  {
>         int i;
>
> +       if (!vm->scratch[0])
> +               return;
> +
>         for (i = 0; i <= vm->top; i++)
>                 i915_gem_object_put(vm->scratch[i]);
>  }
> --
> 2.37.3
>




[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux