| Message ID | 20260610093812.3604734-1-qi.hou@nxp.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi, thank you for the patch. Qi Hou <qi.hou@nxp.com> writes: > When HAVE_DEBAYER_EGL is enabled, the SoftwareIsp constructor > unconditionally creates a DebayerEGL instance. If the platform lacks > EGL surfaceless support, DebayerEGL::start() fails with -ENODEV after > SwStatsCpu ownership has already been moved, making fallback to > DebayerCpu impossible. > > Add a static eGL::isAvailable() method that probes whether EGL > surfaceless rendering can be initialised. Extract the common display > obtaining logic (eglBindAPI, eglGetPlatformDisplay, eglInitialize) > into a private eGL::probeDisplay() helper that is shared by both > isAvailable() and initEGLContext(), avoiding code duplication. > > The SoftwareIsp constructor now calls eGL::isAvailable() before > creating DebayerEGL. If EGL is not available, the existing fallback > path creates a DebayerCpu instance instead. > > Signed-off-by: Qi Hou <qi.hou@nxp.com> > --- > include/libcamera/internal/egl.h | 4 ++ > src/libcamera/egl.cpp | 60 ++++++++++++++++----- > src/libcamera/software_isp/software_isp.cpp | 10 +++- > 3 files changed, 58 insertions(+), 16 deletions(-) > > diff --git a/include/libcamera/internal/egl.h b/include/libcamera/internal/egl.h > index 57f90d93..27c5c29d 100644 > --- a/include/libcamera/internal/egl.h > +++ b/include/libcamera/internal/egl.h > @@ -99,6 +99,8 @@ private: > class eGL > { > public: > + static bool isAvailable(); I'd put this below the constructor/destructor declarations, attached to initEGLContext declaration. > + > eGL(); > ~eGL(); > > @@ -127,6 +129,8 @@ public: > private: > LIBCAMERA_DISABLE_COPY_AND_MOVE(eGL) > > + static EGLDisplay probeDisplay(); > + And this close to the methods declarations. Otherwise the patch looks good to me. > pid_t tid_; > > EGLDisplay display_ = EGL_NO_DISPLAY; > diff --git a/src/libcamera/egl.cpp b/src/libcamera/egl.cpp > index c185bb7a..860f18dd 100644 > --- a/src/libcamera/egl.cpp > +++ b/src/libcamera/egl.cpp > @@ -267,6 +267,50 @@ void eGL::createTexture2D(eGLImage &eglImage, void *data) > glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE); > } > > +EGLDisplay eGL::probeDisplay() > +{ > + EGLDisplay display; > + > + 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); > + > + if (display == EGL_NO_DISPLAY) { > + LOG(eGL, Info) << "Unable to get EGL display"; > + return EGL_NO_DISPLAY; > + } > + > + if (eglInitialize(display, nullptr, nullptr) != EGL_TRUE) { > + LOG(eGL, Error) << "eglInitialize fail"; > + return EGL_NO_DISPLAY; > + } > + > + 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 Initialise the EGL context > * > @@ -297,21 +341,9 @@ int eGL::initEGLContext() > EGLint numConfigs; > EGLConfig config; > > - if (!eglBindAPI(EGL_OPENGL_ES_API)) { > - LOG(eGL, Error) << "API bind fail"; > - goto fail; > - } > - > - display_ = eglGetPlatformDisplay(EGL_PLATFORM_SURFACELESS_MESA, > - EGL_DEFAULT_DISPLAY, > - nullptr); > + display_ = probeDisplay(); > if (display_ == EGL_NO_DISPLAY) { > - LOG(eGL, Error) << "Unable to get EGL display"; > - goto fail; > - } > - > - if (eglInitialize(display_, nullptr, nullptr) != EGL_TRUE) { > - LOG(eGL, Error) << "eglInitialize fail"; > + LOG(eGL, Error) << "Unable to probe display"; > goto fail; > } > > diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp > index 781cf02f..ff8c3465 100644 > --- a/src/libcamera/software_isp/software_isp.cpp > +++ b/src/libcamera/software_isp/software_isp.cpp > @@ -119,8 +119,14 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, > } > } > > - if (!softISPMode || softISPMode == "gpu") > - debayer_ = std::make_unique<DebayerEGL>(std::move(stats), cm); > + if (!softISPMode || softISPMode == "gpu") { > + if (eGL::isAvailable()) { > + debayer_ = std::make_unique<DebayerEGL>(std::move(stats), cm); > + } else { > + LOG(SoftwareIsp, Info) > + << "EGL not available, falling back to CPU debayer"; > + } > + } > > #endif > if (!debayer_)
Hi Milan Zamazal, Thank you for your comments. I have updated the patch and sent out v3. Please help review. Regards, Qi Hou -----Original Message----- From: Milan Zamazal <mzamazal@redhat.com> Sent: Thursday, June 11, 2026 3:52 PM To: Qi Hou <qi.hou@nxp.com> Cc: libcamera-devel@lists.libcamera.org; Jared Hu <jared.hu@nxp.com>; Julien Vuillaumier <julien.vuillaumier@nxp.com> Subject: [EXT] Re: [PATCH v2] libcamera: software_isp: Probe EGL availability before creating DebayerEGL [You don't often get email from mzamazal@redhat.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button Hi, thank you for the patch. Qi Hou <qi.hou@nxp.com> writes: > When HAVE_DEBAYER_EGL is enabled, the SoftwareIsp constructor > unconditionally creates a DebayerEGL instance. If the platform lacks > EGL surfaceless support, DebayerEGL::start() fails with -ENODEV after > SwStatsCpu ownership has already been moved, making fallback to > DebayerCpu impossible. > > Add a static eGL::isAvailable() method that probes whether EGL > surfaceless rendering can be initialised. Extract the common display > obtaining logic (eglBindAPI, eglGetPlatformDisplay, eglInitialize) > into a private eGL::probeDisplay() helper that is shared by both > isAvailable() and initEGLContext(), avoiding code duplication. > > The SoftwareIsp constructor now calls eGL::isAvailable() before > creating DebayerEGL. If EGL is not available, the existing fallback > path creates a DebayerCpu instance instead. > > Signed-off-by: Qi Hou <qi.hou@nxp.com> > --- > include/libcamera/internal/egl.h | 4 ++ > src/libcamera/egl.cpp | 60 ++++++++++++++++----- > src/libcamera/software_isp/software_isp.cpp | 10 +++- > 3 files changed, 58 insertions(+), 16 deletions(-) > > diff --git a/include/libcamera/internal/egl.h > b/include/libcamera/internal/egl.h > index 57f90d93..27c5c29d 100644 > --- a/include/libcamera/internal/egl.h > +++ b/include/libcamera/internal/egl.h > @@ -99,6 +99,8 @@ private: > class eGL > { > public: > + static bool isAvailable(); I'd put this below the constructor/destructor declarations, attached to initEGLContext declaration. > + > eGL(); > ~eGL(); > > @@ -127,6 +129,8 @@ public: > private: > LIBCAMERA_DISABLE_COPY_AND_MOVE(eGL) > > + static EGLDisplay probeDisplay(); > + And this close to the methods declarations. Otherwise the patch looks good to me. > pid_t tid_; > > EGLDisplay display_ = EGL_NO_DISPLAY; diff --git > a/src/libcamera/egl.cpp b/src/libcamera/egl.cpp index > c185bb7a..860f18dd 100644 > --- a/src/libcamera/egl.cpp > +++ b/src/libcamera/egl.cpp > @@ -267,6 +267,50 @@ void eGL::createTexture2D(eGLImage &eglImage, void *data) > glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, > GL_CLAMP_TO_EDGE); } > > +EGLDisplay eGL::probeDisplay() > +{ > + EGLDisplay display; > + > + 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); > + > + if (display == EGL_NO_DISPLAY) { > + LOG(eGL, Info) << "Unable to get EGL display"; > + return EGL_NO_DISPLAY; > + } > + > + if (eglInitialize(display, nullptr, nullptr) != EGL_TRUE) { > + LOG(eGL, Error) << "eglInitialize fail"; > + return EGL_NO_DISPLAY; > + } > + > + 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 Initialise the EGL context > * > @@ -297,21 +341,9 @@ int eGL::initEGLContext() > EGLint numConfigs; > EGLConfig config; > > - if (!eglBindAPI(EGL_OPENGL_ES_API)) { > - LOG(eGL, Error) << "API bind fail"; > - goto fail; > - } > - > - display_ = eglGetPlatformDisplay(EGL_PLATFORM_SURFACELESS_MESA, > - EGL_DEFAULT_DISPLAY, > - nullptr); > + display_ = probeDisplay(); > if (display_ == EGL_NO_DISPLAY) { > - LOG(eGL, Error) << "Unable to get EGL display"; > - goto fail; > - } > - > - if (eglInitialize(display_, nullptr, nullptr) != EGL_TRUE) { > - LOG(eGL, Error) << "eglInitialize fail"; > + LOG(eGL, Error) << "Unable to probe display"; > goto fail; > } > > diff --git a/src/libcamera/software_isp/software_isp.cpp > b/src/libcamera/software_isp/software_isp.cpp > index 781cf02f..ff8c3465 100644 > --- a/src/libcamera/software_isp/software_isp.cpp > +++ b/src/libcamera/software_isp/software_isp.cpp > @@ -119,8 +119,14 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, > } > } > > - if (!softISPMode || softISPMode == "gpu") > - debayer_ = std::make_unique<DebayerEGL>(std::move(stats), cm); > + if (!softISPMode || softISPMode == "gpu") { > + if (eGL::isAvailable()) { > + debayer_ = std::make_unique<DebayerEGL>(std::move(stats), cm); > + } else { > + LOG(SoftwareIsp, Info) > + << "EGL not available, falling back to CPU debayer"; > + } > + } > > #endif > if (!debayer_)
diff --git a/include/libcamera/internal/egl.h b/include/libcamera/internal/egl.h index 57f90d93..27c5c29d 100644 --- a/include/libcamera/internal/egl.h +++ b/include/libcamera/internal/egl.h @@ -99,6 +99,8 @@ private: class eGL { public: + static bool isAvailable(); + eGL(); ~eGL(); @@ -127,6 +129,8 @@ public: private: LIBCAMERA_DISABLE_COPY_AND_MOVE(eGL) + static EGLDisplay probeDisplay(); + pid_t tid_; EGLDisplay display_ = EGL_NO_DISPLAY; diff --git a/src/libcamera/egl.cpp b/src/libcamera/egl.cpp index c185bb7a..860f18dd 100644 --- a/src/libcamera/egl.cpp +++ b/src/libcamera/egl.cpp @@ -267,6 +267,50 @@ void eGL::createTexture2D(eGLImage &eglImage, void *data) glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE); } +EGLDisplay eGL::probeDisplay() +{ + EGLDisplay display; + + 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); + + if (display == EGL_NO_DISPLAY) { + LOG(eGL, Info) << "Unable to get EGL display"; + return EGL_NO_DISPLAY; + } + + if (eglInitialize(display, nullptr, nullptr) != EGL_TRUE) { + LOG(eGL, Error) << "eglInitialize fail"; + return EGL_NO_DISPLAY; + } + + 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 Initialise the EGL context * @@ -297,21 +341,9 @@ int eGL::initEGLContext() EGLint numConfigs; EGLConfig config; - if (!eglBindAPI(EGL_OPENGL_ES_API)) { - LOG(eGL, Error) << "API bind fail"; - goto fail; - } - - display_ = eglGetPlatformDisplay(EGL_PLATFORM_SURFACELESS_MESA, - EGL_DEFAULT_DISPLAY, - nullptr); + display_ = probeDisplay(); if (display_ == EGL_NO_DISPLAY) { - LOG(eGL, Error) << "Unable to get EGL display"; - goto fail; - } - - if (eglInitialize(display_, nullptr, nullptr) != EGL_TRUE) { - LOG(eGL, Error) << "eglInitialize fail"; + LOG(eGL, Error) << "Unable to probe display"; goto fail; } diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp index 781cf02f..ff8c3465 100644 --- a/src/libcamera/software_isp/software_isp.cpp +++ b/src/libcamera/software_isp/software_isp.cpp @@ -119,8 +119,14 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, } } - if (!softISPMode || softISPMode == "gpu") - debayer_ = std::make_unique<DebayerEGL>(std::move(stats), cm); + if (!softISPMode || softISPMode == "gpu") { + if (eGL::isAvailable()) { + debayer_ = std::make_unique<DebayerEGL>(std::move(stats), cm); + } else { + LOG(SoftwareIsp, Info) + << "EGL not available, falling back to CPU debayer"; + } + } #endif if (!debayer_)
When HAVE_DEBAYER_EGL is enabled, the SoftwareIsp constructor unconditionally creates a DebayerEGL instance. If the platform lacks EGL surfaceless support, DebayerEGL::start() fails with -ENODEV after SwStatsCpu ownership has already been moved, making fallback to DebayerCpu impossible. Add a static eGL::isAvailable() method that probes whether EGL surfaceless rendering can be initialised. Extract the common display obtaining logic (eglBindAPI, eglGetPlatformDisplay, eglInitialize) into a private eGL::probeDisplay() helper that is shared by both isAvailable() and initEGLContext(), avoiding code duplication. The SoftwareIsp constructor now calls eGL::isAvailable() before creating DebayerEGL. If EGL is not available, the existing fallback path creates a DebayerCpu instance instead. Signed-off-by: Qi Hou <qi.hou@nxp.com> --- include/libcamera/internal/egl.h | 4 ++ src/libcamera/egl.cpp | 60 ++++++++++++++++----- src/libcamera/software_isp/software_isp.cpp | 10 +++- 3 files changed, 58 insertions(+), 16 deletions(-)