[Intel-gfx] [PATCH] drm/i915/gt: Use i915_vm_put on ppgtt_create error paths
Matthew Auld
matthew.william.auld at gmail.com
Mon Sep 26 15:38:57 UTC 2022
On Mon, 26 Sept 2022 at 16:34, Matthew Auld <matthew.auld at intel.com> wrote:
>
> From: Chris Wilson <chris.p.wilson at intel.com>
>
> 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 at intel.com>
> Fixes: 4d8151ae5329 ("drm/i915: Don't free shared locks while shared")
> Cc: Thomas Hellström <thomas.hellstrom at linux.intel.com>
> Cc: Matthew Auld <matthew.auld at intel.com>
> Cc: <stable at vger.kernel.org> # v5.14+
Tested locally on dg2 (with small-bar), and no longer goes down in
flames it seems,
Reviewed-by: Matthew Auld <matthew.auld at intel.com>
> Signed-off-by: Matthew Auld <matthew.auld at intel.com>
> ---
> 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
>
More information about the Intel-gfx
mailing list