i915/tests/gem_exec_big: prefer PROT_WRITE
diff mbox series

Message ID 20200124164131.39591-1-matthew.auld@intel.com
State New
Headers show
Series
  • i915/tests/gem_exec_big: prefer PROT_WRITE
Related show

Commit Message

Matthew Auld Jan. 24, 2020, 4:41 p.m. UTC
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(-)

Comments

Chris Wilson Jan. 24, 2020, 4:46 p.m. UTC | #1
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
Matthew Auld Jan. 24, 2020, 4:54 p.m. UTC | #2
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
>
Chris Wilson Jan. 24, 2020, 5:10 p.m. UTC | #3
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

Patch
diff mbox series

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;