Re: [PATCH v2 2/3] drm/i915: Wrap a timer into a i915_sw_fence

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

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@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.

> +{
> +	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 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?

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.

But I think it would be much better to move it under selftests/ if possible.

Regards,

Tvrtko

>   #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
>   #include "selftests/i915_sw_fence.c"
>   #endif
> diff --git a/drivers/gpu/drm/i915/i915_sw_fence.h b/drivers/gpu/drm/i915/i915_sw_fence.h
> index fe2ef4dadfc6..c111a89a927a 100644
> --- a/drivers/gpu/drm/i915/i915_sw_fence.h
> +++ b/drivers/gpu/drm/i915/i915_sw_fence.h
> @@ -61,6 +61,9 @@ void i915_sw_fence_fini(struct i915_sw_fence *fence);
>   static inline void i915_sw_fence_fini(struct i915_sw_fence *fence) {}
>   #endif
>   
> +struct i915_sw_fence *i915_sw_fence_create_timer(long timeout, gfp_t gfp);
> +void i915_sw_fence_timer_flush(struct i915_sw_fence *fence);
> +
>   void i915_sw_fence_commit(struct i915_sw_fence *fence);
>   
>   int i915_sw_fence_await_sw_fence(struct i915_sw_fence *fence,
> diff --git a/drivers/gpu/drm/i915/selftests/i915_sw_fence.c b/drivers/gpu/drm/i915/selftests/i915_sw_fence.c
> index 19d145d6bf52..e51ab4310e1e 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_sw_fence.c
> +++ b/drivers/gpu/drm/i915/selftests/i915_sw_fence.c
> @@ -24,6 +24,7 @@
>   
>   #include <linux/completion.h>
>   #include <linux/delay.h>
> +#include <linux/prime_numbers.h>
>   
>   #include "../i915_selftest.h"
>   
> @@ -565,6 +566,47 @@ static int test_ipc(void *arg)
>   	return ret;
>   }
>   
> +static int test_timer(void *arg)
> +{
> +	struct i915_sw_fence *fence;
> +	unsigned long target, delay;
> +
> +	fence = i915_sw_fence_create_timer(target = jiffies, GFP_KERNEL);
> +	if (!i915_sw_fence_done(fence)) {
> +		pr_err("Fence with immediate expiration not signaled\n");
> +		goto err;
> +	}
> +	i915_sw_fence_timer_flush(fence);
> +
> +	for_each_prime_number(delay, HZ/2) {
> +		fence = i915_sw_fence_create_timer(target = jiffies + delay,
> +						   GFP_KERNEL);
> +		if (i915_sw_fence_done(fence)) {
> +			pr_err("Fence with future expiration (%lu jiffies) already signaled\n", delay);
> +			goto err;
> +		}
> +
> +		i915_sw_fence_wait(fence);
> +		if (!i915_sw_fence_done(fence)) {
> +			pr_err("Fence not signaled after wait\n");
> +			goto err;
> +		}
> +		if (time_before(jiffies, target)) {
> +			pr_err("Fence signaled too early, target=%lu, now=%lu\n",
> +			       target, jiffies);
> +			goto err;
> +		}
> +
> +		i915_sw_fence_timer_flush(fence);
> +	}
> +
> +	return 0;
> +
> +err:
> +	i915_sw_fence_timer_flush(fence);
> +	return -EINVAL;
> +}
> +
>   int i915_sw_fence_mock_selftests(void)
>   {
>   	static const struct i915_subtest tests[] = {
> @@ -576,6 +618,7 @@ int i915_sw_fence_mock_selftests(void)
>   		SUBTEST(test_C_AB),
>   		SUBTEST(test_chain),
>   		SUBTEST(test_ipc),
> +		SUBTEST(test_timer),
>   	};
>   
>   	return i915_subtests(tests, NULL);
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx