| Message ID | 1442306007-10792-4-git-send-email-ankitprasad.r.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Sep 15, 2015 at 02:03:26PM +0530, ankitprasad.r.sharma@intel.com wrote: > -struct drm_i915_gem_object * > -i915_gem_object_create_stolen(struct drm_device *dev, u64 size) > +static bool mark_free(struct drm_i915_gem_object *obj, struct list_head *unwind) > +{ > + if (obj->stolen == NULL) > + return false; BUG_ON(obj->stolen == NULL); > + > + if (obj->madv != I915_MADV_DONTNEED) > + return false; > + > + if (obj->pin_display) > + return false; > + > + if (I915_BO_IS_ACTIVE(obj)) > + return false; We want to add both active/inactive objects (ordering by the caller to prioritise inactive objects). > + list_add(&obj->tmp_link, unwind); > + return drm_mm_scan_add_block(&obj->stolen->base); > +} > + > +static int > +stolen_evict(struct drm_i915_private *dev_priv, u64 size) > { > - struct drm_i915_private *dev_priv = dev->dev_private; > struct drm_i915_gem_object *obj; > - struct drm_mm_node *stolen; > + struct list_head unwind, evict; > + struct i915_stolen_node *iter; > int ret; > > - if (!drm_mm_initialized(&dev_priv->mm.stolen)) > - return NULL; > + drm_mm_init_scan(&dev_priv->mm.stolen, size, 0, 0); > + INIT_LIST_HEAD(&unwind); > + > + list_for_each_entry(iter, &dev_priv->mm.stolen_list, mm_link) { > + if (I915_BO_IS_ACTIVE(iter->obj)) > + continue; > + > + if (mark_free(iter->obj, &unwind)) > + goto found; > + } > + > + list_for_each_entry(iter, &dev_priv->mm.stolen_list, mm_link) { > + if (!I915_BO_IS_ACTIVE(iter->obj)) > + continue; > + > + if (mark_free(iter->obj, &unwind)) > + goto found; > + } If I have my micro-optimising hat on, I would actually rewrite this as soemthing like: for (int phase = 0; phase <= 1; phase++) { list_for_each_entry(iter, &dev_priv->mm.stolen_list, mm_link) { if (I915_BO_IS_ACTIVE(iter->obj) != phase) continue; if (mark_free(iter->obj, &unwind)) goto found; } } as the compiler will find that easier to perform magic with. We also probably want to do i915_gem_retire_requests() first (so that any recently idle objects are moved to the front queue). > +found: > + INIT_LIST_HEAD(&evict); > + while (!list_empty(&unwind)) { > + obj = list_first_entry(&unwind, > + struct drm_i915_gem_object, > + tmp_link); > + list_del_init(&obj->tmp_link); The fun thing with tmp_link is that we don't need to set it to clear (either here or during object construction). -Chris
On 09/15/2015 09:33 AM, ankitprasad.r.sharma@intel.com wrote: > From: Chris Wilson <chris at chris-wilson.co.uk> > > If we run out of stolen memory when trying to allocate an object, see if > we can reap enough purgeable objects to free up enough contiguous free > space for the allocation. This is in principle very much like evicting > objects to free up enough contiguous space in the vma when binding > a new object - and you will be forgiven for thinking that the code looks > very similar. > > At the moment, we do not allow userspace to allocate objects in stolen, > so there is neither the memory pressure to trigger stolen eviction nor > any purgeable objects inside the stolen arena. However, this will change > in the near future, and so better management and defragmentation of > stolen memory will become a real issue. > > v2: Remember to remove the drm_mm_node. > > v3: Rebased to the latest drm-intel-nightly (Ankit) > > v4: corrected if-else braces format (Tvrtko/kerneldoc) > > v5: Rebased to the latest drm-intel-nightly (Ankit) > Added a seperate list to maintain purgable objects from stolen memory > region (Chris/Daniel) > > Testcase: igt/gem_stolen > > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk> > Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 4 +- > drivers/gpu/drm/i915/i915_drv.h | 17 +++- > drivers/gpu/drm/i915/i915_gem.c | 16 +++ > drivers/gpu/drm/i915/i915_gem_stolen.c | 176 ++++++++++++++++++++++++++++----- > drivers/gpu/drm/i915/intel_pm.c | 4 +- > 5 files changed, 187 insertions(+), 30 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index 7a28de5..0db8c47 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -179,7 +179,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj) > seq_puts(m, ")"); > } > if (obj->stolen) > - seq_printf(m, " (stolen: %08llx)", obj->stolen->start); > + seq_printf(m, " (stolen: %08llx)", obj->stolen->base.start); > if (obj->pin_display || obj->fault_mappable) { > char s[3], *t = s; > if (obj->pin_display) > @@ -258,7 +258,7 @@ static int obj_rank_by_stolen(void *priv, > struct drm_i915_gem_object *b = > container_of(B, struct drm_i915_gem_object, obj_exec_link); > > - return a->stolen->start - b->stolen->start; > + return a->stolen->base.start - b->stolen->base.start; > } > > static int i915_gem_stolen_list_info(struct seq_file *m, void *data) > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index e6ef083..37ee32d 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -841,6 +841,12 @@ struct i915_ctx_hang_stats { > bool banned; > }; > > +struct i915_stolen_node { > + struct drm_mm_node base; > + struct list_head mm_link; > + struct drm_i915_gem_object *obj; > +}; > + > /* This must match up with the value previously used for execbuf2.rsvd1. */ > #define DEFAULT_CONTEXT_HANDLE 0 > > @@ -1268,6 +1274,13 @@ struct i915_gem_mm { > */ > struct list_head unbound_list; > > + /** > + * List of stolen objects that have been marked as purgeable and > + * thus available for reaping if we need more space for a new > + * allocation. Ordered by time of marking purgeable. > + */ > + struct list_head stolen_list; > + > /** Usable portion of the GTT for GEM */ > unsigned long stolen_base; /* limited to low memory (32-bit) */ > > @@ -2026,7 +2039,7 @@ struct drm_i915_gem_object { > struct list_head vma_list; > > /** Stolen memory for this object, instead of being backed by shmem. */ > - struct drm_mm_node *stolen; > + struct i915_stolen_node *stolen; > struct list_head global_list; > > struct list_head ring_list[I915_NUM_RINGS]; > @@ -2034,6 +2047,7 @@ struct drm_i915_gem_object { > struct list_head obj_exec_link; > > struct list_head batch_pool_link; > + struct list_head tmp_link; > > /** > * This is set if the object is on the active lists (has pending > @@ -2150,6 +2164,7 @@ struct drm_i915_gem_object { > }; > }; > #define to_intel_bo(x) container_of(x, struct drm_i915_gem_object, base) > +#define I915_BO_IS_ACTIVE(__obj) (__obj->active) > > void i915_gem_track_fb(struct drm_i915_gem_object *old, > struct drm_i915_gem_object *new, > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 6568a7f..85025b1 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -4228,6 +4228,20 @@ i915_gem_madvise_ioctl(struct drm_device *dev, void *data, > if (obj->madv == I915_MADV_DONTNEED && obj->pages == NULL) > i915_gem_object_truncate(obj); > > + if (obj->stolen) { > + switch (obj->madv) { > + case I915_MADV_WILLNEED: > + list_del_init(&obj->stolen->mm_link); > + break; > + case I915_MADV_DONTNEED: > + list_move(&obj->stolen->mm_link, > + &dev_priv->mm.stolen_list); > + break; > + default: > + break; > + } > + } > + > args->retained = obj->madv != __I915_MADV_PURGED; > > out: > @@ -4248,6 +4262,7 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj, > INIT_LIST_HEAD(&obj->obj_exec_link); > INIT_LIST_HEAD(&obj->vma_list); > INIT_LIST_HEAD(&obj->batch_pool_link); > + INIT_LIST_HEAD(&obj->tmp_link); > > obj->ops = ops; > > @@ -4898,6 +4913,7 @@ i915_gem_load(struct drm_device *dev) > INIT_LIST_HEAD(&dev_priv->context_list); > INIT_LIST_HEAD(&dev_priv->mm.unbound_list); > INIT_LIST_HEAD(&dev_priv->mm.bound_list); > + INIT_LIST_HEAD(&dev_priv->mm.stolen_list); > INIT_LIST_HEAD(&dev_priv->mm.fence_list); > for (i = 0; i < I915_NUM_RINGS; i++) > init_ring_lists(&dev_priv->ring[i]); > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c > index 99f2bce..430e0b0 100644 > --- a/drivers/gpu/drm/i915/i915_gem_stolen.c > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c > @@ -52,8 +52,8 @@ int i915_gem_stolen_insert_node(struct drm_i915_private *dev_priv, > return -ENODEV; > > mutex_lock(&dev_priv->mm.stolen_lock); > - ret = drm_mm_insert_node(&dev_priv->mm.stolen, node, size, alignment, > - DRM_MM_SEARCH_DEFAULT); > + ret = drm_mm_insert_node(&dev_priv->mm.stolen, node, > + size, alignment, DRM_MM_SEARCH_DEFAULT); There is no change here. > mutex_unlock(&dev_priv->mm.stolen_lock); > > return ret; > @@ -407,18 +407,19 @@ static void i915_gem_object_put_pages_stolen(struct drm_i915_gem_object *obj) > kfree(obj->pages); > } > > - > > static void > i915_gem_object_release_stolen(struct drm_i915_gem_object *obj) > { > struct drm_i915_private *dev_priv = obj->base.dev->dev_private; > > if (obj->stolen) { > - i915_gem_stolen_remove_node(dev_priv, obj->stolen); > + list_del(&obj->stolen->mm_link); > + i915_gem_stolen_remove_node(dev_priv, &obj->stolen->base); > kfree(obj->stolen); > obj->stolen = NULL; > } > } > + > static const struct drm_i915_gem_object_ops i915_gem_object_stolen_ops = { > .get_pages = i915_gem_object_get_pages_stolen, > .put_pages = i915_gem_object_put_pages_stolen, > @@ -427,7 +428,7 @@ static const struct drm_i915_gem_object_ops i915_gem_object_stolen_ops = { > > static struct drm_i915_gem_object * > _i915_gem_object_create_stolen(struct drm_device *dev, > - struct drm_mm_node *stolen) > + struct i915_stolen_node *stolen) > { > struct drm_i915_gem_object *obj; > > @@ -435,17 +436,21 @@ _i915_gem_object_create_stolen(struct drm_device *dev, > if (obj == NULL) > return NULL; > > - drm_gem_private_object_init(dev, &obj->base, stolen->size); > + drm_gem_private_object_init(dev, &obj->base, stolen->base.size); > i915_gem_object_init(obj, &i915_gem_object_stolen_ops); > > obj->pages = i915_pages_create_for_stolen(dev, > - stolen->start, stolen->size); > + stolen->base.start, > + stolen->base.size); > if (obj->pages == NULL) > goto cleanup; > > i915_gem_object_pin_pages(obj); > obj->stolen = stolen; > > + stolen->obj = obj; > + INIT_LIST_HEAD(&stolen->mm_link); > + > obj->base.read_domains = I915_GEM_DOMAIN_CPU | I915_GEM_DOMAIN_GTT; > obj->cache_level = HAS_LLC(dev) ? I915_CACHE_LLC : I915_CACHE_NONE; > > @@ -456,36 +461,157 @@ cleanup: > return NULL; > } > > -struct drm_i915_gem_object * > -i915_gem_object_create_stolen(struct drm_device *dev, u64 size) > +static bool mark_free(struct drm_i915_gem_object *obj, struct list_head *unwind) > +{ > + if (obj->stolen == NULL) > + return false; As Chris said, just WARN_ON instead of BUG_ON please. > + if (obj->madv != I915_MADV_DONTNEED) > + return false; > + > + if (obj->pin_display) > + return false; > + > + if (I915_BO_IS_ACTIVE(obj)) > + return false; Chris already spotted this will prevent callers from working as they expect. > + list_add(&obj->tmp_link, unwind); > + return drm_mm_scan_add_block(&obj->stolen->base); > +} > + > +static int > +stolen_evict(struct drm_i915_private *dev_priv, u64 size) > { > - struct drm_i915_private *dev_priv = dev->dev_private; > struct drm_i915_gem_object *obj; > - struct drm_mm_node *stolen; > + struct list_head unwind, evict; > + struct i915_stolen_node *iter; > int ret; > > - if (!drm_mm_initialized(&dev_priv->mm.stolen)) > - return NULL; > + drm_mm_init_scan(&dev_priv->mm.stolen, size, 0, 0); > + INIT_LIST_HEAD(&unwind); > + > + list_for_each_entry(iter, &dev_priv->mm.stolen_list, mm_link) { > + if (I915_BO_IS_ACTIVE(iter->obj)) > + continue; > + > + if (mark_free(iter->obj, &unwind)) > + goto found; > + } > + > + list_for_each_entry(iter, &dev_priv->mm.stolen_list, mm_link) { > + if (!I915_BO_IS_ACTIVE(iter->obj)) > + continue; > + > + if (mark_free(iter->obj, &unwind)) > + goto found; > + } > + > +found: > + INIT_LIST_HEAD(&evict); > + while (!list_empty(&unwind)) { > + obj = list_first_entry(&unwind, > + struct drm_i915_gem_object, > + tmp_link); > + list_del_init(&obj->tmp_link); > + > + if (drm_mm_scan_remove_block(&obj->stolen->base)) { > + list_add(&obj->tmp_link, &evict); > + drm_gem_object_reference(&obj->base); > + } In what circumstances can drm_mm_scan_remove_block fail? > + } > + > + ret = 0; > + while (!list_empty(&evict)) { > + obj = list_first_entry(&evict, > + struct drm_i915_gem_object, > + tmp_link); > + list_del_init(&obj->tmp_link); > + > + if (ret == 0) { > + struct i915_vma *vma, *vma_next; > + > + list_for_each_entry_safe(vma, vma_next, > + &obj->vma_list, > + vma_link) > + if (i915_vma_unbind(vma)) > + break; Because of VMA unbinding stolen creation now again needs struct_mutex. I think putting held assertion somewhere in the call chain is now appropriate. The rest looked good to me. Regards, Tvrtko
On Tue, 2015-09-15 at 16:14 +0100, Tvrtko Ursulin wrote: > On 09/15/2015 09:33 AM, ankitprasad.r.sharma@intel.com wrote: > > From: Chris Wilson <chris at chris-wilson.co.uk> > > > > If we run out of stolen memory when trying to allocate an object, see if > > we can reap enough purgeable objects to free up enough contiguous free > > space for the allocation. This is in principle very much like evicting > > objects to free up enough contiguous space in the vma when binding > > a new object - and you will be forgiven for thinking that the code looks > > very similar. > > > > At the moment, we do not allow userspace to allocate objects in stolen, > > so there is neither the memory pressure to trigger stolen eviction nor > > any purgeable objects inside the stolen arena. However, this will change > > in the near future, and so better management and defragmentation of > > stolen memory will become a real issue. > > > > v2: Remember to remove the drm_mm_node. > > > > v3: Rebased to the latest drm-intel-nightly (Ankit) > > > > v4: corrected if-else braces format (Tvrtko/kerneldoc) > > > > v5: Rebased to the latest drm-intel-nightly (Ankit) > > Added a seperate list to maintain purgable objects from stolen memory > > region (Chris/Daniel) > > > > Testcase: igt/gem_stolen > > > > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk> > > Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com> > > --- > > drivers/gpu/drm/i915/i915_debugfs.c | 4 +- > > drivers/gpu/drm/i915/i915_drv.h | 17 +++- > > drivers/gpu/drm/i915/i915_gem.c | 16 +++ > > drivers/gpu/drm/i915/i915_gem_stolen.c | 176 ++++++++++++++++++++++++++++----- > > drivers/gpu/drm/i915/intel_pm.c | 4 +- > > 5 files changed, 187 insertions(+), 30 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > > index 7a28de5..0db8c47 100644 > > --- a/drivers/gpu/drm/i915/i915_debugfs.c > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > > @@ -179,7 +179,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj) > > seq_puts(m, ")"); > > } > > if (obj->stolen) > > - seq_printf(m, " (stolen: %08llx)", obj->stolen->start); > > + seq_printf(m, " (stolen: %08llx)", obj->stolen->base.start); > > if (obj->pin_display || obj->fault_mappable) { > > char s[3], *t = s; > > if (obj->pin_display) > > @@ -258,7 +258,7 @@ static int obj_rank_by_stolen(void *priv, > > struct drm_i915_gem_object *b = > > container_of(B, struct drm_i915_gem_object, obj_exec_link); > > > > - return a->stolen->start - b->stolen->start; > > + return a->stolen->base.start - b->stolen->base.start; > > } > > > > static int i915_gem_stolen_list_info(struct seq_file *m, void *data) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index e6ef083..37ee32d 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -841,6 +841,12 @@ struct i915_ctx_hang_stats { > > bool banned; > > }; > > > > +struct i915_stolen_node { > > + struct drm_mm_node base; > > + struct list_head mm_link; > > + struct drm_i915_gem_object *obj; > > +}; > > + > > /* This must match up with the value previously used for execbuf2.rsvd1. */ > > #define DEFAULT_CONTEXT_HANDLE 0 > > > > @@ -1268,6 +1274,13 @@ struct i915_gem_mm { > > */ > > struct list_head unbound_list; > > > > + /** > > + * List of stolen objects that have been marked as purgeable and > > + * thus available for reaping if we need more space for a new > > + * allocation. Ordered by time of marking purgeable. > > + */ > > + struct list_head stolen_list; > > + > > /** Usable portion of the GTT for GEM */ > > unsigned long stolen_base; /* limited to low memory (32-bit) */ > > > > @@ -2026,7 +2039,7 @@ struct drm_i915_gem_object { > > struct list_head vma_list; > > > > /** Stolen memory for this object, instead of being backed by shmem. */ > > - struct drm_mm_node *stolen; > > + struct i915_stolen_node *stolen; > > struct list_head global_list; > > > > struct list_head ring_list[I915_NUM_RINGS]; > > @@ -2034,6 +2047,7 @@ struct drm_i915_gem_object { > > struct list_head obj_exec_link; > > > > struct list_head batch_pool_link; > > + struct list_head tmp_link; > > > > /** > > * This is set if the object is on the active lists (has pending > > @@ -2150,6 +2164,7 @@ struct drm_i915_gem_object { > > }; > > }; > > #define to_intel_bo(x) container_of(x, struct drm_i915_gem_object, base) > > +#define I915_BO_IS_ACTIVE(__obj) (__obj->active) > > > > void i915_gem_track_fb(struct drm_i915_gem_object *old, > > struct drm_i915_gem_object *new, > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > index 6568a7f..85025b1 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -4228,6 +4228,20 @@ i915_gem_madvise_ioctl(struct drm_device *dev, void *data, > > if (obj->madv == I915_MADV_DONTNEED && obj->pages == NULL) > > i915_gem_object_truncate(obj); > > > > + if (obj->stolen) { > > + switch (obj->madv) { > > + case I915_MADV_WILLNEED: > > + list_del_init(&obj->stolen->mm_link); > > + break; > > + case I915_MADV_DONTNEED: > > + list_move(&obj->stolen->mm_link, > > + &dev_priv->mm.stolen_list); > > + break; > > + default: > > + break; > > + } > > + } > > + > > args->retained = obj->madv != __I915_MADV_PURGED; > > > > out: > > @@ -4248,6 +4262,7 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj, > > INIT_LIST_HEAD(&obj->obj_exec_link); > > INIT_LIST_HEAD(&obj->vma_list); > > INIT_LIST_HEAD(&obj->batch_pool_link); > > + INIT_LIST_HEAD(&obj->tmp_link); > > > > obj->ops = ops; > > > > @@ -4898,6 +4913,7 @@ i915_gem_load(struct drm_device *dev) > > INIT_LIST_HEAD(&dev_priv->context_list); > > INIT_LIST_HEAD(&dev_priv->mm.unbound_list); > > INIT_LIST_HEAD(&dev_priv->mm.bound_list); > > + INIT_LIST_HEAD(&dev_priv->mm.stolen_list); > > INIT_LIST_HEAD(&dev_priv->mm.fence_list); > > for (i = 0; i < I915_NUM_RINGS; i++) > > init_ring_lists(&dev_priv->ring[i]); > > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c > > index 99f2bce..430e0b0 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_stolen.c > > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c > > @@ -52,8 +52,8 @@ int i915_gem_stolen_insert_node(struct drm_i915_private *dev_priv, > > return -ENODEV; > > > > mutex_lock(&dev_priv->mm.stolen_lock); > > - ret = drm_mm_insert_node(&dev_priv->mm.stolen, node, size, alignment, > > - DRM_MM_SEARCH_DEFAULT); > > + ret = drm_mm_insert_node(&dev_priv->mm.stolen, node, > > + size, alignment, DRM_MM_SEARCH_DEFAULT); > > There is no change here. > > > mutex_unlock(&dev_priv->mm.stolen_lock); > > > > return ret; > > @@ -407,18 +407,19 @@ static void i915_gem_object_put_pages_stolen(struct drm_i915_gem_object *obj) > > kfree(obj->pages); > > } > > > > - > > > > static void > > i915_gem_object_release_stolen(struct drm_i915_gem_object *obj) > > { > > struct drm_i915_private *dev_priv = obj->base.dev->dev_private; > > > > if (obj->stolen) { > > - i915_gem_stolen_remove_node(dev_priv, obj->stolen); > > + list_del(&obj->stolen->mm_link); > > + i915_gem_stolen_remove_node(dev_priv, &obj->stolen->base); > > kfree(obj->stolen); > > obj->stolen = NULL; > > } > > } > > + > > static const struct drm_i915_gem_object_ops i915_gem_object_stolen_ops = { > > .get_pages = i915_gem_object_get_pages_stolen, > > .put_pages = i915_gem_object_put_pages_stolen, > > @@ -427,7 +428,7 @@ static const struct drm_i915_gem_object_ops i915_gem_object_stolen_ops = { > > > > static struct drm_i915_gem_object * > > _i915_gem_object_create_stolen(struct drm_device *dev, > > - struct drm_mm_node *stolen) > > + struct i915_stolen_node *stolen) > > { > > struct drm_i915_gem_object *obj; > > > > @@ -435,17 +436,21 @@ _i915_gem_object_create_stolen(struct drm_device *dev, > > if (obj == NULL) > > return NULL; > > > > - drm_gem_private_object_init(dev, &obj->base, stolen->size); > > + drm_gem_private_object_init(dev, &obj->base, stolen->base.size); > > i915_gem_object_init(obj, &i915_gem_object_stolen_ops); > > > > obj->pages = i915_pages_create_for_stolen(dev, > > - stolen->start, stolen->size); > > + stolen->base.start, > > + stolen->base.size); > > if (obj->pages == NULL) > > goto cleanup; > > > > i915_gem_object_pin_pages(obj); > > obj->stolen = stolen; > > > > + stolen->obj = obj; > > + INIT_LIST_HEAD(&stolen->mm_link); > > + > > obj->base.read_domains = I915_GEM_DOMAIN_CPU | I915_GEM_DOMAIN_GTT; > > obj->cache_level = HAS_LLC(dev) ? I915_CACHE_LLC : I915_CACHE_NONE; > > > > @@ -456,36 +461,157 @@ cleanup: > > return NULL; > > } > > > > -struct drm_i915_gem_object * > > -i915_gem_object_create_stolen(struct drm_device *dev, u64 size) > > +static bool mark_free(struct drm_i915_gem_object *obj, struct list_head *unwind) > > +{ > > + if (obj->stolen == NULL) > > + return false; > > As Chris said, just WARN_ON instead of BUG_ON please. > > > + if (obj->madv != I915_MADV_DONTNEED) > > + return false; > > + > > + if (obj->pin_display) > > + return false; > > + > > + if (I915_BO_IS_ACTIVE(obj)) > > + return false; > > Chris already spotted this will prevent callers from working as they expect. > > > + list_add(&obj->tmp_link, unwind); > > + return drm_mm_scan_add_block(&obj->stolen->base); > > +} > > + > > +static int > > +stolen_evict(struct drm_i915_private *dev_priv, u64 size) > > { > > - struct drm_i915_private *dev_priv = dev->dev_private; > > struct drm_i915_gem_object *obj; > > - struct drm_mm_node *stolen; > > + struct list_head unwind, evict; > > + struct i915_stolen_node *iter; > > int ret; > > > > - if (!drm_mm_initialized(&dev_priv->mm.stolen)) > > - return NULL; > > + drm_mm_init_scan(&dev_priv->mm.stolen, size, 0, 0); > > + INIT_LIST_HEAD(&unwind); > > + > > + list_for_each_entry(iter, &dev_priv->mm.stolen_list, mm_link) { > > + if (I915_BO_IS_ACTIVE(iter->obj)) > > + continue; > > + > > + if (mark_free(iter->obj, &unwind)) > > + goto found; > > + } > > + > > + list_for_each_entry(iter, &dev_priv->mm.stolen_list, mm_link) { > > + if (!I915_BO_IS_ACTIVE(iter->obj)) > > + continue; > > + > > + if (mark_free(iter->obj, &unwind)) > > + goto found; > > + } > > + > > +found: > > + INIT_LIST_HEAD(&evict); > > + while (!list_empty(&unwind)) { > > + obj = list_first_entry(&unwind, > > + struct drm_i915_gem_object, > > + tmp_link); > > + list_del_init(&obj->tmp_link); > > + > > + if (drm_mm_scan_remove_block(&obj->stolen->base)) { > > + list_add(&obj->tmp_link, &evict); > > + drm_gem_object_reference(&obj->base); > > + } > > In what circumstances can drm_mm_scan_remove_block fail? It works some thing like this: If there are 10 purgable nodes in the unwind list and 4 of them are positioned in a way to reap enough contiguous space for the new object (not necessarily purging all nodes will give us the amount of space we need), then for the remaining 6 nodes drm_mm_scan_remove_block will fail, while the rest will be removed to make space for the new object. Thanks, Ankit
On Wed, Sep 16, 2015 at 02:31:38PM +0530, Ankitprasad Sharma wrote: > On Tue, 2015-09-15 at 16:14 +0100, Tvrtko Ursulin wrote: > > On 09/15/2015 09:33 AM, ankitprasad.r.sharma@intel.com wrote: > > In what circumstances can drm_mm_scan_remove_block fail? > It works some thing like this: > If there are 10 purgable nodes in the unwind list and 4 of them are > positioned in a way to reap enough contiguous space for the new object > (not necessarily purging all nodes will give us the amount of space we > need), then for the remaining 6 nodes drm_mm_scan_remove_block will > fail, while the rest will be removed to make space for the new object. Quoting the nice kerneldoc we have: * Returns: * True if this block should be evicted, false otherwise. Will always * return false when no hole has been found. Was that not clear enough or did you simply not bother to read the docs? If it's not clear (together with the obligatory DOC: overview section) we need to improve them ... Thanks, Daniel
On 09/23/2015 10:28 AM, Daniel Vetter wrote: > On Wed, Sep 16, 2015 at 02:31:38PM +0530, Ankitprasad Sharma wrote: >> On Tue, 2015-09-15 at 16:14 +0100, Tvrtko Ursulin wrote: >>> On 09/15/2015 09:33 AM, ankitprasad.r.sharma@intel.com wrote: >>> In what circumstances can drm_mm_scan_remove_block fail? >> It works some thing like this: >> If there are 10 purgable nodes in the unwind list and 4 of them are >> positioned in a way to reap enough contiguous space for the new object >> (not necessarily purging all nodes will give us the amount of space we >> need), then for the remaining 6 nodes drm_mm_scan_remove_block will >> fail, while the rest will be removed to make space for the new object. > > Quoting the nice kerneldoc we have: > > * Returns: > * True if this block should be evicted, false otherwise. Will always > * return false when no hole has been found. > > Was that not clear enough or did you simply not bother to read the docs? > If it's not clear (together with the obligatory DOC: overview section) we > need to improve them ... Not sure to whom you are addressing this? I did not read the docs, after years of no docs the notion that there aren't any is kind of hard embedded in me at least. :) Ankit's explanation is also more detailed, answers the interesting question, again for me at least. Regards, Tvrtko
On Wed, Sep 23, 2015 at 10:30:51AM +0100, Tvrtko Ursulin wrote: > > On 09/23/2015 10:28 AM, Daniel Vetter wrote: > >On Wed, Sep 16, 2015 at 02:31:38PM +0530, Ankitprasad Sharma wrote: > >>On Tue, 2015-09-15 at 16:14 +0100, Tvrtko Ursulin wrote: > >>>On 09/15/2015 09:33 AM, ankitprasad.r.sharma@intel.com wrote: > >>>In what circumstances can drm_mm_scan_remove_block fail? > >>It works some thing like this: > >>If there are 10 purgable nodes in the unwind list and 4 of them are > >>positioned in a way to reap enough contiguous space for the new object > >>(not necessarily purging all nodes will give us the amount of space we > >>need), then for the remaining 6 nodes drm_mm_scan_remove_block will > >>fail, while the rest will be removed to make space for the new object. > > > >Quoting the nice kerneldoc we have: > > > > * Returns: > > * True if this block should be evicted, false otherwise. Will always > > * return false when no hole has been found. > > > >Was that not clear enough or did you simply not bother to read the docs? > >If it's not clear (together with the obligatory DOC: overview section) we > >need to improve them ... > > Not sure to whom you are addressing this? I did not read the docs, after > years of no docs the notion that there aren't any is kind of hard embedded > in me at least. :) > > Ankit's explanation is also more detailed, answers the interesting question, > again for me at least. Question was to you, and looks like the answer is "didn't read the docs". They do explain (in more detail) pretty much everything what Ankitprasad said, just wanted to make sure we are covered there. Which reminds me: Do we have kerneldoc for stolen (and this new feature here) too? Ankitprasad? -Daniel
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 7a28de5..0db8c47 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -179,7 +179,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj) seq_puts(m, ")"); } if (obj->stolen) - seq_printf(m, " (stolen: %08llx)", obj->stolen->start); + seq_printf(m, " (stolen: %08llx)", obj->stolen->base.start); if (obj->pin_display || obj->fault_mappable) { char s[3], *t = s; if (obj->pin_display) @@ -258,7 +258,7 @@ static int obj_rank_by_stolen(void *priv, struct drm_i915_gem_object *b = container_of(B, struct drm_i915_gem_object, obj_exec_link); - return a->stolen->start - b->stolen->start; + return a->stolen->base.start - b->stolen->base.start; } static int i915_gem_stolen_list_info(struct seq_file *m, void *data) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index e6ef083..37ee32d 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -841,6 +841,12 @@ struct i915_ctx_hang_stats { bool banned; }; +struct i915_stolen_node { + struct drm_mm_node base; + struct list_head mm_link; + struct drm_i915_gem_object *obj; +}; + /* This must match up with the value previously used for execbuf2.rsvd1. */ #define DEFAULT_CONTEXT_HANDLE 0 @@ -1268,6 +1274,13 @@ struct i915_gem_mm { */ struct list_head unbound_list; + /** + * List of stolen objects that have been marked as purgeable and + * thus available for reaping if we need more space for a new + * allocation. Ordered by time of marking purgeable. + */ + struct list_head stolen_list; + /** Usable portion of the GTT for GEM */ unsigned long stolen_base; /* limited to low memory (32-bit) */ @@ -2026,7 +2039,7 @@ struct drm_i915_gem_object { struct list_head vma_list; /** Stolen memory for this object, instead of being backed by shmem. */ - struct drm_mm_node *stolen; + struct i915_stolen_node *stolen; struct list_head global_list; struct list_head ring_list[I915_NUM_RINGS]; @@ -2034,6 +2047,7 @@ struct drm_i915_gem_object { struct list_head obj_exec_link; struct list_head batch_pool_link; + struct list_head tmp_link; /** * This is set if the object is on the active lists (has pending @@ -2150,6 +2164,7 @@ struct drm_i915_gem_object { }; }; #define to_intel_bo(x) container_of(x, struct drm_i915_gem_object, base) +#define I915_BO_IS_ACTIVE(__obj) (__obj->active) void i915_gem_track_fb(struct drm_i915_gem_object *old, struct drm_i915_gem_object *new, diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 6568a7f..85025b1 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -4228,6 +4228,20 @@ i915_gem_madvise_ioctl(struct drm_device *dev, void *data, if (obj->madv == I915_MADV_DONTNEED && obj->pages == NULL) i915_gem_object_truncate(obj); + if (obj->stolen) { + switch (obj->madv) { + case I915_MADV_WILLNEED: + list_del_init(&obj->stolen->mm_link); + break; + case I915_MADV_DONTNEED: + list_move(&obj->stolen->mm_link, + &dev_priv->mm.stolen_list); + break; + default: + break; + } + } + args->retained = obj->madv != __I915_MADV_PURGED; out: @@ -4248,6 +4262,7 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj, INIT_LIST_HEAD(&obj->obj_exec_link); INIT_LIST_HEAD(&obj->vma_list); INIT_LIST_HEAD(&obj->batch_pool_link); + INIT_LIST_HEAD(&obj->tmp_link); obj->ops = ops; @@ -4898,6 +4913,7 @@ i915_gem_load(struct drm_device *dev) INIT_LIST_HEAD(&dev_priv->context_list); INIT_LIST_HEAD(&dev_priv->mm.unbound_list); INIT_LIST_HEAD(&dev_priv->mm.bound_list); + INIT_LIST_HEAD(&dev_priv->mm.stolen_list); INIT_LIST_HEAD(&dev_priv->mm.fence_list); for (i = 0; i < I915_NUM_RINGS; i++) init_ring_lists(&dev_priv->ring[i]); diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c index 99f2bce..430e0b0 100644 --- a/drivers/gpu/drm/i915/i915_gem_stolen.c +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c @@ -52,8 +52,8 @@ int i915_gem_stolen_insert_node(struct drm_i915_private *dev_priv, return -ENODEV; mutex_lock(&dev_priv->mm.stolen_lock); - ret = drm_mm_insert_node(&dev_priv->mm.stolen, node, size, alignment, - DRM_MM_SEARCH_DEFAULT); + ret = drm_mm_insert_node(&dev_priv->mm.stolen, node, + size, alignment, DRM_MM_SEARCH_DEFAULT); mutex_unlock(&dev_priv->mm.stolen_lock); return ret; @@ -407,18 +407,19 @@ static void i915_gem_object_put_pages_stolen(struct drm_i915_gem_object *obj) kfree(obj->pages); } - static void i915_gem_object_release_stolen(struct drm_i915_gem_object *obj) { struct drm_i915_private *dev_priv = obj->base.dev->dev_private; if (obj->stolen) { - i915_gem_stolen_remove_node(dev_priv, obj->stolen); + list_del(&obj->stolen->mm_link); + i915_gem_stolen_remove_node(dev_priv, &obj->stolen->base); kfree(obj->stolen); obj->stolen = NULL; } } + static const struct drm_i915_gem_object_ops i915_gem_object_stolen_ops = { .get_pages = i915_gem_object_get_pages_stolen, .put_pages = i915_gem_object_put_pages_stolen, @@ -427,7 +428,7 @@ static const struct drm_i915_gem_object_ops i915_gem_object_stolen_ops = { static struct drm_i915_gem_object * _i915_gem_object_create_stolen(struct drm_device *dev, - struct drm_mm_node *stolen) + struct i915_stolen_node *stolen) { struct drm_i915_gem_object *obj; @@ -435,17 +436,21 @@ _i915_gem_object_create_stolen(struct drm_device *dev, if (obj == NULL) return NULL; - drm_gem_private_object_init(dev, &obj->base, stolen->size); + drm_gem_private_object_init(dev, &obj->base, stolen->base.size); i915_gem_object_init(obj, &i915_gem_object_stolen_ops); obj->pages = i915_pages_create_for_stolen(dev, - stolen->start, stolen->size); + stolen->base.start, + stolen->base.size); if (obj->pages == NULL) goto cleanup; i915_gem_object_pin_pages(obj); obj->stolen = stolen; + stolen->obj = obj; + INIT_LIST_HEAD(&stolen->mm_link); + obj->base.read_domains = I915_GEM_DOMAIN_CPU | I915_GEM_DOMAIN_GTT; obj->cache_level = HAS_LLC(dev) ? I915_CACHE_LLC : I915_CACHE_NONE; @@ -456,36 +461,157 @@ cleanup: return NULL; } -struct drm_i915_gem_object * -i915_gem_object_create_stolen(struct drm_device *dev, u64 size) +static bool mark_free(struct drm_i915_gem_object *obj, struct list_head *unwind) +{ + if (obj->stolen == NULL) + return false; + + if (obj->madv != I915_MADV_DONTNEED) + return false; + + if (obj->pin_display) + return false; + + if (I915_BO_IS_ACTIVE(obj)) + return false; + + list_add(&obj->tmp_link, unwind); + return drm_mm_scan_add_block(&obj->stolen->base); +} + +static int +stolen_evict(struct drm_i915_private *dev_priv, u64 size) { - struct drm_i915_private *dev_priv = dev->dev_private; struct drm_i915_gem_object *obj; - struct drm_mm_node *stolen; + struct list_head unwind, evict; + struct i915_stolen_node *iter; int ret; - if (!drm_mm_initialized(&dev_priv->mm.stolen)) - return NULL; + drm_mm_init_scan(&dev_priv->mm.stolen, size, 0, 0); + INIT_LIST_HEAD(&unwind); + + list_for_each_entry(iter, &dev_priv->mm.stolen_list, mm_link) { + if (I915_BO_IS_ACTIVE(iter->obj)) + continue; + + if (mark_free(iter->obj, &unwind)) + goto found; + } + + list_for_each_entry(iter, &dev_priv->mm.stolen_list, mm_link) { + if (!I915_BO_IS_ACTIVE(iter->obj)) + continue; + + if (mark_free(iter->obj, &unwind)) + goto found; + } + +found: + INIT_LIST_HEAD(&evict); + while (!list_empty(&unwind)) { + obj = list_first_entry(&unwind, + struct drm_i915_gem_object, + tmp_link); + list_del_init(&obj->tmp_link); + + if (drm_mm_scan_remove_block(&obj->stolen->base)) { + list_add(&obj->tmp_link, &evict); + drm_gem_object_reference(&obj->base); + } + } + + ret = 0; + while (!list_empty(&evict)) { + obj = list_first_entry(&evict, + struct drm_i915_gem_object, + tmp_link); + list_del_init(&obj->tmp_link); + + if (ret == 0) { + struct i915_vma *vma, *vma_next; + + list_for_each_entry_safe(vma, vma_next, + &obj->vma_list, + vma_link) + if (i915_vma_unbind(vma)) + break; + + /* Stolen pins its pages to prevent the + * normal shrinker from processing stolen + * objects. + */ + i915_gem_object_unpin_pages(obj); + + ret = i915_gem_object_put_pages(obj); + if (ret == 0) { + i915_gem_object_release_stolen(obj); + obj->madv = __I915_MADV_PURGED; + } else { + i915_gem_object_pin_pages(obj); + } + } + + drm_gem_object_unreference(&obj->base); + } + + return ret; +} + +static struct i915_stolen_node * +stolen_alloc(struct drm_i915_private *dev_priv, u64 size) +{ + struct i915_stolen_node *stolen; + int ret; - DRM_DEBUG_KMS("creating stolen object: size=%llx\n", size); if (size == 0) - return NULL; + return ERR_PTR(-EINVAL); stolen = kzalloc(sizeof(*stolen), GFP_KERNEL); if (!stolen) - return NULL; + return ERR_PTR(-ENOMEM); - ret = i915_gem_stolen_insert_node(dev_priv, stolen, size, 4096); + ret = i915_gem_stolen_insert_node(dev_priv, &stolen->base, size, 4096); + if (ret == 0) + goto out; + + /* No more stolen memory available, or too fragmented. + * Try evicting purgeable objects and search again. + */ + ret = stolen_evict(dev_priv, size); + + if (ret == 0) + ret = i915_gem_stolen_insert_node(dev_priv, &stolen->base, + size, 0); +out: if (ret) { kfree(stolen); - return NULL; + stolen = ERR_PTR(ret); } + return stolen; +} + +struct drm_i915_gem_object * +i915_gem_object_create_stolen(struct drm_device *dev, u64 size) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + struct drm_i915_gem_object *obj; + struct i915_stolen_node *stolen; + + if (!drm_mm_initialized(&dev_priv->mm.stolen)) + return NULL; + + DRM_DEBUG_KMS("creating stolen object: size=%llx\n", size); + + stolen = stolen_alloc(dev_priv, size); + if (IS_ERR(stolen)) + return NULL; + obj = _i915_gem_object_create_stolen(dev, stolen); if (obj) return obj; - i915_gem_stolen_remove_node(dev_priv, stolen); + i915_gem_stolen_remove_node(dev_priv, &stolen->base); kfree(stolen); return NULL; } @@ -499,7 +625,7 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev, struct drm_i915_private *dev_priv = dev->dev_private; struct i915_address_space *ggtt = &dev_priv->gtt.base; struct drm_i915_gem_object *obj; - struct drm_mm_node *stolen; + struct i915_stolen_node *stolen; struct i915_vma *vma; int ret; @@ -518,10 +644,10 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev, if (!stolen) return NULL; - stolen->start = stolen_offset; - stolen->size = size; + stolen->base.start = stolen_offset; + stolen->base.size = size; mutex_lock(&dev_priv->mm.stolen_lock); - ret = drm_mm_reserve_node(&dev_priv->mm.stolen, stolen); + ret = drm_mm_reserve_node(&dev_priv->mm.stolen, &stolen->base); mutex_unlock(&dev_priv->mm.stolen_lock); if (ret) { DRM_DEBUG_KMS("failed to allocate stolen space\n"); @@ -532,7 +658,7 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev, obj = _i915_gem_object_create_stolen(dev, stolen); if (obj == NULL) { DRM_DEBUG_KMS("failed to allocate stolen object\n"); - i915_gem_stolen_remove_node(dev_priv, stolen); + i915_gem_stolen_remove_node(dev_priv, &stolen->base); kfree(stolen); return NULL; } @@ -573,7 +699,7 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev, err_vma: i915_gem_vma_destroy(vma); err_out: - i915_gem_stolen_remove_node(dev_priv, stolen); + i915_gem_stolen_remove_node(dev_priv, &stolen->base); kfree(stolen); drm_gem_object_unreference(&obj->base); return NULL; diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index fff0c22..51f2f91 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -5235,7 +5235,7 @@ static void valleyview_check_pctx(struct drm_i915_private *dev_priv) unsigned long pctx_addr = I915_READ(VLV_PCBR) & ~4095; WARN_ON(pctx_addr != dev_priv->mm.stolen_base + - dev_priv->vlv_pctx->stolen->start); + dev_priv->vlv_pctx->stolen->base.start); } @@ -5309,7 +5309,7 @@ static void valleyview_setup_pctx(struct drm_device *dev) return; } - pctx_paddr = dev_priv->mm.stolen_base + pctx->stolen->start; + pctx_paddr = dev_priv->mm.stolen_base + pctx->stolen->base.start; I915_WRITE(VLV_PCBR, pctx_paddr); out: