[01/10] libcamera: v4l2_videodevice: Output cache hit as a parameter
diff mbox series

Message ID 20260624085849.873784-2-bryan.odonoghue@linaro.org
State New
Headers show
Series
  • libcamera: software_isp: gpu: Add go faster stripes
Related show

Commit Message

Bryan O'Donoghue June 24, 2026, 8:58 a.m. UTC 
The existing V4L2BufferCache::get routine evaluates if a cache hit has
occured by way of an internal variable. It is in fact very useful to a user
of this API to understand if a hit has occured as using logic may wish to
differentiate based on hit or miss.

This differentiation is required for GPUISP. Rather than add a routine to
interrogate if a cache hit exists - add an output parameter to the get
routine.

In simple terms if you are trying to cache data about the last thing you
want to to is interrogate the cache twice so, a `hit` output parameter adds
a small cost in the stack for a large pivot in using code's ability to
understand how much work it needs to do on hit v miss.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 include/libcamera/internal/v4l2_videodevice.h |  2 +-
 src/libcamera/v4l2_videodevice.cpp            |  9 ++++++---
 test/v4l2_videodevice/buffer_cache.cpp        | 14 +++++++++-----
 3 files changed, 16 insertions(+), 9 deletions(-)

Comments

Robert Mader June 24, 2026, 11:48 a.m. UTC | #1 
Hi, thanks for the patch

On 24.06.26 10:58, Bryan O'Donoghue wrote:
> The existing V4L2BufferCache::get routine evaluates if a cache hit has
> occured by way of an internal variable. It is in fact very useful to a user
> of this API to understand if a hit has occured as using logic may wish to
> differentiate based on hit or miss.
>
> This differentiation is required for GPUISP. Rather than add a routine to
> interrogate if a cache hit exists - add an output parameter to the get
> routine.
>
> In simple terms if you are trying to cache data about the last thing you
> want to to is interrogate the cache twice so, a `hit` output parameter adds
nit: to do
> a small cost in the stack for a large pivot in using code's ability to
> understand how much work it needs to do on hit v miss.
>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

Looks sensible to me. Not sure if my R-B really counts here, however:

Reviewed-by: Robert Mader <robert.mader@collabora.com>

