[I missed Chad on the original Cc somehow; hopefully he can find the original mails.]
On Fri, Apr 18, 2014 at 04:49:43PM -0700, Ian Romanick wrote: > 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. Thanks for the review Ian! > > 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.sh...@linux.intel.com> > > Cc: Chad Versace <chad.vers...@linux.intel.com> > > Cc: Kristian Høgsberg <k...@bitplanet.net> > > Cc: Jamey Sharp <ja...@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. Ok, will do. > > #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. :) It only adds the define if we have support for 64-bit integers, since the extension writes 64-bit counters. I think Chad said KHRONOS_SUPPORT_INT64 should always be true, and if so, I can just remove all the #ifdefs. > > +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. Ok, makes sense. I do think I'm missing some context about what defines those broader categories. I can infer KHR is Khronos, but is there a list somewhere of the other categories and their common abbreviations? Here's more patch context: _EGL_CHECK_EXTENSION(MESA_screen_surface); _EGL_CHECK_EXTENSION(MESA_copy_context); _EGL_CHECK_EXTENSION(MESA_drm_display); _EGL_CHECK_EXTENSION(MESA_drm_image); _EGL_CHECK_EXTENSION(MESA_configless_context); _EGL_CHECK_EXTENSION(WL_bind_wayland_display); _EGL_CHECK_EXTENSION(WL_create_wayland_buffer_from_image); _EGL_CHECK_EXTENSION(KHR_image_base); _EGL_CHECK_EXTENSION(KHR_image_pixmap); if (dpy->Extensions.KHR_image_base && dpy->Extensions.KHR_image_pixmap) _eglAppendExtension(&exts, "EGL_KHR_image"); _EGL_CHECK_EXTENSION(KHR_vg_parent_image); _EGL_CHECK_EXTENSION(KHR_gl_texture_2D_image); _EGL_CHECK_EXTENSION(KHR_gl_texture_cubemap_image); _EGL_CHECK_EXTENSION(KHR_gl_texture_3D_image); _EGL_CHECK_EXTENSION(KHR_gl_renderbuffer_image); _EGL_CHECK_EXTENSION(KHR_reusable_sync); _EGL_CHECK_EXTENSION(KHR_fence_sync); _EGL_CHECK_EXTENSION(KHR_surfaceless_context); _EGL_CHECK_EXTENSION(KHR_create_context); _EGL_CHECK_EXTENSION(NOK_swap_region); _EGL_CHECK_EXTENSION(NOK_texture_from_pixmap); _EGL_CHECK_EXTENSION(ANDROID_image_native_buffer); _EGL_CHECK_EXTENSION(EXT_create_context_robustness); _EGL_CHECK_EXTENSION(EXT_buffer_age); _EGL_CHECK_EXTENSION(EXT_swap_buffers_with_damage); _EGL_CHECK_EXTENSION(EXT_image_dma_buf_import); _EGL_CHECK_EXTENSION(NV_post_sub_buffer); + _EGL_CHECK_EXTENSION(CHROMIUM_get_sync_values); #undef _EGL_CHECK_EXTENSION } I put the extension at the end of the list, but maybe it should go between ANDROID_ and EXT_? Sarah Sharp _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
