[Intel-gfx] [PATCH] drm/syncobj: flatten dma_fence_chains on transfer

[Lionel Landwerlin]

On 25/05/2022 12:26, Lionel Landwerlin wrote:
> On 25/05/2022 11:24, Christian König wrote:
>> Am 25.05.22 um 08:47 schrieb Lionel Landwerlin:
>>> On 09/02/2022 20:26, Christian König wrote:
>>>> It is illegal to add a dma_fence_chain as timeline point. Flatten out
>>>> the fences into a dma_fence_array instead.
>>>>
>>>> Signed-off-by: Christian König <christian.koenig at amd.com>
>>>> ---
>>>>   drivers/gpu/drm/drm_syncobj.c | 61 
>>>> ++++++++++++++++++++++++++++++++---
>>>>   1 file changed, 56 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_syncobj.c 
>>>> b/drivers/gpu/drm/drm_syncobj.c
>>>> index c313a5b4549c..7e48dcd1bee4 100644
>>>> --- a/drivers/gpu/drm/drm_syncobj.c
>>>> +++ b/drivers/gpu/drm/drm_syncobj.c
>>>> @@ -853,12 +853,57 @@ drm_syncobj_fd_to_handle_ioctl(struct 
>>>> drm_device *dev, void *data,
>>>>                       &args->handle);
>>>>   }
>>>>   +
>>>> +/*
>>>> + * Try to flatten a dma_fence_chain into a dma_fence_array so that 
>>>> it can be
>>>> + * added as timeline fence to a chain again.
>>>> + */
>>>> +static int drm_syncobj_flatten_chain(struct dma_fence **f)
>>>> +{
>>>> +    struct dma_fence_chain *chain = to_dma_fence_chain(*f);
>>>> +    struct dma_fence *tmp, **fences;
>>>> +    struct dma_fence_array *array;
>>>> +    unsigned int count;
>>>> +
>>>> +    if (!chain)
>>>> +        return 0;
>>>> +
>>>> +    count = 0;
>>>> +    dma_fence_chain_for_each(tmp, &chain->base)
>>>> +        ++count;
>>>> +
>>>> +    fences = kmalloc_array(count, sizeof(*fences), GFP_KERNEL);
>>>> +    if (!fences)
>>>> +        return -ENOMEM;
>>>> +
>>>> +    count = 0;
>>>> +    dma_fence_chain_for_each(tmp, &chain->base)
>>>> +        fences[count++] = dma_fence_get(tmp);
>>>> +
>>>> +    array = dma_fence_array_create(count, fences,
>>>> +                       dma_fence_context_alloc(1),
>>>
>>>
>>> Hi Christian,
>>>
>>>
>>> Sorry for the late answer to this.
>>>
>>>
>>> It appears this commit is trying to remove the warnings added by 
>>> "dma-buf: Warn about dma_fence_chain container rules"
>>
>> Yes, correct. We are now enforcing some rules with warnings and this 
>> here bubbled up.
>>
>>>
>>> But the context allocation you added just above is breaking some 
>>> tests. In particular igt at syncobj_timeline@transfer-timeline-point
>>>
>>> That test transfer points into the timeline at point 3 and expects 
>>> that we'll still on the previous points to complete.
>>
>> Hui what? I don't understand the problem you are seeing here. What 
>> exactly is the test doing?
>>
>>>
>>> In my opinion we should be reusing the previous context number if 
>>> there is one and only allocate if we don't have a point.
>>
>> Scratching my head what you mean with that. The functionality 
>> transfers a synchronization fence from one timeline to another.
>>
>> So as far as I can see the new point should be part of the timeline 
>> of the syncobj we are transferring to.
>>
>>> If the application wants to not depend on previous points for wait 
>>> operations, it can reset the syncobj prior to adding a new point.
>>
>> Well we should never lose synchronization. So what happens is that 
>> when we do the transfer all the fences of the source are flattened 
>> out into an array. And that array is then added as new point into the 
>> destination timeline.
>
>
> In this case would be broken :
>
>
> syncobjA <- signal point 1
>
> syncobjA <- import syncobjB point 1 into syncobjA point 2
>
> syncobjA <- query returns 0
>
>
> -Lionel


Err... Let's double check with my colleagues.

It seems we're running into a test failure in IGT with this patch, but 
now I have doubts that it's where the problem lies.


-Lionel


>
>
>>
>> Where exactly is the problem?
>>
>> Regards,
>> Christian.
>>
>>>
>>>
>>>
>>> Cheers,
>>>
>>>
>>> -Lionel
>>>
>>>
>>>
>>>> +                       1, false);
>>>> +    if (!array)
>>>> +        goto free_fences;
>>>> +
>>>> +    dma_fence_put(*f);
>>>> +    *f = &array->base;
>>>> +    return 0;
>>>> +
>>>> +free_fences:
>>>> +    while (count--)
>>>> +        dma_fence_put(fences[count]);
>>>> +
>>>> +    kfree(fences);
>>>> +    return -ENOMEM;
>>>> +}
>>>> +
>>>>   static int drm_syncobj_transfer_to_timeline(struct drm_file 
>>>> *file_private,
>>>>                           struct drm_syncobj_transfer *args)
>>>>   {
>>>>       struct drm_syncobj *timeline_syncobj = NULL;
>>>> -    struct dma_fence *fence;
>>>>       struct dma_fence_chain *chain;
>>>> +    struct dma_fence *fence;
>>>>       int ret;
>>>>         timeline_syncobj = drm_syncobj_find(file_private, 
>>>> args->dst_handle);
>>>> @@ -869,16 +914,22 @@ static int 
>>>> drm_syncobj_transfer_to_timeline(struct drm_file *file_private,
>>>>                        args->src_point, args->flags,
>>>>                        &fence);
>>>>       if (ret)
>>>> -        goto err;
>>>> +        goto err_put_timeline;
>>>> +
>>>> +    ret = drm_syncobj_flatten_chain(&fence);
>>>> +    if (ret)
>>>> +        goto err_free_fence;
>>>> +
>>>>       chain = dma_fence_chain_alloc();
>>>>       if (!chain) {
>>>>           ret = -ENOMEM;
>>>> -        goto err1;
>>>> +        goto err_free_fence;
>>>>       }
>>>> +
>>>>       drm_syncobj_add_point(timeline_syncobj, chain, fence, 
>>>> args->dst_point);
>>>> -err1:
>>>> +err_free_fence:
>>>>       dma_fence_put(fence);
>>>> -err:
>>>> +err_put_timeline:
>>>>       drm_syncobj_put(timeline_syncobj);
>>>>         return ret;
>>>
>>>
>>
>