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

[Tomasz Figa]

Hi Nicolai,

On Thu, Nov 22, 2018 at 6:19 PM Haehnle, Nicolai
<Nicolai.Haehnle at 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 <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);

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