Quoting Tvrtko Ursulin (2019-07-31 07:12:35)
>
> On 30/07/2019 18:07, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-07-30 16:20:08)
> >>
> >> On 30/07/2019 09:04, Ramalingam C wrote:
> >>> On 2019-07-30 at 13:05:27 +0100, Tvrtko Ursulin wrote:
> >>>>
> >>>> On 30/07/2019 04:58, Ramalingam C wrote:
> >>>>> +bool gem_eb_flags_are_different_engines(unsigned eb_flag1, unsigned
> >>>>> eb_flag2)
> >>>>
> >>>> I think we try to avoid implied int but not sure in this case whether to
> >>>> suggest unsigned int, long or uint64_t. If we are suggesting in the
> >>>> function name that any flags can be passed in perhaps it should be
> >>>> uint64_t and then we filter on the engine bits (flags.. &=
> >>>> I915_EXEC_RING_MASK | (3 << 13)) before checking. Yeah, I think that
> >>>> would be more robust for a generic helper.
> >>>>
> >>>> And add a doc blurb for this helper since it is non-obvious why we went
> >>>> for different and not same. My thinking was the name different would be
> >>>> clearer to express kind of tri-state nature of this check. (Flags may be
> >>>> different, but engines are not guaranteed to be different.) Have I
> >>>> over-complicated it? Do we need to make it clearer by naming it
> >>>> gem_eb_flags_are_guaranteed_different_engines? :)
> >>> For me current shape looks good enough :) I will use the uint64_t for
> >>> parameter types.
> >>
> >> Okay but please add some documentation for the helper (we've been very
> >> bad in this work in this respect so far) and also filter out non-engine
> >> selection bits from the flags before doing the checks.
> >>
> >>>>
> >>>>> +{
> >>>>> + if (eb_flag1 == eb_flag2)
> >>>>> + return false;
> >>>>> +
> >>>>> + /* DEFAULT stands for RENDER in legacy uAPI*/
> >>>>> + if ((eb_flag1 == I915_EXEC_DEFAULT && eb_flag2 == I915_EXEC_RENDER)
> >>>>> ||
> >>>>> + (eb_flag1 == I915_EXEC_RENDER && eb_flag2 ==
> >>>>> I915_EXEC_DEFAULT))
> >>>>> + return false;
> >>>>> +
> >>>>> + /*
> >>>>> + * BSD could be executed on BSD1/BSD2. So BSD and BSD-n could be
> >>>>> + * same engine.
> >>>>> + */
> >>>>> + if ((eb_flag1 == I915_EXEC_BSD) &&
> >>>>> + (eb_flag2 & ~I915_EXEC_BSD_MASK) == I915_EXEC_BSD)
> >>>>> + return false;
> >>>>> +
> >>>>> + if ((eb_flag2 == I915_EXEC_BSD) &&
> >>>>> + (eb_flag1 & ~I915_EXEC_BSD_MASK) == I915_EXEC_BSD)
> >>>>> + return false;
> >>>>
> >>>> I think this works.
> >>>>
> >>>> I've also come up with something to merge the two checks, not 100% it's
> >>>> correct or more readable:
> >>>>
> >>>> if (((flag1 | flag2) & I915_EXEC_RING_MASK)) == I915_EXEC_BSD && // at
> >>>> least one is BSD
> >>>> !((flag1 ^ flag2) & I915_EXEC_RING_MASK) && // both are BSD
> >>>> (((flag1 | flag2) & (3 << 13))) != 3) // not guaranteed different
> >>>> return false;
> >>>>
> >>>> Would need feeding in some values and checking it works as expected.
> >>>> Probably not worth it since I doubt it is more readable.
> >>> readability perspective, we could stick to the original version. If we
> >>> want to go ahead we need to do below ops:
> >>
> >> Stick with your version I think.
> >>
> >> Chris is being quiet BTW. Either we are below his radar and he'll scream
> >> later, or we managed to approach something he finds passable. ;)
> >
> > Just waiting until I have to use it in anger :)
> >
> > Gut feeling is that I won't have to use it, in that if I have two
> > different timelines pointing to the same physical engine, do I really
> > care? The situations where I would have distinct engine mappings strike
> > me as being cases where I'm testing timelines; otherwise I would create
> > contexts with the same ctx->engines[] map.
>
> I do dislike this complication of testing uapi flags but figuring out
> the physical engines under the covers. Do you think it would be clearer
> do drop this helper and instead use two contexts in this test? It would
> make it dependent on contexts though..
Going back to look at the use case...
tests/i915/gem_exec_async.c | 81 ++++++++++++++++++++++++++++---------
1 file changed, 63 insertions(+), 18 deletions(-)
diff --git a/tests/i915/gem_exec_async.c b/tests/i915/gem_exec_async.c
index 9a06af7e2..fafca98ad 100644
--- a/tests/i915/gem_exec_async.c
+++ b/tests/i915/gem_exec_async.c
@@ -80,7 +80,10 @@ static void store_dword(int fd, unsigned ring,
gem_close(fd, obj[1].handle);
}
-static void one(int fd, unsigned ring, uint32_t flags)
+static void one(int fd,
+ unsigned int idx,
+ const struct intel_execution_engine2 *engines,
+ unsigned int num_engines)
{
const int gen = intel_gen(intel_get_drm_devid(fd));
struct drm_i915_gem_exec_object2 obj[2];
@@ -88,7 +91,6 @@ static void one(int fd, unsigned ring, uint32_t flags)
#define BATCH 1
struct drm_i915_gem_relocation_entry reloc;
struct drm_i915_gem_execbuffer2 execbuf;
- unsigned int other;
uint32_t *batch;
int i;
@@ -138,19 +140,17 @@ static void one(int fd, unsigned ring, uint32_t flags)
memset(&execbuf, 0, sizeof(execbuf));
execbuf.buffers_ptr = to_user_pointer(obj);
execbuf.buffer_count = 2;
- execbuf.flags = ring | flags;
+ execbuf.flags = engines[idx].flags;
igt_require(__gem_execbuf(fd, &execbuf) == 0);
gem_close(fd, obj[BATCH].handle);
i = 0;
- for_each_physical_engine(fd, other) {
- if (other == ring)
+ for (unsigned int other = 0; other < num_engines; other++) {
+ if (other == idx)
continue;
- if (!gem_can_store_dword(fd, other))
- continue;
-
- store_dword(fd, other, obj[SCRATCH].handle, 4*i, i);
+ store_dword(fd, engines[other].flags,
+ obj[SCRATCH].handle, 4*i, i);
i++;
}
@@ -185,7 +185,6 @@ static bool has_async_execbuf(int fd)
igt_main
{
- const struct intel_execution_engine *e;
int fd = -1;
igt_skip_on_simulation();
@@ -199,15 +198,61 @@ igt_main
igt_fork_hang_detector(fd);
}
- for (e = intel_execution_engines; e->name; e++) {
- /* default exec-id is purely symbolic */
- if (e->exec_id == 0)
- continue;
+ /* First up, check the legacy engine selector ABI for independence */
+ igt_subtest_group {
+ struct intel_execution_engine2 engines[64];
+ unsigned int num_engines = 0;
+
+ igt_fixture {
+ const struct intel_execution_engine *e;
+
+ for (e = intel_execution_engines; e->name; e++) {
+ if (!gem_ring_has_physical_engine(fd,
e->exec_id | e->flags))
+ continue;
+
+ /* Must be unique, no unknowable BSD aliases! */
+ engines[num_engines] =
+ gem_eb_flags_to_engine(e->exec_id |
e->flags);
+ if (engines[num_engines].flags != -1)
+ continue;
+
+ if (!gem_can_store_dword(fd,
engines[num_engines].flags))
+ continue;
+
+ num_engines++;
+ if (num_engines == ARRAY_SIZE(engines))
+ break;
+ }
+ }
+
+ for (unsigned int n = 0; n < num_engines; n++) {
+ igt_subtest_f("legacy-concurrent-writes-%s",
+ engines[n].name)
+ one(fd, n, engines, num_engines);
+ }
+ }
+
+ /* Same again, but using a ctx->engine[] map and indices */
+ igt_subtest_group {
+ struct intel_execution_engine2 engines[64];
+ unsigned int num_engines = 0;
+
+ igt_fixture {
+ const struct intel_execution_engine2 *e;
+
+ __for_each_physical_engine(fd, e) {
+ if (!gem_class_can_store_dword(fd, e->class))
+ continue;
+
+ engines[num_engines++] = *e;
+ if (num_engines == ARRAY_SIZE(engines))
+ break;
+ }
+ }
- igt_subtest_f("concurrent-writes-%s", e->name) {
- igt_require(gem_ring_has_physical_engine(fd, e->exec_id
| e->flags));
- igt_require(gem_can_store_dword(fd, e->exec_id |
e->flags));
- one(fd, e->exec_id, e->flags);
+ for (unsigned int n = 0; n < num_engines; n++) {
+ igt_subtest_f("concurrent-writes-%s", engines[n].name)
+ one(fd, n, engines, num_engines);
}
}
(Fill in the gaps for the new structs!)
Is more of what I was expecting. (Eventually no one will notice the BSD
aliasing bit rotting and we can remove it from the uABI.)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
