| Message ID | 20260624085849.873784-2-bryan.odonoghue@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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())
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())
On 24/06/2026 15:38, Laurent Pinchart wrote:
> - Don't use V4L2BufferCache in eGL, write your own cache handling logic.
I'm happy to do this, I acted on Barnabas suggestion of "something like
V4L2BufferCache" perhaps too literally ;)
---
bod
2026. 06. 24. 16:38 keltezéssel, Laurent Pinchart írta: > 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 Maybe it could be be part of the return value, e.g. `std::pair<int, bool>` or similar. (Or I suppose `bool *hit = nullptr` as an optional out parameter, but I like the return value better.) > 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. I would somewhat be in favor of making something "generic" that would work for both cases, since the idea is the same in both cases: have a cache of framebuffer -> index mappings, and I don't see that it would fundamentally change. So it seems to me that it wouldn't be a mistake to try to avoid duplication here. > >> { >> - 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()) > > -- > Regards, > > Laurent Pinchart
On Thu, Jun 25, 2026 at 12:22:36PM +0200, Barnabás Pőcze wrote: > 2026. 06. 24. 16:38 keltezéssel, Laurent Pinchart írta: > > 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 > > Maybe it could be be part of the return value, e.g. `std::pair<int, bool>` or similar. > (Or I suppose `bool *hit = nullptr` as an optional out parameter, but I like the return > value better.) Returning a pair (or tuple - would there be a meaningful difference ?) seems fine to me. I think we're a bit too shy in libcamera when it comes to returning tuples and using structured binding. > > 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. > > I would somewhat be in favor of making something "generic" that would work for > both cases, since the idea is the same in both cases: have a cache of > framebuffer -> index mappings, and I don't see that it would fundamentally change. > So it seems to me that it wouldn't be a mistake to try to avoid duplication here. Ack. > >> { > >> - 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())
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())
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(-)