[PATCH] intel: Delay testing for userptr until first use

[PATCH] intel: Delay testing for userptr until first use

[Chris Wilson]

Running __mmu_notifier_register() is surprisingly expensive, so let's
not do that unless we have to.

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

diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
index f2f4fea..addf413 100644
--- a/intel/intel_bufmgr_gem.c
+++ b/intel/intel_bufmgr_gem.c
@@ -935,6 +935,70 @@ drm_intel_gem_bo_alloc_userptr(drm_intel_bufmgr *bufmgr,
 	return &bo_gem->bo;
 }
 
+static bool
+has_userptr(drm_intel_bufmgr_gem *bufmgr_gem)
+{
+	int ret;
+	void *ptr;
+	long pgsz;
+	struct drm_i915_gem_userptr userptr;
+	struct drm_gem_close close_bo;
+
+	pgsz = sysconf(_SC_PAGESIZE);
+	assert(pgsz > 0);
+
+	ret = posix_memalign(&ptr, pgsz, pgsz);
+	if (ret) {
+		DBG("Failed to get a page (%ld) for userptr detection!\n",
+			pgsz);
+		return false;
+	}
+
+	memset(&userptr, 0, sizeof(userptr));
+	userptr.user_ptr = (__u64)(unsigned long)ptr;
+	userptr.user_size = pgsz;
+
+retry:
+	ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_I915_GEM_USERPTR, &userptr);
+	if (ret) {
+		if (errno == ENODEV && userptr.flags == 0) {
+			userptr.flags = I915_USERPTR_UNSYNCHRONIZED;
+			goto retry;
+		}
+		free(ptr);
+		return false;
+	}
+
+	close_bo.handle = userptr.handle;
+	ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_GEM_CLOSE, &close_bo);
+	free(ptr);
+	if (ret) {
+		fprintf(stderr, "Failed to release test userptr object! (%d) "
+				"i915 kernel driver may not be sane!\n", errno);
+		return false;
+	}
+
+	return true;
+}
+
+static drm_intel_bo *
+check_bo_alloc_userptr(drm_intel_bufmgr *bufmgr,
+		       const char *name,
+		       void *addr,
+		       uint32_t tiling_mode,
+		       uint32_t stride,
+		       unsigned long size,
+		       unsigned long flags)
+{
+	if (has_userptr((drm_intel_bufmgr_gem *)bufmgr))
+		bufmgr->bo_alloc_userptr = drm_intel_bo_alloc_userptr;
+	else
+		bufmgr->bo_alloc_userptr = NULL;
+
+	return drm_intel_bo_alloc_userptr(bufmgr, name, addr,
+					  tiling_mode, stride, size, flags);
+}
+
 /**
  * Returns a drm_intel_bo wrapping the given buffer object handle.
  *
@@ -3366,52 +3430,6 @@ drm_intel_bufmgr_gem_unref(drm_intel_bufmgr *bufmgr)
 	}
 }
 
-static bool
-has_userptr(drm_intel_bufmgr_gem *bufmgr_gem)
-{
-	int ret;
-	void *ptr;
-	long pgsz;
-	struct drm_i915_gem_userptr userptr;
-	struct drm_gem_close close_bo;
-
-	pgsz = sysconf(_SC_PAGESIZE);
-	assert(pgsz > 0);
-
-	ret = posix_memalign(&ptr, pgsz, pgsz);
-	if (ret) {
-		DBG("Failed to get a page (%ld) for userptr detection!\n",
-			pgsz);
-		return false;
-	}
-
-	memset(&userptr, 0, sizeof(userptr));
-	userptr.user_ptr = (__u64)(unsigned long)ptr;
-	userptr.user_size = pgsz;
-
-retry:
-	ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_I915_GEM_USERPTR, &userptr);
-	if (ret) {
-		if (errno == ENODEV && userptr.flags == 0) {
-			userptr.flags = I915_USERPTR_UNSYNCHRONIZED;
-			goto retry;
-		}
-		free(ptr);
-		return false;
-	}
-
-	close_bo.handle = userptr.handle;
-	ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_GEM_CLOSE, &close_bo);
-	free(ptr);
-	if (ret) {
-		fprintf(stderr, "Failed to release test userptr object! (%d) "
-				"i915 kernel driver may not be sane!\n", errno);
-		return false;
-	}
-
-	return true;
-}
-
 /**
  * Initializes the GEM buffer manager, which uses the kernel to allocate, map,
  * and manage map buffer objections.
@@ -3515,9 +3533,7 @@ drm_intel_bufmgr_gem_init(int fd, int batch_size)
 	ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_I915_GETPARAM, &gp);
 	bufmgr_gem->has_relaxed_fencing = ret == 0;
 
-	if (has_userptr(bufmgr_gem))
-		bufmgr_gem->bufmgr.bo_alloc_userptr =
-			drm_intel_gem_bo_alloc_userptr;
+	bufmgr_gem->bufmgr.bo_alloc_userptr = check_bo_alloc_userptr;
 
 	gp.param = I915_PARAM_HAS_WAIT_TIMEOUT;
 	ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_I915_GETPARAM, &gp);
-- 
2.1.1

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

Re: [PATCH] intel: Delay testing for userptr until first use

[Chris Wilson]

On Tue, Nov 04, 2014 at 02:31:29PM +0000, Chris Wilson wrote:
> +static drm_intel_bo *
> +check_bo_alloc_userptr(drm_intel_bufmgr *bufmgr,
> +		       const char *name,
> +		       void *addr,
> +		       uint32_t tiling_mode,
> +		       uint32_t stride,
> +		       unsigned long size,
> +		       unsigned long flags)
> +{
> +	if (has_userptr((drm_intel_bufmgr_gem *)bufmgr))
> +		bufmgr->bo_alloc_userptr = drm_intel_bo_alloc_userptr;

bufmgr->bo_alloc_userptr = drm_intel_gem_bo_alloc_userptr; ofc
-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] intel: Delay testing for userptr until first use

[Tvrtko Ursulin]

On 11/04/2014 02:31 PM, Chris Wilson wrote:
> Running __mmu_notifier_register() is surprisingly expensive, so let's
> not do that unless we have to.

Affects some program startup or what? What is the cost? I would add some 
notes in the commit for future reference.

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   intel/intel_bufmgr_gem.c | 114 +++++++++++++++++++++++++++--------------------
>   1 file changed, 65 insertions(+), 49 deletions(-)
>
> diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
> index f2f4fea..addf413 100644
> --- a/intel/intel_bufmgr_gem.c
> +++ b/intel/intel_bufmgr_gem.c
> @@ -935,6 +935,70 @@ drm_intel_gem_bo_alloc_userptr(drm_intel_bufmgr *bufmgr,
>   	return &bo_gem->bo;
>   }
>
> +static bool
> +has_userptr(drm_intel_bufmgr_gem *bufmgr_gem)
> +{
> +	int ret;
> +	void *ptr;
> +	long pgsz;
> +	struct drm_i915_gem_userptr userptr;
> +	struct drm_gem_close close_bo;
> +
> +	pgsz = sysconf(_SC_PAGESIZE);
> +	assert(pgsz > 0);
> +
> +	ret = posix_memalign(&ptr, pgsz, pgsz);
> +	if (ret) {
> +		DBG("Failed to get a page (%ld) for userptr detection!\n",
> +			pgsz);
> +		return false;
> +	}
> +
> +	memset(&userptr, 0, sizeof(userptr));
> +	userptr.user_ptr = (__u64)(unsigned long)ptr;
> +	userptr.user_size = pgsz;
> +
> +retry:
> +	ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_I915_GEM_USERPTR, &userptr);
> +	if (ret) {
> +		if (errno == ENODEV && userptr.flags == 0) {
> +			userptr.flags = I915_USERPTR_UNSYNCHRONIZED;
> +			goto retry;
> +		}
> +		free(ptr);
> +		return false;
> +	}
> +
> +	close_bo.handle = userptr.handle;
> +	ret = drmIoctl(bufmgr_gem->fd, DRM_IOCTL_GEM_CLOSE, &close_bo);
> +	free(ptr);
> +	if (ret) {
> +		fprintf(stderr, "Failed to release test userptr object! (%d) "
> +				"i915 kernel driver may not be sane!\n", errno);
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
> +static drm_intel_bo *
> +check_bo_alloc_userptr(drm_intel_bufmgr *bufmgr,
> +		       const char *name,
> +		       void *addr,
> +		       uint32_t tiling_mode,
> +		       uint32_t stride,
> +		       unsigned long size,
> +		       unsigned long flags)
> +{
> +	if (has_userptr((drm_intel_bufmgr_gem *)bufmgr))
> +		bufmgr->bo_alloc_userptr = drm_intel_bo_alloc_userptr;

I wouldn't have spotted this - so retroactively compile tested I assume?

Otherwise,
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

Re: [PATCH] intel: Delay testing for userptr until first use

[Chris Wilson]

On Tue, Nov 04, 2014 at 03:14:04PM +0000, Tvrtko Ursulin wrote:
> 
> On 11/04/2014 02:31 PM, Chris Wilson wrote:
> >Running __mmu_notifier_register() is surprisingly expensive, so let's
> >not do that unless we have to.
> 
> Affects some program startup or what? What is the cost? I would add
> some notes in the commit for future reference.

It was affecting a badly behavely igt test, that was provoking the
linear walk in mm_take_all_locks() and then causing each one to restart
due to signals... Performance fell off a cliff. This was extreme, but
one can suppose that on a similarly stressed system startup performance
will also degrade.
-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] intel: Delay testing for userptr until first use

[Tvrtko Ursulin]

On 11/04/2014 06:44 PM, Chris Wilson wrote:
> On Tue, Nov 04, 2014 at 03:14:04PM +0000, Tvrtko Ursulin wrote:
>>
>> On 11/04/2014 02:31 PM, Chris Wilson wrote:
>>> Running __mmu_notifier_register() is surprisingly expensive, so let's
>>> not do that unless we have to.
>>
>> Affects some program startup or what? What is the cost? I would add
>> some notes in the commit for future reference.
>
> It was affecting a badly behavely igt test, that was provoking the
> linear walk in mm_take_all_locks() and then causing each one to restart
> due to signals... Performance fell off a cliff. This was extreme, but
> one can suppose that on a similarly stressed system startup performance
> will also degrade.

Oh, so restarting the ioctl based on EINTR which propagates all the way 
up from mm_take_all_locks? Can't really imagine a signal torture similar 
to IGT in real life but check on first use makes sense anyway.

Regards,

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