[PATCH v5 0/4] Support for creating/using Stolen memory backed objects

[PATCH v5 0/4] Support for creating/using Stolen memory backed objects

[ankitprasad.r.sharma]

From: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>

This patch series adds support for creating/using Stolen memory backed
objects.

Despite being a unified memory architecture (UMA) some bits of memory
are more equal than others. In particular we have the thorny issue of
stolen memory, memory stolen from the system by the BIOS and reserved
for igfx use. Stolen memory is required for some functions of the GPU
and display engine, but in general it goes wasted. Whilst we cannot
return it back to the system, we need to find some other method for
utilising it. As we do not support direct access to the physical address
in the stolen region, it behaves like a different class of memory,
closer in kin to local GPU memory. This strongly suggests that we need a
placement model like TTM if we are to fully utilize these discrete
chunks of differing memory.

To add support for creating Stolen memory backed objects, we extend the
drm_i915_gem_create structure, by adding a new flag through which user
can specify the preference to allocate the object from stolen memory,
which if set, an attempt will be made to allocate the object from stolen
memory subject to the availability of free space in the stolen region.

This patch series adds support for clearing buffer objects via CPU/GTT.
This is particularly useful for clearing out the memory from stolen
region, but can also be used for other shmem allocated objects. Currently
being used for buffers allocated in the stolen region. Also adding support
for stealing purgable stolen pages, if we run out of stolen memory when
trying to allocate an object.

v2: Added support for read/write from/to objects not backed by
shmem using the pread/pwrite interface.
Also extended the current get_aperture ioctl to retrieve the
total and available size of the stolen region

v3: Removed the extended get_aperture ioctl patch 5 (to be submitted as
part of other patch series), addressed comments by Chris about pread/pwrite
for non shmem backed objects

v4: Rebased to the latest drm-intel-nightly

v5: Addressed comments, replaced patch 1/4 "Clearing buffers via blitter
engine" by "Clearing buffers via CPU/GTT"

This can be verified using IGT tests: igt/gem_stolen, igt/gem_create

Ankitprasad Sharma (3):
  drm/i915: Clearing buffer objects via CPU/GTT
  drm/i915: Support for creating Stolen memory backed objects
  drm/i915: Support for pread/pwrite from/to non shmem backed objects

Chris Wilson (1):
  drm/i915: Add support for stealing purgable stolen pages

 drivers/gpu/drm/i915/i915_dma.c        |   3 +
 drivers/gpu/drm/i915/i915_drv.h        |   1 +
 drivers/gpu/drm/i915/i915_gem.c        | 205 +++++++++++++++++++++++++++++----
 drivers/gpu/drm/i915/i915_gem_stolen.c | 122 ++++++++++++++++++--
 include/uapi/drm/i915_drm.h            |  15 +++
 5 files changed, 314 insertions(+), 32 deletions(-)

-- 
1.9.1

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

[PATCH 1/4] drm/i915: Clearing buffer objects via CPU/GTT

[ankitprasad.r.sharma]

From: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>

This patch adds support for clearing buffer objects via CPU/GTT. This
is particularly useful for clearing out the non shmem backed objects.
Currently intend to use this only for buffers allocated from stolen
region.

Testcase: igt/gem_stolen

Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |  1 +
 drivers/gpu/drm/i915/i915_gem.c | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ea9caf2..f6af9c0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2764,6 +2764,7 @@ int i915_gem_obj_prepare_shmem_read(struct drm_i915_gem_object *obj,
 				    int *needs_clflush);
 
 int __must_check i915_gem_object_get_pages(struct drm_i915_gem_object *obj);
+int i915_gem_clear_object(struct drm_i915_gem_object *obj);
 
 static inline int __sg_page_count(struct scatterlist *sg)
 {
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index a2a4a27..fc434ae 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5469,3 +5469,37 @@ bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj)
 	return false;
 }
 
+int i915_gem_clear_object(struct drm_i915_gem_object *obj)
+{
+	int ret = 0;
+	char __iomem *base;
+	int size = obj->base.size;
+	struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
+	unsigned alignment = 0;
+
+	ret = i915_gem_obj_ggtt_pin(obj, alignment, PIN_MAPPABLE);
+	if (ret) {
+		DRM_ERROR("Mapping of gem object to GTT failed\n");
+		return ret;
+	}
+
+	ret = i915_gem_object_put_fence(obj);
+	if (ret)
+		goto unpin;
+
+	/* Get the CPU virtual address of the buffer */
+	base = ioremap_wc(dev_priv->gtt.mappable_base +
+			  i915_gem_obj_ggtt_offset(obj), size);
+	if (base == NULL) {
+		DRM_ERROR("Mapping of gem object to CPU failed\n");
+		ret = -ENOSPC;
+		goto unpin;
+	}
+
+	memset_io(base, 0, size);
+
+	iounmap(base);
+unpin:
+	i915_gem_object_ggtt_unpin(obj);
+	return ret;
+}
-- 
1.9.1

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

[PATCH 2/4] drm/i915: Support for creating Stolen memory backed objects

[ankitprasad.r.sharma]

From: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>

Extend the drm_i915_gem_create structure to add support for
creating Stolen memory backed objects. Added a new flag through
which user can specify the preference to allocate the object from
stolen memory, which if set, an attempt will be made to allocate
the object from stolen memory subject to the availability of
free space in the stolen region.

v2: Rebased to the latest drm-intel-nightly (Ankit)

v3: Changed versioning of GEM_CREATE param, added new comments (Tvrtko)

Testcase: igt/gem_stolen

Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c |  3 +++
 drivers/gpu/drm/i915/i915_gem.c | 33 +++++++++++++++++++++++++++++----
 include/uapi/drm/i915_drm.h     | 15 +++++++++++++++
 3 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index c5349fa..bfb07ab 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -167,6 +167,9 @@ static int i915_getparam(struct drm_device *dev, void *data,
 		value = i915.enable_hangcheck &&
 			intel_has_gpu_reset(dev);
 		break;
+	case I915_PARAM_CREATE_VERSION:
+		value = 2;
+		break;
 	default:
 		DRM_DEBUG("Unknown parameter %d\n", param->param);
 		return -EINVAL;
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index fc434ae..9e7e182 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -391,7 +391,8 @@ static int
 i915_gem_create(struct drm_file *file,
 		struct drm_device *dev,
 		uint64_t size,
-		uint32_t *handle_p)
+		uint32_t *handle_p,
+		uint32_t flags)
 {
 	struct drm_i915_gem_object *obj;
 	int ret;
@@ -401,8 +402,31 @@ i915_gem_create(struct drm_file *file,
 	if (size == 0)
 		return -EINVAL;
 
+	if (flags & ~(I915_CREATE_PLACEMENT_STOLEN))
+		return -EINVAL;
+
 	/* Allocate the new object */
-	obj = i915_gem_alloc_object(dev, size);
+	if (flags & I915_CREATE_PLACEMENT_STOLEN) {
+		mutex_lock(&dev->struct_mutex);
+		obj = i915_gem_object_create_stolen(dev, size);
+		if (!obj) {
+			mutex_unlock(&dev->struct_mutex);
+			return -ENOMEM;
+		}
+
+		/* Always clear fresh buffers before handing to userspace */
+		ret = i915_gem_clear_object(obj);
+		if (ret) {
+			drm_gem_object_unreference(&obj->base);
+			mutex_unlock(&dev->struct_mutex);
+			return ret;
+		}
+
+		mutex_unlock(&dev->struct_mutex);
+	} else {
+		obj = i915_gem_alloc_object(dev, size);
+	}
+
 	if (obj == NULL)
 		return -ENOMEM;
 
@@ -425,7 +449,7 @@ i915_gem_dumb_create(struct drm_file *file,
 	args->pitch = ALIGN(args->width * DIV_ROUND_UP(args->bpp, 8), 64);
 	args->size = args->pitch * args->height;
 	return i915_gem_create(file, dev,
-			       args->size, &args->handle);
+			       args->size, &args->handle, 0);
 }
 
 /**
@@ -438,7 +462,8 @@ i915_gem_create_ioctl(struct drm_device *dev, void *data,
 	struct drm_i915_gem_create *args = data;
 
 	return i915_gem_create(file, dev,
-			       args->size, &args->handle);
+			       args->size, &args->handle,
+			       args->flags);
 }
 
 static inline int
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index f88cc1c..f041016 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -355,6 +355,7 @@ typedef struct drm_i915_irq_wait {
 #define I915_PARAM_SUBSLICE_TOTAL	 33
 #define I915_PARAM_EU_TOTAL		 34
 #define I915_PARAM_HAS_GPU_RESET	 35
+#define I915_PARAM_CREATE_VERSION	 36
 
 typedef struct drm_i915_getparam {
 	int param;
@@ -450,6 +451,20 @@ struct drm_i915_gem_create {
 	 */
 	__u32 handle;
 	__u32 pad;
+	/**
+	 * Requested flags (currently used for placement
+	 * (which memory domain))
+	 *
+	 * You can request that the object be created from special memory
+	 * rather than regular system pages using this parameter. Such
+	 * irregular objects may have certain restrictions (such as CPU
+	 * access to a stolen object is verboten).
+	 *
+	 * This can be used in the future for other purposes too
+	 * e.g. specifying tiling/caching/madvise
+	 */
+	__u32 flags;
+#define I915_CREATE_PLACEMENT_STOLEN (1<<0) /* Cannot use CPU mmaps */
 };
 
 struct drm_i915_gem_pread {
-- 
1.9.1

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

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

[ankitprasad.r.sharma]

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: correctedted if-else braces format (Tvrtko/kerneldoc)

Testcase: igt/gem_stolen

Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_stolen.c | 122 ++++++++++++++++++++++++++++++---
 1 file changed, 111 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 348ed5a..eaf0bdd 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -430,18 +430,29 @@ cleanup:
 	return NULL;
 }
 
