[Intel-gfx] [igt-dev] [PATCH i-g-t] i915/gem_mmap_gtt: Markup a copy of GTT set-domains

[Chris Wilson]

Quoting Ville Syrjälä (2019-04-15 13:23:54)
> On Sat, Apr 13, 2019 at 02:27:57PM +0100, Chris Wilson wrote:
> > We have to control the cache domains, especially important before first
> > writing into the object.
> > 
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > ---
> >  tests/i915/gem_mmap_gtt.c | 26 ++++++++++++++++----------
> >  1 file changed, 16 insertions(+), 10 deletions(-)
> > 
> > diff --git a/tests/i915/gem_mmap_gtt.c b/tests/i915/gem_mmap_gtt.c
> > index 58922ee32..ab7d3f2d0 100644
> > --- a/tests/i915/gem_mmap_gtt.c
> > +++ b/tests/i915/gem_mmap_gtt.c
> > @@ -678,27 +678,26 @@ test_huge_copy(int fd, int huge, int tiling_a, int tiling_b, int ncpus)
> >  
> >       igt_fork(child, ncpus) {
> >               uint64_t valid_size = huge_object_size;
> > -             uint32_t bo;
> > +             uint32_t bo[2];
> >               char *a, *b;
> >  
> > -             bo = gem_create(fd, huge_object_size);
> > +             bo[0] = gem_create(fd, huge_object_size);
> >               if (tiling_a) {
> > -                     igt_require(__gem_set_tiling(fd, bo, abs(tiling_a), min_tile_width(devid, tiling_a)) == 0);
> > +                     igt_require(__gem_set_tiling(fd, bo[0], abs(tiling_a), min_tile_width(devid, tiling_a)) == 0);
> >                       valid_size = rounddown(valid_size, tile_row_size(tiling_a, min_tile_width(devid, tiling_a)));
> >               }
> > -             a = __gem_mmap__gtt(fd, bo, huge_object_size, PROT_READ | PROT_WRITE);
> > +             a = __gem_mmap__gtt(fd, bo[0], huge_object_size, PROT_READ | PROT_WRITE);
> >               igt_require(a);
> > -             gem_close(fd, bo);
> >  
> > -             bo = gem_create(fd, huge_object_size);
> > +             bo[1] = gem_create(fd, huge_object_size);
> >               if (tiling_b) {
> > -                     igt_require(__gem_set_tiling(fd, bo, abs(tiling_b), max_tile_width(devid, tiling_b)) == 0);
> > +                     igt_require(__gem_set_tiling(fd, bo[1], abs(tiling_b), max_tile_width(devid, tiling_b)) == 0);
> >                       valid_size = rounddown(valid_size, tile_row_size(tiling_b, max_tile_width(devid, tiling_b)));
> >               }
> > -             b = __gem_mmap__gtt(fd, bo, huge_object_size, PROT_READ | PROT_WRITE);
> > +             b = __gem_mmap__gtt(fd, bo[1], huge_object_size, PROT_READ | PROT_WRITE);
> >               igt_require(b);
> > -             gem_close(fd, bo);
> >  
> > +             gem_set_domain(fd, bo[0], I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
> >               for (i = 0; i < valid_size / PAGE_SIZE; i++) {
> >                       uint32_t *ptr = (uint32_t *)(a + PAGE_SIZE*i);
> >                       for (int j = 0; j < PAGE_SIZE/4; j++)
> > @@ -706,7 +705,7 @@ test_huge_copy(int fd, int huge, int tiling_a, int tiling_b, int ncpus)
> >                       igt_progress("Writing a ", i, valid_size / PAGE_SIZE);
> >               }
> >  
> > -
> > +             gem_set_domain(fd, bo[1], I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
> >               for (i = 0; i < valid_size / PAGE_SIZE; i++) {
> >                       uint32_t *ptr = (uint32_t *)(b + PAGE_SIZE*i);
> >                       for (int j = 0; j < PAGE_SIZE/4; j++)
> > @@ -727,12 +726,19 @@ test_huge_copy(int fd, int huge, int tiling_a, int tiling_b, int ncpus)
> >                                       A_tmp[j] = B_tmp[j];
> >                               else
> >                                       B_tmp[j] = A_tmp[j];
> > +
> > +                     gem_set_domain(fd, bo[0], I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
> 
> why do we repeat this here? I can't see anything that would change the
> domains after the first time we set it.

Just nervous; the one aspect I thoroughly dislike about set-domain is
that it is one sided and does not define a critical section over which
its applies. As such, anything else may change the domain in the middle
of our operation, and that unnerves me.

Making it double-sided has the dilemma of how best to lie to userspace
when you steal control, or how to fixup userspace who forgot to release
their lock.

It's easy to say not to repeat this in future api; defining what else
should be done instead is trickier. At present, such sketches all imply
that userspace is entirely responsible for controlling coherency with
consumers and swapping remains an invisible cache flush.
-Chris