[Intel-gfx] [PATCH 31/32] drm/i915/execlists: Virtual engine bonding

[Chris Wilson]

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