* [igt-dev] [RFC PATCH i-g-t] lib/i915: Generate engine names at runtime
@ 2019-11-15 12:14 Petri Latvala
2019-11-15 12:30 ` [igt-dev] ✗ GitLab.Pipeline: warning for " Patchwork
` (5 more replies)
0 siblings, 6 replies; 10+ messages in thread
From: Petri Latvala @ 2019-11-15 12:14 UTC (permalink / raw)
To: igt-dev; +Cc: Tvrtko Ursulin, Petri Latvala
The kernel supplies us an engine list with engine class and an
instance id. If the hardcoded engine list doesn't have the
class+instance we get, construct the engine name with printf instead
of calling additional engines "unknown".
Signed-off-by: Petri Latvala <petri.latvala@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Andi Shyti <andi.shyti@intel.com>
Cc: Katarzyna Dec <katarzyna.dec@intel.com>
---
lib/i915/gem_engine_topology.c | 27 +++++++++++++++++++--------
lib/i915/gem_engine_topology.h | 2 +-
lib/igt_gt.c | 8 ++++++++
lib/igt_gt.h | 7 ++++++-
4 files changed, 34 insertions(+), 10 deletions(-)
diff --git a/lib/i915/gem_engine_topology.c b/lib/i915/gem_engine_topology.c
index 790d455f..0707f237 100644
--- a/lib/i915/gem_engine_topology.c
+++ b/lib/i915/gem_engine_topology.c
@@ -103,6 +103,7 @@ static void init_engine(struct intel_execution_engine2 *e2,
const struct intel_execution_engine2 *__e2;
static const char *unknown_name = "unknown",
*virtual_name = "virtual";
+ const struct intel_execution_engine_class *cls;
e2->class = class;
e2->instance = instance;
@@ -111,7 +112,7 @@ static void init_engine(struct intel_execution_engine2 *e2,
/* engine is a virtual engine */
if (class == I915_ENGINE_CLASS_INVALID &&
instance == I915_ENGINE_CLASS_INVALID_VIRTUAL) {
- e2->name = virtual_name;
+ strncpy(e2->name, virtual_name, sizeof(e2->name));
e2->is_virtual = true;
return;
}
@@ -120,12 +121,22 @@ static void init_engine(struct intel_execution_engine2 *e2,
if (__e2->class == class && __e2->instance == instance)
break;
- if (__e2->name) {
- e2->name = __e2->name;
+ if (__e2->name[0]) {
+ strncpy(e2->name, __e2->name, sizeof(e2->name));
} else {
- igt_warn("found unknown engine (%d, %d)\n", class, instance);
- e2->name = unknown_name;
- e2->flags = -1;
+ for (cls = intel_execution_engine_class_map; cls->name; cls++) {
+ if (cls->class == class) {
+ snprintf(e2->name, sizeof(e2->name), "%s%d", cls->name, instance);
+ igt_info("unknown but supported engine %s found\n", e2->name);
+ break;
+ }
+ }
+
+ if (!e2->name[0]) {
+ igt_info("found unknown engine (%d, %d)\n", class, instance);
+ strncpy(e2->name, unknown_name, sizeof(e2->name));
+ e2->flags = -1;
+ }
}
/* just to remark it */
@@ -223,7 +234,7 @@ struct intel_engine_data intel_init_engine_list(int fd, uint32_t ctx_id)
struct intel_execution_engine2 *__e2 =
&engine_data.engines[engine_data.nengines];
- __e2->name = e2->name;
+ strncpy(__e2->name, e2->name, sizeof(__e2->name));
__e2->instance = e2->instance;
__e2->class = e2->class;
__e2->flags = e2->flags;
@@ -302,7 +313,7 @@ struct intel_execution_engine2 gem_eb_flags_to_engine(unsigned int flags)
if (ring == I915_EXEC_DEFAULT) {
e2__.flags = I915_EXEC_DEFAULT;
- e2__.name = "default";
+ strncpy(e2__.name, "default", sizeof(e2__.name));
} else {
const struct intel_execution_engine2 *e2;
diff --git a/lib/i915/gem_engine_topology.h b/lib/i915/gem_engine_topology.h
index d98773e0..525741cc 100644
--- a/lib/i915/gem_engine_topology.h
+++ b/lib/i915/gem_engine_topology.h
@@ -61,7 +61,7 @@ bool gem_engine_is_equal(const struct intel_execution_engine2 *e1,
struct intel_execution_engine2 gem_eb_flags_to_engine(unsigned int flags);
#define __for_each_static_engine(e__) \
- for ((e__) = intel_execution_engines2; (e__)->name; (e__)++)
+ for ((e__) = intel_execution_engines2; (e__)->name[0]; (e__)++)
#define for_each_context_engine(fd__, ctx__, e__) \
for (struct intel_engine_data i__ = intel_init_engine_list(fd__, ctx__); \
diff --git a/lib/igt_gt.c b/lib/igt_gt.c
index 256c7cbc..25e6c455 100644
--- a/lib/igt_gt.c
+++ b/lib/igt_gt.c
@@ -596,6 +596,14 @@ const struct intel_execution_engine2 intel_execution_engines2[] = {
{ }
};
+const struct intel_execution_engine_class intel_execution_engine_class_map[] = {
+ { "rcs", I915_ENGINE_CLASS_RENDER },
+ { "bcs", I915_ENGINE_CLASS_COPY },
+ { "vcs", I915_ENGINE_CLASS_VIDEO },
+ { "vecs", I915_ENGINE_CLASS_VIDEO_ENHANCE },
+ { }
+};
+
int gem_execbuf_flags_to_engine_class(unsigned int flags)
{
switch (flags & 0x3f) {
diff --git a/lib/igt_gt.h b/lib/igt_gt.h
index 66088d39..0268031f 100644
--- a/lib/igt_gt.h
+++ b/lib/igt_gt.h
@@ -95,13 +95,18 @@ bool gem_can_store_dword(int fd, unsigned int engine);
bool gem_class_can_store_dword(int fd, int class);
extern const struct intel_execution_engine2 {
- const char *name;
+ char name[10];
int class;
int instance;
uint64_t flags;
bool is_virtual;
} intel_execution_engines2[];
+extern const struct intel_execution_engine_class {
+ const char *name;
+ int class;
+} intel_execution_engine_class_map[];
+
int gem_execbuf_flags_to_engine_class(unsigned int flags);
#endif /* IGT_GT_H */
--
2.19.1
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [igt-dev] ✗ GitLab.Pipeline: warning for lib/i915: Generate engine names at runtime
2019-11-15 12:14 [igt-dev] [RFC PATCH i-g-t] lib/i915: Generate engine names at runtime Petri Latvala
@ 2019-11-15 12:30 ` Patchwork
2019-11-15 12:52 ` [igt-dev] ✓ Fi.CI.BAT: success " Patchwork
` (4 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2019-11-15 12:30 UTC (permalink / raw)
To: Petri Latvala; +Cc: igt-dev
== Series Details ==
Series: lib/i915: Generate engine names at runtime
URL : https://patchwork.freedesktop.org/series/69526/
State : warning
== Summary ==
Did not get list of undocumented tests for this run, something is wrong!
Other than that, pipeline status: FAILED.
see https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/pipelines/79450 for the overview.
build:tests-fedora-clang has failed (https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/-/jobs/953394):
ninja: build stopped: subcommand failed.
ninja: Entering directory `build'
[1/635] Generating version.h with a custom command.
[2/488] Linking target tests/gem_concurrent_blit.
[3/488] Linking target tests/gem_ctx_create.
[4/488] Compiling C object 'tests/59830eb@@gem_ctx_isolation@exe/i915_gem_ctx_isolation.c.o'.
FAILED: tests/59830eb@@gem_ctx_isolation@exe/i915_gem_ctx_isolation.c.o
clang -Itests/59830eb@@gem_ctx_isolation@exe -Itests -I../tests -I../include/drm-uapi -Ilib -I../lib -I../lib/stubs/syscalls -I. -I../ -I/usr/include/cairo -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/freetype2 -I/usr/include/libpng16 -I/usr/include/libdrm -I/usr/include/valgrind -I/usr/include/libdrm/nouveau -Xclang -fcolor-diagnostics -pipe -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wextra -std=gnu11 -O2 -g -D_GNU_SOURCE -include config.h -D_FORTIFY_SOURCE=2 -Wbad-function-cast -Wdeclaration-after-statement -Wformat=2 -Wmissing-declarations -Wmissing-format-attribute -Wmissing-noreturn -Wmissing-prototypes -Wnested-externs -Wold-style-definition -Wpointer-arith -Wredundant-decls -Wshadow -Wstrict-prototypes -Wuninitialized -Wunused -Wno-missing-field-initializers -Wno-pointer-arith -Wno-address-of-packed-member -Wno-sign-compare -Wno-type-limits -Wno-unused-parameter -Wno-unused-result -Werror=address -Werror=array-bounds -Werror=implicit -Werror=init-self -Werror=int-to-pointer-cast -Werror=main -Werror=missing-braces -Werror=nonnull -Werror=pointer-to-int-cast -Werror=return-type -Werror=sequence-point -Werror=trigraphs -Werror=write-strings -fno-builtin-malloc -fno-builtin-calloc -pthread -MD -MQ 'tests/59830eb@@gem_ctx_isolation@exe/i915_gem_ctx_isolation.c.o' -MF 'tests/59830eb@@gem_ctx_isolation@exe/i915_gem_ctx_isolation.c.o.d' -o 'tests/59830eb@@gem_ctx_isolation@exe/i915_gem_ctx_isolation.c.o' -c ../tests/i915/gem_ctx_isolation.c
../tests/i915/gem_ctx_isolation.c:880:10: error: address of array 'e->name' will always evaluate to 'true' [-Werror,-Wpointer-bool-conversion]
e->name; e++) {
~~~^~~~
1 error generated.
ninja: build stopped: subcommand failed.
section_end:1573820968:build_script
^[[0Ksection_start:1573820968:after_script
^[[0Ksection_end:1573820970:after_script
^[[0Ksection_start:1573820970:upload_artifacts_on_failure
^[[0Ksection_end:1573820972:upload_artifacts_on_failure
^[[0K^[[31;1mERROR: Job failed: exit code 1
^[[0;m
== Logs ==
For more details see: https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/pipelines/79450
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 10+ messages in thread
* [igt-dev] ✓ Fi.CI.BAT: success for lib/i915: Generate engine names at runtime
2019-11-15 12:14 [igt-dev] [RFC PATCH i-g-t] lib/i915: Generate engine names at runtime Petri Latvala
2019-11-15 12:30 ` [igt-dev] ✗ GitLab.Pipeline: warning for " Patchwork
@ 2019-11-15 12:52 ` Patchwork
2019-11-15 14:15 ` [igt-dev] [RFC PATCH i-g-t] " Andi Shyti
` (3 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2019-11-15 12:52 UTC (permalink / raw)
To: Petri Latvala; +Cc: igt-dev
== Series Details ==
Series: lib/i915: Generate engine names at runtime
URL : https://patchwork.freedesktop.org/series/69526/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_7351 -> IGTPW_3712
====================================================
Summary
-------
**SUCCESS**
No regressions found.
External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/index.html
Known issues
------------
Here are the changes found in IGTPW_3712 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@i915_selftest@live_gt_heartbeat:
- fi-kbl-r: [PASS][1] -> [DMESG-FAIL][2] ([fdo#112096])
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/fi-kbl-r/igt@i915_selftest@live_gt_heartbeat.html
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/fi-kbl-r/igt@i915_selftest@live_gt_heartbeat.html
* igt@kms_frontbuffer_tracking@basic:
- fi-icl-guc: [PASS][3] -> [FAIL][4] ([fdo#103167])
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/fi-icl-guc/igt@kms_frontbuffer_tracking@basic.html
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/fi-icl-guc/igt@kms_frontbuffer_tracking@basic.html
#### Possible fixes ####
* igt@i915_pm_rpm@module-reload:
- fi-skl-6770hq: [FAIL][5] ([fdo#108511]) -> [PASS][6]
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/fi-skl-6770hq/igt@i915_pm_rpm@module-reload.html
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/fi-skl-6770hq/igt@i915_pm_rpm@module-reload.html
- fi-skl-lmem: [DMESG-WARN][7] ([fdo#112261]) -> [PASS][8]
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/fi-skl-lmem/igt@i915_pm_rpm@module-reload.html
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/fi-skl-lmem/igt@i915_pm_rpm@module-reload.html
* igt@i915_selftest@live_gem_contexts:
- fi-bsw-kefka: [INCOMPLETE][9] ([fdo# 111542]) -> [PASS][10]
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/fi-bsw-kefka/igt@i915_selftest@live_gem_contexts.html
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/fi-bsw-kefka/igt@i915_selftest@live_gem_contexts.html
* igt@kms_busy@basic-flip-pipe-b:
- fi-skl-6770hq: [DMESG-WARN][11] ([fdo#105541]) -> [PASS][12]
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/fi-skl-6770hq/igt@kms_busy@basic-flip-pipe-b.html
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/fi-skl-6770hq/igt@kms_busy@basic-flip-pipe-b.html
* igt@kms_chamelium@hdmi-hpd-fast:
- fi-kbl-7500u: [FAIL][13] ([fdo#111407]) -> [PASS][14]
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo# 111542]: https://bugs.freedesktop.org/show_bug.cgi?id= 111542
[fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
[fdo#105541]: https://bugs.freedesktop.org/show_bug.cgi?id=105541
[fdo#108511]: https://bugs.freedesktop.org/show_bug.cgi?id=108511
[fdo#109964]: https://bugs.freedesktop.org/show_bug.cgi?id=109964
[fdo#111407]: https://bugs.freedesktop.org/show_bug.cgi?id=111407
[fdo#112096]: https://bugs.freedesktop.org/show_bug.cgi?id=112096
[fdo#112261]: https://bugs.freedesktop.org/show_bug.cgi?id=112261
[fdo#112298]: https://bugs.freedesktop.org/show_bug.cgi?id=112298
Participating hosts (49 -> 46)
------------------------------
Additional (1): fi-tgl-u
Missing (4): fi-byt-clapper fi-byt-squawks fi-bsw-cyan fi-hsw-4200u
Build changes
-------------
* CI: CI-20190529 -> None
* IGT: IGT_5288 -> IGTPW_3712
CI-20190529: 20190529
CI_DRM_7351: 6e85a4fe33033f280092a2ddf9e4dff6e1753fcc @ git://anongit.freedesktop.org/gfx-ci/linux
IGTPW_3712: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/index.html
IGT_5288: ff4551e36cd8e573ceb1e450d17a12e3298dc04c @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
== Testlist changes ==
+igt@gem_ctx_isolation@-clean
+igt@gem_ctx_isolation@-dirty-create
+igt@gem_ctx_isolation@-dirty-switch
+igt@gem_ctx_isolation@-none
+igt@gem_ctx_isolation@-nonpriv
+igt@gem_ctx_isolation@-nonpriv-switch
+igt@gem_ctx_isolation@-reset
+igt@gem_ctx_isolation@-s3
+igt@gem_ctx_isolation@-s4
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/index.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [igt-dev] [RFC PATCH i-g-t] lib/i915: Generate engine names at runtime
2019-11-15 12:14 [igt-dev] [RFC PATCH i-g-t] lib/i915: Generate engine names at runtime Petri Latvala
2019-11-15 12:30 ` [igt-dev] ✗ GitLab.Pipeline: warning for " Patchwork
2019-11-15 12:52 ` [igt-dev] ✓ Fi.CI.BAT: success " Patchwork
@ 2019-11-15 14:15 ` Andi Shyti
2019-11-15 16:01 ` Tvrtko Ursulin
` (2 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Andi Shyti @ 2019-11-15 14:15 UTC (permalink / raw)
To: Petri Latvala; +Cc: igt-dev, Tvrtko Ursulin
Hi Petri,
> @@ -120,12 +121,22 @@ static void init_engine(struct intel_execution_engine2 *e2,
> if (__e2->class == class && __e2->instance == instance)
> break;
>
> - if (__e2->name) {
> - e2->name = __e2->name;
> + if (__e2->name[0]) {
> + strncpy(e2->name, __e2->name, sizeof(e2->name));
> } else {
> - igt_warn("found unknown engine (%d, %d)\n", class, instance);
> - e2->name = unknown_name;
> - e2->flags = -1;
> + for (cls = intel_execution_engine_class_map; cls->name; cls++) {
> + if (cls->class == class) {
> + snprintf(e2->name, sizeof(e2->name), "%s%d", cls->name, instance);
> + igt_info("unknown but supported engine %s found\n", e2->name);
> + break;
> + }
> + }
We need to make a decision here and refactor this library
accordingly. We either:
1. decide to get rid of all the static table and keep only
"intel_execution_engine_class" (and maybe we can spare that one
as well).
or
2. add the new classes in the static table as it's meant to be
used.
I honestly do not like any hybrid solution and when this library
was developed in a first place we had to accept lots of
compromises because we needed both.
Now that we have the dynamic_libraries, can we finally remove
all the static tables and finally go for the solution 1?
> +
> + if (!e2->name[0]) {
> + igt_info("found unknown engine (%d, %d)\n", class, instance);
> + strncpy(e2->name, unknown_name, sizeof(e2->name));
nit: just strcpy would do, we know how big is unkown_name and it
won't change.
[...]
> +const struct intel_execution_engine_class intel_execution_engine_class_map[] = {
> + { "rcs", I915_ENGINE_CLASS_RENDER },
> + { "bcs", I915_ENGINE_CLASS_COPY },
> + { "vcs", I915_ENGINE_CLASS_VIDEO },
> + { "vecs", I915_ENGINE_CLASS_VIDEO_ENHANCE },
> + { }
> +};
another one? it would be the third const struct that is used to
get the engine names/instances. I thought we wanted to eliminate
them in the long run :'(
Andi
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [igt-dev] [RFC PATCH i-g-t] lib/i915: Generate engine names at runtime
2019-11-15 12:14 [igt-dev] [RFC PATCH i-g-t] lib/i915: Generate engine names at runtime Petri Latvala
` (2 preceding siblings ...)
2019-11-15 14:15 ` [igt-dev] [RFC PATCH i-g-t] " Andi Shyti
@ 2019-11-15 16:01 ` Tvrtko Ursulin
2019-11-18 10:05 ` Petri Latvala
2019-11-16 18:29 ` [igt-dev] ✗ Fi.CI.IGT: failure for " Patchwork
2020-01-22 13:44 ` [igt-dev] [RFC PATCH i-g-t] " Tvrtko Ursulin
5 siblings, 1 reply; 10+ messages in thread
From: Tvrtko Ursulin @ 2019-11-15 16:01 UTC (permalink / raw)
To: Petri Latvala, igt-dev; +Cc: Tvrtko Ursulin
On 15/11/2019 12:14, Petri Latvala wrote:
> The kernel supplies us an engine list with engine class and an
> instance id. If the hardcoded engine list doesn't have the
> class+instance we get, construct the engine name with printf instead
> of calling additional engines "unknown".
>
> Signed-off-by: Petri Latvala <petri.latvala@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Andi Shyti <andi.shyti@intel.com>
> Cc: Katarzyna Dec <katarzyna.dec@intel.com>
>
> ---
> lib/i915/gem_engine_topology.c | 27 +++++++++++++++++++--------
> lib/i915/gem_engine_topology.h | 2 +-
> lib/igt_gt.c | 8 ++++++++
> lib/igt_gt.h | 7 ++++++-
> 4 files changed, 34 insertions(+), 10 deletions(-)
>
> diff --git a/lib/i915/gem_engine_topology.c b/lib/i915/gem_engine_topology.c
> index 790d455f..0707f237 100644
> --- a/lib/i915/gem_engine_topology.c
> +++ b/lib/i915/gem_engine_topology.c
> @@ -103,6 +103,7 @@ static void init_engine(struct intel_execution_engine2 *e2,
> const struct intel_execution_engine2 *__e2;
> static const char *unknown_name = "unknown",
> *virtual_name = "virtual";
> + const struct intel_execution_engine_class *cls;
>
> e2->class = class;
> e2->instance = instance;
> @@ -111,7 +112,7 @@ static void init_engine(struct intel_execution_engine2 *e2,
> /* engine is a virtual engine */
> if (class == I915_ENGINE_CLASS_INVALID &&
> instance == I915_ENGINE_CLASS_INVALID_VIRTUAL) {
> - e2->name = virtual_name;
> + strncpy(e2->name, virtual_name, sizeof(e2->name));
> e2->is_virtual = true;
> return;
> }
> @@ -120,12 +121,22 @@ static void init_engine(struct intel_execution_engine2 *e2,
> if (__e2->class == class && __e2->instance == instance)
> break;
>
> - if (__e2->name) {
> - e2->name = __e2->name;
> + if (__e2->name[0]) {
> + strncpy(e2->name, __e2->name, sizeof(e2->name));
> } else {
> - igt_warn("found unknown engine (%d, %d)\n", class, instance);
> - e2->name = unknown_name;
> - e2->flags = -1;
> + for (cls = intel_execution_engine_class_map; cls->name; cls++) {
> + if (cls->class == class) {
> + snprintf(e2->name, sizeof(e2->name), "%s%d", cls->name, instance);
> + igt_info("unknown but supported engine %s found\n", e2->name);
Hm not really unknown, just a new instance relative to the static table.
I'd probably downgrade this to debug level?
> + break;
> + }
> + }
> +
> + if (!e2->name[0]) {
> + igt_info("found unknown engine (%d, %d)\n", class, instance);
> + strncpy(e2->name, unknown_name, sizeof(e2->name));
unknown<class>:<instance> ?
And say unknown _class_ in the log message?
> + e2->flags = -1;
> + }
> }
>
> /* just to remark it */
> @@ -223,7 +234,7 @@ struct intel_engine_data intel_init_engine_list(int fd, uint32_t ctx_id)
> struct intel_execution_engine2 *__e2 =
> &engine_data.engines[engine_data.nengines];
>
> - __e2->name = e2->name;
> + strncpy(__e2->name, e2->name, sizeof(__e2->name));
> __e2->instance = e2->instance;
> __e2->class = e2->class;
> __e2->flags = e2->flags;
> @@ -302,7 +313,7 @@ struct intel_execution_engine2 gem_eb_flags_to_engine(unsigned int flags)
>
> if (ring == I915_EXEC_DEFAULT) {
> e2__.flags = I915_EXEC_DEFAULT;
> - e2__.name = "default";
> + strncpy(e2__.name, "default", sizeof(e2__.name));
> } else {
> const struct intel_execution_engine2 *e2;
>
> diff --git a/lib/i915/gem_engine_topology.h b/lib/i915/gem_engine_topology.h
> index d98773e0..525741cc 100644
> --- a/lib/i915/gem_engine_topology.h
> +++ b/lib/i915/gem_engine_topology.h
> @@ -61,7 +61,7 @@ bool gem_engine_is_equal(const struct intel_execution_engine2 *e1,
> struct intel_execution_engine2 gem_eb_flags_to_engine(unsigned int flags);
>
> #define __for_each_static_engine(e__) \
> - for ((e__) = intel_execution_engines2; (e__)->name; (e__)++)
> + for ((e__) = intel_execution_engines2; (e__)->name[0]; (e__)++)
>
> #define for_each_context_engine(fd__, ctx__, e__) \
> for (struct intel_engine_data i__ = intel_init_engine_list(fd__, ctx__); \
> diff --git a/lib/igt_gt.c b/lib/igt_gt.c
> index 256c7cbc..25e6c455 100644
> --- a/lib/igt_gt.c
> +++ b/lib/igt_gt.c
> @@ -596,6 +596,14 @@ const struct intel_execution_engine2 intel_execution_engines2[] = {
> { }
> };
>
> +const struct intel_execution_engine_class intel_execution_engine_class_map[] = {
> + { "rcs", I915_ENGINE_CLASS_RENDER },
> + { "bcs", I915_ENGINE_CLASS_COPY },
> + { "vcs", I915_ENGINE_CLASS_VIDEO },
> + { "vecs", I915_ENGINE_CLASS_VIDEO_ENHANCE },
> + { }
> +};
> +
> int gem_execbuf_flags_to_engine_class(unsigned int flags)
> {
> switch (flags & 0x3f) {
> diff --git a/lib/igt_gt.h b/lib/igt_gt.h
> index 66088d39..0268031f 100644
> --- a/lib/igt_gt.h
> +++ b/lib/igt_gt.h
> @@ -95,13 +95,18 @@ bool gem_can_store_dword(int fd, unsigned int engine);
> bool gem_class_can_store_dword(int fd, int class);
>
> extern const struct intel_execution_engine2 {
> - const char *name;
> + char name[10];
> int class;
> int instance;
> uint64_t flags;
> bool is_virtual;
> } intel_execution_engines2[];
>
> +extern const struct intel_execution_engine_class {
> + const char *name;
> + int class;
> +} intel_execution_engine_class_map[];
> +
> int gem_execbuf_flags_to_engine_class(unsigned int flags);
>
> #endif /* IGT_GT_H */
>
Implementation looks okay to me, but what does it gains us? Potentially
introduces a slight confusion between subtests as enumerated by engine
and engines iterated inside some tests.
Only an interim step before elimination of the static array? But danger
is in the above mismatch.
Regards,
Tvrtko
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 10+ messages in thread
* [igt-dev] ✗ Fi.CI.IGT: failure for lib/i915: Generate engine names at runtime
2019-11-15 12:14 [igt-dev] [RFC PATCH i-g-t] lib/i915: Generate engine names at runtime Petri Latvala
` (3 preceding siblings ...)
2019-11-15 16:01 ` Tvrtko Ursulin
@ 2019-11-16 18:29 ` Patchwork
2020-01-22 13:44 ` [igt-dev] [RFC PATCH i-g-t] " Tvrtko Ursulin
5 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2019-11-16 18:29 UTC (permalink / raw)
To: Petri Latvala; +Cc: igt-dev
== Series Details ==
Series: lib/i915: Generate engine names at runtime
URL : https://patchwork.freedesktop.org/series/69526/
State : failure
== Summary ==
CI Bug Log - changes from CI_DRM_7351_full -> IGTPW_3712_full
====================================================
Summary
-------
**FAILURE**
Serious unknown changes coming with IGTPW_3712_full absolutely need to be
verified manually.
If you think the reported changes have nothing to do with the changes
introduced in IGTPW_3712_full, please notify your bug team to allow them
to document this new failure mode, which will reduce false positives in CI.
External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/index.html
Possible new issues
-------------------
Here are the unknown changes that may have been introduced in IGTPW_3712_full:
### IGT changes ###
#### Possible regressions ####
* {igt@gem_ctx_isolation@-s3} (NEW):
- shard-tglb: NOTRUN -> [INCOMPLETE][1]
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-tglb5/igt@gem_ctx_isolation@-s3.html
* igt@gem_persistent_relocs@forked-interruptible-faulting-reloc-thrash-inactive:
- shard-apl: [PASS][2] -> [DMESG-FAIL][3] +1 similar issue
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-apl3/igt@gem_persistent_relocs@forked-interruptible-faulting-reloc-thrash-inactive.html
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-apl7/igt@gem_persistent_relocs@forked-interruptible-faulting-reloc-thrash-inactive.html
#### Suppressed ####
The following results come from untrusted machines, tests, or statuses.
They do not affect the overall result.
* {igt@gem_exec_parse_blt@bb-secure}:
- shard-tglb: NOTRUN -> [SKIP][4] +3 similar issues
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-tglb7/igt@gem_exec_parse_blt@bb-secure.html
New tests
---------
New tests have been introduced between CI_DRM_7351_full and IGTPW_3712_full:
### New IGT tests (8) ###
* igt@gem_ctx_isolation@-clean:
- Statuses : 6 pass(s)
- Exec time: [0.01, 0.11] s
* igt@gem_ctx_isolation@-dirty-create:
- Statuses : 7 pass(s)
- Exec time: [0.12, 1.05] s
* igt@gem_ctx_isolation@-dirty-switch:
- Statuses : 5 pass(s)
- Exec time: [0.26, 1.11] s
* igt@gem_ctx_isolation@-none:
- Statuses : 7 pass(s)
- Exec time: [0.16, 0.75] s
* igt@gem_ctx_isolation@-nonpriv:
- Statuses : 5 pass(s) 2 skip(s)
- Exec time: [0.0, 1.25] s
* igt@gem_ctx_isolation@-nonpriv-switch:
- Statuses : 5 pass(s) 2 skip(s)
- Exec time: [0.0, 1.57] s
* igt@gem_ctx_isolation@-reset:
- Statuses : 6 pass(s)
- Exec time: [0.08, 0.73] s
* igt@gem_ctx_isolation@-s3:
- Statuses : 1 incomplete(s) 6 pass(s)
- Exec time: [0.0, 5.98] s
Known issues
------------
Here are the changes found in IGTPW_3712_full that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@gem_ctx_exec@basic-invalid-context-vcs1:
- shard-iclb: [PASS][5] -> [SKIP][6] ([fdo#112080]) +19 similar issues
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-iclb1/igt@gem_ctx_exec@basic-invalid-context-vcs1.html
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-iclb7/igt@gem_ctx_exec@basic-invalid-context-vcs1.html
* igt@gem_ctx_isolation@bcs0-s3:
- shard-tglb: [PASS][7] -> [INCOMPLETE][8] ([fdo#111832])
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-tglb7/igt@gem_ctx_isolation@bcs0-s3.html
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-tglb5/igt@gem_ctx_isolation@bcs0-s3.html
* igt@gem_ctx_isolation@vcs1-clean:
- shard-iclb: [PASS][9] -> [SKIP][10] ([fdo#109276] / [fdo#112080]) +4 similar issues
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-iclb2/igt@gem_ctx_isolation@vcs1-clean.html
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-iclb5/igt@gem_ctx_isolation@vcs1-clean.html
* igt@gem_exec_schedule@preempt-other-chain-bsd:
- shard-iclb: [PASS][11] -> [SKIP][12] ([fdo#112146]) +6 similar issues
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-iclb8/igt@gem_exec_schedule@preempt-other-chain-bsd.html
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-iclb2/igt@gem_exec_schedule@preempt-other-chain-bsd.html
* igt@gem_exec_schedule@preempt-queue-chain-bsd2:
- shard-tglb: [PASS][13] -> [INCOMPLETE][14] ([fdo#111677])
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-tglb1/igt@gem_exec_schedule@preempt-queue-chain-bsd2.html
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-tglb6/igt@gem_exec_schedule@preempt-queue-chain-bsd2.html
* igt@gem_exec_schedule@preempt-queue-contexts-bsd1:
- shard-tglb: [PASS][15] -> [INCOMPLETE][16] ([fdo#111606] / [fdo#111677])
[15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-tglb1/igt@gem_exec_schedule@preempt-queue-contexts-bsd1.html
[16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-tglb6/igt@gem_exec_schedule@preempt-queue-contexts-bsd1.html
* igt@gem_userptr_blits@map-fixed-invalidate-busy:
- shard-snb: [PASS][17] -> [DMESG-WARN][18] ([fdo#111870]) +2 similar issues
[17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-snb7/igt@gem_userptr_blits@map-fixed-invalidate-busy.html
[18]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-snb5/igt@gem_userptr_blits@map-fixed-invalidate-busy.html
- shard-hsw: [PASS][19] -> [DMESG-WARN][20] ([fdo#111870])
[19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-hsw2/igt@gem_userptr_blits@map-fixed-invalidate-busy.html
[20]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-hsw7/igt@gem_userptr_blits@map-fixed-invalidate-busy.html
* igt@gem_workarounds@suspend-resume-context:
- shard-apl: [PASS][21] -> [DMESG-WARN][22] ([fdo#108566]) +3 similar issues
[21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-apl1/igt@gem_workarounds@suspend-resume-context.html
[22]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-apl1/igt@gem_workarounds@suspend-resume-context.html
* igt@i915_pm_rpm@system-suspend:
- shard-tglb: [PASS][23] -> [INCOMPLETE][24] ([fdo#111747] / [fdo#111850])
[23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-tglb3/igt@i915_pm_rpm@system-suspend.html
[24]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-tglb1/igt@i915_pm_rpm@system-suspend.html
* igt@kms_flip@flip-vs-suspend:
- shard-kbl: [PASS][25] -> [DMESG-WARN][26] ([fdo#108566])
[25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-kbl4/igt@kms_flip@flip-vs-suspend.html
[26]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-kbl1/igt@kms_flip@flip-vs-suspend.html
* igt@kms_frontbuffer_tracking@fbc-1p-pri-indfb-multidraw:
- shard-iclb: [PASS][27] -> [FAIL][28] ([fdo#103167])
[27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-iclb8/igt@kms_frontbuffer_tracking@fbc-1p-pri-indfb-multidraw.html
[28]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-iclb2/igt@kms_frontbuffer_tracking@fbc-1p-pri-indfb-multidraw.html
* igt@kms_frontbuffer_tracking@fbc-1p-primscrn-indfb-plflip-blt:
- shard-kbl: [PASS][29] -> [FAIL][30] ([fdo#103167])
[29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-kbl4/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-indfb-plflip-blt.html
[30]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-kbl1/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-indfb-plflip-blt.html
- shard-apl: [PASS][31] -> [FAIL][32] ([fdo#103167])
[31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-apl7/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-indfb-plflip-blt.html
[32]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-apl2/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-indfb-plflip-blt.html
* igt@kms_psr2_su@frontbuffer:
- shard-iclb: [PASS][33] -> [SKIP][34] ([fdo#109642] / [fdo#111068])
[33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-iclb2/igt@kms_psr2_su@frontbuffer.html
[34]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-iclb5/igt@kms_psr2_su@frontbuffer.html
* igt@kms_psr@psr2_sprite_plane_move:
- shard-iclb: [PASS][35] -> [SKIP][36] ([fdo#109441]) +2 similar issues
[35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-iclb2/igt@kms_psr@psr2_sprite_plane_move.html
[36]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-iclb6/igt@kms_psr@psr2_sprite_plane_move.html
* igt@kms_psr@psr2_suspend:
- shard-tglb: [PASS][37] -> [INCOMPLETE][38] ([fdo#111832] / [fdo#111850]) +1 similar issue
[37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-tglb7/igt@kms_psr@psr2_suspend.html
[38]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-tglb1/igt@kms_psr@psr2_suspend.html
* igt@kms_vblank@pipe-a-ts-continuation-dpms-suspend:
- shard-tglb: [PASS][39] -> [INCOMPLETE][40] ([fdo#111850]) +1 similar issue
[39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-tglb1/igt@kms_vblank@pipe-a-ts-continuation-dpms-suspend.html
[40]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-tglb2/igt@kms_vblank@pipe-a-ts-continuation-dpms-suspend.html
* igt@kms_vblank@pipe-c-ts-continuation-dpms-suspend:
- shard-kbl: [PASS][41] -> [INCOMPLETE][42] ([fdo#103665])
[41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-kbl2/igt@kms_vblank@pipe-c-ts-continuation-dpms-suspend.html
[42]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-kbl3/igt@kms_vblank@pipe-c-ts-continuation-dpms-suspend.html
* igt@prime_vgem@fence-wait-bsd2:
- shard-iclb: [PASS][43] -> [SKIP][44] ([fdo#109276]) +15 similar issues
[43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-iclb1/igt@prime_vgem@fence-wait-bsd2.html
[44]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-iclb7/igt@prime_vgem@fence-wait-bsd2.html
#### Possible fixes ####
* igt@gem_busy@busy-vcs1:
- shard-iclb: [SKIP][45] ([fdo#112080]) -> [PASS][46] +7 similar issues
[45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-iclb3/igt@gem_busy@busy-vcs1.html
[46]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-iclb2/igt@gem_busy@busy-vcs1.html
* igt@gem_ctx_isolation@bcs0-s3:
- shard-apl: [DMESG-WARN][47] ([fdo#108566]) -> [PASS][48] +1 similar issue
[47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-apl6/igt@gem_ctx_isolation@bcs0-s3.html
[48]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-apl8/igt@gem_ctx_isolation@bcs0-s3.html
* igt@gem_ctx_persistence@vcs1-queued:
- shard-iclb: [SKIP][49] ([fdo#109276] / [fdo#112080]) -> [PASS][50] +4 similar issues
[49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-iclb6/igt@gem_ctx_persistence@vcs1-queued.html
[50]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-iclb1/igt@gem_ctx_persistence@vcs1-queued.html
* igt@gem_ctx_shared@exec-shared-gtt-bsd:
- shard-kbl: [FAIL][51] -> [PASS][52]
[51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-kbl3/igt@gem_ctx_shared@exec-shared-gtt-bsd.html
[52]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-kbl2/igt@gem_ctx_shared@exec-shared-gtt-bsd.html
* igt@gem_exec_balancer@smoke:
- shard-iclb: [SKIP][53] ([fdo#110854]) -> [PASS][54]
[53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-iclb5/igt@gem_exec_balancer@smoke.html
[54]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-iclb4/igt@gem_exec_balancer@smoke.html
* igt@gem_exec_schedule@out-order-bsd2:
- shard-iclb: [SKIP][55] ([fdo#109276]) -> [PASS][56] +14 similar issues
[55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-iclb8/igt@gem_exec_schedule@out-order-bsd2.html
[56]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-iclb1/igt@gem_exec_schedule@out-order-bsd2.html
* igt@gem_exec_schedule@preempt-queue-bsd1:
- shard-tglb: [INCOMPLETE][57] ([fdo#111677]) -> [PASS][58]
[57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-tglb6/igt@gem_exec_schedule@preempt-queue-bsd1.html
[58]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-tglb1/igt@gem_exec_schedule@preempt-queue-bsd1.html
* igt@gem_exec_schedule@preempt-queue-contexts-render:
- shard-tglb: [INCOMPLETE][59] ([fdo#111606] / [fdo#111677]) -> [PASS][60]
[59]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-tglb6/igt@gem_exec_schedule@preempt-queue-contexts-render.html
[60]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-tglb5/igt@gem_exec_schedule@preempt-queue-contexts-render.html
* igt@gem_exec_schedule@wide-bsd:
- shard-iclb: [SKIP][61] ([fdo#112146]) -> [PASS][62] +7 similar issues
[61]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-iclb4/igt@gem_exec_schedule@wide-bsd.html
[62]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-iclb7/igt@gem_exec_schedule@wide-bsd.html
* igt@gem_persistent_relocs@forked-thrashing:
- shard-snb: [FAIL][63] ([fdo#112037]) -> [PASS][64]
[63]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-snb7/igt@gem_persistent_relocs@forked-thrashing.html
[64]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-snb2/igt@gem_persistent_relocs@forked-thrashing.html
* igt@gem_userptr_blits@map-fixed-invalidate-overlap-busy:
- shard-snb: [DMESG-WARN][65] ([fdo#111870]) -> [PASS][66] +1 similar issue
[65]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-snb4/igt@gem_userptr_blits@map-fixed-invalidate-overlap-busy.html
[66]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-snb1/igt@gem_userptr_blits@map-fixed-invalidate-overlap-busy.html
* igt@gem_userptr_blits@sync-unmap-after-close:
- shard-hsw: [DMESG-WARN][67] ([fdo#111870]) -> [PASS][68] +2 similar issues
[67]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-hsw6/igt@gem_userptr_blits@sync-unmap-after-close.html
[68]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-hsw5/igt@gem_userptr_blits@sync-unmap-after-close.html
* igt@i915_selftest@live_requests:
- shard-tglb: [INCOMPLETE][69] ([fdo#112057]) -> [PASS][70]
[69]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-tglb9/igt@i915_selftest@live_requests.html
[70]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-tglb8/igt@i915_selftest@live_requests.html
* igt@kms_busy@extended-pageflip-hang-newfb-render-pipe-b:
- shard-kbl: [DMESG-WARN][71] ([fdo#103313] / [fdo#105345]) -> [PASS][72]
[71]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-kbl2/igt@kms_busy@extended-pageflip-hang-newfb-render-pipe-b.html
[72]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-kbl1/igt@kms_busy@extended-pageflip-hang-newfb-render-pipe-b.html
* igt@kms_cursor_crc@pipe-a-cursor-128x42-offscreen:
- shard-tglb: [INCOMPLETE][73] ([fdo#111747]) -> [PASS][74]
[73]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-tglb1/igt@kms_cursor_crc@pipe-a-cursor-128x42-offscreen.html
[74]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-tglb3/igt@kms_cursor_crc@pipe-a-cursor-128x42-offscreen.html
* igt@kms_cursor_crc@pipe-b-cursor-128x42-random:
- shard-apl: [FAIL][75] ([fdo#103232]) -> [PASS][76]
[75]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-apl8/igt@kms_cursor_crc@pipe-b-cursor-128x42-random.html
[76]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-apl1/igt@kms_cursor_crc@pipe-b-cursor-128x42-random.html
- shard-kbl: [FAIL][77] ([fdo#103232]) -> [PASS][78]
[77]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-kbl3/igt@kms_cursor_crc@pipe-b-cursor-128x42-random.html
[78]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-kbl2/igt@kms_cursor_crc@pipe-b-cursor-128x42-random.html
* igt@kms_cursor_crc@pipe-c-cursor-128x42-sliding:
- shard-hsw: [FAIL][79] ([fdo#107814]) -> [PASS][80]
[79]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-hsw5/igt@kms_cursor_crc@pipe-c-cursor-128x42-sliding.html
[80]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-hsw5/igt@kms_cursor_crc@pipe-c-cursor-128x42-sliding.html
* igt@kms_draw_crc@draw-method-xrgb2101010-blt-untiled:
- shard-hsw: [DMESG-WARN][81] ([fdo#102614]) -> [PASS][82] +2 similar issues
[81]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-hsw5/igt@kms_draw_crc@draw-method-xrgb2101010-blt-untiled.html
[82]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-hsw6/igt@kms_draw_crc@draw-method-xrgb2101010-blt-untiled.html
* igt@kms_fbcon_fbt@fbc-suspend:
- shard-tglb: [INCOMPLETE][83] ([fdo#111747] / [fdo#111832] / [fdo#111850]) -> [PASS][84]
[83]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-tglb4/igt@kms_fbcon_fbt@fbc-suspend.html
[84]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-tglb9/igt@kms_fbcon_fbt@fbc-suspend.html
* igt@kms_flip@flip-vs-suspend:
- shard-tglb: [INCOMPLETE][85] ([fdo#111850] / [fdo#112031]) -> [PASS][86]
[85]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-tglb5/igt@kms_flip@flip-vs-suspend.html
[86]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-tglb6/igt@kms_flip@flip-vs-suspend.html
* igt@kms_frontbuffer_tracking@fbc-1p-primscrn-shrfb-plflip-blt:
- shard-apl: [FAIL][87] ([fdo#103167]) -> [PASS][88]
[87]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-apl8/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-shrfb-plflip-blt.html
[88]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-apl2/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-shrfb-plflip-blt.html
* igt@kms_frontbuffer_tracking@fbcpsr-1p-pri-indfb-multidraw:
- shard-tglb: [FAIL][89] ([fdo#103167]) -> [PASS][90]
[89]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-tglb9/igt@kms_frontbuffer_tracking@fbcpsr-1p-pri-indfb-multidraw.html
[90]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-tglb5/igt@kms_frontbuffer_tracking@fbcpsr-1p-pri-indfb-multidraw.html
* igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-render:
- shard-iclb: [FAIL][91] ([fdo#103167]) -> [PASS][92] +9 similar issues
[91]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-iclb8/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-render.html
[92]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-iclb2/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-render.html
* igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
- shard-tglb: [INCOMPLETE][93] ([fdo#111832] / [fdo#111850]) -> [PASS][94] +1 similar issue
[93]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-tglb3/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a.html
[94]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-tglb4/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a.html
* igt@kms_psr@no_drrs:
- shard-iclb: [FAIL][95] ([fdo#108341]) -> [PASS][96]
[95]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-iclb1/igt@kms_psr@no_drrs.html
[96]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-iclb4/igt@kms_psr@no_drrs.html
* igt@kms_psr@psr2_cursor_mmap_gtt:
- shard-iclb: [SKIP][97] ([fdo#109441]) -> [PASS][98]
[97]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7351/shard-iclb1/igt@kms_psr@psr2_cursor_mmap_gtt.html
[98]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/shard-iclb2/igt@kms_
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3712/index.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [igt-dev] [RFC PATCH i-g-t] lib/i915: Generate engine names at runtime
2019-11-15 16:01 ` Tvrtko Ursulin
@ 2019-11-18 10:05 ` Petri Latvala
2019-11-18 11:33 ` Tvrtko Ursulin
0 siblings, 1 reply; 10+ messages in thread
From: Petri Latvala @ 2019-11-18 10:05 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: igt-dev, Tvrtko Ursulin
On Fri, Nov 15, 2019 at 04:01:45PM +0000, Tvrtko Ursulin wrote:
>
> On 15/11/2019 12:14, Petri Latvala wrote:
> > The kernel supplies us an engine list with engine class and an
> > instance id. If the hardcoded engine list doesn't have the
> > class+instance we get, construct the engine name with printf instead
> > of calling additional engines "unknown".
> >
> > Signed-off-by: Petri Latvala <petri.latvala@intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Andi Shyti <andi.shyti@intel.com>
> > Cc: Katarzyna Dec <katarzyna.dec@intel.com>
> >
> > ---
> > lib/i915/gem_engine_topology.c | 27 +++++++++++++++++++--------
> > lib/i915/gem_engine_topology.h | 2 +-
> > lib/igt_gt.c | 8 ++++++++
> > lib/igt_gt.h | 7 ++++++-
> > 4 files changed, 34 insertions(+), 10 deletions(-)
> >
> > diff --git a/lib/i915/gem_engine_topology.c b/lib/i915/gem_engine_topology.c
> > index 790d455f..0707f237 100644
> > --- a/lib/i915/gem_engine_topology.c
> > +++ b/lib/i915/gem_engine_topology.c
> > @@ -103,6 +103,7 @@ static void init_engine(struct intel_execution_engine2 *e2,
> > const struct intel_execution_engine2 *__e2;
> > static const char *unknown_name = "unknown",
> > *virtual_name = "virtual";
> > + const struct intel_execution_engine_class *cls;
> > e2->class = class;
> > e2->instance = instance;
> > @@ -111,7 +112,7 @@ static void init_engine(struct intel_execution_engine2 *e2,
> > /* engine is a virtual engine */
> > if (class == I915_ENGINE_CLASS_INVALID &&
> > instance == I915_ENGINE_CLASS_INVALID_VIRTUAL) {
> > - e2->name = virtual_name;
> > + strncpy(e2->name, virtual_name, sizeof(e2->name));
> > e2->is_virtual = true;
> > return;
> > }
> > @@ -120,12 +121,22 @@ static void init_engine(struct intel_execution_engine2 *e2,
> > if (__e2->class == class && __e2->instance == instance)
> > break;
> > - if (__e2->name) {
> > - e2->name = __e2->name;
> > + if (__e2->name[0]) {
> > + strncpy(e2->name, __e2->name, sizeof(e2->name));
> > } else {
> > - igt_warn("found unknown engine (%d, %d)\n", class, instance);
> > - e2->name = unknown_name;
> > - e2->flags = -1;
> > + for (cls = intel_execution_engine_class_map; cls->name; cls++) {
> > + if (cls->class == class) {
> > + snprintf(e2->name, sizeof(e2->name), "%s%d", cls->name, instance);
> > + igt_info("unknown but supported engine %s found\n", e2->name);
>
> Hm not really unknown, just a new instance relative to the static table. I'd
> probably downgrade this to debug level?
Ah, a leftover from my debugging, ironically.
>
> > + break;
> > + }
> > + }
> > +
> > + if (!e2->name[0]) {
> > + igt_info("found unknown engine (%d, %d)\n", class, instance);
> > + strncpy(e2->name, unknown_name, sizeof(e2->name));
>
> unknown<class>:<instance> ?
>
> And say unknown _class_ in the log message?
Ok.
>
>
> > + e2->flags = -1;
> > + }
> > }
> > /* just to remark it */
> > @@ -223,7 +234,7 @@ struct intel_engine_data intel_init_engine_list(int fd, uint32_t ctx_id)
> > struct intel_execution_engine2 *__e2 =
> > &engine_data.engines[engine_data.nengines];
> > - __e2->name = e2->name;
> > + strncpy(__e2->name, e2->name, sizeof(__e2->name));
> > __e2->instance = e2->instance;
> > __e2->class = e2->class;
> > __e2->flags = e2->flags;
> > @@ -302,7 +313,7 @@ struct intel_execution_engine2 gem_eb_flags_to_engine(unsigned int flags)
> > if (ring == I915_EXEC_DEFAULT) {
> > e2__.flags = I915_EXEC_DEFAULT;
> > - e2__.name = "default";
> > + strncpy(e2__.name, "default", sizeof(e2__.name));
> > } else {
> > const struct intel_execution_engine2 *e2;
> > diff --git a/lib/i915/gem_engine_topology.h b/lib/i915/gem_engine_topology.h
> > index d98773e0..525741cc 100644
> > --- a/lib/i915/gem_engine_topology.h
> > +++ b/lib/i915/gem_engine_topology.h
> > @@ -61,7 +61,7 @@ bool gem_engine_is_equal(const struct intel_execution_engine2 *e1,
> > struct intel_execution_engine2 gem_eb_flags_to_engine(unsigned int flags);
> > #define __for_each_static_engine(e__) \
> > - for ((e__) = intel_execution_engines2; (e__)->name; (e__)++)
> > + for ((e__) = intel_execution_engines2; (e__)->name[0]; (e__)++)
> > #define for_each_context_engine(fd__, ctx__, e__) \
> > for (struct intel_engine_data i__ = intel_init_engine_list(fd__, ctx__); \
> > diff --git a/lib/igt_gt.c b/lib/igt_gt.c
> > index 256c7cbc..25e6c455 100644
> > --- a/lib/igt_gt.c
> > +++ b/lib/igt_gt.c
> > @@ -596,6 +596,14 @@ const struct intel_execution_engine2 intel_execution_engines2[] = {
> > { }
> > };
> > +const struct intel_execution_engine_class intel_execution_engine_class_map[] = {
> > + { "rcs", I915_ENGINE_CLASS_RENDER },
> > + { "bcs", I915_ENGINE_CLASS_COPY },
> > + { "vcs", I915_ENGINE_CLASS_VIDEO },
> > + { "vecs", I915_ENGINE_CLASS_VIDEO_ENHANCE },
> > + { }
> > +};
> > +
> > int gem_execbuf_flags_to_engine_class(unsigned int flags)
> > {
> > switch (flags & 0x3f) {
> > diff --git a/lib/igt_gt.h b/lib/igt_gt.h
> > index 66088d39..0268031f 100644
> > --- a/lib/igt_gt.h
> > +++ b/lib/igt_gt.h
> > @@ -95,13 +95,18 @@ bool gem_can_store_dword(int fd, unsigned int engine);
> > bool gem_class_can_store_dword(int fd, int class);
> > extern const struct intel_execution_engine2 {
> > - const char *name;
> > + char name[10];
> > int class;
> > int instance;
> > uint64_t flags;
> > bool is_virtual;
> > } intel_execution_engines2[];
> > +extern const struct intel_execution_engine_class {
> > + const char *name;
> > + int class;
> > +} intel_execution_engine_class_map[];
> > +
> > int gem_execbuf_flags_to_engine_class(unsigned int flags);
> > #endif /* IGT_GT_H */
> >
>
> Implementation looks okay to me, but what does it gains us? Potentially
> introduces a slight confusion between subtests as enumerated by engine and
> engines iterated inside some tests.
When all tests that enumerate subtests per engine have been converted
to dynamic subtests, this reduces the possible maintenance burden in
the coming years when we might have more engines. Imagine having a new
device tomorrow with vcs3 for example. With this patch it doesn't
require further IGT changes to be tested.
I should have said that merging this practically requires all
per-engine subtest enumeration converted to dynamic subtests first...
>
> Only an interim step before elimination of the static array? But danger is
> in the above mismatch.
What mismatch? *confusion* If you mean the enumeration, I hope the
above paragraph alleviates the dangers you see.
Interim step yes, I wouldn't call this anywhere close to clean code
atm.
Currently we need the static array for two things:
1) subtest enumeration
2) carrying the flags for the "old-style" execution
I'm hoping we can drop 1) sooner than later.
--
Petri Latvala
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [igt-dev] [RFC PATCH i-g-t] lib/i915: Generate engine names at runtime
2019-11-18 10:05 ` Petri Latvala
@ 2019-11-18 11:33 ` Tvrtko Ursulin
0 siblings, 0 replies; 10+ messages in thread
From: Tvrtko Ursulin @ 2019-11-18 11:33 UTC (permalink / raw)
To: Petri Latvala; +Cc: igt-dev, Tvrtko Ursulin
On 18/11/2019 10:05, Petri Latvala wrote:
> On Fri, Nov 15, 2019 at 04:01:45PM +0000, Tvrtko Ursulin wrote:
>> On 15/11/2019 12:14, Petri Latvala wrote:
>>> The kernel supplies us an engine list with engine class and an
>>> instance id. If the hardcoded engine list doesn't have the
>>> class+instance we get, construct the engine name with printf instead
>>> of calling additional engines "unknown".
>>>
>>> Signed-off-by: Petri Latvala <petri.latvala@intel.com>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Cc: Andi Shyti <andi.shyti@intel.com>
>>> Cc: Katarzyna Dec <katarzyna.dec@intel.com>
>>>
>>> ---
>>> lib/i915/gem_engine_topology.c | 27 +++++++++++++++++++--------
>>> lib/i915/gem_engine_topology.h | 2 +-
>>> lib/igt_gt.c | 8 ++++++++
>>> lib/igt_gt.h | 7 ++++++-
>>> 4 files changed, 34 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/lib/i915/gem_engine_topology.c b/lib/i915/gem_engine_topology.c
>>> index 790d455f..0707f237 100644
>>> --- a/lib/i915/gem_engine_topology.c
>>> +++ b/lib/i915/gem_engine_topology.c
>>> @@ -103,6 +103,7 @@ static void init_engine(struct intel_execution_engine2 *e2,
>>> const struct intel_execution_engine2 *__e2;
>>> static const char *unknown_name = "unknown",
>>> *virtual_name = "virtual";
>>> + const struct intel_execution_engine_class *cls;
>>> e2->class = class;
>>> e2->instance = instance;
>>> @@ -111,7 +112,7 @@ static void init_engine(struct intel_execution_engine2 *e2,
>>> /* engine is a virtual engine */
>>> if (class == I915_ENGINE_CLASS_INVALID &&
>>> instance == I915_ENGINE_CLASS_INVALID_VIRTUAL) {
>>> - e2->name = virtual_name;
>>> + strncpy(e2->name, virtual_name, sizeof(e2->name));
>>> e2->is_virtual = true;
>>> return;
>>> }
>>> @@ -120,12 +121,22 @@ static void init_engine(struct intel_execution_engine2 *e2,
>>> if (__e2->class == class && __e2->instance == instance)
>>> break;
>>> - if (__e2->name) {
>>> - e2->name = __e2->name;
>>> + if (__e2->name[0]) {
>>> + strncpy(e2->name, __e2->name, sizeof(e2->name));
>>> } else {
>>> - igt_warn("found unknown engine (%d, %d)\n", class, instance);
>>> - e2->name = unknown_name;
>>> - e2->flags = -1;
>>> + for (cls = intel_execution_engine_class_map; cls->name; cls++) {
>>> + if (cls->class == class) {
>>> + snprintf(e2->name, sizeof(e2->name), "%s%d", cls->name, instance);
>>> + igt_info("unknown but supported engine %s found\n", e2->name);
>>
>> Hm not really unknown, just a new instance relative to the static table. I'd
>> probably downgrade this to debug level?
>
> Ah, a leftover from my debugging, ironically.
>
>>
>>> + break;
>>> + }
>>> + }
>>> +
>>> + if (!e2->name[0]) {
>>> + igt_info("found unknown engine (%d, %d)\n", class, instance);
>>> + strncpy(e2->name, unknown_name, sizeof(e2->name));
>>
>> unknown<class>:<instance> ?
>>
>> And say unknown _class_ in the log message?
>
> Ok.
>
>>
>>
>>> + e2->flags = -1;
>>> + }
>>> }
>>> /* just to remark it */
>>> @@ -223,7 +234,7 @@ struct intel_engine_data intel_init_engine_list(int fd, uint32_t ctx_id)
>>> struct intel_execution_engine2 *__e2 =
>>> &engine_data.engines[engine_data.nengines];
>>> - __e2->name = e2->name;
>>> + strncpy(__e2->name, e2->name, sizeof(__e2->name));
>>> __e2->instance = e2->instance;
>>> __e2->class = e2->class;
>>> __e2->flags = e2->flags;
>>> @@ -302,7 +313,7 @@ struct intel_execution_engine2 gem_eb_flags_to_engine(unsigned int flags)
>>> if (ring == I915_EXEC_DEFAULT) {
>>> e2__.flags = I915_EXEC_DEFAULT;
>>> - e2__.name = "default";
>>> + strncpy(e2__.name, "default", sizeof(e2__.name));
>>> } else {
>>> const struct intel_execution_engine2 *e2;
>>> diff --git a/lib/i915/gem_engine_topology.h b/lib/i915/gem_engine_topology.h
>>> index d98773e0..525741cc 100644
>>> --- a/lib/i915/gem_engine_topology.h
>>> +++ b/lib/i915/gem_engine_topology.h
>>> @@ -61,7 +61,7 @@ bool gem_engine_is_equal(const struct intel_execution_engine2 *e1,
>>> struct intel_execution_engine2 gem_eb_flags_to_engine(unsigned int flags);
>>> #define __for_each_static_engine(e__) \
>>> - for ((e__) = intel_execution_engines2; (e__)->name; (e__)++)
>>> + for ((e__) = intel_execution_engines2; (e__)->name[0]; (e__)++)
>>> #define for_each_context_engine(fd__, ctx__, e__) \
>>> for (struct intel_engine_data i__ = intel_init_engine_list(fd__, ctx__); \
>>> diff --git a/lib/igt_gt.c b/lib/igt_gt.c
>>> index 256c7cbc..25e6c455 100644
>>> --- a/lib/igt_gt.c
>>> +++ b/lib/igt_gt.c
>>> @@ -596,6 +596,14 @@ const struct intel_execution_engine2 intel_execution_engines2[] = {
>>> { }
>>> };
>>> +const struct intel_execution_engine_class intel_execution_engine_class_map[] = {
>>> + { "rcs", I915_ENGINE_CLASS_RENDER },
>>> + { "bcs", I915_ENGINE_CLASS_COPY },
>>> + { "vcs", I915_ENGINE_CLASS_VIDEO },
>>> + { "vecs", I915_ENGINE_CLASS_VIDEO_ENHANCE },
>>> + { }
>>> +};
>>> +
>>> int gem_execbuf_flags_to_engine_class(unsigned int flags)
>>> {
>>> switch (flags & 0x3f) {
>>> diff --git a/lib/igt_gt.h b/lib/igt_gt.h
>>> index 66088d39..0268031f 100644
>>> --- a/lib/igt_gt.h
>>> +++ b/lib/igt_gt.h
>>> @@ -95,13 +95,18 @@ bool gem_can_store_dword(int fd, unsigned int engine);
>>> bool gem_class_can_store_dword(int fd, int class);
>>> extern const struct intel_execution_engine2 {
>>> - const char *name;
>>> + char name[10];
>>> int class;
>>> int instance;
>>> uint64_t flags;
>>> bool is_virtual;
>>> } intel_execution_engines2[];
>>> +extern const struct intel_execution_engine_class {
>>> + const char *name;
>>> + int class;
>>> +} intel_execution_engine_class_map[];
>>> +
>>> int gem_execbuf_flags_to_engine_class(unsigned int flags);
>>> #endif /* IGT_GT_H */
>>>
>>
>> Implementation looks okay to me, but what does it gains us? Potentially
>> introduces a slight confusion between subtests as enumerated by engine and
>> engines iterated inside some tests.
>
> When all tests that enumerate subtests per engine have been converted
> to dynamic subtests, this reduces the possible maintenance burden in
> the coming years when we might have more engines. Imagine having a new
> device tomorrow with vcs3 for example. With this patch it doesn't
> require further IGT changes to be tested.
>
> I should have said that merging this practically requires all
> per-engine subtest enumeration converted to dynamic subtests first...
>
>
>>
>> Only an interim step before elimination of the static array? But danger is
>> in the above mismatch.
>
> What mismatch? *confusion* If you mean the enumeration, I hope the
> above paragraph alleviates the dangers you see.
>
> Interim step yes, I wouldn't call this anywhere close to clean code
> atm.
>
> Currently we need the static array for two things:
>
> 1) subtest enumeration
> 2) carrying the flags for the "old-style" execution
>
> I'm hoping we can drop 1) sooner than later.
No need to explain the advantages. ;) If this would be merged together
with removal of the static array, that is, all engine tests were
converted to dynamic, there would be no issue. But if we merge this
before, then we have a confusion where some tests, which seemingly use
the new API (have been converted), can see some engines and some cannot.
I think that complaint would go away if you didn't include the "unknown
but supported" change in this patch. Then it would behave like the
current code where if the engine is not in the static list it always
refuses to use it.
Although I am not sure how important that is, I guess it is open for
discussion.
Perhaps you could split the patch into two, where the first one just
adds the runtime name generation, and second one extends with support
for unknown engines of a known class?
Regards,
Tvrtko
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [igt-dev] [RFC PATCH i-g-t] lib/i915: Generate engine names at runtime
2019-11-15 12:14 [igt-dev] [RFC PATCH i-g-t] lib/i915: Generate engine names at runtime Petri Latvala
` (4 preceding siblings ...)
2019-11-16 18:29 ` [igt-dev] ✗ Fi.CI.IGT: failure for " Patchwork
@ 2020-01-22 13:44 ` Tvrtko Ursulin
2020-01-22 13:52 ` Petri Latvala
5 siblings, 1 reply; 10+ messages in thread
From: Tvrtko Ursulin @ 2020-01-22 13:44 UTC (permalink / raw)
To: Petri Latvala, igt-dev; +Cc: Tvrtko Ursulin
On 15/11/2019 12:14, Petri Latvala wrote:
> The kernel supplies us an engine list with engine class and an
> instance id. If the hardcoded engine list doesn't have the
> class+instance we get, construct the engine name with printf instead
> of calling additional engines "unknown".
>
> Signed-off-by: Petri Latvala <petri.latvala@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Andi Shyti <andi.shyti@intel.com>
> Cc: Katarzyna Dec <katarzyna.dec@intel.com>
>
> ---
> lib/i915/gem_engine_topology.c | 27 +++++++++++++++++++--------
> lib/i915/gem_engine_topology.h | 2 +-
> lib/igt_gt.c | 8 ++++++++
> lib/igt_gt.h | 7 ++++++-
> 4 files changed, 34 insertions(+), 10 deletions(-)
>
> diff --git a/lib/i915/gem_engine_topology.c b/lib/i915/gem_engine_topology.c
> index 790d455f..0707f237 100644
> --- a/lib/i915/gem_engine_topology.c
> +++ b/lib/i915/gem_engine_topology.c
> @@ -103,6 +103,7 @@ static void init_engine(struct intel_execution_engine2 *e2,
> const struct intel_execution_engine2 *__e2;
> static const char *unknown_name = "unknown",
> *virtual_name = "virtual";
> + const struct intel_execution_engine_class *cls;
>
> e2->class = class;
> e2->instance = instance;
> @@ -111,7 +112,7 @@ static void init_engine(struct intel_execution_engine2 *e2,
> /* engine is a virtual engine */
> if (class == I915_ENGINE_CLASS_INVALID &&
> instance == I915_ENGINE_CLASS_INVALID_VIRTUAL) {
> - e2->name = virtual_name;
> + strncpy(e2->name, virtual_name, sizeof(e2->name));
> e2->is_virtual = true;
> return;
> }
> @@ -120,12 +121,22 @@ static void init_engine(struct intel_execution_engine2 *e2,
> if (__e2->class == class && __e2->instance == instance)
> break;
>
> - if (__e2->name) {
> - e2->name = __e2->name;
> + if (__e2->name[0]) {
> + strncpy(e2->name, __e2->name, sizeof(e2->name));
> } else {
> - igt_warn("found unknown engine (%d, %d)\n", class, instance);
> - e2->name = unknown_name;
> - e2->flags = -1;
> + for (cls = intel_execution_engine_class_map; cls->name; cls++) {
> + if (cls->class == class) {
> + snprintf(e2->name, sizeof(e2->name), "%s%d", cls->name, instance);
> + igt_info("unknown but supported engine %s found\n", e2->name);
> + break;
> + }
> + }
> +
> + if (!e2->name[0]) {
> + igt_info("found unknown engine (%d, %d)\n", class, instance);
> + strncpy(e2->name, unknown_name, sizeof(e2->name));
> + e2->flags = -1;
> + }
> }
>
> /* just to remark it */
> @@ -223,7 +234,7 @@ struct intel_engine_data intel_init_engine_list(int fd, uint32_t ctx_id)
> struct intel_execution_engine2 *__e2 =
> &engine_data.engines[engine_data.nengines];
>
> - __e2->name = e2->name;
> + strncpy(__e2->name, e2->name, sizeof(__e2->name));
> __e2->instance = e2->instance;
> __e2->class = e2->class;
> __e2->flags = e2->flags;
> @@ -302,7 +313,7 @@ struct intel_execution_engine2 gem_eb_flags_to_engine(unsigned int flags)
>
> if (ring == I915_EXEC_DEFAULT) {
> e2__.flags = I915_EXEC_DEFAULT;
> - e2__.name = "default";
> + strncpy(e2__.name, "default", sizeof(e2__.name));
> } else {
> const struct intel_execution_engine2 *e2;
>
> diff --git a/lib/i915/gem_engine_topology.h b/lib/i915/gem_engine_topology.h
> index d98773e0..525741cc 100644
> --- a/lib/i915/gem_engine_topology.h
> +++ b/lib/i915/gem_engine_topology.h
> @@ -61,7 +61,7 @@ bool gem_engine_is_equal(const struct intel_execution_engine2 *e1,
> struct intel_execution_engine2 gem_eb_flags_to_engine(unsigned int flags);
>
> #define __for_each_static_engine(e__) \
> - for ((e__) = intel_execution_engines2; (e__)->name; (e__)++)
> + for ((e__) = intel_execution_engines2; (e__)->name[0]; (e__)++)
>
> #define for_each_context_engine(fd__, ctx__, e__) \
> for (struct intel_engine_data i__ = intel_init_engine_list(fd__, ctx__); \
> diff --git a/lib/igt_gt.c b/lib/igt_gt.c
> index 256c7cbc..25e6c455 100644
> --- a/lib/igt_gt.c
> +++ b/lib/igt_gt.c
> @@ -596,6 +596,14 @@ const struct intel_execution_engine2 intel_execution_engines2[] = {
> { }
> };
>
> +const struct intel_execution_engine_class intel_execution_engine_class_map[] = {
> + { "rcs", I915_ENGINE_CLASS_RENDER },
> + { "bcs", I915_ENGINE_CLASS_COPY },
> + { "vcs", I915_ENGINE_CLASS_VIDEO },
> + { "vecs", I915_ENGINE_CLASS_VIDEO_ENHANCE },
> + { }
> +};
> +
> int gem_execbuf_flags_to_engine_class(unsigned int flags)
> {
> switch (flags & 0x3f) {
> diff --git a/lib/igt_gt.h b/lib/igt_gt.h
> index 66088d39..0268031f 100644
> --- a/lib/igt_gt.h
> +++ b/lib/igt_gt.h
> @@ -95,13 +95,18 @@ bool gem_can_store_dword(int fd, unsigned int engine);
> bool gem_class_can_store_dword(int fd, int class);
>
> extern const struct intel_execution_engine2 {
> - const char *name;
> + char name[10];
You have the same bug as I do in
https://patchwork.freedesktop.org/series/72384/ in that
__for_each_static_engine has no engine names? :) And I have another bug
that I missed to adjust the iterator macro. :)
Otherwise similar implementations. Lets do one of the two and get it
over with... You were here first so do you want to send an update
including past and latest tweaks? Or if you are busy I'll tweak my
version into submission.
Regards,
Tvrtko
> int class;
> int instance;
> uint64_t flags;
> bool is_virtual;
> } intel_execution_engines2[];
>
> +extern const struct intel_execution_engine_class {
> + const char *name;
> + int class;
> +} intel_execution_engine_class_map[];
> +
> int gem_execbuf_flags_to_engine_class(unsigned int flags);
>
> #endif /* IGT_GT_H */
>
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [igt-dev] [RFC PATCH i-g-t] lib/i915: Generate engine names at runtime
2020-01-22 13:44 ` [igt-dev] [RFC PATCH i-g-t] " Tvrtko Ursulin
@ 2020-01-22 13:52 ` Petri Latvala
0 siblings, 0 replies; 10+ messages in thread
From: Petri Latvala @ 2020-01-22 13:52 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: igt-dev, Tvrtko Ursulin
On Wed, Jan 22, 2020 at 01:44:58PM +0000, Tvrtko Ursulin wrote:
>
> On 15/11/2019 12:14, Petri Latvala wrote:
> > The kernel supplies us an engine list with engine class and an
> > instance id. If the hardcoded engine list doesn't have the
> > class+instance we get, construct the engine name with printf instead
> > of calling additional engines "unknown".
> >
> > Signed-off-by: Petri Latvala <petri.latvala@intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Andi Shyti <andi.shyti@intel.com>
> > Cc: Katarzyna Dec <katarzyna.dec@intel.com>
> >
> > ---
> > lib/i915/gem_engine_topology.c | 27 +++++++++++++++++++--------
> > lib/i915/gem_engine_topology.h | 2 +-
> > lib/igt_gt.c | 8 ++++++++
> > lib/igt_gt.h | 7 ++++++-
> > 4 files changed, 34 insertions(+), 10 deletions(-)
> >
> > diff --git a/lib/i915/gem_engine_topology.c b/lib/i915/gem_engine_topology.c
> > index 790d455f..0707f237 100644
> > --- a/lib/i915/gem_engine_topology.c
> > +++ b/lib/i915/gem_engine_topology.c
> > @@ -103,6 +103,7 @@ static void init_engine(struct intel_execution_engine2 *e2,
> > const struct intel_execution_engine2 *__e2;
> > static const char *unknown_name = "unknown",
> > *virtual_name = "virtual";
> > + const struct intel_execution_engine_class *cls;
> > e2->class = class;
> > e2->instance = instance;
> > @@ -111,7 +112,7 @@ static void init_engine(struct intel_execution_engine2 *e2,
> > /* engine is a virtual engine */
> > if (class == I915_ENGINE_CLASS_INVALID &&
> > instance == I915_ENGINE_CLASS_INVALID_VIRTUAL) {
> > - e2->name = virtual_name;
> > + strncpy(e2->name, virtual_name, sizeof(e2->name));
> > e2->is_virtual = true;
> > return;
> > }
> > @@ -120,12 +121,22 @@ static void init_engine(struct intel_execution_engine2 *e2,
> > if (__e2->class == class && __e2->instance == instance)
> > break;
> > - if (__e2->name) {
> > - e2->name = __e2->name;
> > + if (__e2->name[0]) {
> > + strncpy(e2->name, __e2->name, sizeof(e2->name));
> > } else {
> > - igt_warn("found unknown engine (%d, %d)\n", class, instance);
> > - e2->name = unknown_name;
> > - e2->flags = -1;
> > + for (cls = intel_execution_engine_class_map; cls->name; cls++) {
> > + if (cls->class == class) {
> > + snprintf(e2->name, sizeof(e2->name), "%s%d", cls->name, instance);
> > + igt_info("unknown but supported engine %s found\n", e2->name);
> > + break;
> > + }
> > + }
> > +
> > + if (!e2->name[0]) {
> > + igt_info("found unknown engine (%d, %d)\n", class, instance);
> > + strncpy(e2->name, unknown_name, sizeof(e2->name));
> > + e2->flags = -1;
> > + }
> > }
> > /* just to remark it */
> > @@ -223,7 +234,7 @@ struct intel_engine_data intel_init_engine_list(int fd, uint32_t ctx_id)
> > struct intel_execution_engine2 *__e2 =
> > &engine_data.engines[engine_data.nengines];
> > - __e2->name = e2->name;
> > + strncpy(__e2->name, e2->name, sizeof(__e2->name));
> > __e2->instance = e2->instance;
> > __e2->class = e2->class;
> > __e2->flags = e2->flags;
> > @@ -302,7 +313,7 @@ struct intel_execution_engine2 gem_eb_flags_to_engine(unsigned int flags)
> > if (ring == I915_EXEC_DEFAULT) {
> > e2__.flags = I915_EXEC_DEFAULT;
> > - e2__.name = "default";
> > + strncpy(e2__.name, "default", sizeof(e2__.name));
> > } else {
> > const struct intel_execution_engine2 *e2;
> > diff --git a/lib/i915/gem_engine_topology.h b/lib/i915/gem_engine_topology.h
> > index d98773e0..525741cc 100644
> > --- a/lib/i915/gem_engine_topology.h
> > +++ b/lib/i915/gem_engine_topology.h
> > @@ -61,7 +61,7 @@ bool gem_engine_is_equal(const struct intel_execution_engine2 *e1,
> > struct intel_execution_engine2 gem_eb_flags_to_engine(unsigned int flags);
> > #define __for_each_static_engine(e__) \
> > - for ((e__) = intel_execution_engines2; (e__)->name; (e__)++)
> > + for ((e__) = intel_execution_engines2; (e__)->name[0]; (e__)++)
> > #define for_each_context_engine(fd__, ctx__, e__) \
> > for (struct intel_engine_data i__ = intel_init_engine_list(fd__, ctx__); \
> > diff --git a/lib/igt_gt.c b/lib/igt_gt.c
> > index 256c7cbc..25e6c455 100644
> > --- a/lib/igt_gt.c
> > +++ b/lib/igt_gt.c
> > @@ -596,6 +596,14 @@ const struct intel_execution_engine2 intel_execution_engines2[] = {
> > { }
> > };
> > +const struct intel_execution_engine_class intel_execution_engine_class_map[] = {
> > + { "rcs", I915_ENGINE_CLASS_RENDER },
> > + { "bcs", I915_ENGINE_CLASS_COPY },
> > + { "vcs", I915_ENGINE_CLASS_VIDEO },
> > + { "vecs", I915_ENGINE_CLASS_VIDEO_ENHANCE },
> > + { }
> > +};
> > +
> > int gem_execbuf_flags_to_engine_class(unsigned int flags)
> > {
> > switch (flags & 0x3f) {
> > diff --git a/lib/igt_gt.h b/lib/igt_gt.h
> > index 66088d39..0268031f 100644
> > --- a/lib/igt_gt.h
> > +++ b/lib/igt_gt.h
> > @@ -95,13 +95,18 @@ bool gem_can_store_dword(int fd, unsigned int engine);
> > bool gem_class_can_store_dword(int fd, int class);
> > extern const struct intel_execution_engine2 {
> > - const char *name;
> > + char name[10];
>
> You have the same bug as I do in
> https://patchwork.freedesktop.org/series/72384/ in that
> __for_each_static_engine has no engine names? :) And I have another bug that
> I missed to adjust the iterator macro. :)
See the chunk above that edits __for_each_static_engine to check
(e__)->name[0] instead of (e__)->name, I think that was missing in
your version.
> Otherwise similar implementations. Lets do one of the two and get it over
> with... You were here first so do you want to send an update including past
> and latest tweaks? Or if you are busy I'll tweak my version into submission.
The minor details like cleaner looking loops over the name table and
the way you did init_engine() makes me lean towards going with your
code. I'll be happy to review it.
--
Petri Latvala
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-01-22 13:52 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-11-15 12:14 [igt-dev] [RFC PATCH i-g-t] lib/i915: Generate engine names at runtime Petri Latvala
2019-11-15 12:30 ` [igt-dev] ✗ GitLab.Pipeline: warning for " Patchwork
2019-11-15 12:52 ` [igt-dev] ✓ Fi.CI.BAT: success " Patchwork
2019-11-15 14:15 ` [igt-dev] [RFC PATCH i-g-t] " Andi Shyti
2019-11-15 16:01 ` Tvrtko Ursulin
2019-11-18 10:05 ` Petri Latvala
2019-11-18 11:33 ` Tvrtko Ursulin
2019-11-16 18:29 ` [igt-dev] ✗ Fi.CI.IGT: failure for " Patchwork
2020-01-22 13:44 ` [igt-dev] [RFC PATCH i-g-t] " Tvrtko Ursulin
2020-01-22 13:52 ` Petri Latvala
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox