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....@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....@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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev