[1/3] egl: Add more parameters to createInputDMABufTexture2D()
diff mbox series

Message ID 20260503114002.139255-2-robert.mader@collabora.com
State Superseded
Headers show
Series
  • software_isp: Implement DMABuf import for input buffers
Related show

Commit Message

Robert Mader May 3, 2026, 11:40 a.m. UTC 
In order to match createTexture2D(). This will allow us to replace the
later with the former going forward.

Also demote an error log to debug, avoiding flodding the logs in cases
where import failure is an expected result.

Signed-off-by: Robert Mader <robert.mader@collabora.com>
---
 include/libcamera/internal/egl.h |  4 ++--
 src/libcamera/egl.cpp            | 37 +++++++++++++++++++++++++-------
 2 files changed, 31 insertions(+), 10 deletions(-)

Comments

Kieran Bingham May 3, 2026, 12:34 p.m. UTC | #1 
Quoting Robert Mader (2026-05-03 12:40:00)
> In order to match createTexture2D(). This will allow us to replace the
> later with the former going forward.
> 
> Also demote an error log to debug, avoiding flodding the logs in cases
> where import failure is an expected result.
> 
> Signed-off-by: Robert Mader <robert.mader@collabora.com>
> ---
>  include/libcamera/internal/egl.h |  4 ++--
>  src/libcamera/egl.cpp            | 37 +++++++++++++++++++++++++-------
>  2 files changed, 31 insertions(+), 10 deletions(-)
> 
> diff --git a/include/libcamera/internal/egl.h b/include/libcamera/internal/egl.h
> index 0ad2320b1..bd2f20e33 100644
> --- a/include/libcamera/internal/egl.h
> +++ b/include/libcamera/internal/egl.h
> @@ -101,7 +101,7 @@ public:
>  
>         int initEGLContext();
>  
> -       int createInputDMABufTexture2D(eGLImage &eglImage, int fd);
> +       int createInputDMABufTexture2D(eGLImage &eglImage, GLint format, uint32_t width, uint32_t height, int fd);
>         int createOutputDMABufTexture2D(eGLImage &eglImage, int fd);
>         void createTexture2D(eGLImage &eglImage, GLint format, uint32_t width, uint32_t height, void *data);
>  
> @@ -133,7 +133,7 @@ private:
>                           unsigned int shaderDataLen,
>                           Span<const std::string> shaderEnv);
>  
> -       int createDMABufTexture2D(eGLImage &eglImage, int fd, bool output);
> +       int createDMABufTexture2D(eGLImage &eglImage, int fd, uint32_t drm_format, uint32_t width, uint32_t height, bool output);
>  
>         PFNGLEGLIMAGETARGETTEXTURE2DOESPROC glEGLImageTargetTexture2DOES;
>         PFNEGLCREATEIMAGEKHRPROC eglCreateImageKHR;
> diff --git a/src/libcamera/egl.cpp b/src/libcamera/egl.cpp
> index f65929470..c30ed95a7 100644
> --- a/src/libcamera/egl.cpp
> +++ b/src/libcamera/egl.cpp
> @@ -19,6 +19,7 @@
>  
>  #include <libcamera/base/thread.h>
>  
> +#include <GLES3/gl32.h>
>  #include <libdrm/drm_fourcc.h>
>  
>  namespace libcamera {
> @@ -102,6 +103,9 @@ void eGL::syncOutput()
>   * \brief Create a DMA-BUF backed 2D texture
>   * \param[in,out] eglImage EGL image to associate with the DMA-BUF
>   * \param[in] fd DMA-BUF file descriptor
> + * \param[in] drm_format the DRM fourcc
> + * \param[in] width the buffer width
> + * \param[in] height the buffer height
>   * \param[in] output If true, create framebuffer for render target
>   *
>   * Internal implementation for creating DMA-BUF textures. Creates an EGL
> @@ -110,15 +114,15 @@ void eGL::syncOutput()
>   *
>   * \return 0 on success, or -ENODEV on failure
>   */
> -int eGL::createDMABufTexture2D(eGLImage &eglImage, int fd, bool output)
> +int eGL::createDMABufTexture2D(eGLImage &eglImage, int fd, uint32_t drm_format, uint32_t width, uint32_t height, bool output)
>  {
>         ASSERT(tid_ == Thread::currentId());
>  
>         // clang-format off
>         EGLint image_attrs[] = {
> -               EGL_WIDTH, (EGLint)eglImage.width_,
> -               EGL_HEIGHT, (EGLint)eglImage.height_,
> -               EGL_LINUX_DRM_FOURCC_EXT, DRM_FORMAT_ARGB8888,
> +               EGL_WIDTH, (EGLint)width,
> +               EGL_HEIGHT, (EGLint)height,
> +               EGL_LINUX_DRM_FOURCC_EXT, (EGLint)drm_format,
>                 EGL_DMA_BUF_PLANE0_FD_EXT, fd,
>                 EGL_DMA_BUF_PLANE0_OFFSET_EXT, 0,
>                 EGL_DMA_BUF_PLANE0_PITCH_EXT, (EGLint)eglImage.stride_,
> @@ -133,7 +137,7 @@ int eGL::createDMABufTexture2D(eGLImage &eglImage, int fd, bool output)
>                                               NULL, image_attrs);
>  
>         if (image == EGL_NO_IMAGE_KHR) {
> -               LOG(eGL, Error) << "eglCreateImageKHR fail";
> +               LOG(eGL, Debug) << "eglCreateImageKHR fail";
>                 return -ENODEV;
>         }
>  
> @@ -171,6 +175,9 @@ int eGL::createDMABufTexture2D(eGLImage &eglImage, int fd, bool output)
>  /**
>   * \brief Create an input DMA-BUF backed texture
>   * \param[in,out] eglImage EGL image to associate with the DMA-BUF
> + * \param[in] format the GL format
> + * \param[in] width the buffer width
> + * \param[in] height the buffer height
>   * \param[in] fd DMA-BUF file descriptor
>   *
>   * Creates an EGL image from a DMA-BUF file descriptor and binds it to
> @@ -179,11 +186,25 @@ int eGL::createDMABufTexture2D(eGLImage &eglImage, int fd, bool output)
>   *
>   * \return 0 on success, or -ENODEV on failure
>   */
> -int eGL::createInputDMABufTexture2D(eGLImage &eglImage, int fd)
> +int eGL::createInputDMABufTexture2D(eGLImage &eglImage, GLint format, uint32_t width, uint32_t height, int fd)
>  {
> +       EGLint drm_format;
> +
>         ASSERT(tid_ == Thread::currentId());
>  
> -       return createDMABufTexture2D(eglImage, fd, false);
> +       switch (format) {
> +       case GL_LUMINANCE:
> +               drm_format = DRM_FORMAT_R8;
> +               break;
> +       case GL_RG:
> +               drm_format = DRM_FORMAT_RG88;
> +               break;
> +       default:
> +               LOG(eGL, Error) << "unhandled GL format";
> +               return -ENODEV;
> +       }
> +
> +       return createDMABufTexture2D(eglImage, fd, drm_format, width, height, false);
>  }
>  
>  /**
> @@ -202,7 +223,7 @@ int eGL::createOutputDMABufTexture2D(eGLImage &eglImage, int fd)
>  {
>         ASSERT(tid_ == Thread::currentId());
>  
> -       return createDMABufTexture2D(eglImage, fd, true);
> +       return createDMABufTexture2D(eglImage, fd, DRM_FORMAT_ARGB8888, eglImage.width_, eglImage.height_, true);

This looks like something wrong perhaps crept in, unless I'm
misunderstanding or there's another overload - here we have 

eGL::createInputDMABufTexture2D(eGLImage &eglImage, GLint format, uint32_t width, uint32_t height, int fd)

but this usage is

  eGLImage, fd, format, width, height,  bool ?

>  }
>  
>  /**
> -- 
> 2.54.0
>
Robert Mader May 3, 2026, 10:32 p.m. UTC | #2 
Hi,

On 03.05.26 14:34, Kieran Bingham wrote:
>> ...
>>   
>>   /**
>> @@ -202,7 +223,7 @@ int eGL::createOutputDMABufTexture2D(eGLImage &eglImage, int fd)
>>   {
>>          ASSERT(tid_ == Thread::currentId());
>>   
>> -       return createDMABufTexture2D(eglImage, fd, true);
>> +       return createDMABufTexture2D(eglImage, fd, DRM_FORMAT_ARGB8888, eglImage.width_, eglImage.height_, true);
> This looks like something wrong perhaps crept in, unless I'm
> misunderstanding or there's another overload - here we have
>
> eGL::createInputDMABufTexture2D(eGLImage &eglImage, GLint format, uint32_t width, uint32_t height, int fd)
>
> but this usage is
>
>    eGLImage, fd, format, width, height,  bool ?

I think you're confusing `createDMABufTexture2D()` and 
`createInputDMABufTexture2D()` ;)

FWIW., IMO we could consider dropping `createInputDMABufTexture2D()` and 
`createOutputDMABufTexture2D()` - both small wrappers around 
`createDMABufTexture2D()` - and directly expose the later.
Kieran Bingham May 4, 2026, 7:22 a.m. UTC | #3 
Quoting Robert Mader (2026-05-03 23:32:23)
> Hi,
> 
> On 03.05.26 14:34, Kieran Bingham wrote:
> >> ...
> >>   
> >>   /**
> >> @@ -202,7 +223,7 @@ int eGL::createOutputDMABufTexture2D(eGLImage &eglImage, int fd)
> >>   {
> >>          ASSERT(tid_ == Thread::currentId());
> >>   
> >> -       return createDMABufTexture2D(eglImage, fd, true);
> >> +       return createDMABufTexture2D(eglImage, fd, DRM_FORMAT_ARGB8888, eglImage.width_, eglImage.height_, true);
> > This looks like something wrong perhaps crept in, unless I'm
> > misunderstanding or there's another overload - here we have
> >
> > eGL::createInputDMABufTexture2D(eGLImage &eglImage, GLint format, uint32_t width, uint32_t height, int fd)
> >
> > but this usage is
> >
> >    eGLImage, fd, format, width, height,  bool ?
> 
> I think you're confusing `createDMABufTexture2D()` and 
> `createInputDMABufTexture2D()` ;)

Ugh indeed. Sorry, tired eyes and trying to read code in a mail
client...

> 
> FWIW., IMO we could consider dropping `createInputDMABufTexture2D()` and 
> `createOutputDMABufTexture2D()` - both small wrappers around 
> `createDMABufTexture2D()` - and directly expose the later.

Well it seems like longSpelledOutNames and longSpelledOutDiffNames can
be difficult to spot so if you think there's a cleanup opportunity that
would make the code better then maybe it's worth seeing what it looks
like - but no specific requirement.

I'll check through the rest of the patches and push up to CI to test.

--
Kieran
Robert Mader May 4, 2026, 8:02 a.m. UTC | #4 
On 04.05.26 09:22, Kieran Bingham wrote:
> Quoting Robert Mader (2026-05-03 23:32:23)
>> Hi,
>>
>> On 03.05.26 14:34, Kieran Bingham wrote:
>>>> ...
>>>>    
>>>>    /**
>>>> @@ -202,7 +223,7 @@ int eGL::createOutputDMABufTexture2D(eGLImage &eglImage, int fd)
>>>>    {
>>>>           ASSERT(tid_ == Thread::currentId());
>>>>    
>>>> -       return createDMABufTexture2D(eglImage, fd, true);
>>>> +       return createDMABufTexture2D(eglImage, fd, DRM_FORMAT_ARGB8888, eglImage.width_, eglImage.height_, true);
>>> This looks like something wrong perhaps crept in, unless I'm
>>> misunderstanding or there's another overload - here we have
>>>
>>> eGL::createInputDMABufTexture2D(eGLImage &eglImage, GLint format, uint32_t width, uint32_t height, int fd)
>>>
>>> but this usage is
>>>
>>>     eGLImage, fd, format, width, height,  bool ?
>> I think you're confusing `createDMABufTexture2D()` and
>> `createInputDMABufTexture2D()` ;)
> Ugh indeed. Sorry, tired eyes and trying to read code in a mail
> client...
>
>> FWIW., IMO we could consider dropping `createInputDMABufTexture2D()` and
>> `createOutputDMABufTexture2D()` - both small wrappers around
>> `createDMABufTexture2D()` - and directly expose the later.
> Well it seems like longSpelledOutNames and longSpelledOutDiffNames can
> be difficult to spot so if you think there's a cleanup opportunity that
> would make the code better then maybe it's worth seeing what it looks
> like - but no specific requirement.
>
> I'll check through the rest of the patches and push up to CI to test.
>
> --
> Kieran

Thanks! Just ran the style checker locally - had a setup issue with that 
yesterday - and there is one wrong indentation in the second patch:

> --- src/libcamera/software_isp/debayer_egl.cpp +++ 
> src/libcamera/software_isp/debayer_egl.cpp @@ -555,7 +555,7 @@ /* 
> Calculate stats for the whole frame */ if (dmaSyncers.empty() && 
> (frame % SwStatsCpu::kStatPerNumFrames) == 0) - 
> dmaSyncBegin(dmaSyncers, input, nullptr); + dmaSyncBegin(dmaSyncers, 
> input, nullptr); stats_->processFrame(frame, 0, input); 
> dmaSyncers.clear();

In case we don't need a v2 otherwise, is that something you can fix 
up while applying? 😬

Patch
diff mbox series 

diff --git a/include/libcamera/internal/egl.h b/include/libcamera/internal/egl.h
index 0ad2320b1..bd2f20e33 100644
--- a/include/libcamera/internal/egl.h
+++ b/include/libcamera/internal/egl.h
@@ -101,7 +101,7 @@  public:
 
 	int initEGLContext();
 
-	int createInputDMABufTexture2D(eGLImage &eglImage, int fd);
+	int createInputDMABufTexture2D(eGLImage &eglImage, GLint format, uint32_t width, uint32_t height, int fd);
 	int createOutputDMABufTexture2D(eGLImage &eglImage, int fd);
 	void createTexture2D(eGLImage &eglImage, GLint format, uint32_t width, uint32_t height, void *data);
 
@@ -133,7 +133,7 @@  private:
 			  unsigned int shaderDataLen,
 			  Span<const std::string> shaderEnv);
 
-	int createDMABufTexture2D(eGLImage &eglImage, int fd, bool output);
+	int createDMABufTexture2D(eGLImage &eglImage, int fd, uint32_t drm_format, uint32_t width, uint32_t height, bool output);
 
 	PFNGLEGLIMAGETARGETTEXTURE2DOESPROC glEGLImageTargetTexture2DOES;
 	PFNEGLCREATEIMAGEKHRPROC eglCreateImageKHR;
diff --git a/src/libcamera/egl.cpp b/src/libcamera/egl.cpp
index f65929470..c30ed95a7 100644
--- a/src/libcamera/egl.cpp
+++ b/src/libcamera/egl.cpp
@@ -19,6 +19,7 @@ 
 
 #include <libcamera/base/thread.h>
 
+#include <GLES3/gl32.h>
 #include <libdrm/drm_fourcc.h>
 
 namespace libcamera {
@@ -102,6 +103,9 @@  void eGL::syncOutput()
  * \brief Create a DMA-BUF backed 2D texture
  * \param[in,out] eglImage EGL image to associate with the DMA-BUF
  * \param[in] fd DMA-BUF file descriptor
+ * \param[in] drm_format the DRM fourcc
+ * \param[in] width the buffer width
+ * \param[in] height the buffer height
  * \param[in] output If true, create framebuffer for render target
  *
  * Internal implementation for creating DMA-BUF textures. Creates an EGL
@@ -110,15 +114,15 @@  void eGL::syncOutput()
  *
  * \return 0 on success, or -ENODEV on failure
  */
-int eGL::createDMABufTexture2D(eGLImage &eglImage, int fd, bool output)
+int eGL::createDMABufTexture2D(eGLImage &eglImage, int fd, uint32_t drm_format, uint32_t width, uint32_t height, bool output)
 {
 	ASSERT(tid_ == Thread::currentId());
 
 	// clang-format off
 	EGLint image_attrs[] = {
-		EGL_WIDTH, (EGLint)eglImage.width_,
-		EGL_HEIGHT, (EGLint)eglImage.height_,
-		EGL_LINUX_DRM_FOURCC_EXT, DRM_FORMAT_ARGB8888,
+		EGL_WIDTH, (EGLint)width,
+		EGL_HEIGHT, (EGLint)height,
+		EGL_LINUX_DRM_FOURCC_EXT, (EGLint)drm_format,
 		EGL_DMA_BUF_PLANE0_FD_EXT, fd,
 		EGL_DMA_BUF_PLANE0_OFFSET_EXT, 0,
 		EGL_DMA_BUF_PLANE0_PITCH_EXT, (EGLint)eglImage.stride_,
@@ -133,7 +137,7 @@  int eGL::createDMABufTexture2D(eGLImage &eglImage, int fd, bool output)
 					      NULL, image_attrs);
 
 	if (image == EGL_NO_IMAGE_KHR) {
-		LOG(eGL, Error) << "eglCreateImageKHR fail";
+		LOG(eGL, Debug) << "eglCreateImageKHR fail";
 		return -ENODEV;
 	}
 
@@ -171,6 +175,9 @@  int eGL::createDMABufTexture2D(eGLImage &eglImage, int fd, bool output)
 /**
  * \brief Create an input DMA-BUF backed texture
  * \param[in,out] eglImage EGL image to associate with the DMA-BUF
+ * \param[in] format the GL format
+ * \param[in] width the buffer width
+ * \param[in] height the buffer height
  * \param[in] fd DMA-BUF file descriptor
  *
  * Creates an EGL image from a DMA-BUF file descriptor and binds it to
@@ -179,11 +186,25 @@  int eGL::createDMABufTexture2D(eGLImage &eglImage, int fd, bool output)
  *
  * \return 0 on success, or -ENODEV on failure
  */
-int eGL::createInputDMABufTexture2D(eGLImage &eglImage, int fd)
+int eGL::createInputDMABufTexture2D(eGLImage &eglImage, GLint format, uint32_t width, uint32_t height, int fd)
 {
+	EGLint drm_format;
+
 	ASSERT(tid_ == Thread::currentId());
 
-	return createDMABufTexture2D(eglImage, fd, false);
+	switch (format) {
+	case GL_LUMINANCE:
+		drm_format = DRM_FORMAT_R8;
+		break;
+	case GL_RG:
+		drm_format = DRM_FORMAT_RG88;
+		break;
+	default:
+		LOG(eGL, Error) << "unhandled GL format";
+		return -ENODEV;
+	}
+
+	return createDMABufTexture2D(eglImage, fd, drm_format, width, height, false);
 }
 
 /**
@@ -202,7 +223,7 @@  int eGL::createOutputDMABufTexture2D(eGLImage &eglImage, int fd)
 {
 	ASSERT(tid_ == Thread::currentId());
 
-	return createDMABufTexture2D(eglImage, fd, true);
+	return createDMABufTexture2D(eglImage, fd, DRM_FORMAT_ARGB8888, eglImage.width_, eglImage.height_, true);
 }
 
 /**