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

[Lionel Landwerlin <lionel.g.landwerlin@intel.com>]

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.


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