-struct drm_i915_gem_object *
-i915_gem_object_create_stolen(struct drm_device *dev, u32 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 (i915_gem_obj_is_pinned(obj))
+		return false;
+
+	list_add(&obj->obj_exec_link, unwind);
+	return drm_mm_scan_add_block(obj->stolen);
+}
+
+static struct drm_mm_node *
+stolen_alloc(struct drm_i915_private *dev_priv, u32 size)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_i915_gem_object *obj;
 	struct drm_mm_node *stolen;
+	struct drm_i915_gem_object *obj;
+	struct list_head unwind, evict;
 	int ret;
 
-	if (!drm_mm_initialized(&dev_priv->mm.stolen))
-		return NULL;
-
-	DRM_DEBUG_KMS("creating stolen object: size=%x\n", size);
 	if (size == 0)
 		return NULL;
 
@@ -451,11 +462,100 @@ i915_gem_object_create_stolen(struct drm_device *dev, u32 size)
 
 	ret = drm_mm_insert_node(&dev_priv->mm.stolen, stolen, size,
 				 4096, DRM_MM_SEARCH_DEFAULT);
-	if (ret) {
-		kfree(stolen);
-		return NULL;
+	if (ret == 0)
+		return stolen;
+
+	/* No more stolen memory available, or too fragmented.
+	 * Try evicting purgeable objects and search again.
+	 */
+
+	drm_mm_init_scan(&dev_priv->mm.stolen, size, 4096, 0);
+	INIT_LIST_HEAD(&unwind);
+
+	list_for_each_entry(obj, &dev_priv->mm.unbound_list, global_list)
+		if (mark_free(obj, &unwind))
+			goto found;
+
+	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list)
+		if (mark_free(obj, &unwind))
+			goto found;
+
+found:
+	INIT_LIST_HEAD(&evict);
+	while (!list_empty(&unwind)) {
+		obj = list_first_entry(&unwind,
+				       struct drm_i915_gem_object,
+				       obj_exec_link);
+		list_del_init(&obj->obj_exec_link);
+
+		if (drm_mm_scan_remove_block(obj->stolen)) {
+			list_add(&obj->obj_exec_link, &evict);
+			drm_gem_object_reference(&obj->base);
+		}
 	}
 
+	ret = 0;
+	while (!list_empty(&evict)) {
+		obj = list_first_entry(&evict,
+				       struct drm_i915_gem_object,
+				       obj_exec_link);
+		list_del_init(&obj->obj_exec_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);
+	}
+
+	if (ret == 0)
+		ret = drm_mm_insert_node(&dev_priv->mm.stolen, stolen, size,
+					 4096, DRM_MM_SEARCH_DEFAULT);
+	if (ret == 0)
+		return stolen;
+
+	kfree(stolen);
+	return NULL;
+}
+
+struct drm_i915_gem_object *
+i915_gem_object_create_stolen(struct drm_device *dev, u32 size)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_gem_object *obj;
+	struct drm_mm_node *stolen;
+
+	lockdep_assert_held(&dev->struct_mutex);
+
+	if (!drm_mm_initialized(&dev_priv->mm.stolen))
+		return NULL;
+
+	DRM_DEBUG_KMS("creating stolen object: size=%x\n", size);
+
+	stolen = stolen_alloc(dev_priv, size);
+	if (stolen == NULL)
+		return NULL;
+
 	obj = _i915_gem_object_create_stolen(dev, stolen);
 	if (obj)
 		return obj;
-- 
1.9.1

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

[PATCH 4/4] drm/i915: Support for pread/pwrite from/to non shmem backed objects

[ankitprasad.r.sharma]

From: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>

This patch adds support for extending the pread/pwrite functionality
for objects not backed by shmem. The access will be made through
gtt interface.
This will cover prime objects as well as stolen memory backed objects
but for userptr objects it is still forbidden.

v2: Drop locks around slow_user_access, prefault the pages before
access (Chris)

v3: Rebased to the latest drm-intel-nightly (Ankit)

v4: Moved page base & offset calculations outside the copy loop,
corrected data types for size and offset variables, corrected if-else
braces format (Tvrtko/kerneldocs)

Testcase: igt/gem_stolen

Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 138 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 121 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 9e7e182..4321178 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -631,6 +631,102 @@ shmem_pread_slow(struct page *page, int shmem_page_offset, int page_length,
 	return ret ? - EFAULT : 0;
 }
 
+static inline uint64_t
+slow_user_access(struct io_mapping *mapping,
+		 uint64_t page_base, int page_offset,
+		 char __user *user_data,
+		 int length, bool pwrite)
+{
+	void __iomem *vaddr_inatomic;
+	void *vaddr;
+	uint64_t unwritten;
+
+	vaddr_inatomic = io_mapping_map_wc(mapping, page_base);
+	/* We can use the cpu mem copy function because this is X86. */
+	vaddr = (void __force *)vaddr_inatomic + page_offset;
+	if (pwrite)
+		unwritten = __copy_from_user(vaddr, user_data, length);
+	else
+		unwritten = __copy_to_user(user_data, vaddr, length);
+
+	io_mapping_unmap(vaddr_inatomic);
+	return unwritten;
+}
+
+static int
+i915_gem_gtt_pread_pwrite(struct drm_device *dev,
+			  struct drm_i915_gem_object *obj, uint64_t size,
+			  uint64_t data_offset, uint64_t data_ptr, bool pwrite)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	char __user *user_data;
+	uint64_t remain;
+	uint64_t offset, page_base;
+	int page_offset, page_length, ret = 0;
+
+	ret = i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE);
+	if (ret)
+		goto out;
+
+	ret = i915_gem_object_set_to_gtt_domain(obj, pwrite);
+	if (ret)
+		goto out_unpin;
+
+	ret = i915_gem_object_put_fence(obj);
+	if (ret)
+		goto out_unpin;
+
+	user_data = to_user_ptr(data_ptr);
+	remain = size;
+	offset = i915_gem_obj_ggtt_offset(obj) + data_offset;
+
+	if (pwrite)
+		intel_fb_obj_invalidate(obj, ORIGIN_GTT);
+
+	mutex_unlock(&dev->struct_mutex);
+	if (!pwrite && likely(!i915.prefault_disable))
+		ret = fault_in_multipages_writeable(user_data, remain);
+
+	/*
+	 * page_offset = offset within page
+	 * page_base = page offset within aperture
+	 */
+	page_offset = offset_in_page(offset);
+	page_base = offset & PAGE_MASK;
+
+	while (remain > 0) {
+		/* page_length = bytes to copy for this page */
+		page_length = remain;
+		if ((page_offset + remain) > PAGE_SIZE)
+			page_length = PAGE_SIZE - page_offset;
+
+		/* This is a slow read/write as it tries to read from
+		 * and write to user memory which may result into page
+		 * faults
+		 */
+		ret = slow_user_access(dev_priv->gtt.mappable, page_base,
+				       page_offset, user_data,
+				       page_length, pwrite);
+
+		if (ret) {
+			ret = -EFAULT;
+			break;
+		}
+
+		remain -= page_length;
+		user_data += page_length;
+		page_base += page_length;
+		page_offset = 0;
+	}
+
+	mutex_lock(&dev->struct_mutex);
+
+out_unpin:
+	i915_gem_object_ggtt_unpin(obj);
+out:
+	return ret;
+}
+
 static int
 i915_gem_shmem_pread(struct drm_device *dev,
 		     struct drm_i915_gem_object *obj,
@@ -754,17 +850,20 @@ i915_gem_pread_ioctl(struct drm_device *dev, void *data,
 		goto out;
 	}
 
-	/* prime objects have no backing filp to GEM pread/pwrite
-	 * pages from.
-	 */
-	if (!obj->base.filp) {
-		ret = -EINVAL;
-		goto out;
-	}
-
 	trace_i915_gem_object_pread(obj, args->offset, args->size);
 
-	ret = i915_gem_shmem_pread(dev, obj, args, file);
+	/* pread for non shmem backed objects */
+	if (!obj->base.filp) {
+		if (obj->tiling_mode == I915_TILING_NONE)
+			ret = i915_gem_gtt_pread_pwrite(dev, obj, args->size,
+							args->offset,
+							args->data_ptr,
+							false);
+		else
+			ret = -EINVAL;
+	} else {
+		ret = i915_gem_shmem_pread(dev, obj, args, file);
+	}
 
 out:
 	drm_gem_object_unreference(&obj->base);
@@ -1105,17 +1204,22 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
 		goto out;
 	}
 
-	/* prime objects have no backing filp to GEM pread/pwrite
-	 * pages from.
-	 */
-	if (!obj->base.filp) {
-		ret = -EINVAL;
-		goto out;
-	}
-
 	trace_i915_gem_object_pwrite(obj, args->offset, args->size);
 
 	ret = -EFAULT;
