[igt-dev] [PATCH i-g-t 03/12] lib/i915: Distinguish I915_MMAP_OFFSET_FIXED with an invalid domain

[Zbigniew Kempczyński]

On Fri, Sep 02, 2022 at 11:41:54AM +0300, Petri Latvala wrote:
> On Thu, Sep 01, 2022 at 07:58:13PM +0200, Zbigniew Kempczyński wrote:
> > On Thu, Sep 01, 2022 at 01:44:33PM +0200, Zbigniew Kempczyński wrote:
> > > From: Chris Wilson <chris.p.wilson at linux.intel.com>
> > > 
> > > Don't treat OFFSET_FIXED as using DOMAIN_CPU [0], but give it an invalid
> > > domain so we can avoid using in tests.
> > 
> > I915_GEM_DOMAIN_CPU == 1, so 
> > > 
> > > Signed-off-by: Chris Wilson <chris.p.wilson at linux.intel.com>
> > > ---
> > >  lib/i915/gem_mman.c          |  2 +-
> > >  tests/i915/gem_mmap_offset.c | 12 ++++++++----
> > >  2 files changed, 9 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/lib/i915/gem_mman.c b/lib/i915/gem_mman.c
> > > index aa9ac6f3d7..48e3f8c991 100644
> > > --- a/lib/i915/gem_mman.c
> > > +++ b/lib/i915/gem_mman.c
> > > @@ -678,7 +678,7 @@ const struct mmap_offset mmap_offset_types[] = {
> > >  	{ "wb", I915_MMAP_OFFSET_WB, I915_GEM_DOMAIN_CPU },
> > >  	{ "wc", I915_MMAP_OFFSET_WC, I915_GEM_DOMAIN_WC },
> > >  	{ "uc", I915_MMAP_OFFSET_UC, I915_GEM_DOMAIN_WC },
> > > -	{ "fixed", I915_MMAP_OFFSET_FIXED, 0},
> > > +	{ "fixed", I915_MMAP_OFFSET_FIXED, -1 },
> > 
> > this is not needed.
> > 
> > >  	{},
> > >  };
> > >  
> > > diff --git a/tests/i915/gem_mmap_offset.c b/tests/i915/gem_mmap_offset.c
> > > index 5e6b19eb34..d1075c32eb 100644
> > > --- a/tests/i915/gem_mmap_offset.c
> > > +++ b/tests/i915/gem_mmap_offset.c
> > > @@ -148,7 +148,8 @@ static void basic_uaf(int i915)
> > >  		}
> > >  
> > >  		expected = calloc(obj_size, sizeof(*expected));
> > > -		gem_set_domain(i915, handle, t->domain, 0);
> > > +		if (t->domain != -1)
> > 
> > Just if (t->domain) is enough here.
> 
> But that does different things, doesn't it?

Each I915_GEM_DOMAIN_* > 0 so change to put -1 is not necessary
and zero is enough for 'fixed'.

I've dropped change in gem_mman.c adding conditional to gem_mmap_offset
only:

https://patchwork.freedesktop.org/patch/500885/?series=108022&rev=2

--
Zbigniew

> 
> 
> -- 
> Petri Latvala
> 
> 
> > 
> > --
> > Zbigniew
> > 
> > > +			gem_set_domain(i915, handle, t->domain, 0);
> > >  		igt_assert_f(memcmp(addr, expected, obj_size) == 0,
> > >  			     "mmap(%s) not clear on gem_create()\n",
> > >  			     t->name);
> > > @@ -157,15 +158,18 @@ static void basic_uaf(int i915)
> > >  		buf = calloc(obj_size, sizeof(*buf));
> > >  		memset(buf + 1024, 0x01, 1024);
> > >  		gem_write(i915, handle, 0, buf, obj_size);
> > > -		gem_set_domain(i915, handle, t->domain, 0);
> > > +		if (t->domain != -1)
> > > +			gem_set_domain(i915, handle, t->domain, 0);
> > >  		igt_assert_f(memcmp(buf, addr, obj_size) == 0,
> > >  			     "mmap(%s) not coherent with gem_write()\n",
> > >  			     t->name);
> > >  
> > > -		gem_set_domain(i915, handle, t->domain, t->domain);
> > > +		if (t->domain != -1)
> > > +			gem_set_domain(i915, handle, t->domain, t->domain);
> > >  		memset(addr + 2048, 0xff, 1024);
> > >  		gem_read(i915, handle, 0, buf, obj_size);
> > > -		gem_set_domain(i915, handle, t->domain, 0);
> > > +		if (t->domain != -1)
> > > +			gem_set_domain(i915, handle, t->domain, 0);
> > >  		igt_assert_f(memcmp(buf, addr, obj_size) == 0,
> > >  			     "mmap(%s) not coherent with gem_read()\n",
> > >  			     t->name);
> > > -- 
> > > 2.34.1
> > >