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

[Thomas Hellström (Intel)]

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>