Hi Nicolai, On Thu, Nov 22, 2018 at 6:19 PM Haehnle, Nicolai <nicolai.haeh...@amd.com> wrote: > > 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);
Indeed, there is no need for this reference dance, not sure what I had in mind when writing that. Let me respin a simplified version. Thanks for feedback! Best regards, Tomasz _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
