[libcamera-devel] [PATCH v3 5/5] libcamera: v4l2_videodevice: Empty the V4L2 buffer cache on streamOff()

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

On Mon, Mar 21, 2022 at 10:27:02AM +0000, Naushir Patuck via libcamera-devel wrote:
> When streamOff() is called, ensure the cache entires for the remaining queued

s/entires/entries/

> buffers are freed since this will not happen via the dequeueBuffer() mechanism.
> 
> Additionally, add a V4L2BufferCache::isEmpty() function and assert that the
> cache is empty at the end of the streamOff() call.
> 
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> Tested-by: David Plowman <david.plowman at raspberrypi.com>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>  include/libcamera/internal/v4l2_videodevice.h |  1 +
>  src/libcamera/v4l2_videodevice.cpp            | 16 ++++++++++++++++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> index 2d2ccc477c91..37747c0b2f27 100644
> --- a/include/libcamera/internal/v4l2_videodevice.h
> +++ b/include/libcamera/internal/v4l2_videodevice.h
> @@ -126,6 +126,7 @@ public:
>  
>  	int get(const FrameBuffer &buffer);
>  	void put(unsigned int index);
> +	bool isEmpty() const;

I'd move this before get() and put(), as we usually declare the const
query functions first. Same in the .cpp file. This can be handled when
applying.

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

>  private:
>  	class Entry
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 5f36ee20710d..9da82697e7f0 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -262,6 +262,19 @@ void V4L2BufferCache::put(unsigned int index)
>  	cache_[index].free_ = true;
>  }
>  
> +/**
> + * \brief Check if all the entries in the cache are unused
> + */
> +bool V4L2BufferCache::isEmpty() const
> +{
> +	for (auto const &entry : cache_) {
> +		if (!entry.free_)
> +			return false;
> +	}
> +
> +	return true;
> +}
> +
>  V4L2BufferCache::Entry::Entry()
>  	: free_(true), lastUsed_(0)
>  {
> @@ -1832,10 +1845,13 @@ int V4L2VideoDevice::streamOff()
>  	for (auto it : queuedBuffers_) {
>  		FrameBuffer *buffer = it.second;
>  
> +		cache_->put(it.first);
>  		buffer->metadata_.status = FrameMetadata::FrameCancelled;
>  		bufferReady.emit(buffer);
>  	}
>  
> +	ASSERT(cache_->isEmpty());
> +
>  	queuedBuffers_.clear();
>  	fdBufferNotifier_->setEnabled(false);
>  	streaming_ = false;

-- 
Regards,

Laurent Pinchart