[Mesa-dev] [PATCH] egl: Add GetSyncValuesCHROMIUM extension.
Ian Romanick
idr at freedesktop.org
Fri Apr 18 16:49:43 PDT 2014
On 04/18/2014 03:37 PM, Sarah Sharp wrote:
> Chromium defined a new GL extension (that isn't registered with Khronos).
> We need to add an EGL extension for it, so we can migrate ChromeOS on
> Intel systems to use EGL instead of GLX.
>
> http://git.chromium.org/gitweb/?p=chromium/src/third_party/khronos.git;a=commitdiff;h=27cbfdab35c601f70aa150581ad1448d0401f447
>
> The extension is similar to the GLX extension OML_sync_control, but only
> defines one function, eglGetSyncValuesCHROMIUM, which is equivalent to
> glXGetSyncValuesOML.
>
> http://www.opengl.org/registry/specs/OML/glx_sync_control.txt
>
> One open question:
>
> I've used the normal (error checked) version of xcb_dri2_get_msc. The
> GLX implementation of glXGetSyncValuesOML uses the unchecked version,
> but I'm not convinced it's necessary.
>
> I talked to Jamey Sharp, and he doesn't understand why most of Mesa
> calls the unchecked versions of XCB functions. He thought most
> developers would want to use the normal (checked) versions, since the
> unchecked versions may segfault on errors. Mesa can always call the
> checked version, but ignore the error that's set, so it doesn't have to
> use the unchecked version.
When I added some XCB code to Mesa a couple years ago, it wasn't
completely clear to me what the difference was between the checked and
unchecked versions. See 6ccda72 and 588042a. My recollection was that
the _checked versions returned a cookie that you had to do something
with. For a bunch of the cases, I think we don't want to do the extra
bookkeeping (or we don't want the extra opportunity for a memory) leak.
Is there more difference than I know?
I have 3 additional comments in the code below.
> I talked to Kristen Høgsberg, who added most of the Mesa XCB calls, and
> he said he copied the style from Chris Wilson. If using the unchecked
> versions isn't required, we should look into moving the XCB calls in
> Mesa to the normal checked versions. Otherwise people will just keep
> copy-pasting the unchecked versions around.
>
> Signed-off-by: Sarah Sharp <sarah.a.sharp at linux.intel.com>
> Cc: Chad Versace <chad.versace at linux.intel.com>
> Cc: Kristian Høgsberg <krh at bitplanet.net>
> Cc: Jamey Sharp <jamey at minilop.net>
> ---
> include/EGL/eglext.h | 10 ++++++++++
> src/egl/drivers/dri2/egl_dri2.c | 15 +++++++++++++++
> src/egl/drivers/dri2/egl_dri2.h | 4 ++++
> src/egl/drivers/dri2/egl_dri2_fallbacks.h | 8 ++++++++
> src/egl/drivers/dri2/platform_android.c | 1 +
> src/egl/drivers/dri2/platform_drm.c | 1 +
> src/egl/drivers/dri2/platform_wayland.c | 1 +
> src/egl/drivers/dri2/platform_x11.c | 27 +++++++++++++++++++++++++++
> src/egl/main/eglapi.c | 27 +++++++++++++++++++++++++++
> src/egl/main/eglapi.h | 7 +++++++
> src/egl/main/egldisplay.h | 1 +
> src/egl/main/eglmisc.c | 1 +
> 12 files changed, 103 insertions(+)
>
> diff --git a/include/EGL/eglext.h b/include/EGL/eglext.h
> index 243da4a..097ad68 100644
> --- a/include/EGL/eglext.h
> +++ b/include/EGL/eglext.h
> @@ -588,6 +588,16 @@ EGLAPI EGLBoolean EGLAPIENTRY eglPostSubBufferNV (EGLDisplay dpy, EGLSurface sur
> #endif
> #endif /* EGL_NV_post_sub_buffer */
>
> +#if KHRONOS_SUPPORT_INT64 /* EGLSyncControlCHROMIUM requires 64-bit uint support */
> +#ifndef EGL_CHROMIUM_sync_control
> +#define EGL_CHROMIUM_sync_control 1
> +#ifdef EGL_EGLEXT_PROTOTYPES
> +EGLAPI EGLBoolean EGLAPIENTRY eglGetSyncValuesCHROMIUM(EGLDisplay dpy, EGLSurface surface, EGLuint64KHR *ust, EGLuint64KHR *msc, EGLuint64KHR *sbc);
> +#endif /* EGL_EGLEXT_PROTOTYPES */
> +typedef EGLBoolean (EGLAPIENTRYP PFNEGLGETSYNCVALUESCHROMIUMPROC)(EGLDisplay dpy, EGLSurface surface, EGLuint64KHR *ust, EGLuint64KHR *msc, EGLuint64KHR *sbc);
> +#endif
> +#endif
> +
It's very annoying that this extension isn't in the Khronos registry.
The problem is we periodically import new versions of eglext.h from the
registry. If the next import doesn't have this extension, this code
will be lost.
I think we need to put this in eglmesaext.h instead.
> #ifndef EGL_NV_stream_sync
> #define EGL_NV_stream_sync 1
> #define EGL_SYNC_NEW_FRAME_NV 0x321F
> diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
> index dc541ad..df8d9af 100644
> --- a/src/egl/drivers/dri2/egl_dri2.c
> +++ b/src/egl/drivers/dri2/egl_dri2.c
> @@ -1386,6 +1386,18 @@ dri2_create_image_wayland_wl_buffer(_EGLDisplay *disp, _EGLContext *ctx,
> }
> #endif
>
> +#ifdef EGL_CHROMIUM_sync_control
Is the ifdef actually necessary? Afterall, this patch adds that define. :)
> +static EGLBoolean
> +dri2_get_sync_values_chromium(_EGLDisplay *dpy, _EGLSurface *surf,
> + EGLuint64KHR *ust, EGLuint64KHR *msc,
> + EGLuint64KHR *sbc)
> +{
> + struct dri2_egl_display *dri2_dpy = dri2_egl_display(dpy);
> + return dri2_dpy->vtbl->get_sync_values(dpy, surf, ust, msc, sbc);
> +}
> +#endif
> +
> +
> /**
> * Set the error code after a call to
> * dri2_egl_image::dri_image::createImageFromTexture.
> @@ -2177,6 +2189,9 @@ _eglBuiltInDriverDRI2(const char *args)
> dri2_drv->base.API.UnbindWaylandDisplayWL = dri2_unbind_wayland_display_wl;
> dri2_drv->base.API.QueryWaylandBufferWL = dri2_query_wayland_buffer_wl;
> #endif
> +#ifdef EGL_CHROMIUM_sync_control
> + dri2_drv->base.API.GetSyncValuesCHROMIUM = dri2_get_sync_values_chromium;
> +#endif
>
> dri2_drv->base.Name = "DRI2";
> dri2_drv->base.Unload = dri2_unload;
> diff --git a/src/egl/drivers/dri2/egl_dri2.h b/src/egl/drivers/dri2/egl_dri2.h
> index e62e265..44f26fb 100644
> --- a/src/egl/drivers/dri2/egl_dri2.h
> +++ b/src/egl/drivers/dri2/egl_dri2.h
> @@ -138,6 +138,10 @@ struct dri2_egl_display_vtbl {
>
> struct wl_buffer* (*create_wayland_buffer_from_image)(
> _EGLDriver *drv, _EGLDisplay *dpy, _EGLImage *img);
> +
> + EGLBoolean (*get_sync_values)(_EGLDisplay *display, _EGLSurface *surface,
> + EGLuint64KHR *ust, EGLuint64KHR *msc,
> + EGLuint64KHR *sbc);
> };
>
> struct dri2_egl_display
> diff --git a/src/egl/drivers/dri2/egl_dri2_fallbacks.h b/src/egl/drivers/dri2/egl_dri2_fallbacks.h
> index a5cf344..9cba001 100644
> --- a/src/egl/drivers/dri2/egl_dri2_fallbacks.h
> +++ b/src/egl/drivers/dri2/egl_dri2_fallbacks.h
> @@ -98,3 +98,11 @@ dri2_fallback_create_wayland_buffer_from_image(_EGLDriver *drv,
> {
> return NULL;
> }
> +
> +static inline EGLBoolean
> +dri2_fallback_get_sync_values(_EGLDisplay *dpy, _EGLSurface *surf,
> + EGLuint64KHR *ust, EGLuint64KHR *msc,
> + EGLuint64KHR *sbc)
> +{
> + return EGL_FALSE;
> +}
> diff --git a/src/egl/drivers/dri2/platform_android.c b/src/egl/drivers/dri2/platform_android.c
> index 7b1db76..71948bd 100644
> --- a/src/egl/drivers/dri2/platform_android.c
> +++ b/src/egl/drivers/dri2/platform_android.c
> @@ -650,6 +650,7 @@ static struct dri2_egl_display_vtbl droid_display_vtbl = {
> .copy_buffers = dri2_fallback_copy_buffers,
> .query_buffer_age = dri2_fallback_query_buffer_age,
> .create_wayland_buffer_from_image = dri2_fallback_create_wayland_buffer_from_image,
> + .get_sync_values = dri2_fallback_get_sync_values,
> };
>
> EGLBoolean
> diff --git a/src/egl/drivers/dri2/platform_drm.c b/src/egl/drivers/dri2/platform_drm.c
> index 9a7633a..52d8b49 100644
> --- a/src/egl/drivers/dri2/platform_drm.c
> +++ b/src/egl/drivers/dri2/platform_drm.c
> @@ -472,6 +472,7 @@ static struct dri2_egl_display_vtbl dri2_drm_display_vtbl = {
> .copy_buffers = dri2_fallback_copy_buffers,
> .query_buffer_age = dri2_drm_query_buffer_age,
> .create_wayland_buffer_from_image = dri2_fallback_create_wayland_buffer_from_image,
> + .get_sync_values = dri2_fallback_get_sync_values,
> };
>
> EGLBoolean
> diff --git a/src/egl/drivers/dri2/platform_wayland.c b/src/egl/drivers/dri2/platform_wayland.c
> index 691f3e1..efc5546 100644
> --- a/src/egl/drivers/dri2/platform_wayland.c
> +++ b/src/egl/drivers/dri2/platform_wayland.c
> @@ -964,6 +964,7 @@ static struct dri2_egl_display_vtbl dri2_wl_display_vtbl = {
> .copy_buffers = dri2_fallback_copy_buffers,
> .query_buffer_age = dri2_wl_query_buffer_age,
> .create_wayland_buffer_from_image = dri2_wl_create_wayland_buffer_from_image,
> + .get_sync_values = dri2_fallback_get_sync_values,
> };
>
> EGLBoolean
> diff --git a/src/egl/drivers/dri2/platform_x11.c b/src/egl/drivers/dri2/platform_x11.c
> index 7b585a2..a6b03cc 100644
> --- a/src/egl/drivers/dri2/platform_x11.c
> +++ b/src/egl/drivers/dri2/platform_x11.c
> @@ -1008,6 +1008,30 @@ dri2_x11_swrast_create_image_khr(_EGLDriver *drv, _EGLDisplay *disp,
> return NULL;
> }
>
> +static EGLBoolean
> +dri2_x11_get_sync_values(_EGLDisplay *display, _EGLSurface *surface,
> + EGLuint64KHR *ust, EGLuint64KHR *msc,
> + EGLuint64KHR *sbc)
> +{
> + struct dri2_egl_display *dri2_dpy = dri2_egl_display(display);
> + struct dri2_egl_surface *dri2_surf = dri2_egl_surface(surface);
> + xcb_dri2_get_msc_cookie_t cookie;
> + xcb_dri2_get_msc_reply_t *reply;
> +
> + cookie = xcb_dri2_get_msc(dri2_dpy->conn, dri2_surf->drawable);
> + reply = xcb_dri2_get_msc_reply(dri2_dpy->conn, cookie, NULL);
> +
> + if (!reply)
> + return EGL_FALSE;
> +
> + *ust = ((EGLuint64KHR) reply->ust_hi << 32) | reply->ust_lo;
> + *msc = ((EGLuint64KHR) reply->msc_hi << 32) | reply->msc_lo;
> + *sbc = ((EGLuint64KHR) reply->sbc_hi << 32) | reply->sbc_lo;
> + free(reply);
> +
> + return EGL_TRUE;
> +}
> +
> static struct dri2_egl_display_vtbl dri2_x11_swrast_display_vtbl = {
> .authenticate = NULL,
> .create_window_surface = dri2_x11_create_window_surface,
> @@ -1022,6 +1046,7 @@ static struct dri2_egl_display_vtbl dri2_x11_swrast_display_vtbl = {
> .copy_buffers = dri2_x11_copy_buffers,
> .query_buffer_age = dri2_fallback_query_buffer_age,
> .create_wayland_buffer_from_image = dri2_fallback_create_wayland_buffer_from_image,
> + .get_sync_values = dri2_fallback_get_sync_values,
> };
>
> static struct dri2_egl_display_vtbl dri2_x11_display_vtbl = {
> @@ -1039,6 +1064,7 @@ static struct dri2_egl_display_vtbl dri2_x11_display_vtbl = {
> .copy_buffers = dri2_x11_copy_buffers,
> .query_buffer_age = dri2_fallback_query_buffer_age,
> .create_wayland_buffer_from_image = dri2_fallback_create_wayland_buffer_from_image,
> + .get_sync_values = dri2_x11_get_sync_values,
> };
>
> static EGLBoolean
> @@ -1243,6 +1269,7 @@ dri2_initialize_x11_dri2(_EGLDriver *drv, _EGLDisplay *disp)
> disp->Extensions.NOK_swap_region = EGL_TRUE;
> disp->Extensions.NOK_texture_from_pixmap = EGL_TRUE;
> disp->Extensions.NV_post_sub_buffer = EGL_TRUE;
> + disp->Extensions.CHROMIUM_get_sync_values = EGL_TRUE;
>
> #ifdef HAVE_WAYLAND_PLATFORM
> disp->Extensions.WL_bind_wayland_display = EGL_TRUE;
> diff --git a/src/egl/main/eglapi.c b/src/egl/main/eglapi.c
> index 219d8e6..d876995 100644
> --- a/src/egl/main/eglapi.c
> +++ b/src/egl/main/eglapi.c
> @@ -1086,6 +1086,9 @@ eglGetProcAddress(const char *procname)
> { "eglGetPlatformDisplayEXT", (_EGLProc) eglGetPlatformDisplayEXT },
> { "eglCreatePlatformWindowSurfaceEXT", (_EGLProc) eglCreatePlatformWindowSurfaceEXT },
> { "eglCreatePlatformPixmapSurfaceEXT", (_EGLProc) eglCreatePlatformPixmapSurfaceEXT },
> +#ifdef EGL_CHROMIUM_sync_control
> + { "eglGetSyncValuesCHROMIUM", (_EGLProc) eglGetSyncValuesCHROMIUM },
> +#endif
> { NULL, NULL }
> };
> EGLint i;
> @@ -1751,3 +1754,27 @@ eglPostSubBufferNV(EGLDisplay dpy, EGLSurface surface,
>
> RETURN_EGL_EVAL(disp, ret);
> }
> +
> +#ifdef EGL_CHROMIUM_sync_control
> +EGLBoolean EGLAPIENTRY
> +eglGetSyncValuesCHROMIUM(EGLDisplay display, EGLSurface surface,
> + EGLuint64KHR *ust, EGLuint64KHR *msc,
> + EGLuint64KHR *sbc)
> +{
> + _EGLDisplay *disp = _eglLockDisplay(display);
> + _EGLSurface *surf = _eglLookupSurface(surface, disp);
> + _EGLDriver *drv;
> + EGLBoolean ret;
> +
> + _EGL_CHECK_SURFACE(disp, surf, EGL_FALSE, drv);
> + if (!disp->Extensions.CHROMIUM_get_sync_values)
> + RETURN_EGL_EVAL(disp, EGL_FALSE);
> +
> + if (!ust || !msc || !sbc)
> + RETURN_EGL_EVAL(disp, EGL_FALSE);
> +
> + ret = drv->API.GetSyncValuesCHROMIUM(disp, surf, ust, msc, sbc);
> +
> + RETURN_EGL_EVAL(disp, ret);
> +}
> +#endif
> diff --git a/src/egl/main/eglapi.h b/src/egl/main/eglapi.h
> index f20ce5b..f176aa3 100644
> --- a/src/egl/main/eglapi.h
> +++ b/src/egl/main/eglapi.h
> @@ -139,6 +139,10 @@ typedef EGLint (*QueryBufferAge_t)(_EGLDriver *drv,
> typedef EGLBoolean (*SwapBuffersWithDamageEXT_t) (_EGLDriver *drv, _EGLDisplay *dpy, _EGLSurface *surface, const EGLint *rects, EGLint n_rects);
> #endif
>
> +#ifdef EGL_CHROMIUM_sync_control
> +typedef EGLBoolean (*GetSyncValuesCHROMIUM_t) (_EGLDisplay *dpy, _EGLSurface *surface, EGLuint64KHR *ust, EGLuint64KHR *msc, EGLuint64KHR *sbc);
> +#endif
> +
> /**
> * The API dispatcher jumps through these functions
> */
> @@ -225,6 +229,9 @@ struct _egl_api
> PostSubBufferNV_t PostSubBufferNV;
>
> QueryBufferAge_t QueryBufferAge;
> +#ifdef EGL_CHROMIUM_sync_control
> + GetSyncValuesCHROMIUM_t GetSyncValuesCHROMIUM;
> +#endif
> };
>
> #endif /* EGLAPI_INCLUDED */
> diff --git a/src/egl/main/egldisplay.h b/src/egl/main/egldisplay.h
> index 970c21a..2dec6e4 100644
> --- a/src/egl/main/egldisplay.h
> +++ b/src/egl/main/egldisplay.h
> @@ -119,6 +119,7 @@ struct _egl_extensions
> EGLBoolean EXT_buffer_age;
> EGLBoolean EXT_swap_buffers_with_damage;
> EGLBoolean EXT_image_dma_buf_import;
> + EGLBoolean CHROMIUM_get_sync_values;
> };
>
>
> diff --git a/src/egl/main/eglmisc.c b/src/egl/main/eglmisc.c
> index 65669d8..d867d1f 100644
> --- a/src/egl/main/eglmisc.c
> +++ b/src/egl/main/eglmisc.c
> @@ -123,6 +123,7 @@ _eglUpdateExtensionsString(_EGLDisplay *dpy)
> _EGL_CHECK_EXTENSION(EXT_image_dma_buf_import);
>
> _EGL_CHECK_EXTENSION(NV_post_sub_buffer);
> + _EGL_CHECK_EXTENSION(CHROMIUM_get_sync_values);
Most places we try to sort extensions first by broad category (KHR, ARB,
OES, others), then alphabetically. There's not enough context in the
patch to know whether or not that's followed here.
> #undef _EGL_CHECK_EXTENSION
> }
>
More information about the mesa-dev
mailing list