+
+	/* pwrite for non shmem backed objects */
+	if (!obj->base.filp) {
+		if (obj->tiling_mode == I915_TILING_NONE)
+			ret = i915_gem_gtt_pread_pwrite(dev, obj, args->size,
+							args->offset,
+							args->data_ptr,
+							true);
+		else
+			ret = -EINVAL;
+
+		goto out;
+	}
 	/* We can only do the GTT pwrite on untiled buffers, as otherwise
 	 * it would end up going through the fenced access, and we'll get
 	 * different detiling behavior between reading and writing.
-- 
1.9.1

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

Re: [PATCH 4/4] drm/i915: Support for pread/pwrite from/to non shmem backed objects

[Chris Wilson]

On Wed, Jul 22, 2015 at 07:21:49PM +0530, ankitprasad.r.sharma@intel.com wrote:
>  static int
>  i915_gem_shmem_pread(struct drm_device *dev,
>  		     struct drm_i915_gem_object *obj,
> @@ -754,17 +850,20 @@ i915_gem_pread_ioctl(struct drm_device *dev, void *data,
>  		goto out;
>  	}
>  
> -	/* prime objects have no backing filp to GEM pread/pwrite
> -	 * pages from.
> -	 */
> -	if (!obj->base.filp) {
> -		ret = -EINVAL;
> -		goto out;
> -	}
> -
>  	trace_i915_gem_object_pread(obj, args->offset, args->size);
>  
> -	ret = i915_gem_shmem_pread(dev, obj, args, file);
> +	/* pread for non shmem backed objects */
> +	if (!obj->base.filp) {
> +		if (obj->tiling_mode == I915_TILING_NONE)

pread/pwrite is defined as a cpu linear, the restriction upon tiling is
a simplification of handling swizzling.

> +			ret = i915_gem_gtt_pread_pwrite(dev, obj, args->size,
> +							args->offset,
> +							args->data_ptr,
> +							false);
> +		else
> +			ret = -EINVAL;
> +	} else {
> +		ret = i915_gem_shmem_pread(dev, obj, args, file);
> +	}
>  
>  out:
>  	drm_gem_object_unreference(&obj->base);
> @@ -1105,17 +1204,22 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
>  		goto out;
>  	}
>  
> -	/* prime objects have no backing filp to GEM pread/pwrite
> -	 * pages from.
> -	 */
> -	if (!obj->base.filp) {
> -		ret = -EINVAL;
> -		goto out;
> -	}
> -
>  	trace_i915_gem_object_pwrite(obj, args->offset, args->size);
>  
>  	ret = -EFAULT;
> +
> +	/* pwrite for non shmem backed objects */
> +	if (!obj->base.filp) {
> +		if (obj->tiling_mode == I915_TILING_NONE)
> +			ret = i915_gem_gtt_pread_pwrite(dev, obj, args->size,
> +							args->offset,
> +							args->data_ptr,
> +							true);
> +		else
> +			ret = -EINVAL;
> +
> +		goto out;

The fast pwrite code always works for obj->base.filp == NULL. To easily
make it generic and handle the cases where we cannot fallback to shem,
undo the PIN_NONBLOCK.

Then you just want something like
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index d3016f37cd4d..f2284a27dd6d 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1114,8 +1114,9 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
         * perspective, requiring manual detiling by the client.
         */
        if (obj->tiling_mode == I915_TILING_NONE &&
-           obj->base.write_domain != I915_GEM_DOMAIN_CPU &&
-           cpu_write_needs_clflush(obj)) {
+           (obj->base.filp == NULL ||
+            (obj->base.write_domain != I915_GEM_DOMAIN_CPU &&
+             cpu_write_needs_clflush(obj)))) {
                ret = i915_gem_gtt_pwrite_fast(dev_priv, obj, args, file);
                /* Note that the gtt paths might fail with non-page-backed user
                 * pointers (e.g. gtt mappings when moving data between
@@ -1125,7 +1126,7 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
        if (ret == -EFAULT || ret == -ENOSPC) {
                if (obj->phys_handle)
                        ret = i915_gem_phys_pwrite(obj, args, file);
-               else
+               else if (obj->filp)
                        ret = i915_gem_shmem_pwrite(dev, obj, args, file);
        }
 

to enable pwrite access to stolen.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/i915: Clearing buffer objects via CPU/GTT

[Tvrtko Ursulin]

Hi,

On 07/22/2015 02:51 PM, ankitprasad.r.sharma@intel.com wrote:
> From: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
>
> This patch adds support for clearing buffer objects via CPU/GTT. This
> is particularly useful for clearing out the non shmem backed objects.
> Currently intend to use this only for buffers allocated from stolen
> region.
>
> Testcase: igt/gem_stolen
>
> Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h |  1 +
>   drivers/gpu/drm/i915/i915_gem.c | 34 ++++++++++++++++++++++++++++++++++
>   2 files changed, 35 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ea9caf2..f6af9c0 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2764,6 +2764,7 @@ int i915_gem_obj_prepare_shmem_read(struct drm_i915_gem_object *obj,
>   				    int *needs_clflush);
>
>   int __must_check i915_gem_object_get_pages(struct drm_i915_gem_object *obj);
> +int i915_gem_clear_object(struct drm_i915_gem_object *obj);
>
>   static inline int __sg_page_count(struct scatterlist *sg)
>   {
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index a2a4a27..fc434ae 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -5469,3 +5469,37 @@ bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj)
>   	return false;
>   }
>
> +int i915_gem_clear_object(struct drm_i915_gem_object *obj)

Please add kernel doc for the function.

> +{
> +	int ret = 0;

No need to set to zero.

> +	char __iomem *base;
> +	int size = obj->base.size;

I think obj->base.size is a size_t at the moment so this could cause 
compiler to warn?

> +	struct drm_i915_private *dev_priv = obj->base.dev->dev_private;

Again compiler should warn here due implicit pointer casting? I think 
correct is "dev_priv = to_i915(obj->base.dev)".

> +	unsigned alignment = 0;

Don't even need this as variable since it is used only once.

> +
> +	ret = i915_gem_obj_ggtt_pin(obj, alignment, PIN_MAPPABLE);
> +	if (ret) {
> +		DRM_ERROR("Mapping of gem object to GTT failed\n");

Maybe end the sentence with "!" ?

> +		return ret;
> +	}
> +
> +	ret = i915_gem_object_put_fence(obj);
> +	if (ret)
> +		goto unpin;

This I don't understand. Why it is OK to release a fence and never 
re-establish it? Could that surprise some callers?

Regards,

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

Re: [PATCH 1/4] drm/i915: Clearing buffer objects via CPU/GTT

[Chris Wilson]

On Wed, Jul 22, 2015 at 04:01:33PM +0100, Tvrtko Ursulin wrote:
> >+	ret = i915_gem_obj_ggtt_pin(obj, alignment, PIN_MAPPABLE);
> >+	if (ret) {
> >+		DRM_ERROR("Mapping of gem object to GTT failed\n");
> 
> Maybe end the sentence with "!" ?

Or better yet, don't do anything. The error is propagate back to the
user, and is going to be -EINTR.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/i915: Clearing buffer objects via CPU/GTT

[Chris Wilson]

On Wed, Jul 22, 2015 at 04:01:33PM +0100, Tvrtko Ursulin wrote:
> This I don't understand. Why it is OK to release a fence and never
> re-establish it? Could that surprise some callers?

We always call get_fence() before we need it (and pin it as required),
because it can be revoked at anytime as they are a scarce resource.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

On Wed, Jul 22, 2015 at 07:21:48PM +0530, ankitprasad.r.sharma@intel.com wrote:
> +struct drm_i915_gem_object *
> +i915_gem_object_create_stolen(struct drm_device *dev, u32 size)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_i915_gem_object *obj;
> +	struct drm_mm_node *stolen;
> +
> +	lockdep_assert_held(&dev->struct_mutex);

We are now using dev->smm.stolen_lock instead and do not require the
caller to take struct_mutex. Please fix up the locking accordingly.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] drm/i915: Support for creating Stolen memory backed objects

[Tvrtko Ursulin]

Hi,

On 07/22/2015 02:51 PM, ankitprasad.r.sharma@intel.com wrote:
> From: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
>
> Extend the drm_i915_gem_create structure to add support for
> creating Stolen memory backed objects. Added a new flag through
> which user can specify the preference to allocate the object from
> stolen memory, which if set, an attempt will be made to allocate
> the object from stolen memory subject to the availability of
> free space in the stolen region.
>
> v2: Rebased to the latest drm-intel-nightly (Ankit)
>
> v3: Changed versioning of GEM_CREATE param, added new comments (Tvrtko)
>
> Testcase: igt/gem_stolen
>
> Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_dma.c |  3 +++
>   drivers/gpu/drm/i915/i915_gem.c | 33 +++++++++++++++++++++++++++++----
>   include/uapi/drm/i915_drm.h     | 15 +++++++++++++++
>   3 files changed, 47 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index c5349fa..bfb07ab 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -167,6 +167,9 @@ static int i915_getparam(struct drm_device *dev, void *data,
>   		value = i915.enable_hangcheck &&
>   			intel_has_gpu_reset(dev);
>   		break;
> +	case I915_PARAM_CREATE_VERSION:
> +		value = 2;
> +		break;
>   	default:
>   		DRM_DEBUG("Unknown parameter %d\n", param->param);
>   		return -EINVAL;
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index fc434ae..9e7e182 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -391,7 +391,8 @@ static int
>   i915_gem_create(struct drm_file *file,
>   		struct drm_device *dev,
>   		uint64_t size,
> -		uint32_t *handle_p)
> +		uint32_t *handle_p,
> +		uint32_t flags)
>   {
>   	struct drm_i915_gem_object *obj;
>   	int ret;
> @@ -401,8 +402,31 @@ i915_gem_create(struct drm_file *file,
>   	if (size == 0)
>   		return -EINVAL;
>
> +	if (flags & ~(I915_CREATE_PLACEMENT_STOLEN))
> +		return -EINVAL;
> +
>   	/* Allocate the new object */
> -	obj = i915_gem_alloc_object(dev, size);
> +	if (flags & I915_CREATE_PLACEMENT_STOLEN) {
> +		mutex_lock(&dev->struct_mutex);
> +		obj = i915_gem_object_create_stolen(dev, size);

Is the compiler not complaining that "uint64_t size" is being passed 
into "u32 size" here?

Either since there are no checks, can't userspace overflow u32 with a 
right value and get success and much smaller object than intended?

Perhaps should test with an IGT.

Regards,

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

Re: [PATCH 1/4] drm/i915: Clearing buffer objects via CPU/GTT

[Tvrtko Ursulin]

On 07/22/2015 04:06 PM, Chris Wilson wrote:
> On Wed, Jul 22, 2015 at 04:01:33PM +0100, Tvrtko Ursulin wrote:
>> This I don't understand. Why it is OK to release a fence and never
>> re-establish it? Could that surprise some callers?
>
> We always call get_fence() before we need it (and pin it as required),
> because it can be revoked at anytime as they are a scarce resource.

Oookay, and why it is important to release it here? Does it warrant a 
comment or everyone knows this?

Regards,

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

Re: [PATCH 1/4] drm/i915: Clearing buffer objects via CPU/GTT

[Chris Wilson]

On Wed, Jul 22, 2015 at 04:16:20PM +0100, Tvrtko Ursulin wrote:
> 
> On 07/22/2015 04:06 PM, Chris Wilson wrote:
> >On Wed, Jul 22, 2015 at 04:01:33PM +0100, Tvrtko Ursulin wrote:
> >>This I don't understand. Why it is OK to release a fence and never
> >>re-establish it? Could that surprise some callers?
> >
> >We always call get_fence() before we need it (and pin it as required),
> >because it can be revoked at anytime as they are a scarce resource.
> 
> Oookay, and why it is important to release it here? Does it warrant
> a comment or everyone knows this?

There is fun to be had as the destination address is detiled by the
hardware meaning that it is possible for the writes into the last tile
row to be after the GTT address of the object. We should have allocated
sufficient space in the GTT to cover all fenced writes, but that is
actually only true on gen2/3.

There is also the fun that avoiding the fence (besides being a resource
that we may have to wait for and cause pagefaults elsewhere) is a bit
faster than writing through one.

So not using a fence is faster and less error prone.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] drm/i915: Support for creating Stolen memory backed objects

[Chris Wilson]

On Wed, Jul 22, 2015 at 04:14:37PM +0100, Tvrtko Ursulin wrote:
> 
> Hi,
> 
> On 07/22/2015 02:51 PM, ankitprasad.r.sharma@intel.com wrote:
> >From: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
> >
> >Extend the drm_i915_gem_create structure to add support for
> >creating Stolen memory backed objects. Added a new flag through
> >which user can specify the preference to allocate the object from
> >stolen memory, which if set, an attempt will be made to allocate
> >the object from stolen memory subject to the availability of
> >free space in the stolen region.
> >
> >v2: Rebased to the latest drm-intel-nightly (Ankit)
> >
> >v3: Changed versioning of GEM_CREATE param, added new comments (Tvrtko)
> >
> >Testcase: igt/gem_stolen
> >
> >Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
> >---
> >  drivers/gpu/drm/i915/i915_dma.c |  3 +++
> >  drivers/gpu/drm/i915/i915_gem.c | 33 +++++++++++++++++++++++++++++----
> >  include/uapi/drm/i915_drm.h     | 15 +++++++++++++++
> >  3 files changed, 47 insertions(+), 4 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> >index c5349fa..bfb07ab 100644
> >--- a/drivers/gpu/drm/i915/i915_dma.c
> >+++ b/drivers/gpu/drm/i915/i915_dma.c
> >@@ -167,6 +167,9 @@ static int i915_getparam(struct drm_device *dev, void *data,
> >  		value = i915.enable_hangcheck &&
> >  			intel_has_gpu_reset(dev);
> >  		break;
> >+	case I915_PARAM_CREATE_VERSION:
> >+		value = 2;
> >+		break;
> >  	default:
> >  		DRM_DEBUG("Unknown parameter %d\n", param->param);
> >  		return -EINVAL;
> >diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >index fc434ae..9e7e182 100644
> >--- a/drivers/gpu/drm/i915/i915_gem.c
> >+++ b/drivers/gpu/drm/i915/i915_gem.c
> >@@ -391,7 +391,8 @@ static int
> >  i915_gem_create(struct drm_file *file,
> >  		struct drm_device *dev,
> >  		uint64_t size,
> >-		uint32_t *handle_p)
> >+		uint32_t *handle_p,
> >+		uint32_t flags)

out parameters go at the end, please.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915: Support for pread/pwrite from/to non shmem backed objects

[Tvrtko Ursulin]

Hi,

On 07/22/2015 02:51 PM, ankitprasad.r.sharma@intel.com wrote:
> From: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
>
> This patch adds support for extending the pread/pwrite functionality
> for objects not backed by shmem. The access will be made through
> gtt interface.
> This will cover prime objects as well as stolen memory backed objects
> but for userptr objects it is still forbidden.
>
> v2: Drop locks around slow_user_access, prefault the pages before
> access (Chris)
>
> v3: Rebased to the latest drm-intel-nightly (Ankit)
>
> v4: Moved page base & offset calculations outside the copy loop,
> corrected data types for size and offset variables, corrected if-else
> braces format (Tvrtko/kerneldocs)
>
> Testcase: igt/gem_stolen
>
> Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem.c | 138 +++++++++++++++++++++++++++++++++++-----
>   1 file changed, 121 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 9e7e182..4321178 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -631,6 +631,102 @@ shmem_pread_slow(struct page *page, int shmem_page_offset, int page_length,
>   	return ret ? - EFAULT : 0;
>   }
>
> +static inline uint64_t
> +slow_user_access(struct io_mapping *mapping,
> +		 uint64_t page_base, int page_offset,
> +		 char __user *user_data,
> +		 int length, bool pwrite)
> +{
> +	void __iomem *vaddr_inatomic;
> +	void *vaddr;
> +	uint64_t unwritten;
> +
> +	vaddr_inatomic = io_mapping_map_wc(mapping, page_base);
> +	/* We can use the cpu mem copy function because this is X86. */
> +	vaddr = (void __force *)vaddr_inatomic + page_offset;
> +	if (pwrite)
> +		unwritten = __copy_from_user(vaddr, user_data, length);
> +	else
> +		unwritten = __copy_to_user(user_data, vaddr, length);
> +
> +	io_mapping_unmap(vaddr_inatomic);
> +	return unwritten;
> +}
> +
> +static int
> +i915_gem_gtt_pread_pwrite(struct drm_device *dev,
> +			  struct drm_i915_gem_object *obj, uint64_t size,
> +			  uint64_t data_offset, uint64_t data_ptr, bool pwrite)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	char __user *user_data;
> +	uint64_t remain;
> +	uint64_t offset, page_base;
> +	int page_offset, page_length, ret = 0;
> +
> +	ret = i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE);
> +	if (ret)
> +		goto out;
> +
> +	ret = i915_gem_object_set_to_gtt_domain(obj, pwrite);
> +	if (ret)
> +		goto out_unpin;
> +
> +	ret = i915_gem_object_put_fence(obj);
> +	if (ret)
> +		goto out_unpin;
> +
> +	user_data = to_user_ptr(data_ptr);
> +	remain = size;
> +	offset = i915_gem_obj_ggtt_offset(obj) + data_offset;
> +
> +	if (pwrite)
> +		intel_fb_obj_invalidate(obj, ORIGIN_GTT);
> +
> +	mutex_unlock(&dev->struct_mutex);
> +	if (!pwrite && likely(!i915.prefault_disable))
> +		ret = fault_in_multipages_writeable(user_data, remain);
> +
> +	/*
> +	 * page_offset = offset within page
> +	 * page_base = page offset within aperture
> +	 */
> +	page_offset = offset_in_page(offset);
> +	page_base = offset & PAGE_MASK;
> +
> +	while (remain > 0) {
> +		/* page_length = bytes to copy for this page */
> +		page_length = remain;
> +		if ((page_offset + remain) > PAGE_SIZE)
> +			page_length = PAGE_SIZE - page_offset;
> +
> +		/* This is a slow read/write as it tries to read from
> +		 * and write to user memory which may result into page
> +		 * faults
> +		 */
> +		ret = slow_user_access(dev_priv->gtt.mappable, page_base,
> +				       page_offset, user_data,
> +				       page_length, pwrite);
> +
> +		if (ret) {
> +			ret = -EFAULT;
> +			break;
> +		}
> +
> +		remain -= page_length;
> +		user_data += page_length;
> +		page_base += page_length;
> +		page_offset = 0;
> +	}
> +
> +	mutex_lock(&dev->struct_mutex);

My objection here was that we are re-acquiring the mutex in 
non-interruptible mode, while the caller had it interruptible. It am not 
100% what the conclusion back then was?

But also, why it is even necessary to drop the mutex here?

Regards,

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

Re: [PATCH 4/4] drm/i915: Support for pread/pwrite from/to non shmem backed objects

[Daniel Vetter]

On Wed, Jul 22, 2015 at 04:46:00PM +0100, Tvrtko Ursulin wrote:
> But also, why it is even necessary to drop the mutex here?

Locking inversion with our own pagefault handler. Happens since at least
mesa can return gtt mmaps to gl clients, which can then in turn try to
upload that with pwrite somewhere else. Also userspace could be just plain
nasty and try to deadlock the kernel ;-)

