| Message ID | 20260724014338.1850939-1-qi.hou@oss.nxp.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
2026. 07. 24. 3:43 keltezéssel, qi.hou@oss.nxp.com írta: > From: Qi Hou <qi.hou@nxp.com> > > isAvailable() previously called probeDisplay() which ran eglBindAPI(), > eglGetPlatformDisplay() and eglInitialize(), then immediately terminated > the display with eglTerminate(). When SoftwareIsp subsequently called > initEGLContext(), probeDisplay() repeated a full initialisation sequence. > > Avoid this double init/teardown by caching the EGLDisplay handle inside > probeDisplay() using static parameters. The first call initialises EGL > and stores the result, all later calls return the cached handle > without reinitialising. > > isAvailable() is simplified to return probeDisplay() != EGL_NO_DISPLAY > and no longer calls eglTerminate(), because the cached display should > be live for reuse by initEGLContext(). > --- > src/libcamera/egl.cpp | 31 +++++++++++++++++-------------- > 1 file changed, 17 insertions(+), 14 deletions(-) > > diff --git a/src/libcamera/egl.cpp b/src/libcamera/egl.cpp > index 7ec7a654d..f5200ee76 100644 > --- a/src/libcamera/egl.cpp > +++ b/src/libcamera/egl.cpp > @@ -301,46 +301,49 @@ void eGL::createTexture2D(eGLImage &eglImage, void *data) > > EGLDisplay eGL::probeDisplay() > { > - EGLDisplay display; > + static EGLDisplay cachedDisplay = EGL_NO_DISPLAY; > + static bool probed = false; I'm not a fan of adding more mutable global state. As an alternative: * modify `eGL` to take the EGLDisplay in its constructor and adjust `initEGLContext()` to not retrieve the display * adjust `DebayerEGL` accordingly * adjust `SoftwareISP` to call `probeDisplay()` and forward the result Here is a prototype of the idea: diff --git a/include/libcamera/internal/egl.h b/include/libcamera/internal/egl.h index 1e31d490bd..7ef1ca0d93 100644 --- a/include/libcamera/internal/egl.h +++ b/include/libcamera/internal/egl.h @@ -99,11 +99,11 @@ private: class eGL { public: - eGL(); + eGL(EGLDisplay display); ~eGL(); int initEGLContext(); - static bool isAvailable(); + static EGLDisplay probeDisplay(); int createInputDMABufTexture2D(eGLImage &eglImage, int fd); int createOutputDMABufTexture2D(eGLImage &eglImage, int fd); @@ -137,7 +137,6 @@ private: EGLContext context_ = EGL_NO_CONTEXT; EGLSurface surface_ = EGL_NO_SURFACE; - static EGLDisplay probeDisplay(); int compileShader(int shaderType, GLuint &shaderId, Span<const unsigned char> shaderData, Span<const std::string> shaderEnv); diff --git a/src/libcamera/egl.cpp b/src/libcamera/egl.cpp index 7ec7a654da..3d1a15493a 100644 --- a/src/libcamera/egl.cpp +++ b/src/libcamera/egl.cpp @@ -64,12 +64,15 @@ LOG_DEFINE_CATEGORY(eGL) /** * \brief Construct an EGL helper + * \param[in] display The EGL display to use * * Creates an eGL instance with uninitialised context. Call initEGLContext() - * to set up the EGL display, context, and load extension functions. + * to set up the EGL context, and load extension functions. */ -eGL::eGL() +eGL::eGL(EGLDisplay display) + : display_(display) { + ASSERT(display_ != EGL_NO_DISPLAY); } /** @@ -299,6 +302,13 @@ void eGL::createTexture2D(eGLImage &eglImage, void *data) glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE); } +/** + * \brief Try to create an EGL display for surfaceless rendering + * + * Tries to create an EGL display for surfaceless rendering. + * + * \return A EGL display handle if successfull, otherwise \a EGL_NO_DISPLAY + */ EGLDisplay eGL::probeDisplay() { EGLDisplay display; @@ -325,24 +335,6 @@ EGLDisplay eGL::probeDisplay() return display; } -/** - * \brief Probe whether EGL surfaceless rendering is available - * - * Checks if an EGL surfaceless display can be obtained and initialised. - * The display is immediately terminated so that no resources are leaked. - * - * \return True if EGL surfaceless rendering is available, false otherwise - */ -bool eGL::isAvailable() -{ - EGLDisplay display = probeDisplay(); - if (display == EGL_NO_DISPLAY) - return false; - - eglTerminate(display); - return true; -} - /** * \brief Update a 2D texture already created * \param[in,out] eglImage EGL image to associate with the texture @@ -403,12 +395,6 @@ int eGL::initEGLContext() EGLint numConfigs; EGLConfig config; - display_ = probeDisplay(); - if (display_ == EGL_NO_DISPLAY) { - LOG(eGL, Error) << "Unable to probe display"; - goto fail; - } - LOG(eGL, Info) << "EGL: EGL_VERSION: " << eglQueryString(display_, EGL_VERSION); LOG(eGL, Info) << "EGL: EGL_VENDOR: " << eglQueryString(display_, EGL_VENDOR); LOG(eGL, Info) << "EGL: EGL_CLIENT_APIS: " << eglQueryString(display_, EGL_CLIENT_APIS); diff --git a/src/libcamera/software_isp/debayer_egl.cpp b/src/libcamera/software_isp/debayer_egl.cpp index dd20412194..20b478b1c3 100644 --- a/src/libcamera/software_isp/debayer_egl.cpp +++ b/src/libcamera/software_isp/debayer_egl.cpp @@ -40,9 +40,10 @@ namespace libcamera { * \brief Construct a DebayerEGL object * \param[in] stats Statistics processing object * \param[in] cm The camera manager + * \param[in] display The EGL display to use */ -DebayerEGL::DebayerEGL(std::unique_ptr<SwStatsCpu> stats, const CameraManager &cm) - : Debayer(cm), stats_(std::move(stats)) +DebayerEGL::DebayerEGL(std::unique_ptr<SwStatsCpu> stats, const CameraManager &cm, EGLDisplay display) + : Debayer(cm), stats_(std::move(stats)), egl_(display) { } diff --git a/src/libcamera/software_isp/debayer_egl.h b/src/libcamera/software_isp/debayer_egl.h index e613639f5c..30e51a4773 100644 --- a/src/libcamera/software_isp/debayer_egl.h +++ b/src/libcamera/software_isp/debayer_egl.h @@ -40,7 +40,7 @@ class CameraManager; class DebayerEGL : public Debayer { public: - DebayerEGL(std::unique_ptr<SwStatsCpu> stats, const CameraManager &cm); + DebayerEGL(std::unique_ptr<SwStatsCpu> stats, const CameraManager &cm, EGLDisplay display); ~DebayerEGL(); int configure(const StreamConfiguration &inputCfg, diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp index c73a16ce0a..c7165771c6 100644 --- a/src/libcamera/software_isp/software_isp.cpp +++ b/src/libcamera/software_isp/software_isp.cpp @@ -120,8 +120,9 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, } if (!softISPMode || softISPMode == "gpu") { - if (eGL::isAvailable()) { - debayer_ = std::make_unique<DebayerEGL>(std::move(stats), cm); + auto display = eGL::probeDisplay(); + if (display != EGL_NO_DISPLAY) { + debayer_ = std::make_unique<DebayerEGL>(std::move(stats), cm, display); } else { LOG(SoftwareIsp, Info) << "EGL not available, falling back to CPU debayer"; > + > + if (probed) > + return cachedDisplay; > + > + probed = true; > > if (!eglBindAPI(EGL_OPENGL_ES_API)) { > LOG(eGL, Info) << "API bind fail"; > return EGL_NO_DISPLAY; > } > > - display = eglGetPlatformDisplay(EGL_PLATFORM_SURFACELESS_MESA, > - EGL_DEFAULT_DISPLAY, > - nullptr); > + cachedDisplay = eglGetPlatformDisplay(EGL_PLATFORM_SURFACELESS_MESA, > + EGL_DEFAULT_DISPLAY, > + nullptr); > > - if (display == EGL_NO_DISPLAY) { > + if (cachedDisplay == EGL_NO_DISPLAY) { > LOG(eGL, Info) << "Unable to get EGL display"; > return EGL_NO_DISPLAY; > } > > - if (eglInitialize(display, nullptr, nullptr) != EGL_TRUE) { > + if (eglInitialize(cachedDisplay, nullptr, nullptr) != EGL_TRUE) { > LOG(eGL, Error) << "eglInitialize fail"; > + cachedDisplay = EGL_NO_DISPLAY; > return EGL_NO_DISPLAY; > } > > - return display; > + return cachedDisplay; > } > > /** > * \brief Probe whether EGL surfaceless rendering is available > * > * Checks if an EGL surfaceless display can be obtained and initialised. > - * The display is immediately terminated so that no resources are leaked. > + * The result is cached so that a subsequent call to initEGLContext() reuses > + * the already-initialised display without a redundant init/teardown cycle. > * > * \return True if EGL surfaceless rendering is available, false otherwise > */ > bool eGL::isAvailable() > { > - EGLDisplay display = probeDisplay(); > - if (display == EGL_NO_DISPLAY) > - return false; > - > - eglTerminate(display); > - return true; > + return probeDisplay() != EGL_NO_DISPLAY; > } > > /**
Hi Barnabás Pőcze, Thanks for your detailed review. I have sent out v2 named as "[v2] libcamera: egl: Pass EGLDisplay to eGL constructor to avoid double init". Regards, Qi Hou -----Original Message----- From: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> Sent: Friday, July 24, 2026 6:06 PM To: Qi Hou (OSS) <qi.hou@oss.nxp.com>; libcamera-devel@lists.libcamera.org Cc: Jared Hu <jared.hu@nxp.com>; Qi Hou <qi.hou@nxp.com>; Julien Vuillaumier <julien.vuillaumier@nxp.com> Subject: Re: [PATCH] libcamera: egl: Cache probed EGLDisplay to avoid redundant init/teardown 2026. 07. 24. 3:43 keltezéssel, qi.hou@oss.nxp.com írta: > From: Qi Hou <qi.hou@nxp.com> > > isAvailable() previously called probeDisplay() which ran eglBindAPI(), > eglGetPlatformDisplay() and eglInitialize(), then immediately > terminated the display with eglTerminate(). When SoftwareIsp > subsequently called initEGLContext(), probeDisplay() repeated a full initialisation sequence. > > Avoid this double init/teardown by caching the EGLDisplay handle > inside > probeDisplay() using static parameters. The first call initialises EGL > and stores the result, all later calls return the cached handle > without reinitialising. > > isAvailable() is simplified to return probeDisplay() != EGL_NO_DISPLAY > and no longer calls eglTerminate(), because the cached display should > be live for reuse by initEGLContext(). > --- > src/libcamera/egl.cpp | 31 +++++++++++++++++-------------- > 1 file changed, 17 insertions(+), 14 deletions(-) > > diff --git a/src/libcamera/egl.cpp b/src/libcamera/egl.cpp index > 7ec7a654d..f5200ee76 100644 > --- a/src/libcamera/egl.cpp > +++ b/src/libcamera/egl.cpp > @@ -301,46 +301,49 @@ void eGL::createTexture2D(eGLImage &eglImage, > void *data) > > EGLDisplay eGL::probeDisplay() > { > - EGLDisplay display; > + static EGLDisplay cachedDisplay = EGL_NO_DISPLAY; > + static bool probed = false; I'm not a fan of adding more mutable global state. As an alternative: * modify `eGL` to take the EGLDisplay in its constructor and adjust `initEGLContext()` to not retrieve the display * adjust `DebayerEGL` accordingly * adjust `SoftwareISP` to call `probeDisplay()` and forward the result Here is a prototype of the idea: diff --git a/include/libcamera/internal/egl.h b/include/libcamera/internal/egl.h index 1e31d490bd..7ef1ca0d93 100644 --- a/include/libcamera/internal/egl.h +++ b/include/libcamera/internal/egl.h @@ -99,11 +99,11 @@ private: class eGL { public: - eGL(); + eGL(EGLDisplay display); ~eGL(); int initEGLContext(); - static bool isAvailable(); + static EGLDisplay probeDisplay(); int createInputDMABufTexture2D(eGLImage &eglImage, int fd); int createOutputDMABufTexture2D(eGLImage &eglImage, int fd); @@ -137,7 +137,6 @@ private: EGLContext context_ = EGL_NO_CONTEXT; EGLSurface surface_ = EGL_NO_SURFACE; - static EGLDisplay probeDisplay(); int compileShader(int shaderType, GLuint &shaderId, Span<const unsigned char> shaderData, Span<const std::string> shaderEnv); diff --git a/src/libcamera/egl.cpp b/src/libcamera/egl.cpp index 7ec7a654da..3d1a15493a 100644 --- a/src/libcamera/egl.cpp +++ b/src/libcamera/egl.cpp @@ -64,12 +64,15 @@ LOG_DEFINE_CATEGORY(eGL) /** * \brief Construct an EGL helper + * \param[in] display The EGL display to use * * Creates an eGL instance with uninitialised context. Call initEGLContext() - * to set up the EGL display, context, and load extension functions. + * to set up the EGL context, and load extension functions. */ -eGL::eGL() +eGL::eGL(EGLDisplay display) + : display_(display) { + ASSERT(display_ != EGL_NO_DISPLAY); } /** @@ -299,6 +302,13 @@ void eGL::createTexture2D(eGLImage &eglImage, void *data) glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE); } +/** + * \brief Try to create an EGL display for surfaceless rendering + * + * Tries to create an EGL display for surfaceless rendering. + * + * \return A EGL display handle if successfull, otherwise \a +EGL_NO_DISPLAY */ EGLDisplay eGL::probeDisplay() { EGLDisplay display; @@ -325,24 +335,6 @@ EGLDisplay eGL::probeDisplay() return display; } -/** - * \brief Probe whether EGL surfaceless rendering is available - * - * Checks if an EGL surfaceless display can be obtained and initialised. - * The display is immediately terminated so that no resources are leaked. - * - * \return True if EGL surfaceless rendering is available, false otherwise - */ -bool eGL::isAvailable() -{ - EGLDisplay display = probeDisplay(); - if (display == EGL_NO_DISPLAY) - return false; - - eglTerminate(display); - return true; -} - /** * \brief Update a 2D texture already created * \param[in,out] eglImage EGL image to associate with the texture @@ -403,12 +395,6 @@ int eGL::initEGLContext() EGLint numConfigs; EGLConfig config; - display_ = probeDisplay(); - if (display_ == EGL_NO_DISPLAY) { - LOG(eGL, Error) << "Unable to probe display"; - goto fail; - } - LOG(eGL, Info) << "EGL: EGL_VERSION: " << eglQueryString(display_, EGL_VERSION); LOG(eGL, Info) << "EGL: EGL_VENDOR: " << eglQueryString(display_, EGL_VENDOR); LOG(eGL, Info) << "EGL: EGL_CLIENT_APIS: " << eglQueryString(display_, EGL_CLIENT_APIS); diff --git a/src/libcamera/software_isp/debayer_egl.cpp b/src/libcamera/software_isp/debayer_egl.cpp index dd20412194..20b478b1c3 100644 --- a/src/libcamera/software_isp/debayer_egl.cpp +++ b/src/libcamera/software_isp/debayer_egl.cpp @@ -40,9 +40,10 @@ namespace libcamera { * \brief Construct a DebayerEGL object * \param[in] stats Statistics processing object * \param[in] cm The camera manager + * \param[in] display The EGL display to use */ -DebayerEGL::DebayerEGL(std::unique_ptr<SwStatsCpu> stats, const CameraManager &cm) - : Debayer(cm), stats_(std::move(stats)) +DebayerEGL::DebayerEGL(std::unique_ptr<SwStatsCpu> stats, const CameraManager &cm, EGLDisplay display) + : Debayer(cm), stats_(std::move(stats)), egl_(display) { } diff --git a/src/libcamera/software_isp/debayer_egl.h b/src/libcamera/software_isp/debayer_egl.h index e613639f5c..30e51a4773 100644 --- a/src/libcamera/software_isp/debayer_egl.h +++ b/src/libcamera/software_isp/debayer_egl.h @@ -40,7 +40,7 @@ class CameraManager; class DebayerEGL : public Debayer { public: - DebayerEGL(std::unique_ptr<SwStatsCpu> stats, const CameraManager &cm); + DebayerEGL(std::unique_ptr<SwStatsCpu> stats, const CameraManager &cm, +EGLDisplay display); ~DebayerEGL(); int configure(const StreamConfiguration &inputCfg, diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp index c73a16ce0a..c7165771c6 100644 --- a/src/libcamera/software_isp/software_isp.cpp +++ b/src/libcamera/software_isp/software_isp.cpp @@ -120,8 +120,9 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, } if (!softISPMode || softISPMode == "gpu") { - if (eGL::isAvailable()) { - debayer_ = std::make_unique<DebayerEGL>(std::move(stats), cm); + auto display = eGL::probeDisplay(); + if (display != EGL_NO_DISPLAY) { + debayer_ = std::make_unique<DebayerEGL>(std::move(stats), cm, +display); } else { LOG(SoftwareIsp, Info) << "EGL not available, falling back to CPU debayer"; > + > + if (probed) > + return cachedDisplay; > + > + probed = true; > > if (!eglBindAPI(EGL_OPENGL_ES_API)) { > LOG(eGL, Info) << "API bind fail"; > return EGL_NO_DISPLAY; > } > > - display = eglGetPlatformDisplay(EGL_PLATFORM_SURFACELESS_MESA, > - EGL_DEFAULT_DISPLAY, > - nullptr); > + cachedDisplay = eglGetPlatformDisplay(EGL_PLATFORM_SURFACELESS_MESA, > + EGL_DEFAULT_DISPLAY, > + nullptr); > > - if (display == EGL_NO_DISPLAY) { > + if (cachedDisplay == EGL_NO_DISPLAY) { > LOG(eGL, Info) << "Unable to get EGL display"; > return EGL_NO_DISPLAY; > } > > - if (eglInitialize(display, nullptr, nullptr) != EGL_TRUE) { > + if (eglInitialize(cachedDisplay, nullptr, nullptr) != EGL_TRUE) { > LOG(eGL, Error) << "eglInitialize fail"; > + cachedDisplay = EGL_NO_DISPLAY; > return EGL_NO_DISPLAY; > } > > - return display; > + return cachedDisplay; > } > > /** > * \brief Probe whether EGL surfaceless rendering is available > * > * Checks if an EGL surfaceless display can be obtained and initialised. > - * The display is immediately terminated so that no resources are leaked. > + * The result is cached so that a subsequent call to initEGLContext() > + reuses > + * the already-initialised display without a redundant init/teardown cycle. > * > * \return True if EGL surfaceless rendering is available, false otherwise > */ > bool eGL::isAvailable() > { > - EGLDisplay display = probeDisplay(); > - if (display == EGL_NO_DISPLAY) > - return false; > - > - eglTerminate(display); > - return true; > + return probeDisplay() != EGL_NO_DISPLAY; > } > > /**
diff --git a/src/libcamera/egl.cpp b/src/libcamera/egl.cpp index 7ec7a654d..f5200ee76 100644 --- a/src/libcamera/egl.cpp +++ b/src/libcamera/egl.cpp @@ -301,46 +301,49 @@ void eGL::createTexture2D(eGLImage &eglImage, void *data) EGLDisplay eGL::probeDisplay() { - EGLDisplay display; + static EGLDisplay cachedDisplay = EGL_NO_DISPLAY; + static bool probed = false; + + if (probed) + return cachedDisplay; + + probed = true; if (!eglBindAPI(EGL_OPENGL_ES_API)) { LOG(eGL, Info) << "API bind fail"; return EGL_NO_DISPLAY; } - display = eglGetPlatformDisplay(EGL_PLATFORM_SURFACELESS_MESA, - EGL_DEFAULT_DISPLAY, - nullptr); + cachedDisplay = eglGetPlatformDisplay(EGL_PLATFORM_SURFACELESS_MESA, + EGL_DEFAULT_DISPLAY, + nullptr); - if (display == EGL_NO_DISPLAY) { + if (cachedDisplay == EGL_NO_DISPLAY) { LOG(eGL, Info) << "Unable to get EGL display"; return EGL_NO_DISPLAY; } - if (eglInitialize(display, nullptr, nullptr) != EGL_TRUE) { + if (eglInitialize(cachedDisplay, nullptr, nullptr) != EGL_TRUE) { LOG(eGL, Error) << "eglInitialize fail"; + cachedDisplay = EGL_NO_DISPLAY; return EGL_NO_DISPLAY; } - return display; + return cachedDisplay; } /** * \brief Probe whether EGL surfaceless rendering is available * * Checks if an EGL surfaceless display can be obtained and initialised. - * The display is immediately terminated so that no resources are leaked. + * The result is cached so that a subsequent call to initEGLContext() reuses + * the already-initialised display without a redundant init/teardown cycle. * * \return True if EGL surfaceless rendering is available, false otherwise */ bool eGL::isAvailable() { - EGLDisplay display = probeDisplay(); - if (display == EGL_NO_DISPLAY) - return false; - - eglTerminate(display); - return true; + return probeDisplay() != EGL_NO_DISPLAY; } /**
From: Qi Hou <qi.hou@nxp.com> isAvailable() previously called probeDisplay() which ran eglBindAPI(), eglGetPlatformDisplay() and eglInitialize(), then immediately terminated the display with eglTerminate(). When SoftwareIsp subsequently called initEGLContext(), probeDisplay() repeated a full initialisation sequence. Avoid this double init/teardown by caching the EGLDisplay handle inside probeDisplay() using static parameters. The first call initialises EGL and stores the result, all later calls return the cached handle without reinitialising. isAvailable() is simplified to return probeDisplay() != EGL_NO_DISPLAY and no longer calls eglTerminate(), because the cached display should be live for reuse by initEGLContext(). --- src/libcamera/egl.cpp | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-)