[libcamera-devel] [PATCH v2 1/4] pipeline: rpi: Increase buffer import count to 32

[Jacopo Mondi]

Hi Naush

On Tue, Jul 25, 2023 at 09:55:37AM +0100, Naushir Patuck via libcamera-devel wrote:
> Hardcode the maximum number of buffers imported to the V4L2 video device
> to 32. This only has a minor disadvantage of over-allocating cache slots
> and V4L2 buffer indexes, but does allow more headroom for using dma
> buffers allocated from outside of libcamera.
>
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
>  .../pipeline/rpi/common/rpi_stream.cpp        | 33 +++++--------------
>  1 file changed, 8 insertions(+), 25 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp
> index c158843cb0ed..e9ad1e6f0848 100644
> --- a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp
> +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp
> @@ -95,34 +95,17 @@ int Stream::prepareBuffers(unsigned int count)
>  	int ret;
>
>  	if (!(flags_ & StreamFlag::ImportOnly)) {
> -		if (count) {
> -			/* Export some frame buffers for internal use. */
> -			ret = dev_->exportBuffers(count, &internalBuffers_);
> -			if (ret < 0)
> -				return ret;
> -
> -			/* Add these exported buffers to the internal/external buffer list. */
> -			setExportedBuffers(&internalBuffers_);
> -			resetBuffers();
> -		}
> +		/* Export some frame buffers for internal use. */
> +		ret = dev_->exportBuffers(count, &internalBuffers_);
> +		if (ret < 0)
> +			return ret;
>
> -		/* We must import all internal/external exported buffers. */
> -		count = bufferMap_.size();
> +		/* Add these exported buffers to the internal/external buffer list. */
> +		setExportedBuffers(&internalBuffers_);
> +		resetBuffers();
>  	}
>
> -	/*
> -	 * If this is an external stream, we must allocate slots for buffers that
> -	 * might be externally allocated. We have no indication of how many buffers
> -	 * may be used, so this might overallocate slots in the buffer cache.
> -	 * Similarly, if this stream is only importing buffers, we do the same.
> -	 *
> -	 * \todo Find a better heuristic, or, even better, an exact solution to
> -	 * this issue.
> -	 */
> -	if ((flags_ & StreamFlag::External) || (flags_ & StreamFlag::ImportOnly))
> -		count = count * 2;
> -
> -	return dev_->importBuffers(count);
> +	return dev_->importBuffers(32);

Might be good to define 32 somewhere as constexpr to avoid magic
numbers, but apart from this

Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>

Thanks
  j

>  }
>
>  int Stream::queueBuffer(FrameBuffer *buffer)
> --
> 2.34.1
>