Dropping the struct_mutex is SOP for the copy_from/to_user slowpath.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915: Support for pread/pwrite from/to non shmem backed objects

[Tvrtko Ursulin]

On 07/22/2015 05:05 PM, Daniel Vetter wrote:
> On Wed, Jul 22, 2015 at 04:46:00PM +0100, Tvrtko Ursulin wrote:
>> But also, why it is even necessary to drop the mutex here?
>
> Locking inversion with our own pagefault handler. Happens since at least
> mesa can return gtt mmaps to gl clients, which can then in turn try to
> upload that with pwrite somewhere else. Also userspace could be just plain
> nasty and try to deadlock the kernel ;-)

Yeah blame userspace. ;D

> Dropping the struct_mutex is SOP for the copy_from/to_user slowpath.

I suspected something like that but did not feel like thinking too much. 
:) When encountering unusual things like dropping and re-acquiring locks 
I expect to see comments explaining it.

Regards,

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

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

[Daniel Vetter]

On Wed, Jul 22, 2015 at 07:21:48PM +0530, 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: correctedted if-else braces format (Tvrtko/kerneldoc)
> 
> Testcase: igt/gem_stolen
> 
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_stolen.c | 122 ++++++++++++++++++++++++++++++---
>  1 file changed, 111 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index 348ed5a..eaf0bdd 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -430,18 +430,29 @@ cleanup:
>  	return NULL;
>  }
>  
> -struct drm_i915_gem_object *
> -i915_gem_object_create_stolen(struct drm_device *dev, u32 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 (i915_gem_obj_is_pinned(obj))
> +		return false;
> +
> +	list_add(&obj->obj_exec_link, unwind);
> +	return drm_mm_scan_add_block(obj->stolen);
> +}
> +
> +static struct drm_mm_node *
> +stolen_alloc(struct drm_i915_private *dev_priv, u32 size)
>  {
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct drm_i915_gem_object *obj;
>  	struct drm_mm_node *stolen;
> +	struct drm_i915_gem_object *obj;
> +	struct list_head unwind, evict;
>  	int ret;
>  
> -	if (!drm_mm_initialized(&dev_priv->mm.stolen))
> -		return NULL;
> -
> -	DRM_DEBUG_KMS("creating stolen object: size=%x\n", size);
>  	if (size == 0)
>  		return NULL;
>  
> @@ -451,11 +462,100 @@ i915_gem_object_create_stolen(struct drm_device *dev, u32 size)
>  
>  	ret = drm_mm_insert_node(&dev_priv->mm.stolen, stolen, size,
>  				 4096, DRM_MM_SEARCH_DEFAULT);
> -	if (ret) {
> -		kfree(stolen);
> -		return NULL;
> +	if (ret == 0)
> +		return stolen;
> +
> +	/* No more stolen memory available, or too fragmented.
> +	 * Try evicting purgeable objects and search again.
> +	 */
> +
> +	drm_mm_init_scan(&dev_priv->mm.stolen, size, 4096, 0);
> +	INIT_LIST_HEAD(&unwind);
> +
> +	list_for_each_entry(obj, &dev_priv->mm.unbound_list, global_list)
> +		if (mark_free(obj, &unwind))
> +			goto found;
> +
> +	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list)
> +		if (mark_free(obj, &unwind))
> +			goto found;

Chris and I just discussed on irc that the bound_list isn't in a great LRU
order right now and Chris sent out a fix for that. But it only works if we
preferrentially shrink inactive objects first. Worth the bother or just a
FIXME? For the fb use-case alone it's not needed since we can't remove the
fb until it's no longer being displayed (otherwise the backwards-compat
code kicks in and synchronously kills the display at RMFB time), and that
pretty much means we can't put the underlying bo into any cache (and mark
it purgeable) either. But a FIXME comment here would be good for sure,
just in case this assumption ever gets broken.
-Daniel

