Use rwlocks for EGLDisplays and EGLSurfaces#21
Open
kbrenneman wants to merge 7 commits intoNVIDIA:mainfrom
Open
Use rwlocks for EGLDisplays and EGLSurfaces#21kbrenneman wants to merge 7 commits intoNVIDIA:mainfrom
kbrenneman wants to merge 7 commits intoNVIDIA:mainfrom
Conversation
Use an rwlock to protect the display's surface list. eglCreate*Surface, eglDestroySurface, and eglTerminate all take the write lock. All other functions that operate on an EGLSurface take the read lock. This will allow most EGL functions that take an EGLSurface to run concurrently, without worrying about another thread calling eglDestroySurface or eglTerminate. That works because, outside of creation and destruction, almost everything in EplSurface is effectively read-only. The only thing that might change is the internal EGLSurface handle, and with the new platform surface interface, that won't need need to change either. This also changes and simplifies surface destruction. Since eglSwapBuffers et. al. don't need to worry about another thread destroying an EplSurface, EplSurface no longer needs to be refcounted. Now, the EplSurface is always fully destroyed during the eglDestroySurface hook, and so we can remove EplImplFuncs::FreeSurface. Also remove eplSurfaceAcquire and eplSurfaceRelease. Instead, there's a new eplDisplayLockSurfaceList function, which takes the read lock for the surface list and returns a pointer to the list head, and eplSurfaceListLookup, which scans the surface list for an external EGLSurface handle. As a convenience, eplHookDisplaySurface and eplHookDisplaySurfaceEnd are wrappers to the functions above to handle looking up an EGLDisplay and an EGLSurface, with the appropriate locking and unlocking.
Use an rwlock to protect against concurrent calls to eglTerminate, instead of using a secondary refcount. Removed EplDisplay::use_count, and instead added EplDisplay::init_lock, which is a read/write lock that protects the eglInitialize count. eglInitialize, eglTermiante, and teardown will all take the write lock. Any function which needs an initialized EGLDisplay will take the read lock. That's enough to prevent any other threads from terminating the EGLDisplay out from under it, without otherwise blocking concurrent functions. As with surfaces, a read lock is fine. The surface list has its own lock, and outside of eglInitialize/eglTerminate, everything else in EplDisplay has its own lock. In particular, eglSwapBuffers implementations no longer need to temporarily unlock the display before perfoming long waits or reads. Unlocking like that makes it much harder to reason about, because you have to deal with the case where another thread calls eglDestroySurface or eglTerminate. In addition, the use_count field would give the wrong behavior in one (admittedly contrived) case: If another thread calls eglTerminate and then eglInitilize during that temporary unlock, then they get effectively ignored, and none of the cleanup stuff in eglTerminate happens.
Add an explicit dependency on 'egl' to the meson.build files.
Remove eplGetCurrentDisplay, and replace it with a eplGetCurrentSurface function that returns both the current EGLDisplay and the current EGLSurface. That makes it possible to use eplHookDisplaySurface and eplHookDisplaySurfaceEnd with functions that operate on the current EGLSurface.
Add an optional EplImplFuncs::SwapInterval function to handle eglSwapInterval calls. If it's not NULL, then the base library will provide a hook function for eglSwapInterval. HookSwapInterval will handle looking up the current EGLDisplay and EGLSurface and making sure that they're valid, including all the necessary locking.
Removed all of the temporary unlocking in eglSwapBuffers. Now that we use an rwlock, that's no longer necessary. Updated eplX11CreateWindowSurface and eplX11CreatePixmapSurface to take the existing surface list as a parameter. Merged eplX11FreeWindow into eplX11DestroyWindow. Changed to use EplImplFuncs::SwapInterval instead of providing the entire hook function.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This updates egl-x11 to use the locking changes that went into the base library for the new egl-wayland2 library.
The core of the change is that instead of having a per-display mutex, there's a pair of rwlocks: One for eglInitialize/eglTerminate, and one for EGLSurface creation and destruction.
For the eglInitialize/eglTerminate lock, eglInitialize and eglTerminate will take the write lock, and all other EGL functions will take the read lock. That way, other EGL functions can run concurrently, but they don't need to worry about the EGLDisplay getting terminated out from under them.
The second lock protects the EGLSurface list. It'll take the write lock for eglCreateWindow/PixmapSurface and eglDestroySurface, and it'll take the read lock in any other function that takes an EGLSurface.
Everything else in EplDisplay is immutable after EGLDisplay creation, so it doesn't need any locking.
eglSwapBuffersis the really interesting one there, since we don't want to block other functions while we're blocked waiting for rendering or window system events. Currently, that means unlocking the display mutex before blocking (and the per-surface mutex to avoid a deadlock when we lock them again), which opens up problems if another thread calls eglDestroySurface or eglTerminate.With this change, multiple threads can call eglSwapBuffers concurrently (on different EGLSurfaces, obviously), including any blocking waits, without needing to unlock anything.
Aside from being much simpler and easier to reason about, that also means we can get rid of EplDisplay::use_count, which fixes a potential bug: If an app manages to call eglTerminate and then eglInitialize during eglSwapBuffers's temporary unlock, then both of them would basically get ignored.
In addition, since we can always just clean up and free the EplSurface struct in eglDestroySurface, EplSurface doesn't need to be refcounted, and we don't have to deal with half-destroyed surfaces anymore.