| Message ID | 20210518082701.997251-2-thomas.hellstrom@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Move LMEM (VRAM) management over to TTM | expand |
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>
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@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> Ah yes, thanks. That was a leftover from an earlier version... /Thomas
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); } static void vma_put_pages(struct i915_vma *vma)