Re: [PATCH 3/4] drm/i915: Add support for stealing purgable stolen pages

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, 2015-09-15 at 16:14 +0100, Tvrtko Ursulin wrote:
> On 09/15/2015 09:33 AM, ankitprasad.r.sharma@xxxxxxxxx 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@xxxxxxxxx>
> > ---
> >   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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux