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

[Lionel Landwerlin]

On 30/05/2022 14:40, Christian König wrote:
> Am 30.05.22 um 12:09 schrieb Lionel Landwerlin:
>> On 30/05/2022 12:52, Christian König wrote:
>>> Am 25.05.22 um 23:59 schrieb Lucas De Marchi:
>>>> On Wed, May 25, 2022 at 12:38:51PM +0200, Christian König wrote:
>>>>> Am 25.05.22 um 11:35 schrieb Lionel Landwerlin:
>>>>>> [SNIP]
>>>>>>
>>>>>> 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.
>>>>>
>>>>> Yeah, exactly that's what I couldn't understand as well.
>>>>>
>>>>> What you describe above should still work fine.
>>>>>
>>>>> Thanks for taking a look into this,
>>>>> Christian.
>>>>
>>>> With some additional prints:
>>>>
>>>> [  210.742634] Console: switching to colour dummy device 80x25
>>>> [  210.742686] [IGT] syncobj_timeline: executing
>>>> [  210.756988] [IGT] syncobj_timeline: starting subtest 
>>>> transfer-timeline-point
>>>> [  210.757364] [drm:drm_syncobj_transfer_ioctl] *ERROR* adding 
>>>> fence0 signaled=1
>>>> [  210.764543] [drm:drm_syncobj_transfer_ioctl] *ERROR* resulting 
>>>> array fence signaled=0
>>>> [  210.800469] [IGT] syncobj_timeline: exiting, ret=98
>>>> [  210.825426] Console: switching to colour frame buffer device 240x67
>>>>
>>>>
>>>> still learning this part of the code but AFAICS the problem is because
>>>> when we are creating the array, the 'signaled' doesn't propagate to 
>>>> the
>>>> array.
>>>
>>> Yeah, but that is intentionally. The array should only signal when 
>>> requested.
>>>
>>> I still don't get what the test case here is checking.
>>
>>
>> There must be something I don't know about fence arrays.
>>
>> You seem to say that creating an array of signaled fences will not 
>> make the array signaled.
>
> Exactly that, yes. The array delays it's signaling until somebody asks 
> for it.
>
> In other words the fences inside the array are check only after 
> someone calls dma_fence_enable_sw_signaling() which in turn calls 
> dma_fence_array_enable_signaling().
>
> It is certainly possible that nobody does that in the drm_syncobj and 
> because of this the array never signals.
>
> Regards,
> Christian.


Thanks,


Yeah I guess dma_fence_enable_sw_signaling() is never called for sw_sync.

Don't we also want to call it right at the end of 
drm_syncobj_flatten_chain() ?


-Lionel


>
>>
>>
>> This is the situation with this IGT test.
>>
>> We started with a syncobj with point 1 & 2 signaled.
>>
>> We take point 2 and import it as a new point 3 on the same syncobj.
>>
>> We expect point 3 to be signaled as well and it's not.
>>
>>
>> Thanks,
>>
>>
>> -Lionel
>>
>>
>>>
>>> Regards,
>>> Christian.
>>>
>>>>
>>>> dma_fence_array_create() {
>>>>     ...
>>>>     atomic_set(&array->num_pending, signal_on_any ? 1 : num_fences);
>>>>     ...
>>>> }
>>>>
>>>> This is not considering the fact that some of the fences could already
>>>> have been signaled as is the case in the 
>>>> igt at syncobj_timeline@transfer-timeline-point
>>>> test. See 
>>>> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11693/shard-dg1-12/igt@syncobj_timeline@transfer-timeline-point.html
>>>>
>>>> Quick patch on this function fixes it for me:
>>>>
>>>> ---------8<----------------
>>>> Subject: [PATCH] dma-buf: Honor already signaled fences on array 
>>>> creation
>>>>
>>>> When creating an array, array->num_pending is marked with the 
>>>> number of
>>>> fences. However the fences could alredy have been signaled. Propagate
>>>> num_pending to the array by looking at each individual fence the array
>>>> contains.
>>>>
>>>> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>>>> ---
>>>>  drivers/dma-buf/dma-fence-array.c | 11 ++++++++++-
>>>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/dma-buf/dma-fence-array.c 
>>>> b/drivers/dma-buf/dma-fence-array.c
>>>> index 5c8a7084577b..32f491c32fa0 100644
>>>> --- a/drivers/dma-buf/dma-fence-array.c
>>>> +++ b/drivers/dma-buf/dma-fence-array.c
>>>> @@ -158,6 +158,8 @@ struct dma_fence_array 
>>>> *dma_fence_array_create(int num_fences,
>>>>  {
>>>>      struct dma_fence_array *array;
>>>>      size_t size = sizeof(*array);
>>>> +    unsigned num_pending = 0;
>>>> +    struct dma_fence **f;
>>>>
>>>>      WARN_ON(!num_fences || !fences);
>>>>
>>>> @@ -173,7 +175,14 @@ struct dma_fence_array 
>>>> *dma_fence_array_create(int num_fences,
>>>>      init_irq_work(&array->work, irq_dma_fence_array_work);
>>>>
>>>>      array->num_fences = num_fences;
>>>> -    atomic_set(&array->num_pending, signal_on_any ? 1 : num_fences);
>>>> +
>>>> +    for (f = fences; f < fences + num_fences; f++)
>>>> +        num_pending += !dma_fence_is_signaled(*f);
>>>> +
>>>> +    if (signal_on_any)
>>>> +        num_pending = !!num_pending;
>>>> +
>>>> +    atomic_set(&array->num_pending, num_pending);
>>>>      array->fences = fences;
>>>>
>>>>      array->base.error = PENDING_ERROR;
>>>
>>
>