[i-g-t,4/6] benchmarks/ tests/i915/: drop usage of __gem_mmap__cpu in code

Submitted by Kalamarz, Lukasz on Jan. 9, 2019, 5:40 p.m.

Details

Message ID 20190109174057.16835-4-lukasz.kalamarz@intel.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in IGT

Not browsing as part of any series.

Commit Message

Kalamarz, Lukasz Jan. 9, 2019, 5:40 p.m.
With implementation of __gem_mmap we can drop __gem_mmap__cpu
and instead use refactored function.

Signed-off-by: Lukasz Kalamarz <lukasz.kalamarz@intel.com>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Katarzyna Dec <katarzyna.dec@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 benchmarks/gem_exec_reloc.c      | 6 +++---
 tests/i915/gem_exec_big.c        | 2 +-
 tests/i915/gem_exec_lut_handle.c | 6 +++---
 tests/i915/gem_mmap.c            | 4 ++--
 tests/i915/gem_stolen.c          | 2 +-
 5 files changed, 10 insertions(+), 10 deletions(-)

Patch hide | download patch | download mbox

diff --git a/benchmarks/gem_exec_reloc.c b/benchmarks/gem_exec_reloc.c
index f9487d95..40a1d500 100644
--- a/benchmarks/gem_exec_reloc.c
+++ b/benchmarks/gem_exec_reloc.c
@@ -116,13 +116,13 @@  static int run(unsigned batch_size,
 	if (num_relocs) {
 		size = ALIGN(sizeof(*mem_reloc)*num_relocs, 4096);
 		reloc_handle = gem_create(fd, size);
-		reloc = __gem_mmap__cpu(fd, reloc_handle, 0, size, PROT_READ | PROT_WRITE);
+		reloc = __gem_mmap(fd, reloc_handle, 0, size, PROT_READ | PROT_WRITE, false);
 		memcpy(reloc, mem_reloc, sizeof(*mem_reloc)*num_relocs);
 		munmap(reloc, size);
 
 		if (flags & FAULT) {
 			igt_disable_prefault();
-			reloc = __gem_mmap__cpu(fd, reloc_handle, 0, size, PROT_READ | PROT_WRITE);
+			reloc = __gem_mmap(fd, reloc_handle, 0, size, PROT_READ | PROT_WRITE, false);
 		} else
 			reloc = mem_reloc;
 	}
@@ -163,7 +163,7 @@  static int run(unsigned batch_size,
 			}
 			if (flags & FAULT && reloc) {
 				munmap(reloc, size);
-				reloc = __gem_mmap__cpu(fd, reloc_handle, 0, size, PROT_READ | PROT_WRITE);
+				reloc = __gem_mmap(fd, reloc_handle, 0, size, PROT_READ | PROT_WRITE, false);
 				gem_exec[num_objects].relocs_ptr = (uintptr_t)reloc;
 			}
 			gem_execbuf(fd, &execbuf);
diff --git a/tests/i915/gem_exec_big.c b/tests/i915/gem_exec_big.c
index a15672f6..b57560af 100644
--- a/tests/i915/gem_exec_big.c
+++ b/tests/i915/gem_exec_big.c
@@ -220,7 +220,7 @@  igt_simple_main
 		gem_write(fd, handle, 0, batch, sizeof(batch));
 
 		if (!FORCE_PREAD_PWRITE && gem_has_llc(fd))
-			ptr = __gem_mmap__cpu(fd, handle, 0, batch_size, PROT_READ);
+			ptr = __gem_mmap(fd, handle, 0, batch_size, PROT_READ, false);
 		else if (!FORCE_PREAD_PWRITE && gem_mmap__has_wc(fd))
 			ptr = __gem_mmap__wc(fd, handle, 0, batch_size, PROT_READ);
 		else
diff --git a/tests/i915/gem_exec_lut_handle.c b/tests/i915/gem_exec_lut_handle.c
index 98e6ae5a..9929f0c7 100644
--- a/tests/i915/gem_exec_lut_handle.c
+++ b/tests/i915/gem_exec_lut_handle.c
@@ -148,7 +148,7 @@  igt_simple_main
 				struct timeval start, end;
 
 				if (p->flags & FAULT)
-					reloc = __gem_mmap__cpu(fd, reloc_handle, 0, size, PROT_READ | PROT_WRITE);
+					reloc = __gem_mmap(fd, reloc_handle, 0, size, PROT_READ | PROT_WRITE, false);
 				else
 					reloc = mem_reloc;
 
@@ -182,7 +182,7 @@  igt_simple_main
 					}
 					if (p->flags & FAULT) {
 						munmap(reloc, size);
-						reloc = __gem_mmap__cpu(fd, reloc_handle, 0, size, PROT_READ | PROT_WRITE);
+						reloc = __gem_mmap(fd, reloc_handle, 0, size, PROT_READ | PROT_WRITE, false);
 						gem_exec[MAX_NUM_EXEC].relocs_ptr = to_user_pointer(reloc);
 					}
 					gem_execbuf(fd, &execbuf);
