Re: [PATCH v3] drm/i915/ttm: Rework object initialization slightly

[Matthew Auld <matthew.auld@xxxxxxxxx>]

On 30/09/2021 12:32, Thomas Hellström wrote:
> We may end up in i915_ttm_bo_destroy() in an error path before the
> object is fully initialized. In that case it's not correct to call
> __i915_gem_free_object(), because that function
> a) Assumes the gem object refcount is 0, which it isn't.
> b) frees the placements which are owned by the caller until the
> init_object() region ops returns successfully. Fix this by providing
> a lightweight cleanup function __i915_gem_object_fini() which is also
> called by __i915_gem_free_object().
>
> While doing this, also make sure we call dma_resv_fini() as part of
> ordinary object destruction and not from the RCU callback that frees
> the object. This will help track down bugs where the object is incorrectly
> locked from an RCU lookup.
>
> Finally, make sure the object isn't put on the region list until it's
> either locked or fully initialized in order to block list processing of
> partially initialized objects.
>
> v2:
> - The TTM object backend memory was freed before the gem pages were
>    put. Separate this functionality into __i915_gem_object_pages_fini()
>    and call it from the TTM delete_mem_notify() callback.
> v3:
> - Include i915_gem_object_free_mmaps() in __i915_gem_object_pages_fini()
>    to make sure we don't inadvertedly introduce a race.
>
> Signed-off-by: Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx>
> Reviewed-by: Matthew Auld <matthew.auld@xxxxxxxxx> #v1

R-b still stands.