> +
> +found:
> +	INIT_LIST_HEAD(&evict);
> +	while (!list_empty(&unwind)) {
> +		obj = list_first_entry(&unwind,
> +				       struct drm_i915_gem_object,
> +				       obj_exec_link);
> +		list_del_init(&obj->obj_exec_link);
> +
> +		if (drm_mm_scan_remove_block(obj->stolen)) {
> +			list_add(&obj->obj_exec_link, &evict);
> +			drm_gem_object_reference(&obj->base);
> +		}
>  	}
>  
> +	ret = 0;
> +	while (!list_empty(&evict)) {
> +		obj = list_first_entry(&evict,
> +				       struct drm_i915_gem_object,
> +				       obj_exec_link);
> +		list_del_init(&obj->obj_exec_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);
> +	}
> +
> +	if (ret == 0)
> +		ret = drm_mm_insert_node(&dev_priv->mm.stolen, stolen, size,
> +					 4096, DRM_MM_SEARCH_DEFAULT);
> +	if (ret == 0)
> +		return stolen;
> +
> +	kfree(stolen);
> +	return NULL;
> +}
> +
> +struct drm_i915_gem_object *
> +i915_gem_object_create_stolen(struct drm_device *dev, u32 size)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_i915_gem_object *obj;
> +	struct drm_mm_node *stolen;
> +
> +	lockdep_assert_held(&dev->struct_mutex);
> +
> +	if (!drm_mm_initialized(&dev_priv->mm.stolen))
> +		return NULL;
> +
> +	DRM_DEBUG_KMS("creating stolen object: size=%x\n", size);
> +
> +	stolen = stolen_alloc(dev_priv, size);
> +	if (stolen == NULL)
> +		return NULL;
> +
>  	obj = _i915_gem_object_create_stolen(dev, stolen);
>  	if (obj)
>  		return obj;
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

On Mon, Jul 27, 2015 at 11:38:13AM +0200, Daniel Vetter wrote:
> Chris and I just discussed on irc that the bound_list isn't in a great LRU
> order right now and Chris sent out a fix for that. But it only works if we
> preferrentially shrink inactive objects first. Worth the bother or just a
> FIXME? For the fb use-case alone it's not needed since we can't remove the
> fb until it's no longer being displayed (otherwise the backwards-compat
> code kicks in and synchronously kills the display at RMFB time), and that
> pretty much means we can't put the underlying bo into any cache (and mark
> it purgeable) either. But a FIXME comment here would be good for sure,
> just in case this assumption ever gets broken.

I've been mucking around with patch a bit (with contexts-from-stolen
reenabled) and the list ierators used here are terrible; severely
impacting our allocations by a few orders of magnitude (imagine having
just the ggtt full of 4k objects, let alone several ppgtt all full of
their own bound 4k objetcs).

To combat this will require a special purgeable list maintaind by
madv(), and subclassing the struct drm_mm_node to hold our extra
details.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915: Support for pread/pwrite from/to non shmem backed objects

[Goel, Akash]

