On 18/04/2019 10:59, Chris Wilson wrote:
Quoting Tvrtko Ursulin (2019-04-18 10:50:30)
On 18/04/2019 10:13, Chris Wilson wrote:
Quoting Tvrtko Ursulin (2019-04-18 09:57:43)
On 18/04/2019 07:57, Chris Wilson wrote:
Quoting Tvrtko Ursulin (2019-04-18 07:47:51)
On 17/04/2019 08:56, Chris Wilson wrote:
+static void
+virtual_bond_execute(struct i915_request *rq, struct dma_fence *signal)
+{
+ struct virtual_engine *ve = to_virtual_engine(rq->engine);
+ struct ve_bond *bond;
+
+ bond = virtual_find_bond(ve, to_request(signal)->engine);
+ if (bond) {
+ intel_engine_mask_t old, new, cmp;
+
+ cmp = READ_ONCE(rq->execution_mask);
+ do {
+ old = cmp;
+ new = cmp & bond->sibling_mask;
+ } while ((cmp = cmpxchg(&rq->execution_mask, old, new)) != old);
Loop implies someone else might be modifying the rq->execution_mask in
parallel?
There's nothing that prevents there being multiple bonds being
executed simultaneously (other than practicality). There's also nothing
that says this should be the only way to modify rq->execution_mask in
the future.
But request is one, how can it be submitted multiple times simultaneously?
You mean "How can it be signaled multiple times simultaneously?"
Okay yes, signaled. You could give same submit fence to multiple slaves,
but you can't have same slave request receive notification from multiple
masters.
Or you can if you build a composite fence and pass that in? Is this the
story about signal-on-any vs signal-on-all?
There's nothing inherent in the design to prevent virtual_bond_execute
being called multiple times given multiple fences along one or more
engines.
There's a practical limitation in the proposed uAPI to limit it to a
single submit-fence, but may indeed be a composite fence. There's also
the question of whether to squeeze in syncobj support.
Ok. Just drop in a comment with the loop please.
+
+ err = check_user_mbz(&ext->flags);
+ if (err)
+ return err;
+
+ for (n = 0; n < ARRAY_SIZE(ext->mbz64); n++) {
+ err = check_user_mbz(&ext->mbz64[n]);
+ if (err)
+ return err;
+ }
+
+ if (get_user(class, &ext->master_class))
+ return -EFAULT;
+
+ if (get_user(instance, &ext->master_instance))
+ return -EFAULT;
+
+ master = intel_engine_lookup_user(set->ctx->i915, class, instance);
+ if (!master) {
+ DRM_DEBUG("Unrecognised master engine: { class:%d, instance:%d }\n",
+ class, instance);
+ return -EINVAL;
+ }
+
+ if (get_user(num_bonds, &ext->num_bonds))
+ return -EFAULT;
Should num_bonds > virtual->num_siblings be an error?
They could specify the same bond multiple times for whatever reason (and
probably should allow skipping NONE?), if the target doesn't exist that's
definitely an error.
So which bond we pick if they specify multiple ones? Just the first one
found. Hm actually I was thinking about making sure each master is only
specified once, not siblings. For siblings we indeed do not care.
No, it's a mask of if parent executes on master, use this set of
children.
I was reasonably happy to use a cumulative mask if master is specified
by more than one bond ext; but maybe it should be an intersection. Hmm.
Do you see a realistic and making sense use case for specifying the same
master in multiple bonds? If not I'd just disallow it and then we don't
have a question of union vs intersection policy.
Rather the opposite, I don't see that it breaks anything nor need it be
ill-defined, hence no reason to reject as a means to protect oneself.
I don't mean it's dangerous or poorly defined. In that sense your
current implementation of using an union is fine. (Earlier I forgot that
you skip creating multiple bond objects in this case.)
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
