Re: [igt-dev] [PATCH i-g-t 4/6] benchmarks/ tests/i915/: drop usage of __gem_mmap__cpu in code

[Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>]

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);
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev