[PATCH 1/6] intel: Defer setting madv on the bo cache

[PATCH 1/6] intel: Defer setting madv on the bo cache

[Chris Wilson]

Convert the bo-cache into 2 phases:

1. A two second cache of recently used buffers, keep untouched.
2. A two second cache of older buffers, marked for eviction.

This helps reduce ioctl traffic on a rapid turnover in working sets. The
issue is that even a 2 second window is enough for an application to
fill all of memory with inactive buffers (and we would rely on the
oom-killer identifying the right culprit).

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Ben Widawsky <benjamin.widawsky@intel.com>
---
 intel/intel_bufmgr_gem.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
index 60c06fc..eeb9acf 100644
--- a/intel/intel_bufmgr_gem.c
+++ b/intel/intel_bufmgr_gem.c
@@ -216,6 +216,11 @@ struct _drm_intel_bo_gem {
 	bool used_as_reloc_target;
 
 	/**
+	 * Are we keeping this buffer around in semi-permenant cache?
+	 */
+	bool dontneed;
+
+	/**
 	 * Boolean of whether we have encountered an error whilst building the relocation tree.
 	 */
 	bool has_error;
@@ -719,7 +724,8 @@ retry:
 		}
 
 		if (alloc_from_cache) {
-			if (!drm_intel_gem_bo_madvise_internal
+			if (bo_gem->dontneed &&
+			    !drm_intel_gem_bo_madvise_internal
 			    (bufmgr_gem, bo_gem, I915_MADV_WILLNEED)) {
 				drm_intel_gem_bo_free(&bo_gem->bo);
 				drm_intel_gem_bo_cache_purge_bucket(bufmgr_gem,
@@ -1166,19 +1172,28 @@ drm_intel_gem_cleanup_bo_cache(drm_intel_bufmgr_gem *bufmgr_gem, time_t time)
 	for (i = 0; i < bufmgr_gem->num_buckets; i++) {
 		struct drm_intel_gem_bo_bucket *bucket =
 		    &bufmgr_gem->cache_bucket[i];
+		drmMMListHead madv;
 
+		DRMINITLISTHEAD(&madv);
 		while (!DRMLISTEMPTY(&bucket->head)) {
 			drm_intel_bo_gem *bo_gem;
 
 			bo_gem = DRMLISTENTRY(drm_intel_bo_gem,
 					      bucket->head.next, head);
-			if (time - bo_gem->free_time <= 1)
+			if (time - bo_gem->free_time < 2*(1+bo_gem->dontneed))
 				break;
 
 			DRMLISTDEL(&bo_gem->head);
-
-			drm_intel_gem_bo_free(&bo_gem->bo);
+			if (bo_gem->dontneed) {
+				drm_intel_gem_bo_free(&bo_gem->bo);
+			} else {
+				drm_intel_gem_bo_madvise_internal(bufmgr_gem, bo_gem,
+								  I915_MADV_DONTNEED);
+				bo_gem->dontneed = 1;
+				DRMLISTADDTAIL(&bo_gem->head, &madv);
+			}
 		}
+		DRMLISTJOIN(&madv, &bucket->head);
 	}
 
 	bufmgr_gem->time = time;
@@ -1289,9 +1304,7 @@ drm_intel_gem_bo_unreference_final(drm_intel_bo *bo, time_t time)
 
 	bucket = drm_intel_gem_bo_bucket_for_size(bufmgr_gem, bo->size);
 	/* Put the buffer into our internal cache for reuse if we can. */
-	if (bufmgr_gem->bo_reuse && bo_gem->reusable && bucket != NULL &&
-	    drm_intel_gem_bo_madvise_internal(bufmgr_gem, bo_gem,
-					      I915_MADV_DONTNEED)) {
+	if (bufmgr_gem->bo_reuse && bo_gem->reusable && bucket != NULL) {
 		bo_gem->free_time = time;
 
 		bo_gem->name = NULL;
-- 
2.1.4

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

[PATCH 2/6] intel: Use WAIT for wait-rendering

[Chris Wilson]

Using WAIT is preferrable to SET_DOMAIN as it doesn't have
any domain management side-effects - but has the same flushing
semantics.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 intel/intel_bufmgr_gem.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
index eeb9acf..43cbae5 100644
--- a/intel/intel_bufmgr_gem.c
+++ b/intel/intel_bufmgr_gem.c
@@ -1729,6 +1729,24 @@ drm_intel_gem_bo_get_subdata(drm_intel_bo *bo, unsigned long offset,
 static void
 drm_intel_gem_bo_wait_rendering(drm_intel_bo *bo)
 {
+	drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *) bo->bufmgr;
+
+	/* Using WAIT is preferrable to SET_DOMAIN as it doesn't have
+	 * any domain management side-effects - but has the same flushing
+	 * semantics.
+	 */
+	if (bufmgr_gem->has_wait_timeout) {
+		struct drm_i915_gem_wait wait;
+
+		memclear(wait);
+		wait.bo_handle = bo->handle;
+		wait.timeout_ns = -1;
+		if (drmIoctl(bufmgr_gem->fd,
+			     DRM_IOCTL_I915_GEM_WAIT,
+			     &wait) == 0)
+			return;
+	}
+
 	drm_intel_gem_bo_start_gtt_access(bo, 1);
 }
 
-- 
2.1.4

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

[PATCH 3/6] intel: Export raw GEM mmap interfaces

[Chris Wilson]

Export a set of interfaces to allow the caller to have precise control
over mapping the buffer - but still provide caching of the mmaps between
callers.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 intel/intel_bufmgr.h     |   4 ++
 intel/intel_bufmgr_gem.c | 148 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 152 insertions(+)

diff --git a/intel/intel_bufmgr.h b/intel/intel_bufmgr.h
index 285919e..f35c59b 100644
--- a/intel/intel_bufmgr.h
+++ b/intel/intel_bufmgr.h
@@ -178,6 +178,10 @@ int drm_intel_gem_bo_map_unsynchronized(drm_intel_bo *bo);
 int drm_intel_gem_bo_map_gtt(drm_intel_bo *bo);
 int drm_intel_gem_bo_unmap_gtt(drm_intel_bo *bo);
 
+void *drm_intel_gem_bo_map__cpu(drm_intel_bo *bo);
+void *drm_intel_gem_bo_map__gtt(drm_intel_bo *bo);
+void *drm_intel_gem_bo_map__wc(drm_intel_bo *bo);
+
 int drm_intel_gem_bo_get_reloc_count(drm_intel_bo *bo);
 void drm_intel_gem_bo_clear_relocs(drm_intel_bo *bo, int start);
 void drm_intel_gem_bo_start_gtt_access(drm_intel_bo *bo, int write_enable);
diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
index 43cbae5..61a248f 100644
--- a/intel/intel_bufmgr_gem.c
+++ b/intel/intel_bufmgr_gem.c
@@ -191,6 +191,8 @@ struct _drm_intel_bo_gem {
 	void *mem_virtual;
 	/** GTT virtual address for the buffer, saved across map/unmap cycles */
 	void *gtt_virtual;
+	/** WC CPU address for the buffer, saved across map/unmap cycles */
+	void *wc_virtual;
 	/**
 	 * Virtual address of the buffer allocated by user, used for userptr
 	 * objects only.
@@ -1129,6 +1131,11 @@ drm_intel_gem_bo_free(drm_intel_bo *bo)
 		drm_munmap(bo_gem->mem_virtual, bo_gem->bo.size);
 		bufmgr_gem->vma_count--;
 	}
+	if (bo_gem->wc_virtual) {
+		VG(VALGRIND_FREELIKE_BLOCK(bo_gem->wc_virtual, 0));
+		drm_munmap(bo_gem->wc_virtual, bo_gem->bo.size);
+		bufmgr_gem->vma_count--;
+	}
 	if (bo_gem->gtt_virtual) {
 		drm_munmap(bo_gem->gtt_virtual, bo_gem->bo.size);
 		bufmgr_gem->vma_count--;
@@ -1155,6 +1162,9 @@ drm_intel_gem_bo_mark_mmaps_incoherent(drm_intel_bo *bo)
 	if (bo_gem->mem_virtual)
 		VALGRIND_MAKE_MEM_NOACCESS(bo_gem->mem_virtual, bo->size);
 
+	if (bo_gem->wc_virtual)
+		VALGRIND_MAKE_MEM_NOACCESS(bo_gem->wc_virtual, bo->size);
+
 	if (bo_gem->gtt_virtual)
 		VALGRIND_MAKE_MEM_NOACCESS(bo_gem->gtt_virtual, bo->size);
 #endif
@@ -1228,6 +1238,11 @@ static void drm_intel_gem_bo_purge_vma_cache(drm_intel_bufmgr_gem *bufmgr_gem)
 			bo_gem->mem_virtual = NULL;
 			bufmgr_gem->vma_count--;
 		}
+		if (bo_gem->wc_virtual) {
+			drm_munmap(bo_gem->wc_virtual, bo_gem->bo.size);
+			bo_gem->wc_virtual = NULL;
+			bufmgr_gem->vma_count--;
+		}
 		if (bo_gem->gtt_virtual) {
 			drm_munmap(bo_gem->gtt_virtual, bo_gem->bo.size);
 			bo_gem->gtt_virtual = NULL;
@@ -1243,6 +1258,8 @@ static void drm_intel_gem_bo_close_vma(drm_intel_bufmgr_gem *bufmgr_gem,
 	DRMLISTADDTAIL(&bo_gem->vma_list, &bufmgr_gem->vma_cache);
 	if (bo_gem->mem_virtual)
 		bufmgr_gem->vma_count++;
+	if (bo_gem->wc_virtual)
+		bufmgr_gem->vma_count++;
 	if (bo_gem->gtt_virtual)
 		bufmgr_gem->vma_count++;
 	drm_intel_gem_bo_purge_vma_cache(bufmgr_gem);
@@ -1255,6 +1272,8 @@ static void drm_intel_gem_bo_open_vma(drm_intel_bufmgr_gem *bufmgr_gem,
 	DRMLISTDEL(&bo_gem->vma_list);
 	if (bo_gem->mem_virtual)
 		bufmgr_gem->vma_count--;
+	if (bo_gem->wc_virtual)
+		bufmgr_gem->vma_count--;
 	if (bo_gem->gtt_virtual)
 		bufmgr_gem->vma_count--;
 	drm_intel_gem_bo_purge_vma_cache(bufmgr_gem);
@@ -3516,6 +3535,135 @@ drm_intel_bufmgr_gem_unref(drm_intel_bufmgr *bufmgr)
 	}
 }
 
+void *drm_intel_gem_bo_map__gtt(drm_intel_bo *bo)
+{
+	drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *) bo->bufmgr;
+	drm_intel_bo_gem *bo_gem = (drm_intel_bo_gem *) bo;
+
+	if (bo_gem->is_userptr)
+		return NULL;
+
+	pthread_mutex_lock(&bufmgr_gem->lock);
+	if (bo_gem->map_count++ == 0)
+		drm_intel_gem_bo_open_vma(bufmgr_gem, bo_gem);
+
+	if (bo_gem->gtt_virtual == NULL) {
+		struct drm_i915_gem_mmap_gtt mmap_arg;
+
+		DBG("bo_map_gtt: mmap %d (%s), map_count=%d\n",
+		    bo_gem->gem_handle, bo_gem->name, bo_gem->map_count);
+
+		memclear(mmap_arg);
+		mmap_arg.handle = bo_gem->gem_handle;
+
+		/* Get the fake offset back... */
+		bo_gem->gtt_virtual = MAP_FAILED;
+		if (drmIoctl(bufmgr_gem->fd,
+			     DRM_IOCTL_I915_GEM_MMAP_GTT,
+			     &mmap_arg) == 0) {
+			/* and mmap it */
+			bo_gem->gtt_virtual = drm_mmap(0, bo->size, PROT_READ | PROT_WRITE,
+						       MAP_SHARED, bufmgr_gem->fd,
+						       mmap_arg.offset);
+		}
+		if (bo_gem->gtt_virtual == MAP_FAILED) {
+			if (--bo_gem->map_count == 0)
+				drm_intel_gem_bo_close_vma(bufmgr_gem, bo_gem);
+			bo_gem->gtt_virtual = NULL;
+		}
+	}
+	pthread_mutex_unlock(&bufmgr_gem->lock);
+
+	return bo_gem->gtt_virtual;
+}
+
+void *drm_intel_gem_bo_map__cpu(drm_intel_bo *bo)
+{
+	drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *) bo->bufmgr;
+	drm_intel_bo_gem *bo_gem = (drm_intel_bo_gem *) bo;
+
+	if (bo_gem->is_userptr) {
+		/* Return the same user ptr */
+		return bo_gem->user_virtual;
+	}
+
+	pthread_mutex_lock(&bufmgr_gem->lock);
+
+	if (bo_gem->map_count++ == 0)
+		drm_intel_gem_bo_open_vma(bufmgr_gem, bo_gem);
+
+	if (!bo_gem->mem_virtual) {
+		struct drm_i915_gem_mmap mmap_arg;
+
+		DBG("bo_map: %d (%s), map_count=%d\n",
+		    bo_gem->gem_handle, bo_gem->name, bo_gem->map_count);
+
+		memclear(mmap_arg);
+		mmap_arg.handle = bo_gem->gem_handle;
+		mmap_arg.size = bo->size;
+		if (drmIoctl(bufmgr_gem->fd,
+			     DRM_IOCTL_I915_GEM_MMAP,
+			     &mmap_arg)) {
+			DBG("%s:%d: Error mapping buffer %d (%s): %s .\n",
+			    __FILE__, __LINE__, bo_gem->gem_handle,
+			    bo_gem->name, strerror(errno));
+			if (--bo_gem->map_count == 0)
+				drm_intel_gem_bo_close_vma(bufmgr_gem, bo_gem);
+		} else {
+			VG(VALGRIND_MALLOCLIKE_BLOCK(mmap_arg.addr_ptr, mmap_arg.size, 0, 1));
+			bo_gem->mem_virtual = (void *)(uintptr_t) mmap_arg.addr_ptr;
+		}
+	}
+	DBG("bo_map: %d (%s) -> %p\n", bo_gem->gem_handle, bo_gem->name,
+	    bo_gem->mem_virtual);
+	pthread_mutex_unlock(&bufmgr_gem->lock);
+
+	return bo_gem->mem_virtual;
+}
+
+void *drm_intel_gem_bo_map__wc(drm_intel_bo *bo)
+{
+	drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *) bo->bufmgr;
+	drm_intel_bo_gem *bo_gem = (drm_intel_bo_gem *) bo;
+
+	if (bo_gem->is_userptr)
+		return NULL;
+
+	pthread_mutex_lock(&bufmgr_gem->lock);
+
+	if (bo_gem->map_count++ == 0)
+		drm_intel_gem_bo_open_vma(bufmgr_gem, bo_gem);
+
+	if (!bo_gem->wc_virtual) {
+		struct drm_i915_gem_mmap mmap_arg;
+
+		DBG("bo_map: %d (%s), map_count=%d\n",
+		    bo_gem->gem_handle, bo_gem->name, bo_gem->map_count);
+
+		memclear(mmap_arg);
+		mmap_arg.handle = bo_gem->gem_handle;
+		mmap_arg.size = bo->size;
+		mmap_arg.flags = I915_MMAP_WC;
+		if (drmIoctl(bufmgr_gem->fd,
+			     DRM_IOCTL_I915_GEM_MMAP,
+			     &mmap_arg)) {
+			DBG("%s:%d: Error mapping buffer %d (%s): %s .\n",
+			    __FILE__, __LINE__, bo_gem->gem_handle,
+			    bo_gem->name, strerror(errno));
+			if (--bo_gem->map_count == 0)
+				drm_intel_gem_bo_close_vma(bufmgr_gem, bo_gem);
+		} else {
+			VG(VALGRIND_MALLOCLIKE_BLOCK(mmap_arg.addr_ptr, mmap_arg.size, 0, 1));
+			bo_gem->wc_virtual = (void *)(uintptr_t) mmap_arg.addr_ptr;
+		}
+	}
+	DBG("bo_map: %d (%s) -> %p\n", bo_gem->gem_handle, bo_gem->name,
+	    bo_gem->wc_virtual);
+	pthread_mutex_unlock(&bufmgr_gem->lock);
+
+	return bo_gem->wc_virtual;
+}
+
 /**
  * Initializes the GEM buffer manager, which uses the kernel to allocate, map,
  * and manage map buffer objections.
-- 
2.1.4

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

[PATCH 4/6] intel: Keep the caches in loose active order

[Chris Wilson]

Though we don't have precise tracking for when buffers in the cache list
become idle (we don't have request tracking at this layer), we do have
some basic assumptions that the head is inactive and the tail active. So
when inserting buffers in the cache, it would be good not to completely
ignore those assumptions.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 intel/intel_bufmgr_gem.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
index 61a248f..2cbe973 100644
--- a/intel/intel_bufmgr_gem.c
+++ b/intel/intel_bufmgr_gem.c
@@ -1329,7 +1329,10 @@ drm_intel_gem_bo_unreference_final(drm_intel_bo *bo, time_t time)
 		bo_gem->name = NULL;
 		bo_gem->validate_index = -1;
 
-		DRMLISTADDTAIL(&bo_gem->head, &bucket->head);
+		if (drm_intel_gem_bo_busy(&bo_gem->bo))
+			DRMLISTADDTAIL(&bo_gem->head, &bucket->head);
+		else
+			DRMLISTADD(&bo_gem->head, &bucket->head);
 	} else {
 		drm_intel_gem_bo_free(bo);
 	}
-- 
2.1.4

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

[PATCH 5/6] intel: Round large allocations upto PAGE_SIZE

[Chris Wilson]

The kernel interface requires us to request only PAGE_SIZE aligned
buffers, so make sure that all allocation requests are indeed sized
correctly (and in the process remove the excess calls to getpagesize()).

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 intel/intel_bufmgr_gem.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
index 2cbe973..22f7501 100644
--- a/intel/intel_bufmgr_gem.c
+++ b/intel/intel_bufmgr_gem.c
@@ -671,15 +671,10 @@ drm_intel_gem_bo_alloc_internal(drm_intel_bufmgr *bufmgr,
 {
 	drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *) bufmgr;
 	drm_intel_bo_gem *bo_gem;
-	unsigned int page_size = getpagesize();
-	int ret;
 	struct drm_intel_gem_bo_bucket *bucket;
 	bool alloc_from_cache;
 	unsigned long bo_size;
-	bool for_render = false;
-
-	if (flags & BO_ALLOC_FOR_RENDER)
-		for_render = true;
+	int ret;
 
 	/* Round the allocated size up to a power of two number of pages. */
 	bucket = drm_intel_gem_bo_bucket_for_size(bufmgr_gem, size);
@@ -688,9 +683,8 @@ drm_intel_gem_bo_alloc_internal(drm_intel_bufmgr *bufmgr,
 	 * allocation up.
 	 */
 	if (bucket == NULL) {
-		bo_size = size;
-		if (bo_size < page_size)
-			bo_size = page_size;
+		unsigned int page_size = getpagesize();
+		bo_size = ALIGN(size, page_size);
 	} else {
 		bo_size = bucket->size;
 	}
@@ -700,7 +694,7 @@ drm_intel_gem_bo_alloc_internal(drm_intel_bufmgr *bufmgr,
 retry:
 	alloc_from_cache = false;
 	if (bucket != NULL && !DRMLISTEMPTY(&bucket->head)) {
-		if (for_render) {
+		if (flags & BO_ALLOC_FOR_RENDER) {
 			/* Allocate new render-target BOs from the tail (MRU)
 			 * of the list, as it will likely be hot in the GPU
 			 * cache and in the aperture for us.
-- 
2.1.4

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

[PATCH 6/6] intel: Keep live buffers in the cache

[Chris Wilson]

If the GPU is still processing the buffers, then keep them alive as the
pages are still pinned and hot - ripe for reuse.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 intel/intel_bufmgr_gem.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
index 22f7501..53457cd 100644
--- a/intel/intel_bufmgr_gem.c
+++ b/intel/intel_bufmgr_gem.c
@@ -1187,6 +1187,9 @@ drm_intel_gem_cleanup_bo_cache(drm_intel_bufmgr_gem *bufmgr_gem, time_t time)
 			if (time - bo_gem->free_time < 2*(1+bo_gem->dontneed))
 				break;
 
+			if (drm_intel_gem_bo_busy(&bo_gem->bo))
+				break;
+
 			DRMLISTDEL(&bo_gem->head);
 			if (bo_gem->dontneed) {
 				drm_intel_gem_bo_free(&bo_gem->bo);
-- 
2.1.4

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

Re: [PATCH 1/6] intel: Defer setting madv on the bo cache

[Ben Widawsky]

Sorry. I know I asked some questions last time around, but I am having trouble
finding the response.

On Tue, May 05, 2015 at 09:53:55AM +0100, Chris Wilson wrote:
> Convert the bo-cache into 2 phases:
> 
> 1. A two second cache of recently used buffers, keep untouched.
> 2. A two second cache of older buffers, marked for eviction.
> 
> This helps reduce ioctl traffic on a rapid turnover in working sets. The
> issue is that even a 2 second window is enough for an application to
> fill all of memory with inactive buffers (and we would rely on the
> oom-killer identifying the right culprit).
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> ---
>  intel/intel_bufmgr_gem.c | 27 ++++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
> index 60c06fc..eeb9acf 100644
> --- a/intel/intel_bufmgr_gem.c
> +++ b/intel/intel_bufmgr_gem.c
> @@ -216,6 +216,11 @@ struct _drm_intel_bo_gem {
>  	bool used_as_reloc_target;
>  
>  	/**
> +	 * Are we keeping this buffer around in semi-permenant cache?
> +	 */
> +	bool dontneed;
> +
> +	/**
>  	 * Boolean of whether we have encountered an error whilst building the relocation tree.
>  	 */
>  	bool has_error;
> @@ -719,7 +724,8 @@ retry:
>  		}
>  
>  		if (alloc_from_cache) {
> -			if (!drm_intel_gem_bo_madvise_internal
> +			if (bo_gem->dontneed &&
> +			    !drm_intel_gem_bo_madvise_internal

Why wouldn't you set dontneed = false? Just a thought, but it seems like a nicer
solution here would just be a state variable tracking what we last sent with the
madv ioctl.

enum {
  WILLNEED
  DONTNEED
}

With that, it might be cool for the curious to put a histogram/hysteresis thing.
Every time you hit this condition to see how many times we flip flop.

>  			    (bufmgr_gem, bo_gem, I915_MADV_WILLNEED)) {
>  				drm_intel_gem_bo_free(&bo_gem->bo);
>  				drm_intel_gem_bo_cache_purge_bucket(bufmgr_gem,
> @@ -1166,19 +1172,28 @@ drm_intel_gem_cleanup_bo_cache(drm_intel_bufmgr_gem *bufmgr_gem, time_t time)
>  	for (i = 0; i < bufmgr_gem->num_buckets; i++) {
>  		struct drm_intel_gem_bo_bucket *bucket =
>  		    &bufmgr_gem->cache_bucket[i];
> +		drmMMListHead madv;
>  
> +		DRMINITLISTHEAD(&madv);
>  		while (!DRMLISTEMPTY(&bucket->head)) {
>  			drm_intel_bo_gem *bo_gem;
>  
>  			bo_gem = DRMLISTENTRY(drm_intel_bo_gem,
>  					      bucket->head.next, head);
> -			if (time - bo_gem->free_time <= 1)
> +			if (time - bo_gem->free_time < 2*(1+bo_gem->dontneed))
>  				break;
>  
>  			DRMLISTDEL(&bo_gem->head);
> -
> -			drm_intel_gem_bo_free(&bo_gem->bo);
> +			if (bo_gem->dontneed) {
> +				drm_intel_gem_bo_free(&bo_gem->bo);
> +			} else {
> +				drm_intel_gem_bo_madvise_internal(bufmgr_gem, bo_gem,
> +								  I915_MADV_DONTNEED);
> +				bo_gem->dontneed = 1;
> +				DRMLISTADDTAIL(&bo_gem->head, &madv);
> +			}
>  		}

Again with above, how about?
assert(!bo_gem->dontneed);
bo_gem->dontneed = true;

Also I think some comments here explaining what you're doing overall would bce
nice since I surely won't remember it in >= 1 minutes.

> +		DRMLISTJOIN(&madv, &bucket->head);
>  	}
>  
>  	bufmgr_gem->time = time;
> @@ -1289,9 +1304,7 @@ drm_intel_gem_bo_unreference_final(drm_intel_bo *bo, time_t time)
>  
>  	bucket = drm_intel_gem_bo_bucket_for_size(bufmgr_gem, bo->size);
>  	/* Put the buffer into our internal cache for reuse if we can. */
> -	if (bufmgr_gem->bo_reuse && bo_gem->reusable && bucket != NULL &&
> -	    drm_intel_gem_bo_madvise_internal(bufmgr_gem, bo_gem,
> -					      I915_MADV_DONTNEED)) {
> +	if (bufmgr_gem->bo_reuse && bo_gem->reusable && bucket != NULL) {
>  		bo_gem->free_time = time;
>  
>  		bo_gem->name = NULL;

Just to be clear, you're reducing IOCTL traffic by deferring an IOCTL until you
cleanup the BO cache.

I think I said it last time, overall the idea seems fine to me.


-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/6] intel: Defer setting madv on the bo cache

[Chris Wilson]

On Fri, Jun 05, 2015 at 09:34:23AM -0700, Ben Widawsky wrote:
> Sorry. I know I asked some questions last time around, but I am having trouble
> finding the response.

You thought the existing code used a one second timeout and so wanted a
separated patch to convert from age <= 1 to age < 2.

Other than that you should have pointed out that dontneed should be
moved into madvise_internal() so that it is kept in sync with the kernel
value to avoid EFAULT embarrassment.

> Just to be clear, you're reducing IOCTL traffic by deferring an IOCTL until you
> cleanup the BO cache.

Right and thereby reducing ioctls.

> I think I said it last time, overall the idea seems fine to me.

The change is marginal except when there is a lot of mutex contention
(e.g. other clients clflushing) and then the effect is to push the
contention to until it is unavoidable.
-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