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

[Petri Latvala]

On Fri, Sep 02, 2022 at 12:08:16PM +0200, Zbigniew Kempczyński wrote:
> 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'.

Ah, so t->domain can never be -1?


-- 
Petri Latvala





> 
> 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
> > > >