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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
