Re: [Mesa-dev] [PATCH] egl: adds EGL_KHR_reusable_sync to egl_dri

[Marek Olšák]

On Tue, Mar 22, 2016 at 1:06 AM, dw kim  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  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 
>> > ---
>> >  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 
>> >  #include 
>> >  #include 
>> > +#include 
>> > +#include 
>> >  #ifdef HAVE_LIBDRM
>> >  #include 
>> >  #include 
>> > @@ -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(>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(_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(_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) {
>> > + 

Re: [Mesa-dev] [PATCH] egl: adds EGL_KHR_reusable_sync to egl_dri

[dw kim]

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  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 
> > ---
> >  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 
> >  #include 
> >  #include 
> > +#include 
> > +#include 
> >  #ifdef HAVE_LIBDRM
> >  #include 
> >  #include 
> > @@ -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(>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(_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(_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 

Re: [Mesa-dev] [PATCH] egl: adds EGL_KHR_reusable_sync to egl_dri

[Marek Olšák]

On Wed, Mar 9, 2016 at 2:28 AM, Dongwon Kim  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 
> ---
>  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 
>  #include 
>  #include 
> +#include 
> +#include 
>  #ifdef HAVE_LIBDRM
>  #include 
>  #include 
> @@ -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(>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(_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(_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.

> +  }
> +   }
> +
> +   err = dri2_egl_unref_sync(dri2_dpy,