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

[Maarten Lankhorst]

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>