[24/30] libcamera: software_isp: egl: Add eglBenchMark
diff mbox series

Message ID 20260618122245.946138-25-bryan.odonoghue@linaro.org
State RFC
Headers show
Series
  • RFC/RFT: gpuisp: Multipass with speed optimisations on top
Related show

Commit Message

Bryan O'Donoghue June 18, 2026, 12:22 p.m. UTC
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 include/libcamera/internal/egl.h | 52 ++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

Comments

Milan Zamazal June 25, 2026, 6:38 a.m. UTC | #1
Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes:

> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  include/libcamera/internal/egl.h | 52 ++++++++++++++++++++++++++++++++
>  1 file changed, 52 insertions(+)
>
> diff --git a/include/libcamera/internal/egl.h b/include/libcamera/internal/egl.h
> index 0e57678e8..8f905416a 100644
> --- a/include/libcamera/internal/egl.h
> +++ b/include/libcamera/internal/egl.h
> @@ -127,6 +127,8 @@ public:
>  	void syncOutput();
>  	void flushOutput();
>  
> +	friend class eGLBenchMark;
> +
>  private:
>  	LIBCAMERA_DISABLE_COPY_AND_MOVE(eGL)
>  
> @@ -152,4 +154,54 @@ private:
>  	PFNGLGETQUERYOBJECTUIVPROC glGetQueryObjectui64v;
>  	PFNGLGENQUERIESPROC glGenQueries;
>  };
> +
> +class eGLBenchMark {
> +public:
> +	eGLBenchMark() {}
> +	~eGLBenchMark() {}
> +
> +	void init(void)

s/void//

> +	{
> +		glGenQueries(1, &glQueryTimeId_); 

An extra space at the end of the line.

> +	}
> +
> +	void begin(eGL &egl)
> +	{
> +		egl.glBeginQuery(GL_TIME_ELAPSED_EXT, glQueryTimeId_);
> +	}
> +
> +	void end(eGL &egl)
> +	{
> +		egl.glEndQuery(GL_TIME_ELAPSED_EXT);
> +	}
> +
> +	GLuint64 getTimeElapsedSync(eGL &egl)
> +	{
> +		// Caution sync will wait until result is available

C-style comments should be used.

> +		return getTimeElapsed(egl, true);
> +	}
> +
> +	GLuint64 getTimeElapsedAsync(eGL &egl)
> +	{
> +		return getTimeElapsed(egl, false);
> +	}

Is this used anywhere?  If yes, why two wrappers rather than using
getTimeElapsed directly?  I also wonder how the async version would be
used.

> +private:
> +	GLuint glQueryTimeId_;
> +
> +	GLuint64 getTimeElapsed(eGL &egl, bool sync)
> +	{
> +		GLuint time_elapsed_ns = 0;
> +		GLuint available = 0;
> +
> +		if (!sync)
> +			egl.glGetQueryObjectuiv(glQueryTimeId_, GL_QUERY_RESULT_AVAILABLE, &available);
> +
> +		if (available || sync)
> +			egl.glGetQueryObjectui64v(glQueryTimeId_, GL_QUERY_RESULT, &time_elapsed_ns);
> +
> +		return time_elapsed_ns;
> +	}
> +};
> +
>  } //namespace libcamera
Bryan O'Donoghue June 25, 2026, 8:29 a.m. UTC | #2
On 25/06/2026 07:38, Milan Zamazal wrote:
>> +	GLuint64 getTimeElapsedAsync(eGL &egl)
>> +	{
>> +		return getTimeElapsed(egl, false);
>> +	}
> Is this used anywhere?  If yes, why two wrappers rather than using
> getTimeElapsed directly?  I also wonder how the async version would be
> used.

Sync blocks until the GPU is done, it is the equivalent of glFinish() so 
you should be very very careful about using it.

In a 'normal' runtime scenario you'd use async - if benchmark data is 
available great print it out, if not carry on.

The sync version OTOH can be buried in a LOG(.., Debug) - at least 
that's the thinking.

---
bod
Milan Zamazal June 25, 2026, 1:28 p.m. UTC | #3
Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes:

> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  include/libcamera/internal/egl.h | 52 ++++++++++++++++++++++++++++++++
>  1 file changed, 52 insertions(+)
>
> diff --git a/include/libcamera/internal/egl.h b/include/libcamera/internal/egl.h
> index 0e57678e8..8f905416a 100644
> --- a/include/libcamera/internal/egl.h
> +++ b/include/libcamera/internal/egl.h
> @@ -127,6 +127,8 @@ public:
>  	void syncOutput();
>  	void flushOutput();
>  
> +	friend class eGLBenchMark;
> +
>  private:
>  	LIBCAMERA_DISABLE_COPY_AND_MOVE(eGL)
>  
> @@ -152,4 +154,54 @@ private:
>  	PFNGLGETQUERYOBJECTUIVPROC glGetQueryObjectui64v;
>  	PFNGLGENQUERIESPROC glGenQueries;
>  };
> +
> +class eGLBenchMark {
> +public:
> +	eGLBenchMark() {}
> +	~eGLBenchMark() {}
> +
> +	void init(void)
> +	{
> +		glGenQueries(1, &glQueryTimeId_); 
> +	}
> +
> +	void begin(eGL &egl)
> +	{
> +		egl.glBeginQuery(GL_TIME_ELAPSED_EXT, glQueryTimeId_);

This call fails for me with GL_INVALID_ENUM.  According to the doc:

  The query targets GL_ANY_SAMPLES_PASSED, and GL_TIME_ELAPSED are
  availale only if the GL version is 3.3 or higher.

(The spelling typo is by Khronos...)

The reported GL version in my environment is:

  EGL: GL_VERSION: OpenGL ES 3.2

Which is apparently the problem.  That means:

- The GL calls here should check for errors.  Due to this unchecked
  error, the next glGetError() check later in processing returns /this/
  error and the whole processing fails.

- The benchmark should be disabled or so if GL_TIME_ELAPSED is
  unsupported.

BTW there are additional problems with unpacked input and shaders
(unrelated to this patch), I got it at least running for now but the
output is wrong, working on it.

> +	}
> +
> +	void end(eGL &egl)
> +	{
> +		egl.glEndQuery(GL_TIME_ELAPSED_EXT);
> +	}
> +
> +	GLuint64 getTimeElapsedSync(eGL &egl)
> +	{
> +		// Caution sync will wait until result is available
> +		return getTimeElapsed(egl, true);
> +	}
> +
> +	GLuint64 getTimeElapsedAsync(eGL &egl)
> +	{
> +		return getTimeElapsed(egl, false);
> +	}
> +
> +private:
> +	GLuint glQueryTimeId_;
> +
> +	GLuint64 getTimeElapsed(eGL &egl, bool sync)
> +	{
> +		GLuint time_elapsed_ns = 0;
> +		GLuint available = 0;
> +
> +		if (!sync)
> +			egl.glGetQueryObjectuiv(glQueryTimeId_, GL_QUERY_RESULT_AVAILABLE, &available);
> +
> +		if (available || sync)
> +			egl.glGetQueryObjectui64v(glQueryTimeId_, GL_QUERY_RESULT, &time_elapsed_ns);
> +
> +		return time_elapsed_ns;
> +	}
> +};
> +
>  } //namespace libcamera
Bryan O'Donoghue June 25, 2026, 2:10 p.m. UTC | #4
On 25/06/2026 14:28, Milan Zamazal wrote:
> This call fails for me with GL_INVALID_ENUM.  According to the doc:
> 
>    The query targets GL_ANY_SAMPLES_PASSED, and GL_TIME_ELAPSED are
>    availale only if the GL version is 3.3 or higher.
> 
> (The spelling typo is by Khronos...)
> 
> The reported GL version in my environment is:
> 
>    EGL: GL_VERSION: OpenGL ES 3.2

My env is 3.2 also. I suspect there is a feature flag you are supposed 
to interrogate not a version number.

Will investigate.

---
bod

Patch
diff mbox series

diff --git a/include/libcamera/internal/egl.h b/include/libcamera/internal/egl.h
index 0e57678e8..8f905416a 100644
--- a/include/libcamera/internal/egl.h
+++ b/include/libcamera/internal/egl.h
@@ -127,6 +127,8 @@  public:
 	void syncOutput();
 	void flushOutput();
 
+	friend class eGLBenchMark;
+
 private:
 	LIBCAMERA_DISABLE_COPY_AND_MOVE(eGL)
 
@@ -152,4 +154,54 @@  private:
 	PFNGLGETQUERYOBJECTUIVPROC glGetQueryObjectui64v;
 	PFNGLGENQUERIESPROC glGenQueries;
 };
+
+class eGLBenchMark {
+public:
+	eGLBenchMark() {}
+	~eGLBenchMark() {}
+
+	void init(void)
+	{
+		glGenQueries(1, &glQueryTimeId_); 
+	}
+
+	void begin(eGL &egl)
+	{
+		egl.glBeginQuery(GL_TIME_ELAPSED_EXT, glQueryTimeId_);
+	}
+
+	void end(eGL &egl)
+	{
+		egl.glEndQuery(GL_TIME_ELAPSED_EXT);
+	}
+
+	GLuint64 getTimeElapsedSync(eGL &egl)
+	{
+		// Caution sync will wait until result is available
+		return getTimeElapsed(egl, true);
+	}
+
+	GLuint64 getTimeElapsedAsync(eGL &egl)
+	{
+		return getTimeElapsed(egl, false);
+	}
+
+private:
+	GLuint glQueryTimeId_;
+
+	GLuint64 getTimeElapsed(eGL &egl, bool sync)
+	{
+		GLuint time_elapsed_ns = 0;
+		GLuint available = 0;
+
+		if (!sync)
+			egl.glGetQueryObjectuiv(glQueryTimeId_, GL_QUERY_RESULT_AVAILABLE, &available);
+
+		if (available || sync)
+			egl.glGetQueryObjectui64v(glQueryTimeId_, GL_QUERY_RESULT, &time_elapsed_ns);
+
+		return time_elapsed_ns;
+	}
+};
+
 } //namespace libcamera