[Mesa-dev] [PATCH] llvmpipe: Always return some fence in flush

[Haehnle, Nicolai]

On 22.11.18 06:40, Tomasz Figa wrote:
> Hi Brian, Keith,
> 
> +Some more Chromium folks for visibility.
> 
> On Wed, Aug 22, 2018 at 4:21 PM Tomasz Figa <tfiga at chromium.org> wrote:
>>
>> Hi Michel,
>>
>> On Thu, Aug 16, 2018 at 6:43 PM Michel Dänzer <michel at daenzer.net> wrote:
>>>
>>> On 2018-08-16 11:34 AM, Tomasz Figa wrote:
>>>> If there is no last fence, due to no rendering happening yet, just
>>>> create a new signaled fence and return it, to match the expectations of
>>>> the EGL sync fence API.
>>>>
>>>> Fixes random "Could not create sync fence 0x3003" assertion failures from
>>>> Skia on Android, coming from the following code:
>>>>
>>>> https://android.googlesource.com/platform/frameworks/base/+/master/libs/hwui/pipeline/skia/SkiaOpenGLPipeline.cpp#427
>>>>
>>>> Reproducible especially with thread count >= 4.
>>>>
>>>> Signed-off-by: Tomasz Figa <tfiga at chromium.org>
>>>> ---
>>>>   src/gallium/drivers/llvmpipe/lp_setup.c | 8 +++++++-
>>>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/src/gallium/drivers/llvmpipe/lp_setup.c b/src/gallium/drivers/llvmpipe/lp_setup.c
>>>> index b087369473..a6f1b54d69 100644
>>>> --- a/src/gallium/drivers/llvmpipe/lp_setup.c
>>>> +++ b/src/gallium/drivers/llvmpipe/lp_setup.c
>>>> @@ -360,7 +360,13 @@ lp_setup_flush( struct lp_setup_context *setup,
>>>>      set_scene_state( setup, SETUP_FLUSHED, reason );
>>>>
>>>>      if (fence) {
>>>> -      lp_fence_reference((struct lp_fence **)fence, setup->last_fence);
>>>> +      struct lp_fence *lp_fence = NULL;
>>>> +
>>>> +      lp_fence_reference(&lp_fence, setup->last_fence);
>>>> +      if (!lp_fence)
>>>> +         lp_fence = lp_fence_create(0);
>>>> +      lp_fence_reference((struct lp_fence **)fence, lp_fence);
>>>> +      lp_fence_reference(&lp_fence, NULL);
>>>>      }
>>>>   }
>>>>
>>>>
>>>
>>> Other drivers keep around a reference to the last fence in the context,
>>> and return that if there's no new work to flush.
>>
>> Thanks for taking a look.
>>
>> Right, it sounds like a sane thing to do. LLVMpipe, however, seems to
>> explicitly destroy the fence whenever a rendering pass completes and I
>> didn't want to change that without understanding the intentions behind
>> that. Precisely, it's lp_scene_end_rasterization():
>> https://gitlab.freedesktop.org/mesa/mesa/blob/master/src/gallium/drivers/llvmpipe/lp_scene.c#L292
>>
>> Also, this still wouldn't solve the problem of an EGL sync fence being
>> created and waited on without any rendering happening at all, which is
>> also likely to happen with Android code pointed to in the commit
>> message. Obviously that could be dealt with by creating a signaled
>> fence in lp_setup_create(), though.
>>
>> Let me add Keith and Brian for more visibility.
> 
> Any thoughts on this?

Your analysis seems correct to me.

Note that I wouldn't worry too much about creating a new fence object. 
radeonsi creates a new Gallium pipe fence object on every call to 
pipe->flush, regardless of the state of the system. This is because 
internally, radeonsi's Gallium pipe fences are a union of an SDMA and a 
GFX fence.

So creating a new fence, as your patch does, should be perfectly fine if 
you know that all previous work has finished.

I do think your patch is needlessly convoluted though. Why not just

        lp_fence_reference((struct lp_fence **)fence, setup->last_fence);
+      if (!*fence)
+         *fence = (struct pipe_fence_handle *)lp_fence_create(0);

Cheers,
Nicolai

> 
> Best regards,
> Tomasz
>