@@ -212,7 +212,7 @@  igt_simple_main
 					}
 					if (p->flags & FAULT) {
 						munmap(reloc, size);
-						reloc = __gem_mmap__cpu(fd, reloc_handle, 0, size, PROT_READ | PROT_WRITE);
+						reloc = __gem_mmap(fd, reloc_handle, 0, size, PROT_READ | PROT_WRITE, false);
 						gem_exec[MAX_NUM_EXEC].relocs_ptr = to_user_pointer(reloc);
 					}
 					gem_execbuf(fd, &execbuf);
diff --git a/tests/i915/gem_mmap.c b/tests/i915/gem_mmap.c
index 0ed15878..5cbea456 100644
--- a/tests/i915/gem_mmap.c
+++ b/tests/i915/gem_mmap.c
@@ -80,8 +80,8 @@  test_huge_bo(int huge)
 	bo = gem_create(fd, huge_object_size);
 
 	/* Obtain CPU mapping for the object. */
-	ptr_cpu = __gem_mmap__cpu(fd, bo, 0, huge_object_size,
-				PROT_READ | PROT_WRITE);
+	ptr_cpu = __gem_mmap(fd, bo, 0, huge_object_size,
+			     PROT_READ | PROT_WRITE, false);
 	igt_require(ptr_cpu);
 	gem_set_domain(fd, bo, I915_GEM_DOMAIN_CPU, I915_GEM_DOMAIN_CPU);
 	gem_close(fd, bo);
diff --git a/tests/i915/gem_stolen.c b/tests/i915/gem_stolen.c
index 1d489976..9b88c440 100644
--- a/tests/i915/gem_stolen.c
+++ b/tests/i915/gem_stolen.c
@@ -392,7 +392,7 @@  stolen_no_mmap(int fd)
 
 	handle = gem_create_stolen(fd, SIZE);
 
-	addr = __gem_mmap__cpu(fd, handle, 0, SIZE, PROT_READ | PROT_WRITE);
+	addr = __gem_mmap(fd, handle, 0, SIZE, PROT_READ | PROT_WRITE, false);
 	igt_assert(addr == NULL);
 
 	gem_close(fd, handle);

Comments

On 01/09/2019 09:40 AM, Lukasz Kalamarz wrote:
> With implementation of __gem_mmap we can drop __gem_mmap__cpu
> and instead use refactored function.

Ok now I see why you didn't make it static in patch 2 :)
I'd personally prefer to keep __gem_mmap__cpu and __gem_mmap__wc as 
they're paired with gem_mmap__cpu and gem_mmap__wc and also make it 
explicit what mapping we're going for, but I'm not going to block if the 
majority prefers to get rid of them.

