[PATCH] drm/i915: Batch copy_from_user for relocation processing

[PATCH] drm/i915: Batch copy_from_user for relocation processing

[Chris Wilson]

Originally the code tried to allocate a large enough array to perform
the copy using vmalloc, performance wasn't great and throughput was
improved by processing each individual relocation entry separately.
This too is not as efficient as one would desire. A compromise is then
to allocate a single page (since that is relatively cheap) and process
the relocations in page size batches.

x11perf -copywinwin10:	n450/pnv	i3-330m		i5-2520m (cpu)
               Before: 	  249000	 785000		 1280000 (80%)
                After:	  264000	 896000		 1280000 (65%)

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |   92 +++++++++++++++++++++++-----
 1 files changed, 77 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 2d5d41b..1da04db 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -407,28 +407,90 @@ i915_gem_execbuffer_relocate_object(struct drm_i915_gem_object *obj,
 {
 	struct drm_i915_gem_relocation_entry __user *user_relocs;
 	struct drm_i915_gem_exec_object2 *entry = obj->exec_entry;
-	int i, ret;
+	int i, ret = 0;
+
+	if (entry->relocation_count == 0)
+		return 0;
+
+#define N_RELOC_PER_PAGE \
+	(PAGE_SIZE / sizeof(struct drm_i915_gem_relocation_entry))
 
 	user_relocs = (void __user *)(uintptr_t)entry->relocs_ptr;
-	for (i = 0; i < entry->relocation_count; i++) {
-		struct drm_i915_gem_relocation_entry reloc;
 
-		if (__copy_from_user_inatomic(&reloc,
-					      user_relocs+i,
-					      sizeof(reloc)))
-			return -EFAULT;
+	if (entry->relocation_count < N_RELOC_PER_PAGE / 2) {
+fallback:
+		for (i = 0; i < entry->relocation_count; i++) {
+			struct drm_i915_gem_relocation_entry reloc;
+			u64 offset;
 
-		ret = i915_gem_execbuffer_relocate_entry(obj, eb, &reloc);
-		if (ret)
-			return ret;
+			if (__copy_from_user_inatomic(&reloc,
+						      user_relocs+i,
+						      sizeof(reloc)))
+				return -EFAULT;
 
-		if (__copy_to_user_inatomic(&user_relocs[i].presumed_offset,
-					    &reloc.presumed_offset,
-					    sizeof(reloc.presumed_offset)))
-			return -EFAULT;
+			offset = reloc.presumed_offset;
+
+			ret = i915_gem_execbuffer_relocate_entry(obj, eb, &reloc);
+			if (ret)
+				return ret;
+
+			if (reloc.presumed_offset != offset &&
+			    __copy_to_user_inatomic(&user_relocs[i].presumed_offset,
+						    &reloc.presumed_offset,
+						    sizeof(reloc.presumed_offset)))
+				return -EFAULT;
+		}
+	} else {
+		unsigned long page;
+		int remain;
+
+		page = __get_free_page(GFP_TEMPORARY);
+		if (unlikely(page == 0))
+			goto fallback;
+
+		i = 0;
+		remain = entry->relocation_count;
+		do {
+			struct drm_i915_gem_relocation_entry *r =
+				(struct drm_i915_gem_relocation_entry *)page;
+			int count = remain;
+			if (count > N_RELOC_PER_PAGE)
+				count = N_RELOC_PER_PAGE;
+			remain -= count;
+
+			if (__copy_from_user_inatomic(r, user_relocs+i,
+						      count*sizeof(r[0]))) {
+				ret = -EFAULT;
+				goto err;
+			}
+
+			do {
+				u64 offset = r->presumed_offset;
+
+				ret = i915_gem_execbuffer_relocate_entry(obj, eb, r);
+				if (ret)
+					goto err;
+
+				if (r->presumed_offset != offset &&
+				    __copy_to_user_inatomic(&user_relocs[i].presumed_offset,
+							    &r->presumed_offset,
+							    sizeof(r->presumed_offset))) {
+					ret = -EFAULT;
+					goto err;
+				}
+
+				i++;
+				r++;
+			} while (--count);
+		} while (remain);
+
+		ret = 0;
+err:
+		free_page(page);
 	}
 
-	return 0;
+	return ret;
+#undef N_RELOC_PER_PAGE
 }
 
 static int
-- 
1.7.9.1

Re: [PATCH] drm/i915: Batch copy_from_user for relocation processing

[Daniel Vetter]

On Sat, Mar 24, 2012 at 12:20:24AM +0000, Chris Wilson wrote:
> Originally the code tried to allocate a large enough array to perform
> the copy using vmalloc, performance wasn't great and throughput was
> improved by processing each individual relocation entry separately.
> This too is not as efficient as one would desire. A compromise is then
> to allocate a single page (since that is relatively cheap) and process
> the relocations in page size batches.
> 
> x11perf -copywinwin10:	n450/pnv	i3-330m		i5-2520m (cpu)
>                Before: 	  249000	 785000		 1280000 (80%)
>                 After:	  264000	 896000		 1280000 (65%)
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |   92 +++++++++++++++++++++++-----
>  1 files changed, 77 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 2d5d41b..1da04db 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -407,28 +407,90 @@ i915_gem_execbuffer_relocate_object(struct drm_i915_gem_object *obj,
>  {
>  	struct drm_i915_gem_relocation_entry __user *user_relocs;
>  	struct drm_i915_gem_exec_object2 *entry = obj->exec_entry;
> -	int i, ret;
> +	int i, ret = 0;
> +
> +	if (entry->relocation_count == 0)
> +		return 0;
> +
> +#define N_RELOC_PER_PAGE \
> +	(PAGE_SIZE / sizeof(struct drm_i915_gem_relocation_entry))
>  
>  	user_relocs = (void __user *)(uintptr_t)entry->relocs_ptr;
> -	for (i = 0; i < entry->relocation_count; i++) {
> -		struct drm_i915_gem_relocation_entry reloc;
>  
> -		if (__copy_from_user_inatomic(&reloc,
> -					      user_relocs+i,
> -					      sizeof(reloc)))
> -			return -EFAULT;
> +	if (entry->relocation_count < N_RELOC_PER_PAGE / 2) {
> +fallback:

We don't we go slightly more over board with optimiziting this?
drm_i915_gem_relocation_entry is on 32 bytes, so we can fit a few onto the
stack. Hence what about this before the loop?

drm_i915_gem_relocation_entry stack_arr[512/sizeof(reloc_entry)];

if (relocation_count > N_RELOC_PER_PAGE) {
	tmp_stor = __get_free_page;
	if (!tmp_stor)
		goto fallback;
	tmp_stor_size = N_RELOC_PER_PAGE;
} else {
fallback:
	tmp_stor = stackk_arr;
	tmp_stor_size = 512/sizeof(reloc_entry);
}

Then we also wouldn't have the not-so-beautiful duplication in code. Maybe
the added complexity of the page_alloc path isn't even worth it anymore.
Can I bother you to play around with this?

Cheers, Daniel

> +		for (i = 0; i < entry->relocation_count; i++) {
> +			struct drm_i915_gem_relocation_entry reloc;
> +			u64 offset;
>  
> -		ret = i915_gem_execbuffer_relocate_entry(obj, eb, &reloc);
> -		if (ret)
> -			return ret;
> +			if (__copy_from_user_inatomic(&reloc,
> +						      user_relocs+i,
> +						      sizeof(reloc)))
> +				return -EFAULT;
>  
> -		if (__copy_to_user_inatomic(&user_relocs[i].presumed_offset,
> -					    &reloc.presumed_offset,
> -					    sizeof(reloc.presumed_offset)))
> -			return -EFAULT;
> +			offset = reloc.presumed_offset;
> +
> +			ret = i915_gem_execbuffer_relocate_entry(obj, eb, &reloc);
> +			if (ret)
> +				return ret;
> +
> +			if (reloc.presumed_offset != offset &&
> +			    __copy_to_user_inatomic(&user_relocs[i].presumed_offset,
> +						    &reloc.presumed_offset,
> +						    sizeof(reloc.presumed_offset)))
> +				return -EFAULT;
> +		}
> +	} else {
> +		unsigned long page;
> +		int remain;
> +
> +		page = __get_free_page(GFP_TEMPORARY);
> +		if (unlikely(page == 0))
> +			goto fallback;
> +
> +		i = 0;
> +		remain = entry->relocation_count;
> +		do {
> +			struct drm_i915_gem_relocation_entry *r =
> +				(struct drm_i915_gem_relocation_entry *)page;
> +			int count = remain;
> +			if (count > N_RELOC_PER_PAGE)
> +				count = N_RELOC_PER_PAGE;
> +			remain -= count;
> +
> +			if (__copy_from_user_inatomic(r, user_relocs+i,
> +						      count*sizeof(r[0]))) {
> +				ret = -EFAULT;
> +				goto err;
> +			}
> +
> +			do {
> +				u64 offset = r->presumed_offset;
> +
> +				ret = i915_gem_execbuffer_relocate_entry(obj, eb, r);
> +				if (ret)
> +					goto err;
> +
> +				if (r->presumed_offset != offset &&
> +				    __copy_to_user_inatomic(&user_relocs[i].presumed_offset,
> +							    &r->presumed_offset,
> +							    sizeof(r->presumed_offset))) {
> +					ret = -EFAULT;
> +					goto err;
> +				}
> +
> +				i++;
> +				r++;
> +			} while (--count);
> +		} while (remain);
> +
> +		ret = 0;
> +err:
> +		free_page(page);
>  	}
>  
> -	return 0;
> +	return ret;
> +#undef N_RELOC_PER_PAGE
>  }
>  
>  static int
> -- 
> 1.7.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH] drm/i915: Batch copy_from_user for relocation processing

[Chris Wilson]

Originally the code tried to allocate a large enough array to perform
the copy using vmalloc, performance wasn't great and throughput was
improved by processing each individual relocation entry separately.
This too is not as efficient as one would desire. A compromise would be
to allocate a single page, or to allocate a few entries on the stack,
and process the copy in batches. The latter gives simpler code and more
consistent performance due to a lack of heuristic.

x11perf -copywinwin10:	n450/pnv	i3-330m		i5-2520m (cpu)
               before: 	  249000	 785000		 1280000 (80%)
                 page:	  264000	 896000		 1280000 (65%)
             on-stack:	  264000	 902000		 1280000 (67%)

v2: Use 512-bytes of stack for batching rather than allocate a page.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |   45 ++++++++++++++++++++--------
 1 files changed, 32 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 2d5d41b..5f01444 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -405,30 +405,49 @@ static int
 i915_gem_execbuffer_relocate_object(struct drm_i915_gem_object *obj,
 				    struct eb_objects *eb)
 {
+#define N_RELOC(x) ((x) / sizeof(struct drm_i915_gem_relocation_entry))
+	struct drm_i915_gem_relocation_entry stack_reloc[N_RELOC(512)];
 	struct drm_i915_gem_relocation_entry __user *user_relocs;
 	struct drm_i915_gem_exec_object2 *entry = obj->exec_entry;
 	int i, ret;
 
+	if (entry->relocation_count == 0)
+		return 0;
+
 	user_relocs = (void __user *)(uintptr_t)entry->relocs_ptr;
-	for (i = 0; i < entry->relocation_count; i++) {
-		struct drm_i915_gem_relocation_entry reloc;
 
-		if (__copy_from_user_inatomic(&reloc,
-					      user_relocs+i,
-					      sizeof(reloc)))
+	i = entry->relocation_count;
+	do {
+		struct drm_i915_gem_relocation_entry *r = stack_reloc;
+		int count = i;
+		if (count > ARRAY_SIZE(stack_reloc))
+			count = ARRAY_SIZE(stack_reloc);
+		i -= count;
+
+		if (__copy_from_user_inatomic(r, user_relocs, count*sizeof(r[0])))
 			return -EFAULT;
 
-		ret = i915_gem_execbuffer_relocate_entry(obj, eb, &reloc);
-		if (ret)
-			return ret;
+		do {
+			u64 offset = r->presumed_offset;
 
-		if (__copy_to_user_inatomic(&user_relocs[i].presumed_offset,
-					    &reloc.presumed_offset,
-					    sizeof(reloc.presumed_offset)))
-			return -EFAULT;
-	}
+			ret = i915_gem_execbuffer_relocate_entry(obj, eb, r);
+			if (ret)
+				return ret;
+
+			if (r->presumed_offset != offset &&
+			    __copy_to_user_inatomic(&user_relocs->presumed_offset,
+						    &r->presumed_offset,
+						    sizeof(r->presumed_offset))) {
+				return -EFAULT;
+			}
+
+			user_relocs++;
+			r++;
+		} while (--count);
+	} while (i);
 
 	return 0;
+#undef N_RELOC
 }
 
 static int
-- 
1.7.9.1

[PATCH] drm/i915: Batch copy_from_user for relocation processing

[Chris Wilson]

Originally the code tried to allocate a large enough array to perform
the copy using vmalloc, performance wasn't great and throughput was
improved by processing each individual relocation entry separately.
This too is not as efficient as one would desire. A compromise would be
to allocate a single page, or to allocate a few entries on the stack,
and process the copy in batches. The latter gives simpler code and more
consistent performance due to a lack of heuristic.

x11perf -copywinwin10:	n450/pnv	i3-330m		i5-2520m (cpu)
               before: 	  249000	 785000		 1280000 (80%)
                 page:	  264000	 896000		 1280000 (65%)
             on-stack:	  264000	 902000		 1280000 (67%)

v2: Use 512-bytes of stack for batching rather than allocate a page.
v3: Tidy the code slightly with more descriptive variable names

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |   42 +++++++++++++++++++--------
 1 files changed, 29 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 2d5d41b..6ae82ac 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -405,30 +405,46 @@ static int
 i915_gem_execbuffer_relocate_object(struct drm_i915_gem_object *obj,
 				    struct eb_objects *eb)
 {
+#define N_RELOC(x) ((x) / sizeof(struct drm_i915_gem_relocation_entry))
+	struct drm_i915_gem_relocation_entry stack_reloc[N_RELOC(512)];
 	struct drm_i915_gem_relocation_entry __user *user_relocs;
 	struct drm_i915_gem_exec_object2 *entry = obj->exec_entry;
-	int i, ret;
+	int remain, ret;
 
 	user_relocs = (void __user *)(uintptr_t)entry->relocs_ptr;
-	for (i = 0; i < entry->relocation_count; i++) {
-		struct drm_i915_gem_relocation_entry reloc;
 
-		if (__copy_from_user_inatomic(&reloc,
-					      user_relocs+i,
-					      sizeof(reloc)))
+	remain = entry->relocation_count;
+	while (remain) {
+		struct drm_i915_gem_relocation_entry *r = stack_reloc;
+		int count = remain;
+		if (count > ARRAY_SIZE(stack_reloc))
+			count = ARRAY_SIZE(stack_reloc);
+		remain -= count;
+
+		if (__copy_from_user_inatomic(r, user_relocs, count*sizeof(r[0])))
 			return -EFAULT;
 
-		ret = i915_gem_execbuffer_relocate_entry(obj, eb, &reloc);
-		if (ret)
-			return ret;
+		do {
+			u64 offset = r->presumed_offset;
 
-		if (__copy_to_user_inatomic(&user_relocs[i].presumed_offset,
-					    &reloc.presumed_offset,
-					    sizeof(reloc.presumed_offset)))
-			return -EFAULT;
+			ret = i915_gem_execbuffer_relocate_entry(obj, eb, r);
+			if (ret)
+				return ret;
+
+			if (r->presumed_offset != offset &&
+			    __copy_to_user_inatomic(&user_relocs->presumed_offset,
+						    &r->presumed_offset,
+						    sizeof(r->presumed_offset))) {
+				return -EFAULT;
+			}
+
+			user_relocs++;
+			r++;
+		} while (--count);
 	}
 
 	return 0;
+#undef N_RELOC
 }
 
 static int
-- 
1.7.9.1

Re: [PATCH] drm/i915: Batch copy_from_user for relocation processing

[Daniel Vetter]

On Sat, Mar 24, 2012 at 08:12:53PM +0000, Chris Wilson wrote:
> Originally the code tried to allocate a large enough array to perform
> the copy using vmalloc, performance wasn't great and throughput was
> improved by processing each individual relocation entry separately.
> This too is not as efficient as one would desire. A compromise would be
> to allocate a single page, or to allocate a few entries on the stack,
> and process the copy in batches. The latter gives simpler code and more
> consistent performance due to a lack of heuristic.
> 
> x11perf -copywinwin10:	n450/pnv	i3-330m		i5-2520m (cpu)
>                before: 	  249000	 785000		 1280000 (80%)
>                  page:	  264000	 896000		 1280000 (65%)
>              on-stack:	  264000	 902000		 1280000 (67%)
> 
> v2: Use 512-bytes of stack for batching rather than allocate a page.
> v3: Tidy the code slightly with more descriptive variable names
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
... and also queued up for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48