[PATCH 01/10] dma-buf/fence: add fence_wait_any_timeout function

[Christian König]

On 26.10.2015 14:18, Maarten Lankhorst wrote:
> Op 26-10-15 om 13:29 schreef Christian König:
>> From: Christian König <christian.koenig at amd.com>
>>
>> Waiting for the first fence in an array of fences to signal.
>>
>> This is useful for device driver specific resource managers
>> and also Vulkan needs something similar.
>>
>> Signed-off-by: Christian König <christian.koenig at amd.com>
>> Reviewed-by: Alex Deucher <alexander.deucher at amd.com>
>> ---
>>   drivers/dma-buf/fence.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++++
>>   include/linux/fence.h   |  3 +-
>>   2 files changed, 98 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/dma-buf/fence.c b/drivers/dma-buf/fence.c
>> index 50ef8bd..218623f 100644
>> --- a/drivers/dma-buf/fence.c
>> +++ b/drivers/dma-buf/fence.c
>> @@ -397,6 +397,102 @@ out:
>>   }
>>   EXPORT_SYMBOL(fence_default_wait);
>>   
>> +static bool
>> +fence_test_signaled_any(struct fence **fences, uint32_t count)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < count; ++i) {
>> +		struct fence *fence = fences[i];
>> +		if (test_bit(FENCE_FLAG_SIGNALED_BIT, &fence->flags))
>> +			return true;
>> +	}
>> +	return false;
>> +}
>> +
>> +/**
>> + * fence_wait_any_timeout - sleep until any fence gets signaled
>> + * or until timeout elapses
>> + * @fences:	[in]	array of fences to wait on
>> + * @count:	[in]	number of fences to wait on
>> + * @intr:	[in]	if true, do an interruptible wait
>> + * @timeout:	[in]	timeout value in jiffies, or MAX_SCHEDULE_TIMEOUT
>> + *
>> + * Returns -EINVAL on custom fence wait implementation, -ERESTARTSYS if
>> + * interrupted, 0 if the wait timed out, or the remaining timeout in jiffies
>> + * on success.
>> + *
>> + * Synchronous waits for the first fence in the array to be signaled. The
>> + * caller needs to hold a reference to all fences in the array, otherwise a
>> + * fence might be freed before return, resulting in undefined behavior.
>> + */
>> +signed long
>> +fence_wait_any_timeout(struct fence **fences, uint32_t count,
>> +		       bool intr, signed long timeout)
>> +{
>> +	struct default_wait_cb *cb;
>> +	signed long ret = timeout;
>> +	unsigned i;
>> +
>> +	if (WARN_ON(!fences))
>> +		return -EINVAL;
> This should probably have a check for count == 0 before this WARN_ON, so it doesn't wait an infinite amount of time when count == 0.
> And it also needs to special case timeout == 0 even before that, so it still returns 1 in that case to be compatible with other wait functions.

Makes sense, going to add the extra checks.

>
>> +	cb = kcalloc(count, sizeof(struct default_wait_cb), GFP_KERNEL);
>> +	if (cb == NULL) {
>> +		ret = -ENOMEM;
>> +		goto err_free_cb;
>> +	}
>> +
>> +	for (i = 0; i < count; ++i) {
>> +		struct fence *fence = fences[i];
>> +
>> +		if (fence == NULL)
>> +			continue;
> Not sure that NULL fences should be handled here.. and in the amdgpu case surely if fence is null that means
> you don't need to wait for any fence to be signaled?

Good point.

The use case in amdgpu is that we know how many fences we will maximum 
have (one per engine). So we allocate the array on the stack and fill in 
each slot with the earliest fence we can find, some slots can obviously 
be unused and so NULL.

Thought about removing that as well by accumulating the fences before 
calling the function, but couldn't find a good argument for doing so. 
That we wait forever if all fences are NULL is obviously a good argument.

What do you prefer that we accumulate the fence in amdgpu before calling 
the function or an extra check here that return -EINVAL if all fences 
are NULL?

>> +
>> +		if (fence->ops->wait != fence_default_wait) {
>> +			ret = -EINVAL;
>> +			goto fence_rm_cb;
>> +		}
>> +
>> +		cb[i].task = current;
>> +		if (fence_add_callback(fence, &cb[i].base,
>> +				       fence_default_wait_cb)) {
>> +			/* This fence is already signaled */
>> +			goto fence_rm_cb;
>> +		}
>> +	}
>> +
>> +	while (ret > 0) {
>> +		if (intr)
>> +			set_current_state(TASK_INTERRUPTIBLE);
>> +		else
>> +			set_current_state(TASK_UNINTERRUPTIBLE);
>> +
>> +		if (fence_test_signaled_any(fences, count))
>> +			break;
>> +
>> +		ret = schedule_timeout(ret);
>> +
>> +		if (ret > 0 && intr && signal_pending(current))
>> +			ret = -ERESTARTSYS;
>> +	}
>> +
>> +	__set_current_state(TASK_RUNNING);
>> +
>> +fence_rm_cb:
>> +	for (i = 0; i < count; ++i) {
> If i is not used elsewhere then the fence_remove_callback could be made
> unconditional and the loop changed to
> while (i-- > 0) {

Yeah, going to change that as well.

Regards,
Christian.

>> +		struct fence *fence = fences[i];
>> +		if (fence && cb[i].base.func)
>> +			fence_remove_callback(fence, &cb[i].base);
>> +	}
>> +
>> +err_free_cb:
>> +	kfree(cb);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL(fence_wait_any_timeout);
>> +
>>   /**
>>    * fence_init - Initialize a custom fence.
>>    * @fence:	[in]	the fence to initialize
>> diff --git a/include/linux/fence.h b/include/linux/fence.h
>> index 39efee1..a4084d6 100644
>> --- a/include/linux/fence.h
>> +++ b/include/linux/fence.h
>> @@ -305,7 +305,8 @@ static inline struct fence *fence_later(struct fence *f1, struct fence *f2)
>>   }
>>   
>>   signed long fence_wait_timeout(struct fence *, bool intr, signed long timeout);
>> -
>> +signed long fence_wait_any_timeout(struct fence **fences, uint32_t count,
>> +				   bool intr, signed long timeout);
>>   
>>   /**
>>    * fence_wait - sleep until the fence gets signaled