[igt-dev] [PATCH i-g-t v5 2/3] tests: add i915 query tests
Lionel Landwerlin
lionel.g.landwerlin at intel.com
Thu Mar 8 17:51:21 UTC 2018
On 08/03/18 15:44, Tvrtko Ursulin wrote:
>
> 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?
Indeed, making a define for it.
>
>>>
>>>> +
>>>> + _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.
Done.
>
>>>> +
>>>> + 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.
You're right, I'll simplify this.
>
>>>
>>>> + 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.
slice/subslice count doesn't, but EU count does :
https://cgit.freedesktop.org/drm-intel/commit/?id=b8ec759e6f1c6da0418238df066a0f1ef8fd2075
>
> 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
>
>
>
More information about the igt-dev
mailing list