Re: [PATCH 12/18] drm/i915: Allow userspace to clone contexts on creation

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 21/03/2019 14:38, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-03-20 13:13:45)
>>
>> On 19/03/2019 11:57, Chris Wilson wrote:
>>> A usecase arose out of handling context recovery in mesa, whereby they
>>> wish to recreate a context with fresh logical state but preserving all
>>> other details of the original. Currently, they create a new context and
>>> iterate over which bits they want to copy across, but it would much more
>>> convenient if they were able to just pass in a target context to clone
>>> during creation. This essentially extends the setparam during creation
>>> to pull the details from a target context instead of the user supplied
>>> parameters.
>>>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> ---
>>>    drivers/gpu/drm/i915/i915_gem_context.c | 154 ++++++++++++++++++++++++
>>>    include/uapi/drm/i915_drm.h             |  14 +++
>>>    2 files changed, 168 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
>>> index fc1f64e19507..f36648329074 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem_context.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
>>> @@ -1500,8 +1500,162 @@ static int create_setparam(struct i915_user_extension __user *ext, void *data)
>>>        return ctx_setparam(arg->ctx, &local.param);
>>>    }
>>>    
>>> +static int clone_flags(struct i915_gem_context *dst,
>>> +                    struct i915_gem_context *src)
>>> +{
>>> +     dst->user_flags = src->user_flags;
>>> +     return 0;
>>> +}
>>> +
>>> +static int clone_schedattr(struct i915_gem_context *dst,
>>> +                        struct i915_gem_context *src)
>>> +{
>>> +     dst->sched = src->sched;
>>> +     return 0;
>>> +}
>>> +
>>> +static int clone_sseu(struct i915_gem_context *dst,
>>> +                   struct i915_gem_context *src)
>>> +{
>>> +     const struct intel_sseu default_sseu =
>>> +             intel_device_default_sseu(dst->i915);
>>> +     struct intel_engine_cs *engine;
>>> +     enum intel_engine_id id;
>>> +
>>> +     for_each_engine(engine, dst->i915, id) {
>>
>> Hm in the load balancing patch this needs to be extended so the veng ce
>> is also handled here.
>>
>> Possibly even when adding engine map the loop needs to iterate the map
>> and not for_each_engine?
> 
> One problem is that it is hard to match a veng in one context with
> another context, there may even be several :|
> 
> And then in clone_engines, we create a fresh virtual engine. So a nasty
> interoperation with clone_engines.
> 
> Bleugh.

Hmm indeed..

CLONE_ENGINES + CLONE_SSEU is possible, but order of cloning is 
important, right?

CLONE_SSEU without CLONE_ENGINES is less well intuitively defined. 
Contexts might not be "compatible" as you say. Should the code check if 
the map matches and veng instances match in their masks? Sounds 
questionable since otherwise in the patch you took the approach of 
cloning what can be cloned.

What seemed a simple patch is becoming more complicated.

>>> +             struct intel_context *ce;
>>> +             struct intel_sseu sseu;
>>> +
>>> +             ce = intel_context_lookup(src, engine);
>>> +             if (!ce)
>>> +                     continue;
>>> +
>>> +             sseu = ce->sseu;
>>> +             if (!memcmp(&sseu, &default_sseu, sizeof(sseu)))
>>
>> Could memcmp against &ce->sseu directly and keep src_ce and dst_ce so
>> you can copy over without a temporary copy on stack?
> 
> At one point, the locking favoured making a local sseu to avoid
> overlapping locks. Hmm, sseu = ce->sseu could still tear. Pedantically
> that copy should be locked.

True!

>>> +                     continue;
>>> +
>>> +             ce = intel_context_pin_lock(dst, engine);
>>> +             if (IS_ERR(ce))
>>> +                     return PTR_ERR(ce);
>>> +
>>> +             ce->sseu = sseu;
>>> +             intel_context_pin_unlock(ce);
>>> +     }
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int clone_timeline(struct i915_gem_context *dst,
>>> +                       struct i915_gem_context *src)
>>> +{
>>> +     if (src->timeline) {
>>> +             GEM_BUG_ON(src->timeline == dst->timeline);
>>> +
>>> +             if (dst->timeline)
>>> +                     i915_timeline_put(dst->timeline);
>>> +             dst->timeline = i915_timeline_get(src->timeline);
>>> +     }
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int clone_vm(struct i915_gem_context *dst,
>>> +                 struct i915_gem_context *src)
>>> +{
>>> +     struct i915_hw_ppgtt *ppgtt;
>>> +
>>> +     rcu_read_lock();
>>> +     do {
>>> +             ppgtt = READ_ONCE(src->ppgtt);
>>> +             if (!ppgtt)
>>> +                     break;
>>> +
>>> +             if (!kref_get_unless_zero(&ppgtt->ref))
>>> +                     continue;
>>> +
>>> +             /*
>>> +              * This ppgtt may have be reallocated between
>>> +              * the read and the kref, and reassigned to a third
>>> +              * context. In order to avoid inadvertent sharing
>>> +              * of this ppgtt with that third context (and not
>>> +              * src), we have to confirm that we have the same
>>> +              * ppgtt after passing through the strong memory
>>> +              * barrier implied by a successful
>>> +              * kref_get_unless_zero().
>>> +              *
>>> +              * Once we have acquired the current ppgtt of src,
>>> +              * we no longer care if it is released from src, as
>>> +              * it cannot be reallocated elsewhere.
>>> +              */
>>> +
>>> +             if (ppgtt == READ_ONCE(src->ppgtt))
>>> +                     break;
>>> +
>>> +             i915_ppgtt_put(ppgtt);
>>> +     } while (1);
>>> +     rcu_read_unlock();
>>
>> I still have the same problem. What if you added here:
>>
>> GEM_BUG_ON(ppgtt != READ_ONCE(src->ppgtt));
>>
>> Could it trigger? If so what is the point in the last check in the loop
>> above?
> 
> Yes, it can trigger, as there is no outer mutex guarding the assignment
> of src->ppgtt with our read. And that is why the check has to exist --
> because it can be reassigned during the first read and before we acquire
> the kref, and so ppgtt may have been freed and then reallocated and
> assigned to a new ctx during that interval. We don't care that
> src->ppgtt gets updated after we have taken a copy, we care that ppgtt
> may get reused on another ctx entirely.

Okay I get it, RCU allocation recycling? Makes sense in that case.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx