| Message ID | 20170703101412.30820-2-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/07/2017 11:14, Chris Wilson wrote: > Following the pattern now used for obj->mm.pages, use just pin_fence and > unpin_fence to control access to the fence registers. I.e. instead of > calling get_fence(); pin_fence(), we now just need to call pin_fence(). > This will make it easier to reduce the locking requirements around > fence registers. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_drv.h | 4 ---- > drivers/gpu/drm/i915/i915_gem.c | 3 ++- > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 10 +++++----- > drivers/gpu/drm/i915/i915_gem_fence_reg.c | 27 ++++++++++++++++++++++----- > drivers/gpu/drm/i915/i915_vma.c | 3 +-- > drivers/gpu/drm/i915/i915_vma.h | 20 ++++++++------------ > drivers/gpu/drm/i915/intel_display.c | 3 +-- > 7 files changed, 39 insertions(+), 31 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 0029bb949e90..b08b56a2e680 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -3545,10 +3545,6 @@ i915_vm_to_ppgtt(struct i915_address_space *vm) > return container_of(vm, struct i915_hw_ppgtt, base); > } > > -/* i915_gem_fence_reg.c */ > -int __must_check i915_vma_get_fence(struct i915_vma *vma); > -int __must_check i915_vma_put_fence(struct i915_vma *vma); > - > void i915_gem_revoke_fences(struct drm_i915_private *dev_priv); > void i915_gem_restore_fences(struct drm_i915_private *dev_priv); > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 1b2dfa8bdeef..939a299260e9 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -1941,7 +1941,7 @@ int i915_gem_fault(struct vm_fault *vmf) > if (ret) > goto err_unpin; > > - ret = i915_vma_get_fence(vma); > + ret = i915_vma_pin_fence(vma); Hm, I think fence stealing stops working with this change? Regards, Tvrtko > if (ret) > goto err_unpin; > > @@ -1957,6 +1957,7 @@ int i915_gem_fault(struct vm_fault *vmf) > min_t(u64, vma->size, area->vm_end - area->vm_start), > &ggtt->mappable); > > + i915_vma_unpin_fence(vma); > err_unpin: > __i915_vma_unpin(vma); > err_unlock: > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > index 929f275e67aa..22a9f5358322 100644 > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > @@ -365,12 +365,12 @@ eb_pin_vma(struct i915_execbuffer *eb, > return; > > if (unlikely(entry->flags & EXEC_OBJECT_NEEDS_FENCE)) { > - if (unlikely(i915_vma_get_fence(vma))) { > + if (unlikely(i915_vma_pin_fence(vma))) { > i915_vma_unpin(vma); > return; > } > > - if (i915_vma_pin_fence(vma)) > + if (vma->fence) > entry->flags |= __EXEC_OBJECT_HAS_FENCE; > } > > @@ -384,7 +384,7 @@ __eb_unreserve_vma(struct i915_vma *vma, > GEM_BUG_ON(!(entry->flags & __EXEC_OBJECT_HAS_PIN)); > > if (unlikely(entry->flags & __EXEC_OBJECT_HAS_FENCE)) > - i915_vma_unpin_fence(vma); > + __i915_vma_unpin_fence(vma); > > __i915_vma_unpin(vma); > } > @@ -564,13 +564,13 @@ static int eb_reserve_vma(const struct i915_execbuffer *eb, > GEM_BUG_ON(eb_vma_misplaced(entry, vma)); > > if (unlikely(entry->flags & EXEC_OBJECT_NEEDS_FENCE)) { > - err = i915_vma_get_fence(vma); > + err = i915_vma_pin_fence(vma); > if (unlikely(err)) { > i915_vma_unpin(vma); > return err; > } > > - if (i915_vma_pin_fence(vma)) > + if (vma->fence) > entry->flags |= __EXEC_OBJECT_HAS_FENCE; > } > > diff --git a/drivers/gpu/drm/i915/i915_gem_fence_reg.c b/drivers/gpu/drm/i915/i915_gem_fence_reg.c > index 5fe2cd8c8f28..55ac7bc14fce 100644 > --- a/drivers/gpu/drm/i915/i915_gem_fence_reg.c > +++ b/drivers/gpu/drm/i915/i915_gem_fence_reg.c > @@ -280,8 +280,7 @@ static int fence_update(struct drm_i915_fence_reg *fence, > * > * 0 on success, negative error code on failure. > */ > -int > -i915_vma_put_fence(struct i915_vma *vma) > +int i915_vma_put_fence(struct i915_vma *vma) > { > struct drm_i915_fence_reg *fence = vma->fence; > > @@ -299,6 +298,8 @@ static struct drm_i915_fence_reg *fence_find(struct drm_i915_private *dev_priv) > struct drm_i915_fence_reg *fence; > > list_for_each_entry(fence, &dev_priv->mm.fence_list, link) { > + GEM_BUG_ON(fence->vma && fence->vma->fence != fence); > + > if (fence->pin_count) > continue; > > @@ -313,7 +314,7 @@ static struct drm_i915_fence_reg *fence_find(struct drm_i915_private *dev_priv) > } > > /** > - * i915_vma_get_fence - set up fencing for a vma > + * i915_vma_pin_fence - set up fencing for a vma > * @vma: vma to map through a fence reg > * > * When mapping objects through the GTT, userspace wants to be able to write > @@ -331,10 +332,11 @@ static struct drm_i915_fence_reg *fence_find(struct drm_i915_private *dev_priv) > * 0 on success, negative error code on failure. > */ > int > -i915_vma_get_fence(struct i915_vma *vma) > +i915_vma_pin_fence(struct i915_vma *vma) > { > struct drm_i915_fence_reg *fence; > struct i915_vma *set = i915_gem_object_is_tiled(vma->obj) ? vma : NULL; > + int err; > > /* Note that we revoke fences on runtime suspend. Therefore the user > * must keep the device awake whilst using the fence. > @@ -344,6 +346,8 @@ i915_vma_get_fence(struct i915_vma *vma) > /* Just update our place in the LRU if our fence is getting reused. */ > if (vma->fence) { > fence = vma->fence; > + GEM_BUG_ON(fence->vma != vma); > + fence->pin_count++; > if (!fence->dirty) { > list_move_tail(&fence->link, > &fence->i915->mm.fence_list); > @@ -353,10 +357,19 @@ i915_vma_get_fence(struct i915_vma *vma) > fence = fence_find(vma->vm->i915); > if (IS_ERR(fence)) > return PTR_ERR(fence); > + fence->pin_count++; > } else > return 0; > > - return fence_update(fence, set); > + err = fence_update(fence, set); > + if (err) > + goto err_unpin; > + > + return 0; > + > +err_unpin: > + fence->pin_count--; > + return err; > } > > /** > @@ -378,6 +391,8 @@ void i915_gem_revoke_fences(struct drm_i915_private *dev_priv) > for (i = 0; i < dev_priv->num_fence_regs; i++) { > struct drm_i915_fence_reg *fence = &dev_priv->fence_regs[i]; > > + GEM_BUG_ON(fence->vma && fence->vma->fence != fence); > + > if (fence->vma) > i915_gem_release_mmap(fence->vma->obj); > } > @@ -399,6 +414,8 @@ void i915_gem_restore_fences(struct drm_i915_private *dev_priv) > struct drm_i915_fence_reg *reg = &dev_priv->fence_regs[i]; > struct i915_vma *vma = reg->vma; > > + GEM_BUG_ON(vma && vma->fence != reg); > + > /* > * Commit delayed tiling changes if we have an object still > * attached to the fence, otherwise just clear the fence. > diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c > index d7330d3eeab6..efbfee8eac99 100644 > --- a/drivers/gpu/drm/i915/i915_vma.c > +++ b/drivers/gpu/drm/i915/i915_vma.c > @@ -303,12 +303,11 @@ void __iomem *i915_vma_pin_iomap(struct i915_vma *vma) > > __i915_vma_pin(vma); > > - ret = i915_vma_get_fence(vma); > + ret = i915_vma_pin_fence(vma); > if (ret) { > __i915_vma_unpin(vma); > return IO_ERR_PTR(ret); > } > - i915_vma_pin_fence(vma); > > return ptr; > } > diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h > index 15e86e50a825..19f58af4f1bf 100644 > --- a/drivers/gpu/drm/i915/i915_vma.h > +++ b/drivers/gpu/drm/i915/i915_vma.h > @@ -342,15 +342,13 @@ static inline struct page *i915_vma_first_page(struct i915_vma *vma) > * > * True if the vma has a fence, false otherwise. > */ > -static inline bool > -i915_vma_pin_fence(struct i915_vma *vma) > +int i915_vma_pin_fence(struct i915_vma *vma); > +int __must_check i915_vma_put_fence(struct i915_vma *vma); > + > +static inline void __i915_vma_unpin_fence(struct i915_vma *vma) > { > - lockdep_assert_held(&vma->obj->base.dev->struct_mutex); > - if (vma->fence) { > - vma->fence->pin_count++; > - return true; > - } else > - return false; > + GEM_BUG_ON(vma->fence->pin_count <= 0); > + vma->fence->pin_count--; > } > > /** > @@ -365,10 +363,8 @@ static inline void > i915_vma_unpin_fence(struct i915_vma *vma) > { > lockdep_assert_held(&vma->obj->base.dev->struct_mutex); > - if (vma->fence) { > - GEM_BUG_ON(vma->fence->pin_count <= 0); > - vma->fence->pin_count--; > - } > + if (vma->fence) > + __i915_vma_unpin_fence(vma); > } > > #endif > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 4e03ca6c946f..432bbaf460b4 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -2183,8 +2183,7 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, unsigned int rotation) > * something and try to run the system in a "less than optimal" > * mode that matches the user configuration. > */ > - if (i915_vma_get_fence(vma) == 0) > - i915_vma_pin_fence(vma); > + i915_vma_pin_fence(vma); > } > > i915_vma_get(vma); >
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 0029bb949e90..b08b56a2e680 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -3545,10 +3545,6 @@ i915_vm_to_ppgtt(struct i915_address_space *vm) return container_of(vm, struct i915_hw_ppgtt, base); } -/* i915_gem_fence_reg.c */ -int __must_check i915_vma_get_fence(struct i915_vma *vma); -int __must_check i915_vma_put_fence(struct i915_vma *vma); - void i915_gem_revoke_fences(struct drm_i915_private *dev_priv); void i915_gem_restore_fences(struct drm_i915_private *dev_priv); diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 1b2dfa8bdeef..939a299260e9 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1941,7 +1941,7 @@ int i915_gem_fault(struct vm_fault *vmf) if (ret) goto err_unpin; - ret = i915_vma_get_fence(vma); + ret = i915_vma_pin_fence(vma); if (ret) goto err_unpin; @@ -1957,6 +1957,7 @@ int i915_gem_fault(struct vm_fault *vmf) min_t(u64, vma->size, area->vm_end - area->vm_start), &ggtt->mappable); + i915_vma_unpin_fence(vma); err_unpin: __i915_vma_unpin(vma); err_unlock: diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index 929f275e67aa..22a9f5358322 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -365,12 +365,12 @@ eb_pin_vma(struct i915_execbuffer *eb, return; if (unlikely(entry->flags & EXEC_OBJECT_NEEDS_FENCE)) { - if (unlikely(i915_vma_get_fence(vma))) { + if (unlikely(i915_vma_pin_fence(vma))) { i915_vma_unpin(vma); return; } - if (i915_vma_pin_fence(vma)) + if (vma->fence) entry->flags |= __EXEC_OBJECT_HAS_FENCE; } @@ -384,7 +384,7 @@ __eb_unreserve_vma(struct i915_vma *vma, GEM_BUG_ON(!(entry->flags & __EXEC_OBJECT_HAS_PIN)); if (unlikely(entry->flags & __EXEC_OBJECT_HAS_FENCE)) - i915_vma_unpin_fence(vma); + __i915_vma_unpin_fence(vma); __i915_vma_unpin(vma); } @@ -564,13 +564,13 @@ static int eb_reserve_vma(const struct i915_execbuffer *eb, GEM_BUG_ON(eb_vma_misplaced(entry, vma)); if (unlikely(entry->flags & EXEC_OBJECT_NEEDS_FENCE)) { - err = i915_vma_get_fence(vma); + err = i915_vma_pin_fence(vma); if (unlikely(err)) { i915_vma_unpin(vma); return err; } - if (i915_vma_pin_fence(vma)) + if (vma->fence) entry->flags |= __EXEC_OBJECT_HAS_FENCE; } diff --git a/drivers/gpu/drm/i915/i915_gem_fence_reg.c b/drivers/gpu/drm/i915/i915_gem_fence_reg.c index 5fe2cd8c8f28..55ac7bc14fce 100644 --- a/drivers/gpu/drm/i915/i915_gem_fence_reg.c +++ b/drivers/gpu/drm/i915/i915_gem_fence_reg.c @@ -280,8 +280,7 @@ static int fence_update(struct drm_i915_fence_reg *fence, * * 0 on success, negative error code on failure. */ -int -i915_vma_put_fence(struct i915_vma *vma) +int i915_vma_put_fence(struct i915_vma *vma) { struct drm_i915_fence_reg *fence = vma->fence; @@ -299,6 +298,8 @@ static struct drm_i915_fence_reg *fence_find(struct drm_i915_private *dev_priv) struct drm_i915_fence_reg *fence; list_for_each_entry(fence, &dev_priv->mm.fence_list, link) { + GEM_BUG_ON(fence->vma && fence->vma->fence != fence); + if (fence->pin_count) continue; @@ -313,7 +314,7 @@ static struct drm_i915_fence_reg *fence_find(struct drm_i915_private *dev_priv) } /** - * i915_vma_get_fence - set up fencing for a vma + * i915_vma_pin_fence - set up fencing for a vma * @vma: vma to map through a fence reg * * When mapping objects through the GTT, userspace wants to be able to write @@ -331,10 +332,11 @@ static struct drm_i915_fence_reg *fence_find(struct drm_i915_private *dev_priv) * 0 on success, negative error code on failure. */ int -i915_vma_get_fence(struct i915_vma *vma) +i915_vma_pin_fence(struct i915_vma *vma) { struct drm_i915_fence_reg *fence; struct i915_vma *set = i915_gem_object_is_tiled(vma->obj) ? vma : NULL; + int err; /* Note that we revoke fences on runtime suspend. Therefore the user * must keep the device awake whilst using the fence. @@ -344,6 +346,8 @@ i915_vma_get_fence(struct i915_vma *vma) /* Just update our place in the LRU if our fence is getting reused. */ if (vma->fence) { fence = vma->fence; + GEM_BUG_ON(fence->vma != vma); + fence->pin_count++; if (!fence->dirty) { list_move_tail(&fence->link, &fence->i915->mm.fence_list); @@ -353,10 +357,19 @@ i915_vma_get_fence(struct i915_vma *vma) fence = fence_find(vma->vm->i915); if (IS_ERR(fence)) return PTR_ERR(fence); + fence->pin_count++; } else return 0; - return fence_update(fence, set); + err = fence_update(fence, set); + if (err) + goto err_unpin; + + return 0; + +err_unpin: + fence->pin_count--; + return err; } /** @@ -378,6 +391,8 @@ void i915_gem_revoke_fences(struct drm_i915_private *dev_priv) for (i = 0; i < dev_priv->num_fence_regs; i++) { struct drm_i915_fence_reg *fence = &dev_priv->fence_regs[i]; + GEM_BUG_ON(fence->vma && fence->vma->fence != fence); + if (fence->vma) i915_gem_release_mmap(fence->vma->obj); } @@ -399,6 +414,8 @@ void i915_gem_restore_fences(struct drm_i915_private *dev_priv) struct drm_i915_fence_reg *reg = &dev_priv->fence_regs[i]; struct i915_vma *vma = reg->vma; + GEM_BUG_ON(vma && vma->fence != reg); + /* * Commit delayed tiling changes if we have an object still * attached to the fence, otherwise just clear the fence. diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c index d7330d3eeab6..efbfee8eac99 100644 --- a/drivers/gpu/drm/i915/i915_vma.c +++ b/drivers/gpu/drm/i915/i915_vma.c @@ -303,12 +303,11 @@ void __iomem *i915_vma_pin_iomap(struct i915_vma *vma) __i915_vma_pin(vma); - ret = i915_vma_get_fence(vma); + ret = i915_vma_pin_fence(vma); if (ret) { __i915_vma_unpin(vma); return IO_ERR_PTR(ret); } - i915_vma_pin_fence(vma); return ptr; } diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h index 15e86e50a825..19f58af4f1bf 100644 --- a/drivers/gpu/drm/i915/i915_vma.h +++ b/drivers/gpu/drm/i915/i915_vma.h @@ -342,15 +342,13 @@ static inline struct page *i915_vma_first_page(struct i915_vma *vma) * * True if the vma has a fence, false otherwise. */ -static inline bool -i915_vma_pin_fence(struct i915_vma *vma) +int i915_vma_pin_fence(struct i915_vma *vma); +int __must_check i915_vma_put_fence(struct i915_vma *vma); + +static inline void __i915_vma_unpin_fence(struct i915_vma *vma) { - lockdep_assert_held(&vma->obj->base.dev->struct_mutex); - if (vma->fence) { - vma->fence->pin_count++; - return true; - } else - return false; + GEM_BUG_ON(vma->fence->pin_count <= 0); + vma->fence->pin_count--; } /** @@ -365,10 +363,8 @@ static inline void i915_vma_unpin_fence(struct i915_vma *vma) { lockdep_assert_held(&vma->obj->base.dev->struct_mutex); - if (vma->fence) { - GEM_BUG_ON(vma->fence->pin_count <= 0); - vma->fence->pin_count--; - } + if (vma->fence) + __i915_vma_unpin_fence(vma); } #endif diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 4e03ca6c946f..432bbaf460b4 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2183,8 +2183,7 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, unsigned int rotation) * something and try to run the system in a "less than optimal" * mode that matches the user configuration. */ - if (i915_vma_get_fence(vma) == 0) - i915_vma_pin_fence(vma); + i915_vma_pin_fence(vma); } i915_vma_get(vma);
Following the pattern now used for obj->mm.pages, use just pin_fence and unpin_fence to control access to the fence registers. I.e. instead of calling get_fence(); pin_fence(), we now just need to call pin_fence(). This will make it easier to reduce the locking requirements around fence registers. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_drv.h | 4 ---- drivers/gpu/drm/i915/i915_gem.c | 3 ++- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 10 +++++----- drivers/gpu/drm/i915/i915_gem_fence_reg.c | 27 ++++++++++++++++++++++----- drivers/gpu/drm/i915/i915_vma.c | 3 +-- drivers/gpu/drm/i915/i915_vma.h | 20 ++++++++------------ drivers/gpu/drm/i915/intel_display.c | 3 +-- 7 files changed, 39 insertions(+), 31 deletions(-)