[Intel-gfx] [PATCH v2 10/16] drm/i915: Make i915_gem_evict_vm work correctly for already locked objects

[Maarten Lankhorst]

On 08-12-2021 13:07, Matthew Auld wrote:
> On Mon, 29 Nov 2021 at 13:58, Maarten Lankhorst
> <maarten.lankhorst at linux.intel.com> wrote:
>> i915_gem_execbuf will call i915_gem_evict_vm() after failing to pin
>> all objects in the first round. We are about to remove those short-term
>> pins, but even without those the objects are still locked. Add a special
>> case to allow i915_gem_evict_vm to evict locked objects as well.
>>
>> This might also allow multiple objects sharing the same resv to be evicted.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> Reviewed-by: Matthew Auld <matthew.auld at intel.com>
>
> Do we need similar treatment for stuff like evict_for_node etc?

Unfortunately not, we would risk evicting newly bound objects when we completely drop short term pinning

evict_vm is the exception, because you expect it to clean up the entire vm as much as possible, and is called explictly, not as a part of pinning. :)

>> ---
>>  drivers/gpu/drm/i915/i915_gem_evict.c | 23 ++++++++++++++++++++++-
>>  1 file changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
>> index 24f5e3345e43..f502a617b35c 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_evict.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_evict.c
>> @@ -410,21 +410,42 @@ int i915_gem_evict_vm(struct i915_address_space *vm, struct i915_gem_ww_ctx *ww)
>>         do {
>>                 struct i915_vma *vma, *vn;
>>                 LIST_HEAD(eviction_list);
>> +               LIST_HEAD(locked_eviction_list);
>>
>>                 list_for_each_entry(vma, &vm->bound_list, vm_link) {
>>                         if (i915_vma_is_pinned(vma))
>>                                 continue;
>>
>> +                       /*
>> +                        * If we already own the lock, trylock fails. In case the resv
>> +                        * is shared among multiple objects, we still need the object ref.
>> +                        */
>> +                       if (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))
>>                                 continue;
>>
>>                         __i915_vma_pin(vma);
>>                         list_add(&vma->evict_link, &eviction_list);
>>                 }
>> -               if (list_empty(&eviction_list))
>> +               if (list_empty(&eviction_list) && list_empty(&locked_eviction_list))
>>                         break;
>>
>>                 ret = 0;
>> +               /* Unbind locked objects first, before unlocking the eviction_list */
>> +               list_for_each_entry_safe(vma, vn, &locked_eviction_list, evict_link) {
>> +                       __i915_vma_unpin(vma);
>> +
>> +                       if (ret == 0)
>> +                               ret = __i915_vma_unbind(vma);
>> +                       if (ret != -EINTR) /* "Get me out of here!" */
>> +                               ret = 0;
>> +               }
>> +
>>                 list_for_each_entry_safe(vma, vn, &eviction_list, evict_link) {
>>                         __i915_vma_unpin(vma);
>>                         if (ret == 0)
>> --
>> 2.34.0
>>