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

[Lionel Landwerlin]

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

>
>>       kms_3d \
>>       kms_addfb_basic \
>>       kms_atomic \
>> diff --git a/tests/i915_query.c b/tests/i915_query.c
>> new file mode 100644
>> index 00000000..21621b78
>> --- /dev/null
>> +++ b/tests/i915_query.c
>> @@ -0,0 +1,314 @@
>> +/*
>> + * Copyright © 2017 Intel Corporation
>> + *
>> + * Permission is hereby granted, free of charge, to any person 
>> obtaining a
>> + * copy of this software and associated documentation files (the 
>> "Software"),
>> + * to deal in the Software without restriction, including without 
>> limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, 
>> sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom 
>> the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including 
>> the next
>> + * paragraph) shall be included in all copies or substantial 
>> portions of the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, 
>> EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF 
>> MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO 
>> EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES 
>> OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, 
>> ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR 
>> OTHER DEALINGS
>> + * IN THE SOFTWARE.
>> + */
>> +
>> +#include "igt.h"
>> +
>> +#include <limits.h>
>> +
>> +IGT_TEST_DESCRIPTION("Testing the i915 query uAPI.");
>> +
>> +static int
>> +__i915_query(int fd, struct drm_i915_query *q)
>> +{
>> +    if (igt_ioctl(fd, DRM_IOCTL_I915_QUERY, q))
>> +        return -errno;
>> +    return 0;
>> +}
>> +
>> +static int
>> +__i915_query_item(int fd, struct drm_i915_query_item *items, 
>> uint32_t n_items)
>> +{
>> +    struct drm_i915_query q = {
>> +        .num_items = n_items,
>> +        .items_ptr = to_user_pointer(items),
>> +    };
>> +    return __i915_query(fd, &q);
>> +}
>> +
>> +#define i915_query_item(fd, items, n_items) do { \
>> +        igt_assert_eq(__i915_query_item(fd, items, n_items), 0); \
>> +        errno = 0; \
>> +    } while (0)
>> +#define i915_query_item_err(fd, items, n_items, err) do { \
>> +        igt_assert_eq(__i915_query_item(fd, items, n_items), -err); \
>> +    } while (0)
>> +
>
> Suggest plural i915_query_items for the above.

Done.

>
>> +static bool has_query_supports(int fd)
>> +{
>> +    struct drm_i915_query query = {};
>> +
>> +    return __i915_query(fd, &query) == 0;
>> +}
>> +
>> +static void test_query_garbage(int fd)
>> +{
>> +    struct drm_i915_query_item items[2];
>> +    struct drm_i915_query_item *items_ptr;
>> +    int i, len, ret;
>> +
>> +    i915_query_item_err(fd, (void *) ULONG_MAX, 1, EFAULT);
>> +
>> +    i915_query_item_err(fd, (void *) 0, 1, EFAULT);
>
> Could add comments throughout to explain what is being tested.

Yep.

>
> Test for non-zero flags field? Both in the query and query item.

Done.

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

>
>> +    i915_query_item(fd, items, 2);
>> +    igt_assert_eq(items[0].length, -EINVAL);
>> +    igt_assert_eq(items[1].length, -EINVAL);
>> +
>> +    memset(items, 0, sizeof(items));
>> +    items[0].query_id = DRM_I915_QUERY_TOPOLOGY_INFO;
>> +    items[1].query_id = DRM_I915_QUERY_TOPOLOGY_INFO; > + 
>> items[1].length = sizeof(struct drm_i915_query_topology_info) - 1;
>> +    i915_query_item(fd, items, 2);
>> +    igt_assert_lte(0, items[0].length);
>> +    igt_assert_eq(items[1].length, -EINVAL);
>
> Tricky one - ideally we would want to split testing of the query API 
> from specific queries, but then we don't have any queries to test 
> with.. can't split it then.
>
>> +
>> +    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? :)

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

>
> And assert cannot be either this or that, it has to know explicitly or 
> split into two tests or something.
>
>> +    munmap(items_ptr, len);
>> +}
>> +
>> +static bool query_topology_supported(int fd)
>> +{
>> +    struct drm_i915_query_item item = {
>> +        .query_id = DRM_I915_QUERY_TOPOLOGY_INFO,
>> +    };
>> +
>> +    return __i915_query_item(fd, &item, 1) == 0 && item.length > 0;
>> +}
>> +
>> +static void test_query_topology_pre_gen8(int fd)
>> +{
>> +    struct drm_i915_query_item item = {
>> +        .query_id = DRM_I915_QUERY_TOPOLOGY_INFO,
>> +    };
>> +
>> +    i915_query_item(fd, &item, 1);
>> +    igt_assert_eq(item.length, -ENODEV);
>> +}
>> +
>> +#define DIV_ROUND_UP(val, div) (ALIGN(val, div) / div)
>> +
>> +static bool
>> +slice_available(const struct drm_i915_query_topology_info *topo_info,
>> +        int s)
>> +{
>> +    return (topo_info->data[s / 8] >> (s % 8)) & 1;
>> +}
>> +
>> +static bool
>> +subslice_available(const struct drm_i915_query_topology_info 
>> *topo_info,
>> +           int s, int ss)
>> +{
>> +    return (topo_info->data[topo_info->subslice_offset +
>> +                s * topo_info->subslice_stride +
>> +                ss / 8] >> (ss % 8)) & 1;
>> +}
>> +
>> +static bool
>> +eu_available(const struct drm_i915_query_topology_info *topo_info,
>> +         int s, int ss, int eu)
>> +{
>> +    return (topo_info->data[topo_info->eu_offset +
>> +                (s * topo_info->max_subslices + ss) * 
>> topo_info->eu_stride +
>> +                eu / 8] >> (eu % 8)) & 1;
>> +}
>> +
>> +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).

