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

On Mon, 26 Sept 2022 at 16:34, Matthew Auld matthew.auld@intel.com wrote:

From: Chris Wilson chris.p.wilson@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@intel.com Fixes: 4d8151ae5329 ("drm/i915: Don't free shared locks while shared") Cc: Thomas Hellström thomas.hellstrom@linux.intel.com Cc: Matthew Auld matthew.auld@intel.com Cc: stable@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@intel.com

Signed-off-by: Matthew Auld matthew.auld@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