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?
+static int
+set_engines__bond(struct i915_user_extension __user *base, void *data)
+{
+ struct i915_context_engines_bond __user *ext =
+ container_of_user(base, typeof(*ext), base);
+ const struct set_engines *set = data;
+ struct intel_engine_cs *virtual;
+ struct intel_engine_cs *master;
+ u16 class, instance;
+ u16 idx, num_bonds;
+ int err, n;
+
+ if (get_user(idx, &ext->virtual_index))
+ return -EFAULT;
+
+ if (idx >= set->engines->num_engines) {
+ DRM_DEBUG("Invalid index for virtual engine: %d >= %d\n",
+ idx, set->engines->num_engines);
+ return -EINVAL;
+ }
+
+ idx = array_index_nospec(idx, set->engines->num_engines);
+ if (!set->engines->engines[idx]) {
+ DRM_DEBUG("Invalid engine at %d\n", idx);
+ return -EINVAL;
+ }
+
+ /*
+ * A non-virtual engine has 0 siblings to choose between; and submit
+ * fence will always be directed to the one engine.
+ */
+ virtual = set->engines->engines[idx]->engine;
+ if (!intel_engine_is_virtual(virtual))
+ return 0;
Hmm wouldn't we strictly speaking need to distinguish between uAPI
errors and auto-magic-single-veng-replacement? Latter is OK to return
success, but former should be reported as -EINVAL I think.
Is it a uAPI error if it works? :)
It works but what is the practical use? It more signals userspace got
it's configuration wrong and if we silently accept it gets more
difficult to figure out.
+
+ 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.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
