[Mesa-dev] [PATCH] egl/dri2: Do not need to check return value from mtx_lock

[Dongwon Kim]

Yes, you are right. It is duplicate EGL_FALSE check and _eglError
call in the end of the function.. Also, I don't see any good reason
to check return value of cnd_wait (though we still need to check it in
timedwait case.). I will prepare a new patch with all of these 
taken into acocunt.

On Mon, Aug 15, 2016 at 02:45:04PM +0100, Emil Velikov wrote:
> Hi Kim,
> 
> On 28 July 2016 at 22:38, Dongwon Kim <dongwon.kim at intel.com> wrote:
> > This removes unnecessary error checks on return result of mtx_lock
> > calls as in all other places in MESA source since there is no chance
> > that mtx_lock returns any of error codes in current implementation.
> >
> > Signed-off-by: Dongwon Kim <dongwon.kim at intel.com>
> > ---
> >  src/egl/drivers/dri2/egl_dri2.c | 21 ++++++---------------
> >  1 file changed, 6 insertions(+), 15 deletions(-)
> >
> > diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
> > index ac2be86..26b80d4 100644
> > --- a/src/egl/drivers/dri2/egl_dri2.c
> > +++ b/src/egl/drivers/dri2/egl_dri2.c
> > @@ -2578,10 +2578,7 @@ dri2_client_wait_sync(_EGLDriver *drv, _EGLDisplay *dpy, _EGLSync *sync,
> >
> >        /* 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;
> > -         }
> > +         mtx_lock(&dri2_sync->mutex);
> >
> >           ret = cnd_wait(&dri2_sync->cond, &dri2_sync->mutex);
> >
> > @@ -2609,10 +2606,7 @@ dri2_client_wait_sync(_EGLDriver *drv, _EGLDisplay *dpy, _EGLSync *sync,
> >                 expire.nsec -= 1000000000L;
> >              }
> >
> > -            if (mtx_lock(&dri2_sync->mutex)) {
> > -               ret = EGL_FALSE;
> > -               goto cleanup;
> > -            }
> > +            mtx_lock(&dri2_sync->mutex);
> >
> >              ret = cnd_timedwait(&dri2_sync->cond, &dri2_sync->mutex, &expire);
> >
> > @@ -2632,15 +2626,12 @@ dri2_client_wait_sync(_EGLDriver *drv, _EGLDisplay *dpy, _EGLSync *sync,
> >        break;
> >    }
> >
> > - cleanup:
> > -   dri2_egl_unref_sync(dri2_dpy, dri2_sync);
> > +  dri2_egl_unref_sync(dri2_dpy, dri2_sync);
> >
> > -   if (ret == EGL_FALSE) {
> > -      _eglError(EGL_BAD_ACCESS, "eglClientWaitSyncKHR");
> > -      return EGL_FALSE;
> > -   }
> > +  if (ret == EGL_FALSE)
> > +     _eglError(EGL_BAD_ACCESS, "eglClientWaitSyncKHR");
> >
> > -   return ret;
> > +  return ret;
> Patch looks great and although looking more into it - the original
> implementation is calling _eglError multiple times.
> Initially as the errors are detected and secondly in the cleanup path.
> Can we drop either one please ?
> 
> Aside: all the users of cnd_wait (its gallium wrapper or the core
> pthread function) don't bother checking the return value. Worth
> keeping/dropping the check ?
> 
> Regardless of which one(s) get nuked (or the cnd_wait comment) the patch is
> Reviewed-by: Emil Velikov <emil.velikov at collabora.com>
> 
> Thanks
> Emil