| Message ID | 1375315222-4785-19-git-send-email-ben@bwidawsk.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jul 31, 2013 at 05:00:11PM -0700, Ben Widawsky wrote: > Eviction code, like the rest of the converted code needs to be aware of > the address space for which it is evicting (or the everything case, all > addresses). With the updated bind/unbind interfaces of the last patch, > we can now safely move the eviction code over. > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> Two comments below. -Daniel > --- > drivers/gpu/drm/i915/i915_drv.h | 4 ++- > drivers/gpu/drm/i915/i915_gem.c | 2 +- > drivers/gpu/drm/i915/i915_gem_evict.c | 53 +++++++++++++++++++---------------- > 3 files changed, 33 insertions(+), 26 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 0610588..bf1ecef 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1946,7 +1946,9 @@ static inline void i915_gem_chipset_flush(struct drm_device *dev) > > > /* i915_gem_evict.c */ > -int __must_check i915_gem_evict_something(struct drm_device *dev, int min_size, > +int __must_check i915_gem_evict_something(struct drm_device *dev, > + struct i915_address_space *vm, > + int min_size, > unsigned alignment, > unsigned cache_level, > bool mappable, > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 0cb36c2..1013105 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -3159,7 +3159,7 @@ search_free: > size, alignment, > obj->cache_level, 0, gtt_max); > if (ret) { > - ret = i915_gem_evict_something(dev, size, alignment, > + ret = i915_gem_evict_something(dev, vm, size, alignment, > obj->cache_level, > map_and_fenceable, > nonblocking); > diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c > index 9205a41..61bf5e2 100644 > --- a/drivers/gpu/drm/i915/i915_gem_evict.c > +++ b/drivers/gpu/drm/i915/i915_gem_evict.c > @@ -32,26 +32,21 @@ > #include "i915_trace.h" > > static bool > -mark_free(struct drm_i915_gem_object *obj, struct list_head *unwind) > +mark_free(struct i915_vma *vma, struct list_head *unwind) > { > - struct drm_device *dev = obj->base.dev; > - struct drm_i915_private *dev_priv = dev->dev_private; > - struct i915_vma *vma = i915_gem_obj_to_vma(obj, &dev_priv->gtt.base); > - > - if (obj->pin_count) > + if (vma->obj->pin_count) > return false; > > - list_add(&obj->exec_list, unwind); > + list_add(&vma->obj->exec_list, unwind); > return drm_mm_scan_add_block(&vma->node); > } > > int > -i915_gem_evict_something(struct drm_device *dev, int min_size, > - unsigned alignment, unsigned cache_level, > +i915_gem_evict_something(struct drm_device *dev, struct i915_address_space *vm, > + int min_size, unsigned alignment, unsigned cache_level, > bool mappable, bool nonblocking) > { > drm_i915_private_t *dev_priv = dev->dev_private; > - struct i915_address_space *vm = &dev_priv->gtt.base; > struct list_head eviction_list, unwind_list; > struct i915_vma *vma; > struct drm_i915_gem_object *obj; > @@ -83,16 +78,18 @@ i915_gem_evict_something(struct drm_device *dev, int min_size, > */ > > INIT_LIST_HEAD(&unwind_list); > - if (mappable) > + if (mappable) { > + BUG_ON(!i915_is_ggtt(vm)); > drm_mm_init_scan_with_range(&vm->mm, min_size, > alignment, cache_level, 0, > dev_priv->gtt.mappable_end); > - else > + } else > drm_mm_init_scan(&vm->mm, min_size, alignment, cache_level); > > /* First see if there is a large enough contiguous idle region... */ > list_for_each_entry(obj, &vm->inactive_list, mm_list) { > - if (mark_free(obj, &unwind_list)) > + struct i915_vma *vma = i915_gem_obj_to_vma(obj, vm); > + if (mark_free(vma, &unwind_list)) > goto found; > } > > @@ -101,7 +98,8 @@ i915_gem_evict_something(struct drm_device *dev, int min_size, > > /* Now merge in the soon-to-be-expired objects... */ > list_for_each_entry(obj, &vm->active_list, mm_list) { > - if (mark_free(obj, &unwind_list)) > + struct i915_vma *vma = i915_gem_obj_to_vma(obj, vm); > + if (mark_free(vma, &unwind_list)) > goto found; > } > > @@ -111,7 +109,7 @@ none: > obj = list_first_entry(&unwind_list, > struct drm_i915_gem_object, > exec_list); > - vma = i915_gem_obj_to_vma(obj, &dev_priv->gtt.base); > + vma = i915_gem_obj_to_vma(obj, vm); > ret = drm_mm_scan_remove_block(&vma->node); > BUG_ON(ret); > > @@ -132,7 +130,7 @@ found: > obj = list_first_entry(&unwind_list, > struct drm_i915_gem_object, > exec_list); > - vma = i915_gem_obj_to_vma(obj, &dev_priv->gtt.base); > + vma = i915_gem_obj_to_vma(obj, vm); > if (drm_mm_scan_remove_block(&vma->node)) { > list_move(&obj->exec_list, &eviction_list); > drm_gem_object_reference(&obj->base); > @@ -147,7 +145,7 @@ found: > struct drm_i915_gem_object, > exec_list); > if (ret == 0) > - ret = i915_gem_object_ggtt_unbind(obj); > + ret = i915_vma_unbind(i915_gem_obj_to_vma(obj, vm)); Again I think the ggtt_unbind->vma_unbind conversion seems to leak the vma. It feels like vma_unbind should call vma_destroy? > > list_del_init(&obj->exec_list); > drm_gem_object_unreference(&obj->base); > @@ -160,13 +158,18 @@ int > i915_gem_evict_everything(struct drm_device *dev) > { > drm_i915_private_t *dev_priv = dev->dev_private; > - struct i915_address_space *vm = &dev_priv->gtt.base; > + struct i915_address_space *vm; > struct drm_i915_gem_object *obj, *next; > - bool lists_empty; > + bool lists_empty = true; > int ret; > > - lists_empty = (list_empty(&vm->inactive_list) && > - list_empty(&vm->active_list)); > + list_for_each_entry(vm, &dev_priv->vm_list, global_link) { > + lists_empty = (list_empty(&vm->inactive_list) && > + list_empty(&vm->active_list)); > + if (!lists_empty) > + lists_empty = false; > + } > + > if (lists_empty) > return -ENOSPC; > > @@ -183,9 +186,11 @@ i915_gem_evict_everything(struct drm_device *dev) > i915_gem_retire_requests(dev); > > /* Having flushed everything, unbind() should never raise an error */ > - list_for_each_entry_safe(obj, next, &vm->inactive_list, mm_list) > - if (obj->pin_count == 0) > - WARN_ON(i915_gem_object_ggtt_unbind(obj)); > + list_for_each_entry(vm, &dev_priv->vm_list, global_link) { > + list_for_each_entry_safe(obj, next, &vm->inactive_list, mm_list) > + if (obj->pin_count == 0) > + WARN_ON(i915_vma_unbind(i915_gem_obj_to_vma(obj, vm))); > + } The conversion of evict_everything looks a bit strange. Essentially we have tree callers: - ums+gem support code in leavevt to rid the gtt of all gem objects when the userspace X ums ddx stops controlling the hw. - When we seriously ran out of memory in, shrink_all. - In execbuf when we've fragmented the gtt address space so badly that we need to start over completely fresh. With this it imo would make sense to just loop over the global bound object lists. But for the execbuf caller adding a vm parameter (and only evicting from that special vm, skipping all others) would make sense. Other callers would pass NULL since they want everything to get evicted. Volunteered for that follow-up? > > return 0; > } > -- > 1.8.3.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Aug 06, 2013 at 08:39:50PM +0200, Daniel Vetter wrote: > On Wed, Jul 31, 2013 at 05:00:11PM -0700, Ben Widawsky wrote: > > Eviction code, like the rest of the converted code needs to be aware of > > the address space for which it is evicting (or the everything case, all > > addresses). With the updated bind/unbind interfaces of the last patch, > > we can now safely move the eviction code over. > > > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > Two comments below. > -Daniel > > > --- > > drivers/gpu/drm/i915/i915_drv.h | 4 ++- > > drivers/gpu/drm/i915/i915_gem.c | 2 +- > > drivers/gpu/drm/i915/i915_gem_evict.c | 53 +++++++++++++++++++---------------- > > 3 files changed, 33 insertions(+), 26 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index 0610588..bf1ecef 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -1946,7 +1946,9 @@ static inline void i915_gem_chipset_flush(struct drm_device *dev) > > > > > > /* i915_gem_evict.c */ > > -int __must_check i915_gem_evict_something(struct drm_device *dev, int min_size, > > +int __must_check i915_gem_evict_something(struct drm_device *dev, > > + struct i915_address_space *vm, > > + int min_size, > > unsigned alignment, > > unsigned cache_level, > > bool mappable, > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > index 0cb36c2..1013105 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -3159,7 +3159,7 @@ search_free: > > size, alignment, > > obj->cache_level, 0, gtt_max); > > if (ret) { > > - ret = i915_gem_evict_something(dev, size, alignment, > > + ret = i915_gem_evict_something(dev, vm, size, alignment, > > obj->cache_level, > > map_and_fenceable, > > nonblocking); > > diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c > > index 9205a41..61bf5e2 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_evict.c > > +++ b/drivers/gpu/drm/i915/i915_gem_evict.c > > @@ -32,26 +32,21 @@ > > #include "i915_trace.h" > > > > static bool > > -mark_free(struct drm_i915_gem_object *obj, struct list_head *unwind) > > +mark_free(struct i915_vma *vma, struct list_head *unwind) > > { > > - struct drm_device *dev = obj->base.dev; > > - struct drm_i915_private *dev_priv = dev->dev_private; > > - struct i915_vma *vma = i915_gem_obj_to_vma(obj, &dev_priv->gtt.base); > > - > > - if (obj->pin_count) > > + if (vma->obj->pin_count) > > return false; > > > > - list_add(&obj->exec_list, unwind); > > + list_add(&vma->obj->exec_list, unwind); > > return drm_mm_scan_add_block(&vma->node); > > } > > > > int > > -i915_gem_evict_something(struct drm_device *dev, int min_size, > > - unsigned alignment, unsigned cache_level, > > +i915_gem_evict_something(struct drm_device *dev, struct i915_address_space *vm, > > + int min_size, unsigned alignment, unsigned cache_level, > > bool mappable, bool nonblocking) > > { > > drm_i915_private_t *dev_priv = dev->dev_private; > > - struct i915_address_space *vm = &dev_priv->gtt.base; > > struct list_head eviction_list, unwind_list; > > struct i915_vma *vma; > > struct drm_i915_gem_object *obj; > > @@ -83,16 +78,18 @@ i915_gem_evict_something(struct drm_device *dev, int min_size, > > */ > > > > INIT_LIST_HEAD(&unwind_list); > > - if (mappable) > > + if (mappable) { > > + BUG_ON(!i915_is_ggtt(vm)); > > drm_mm_init_scan_with_range(&vm->mm, min_size, > > alignment, cache_level, 0, > > dev_priv->gtt.mappable_end); > > - else > > + } else > > drm_mm_init_scan(&vm->mm, min_size, alignment, cache_level); > > > > /* First see if there is a large enough contiguous idle region... */ > > list_for_each_entry(obj, &vm->inactive_list, mm_list) { > > - if (mark_free(obj, &unwind_list)) > > + struct i915_vma *vma = i915_gem_obj_to_vma(obj, vm); > > + if (mark_free(vma, &unwind_list)) > > goto found; > > } > > > > @@ -101,7 +98,8 @@ i915_gem_evict_something(struct drm_device *dev, int min_size, > > > > /* Now merge in the soon-to-be-expired objects... */ > > list_for_each_entry(obj, &vm->active_list, mm_list) { > > - if (mark_free(obj, &unwind_list)) > > + struct i915_vma *vma = i915_gem_obj_to_vma(obj, vm); > > + if (mark_free(vma, &unwind_list)) > > goto found; > > } > > > > @@ -111,7 +109,7 @@ none: > > obj = list_first_entry(&unwind_list, > > struct drm_i915_gem_object, > > exec_list); > > - vma = i915_gem_obj_to_vma(obj, &dev_priv->gtt.base); > > + vma = i915_gem_obj_to_vma(obj, vm); > > ret = drm_mm_scan_remove_block(&vma->node); > > BUG_ON(ret); > > > > @@ -132,7 +130,7 @@ found: > > obj = list_first_entry(&unwind_list, > > struct drm_i915_gem_object, > > exec_list); > > - vma = i915_gem_obj_to_vma(obj, &dev_priv->gtt.base); > > + vma = i915_gem_obj_to_vma(obj, vm); > > if (drm_mm_scan_remove_block(&vma->node)) { > > list_move(&obj->exec_list, &eviction_list); > > drm_gem_object_reference(&obj->base); > > @@ -147,7 +145,7 @@ found: > > struct drm_i915_gem_object, > > exec_list); > > if (ret == 0) > > - ret = i915_gem_object_ggtt_unbind(obj); > > + ret = i915_vma_unbind(i915_gem_obj_to_vma(obj, vm)); > > Again I think the ggtt_unbind->vma_unbind conversion seems to leak the > vma. It feels like vma_unbind should call vma_destroy? > The VMA should get cleaned up when an object backing the vmas is destroyed also. I agree that at present, unbind and destroy have little distinction, but I can foresee that changing. Proper faulting is one such case OTTOMH Anyway, let me know if your leak concern is addressed. > > > > list_del_init(&obj->exec_list); > > drm_gem_object_unreference(&obj->base); > > @@ -160,13 +158,18 @@ int > > i915_gem_evict_everything(struct drm_device *dev) > > { > > drm_i915_private_t *dev_priv = dev->dev_private; > > - struct i915_address_space *vm = &dev_priv->gtt.base; > > + struct i915_address_space *vm; > > struct drm_i915_gem_object *obj, *next; > > - bool lists_empty; > > + bool lists_empty = true; > > int ret; > > > > - lists_empty = (list_empty(&vm->inactive_list) && > > - list_empty(&vm->active_list)); > > + list_for_each_entry(vm, &dev_priv->vm_list, global_link) { > > + lists_empty = (list_empty(&vm->inactive_list) && > > + list_empty(&vm->active_list)); > > + if (!lists_empty) > > + lists_empty = false; > > + } > > + > > if (lists_empty) > > return -ENOSPC; > > > > @@ -183,9 +186,11 @@ i915_gem_evict_everything(struct drm_device *dev) > > i915_gem_retire_requests(dev); > > > > /* Having flushed everything, unbind() should never raise an error */ > > - list_for_each_entry_safe(obj, next, &vm->inactive_list, mm_list) > > - if (obj->pin_count == 0) > > - WARN_ON(i915_gem_object_ggtt_unbind(obj)); > > + list_for_each_entry(vm, &dev_priv->vm_list, global_link) { > > + list_for_each_entry_safe(obj, next, &vm->inactive_list, mm_list) > > + if (obj->pin_count == 0) > > + WARN_ON(i915_vma_unbind(i915_gem_obj_to_vma(obj, vm))); > > + } > > The conversion of evict_everything looks a bit strange. Essentially we > have tree callers: > - ums+gem support code in leavevt to rid the gtt of all gem objects when > the userspace X ums ddx stops controlling the hw. > - When we seriously ran out of memory in, shrink_all. > - In execbuf when we've fragmented the gtt address space so badly that we > need to start over completely fresh. > > With this it imo would make sense to just loop over the global bound > object lists. But for the execbuf caller adding a vm parameter (and only > evicting from that special vm, skipping all others) would make sense. > Other callers would pass NULL since they want everything to get evicted. > Volunteered for that follow-up? > We need evict_everything for #1. For #2, we call evict_something already for the vm when we go through the out of space path. If that failed, evicting everything for a specific VM is just the same operation. But maybe I've glossed over something in the details. Please correct if I'm wrong. Is there a case where evict something might fail with ENOSPC, and evict everything in a VM would help? > > > > return 0; > > }
On Tue, Aug 06, 2013 at 02:27:39PM -0700, Ben Widawsky wrote: > On Tue, Aug 06, 2013 at 08:39:50PM +0200, Daniel Vetter wrote: > > On Wed, Jul 31, 2013 at 05:00:11PM -0700, Ben Widawsky wrote: > > > @@ -183,9 +186,11 @@ i915_gem_evict_everything(struct drm_device *dev) > > > i915_gem_retire_requests(dev); > > > > > > /* Having flushed everything, unbind() should never raise an error */ > > > - list_for_each_entry_safe(obj, next, &vm->inactive_list, mm_list) > > > - if (obj->pin_count == 0) > > > - WARN_ON(i915_gem_object_ggtt_unbind(obj)); > > > + list_for_each_entry(vm, &dev_priv->vm_list, global_link) { > > > + list_for_each_entry_safe(obj, next, &vm->inactive_list, mm_list) > > > + if (obj->pin_count == 0) > > > + WARN_ON(i915_vma_unbind(i915_gem_obj_to_vma(obj, vm))); > > > + } > > > > The conversion of evict_everything looks a bit strange. Essentially we > > have tree callers: > > - ums+gem support code in leavevt to rid the gtt of all gem objects when > > the userspace X ums ddx stops controlling the hw. > > - When we seriously ran out of memory in, shrink_all. > > - In execbuf when we've fragmented the gtt address space so badly that we > > need to start over completely fresh. > > > > With this it imo would make sense to just loop over the global bound > > object lists. But for the execbuf caller adding a vm parameter (and only > > evicting from that special vm, skipping all others) would make sense. > > Other callers would pass NULL since they want everything to get evicted. > > Volunteered for that follow-up? > > > > We need evict_everything for #1. For #2, we call evict_something already > for the vm when we go through the out of space path. If that failed, > evicting everything for a specific VM is just the same operation. But > maybe I've glossed over something in the details. Please correct if I'm > wrong. Is there a case where evict something might fail with ENOSPC, and > evict everything in a VM would help? Yes, when we've terminally fragmented the gtt we kick out everything and start over. That's the 3rd usecase. -Daniel
On Tue, Aug 06, 2013 at 11:29:49PM +0200, Daniel Vetter wrote: > On Tue, Aug 06, 2013 at 02:27:39PM -0700, Ben Widawsky wrote: > > On Tue, Aug 06, 2013 at 08:39:50PM +0200, Daniel Vetter wrote: > > > On Wed, Jul 31, 2013 at 05:00:11PM -0700, Ben Widawsky wrote: > > > > @@ -183,9 +186,11 @@ i915_gem_evict_everything(struct drm_device *dev) > > > > i915_gem_retire_requests(dev); > > > > > > > > /* Having flushed everything, unbind() should never raise an error */ > > > > - list_for_each_entry_safe(obj, next, &vm->inactive_list, mm_list) > > > > - if (obj->pin_count == 0) > > > > - WARN_ON(i915_gem_object_ggtt_unbind(obj)); > > > > + list_for_each_entry(vm, &dev_priv->vm_list, global_link) { > > > > + list_for_each_entry_safe(obj, next, &vm->inactive_list, mm_list) > > > > + if (obj->pin_count == 0) > > > > + WARN_ON(i915_vma_unbind(i915_gem_obj_to_vma(obj, vm))); > > > > + } > > > > > > The conversion of evict_everything looks a bit strange. Essentially we > > > have tree callers: > > > - ums+gem support code in leavevt to rid the gtt of all gem objects when > > > the userspace X ums ddx stops controlling the hw. > > > - When we seriously ran out of memory in, shrink_all. > > > - In execbuf when we've fragmented the gtt address space so badly that we > > > need to start over completely fresh. > > > > > > With this it imo would make sense to just loop over the global bound > > > object lists. But for the execbuf caller adding a vm parameter (and only > > > evicting from that special vm, skipping all others) would make sense. > > > Other callers would pass NULL since they want everything to get evicted. > > > Volunteered for that follow-up? > > > > > > > We need evict_everything for #1. For #2, we call evict_something already > > for the vm when we go through the out of space path. If that failed, > > evicting everything for a specific VM is just the same operation. But > > maybe I've glossed over something in the details. Please correct if I'm > > wrong. Is there a case where evict something might fail with ENOSPC, and > > evict everything in a VM would help? > > Yes, when we've terminally fragmented the gtt we kick out everything and > start over. That's the 3rd usecase. > -Daniel I am not seeing it. To me evict_something is what you want, and the fix for wherever the 3rd usecase is (please point it out, I'm dense) is it should call evict_something, not evict_everything. If by GTT you mean the aperture... that's kind of a different can of worms completely. In that case I don't think you want to do anything per VM, though potentially you can do it that way and be a little fairer.
On Wed, Aug 7, 2013 at 12:57 AM, Ben Widawsky <ben@bwidawsk.net> wrote: >> > We need evict_everything for #1. For #2, we call evict_something already >> > for the vm when we go through the out of space path. If that failed, >> > evicting everything for a specific VM is just the same operation. But >> > maybe I've glossed over something in the details. Please correct if I'm >> > wrong. Is there a case where evict something might fail with ENOSPC, and >> > evict everything in a VM would help? >> >> Yes, when we've terminally fragmented the gtt we kick out everything and >> start over. That's the 3rd usecase. > > I am not seeing it. To me evict_something is what you want, and the fix > for wherever the 3rd usecase is (please point it out, I'm dense) is it > should call evict_something, not evict_everything. See the call to evict_everything in i915_gem_execbuffer.c:i915_gem_execbuffer_reserve Cheers, Daniel
On Wed, Aug 07, 2013 at 12:59:29AM +0200, Daniel Vetter wrote: > On Wed, Aug 7, 2013 at 12:57 AM, Ben Widawsky <ben@bwidawsk.net> wrote: > >> > We need evict_everything for #1. For #2, we call evict_something already > >> > for the vm when we go through the out of space path. If that failed, > >> > evicting everything for a specific VM is just the same operation. But > >> > maybe I've glossed over something in the details. Please correct if I'm > >> > wrong. Is there a case where evict something might fail with ENOSPC, and > >> > evict everything in a VM would help? > >> > >> Yes, when we've terminally fragmented the gtt we kick out everything and > >> start over. That's the 3rd usecase. > > > > I am not seeing it. To me evict_something is what you want, and the fix > > for wherever the 3rd usecase is (please point it out, I'm dense) is it > > should call evict_something, not evict_everything. > > See the call to evict_everything in > i915_gem_execbuffer.c:i915_gem_execbuffer_reserve > As I was saying in the first response - you only hit this if evict_something() for a vm fails, right? That's the way ret == ENOSPC AFAICT.
On Wed, Aug 7, 2013 at 1:25 AM, Ben Widawsky <ben@bwidawsk.net> wrote: > On Wed, Aug 07, 2013 at 12:59:29AM +0200, Daniel Vetter wrote: >> On Wed, Aug 7, 2013 at 12:57 AM, Ben Widawsky <ben@bwidawsk.net> wrote: >> >> > We need evict_everything for #1. For #2, we call evict_something already >> >> > for the vm when we go through the out of space path. If that failed, >> >> > evicting everything for a specific VM is just the same operation. But >> >> > maybe I've glossed over something in the details. Please correct if I'm >> >> > wrong. Is there a case where evict something might fail with ENOSPC, and >> >> > evict everything in a VM would help? >> >> >> >> Yes, when we've terminally fragmented the gtt we kick out everything and >> >> start over. That's the 3rd usecase. >> > >> > I am not seeing it. To me evict_something is what you want, and the fix >> > for wherever the 3rd usecase is (please point it out, I'm dense) is it >> > should call evict_something, not evict_everything. >> >> See the call to evict_everything in >> i915_gem_execbuffer.c:i915_gem_execbuffer_reserve >> > > As I was saying in the first response - you only hit this if > evict_something() for a vm fails, right? That's the way ret == ENOSPC > AFAICT. Like I've said if we can't fit a batch we do a last ditch effort of evicting everything and starting over anew. That's also what the retry logic in there is for. This happens after evict_something failed. Dunno what exactly isn't clear or what's confusing ... -Daniel
On Wed, Aug 07, 2013 at 01:44:58AM +0200, Daniel Vetter wrote: > On Wed, Aug 7, 2013 at 1:25 AM, Ben Widawsky <ben@bwidawsk.net> wrote: > > On Wed, Aug 07, 2013 at 12:59:29AM +0200, Daniel Vetter wrote: > >> On Wed, Aug 7, 2013 at 12:57 AM, Ben Widawsky <ben@bwidawsk.net> wrote: > >> >> > We need evict_everything for #1. For #2, we call evict_something already > >> >> > for the vm when we go through the out of space path. If that failed, > >> >> > evicting everything for a specific VM is just the same operation. But > >> >> > maybe I've glossed over something in the details. Please correct if I'm > >> >> > wrong. Is there a case where evict something might fail with ENOSPC, and > >> >> > evict everything in a VM would help? > >> >> > >> >> Yes, when we've terminally fragmented the gtt we kick out everything and > >> >> start over. That's the 3rd usecase. > >> > > >> > I am not seeing it. To me evict_something is what you want, and the fix > >> > for wherever the 3rd usecase is (please point it out, I'm dense) is it > >> > should call evict_something, not evict_everything. > >> > >> See the call to evict_everything in > >> i915_gem_execbuffer.c:i915_gem_execbuffer_reserve > >> > > > > As I was saying in the first response - you only hit this if > > evict_something() for a vm fails, right? That's the way ret == ENOSPC > > AFAICT. > > Like I've said if we can't fit a batch we do a last ditch effort of > evicting everything and starting over anew. That's also what the retry > logic in there is for. This happens after evict_something failed. > Dunno what exactly isn't clear or what's confusing ... > -Daniel Okay, sorted this out on IRC. You'll get a new patch as described with a new function for per vm eviction (which will just idle, and call evict_something() with proper args)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 0610588..bf1ecef 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1946,7 +1946,9 @@ static inline void i915_gem_chipset_flush(struct drm_device *dev) /* i915_gem_evict.c */ -int __must_check i915_gem_evict_something(struct drm_device *dev, int min_size, +int __must_check i915_gem_evict_something(struct drm_device *dev, + struct i915_address_space *vm, + int min_size, unsigned alignment, unsigned cache_level, bool mappable, diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 0cb36c2..1013105 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -3159,7 +3159,7 @@ search_free: size, alignment, obj->cache_level, 0, gtt_max); if (ret) { - ret = i915_gem_evict_something(dev, size, alignment, + ret = i915_gem_evict_something(dev, vm, size, alignment, obj->cache_level, map_and_fenceable, nonblocking); diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c index 9205a41..61bf5e2 100644 --- a/drivers/gpu/drm/i915/i915_gem_evict.c +++ b/drivers/gpu/drm/i915/i915_gem_evict.c @@ -32,26 +32,21 @@ #include "i915_trace.h" static bool -mark_free(struct drm_i915_gem_object *obj, struct list_head *unwind) +mark_free(struct i915_vma *vma, struct list_head *unwind) { - struct drm_device *dev = obj->base.dev; - struct drm_i915_private *dev_priv = dev->dev_private; - struct i915_vma *vma = i915_gem_obj_to_vma(obj, &dev_priv->gtt.base); - - if (obj->pin_count) + if (vma->obj->pin_count) return false; - list_add(&obj->exec_list, unwind); + list_add(&vma->obj->exec_list, unwind); return drm_mm_scan_add_block(&vma->node); } int -i915_gem_evict_something(struct drm_device *dev, int min_size, - unsigned alignment, unsigned cache_level, +i915_gem_evict_something(struct drm_device *dev, struct i915_address_space *vm, + int min_size, unsigned alignment, unsigned cache_level, bool mappable, bool nonblocking) { drm_i915_private_t *dev_priv = dev->dev_private; - struct i915_address_space *vm = &dev_priv->gtt.base; struct list_head eviction_list, unwind_list; struct i915_vma *vma; struct drm_i915_gem_object *obj; @@ -83,16 +78,18 @@ i915_gem_evict_something(struct drm_device *dev, int min_size, */ INIT_LIST_HEAD(&unwind_list); - if (mappable) + if (mappable) { + BUG_ON(!i915_is_ggtt(vm)); drm_mm_init_scan_with_range(&vm->mm, min_size, alignment, cache_level, 0, dev_priv->gtt.mappable_end); - else + } else drm_mm_init_scan(&vm->mm, min_size, alignment, cache_level); /* First see if there is a large enough contiguous idle region... */ list_for_each_entry(obj, &vm->inactive_list, mm_list) { - if (mark_free(obj, &unwind_list)) + struct i915_vma *vma = i915_gem_obj_to_vma(obj, vm); + if (mark_free(vma, &unwind_list)) goto found; } @@ -101,7 +98,8 @@ i915_gem_evict_something(struct drm_device *dev, int min_size, /* Now merge in the soon-to-be-expired objects... */ list_for_each_entry(obj, &vm->active_list, mm_list) { - if (mark_free(obj, &unwind_list)) + struct i915_vma *vma = i915_gem_obj_to_vma(obj, vm); + if (mark_free(vma, &unwind_list)) goto found; } @@ -111,7 +109,7 @@ none: obj = list_first_entry(&unwind_list, struct drm_i915_gem_object, exec_list); - vma = i915_gem_obj_to_vma(obj, &dev_priv->gtt.base); + vma = i915_gem_obj_to_vma(obj, vm); ret = drm_mm_scan_remove_block(&vma->node); BUG_ON(ret); @@ -132,7 +130,7 @@ found: obj = list_first_entry(&unwind_list, struct drm_i915_gem_object, exec_list); - vma = i915_gem_obj_to_vma(obj, &dev_priv->gtt.base); + vma = i915_gem_obj_to_vma(obj, vm); if (drm_mm_scan_remove_block(&vma->node)) { list_move(&obj->exec_list, &eviction_list); drm_gem_object_reference(&obj->base); @@ -147,7 +145,7 @@ found: struct drm_i915_gem_object, exec_list); if (ret == 0) - ret = i915_gem_object_ggtt_unbind(obj); + ret = i915_vma_unbind(i915_gem_obj_to_vma(obj, vm)); list_del_init(&obj->exec_list); drm_gem_object_unreference(&obj->base); @@ -160,13 +158,18 @@ int i915_gem_evict_everything(struct drm_device *dev) { drm_i915_private_t *dev_priv = dev->dev_private; - struct i915_address_space *vm = &dev_priv->gtt.base; + struct i915_address_space *vm; struct drm_i915_gem_object *obj, *next; - bool lists_empty; + bool lists_empty = true; int ret; - lists_empty = (list_empty(&vm->inactive_list) && - list_empty(&vm->active_list)); + list_for_each_entry(vm, &dev_priv->vm_list, global_link) { + lists_empty = (list_empty(&vm->inactive_list) && + list_empty(&vm->active_list)); + if (!lists_empty) + lists_empty = false; + } + if (lists_empty) return -ENOSPC; @@ -183,9 +186,11 @@ i915_gem_evict_everything(struct drm_device *dev) i915_gem_retire_requests(dev); /* Having flushed everything, unbind() should never raise an error */ - list_for_each_entry_safe(obj, next, &vm->inactive_list, mm_list) - if (obj->pin_count == 0) - WARN_ON(i915_gem_object_ggtt_unbind(obj)); + list_for_each_entry(vm, &dev_priv->vm_list, global_link) { + list_for_each_entry_safe(obj, next, &vm->inactive_list, mm_list) + if (obj->pin_count == 0) + WARN_ON(i915_vma_unbind(i915_gem_obj_to_vma(obj, vm))); + } return 0; }
Eviction code, like the rest of the converted code needs to be aware of the address space for which it is evicting (or the everything case, all addresses). With the updated bind/unbind interfaces of the last patch, we can now safely move the eviction code over. Signed-off-by: Ben Widawsky <ben@bwidawsk.net> --- drivers/gpu/drm/i915/i915_drv.h | 4 ++- drivers/gpu/drm/i915/i915_gem.c | 2 +- drivers/gpu/drm/i915/i915_gem_evict.c | 53 +++++++++++++++++++---------------- 3 files changed, 33 insertions(+), 26 deletions(-)