This patch passed all my tests, some of which were failing before
applying the patch. Thanks.
Reviewed-by: Mani Milani <mani@xxxxxxxxxxxx>
Tested-by: Mani Milani <mani@xxxxxxxxxxxx>
On Thu, Dec 15, 2022 at 4:46 PM Mani Milani <mani@xxxxxxxxxxxx> 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@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;
> > >> + }