> 
> Signed-off-by: Lukasz Kalamarz <lukasz.kalamarz@intel.com>
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Cc: Katarzyna Dec <katarzyna.dec@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
>   benchmarks/gem_exec_reloc.c      | 6 +++---
>   tests/i915/gem_exec_big.c        | 2 +-
>   tests/i915/gem_exec_lut_handle.c | 6 +++---
>   tests/i915/gem_mmap.c            | 4 ++--
>   tests/i915/gem_stolen.c          | 2 +-
>   5 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/benchmarks/gem_exec_reloc.c b/benchmarks/gem_exec_reloc.c
> index f9487d95..40a1d500 100644
> --- a/benchmarks/gem_exec_reloc.c
> +++ b/benchmarks/gem_exec_reloc.c
> @@ -116,13 +116,13 @@ static int run(unsigned batch_size,
>   	if (num_relocs) {
>   		size = ALIGN(sizeof(*mem_reloc)*num_relocs, 4096);
>   		reloc_handle = gem_create(fd, size);
> -		reloc = __gem_mmap__cpu(fd, reloc_handle, 0, size, PROT_READ | PROT_WRITE);
> +		reloc = __gem_mmap(fd, reloc_handle, 0, size, PROT_READ | PROT_WRITE, false);

I'm wondering what the reason is not to use gem_mmap__cpu here and in 
the other cases where we pass the mapping as a reloc. From what I can 
see we do access the pointer (e.g. with the memcpy the line below or 
passing it to execbuf), so I'd expect we'd want to make sure it is valid

Daniele

>   		memcpy(reloc, mem_reloc, sizeof(*mem_reloc)*num_relocs);
>   		munmap(reloc, size);
>   
>   		if (flags & FAULT) {
>   			igt_disable_prefault();
> -			reloc = __gem_mmap__cpu(fd, reloc_handle, 0, size, PROT_READ | PROT_WRITE);
> +			reloc = __gem_mmap(fd, reloc_handle, 0, size, PROT_READ | PROT_WRITE, false);
>   		} else
>   			reloc = mem_reloc;
>   	}
> @@ -163,7 +163,7 @@ static int run(unsigned batch_size,
>   			}
>   			if (flags & FAULT && reloc) {
>   				munmap(reloc, size);
> -				reloc = __gem_mmap__cpu(fd, reloc_handle, 0, size, PROT_READ | PROT_WRITE);
> +				reloc = __gem_mmap(fd, reloc_handle, 0, size, PROT_READ | PROT_WRITE, false);
>   				gem_exec[num_objects].relocs_ptr = (uintptr_t)reloc;
>   			}
>   			gem_execbuf(fd, &execbuf);
> diff --git a/tests/i915/gem_exec_big.c b/tests/i915/gem_exec_big.c
> index a15672f6..b57560af 100644
> --- a/tests/i915/gem_exec_big.c
> +++ b/tests/i915/gem_exec_big.c
> @@ -220,7 +220,7 @@ igt_simple_main
>   		gem_write(fd, handle, 0, batch, sizeof(batch));
>   
>   		if (!FORCE_PREAD_PWRITE && gem_has_llc(fd))
> -			ptr = __gem_mmap__cpu(fd, handle, 0, batch_size, PROT_READ);
> +			ptr = __gem_mmap(fd, handle, 0, batch_size, PROT_READ, false);
>   		else if (!FORCE_PREAD_PWRITE && gem_mmap__has_wc(fd))
>   			ptr = __gem_mmap__wc(fd, handle, 0, batch_size, PROT_READ);
>   		else
> diff --git a/tests/i915/gem_exec_lut_handle.c b/tests/i915/gem_exec_lut_handle.c
> index 98e6ae5a..9929f0c7 100644
> --- a/tests/i915/gem_exec_lut_handle.c
> +++ b/tests/i915/gem_exec_lut_handle.c
> @@ -148,7 +148,7 @@ igt_simple_main
>   				struct timeval start, end;
>   
>   				if (p->flags & FAULT)
> -					reloc = __gem_mmap__cpu(fd, reloc_handle, 0, size, PROT_READ | PROT_WRITE);
> +					reloc = __gem_mmap(fd, reloc_handle, 0, size, PROT_READ | PROT_WRITE, false);
>   				else
>   					reloc = mem_reloc;
>   
> @@ -182,7 +182,7 @@ igt_simple_main
>   					}
>   					if (p->flags & FAULT) {
>   						munmap(reloc, size);
> -						reloc = __gem_mmap__cpu(fd, reloc_handle, 0, size, PROT_READ | PROT_WRITE);
> +						reloc = __gem_mmap(fd, reloc_handle, 0, size, PROT_READ | PROT_WRITE, false);
>   						gem_exec[MAX_NUM_EXEC].relocs_ptr = to_user_pointer(reloc);
>   					}
>   					gem_execbuf(fd, &execbuf);
> @@ -212,7 +212,7 @@ igt_simple_main
>   					}
>   					if (p->flags & FAULT) {
>   						munmap(reloc, size);
> -						reloc = __gem_mmap__cpu(fd, reloc_handle, 0, size, PROT_READ | PROT_WRITE);
> +						reloc = __gem_mmap(fd, reloc_handle, 0, size, PROT_READ | PROT_WRITE, false);
>   						gem_exec[MAX_NUM_EXEC].relocs_ptr = to_user_pointer(reloc);
>   					}
>   					gem_execbuf(fd, &execbuf);
> diff --git a/tests/i915/gem_mmap.c b/tests/i915/gem_mmap.c
> index 0ed15878..5cbea456 100644
> --- a/tests/i915/gem_mmap.c
> +++ b/tests/i915/gem_mmap.c
> @@ -80,8 +80,8 @@ test_huge_bo(int huge)
>   	bo = gem_create(fd, huge_object_size);
>   
>   	/* Obtain CPU mapping for the object. */
> -	ptr_cpu = __gem_mmap__cpu(fd, bo, 0, huge_object_size,
> -				PROT_READ | PROT_WRITE);
> +	ptr_cpu = __gem_mmap(fd, bo, 0, huge_object_size,
> +			     PROT_READ | PROT_WRITE, false);
>   	igt_require(ptr_cpu);
>   	gem_set_domain(fd, bo, I915_GEM_DOMAIN_CPU, I915_GEM_DOMAIN_CPU);
>   	gem_close(fd, bo);
> diff --git a/tests/i915/gem_stolen.c b/tests/i915/gem_stolen.c
> index 1d489976..9b88c440 100644
> --- a/tests/i915/gem_stolen.c
> +++ b/tests/i915/gem_stolen.c
> @@ -392,7 +392,7 @@ stolen_no_mmap(int fd)
>   
>   	handle = gem_create_stolen(fd, SIZE);
>   
> -	addr = __gem_mmap__cpu(fd, handle, 0, SIZE, PROT_READ | PROT_WRITE);
> +	addr = __gem_mmap(fd, handle, 0, SIZE, PROT_READ | PROT_WRITE, false);
>   	igt_assert(addr == NULL);
>   
>   	gem_close(fd, handle);
>
Quoting Lukasz Kalamarz (2019-01-09 17:40:55)
> With implementation of __gem_mmap we can drop __gem_mmap__cpu
> and instead use refactored function.

No, I would rather keep the mode (__cpu, __wc, __gtt) explicit.
-Chris