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 >
