[igt-dev] [RFC PATCH i-g-t v4 4/4] tests/gem_ctx_shared: Align objects using minimum GTT alignment

[Chris Wilson]

Quoting Janusz Krzysztofik (2019-11-04 09:39:32)
> Hi Chris,
> 
> On Friday, November 1, 2019 10:42:18 AM CET Chris Wilson wrote:
> > Quoting Janusz Krzysztofik (2019-10-31 15:28:57)
> > > exec-shared-gtt-* subtests use hardcoded values for object size and
> > > softpin offset, based on 4kB GTT alignment assumption.  That may result
> > > in those subtests failing when run on future backing stores with
> > > possibly larger minimum page sizes.
> > > 
> > > Replace hardcoded constants with values calculated from minimum GTT
> > > alignment of actual backing store the test is running on.
> > > 
> > > v2: Update helper name, use 'minimum GTT alignment' term across the
> > >     change, adjust variable name.
> > > 
> > > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik at linux.intel.com>
> > > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > > ---
> > >  tests/i915/gem_ctx_shared.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/tests/i915/gem_ctx_shared.c b/tests/i915/gem_ctx_shared.c
> > > index 6d8cbcce..1e9c7f78 100644
> > > --- a/tests/i915/gem_ctx_shared.c
> > > +++ b/tests/i915/gem_ctx_shared.c
> > > @@ -195,6 +195,7 @@ static void exec_shared_gtt(int i915, unsigned int 
> ring)
> > >         uint32_t scratch, *s;
> > >         uint32_t batch, cs[16];
> > >         uint64_t offset;
> > > +       uint64_t alignment;
> > >         int i;
> > >  
> > >         gem_require_ring(i915, ring);
> > > @@ -203,7 +204,8 @@ static void exec_shared_gtt(int i915, unsigned int 
> ring)
> > >         clone = gem_context_clone(i915, 0, I915_CONTEXT_CLONE_VM, 0);
> > >  
> > >         /* Find a hole big enough for both objects later */
> > > -       scratch = gem_create(i915, 16384);
> > > +       alignment = 2 * gem_gtt_min_alignment(i915);
> 
> alignment = one page for an object + one page for stride
> 
> > > +       scratch = gem_create(i915, 2 * alignment);
> 
> space reserved = 2 * (one page for an object + one page for stride)
> 
> > >         gem_write(i915, scratch, 0, &bbe, sizeof(bbe));
> > >         obj.handle = scratch;
> > >         gem_execbuf(i915, &execbuf);
> > > @@ -246,7 +248,7 @@ static void exec_shared_gtt(int i915, unsigned int 
> ring)
> > >         gem_write(i915, batch, 0, cs, sizeof(cs));
> > >  
> > >         obj.handle = batch;
> > > -       obj.offset += 8192; /* make sure we don't cause an eviction! */
> > > +       obj.offset += alignment; /* make sure we don't cause an eviction! 
> */
> 
> offset += (one page for an object + one page for stride)
> 
> > It's 'stride' here. It's leaving a guard page in between, just in case
> > page coloring demands it.
> 
> Assuming I correctly understood the intentions here but my implementation 
> is not readable clearly enough, how do you think this should be fixed?  How 
> about a comment '/* one page for an object + one page for stride */' above the 
> line where the 'alignment' variable is assigned the value of 2 * minimum GTT 
> alignment?

I think "stride = 2 * min_alignment()" concisely describes the intent.
-Chris