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@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx>
