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

[Lionel Landwerlin <lionel.g.landwerlin@intel.com>]

On 16/01/18 21:34, Chris Wilson wrote:
> Quoting Lionel Landwerlin (2018-01-16 16:07:28)
>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>> ---
>>   tests/Makefile.sources |   1 +
>>   tests/meson.build      |   1 +
>>   tests/query.c          | 268 +++++++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 270 insertions(+)
>>   create mode 100644 tests/query.c
>>
>> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
>> index e4e06d01..390cc82b 100644
>> --- a/tests/Makefile.sources
>> +++ b/tests/Makefile.sources
>> @@ -227,6 +227,7 @@ TESTS_progs = \
>>          prime_self_import \
>>          prime_udl \
>>          prime_vgem \
>> +       query \
>>          sw_sync \
>>          syncobj_basic \
>>          syncobj_wait \
>> diff --git a/tests/meson.build b/tests/meson.build
>> index 4c4bee1d..70df38f3 100644
>> --- a/tests/meson.build
>> +++ b/tests/meson.build
>> @@ -204,6 +204,7 @@ test_progs = [
>>          'prime_self_import',
>>          'prime_udl',
>>          'prime_vgem',
>> +       'query',
>>          'sw_sync',
>>          'syncobj_basic',
>>          'syncobj_wait',
>> diff --git a/tests/query.c b/tests/query.c
>> new file mode 100644
>> index 00000000..cb5aedd4
>> --- /dev/null
>> +++ b/tests/query.c
>> @@ -0,0 +1,268 @@
>> +/*
>> + * 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"
>> +
>> +IGT_TEST_DESCRIPTION("Testing the query uAPI.");
>> +
>> +static bool has_query_supports(int fd)
>> +{
>> +       struct drm_i915_query query = {};
>> +
>> +       return igt_ioctl(fd, DRM_IOCTL_I915_QUERY, &query) == 0;
>> +}
>> +
>> +static void test_query_garbage(int fd)
>> +{
>> +       struct drm_i915_query query;
>> +       struct drm_i915_query_item items[2];
>> +
>> +       memset(&query, 0, sizeof(query));
>> +       query.num_items = 1;
>> +       query.items_ptr = 0xffffffff;
> That might be legal on 64b. I think you mean -1.

Thanks, will do.

>
> For bonus points. .items_ptr = - num_items * sizeof(query) and variable
> number of items.

Do you mean .items_ptr = - num_items * sizeof(items[0]) ?

I don't understand what you want to test by "variable number of items".

>
>> +       do_ioctl_err(fd, DRM_IOCTL_I915_QUERY, &query, EFAULT);
> do_ioctl_err() you never want to see the error message.
>
> int __i915_query(int fd, struct drm_i915_query *q); /* returns err */
> void i915_query(int fd, struct drm_i915_query *q); /* asserts success */

I get int __i915_query() but not void i915_query().
In my experience you want to see the assert in the test function. If 
that's in a utility function, then you have no idea where it failed when 
reading the CI logs.

>
>> +
>> +       memset(&query, 0, sizeof(query));
>> +       query.num_items = 1;
>> +       query.items_ptr = 0;
>> +       do_ioctl_err(fd, DRM_IOCTL_I915_QUERY, &query, EFAULT);
>> +
>> +       memset(&query, 0, sizeof(query));
>> +       query.num_items = 2;
>> +       query.items_ptr = (uintptr_t) items;
> .items_ptr = to_user_pointer(item);
>
> Just reads nicer.

Thanks.

>
> Also check overflow of num_items * items_ptr. So num_items = (INT_MAX,
> UINT_MAX, LONG_MAX, ULONG_MAX etc) / sizeof(query) + 1.

Again, not sure what this is supposed to test. We should get an EFAULT 
as soon as we're outside mapped address space.

>
>> +       memset(items, 0, sizeof(items));
>> +       items[0].query_id = 0xffffffff;
>> +       do_ioctl_err(fd, DRM_IOCTL_I915_QUERY, &query, EINVAL);
>> +
>> +       memset(&query, 0, sizeof(query));
>> +       query.num_items = 2;
>> +       query.items_ptr = (uintptr_t) items;
>> +       memset(items, 0, sizeof(items));
>> +       do_ioctl_err(fd, DRM_IOCTL_I915_QUERY, &query, EINVAL);
>> +
>> +       memset(&query, 0, sizeof(query));
>> +       query.num_items = 2;
>> +       query.items_ptr = (uintptr_t) items;
>> +       memset(items, 0, sizeof(items));
>> +       items[0].query_id = DRM_I915_QUERY_SLICE_INFO;
>> +       items[0].length = 0xffffffff;
>> +       items[1].query_id = DRM_I915_QUERY_SUBSLICE_INFO;
>> +       do_ioctl_err(fd, DRM_IOCTL_I915_QUERY, &query, EINVAL);
>> +}
>> +
>> +static bool query_topology_supported(int fd)
>> +{
>> +       struct drm_i915_query query;
>> +       struct drm_i915_query_item item;
>> +
>> +       memset(&query, 0, sizeof(query));
>> +       query.num_items = 1;
>> +       query.items_ptr = (uintptr_t) &item;
>> +
>> +       memset(&item, 0, sizeof(item));
>> +       item.query_id = DRM_I915_QUERY_SLICE_INFO;
> 	struct drm_i915_query_item item = {
> 		.query_id = DRM_I915_QUERY_SLICE_INFO,
> 	};
>
> 	return __i915_query_item(fd, &item) == 0;
>
> __i915_query_item(int fd, *item) {
> 	struct drm_i915_query q = {
> 		.num_items = 1,
> 		.items_ptr = to_user_pointer(item),
> 	}
> 	return __i915_query(fd, &q);
> }
>
> Or __i915_query_one() ?
> -Chris
Let's go with __i915_query_item(fd, items_ptr, n_items).
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx