Thanks for the explanations Matthew. It all makes sense now. I will
now test this patch further and report back the results.
There is just one comment block that needs to be updated I think. See below:
On Wed, Dec 14, 2022 at 10:47 PM Matthew Auld <matthew.auld@xxxxxxxxx> wrote:
>
...
> >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> >> index 86956b902c97..e2ce1e4e9723 100644
> >> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> >> @@ -745,25 +745,44 @@ static int eb_reserve(struct i915_execbuffer *eb)
> >> *
> >> * Defragmenting is skipped if all objects are pinned at a fixed location.
> >> */
Could you please update the comment block above and add a little
explanation for the new pass=3 you added?
> >> - for (pass = 0; pass <= 2; pass++) {
> >> + for (pass = 0; pass <= 3; pass++) {
> >> int pin_flags = PIN_USER | PIN_VALIDATE;
> >>
> >> if (pass == 0)
> >> pin_flags |= PIN_NONBLOCK;
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
> >> index 4cfe36b0366b..c02ebd6900ae 100644
> >> --- a/drivers/gpu/drm/i915/i915_gem_evict.c
> >> +++ b/drivers/gpu/drm/i915/i915_gem_evict.c
> >> @@ -441,6 +441,11 @@ int i915_gem_evict_for_node(struct i915_address_space *vm,
> >> * @vm: Address space to cleanse
> >> * @ww: An optional struct i915_gem_ww_ctx. If not NULL, i915_gem_evict_vm
> >> * will be able to evict vma's locked by the ww as well.
> >> + * @busy_bo: Optional pointer to struct drm_i915_gem_object. If not NULL, then
> >> + * in the event i915_gem_evict_vm() is unable to trylock an object for eviction,
> >> + * then @busy_bo will point to it. -EBUSY is also returned. The caller must drop
> >> + * the vm->mutex, before trying again to acquire the contended lock. The caller
> >> + * also owns a reference to the object.
> >> *
> >> * This function evicts all vmas from a vm.
> >> *
> >> @@ -450,7 +455,8 @@ int i915_gem_evict_for_node(struct i915_address_space *vm,
> >> * To clarify: This is for freeing up virtual address space, not for freeing
> >> * memory in e.g. the shrinker.
> >> */
> >> -int i915_gem_evict_vm(struct i915_address_space *vm, struct i915_gem_ww_ctx *ww)
> >> +int i915_gem_evict_vm(struct i915_address_space *vm, struct i915_gem_ww_ctx *ww,
> >> + struct drm_i915_gem_object **busy_bo)
> >> {
> >> int ret = 0;
> >>
> >> @@ -482,15 +488,22 @@ int i915_gem_evict_vm(struct i915_address_space *vm, struct i915_gem_ww_ctx *ww)
> >> * the resv is shared among multiple objects, we still
> >> * need the object ref.
> >> */
> >> - if (dying_vma(vma) ||
> >> + if (!i915_gem_object_get_rcu(vma->obj) ||
Oops, sorry, I had missed the one line change above. After you pointed
that out, all the 'i915_gem_object_put()' calls now make perfect
sense. Thanks.
> >> (ww && (dma_resv_locking_ctx(vma->obj->base.resv) == &ww->ctx))) {
> >> __i915_vma_pin(vma);
> >> list_add(&vma->evict_link, &locked_eviction_list);
> >> continue;
> >> }
> >>
> >> - if (!i915_gem_object_trylock(vma->obj, ww))
> >> + if (!i915_gem_object_trylock(vma->obj, ww)) {
> >> + if (busy_bo) {
> >> + *busy_bo = vma->obj; /* holds ref */
> >> + ret = -EBUSY;
> >> + break;
> >> + }
> >> + i915_gem_object_put(vma->obj);
> >> continue;
> >> + }
