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

[Jacopo Mondi]

Hi Barnabás

On Mon, Mar 10, 2025 at 06:03:26PM +0100, Barnabás Pőcze wrote:
> 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.
>
> Signed-off-by: Barnabás Pőcze <barnabas.pocze at ideasonboard.com>

This had me do more reasoning than expected, so let me see if I got
this right.

The buffer cached is accessed at buffer queue time (get()) and at
buffer dequeue time (put()). Dequeue is an async event generated by
the video device while buffer queuing is triggered by the application.

dequeue is triggered by an event dispatcher, while queueRequest runs in
the pipeline handler thread (which is the same thread where
all other Object (but Threads) run in libcamera).

As the event dispatcher is setEnabled(true) at the first call to
queueBuffers, the thread it runs on is again the pipeline handler thread.

So yeah, seems safe indeed

Reviewed-by: Jacopo Mondi <jacopo.mondi 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;
> --
> 2.48.1
>