[Intel-gfx] [PATCH] drm/i915: improve the catch-all evict to handle lock contention

[Mani Milani]

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 at intel.com> 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;
> >> +                       }