[igt-dev] [PATCH i-g-t v5 2/3] tests: add i915 query tests

[Tvrtko Ursulin]

On 08/03/2018 14:31, Lionel Landwerlin wrote:
> On 08/03/18 11:22, Tvrtko Ursulin wrote:
>>
>> Commit message is missing.
>>
>> On 26/02/2018 17:59, Lionel Landwerlin wrote:
>>> v2: Complete invalid cases (Chris)
>>>      Some styling (to_user_pointer, etc...) (Chris)
>>>      New error check, through item.length (Chris)
>>>
>>> v3: Update for new uAPI iteration (Lionel)
>>>
>>> v4: Return errno from a single point (Chris)
>>>      Poising checks (Chris)
>>>
>>> v5: Add more debug traces (Lionel)
>>>      Update uAPI (Joonas/Lionel)
>>>      Make sure Haswell is tested (Lionel)
>>>
>>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
>>> ---
>>>   tests/Makefile.sources |   1 +
>>>   tests/i915_query.c     | 314 
>>> +++++++++++++++++++++++++++++++++++++++++++++++++
>>>   tests/meson.build      |   1 +
>>>   3 files changed, 316 insertions(+)
>>>   create mode 100644 tests/i915_query.c
>>>
>>> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
>>> index 23f859be..b4c8a913 100644
>>> --- a/tests/Makefile.sources
>>> +++ b/tests/Makefile.sources
>>> @@ -168,6 +168,7 @@ TESTS_progs = \
>>>       gen3_render_tiledy_blits \
>>>       gen7_forcewake_mt \
>>>       gvt_basic \
>>> +    i915_query \
>>
>> Interesting question how we want to name this test. We don't have any 
>> i915_ prefix tests, but for instance there is drv_getparams_basic, 
>> suggesting this could be called drv_query_ioctl or something?
>>
>> Open for discussion I guess.
> 
> I've been considering renaming perf.c (that's a pretty confusing one...)
> 
> One could argue that if IGT tests more than intel devices, we should 
> have a driver prefix (I see vc4 has).

Yes, to an extent, but we got prior use on drv_ so until a grander 
scheme is devised I'd stick with it. But no huge complaints towards 
keeping i915 either, it's not really that important.

>>> +
>>> +    memset(items, 0, sizeof(items));
>>> +    i915_query_item_err(fd, items, 1, EINVAL);
>>> +
>>> +    memset(items, 0, sizeof(items));
>>> +    items[0].query_id = ULONG_MAX;
>>> +    items[1].query_id = ULONG_MAX - 2;
>>
>> What is special about ULONG_MAX - 2 versus ULONG_MAX? And not 
>> ULONG_MAX - 1, or some other number?
> 
> Nothing really... It's just a really bit number that we're unlikely to 
> reach.

I'd maybe first go with one invalid query id, then second test try two 
different invalid ids. So it is at least more obvious what's happening.

Oh another missed test case: invalid query (in different ways) followed 
by a valid one. First must fail, second succeed. And in opposite order 
as well.

>>> +    items_ptr = mmap(0, 4096, PROT_WRITE, MAP_PRIVATE | MAP_ANON, 
>>> -1, 0);
>>> +    items_ptr[0].query_id = DRM_I915_QUERY_TOPOLOGY_INFO;
>>> +    i915_query_item(fd, items_ptr, 1);
>>> +    igt_assert(items_ptr[0].length >= sizeof(struct 
>>> drm_i915_query_topology_info));
>>> +    munmap(items_ptr, 4096);
>>> +    i915_query_item_err(fd, items_ptr, 1, EFAULT);
>>
>> Another good test would be passing in a read only mapping and checking 
>> for EFAULT when length writeback fails.
> 
> Hm... how to do you write something sensible into a read only mapping so 
> that the kernel would at least try to write to it? :)

Make it read-only after populating it. :) man mprotect.

