[PATCH v1] libcamera: v4l2_videodevice: `lastUsedCounter_` need not be atomic

[Laurent Pinchart]

On Wed, Mar 19, 2025 at 04:34:44PM +0100, Barnabás Pőcze wrote:
> 2025. 03. 19. 15:53 keltezéssel, Kieran Bingham írta:
> > Quoting Barnabás Pőcze (2025-03-10 17:03:26)
> >> The `V4L2BufferCache` type is not thread-safe. Its `lastUsedCounter_`
> >> member is not used in contexts where its atomicity would matter.
> >> So it does not need to be have an atomic type.
> > 
> > Any indications as to why this was using atomic ? Maybe just
> > cautious development when it was first put in ?
> 
> Not sure. It was Laurent who suggested it: https://patchwork.libcamera.org/patch/2878/#3889
> 
> There is only a single place where `lastUsedCounter_` might be used
> by multiple threads: `V4L2BufferCache::get()`. But that function
> is not thread safe, so either `lastUsedCounter_` is unnecessarily
> atomic or race conditions are rampant already. As far as I can
> tell it is the former.

Agreed.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> > As long as we're convinced it's still safe and passes the tests, I'm
> > fine with it. It's certainly simpler code.
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > 
> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze at ideasonboard.com>
> >> ---
> >>   include/libcamera/internal/v4l2_videodevice.h | 2 +-
> >>   src/libcamera/v4l2_videodevice.cpp            | 4 ++--
> >>   2 files changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> >> index f021c2a01..e52b50c67 100644
> >> --- a/include/libcamera/internal/v4l2_videodevice.h
> >> +++ b/include/libcamera/internal/v4l2_videodevice.h
> >> @@ -158,7 +158,7 @@ private:
> >>                  std::vector<Plane> planes_;
> >>          };
> >>   
> >> -       std::atomic<uint64_t> lastUsedCounter_;
> >> +       uint64_t lastUsedCounter_;
> >>          std::vector<Entry> cache_;
> >>          /* \todo Expose the miss counter through an instrumentation API. */
> >>          unsigned int missCounter_;
> >> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> >> index e241eb47b..f5b3fa09d 100644
> >> --- a/src/libcamera/v4l2_videodevice.cpp
> >> +++ b/src/libcamera/v4l2_videodevice.cpp
> >> @@ -190,7 +190,7 @@ V4L2BufferCache::V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>>
> >>   {
> >>          for (const std::unique_ptr<FrameBuffer> &buffer : buffers)
> >>                  cache_.emplace_back(true,
> >> -                                   lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel),
> >> +                                   lastUsedCounter_++,
> >>                                      *buffer.get());
> >>   }
> >>   
> >> @@ -258,7 +258,7 @@ int V4L2BufferCache::get(const FrameBuffer &buffer)
> >>                  return -ENOENT;
> >>   
> >>          cache_[use] = Entry(false,
> >> -                           lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel),
> >> +                           lastUsedCounter_++,
> >>                              buffer);
> >>   
> >>          return use;

-- 
Regards,

Laurent Pinchart