[Mesa-dev] [PATCH 11/15] egl: add eglGetSyncAttrib

[Marek Olšák]

Hi Chad,

A new patch is attached.

Marek

On Wed, May 27, 2015 at 8:59 PM, Chad Versace <chad.versace at intel.com> wrote:
> On Wed 13 May 2015, Marek Olšák wrote:
>> From: Marek Olšák <marek.olsak at amd.com>
>>
>> ---
>>  src/egl/main/eglapi.c  | 14 +++++++++++++-
>>  src/egl/main/eglapi.h  |  2 +-
>>  src/egl/main/eglsync.c |  2 +-
>>  src/egl/main/eglsync.h |  2 +-
>>  4 files changed, 16 insertions(+), 4 deletions(-)
>>
>
> I see two issues below.
>
>> diff --git a/src/egl/main/eglapi.c b/src/egl/main/eglapi.c
>> index 544f7e4..6457798 100644
>> --- a/src/egl/main/eglapi.c
>> +++ b/src/egl/main/eglapi.c
>
>
>> @@ -1558,6 +1559,17 @@ eglGetSyncAttribKHR(EGLDisplay dpy, EGLSync sync, EGLint attribute, EGLint *valu
>>  }
>>
>>
>> +EGLBoolean EGLAPIENTRY
>> +eglGetSyncAttribKHR(EGLDisplay dpy, EGLSync sync, EGLint attribute, EGLint *value)
>> +{
>> +   EGLAttrib attrib = *value;
>> +   EGLBoolean result = eglGetSyncAttrib(dpy, sync, attribute, &attrib);
>> +
>> +   *value = attrib;
>> +   return result;
>> +}
>
> This change violates the EGL_KHR_fence_sync spec, which says this
> regarding eglGetSyncAttribKHR:
>
>     If any error occurs, <*value> is not modified.
>
> I think it's a good idea to add this quote to the function body so that
> future devs don't accidentally make the same mistake.
>
>
>> diff --git a/src/egl/main/eglapi.h b/src/egl/main/eglapi.h
>> index 44f589d..fdd3b05 100644
>> --- a/src/egl/main/eglapi.h
>> +++ b/src/egl/main/eglapi.h
>> @@ -91,7 +91,7 @@ typedef EGLBoolean (*DestroySyncKHR_t)(_EGLDriver *drv, _EGLDisplay *dpy, _EGLSy
>>  typedef EGLint (*ClientWaitSyncKHR_t)(_EGLDriver *drv, _EGLDisplay *dpy, _EGLSync *sync, EGLint flags, EGLTime timeout);
>>  typedef EGLint (*WaitSyncKHR_t)(_EGLDriver *drv, _EGLDisplay *dpy, _EGLSync *sync);
>>  typedef EGLBoolean (*SignalSyncKHR_t)(_EGLDriver *drv, _EGLDisplay *dpy, _EGLSync *sync, EGLenum mode);
>> -typedef EGLBoolean (*GetSyncAttribKHR_t)(_EGLDriver *drv, _EGLDisplay *dpy, _EGLSync *sync, EGLint attribute, EGLint *value);
>> +typedef EGLBoolean (*GetSyncAttribKHR_t)(_EGLDriver *drv, _EGLDisplay *dpy, _EGLSync *sync, EGLint attribute, EGLAttrib *value);
>
> I think changing this change is potentially dangerous, because
> post-patch GetSyncAttribKHR_t has the signature of eglGetSyncAttrib and
> *not* eglGetSyncAttribKHR. The code will compile, but this discrepancy
> looks like a honeypot for accidental function misuse.
>
> And...
>
>>  #ifdef EGL_NOK_swap_region
>> diff --git a/src/egl/main/eglsync.c b/src/egl/main/eglsync.c
>> index 205cdc0..a09d991 100644
>> --- a/src/egl/main/eglsync.c
>> +++ b/src/egl/main/eglsync.c
>> @@ -142,7 +142,7 @@ _eglInitSync(_EGLSync *sync, _EGLDisplay *dpy, EGLenum type,
>>
>>  EGLBoolean
>>  _eglGetSyncAttribKHR(_EGLDriver *drv, _EGLDisplay *dpy, _EGLSync *sync,
>> -                     EGLint attribute, EGLint *value)
>> +                     EGLint attribute, EGLAttrib *value)
>
> I have the same issue here. Let's avoid introducing confusing
> discrepancies between function names and their signatures. Just name
> this one and the typedef to '_eglGetSyncAttrib' and 'GetSyncAttrib_t'.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-egl-add-eglGetSyncAttrib-v2.patch
Type: text/x-patch
Size: 5414 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150528/5b807d99/attachment.bin>