>>
>>> +
>>> +    len = sizeof(struct drm_i915_query_item) * 10;
>>> +    items_ptr = mmap(0, len, PROT_WRITE, MAP_PRIVATE | MAP_ANON, -1, 
>>> 0);
>>> +    for (i = 0; i < 10; i++)
>>> +        items_ptr[i].query_id = DRM_I915_QUERY_TOPOLOGY_INFO;
>>> +    ret = __i915_query_item(fd, items_ptr,
>>> +                INT_MAX / sizeof(struct drm_i915_query_item) + 1);
>>> +    igt_assert(ret == -EFAULT || ret == -EINVAL);
>>
>> What is this testing? Ten queries, all write only so will EFAULT. But 
>> then you pass in nitems = INT_MAX / sizeof.. which is what exactly? I 
>> don't get it.
> 
> Trying to get the kernel to read into unmapped address space and 
> expecting an EFAULT or if we're unlucky and it ends up into another 
> mapping invalid query items.

You need to set up the mappings explicitly and not count on luck. Create 
exact sequence of mapped r/w, mapped-r, mapped-w and unmapped query 
items. You can use MAP_FIXED, mremap and mprotect to engineer exact 
layouts to test against.

Helper which processes input arrays like:

struct ... {
	query-id;
	enum { unmapped, ro, rw, w, NULL };
	len;
	expected-result;
	expected-lenght;
} query_items[] = {
	{ TOPOLOGY, rw, ... ,0, .. },
  	{ TOPOLOGY, ro, ..., -EFAULT, ... },
	{ INVALID, rw, ..., -EINVAL, ... },
	{ TOPOLOGY, w, ..., -EFAULT, ... },
	{ TOPOLOGY, NULL, 0, 0, len },
};

ret = test_items(fd, query_items, ARRAY_SIZE(query_items));
igt_assert_eq(ret, 0);

Comes to mind. Very roughly though, so see if it makes sense to you.

Then you can have different query item lists defined like that and just 
feed them to test helper. Even 2d array of test items for this class of 
test scenarios would work so you would just iterate it. Maybe store the 
expected return from the drm_i915_query ioctl for each list.

>>> +static void
>>> +test_query_topology_coherent_slice_mask(int fd)
>>> +{
>>> +    struct drm_i915_query_item item;
>>> +    uint8_t *_topo_info;
>>> +    struct drm_i915_query_topology_info *topo_info;
>>> +    drm_i915_getparam_t gp;
>>> +    int slice_mask, subslice_mask;
>>> +    int s, topology_slices, topology_subslices_slice0;
>>
>> Not in order of use, or width, never mind.
>>
>> Some of the types look like should be unsigned or u32?
> 
> getparam returns int, so I made the rest the same (since we want to 
> compare them).

igt_assert_eq_u32(int, int) below is what caught my eye. Don't know then.

>>
>>> +
>>> +    gp.param = I915_PARAM_SLICE_MASK;
>>> +    gp.value = &slice_mask;
>>> +    igt_skip_on(igt_ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp) != 0);
>>> +
>>> +    gp.param = I915_PARAM_SUBSLICE_MASK;
>>> +    gp.value = &subslice_mask;
>>> +    igt_skip_on(igt_ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp) != 0);
>>> +
>>> +    /* Slices */
>>> +    memset(&item, 0, sizeof(item));
>>> +    item.query_id = DRM_I915_QUERY_TOPOLOGY_INFO;
>>> +    i915_query_item(fd, &item, 1);
>>> +    igt_assert(item.length > 0);
>>
>> Is it possible to check for the exact value here? Or at least minimum 
>> amounting to the v1 query minimum?
> 
> Can't check for an exact value, it completely depends on the GT size.
> I guess we can do >= 3 (at least one byte for each section (slice, 
> subslice, eus).

Isnt' sizeof(struct drm_i915_query_topology_info) included in the 
length? So it should be at least that big, no?

>>
>>> +
>>> +    _topo_info = malloc(3 * item.length);
>>
>> Assert on malloc.
>>
>> What is 3?
> 
> Some room on both sides, to verify that the kernel doesn't write outside 
> the bounds.

Yeah figured it out below. Put a comment please.

>>> +
>>> +    free(_topo_info);
>>> +}
>>> +
>>> +static void
>>> +test_query_topology_matches_eu_total(int fd)
>>> +{
>>> +    struct drm_i915_query_item item;
>>> +    struct drm_i915_query_topology_info *topo_info;
>>> +    drm_i915_getparam_t gp;
>>> +    int n_eus, n_eus_topology, s;
>>> +
>>> +    gp.param = I915_PARAM_EU_TOTAL;
>>> +    gp.value = &n_eus;
>>> +    do_ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
>>> +    igt_debug("n_eus=%i\n", n_eus);
>>> +
>>> +    memset(&item, 0, sizeof(item));
>>> +    item.query_id = DRM_I915_QUERY_TOPOLOGY_INFO;
>>> +    i915_query_item(fd, &item, 1);
>>> +
>>> +    topo_info = (struct drm_i915_query_topology_info *) calloc(1, 
>>> item.length);
>>> +
>>> +    item.data_ptr = to_user_pointer(topo_info);
>>> +    i915_query_item(fd, &item, 1);
>>> +
>>> +    igt_debug("max_slices=%hu max_subslices=%hu 
>>> max_eus_per_subslice=%hu\n",
>>> +          topo_info->max_slices, topo_info->max_subslices,
>>> +          topo_info->max_eus_per_subslice);
>>> +    igt_debug(" subslice_offset=%hu subslice_stride=%hu\n",
>>> +          topo_info->subslice_offset, topo_info->subslice_stride);
>>> +    igt_debug(" eu_offset=%hu eu_stride=%hu\n",
>>> +          topo_info->eu_offset, topo_info->eu_stride);
>>> +
>>> +    n_eus_topology = 0;
>>> +    for (s = 0; s < topo_info->max_slices; s++) {
>>> +        int ss;
>>> +
>>> +        igt_debug("slice%i:\n", s);
>>> +
>>> +        for (ss = 0; ss < topo_info->max_subslices; ss++) {
>>> +            int eu, n_subslice_eus = 0;
>>> +
>>> +            igt_debug("\tsubslice: %i\n", ss);
>>> +
>>> +            igt_debug("\t\teu_mask: 0b");
>>> +            for (eu = 0; eu < topo_info->max_eus_per_subslice; eu++) {
>>> +                uint8_t val = eu_available(topo_info, s, ss,
>>> + topo_info->max_eus_per_subslice - 1 - eu);
>>
>> Why is the eu parameter reversed on not simply eu?
> 
> Because of the printf ordering (starting with most significant bit).

For debug.. grumble. Ok. Hm, but are you printing out more than one u8 
here, or will you be, and will bit ordering be correct then?
>>> + igt_require(query_topology_supported(fd));
>>
>> I am actually not sure if this should be igt_assert or igt_require. Do 
>> we actually want to support running new igts on old kernels, or make 
>> the test stronger by failing when it thinks it should work. Will need 
>> to ask for second opinions.
>>
>> It would have skipped already from the fixture if query API is not 
>> supported.
> 
> I was under the impression that we wanted some kind of old kernel 
> support (like 4.15).

But in fixture the whole thing skip already if there is no query support 
in the kernel. And there will be no kernels with query but no topology.

>>
>>> + test_query_topology_coherent_slice_mask(fd);
>>> +    }
>>> +
>>> +    igt_subtest("query-topology-matches-eu-total") {
>>> +        igt_require(AT_LEAST_GEN(devid, 8) || IS_HASWELL(devid));
>>> +        igt_require(query_topology_supported(fd));
>>> +        test_query_topology_matches_eu_total(fd);
>>> +    }
>>> +
>>
>> Is it possible to add some tests which run on platforms where we can 
>> 100% imply the slice/subslice configuration from the devid and then 
>> verify topology query is returning expected data for all slices etc?
> 
> That's tricky. I don't know what we actually ship (are any big cores 
> ever fused off in the field? Is it just atoms?)
> I have a production big core BDW laptop with lots of fused stuff, so my 
> guess is no...

Haswell maybe? According to haswell_sseu_info_init in the kernel 
subslice masks are purely pci id based. EU count wouldn need a register 
read.

Bottom line is we could also read fuse registers from IGT and verify 
state between the two.

At least Haswell look easy enough for some basic coverage. I won't say 
we need to do all, but at least we need to look at more than just 
subslice 0.

Regards,

Tvrtko