[libcamera-devel] [PATCH v9 05/18] libcamera: pipeline: ipu3: Don't rely on bufferCount
Umang Jain
umang.jain at ideasonboard.com
Tue Dec 20 14:24:18 CET 2022
Hi Jacopo,
On 12/16/22 8:22 PM, Jacopo Mondi via libcamera-devel wrote:
> Hi Paul
>
> On Fri, Dec 16, 2022 at 09:29:26PM +0900, Paul Elder via libcamera-devel wrote:
>> From: Nícolas F. R. A. Prado <nfraprado at collabora.com>
>>
>> Currently the ipu3 pipeline handler relies on bufferCount to decide on
>> the number of V4L2 buffer slots to reserve as well as for the number of
>> buffers to allocate internally for the CIO2 raw buffers and
>> parameter-statistics ImgU buffer pairs. Instead, the number of internal
>> buffers should be the minimum required by the pipeline to keep the
>> requests flowing without frame drops, in order to avoid wasting memory,
>> while the number of V4L2 buffer slots should be greater than the
>> expected number of requests queued by the application, in order to avoid
>> thrashing dmabuf mappings, which would degrade performance.
>>
>> Stop relying on bufferCount for these numbers and instead set them to
>> appropriate, and independent, constants.
>>
>> Signed-off-by: Nícolas F. R. A. Prado <nfraprado at collabora.com>
>> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
>> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
>>
>> ---
>> Changes in v9:
>> - rebase
>>
>> Changes in v8:
>> - New
>> ---
>> src/libcamera/pipeline/ipu3/cio2.cpp | 4 ++--
>> src/libcamera/pipeline/ipu3/cio2.h | 16 +++++++++++++++-
>> src/libcamera/pipeline/ipu3/imgu.cpp | 12 ++++++------
>> src/libcamera/pipeline/ipu3/imgu.h | 15 ++++++++++++++-
>> src/libcamera/pipeline/ipu3/ipu3.cpp | 20 ++++++++------------
>> 5 files changed, 45 insertions(+), 22 deletions(-)
>>
>> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
>> index d4e523af..feb69991 100644
>> --- a/src/libcamera/pipeline/ipu3/cio2.cpp
>> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
>> @@ -335,13 +335,13 @@ int CIO2Device::exportBuffers(unsigned int count,
>> return output_->exportBuffers(count, buffers);
>> }
>>
>> -int CIO2Device::start()
>> +int CIO2Device::start(unsigned int bufferSlotCount)
>> {
>> int ret = output_->exportBuffers(kBufferCount, &buffers_);
>> if (ret < 0)
>> return ret;
>>
>> - ret = output_->importBuffers(kBufferCount);
>> + ret = output_->importBuffers(bufferSlotCount);
>> if (ret)
>> LOG(IPU3, Error) << "Failed to import CIO2 buffers";
>>
>> diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h
>> index 68504a2d..8ed208d3 100644
>> --- a/src/libcamera/pipeline/ipu3/cio2.h
>> +++ b/src/libcamera/pipeline/ipu3/cio2.h
>> @@ -30,6 +30,20 @@ struct StreamConfiguration;
>> class CIO2Device
>> {
>> public:
>> + /*
>> + * This many internal buffers for the CIO2 ensures that the pipeline
>> + * runs smoothly, without frame drops. This number considers:
>> + * - one buffer being DMA'ed to in CIO2
>> + * - one buffer programmed by the CIO2 as the next buffer
>> + * - one buffer under processing in ImgU
>> + * - one extra idle buffer queued to CIO2, to account for possible
>> + * delays in requeuing the buffer from ImgU back to CIO2
>> + *
>> + * Transient situations can arise when one of the parts, CIO2 or ImgU,
>> + * finishes its processing first and experiences a lack of buffers, but
>> + * they will shortly after return to the state described above as the
>> + * other part catches up.
>> + */
>> static constexpr unsigned int kBufferCount = 4;
>>
>> CIO2Device();
>> @@ -48,7 +62,7 @@ public:
>> V4L2SubdeviceFormat getSensorFormat(const std::vector<unsigned int> &mbusCodes,
>> const Size &size) const;
>>
>> - int start();
>> + int start(unsigned int bufferSlotCount);
>> int stop();
>>
>> CameraSensor *sensor() { return sensor_.get(); }
>> diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp
>> index 531879f1..fa920d87 100644
>> --- a/src/libcamera/pipeline/ipu3/imgu.cpp
>> +++ b/src/libcamera/pipeline/ipu3/imgu.cpp
>> @@ -576,22 +576,22 @@ int ImgUDevice::configureVideoDevice(V4L2VideoDevice *dev, unsigned int pad,
>> /**
>> * \brief Allocate buffers for all the ImgU video devices
>> */
>> -int ImgUDevice::allocateBuffers(unsigned int bufferCount)
>> +int ImgUDevice::allocateBuffers(unsigned int bufferSlotCount)
>> {
>> /* Share buffers between CIO2 output and ImgU input. */
>> - int ret = input_->importBuffers(bufferCount);
>> + int ret = input_->importBuffers(bufferSlotCount);
>> if (ret) {
>> LOG(IPU3, Error) << "Failed to import ImgU input buffers";
>> return ret;
>> }
>>
>> - ret = param_->allocateBuffers(bufferCount, ¶mBuffers_);
>> + ret = param_->allocateBuffers(kImgUInternalBufferCount, ¶mBuffers_);
> shouldn't stats and parameters use the same number of buffers
> allocated in the input and output devices ? If you run short of stats
> or param you will stall the pipeline, won't you ?
I don't think so - I remember using only 1 parameter buffer while
buffers > 1 for input and output nodes for standalone IMGU streaming
https://patchwork.libcamera.org/patch/16988/
>
>> if (ret < 0) {
>> LOG(IPU3, Error) << "Failed to allocate ImgU param buffers";
>> goto error;
>> }
>>
>> - ret = stat_->allocateBuffers(bufferCount, &statBuffers_);
>> + ret = stat_->allocateBuffers(kImgUInternalBufferCount, &statBuffers_);
>> if (ret < 0) {
>> LOG(IPU3, Error) << "Failed to allocate ImgU stat buffers";
>> goto error;
>> @@ -602,13 +602,13 @@ int ImgUDevice::allocateBuffers(unsigned int bufferCount)
>> * corresponding stream is active or inactive, as the driver needs
>> * buffers to be requested on the V4L2 devices in order to operate.
>> */
>> - ret = output_->importBuffers(bufferCount);
>> + ret = output_->importBuffers(bufferSlotCount);
>> if (ret < 0) {
>> LOG(IPU3, Error) << "Failed to import ImgU output buffers";
>> goto error;
>> }
>>
>> - ret = viewfinder_->importBuffers(bufferCount);
>> + ret = viewfinder_->importBuffers(bufferSlotCount);
>> if (ret < 0) {
>> LOG(IPU3, Error) << "Failed to import ImgU viewfinder buffers";
>> goto error;
>> diff --git a/src/libcamera/pipeline/ipu3/imgu.h b/src/libcamera/pipeline/ipu3/imgu.h
>> index 0af4dd8a..85873961 100644
>> --- a/src/libcamera/pipeline/ipu3/imgu.h
>> +++ b/src/libcamera/pipeline/ipu3/imgu.h
>> @@ -84,7 +84,7 @@ public:
>> outputFormat);
>> }
>>
>> - int allocateBuffers(unsigned int bufferCount);
>> + int allocateBuffers(unsigned int bufferSlotCount);
>> void freeBuffers();
>>
>> int start();
>> @@ -119,6 +119,19 @@ private:
>>
>> std::string name_;
>> MediaDevice *media_;
>> +
>> + /*
>> + * This many internal buffers (or rather parameter and statistics buffer
>> + * pairs) for the ImgU ensures that the pipeline runs smoothly, without
>> + * frame drops. This number considers:
>> + * - three buffers queued to the CIO2 (Since these buffers are bound to
>> + * CIO2 buffers before queuing to the CIO2)
>> + * - one buffer under processing in ImgU
>> + *
>> + * \todo Update this number when we make these buffers only get added to
>> + * the FrameInfo after the raw buffers are dequeued from CIO2.
>> + */
>> + static constexpr unsigned int kImgUInternalBufferCount = 4;
>> };
>>
>> } /* namespace libcamera */
>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> index bab2db65..4d8fcfeb 100644
>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> @@ -160,7 +160,7 @@ private:
>> int updateControls(IPU3CameraData *data);
>> int registerCameras();
>>
>> - int allocateBuffers(Camera *camera);
>> + int allocateBuffers(Camera *camera, unsigned int bufferSlotCount);
>> int freeBuffers(Camera *camera);
>>
>> ImgUDevice imgu0_;
>> @@ -169,6 +169,8 @@ private:
>> MediaDevice *imguMediaDev_;
>>
>> std::vector<IPABuffer> ipaBuffers_;
>> +
>> + static constexpr unsigned int kIPU3BufferSlotCount = 16;
>> };
>>
>> IPU3CameraConfiguration::IPU3CameraConfiguration(IPU3CameraData *data)
>> @@ -710,20 +712,14 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,
>> * In order to be able to start the 'viewfinder' and 'stat' nodes, we need
>> * memory to be reserved.
>> */
>> -int PipelineHandlerIPU3::allocateBuffers(Camera *camera)
>> +int PipelineHandlerIPU3::allocateBuffers(Camera *camera,
>> + unsigned int bufferSlotCount)
>> {
>> IPU3CameraData *data = cameraData(camera);
>> ImgUDevice *imgu = data->imgu_;
>> - unsigned int bufferCount;
>> int ret;
>>
>> - bufferCount = std::max({
>> - data->outStream_.configuration().bufferCount,
>> - data->vfStream_.configuration().bufferCount,
>> - data->rawStream_.configuration().bufferCount,
>> - });
>> -
>> - ret = imgu->allocateBuffers(bufferCount);
>> + ret = imgu->allocateBuffers(bufferSlotCount);
>> if (ret < 0)
>> return ret;
>>
>> @@ -781,7 +777,7 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis
>> return ret;
>>
>> /* Allocate buffers for internal pipeline usage. */
>> - ret = allocateBuffers(camera);
>> + ret = allocateBuffers(camera, kIPU3BufferSlotCount);
>> if (ret)
>> return ret;
>>
>> @@ -795,7 +791,7 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis
>> * Start the ImgU video devices, buffers will be queued to the
>> * ImgU output and viewfinder when requests will be queued.
>> */
>> - ret = cio2->start();
>> + ret = cio2->start(kIPU3BufferSlotCount);
>> if (ret)
>> goto error;
>>
>> --
>> 2.35.1
>>
More information about the libcamera-devel
mailing list