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

[Lionel Landwerlin]

On 15/02/18 12:20, Chris Wilson wrote:
> Quoting Lionel Landwerlin (2018-02-15 12:03:08)
>> 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)
>>
>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
>> ---
>>   tests/Makefile.sources |   1 +
>>   tests/i915_query.c     | 300 +++++++++++++++++++++++++++++++++++++++++++++++++
>>   tests/meson.build      |   1 +
>>   3 files changed, 302 insertions(+)
>>   create mode 100644 tests/i915_query.c
>>
>> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
>> index 870c9093..d8e55e8b 100644
>> --- a/tests/Makefile.sources
>> +++ b/tests/Makefile.sources
>> @@ -166,6 +166,7 @@ TESTS_progs = \
>>          gen3_render_tiledy_blits \
>>          gen7_forcewake_mt \
>>          gvt_basic \
>> +       i915_query \
>>          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..aba7b082
>> --- /dev/null
>> +++ b/tests/i915_query.c
>> @@ -0,0 +1,300 @@
>> +/*
>> + * 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)
>> +{
>> +       return igt_ioctl(fd, DRM_IOCTL_I915_QUERY, q);
> Please get into the habit of returning the errno so that we don't have
> to rely on errno being valid from the time of failure to the time of
> checking.
>
> int err = 0;
> if (igt_ioctl(...))
> 	err = -errno;
> return err;
>
> E.g., even
> 	igt_assert(_i915_query() == -1);
> 	igt_assert(errno == -EINVAL);
> is subject to creativity on behalf of igt.

Thanks, will do.

>
>> +static void
>> +test_query_topology_coherent_slice_mask(int fd)
>> +{
>> +       struct drm_i915_query_item item;
>> +       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;
>> +
>> +       gp.param = I915_PARAM_SLICE_MASK;
>> +       gp.value = &slice_mask;
>> +       do_ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
> Shouldn't these be handling ENODEV?

I think we've handled that by running this only on gen8+ in the caller.

>
>> +       gp.param = I915_PARAM_SUBSLICE_MASK;
>> +       gp.value = &subslice_mask;
>> +       do_ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
>> +
>> +       /* 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);
>> +
>> +       topo_info = (struct drm_i915_query_topology_info *) calloc(1, item.length);
> igt_assert(topo_info); cast is unnecessary
> pre-zero or poisoned?
>
> Bonus points for quarantining it and checking the guards.

On it.

>
>> +       item.data_ptr = to_user_pointer(topo_info);
>> +       i915_query_item(fd, &item, 1);
>> +       igt_assert(item.length > 0);
>> +
>> +       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(slice_mask == topology_slices);
> igt_assert_eq();

Sure.

>> +
>> +       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(subslice_mask == topology_subslices_slice0);
> igt_assert_eq_u32() ?

Sure.

>
>> +
>> +       free(topo_info);
>> +}
> -Chris
>