Skip to content

Commit dc49c21

Browse files
committed
GPU Display: Improve synchronization and avoid possible race condition at startup
1 parent 2554006 commit dc49c21

9 files changed

Lines changed: 68 additions & 72 deletions

‎GPU/GPUTracking/display/GPUDisplay.cxx‎

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -171,12 +171,22 @@ int32_t GPUDisplay::InitDisplay(bool initFailure)
171171
if (!initFailure) {
172172
retVal = InitDisplay_internal();
173173
}
174+
if (retVal == 0) {
175+
buildConfig();
176+
if (mConfig.listFrontendOptionHandling) {
177+
mFrontend->listConfig();
178+
}
179+
}
174180
} catch (const std::runtime_error& e) {
175181
GPUError("%s", e.what());
176182
retVal = 1;
177183
}
178-
mInitResult = retVal == 0 ? 1 : -1;
179184
mOldCfg = {mCfgH, mCfgL, mCfgR};
185+
std::unique_lock lock(mFrontend->mExitStateMutex);
186+
mInitResult = retVal == 0 ? 1 : -1;
187+
mFrontend->mDisplayRunning = retVal == 0; // Only after successful initialization, StopDisplay waits for it to become false again
188+
mFrontend->mInitCV.notify_all();
189+
mFrontend->mInitCV.wait(lock, [this] { return mFrontend->mStartDisplayDone; }); // Both threads have finished their initialization
180190
return (retVal);
181191
}
182192

@@ -791,11 +801,5 @@ void GPUDisplay::WaitTillEventShown()
791801
int32_t GPUDisplay::StartDisplay()
792802
{
793803
mLoadAndShowEvent = true;
794-
if (mFrontend->StartDisplay()) {
795-
return (1);
796-
}
797-
while (mInitResult == 0) {
798-
Sleep(10);
799-
}
800-
return (mInitResult != 1);
804+
return mFrontend->StartDisplay();
801805
}

‎GPU/GPUTracking/display/GPUDisplay.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -330,7 +330,7 @@ class GPUDisplay : public GPUDisplayInterface
330330

331331
std::vector<threadVertexBuffer> mThreadBuffers;
332332
std::vector<std::vector<std::array<std::array<vecpod<int32_t>, 2>, NSECTORS>>> mThreadTracks;
333-
volatile int32_t mInitResult = 0;
333+
int32_t mInitResult = 0; // 1 success, -1 failure, protected by mFrontend->mExitStateMutex
334334

335335
float mFPSScale = 1, mFPSScaleadjust = 0;
336336
int32_t mFramesDone = 0, mFramesDoneFPS = 0;

‎GPU/GPUTracking/display/frontend/GPUDisplayFrontend.cxx‎

Lines changed: 43 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,9 @@
1818

1919
#ifdef _WIN32
2020
#include "GPUDisplayFrontendWindows.h"
21-
#else
22-
#include <pthread.h>
2321
#endif
22+
#include <system_error>
23+
#include <thread>
2424

2525
#ifdef GPUCA_BUILD_EVENT_DISPLAY_X11
2626
#include "GPUDisplayFrontendX11.h"
@@ -61,22 +61,31 @@ class GPUDisplayGUIWrapper
6161
using namespace o2::gpu;
6262

6363
GPUDisplayFrontend::GPUDisplayFrontend() : mConsole(std::make_unique<GPUDisplayConsole>(std::string(DISPLAY_WINDOW_NAME) + " Console")) {}
64-
GPUDisplayFrontend::~GPUDisplayFrontend() = default;
64+
GPUDisplayFrontend::~GPUDisplayFrontend()
65+
{
66+
if (mFrontendThread.joinable()) { // Started but not stopped, as the detached thread before
67+
mFrontendThread.detach();
68+
}
69+
}
6570

