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

[Mani Milani]

This patch passed all my tests, some of which were failing before
applying the patch. Thanks.

Reviewed-by: Mani Milani <mani at chromium.org>
Tested-by: Mani Milani <mani at chromium.org>

On Thu, Dec 15, 2022 at 4:46 PM Mani Milani <mani at chromium.org> wrote:
>
> 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;
> > >> +                       }