Hi Emil, I don't think I still have a chance to update this to version 4 since this has already been pushed according to Marek... However, I will follow up your suggestion with a separate patch once this is completely merged..
-DW On Tue, Apr 05, 2016 at 03:09:41PM +0100, Emil Velikov wrote: > Hi Dongwon, > > On 5 April 2016 at 01:14, 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 > > > > v3 > > - dri2_egl_unref_sync now became 'void' type. No more error check > > is needed for this function call as a result. > > - (bug fix) resolved issue with duplicated unlocking of display in > > eglClientWaitSync when type of sync is "EGL_KHR_REUSABLE_SYNC" > > > > Signed-off-by: Dongwon Kim <dongwon....@intel.com> > > --- > > src/egl/drivers/dri2/egl_dri2.c | 192 > > ++++++++++++++++++++++++++++++++++++++-- > > src/egl/drivers/dri2/egl_dri2.h | 2 + > > src/egl/main/eglapi.c | 17 +++- > > src/egl/main/eglsync.c | 3 +- > > 4 files changed, 206 insertions(+), 8 deletions(-) > > > > diff --git a/src/egl/drivers/dri2/egl_dri2.c > > b/src/egl/drivers/dri2/egl_dri2.c > > index 8f50f0c..490b040 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) { > > @@ -2394,7 +2398,12 @@ 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); > > } > > } > > @@ -2408,6 +2417,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 +2461,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 */ > typo -> initialize > > > + 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 +2503,27 @@ 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; > > + > > + /* 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; > > + } > > + } > > dri2_egl_unref_sync(dri2_dpy, dri2_sync); > > - return EGL_TRUE; > > + > > + return ret; > > } > > > > static EGLint > > @@ -2471,10 +2531,16 @@ 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; > > + > Please squash the above whitespace. > > > EGLint ret = EGL_CONDITION_SATISFIED_KHR; > > > > /* The EGL_KHR_fence_sync spec states: > > @@ -2488,17 +2554,130 @@ 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)) { > It's quite admirable that you've added the lock/unlock error checking. > Then again all of mesa (some 200+ cases of lock/unlock) don't bother, > so I'd just drop them for now. > > > + /* if reusable sync has not been yet signaled */ > > + if (dri2_sync->base.SyncStatus != EGL_SIGNALED_KHR) { > > + clock_gettime(CLOCK_MONOTONIC, ¤t); > > + > > + /* calculating when to expire */ > > + expire.nsec = timeout % 1000000000L; > > + expire.sec = timeout / 1000000000L; > > + > > + expire.nsec += current.tv_nsec; > > + expire.sec += current.tv_sec; > > + > > + /* expire.nsec now is a number between 0 and 1999999998 */ > > + if (expire.nsec > 999999999L) { > > + expire.sec++; > > + expire.nsec -= 1000000000L; > Albeit unlikely to change these soon, they feel error prone. Might > replacing them with 1000 * 1000 * 1000L derived equivalents ? > > > + } > > + > > + 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; > Afaics this is wrong. The eglClientWaitSyncKHR does not return > EGL_TRUE/FALSE but > EGL_TIMEOUT_EXPIRED_KHR/EGL_CONDITION_SATISFIED_KHR. > > > + } > > + } > > + } > > + } > > + break; > > + } > > + > > + cleanup: > > dri2_egl_unref_sync(dri2_dpy, dri2_sync); > > + > > + if (ret == EGL_FALSE) { > > + _eglError(EGL_BAD_ACCESS, "eglClientWaitSyncKHR"); > > + return EGL_FALSE; > > + } > > + > With the above two suggestions (removed lock/unlock error handling and > feeding the correct value to ret, this hunk can be dropped. > > Thanks for the contribution. > -Emil _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
