All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/3] drm/i915: add prefault_disable module option
@ 2013-07-19  5:51 Xiong Zhang
  2013-07-19  5:51 ` [PATCH 2/3] drm/i915: add debug info in slow path Xiong Zhang
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Xiong Zhang @ 2013-07-19  5:51 UTC (permalink / raw)
  To: intel-gfx

prefault is stll enabled by default which prevent most of pwrite/pread/reloc
from running slow path, in order to verify these slow pathes, prefault need
to be disabled.

Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c            |  5 +++++
 drivers/gpu/drm/i915/i915_drv.h            |  1 +
 drivers/gpu/drm/i915/i915_gem.c            | 12 +++++++-----
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  6 ++++--
 4 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index b07362f..dac6bd3 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -137,6 +137,11 @@ module_param_named(fastboot, i915_fastboot, bool, 0600);
 MODULE_PARM_DESC(fastboot, "Try to skip unnecessary mode sets at boot time "
 		 "(default: false)");
 
+bool i915_prefault_disable __read_mostly = false;
+module_param_named(prefault_disable, i915_prefault_disable, bool, 0600);
+MODULE_PARM_DESC(prefault_disable,
+		"Try to disable pre page fault for pread/pwrite/reloc (default:false)");
+
 static struct drm_driver driver;
 extern int intel_agp_enabled;
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7fdc8e3..9516e19 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1587,6 +1587,7 @@ extern unsigned int i915_preliminary_hw_support __read_mostly;
 extern int i915_disable_power_well __read_mostly;
 extern int i915_enable_ips __read_mostly;
 extern bool i915_fastboot __read_mostly;
+extern bool i915_prefault_disable __read_mostly;
 
 extern int i915_suspend(struct drm_device *dev, pm_message_t state);
 extern int i915_resume(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index c9d9d20..de59154 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -465,7 +465,7 @@ i915_gem_shmem_pread(struct drm_device *dev,
 
 		mutex_unlock(&dev->struct_mutex);
 
-		if (!prefaulted) {
+		if (likely(!i915_prefault_disable) && !prefaulted) {
 			ret = fault_in_multipages_writeable(user_data, remain);
 			/* Userspace is tricking us, but we've already clobbered
 			 * its pages with the prefault and promised to write the
@@ -860,10 +860,12 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
 		       args->size))
 		return -EFAULT;
 
-	ret = fault_in_multipages_readable(to_user_ptr(args->data_ptr),
-					   args->size);
-	if (ret)
-		return -EFAULT;
+	if (likely(!i915_prefault_disable)) {
+		ret = fault_in_multipages_readable(to_user_ptr(args->data_ptr),
+						   args->size);
+		if (ret)
+			return -EFAULT;
+	}
 
 	ret = i915_mutex_lock_interruptible(dev);
 	if (ret)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 1b58694..1734825 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -759,8 +759,10 @@ validate_exec_list(struct drm_i915_gem_exec_object2 *exec,
 		if (!access_ok(VERIFY_WRITE, ptr, length))
 			return -EFAULT;
 
-		if (fault_in_multipages_readable(ptr, length))
-			return -EFAULT;
+		if (likely(!i915_prefault_disable)) {
+			if (fault_in_multipages_readable(ptr, length))
+				return -EFAULT;
+		}
 	}
 
 	return 0;
-- 
1.8.3.2

^ permalink raw reply related	[flat|nested] 9+ messages in thread
* [PATCH 2/3] drm/i915: add debug info in slow path
  2013-07-19  5:51 [PATCH 1/3] drm/i915: add prefault_disable module option Xiong Zhang
@ 2013-07-19  5:51 ` Xiong Zhang
  2013-07-19  7:24   ` Daniel Vetter
  2013-07-19  5:51 ` [PATCH 3/3] drm/i915: run shmem_pread_fast() after page fault Xiong Zhang
  2013-07-19  7:23 ` [PATCH 1/3] drm/i915: add prefault_disable module option Daniel Vetter
  2 siblings, 1 reply; 9+ messages in thread
From: Xiong Zhang @ 2013-07-19  5:51 UTC (permalink / raw)
  To: intel-gfx

Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c            | 4 ++++
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index de59154..f194d7e 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -376,6 +376,8 @@ shmem_pread_slow(struct page *page, int shmem_page_offset, int page_length,
 	char *vaddr;
 	int ret;
 
+	DRM_DEBUG("execute pread slow path\n");
+
 	vaddr = kmap(page);
 	if (needs_clflush)
 		shmem_clflush_swizzled_range(vaddr + shmem_page_offset,
@@ -689,6 +691,8 @@ shmem_pwrite_slow(struct page *page, int shmem_page_offset, int page_length,
 	char *vaddr;
 	int ret;
 
+	DRM_DEBUG("execute pwrite slow path\n");
+
 	vaddr = kmap(page);
 	if (unlikely(needs_clflush_before || page_do_bit17_swizzling))
 		shmem_clflush_swizzled_range(vaddr + shmem_page_offset,
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 1734825..3065297 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -587,6 +587,7 @@ i915_gem_execbuffer_relocate_slow(struct drm_device *dev,
 	int i, total, ret;
 	int count = args->buffer_count;
 
+	DRM_DEBUG("Execute relocate slow path\n");
 	/* We may process another execbuffer during the unlock... */
 	while (!list_empty(&eb->objects)) {
 		obj = list_first_entry(&eb->objects,
-- 
1.8.3.2

^ permalink raw reply related	[flat|nested] 9+ messages in thread
* [PATCH 3/3] drm/i915: run shmem_pread_fast() after page fault
  2013-07-19  5:51 [PATCH 1/3] drm/i915: add prefault_disable module option Xiong Zhang
  2013-07-19  5:51 ` [PATCH 2/3] drm/i915: add debug info in slow path Xiong Zhang
