[Mesa-dev] [PATCH v2] egl: add EGL_KHR_reusable_sync to egl_dri
dw kim
dongwon.kim at intel.com
Mon Apr 4 17:37:46 UTC 2016
On Mon, Apr 04, 2016 at 01:19:13PM +0200, Marek Olšák wrote:
> This looks good in general. Just some small nitpicks below.
>
> On Sat, Apr 2, 2016 at 1:46 AM, Dongwon Kim <dongwon.kim at intel.com> wrote:
> > This patch enables an EGL extension, EGL_KHR_reusable_sync.
> > This new extension basically provides a way for multiple APIs or
> > threads to be excuted synchronously via a "reusable sync"
> > primitive shared by those threads/API calls.
> >
> > This was implemented based on the specification at
> >
> > https://www.khronos.org/registry/egl/extensions/KHR/EGL_KHR_reusable_sync.txt
> >
> > v2
> > - use thread functions defined in C11/threads.h instead of
> > using direct pthread calls
> > - make the timeout set with reference to CLOCK_MONOTONIC
> > - cleaned up the way expiration time is calculated
> > - (bug fix) in dri2_client_wait_sync, case EGL_SYNC_CL_EVENT_KHR
> > has been added.
> > - (bug fix) in dri2_destroy_sync, return from cond_broadcast
> > call is now stored in 'err' intead of 'ret' to prevent 'ret'
> > from being reset to 'EGL_FALSE' even in successful case
> > - corrected minor syntax problems
> >
> > Signed-off-by: Dongwon Kim <dongwon.kim at intel.com>
> > ---
> > src/egl/drivers/dri2/egl_dri2.c | 210 ++++++++++++++++++++++++++++++++++++++--
> > src/egl/drivers/dri2/egl_dri2.h | 2 +
> > src/egl/main/eglapi.c | 8 ++
> > src/egl/main/eglsync.c | 3 +-
> > 4 files changed, 213 insertions(+), 10 deletions(-)
> >
> > diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
> > index 8f50f0c..843cd53 100644
> > --- a/src/egl/drivers/dri2/egl_dri2.c
> > +++ b/src/egl/drivers/dri2/egl_dri2.c
> > @@ -38,6 +38,8 @@
> > #include <fcntl.h>
> > #include <errno.h>
> > #include <unistd.h>
> > +#include <c11/threads.h>
> > +#include <time.h>
> > #ifdef HAVE_LIBDRM
> > #include <xf86drm.h>
> > #include <drm_fourcc.h>
> > @@ -623,6 +625,8 @@ dri2_setup_screen(_EGLDisplay *disp)
> > disp->Extensions.KHR_cl_event2 = EGL_TRUE;
> > }
> >
> > + disp->Extensions.KHR_reusable_sync = EGL_TRUE;
> > +
> > if (dri2_dpy->image) {
> > if (dri2_dpy->image->base.version >= 10 &&
> > dri2_dpy->image->getCapabilities != NULL) {
> > @@ -2389,14 +2393,22 @@ dri2_egl_ref_sync(struct dri2_egl_sync *sync)
> > p_atomic_inc(&sync->refcount);
> > }
> >
> > -static void
> > +static EGLint
>
> Since this function only returns EGL_TRUE, the return type can just be void.
Thanks for pointing this out. I missed the point that cnd_wait is not returning
any error. I will fix this in v3.
>
> > dri2_egl_unref_sync(struct dri2_egl_display *dri2_dpy,
> > struct dri2_egl_sync *dri2_sync)
> > {
> > if (p_atomic_dec_zero(&dri2_sync->refcount)) {
> > - dri2_dpy->fence->destroy_fence(dri2_dpy->dri_screen, dri2_sync->fence);
> > + if (dri2_sync->base.Type == EGL_SYNC_REUSABLE_KHR) {
> > + cnd_destroy(&dri2_sync->cond);
> > + }
> > +
> > + if (dri2_sync->fence)
> > + dri2_dpy->fence->destroy_fence(dri2_dpy->dri_screen, dri2_sync->fence);
> > +
> > free(dri2_sync);
> > }
> > +
> > + return EGL_TRUE;
> > }
> >
> > static _EGLSync *
> > @@ -2408,6 +2420,8 @@ dri2_create_sync(_EGLDriver *drv, _EGLDisplay *dpy,
> > struct dri2_egl_display *dri2_dpy = dri2_egl_display(dpy);
> > struct dri2_egl_context *dri2_ctx = dri2_egl_context(ctx);
> > struct dri2_egl_sync *dri2_sync;
> > + EGLint ret;
> > + pthread_condattr_t attr;
> >
> > dri2_sync = calloc(1, sizeof(struct dri2_egl_sync));
> > if (!dri2_sync) {
> > @@ -2450,6 +2464,37 @@ dri2_create_sync(_EGLDriver *drv, _EGLDisplay *dpy,
> > dri2_sync->fence, 0, 0))
> > dri2_sync->base.SyncStatus = EGL_SIGNALED_KHR;
> > break;
> > +
> > + case EGL_SYNC_REUSABLE_KHR:
> > + /* intialize attr */
> > + ret = pthread_condattr_init(&attr);
> > +
> > + if (ret) {
> > + _eglError(EGL_BAD_ACCESS, "eglCreateSyncKHR");
> > + free(dri2_sync);
> > + return NULL;
> > + }
> > +
> > + /* change clock attribute to CLOCK_MONOTONIC */
> > + ret = pthread_condattr_setclock(&attr, CLOCK_MONOTONIC);
> > +
> > + if (ret) {
> > + _eglError(EGL_BAD_ACCESS, "eglCreateSyncKHR");
> > + free(dri2_sync);
> > + return NULL;
> > + }
> > +
> > + ret = pthread_cond_init(&dri2_sync->cond, &attr);
> > +
> > + if (ret) {
> > + _eglError(EGL_BAD_ACCESS, "eglCreateSyncKHR");
> > + free(dri2_sync);
> > + return NULL;
> > + }
> > +
> > + /* initial status of reusable sync must be "unsignaled" */
> > + dri2_sync->base.SyncStatus = EGL_UNSIGNALED_KHR;
> > + break;
> > }
> >
> > p_atomic_set(&dri2_sync->refcount, 1);
> > @@ -2461,9 +2506,33 @@ dri2_destroy_sync(_EGLDriver *drv, _EGLDisplay *dpy, _EGLSync *sync)
> > {
> > struct dri2_egl_display *dri2_dpy = dri2_egl_display(dpy);
> > struct dri2_egl_sync *dri2_sync = dri2_egl_sync(sync);
> > + EGLint ret = EGL_TRUE;
> > + EGLint err;
> >
> > - dri2_egl_unref_sync(dri2_dpy, dri2_sync);
> > - return EGL_TRUE;
> > + /* if type of sync is EGL_SYNC_REUSABLE_KHR and it is not signaled yet,
> > + * then unlock all threads possibly blocked by the reusable sync before
> > + * destroying it.
> > + */
> > + if (dri2_sync->base.Type == EGL_SYNC_REUSABLE_KHR &&
> > + dri2_sync->base.SyncStatus == EGL_UNSIGNALED_KHR) {
> > + dri2_sync->base.SyncStatus = EGL_SIGNALED_KHR;
> > + /* unblock all threads currently blocked by sync */
> > + err = cnd_broadcast(&dri2_sync->cond);
> > +
> > + if (err) {
> > + _eglError(EGL_BAD_ACCESS, "eglDestroySyncKHR");
> > + ret = EGL_FALSE;
> > + }
> > + }
> > +
> > + err = dri2_egl_unref_sync(dri2_dpy, dri2_sync);
> > +
> > + if (err == EGL_FALSE) {
> > + _eglError(EGL_BAD_ACCESS, "eglDestroySyncKHR");
> > + ret = EGL_FALSE;
> > + }
> > +
> > + return ret;
> > }
> >
> > static EGLint
> > @@ -2471,11 +2540,18 @@ dri2_client_wait_sync(_EGLDriver *drv, _EGLDisplay *dpy, _EGLSync *sync,
> > EGLint flags, EGLTime timeout)
> > {
> > _EGLContext *ctx = _eglGetCurrentContext();
> > + struct dri2_egl_driver *dri2_drv = dri2_egl_driver(drv);
> > struct dri2_egl_display *dri2_dpy = dri2_egl_display(dpy);
> > struct dri2_egl_context *dri2_ctx = dri2_egl_context(ctx);
> > struct dri2_egl_sync *dri2_sync = dri2_egl_sync(sync);
> > unsigned wait_flags = 0;
> > +
> > + /* timespecs for cnd_timedwait */
> > + struct timespec current;
> > + xtime expire;
> > +
> > EGLint ret = EGL_CONDITION_SATISFIED_KHR;
> > + EGLint err;
> >
> > /* The EGL_KHR_fence_sync spec states:
> > *
> > @@ -2488,17 +2564,132 @@ dri2_client_wait_sync(_EGLDriver *drv, _EGLDisplay *dpy, _EGLSync *sync,
> > /* the sync object should take a reference while waiting */
> > dri2_egl_ref_sync(dri2_sync);
> >
> > - if (dri2_dpy->fence->client_wait_sync(dri2_ctx ? dri2_ctx->dri_context : NULL,
> > + switch (sync->Type) {
> > + case EGL_SYNC_FENCE_KHR:
> > + case EGL_SYNC_CL_EVENT_KHR:
> > + if (dri2_dpy->fence->client_wait_sync(dri2_ctx ? dri2_ctx->dri_context : NULL,
> > dri2_sync->fence, wait_flags,
> > timeout))
> > - dri2_sync->base.SyncStatus = EGL_SIGNALED_KHR;
> > - else
> > - ret = EGL_TIMEOUT_EXPIRED_KHR;
> > + dri2_sync->base.SyncStatus = EGL_SIGNALED_KHR;
> > + else
> > + ret = EGL_TIMEOUT_EXPIRED_KHR;
> > + break;
> > +
> > + case EGL_SYNC_REUSABLE_KHR:
> > + if (dri2_ctx && dri2_sync->base.SyncStatus == EGL_UNSIGNALED_KHR &&
> > + (flags & EGL_SYNC_FLUSH_COMMANDS_BIT_KHR)) {
> > + /* flush context if EGL_SYNC_FLUSH_COMMANDS_BIT_KHR is set */
> > + if (dri2_drv->glFlush)
> > + dri2_drv->glFlush();
> > + }
> > +
> > + /* if timeout is EGL_FOREVER_KHR, it should wait without any timeout.*/
> > + if (timeout == EGL_FOREVER_KHR) {
> > + if (mtx_lock(&dri2_sync->mutex)) {
> > + ret = EGL_FALSE;
> > + goto cleanup;
> > + }
> > +
> > + ret = cnd_wait(&dri2_sync->cond, &dri2_sync->mutex);
> > +
> > + if (mtx_unlock(&dri2_sync->mutex)) {
> > + ret = EGL_FALSE;
> > + goto cleanup;
> > + }
> > +
> > + if (ret) {
> > + _eglError(EGL_BAD_PARAMETER, "eglClientWaitSyncKHR");
> > + ret = EGL_FALSE;
> > + }
> > + } else {
> > + /* if reusable sync has not been yet signaled */
> > + if (dri2_sync->base.SyncStatus != EGL_SIGNALED_KHR) {
> > +
>
> Empty line not needed.
I will remove the line (v3)
>
> > + clock_gettime(CLOCK_MONOTONIC, ¤t);
> > +
> > + /* calculating when to expire */
> > + expire.nsec = timeout%1000000000L;
> > + expire.sec = timeout/1000000000L;
>
> spaces around % and / please.
I will add spaces (v3)
>
> > +
> > + expire.nsec += current.tv_nsec;
> > + expire.sec += current.tv_sec;
> > +
> > + /* expire.nsec should be smaller than 2000000000L */
>
> Did you mean 1000000000?
actually, we have sum of two numbers, timeout % 10^9 and
current.tv_nsec, both of which are smaller than 10^9 in
expire.nsec. This means expire.nsec wouldn't exceed 1999999998
(999999999 + 999999999). Anyway, 2000000000 is not quite right.
It should actually be 1999999999. I will correct the comment in v3.
>
> > + if (expire.nsec > 999999999L) {
> > + expire.sec++;
> > + expire.nsec -= 1000000000L;
> > + }
> > +
> > + if (mtx_lock(&dri2_sync->mutex)) {
> > + ret = EGL_FALSE;
> > + goto cleanup;
> > + }
> > +
> > + ret = cnd_timedwait(&dri2_sync->cond, &dri2_sync->mutex, &expire);
> > +
> > + if (mtx_unlock(&dri2_sync->mutex)) {
> > + ret = EGL_FALSE;
> > + goto cleanup;
> > + }
> > +
> > + if (ret)
> > + if (ret == thrd_busy) {
> > + if (dri2_sync->base.SyncStatus == EGL_UNSIGNALED_KHR) {
> > + ret = EGL_TIMEOUT_EXPIRED_KHR;
> > + } else {
> > + _eglError(EGL_BAD_ACCESS, "eglClientWaitSyncKHR");
> > + ret = EGL_FALSE;
> > + }
> > + }
> > + }
> > + }
> > + break;
> > + }
> > +
> > + cleanup:
> > + err = dri2_egl_unref_sync(dri2_dpy, dri2_sync);
> > +
> > + /* fail to unreference dri2_sync */
> > + if (ret == EGL_FALSE || err == EGL_FALSE) {
> > + _eglError(EGL_BAD_ACCESS, "eglClientWaitSyncKHR");
> > + return EGL_FALSE;
> > + }
> >
> > - dri2_egl_unref_sync(dri2_dpy, dri2_sync);
> > return ret;
> > }
> >
> > +static EGLBoolean
> > +dri2_signal_sync(_EGLDriver *drv, _EGLDisplay *dpy, _EGLSync *sync,
> > + EGLenum mode)
> > +{
> > + struct dri2_egl_sync *dri2_sync = dri2_egl_sync(sync);
> > + EGLint ret;
> > +
> > + if (sync->Type != EGL_SYNC_REUSABLE_KHR) {
> > + _eglError(EGL_BAD_MATCH, "eglSignalSyncKHR");
> > + return EGL_FALSE;
> > + }
> > +
> > + if (mode != EGL_SIGNALED_KHR && mode != EGL_UNSIGNALED_KHR) {
> > + _eglError(EGL_BAD_ATTRIBUTE, "eglSignalSyncKHR");
> > + return EGL_FALSE;
> > + }
> > +
> > + dri2_sync->base.SyncStatus = mode;
> > +
> > + if (mode == EGL_SIGNALED_KHR) {
> > + ret = cnd_broadcast(&dri2_sync->cond);
> > +
> > + /* fail to broadcast */
> > + if (ret) {
> > + _eglError(EGL_BAD_ACCESS, "eglSignalSyncKHR");
> > + return EGL_FALSE;
> > + }
> > + }
> > +
> > + return EGL_TRUE;
> > +}
> > +
> > static EGLint
> > dri2_server_wait_sync(_EGLDriver *drv, _EGLDisplay *dpy, _EGLSync *sync)
> > {
> > @@ -2620,6 +2811,7 @@ _eglBuiltInDriverDRI2(const char *args)
> > dri2_drv->base.API.GetSyncValuesCHROMIUM = dri2_get_sync_values_chromium;
> > dri2_drv->base.API.CreateSyncKHR = dri2_create_sync;
> > dri2_drv->base.API.ClientWaitSyncKHR = dri2_client_wait_sync;
> > + dri2_drv->base.API.SignalSyncKHR = dri2_signal_sync;
> > dri2_drv->base.API.WaitSyncKHR = dri2_server_wait_sync;
> > dri2_drv->base.API.DestroySyncKHR = dri2_destroy_sync;
> >
> > diff --git a/src/egl/drivers/dri2/egl_dri2.h b/src/egl/drivers/dri2/egl_dri2.h
> > index 52ad92b..ef79939 100644
> > --- a/src/egl/drivers/dri2/egl_dri2.h
> > +++ b/src/egl/drivers/dri2/egl_dri2.h
> > @@ -307,6 +307,8 @@ struct dri2_egl_image
> >
> > struct dri2_egl_sync {
> > _EGLSync base;
> > + mtx_t mutex;
> > + cnd_t cond;
> > int refcount;
> > void *fence;
> > };
> > diff --git a/src/egl/main/eglapi.c b/src/egl/main/eglapi.c
> > index dd145a1..6d7fd88 100644
> > --- a/src/egl/main/eglapi.c
> > +++ b/src/egl/main/eglapi.c
> > @@ -1467,6 +1467,14 @@ eglClientWaitSync(EGLDisplay dpy, EGLSync sync, EGLint flags, EGLTime timeout)
> > if (s->SyncStatus == EGL_SIGNALED_KHR)
> > RETURN_EGL_EVAL(disp, EGL_CONDITION_SATISFIED_KHR);
> >
> > + /* if sync type is EGL_SYNC_REUSABLE_KHR, dpy should be
> > + * unlocked here to allow other threads also to be able to
> > + * go into waiting state.
> > + */
> > +
> > + if (s->Type == EGL_SYNC_REUSABLE_KHR)
> > + _eglUnlockDisplay(dpy);
>
> With this, _eglUnlockDisplay is called twice: here and as part of
> RETURN_EGL_EVAL.
I will fix this in v3.
>
> > +
> > ret = drv->API.ClientWaitSyncKHR(drv, disp, s, flags, timeout);
> >
> > RETURN_EGL_EVAL(disp, ret);
>
> How did you test this?
I used DEQP test suite to test basic error handling. Then used
our own 3D test tool to verify client_wait sync with multiple
threads. We tested accuracy of timeout and wait-release mechanism
via reusable sync.
Aside from this, I am planning to port our in-house test to piglit
suite shortly after.
>
> Marek
More information about the mesa-dev
mailing list