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

["Christian König" <ckoenig.leichtzumerken@gmail.com>]

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.

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;