[Intel-gfx] [PATCH 04/11] drm/i915: Generalise GPU activity tracking

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-01-30 12:02:02)
> 
> On 30/01/2019 02:18, Chris Wilson wrote:
> > +struct active_node {
> > +     struct i915_gem_active base;
> > +     struct i915_active *ref;
> > +     struct rb_node node;
> > +     u64 timeline;
> > +};
> > +
> > +static void
> > +__active_retire(struct i915_active *ref)
> > +{
> 
> You wouldn't consider naming this variable 'active' throughout? Ref 
> reminds me so much of a kref eg. fence->refcount. Although 'active' has 
> been used for gem_active so far. Not a blocker though, I'll get used to it.

Similarity to kref wasn't a coincidence, because we also use the concept
of active reference for this as well.

I'm not sold on the name yet. I liked active_tracker but thought that
was slightly too long for something I expect to be fairly ubiquitous.
struct reservation_object, I don't like because reservation is a phase
before building the request.

i915_shared_fence and i915_exclusive_fence; maybe? But fences tend to
be the single-shot, aka i915_request.

i915_hedgerow. I'm being silly now.

If only I can come up with as catchy a name as rcu.

rgu? Read-gpu-update. grf; gpu-read-reference.

> > +int i915_active_ref(struct i915_active *ref,
> > +                 u64 timeline,
> > +                 struct i915_request *rq)
> > +{
> > +     struct i915_gem_active *active;
> > +
> > +     active = active_instance(ref, timeline);
> > +     if (IS_ERR(active))
> > +             return PTR_ERR(active);
> > +
> > +     if (!i915_gem_active_isset(active))
> > +             ref->count++;
> 
> Could stick a super-paranoid overflow GEM_BUG_ON here.

Yeah. Wouldn't it be nice if refcount_t wasn't quite so tied to
refcount_t. Just a plain old count_t and atomic_count_t.

> > -     if (!i915_gem_active_isset(active) && !vma->active_count++)
> > +     if (!vma->active.count)
> >               obj->active_count++;
> > -     i915_gem_active_set(active, rq);
> > +
> > +     if (unlikely(i915_active_ref(&vma->active, rq->fence.context, rq))) {
> > +             if (!vma->active.count)
> > +                     obj->active_count--;
> > +             return -ENOMEM;
> > +     }
> 
> Optionally you could make i915_active_ref return the old ref count or 
> error. Then this could become simpler:
> 
>         ret = i915_active_ref(..);
>         if (unlikely(ret < 0))
>                 return -ENOMEM;
>         else if (ret == 0)
>                 obj->active_count++;

Heh, didn't immediately strike me as simpler, but I did also consider
it.

I think for the atomic variant, we may just pass an init_func(). So
watch this space.
 
> Body looks good. I left selftests for after I get a further in the series.

They weren't very exciting I'm afraid, just aiming to have minimal
walking through the api points.
-Chris