[Intel-gfx] [PATCH v2 2/3] drm/i915: Wrap a timer into a i915_sw_fence

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-10-11 13:20:25)
> 
> On 10/10/2017 22:38, Chris Wilson wrote:
> > For some selftests, we want to issue requests but delay them going to
> > hardware. Furthermore, we don't want those requests to block
> > indefinitely (or else we may hang the driver and block testing) so we
> > want to employ a timeout. So naturally we want a fence that is
> > automatically signaled by a timer.
> > 
> > v2: Add kselftests.
> > 
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > ---
> >   drivers/gpu/drm/i915/i915_sw_fence.c           | 64 ++++++++++++++++++++++++++
> >   drivers/gpu/drm/i915/i915_sw_fence.h           |  3 ++
> >   drivers/gpu/drm/i915/selftests/i915_sw_fence.c | 43 +++++++++++++++++
> >   3 files changed, 110 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c
> > index 808ea4d5b962..388424a95ac9 100644
> > --- a/drivers/gpu/drm/i915/i915_sw_fence.c
> > +++ b/drivers/gpu/drm/i915/i915_sw_fence.c
> > @@ -506,6 +506,70 @@ int i915_sw_fence_await_reservation(struct i915_sw_fence *fence,
> >       return ret;
> >   }
> >   
> > +struct timer_fence {
> 
> timed fence?
> 
> > +     struct i915_sw_fence base;
> > +     struct timer_list timer;
> > +     struct kref ref;
> > +};
> > +
> > +static void timer_fence_wake(unsigned long data)
> > +{
> > +     struct timer_fence *tf = (struct timer_fence *)data;
> > +
> > +     i915_sw_fence_complete(&tf->base);
> > +}
> > +
> > +static void i915_sw_fence_timer_free(struct kref *ref)
> > +{
> > +     struct timer_fence *tf = container_of(ref, typeof(*tf), ref);
> > +
> > +     kfree(tf);
> > +}
> > +
> > +static void i915_sw_fence_timer_put(struct i915_sw_fence *fence)
> > +{
> > +     struct timer_fence *tf = container_of(fence, typeof(*tf), base);
> > +
> > +     kref_put(&tf->ref, i915_sw_fence_timer_free);
> > +}
> > +
> > +static int __i915_sw_fence_call
> > +timer_fence_notify(struct i915_sw_fence *fence, enum i915_sw_fence_notify state)
> > +{
> > +     if (state == FENCE_FREE)
> > +             i915_sw_fence_timer_put(fence);
> > +
> > +     return NOTIFY_DONE;
> > +}
> > +
> > +struct i915_sw_fence *i915_sw_fence_create_timer(long timeout, gfp_t gfp)
> 
> Depending on the outcome of the public vs test private question (see 
> below), you might want to rename timeout to expired to signify it is in 
> absolute jiffies. And also add kernel doc if it will be public.

timed_fence + expires are good suggestions.

> > +{
> > +     struct timer_fence *tf;
> > +
> > +     tf = kmalloc(sizeof(*tf), gfp);
> > +     if (!tf)
> > +             return ERR_PTR(-ENOMEM);
> > +
> > +     i915_sw_fence_init(&tf->base, timer_fence_notify);
> > +     kref_init(&tf->ref);
> > +
> > +     setup_timer(&tf->timer, timer_fence_wake, (unsigned long)tf);
> > +     mod_timer(&tf->timer, timeout);
> > +
> > +     kref_get(&tf->ref);
> > +     return &tf->base;
> > +}
> > +
> > +void i915_sw_fence_timer_flush(struct i915_sw_fence *fence)
> > +{
> > +     struct timer_fence *tf = container_of(fence, typeof(*tf), base);
> > +
> > +     if (del_timer_sync(&tf->timer))
> > +             i915_sw_fence_complete(&tf->base);
> > +
> > +     i915_sw_fence_timer_put(fence);
> 
> A bit unusual that flush destroys an object? Should i915_sw_fence_fini 
> just be made work on these to come close to OO design it partially tries 
> to do?

I repeated the pattern "flush; put" everytime so I decided to hide the ref
and just have the second API point consume the fence.

Flush is the right semantic for the first action on the fence, but not a
great name for the combined operation.

I did consider doing i915_sw_fence_init_timer() and timed_fence_fini()
but wasn't as keen on exporting the struct. Though really if we follow
the selftests/ direction, that isn't an issue.
 
> I reckon there are some private functions it uses so it has to live in 
> i915_sw_fence.c or it could be moved under selftests completely?

We can push it into selftests so that it's only built then.
 
> Quick glance - only i915_sw_fence_complete it seems? Could you use 
> i915_sw_fence_commit instead? No idea what the interaction with debug 
> objects there would mean.

Originally kfence exposed both the await / complete semantics. And also
provided a kfence wrapper for a timer. Since then that timer was
absorbed into the external fence wrapper, and I'm not keen on exporting
await / complete functions and choose to export the timer instead.
(Mostly because we already have the timer interaction inside
i915_sw_fence.c so it felt a reasonable extension of that.)
-Chris