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

[Ville Syrjälä]

On Mon, Apr 15, 2019 at 02:18:28PM +0100, Chris Wilson wrote:
> 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.

Fair enough.
Reviewed-by: Ville Syrjälä <ville.syrjala at linux.intel.com>

-- 
Ville Syrjälä
Intel