[PATCH v2 01/15] drm/i915: Untangle the vma pages_mutex

[Thomas Hellström]

On 5/18/21 1:12 PM, Maarten Lankhorst wrote:
> Hey,
>
> This needs a small fix, otherwise looks good.
>
> Op 18-05-2021 om 10:26 schreef Thomas Hellström:
>> From: Thomas Hellström <thomas.hellstrom at intel.com>
>>
>> Any sleeping dma_resv lock taken while the vma pages_mutex is held
>> will cause a lockdep splat.
>> Move the i915_gem_object_pin_pages() call out of the pages_mutex
>> critical section.
>>
>> Signed-off-by: Thomas Hellström <thomas.hellstrom at intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_vma.c | 33 +++++++++++++++++++--------------
>>   1 file changed, 19 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
>> index a6cd0fa62847..7b1c0f4e60d7 100644
>> --- a/drivers/gpu/drm/i915/i915_vma.c
>> +++ b/drivers/gpu/drm/i915/i915_vma.c
>> @@ -800,32 +800,37 @@ static bool try_qad_pin(struct i915_vma *vma, unsigned int flags)
>>   static int vma_get_pages(struct i915_vma *vma)
>>   {
>>   	int err = 0;
>> +	bool pinned_pages = false;
>>   
>>   	if (atomic_add_unless(&vma->pages_count, 1, 0))
>>   		return 0;
>>   
>> +	if (vma->obj) {
>> +		err = i915_gem_object_pin_pages(vma->obj);
>> +		if (err)
>> +			return err;
>> +		pinned_pages = true;
>> +	}
>> +
>>   	/* Allocations ahoy! */
>> -	if (mutex_lock_interruptible(&vma->pages_mutex))
>> -		return -EINTR;
>> +	if (mutex_lock_interruptible(&vma->pages_mutex)) {
>> +		err = -EINTR;
>> +		goto unpin;
>> +	}
>>   
>>   	if (!atomic_read(&vma->pages_count)) {
>> -		if (vma->obj) {
>> -			err = i915_gem_object_pin_pages(vma->obj);
>> -			if (err)
>> -				goto unlock;
>> -		}
>> -
>>   		err = vma->ops->set_pages(vma);
>> -		if (err) {
>> -			if (vma->obj)
>> -				i915_gem_object_unpin_pages(vma->obj);
>> +		if (err)
>>   			goto unlock;
>> -		}
>> +		pinned_pages = false;
>>   	}
>>   	atomic_inc(&vma->pages_count);
>>   
>>   unlock:
>>   	mutex_unlock(&vma->pages_mutex);
>> +unpin:
>> +	if (pinned_pages)
>> +		__i915_gem_object_unpin_pages(vma->obj);
>>   
>>   	return err;
>>   }
>> @@ -838,10 +843,10 @@ static void __vma_put_pages(struct i915_vma *vma, unsigned int count)
>>   	if (atomic_sub_return(count, &vma->pages_count) == 0) {
>>   		vma->ops->clear_pages(vma);
>>   		GEM_BUG_ON(vma->pages);
>> -		if (vma->obj)
>> -			i915_gem_object_unpin_pages(vma->obj);
>>   	}
>>   	mutex_unlock(&vma->pages_mutex);
>> +	if (vma->obj)
>> +		i915_gem_object_unpin_pages(vma->obj);
> You're unconditionally unpinning pages here, if pages_count wasn't dropped to 0, we will go negative.
>
> With that fixed:
>
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>

Ah yes, thanks. That was a leftover from an earlier version...

/Thomas