[Intel-gfx] [PATCH 4/6] drm/i915: Add support for stealing purgable stolen pages
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Fri Nov 13 08:35:54 PST 2015
Hi,
On 11/11/15 10:36, ankitprasad.r.sharma at 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)
>
> v6: Compiler optimization (merging 2 single loops into one for() loop),
> corrected code for object eviction, retire_requests before starting
> object eviction (Chris)
>
> v7: Added kernel doc for i915_gem_object_create_stolen()
>
> v8: Check for struct_mutex lock before creating object from stolen
> region (Tvrtko)
>
> Testcase: igt/gem_stolen
>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma at intel.com>
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 6 +-
> drivers/gpu/drm/i915/i915_drv.h | 17 +++-
> drivers/gpu/drm/i915/i915_gem.c | 16 ++++
> drivers/gpu/drm/i915/i915_gem_stolen.c | 170 +++++++++++++++++++++++++++++----
> drivers/gpu/drm/i915/intel_pm.c | 4 +-
> 5 files changed, 188 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 5659d4c..89b0fec 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -174,7 +174,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)
> @@ -253,9 +253,9 @@ 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);
>
> - if (a->stolen->start < b->stolen->start)
> + if (a->stolen->base.start < b->stolen->base.start)
> return -1;
> - if (a->stolen->start > b->stolen->start)
> + if (a->stolen->base.start > b->stolen->base.start)
> return 1;
> return 0;
> }
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 9c731f6..2c75c32 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
>
> @@ -1252,6 +1258,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) */
>
> @@ -2032,7 +2045,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];
> @@ -2040,6 +2053,7 @@ struct drm_i915_gem_object {
> struct list_head obj_exec_link;
>
> struct list_head batch_pool_link;
> + struct list_head tmp_link;
Could you put a comment describing if it is safe or not for potential
other users to use tmp_link, or with what considerations?
Or alternatively use a dedicated name so it will be obvious it is for
stolen list only?
> /**
> * This is set if the object is on the active lists (has pending
> @@ -2156,6 +2170,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)
Do you need this macro, it is used only once and it is maybe too trivial?
> 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 57f02aa..2d8c9e0 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4359,6 +4359,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:
> @@ -4379,6 +4393,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;
>
> @@ -4996,6 +5011,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 0b0ce11..b9410a4 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -542,7 +542,8 @@ 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;
> }
> @@ -555,7 +556,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;
> int ret = 0;
> @@ -564,11 +565,12 @@ _i915_gem_object_create_stolen(struct drm_device *dev,
> if (obj == NULL)
> return ERR_PTR(-ENOMEM);
>
> - 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 (IS_ERR(obj->pages)) {
> ret = PTR_ERR(obj->pages);
> goto cleanup;
> @@ -577,6 +579,9 @@ _i915_gem_object_create_stolen(struct drm_device *dev,
> 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;
>
> @@ -587,18 +592,102 @@ cleanup:
> return ERR_PTR(ret);
> }
>
> -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)
> +{
> + BUG_ON(obj->stolen == NULL);
:(
> +
> + if (obj->madv != I915_MADV_DONTNEED)
> + return false;
> +
> + if (obj->pin_display)
> + 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;
> - int ret;
> + struct list_head unwind, evict;
> + struct i915_stolen_node *iter;
> + int ret, active;
>
> - if (!drm_mm_initialized(&dev_priv->mm.stolen))
> - return ERR_PTR(-ENODEV);
> + drm_mm_init_scan(&dev_priv->mm.stolen, size, 0, 0);
> + INIT_LIST_HEAD(&unwind);
> +
> + /* Retire all requests before creating the evict list */
> + i915_gem_retire_requests(dev_priv->dev);
> +
> + for (active = 0; active <= 1; active++) {
> + list_for_each_entry(iter, &dev_priv->mm.stolen_list, mm_link) {
> + if (I915_BO_IS_ACTIVE(iter->obj) != active)
> + 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(&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(&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 ERR_PTR(-EINVAL);
>
> @@ -606,17 +695,60 @@ i915_gem_object_create_stolen(struct drm_device *dev, u64 size)
> if (!stolen)
> 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);
I am curious about the second attempt asking for no alignment.
Everything is always at least 4k aligned right? So if you passed in 4k
in the 2nd attempt it would either work, or it wouldn't work with zero
alignment either, no?
Regards,
Tvrtko
More information about the Intel-gfx
mailing list