[Intel-gfx] [PATCH] drm/i915: Use trylock instead of blocking lock for __i915_gem_free_objects.

[Maarten Lankhorst]

On 2021-12-22 20:43, Thomas Hellström (Intel) wrote:
>
> On 12/22/21 16:56, Maarten Lankhorst wrote:
>> Convert free_work into delayed_work, similar to ttm to allow converting the
>> blocking lock in __i915_gem_free_objects to a trylock.
>>
>> Unlike ttm, the object should already be idle, as it's kept alive
>> by a reference through struct i915_vma->active, which is dropped
>> after all vma's are idle.
>>
>> Because of this, we can use a no wait by default, or when the lock
>> is contested, we use ttm's 10 ms.
>>
>> The trylock should only fail when the object is sharing it's resv with
>> other objects, and typically objects are not kept locked for a long
>> time, so we can safely retry on failure.
>>
>> Fixes: be7612fd6665 ("drm/i915: Require object lock when freeing pages during destruction")
>> Testcase: igt/gem_exec_alignment/pi*
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/gem/i915_gem_object.c | 14 ++++++++++----
>>   drivers/gpu/drm/i915/i915_drv.h            |  4 ++--
>>   2 files changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
>> index 39cd563544a5..d87b508b59b1 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
>> @@ -331,7 +331,13 @@ static void __i915_gem_free_objects(struct drm_i915_private *i915,
>>               continue;
>>           }
>>   -        i915_gem_object_lock(obj, NULL);
>> +        if (!i915_gem_object_trylock(obj, NULL)) {
>> +            /* busy, toss it back to the pile */
>> +            if (llist_add(&obj->freed, &i915->mm.free_list))
>> +                queue_delayed_work(i915->wq, &i915->mm.free_work, msecs_to_jiffies(10));
>
> i915->wq is ordered. From what I can tell, with queue_delayed_work(), the work doesn't get inserted into the queue order until the delay expires, right? So we don't unnecessarily hold up other objects getting freed?
>
>> +            continue;
>> +        }
>> +
>>           __i915_gem_object_pages_fini(obj);
>>           i915_gem_object_unlock(obj);
>>           __i915_gem_free_object(obj);
>> @@ -353,7 +359,7 @@ void i915_gem_flush_free_objects(struct drm_i915_private *i915)
>>   static void __i915_gem_free_work(struct work_struct *work)
>>   {
>>       struct drm_i915_private *i915 =
>> -        container_of(work, struct drm_i915_private, mm.free_work);
>> +        container_of(work, struct drm_i915_private, mm.free_work.work);
>>         i915_gem_flush_free_objects(i915);
>>   }
>> @@ -385,7 +391,7 @@ static void i915_gem_free_object(struct drm_gem_object *gem_obj)
>>        */
>>         if (llist_add(&obj->freed, &i915->mm.free_list))
>> -        queue_work(i915->wq, &i915->mm.free_work);
>> +        queue_delayed_work(i915->wq, &i915->mm.free_work, 0);
>>   }
>>     void __i915_gem_object_flush_frontbuffer(struct drm_i915_gem_object *obj,
>> @@ -710,7 +716,7 @@ bool i915_gem_object_placement_possible(struct drm_i915_gem_object *obj,
>>     void i915_gem_init__objects(struct drm_i915_private *i915)
>>   {
>> -    INIT_WORK(&i915->mm.free_work, __i915_gem_free_work);
>> +    INIT_DELAYED_WORK(&i915->mm.free_work, __i915_gem_free_work);
>>   }
>>     void i915_objects_module_exit(void)
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index c8fddb7e61c9..beeb42a14aae 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -465,7 +465,7 @@ struct i915_gem_mm {
>>        * List of objects which are pending destruction.
>>        */
>>       struct llist_head free_list;
>> -    struct work_struct free_work;
>> +    struct delayed_work free_work;
>>       /**
>>        * Count of objects pending destructions. Used to skip needlessly
>>        * waiting on an RCU barrier if no objects are waiting to be freed.
>> @@ -1625,7 +1625,7 @@ static inline void i915_gem_drain_freed_objects(struct drm_i915_private *i915)
>>        * armed the work again.
>>        */
>>       while (atomic_read(&i915->mm.free_count)) {
>> -        flush_work(&i915->mm.free_work);
>> +        flush_delayed_work(&i915->mm.free_work);
>>           flush_delayed_work(&i915->bdev.wq);
>>           rcu_barrier();
>>       }
>
> Otherwise LGTM.
>
> Reviewed-by: Thomas Hellström <thomas.hellstrom at linux.intel.com>
>
>
>
>
Thanks, pushed!