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@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)@@ -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;}