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

[Dongwon Kim]

Sure, I will. Thanks

On Tue, Aug 23, 2016 at 11:39:52AM +0100, Emil Velikov wrote:
> Hi Dongwon,
> 
> On 15 August 2016 at 23:20, Dongwon Kim  wrote:
> > A new patch, "[PATCH] egl/dri2: remove error checks on return values from 
> > mtx_lock
> > and cnd_wait" containing additional clean-up has been submitted. Please 
> > disregard
> > this one.
> >
> Thanks for the update. For the future please add this is information
> in the new patch (after the --- line). Using [PATCH v2] and revision
> log would also be appreciated.
> 
> -Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Emil Velikov]

Hi Dongwon,

On 15 August 2016 at 23:20, Dongwon Kim  wrote:
> A new patch, "[PATCH] egl/dri2: remove error checks on return values from 
> mtx_lock
> and cnd_wait" containing additional clean-up has been submitted. Please 
> disregard
> this one.
>
Thanks for the update. For the future please add this is information
in the new patch (after the --- line). Using [PATCH v2] and revision
log would also be appreciated.

-Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Dongwon Kim]

A new patch, "[PATCH] egl/dri2: remove error checks on return values from 
mtx_lock
and cnd_wait" containing additional clean-up has been submitted. Please 
disregard 
this one.

On Thu, Jul 28, 2016 at 02:38:35PM -0700, Dongwon Kim 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 
> ---
>  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(_sync->mutex)) {
> -ret = EGL_FALSE;
> -goto cleanup;
> - }
> + mtx_lock(_sync->mutex);
>  
>   ret = cnd_wait(_sync->cond, _sync->mutex);
>  
> @@ -2609,10 +2606,7 @@ dri2_client_wait_sync(_EGLDriver *drv, _EGLDisplay 
> *dpy, _EGLSync *sync,
> expire.nsec -= 10L;
>  }
>  
> -if (mtx_lock(_sync->mutex)) {
> -   ret = EGL_FALSE;
> -   goto cleanup;
> -}
> +mtx_lock(_sync->mutex);
>  
>  ret = cnd_timedwait(_sync->cond, _sync->mutex, 
> );
>  
> @@ -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;
>  }
>  
>  static EGLBoolean
> -- 
> 2.7.4
> 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Dongwon Kim]

Emil,

I just submitted a new patch "[PATCH] egl/dri2: remove error checks on 
return values from mtx_lock and cnd_wait" that replaces the old one.
Please review the new one and disregard the original one, "egl/dri2: 
Do not need to check return value from mtx_lock".  

On Mon, Aug 15, 2016 at 02:45:04PM +0100, Emil Velikov wrote:
> Hi Kim,
> 
> On 28 July 2016 at 22:38, Dongwon Kim  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 
> > ---
> >  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(_sync->mutex)) {
> > -ret = EGL_FALSE;
> > -goto cleanup;
> > - }
> > + mtx_lock(_sync->mutex);
> >
> >   ret = cnd_wait(_sync->cond, _sync->mutex);
> >
> > @@ -2609,10 +2606,7 @@ dri2_client_wait_sync(_EGLDriver *drv, _EGLDisplay 
> > *dpy, _EGLSync *sync,
> > expire.nsec -= 10L;
> >  }
> >
> > -if (mtx_lock(_sync->mutex)) {
> > -   ret = EGL_FALSE;
> > -   goto cleanup;
> > -}
> > +mtx_lock(_sync->mutex);
> >
> >  ret = cnd_timedwait(_sync->cond, _sync->mutex, 
> > );
> >
> > @@ -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 
> 
> Thanks
> Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [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  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 
> > ---
> >  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(_sync->mutex)) {
> > -ret = EGL_FALSE;
> > -goto cleanup;
> > - }
> > + mtx_lock(_sync->mutex);
> >
> >   ret = cnd_wait(_sync->cond, _sync->mutex);
> >
> > @@ -2609,10 +2606,7 @@ dri2_client_wait_sync(_EGLDriver *drv, _EGLDisplay 
> > *dpy, _EGLSync *sync,
> > expire.nsec -= 10L;
> >  }
> >
> > -if (mtx_lock(_sync->mutex)) {
> > -   ret = EGL_FALSE;
> > -   goto cleanup;
> > -}
> > +mtx_lock(_sync->mutex);
> >
> >  ret = cnd_timedwait(_sync->cond, _sync->mutex, 
> > );
> >
> > @@ -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 
> 
> Thanks
> Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Emil Velikov]

Hi Kim,

On 28 July 2016 at 22:38, Dongwon Kim  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 
> ---
>  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(_sync->mutex)) {
> -ret = EGL_FALSE;
> -goto cleanup;
> - }
> + mtx_lock(_sync->mutex);
>
>   ret = cnd_wait(_sync->cond, _sync->mutex);
>
> @@ -2609,10 +2606,7 @@ dri2_client_wait_sync(_EGLDriver *drv, _EGLDisplay 
> *dpy, _EGLSync *sync,
> expire.nsec -= 10L;
>  }
>
> -if (mtx_lock(_sync->mutex)) {
> -   ret = EGL_FALSE;
> -   goto cleanup;
> -}
> +mtx_lock(_sync->mutex);
>
>  ret = cnd_timedwait(_sync->cond, _sync->mutex, 
> );
>
> @@ -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 

Thanks
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev