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

[Chris Wilson]

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.

> >>>>> +
> >>>>> +     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.
-Chris