Re: [Mesa-dev] [PATCH 1/3] llvmpipe: add lp_fence_timedwait() helper
On Tue, 16 Apr 2019 at 11:50, Gustaw Smolarczyk wrote:
>
> wt., 16 kwi 2019 o 12:11 Emil Velikov napisał(a):
> >
> > On Thu, 11 Apr 2019 at 17:55, Gustaw Smolarczyk
> > wrote:
> > >
> > > czw., 11 kwi 2019 o 18:06 Emil Velikov
> > > 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
> > > > Signed-off-by: Emil Velikov
> > > > ---
> > > > 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 % 10L,
> > > > + .tv_sec = timeout / 10L,
> > > > + };
> > >
> > > 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(>mutex);
> > > > + assert(f->issued);
> > > > + while (f->count < f->rank) {
> > > > + ret = cnd_timedwait(>signalled, >mutex, );
> > >
> > > 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(>mutex);
> >assert(f->issued);
> >if (f->count < f->rank)
> > ret = cnd_timedwait(>signalled, >mutex, );
> >
> >mtx_unlock(>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
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/3] llvmpipe: add lp_fence_timedwait() helper
wt., 16 kwi 2019 o 12:11 Emil Velikov napisał(a):
>
> On Thu, 11 Apr 2019 at 17:55, Gustaw Smolarczyk wrote:
> >
> > czw., 11 kwi 2019 o 18:06 Emil Velikov
> > 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
> > > Signed-off-by: Emil Velikov
> > > ---
> > > 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 % 10L,
> > > + .tv_sec = timeout / 10L,
> > > + };
> >
> > 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(>mutex);
> > > + assert(f->issued);
> > > + while (f->count < f->rank) {
> > > + ret = cnd_timedwait(>signalled, >mutex, );
> >
> > 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(>mutex);
>assert(f->issued);
>if (f->count < f->rank)
> ret = cnd_timedwait(>signalled, >mutex, );
>
>mtx_unlock(>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 for the review :-)
> -Emil
Regards,
Gustaw Smolarczyk
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/3] llvmpipe: add lp_fence_timedwait() helper
On Thu, 11 Apr 2019 at 17:55, Gustaw Smolarczyk wrote:
>
> czw., 11 kwi 2019 o 18:06 Emil Velikov 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
> > Signed-off-by: Emil Velikov
> > ---
> > 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 % 10L,
> > + .tv_sec = timeout / 10L,
> > + };
>
> 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(>mutex);
> > + assert(f->issued);
> > + while (f->count < f->rank) {
> > + ret = cnd_timedwait(>signalled, >mutex, );
>
> 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(>mutex);
assert(f->issued);
if (f->count < f->rank)
ret = cnd_timedwait(>signalled, >mutex, );
mtx_unlock(>mutex);
return (f->count >= f->rank && ret == thrd_success);
Thanks for the review :-)
-Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/3] llvmpipe: add lp_fence_timedwait() helper
Looks correct to me.
For the series,
Reviewed-by: Roland Scheidegger
Am 11.04.19 um 18:05 schrieb Emil Velikov:
> The function is analogous to lp_fence_wait() while taking at timeout
> (ns) parameter, as needed for EGL fence/sync.
>
> Cc: Roland Scheidegger
> Signed-off-by: Emil Velikov
> ---
> 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 % 10L,
> + .tv_sec = timeout / 10L,
> + };
> + int ret;
> +
> + if (LP_DEBUG & DEBUG_FENCE)
> + debug_printf("%s %d\n", __FUNCTION__, f->id);
> +
> + mtx_lock(>mutex);
> + assert(f->issued);
> + while (f->count < f->rank) {
> + ret = cnd_timedwait(>signalled, >mutex, );
> + }
> + mtx_unlock(>mutex);
> + return ret == thrd_success;
> +}
> +
> +
> diff --git a/src/gallium/drivers/llvmpipe/lp_fence.h
> b/src/gallium/drivers/llvmpipe/lp_fence.h
> index b72026492c6..5ba746d22d1 100644
> --- a/src/gallium/drivers/llvmpipe/lp_fence.h
> +++ b/src/gallium/drivers/llvmpipe/lp_fence.h
> @@ -65,6 +65,9 @@ lp_fence_signalled(struct lp_fence *fence);
> void
> lp_fence_wait(struct lp_fence *fence);
>
> +boolean
> +lp_fence_timedwait(struct lp_fence *fence, uint64_t timeout);
> +
> void
> llvmpipe_init_screen_fence_funcs(struct pipe_screen *screen);
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/3] llvmpipe: add lp_fence_timedwait() helper
czw., 11 kwi 2019 o 18:06 Emil Velikov 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
> Signed-off-by: Emil Velikov
> ---
> 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 % 10L,
> + .tv_sec = timeout / 10L,
> + };
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.
[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(>mutex);
> + assert(f->issued);
> + while (f->count < f->rank) {
> + ret = cnd_timedwait(>signalled, >mutex, );
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.
Regards,
Gustaw Smolarczyk
> + }
> + mtx_unlock(>mutex);
> + return ret == thrd_success;
> +}
> +
> +
> diff --git a/src/gallium/drivers/llvmpipe/lp_fence.h
> b/src/gallium/drivers/llvmpipe/lp_fence.h
> index b72026492c6..5ba746d22d1 100644
> --- a/src/gallium/drivers/llvmpipe/lp_fence.h
> +++ b/src/gallium/drivers/llvmpipe/lp_fence.h
> @@ -65,6 +65,9 @@ lp_fence_signalled(struct lp_fence *fence);
> void
> lp_fence_wait(struct lp_fence *fence);
>
> +boolean
> +lp_fence_timedwait(struct lp_fence *fence, uint64_t timeout);
> +
> void
> llvmpipe_init_screen_fence_funcs(struct pipe_screen *screen);
>
> --
> 2.20.1
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 1/3] llvmpipe: add lp_fence_timedwait() helper
The function is analogous to lp_fence_wait() while taking at timeout
(ns) parameter, as needed for EGL fence/sync.
Cc: Roland Scheidegger
Signed-off-by: Emil Velikov
---
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 % 10L,
+ .tv_sec = timeout / 10L,
+ };
+ int ret;
+
+ if (LP_DEBUG & DEBUG_FENCE)
+ debug_printf("%s %d\n", __FUNCTION__, f->id);
+
+ mtx_lock(>mutex);
+ assert(f->issued);
+ while (f->count < f->rank) {
+ ret = cnd_timedwait(>signalled, >mutex, );
+ }
+ mtx_unlock(>mutex);
+ return ret == thrd_success;
+}
+
+
diff --git a/src/gallium/drivers/llvmpipe/lp_fence.h
b/src/gallium/drivers/llvmpipe/lp_fence.h
index b72026492c6..5ba746d22d1 100644
--- a/src/gallium/drivers/llvmpipe/lp_fence.h
+++ b/src/gallium/drivers/llvmpipe/lp_fence.h
@@ -65,6 +65,9 @@ lp_fence_signalled(struct lp_fence *fence);
void
lp_fence_wait(struct lp_fence *fence);
+boolean
+lp_fence_timedwait(struct lp_fence *fence, uint64_t timeout);
+
void
llvmpipe_init_screen_fence_funcs(struct pipe_screen *screen);
--
2.20.1
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
