[Mesa-dev] [PATCH] egl: adds EGL_KHR_reusable_sync to egl_dri
Marek Olšák
maraeo at gmail.com
Tue Mar 22 15:59:19 UTC 2016
On Tue, Mar 22, 2016 at 1:06 AM, dw kim <dongwon.kim at 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.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
>> >
>> > Signed-off-by: Dongwon Kim <dongwon.kim at 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
More information about the mesa-dev
mailing list