[PATCH] drm/syncobj: Deal with signalled fences in transfer.

[Christian König]

Am 07.12.21 um 08:10 schrieb Lionel Landwerlin:
> On 07/12/2021 03:32, Bas Nieuwenhuizen wrote:
>> See the comments in the code. Basically if the seqno is already
>> signalled then we get a NULL fence. If we then put the NULL fence
>> in a binary syncobj it counts as unsignalled, making that syncobj
>> pretty much useless for all expected uses.
>>
>> Not 100% sure about the transfer to a timeline syncobj but I
>> believe it is needed there too, as AFAICT the add_point function
>> assumes the fence isn't NULL.
>>
>> Fixes: ea569910cbab ("drm/syncobj: add transition iotcls between 
>> binary and timeline v2")
>> Cc: stable at vger.kernel.org
>> Signed-off-by: Bas Nieuwenhuizen <bas at basnieuwenhuizen.nl>
>> ---
>>   drivers/gpu/drm/drm_syncobj.c | 26 ++++++++++++++++++++++++++
>>   1 file changed, 26 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_syncobj.c 
>> b/drivers/gpu/drm/drm_syncobj.c
>> index fdd2ec87cdd1..eb28a40400d2 100644
>> --- a/drivers/gpu/drm/drm_syncobj.c
>> +++ b/drivers/gpu/drm/drm_syncobj.c
>> @@ -861,6 +861,19 @@ static int 
>> drm_syncobj_transfer_to_timeline(struct drm_file *file_private,
>>                        &fence);
>>       if (ret)
>>           goto err;
>> +
>> +    /* If the requested seqno is already signaled 
>> drm_syncobj_find_fence may
>> +     * return a NULL fence. To make sure the recipient gets 
>> signalled, use
>> +     * a new fence instead.
>> +     */
>> +    if (!fence) {
>> +        fence = dma_fence_allocate_private_stub();
>> +        if (!fence) {
>> +            ret = -ENOMEM;
>> +            goto err;
>> +        }
>> +    }
>> +
>
>
> Shouldn't we fix drm_syncobj_find_fence() instead?

Mhm, now that you mention it. Bas, why do you think that 
dma_fence_chain_find_seqno() may return NULL when the fence is already 
signaled?

Double checking the code that should never ever happen.

Regards,
Christian.

>
> By returning a stub fence for the timeline case if there isn't one.
>
>
> Because the same NULL fence check appears missing in amdgpu (and 
> probably other drivers).
>
>
> Also we should have tests for this in IGT.
>
> AMD contributed some tests when this code was written but they never 
> got reviewed :(
>
>
> -Lionel
>
>
>>       chain = kzalloc(sizeof(struct dma_fence_chain), GFP_KERNEL);
>>       if (!chain) {
>>           ret = -ENOMEM;
>> @@ -890,6 +903,19 @@ drm_syncobj_transfer_to_binary(struct drm_file 
>> *file_private,
>>                        args->src_point, args->flags, &fence);
>>       if (ret)
>>           goto err;
>> +
>> +    /* If the requested seqno is already signaled 
>> drm_syncobj_find_fence may
>> +     * return a NULL fence. To make sure the recipient gets 
>> signalled, use
>> +     * a new fence instead.
>> +     */
>> +    if (!fence) {
>> +        fence = dma_fence_allocate_private_stub();
>> +        if (!fence) {
>> +            ret = -ENOMEM;
>> +            goto err;
>> +        }
>> +    }
>> +
>>       drm_syncobj_replace_fence(binary_syncobj, fence);
>>       dma_fence_put(fence);
>>   err:
>
>