[Intel-gfx] [PATCH v2 02/15] drm/i915: Don't free shared locks while shared

[Thomas Hellström (Intel)]

On 5/18/21 1:18 PM, Maarten Lankhorst wrote:
> Op 18-05-2021 om 10:26 schreef Thomas Hellström:
>> We are currently sharing the VM reservation locks across a number of
>> gem objects with page-table memory. Since TTM will individiualize the
>> reservation locks when freeing objects, including accessing the shared
>> locks, make sure that the shared locks are not freed until that is done.
>> For PPGTT we add an additional refcount, for GGTT we take additional
>> measures to make sure objects sharing the GGTT reservation lock are
>> freed at GGTT takedown
>>
>> Signed-off-by: Thomas Hellström <thomas.hellstrom at linux.intel.com>
>> ---
>> v2: Try harder to make sure objects sharing the GGTT reservation lock are
>> freed at GGTT takedown.
>> ---
>>   drivers/gpu/drm/i915/gem/i915_gem_object.c    |  3 ++
>>   .../gpu/drm/i915/gem/i915_gem_object_types.h  |  1 +
>>   drivers/gpu/drm/i915/gt/intel_ggtt.c          | 19 ++++++--
>>   drivers/gpu/drm/i915/gt/intel_gtt.c           | 45 +++++++++++++++----
>>   drivers/gpu/drm/i915/gt/intel_gtt.h           | 30 ++++++++++++-
>>   drivers/gpu/drm/i915/gt/intel_ppgtt.c         |  2 +-
>>   drivers/gpu/drm/i915/i915_drv.c               |  5 +++
>>   7 files changed, 92 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
>> index 28144410df86..abadf0994ad0 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
>> @@ -252,6 +252,9 @@ static void __i915_gem_free_objects(struct drm_i915_private *i915,
>>   		if (obj->mm.n_placements > 1)
>>   			kfree(obj->mm.placements);
>>   
>> +		if (obj->resv_shared_from)
>> +			i915_vm_resv_put(obj->resv_shared_from);
>> +
>>   		/* But keep the pointer alive for RCU-protected lookups */
>>   		call_rcu(&obj->rcu, __i915_gem_free_object_rcu);
>>   		cond_resched();
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
>> index 0727d0c76aa0..450340a73186 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
>> @@ -149,6 +149,7 @@ struct drm_i915_gem_object {
>>   	 * when i915_gem_ww_ctx_backoff() or i915_gem_ww_ctx_fini() are called.
>>   	 */
>>   	struct list_head obj_link;
>> +	struct dma_resv *resv_shared_from;
> Since this can only be a vm object, would it make sense to make this a pointer to the vm address space, so we can call vm_resv_put on it directly?
>
> The current pointer type and name makes it look generic, but if you try to use it with anything but an address space, it will blow up.
>
> Otherwise looks good. I guess we cannot force all bo's to be deleted before the vm is freed. :-)
>
> So with that fixed
>
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>

Thanks, I'll take a look at that.

Thomas