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.
> > +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? :)
> > +
> > + 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.
> > +/*
> > + * i915_context_engines_bond:
> > + *
> > + * Constructed bonded pairs for execution within a virtual engine.
> > + *
> > + * All engines are equal, but some are more equal than others. Given
> > + * the distribution of resources in the HW, it may be preferable to run
> > + * a request on a given subset of engines in parallel to a request on a
> > + * specific engine. We enable this selection of engines within a virtual
> > + * engine by specifying bonding pairs, for any given master engine we will
> > + * only execute on one of the corresponding siblings within the virtual engine.
> > + *
> > + * To execute a request in parallel on the master engine and a sibling requires
> > + * coordination with a I915_EXEC_FENCE_SUBMIT.
> > + */
> > +struct i915_context_engines_bond {
> > + struct i915_user_extension base;
> > +
> > + __u16 virtual_index; /* index of virtual engine in ctx->engines[] */
> > + __u16 num_bonds;
> > +
> > + __u16 master_class;
> > + __u16 master_instance;
>
> struct i915_engine_class_instance master; ?
Yup, the oversight struck me when updating the igt.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
