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

[Tomasz Figa]

Hi Roland,

On Thu, Jan 10, 2019 at 1:36 AM Roland Scheidegger <sroland at vmware.com> wrote:
>
> Sorry but I had to revert this, as we've seen lots of assertion failures
> (src/gallium/drivers/llvmpipe/lp_fence.c:120: lp_fence_wait: Assertion
> `f->issued' failed.). For instance with libgl_xlib state tracker and piglit.
> I'm not entirely sure if it's really safe to just remove the assert or

Understood. Sorry for not spotting this in my testing. Would you be
able to help with more details on how to reproduce these failures?

> if it needs some more work, and I don't have time right now for a
> thorough investigation, but I'll happily take new patches...

Perhaps we could make these dummy fences "issued". I'll check how this
works in the code.

Best regards,
Tomasz

>
> Roland
>
>
> Am 09.01.19 um 02:09 schrieb Roland Scheidegger:
> > Am 07.01.19 um 09:54 schrieb Tomasz Figa:
> >> On Sun, Dec 23, 2018 at 12:55 AM Roland Scheidegger <sroland at vmware.com> wrote:
> >>>
> >>> Alright, I guess it should work...
> >>>
> >>> Reviewed-by: Roland Scheidegger <sroland at vmware.com>
> >>>
> >>
> >> Thanks!
> >>
> >> Would we have anyone who could help to commit it?
> >
> > Pushed (albeit I forgot the R-b on it, ah well...)
> >
> > Roland
> >
> >>
> >> (I know that I was supposed to apply for commit rights, but I expect
> >> my contribution rate to be relatively low, due to a shift to different
> >> areas, so I don't think I'm a good candidate for a committer anymore.)
> >>
> >> Best regards,
> >> Tomasz
> >>
> >>>
> >>> Am 14.12.18 um 09:17 schrieb Tomasz Figa:
> >>>> 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://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fandroid.googlesource.com%2Fplatform%2Fframeworks%2Fbase%2F%2B%2Fmaster%2Flibs%2Fhwui%2Fpipeline%2Fskia%2FSkiaOpenGLPipeline.cpp%23427&data=02%7C01%7Csroland%40vmware.com%7Ccb06f4e1c9164a7871cb08d675cf20c7%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636825929820495738&sdata=6hmZk%2BXWaQk%2B5XAKjxFSybOSpCVwzvKemYgZQ1rtpvg%3D&reserved=0
> >>>>
> >>>> Reproducible especially with thread count >= 4.
> >>>>
> >>>> One could make the driver always keep the reference to the last fence,
> >>>> but:
> >>>>
> >>>>  - the driver seems to explicitly destroy the fence whenever a rendering
> >>>>    pass completes and changing that would require a significant functional
> >>>>    change to the code. (Specifically, in lp_scene_end_rasterization().)
> >>>>
> >>>>  - it 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.
> >>>>
> >>>> Therefore, the simple approach of always creating a fence is taken,
> >>>> similarly to other drivers, such as radeonsi.
> >>>>
> >>>> Tested with piglit llvmpipe suite with no regressions and following
> >>>> tests fixed:
> >>>>
> >>>> egl_khr_fence_sync
> >>>>  conformance
> >>>>   eglclientwaitsynckhr_flag_sync_flush
> >>>>   eglclientwaitsynckhr_nonzero_timeout
> >>>>   eglclientwaitsynckhr_zero_timeout
> >>>>   eglcreatesynckhr_default_attributes
> >>>>   eglgetsyncattribkhr_invalid_attrib
> >>>>   eglgetsyncattribkhr_sync_status
> >>>>
> >>>> v2:
> >>>>  - remove the useless lp_fence_reference() dance (Nicolai),
> >>>>  - explain why creating the dummy fence is the right approach.
> >>>>
> >>>> Signed-off-by: Tomasz Figa <tfiga at chromium.org>
> >>>> ---
> >>>>  src/gallium/drivers/llvmpipe/lp_setup.c | 2 ++
> >>>>  1 file changed, 2 insertions(+)
> >>>>
> >>>> diff --git a/src/gallium/drivers/llvmpipe/lp_setup.c b/src/gallium/drivers/llvmpipe/lp_setup.c
> >>>> index b087369473..e72e119c8a 100644
> >>>> --- a/src/gallium/drivers/llvmpipe/lp_setup.c
> >>>> +++ b/src/gallium/drivers/llvmpipe/lp_setup.c
> >>>> @@ -361,6 +361,8 @@ lp_setup_flush( struct lp_setup_context *setup,
> >>>>
> >>>>     if (fence) {
> >>>>        lp_fence_reference((struct lp_fence **)fence, setup->last_fence);
> >>>> +      if (!*fence)
> >>>> +         *fence = (struct pipe_fence_handle *)lp_fence_create(0);
> >>>>     }
> >>>>  }
> >>>>
> >>>>
> >>>
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fmesa-dev&data=02%7C01%7Csroland%40vmware.com%7Ccb06f4e1c9164a7871cb08d675cf20c7%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636825929820495738&sdata=FeFqwQi9AUVDWaUw7lLMtAci6wWjE44vqwwjVwysY3o%3D&reserved=0
> >
>