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

[Matthew Auld <matthew.auld@intel.com>]

On 27/09/2021 16:10, 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.
> 
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_object.c | 18 ++++++++++--
>   drivers/gpu/drm/i915/gem/i915_gem_object.h |  3 ++
>   drivers/gpu/drm/i915/gem/i915_gem_ttm.c    | 32 +++++++++++++---------
>   3 files changed, 38 insertions(+), 15 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..244e555f9bba 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> @@ -89,6 +89,20 @@ 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 cleanup
> + *
> + * This function cleans up gem object fields that are set up by
> + * drm_gem_private_object_init() and i915_gem_object_init().
> + */
> +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 +188,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));
> @@ -223,7 +236,6 @@ void __i915_gem_free_object(struct drm_i915_gem_object *obj)
>   						       obj_link))) {
>   			GEM_BUG_ON(vma->obj != obj);
>   			spin_unlock(&obj->vma.lock);
> -
>   			__i915_vma_put(vma);

Unrelated change?

Not seeing any DG1 machines in CI currently, so assuming this was tested 
locally,
Reviewed-by: Matthew Auld <matthew.auld@intel.com>