@ 2013-07-19  5:51 ` Xiong Zhang
  2013-07-19  7:29   ` Daniel Vetter
  2013-07-19  7:23 ` [PATCH 1/3] drm/i915: add prefault_disable module option Daniel Vetter
  2 siblings, 1 reply; 9+ messages in thread
From: Xiong Zhang @ 2013-07-19  5:51 UTC (permalink / raw)
  To: intel-gfx

fault_in_multipages_writeable() will load fault page into physical
memory, then pread can run fast path, no need to run slow path

Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index f194d7e..982df1e 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -459,6 +459,7 @@ i915_gem_shmem_pread(struct drm_device *dev,
 		page_do_bit17_swizzling = obj_do_bit17_swizzling &&
 			(page_to_phys(page) & (1 << 17)) != 0;
 
+read_again:
 		ret = shmem_pread_fast(page, shmem_page_offset, page_length,
 				       user_data, page_do_bit17_swizzling,
 				       needs_clflush);
@@ -475,6 +476,8 @@ i915_gem_shmem_pread(struct drm_device *dev,
 			 * and just continue. */
 			(void)ret;
 			prefaulted = 1;
+			mutex_lock(&dev->struct_mutex);
+			goto read_again;
 		}
 
 		ret = shmem_pread_slow(page, shmem_page_offset, page_length,
-- 
1.8.3.2

^ permalink raw reply related	[flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] drm/i915: add prefault_disable module option
  2013-07-19  5:51 [PATCH 1/3] drm/i915: add prefault_disable module option Xiong Zhang
  2013-07-19  5:51 ` [PATCH 2/3] drm/i915: add debug info in slow path Xiong Zhang
  2013-07-19  5:51 ` [PATCH 3/3] drm/i915: run shmem_pread_fast() after page fault Xiong Zhang
@ 2013-07-19  7:23 ` Daniel Vetter
  2 siblings, 0 replies; 9+ messages in thread
From: Daniel Vetter @ 2013-07-19  7:23 UTC (permalink / raw)
  To: Xiong Zhang; +Cc: intel-gfx

On Fri, Jul 19, 2013 at 01:51:24PM +0800, Xiong Zhang wrote:
> prefault is stll enabled by default which prevent most of pwrite/pread/reloc
> from running slow path, in order to verify these slow pathes, prefault need
> to be disabled.
> 
> Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com>

