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

["Christian König" <deathsimple@vodafone.de>]

> 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?
Forget that question, I realized that the current check in amdgpu for 
this doesn't work any more anyway (even without this patch).

I've just send an updated version of the patch to you, please review and 
comment.

Thanks,
Christian.

On 26.10.2015 14:45, Christian König wrote:
> 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@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@amd.com>
>>> Reviewed-by: Alex Deucher <alexander.deucher@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
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel