[libcamera-devel,v9,04/18] libcamera: pipeline: raspberrypi: Don't rely on bufferCount
diff mbox series

Message ID 20221216122939.256534-5-paul.elder@ideasonboard.com
State Superseded
Headers show
Series
  • lc-compliance: Add test to queue more requests than hardware depth
Related show

Commit Message

Paul Elder Dec. 16, 2022, 12:29 p.m. UTC 
From: Nícolas F. R. A. Prado <nfraprado@collabora.com>

Currently the raspberrypi pipeline handler relies on bufferCount to
decide on the number of buffers to allocate internally and for the
number of V4L2 buffer slots to reserve. Instead, the number of internal
buffers should be the minimum required by the pipeline to keep the
requests flowing, in order to avoid wasting memory, while the number of
V4L2 buffer slots should be greater than the expected number of requests
queued, in order to avoid thrashing dmabuf mappings, which would degrade
performance.

Extend the Stream class to receive the number of internal buffers that
should be allocated, and optionally the number of buffer slots to
reserve. Use these new member variables to specify better suited numbers
for internal buffers and buffer slots for each stream individually,
which also allows us to remove bufferCount.

Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

---
Changes in v9:
- rebased
  - I've decided that the buffer allocation decisions that Nícolas
    implemented covered the same cases that were added in
    PipelineHandlerRPi::prepareBuffers(), but in a slightly nicer way,
    especially considering that bufferCount is to be removed from
    StreamConfiguration in this series. Comments welcome, of course.

Changes in v8:
- Reworked buffer allocation handling in the raspberrypi pipeline handler
- New
---
 .../pipeline/raspberrypi/raspberrypi.cpp      | 83 +++++++------------
 .../pipeline/raspberrypi/rpi_stream.cpp       | 39 +++++----
 .../pipeline/raspberrypi/rpi_stream.h         | 24 +++++-
 3 files changed, 71 insertions(+), 75 deletions(-)

Comments

Naushir Patuck Dec. 16, 2022, 1:39 p.m. UTC | #1 
Hi Paul,

On Fri, 16 Dec 2022 at 12:30, Paul Elder via libcamera-devel <
libcamera-devel@lists.libcamera.org> wrote:

> From: Nícolas F. R. A. Prado <nfraprado@collabora.com>
>
> Currently the raspberrypi pipeline handler relies on bufferCount to
> decide on the number of buffers to allocate internally and for the
> number of V4L2 buffer slots to reserve. Instead, the number of internal
> buffers should be the minimum required by the pipeline to keep the
> requests flowing, in order to avoid wasting memory, while the number of
> V4L2 buffer slots should be greater than the expected number of requests
> queued, in order to avoid thrashing dmabuf mappings, which would degrade
> performance.
>
> Extend the Stream class to receive the number of internal buffers that
> should be allocated, and optionally the number of buffer slots to
> reserve. Use these new member variables to specify better suited numbers
> for internal buffers and buffer slots for each stream individually,
> which also allows us to remove bufferCount.
>
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
>
>
I'm afraid this is going to conflict badly with my changes in [1] where I've
made substantial optimisations to the buffer allocation logic :-(

However, if the aim of this commit is to remove the use of bufferCount from
prepareBuffers(), then this ought to be a simple change on top of [1]:

diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
index ec416ab655c7..77f529ea0b46 100644
--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
@@ -1511,7 +1511,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)

        for (Stream *s : camera->streams()) {
                if (s == &data->unicam_[Unicam::Image]) {
-                       numRawBuffers = s->configuration().bufferCount;
+                       numRawBuffers =
data->unicam_[Unicam::Image].getBuffers().size();
                        /*
                         * If the application provides a guarantees that
Unicam
                         * image buffers will always be provided for the
RAW stream

There is also the parallel discussion of using StreamConfig::bufferCout to
provide applications with minimum buffer/requests required for the pipeline
to
run correctly.  So we may not be able to remove this just yet...

Regards,
Naush

[1] https://patchwork.libcamera.org/project/libcamera/list/?series=3663



> ---
> Changes in v9:
> - rebased
>   - I've decided that the buffer allocation decisions that Nícolas
>     implemented covered the same cases that were added in
>     PipelineHandlerRPi::prepareBuffers(), but in a slightly nicer way,
>     especially considering that bufferCount is to be removed from
>     StreamConfiguration in this series. Comments welcome, of course.
>
> Changes in v8:
> - Reworked buffer allocation handling in the raspberrypi pipeline handler
> - New
> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 83 +++++++------------
>  .../pipeline/raspberrypi/rpi_stream.cpp       | 39 +++++----
>  .../pipeline/raspberrypi/rpi_stream.h         | 24 +++++-
>  3 files changed, 71 insertions(+), 75 deletions(-)
>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 4641c76f..72502c36 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -341,6 +341,25 @@ public:
>         void releaseDevice(Camera *camera) override;
>
>  private:
> +       /*
> +        * The number of buffers to allocate internally for transferring
> raw
> +        * frames from the Unicam Image stream to the ISP Input stream. It
> is
> +        * defined such that:
> +        * - one buffer is being written to in Unicam Image
> +        * - one buffer is being processed in ISP Input
> +        * - one extra buffer is queued to Unicam Image
> +        */
> +       static constexpr unsigned int kInternalRawBufferCount = 3;
> +
> +       /*
> +        * The number of buffers to allocate internally for the external
> +        * streams. They're used to drop the first few frames while the IPA
> +        * algorithms converge. It is defined such that:
> +        * - one buffer is being used on the stream
> +        * - one extra buffer is queued
> +        */
> +       static constexpr unsigned int kInternalDropoutBufferCount = 2;
> +
>         RPiCameraData *cameraData(Camera *camera)
>         {
>                 return static_cast<RPiCameraData *>(camera->_d());
> @@ -1221,21 +1240,23 @@ int PipelineHandlerRPi::registerCamera(MediaDevice
> *unicam, MediaDevice *isp, Me
>                 return -ENOENT;
>
>         /* Locate and open the unicam video streams. */
> -       data->unicam_[Unicam::Image] = RPi::Stream("Unicam Image",
> unicamImage);
> +       data->unicam_[Unicam::Image] = RPi::Stream("Unicam Image",
> unicamImage,
> +
> kInternalRawBufferCount);
>
>         /* An embedded data node will not be present if the sensor does
> not support it. */
>         MediaEntity *unicamEmbedded =
> unicam->getEntityByName("unicam-embedded");
>         if (unicamEmbedded) {
> -               data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam
> Embedded", unicamEmbedded);
> +               data->unicam_[Unicam::Embedded] =
> +                       RPi::Stream("Unicam Embedded", unicamEmbedded,
> kInternalRawBufferCount);
>
> data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data.get(),
>
>  &RPiCameraData::unicamBufferDequeue);
>         }
>
>         /* Tag the ISP input stream as an import stream. */
> -       data->isp_[Isp::Input] = RPi::Stream("ISP Input", ispOutput0,
> true);
> -       data->isp_[Isp::Output0] = RPi::Stream("ISP Output0", ispCapture1);
> -       data->isp_[Isp::Output1] = RPi::Stream("ISP Output1", ispCapture2);
> -       data->isp_[Isp::Stats] = RPi::Stream("ISP Stats", ispCapture3);
> +       data->isp_[Isp::Input] = RPi::Stream("ISP Input", ispOutput0, 0,
> kInternalRawBufferCount, true);
> +       data->isp_[Isp::Output0] = RPi::Stream("ISP Output0", ispCapture1,
> kInternalDropoutBufferCount);
> +       data->isp_[Isp::Output1] = RPi::Stream("ISP Output1", ispCapture2,
> kInternalDropoutBufferCount);
> +       data->isp_[Isp::Stats] = RPi::Stream("ISP Stats", ispCapture3,
> kInternalDropoutBufferCount);
>
>         /* Wire up all the buffer connections. */
>
> data->unicam_[Unicam::Image].dev()->dequeueTimeout.connect(data.get(),
> &RPiCameraData::unicamTimeout);
> @@ -1428,57 +1449,11 @@ int PipelineHandlerRPi::queueAllBuffers(Camera
> *camera)
>  int PipelineHandlerRPi::prepareBuffers(Camera *camera)
>  {
>         RPiCameraData *data = cameraData(camera);
> -       unsigned int numRawBuffers = 0;
>         int ret;
>
> -       for (Stream *s : camera->streams()) {
> -               if (isRaw(s->configuration().pixelFormat)) {
> -                       numRawBuffers = s->configuration().bufferCount;
> -                       break;
> -               }
> -       }
> -
> -       /* Decide how many internal buffers to allocate. */
> +       /* Allocate internal buffers. */
>         for (auto const stream : data->streams_) {
> -               unsigned int numBuffers;
> -               /*
> -                * For Unicam, allocate a minimum of 4 buffers as we want
> -                * to avoid any frame drops.
> -                */
> -               constexpr unsigned int minBuffers = 4;
> -               if (stream == &data->unicam_[Unicam::Image]) {
> -                       /*
> -                        * If an application has configured a RAW stream,
> allocate
> -                        * additional buffers to make up the minimum, but
> ensure
> -                        * we have at least 2 sets of internal buffers to
> use to
> -                        * minimise frame drops.
> -                        */
> -                       numBuffers = std::max<int>(2, minBuffers -
> numRawBuffers);
> -               } else if (stream == &data->isp_[Isp::Input]) {
> -                       /*
> -                        * ISP input buffers are imported from Unicam, so
> follow
> -                        * similar logic as above to count all the RAW
> buffers
> -                        * available.
> -                        */
> -                       numBuffers = numRawBuffers + std::max<int>(2,
> minBuffers - numRawBuffers);
> -
> -               } else if (stream == &data->unicam_[Unicam::Embedded]) {
> -                       /*
> -                        * Embedded data buffers are (currently) for
> internal use,
> -                        * so allocate the minimum required to avoid frame
> drops.
> -                        */
> -                       numBuffers = minBuffers;
> -               } else {
> -                       /*
> -                        * Since the ISP runs synchronous with the IPA and
> requests,
> -                        * we only ever need one set of internal buffers.
> Any buffers
> -                        * the application wants to hold onto will already
> be exported
> -                        * through
> PipelineHandlerRPi::exportFrameBuffers().
> -                        */
> -                       numBuffers = 1;
> -               }
> -
> -               ret = stream->prepareBuffers(numBuffers);
> +               ret = stream->prepareBuffers();
>                 if (ret < 0)
>                         return ret;
>         }
> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> index 2bb10f25..cde04efa 100644
> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> @@ -84,14 +84,24 @@ void Stream::removeExternalBuffer(FrameBuffer *buffer)
>         bufferMap_.erase(id);
>  }
>
> -int Stream::prepareBuffers(unsigned int count)
> +int Stream::prepareBuffers()
>  {
> +       unsigned int slotCount;
>         int ret;
>
>         if (!importOnly_) {
> -               if (count) {
> +               /*
> +                * External streams overallocate buffer slots in order to
> handle
> +                * the buffers queued from applications without degrading
> +                * performance. Internal streams only need to have enough
> buffer
> +                * slots to have the internal buffers queued.
> +                */
> +               slotCount = isExternal() ? kRPIExternalBufferSlotCount
> +                                        : internalBufferCount_;
> +
> +               if (internalBufferCount_) {
>                         /* Export some frame buffers for internal use. */
> -                       ret = dev_->exportBuffers(count,
> &internalBuffers_);
> +                       ret = dev_->exportBuffers(internalBufferCount_,
> &internalBuffers_);
>                         if (ret < 0)
>                                 return ret;
>
> @@ -100,23 +110,16 @@ int Stream::prepareBuffers(unsigned int count)
>                         resetBuffers();
>                 }
>
> -               /* We must import all internal/external exported buffers.
> */
> -               count = bufferMap_.size();
> +       } else {
> +               /*
> +                * An importOnly stream doesn't have its own internal
> buffers,
> +                * so it needs to have the number of buffer slots to
> reserve
> +                * explicitly declared.
> +                */
> +               slotCount = bufferSlotCount_;
>         }
>
> -       /*
> -        * 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 (isExternal() || importOnly_)
> -               count = count * 2;
> -
> -       return dev_->importBuffers(count);
> +       return dev_->importBuffers(slotCount);
>  }
>
>  int Stream::queueBuffer(FrameBuffer *buffer)
> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> index b8bd79cf..e63bf02b 100644
> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> @@ -42,9 +42,13 @@ public:
>         {
>         }
>
> -       Stream(const char *name, MediaEntity *dev, bool importOnly = false)
> +       Stream(const char *name, MediaEntity *dev,
> +              unsigned int internalBufferCount,
> +              unsigned int bufferSlotCount = 0, bool importOnly = false)
>                 : external_(false), importOnly_(importOnly), name_(name),
> -                 dev_(std::make_unique<V4L2VideoDevice>(dev)),
> id_(BufferMask::MaskID)
> +                 dev_(std::make_unique<V4L2VideoDevice>(dev)),
> id_(BufferMask::MaskID),
> +                 internalBufferCount_(internalBufferCount),
> +                 bufferSlotCount_(bufferSlotCount)
>         {
>         }
>
> @@ -63,7 +67,7 @@ public:
>         void setExternalBuffer(FrameBuffer *buffer);
>         void removeExternalBuffer(FrameBuffer *buffer);
>
> -       int prepareBuffers(unsigned int count);
> +       int prepareBuffers();
>         int queueBuffer(FrameBuffer *buffer);
>         void returnBuffer(FrameBuffer *buffer);
>
> @@ -71,6 +75,8 @@ public:
>         void releaseBuffers();
>
>  private:
> +       static constexpr unsigned int kRPIExternalBufferSlotCount = 16;
> +
>         class IdGenerator
>         {
>         public:
> @@ -133,6 +139,18 @@ private:
>         /* All frame buffers associated with this device stream. */
>         BufferMap bufferMap_;
>
> +       /*
> +        * The number of buffers that should be allocated internally for
> this
> +        * stream.
> +        */
> +       unsigned int internalBufferCount_;
> +
> +       /*
> +        * The number of buffer slots that should be reserved for this
> stream.
> +        * Only relevant for importOnly streams.
> +        */
> +       unsigned int bufferSlotCount_;
> +
>         /*
>          * List of frame buffers that we can use if none have been
> provided by
>          * the application for external streams. This is populated by the
> --
> 2.35.1
>
>
Jacopo Mondi Dec. 16, 2022, 2:46 p.m. UTC | #2 
Hi Paul

On Fri, Dec 16, 2022 at 09:29:25PM +0900, Paul Elder via libcamera-devel wrote:
> From: Nícolas F. R. A. Prado <nfraprado@collabora.com>
>
> Currently the raspberrypi pipeline handler relies on bufferCount to
> decide on the number of buffers to allocate internally and for the
> number of V4L2 buffer slots to reserve. Instead, the number of internal
> buffers should be the minimum required by the pipeline to keep the
> requests flowing, in order to avoid wasting memory, while the number of
> V4L2 buffer slots should be greater than the expected number of requests
> queued, in order to avoid thrashing dmabuf mappings, which would degrade
> performance.
>
> Extend the Stream class to receive the number of internal buffers that
> should be allocated, and optionally the number of buffer slots to
> reserve. Use these new member variables to specify better suited numbers
> for internal buffers and buffer slots for each stream individually,
> which also allows us to remove bufferCount.
>
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
>
> ---
> Changes in v9:
> - rebased
>   - I've decided that the buffer allocation decisions that Nícolas
>     implemented covered the same cases that were added in
>     PipelineHandlerRPi::prepareBuffers(), but in a slightly nicer way,
>     especially considering that bufferCount is to be removed from
>     StreamConfiguration in this series. Comments welcome, of course.
>
> Changes in v8:
> - Reworked buffer allocation handling in the raspberrypi pipeline handler
> - New
> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 83 +++++++------------
>  .../pipeline/raspberrypi/rpi_stream.cpp       | 39 +++++----
>  .../pipeline/raspberrypi/rpi_stream.h         | 24 +++++-
>  3 files changed, 71 insertions(+), 75 deletions(-)
>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 4641c76f..72502c36 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -341,6 +341,25 @@ public:
>  	void releaseDevice(Camera *camera) override;
>
>  private:
> +	/*
> +	 * The number of buffers to allocate internally for transferring raw
> +	 * frames from the Unicam Image stream to the ISP Input stream. It is
> +	 * defined such that:
> +	 * - one buffer is being written to in Unicam Image
> +	 * - one buffer is being processed in ISP Input
> +	 * - one extra buffer is queued to Unicam Image
> +	 */
> +	static constexpr unsigned int kInternalRawBufferCount = 3;
> +
> +	/*
> +	 * The number of buffers to allocate internally for the external
> +	 * streams. They're used to drop the first few frames while the IPA
> +	 * algorithms converge. It is defined such that:
> +	 * - one buffer is being used on the stream
> +	 * - one extra buffer is queued
> +	 */
> +	static constexpr unsigned int kInternalDropoutBufferCount = 2;
> +
>  	RPiCameraData *cameraData(Camera *camera)
>  	{
>  		return static_cast<RPiCameraData *>(camera->_d());
> @@ -1221,21 +1240,23 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me
>  		return -ENOENT;
>
>  	/* Locate and open the unicam video streams. */
> -	data->unicam_[Unicam::Image] = RPi::Stream("Unicam Image", unicamImage);
> +	data->unicam_[Unicam::Image] = RPi::Stream("Unicam Image", unicamImage,
> +						   kInternalRawBufferCount);
>
>  	/* An embedded data node will not be present if the sensor does not support it. */
>  	MediaEntity *unicamEmbedded = unicam->getEntityByName("unicam-embedded");
>  	if (unicamEmbedded) {
> -		data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam Embedded", unicamEmbedded);
> +		data->unicam_[Unicam::Embedded] =
> +			RPi::Stream("Unicam Embedded", unicamEmbedded, kInternalRawBufferCount);
>  		data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data.get(),
>  									   &RPiCameraData::unicamBufferDequeue);
>  	}
>
>  	/* Tag the ISP input stream as an import stream. */
> -	data->isp_[Isp::Input] = RPi::Stream("ISP Input", ispOutput0, true);
> -	data->isp_[Isp::Output0] = RPi::Stream("ISP Output0", ispCapture1);
> -	data->isp_[Isp::Output1] = RPi::Stream("ISP Output1", ispCapture2);
> -	data->isp_[Isp::Stats] = RPi::Stream("ISP Stats", ispCapture3);
> +	data->isp_[Isp::Input] = RPi::Stream("ISP Input", ispOutput0, 0, kInternalRawBufferCount, true);
> +	data->isp_[Isp::Output0] = RPi::Stream("ISP Output0", ispCapture1, kInternalDropoutBufferCount);
> +	data->isp_[Isp::Output1] = RPi::Stream("ISP Output1", ispCapture2, kInternalDropoutBufferCount);
> +	data->isp_[Isp::Stats] = RPi::Stream("ISP Stats", ispCapture3, kInternalDropoutBufferCount);
>
>  	/* Wire up all the buffer connections. */
>  	data->unicam_[Unicam::Image].dev()->dequeueTimeout.connect(data.get(), &RPiCameraData::unicamTimeout);
> @@ -1428,57 +1449,11 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera)
>  int PipelineHandlerRPi::prepareBuffers(Camera *camera)
>  {
>  	RPiCameraData *data = cameraData(camera);
> -	unsigned int numRawBuffers = 0;
>  	int ret;
>
> -	for (Stream *s : camera->streams()) {
> -		if (isRaw(s->configuration().pixelFormat)) {
> -			numRawBuffers = s->configuration().bufferCount;
> -			break;
> -		}
> -	}
> -
> -	/* Decide how many internal buffers to allocate. */
> +	/* Allocate internal buffers. */
>  	for (auto const stream : data->streams_) {
> -		unsigned int numBuffers;
> -		/*
> -		 * For Unicam, allocate a minimum of 4 buffers as we want
> -		 * to avoid any frame drops.
> -		 */

> -		constexpr unsigned int minBuffers = 4;
> -		if (stream == &data->unicam_[Unicam::Image]) {
> -			/*
> -			 * If an application has configured a RAW stream, allocate
> -			 * additional buffers to make up the minimum, but ensure
> -			 * we have at least 2 sets of internal buffers to use to
> -			 * minimise frame drops.
> -			 */
> -			numBuffers = std::max<int>(2, minBuffers - numRawBuffers);

For Unicam the old code took into account the external RAW buffers,
and if none was requested 4 buffers where allocated. So it seems a
number between 2 and 4 was possible. That's why you chose 3 in

	static constexpr unsigned int kInternalRawBufferCount = 3;

> -		} else if (stream == &data->isp_[Isp::Input]) {
> -			/*
> -			 * ISP input buffers are imported from Unicam, so follow
> -			 * similar logic as above to count all the RAW buffers
> -			 * available.
> -			 */
> -			numBuffers = numRawBuffers + std::max<int>(2, minBuffers - numRawBuffers);
> -
> -		} else if (stream == &data->unicam_[Unicam::Embedded]) {
> -			/*
> -			 * Embedded data buffers are (currently) for internal use,
> -			 * so allocate the minimum required to avoid frame drops.
> -			 */
> -			numBuffers = minBuffers;

This was 4 and now it's 3

The rest of the code seems to work as it used to, but it would be
great to have a confirmation from RPi and a test run on the platform

Thanks
  j


> -		} else {
> -			/*
> -			 * Since the ISP runs synchronous with the IPA and requests,
> -			 * we only ever need one set of internal buffers. Any buffers
> -			 * the application wants to hold onto will already be exported
> -			 * through PipelineHandlerRPi::exportFrameBuffers().
> -			 */
> -			numBuffers = 1;
> -		}
> -
> -		ret = stream->prepareBuffers(numBuffers);
> +		ret = stream->prepareBuffers();
>  		if (ret < 0)
>  			return ret;
>  	}
> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> index 2bb10f25..cde04efa 100644
> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> @@ -84,14 +84,24 @@ void Stream::removeExternalBuffer(FrameBuffer *buffer)
>  	bufferMap_.erase(id);
>  }
>
> -int Stream::prepareBuffers(unsigned int count)
> +int Stream::prepareBuffers()
>  {
> +	unsigned int slotCount;
>  	int ret;
>
>  	if (!importOnly_) {
> -		if (count) {
> +		/*
> +		 * External streams overallocate buffer slots in order to handle
> +		 * the buffers queued from applications without degrading
> +		 * performance. Internal streams only need to have enough buffer
> +		 * slots to have the internal buffers queued.
> +		 */
> +		slotCount = isExternal() ? kRPIExternalBufferSlotCount
> +					 : internalBufferCount_;
> +
> +		if (internalBufferCount_) {
>  			/* Export some frame buffers for internal use. */
> -			ret = dev_->exportBuffers(count, &internalBuffers_);
> +			ret = dev_->exportBuffers(internalBufferCount_, &internalBuffers_);
>  			if (ret < 0)
>  				return ret;
>
> @@ -100,23 +110,16 @@ int Stream::prepareBuffers(unsigned int count)
>  			resetBuffers();
>  		}
>
> -		/* We must import all internal/external exported buffers. */
> -		count = bufferMap_.size();
> +	} else {
> +		/*
> +		 * An importOnly stream doesn't have its own internal buffers,
> +		 * so it needs to have the number of buffer slots to reserve
> +		 * explicitly declared.
> +		 */
> +		slotCount = bufferSlotCount_;
>  	}
>
> -	/*
> -	 * 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 (isExternal() || importOnly_)
> -		count = count * 2;
> -
> -	return dev_->importBuffers(count);
> +	return dev_->importBuffers(slotCount);
>  }
>
>  int Stream::queueBuffer(FrameBuffer *buffer)
> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> index b8bd79cf..e63bf02b 100644
> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> @@ -42,9 +42,13 @@ public:
>  	{
>  	}
>
> -	Stream(const char *name, MediaEntity *dev, bool importOnly = false)
> +	Stream(const char *name, MediaEntity *dev,
> +	       unsigned int internalBufferCount,
> +	       unsigned int bufferSlotCount = 0, bool importOnly = false)
>  		: external_(false), importOnly_(importOnly), name_(name),
> -		  dev_(std::make_unique<V4L2VideoDevice>(dev)), id_(BufferMask::MaskID)
> +		  dev_(std::make_unique<V4L2VideoDevice>(dev)), id_(BufferMask::MaskID),
> +		  internalBufferCount_(internalBufferCount),
> +		  bufferSlotCount_(bufferSlotCount)
>  	{
>  	}
>
> @@ -63,7 +67,7 @@ public:
>  	void setExternalBuffer(FrameBuffer *buffer);
>  	void removeExternalBuffer(FrameBuffer *buffer);
>
> -	int prepareBuffers(unsigned int count);
> +	int prepareBuffers();
>  	int queueBuffer(FrameBuffer *buffer);
>  	void returnBuffer(FrameBuffer *buffer);
>
> @@ -71,6 +75,8 @@ public:
>  	void releaseBuffers();
>
>  private:
> +	static constexpr unsigned int kRPIExternalBufferSlotCount = 16;
> +
>  	class IdGenerator
>  	{
>  	public:
> @@ -133,6 +139,18 @@ private:
>  	/* All frame buffers associated with this device stream. */
>  	BufferMap bufferMap_;
>
> +	/*
> +	 * The number of buffers that should be allocated internally for this
> +	 * stream.
> +	 */
> +	unsigned int internalBufferCount_;
> +
> +	/*
> +	 * The number of buffer slots that should be reserved for this stream.
> +	 * Only relevant for importOnly streams.
> +	 */
> +	unsigned int bufferSlotCount_;
> +
>  	/*
>  	 * List of frame buffers that we can use if none have been provided by
>  	 * the application for external streams. This is populated by the
> --
> 2.35.1
>
Paul Elder Dec. 28, 2022, 2:21 a.m. UTC | #3 
Hi Naush,

On Fri, Dec 16, 2022 at 01:39:23PM +0000, Naushir Patuck wrote:
> Hi Paul,
> 
> On Fri, 16 Dec 2022 at 12:30, Paul Elder via libcamera-devel <
> libcamera-devel@lists.libcamera.org> wrote:
> 
>     From: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> 
>     Currently the raspberrypi pipeline handler relies on bufferCount to
>     decide on the number of buffers to allocate internally and for the
>     number of V4L2 buffer slots to reserve. Instead, the number of internal
>     buffers should be the minimum required by the pipeline to keep the
>     requests flowing, in order to avoid wasting memory, while the number of
>     V4L2 buffer slots should be greater than the expected number of requests
>     queued, in order to avoid thrashing dmabuf mappings, which would degrade
>     performance.
> 
>     Extend the Stream class to receive the number of internal buffers that
>     should be allocated, and optionally the number of buffer slots to
>     reserve. Use these new member variables to specify better suited numbers
>     for internal buffers and buffer slots for each stream individually,
>     which also allows us to remove bufferCount.
> 
>     Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
>     Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> 
> 
> I'm afraid this is going to conflict badly with my changes in [1] where I've
> made substantial optimisations to the buffer allocation logic :-(

Oh... yeah.

> 
> However, if the aim of this commit is to remove the use of bufferCount from

Yeah that's the goal.

> prepareBuffers(), then this ought to be a simple change on top of [1]:
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera
> /pipeline/raspberrypi/raspberrypi.cpp
> index ec416ab655c7..77f529ea0b46 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1511,7 +1511,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> 
>         for (Stream *s : camera->streams()) {
>                 if (s == &data->unicam_[Unicam::Image]) {
> -                       numRawBuffers = s->configuration().bufferCount;
> +                       numRawBuffers = data->unicam_[Unicam::Image].getBuffers
> ().size();
>                         /*
>                          * If the application provides a guarantees that Unicam
>                          * image buffers will always be provided for the RAW
> stream

Are you saying that this complex patch can just be reduced to that
hunk...?

I have a feeling that this series might make it in first.

> There is also the parallel discussion of using StreamConfig::bufferCout to
> provide applications with minimum buffer/requests required for the pipeline to
> run correctly.  So we may not be able to remove this just yet...

This series introduces a MinimumRequests property which reports to
applications the minimum number of requests required for the pipeline to
run without frame drops, so I think this covers that.


Thanks,

Paul

> 
> [1] https://patchwork.libcamera.org/project/libcamera/list/?series=3663
> 
>  
> 
>     ---
>     Changes in v9:
>     - rebased
>       - I've decided that the buffer allocation decisions that Nícolas
>         implemented covered the same cases that were added in
>         PipelineHandlerRPi::prepareBuffers(), but in a slightly nicer way,
>         especially considering that bufferCount is to be removed from
>         StreamConfiguration in this series. Comments welcome, of course.
> 
>     Changes in v8:
>     - Reworked buffer allocation handling in the raspberrypi pipeline handler
>     - New
>     ---
>      .../pipeline/raspberrypi/raspberrypi.cpp      | 83 +++++++------------
>      .../pipeline/raspberrypi/rpi_stream.cpp       | 39 +++++----
>      .../pipeline/raspberrypi/rpi_stream.h         | 24 +++++-
>      3 files changed, 71 insertions(+), 75 deletions(-)
> 
>     diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/
>     libcamera/pipeline/raspberrypi/raspberrypi.cpp
>     index 4641c76f..72502c36 100644
>     --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>     +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>     @@ -341,6 +341,25 @@ public:
>             void releaseDevice(Camera *camera) override;
> 
>      private:
>     +       /*
>     +        * The number of buffers to allocate internally for transferring
>     raw
>     +        * frames from the Unicam Image stream to the ISP Input stream. It
>     is
>     +        * defined such that:
>     +        * - one buffer is being written to in Unicam Image
>     +        * - one buffer is being processed in ISP Input
>     +        * - one extra buffer is queued to Unicam Image
>     +        */
>     +       static constexpr unsigned int kInternalRawBufferCount = 3;
>     +
>     +       /*
>     +        * The number of buffers to allocate internally for the external
>     +        * streams. They're used to drop the first few frames while the IPA
>     +        * algorithms converge. It is defined such that:
>     +        * - one buffer is being used on the stream
>     +        * - one extra buffer is queued
>     +        */
>     +       static constexpr unsigned int kInternalDropoutBufferCount = 2;
>     +
>             RPiCameraData *cameraData(Camera *camera)
>             {
>                     return static_cast<RPiCameraData *>(camera->_d());
>     @@ -1221,21 +1240,23 @@ int PipelineHandlerRPi::registerCamera(MediaDevice
>     *unicam, MediaDevice *isp, Me
>                     return -ENOENT;
> 
>             /* Locate and open the unicam video streams. */
>     -       data->unicam_[Unicam::Image] = RPi::Stream("Unicam Image",
>     unicamImage);
>     +       data->unicam_[Unicam::Image] = RPi::Stream("Unicam Image",
>     unicamImage,
>     +                                                 
>     kInternalRawBufferCount);
> 
>             /* An embedded data node will not be present if the sensor does not
>     support it. */
>             MediaEntity *unicamEmbedded = unicam->getEntityByName
>     ("unicam-embedded");
>             if (unicamEmbedded) {
>     -               data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam
>     Embedded", unicamEmbedded);
>     +               data->unicam_[Unicam::Embedded] =
>     +                       RPi::Stream("Unicam Embedded", unicamEmbedded,
>     kInternalRawBufferCount);
>                     data->unicam_[Unicam::Embedded].dev()->bufferReady.connect
>     (data.get(),
>                                                                                
>     &RPiCameraData::unicamBufferDequeue);
>             }
> 
>             /* Tag the ISP input stream as an import stream. */
>     -       data->isp_[Isp::Input] = RPi::Stream("ISP Input", ispOutput0,
>     true);
>     -       data->isp_[Isp::Output0] = RPi::Stream("ISP Output0", ispCapture1);
>     -       data->isp_[Isp::Output1] = RPi::Stream("ISP Output1", ispCapture2);
>     -       data->isp_[Isp::Stats] = RPi::Stream("ISP Stats", ispCapture3);
>     +       data->isp_[Isp::Input] = RPi::Stream("ISP Input", ispOutput0, 0,
>     kInternalRawBufferCount, true);
>     +       data->isp_[Isp::Output0] = RPi::Stream("ISP Output0", ispCapture1,
>     kInternalDropoutBufferCount);
>     +       data->isp_[Isp::Output1] = RPi::Stream("ISP Output1", ispCapture2,
>     kInternalDropoutBufferCount);
>     +       data->isp_[Isp::Stats] = RPi::Stream("ISP Stats", ispCapture3,
>     kInternalDropoutBufferCount);
> 
>             /* Wire up all the buffer connections. */
>             data->unicam_[Unicam::Image].dev()->dequeueTimeout.connect(data.get
>     (), &RPiCameraData::unicamTimeout);
>     @@ -1428,57 +1449,11 @@ int PipelineHandlerRPi::queueAllBuffers(Camera
>     *camera)
>      int PipelineHandlerRPi::prepareBuffers(Camera *camera)
>      {
>             RPiCameraData *data = cameraData(camera);
>     -       unsigned int numRawBuffers = 0;
>             int ret;
> 
>     -       for (Stream *s : camera->streams()) {
>     -               if (isRaw(s->configuration().pixelFormat)) {
>     -                       numRawBuffers = s->configuration().bufferCount;
>     -                       break;
>     -               }
>     -       }
>     -
>     -       /* Decide how many internal buffers to allocate. */
>     +       /* Allocate internal buffers. */
>             for (auto const stream : data->streams_) {
>     -               unsigned int numBuffers;
>     -               /*
>     -                * For Unicam, allocate a minimum of 4 buffers as we want
>     -                * to avoid any frame drops.
>     -                */
>     -               constexpr unsigned int minBuffers = 4;
>     -               if (stream == &data->unicam_[Unicam::Image]) {
>     -                       /*
>     -                        * If an application has configured a RAW stream,
>     allocate
>     -                        * additional buffers to make up the minimum, but
>     ensure
>     -                        * we have at least 2 sets of internal buffers to
>     use to
>     -                        * minimise frame drops.
>     -                        */
>     -                       numBuffers = std::max<int>(2, minBuffers -
>     numRawBuffers);
>     -               } else if (stream == &data->isp_[Isp::Input]) {
>     -                       /*
>     -                        * ISP input buffers are imported from Unicam, so
>     follow
>     -                        * similar logic as above to count all the RAW
>     buffers
>     -                        * available.
>     -                        */
>     -                       numBuffers = numRawBuffers + std::max<int>(2,
>     minBuffers - numRawBuffers);
>     -
>     -               } else if (stream == &data->unicam_[Unicam::Embedded]) {
>     -                       /*
>     -                        * Embedded data buffers are (currently) for
>     internal use,
>     -                        * so allocate the minimum required to avoid frame
>     drops.
>     -                        */
>     -                       numBuffers = minBuffers;
>     -               } else {
>     -                       /*
>     -                        * Since the ISP runs synchronous with the IPA and
>     requests,
>     -                        * we only ever need one set of internal buffers.
>     Any buffers
>     -                        * the application wants to hold onto will already
>     be exported
>     -                        * through PipelineHandlerRPi::exportFrameBuffers
>     ().
>     -                        */
>     -                       numBuffers = 1;
>     -               }
>     -
>     -               ret = stream->prepareBuffers(numBuffers);
>     +               ret = stream->prepareBuffers();
>                     if (ret < 0)
>                             return ret;
>             }
>     diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/
>     libcamera/pipeline/raspberrypi/rpi_stream.cpp
>     index 2bb10f25..cde04efa 100644
>     --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
>     +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
>     @@ -84,14 +84,24 @@ void Stream::removeExternalBuffer(FrameBuffer *buffer)
>             bufferMap_.erase(id);
>      }
> 
>     -int Stream::prepareBuffers(unsigned int count)
>     +int Stream::prepareBuffers()
>      {
>     +       unsigned int slotCount;
>             int ret;
> 
>             if (!importOnly_) {
>     -               if (count) {
>     +               /*
>     +                * External streams overallocate buffer slots in order to
>     handle
>     +                * the buffers queued from applications without degrading
>     +                * performance. Internal streams only need to have enough
>     buffer
>     +                * slots to have the internal buffers queued.
>     +                */
>     +               slotCount = isExternal() ? kRPIExternalBufferSlotCount
>     +                                        : internalBufferCount_;
>     +
>     +               if (internalBufferCount_) {
>                             /* Export some frame buffers for internal use. */
>     -                       ret = dev_->exportBuffers(count, &
>     internalBuffers_);
>     +                       ret = dev_->exportBuffers(internalBufferCount_, &
>     internalBuffers_);
>                             if (ret < 0)
>                                     return ret;
> 
>     @@ -100,23 +110,16 @@ int Stream::prepareBuffers(unsigned int count)
>                             resetBuffers();
>                     }
> 
>     -               /* We must import all internal/external exported buffers. *
>     /
>     -               count = bufferMap_.size();
>     +       } else {
>     +               /*
>     +                * An importOnly stream doesn't have its own internal
>     buffers,
>     +                * so it needs to have the number of buffer slots to
>     reserve
>     +                * explicitly declared.
>     +                */
>     +               slotCount = bufferSlotCount_;
>             }
> 
>     -       /*
>     -        * 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 (isExternal() || importOnly_)
>     -               count = count * 2;
>     -
>     -       return dev_->importBuffers(count);
>     +       return dev_->importBuffers(slotCount);
>      }
> 
>      int Stream::queueBuffer(FrameBuffer *buffer)
>     diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/
>     libcamera/pipeline/raspberrypi/rpi_stream.h
>     index b8bd79cf..e63bf02b 100644
>     --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h
>     +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
>     @@ -42,9 +42,13 @@ public:
>             {
>             }
> 
>     -       Stream(const char *name, MediaEntity *dev, bool importOnly = false)
>     +       Stream(const char *name, MediaEntity *dev,
>     +              unsigned int internalBufferCount,
>     +              unsigned int bufferSlotCount = 0, bool importOnly = false)
>                     : external_(false), importOnly_(importOnly), name_(name),
>     -                 dev_(std::make_unique<V4L2VideoDevice>(dev)), id_
>     (BufferMask::MaskID)
>     +                 dev_(std::make_unique<V4L2VideoDevice>(dev)), id_
>     (BufferMask::MaskID),
>     +                 internalBufferCount_(internalBufferCount),
>     +                 bufferSlotCount_(bufferSlotCount)
>             {
>             }
> 
>     @@ -63,7 +67,7 @@ public:
>             void setExternalBuffer(FrameBuffer *buffer);
>             void removeExternalBuffer(FrameBuffer *buffer);
> 
>     -       int prepareBuffers(unsigned int count);
>     +       int prepareBuffers();
>             int queueBuffer(FrameBuffer *buffer);
>             void returnBuffer(FrameBuffer *buffer);
> 
>     @@ -71,6 +75,8 @@ public:
>             void releaseBuffers();
> 
>      private:
>     +       static constexpr unsigned int kRPIExternalBufferSlotCount = 16;
>     +
>             class IdGenerator
>             {
>             public:
>     @@ -133,6 +139,18 @@ private:
>             /* All frame buffers associated with this device stream. */
>             BufferMap bufferMap_;
> 
>     +       /*
>     +        * The number of buffers that should be allocated internally for
>     this
>     +        * stream.
>     +        */
>     +       unsigned int internalBufferCount_;
>     +
>     +       /*
>     +        * The number of buffer slots that should be reserved for this
>     stream.
>     +        * Only relevant for importOnly streams.
>     +        */
>     +       unsigned int bufferSlotCount_;
>     +
>             /*
>              * List of frame buffers that we can use if none have been provided
>     by
>              * the application for external streams. This is populated by the
>     --
>     2.35.1
> 
>

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
index 4641c76f..72502c36 100644
--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
@@ -341,6 +341,25 @@  public:
 	void releaseDevice(Camera *camera) override;
 
 private:
+	/*
+	 * The number of buffers to allocate internally for transferring raw
+	 * frames from the Unicam Image stream to the ISP Input stream. It is
+	 * defined such that:
+	 * - one buffer is being written to in Unicam Image
+	 * - one buffer is being processed in ISP Input
+	 * - one extra buffer is queued to Unicam Image
+	 */
+	static constexpr unsigned int kInternalRawBufferCount = 3;
+
+	/*
+	 * The number of buffers to allocate internally for the external
+	 * streams. They're used to drop the first few frames while the IPA
+	 * algorithms converge. It is defined such that:
+	 * - one buffer is being used on the stream
+	 * - one extra buffer is queued
+	 */
+	static constexpr unsigned int kInternalDropoutBufferCount = 2;
+
 	RPiCameraData *cameraData(Camera *camera)
 	{
 		return static_cast<RPiCameraData *>(camera->_d());
@@ -1221,21 +1240,23 @@  int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me
 		return -ENOENT;
 
 	/* Locate and open the unicam video streams. */
-	data->unicam_[Unicam::Image] = RPi::Stream("Unicam Image", unicamImage);
+	data->unicam_[Unicam::Image] = RPi::Stream("Unicam Image", unicamImage,
+						   kInternalRawBufferCount);
 
 	/* An embedded data node will not be present if the sensor does not support it. */
 	MediaEntity *unicamEmbedded = unicam->getEntityByName("unicam-embedded");
 	if (unicamEmbedded) {
-		data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam Embedded", unicamEmbedded);
+		data->unicam_[Unicam::Embedded] =
+			RPi::Stream("Unicam Embedded", unicamEmbedded, kInternalRawBufferCount);
 		data->unicam_[Unicam::Embedded].dev()->bufferReady.connect(data.get(),
 									   &RPiCameraData::unicamBufferDequeue);
 	}
 
 	/* Tag the ISP input stream as an import stream. */
-	data->isp_[Isp::Input] = RPi::Stream("ISP Input", ispOutput0, true);
-	data->isp_[Isp::Output0] = RPi::Stream("ISP Output0", ispCapture1);
-	data->isp_[Isp::Output1] = RPi::Stream("ISP Output1", ispCapture2);
-	data->isp_[Isp::Stats] = RPi::Stream("ISP Stats", ispCapture3);
+	data->isp_[Isp::Input] = RPi::Stream("ISP Input", ispOutput0, 0, kInternalRawBufferCount, true);
+	data->isp_[Isp::Output0] = RPi::Stream("ISP Output0", ispCapture1, kInternalDropoutBufferCount);
+	data->isp_[Isp::Output1] = RPi::Stream("ISP Output1", ispCapture2, kInternalDropoutBufferCount);
+	data->isp_[Isp::Stats] = RPi::Stream("ISP Stats", ispCapture3, kInternalDropoutBufferCount);
 
 	/* Wire up all the buffer connections. */
 	data->unicam_[Unicam::Image].dev()->dequeueTimeout.connect(data.get(), &RPiCameraData::unicamTimeout);
@@ -1428,57 +1449,11 @@  int PipelineHandlerRPi::queueAllBuffers(Camera *camera)
 int PipelineHandlerRPi::prepareBuffers(Camera *camera)
 {
 	RPiCameraData *data = cameraData(camera);
-	unsigned int numRawBuffers = 0;
 	int ret;
 
-	for (Stream *s : camera->streams()) {
-		if (isRaw(s->configuration().pixelFormat)) {
-			numRawBuffers = s->configuration().bufferCount;
-			break;
-		}
-	}
-
-	/* Decide how many internal buffers to allocate. */
+	/* Allocate internal buffers. */
 	for (auto const stream : data->streams_) {
-		unsigned int numBuffers;
-		/*
-		 * For Unicam, allocate a minimum of 4 buffers as we want
-		 * to avoid any frame drops.
-		 */
-		constexpr unsigned int minBuffers = 4;
-		if (stream == &data->unicam_[Unicam::Image]) {
-			/*
-			 * If an application has configured a RAW stream, allocate
-			 * additional buffers to make up the minimum, but ensure
-			 * we have at least 2 sets of internal buffers to use to
-			 * minimise frame drops.
-			 */
-			numBuffers = std::max<int>(2, minBuffers - numRawBuffers);
-		} else if (stream == &data->isp_[Isp::Input]) {
-			/*
-			 * ISP input buffers are imported from Unicam, so follow
-			 * similar logic as above to count all the RAW buffers
-			 * available.
-			 */
-			numBuffers = numRawBuffers + std::max<int>(2, minBuffers - numRawBuffers);
-
-		} else if (stream == &data->unicam_[Unicam::Embedded]) {
-			/*
-			 * Embedded data buffers are (currently) for internal use,
-			 * so allocate the minimum required to avoid frame drops.
-			 */
-			numBuffers = minBuffers;
-		} else {
-			/*
-			 * Since the ISP runs synchronous with the IPA and requests,
-			 * we only ever need one set of internal buffers. Any buffers
-			 * the application wants to hold onto will already be exported
-			 * through PipelineHandlerRPi::exportFrameBuffers().
-			 */
-			numBuffers = 1;
-		}
-
-		ret = stream->prepareBuffers(numBuffers);
+		ret = stream->prepareBuffers();
 		if (ret < 0)
 			return ret;
 	}
diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
index 2bb10f25..cde04efa 100644
--- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
+++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
@@ -84,14 +84,24 @@  void Stream::removeExternalBuffer(FrameBuffer *buffer)
 	bufferMap_.erase(id);
 }
 
-int Stream::prepareBuffers(unsigned int count)
+int Stream::prepareBuffers()
 {
+	unsigned int slotCount;
 	int ret;
 
 	if (!importOnly_) {
-		if (count) {
+		/*
+		 * External streams overallocate buffer slots in order to handle
+		 * the buffers queued from applications without degrading
+		 * performance. Internal streams only need to have enough buffer
+		 * slots to have the internal buffers queued.
+		 */
+		slotCount = isExternal() ? kRPIExternalBufferSlotCount
+					 : internalBufferCount_;
+
+		if (internalBufferCount_) {
 			/* Export some frame buffers for internal use. */
-			ret = dev_->exportBuffers(count, &internalBuffers_);
+			ret = dev_->exportBuffers(internalBufferCount_, &internalBuffers_);
 			if (ret < 0)
 				return ret;
 
@@ -100,23 +110,16 @@  int Stream::prepareBuffers(unsigned int count)
 			resetBuffers();
 		}
 
-		/* We must import all internal/external exported buffers. */
-		count = bufferMap_.size();
+	} else {
+		/*
+		 * An importOnly stream doesn't have its own internal buffers,
+		 * so it needs to have the number of buffer slots to reserve
+		 * explicitly declared.
+		 */
+		slotCount = bufferSlotCount_;
 	}
 
-	/*
-	 * 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 (isExternal() || importOnly_)
-		count = count * 2;
-
-	return dev_->importBuffers(count);
+	return dev_->importBuffers(slotCount);
 }
 
 int Stream::queueBuffer(FrameBuffer *buffer)
diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
index b8bd79cf..e63bf02b 100644
--- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h
+++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
@@ -42,9 +42,13 @@  public:
 	{
 	}
 
-	Stream(const char *name, MediaEntity *dev, bool importOnly = false)
+	Stream(const char *name, MediaEntity *dev,
+	       unsigned int internalBufferCount,
+	       unsigned int bufferSlotCount = 0, bool importOnly = false)
 		: external_(false), importOnly_(importOnly), name_(name),
-		  dev_(std::make_unique<V4L2VideoDevice>(dev)), id_(BufferMask::MaskID)
+		  dev_(std::make_unique<V4L2VideoDevice>(dev)), id_(BufferMask::MaskID),
+		  internalBufferCount_(internalBufferCount),
+		  bufferSlotCount_(bufferSlotCount)
 	{
 	}
 
@@ -63,7 +67,7 @@  public:
 	void setExternalBuffer(FrameBuffer *buffer);
 	void removeExternalBuffer(FrameBuffer *buffer);
 
-	int prepareBuffers(unsigned int count);
+	int prepareBuffers();
 	int queueBuffer(FrameBuffer *buffer);
 	void returnBuffer(FrameBuffer *buffer);
 
@@ -71,6 +75,8 @@  public:
 	void releaseBuffers();
 
 private:
+	static constexpr unsigned int kRPIExternalBufferSlotCount = 16;
+
 	class IdGenerator
 	{
 	public:
@@ -133,6 +139,18 @@  private:
 	/* All frame buffers associated with this device stream. */
 	BufferMap bufferMap_;
 
+	/*
+	 * The number of buffers that should be allocated internally for this
+	 * stream.
+	 */
+	unsigned int internalBufferCount_;
+
+	/*
+	 * The number of buffer slots that should be reserved for this stream.
+	 * Only relevant for importOnly streams.
+	 */
+	unsigned int bufferSlotCount_;
+
 	/*
 	 * List of frame buffers that we can use if none have been provided by
 	 * the application for external streams. This is populated by the