> ---
>   drivers/gpu/drm/i915/gem/i915_gem_object.c | 43 +++++++++++++++++++---
>   drivers/gpu/drm/i915/gem/i915_gem_object.h |  5 +++
>   drivers/gpu/drm/i915/gem/i915_gem_ttm.c    | 36 +++++++++++-------
>   3 files changed, 64 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> index 6fb9afb65034..b88b121e244a 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> @@ -89,6 +89,22 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj,
>   	mutex_init(&obj->mm.get_dma_page.lock);
>   }
>   
> +/**
> + * i915_gem_object_fini - Clean up a GEM object initialization
> + * @obj: The gem object to cleanup
> + *
> + * This function cleans up gem object fields that are set up by
> + * drm_gem_private_object_init() and i915_gem_object_init().
> + * It's primarily intended as a helper for backends that need to
> + * clean up the gem object in separate steps.
> + */
> +void __i915_gem_object_fini(struct drm_i915_gem_object *obj)
> +{
> +	mutex_destroy(&obj->mm.get_page.lock);
> +	mutex_destroy(&obj->mm.get_dma_page.lock);
> +	dma_resv_fini(&obj->base._resv);
> +}
> +
>   /**
>    * Mark up the object's coherency levels for a given cache_level
>    * @obj: #drm_i915_gem_object
> @@ -174,7 +190,6 @@ void __i915_gem_free_object_rcu(struct rcu_head *head)
>   		container_of(head, typeof(*obj), rcu);
>   	struct drm_i915_private *i915 = to_i915(obj->base.dev);
>   
> -	dma_resv_fini(&obj->base._resv);
>   	i915_gem_object_free(obj);
>   
>   	GEM_BUG_ON(!atomic_read(&i915->mm.free_count));
> @@ -204,10 +219,17 @@ static void __i915_gem_object_free_mmaps(struct drm_i915_gem_object *obj)
>   	}
>   }
>   
> -void __i915_gem_free_object(struct drm_i915_gem_object *obj)
> +/**
> + * __i915_gem_object_pages_fini - Clean up pages use of a gem object
> + * @obj: The gem object to clean up
> + *
> + * This function cleans up usage of the object mm.pages member. It
> + * is intended for backends that need to clean up a gem object in
> + * separate steps and needs to be called when the object is idle before
> + * the object's backing memory is freed.
> + */
> +void __i915_gem_object_pages_fini(struct drm_i915_gem_object *obj)
>   {
> -	trace_i915_gem_object_destroy(obj);
> -
>   	if (!list_empty(&obj->vma.list)) {
>   		struct i915_vma *vma;
>   
> @@ -233,11 +255,17 @@ void __i915_gem_free_object(struct drm_i915_gem_object *obj)
>   
>   	__i915_gem_object_free_mmaps(obj);
>   
> -	GEM_BUG_ON(!list_empty(&obj->lut_list));
> -
>   	atomic_set(&obj->mm.pages_pin_count, 0);
>   	__i915_gem_object_put_pages(obj);
>   	GEM_BUG_ON(i915_gem_object_has_pages(obj));
> +}
> +
> +void __i915_gem_free_object(struct drm_i915_gem_object *obj)
> +{
> +	trace_i915_gem_object_destroy(obj);
> +
> +	GEM_BUG_ON(!list_empty(&obj->lut_list));
> +
>   	bitmap_free(obj->bit_17);
>   
>   	if (obj->base.import_attach)
> @@ -253,6 +281,8 @@ void __i915_gem_free_object(struct drm_i915_gem_object *obj)
>   
>   	if (obj->shares_resv_from)
>   		i915_vm_resv_put(obj->shares_resv_from);
> +
> +	__i915_gem_object_fini(obj);
>   }
>   
>   static void __i915_gem_free_objects(struct drm_i915_private *i915,
> @@ -266,6 +296,7 @@ static void __i915_gem_free_objects(struct drm_i915_private *i915,
>   			obj->ops->delayed_free(obj);
>   			continue;
>   		}
> +		__i915_gem_object_pages_fini(obj);
>   		__i915_gem_free_object(obj);
>   
>   		/* But keep the pointer alive for RCU-protected lookups */
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> index 3043fcbd31bd..7f9f2e5ba0ec 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> @@ -58,6 +58,9 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj,
>   			  const struct drm_i915_gem_object_ops *ops,
>   			  struct lock_class_key *key,
>   			  unsigned alloc_flags);
> +
> +void __i915_gem_object_fini(struct drm_i915_gem_object *obj);
> +
>   struct drm_i915_gem_object *
>   i915_gem_object_create_shmem(struct drm_i915_private *i915,
>   			     resource_size_t size);
> @@ -582,6 +585,8 @@ bool i915_gem_object_is_shmem(const struct drm_i915_gem_object *obj);
>   
>   void __i915_gem_free_object_rcu(struct rcu_head *head);
>   
> +void __i915_gem_object_pages_fini(struct drm_i915_gem_object *obj);
> +
>   void __i915_gem_free_object(struct drm_i915_gem_object *obj);
>   
>   bool i915_gem_object_evictable(struct drm_i915_gem_object *obj);
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> index b94497989995..a0ab5c44627b 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> @@ -367,8 +367,10 @@ static void i915_ttm_delete_mem_notify(struct ttm_buffer_object *bo)
>   {
>   	struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo);
>   
> -	if (likely(obj))
> +	if (likely(obj)) {
> +		__i915_gem_object_pages_fini(obj);
>   		i915_ttm_free_cached_io_st(obj);
> +	}
>   }
>   
>   static struct intel_memory_region *
> @@ -813,12 +815,9 @@ static void i915_ttm_adjust_lru(struct drm_i915_gem_object *obj)
>    */
>   static void i915_ttm_delayed_free(struct drm_i915_gem_object *obj)
>   {
> -	if (obj->ttm.created) {
> -		ttm_bo_put(i915_gem_to_ttm(obj));
> -	} else {
> -		__i915_gem_free_object(obj);
> -		call_rcu(&obj->rcu, __i915_gem_free_object_rcu);
> -	}
> +	GEM_BUG_ON(!obj->ttm.created);
> +
> +	ttm_bo_put(i915_gem_to_ttm(obj));
>   }
>   
>   static vm_fault_t vm_fault_ttm(struct vm_fault *vmf)
> @@ -898,16 +897,19 @@ void i915_ttm_bo_destroy(struct ttm_buffer_object *bo)
>   {
>   	struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo);
>   
> -	i915_ttm_backup_free(obj);
> -
> -	/* This releases all gem object bindings to the backend. */
> -	__i915_gem_free_object(obj);
> -
>   	i915_gem_object_release_memory_region(obj);
>   	mutex_destroy(&obj->ttm.get_io_page.lock);
>   
> -	if (obj->ttm.created)
> +	if (obj->ttm.created) {
> +		i915_ttm_backup_free(obj);
> +
> +		/* This releases all gem object bindings to the backend. */
> +		__i915_gem_free_object(obj);
> +
>   		call_rcu(&obj->rcu, __i915_gem_free_object_rcu);
> +	} else {
> +		__i915_gem_object_fini(obj);
> +	}
>   }
>   
>   /**
> @@ -936,7 +938,11 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
>   
>   	drm_gem_private_object_init(&i915->drm, &obj->base, size);
>   	i915_gem_object_init(obj, &i915_gem_ttm_obj_ops, &lock_class, flags);
> -	i915_gem_object_init_memory_region(obj, mem);
> +
> +	/* Don't put on a region list until we're either locked or fully initialized. */
> +	obj->mm.region = intel_memory_region_get(mem);
> +	INIT_LIST_HEAD(&obj->mm.region_link);
> +
>   	i915_gem_object_make_unshrinkable(obj);
>   	INIT_RADIX_TREE(&obj->ttm.get_io_page.radix, GFP_KERNEL | __GFP_NOWARN);
>   	mutex_init(&obj->ttm.get_io_page.lock);
> @@ -963,6 +969,8 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
>   		return i915_ttm_err_to_gem(ret);
>   
>   	obj->ttm.created = true;
> +	i915_gem_object_release_memory_region(obj);
> +	i915_gem_object_init_memory_region(obj, mem);
>   	i915_ttm_adjust_domains_after_move(obj);
>   	i915_ttm_adjust_gem_after_move(obj);
>   	i915_gem_object_unlock(obj);