lgtm, queued for -next, thanks for the patch.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.c            |  5 +++++
>  drivers/gpu/drm/i915/i915_drv.h            |  1 +
>  drivers/gpu/drm/i915/i915_gem.c            | 12 +++++++-----
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |  6 ++++--
>  4 files changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index b07362f..dac6bd3 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -137,6 +137,11 @@ module_param_named(fastboot, i915_fastboot, bool, 0600);
>  MODULE_PARM_DESC(fastboot, "Try to skip unnecessary mode sets at boot time "
>  		 "(default: false)");
>  
> +bool i915_prefault_disable __read_mostly = false;
> +module_param_named(prefault_disable, i915_prefault_disable, bool, 0600);
> +MODULE_PARM_DESC(prefault_disable,
> +		"Try to disable pre page fault for pread/pwrite/reloc (default:false)");
> +
>  static struct drm_driver driver;
>  extern int intel_agp_enabled;
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 7fdc8e3..9516e19 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1587,6 +1587,7 @@ extern unsigned int i915_preliminary_hw_support __read_mostly;
>  extern int i915_disable_power_well __read_mostly;
>  extern int i915_enable_ips __read_mostly;
>  extern bool i915_fastboot __read_mostly;
> +extern bool i915_prefault_disable __read_mostly;
>  
>  extern int i915_suspend(struct drm_device *dev, pm_message_t state);
>  extern int i915_resume(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index c9d9d20..de59154 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -465,7 +465,7 @@ i915_gem_shmem_pread(struct drm_device *dev,
>  
>  		mutex_unlock(&dev->struct_mutex);
>  
> -		if (!prefaulted) {
> +		if (likely(!i915_prefault_disable) && !prefaulted) {
>  			ret = fault_in_multipages_writeable(user_data, remain);
>  			/* Userspace is tricking us, but we've already clobbered
>  			 * its pages with the prefault and promised to write the
> @@ -860,10 +860,12 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data,
>  		       args->size))
>  		return -EFAULT;
>  
> -	ret = fault_in_multipages_readable(to_user_ptr(args->data_ptr),
> -					   args->size);
> -	if (ret)
> -		return -EFAULT;
> +	if (likely(!i915_prefault_disable)) {
> +		ret = fault_in_multipages_readable(to_user_ptr(args->data_ptr),
> +						   args->size);
> +		if (ret)
> +			return -EFAULT;
> +	}
>  
>  	ret = i915_mutex_lock_interruptible(dev);
>  	if (ret)
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 1b58694..1734825 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -759,8 +759,10 @@ validate_exec_list(struct drm_i915_gem_exec_object2 *exec,
>  		if (!access_ok(VERIFY_WRITE, ptr, length))
>  			return -EFAULT;
>  
> -		if (fault_in_multipages_readable(ptr, length))
> -			return -EFAULT;
> +		if (likely(!i915_prefault_disable)) {
> +			if (fault_in_multipages_readable(ptr, length))
> +				return -EFAULT;
> +		}
>  	}
>  
>  	return 0;
> -- 
> 1.8.3.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] drm/i915: add debug info in slow path
  2013-07-19  5:51 ` [PATCH 2/3] drm/i915: add debug info in slow path Xiong Zhang
@ 2013-07-19  7:24   ` Daniel Vetter
  0 siblings, 0 replies; 9+ messages in thread
From: Daniel Vetter @ 2013-07-19  7:24 UTC (permalink / raw)
  To: Xiong Zhang; +Cc: intel-gfx

On Fri, Jul 19, 2013 at 01:51:25PM +0800, Xiong Zhang wrote:
> Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com>

I guess this was useful for debugging, but printks are rather expensive.
So I'll drop this - if we ever decide that this is useful information we
could expose it as a tracepoint, but since not even the core file i/o path
has that I don't think we need it.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_gem.c            | 4 ++++
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 1 +
>  2 files changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index de59154..f194d7e 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -376,6 +376,8 @@ shmem_pread_slow(struct page *page, int shmem_page_offset, int page_length,
>  	char *vaddr;
>  	int ret;
>  
> +	DRM_DEBUG("execute pread slow path\n");
> +
>  	vaddr = kmap(page);
>  	if (needs_clflush)
>  		shmem_clflush_swizzled_range(vaddr + shmem_page_offset,
> @@ -689,6 +691,8 @@ shmem_pwrite_slow(struct page *page, int shmem_page_offset, int page_length,
>  	char *vaddr;
>  	int ret;
>  
> +	DRM_DEBUG("execute pwrite slow path\n");
> +
>  	vaddr = kmap(page);
>  	if (unlikely(needs_clflush_before || page_do_bit17_swizzling))
>  		shmem_clflush_swizzled_range(vaddr + shmem_page_offset,
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 1734825..3065297 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -587,6 +587,7 @@ i915_gem_execbuffer_relocate_slow(struct drm_device *dev,
>  	int i, total, ret;
>  	int count = args->buffer_count;
>  
> +	DRM_DEBUG("Execute relocate slow path\n");
>  	/* We may process another execbuffer during the unlock... */
>  	while (!list_empty(&eb->objects)) {
>  		obj = list_first_entry(&eb->objects,
> -- 
> 1.8.3.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] drm/i915: run shmem_pread_fast() after page fault
  2013-07-19  5:51 ` [PATCH 3/3] drm/i915: run shmem_pread_fast() after page fault Xiong Zhang
@ 2013-07-19  7:29   ` Daniel Vetter
  2013-07-19  8:52     ` Zhang, Xiong Y
  0 siblings, 1 reply; 9+ messages in thread
From: Daniel Vetter @ 2013-07-19  7:29 UTC (permalink / raw)
  To: Xiong Zhang; +Cc: intel-gfx

On Fri, Jul 19, 2013 at 01:51:26PM +0800, Xiong Zhang wrote:
> fault_in_multipages_writeable() will load fault page into physical
> memory, then pread can run fast path, no need to run slow path
> 
> Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com>

Hm, this avoids going through the slowpath for the first page. Does this
have an impact on micro-benchmarks (we have a few) or is this just
something you've spotted? I'm a bit vary of making the already complicated
shmem pread/pwrite paths even more complicated. Chris?
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_gem.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index f194d7e..982df1e 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -459,6 +459,7 @@ i915_gem_shmem_pread(struct drm_device *dev,
>  		page_do_bit17_swizzling = obj_do_bit17_swizzling &&
>  			(page_to_phys(page) & (1 << 17)) != 0;
>  
> +read_again:
>  		ret = shmem_pread_fast(page, shmem_page_offset, page_length,
>  				       user_data, page_do_bit17_swizzling,
>  				       needs_clflush);
> @@ -475,6 +476,8 @@ i915_gem_shmem_pread(struct drm_device *dev,
>  			 * and just continue. */
>  			(void)ret;
>  			prefaulted = 1;
> +			mutex_lock(&dev->struct_mutex);
> +			goto read_again;
>  		}
>  
>  		ret = shmem_pread_slow(page, shmem_page_offset, page_length,
> -- 
> 1.8.3.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] drm/i915: run shmem_pread_fast() after page fault
  2013-07-19  7:29   ` Daniel Vetter
@ 2013-07-19  8:52     ` Zhang, Xiong Y
  2013-07-19  8:56       ` Daniel Vetter
  2013-07-19  9:19       ` Chris Wilson
  0 siblings, 2 replies; 9+ messages in thread
From: Zhang, Xiong Y @ 2013-07-19  8:52 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: intel-gfx

[-- Attachment #1: Type: text/plain, Size: 2471 bytes --]

It just influence one page.
During my test, I found one read slow path info when enable prefault. So I add this patch.
Why don't move fault_in_multipages_writeable() from i915_gem_shmem_pread() to the beginning of i915_gem_pread_ioctl() ? 
So the pread_ioctl() is like pwrite_ioctl(). Is the cost of fault_in_multipages_writeable() lager than fault_in_multipages_readable() ?
See the attachment.

thanks
-----Original Message-----
From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
Sent: Friday, July 19, 2013 3:29 PM
To: Zhang, Xiong Y
Cc: intel-gfx@lists.freedesktop.org; Chris Wilson
Subject: Re: [Intel-gfx] [PATCH 3/3] drm/i915: run shmem_pread_fast() after page fault

On Fri, Jul 19, 2013 at 01:51:26PM +0800, Xiong Zhang wrote:
> fault_in_multipages_writeable() will load fault page into physical 
> memory, then pread can run fast path, no need to run slow path
> 
> Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com>

Hm, this avoids going through the slowpath for the first page. Does this have an impact on micro-benchmarks (we have a few) or is this just something you've spotted? I'm a bit vary of making the already complicated shmem pread/pwrite paths even more complicated. Chris?
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_gem.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c 
> b/drivers/gpu/drm/i915/i915_gem.c index f194d7e..982df1e 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -459,6 +459,7 @@ i915_gem_shmem_pread(struct drm_device *dev,
>  		page_do_bit17_swizzling = obj_do_bit17_swizzling &&
>  			(page_to_phys(page) & (1 << 17)) != 0;
>  
> +read_again:
>  		ret = shmem_pread_fast(page, shmem_page_offset, page_length,
>  				       user_data, page_do_bit17_swizzling,
>  				       needs_clflush);
> @@ -475,6 +476,8 @@ i915_gem_shmem_pread(struct drm_device *dev,
>  			 * and just continue. */
>  			(void)ret;
>  			prefaulted = 1;
> +			mutex_lock(&dev->struct_mutex);
> +			goto read_again;
>  		}
>  
>  		ret = shmem_pread_slow(page, shmem_page_offset, page_length,
> --
> 1.8.3.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[-- Attachment #2: 0001-drm-i915-move-page-fault-handle-to-i915_gem_pread_io.patch --]
[-- Type: application/octet-stream, Size: 1781 bytes --]

From 85d8208fef1383cf37c470daa89eec4662860c6e Mon Sep 17 00:00:00 2001
From: Xiong Zhang <xiong.y.zhang@intel.com>
Date: Fri, 19 Jul 2013 16:38:44 +0800
Subject: [PATCH] drm/i915: move page fault handle to i915_gem_pread_ioctl

---
 drivers/gpu/drm/i915/i915_gem.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index f194d7e..2b47119 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -408,7 +408,6 @@ i915_gem_shmem_pread(struct drm_device *dev,
 	loff_t offset;
 	int shmem_page_offset, page_length, ret = 0;
 	int obj_do_bit17_swizzling, page_do_bit17_swizzling;
-	int prefaulted = 0;
 	int needs_clflush = 0;
 	struct sg_page_iter sg_iter;
 
@@ -467,16 +466,6 @@ i915_gem_shmem_pread(struct drm_device *dev,
 
 		mutex_unlock(&dev->struct_mutex);
 
-		if (likely(!i915_prefault_disable) && !prefaulted) {
-			ret = fault_in_multipages_writeable(user_data, remain);
-			/* Userspace is tricking us, but we've already clobbered
-			 * its pages with the prefault and promised to write the
-			 * data up to the first fault. Hence ignore any errors
-			 * and just continue. */
-			(void)ret;
-			prefaulted = 1;
-		}
-
 		ret = shmem_pread_slow(page, shmem_page_offset, page_length,
 				       user_data, page_do_bit17_swizzling,
 				       needs_clflush);
@@ -521,6 +510,14 @@ i915_gem_pread_ioctl(struct drm_device *dev, void *data,
 		       args->size))
 		return -EFAULT;
 
+	if (likely(!i915_prefault_disable)) {
+		ret = fault_in_multipages_writeable(to_user_ptr(args->data_ptr),
+						   args->size);
+		if (ret)
+			return -EFAULT;
+	}
+
+
 	ret = i915_mutex_lock_interruptible(dev);
 	if (ret)
 		return ret;
-- 
1.8.3.2


[-- Attachment #3: Type: text/plain, Size: 159 bytes --]

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

^ permalink raw reply related	[flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] drm/i915: run shmem_pread_fast() after page fault
  2013-07-19  8:52     ` Zhang, Xiong Y
@ 2013-07-19  8:56       ` Daniel Vetter
  2013-07-19  9:19       ` Chris Wilson
  1 sibling, 0 replies; 9+ messages in thread
From: Daniel Vetter @ 2013-07-19  8:56 UTC (permalink / raw)
  To: Zhang, Xiong Y; +Cc: intel-gfx

On Fri, Jul 19, 2013 at 08:52:39AM +0000, Zhang, Xiong Y wrote:
> It just influence one page.
> During my test, I found one read slow path info when enable prefault. So
> I add this patch.  Why don't move fault_in_multipages_writeable() from
> i915_gem_shmem_pread() to the beginning of i915_gem_pread_ioctl() ?  So
> the pread_ioctl() is like pwrite_ioctl(). Is the cost of
> fault_in_multipages_writeable() lager than
> fault_in_multipages_readable() ?  See the attachment.

It's a slight matter of correctness fixed in:

commit 96d79b52701758404cf8701986891afc99ce810b
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Sun Mar 25 19:47:36 2012 +0200

    drm/i915: don't clobber userspace memory before commiting to the pread

The issue is that we can't prefault before we know that we'll do the
write and prefaulting earlier will write garbage into userspace's memory.
Hence the tricky ordering. I guess we should add a comment why this is so
to the code.
-Daniel

> 
> thanks
> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> Sent: Friday, July 19, 2013 3:29 PM
> To: Zhang, Xiong Y
> Cc: intel-gfx@lists.freedesktop.org; Chris Wilson
> Subject: Re: [Intel-gfx] [PATCH 3/3] drm/i915: run shmem_pread_fast() after page fault
> 
> On Fri, Jul 19, 2013 at 01:51:26PM +0800, Xiong Zhang wrote:
> > fault_in_multipages_writeable() will load fault page into physical 
> > memory, then pread can run fast path, no need to run slow path
> > 
> > Signed-off-by: Xiong Zhang <xiong.y.zhang@intel.com>
> 
> Hm, this avoids going through the slowpath for the first page. Does this have an impact on micro-benchmarks (we have a few) or is this just something you've spotted? I'm a bit vary of making the already complicated shmem pread/pwrite paths even more complicated. Chris?
> -Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c 
> > b/drivers/gpu/drm/i915/i915_gem.c index f194d7e..982df1e 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -459,6 +459,7 @@ i915_gem_shmem_pread(struct drm_device *dev,
> >  		page_do_bit17_swizzling = obj_do_bit17_swizzling &&
> >  			(page_to_phys(page) & (1 << 17)) != 0;
> >  
> > +read_again:
> >  		ret = shmem_pread_fast(page, shmem_page_offset, page_length,
> >  				       user_data, page_do_bit17_swizzling,
> >  				       needs_clflush);
> > @@ -475,6 +476,8 @@ i915_gem_shmem_pread(struct drm_device *dev,
> >  			 * and just continue. */
> >  			(void)ret;
> >  			prefaulted = 1;
> > +			mutex_lock(&dev->struct_mutex);
> > +			goto read_again;
> >  		}
> >  
> >  		ret = shmem_pread_slow(page, shmem_page_offset, page_length,
> > --
> > 1.8.3.2
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] drm/i915: run shmem_pread_fast() after page fault
  2013-07-19  8:52     ` Zhang, Xiong Y
  2013-07-19  8:56       ` Daniel Vetter
@ 2013-07-19  9:19       ` Chris Wilson
  1 sibling, 0 replies; 9+ messages in thread
From: Chris Wilson @ 2013-07-19  9:19 UTC (permalink / raw)
  To: Zhang, Xiong Y; +Cc: intel-gfx

On Fri, Jul 19, 2013 at 08:52:39AM +0000, Zhang, Xiong Y wrote:
> It just influence one page.
> During my test, I found one read slow path info when enable prefault. So I add this patch.
> Why don't move fault_in_multipages_writeable() from i915_gem_shmem_pread() to the beginning of i915_gem_pread_ioctl() ? 
> So the pread_ioctl() is like pwrite_ioctl(). Is the cost of fault_in_multipages_writeable() lager than fault_in_multipages_readable() ?

The essence of the problem with fault_in_multipages_writeable() is that
it writes a 0 without us guarranteeing to replace it with correct data.
Real world usage (at least in the ddx) of pread is limited to UXA
readbacks (x11perf -shmget would be the best test case), and more or
less gen2 readbacks in SNA. (For SNA we first try to use CPU bo and
various maps before resorting to pread.)

You will want to extend the microbenchmarks such as i-g-t/gem_gtt_speed
to cover cases which can be prefaulted and where prefaulting helps. If
you can measure a difference between the paths in the current code, you
are ready to see if you can measure an improvement from your patch.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

^ permalink raw reply	[flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-07-19  9:19 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-07-19  5:51 [PATCH 1/3] drm/i915: add prefault_disable module option Xiong Zhang
2013-07-19  5:51 ` [PATCH 2/3] drm/i915: add debug info in slow path Xiong Zhang
2013-07-19  7:24   ` Daniel Vetter
2013-07-19  5:51 ` [PATCH 3/3] drm/i915: run shmem_pread_fast() after page fault Xiong Zhang
2013-07-19  7:29   ` Daniel Vetter
2013-07-19  8:52     ` Zhang, Xiong Y
2013-07-19  8:56       ` Daniel Vetter
2013-07-19  9:19       ` Chris Wilson
2013-07-19  7:23 ` [PATCH 1/3] drm/i915: add prefault_disable module option Daniel Vetter

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.