> ---
>   include/libcamera/internal/v4l2_videodevice.h |  2 +-
>   src/libcamera/v4l2_videodevice.cpp            |  9 ++++++---
>   test/v4l2_videodevice/buffer_cache.cpp        | 14 +++++++++-----
>   3 files changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> index 10367e4e1..49165a565 100644
> --- a/include/libcamera/internal/v4l2_videodevice.h
> +++ b/include/libcamera/internal/v4l2_videodevice.h
> @@ -129,7 +129,7 @@ public:
>   	~V4L2BufferCache();
>   
>   	bool isEmpty() const;
> -	int get(const FrameBuffer &buffer);
> +	int get(const FrameBuffer &buffer, bool &hit);
>   	void put(unsigned int index);
>   
>   private:
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index ca8759830..6496eb324 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -217,6 +217,7 @@ bool V4L2BufferCache::isEmpty() const
>   /**
>    * \brief Find the best V4L2 buffer for a FrameBuffer
>    * \param[in] buffer The FrameBuffer
> + * \param[out] hit. Indicates if there was a cache hit
>    *
>    * Find the best V4L2 buffer index to be used for the FrameBuffer \a buffer
>    * based on previous mappings of frame buffers to V4L2 buffers. If a free V4L2
> @@ -227,12 +228,13 @@ bool V4L2BufferCache::isEmpty() const
>    * \return The index of the best V4L2 buffer, or -ENOENT if no free V4L2 buffer
>    * is available
>    */
> -int V4L2BufferCache::get(const FrameBuffer &buffer)
> +int V4L2BufferCache::get(const FrameBuffer &buffer, bool &hit)
>   {
> -	bool hit = false;
>   	int use = -1;
>   	uint64_t oldest = UINT64_MAX;
>   
> +	hit = false;
> +
>   	for (unsigned int index = 0; index < cache_.size(); index++) {
>   		const Entry &entry = cache_[index];
>   
> @@ -1649,6 +1651,7 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer, const V4L2Request *request
>   {
>   	struct v4l2_plane v4l2Planes[VIDEO_MAX_PLANES] = {};
>   	struct v4l2_buffer buf = {};
> +	bool hit;
>   	int ret;
>   
>   	if (state_ == State::Stopping) {
> @@ -1666,7 +1669,7 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer, const V4L2Request *request
>   		return -ENOENT;
>   	}
>   
> -	ret = cache_->get(*buffer);
> +	ret = cache_->get(*buffer, hit);
>   	if (ret < 0)
>   		return ret;
>   
> diff --git a/test/v4l2_videodevice/buffer_cache.cpp b/test/v4l2_videodevice/buffer_cache.cpp
> index 613e0d7ce..e41c30059 100644
> --- a/test/v4l2_videodevice/buffer_cache.cpp
> +++ b/test/v4l2_videodevice/buffer_cache.cpp
> @@ -35,7 +35,8 @@ public:
>   	{
>   		for (unsigned int i = 0; i < buffers.size() * 100; i++) {
>   			int nBuffer = i % buffers.size();
> -			int index = cache->get(*buffers[nBuffer].get());
> +			bool hit;
> +			int index = cache->get(*buffers[nBuffer].get(), hit);
>   
>   			if (index != nBuffer) {
>   				std::cout << "Expected index " << nBuffer
> @@ -60,7 +61,8 @@ public:
>   
>   		for (unsigned int i = 0; i < buffers.size() * 100; i++) {
>   			int nBuffer = dist(generator_);
> -			int index = cache->get(*buffers[nBuffer].get());
> +			bool hit;
> +			int index = cache->get(*buffers[nBuffer].get(), hit);
>   
>   			if (index < 0) {
>   				std::cout << "Failed lookup from cache"
> @@ -90,7 +92,8 @@ public:
>   
>   		/* Pick a hot buffer at random and store its index. */
>   		int hotBuffer = dist(generator_);
> -		int hotIndex = cache->get(*buffers[hotBuffer].get());
> +		bool hit;
> +		int hotIndex = cache->get(*buffers[hotBuffer].get(), hit);
>   		cache->put(hotIndex);
>   
>   		/*
> @@ -106,7 +109,7 @@ public:
>   			else
>   				nBuffer = dist(generator_);
>   
> -			index = cache->get(*buffers[nBuffer].get());
> +			index = cache->get(*buffers[nBuffer].get(), hit);
>   
>   			if (index < 0) {
>   				std::cout << "Failed lookup from cache"
> @@ -135,7 +138,8 @@ public:
>   
>   		for (const auto &buffer : buffers) {
>   			FrameBuffer &b = *buffer.get();
> -			cache.get(b);
> +			bool hit;
> +			cache.get(b, hit);
>   		}
>   
>   		if (cache.isEmpty())
Laurent Pinchart June 24, 2026, 2:38 p.m. UTC | #2 
On Wed, Jun 24, 2026 at 09:58:40AM +0100, Bryan O'Donoghue wrote:
> The existing V4L2BufferCache::get routine evaluates if a cache hit has
> occured by way of an internal variable. It is in fact very useful to a user
> of this API to understand if a hit has occured as using logic may wish to
> differentiate based on hit or miss.
> 
> This differentiation is required for GPUISP. Rather than add a routine to
> interrogate if a cache hit exists - add an output parameter to the get
> routine.
> 
> In simple terms if you are trying to cache data about the last thing you
> want to to is interrogate the cache twice so, a `hit` output parameter adds
> a small cost in the stack for a large pivot in using code's ability to
> understand how much work it needs to do on hit v miss.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  include/libcamera/internal/v4l2_videodevice.h |  2 +-
>  src/libcamera/v4l2_videodevice.cpp            |  9 ++++++---
>  test/v4l2_videodevice/buffer_cache.cpp        | 14 +++++++++-----
>  3 files changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> index 10367e4e1..49165a565 100644
> --- a/include/libcamera/internal/v4l2_videodevice.h
> +++ b/include/libcamera/internal/v4l2_videodevice.h
> @@ -129,7 +129,7 @@ public:
>  	~V4L2BufferCache();
>  
>  	bool isEmpty() const;
> -	int get(const FrameBuffer &buffer);
> +	int get(const FrameBuffer &buffer, bool &hit);
>  	void put(unsigned int index);
>  
>  private:
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index ca8759830..6496eb324 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -217,6 +217,7 @@ bool V4L2BufferCache::isEmpty() const
>  /**
>   * \brief Find the best V4L2 buffer for a FrameBuffer
>   * \param[in] buffer The FrameBuffer
> + * \param[out] hit. Indicates if there was a cache hit

Wrong syntax.

>   *
>   * Find the best V4L2 buffer index to be used for the FrameBuffer \a buffer
>   * based on previous mappings of frame buffers to V4L2 buffers. If a free V4L2
> @@ -227,12 +228,13 @@ bool V4L2BufferCache::isEmpty() const
>   * \return The index of the best V4L2 buffer, or -ENOENT if no free V4L2 buffer
>   * is available
>   */
> -int V4L2BufferCache::get(const FrameBuffer &buffer)
> +int V4L2BufferCache::get(const FrameBuffer &buffer, bool &hit)

It's not nice to force all callers to add a new local variable. That
being said, I was surprised that the function isn't called anywhere else
than in V4L2VideoDevice::queueBuffer() and in the unit tests.
Investigating a bit more, I realized you use V4L2BufferCache in patch
10/10 only, in a way that is entirely unrelated to V4L2. That seems a
bit of a hack. I see two better options:

- Don't use V4L2BufferCache in eGL, write your own cache handling logic.

- Rename the V4L2BufferCache class and turn it into a more generic
  cache.

The latter will require a bit more work to decide on the scope of that
class.

>  {
> -	bool hit = false;
>  	int use = -1;
>  	uint64_t oldest = UINT64_MAX;
>  
> +	hit = false;
> +
>  	for (unsigned int index = 0; index < cache_.size(); index++) {
>  		const Entry &entry = cache_[index];
>  
> @@ -1649,6 +1651,7 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer, const V4L2Request *request
>  {
>  	struct v4l2_plane v4l2Planes[VIDEO_MAX_PLANES] = {};
>  	struct v4l2_buffer buf = {};
> +	bool hit;
>  	int ret;
>  
>  	if (state_ == State::Stopping) {
> @@ -1666,7 +1669,7 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer, const V4L2Request *request
>  		return -ENOENT;
>  	}
>  
> -	ret = cache_->get(*buffer);
> +	ret = cache_->get(*buffer, hit);
>  	if (ret < 0)
>  		return ret;
>  
> diff --git a/test/v4l2_videodevice/buffer_cache.cpp b/test/v4l2_videodevice/buffer_cache.cpp
> index 613e0d7ce..e41c30059 100644
> --- a/test/v4l2_videodevice/buffer_cache.cpp
> +++ b/test/v4l2_videodevice/buffer_cache.cpp
> @@ -35,7 +35,8 @@ public:
>  	{
>  		for (unsigned int i = 0; i < buffers.size() * 100; i++) {
>  			int nBuffer = i % buffers.size();
> -			int index = cache->get(*buffers[nBuffer].get());
> +			bool hit;
> +			int index = cache->get(*buffers[nBuffer].get(), hit);
>  
>  			if (index != nBuffer) {
>  				std::cout << "Expected index " << nBuffer
> @@ -60,7 +61,8 @@ public:
>  
>  		for (unsigned int i = 0; i < buffers.size() * 100; i++) {
>  			int nBuffer = dist(generator_);
> -			int index = cache->get(*buffers[nBuffer].get());
> +			bool hit;
> +			int index = cache->get(*buffers[nBuffer].get(), hit);
>  
>  			if (index < 0) {
>  				std::cout << "Failed lookup from cache"
> @@ -90,7 +92,8 @@ public:
>  
>  		/* Pick a hot buffer at random and store its index. */
>  		int hotBuffer = dist(generator_);
> -		int hotIndex = cache->get(*buffers[hotBuffer].get());
> +		bool hit;
> +		int hotIndex = cache->get(*buffers[hotBuffer].get(), hit);
>  		cache->put(hotIndex);
>  
>  		/*
> @@ -106,7 +109,7 @@ public:
>  			else
>  				nBuffer = dist(generator_);
>  
> -			index = cache->get(*buffers[nBuffer].get());
> +			index = cache->get(*buffers[nBuffer].get(), hit);
>  
>  			if (index < 0) {
>  				std::cout << "Failed lookup from cache"
> @@ -135,7 +138,8 @@ public:
>  
>  		for (const auto &buffer : buffers) {
>  			FrameBuffer &b = *buffer.get();
> -			cache.get(b);
> +			bool hit;
> +			cache.get(b, hit);
>  		}
>  
>  		if (cache.isEmpty())

Patch
diff mbox series 

diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
index 10367e4e1..49165a565 100644
--- a/include/libcamera/internal/v4l2_videodevice.h
+++ b/include/libcamera/internal/v4l2_videodevice.h
@@ -129,7 +129,7 @@  public:
 	~V4L2BufferCache();
 
 	bool isEmpty() const;
-	int get(const FrameBuffer &buffer);
+	int get(const FrameBuffer &buffer, bool &hit);
 	void put(unsigned int index);
 
 private:
diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
index ca8759830..6496eb324 100644
--- a/src/libcamera/v4l2_videodevice.cpp
+++ b/src/libcamera/v4l2_videodevice.cpp
@@ -217,6 +217,7 @@  bool V4L2BufferCache::isEmpty() const
 /**
  * \brief Find the best V4L2 buffer for a FrameBuffer
  * \param[in] buffer The FrameBuffer
+ * \param[out] hit. Indicates if there was a cache hit
  *
  * Find the best V4L2 buffer index to be used for the FrameBuffer \a buffer
  * based on previous mappings of frame buffers to V4L2 buffers. If a free V4L2
@@ -227,12 +228,13 @@  bool V4L2BufferCache::isEmpty() const
  * \return The index of the best V4L2 buffer, or -ENOENT if no free V4L2 buffer
  * is available
  */
-int V4L2BufferCache::get(const FrameBuffer &buffer)
+int V4L2BufferCache::get(const FrameBuffer &buffer, bool &hit)
 {
-	bool hit = false;
 	int use = -1;
 	uint64_t oldest = UINT64_MAX;
 
+	hit = false;
+
 	for (unsigned int index = 0; index < cache_.size(); index++) {
 		const Entry &entry = cache_[index];
 
@@ -1649,6 +1651,7 @@  int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer, const V4L2Request *request
 {
 	struct v4l2_plane v4l2Planes[VIDEO_MAX_PLANES] = {};
 	struct v4l2_buffer buf = {};
+	bool hit;
 	int ret;
 
 	if (state_ == State::Stopping) {
@@ -1666,7 +1669,7 @@  int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer, const V4L2Request *request
 		return -ENOENT;
 	}
 
-	ret = cache_->get(*buffer);
+	ret = cache_->get(*buffer, hit);
 	if (ret < 0)
 		return ret;
 
diff --git a/test/v4l2_videodevice/buffer_cache.cpp b/test/v4l2_videodevice/buffer_cache.cpp
index 613e0d7ce..e41c30059 100644
--- a/test/v4l2_videodevice/buffer_cache.cpp
+++ b/test/v4l2_videodevice/buffer_cache.cpp
@@ -35,7 +35,8 @@  public:
 	{
 		for (unsigned int i = 0; i < buffers.size() * 100; i++) {
 			int nBuffer = i % buffers.size();
-			int index = cache->get(*buffers[nBuffer].get());
+			bool hit;
+			int index = cache->get(*buffers[nBuffer].get(), hit);
 
 			if (index != nBuffer) {
 				std::cout << "Expected index " << nBuffer
@@ -60,7 +61,8 @@  public:
 
 		for (unsigned int i = 0; i < buffers.size() * 100; i++) {
 			int nBuffer = dist(generator_);
-			int index = cache->get(*buffers[nBuffer].get());
+			bool hit;
+			int index = cache->get(*buffers[nBuffer].get(), hit);
 
 			if (index < 0) {
 				std::cout << "Failed lookup from cache"
@@ -90,7 +92,8 @@  public:
 
 		/* Pick a hot buffer at random and store its index. */
 		int hotBuffer = dist(generator_);
-		int hotIndex = cache->get(*buffers[hotBuffer].get());
+		bool hit;
+		int hotIndex = cache->get(*buffers[hotBuffer].get(), hit);
 		cache->put(hotIndex);
 
 		/*
@@ -106,7 +109,7 @@  public:
 			else
 				nBuffer = dist(generator_);
 
-			index = cache->get(*buffers[nBuffer].get());
+			index = cache->get(*buffers[nBuffer].get(), hit);
 
 			if (index < 0) {
 				std::cout << "Failed lookup from cache"
@@ -135,7 +138,8 @@  public:
 
 		for (const auto &buffer : buffers) {
 			FrameBuffer &b = *buffer.get();
-			cache.get(b);
+			bool hit;
+			cache.get(b, hit);
 		}
 
 		if (cache.isEmpty())