Re: [Intel-gfx] [PATCH i-g-t 6/6] tests/i915_query: Engine queues tests
On 08/06/18 11:02, Tvrtko Ursulin wrote: On 06/06/2018 15:33, Lionel Landwerlin wrote: Just a few suggestions below. Otherwise looks good to me. On 06/06/18 13:49, Tvrtko Ursulin wrote: From: Tvrtko Ursulin Basic tests to cover engine queued/runnable/running metric as reported by the DRM_I915_QUERY_ENGINE_QUEUES query. v2: * Update ABI for i915 changes. * Use igt_spin_busywait_until_running. * Support no hardware contexts. * More comments. (Lionel Landwerlin) Chris Wilson: * Check for sw sync support. * Multiple contexts queued test. * Simplify context and bb allocation. * Fix asserts in the running subtest. v3: * Update for uAPI change. Signed-off-by: Tvrtko Ursulin --- tests/i915_query.c | 442 + 1 file changed, 442 insertions(+) diff --git a/tests/i915_query.c b/tests/i915_query.c index c7de8cbd8371..588428749c50 100644 --- a/tests/i915_query.c +++ b/tests/i915_query.c @@ -22,6 +22,7 @@ */ #include "igt.h" +#include "sw_sync.h" #include @@ -477,8 +478,414 @@ test_query_topology_known_pci_ids(int fd, int devid) free(topo_info); } +#define DRM_I915_QUERY_ENGINE_QUEUES 2 + +struct drm_i915_query_engine_queues { + /** Engine class as in enum drm_i915_gem_engine_class. */ + __u16 class; + + /** Engine instance number. */ + __u16 instance; + + /** Number of requests with unresolved fences and dependencies. */ + __u32 queued; + + /** Number of ready requests waiting on a slot on GPU. */ + __u32 runnable; + + /** Number of requests executing on the GPU. */ + __u32 running; + + __u32 rsvd[6]; +}; I suppose this above would be in the updated i915_drm.h. I thought you would have put it there as part of your first commit. Yes, or I should prefix the copy with local_ as we usually do. I think the point of copying the headers locally was to get rid of the local_ etc... + +static bool query_engine_queues_supported(int fd) +{ + struct drm_i915_query_item item = { + .query_id = DRM_I915_QUERY_ENGINE_QUEUES, + }; + + return __i915_query_items(fd, , 1) == 0 && item.length > 0; +} + +static void engine_queues_invalid(int fd) +{ + struct drm_i915_query_engine_queues queues; + struct drm_i915_query_item item; + unsigned int len; + unsigned int i; + + /* Flags is MBZ. */ + memset(, 0, sizeof(item)); + item.query_id = DRM_I915_QUERY_ENGINE_QUEUES; + item.flags = 1; + i915_query_items(fd, , 1); + igt_assert_eq(item.length, -EINVAL); + + /* Length not zero and not greater or equal required size. */ + memset(, 0, sizeof(item)); + item.query_id = DRM_I915_QUERY_ENGINE_QUEUES; + item.length = 1; + i915_query_items(fd, , 1); + igt_assert_eq(item.length, -EINVAL); + + /* Query correct length. */ + memset(, 0, sizeof(item)); + item.query_id = DRM_I915_QUERY_ENGINE_QUEUES; + i915_query_items(fd, , 1); + igt_assert(item.length >= 0); Could be item.length > 0? True, should be even. :) + len = item.length; + + /* Ivalid pointer. */ + memset(, 0, sizeof(item)); + item.query_id = DRM_I915_QUERY_ENGINE_QUEUES; + item.length = len; + i915_query_items(fd, , 1); + igt_assert_eq(item.length, -EFAULT); Maybe verify ULONG_MAX too? item.data_ptr = ULONG_MAX? Can do. + + /* Reserved fields are MBZ. */ + + for (i = 0; i < ARRAY_SIZE(queues.rsvd); i++) { + memset(, 0, sizeof(queues)); + queues.rsvd[i] = 1; + memset(, 0, sizeof(item)); + item.query_id = DRM_I915_QUERY_ENGINE_QUEUES; + item.length = len; + item.data_ptr = to_user_pointer(); + i915_query_items(fd, , 1); + igt_assert_eq(item.length, -EINVAL); + } + + memset(, 0, sizeof(queues)); + queues.class = -1; class = I915_ENGINE_CLASS_INVALID ? Yes thanks! + memset(, 0, sizeof(item)); + item.query_id = DRM_I915_QUERY_ENGINE_QUEUES; + item.length = len; + item.data_ptr = to_user_pointer(); + i915_query_items(fd, , 1); + igt_assert_eq(item.length, -ENOENT); + + memset(, 0, sizeof(queues)); + queues.instance = -1; + memset(, 0, sizeof(item)); + item.query_id = DRM_I915_QUERY_ENGINE_QUEUES; + item.length = len; + item.data_ptr = to_user_pointer(); + i915_query_items(fd, , 1); + igt_assert_eq(item.length, -ENOENT); +} + +static void engine_queues(int fd, const struct intel_execution_engine2 *e) +{ + struct drm_i915_query_engine_queues queues; + struct drm_i915_query_item item; + unsigned int len; + + /* Query required buffer length. */ + memset(, 0, sizeof(queues)); + memset(, 0, sizeof(item)); + item.query_id = DRM_I915_QUERY_ENGINE_QUEUES; + item.data_ptr = to_user_pointer(); + i915_query_items(fd, , 1); + igt_assert(item.length >= 0); item.length > 0? Marking on TODO.. + igt_assert(item.length <= sizeof(queues)); I guess we can expect item.length == sizeof(queues) here.
Re: [Intel-gfx] [PATCH i-g-t 6/6] tests/i915_query: Engine queues tests
On 06/06/2018 15:33, Lionel Landwerlin wrote: Just a few suggestions below. Otherwise looks good to me. On 06/06/18 13:49, Tvrtko Ursulin wrote: From: Tvrtko Ursulin Basic tests to cover engine queued/runnable/running metric as reported by the DRM_I915_QUERY_ENGINE_QUEUES query. v2: * Update ABI for i915 changes. * Use igt_spin_busywait_until_running. * Support no hardware contexts. * More comments. (Lionel Landwerlin) Chris Wilson: * Check for sw sync support. * Multiple contexts queued test. * Simplify context and bb allocation. * Fix asserts in the running subtest. v3: * Update for uAPI change. Signed-off-by: Tvrtko Ursulin --- tests/i915_query.c | 442 + 1 file changed, 442 insertions(+) diff --git a/tests/i915_query.c b/tests/i915_query.c index c7de8cbd8371..588428749c50 100644 --- a/tests/i915_query.c +++ b/tests/i915_query.c @@ -22,6 +22,7 @@ */ #include "igt.h" +#include "sw_sync.h" #include @@ -477,8 +478,414 @@ test_query_topology_known_pci_ids(int fd, int devid) free(topo_info); } +#define DRM_I915_QUERY_ENGINE_QUEUES 2 + +struct drm_i915_query_engine_queues { + /** Engine class as in enum drm_i915_gem_engine_class. */ + __u16 class; + + /** Engine instance number. */ + __u16 instance; + + /** Number of requests with unresolved fences and dependencies. */ + __u32 queued; + + /** Number of ready requests waiting on a slot on GPU. */ + __u32 runnable; + + /** Number of requests executing on the GPU. */ + __u32 running; + + __u32 rsvd[6]; +}; I suppose this above would be in the updated i915_drm.h. I thought you would have put it there as part of your first commit. Yes, or I should prefix the copy with local_ as we usually do. + +static bool query_engine_queues_supported(int fd) +{ + struct drm_i915_query_item item = { + .query_id = DRM_I915_QUERY_ENGINE_QUEUES, + }; + + return __i915_query_items(fd, , 1) == 0 && item.length > 0; +} + +static void engine_queues_invalid(int fd) +{ + struct drm_i915_query_engine_queues queues; + struct drm_i915_query_item item; + unsigned int len; + unsigned int i; + + /* Flags is MBZ. */ + memset(, 0, sizeof(item)); + item.query_id = DRM_I915_QUERY_ENGINE_QUEUES; + item.flags = 1; + i915_query_items(fd, , 1); + igt_assert_eq(item.length, -EINVAL); + + /* Length not zero and not greater or equal required size. */ + memset(, 0, sizeof(item)); + item.query_id = DRM_I915_QUERY_ENGINE_QUEUES; + item.length = 1; + i915_query_items(fd, , 1); + igt_assert_eq(item.length, -EINVAL); + + /* Query correct length. */ + memset(, 0, sizeof(item)); + item.query_id = DRM_I915_QUERY_ENGINE_QUEUES; + i915_query_items(fd, , 1); + igt_assert(item.length >= 0); Could be item.length > 0? True, should be even. :) + len = item.length; + + /* Ivalid pointer. */ + memset(, 0, sizeof(item)); + item.query_id = DRM_I915_QUERY_ENGINE_QUEUES; + item.length = len; + i915_query_items(fd, , 1); + igt_assert_eq(item.length, -EFAULT); Maybe verify ULONG_MAX too? item.data_ptr = ULONG_MAX? Can do. + + /* Reserved fields are MBZ. */ + + for (i = 0; i < ARRAY_SIZE(queues.rsvd); i++) { + memset(, 0, sizeof(queues)); + queues.rsvd[i] = 1; + memset(, 0, sizeof(item)); + item.query_id = DRM_I915_QUERY_ENGINE_QUEUES; + item.length = len; + item.data_ptr = to_user_pointer(); + i915_query_items(fd, , 1); + igt_assert_eq(item.length, -EINVAL); + } + + memset(, 0, sizeof(queues)); + queues.class = -1; class = I915_ENGINE_CLASS_INVALID ? Yes thanks! + memset(, 0, sizeof(item)); + item.query_id = DRM_I915_QUERY_ENGINE_QUEUES; + item.length = len; + item.data_ptr = to_user_pointer(); + i915_query_items(fd, , 1); + igt_assert_eq(item.length, -ENOENT); + + memset(, 0, sizeof(queues)); + queues.instance = -1; + memset(, 0, sizeof(item)); + item.query_id = DRM_I915_QUERY_ENGINE_QUEUES; + item.length = len; + item.data_ptr = to_user_pointer(); + i915_query_items(fd, , 1); + igt_assert_eq(item.length, -ENOENT); +} + +static void engine_queues(int fd, const struct intel_execution_engine2 *e) +{ + struct drm_i915_query_engine_queues queues; + struct drm_i915_query_item item; + unsigned int len; + + /* Query required buffer length. */ + memset(, 0, sizeof(queues)); + memset(, 0, sizeof(item)); + item.query_id = DRM_I915_QUERY_ENGINE_QUEUES; + item.data_ptr = to_user_pointer(); + i915_query_items(fd, , 1); + igt_assert(item.length >= 0); item.length > 0? Marking on TODO.. + igt_assert(item.length <= sizeof(queues)); I guess we can expect item.length == sizeof(queues) here. Hmm yes, no idea what I was thinking there. :I + len = item.length; + + /* Check length larger than required works and
Re: [Intel-gfx] [PATCH i-g-t 6/6] tests/i915_query: Engine queues tests
Just a few suggestions below. Otherwise looks good to me. On 06/06/18 13:49, Tvrtko Ursulin wrote: From: Tvrtko Ursulin Basic tests to cover engine queued/runnable/running metric as reported by the DRM_I915_QUERY_ENGINE_QUEUES query. v2: * Update ABI for i915 changes. * Use igt_spin_busywait_until_running. * Support no hardware contexts. * More comments. (Lionel Landwerlin) Chris Wilson: * Check for sw sync support. * Multiple contexts queued test. * Simplify context and bb allocation. * Fix asserts in the running subtest. v3: * Update for uAPI change. Signed-off-by: Tvrtko Ursulin --- tests/i915_query.c | 442 + 1 file changed, 442 insertions(+) diff --git a/tests/i915_query.c b/tests/i915_query.c index c7de8cbd8371..588428749c50 100644 --- a/tests/i915_query.c +++ b/tests/i915_query.c @@ -22,6 +22,7 @@ */ #include "igt.h" +#include "sw_sync.h" #include @@ -477,8 +478,414 @@ test_query_topology_known_pci_ids(int fd, int devid) free(topo_info); } +#define DRM_I915_QUERY_ENGINE_QUEUES 2 + +struct drm_i915_query_engine_queues { + /** Engine class as in enum drm_i915_gem_engine_class. */ + __u16 class; + + /** Engine instance number. */ + __u16 instance; + + /** Number of requests with unresolved fences and dependencies. */ + __u32 queued; + + /** Number of ready requests waiting on a slot on GPU. */ + __u32 runnable; + + /** Number of requests executing on the GPU. */ + __u32 running; + + __u32 rsvd[6]; +}; I suppose this above would be in the updated i915_drm.h. I thought you would have put it there as part of your first commit. + +static bool query_engine_queues_supported(int fd) +{ + struct drm_i915_query_item item = { + .query_id = DRM_I915_QUERY_ENGINE_QUEUES, + }; + + return __i915_query_items(fd, , 1) == 0 && item.length > 0; +} + +static void engine_queues_invalid(int fd) +{ + struct drm_i915_query_engine_queues queues; + struct drm_i915_query_item item; + unsigned int len; + unsigned int i; + + /* Flags is MBZ. */ + memset(, 0, sizeof(item)); + item.query_id = DRM_I915_QUERY_ENGINE_QUEUES; + item.flags = 1; + i915_query_items(fd, , 1); + igt_assert_eq(item.length, -EINVAL); + + /* Length not zero and not greater or equal required size. */ + memset(, 0, sizeof(item)); + item.query_id = DRM_I915_QUERY_ENGINE_QUEUES; + item.length = 1; + i915_query_items(fd, , 1); + igt_assert_eq(item.length, -EINVAL); + + /* Query correct length. */ + memset(, 0, sizeof(item)); + item.query_id = DRM_I915_QUERY_ENGINE_QUEUES; + i915_query_items(fd, , 1); + igt_assert(item.length >= 0); Could be item.length > 0? + len = item.length; + + /* Ivalid pointer. */ + memset(, 0, sizeof(item)); + item.query_id = DRM_I915_QUERY_ENGINE_QUEUES; + item.length = len; + i915_query_items(fd, , 1); + igt_assert_eq(item.length, -EFAULT); Maybe verify ULONG_MAX too? + + /* Reserved fields are MBZ. */ + + for (i = 0; i < ARRAY_SIZE(queues.rsvd); i++) { + memset(, 0, sizeof(queues)); + queues.rsvd[i] = 1; + memset(, 0, sizeof(item)); + item.query_id = DRM_I915_QUERY_ENGINE_QUEUES; + item.length = len; + item.data_ptr = to_user_pointer(); + i915_query_items(fd, , 1); + igt_assert_eq(item.length, -EINVAL); + } + + memset(, 0, sizeof(queues)); + queues.class = -1; class = I915_ENGINE_CLASS_INVALID ? + memset(, 0, sizeof(item)); + item.query_id = DRM_I915_QUERY_ENGINE_QUEUES; + item.length = len; + item.data_ptr = to_user_pointer(); + i915_query_items(fd, , 1); + igt_assert_eq(item.length, -ENOENT); + + memset(, 0, sizeof(queues)); + queues.instance = -1; + memset(, 0, sizeof(item)); + item.query_id = DRM_I915_QUERY_ENGINE_QUEUES; + item.length = len; + item.data_ptr = to_user_pointer(); + i915_query_items(fd, , 1); + igt_assert_eq(item.length, -ENOENT); +} + +static void engine_queues(int fd, const struct intel_execution_engine2 *e) +{ + struct drm_i915_query_engine_queues queues; + struct drm_i915_query_item item; + unsigned int len; + + /* Query required buffer length. */ + memset(, 0, sizeof(queues)); + memset(, 0, sizeof(item)); + item.query_id = DRM_I915_QUERY_ENGINE_QUEUES; + item.data_ptr = to_user_pointer(); + i915_query_items(fd, , 1); + igt_assert(item.length >= 0); item.length > 0? + igt_assert(item.length <= sizeof(queues)); I guess we can expect item.length == sizeof(queues) here. + len = item.length; + + /* Check length larger