[igt-dev] [PATCH i-g-t 2/2] lib/i915/gem_mman: mmap to the CPU instead of the GTT in some tests

[Summers, Stuart]

On Tue, 2019-11-19 at 19:20 +0000, Chris Wilson wrote:
> Quoting Summers, Stuart (2019-11-19 18:58:31)
> > On Tue, 2019-11-19 at 18:25 +0000, Chris Wilson wrote:
> > > Quoting Stuart Summers (2019-11-19 18:16:19)
> > > > Do not limit to the GTT for tests which are not specifically
> > > > testing
> > > > capability in the GTT.
> > > > 
> > > > Signed-off-by: Stuart Summers <stuart.summers at intel.com>
> > > > ---
> > > >  lib/i915/gem_mman.c            | 19 +++++++++++++++++++
> > > >  lib/i915/gem_mman.h            |  1 +
> > > >  tests/i915/gem_ctx_shared.c    | 20 +++++---------------
> > > >  tests/i915/gem_exec_schedule.c |  3 +--
> > > >  4 files changed, 26 insertions(+), 17 deletions(-)
> > > > 
> > > > diff --git a/lib/i915/gem_mman.c b/lib/i915/gem_mman.c
> > > > index a0e34aef..d37840f7 100644
> > > > --- a/lib/i915/gem_mman.c
> > > > +++ b/lib/i915/gem_mman.c
> > > > @@ -40,6 +40,25 @@
> > > >  #define VG(x) do {} while (0)
> > > >  #endif
> > > >  
> > > > +void *gem_mmap_host(int fd, uint32_t handle, uint64_t size,
> > > > unsigned prot)
> > > > +{
> > > > +       void *ptr;
> > > > +       uint32_t domain;
> > > > +
> > > > +       if (gem_has_mappable_ggtt(fd)) {
> > > > +               ptr = gem_mmap__gtt(fd, handle, size, prot);
> > > > +               domain = I915_GEM_DOMAIN_GTT;
> > > > +       } else {
> > > > +               ptr = gem_mmap__cpu(fd, handle, 0, size, prot);
> > > > +               domain = I915_GEM_DOMAIN_CPU;
> > > > +       }
> > > > +
> > > > +       gem_set_domain(fd, handle, /* no write hazard lies! */
> > > > +                      domain, domain);
> > > 
> > > And be very careful putting the set-domain in here; as the lies
> > > are
> > > integral to the test and not suitable for libraries. Domain
> > > handling
> > > is
> > > sadly of paramount importance for most tests.
> > 
> > Hm.. so maybe a little more in the kernel to safeguard here. I was
> > thinking of doing that anyway. Then we can adopt the tests as
> > needed.
> 
> /* no write hazard lies! */
> 
> is all about how we are circumventing the kernel's opinion on correct
> behaviour to get at the behaviour we require ;)
> 
> > And for these, maybe the right way to go is to use the new
> > mmap_offset
> > Zbigniew is implementing. The IOCTL posted by Abdiel already has a
> > WC
> > case for that.
> 
> Yes. Pretty much all of these should be using gem_mmap__wc(), and
> whether that is implemented using mmap_offset_ioctl or mmap_ioctl is
> immaterial. The CPUs / platforms that cannot use WC are immaterial to
> most of these tests (these test in particular are only applicable to
> recent gen), but for a select few tests we will need to make sure
> that
> they are still usable with gem_mmap__gtt.
> 
> So a possible gem_mmap__device_coherent() {
> 	ptr = gem_mmap__offset(.type = WC);
> 	if (ptr)
> 		return ptr;
> 
> 	ptr = gem_mmap__wc();
> 	if (ptr)
> 		return ptr;
> 
> 	return gem_mmap__gt();
> }

Yeah I've been playing around locally with something like this (or at
least with the mmap_offset/mmap_gtt, your suggestion is cleaner). The
problem is in application. Do we apply this to all tests which aren't
expliclity doing GGTT testing? Or do we start with something like the
gem_ctx_shared and expand over time? I don't have all of the history
across the tests, so really appreciate the feedback here Chris.

Thanks,
Stuart

> Names and details left to the reader. We did decide that library
> functions like gem_mmap__wc() could use either mmap_offset_ioctl or
> mmap_ioctl as they saw fit; and tests that required exact ioctls
> would
> call those ioctls explicitly.
> -Chris
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 3270 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/igt-dev/attachments/20191119/66363f4a/attachment.bin>