>
>> +
>> +    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).

>
>> +
>> +    _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.

>
>> +    memset(_topo_info, 0xff, 3 * item.length);
>> +    topo_info = (struct drm_i915_query_topology_info *) (_topo_info 
>> + item.length);
>> +    memset(topo_info, 0, item.length);
>> +    igt_assert(topo_info);
>
> Unusual to assert _after_ dereference.

Uh... Removing.

>
>> +
>> +    item.data_ptr = to_user_pointer(topo_info);
>> +    i915_query_item(fd, &item, 1);
>> +    igt_assert(item.length > 0);
>
> Could assert more strongly that this length matches the initial query.
>
>> +
>> +    topology_slices = 0;
>> +    for (s = 0; s < topo_info->max_slices; s++) {
>> +        if (slice_available(topo_info, s))
>> +            topology_slices |= 1UL << s;
>> +    }
>> +
>> +    igt_debug("slice mask getparam=0x%x / query=0x%x\n",
>> +          slice_mask, topology_slices);
>> +
>> +    /* These 2 should always match. */
>> +    igt_assert_eq_u32(slice_mask, topology_slices);
>> +
>> +    topology_subslices_slice0 = 0;
>> +    for (s = 0; s < topo_info->max_subslices; s++) {
>> +        if (subslice_available(topo_info, 0, s))
>> +            topology_subslices_slice0 |= 1UL << s;
>> +    }
>> +
>> +    igt_debug("subslice mask getparam=0x%x / query=0x%x\n",
>> +          subslice_mask, topology_subslices_slice0);
>> +
>> +    /* I915_PARAM_SUBSLICE_MASK returns the value for slice0, we
>> +     * should match the values for the first slice of the
>> +     * topology.
>> +     */
>> +    igt_assert_eq_u32(subslice_mask, topology_subslices_slice0);
>> +
>> +    for (s = 0; s < item.length; s++) {
>> +        igt_assert_eq(_topo_info[s], 0xff);
>> +        igt_assert_eq(_topo_info[2 * item.length + s], 0xff);
>> +    }
>
> Ah so you are checking that kernel did not under or over write?

Yes, Chris' request.

>
> I'd move this into a more generic test than the one dealing with 
> actual topology verification.

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

>
> Do you need to assert that the val matches the expectations from slice 
> and subslice mask - meaning if the s/ss are zero in their masks val 
> must be zero here as well?

Oh right, I can add that. Thanks!

>
>> +                igt_debug("%hhi", val);
>> +                n_subslice_eus += __builtin_popcount(val);
>> +                n_eus_topology += __builtin_popcount(val);
>> +            }
>> +            igt_debug(" (%i)\n", n_subslice_eus);
>> +        }
>> +    }
>> +
>> +    igt_assert(n_eus_topology == n_eus);
>> +}
>> +
>> +igt_main
>> +{
>> +    int fd = -1;
>> +    int devid;
>> +
>> +    igt_fixture {
>> +        fd = drm_open_driver(DRIVER_INTEL);
>> +        igt_require(has_query_supports(fd));
>> +        devid = intel_get_drm_devid(fd);
>> +    }
>> +
>> +    igt_subtest("query-garbage")
>> +        test_query_garbage(fd);
>> +
>> +    igt_subtest("query-topology-pre-gen8") {
>> +        igt_require(intel_gen(devid) < 8 && !IS_HASWELL(devid));
>> +        igt_require(query_topology_supported(fd));
>
> Does it even passes this line pre-gen8? If item length will be -ENODEV 
> then it doesn't, no?

Oh right...
I need to adapt query_topology_supported() for that.

>
>> +        test_query_topology_pre_gen8(fd);
>
> I'd call this subtest query-topology-unsupported - since the test 
> expects ENODEV always.

Done.

>
>> +    }
>> +
>> +    igt_subtest("query-topology-coherent-slice-mask") {
>> +        igt_require(AT_LEAST_GEN(devid, 8) || IS_HASWELL(devid));
>
> Why not simply gen >= 8 instead of AT_LEAST_GEN (first time I see this 
> one)?

Sure.

>
>> + 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).

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

>
>> +    igt_fixture {
>> +        close(fd);
>> +    }
>> +}
>> diff --git a/tests/meson.build b/tests/meson.build
>> index 2a1e6f19..a805011e 100644
>> --- a/tests/meson.build
>> +++ b/tests/meson.build
>> @@ -144,6 +144,7 @@ test_progs = [
>>       'gen3_render_tiledy_blits',
>>       'gen7_forcewake_mt',
>>       'gvt_basic',
>> +    'i915_query',
>>       'kms_3d',
>>       'kms_addfb_basic',
>>       'kms_atomic',
>>
>
> Regards,
>
> Tvrtko
>
Thanks for the review!