[igt-dev] [PATCH i-g-t 2/2] tests/i915-query: add new tests for perf configurations queries

[Chris Wilson]

Quoting Lionel Landwerlin (2018-06-06 12:35:23)
> On 06/06/18 11:32, Chris Wilson wrote:
> > Quoting Lionel Landwerlin (2018-06-06 11:19:20)
> >> On 05/06/18 19:47, Chris Wilson wrote:
> >>> Quoting Lionel Landwerlin (2018-06-05 17:23:23)
> >>>> +       len = ALIGN(query.oa.n_mux_regs * sizeof(uint32_t) * 2, 4096);
> >>>> +       data = mmap(0, len, PROT_WRITE, MAP_PRIVATE | MAP_ANON, -1, 0);
> >>>> +       memset(data, 0, len);
> >>>> +       mprotect(data, len, PROT_READ);
> >>>> +       query.oa.mux_regs_ptr = to_user_pointer(data);
> >>>> +       igt_assert_eq(-EFAULT,
> >>>> +                     query_perf_config_data(fd, sizeof(query), &query.query));
> >>> Just the random thought, have we stuffed gem_mmap__gtt() addresses
> >>> through these yet?
> >>> -Chris
> >>>
> >> I don't understand why this would be an issue.
> > It's a very easy way to be caught out with a lock inversion, as the
> > kernel faults on the user provided GGTT address and calls into
> > i915_gem_fault().
> > -Chris
> >
> Just tried and it seems to work fine.
> I'm not sure what you expected... Is it that if we take a lock on the 
> device from the query api, then we deadlock?
> If true that would be okay, because we're not taking a lock on that (yet).

I didn't expect it to fail! I was just saying that this is something we
need to test as the GGTT mmap is a most peculiar pointer, and it's easy
for us to be caught out by lock inversion when using it. (Also you need
to be sure that we do cause a fault on access inside the kernel which
may be tricky; as well as there being lots of different permutations on
buffer state dictating different paths and locks taken through
i915_gem_fault().)

Back to dreaming of automating all of this.
-Chris