66-
void* GPUDisplayFrontend::FrontendThreadWrapper(void* ptr)
71+
void GPUDisplayFrontend::FrontendThreadWrapper()
6772
{
68-
GPUDisplayFrontend* me = reinterpret_cast<GPUDisplayFrontend*>(ptr);
6973
int32_t retVal;
7074
try {
71-
retVal = me->FrontendMain();
75+
retVal = FrontendMain();
7276
} catch (const std::exception& e) {
7377
GPUError("Error in display frontend: %s", e.what());
7478
retVal = -1;
7579
}
76-
if (retVal && me->mDisplay->mInitResult == 0) { // Failed before InitDisplay was called, report failure to StartDisplay(), which otherwise waits forever
77-
me->InitDisplay(true);
80+
if (retVal) {
81+
std::unique_lock lock(mExitStateMutex);
82+
if (mDisplay->mInitResult == 0) { // Failed before InitDisplay was called, report failure to StartDisplay(), which otherwise waits forever
83+
lock.unlock();
84+
InitDisplay(true);
85+
} else {
86+
mDisplay->mInitResult = -1; // Error after the initialization: StopDisplay does not reset the state for a new start
87+
}
7888
}
79-
return ((void*)(size_t)retVal);
8089
}
8190

8291
void GPUDisplayFrontend::waitForFPSLimit()
@@ -341,35 +350,38 @@ int32_t& GPUDisplayFrontend::drawTextFontSize()
341350

342351
int32_t GPUDisplayFrontend::StartDisplay()
343352
{
344-
#ifndef _WIN32
345-
static pthread_t hThread;
346-
if (pthread_create(&hThread, nullptr, FrontendThreadWrapper, this)) {
347-
GPUError("Coult not Create frontend Thread...");
348-
return (1);
349-
}
350-
#else
351-
HANDLE hThread;
352-
if ((hThread = CreateThread(nullptr, nullptr, &OpenGLWrapper, this, nullptr, nullptr)) == nullptr) {
353-
GPUError("Coult not Create GL Thread...");
353+
try {
354+
mFrontendThread = std::thread(&GPUDisplayFrontend::FrontendThreadWrapper, this);
355+
} catch (const std::system_error& e) {
356+
GPUError("Could not create frontend thread: %s", e.what());
354357
return (1);
355358
}
356-
#endif
357-
mDisplay->buildConfig();
358-
if (mDisplay->cfg().listFrontendOptionHandling) {
359-
listConfig();
360-
}
361-
return (0);
359+
std::unique_lock lock(mExitStateMutex);
360+
mStartDisplayDone = true;
361+
mInitCV.notify_all();
362+
mInitCV.wait(lock, [this] { return mDisplay->mInitResult != 0; }); // Both threads have finished their initialization
363+
return (mDisplay->mInitResult != 1);
362364
}
363365

364366
void GPUDisplayFrontend::StopDisplay()
365367
{
366-
std::unique_lock lock(mExitStateMutex);
367-
if (mDisplayRunning) {
368-
mDisplay->mCfgC.mDisplayControl.store(2);
369-
StopDisplayA();
370-
mDisplay->UnblockDisplay();
368+
{
369+
std::unique_lock lock(mExitStateMutex);
370+
if (mDisplayRunning) {
371+
mDisplay->mCfgC.mDisplayControl.store(2);
372+
StopDisplayA();
373+
mDisplay->UnblockDisplay();
374+
}
375+
mDisplayStoppedCV.wait(lock, [this] { return !mDisplayRunning; });
376+
}
377+
if (mFrontendThread.joinable()) { // Also the wrapper has finished (it sets mInitResult = -1 for errors after the initialization), not holding mExitStateMutex, which the wrapper uses
378+
mFrontendThread.join();
379+
}
380+
std::lock_guard lock(mExitStateMutex);
381+
if (mDisplay->mInitResult == 1) { // For a new StartDisplay, not after a failed initialization or an error (undefined state)
382+
mStartDisplayDone = false;
383+
mDisplay->mInitResult = 0;
371384
}
372-
mDisplayStoppedCV.wait(lock, [this] { return !mDisplayRunning; });
373385
}
374386

375387
void GPUDisplayFrontend::StopDisplayA() {}

‎GPU/GPUTracking/display/frontend/GPUDisplayFrontend.h‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,11 @@
1818
#include "GPUCommonDef.h"
1919
#include "GPUDisplayInterface.h"
2020
#include "GPUDisplayFrontendKeyVars.h"
21+
#include <atomic>
2122
#include <memory>
2223
#include <chrono>
2324
#include <mutex>
25+
#include <thread>
2426
#include <condition_variable>
2527
#include <string>
2628
#include <utility>
@@ -86,9 +88,9 @@ class GPUDisplayFrontend : public GPUDisplayFrontendInterface, public GPUDisplay
8688
protected:
8789
virtual int32_t FrontendMain() = 0;
8890
virtual void StopDisplayA();
89-
static void* FrontendThreadWrapper(void*);
90-
void waitForFPSLimit(); // Sleep until the next frame is due according to fpsRate (only in FPS_LIMIT_RATE mode, see GPUDisplayBackend::fpsLimitMode), or until an external config update is signalled
91-
void signalConfigUpdate(); // Signal a config update from an external source (console keys, Qt GUI), wakes up waitForFPSLimit
91+
void FrontendThreadWrapper(); // Runs FrontendMain in mFrontendThread
92+
void waitForFPSLimit(); // Sleep until the next frame is due according to fpsRate (only in FPS_LIMIT_RATE mode, see GPUDisplayBackend::fpsLimitMode), or until an external config update is signalled
93+
void signalConfigUpdate(); // Signal a config update from an external source (console keys, Qt GUI), wakes up waitForFPSLimit
9294

9395
static constexpr int32_t INIT_WIDTH = 1024, INIT_HEIGHT = 768; // Initial window size, before maximizing
9496
static constexpr int32_t MIN_WIDTH = 200, MIN_HEIGHT = 150; // Minimum window size
@@ -104,6 +106,9 @@ class GPUDisplayFrontend : public GPUDisplayFrontendInterface, public GPUDisplay
104106
bool mConfigUpdatePending = false; // Protected by mConfigUpdateMutex, set by signalConfigUpdate, cleared by HandleExternalGUISignals
105107
std::mutex mExitStateMutex;
106108
std::condition_variable mDisplayStoppedCV; // Notified with mExitStateMutex held when the frontend sets mDisplayRunning to false, StopDisplay waits for it
109+
std::condition_variable mInitCV; // With mExitStateMutex: InitDisplay (frontend thread) and StartDisplay (main thread) wait for each other at their end
110+
std::thread mFrontendThread; // Started by StartDisplay, joined by StopDisplay
111+
bool mStartDisplayDone = false; // Protected by mExitStateMutex
107112
std::array<uint8_t, KEY_MAP_MAX> mKeyDownMap = {};
108113

109114
// Keyboard / Mouse actions
@@ -124,7 +129,7 @@ class GPUDisplayFrontend : public GPUDisplayFrontendInterface, public GPUDisplay
124129

125130
frontendTypes mFrontendType = TYPE_INVALID;
126131
const char* mFrontendName = nullptr;
127-
volatile bool mDisplayRunning = false;
132+
std::atomic<bool> mDisplayRunning = false; // Set at the end of a successful InitDisplay, StopDisplay waits for it to become false again
128133

129134
std::unique_ptr<GPUDisplayGUIChannel> mGUIChannel; // Must outlive mGUI
130135
std::unique_ptr<GPUDisplayGUIWrapper> mGUI;

‎GPU/GPUTracking/display/frontend/GPUDisplayFrontendGlfw.cxx‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -332,11 +332,6 @@ int32_t GPUDisplayFrontendGlfw::FrontendMain()
332332
return (1);
333333
}
334334

335-
{
336-
std::lock_guard lock(mExitStateMutex);
337-
mDisplayRunning = true; // Only after successful initialization, StopDisplay waits for it to become false again
338-
}
339-
340335
int32_t retVal = 0;
341336
while (!glfwWindowShouldClose(mWindow)) {
342337
if (HandleUpdatesAndDrawGLScene()) {

‎GPU/GPUTracking/display/frontend/GPUDisplayFrontendGlut.cxx‎

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,6 @@
2828
#include <X11/Xatom.h>
2929
#endif
3030

31-
#include <pthread.h>
3231
using namespace o2::gpu;
3332

3433
GPUDisplayFrontendGlut::GPUDisplayFrontendGlut()
@@ -230,11 +229,6 @@ int32_t GPUDisplayFrontendGlut::FrontendMain()
230229
fprintf(stderr, "Error initializing GL extension wrapper\n");
231230
return (-1);
232231
}
233-
if (InitDisplay()) {
234-
fprintf(stderr, "Error in OpenGL initialization\n");
235-
return (1);
236-
}
237-
238232
auto mouseMoveFunc = [](int32_t x, int32_t y, void* p) { self(p)->mouseMoveFunc(x, y); };
239233
glutDisplayFuncUcall([](void* p) { self(p)->displayFunc(); }, this); // Callbacks with this as user data, freeglut >= 3.2
240234
glutIdleFuncUcall([](void* p) { self(p)->glutLoopFunc(); }, this);
@@ -250,9 +244,9 @@ int32_t GPUDisplayFrontendGlut::FrontendMain()
250244
applyInitialWindowState();
251245
SetVSync(vsyncEnabled());
252246

253-
{
254-
std::lock_guard lock(mExitStateMutex);
255-
mDisplayRunning = true;
247+
if (InitDisplay()) {
248+
fprintf(stderr, "Error in OpenGL initialization\n");
249+
return (1);
256250
}
257251
glutMainLoop();
258252
{

‎GPU/GPUTracking/display/frontend/GPUDisplayFrontendNone.cxx‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -27,10 +27,6 @@ int32_t GPUDisplayFrontendNone::FrontendMain()
2727
if (InitDisplay()) {
2828
return 1;
2929
}
30-
{
31-
std::lock_guard lock(mExitStateMutex);
32-
mDisplayRunning = true;
33-
}
3430
do {
3531
HandleUpdatesAndDrawGLScene();
3632
waitForFPSLimit();

‎GPU/GPUTracking/display/frontend/GPUDisplayFrontendWayland.cxx‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -564,11 +564,6 @@ int32_t GPUDisplayFrontendWayland::FrontendMain()
564564
return (1);
565565
}
566566

567-
{
568-
std::lock_guard lock(mExitStateMutex);
569-
mDisplayRunning = true;
570-
}
571-
572567
while (mDisplay->mCfgC.mDisplayControl.load() != 2) {
573568
bool error = false;
574569
while (!error && wl_display_prepare_read(mWayland) != 0) {

‎GPU/GPUTracking/display/frontend/GPUDisplayFrontendX11.cxx‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -311,11 +311,6 @@ int32_t GPUDisplayFrontendX11::FrontendMain()
311311
return (1);
312312
}
313313

314-
{
315-
std::lock_guard lock(mExitStateMutex);
316-
mDisplayRunning = true;
317-
}
318-
319314
while (1) {
320315
do {
321316
if (getDisplayControl() == 2) {

0 commit comments

Comments
 (0)