[Intel-gfx] [PATCH] drm/i915/ttm: Rework object initialization slightly

[Thomas Hellström]

On 9/28/21 12:30, Matthew Auld wrote:
> 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 at 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 at intel.com>
>
Thanks for reviewing. Yes it was tested locally, but an additional 
change reviewed another flaw from the moving of the callsite 
__i915_gem_free_object() (we free the backing memory before unbinding, 
unmapping and calling put_pages() on object destruction, so it needs an 
additional fix).

/Thomas