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

[Tvrtko Ursulin]

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