[Mesa-dev] [PATCH] egl: Remove eglQueryString virtual dispatch.

[Chad Versace]

On 03/13/2015 05:59 PM, Matt Turner wrote:

Patch is
Reviewed-by: Chad Versace <chad.versace at intel.com>

> ---
> Chad, you suggested it would be nice to remove the locking from
> eglQueryString, but I don't see a way to do it. eglQueryString has
> to generate EGL_NOT_INITIALIZED if the display is valid but not
> initialized, and without locking it seems that there would be a
> race between eglInitialize and eglQueryString. Is that the case,
> or have I misunderstood something?

I still think this can be done without a lock. Currently the function
(like most other EGL functions) holds the display lock for the duration
of the function call. But eglQueryString really only needs to ensure that
(1) the display is initialized and (2) the display remains initialized
until the call returns. (#2 is important because the user could possibly
call eglTerminate in another thread, and eglTerminate has deferred destruction
semantics). I think code like this could achieve that:

  // A new function that atomically increments the display's internal refcount only
  // if the display is initialized. Return true if the increment succeeds.
  bool
  _eglDisplayReference(_EGLDisplay *disp);

  // Another new function that pairs with _EGLDisplayReference. Returns
  // the number of remaining references.
  uint32_t
  _eglDisplayRelease(_EGLDisplay *disp);
  
  const char *
  eglQueryString(EGLDisplay dpy, ...)
  {
     const char *string = NULL;

     _EGLDisplay *disp = _eglLookupDisplay(dpy);
     if (!_eglDisplayReference(disp)) {
        // emit EGL_NOT_INITIALIZED
        return NULL;
     }

     // Set 'string' in the switch statement.

     _eglDisplayRelease(disp);
     return string;
  }

To remove any races with eglTerminate, you would also need to move the destruction
code from egTerminate into _eglDisplayRelease and rewrite eglTerminate to
essentially just call _eglDisplayRelease.

In this scheme, lockless EGL functions would follow this pattern:
  1. Try to take a reference on the display. The reference will be held for the duration
     of the function call.
  2. Do the action.
  3. Release the reference and return.

I admit it's a non-trivial amount of work. But doing that work would lay some groundwork
for removing the display lock in other places too, even in functions that do real work like
eglSwapBuffers.

The bigger question is: Do we care enough to do all that work? And I think the answer remains
"no" until we find real-word uses of multithreaded EGL where lock contention hurts the app.
I don't know of any multithreaded EGL apps at the moment. Even so, I believe we should take
care when writing new EGL code to not dig ourselves into a deeper hole.