[Intel-gfx] [PATCH] i915/tests/gem_exec_big: prefer PROT_WRITE

[Intel-gfx] [PATCH] i915/tests/gem_exec_big: prefer PROT_WRITE

[Matthew Auld]

Technically mmap__cpu and mmap__wc just ignore the prot value, so it
doesn't really matter, but the intention is to have write access to the
ptr, so make that clear. Also if the underlying mmap__wc were to at some
point use mmap_offset where the prot is not ignored then we won't have
any surprises.

Signed-off-by: Matthew Auld <matthew.auld@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/i915/gem_exec_big.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tests/i915/gem_exec_big.c b/tests/i915/gem_exec_big.c
index c06ee995..a5869330 100644
--- a/tests/i915/gem_exec_big.c
+++ b/tests/i915/gem_exec_big.c
@@ -217,9 +217,9 @@ static void exhaustive(int fd)
 		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__cpu(fd, handle, 0, batch_size, PROT_WRITE);
 		else if (!FORCE_PREAD_PWRITE && gem_mmap__has_wc(fd))
-			ptr = __gem_mmap__wc(fd, handle, 0, batch_size, PROT_READ);
+			ptr = __gem_mmap__wc(fd, handle, 0, batch_size, PROT_WRITE);
 		else
 			ptr = NULL;
 
@@ -281,9 +281,9 @@ static void single(int i915)
 	gem_write(i915, handle, 0, &bbe, sizeof(bbe));
 
 	if (!FORCE_PREAD_PWRITE && gem_has_llc(i915))
-		ptr = __gem_mmap__cpu(i915, handle, 0, batch_size, PROT_READ);
+		ptr = __gem_mmap__cpu(i915, handle, 0, batch_size, PROT_WRITE);
 	else if (!FORCE_PREAD_PWRITE && gem_mmap__has_wc(i915))
-		ptr = __gem_mmap__wc(i915, handle, 0, batch_size, PROT_READ);
+		ptr = __gem_mmap__wc(i915, handle, 0, batch_size, PROT_WRITE);
 	else
 		ptr = NULL;
 
-- 
2.20.1

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

Re: [Intel-gfx] [PATCH] i915/tests/gem_exec_big: prefer PROT_WRITE

[Chris Wilson]

Quoting Matthew Auld (2020-01-24 16:41:31)
> Technically mmap__cpu and mmap__wc just ignore the prot value, so it
> doesn't really matter, but the intention is to have write access to the
> ptr, so make that clear. Also if the underlying mmap__wc were to at some
> point use mmap_offset where the prot is not ignored then we won't have
> any surprises.

The ptr here was just meant for cheaply reading back from the buffer to
verify the relocation took place. E.g., 

-static void exec1(int fd, uint32_t handle, uint64_t reloc_ofs, unsigned flags, char *ptr)
+static void exec1(int fd, uint32_t handle, uint64_t reloc_ofs, unsigned flags, const char *ptr)
 {
        struct drm_i915_gem_execbuffer2 execbuf;
        struct drm_i915_gem_exec_object2 gem_exec[1];
@@ -126,7 +126,7 @@ static void xchg_reloc(void *array, unsigned i, unsigned j)
        *b = tmp;
 }

-static void execN(int fd, uint32_t handle, uint64_t batch_size, unsigned flags, char *ptr)
+static void execN(int fd, uint32_t handle, uint64_t batch_size, unsigned flags, const char *ptr)
 {

What am I missing?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] i915/tests/gem_exec_big: prefer PROT_WRITE

[Matthew Auld]

On 24/01/2020 16:46, Chris Wilson wrote:
> Quoting Matthew Auld (2020-01-24 16:41:31)
>> Technically mmap__cpu and mmap__wc just ignore the prot value, so it
>> doesn't really matter, but the intention is to have write access to the
>> ptr, so make that clear. Also if the underlying mmap__wc were to at some
>> point use mmap_offset where the prot is not ignored then we won't have
>> any surprises.
> 
> The ptr here was just meant for cheaply reading back from the buffer to
> verify the relocation took place. E.g.,
> 
> -static void exec1(int fd, uint32_t handle, uint64_t reloc_ofs, unsigned flags, char *ptr)
> +static void exec1(int fd, uint32_t handle, uint64_t reloc_ofs, unsigned flags, const char *ptr)
>   {
>          struct drm_i915_gem_execbuffer2 execbuf;
>          struct drm_i915_gem_exec_object2 gem_exec[1];
> @@ -126,7 +126,7 @@ static void xchg_reloc(void *array, unsigned i, unsigned j)
>          *b = tmp;
>   }
> 
> -static void execN(int fd, uint32_t handle, uint64_t batch_size, unsigned flags, char *ptr)
> +static void execN(int fd, uint32_t handle, uint64_t batch_size, unsigned flags, const char *ptr)
>   {
> 
> What am I missing?

*(uint64_t *)(ptr + gem_reloc[n].offset) = gem_reloc[n].presumed_offset;

?

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

Re: [Intel-gfx] [PATCH] i915/tests/gem_exec_big: prefer PROT_WRITE

[Chris Wilson]

Quoting Matthew Auld (2020-01-24 16:54:42)
> On 24/01/2020 16:46, Chris Wilson wrote:
> > Quoting Matthew Auld (2020-01-24 16:41:31)
> >> Technically mmap__cpu and mmap__wc just ignore the prot value, so it
> >> doesn't really matter, but the intention is to have write access to the
> >> ptr, so make that clear. Also if the underlying mmap__wc were to at some
> >> point use mmap_offset where the prot is not ignored then we won't have
> >> any surprises.
> > 
> > The ptr here was just meant for cheaply reading back from the buffer to
> > verify the relocation took place. E.g.,
> > 
> > -static void exec1(int fd, uint32_t handle, uint64_t reloc_ofs, unsigned flags, char *ptr)
> > +static void exec1(int fd, uint32_t handle, uint64_t reloc_ofs, unsigned flags, const char *ptr)
> >   {
> >          struct drm_i915_gem_execbuffer2 execbuf;
> >          struct drm_i915_gem_exec_object2 gem_exec[1];
> > @@ -126,7 +126,7 @@ static void xchg_reloc(void *array, unsigned i, unsigned j)
> >          *b = tmp;
> >   }
> > 
> > -static void execN(int fd, uint32_t handle, uint64_t batch_size, unsigned flags, char *ptr)
> > +static void execN(int fd, uint32_t handle, uint64_t batch_size, unsigned flags, const char *ptr)
> >   {
> > 
> > What am I missing?
> 
> *(uint64_t *)(ptr + gem_reloc[n].offset) = gem_reloc[n].presumed_offset;

They say your memory is the first to go.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✗ Fi.CI.BUILD: failure for i915/tests/gem_exec_big: prefer PROT_WRITE

[Patchwork]

== Series Details ==

Series: i915/tests/gem_exec_big: prefer PROT_WRITE
URL   : https://patchwork.freedesktop.org/series/72544/
State : failure

== Summary ==

Applying: i915/tests/gem_exec_big: prefer PROT_WRITE
error: sha1 information is lacking or useless (tests/i915/gem_exec_big.c).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch' to see the failed patch
Patch failed at 0001 i915/tests/gem_exec_big: prefer PROT_WRITE
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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