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

[Emil Velikov]

On Thu, 25 Apr 2019 at 19:24, Gustaw Smolarczyk <wielkiegie at gmail.com> wrote:
>
> czw., 25 kwi 2019 o 20:11 Gustaw Smolarczyk <wielkiegie at gmail.com> napisał(a):
> >
> > czw., 25 kwi 2019 o 19:42 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.
> > >
> > > v2:
> > >  - use absolute UTC time, as per spec (Gustaw)
> > >  - bail out on cnd_timedwait() failure (Gustaw)
> > >
> > > Cc: Gustaw Smolarczyk <wielkiegie at gmail.com>
> > > Cc: Roland Scheidegger <sroland at vmware.com>
> > > Signed-off-by: Emil Velikov <emil.velikov at collabora.com>
> > > Reviewed-by: Roland Scheidegger <sroland at vmware.com> (v1)
> > > ---
> > >  src/gallium/drivers/llvmpipe/lp_fence.c | 30 +++++++++++++++++++++++++
> > >  src/gallium/drivers/llvmpipe/lp_fence.h |  3 +++
> > >  2 files changed, 33 insertions(+)
> > >
> > > diff --git a/src/gallium/drivers/llvmpipe/lp_fence.c b/src/gallium/drivers/llvmpipe/lp_fence.c
> > > index 20cd91cd63d..b79d773bf6c 100644
> > > --- a/src/gallium/drivers/llvmpipe/lp_fence.c
> > > +++ b/src/gallium/drivers/llvmpipe/lp_fence.c
> > > @@ -125,3 +125,33 @@ lp_fence_wait(struct lp_fence *f)
> > >  }
> > >
> > >
> > > +boolean
> > > +lp_fence_timedwait(struct lp_fence *f, uint64_t timeout)
> > > +{
> > > +   struct timespec ts;
> > > +   int ret;
> > > +
> > > +   timespec_get(&ts, TIME_UTC);
> > > +
> > > +   ts.tv_nsec += timeout % 1000000000L;
> > > +   ts.tv_sec += timeout / 1000000000L;
> > > +   if (ts.tv_nsec >= 1000000000L) {
> > > +      ts.tv_sec++;
> > > +      ts.tv_nsec -= 1000000000L;
> > > +   }
> > > +
> > > +   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);
> > > +      if (ret != thrd_success)
> > > +         break;
> > > +   }
> > > +   mtx_unlock(&f->mutex);
> > > +   return (f->count >= f->rank && ret == thrd_success);
>
> Hmm, you are reading from the fence object outside of the critical
> section, which doesn't sound safe. Maybe compute the return value
> before the mutex is unlocked?
>
>    const boolean result = (f->count >= f->rank);
>    mtx_unlock(&f->mutex);
>    return result;
>
> Since f->rank is immutable and f->count never decreases, it might
> still be ok without this change, though it is racy.
>
In all fairness it shouldn't matters that much but it is the correct
thing regardless.
Will fixup and push in a bit. Thanks for the help Gustaw!

Aside: nearly all of mesa get this and the while loop around
cnd_timedwait wrong :-\
We should really fix that one of these days.

Bonus points: our c11 thread.h has a typo in thrd_timeDout (missing d)
and doesn't set/use it where needed.

-Emil