On 7/22/2015 8:09 PM, Chris Wilson wrote:
> On Wed, Jul 22, 2015 at 07:21:49PM +0530, ankitprasad.r.sharma@intel.com wrote:
>>   static int
>>   i915_gem_shmem_pread(struct drm_device *dev,
>>   		     struct drm_i915_gem_object *obj,
>> @@ -754,17 +850,20 @@ i915_gem_pread_ioctl(struct drm_device *dev, void *data,
>>   		goto out;
>>   	}
>>
>> -	/* prime objects have no backing filp to GEM pread/pwrite
>> -	 * pages from.
>> -	 */
>> -	if (!obj->base.filp) {
>> -		ret = -EINVAL;
>> -		goto out;
>> -	}
>> -
>>   	trace_i915_gem_object_pread(obj, args->offset, args->size);
>>
>> -	ret = i915_gem_shmem_pread(dev, obj, args, file);
>> +	/* pread for non shmem backed objects */
>> +	if (!obj->base.filp) {
>> +		if (obj->tiling_mode == I915_TILING_NONE)
>
> pread/pwrite is defined as a cpu linear, the restriction upon tiling is
> a simplification of handling swizzling.
>
>> +			ret = i915_gem_gtt_pread_pwrite(dev, obj, args->size,
>> +							args->offset,
>> +							args->data_ptr,
>> +							false);
>> +		else
>> +			ret = -EINVAL;
>> +	} else {
>> +		ret = i915_gem_shmem_pread(dev, obj, args, file);
>> +	}
>>
>>   out:
>>   	drm_gem_object_unreference(&obj->base);
>> @@ -1105,17 +1204,22 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
>>   		goto out;
>>   	}
>>
>> -	/* prime objects have no backing filp to GEM pread/pwrite
>> -	 * pages from.
>> -	 */
>> -	if (!obj->base.filp) {
>> -		ret = -EINVAL;
>> -		goto out;
>> -	}
>> -
>>   	trace_i915_gem_object_pwrite(obj, args->offset, args->size);
>>
>>   	ret = -EFAULT;
>> +
>> +	/* pwrite for non shmem backed objects */
>> +	if (!obj->base.filp) {
>> +		if (obj->tiling_mode == I915_TILING_NONE)
>> +			ret = i915_gem_gtt_pread_pwrite(dev, obj, args->size,
>> +							args->offset,
>> +							args->data_ptr,
>> +							true);
>> +		else
>> +			ret = -EINVAL;
>> +
>> +		goto out;
>
> The fast pwrite code always works for obj->base.filp == NULL. To easily
> make it generic and handle the cases where we cannot fallback to shem,
> undo the PIN_NONBLOCK.
>
> Then you just want something like
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index d3016f37cd4d..f2284a27dd6d 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1114,8 +1114,9 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
>           * perspective, requiring manual detiling by the client.
>           */
>          if (obj->tiling_mode == I915_TILING_NONE &&

Since the suggestion is to reuse the gtt_pwrite_fast function only for 
non-shmem backed objects, can we also relax the Tiling constraint here, 
apart from removing the PIN_NONBLOCK flag. As anyways there is a 
put_fence already being done in gtt_pwrite_fast function.

Also currently the gtt_pwrite_fast function is non-tolerant to faults, 
incurred while accessing the User space buffer, so can that also be 
relaxed (at least for the non-shmem backed objects) ??

Best regards
Akash

> -           obj->base.write_domain != I915_GEM_DOMAIN_CPU &&
> -           cpu_write_needs_clflush(obj)) {
> +           (obj->base.filp == NULL ||
> +            (obj->base.write_domain != I915_GEM_DOMAIN_CPU &&
> +             cpu_write_needs_clflush(obj)))) {
>                  ret = i915_gem_gtt_pwrite_fast(dev_priv, obj, args, file);
>                  /* Note that the gtt paths might fail with non-page-backed user
>                   * pointers (e.g. gtt mappings when moving data between
> @@ -1125,7 +1126,7 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
>          if (ret == -EFAULT || ret == -ENOSPC) {
>                  if (obj->phys_handle)
>                          ret = i915_gem_phys_pwrite(obj, args, file);
> -               else
> +               else if (obj->filp)
>                          ret = i915_gem_shmem_pwrite(dev, obj, args, file);
>          }
>
>
> to enable pwrite access to stolen.
> -Chris
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Goel, Akash]

On 7/27/2015 3:08 PM, Daniel Vetter wrote:
> On Wed, Jul 22, 2015 at 07:21:48PM +0530, 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: correctedted if-else braces format (Tvrtko/kerneldoc)
>>
>> Testcase: igt/gem_stolen
>>
>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_gem_stolen.c | 122 ++++++++++++++++++++++++++++++---
>>   1 file changed, 111 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
>> index 348ed5a..eaf0bdd 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
>> @@ -430,18 +430,29 @@ cleanup:
>>   	return NULL;
>>   }
>>
>> -struct drm_i915_gem_object *
>> -i915_gem_object_create_stolen(struct drm_device *dev, u32 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 (i915_gem_obj_is_pinned(obj))
>> +		return false;
>> +
>> +	list_add(&obj->obj_exec_link, unwind);
>> +	return drm_mm_scan_add_block(obj->stolen);
>> +}
>> +
>> +static struct drm_mm_node *
>> +stolen_alloc(struct drm_i915_private *dev_priv, u32 size)
>>   {
>> -	struct drm_i915_private *dev_priv = dev->dev_private;
>> -	struct drm_i915_gem_object *obj;
>>   	struct drm_mm_node *stolen;
>> +	struct drm_i915_gem_object *obj;
>> +	struct list_head unwind, evict;
>>   	int ret;
>>
>> -	if (!drm_mm_initialized(&dev_priv->mm.stolen))
>> -		return NULL;
>> -
>> -	DRM_DEBUG_KMS("creating stolen object: size=%x\n", size);
>>   	if (size == 0)
>>   		return NULL;
>>
>> @@ -451,11 +462,100 @@ i915_gem_object_create_stolen(struct drm_device *dev, u32 size)
>>
>>   	ret = drm_mm_insert_node(&dev_priv->mm.stolen, stolen, size,
>>   				 4096, DRM_MM_SEARCH_DEFAULT);
>> -	if (ret) {
>> -		kfree(stolen);
>> -		return NULL;
>> +	if (ret == 0)
>> +		return stolen;
>> +
>> +	/* No more stolen memory available, or too fragmented.
>> +	 * Try evicting purgeable objects and search again.
>> +	 */
>> +
>> +	drm_mm_init_scan(&dev_priv->mm.stolen, size, 4096, 0);
>> +	INIT_LIST_HEAD(&unwind);
>> +
>> +	list_for_each_entry(obj, &dev_priv->mm.unbound_list, global_list)
>> +		if (mark_free(obj, &unwind))
>> +			goto found;
>> +
>> +	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list)
>> +		if (mark_free(obj, &unwind))
>> +			goto found;
>
> Chris and I just discussed on irc that the bound_list isn't in a great LRU
> order right now and Chris sent out a fix for that. But it only works if we
> preferrentially shrink inactive objects first. Worth the bother or just a
> FIXME?
Sorry for the late response. Do you mean to say here that within the 
bound list, first the inactive stolen objects should be considered for 
purge ?
Is it very likely that an active bo will also be marked as purgeable ?

> For the fb use-case alone it's not needed since we can't remove the
> fb until it's no longer being displayed (otherwise the backwards-compat
> code kicks in and synchronously kills the display at RMFB time), and that
> pretty much means we can't put the underlying bo into any cache (and mark
> it purgeable) either.
Here do you mean that a frame buffer bo should not be (or cannot be) 
marked as purgeable by User, if it is still being scanned out ?

Best regards
Akash

  But a FIXME comment here would be good for sure,
> just in case this assumption ever gets broken.
> -Daniel
>
>> +
>> +found:
>> +	INIT_LIST_HEAD(&evict);
>> +	while (!list_empty(&unwind)) {
>> +		obj = list_first_entry(&unwind,
>> +				       struct drm_i915_gem_object,
>> +				       obj_exec_link);
>> +		list_del_init(&obj->obj_exec_link);
>> +
>> +		if (drm_mm_scan_remove_block(obj->stolen)) {
>> +			list_add(&obj->obj_exec_link, &evict);
>> +			drm_gem_object_reference(&obj->base);
>> +		}
>>   	}
>>
>> +	ret = 0;
>> +	while (!list_empty(&evict)) {
>> +		obj = list_first_entry(&evict,
>> +				       struct drm_i915_gem_object,
>> +				       obj_exec_link);
>> +		list_del_init(&obj->obj_exec_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);
>> +	}
>> +
>> +	if (ret == 0)
>> +		ret = drm_mm_insert_node(&dev_priv->mm.stolen, stolen, size,
>> +					 4096, DRM_MM_SEARCH_DEFAULT);
>> +	if (ret == 0)
>> +		return stolen;
>> +
>> +	kfree(stolen);
>> +	return NULL;
>> +}
>> +
>> +struct drm_i915_gem_object *
>> +i915_gem_object_create_stolen(struct drm_device *dev, u32 size)
>> +{
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	struct drm_i915_gem_object *obj;
>> +	struct drm_mm_node *stolen;
>> +
>> +	lockdep_assert_held(&dev->struct_mutex);
>> +
>> +	if (!drm_mm_initialized(&dev_priv->mm.stolen))
>> +		return NULL;
>> +
>> +	DRM_DEBUG_KMS("creating stolen object: size=%x\n", size);
>> +
>> +	stolen = stolen_alloc(dev_priv, size);
>> +	if (stolen == NULL)
>> +		return NULL;
>> +
>>   	obj = _i915_gem_object_create_stolen(dev, stolen);
>>   	if (obj)
>>   		return obj;
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Goel, Akash]

On 7/29/2015 5:34 PM, Chris Wilson wrote:
> On Mon, Jul 27, 2015 at 11:38:13AM +0200, Daniel Vetter wrote:
>> Chris and I just discussed on irc that the bound_list isn't in a great LRU
>> order right now and Chris sent out a fix for that. But it only works if we
>> preferrentially shrink inactive objects first. Worth the bother or just a
>> FIXME? For the fb use-case alone it's not needed since we can't remove the
>> fb until it's no longer being displayed (otherwise the backwards-compat
>> code kicks in and synchronously kills the display at RMFB time), and that
>> pretty much means we can't put the underlying bo into any cache (and mark
>> it purgeable) either. But a FIXME comment here would be good for sure,
>> just in case this assumption ever gets broken.
>
> I've been mucking around with patch a bit (with contexts-from-stolen
> reenabled) and the list ierators used here are terrible; severely
> impacting our allocations by a few orders of magnitude (imagine having
> just the ggtt full of 4k objects, let alone several ppgtt all full of
> their own bound 4k objetcs).
>
> To combat this will require a special purgeable list maintaind by
> madv(), and subclassing the struct drm_mm_node to hold our extra
> details.

Should we add a separate purgeable list for stolen objects ?


/** Stolen memory for this object, instead of being backed by shmem. */
- struct drm_mm_node *stolen;
+ struct i915_gem_stolen *stolen;


struct i915_gem_stolen {
	struct drm_mm_node *node;
	struct list_head purge_list;
};

Best regards
Akash

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

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

[Chris Wilson]

