* [PATCH i-g-t] i915: Improve static engine map for legacy
@ 2019-05-23 8:06 Chris Wilson
2019-05-23 8:33 ` Andi Shyti
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Chris Wilson @ 2019-05-23 8:06 UTC (permalink / raw)
To: intel-gfx; +Cc: igt-dev
We need to keep igt working on linus and dif, or Joonas gets very upset.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Andi Shyti <andi.shyti@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
lib/i915/gem_context.c | 2 +-
lib/i915/gem_engine_topology.c | 15 +++-------
lib/igt_gt.c | 54 ++++------------------------------
lib/igt_gt.h | 17 -----------
tests/i915/gem_ctx_isolation.c | 21 +++++--------
tools/intel_reg.c | 4 +--
6 files changed, 19 insertions(+), 94 deletions(-)
diff --git a/lib/i915/gem_context.c b/lib/i915/gem_context.c
index f94d89cb4..07ab78174 100644
--- a/lib/i915/gem_context.c
+++ b/lib/i915/gem_context.c
@@ -290,7 +290,7 @@ bool gem_context_has_engine(int fd, uint32_t ctx, uint64_t engine)
* wouldn't produce any result.
*/
if ((engine & ~(3<<13)) == I915_EXEC_BSD) {
- if (engine & (3 << 13) && !gem_has_bsd2(fd))
+ if (engine & (2 << 13) && !gem_has_bsd2(fd))
return false;
}
diff --git a/lib/i915/gem_engine_topology.c b/lib/i915/gem_engine_topology.c
index d0c8bd5aa..fdd1b9516 100644
--- a/lib/i915/gem_engine_topology.c
+++ b/lib/i915/gem_engine_topology.c
@@ -223,22 +223,15 @@ 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];
- if (!igt_only_list_subtests()) {
- __e2->flags = gem_class_instance_to_eb_flags(fd,
- e2->class, e2->instance);
-
- if (!gem_has_ring(fd, __e2->flags))
- continue;
- } else {
- __e2->flags = -1; /* 0xfff... */
- }
-
__e2->name = e2->name;
__e2->instance = e2->instance;
__e2->class = e2->class;
+ __e2->flags = e2->flags;
__e2->is_virtual = false;
- engine_data.nengines++;
+ if (igt_only_list_subtests() ||
+ gem_has_ring(fd, e2->flags))
+ engine_data.nengines++;
}
return engine_data;
}
diff --git a/lib/igt_gt.c b/lib/igt_gt.c
index 6b7c037e6..78e3cd089 100644
--- a/lib/igt_gt.c
+++ b/lib/igt_gt.c
@@ -585,12 +585,12 @@ bool gem_can_store_dword(int fd, unsigned int engine)
}
const struct intel_execution_engine2 intel_execution_engines2[] = {
- { "rcs0", I915_ENGINE_CLASS_RENDER, 0 },
- { "bcs0", I915_ENGINE_CLASS_COPY, 0 },
- { "vcs0", I915_ENGINE_CLASS_VIDEO, 0 },
- { "vcs1", I915_ENGINE_CLASS_VIDEO, 1 },
- { "vcs2", I915_ENGINE_CLASS_VIDEO, 2 },
- { "vecs0", I915_ENGINE_CLASS_VIDEO_ENHANCE, 0 },
+ { "rcs0", I915_ENGINE_CLASS_RENDER, 0, I915_EXEC_RENDER },
+ { "bcs0", I915_ENGINE_CLASS_COPY, 0, I915_EXEC_BLT },
+ { "vcs0", I915_ENGINE_CLASS_VIDEO, 0, I915_EXEC_BSD | I915_EXEC_BSD_RING1 },
+ { "vcs1", I915_ENGINE_CLASS_VIDEO, 1, I915_EXEC_BSD | I915_EXEC_BSD_RING2 },
+ { "vcs2", I915_ENGINE_CLASS_VIDEO, 2, -1 },
+ { "vecs0", I915_ENGINE_CLASS_VIDEO_ENHANCE, 0, I915_EXEC_VEBOX },
{ }
};
@@ -611,48 +611,6 @@ int gem_execbuf_flags_to_engine_class(unsigned int flags)
}
}
-unsigned int
-gem_class_instance_to_eb_flags(int gem_fd,
- enum drm_i915_gem_engine_class class,
- unsigned int instance)
-{
- if (class != I915_ENGINE_CLASS_VIDEO)
- igt_assert(instance == 0);
- else
- igt_assert(instance >= 0 && instance <= 1);
-
- switch (class) {
- case I915_ENGINE_CLASS_RENDER:
- return I915_EXEC_RENDER;
- case I915_ENGINE_CLASS_COPY:
- return I915_EXEC_BLT;
- case I915_ENGINE_CLASS_VIDEO:
- if (instance == 0) {
- if (gem_has_bsd2(gem_fd))
- return I915_EXEC_BSD | I915_EXEC_BSD_RING1;
- else
- return I915_EXEC_BSD;
-
- } else {
- return I915_EXEC_BSD | I915_EXEC_BSD_RING2;
- }
- case I915_ENGINE_CLASS_VIDEO_ENHANCE:
- return I915_EXEC_VEBOX;
- case I915_ENGINE_CLASS_INVALID:
- default:
- igt_assert(0);
- };
-}
-
-bool gem_has_engine(int gem_fd,
- enum drm_i915_gem_engine_class class,
- unsigned int instance)
-{
- return gem_has_ring(gem_fd,
- gem_class_instance_to_eb_flags(gem_fd, class,
- instance));
-}
-
bool gem_ring_is_physical_engine(int fd, unsigned ring)
{
if (ring == I915_EXEC_DEFAULT)
diff --git a/lib/igt_gt.h b/lib/igt_gt.h
index 77318e2a8..73b5002a0 100644
--- a/lib/igt_gt.h
+++ b/lib/igt_gt.h
@@ -102,21 +102,4 @@ extern const struct intel_execution_engine2 {
int gem_execbuf_flags_to_engine_class(unsigned int flags);
-unsigned int
-gem_class_instance_to_eb_flags(int gem_fd,
- enum drm_i915_gem_engine_class class,
- unsigned int instance);
-
-bool gem_has_engine(int gem_fd,
- enum drm_i915_gem_engine_class class,
- unsigned int instance);
-
-static inline
-void gem_require_engine(int gem_fd,
- enum drm_i915_gem_engine_class class,
- unsigned int instance)
-{
- igt_require(gem_has_engine(gem_fd, class, instance));
-}
-
#endif /* IGT_GT_H */
diff --git a/tests/i915/gem_ctx_isolation.c b/tests/i915/gem_ctx_isolation.c
index bcd0f4812..5b054c81d 100644
--- a/tests/i915/gem_ctx_isolation.c
+++ b/tests/i915/gem_ctx_isolation.c
@@ -321,8 +321,7 @@ static uint32_t read_regs(int fd,
memset(&execbuf, 0, sizeof(execbuf));
execbuf.buffers_ptr = to_user_pointer(obj);
execbuf.buffer_count = 2;
- execbuf.flags =
- gem_class_instance_to_eb_flags(fd, e->class, e->instance);
+ execbuf.flags = e->flags;
execbuf.rsvd1 = ctx;
gem_execbuf(fd, &execbuf);
gem_close(fd, obj[1].handle);
@@ -377,8 +376,7 @@ static void write_regs(int fd,
memset(&execbuf, 0, sizeof(execbuf));
execbuf.buffers_ptr = to_user_pointer(&obj);
execbuf.buffer_count = 1;
- execbuf.flags =
- gem_class_instance_to_eb_flags(fd, e->class, e->instance);
+ execbuf.flags = e->flags;
execbuf.rsvd1 = ctx;
gem_execbuf(fd, &execbuf);
gem_close(fd, obj.handle);
@@ -448,8 +446,7 @@ static void restore_regs(int fd,
memset(&execbuf, 0, sizeof(execbuf));
execbuf.buffers_ptr = to_user_pointer(obj);
execbuf.buffer_count = 2;
- execbuf.flags =
- gem_class_instance_to_eb_flags(fd, e->class, e->instance);
+ execbuf.flags = e->flags;
execbuf.rsvd1 = ctx;
gem_execbuf(fd, &execbuf);
gem_close(fd, obj[1].handle);
@@ -559,8 +556,7 @@ static void nonpriv(int fd,
0x0505c0c0,
0xdeadbeef
};
- unsigned int engine =
- gem_class_instance_to_eb_flags(fd, e->class, e->instance);
+ unsigned int engine = e->flags;
unsigned int num_values = ARRAY_SIZE(values);
/* Sigh -- hsw: we need cmdparser access to our own registers! */
@@ -616,9 +612,7 @@ static void isolation(int fd,
0xaaaaaaaa,
0xdeadbeef
};
- unsigned int engine = gem_class_instance_to_eb_flags(fd,
- e->class,
- e->instance);
+ unsigned int engine = e->flags;
unsigned int num_values =
flags & (DIRTY1 | DIRTY2) ? ARRAY_SIZE(values) : 1;
@@ -729,8 +723,7 @@ static void preservation(int fd,
0xdeadbeef
};
const unsigned int num_values = ARRAY_SIZE(values);
- unsigned int engine =
- gem_class_instance_to_eb_flags(fd, e->class, e->instance);
+ unsigned int engine = e->flags;
uint32_t ctx[num_values +1 ];
uint32_t regs[num_values + 1][2];
igt_spin_t *spin;
@@ -840,7 +833,7 @@ igt_main
igt_subtest_group {
igt_fixture {
igt_require(has_context_isolation & (1 << e->class));
- gem_require_engine(fd, e->class, e->instance);
+ gem_require_ring(fd, e->flags);
igt_fork_hang_detector(fd);
}
diff --git a/tools/intel_reg.c b/tools/intel_reg.c
index 1247b70b0..e517956b8 100644
--- a/tools/intel_reg.c
+++ b/tools/intel_reg.c
@@ -329,9 +329,7 @@ static int register_srm(struct config *config, struct reg *reg,
memset(&execbuf, 0, sizeof(execbuf));
execbuf.buffers_ptr = to_user_pointer(obj);
execbuf.buffer_count = 2;
- execbuf.flags = gem_class_instance_to_eb_flags(fd,
- engine->class,
- engine->instance);
+ execbuf.flags = engine->flags;
if (secure)
execbuf.flags |= I915_EXEC_SECURE;
--
2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH i-g-t] i915: Improve static engine map for legacy
2019-05-23 8:06 [PATCH i-g-t] i915: Improve static engine map for legacy Chris Wilson
@ 2019-05-23 8:33 ` Andi Shyti
2019-05-23 8:46 ` Tvrtko Ursulin
2019-05-23 9:48 ` Tvrtko Ursulin
2 siblings, 0 replies; 10+ messages in thread
From: Andi Shyti @ 2019-05-23 8:33 UTC (permalink / raw)
To: Chris Wilson; +Cc: igt-dev, intel-gfx
Hi Chris,
On Thu, May 23, 2019 at 09:06:49AM +0100, Chris Wilson wrote:
> We need to keep igt working on linus and dif, or Joonas gets very upset.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Andi Shyti <andi.shyti@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
looks good.
Reviewed-by: Andi Shyti <andi.shyti@intel.com>
Thanks,
Andi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH i-g-t] i915: Improve static engine map for legacy
2019-05-23 8:06 [PATCH i-g-t] i915: Improve static engine map for legacy Chris Wilson
2019-05-23 8:33 ` Andi Shyti
@ 2019-05-23 8:46 ` Tvrtko Ursulin
2019-05-23 9:09 ` Chris Wilson
2019-05-23 9:48 ` Tvrtko Ursulin
2 siblings, 1 reply; 10+ messages in thread
From: Tvrtko Ursulin @ 2019-05-23 8:46 UTC (permalink / raw)
To: Chris Wilson, intel-gfx; +Cc: igt-dev
On 23/05/2019 09:06, Chris Wilson wrote:
> We need to keep igt working on linus and dif, or Joonas gets very upset.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Andi Shyti <andi.shyti@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
> lib/i915/gem_context.c | 2 +-
> lib/i915/gem_engine_topology.c | 15 +++-------
> lib/igt_gt.c | 54 ++++------------------------------
> lib/igt_gt.h | 17 -----------
> tests/i915/gem_ctx_isolation.c | 21 +++++--------
> tools/intel_reg.c | 4 +--
> 6 files changed, 19 insertions(+), 94 deletions(-)
>
> diff --git a/lib/i915/gem_context.c b/lib/i915/gem_context.c
> index f94d89cb4..07ab78174 100644
> --- a/lib/i915/gem_context.c
> +++ b/lib/i915/gem_context.c
> @@ -290,7 +290,7 @@ bool gem_context_has_engine(int fd, uint32_t ctx, uint64_t engine)
> * wouldn't produce any result.
> */
> if ((engine & ~(3<<13)) == I915_EXEC_BSD) {
> - if (engine & (3 << 13) && !gem_has_bsd2(fd))
> + if (engine & (2 << 13) && !gem_has_bsd2(fd))
> return false;
> }
>
> diff --git a/lib/i915/gem_engine_topology.c b/lib/i915/gem_engine_topology.c
> index d0c8bd5aa..fdd1b9516 100644
> --- a/lib/i915/gem_engine_topology.c
> +++ b/lib/i915/gem_engine_topology.c
> @@ -223,22 +223,15 @@ 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];
>
> - if (!igt_only_list_subtests()) {
> - __e2->flags = gem_class_instance_to_eb_flags(fd,
> - e2->class, e2->instance);
> -
> - if (!gem_has_ring(fd, __e2->flags))
> - continue;
> - } else {
> - __e2->flags = -1; /* 0xfff... */
> - }
> -
> __e2->name = e2->name;
> __e2->instance = e2->instance;
> __e2->class = e2->class;
> + __e2->flags = e2->flags;
> __e2->is_virtual = false;
>
> - engine_data.nengines++;
> + if (igt_only_list_subtests() ||
> + gem_has_ring(fd, e2->flags))
> + engine_data.nengines++;
> }
> return engine_data;
> }
> diff --git a/lib/igt_gt.c b/lib/igt_gt.c
> index 6b7c037e6..78e3cd089 100644
> --- a/lib/igt_gt.c
> +++ b/lib/igt_gt.c
> @@ -585,12 +585,12 @@ bool gem_can_store_dword(int fd, unsigned int engine)
> }
>
> const struct intel_execution_engine2 intel_execution_engines2[] = {
> - { "rcs0", I915_ENGINE_CLASS_RENDER, 0 },
> - { "bcs0", I915_ENGINE_CLASS_COPY, 0 },
> - { "vcs0", I915_ENGINE_CLASS_VIDEO, 0 },
> - { "vcs1", I915_ENGINE_CLASS_VIDEO, 1 },
> - { "vcs2", I915_ENGINE_CLASS_VIDEO, 2 },
> - { "vecs0", I915_ENGINE_CLASS_VIDEO_ENHANCE, 0 },
> + { "rcs0", I915_ENGINE_CLASS_RENDER, 0, I915_EXEC_RENDER },
> + { "bcs0", I915_ENGINE_CLASS_COPY, 0, I915_EXEC_BLT },
> + { "vcs0", I915_ENGINE_CLASS_VIDEO, 0, I915_EXEC_BSD | I915_EXEC_BSD_RING1 },
execbuf will reject this on single vcs parts. :( Am I not seeing some
place where you fudge it into compliance?
Regards,
Tvrtko
> + { "vcs1", I915_ENGINE_CLASS_VIDEO, 1, I915_EXEC_BSD | I915_EXEC_BSD_RING2 },
> + { "vcs2", I915_ENGINE_CLASS_VIDEO, 2, -1 },
> + { "vecs0", I915_ENGINE_CLASS_VIDEO_ENHANCE, 0, I915_EXEC_VEBOX },
> { }
> };
>
> @@ -611,48 +611,6 @@ int gem_execbuf_flags_to_engine_class(unsigned int flags)
> }
> }
>
> -unsigned int
> -gem_class_instance_to_eb_flags(int gem_fd,
> - enum drm_i915_gem_engine_class class,
> - unsigned int instance)
> -{
> - if (class != I915_ENGINE_CLASS_VIDEO)
> - igt_assert(instance == 0);
> - else
> - igt_assert(instance >= 0 && instance <= 1);
> -
> - switch (class) {
> - case I915_ENGINE_CLASS_RENDER:
> - return I915_EXEC_RENDER;
> - case I915_ENGINE_CLASS_COPY:
> - return I915_EXEC_BLT;
> - case I915_ENGINE_CLASS_VIDEO:
> - if (instance == 0) {
> - if (gem_has_bsd2(gem_fd))
> - return I915_EXEC_BSD | I915_EXEC_BSD_RING1;
> - else
> - return I915_EXEC_BSD;
> -
> - } else {
> - return I915_EXEC_BSD | I915_EXEC_BSD_RING2;
> - }
> - case I915_ENGINE_CLASS_VIDEO_ENHANCE:
> - return I915_EXEC_VEBOX;
> - case I915_ENGINE_CLASS_INVALID:
> - default:
> - igt_assert(0);
> - };
> -}
> -
> -bool gem_has_engine(int gem_fd,
> - enum drm_i915_gem_engine_class class,
> - unsigned int instance)
> -{
> - return gem_has_ring(gem_fd,
> - gem_class_instance_to_eb_flags(gem_fd, class,
> - instance));
> -}
> -
> bool gem_ring_is_physical_engine(int fd, unsigned ring)
> {
> if (ring == I915_EXEC_DEFAULT)
> diff --git a/lib/igt_gt.h b/lib/igt_gt.h
> index 77318e2a8..73b5002a0 100644
> --- a/lib/igt_gt.h
> +++ b/lib/igt_gt.h
> @@ -102,21 +102,4 @@ extern const struct intel_execution_engine2 {
>
> int gem_execbuf_flags_to_engine_class(unsigned int flags);
>
> -unsigned int
> -gem_class_instance_to_eb_flags(int gem_fd,
> - enum drm_i915_gem_engine_class class,
> - unsigned int instance);
> -
> -bool gem_has_engine(int gem_fd,
> - enum drm_i915_gem_engine_class class,
> - unsigned int instance);
> -
> -static inline
> -void gem_require_engine(int gem_fd,
> - enum drm_i915_gem_engine_class class,
> - unsigned int instance)
> -{
> - igt_require(gem_has_engine(gem_fd, class, instance));
> -}
> -
> #endif /* IGT_GT_H */
> diff --git a/tests/i915/gem_ctx_isolation.c b/tests/i915/gem_ctx_isolation.c
> index bcd0f4812..5b054c81d 100644
> --- a/tests/i915/gem_ctx_isolation.c
> +++ b/tests/i915/gem_ctx_isolation.c
> @@ -321,8 +321,7 @@ static uint32_t read_regs(int fd,
> memset(&execbuf, 0, sizeof(execbuf));
> execbuf.buffers_ptr = to_user_pointer(obj);
> execbuf.buffer_count = 2;
> - execbuf.flags =
> - gem_class_instance_to_eb_flags(fd, e->class, e->instance);
> + execbuf.flags = e->flags;
> execbuf.rsvd1 = ctx;
> gem_execbuf(fd, &execbuf);
> gem_close(fd, obj[1].handle);
> @@ -377,8 +376,7 @@ static void write_regs(int fd,
> memset(&execbuf, 0, sizeof(execbuf));
> execbuf.buffers_ptr = to_user_pointer(&obj);
> execbuf.buffer_count = 1;
> - execbuf.flags =
> - gem_class_instance_to_eb_flags(fd, e->class, e->instance);
> + execbuf.flags = e->flags;
> execbuf.rsvd1 = ctx;
> gem_execbuf(fd, &execbuf);
> gem_close(fd, obj.handle);
> @@ -448,8 +446,7 @@ static void restore_regs(int fd,
> memset(&execbuf, 0, sizeof(execbuf));
> execbuf.buffers_ptr = to_user_pointer(obj);
> execbuf.buffer_count = 2;
> - execbuf.flags =
> - gem_class_instance_to_eb_flags(fd, e->class, e->instance);
> + execbuf.flags = e->flags;
> execbuf.rsvd1 = ctx;
> gem_execbuf(fd, &execbuf);
> gem_close(fd, obj[1].handle);
> @@ -559,8 +556,7 @@ static void nonpriv(int fd,
> 0x0505c0c0,
> 0xdeadbeef
> };
> - unsigned int engine =
> - gem_class_instance_to_eb_flags(fd, e->class, e->instance);
> + unsigned int engine = e->flags;
> unsigned int num_values = ARRAY_SIZE(values);
>
> /* Sigh -- hsw: we need cmdparser access to our own registers! */
> @@ -616,9 +612,7 @@ static void isolation(int fd,
> 0xaaaaaaaa,
> 0xdeadbeef
> };
> - unsigned int engine = gem_class_instance_to_eb_flags(fd,
> - e->class,
> - e->instance);
> + unsigned int engine = e->flags;
> unsigned int num_values =
> flags & (DIRTY1 | DIRTY2) ? ARRAY_SIZE(values) : 1;
>
> @@ -729,8 +723,7 @@ static void preservation(int fd,
> 0xdeadbeef
> };
> const unsigned int num_values = ARRAY_SIZE(values);
> - unsigned int engine =
> - gem_class_instance_to_eb_flags(fd, e->class, e->instance);
> + unsigned int engine = e->flags;
> uint32_t ctx[num_values +1 ];
> uint32_t regs[num_values + 1][2];
> igt_spin_t *spin;
> @@ -840,7 +833,7 @@ igt_main
> igt_subtest_group {
> igt_fixture {
> igt_require(has_context_isolation & (1 << e->class));
> - gem_require_engine(fd, e->class, e->instance);
> + gem_require_ring(fd, e->flags);
> igt_fork_hang_detector(fd);
> }
>
> diff --git a/tools/intel_reg.c b/tools/intel_reg.c
> index 1247b70b0..e517956b8 100644
> --- a/tools/intel_reg.c
> +++ b/tools/intel_reg.c
> @@ -329,9 +329,7 @@ static int register_srm(struct config *config, struct reg *reg,
> memset(&execbuf, 0, sizeof(execbuf));
> execbuf.buffers_ptr = to_user_pointer(obj);
> execbuf.buffer_count = 2;
> - execbuf.flags = gem_class_instance_to_eb_flags(fd,
> - engine->class,
> - engine->instance);
> + execbuf.flags = engine->flags;
> if (secure)
> execbuf.flags |= I915_EXEC_SECURE;
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH i-g-t] i915: Improve static engine map for legacy
2019-05-23 8:46 ` Tvrtko Ursulin
@ 2019-05-23 9:09 ` Chris Wilson
2019-05-23 9:14 ` Chris Wilson
2019-05-23 9:27 ` Tvrtko Ursulin
0 siblings, 2 replies; 10+ messages in thread
From: Chris Wilson @ 2019-05-23 9:09 UTC (permalink / raw)
To: Tvrtko Ursulin, intel-gfx; +Cc: igt-dev
Quoting Tvrtko Ursulin (2019-05-23 09:46:08)
>
> On 23/05/2019 09:06, Chris Wilson wrote:
> > const struct intel_execution_engine2 intel_execution_engines2[] = {
> > - { "rcs0", I915_ENGINE_CLASS_RENDER, 0 },
> > - { "bcs0", I915_ENGINE_CLASS_COPY, 0 },
> > - { "vcs0", I915_ENGINE_CLASS_VIDEO, 0 },
> > - { "vcs1", I915_ENGINE_CLASS_VIDEO, 1 },
> > - { "vcs2", I915_ENGINE_CLASS_VIDEO, 2 },
> > - { "vecs0", I915_ENGINE_CLASS_VIDEO_ENHANCE, 0 },
> > + { "rcs0", I915_ENGINE_CLASS_RENDER, 0, I915_EXEC_RENDER },
> > + { "bcs0", I915_ENGINE_CLASS_COPY, 0, I915_EXEC_BLT },
> > + { "vcs0", I915_ENGINE_CLASS_VIDEO, 0, I915_EXEC_BSD | I915_EXEC_BSD_RING1 },
>
> execbuf will reject this on single vcs parts. :( Am I not seeing some
> place where you fudge it into compliance?
if (user_ring_id == I915_EXEC_BSD && HAS_ENGINE(dev_priv, VCS1)) {
unsigned int bsd_idx = args->flags & I915_EXEC_BSD_MASK;
if (bsd_idx == I915_EXEC_BSD_DEFAULT) {
bsd_idx = gen8_dispatch_bsd_engine(dev_priv, file);
} else if (bsd_idx >= I915_EXEC_BSD_RING1 &&
bsd_idx <= I915_EXEC_BSD_RING2) {
bsd_idx >>= I915_EXEC_BSD_SHIFT;
bsd_idx--;
} else {
DRM_DEBUG("execbuf with unknown bsd ring: %u\n",
bsd_idx);
return NULL;
}
engine = dev_priv->engine[_VCS(bsd_idx)];
} else {
engine = dev_priv->engine[user_ring_map[user_ring_id]];
}
Looks ok to me... I was trying to double check but dif didn't boot on
bsw. Sigh.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH i-g-t] i915: Improve static engine map for legacy
2019-05-23 9:09 ` Chris Wilson
@ 2019-05-23 9:14 ` Chris Wilson
2019-05-23 9:35 ` Tvrtko Ursulin
2019-05-23 9:27 ` Tvrtko Ursulin
1 sibling, 1 reply; 10+ messages in thread
From: Chris Wilson @ 2019-05-23 9:14 UTC (permalink / raw)
To: Tvrtko Ursulin, intel-gfx; +Cc: igt-dev
Quoting Chris Wilson (2019-05-23 10:09:48)
> Quoting Tvrtko Ursulin (2019-05-23 09:46:08)
> >
> > On 23/05/2019 09:06, Chris Wilson wrote:
> > > const struct intel_execution_engine2 intel_execution_engines2[] = {
> > > - { "rcs0", I915_ENGINE_CLASS_RENDER, 0 },
> > > - { "bcs0", I915_ENGINE_CLASS_COPY, 0 },
> > > - { "vcs0", I915_ENGINE_CLASS_VIDEO, 0 },
> > > - { "vcs1", I915_ENGINE_CLASS_VIDEO, 1 },
> > > - { "vcs2", I915_ENGINE_CLASS_VIDEO, 2 },
> > > - { "vecs0", I915_ENGINE_CLASS_VIDEO_ENHANCE, 0 },
> > > + { "rcs0", I915_ENGINE_CLASS_RENDER, 0, I915_EXEC_RENDER },
> > > + { "bcs0", I915_ENGINE_CLASS_COPY, 0, I915_EXEC_BLT },
> > > + { "vcs0", I915_ENGINE_CLASS_VIDEO, 0, I915_EXEC_BSD | I915_EXEC_BSD_RING1 },
> >
> > execbuf will reject this on single vcs parts. :( Am I not seeing some
> > place where you fudge it into compliance?
>
>
> if (user_ring_id == I915_EXEC_BSD && HAS_ENGINE(dev_priv, VCS1)) {
> unsigned int bsd_idx = args->flags & I915_EXEC_BSD_MASK;
>
> if (bsd_idx == I915_EXEC_BSD_DEFAULT) {
> bsd_idx = gen8_dispatch_bsd_engine(dev_priv, file);
> } else if (bsd_idx >= I915_EXEC_BSD_RING1 &&
> bsd_idx <= I915_EXEC_BSD_RING2) {
> bsd_idx >>= I915_EXEC_BSD_SHIFT;
> bsd_idx--;
> } else {
> DRM_DEBUG("execbuf with unknown bsd ring: %u\n",
> bsd_idx);
> return NULL;
> }
>
> engine = dev_priv->engine[_VCS(bsd_idx)];
> } else {
> engine = dev_priv->engine[user_ring_map[user_ring_id]];
> }
>
> Looks ok to me... I was trying to double check but dif didn't boot on
> bsw. Sigh.
So instead I removed vcs1 from bdw, and vcs0 continues to work.
What is still incorrect though:
$ ./build/tests/gem_exec_parallel --list-subtests
...
vcs0
vcs0-contexts
vcs0-fds
vcs1
vcs1-contexts
vcs1-fds
vcs2
vcs2-contexts
vcs2-fds
...
$ sudo ./build/tests/gem_exec_parallel --run vcs1
IGT-Version: 1.23-g82137ba4 (x86_64) (Linux: 5.2.0-rc1+ x86_64)
(gem_exec_parallel:16219) igt_core-WARNING: Unknown subtest: vcs1
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH i-g-t] i915: Improve static engine map for legacy
2019-05-23 9:09 ` Chris Wilson
2019-05-23 9:14 ` Chris Wilson
@ 2019-05-23 9:27 ` Tvrtko Ursulin
1 sibling, 0 replies; 10+ messages in thread
From: Tvrtko Ursulin @ 2019-05-23 9:27 UTC (permalink / raw)
To: Chris Wilson, intel-gfx; +Cc: igt-dev
On 23/05/2019 10:09, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-05-23 09:46:08)
>>
>> On 23/05/2019 09:06, Chris Wilson wrote:
>>> const struct intel_execution_engine2 intel_execution_engines2[] = {
>>> - { "rcs0", I915_ENGINE_CLASS_RENDER, 0 },
>>> - { "bcs0", I915_ENGINE_CLASS_COPY, 0 },
>>> - { "vcs0", I915_ENGINE_CLASS_VIDEO, 0 },
>>> - { "vcs1", I915_ENGINE_CLASS_VIDEO, 1 },
>>> - { "vcs2", I915_ENGINE_CLASS_VIDEO, 2 },
>>> - { "vecs0", I915_ENGINE_CLASS_VIDEO_ENHANCE, 0 },
>>> + { "rcs0", I915_ENGINE_CLASS_RENDER, 0, I915_EXEC_RENDER },
>>> + { "bcs0", I915_ENGINE_CLASS_COPY, 0, I915_EXEC_BLT },
>>> + { "vcs0", I915_ENGINE_CLASS_VIDEO, 0, I915_EXEC_BSD | I915_EXEC_BSD_RING1 },
>>
>> execbuf will reject this on single vcs parts. :( Am I not seeing some
>> place where you fudge it into compliance?
>
>
> if (user_ring_id == I915_EXEC_BSD && HAS_ENGINE(dev_priv, VCS1)) {
> unsigned int bsd_idx = args->flags & I915_EXEC_BSD_MASK;
>
> if (bsd_idx == I915_EXEC_BSD_DEFAULT) {
> bsd_idx = gen8_dispatch_bsd_engine(dev_priv, file);
> } else if (bsd_idx >= I915_EXEC_BSD_RING1 &&
> bsd_idx <= I915_EXEC_BSD_RING2) {
> bsd_idx >>= I915_EXEC_BSD_SHIFT;
> bsd_idx--;
> } else {
> DRM_DEBUG("execbuf with unknown bsd ring: %u\n",
> bsd_idx);
> return NULL;
> }
>
> engine = dev_priv->engine[_VCS(bsd_idx)];
> } else {
> engine = dev_priv->engine[user_ring_map[user_ring_id]];
> }
>
> Looks ok to me... I was trying to double check but dif didn't boot on
> bsw. Sigh.
Hmm I would have sworn we used to do that.. strange. Okay, I'll re-read
your patch. Notice that I did not know you are working on it so have
sent my own fix at similar time.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH i-g-t] i915: Improve static engine map for legacy
2019-05-23 9:14 ` Chris Wilson
@ 2019-05-23 9:35 ` Tvrtko Ursulin
2019-05-23 9:46 ` Chris Wilson
0 siblings, 1 reply; 10+ messages in thread
From: Tvrtko Ursulin @ 2019-05-23 9:35 UTC (permalink / raw)
To: Chris Wilson, intel-gfx; +Cc: igt-dev
On 23/05/2019 10:14, Chris Wilson wrote:
> Quoting Chris Wilson (2019-05-23 10:09:48)
>> Quoting Tvrtko Ursulin (2019-05-23 09:46:08)
>>>
>>> On 23/05/2019 09:06, Chris Wilson wrote:
>>>> const struct intel_execution_engine2 intel_execution_engines2[] = {
>>>> - { "rcs0", I915_ENGINE_CLASS_RENDER, 0 },
>>>> - { "bcs0", I915_ENGINE_CLASS_COPY, 0 },
>>>> - { "vcs0", I915_ENGINE_CLASS_VIDEO, 0 },
>>>> - { "vcs1", I915_ENGINE_CLASS_VIDEO, 1 },
>>>> - { "vcs2", I915_ENGINE_CLASS_VIDEO, 2 },
>>>> - { "vecs0", I915_ENGINE_CLASS_VIDEO_ENHANCE, 0 },
>>>> + { "rcs0", I915_ENGINE_CLASS_RENDER, 0, I915_EXEC_RENDER },
>>>> + { "bcs0", I915_ENGINE_CLASS_COPY, 0, I915_EXEC_BLT },
>>>> + { "vcs0", I915_ENGINE_CLASS_VIDEO, 0, I915_EXEC_BSD | I915_EXEC_BSD_RING1 },
>>>
>>> execbuf will reject this on single vcs parts. :( Am I not seeing some
>>> place where you fudge it into compliance?
>>
>>
>> if (user_ring_id == I915_EXEC_BSD && HAS_ENGINE(dev_priv, VCS1)) {
>> unsigned int bsd_idx = args->flags & I915_EXEC_BSD_MASK;
>>
>> if (bsd_idx == I915_EXEC_BSD_DEFAULT) {
>> bsd_idx = gen8_dispatch_bsd_engine(dev_priv, file);
>> } else if (bsd_idx >= I915_EXEC_BSD_RING1 &&
>> bsd_idx <= I915_EXEC_BSD_RING2) {
>> bsd_idx >>= I915_EXEC_BSD_SHIFT;
>> bsd_idx--;
>> } else {
>> DRM_DEBUG("execbuf with unknown bsd ring: %u\n",
>> bsd_idx);
>> return NULL;
>> }
>>
>> engine = dev_priv->engine[_VCS(bsd_idx)];
>> } else {
>> engine = dev_priv->engine[user_ring_map[user_ring_id]];
>> }
>>
>> Looks ok to me... I was trying to double check but dif didn't boot on
>> bsw. Sigh.
>
> So instead I removed vcs1 from bdw, and vcs0 continues to work.
>
> What is still incorrect though:
> $ ./build/tests/gem_exec_parallel --list-subtests
> ...
> vcs0
> vcs0-contexts
> vcs0-fds
> vcs1
> vcs1-contexts
> vcs1-fds
> vcs2
> vcs2-contexts
> vcs2-fds
> ...
As you know this part is correct since CI mandates we list subtests not
applicable to running platform.
> $ sudo ./build/tests/gem_exec_parallel --run vcs1
> IGT-Version: 1.23-g82137ba4 (x86_64) (Linux: 5.2.0-rc1+ x86_64)
> (gem_exec_parallel:16219) igt_core-WARNING: Unknown subtest: vcs1
I guess we need to make the library function igt_skip instead of not
enumerating the impossible and missing engines.
But it doesn't work.. skips need to be in either subtests or fixtures.
Perhaps the idea of using a single context aware iterator has therefore
fail and we do need two separate ones. :( One just for subtest
enumeration which *only* uses the static list, and another for use in
tests, which skips missing/impossible engines depending on context.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH i-g-t] i915: Improve static engine map for legacy
2019-05-23 9:35 ` Tvrtko Ursulin
@ 2019-05-23 9:46 ` Chris Wilson
0 siblings, 0 replies; 10+ messages in thread
From: Chris Wilson @ 2019-05-23 9:46 UTC (permalink / raw)
To: Tvrtko Ursulin, intel-gfx; +Cc: igt-dev
Quoting Tvrtko Ursulin (2019-05-23 10:35:01)
>
> On 23/05/2019 10:14, Chris Wilson wrote:
> > Quoting Chris Wilson (2019-05-23 10:09:48)
> >> Quoting Tvrtko Ursulin (2019-05-23 09:46:08)
> >>>
> >>> On 23/05/2019 09:06, Chris Wilson wrote:
> >>>> const struct intel_execution_engine2 intel_execution_engines2[] = {
> >>>> - { "rcs0", I915_ENGINE_CLASS_RENDER, 0 },
> >>>> - { "bcs0", I915_ENGINE_CLASS_COPY, 0 },
> >>>> - { "vcs0", I915_ENGINE_CLASS_VIDEO, 0 },
> >>>> - { "vcs1", I915_ENGINE_CLASS_VIDEO, 1 },
> >>>> - { "vcs2", I915_ENGINE_CLASS_VIDEO, 2 },
> >>>> - { "vecs0", I915_ENGINE_CLASS_VIDEO_ENHANCE, 0 },
> >>>> + { "rcs0", I915_ENGINE_CLASS_RENDER, 0, I915_EXEC_RENDER },
> >>>> + { "bcs0", I915_ENGINE_CLASS_COPY, 0, I915_EXEC_BLT },
> >>>> + { "vcs0", I915_ENGINE_CLASS_VIDEO, 0, I915_EXEC_BSD | I915_EXEC_BSD_RING1 },
> >>>
> >>> execbuf will reject this on single vcs parts. :( Am I not seeing some
> >>> place where you fudge it into compliance?
> >>
> >>
> >> if (user_ring_id == I915_EXEC_BSD && HAS_ENGINE(dev_priv, VCS1)) {
> >> unsigned int bsd_idx = args->flags & I915_EXEC_BSD_MASK;
> >>
> >> if (bsd_idx == I915_EXEC_BSD_DEFAULT) {
> >> bsd_idx = gen8_dispatch_bsd_engine(dev_priv, file);
> >> } else if (bsd_idx >= I915_EXEC_BSD_RING1 &&
> >> bsd_idx <= I915_EXEC_BSD_RING2) {
> >> bsd_idx >>= I915_EXEC_BSD_SHIFT;
> >> bsd_idx--;
> >> } else {
> >> DRM_DEBUG("execbuf with unknown bsd ring: %u\n",
> >> bsd_idx);
> >> return NULL;
> >> }
> >>
> >> engine = dev_priv->engine[_VCS(bsd_idx)];
> >> } else {
> >> engine = dev_priv->engine[user_ring_map[user_ring_id]];
> >> }
> >>
> >> Looks ok to me... I was trying to double check but dif didn't boot on
> >> bsw. Sigh.
> >
> > So instead I removed vcs1 from bdw, and vcs0 continues to work.
> >
> > What is still incorrect though:
> > $ ./build/tests/gem_exec_parallel --list-subtests
> > ...
> > vcs0
> > vcs0-contexts
> > vcs0-fds
> > vcs1
> > vcs1-contexts
> > vcs1-fds
> > vcs2
> > vcs2-contexts
> > vcs2-fds
> > ...
>
> As you know this part is correct since CI mandates we list subtests not
> applicable to running platform.
>
> > $ sudo ./build/tests/gem_exec_parallel --run vcs1
> > IGT-Version: 1.23-g82137ba4 (x86_64) (Linux: 5.2.0-rc1+ x86_64)
> > (gem_exec_parallel:16219) igt_core-WARNING: Unknown subtest: vcs1
>
> I guess we need to make the library function igt_skip instead of not
> enumerating the impossible and missing engines.
>
> But it doesn't work.. skips need to be in either subtests or fixtures.
>
> Perhaps the idea of using a single context aware iterator has therefore
> fail and we do need two separate ones. :( One just for subtest
> enumeration which *only* uses the static list, and another for use in
> tests, which skips missing/impossible engines depending on context.
I'm willing to sweep this bit under the carpet so long as it isn't
flagged as a failure to Joonas.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH i-g-t] i915: Improve static engine map for legacy
2019-05-23 8:06 [PATCH i-g-t] i915: Improve static engine map for legacy Chris Wilson
2019-05-23 8:33 ` Andi Shyti
2019-05-23 8:46 ` Tvrtko Ursulin
@ 2019-05-23 9:48 ` Tvrtko Ursulin
2019-05-23 11:23 ` Chris Wilson
2 siblings, 1 reply; 10+ messages in thread
From: Tvrtko Ursulin @ 2019-05-23 9:48 UTC (permalink / raw)
To: Chris Wilson, intel-gfx; +Cc: igt-dev
On 23/05/2019 09:06, Chris Wilson wrote:
> We need to keep igt working on linus and dif, or Joonas gets very upset.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Andi Shyti <andi.shyti@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
> lib/i915/gem_context.c | 2 +-
> lib/i915/gem_engine_topology.c | 15 +++-------
> lib/igt_gt.c | 54 ++++------------------------------
> lib/igt_gt.h | 17 -----------
> tests/i915/gem_ctx_isolation.c | 21 +++++--------
> tools/intel_reg.c | 4 +--
> 6 files changed, 19 insertions(+), 94 deletions(-)
>
> diff --git a/lib/i915/gem_context.c b/lib/i915/gem_context.c
> index f94d89cb4..07ab78174 100644
> --- a/lib/i915/gem_context.c
> +++ b/lib/i915/gem_context.c
> @@ -290,7 +290,7 @@ bool gem_context_has_engine(int fd, uint32_t ctx, uint64_t engine)
> * wouldn't produce any result.
> */
> if ((engine & ~(3<<13)) == I915_EXEC_BSD) {
> - if (engine & (3 << 13) && !gem_has_bsd2(fd))
> + if (engine & (2 << 13) && !gem_has_bsd2(fd))
> return false;
> }
>
> diff --git a/lib/i915/gem_engine_topology.c b/lib/i915/gem_engine_topology.c
> index d0c8bd5aa..fdd1b9516 100644
> --- a/lib/i915/gem_engine_topology.c
> +++ b/lib/i915/gem_engine_topology.c
> @@ -223,22 +223,15 @@ 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];
>
> - if (!igt_only_list_subtests()) {
> - __e2->flags = gem_class_instance_to_eb_flags(fd,
> - e2->class, e2->instance);
> -
> - if (!gem_has_ring(fd, __e2->flags))
> - continue;
> - } else {
> - __e2->flags = -1; /* 0xfff... */
> - }
> -
> __e2->name = e2->name;
> __e2->instance = e2->instance;
> __e2->class = e2->class;
> + __e2->flags = e2->flags;
> __e2->is_virtual = false;
>
> - engine_data.nengines++;
> + if (igt_only_list_subtests() ||
> + gem_has_ring(fd, e2->flags))
> + engine_data.nengines++;
> }
> return engine_data;
> }
> diff --git a/lib/igt_gt.c b/lib/igt_gt.c
> index 6b7c037e6..78e3cd089 100644
> --- a/lib/igt_gt.c
> +++ b/lib/igt_gt.c
> @@ -585,12 +585,12 @@ bool gem_can_store_dword(int fd, unsigned int engine)
> }
>
> const struct intel_execution_engine2 intel_execution_engines2[] = {
> - { "rcs0", I915_ENGINE_CLASS_RENDER, 0 },
> - { "bcs0", I915_ENGINE_CLASS_COPY, 0 },
> - { "vcs0", I915_ENGINE_CLASS_VIDEO, 0 },
> - { "vcs1", I915_ENGINE_CLASS_VIDEO, 1 },
> - { "vcs2", I915_ENGINE_CLASS_VIDEO, 2 },
> - { "vecs0", I915_ENGINE_CLASS_VIDEO_ENHANCE, 0 },
> + { "rcs0", I915_ENGINE_CLASS_RENDER, 0, I915_EXEC_RENDER },
> + { "bcs0", I915_ENGINE_CLASS_COPY, 0, I915_EXEC_BLT },
> + { "vcs0", I915_ENGINE_CLASS_VIDEO, 0, I915_EXEC_BSD | I915_EXEC_BSD_RING1 },
> + { "vcs1", I915_ENGINE_CLASS_VIDEO, 1, I915_EXEC_BSD | I915_EXEC_BSD_RING2 },
> + { "vcs2", I915_ENGINE_CLASS_VIDEO, 2, -1 },
> + { "vecs0", I915_ENGINE_CLASS_VIDEO_ENHANCE, 0, I915_EXEC_VEBOX },
> { }
> };
>
> @@ -611,48 +611,6 @@ int gem_execbuf_flags_to_engine_class(unsigned int flags)
> }
> }
>
> -unsigned int
> -gem_class_instance_to_eb_flags(int gem_fd,
> - enum drm_i915_gem_engine_class class,
> - unsigned int instance)
> -{
> - if (class != I915_ENGINE_CLASS_VIDEO)
> - igt_assert(instance == 0);
> - else
> - igt_assert(instance >= 0 && instance <= 1);
> -
> - switch (class) {
> - case I915_ENGINE_CLASS_RENDER:
> - return I915_EXEC_RENDER;
> - case I915_ENGINE_CLASS_COPY:
> - return I915_EXEC_BLT;
> - case I915_ENGINE_CLASS_VIDEO:
> - if (instance == 0) {
> - if (gem_has_bsd2(gem_fd))
> - return I915_EXEC_BSD | I915_EXEC_BSD_RING1;
> - else
> - return I915_EXEC_BSD;
> -
> - } else {
> - return I915_EXEC_BSD | I915_EXEC_BSD_RING2;
> - }
> - case I915_ENGINE_CLASS_VIDEO_ENHANCE:
> - return I915_EXEC_VEBOX;
> - case I915_ENGINE_CLASS_INVALID:
> - default:
> - igt_assert(0);
> - };
> -}
> -
> -bool gem_has_engine(int gem_fd,
> - enum drm_i915_gem_engine_class class,
> - unsigned int instance)
> -{
> - return gem_has_ring(gem_fd,
> - gem_class_instance_to_eb_flags(gem_fd, class,
> - instance));
> -}
> -
> bool gem_ring_is_physical_engine(int fd, unsigned ring)
> {
> if (ring == I915_EXEC_DEFAULT)
> diff --git a/lib/igt_gt.h b/lib/igt_gt.h
> index 77318e2a8..73b5002a0 100644
> --- a/lib/igt_gt.h
> +++ b/lib/igt_gt.h
> @@ -102,21 +102,4 @@ extern const struct intel_execution_engine2 {
>
> int gem_execbuf_flags_to_engine_class(unsigned int flags);
>
> -unsigned int
> -gem_class_instance_to_eb_flags(int gem_fd,
> - enum drm_i915_gem_engine_class class,
> - unsigned int instance);
> -
> -bool gem_has_engine(int gem_fd,
> - enum drm_i915_gem_engine_class class,
> - unsigned int instance);
> -
> -static inline
> -void gem_require_engine(int gem_fd,
> - enum drm_i915_gem_engine_class class,
> - unsigned int instance)
> -{
> - igt_require(gem_has_engine(gem_fd, class, instance));
> -}
> -
> #endif /* IGT_GT_H */
> diff --git a/tests/i915/gem_ctx_isolation.c b/tests/i915/gem_ctx_isolation.c
> index bcd0f4812..5b054c81d 100644
> --- a/tests/i915/gem_ctx_isolation.c
> +++ b/tests/i915/gem_ctx_isolation.c
> @@ -321,8 +321,7 @@ static uint32_t read_regs(int fd,
> memset(&execbuf, 0, sizeof(execbuf));
> execbuf.buffers_ptr = to_user_pointer(obj);
> execbuf.buffer_count = 2;
> - execbuf.flags =
> - gem_class_instance_to_eb_flags(fd, e->class, e->instance);
> + execbuf.flags = e->flags;
> execbuf.rsvd1 = ctx;
> gem_execbuf(fd, &execbuf);
> gem_close(fd, obj[1].handle);
> @@ -377,8 +376,7 @@ static void write_regs(int fd,
> memset(&execbuf, 0, sizeof(execbuf));
> execbuf.buffers_ptr = to_user_pointer(&obj);
> execbuf.buffer_count = 1;
> - execbuf.flags =
> - gem_class_instance_to_eb_flags(fd, e->class, e->instance);
> + execbuf.flags = e->flags;
> execbuf.rsvd1 = ctx;
> gem_execbuf(fd, &execbuf);
> gem_close(fd, obj.handle);
> @@ -448,8 +446,7 @@ static void restore_regs(int fd,
> memset(&execbuf, 0, sizeof(execbuf));
> execbuf.buffers_ptr = to_user_pointer(obj);
> execbuf.buffer_count = 2;
> - execbuf.flags =
> - gem_class_instance_to_eb_flags(fd, e->class, e->instance);
> + execbuf.flags = e->flags;
> execbuf.rsvd1 = ctx;
> gem_execbuf(fd, &execbuf);
> gem_close(fd, obj[1].handle);
> @@ -559,8 +556,7 @@ static void nonpriv(int fd,
> 0x0505c0c0,
> 0xdeadbeef
> };
> - unsigned int engine =
> - gem_class_instance_to_eb_flags(fd, e->class, e->instance);
> + unsigned int engine = e->flags;
> unsigned int num_values = ARRAY_SIZE(values);
>
> /* Sigh -- hsw: we need cmdparser access to our own registers! */
> @@ -616,9 +612,7 @@ static void isolation(int fd,
> 0xaaaaaaaa,
> 0xdeadbeef
> };
> - unsigned int engine = gem_class_instance_to_eb_flags(fd,
> - e->class,
> - e->instance);
> + unsigned int engine = e->flags;
> unsigned int num_values =
> flags & (DIRTY1 | DIRTY2) ? ARRAY_SIZE(values) : 1;
>
> @@ -729,8 +723,7 @@ static void preservation(int fd,
> 0xdeadbeef
> };
> const unsigned int num_values = ARRAY_SIZE(values);
> - unsigned int engine =
> - gem_class_instance_to_eb_flags(fd, e->class, e->instance);
> + unsigned int engine = e->flags;
> uint32_t ctx[num_values +1 ];
> uint32_t regs[num_values + 1][2];
> igt_spin_t *spin;
> @@ -840,7 +833,7 @@ igt_main
> igt_subtest_group {
> igt_fixture {
> igt_require(has_context_isolation & (1 << e->class));
> - gem_require_engine(fd, e->class, e->instance);
> + gem_require_ring(fd, e->flags);
> igt_fork_hang_detector(fd);
> }
>
> diff --git a/tools/intel_reg.c b/tools/intel_reg.c
> index 1247b70b0..e517956b8 100644
> --- a/tools/intel_reg.c
> +++ b/tools/intel_reg.c
> @@ -329,9 +329,7 @@ static int register_srm(struct config *config, struct reg *reg,
> memset(&execbuf, 0, sizeof(execbuf));
> execbuf.buffers_ptr = to_user_pointer(obj);
> execbuf.buffer_count = 2;
> - execbuf.flags = gem_class_instance_to_eb_flags(fd,
> - engine->class,
> - engine->instance);
> + execbuf.flags = engine->flags;
> if (secure)
> execbuf.flags |= I915_EXEC_SECURE;
>
>
It still puzzles me at which point was I915_EXEC_BSD_RING1 not allowed
on single vcs parts. Was it just a fail in IGT?!? The first hunk in this
patch. Digging in history points to that..
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH i-g-t] i915: Improve static engine map for legacy
2019-05-23 9:48 ` Tvrtko Ursulin
@ 2019-05-23 11:23 ` Chris Wilson
0 siblings, 0 replies; 10+ messages in thread
From: Chris Wilson @ 2019-05-23 11:23 UTC (permalink / raw)
To: Tvrtko Ursulin, intel-gfx; +Cc: igt-dev
Quoting Tvrtko Ursulin (2019-05-23 10:48:27)
>
> On 23/05/2019 09:06, Chris Wilson wrote:
> > We need to keep igt working on linus and dif, or Joonas gets very upset.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Andi Shyti <andi.shyti@intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > ---
> > lib/i915/gem_context.c | 2 +-
> > lib/i915/gem_engine_topology.c | 15 +++-------
> > lib/igt_gt.c | 54 ++++------------------------------
> > lib/igt_gt.h | 17 -----------
> > tests/i915/gem_ctx_isolation.c | 21 +++++--------
> > tools/intel_reg.c | 4 +--
> > 6 files changed, 19 insertions(+), 94 deletions(-)
> >
> > diff --git a/lib/i915/gem_context.c b/lib/i915/gem_context.c
> > index f94d89cb4..07ab78174 100644
> > --- a/lib/i915/gem_context.c
> > +++ b/lib/i915/gem_context.c
> > @@ -290,7 +290,7 @@ bool gem_context_has_engine(int fd, uint32_t ctx, uint64_t engine)
> > * wouldn't produce any result.
> > */
> > if ((engine & ~(3<<13)) == I915_EXEC_BSD) {
> > - if (engine & (3 << 13) && !gem_has_bsd2(fd))
> > + if (engine & (2 << 13) && !gem_has_bsd2(fd))
> > return false;
> > }
> >
> > diff --git a/lib/i915/gem_engine_topology.c b/lib/i915/gem_engine_topology.c
> > index d0c8bd5aa..fdd1b9516 100644
> > --- a/lib/i915/gem_engine_topology.c
> > +++ b/lib/i915/gem_engine_topology.c
> > @@ -223,22 +223,15 @@ 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];
> >
> > - if (!igt_only_list_subtests()) {
> > - __e2->flags = gem_class_instance_to_eb_flags(fd,
> > - e2->class, e2->instance);
> > -
> > - if (!gem_has_ring(fd, __e2->flags))
> > - continue;
> > - } else {
> > - __e2->flags = -1; /* 0xfff... */
> > - }
> > -
> > __e2->name = e2->name;
> > __e2->instance = e2->instance;
> > __e2->class = e2->class;
> > + __e2->flags = e2->flags;
> > __e2->is_virtual = false;
> >
> > - engine_data.nengines++;
> > + if (igt_only_list_subtests() ||
> > + gem_has_ring(fd, e2->flags))
> > + engine_data.nengines++;
> > }
> > return engine_data;
> > }
> > diff --git a/lib/igt_gt.c b/lib/igt_gt.c
> > index 6b7c037e6..78e3cd089 100644
> > --- a/lib/igt_gt.c
> > +++ b/lib/igt_gt.c
> > @@ -585,12 +585,12 @@ bool gem_can_store_dword(int fd, unsigned int engine)
> > }
> >
> > const struct intel_execution_engine2 intel_execution_engines2[] = {
> > - { "rcs0", I915_ENGINE_CLASS_RENDER, 0 },
> > - { "bcs0", I915_ENGINE_CLASS_COPY, 0 },
> > - { "vcs0", I915_ENGINE_CLASS_VIDEO, 0 },
> > - { "vcs1", I915_ENGINE_CLASS_VIDEO, 1 },
> > - { "vcs2", I915_ENGINE_CLASS_VIDEO, 2 },
> > - { "vecs0", I915_ENGINE_CLASS_VIDEO_ENHANCE, 0 },
> > + { "rcs0", I915_ENGINE_CLASS_RENDER, 0, I915_EXEC_RENDER },
> > + { "bcs0", I915_ENGINE_CLASS_COPY, 0, I915_EXEC_BLT },
> > + { "vcs0", I915_ENGINE_CLASS_VIDEO, 0, I915_EXEC_BSD | I915_EXEC_BSD_RING1 },
> > + { "vcs1", I915_ENGINE_CLASS_VIDEO, 1, I915_EXEC_BSD | I915_EXEC_BSD_RING2 },
> > + { "vcs2", I915_ENGINE_CLASS_VIDEO, 2, -1 },
> > + { "vecs0", I915_ENGINE_CLASS_VIDEO_ENHANCE, 0, I915_EXEC_VEBOX },
> > { }
> > };
> >
> > @@ -611,48 +611,6 @@ int gem_execbuf_flags_to_engine_class(unsigned int flags)
> > }
> > }
> >
> > -unsigned int
> > -gem_class_instance_to_eb_flags(int gem_fd,
> > - enum drm_i915_gem_engine_class class,
> > - unsigned int instance)
> > -{
> > - if (class != I915_ENGINE_CLASS_VIDEO)
> > - igt_assert(instance == 0);
> > - else
> > - igt_assert(instance >= 0 && instance <= 1);
> > -
> > - switch (class) {
> > - case I915_ENGINE_CLASS_RENDER:
> > - return I915_EXEC_RENDER;
> > - case I915_ENGINE_CLASS_COPY:
> > - return I915_EXEC_BLT;
> > - case I915_ENGINE_CLASS_VIDEO:
> > - if (instance == 0) {
> > - if (gem_has_bsd2(gem_fd))
> > - return I915_EXEC_BSD | I915_EXEC_BSD_RING1;
> > - else
> > - return I915_EXEC_BSD;
> > -
> > - } else {
> > - return I915_EXEC_BSD | I915_EXEC_BSD_RING2;
> > - }
> > - case I915_ENGINE_CLASS_VIDEO_ENHANCE:
> > - return I915_EXEC_VEBOX;
> > - case I915_ENGINE_CLASS_INVALID:
> > - default:
> > - igt_assert(0);
> > - };
> > -}
> > -
> > -bool gem_has_engine(int gem_fd,
> > - enum drm_i915_gem_engine_class class,
> > - unsigned int instance)
> > -{
> > - return gem_has_ring(gem_fd,
> > - gem_class_instance_to_eb_flags(gem_fd, class,
> > - instance));
> > -}
> > -
> > bool gem_ring_is_physical_engine(int fd, unsigned ring)
> > {
> > if (ring == I915_EXEC_DEFAULT)
> > diff --git a/lib/igt_gt.h b/lib/igt_gt.h
> > index 77318e2a8..73b5002a0 100644
> > --- a/lib/igt_gt.h
> > +++ b/lib/igt_gt.h
> > @@ -102,21 +102,4 @@ extern const struct intel_execution_engine2 {
> >
> > int gem_execbuf_flags_to_engine_class(unsigned int flags);
> >
> > -unsigned int
> > -gem_class_instance_to_eb_flags(int gem_fd,
> > - enum drm_i915_gem_engine_class class,
> > - unsigned int instance);
> > -
> > -bool gem_has_engine(int gem_fd,
> > - enum drm_i915_gem_engine_class class,
> > - unsigned int instance);
> > -
> > -static inline
> > -void gem_require_engine(int gem_fd,
> > - enum drm_i915_gem_engine_class class,
> > - unsigned int instance)
> > -{
> > - igt_require(gem_has_engine(gem_fd, class, instance));
> > -}
> > -
> > #endif /* IGT_GT_H */
> > diff --git a/tests/i915/gem_ctx_isolation.c b/tests/i915/gem_ctx_isolation.c
> > index bcd0f4812..5b054c81d 100644
> > --- a/tests/i915/gem_ctx_isolation.c
> > +++ b/tests/i915/gem_ctx_isolation.c
> > @@ -321,8 +321,7 @@ static uint32_t read_regs(int fd,
> > memset(&execbuf, 0, sizeof(execbuf));
> > execbuf.buffers_ptr = to_user_pointer(obj);
> > execbuf.buffer_count = 2;
> > - execbuf.flags =
> > - gem_class_instance_to_eb_flags(fd, e->class, e->instance);
> > + execbuf.flags = e->flags;
> > execbuf.rsvd1 = ctx;
> > gem_execbuf(fd, &execbuf);
> > gem_close(fd, obj[1].handle);
> > @@ -377,8 +376,7 @@ static void write_regs(int fd,
> > memset(&execbuf, 0, sizeof(execbuf));
> > execbuf.buffers_ptr = to_user_pointer(&obj);
> > execbuf.buffer_count = 1;
> > - execbuf.flags =
> > - gem_class_instance_to_eb_flags(fd, e->class, e->instance);
> > + execbuf.flags = e->flags;
> > execbuf.rsvd1 = ctx;
> > gem_execbuf(fd, &execbuf);
> > gem_close(fd, obj.handle);
> > @@ -448,8 +446,7 @@ static void restore_regs(int fd,
> > memset(&execbuf, 0, sizeof(execbuf));
> > execbuf.buffers_ptr = to_user_pointer(obj);
> > execbuf.buffer_count = 2;
> > - execbuf.flags =
> > - gem_class_instance_to_eb_flags(fd, e->class, e->instance);
> > + execbuf.flags = e->flags;
> > execbuf.rsvd1 = ctx;
> > gem_execbuf(fd, &execbuf);
> > gem_close(fd, obj[1].handle);
> > @@ -559,8 +556,7 @@ static void nonpriv(int fd,
> > 0x0505c0c0,
> > 0xdeadbeef
> > };
> > - unsigned int engine =
> > - gem_class_instance_to_eb_flags(fd, e->class, e->instance);
> > + unsigned int engine = e->flags;
> > unsigned int num_values = ARRAY_SIZE(values);
> >
> > /* Sigh -- hsw: we need cmdparser access to our own registers! */
> > @@ -616,9 +612,7 @@ static void isolation(int fd,
> > 0xaaaaaaaa,
> > 0xdeadbeef
> > };
> > - unsigned int engine = gem_class_instance_to_eb_flags(fd,
> > - e->class,
> > - e->instance);
> > + unsigned int engine = e->flags;
> > unsigned int num_values =
> > flags & (DIRTY1 | DIRTY2) ? ARRAY_SIZE(values) : 1;
> >
> > @@ -729,8 +723,7 @@ static void preservation(int fd,
> > 0xdeadbeef
> > };
> > const unsigned int num_values = ARRAY_SIZE(values);
> > - unsigned int engine =
> > - gem_class_instance_to_eb_flags(fd, e->class, e->instance);
> > + unsigned int engine = e->flags;
> > uint32_t ctx[num_values +1 ];
> > uint32_t regs[num_values + 1][2];
> > igt_spin_t *spin;
> > @@ -840,7 +833,7 @@ igt_main
> > igt_subtest_group {
> > igt_fixture {
> > igt_require(has_context_isolation & (1 << e->class));
> > - gem_require_engine(fd, e->class, e->instance);
> > + gem_require_ring(fd, e->flags);
> > igt_fork_hang_detector(fd);
> > }
> >
> > diff --git a/tools/intel_reg.c b/tools/intel_reg.c
> > index 1247b70b0..e517956b8 100644
> > --- a/tools/intel_reg.c
> > +++ b/tools/intel_reg.c
> > @@ -329,9 +329,7 @@ static int register_srm(struct config *config, struct reg *reg,
> > memset(&execbuf, 0, sizeof(execbuf));
> > execbuf.buffers_ptr = to_user_pointer(obj);
> > execbuf.buffer_count = 2;
> > - execbuf.flags = gem_class_instance_to_eb_flags(fd,
> > - engine->class,
> > - engine->instance);
> > + execbuf.flags = engine->flags;
> > if (secure)
> > execbuf.flags |= I915_EXEC_SECURE;
> >
> >
>
> It still puzzles me at which point was I915_EXEC_BSD_RING1 not allowed
> on single vcs parts. Was it just a fail in IGT?!? The first hunk in this
> patch. Digging in history points to that..
I do recall making a decision to hide vcs1 if !vcs2 so that we didn't
duplicate (vcs, vcs1), and still have the plain randomly chosen vcs.
That is likely the additional source of confusion.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-05-23 11:23 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-05-23 8:06 [PATCH i-g-t] i915: Improve static engine map for legacy Chris Wilson
2019-05-23 8:33 ` Andi Shyti
2019-05-23 8:46 ` Tvrtko Ursulin
2019-05-23 9:09 ` Chris Wilson
2019-05-23 9:14 ` Chris Wilson
2019-05-23 9:35 ` Tvrtko Ursulin
2019-05-23 9:46 ` Chris Wilson
2019-05-23 9:27 ` Tvrtko Ursulin
2019-05-23 9:48 ` Tvrtko Ursulin
2019-05-23 11:23 ` Chris Wilson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox