Hi Chad,
A new patch is attached.
Marek
On Wed, May 27, 2015 at 8:59 PM, Chad Versace <chad.vers...@intel.com> wrote:
> On Wed 13 May 2015, Marek Olšák wrote:
>> From: Marek Olšák <marek.ol...@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'.
From f4fb01a5ebc5ffd11be78fcb9035767e8b0d593c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Ol=C5=A1=C3=A1k?= <marek.ol...@amd.com>
Date: Tue, 12 May 2015 18:14:31 +0200
Subject: [PATCH] egl: add eglGetSyncAttrib (v2)
v2: - don't modify "value" in eglGetSyncAttribKHR after an error
- rename _egl_api::GetSyncAttribKHR -> GetSyncAttrib
- rename GetSyncAttribKHR_t -> GetSyncAttrib_t
- rename _eglGetSyncAttribKHR to _eglGetSyncAttrib
---
src/egl/main/eglapi.c | 25 ++++++++++++++++++++++---
src/egl/main/eglapi.h | 4 ++--
src/egl/main/eglfallbacks.c | 2 +-
src/egl/main/eglsync.c | 4 ++--
src/egl/main/eglsync.h | 4 ++--
5 files changed, 29 insertions(+), 10 deletions(-)
diff --git a/src/egl/main/eglapi.c b/src/egl/main/eglapi.c
index 03a55f1..9696869 100644
--- a/src/egl/main/eglapi.c
+++ b/src/egl/main/eglapi.c
@@ -1427,8 +1427,8 @@ eglSignalSyncKHR(EGLDisplay dpy, EGLSync sync, EGLenum mode)
}
-static EGLBoolean EGLAPIENTRY
-eglGetSyncAttribKHR(EGLDisplay dpy, EGLSync sync, EGLint attribute, EGLint *value)
+EGLBoolean EGLAPIENTRY
+eglGetSyncAttrib(EGLDisplay dpy, EGLSync sync, EGLint attribute, EGLAttrib *value)
{
_EGLDisplay *disp = _eglLockDisplay(dpy);
_EGLSync *s = _eglLookupSync(sync, disp);
@@ -1438,12 +1438,30 @@ eglGetSyncAttribKHR(EGLDisplay dpy, EGLSync sync, EGLint attribute, EGLint *valu
_EGL_CHECK_SYNC(disp, s, EGL_FALSE, drv);
assert(disp->Extensions.KHR_reusable_sync ||
disp->Extensions.KHR_fence_sync);
- ret = drv->API.GetSyncAttribKHR(drv, disp, s, attribute, value);
+ ret = drv->API.GetSyncAttrib(drv, disp, s, attribute, value);
RETURN_EGL_EVAL(disp, ret);
}
+static EGLBoolean EGLAPIENTRY
+eglGetSyncAttribKHR(EGLDisplay dpy, EGLSync sync, EGLint attribute, EGLint *value)
+{
+ EGLAttrib attrib = *value;
+ EGLBoolean result = eglGetSyncAttrib(dpy, sync, attribute, &attrib);
+
+ /* The EGL_KHR_fence_sync spec says this about eglGetSyncAttribKHR:
+ *
+ * If any error occurs, <*value> is not modified.
+ */
+ if (result == EGL_FALSE)
+ return result;
+
+ *value = attrib;
+ return result;
+}
+
+
#ifdef EGL_NOK_swap_region
static EGLBoolean EGLAPIENTRY
@@ -1731,6 +1749,7 @@ eglGetProcAddress(const char *procname)
{ "eglCreateSync", (_EGLProc) eglCreateSync },
{ "eglDestroySync", (_EGLProc) eglDestroySync },
{ "eglClientWaitSync", (_EGLProc) eglClientWaitSync },
+ { "eglGetSyncAttrib", (_EGLProc) eglGetSyncAttrib },
{ "eglWaitSync", (_EGLProc) eglWaitSync },
{ "eglDestroyImage", (_EGLProc) eglDestroyImage },
#ifdef EGL_MESA_drm_display
diff --git a/src/egl/main/eglapi.h b/src/egl/main/eglapi.h
index d2b2eb7..4e0378d 100644
--- a/src/egl/main/eglapi.h
+++ b/src/egl/main/eglapi.h
@@ -96,7 +96,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 (*GetSyncAttrib_t)(_EGLDriver *drv, _EGLDisplay *dpy, _EGLSync *sync, EGLint attribute, EGLAttrib *value);
#ifdef EGL_NOK_swap_region
@@ -178,7 +178,7 @@ struct _egl_api
ClientWaitSyncKHR_t ClientWaitSyncKHR;
WaitSyncKHR_t WaitSyncKHR;
SignalSyncKHR_t SignalSyncKHR;
- GetSyncAttribKHR_t GetSyncAttribKHR;
+ GetSyncAttrib_t GetSyncAttrib;
#ifdef EGL_NOK_swap_region
SwapBuffersRegionNOK_t SwapBuffersRegionNOK;
diff --git a/src/egl/main/eglfallbacks.c b/src/egl/main/eglfallbacks.c
index c44ec6c..3c3701f 100644
--- a/src/egl/main/eglfallbacks.c
+++ b/src/egl/main/eglfallbacks.c
@@ -91,7 +91,7 @@ _eglInitDriverFallbacks(_EGLDriver *drv)
drv->API.ClientWaitSyncKHR = NULL;
drv->API.WaitSyncKHR = NULL;
drv->API.SignalSyncKHR = NULL;
- drv->API.GetSyncAttribKHR = _eglGetSyncAttribKHR;
+ drv->API.GetSyncAttrib = _eglGetSyncAttrib;
#ifdef EGL_MESA_drm_image
drv->API.CreateDRMImageMESA = NULL;
diff --git a/src/egl/main/eglsync.c b/src/egl/main/eglsync.c
index 205cdc0..3019e6e 100644
--- a/src/egl/main/eglsync.c
+++ b/src/egl/main/eglsync.c
@@ -141,8 +141,8 @@ _eglInitSync(_EGLSync *sync, _EGLDisplay *dpy, EGLenum type,
EGLBoolean
-_eglGetSyncAttribKHR(_EGLDriver *drv, _EGLDisplay *dpy, _EGLSync *sync,
- EGLint attribute, EGLint *value)
+_eglGetSyncAttrib(_EGLDriver *drv, _EGLDisplay *dpy, _EGLSync *sync,
+ EGLint attribute, EGLAttrib *value)
{
if (!value)
return _eglError(EGL_BAD_PARAMETER, "eglGetSyncAttribKHR");
diff --git a/src/egl/main/eglsync.h b/src/egl/main/eglsync.h
index 4959cf0..9b2aac8 100644
--- a/src/egl/main/eglsync.h
+++ b/src/egl/main/eglsync.h
@@ -57,8 +57,8 @@ _eglInitSync(_EGLSync *sync, _EGLDisplay *dpy, EGLenum type,
extern EGLBoolean
-_eglGetSyncAttribKHR(_EGLDriver *drv, _EGLDisplay *dpy, _EGLSync *sync,
- EGLint attribute, EGLint *value);
+_eglGetSyncAttrib(_EGLDriver *drv, _EGLDisplay *dpy, _EGLSync *sync,
+ EGLint attribute, EGLAttrib *value);
/**
--
2.1.0
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
