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