| Message ID | 20260727064152.2161526-1-qi.hou@oss.nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
2026. 07. 27. 8:41 keltezéssel, qi.hou@oss.nxp.com írta: > From: Qi Hou <qi.hou@nxp.com> > > isAvailable() previously called probeDisplay() to run a full EGL > initialisation sequence and then immediately terminated the display with > eglTerminate(). When SoftwareIsp subsequently created a DebayerEGL > instance, initEGLContext() would call probeDisplay() again, repeating > the full initialisation. > > Fix this by reusing the display obtained during the availability check. > probeDisplay() is promoted to a public static method and isAvailable() > is removed. The eGL constructor now takes the EGLDisplay as a parameter > and stores it directly, so initEGLContext() no longer needs to call > probeDisplay(). DebayerEGL is updated to accept and forward the display > to eGL, and SoftwareIsp calls eGL::probeDisplay() once, passing the > result straight to DebayerEGL when EGL is available. So this was my suggestion... and of course there is likely an issue. While I have checked, and as far as I can tell `eglGetPlatformDisplay()` should work when called in one thread and used in another; `eglBindAPI()` is directly modifying the thread state, so this change is not ideal (even though it did appear to work on mesa). I'm wondering if `eglGetPlatformDisplay()` and `eglInitialize()` work without `eglBindAPI()`, if that's the case, then I think the easy fix is to move the `eglBindAPI()` call into `initEGLContext()`. The specification (https://registry.khronos.org/EGL/specs/eglspec.1.5.pdf # 3.7) says: Some of the functions described in this section make use of the current rendering API, [...] [...] Applications using multiple client APIs are responsible for ensuring the current rendering API is correct before calling the functions eglCreateContext, eglGetCurrentContext, eglGetCurrentDisplay, eglGetCurrentSurface, eglCopyBuffers, eglSwapBuffers, eglSwapInterval, eglMakeCurrent (when its ctx parameter is EGL_NO_CONTEXT), eglWaitClient, or eglWaitNative. So I think just moving `eglBindAPI()` should be fine? > > Signed-off-by: Qi Hou <qi.hou@nxp.com> > --- > include/libcamera/internal/egl.h | 5 ++- > src/libcamera/egl.cpp | 38 +++++++-------------- > src/libcamera/software_isp/debayer_egl.cpp | 5 +-- > src/libcamera/software_isp/debayer_egl.h | 2 +- > src/libcamera/software_isp/software_isp.cpp | 5 +-- > 5 files changed, 21 insertions(+), 34 deletions(-) > > diff --git a/include/libcamera/internal/egl.h b/include/libcamera/internal/egl.h > index 1e31d490b..7ef1ca0d9 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 7ec7a654d..b5ed0e13a 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 successful, 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 dd2041219..20b478b1c 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 e613639f5..30e51a477 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 c73a16ce0..c7165771c 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";
diff --git a/include/libcamera/internal/egl.h b/include/libcamera/internal/egl.h index 1e31d490b..7ef1ca0d9 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 7ec7a654d..b5ed0e13a 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 successful, 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 dd2041219..20b478b1c 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 e613639f5..30e51a477 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 c73a16ce0..c7165771c 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";