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 <tf...@chromium.org> wrote: >> >> Hi Michel, >> >> On Thu, Aug 16, 2018 at 6:43 PM Michel Dänzer <mic...@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 <tf...@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 > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev