[Mesa-dev] [PATCH 1/3] llvmpipe: add lp_fence_timedwait() helper

[Emil Velikov]

On Tue, 16 Apr 2019 at 11:50, Gustaw Smolarczyk <wielkiegie at gmail.com> wrote:
>
> wt., 16 kwi 2019 o 12:11 Emil Velikov <emil.l.velikov at gmail.com> napisał(a):
> >
> > On Thu, 11 Apr 2019 at 17:55, Gustaw Smolarczyk <wielkiegie at gmail.com> wrote:
> > >
> > > czw., 11 kwi 2019 o 18:06 Emil Velikov <emil.l.velikov at gmail.com> napisał(a):
> > > >
> > > > The function is analogous to lp_fence_wait() while taking at timeout
> > > > (ns) parameter, as needed for EGL fence/sync.
> > > >
> > > > Cc: Roland Scheidegger <sroland at vmware.com>
> > > > Signed-off-by: Emil Velikov <emil.velikov at collabora.com>
> > > > ---
> > > >  src/gallium/drivers/llvmpipe/lp_fence.c | 22 ++++++++++++++++++++++
> > > >  src/gallium/drivers/llvmpipe/lp_fence.h |  3 +++
> > > >  2 files changed, 25 insertions(+)
> > > >
> > > > diff --git a/src/gallium/drivers/llvmpipe/lp_fence.c b/src/gallium/drivers/llvmpipe/lp_fence.c
> > > > index 20cd91cd63d..f8b31a9d6a5 100644
> > > > --- a/src/gallium/drivers/llvmpipe/lp_fence.c
> > > > +++ b/src/gallium/drivers/llvmpipe/lp_fence.c
> > > > @@ -125,3 +125,25 @@ lp_fence_wait(struct lp_fence *f)
> > > >  }
> > > >
> > > >
> > > > +boolean
> > > > +lp_fence_timedwait(struct lp_fence *f, uint64_t timeout)
> > > > +{
> > > > +   struct timespec ts = {
> > > > +      .tv_nsec = timeout % 1000000000L,
> > > > +      .tv_sec = timeout / 1000000000L,
> > > > +   };
> > >
> > > According to the documentation [1] and looking at the implementation
> > > in mesa [2], cnd_timedwait accepts an absolute time in UTC, not
> > > duration. It seems that the fence_finish callback accepts duration.
> > >
> > Agreed, not sure how I missed that one.
> >
> > > [1] https://en.cppreference.com/w/c/thread/cnd_timedwait
> > > [2] https://gitlab.freedesktop.org/mesa/mesa/blob/master/include/c11/threads_posix.h#L135
> > >
> > > > +   int ret;
> > > > +
> > > > +   if (LP_DEBUG & DEBUG_FENCE)
> > > > +      debug_printf("%s %d\n", __FUNCTION__, f->id);
> > > > +
> > > > +   mtx_lock(&f->mutex);
> > > > +   assert(f->issued);
> > > > +   while (f->count < f->rank) {
> > > > +      ret = cnd_timedwait(&f->signalled, &f->mutex, &ts);
> > >
> > > Shouldn't ret be checked for thrd_busy here as well? Otherwise, the
> > > function will busy-wait after the timeout is reached instead of
> > > returning.
> > >
> > Actually this should be tweaked to:
> >  - only wait for the requested timeout
> >  - _regardless_ of how meany threads (and hence ranks) llvmpipe has
> >
> > Something like the following (warning pre-coffee code) should work.
> >
> >    mtx_lock(&f->mutex);
> >    assert(f->issued);
> >    if (f->count < f->rank)
> >       ret = cnd_timedwait(&f->signalled, &f->mutex, &ts);
> >
> >    mtx_unlock(&f->mutex);
> >    return (f->count >= f->rank && ret == thrd_success);
> >
>
> Is it a good idea not to perform cnd_timedwait in a loop? A spurious
> wake up could could shorten the wait time.
>
Thanks Gustaw - had no idea cnd_timedwait can spuriously return.

For anyone else wondering - the C11 spec does not mention anything,
but there's a bug [1] to fix that.
Underlying implementations (such as pthreads [2]) are already pretty
clear about this "interesting" behaviour.

Will send out and updated revision in a moment.

Thanks again.
Emil

[1] http://www.open-std.org/jtc1/sc22/wg14/www/docs/summary.htm#dr_480
[2] https://linux.die.net/man/3/pthread_cond_timedwait