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

[Tvrtko Ursulin]

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