On Fri, Jul 31, 2015 at 08:12:30PM +0530, Goel, Akash wrote:
> 
> 
> On 7/29/2015 5:34 PM, Chris Wilson wrote:
> >On Mon, Jul 27, 2015 at 11:38:13AM +0200, Daniel Vetter wrote:
> >>Chris and I just discussed on irc that the bound_list isn't in a great LRU
> >>order right now and Chris sent out a fix for that. But it only works if we
> >>preferrentially shrink inactive objects first. Worth the bother or just a
> >>FIXME? For the fb use-case alone it's not needed since we can't remove the
> >>fb until it's no longer being displayed (otherwise the backwards-compat
> >>code kicks in and synchronously kills the display at RMFB time), and that
> >>pretty much means we can't put the underlying bo into any cache (and mark
> >>it purgeable) either. But a FIXME comment here would be good for sure,
> >>just in case this assumption ever gets broken.
> >
> >I've been mucking around with patch a bit (with contexts-from-stolen
> >reenabled) and the list ierators used here are terrible; severely
> >impacting our allocations by a few orders of magnitude (imagine having
> >just the ggtt full of 4k objects, let alone several ppgtt all full of
> >their own bound 4k objetcs).
> >
> >To combat this will require a special purgeable list maintaind by
> >madv(), and subclassing the struct drm_mm_node to hold our extra
> >details.
> 
> Should we add a separate purgeable list for stolen objects ?
> 
> 
> /** Stolen memory for this object, instead of being backed by shmem. */
> - struct drm_mm_node *stolen;
> + struct i915_gem_stolen *stolen;
> 
> 
> struct i915_gem_stolen {
> 	struct drm_mm_node *node;
> 	struct list_head purge_list;
> };

Almost. You will need a backpointer from the node to the object so you
can do your list iteration and purge the unwanted object.
http://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=nightly&id=1094f92e6d94190cf1334fd9bd6459ab70801455
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Goel, Akash]

On 7/31/2015 8:36 PM, Chris Wilson wrote:
> On Fri, Jul 31, 2015 at 08:12:30PM +0530, Goel, Akash wrote:
>>
>>
>> On 7/29/2015 5:34 PM, Chris Wilson wrote:
>>> On Mon, Jul 27, 2015 at 11:38:13AM +0200, Daniel Vetter wrote:
>>>> Chris and I just discussed on irc that the bound_list isn't in a great LRU
>>>> order right now and Chris sent out a fix for that. But it only works if we
>>>> preferrentially shrink inactive objects first. Worth the bother or just a
>>>> FIXME? For the fb use-case alone it's not needed since we can't remove the
>>>> fb until it's no longer being displayed (otherwise the backwards-compat
>>>> code kicks in and synchronously kills the display at RMFB time), and that
>>>> pretty much means we can't put the underlying bo into any cache (and mark
>>>> it purgeable) either. But a FIXME comment here would be good for sure,
>>>> just in case this assumption ever gets broken.
>>>
>>> I've been mucking around with patch a bit (with contexts-from-stolen
>>> reenabled) and the list ierators used here are terrible; severely
>>> impacting our allocations by a few orders of magnitude (imagine having
>>> just the ggtt full of 4k objects, let alone several ppgtt all full of
>>> their own bound 4k objetcs).
>>>
>>> To combat this will require a special purgeable list maintaind by
>>> madv(), and subclassing the struct drm_mm_node to hold our extra
>>> details.
>>
>> Should we add a separate purgeable list for stolen objects ?
>>
>>
>> /** Stolen memory for this object, instead of being backed by shmem. */
>> - struct drm_mm_node *stolen;
>> + struct i915_gem_stolen *stolen;
>>
>>
>> struct i915_gem_stolen {
>> 	struct drm_mm_node *node;
>> 	struct list_head purge_list;
>> };
>
> Almost. You will need a backpointer from the node to the object so you
> can do your list iteration and purge the unwanted object.
Agree that a back pointer is also needed, as the stolen structure will 
not be embedded in the object structure. Thanks.

> http://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=nightly&id=1094f92e6d94190cf1334fd9bd6459ab70801455

More thanks for providing the reference implementation.

Best regards
Akash
> -Chris
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915: Support for pread/pwrite from/to non shmem backed objects

[Ankitprasad Sharma]

On Fri, 2015-07-31 at 18:46 +0530, Goel, Akash wrote:
> 
> On 7/22/2015 8:09 PM, Chris Wilson wrote:
> > On Wed, Jul 22, 2015 at 07:21:49PM +0530, ankitprasad.r.sharma@intel.com wrote:
> >>   static int
> >>   i915_gem_shmem_pread(struct drm_device *dev,
> >>   		     struct drm_i915_gem_object *obj,
> >> @@ -754,17 +850,20 @@ i915_gem_pread_ioctl(struct drm_device *dev, void *data,
> >>   		goto out;
> >>   	}
> >>
> >> -	/* prime objects have no backing filp to GEM pread/pwrite
> >> -	 * pages from.
> >> -	 */
> >> -	if (!obj->base.filp) {
> >> -		ret = -EINVAL;
> >> -		goto out;
> >> -	}
> >> -
> >>   	trace_i915_gem_object_pread(obj, args->offset, args->size);
> >>
> >> -	ret = i915_gem_shmem_pread(dev, obj, args, file);
> >> +	/* pread for non shmem backed objects */
> >> +	if (!obj->base.filp) {
> >> +		if (obj->tiling_mode == I915_TILING_NONE)
> >
> > pread/pwrite is defined as a cpu linear, the restriction upon tiling is
> > a simplification of handling swizzling.
> >
> >> +			ret = i915_gem_gtt_pread_pwrite(dev, obj, args->size,
> >> +							args->offset,
> >> +							args->data_ptr,
> >> +							false);
> >> +		else
> >> +			ret = -EINVAL;
> >> +	} else {
> >> +		ret = i915_gem_shmem_pread(dev, obj, args, file);
> >> +	}
> >>
> >>   out:
> >>   	drm_gem_object_unreference(&obj->base);
> >> @@ -1105,17 +1204,22 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
> >>   		goto out;
> >>   	}
> >>
> >> -	/* prime objects have no backing filp to GEM pread/pwrite
> >> -	 * pages from.
> >> -	 */
> >> -	if (!obj->base.filp) {
> >> -		ret = -EINVAL;
> >> -		goto out;
> >> -	}
> >> -
> >>   	trace_i915_gem_object_pwrite(obj, args->offset, args->size);
> >>
> >>   	ret = -EFAULT;
> >> +
> >> +	/* pwrite for non shmem backed objects */
> >> +	if (!obj->base.filp) {
> >> +		if (obj->tiling_mode == I915_TILING_NONE)
> >> +			ret = i915_gem_gtt_pread_pwrite(dev, obj, args->size,
> >> +							args->offset,
> >> +							args->data_ptr,
> >> +							true);
> >> +		else
> >> +			ret = -EINVAL;
> >> +
> >> +		goto out;
> >
> > The fast pwrite code always works for obj->base.filp == NULL. To easily
> > make it generic and handle the cases where we cannot fallback to shem,
> > undo the PIN_NONBLOCK.
> >
> > Then you just want something like
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index d3016f37cd4d..f2284a27dd6d 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -1114,8 +1114,9 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
> >           * perspective, requiring manual detiling by the client.
> >           */
> >          if (obj->tiling_mode == I915_TILING_NONE &&
> 
> Since the suggestion is to reuse the gtt_pwrite_fast function only for 
> non-shmem backed objects, can we also relax the Tiling constraint here, 
> apart from removing the PIN_NONBLOCK flag. As anyways there is a 
> put_fence already being done in gtt_pwrite_fast function.
> 
> Also currently the gtt_pwrite_fast function is non-tolerant to faults, 
> incurred while accessing the User space buffer, so can that also be 
> relaxed (at least for the non-shmem backed objects) ??

Even if we reuse the gtt_pwrite_fast we will have to handle page-faults
for non-shmem backed objects, that will contradict the purpose of
gtt_pwrite_fast. The gtt_pread_pwrite will still be around for pread
purpose.

Also, I think it should be ok to relax the tiling constraint as well, as
we put_fence everytime we try to pread/pwrite from/to a non-shmem-backed
object here.

Thanks,
Ankit

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

Re: [PATCH 4/4] drm/i915: Support for pread/pwrite from/to non shmem backed objects

[Chris Wilson]

On Fri, Jul 31, 2015 at 06:46:20PM +0530, Goel, Akash wrote:
> Since the suggestion is to reuse the gtt_pwrite_fast function only
> for non-shmem backed objects, can we also relax the Tiling
> constraint here, apart from removing the PIN_NONBLOCK flag. As
> anyways there is a put_fence already being done in gtt_pwrite_fast
> function.

The tiling restraint is due to a hardware limitation (the effect of
swizzling), you cannot simply drop it.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tvrtko Ursulin]

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


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

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

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ankitprasad Sharma]

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

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

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

[Tvrtko Ursulin]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[ankitprasad.r.sharma]

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);
 	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:
-- 
1.9.1

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

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

[Tvrtko Ursulin]

On 07/01/2015 10:25 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)
>
> testcase: igt/gem_stolen
>

Tidy "Testcase:" tag again.

> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_gem_stolen.c | 121 ++++++++++++++++++++++++++++++---
>   1 file changed, 110 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index 348ed5a..7e216be 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -430,18 +430,29 @@ cleanup:
>   	return NULL;
>   }
>
> -struct drm_i915_gem_object *
> -i915_gem_object_create_stolen(struct drm_device *dev, u32 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 (i915_gem_obj_is_pinned(obj))
> +		return false;
> +
> +	list_add(&obj->obj_exec_link, unwind);
> +	return drm_mm_scan_add_block(obj->stolen);
> +}
> +
> +static struct drm_mm_node *
> +stolen_alloc(struct drm_i915_private *dev_priv, u32 size)
>   {
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct drm_i915_gem_object *obj;
>   	struct drm_mm_node *stolen;
> +	struct drm_i915_gem_object *obj;
> +	struct list_head unwind, evict;
>   	int ret;
>
> -	if (!drm_mm_initialized(&dev_priv->mm.stolen))
> -		return NULL;
> -
> -	DRM_DEBUG_KMS("creating stolen object: size=%x\n", size);
>   	if (size == 0)
>   		return NULL;
>
> @@ -451,11 +462,99 @@ i915_gem_object_create_stolen(struct drm_device *dev, u32 size)
>
>   	ret = drm_mm_insert_node(&dev_priv->mm.stolen, stolen, size,
>   				 4096, DRM_MM_SEARCH_DEFAULT);
> -	if (ret) {
> -		kfree(stolen);
> -		return NULL;
> +	if (ret == 0)
> +		return stolen;
> +
> +	/* No more stolen memory available, or too fragmented.
> +	 * Try evicting purgeable objects and search again.
> +	 */
> +
> +	drm_mm_init_scan(&dev_priv->mm.stolen, size, 4096, 0);
> +	INIT_LIST_HEAD(&unwind);
> +
> +	list_for_each_entry(obj, &dev_priv->mm.unbound_list, global_list)
> +		if (mark_free(obj, &unwind))
> +			goto found;
> +
> +	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list)
> +		if (mark_free(obj, &unwind))
> +			goto found;
> +
> +found:
> +	INIT_LIST_HEAD(&evict);
> +	while (!list_empty(&unwind)) {
> +		obj = list_first_entry(&unwind,
> +				       struct drm_i915_gem_object,
> +				       obj_exec_link);
> +		list_del_init(&obj->obj_exec_link);
> +
> +		if (drm_mm_scan_remove_block(obj->stolen)) {
> +			list_add(&obj->obj_exec_link, &evict);
> +			drm_gem_object_reference(&obj->base);
> +		}
>   	}
>
> +	ret = 0;
> +	while (!list_empty(&evict)) {
> +		obj = list_first_entry(&evict,
> +				       struct drm_i915_gem_object,
> +				       obj_exec_link);
> +		list_del_init(&obj->obj_exec_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);

Both blocks need curly braces if one has them.

With these two tiny details fixed,

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

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

[ankitprasad.r.sharma]

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)

testcase: igt/gem_stolen

Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_stolen.c | 121 ++++++++++++++++++++++++++++++---
 1 file changed, 110 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 348ed5a..7e216be 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -430,18 +430,29 @@ cleanup:
 	return NULL;
 }
 
-struct drm_i915_gem_object *
-i915_gem_object_create_stolen(struct drm_device *dev, u32 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 (i915_gem_obj_is_pinned(obj))
+		return false;
+
+	list_add(&obj->obj_exec_link, unwind);
+	return drm_mm_scan_add_block(obj->stolen);
+}
+
+static struct drm_mm_node *
+stolen_alloc(struct drm_i915_private *dev_priv, u32 size)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_i915_gem_object *obj;
 	struct drm_mm_node *stolen;
+	struct drm_i915_gem_object *obj;
+	struct list_head unwind, evict;
 	int ret;
 
-	if (!drm_mm_initialized(&dev_priv->mm.stolen))
-		return NULL;
-
-	DRM_DEBUG_KMS("creating stolen object: size=%x\n", size);
 	if (size == 0)
 		return NULL;
 
@@ -451,11 +462,99 @@ i915_gem_object_create_stolen(struct drm_device *dev, u32 size)
 
 	ret = drm_mm_insert_node(&dev_priv->mm.stolen, stolen, size,
 				 4096, DRM_MM_SEARCH_DEFAULT);
-	if (ret) {
-		kfree(stolen);
-		return NULL;
+	if (ret == 0)
+		return stolen;
+
+	/* No more stolen memory available, or too fragmented.
+	 * Try evicting purgeable objects and search again.
+	 */
+
+	drm_mm_init_scan(&dev_priv->mm.stolen, size, 4096, 0);
+	INIT_LIST_HEAD(&unwind);
+
+	list_for_each_entry(obj, &dev_priv->mm.unbound_list, global_list)
+		if (mark_free(obj, &unwind))
+			goto found;
+
+	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list)
+		if (mark_free(obj, &unwind))
+			goto found;
+
+found:
+	INIT_LIST_HEAD(&evict);
+	while (!list_empty(&unwind)) {
+		obj = list_first_entry(&unwind,
+				       struct drm_i915_gem_object,
+				       obj_exec_link);
+		list_del_init(&obj->obj_exec_link);
+
+		if (drm_mm_scan_remove_block(obj->stolen)) {
+			list_add(&obj->obj_exec_link, &evict);
+			drm_gem_object_reference(&obj->base);
+		}
 	}
 
+	ret = 0;
+	while (!list_empty(&evict)) {
+		obj = list_first_entry(&evict,
+				       struct drm_i915_gem_object,
+				       obj_exec_link);
+		list_del_init(&obj->obj_exec_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);
+	}
+
+	if (ret == 0)
+		ret = drm_mm_insert_node(&dev_priv->mm.stolen, stolen, size,
+					 4096, DRM_MM_SEARCH_DEFAULT);
+	if (ret == 0)
+		return stolen;
+
+	kfree(stolen);
+	return NULL;
+}
+
+struct drm_i915_gem_object *
+i915_gem_object_create_stolen(struct drm_device *dev, u32 size)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_gem_object *obj;
+	struct drm_mm_node *stolen;
+
+	lockdep_assert_held(&dev->struct_mutex);
+
+	if (!drm_mm_initialized(&dev_priv->mm.stolen))
+		return NULL;
+
+	DRM_DEBUG_KMS("creating stolen object: size=%x\n", size);
+
+	stolen = stolen_alloc(dev_priv, size);
+	if (stolen == NULL)
+		return NULL;
+
 	obj = _i915_gem_object_create_stolen(dev, stolen);
 	if (obj)
 		return obj;
-- 
1.9.1

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

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

[ankitprasad.r.sharma]

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.

testcase: igt/gem_create_stolen

Signed-off-by: Chris Wilson <chri@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_stolen.c | 121 ++++++++++++++++++++++++++++++---
 1 file changed, 110 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index f8da716..0a38d71 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -430,18 +430,29 @@ cleanup:
 	return NULL;
 }
 
-struct drm_i915_gem_object *
-i915_gem_object_create_stolen(struct drm_device *dev, u32 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 (i915_gem_obj_is_pinned(obj))
+		return false;
+
+	list_add(&obj->obj_exec_link, unwind);
+	return drm_mm_scan_add_block(obj->stolen);
+}
+
+static struct drm_mm_node *
+stolen_alloc(struct drm_i915_private *dev_priv, u32 size)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_i915_gem_object *obj;
 	struct drm_mm_node *stolen;
+	struct drm_i915_gem_object *obj;
+	struct list_head unwind, evict;
 	int ret;
 
-	if (!drm_mm_initialized(&dev_priv->mm.stolen))
-		return NULL;
-
-	DRM_DEBUG_KMS("creating stolen object: size=%x\n", size);
 	if (size == 0)
 		return NULL;
 
@@ -451,11 +462,99 @@ i915_gem_object_create_stolen(struct drm_device *dev, u32 size)
 
 	ret = drm_mm_insert_node(&dev_priv->mm.stolen, stolen, size,
 				 4096, DRM_MM_SEARCH_DEFAULT);
-	if (ret) {
-		kfree(stolen);
-		return NULL;
+	if (ret == 0)
+		return stolen;
+
+	/* No more stolen memory available, or too fragmented.
+	 * Try evicting purgeable objects and search again.
+	 */
+
+	drm_mm_init_scan(&dev_priv->mm.stolen, size, 4096, 0);
+	INIT_LIST_HEAD(&unwind);
+
+	list_for_each_entry(obj, &dev_priv->mm.unbound_list, global_list)
+		if (mark_free(obj, &unwind))
+			goto found;
+
+	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list)
+		if (mark_free(obj, &unwind))
+			goto found;
+
+found:
+	INIT_LIST_HEAD(&evict);
+	while (!list_empty(&unwind)) {
+		obj = list_first_entry(&unwind,
+				       struct drm_i915_gem_object,
+				       obj_exec_link);
+		list_del_init(&obj->obj_exec_link);
+
+		if (drm_mm_scan_remove_block(obj->stolen)) {
+			list_add(&obj->obj_exec_link, &evict);
+			drm_gem_object_reference(&obj->base);
+		}
 	}
 
+	ret = 0;
+	while (!list_empty(&evict)) {
+		obj = list_first_entry(&evict,
+				       struct drm_i915_gem_object,
+				       obj_exec_link);
+		list_del_init(&obj->obj_exec_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);
+	}
+
+	if (ret == 0)
+		ret = drm_mm_insert_node(&dev_priv->mm.stolen, stolen, size,
+					 4096, DRM_MM_SEARCH_DEFAULT);
+	if (ret == 0)
+		return stolen;
+
+	kfree(stolen);
+	return NULL;
+}
+
+struct drm_i915_gem_object *
+i915_gem_object_create_stolen(struct drm_device *dev, u32 size)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_gem_object *obj;
+	struct drm_mm_node *stolen;
+
+	lockdep_assert_held(&dev->struct_mutex);
+
+	if (!drm_mm_initialized(&dev_priv->mm.stolen))
+		return NULL;
+
+	DRM_DEBUG_KMS("creating stolen object: size=%x\n", size);
+
+	stolen = stolen_alloc(dev_priv, size);
+	if (stolen == NULL)
+		return NULL;
+
 	obj = _i915_gem_object_create_stolen(dev, stolen);
 	if (obj)
 		return obj;
-- 
1.9.1

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