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

[Lionel Landwerlin]

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


>
> 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;
>>
>>
>