On Tue, Mar 22, 2016 at 1:06 AM, dw kim <dongwon....@intel.com> wrote: > On Mon, Mar 21, 2016 at 08:35:20PM +0100, Marek Olšák wrote: >> On Wed, Mar 9, 2016 at 2:28 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 >> > >> > Signed-off-by: Dongwon Kim <dongwon....@intel.com> >> > --- >> > src/egl/drivers/dri2/egl_dri2.c | 197 >> > ++++++++++++++++++++++++++++++++++++++-- >> > src/egl/drivers/dri2/egl_dri2.h | 2 + >> > src/egl/main/eglapi.c | 8 ++ >> > src/egl/main/eglsync.c | 3 +- >> > 4 files changed, 200 insertions(+), 10 deletions(-) >> > >> > diff --git a/src/egl/drivers/dri2/egl_dri2.c >> > b/src/egl/drivers/dri2/egl_dri2.c >> > index 8f50f0c..78164e4 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 <pthread.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,33 @@ dri2_egl_ref_sync(struct dri2_egl_sync *sync) >> > p_atomic_inc(&sync->refcount); >> > } >> > >> > -static void >> > +static EGLint >> > dri2_egl_unref_sync(struct dri2_egl_display *dri2_dpy, >> > struct dri2_egl_sync *dri2_sync) >> > { >> > + EGLint ret; >> > + >> > if (p_atomic_dec_zero(&dri2_sync->refcount)) { >> > - dri2_dpy->fence->destroy_fence(dri2_dpy->dri_screen, >> > dri2_sync->fence); >> > + /* mutex and cond should be freed if not freed yet. */ >> > + if (dri2_sync->mutex) >> > + free(dri2_sync->mutex); >> > + >> > + if (dri2_sync->cond) { >> > + ret = pthread_cond_destroy(dri2_sync->cond); >> > + >> > + if (ret) >> > + return EGL_FALSE; >> > + >> > + free(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 +2431,7 @@ 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; >> > >> > dri2_sync = calloc(1, sizeof(struct dri2_egl_sync)); >> > if (!dri2_sync) { >> > @@ -2450,6 +2474,23 @@ 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: >> > + dri2_sync->cond = calloc(1, sizeof(pthread_cond_t)); >> > + dri2_sync->mutex = calloc(1, sizeof(pthread_mutex_t)); >> > + ret = pthread_cond_init(dri2_sync->cond, NULL); >> > + >> > + if (ret) { >> > + _eglError(EGL_BAD_PARAMETER, "eglCreateSyncKHR"); >> > + free(dri2_sync->cond); >> > + free(dri2_sync->mutex); >> > + 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 +2502,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 */ >> > + ret = pthread_cond_broadcast(dri2_sync->cond); >> > + >> > + if (ret) { >> > + _eglError(EGL_BAD_PARAMETER, "eglDestroySyncKHR"); >> > + ret = EGL_FALSE; >> >> BAD_PARAMETER is not the correct error for a pthread_cond_broadcast >> failure. I'm not sure, but I think we shouldn't return an error in >> this case, since the specification doesn't define an error for it. >> According to pthread documentation, this error shouldn't occur if >> dri2_sync->cond is valid, thus it shouldn't be an issue right? >> >> Same for error handling of other pthread functions. > > Don't we need to make it fail anyway if pthread function > gets an error? What about just get it return EGL_FALSE > without any error code? Is it allowed here?
Technically, it would be an out-of-spec behavior, because EGL specifications don't list this case as a possible source of failures. If we want to return an error here, I think the most accurate one is EGL_BAD_ACCESS. > >> >> > + } >> > + } >> > + >> > + err = dri2_egl_unref_sync(dri2_dpy, dri2_sync); >> > + >> > + if (err == EGL_FALSE) { >> > + _eglError(EGL_BAD_PARAMETER, "eglDestroySyncKHR"); >> > + ret = EGL_FALSE; >> > + } >> > + >> > + return ret; >> > } >> > >> > static EGLint >> > @@ -2471,11 +2536,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 pthread_cond_timedwait */ >> > + struct timespec current; >> > + struct timespec expired; >> > + >> > EGLint ret = EGL_CONDITION_SATISFIED_KHR; >> > + EGLint err; >> > >> > /* The EGL_KHR_fence_sync spec states: >> > * >> > @@ -2488,17 +2560,123 @@ 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: >> >> This should also be executed for EGL_SYNC_CL_EVENT_KHR. >> > > Will fix this in version 2 > >> > + 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; >> >> No need to add an empty line here: >> > > Will be fixed in version 2 > >> > + >> > + 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 (pthread_mutex_lock(dri2_sync->mutex)) { >> > + ret = EGL_FALSE; >> > + goto cleanup; >> > + } >> > + >> > + ret = pthread_cond_wait(dri2_sync->cond, dri2_sync->mutex); >> > + >> > + if (pthread_mutex_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) { >> >> Missing spaces around !=. >> > > Will be fixed in version 2 > >> > + >> > + clock_gettime(CLOCK_REALTIME, ¤t); >> >> We should use CLOCK_MONOTONIC everywhere. >> > > Will be fixed in version 2 > >> > + >> > + expired.tv_nsec = current.tv_nsec + timeout; >> >> This needs to handle integer overflows, otherwise you can get negative >> tv_nsec. Also, tv_nsec only has 32 bits on 32-bit CPUs, which should >> also be dealt with. >> > > Will be fixed in version 2 > >> > + expired.tv_sec = current.tv_sec + expired.tv_nsec/1000000000L; >> > + expired.tv_nsec = current.tv_nsec%1000000000L; >> > + >> > + if (pthread_mutex_lock(dri2_sync->mutex)) { >> > + ret = EGL_FALSE; >> > + goto cleanup; >> > + } >> > + >> > + ret = pthread_cond_timedwait(dri2_sync->cond, >> > dri2_sync->mutex, &expired); >> > + >> > + if (pthread_mutex_unlock(dri2_sync->mutex)) { >> > + ret = EGL_FALSE; >> > + goto cleanup; >> > + } >> > + >> > + if (ret) >> > + if (ret == ETIMEDOUT) { >> > + if (dri2_sync->base.SyncStatus==EGL_UNSIGNALED_KHR) { >> > + ret = EGL_TIMEOUT_EXPIRED_KHR; >> > + } else { >> > + _eglError(EGL_BAD_PARAMETER, "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_PARAMETER, "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) { >> >> Missing spaces around !=. >> > > Will be fixed in version 2 > >> > + _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 = pthread_cond_broadcast(dri2_sync->cond); >> > + >> > + /* fail to broadcast */ >> > + if (ret) { >> > + _eglError(EGL_BAD_PARAMETER, "eglSignalSyncKHR"); >> > + return EGL_FALSE; >> > + } >> > + } >> > + >> > + return EGL_TRUE; >> > +} >> > + >> > static EGLint >> > dri2_server_wait_sync(_EGLDriver *drv, _EGLDisplay *dpy, _EGLSync *sync) >> > { >> > @@ -2620,6 +2798,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..f70f384 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; >> > + pthread_mutex_t *mutex; >> > + pthread_cond_t *cond; >> >> The types should be mtx_t and cnd_t from c11/threads.h. The >> corresponding functions from c11/threads.h should be used too. > > Does this mean I need to replace all pthread functions that > I used here with things defined in threads.h intead? I can do > it but can I know why we can't use pthread.h? libEGL already uses c11/threads.h instead of pthreads. I don't know the reason for that, but it would be good to follow that for consistency. > >> >> Also, declaring the variables as pointers seems unnecessary. Instead >> of checking the pointers against NULL, you can declare them as normal >> non-pointer variables and check (base.Type == EGL_SYNC_REUSABLE_KHR) >> instead. > > I thought we can save space by definiting those as pointers. > (basically we don't have to allocate space for mutex and cond > if sync type is not reusable) That's true, but is this unmeasurable memory saving really worth the additional code complexity? Marek _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev