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

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@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@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@linux.intel.com