[libcamera-devel] [PATCH v9 05/18] libcamera: pipeline: ipu3: Don't rely on bufferCount
Paul Elder
paul.elder at ideasonboard.com
Fri Dec 23 23:28:11 CET 2022
Hi Umang,
On Wed, Dec 21, 2022 at 04:09:21PM +0530, Umang Jain wrote:
> Hi Paul,
>
> On 12/16/22 5:59 PM, 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,
>
> This is now sounding like the number of internal buffers should be allocated
> based on MinimumRequests property we just defined? Can it be then pivoted to
> Minimum requests property ? I don't see possibility where internal buffers
> needed to be allocated comes out to be less than the camera's
> MinimumRequests to avoid frame drops.
Indeed I don't think there would be a situation where there would be
less internal buffers than MinimumRequests, but I think (also as is seen
throughout this series) it's possible that the number of internal
buffers could be greater than MinimumRequests.
I suppose it would be less duplication if the number of internal buffers
was defined based on MinimumRequests (eg. kIPU3BufferSlotCount =
MinimumRequests + 1). Well, not the property directly but the quantity
that is fed to the property.
>
> > while the number of V4L2 buffer slots should be greater than the
> > expected number of requests queued by the application, in order to avoid
>
> s/expected number of requests queued by the application/Minimum Requests
> needed to be queued by the application/
>
> I think in this case we want a mutiplier to MinimumRequests so allocate
> larger V4L2 slots, For e.g. in context of this patch,
>
> kIPU3BufferSlotCount = 4 * minimumRequests;
>
> My goal is that, instead of defining arbitrary constants for both IMGU and
> CIO2 internal buffers , we better pivot the buffers counts to
> MinimumRequests so it would be more cleaner and logical ? What do you
> think?
I don't think the IMGU and CIO2 internal buffers are defined off of
arbitrary constants; they're intentional constants with (documented)
rationales. But indeed I think there is benefit in defining them based
on MinimumRequests, such that they end up with the desired quantity.
> > thrashing dmabuf mappings, which would degrade performance.
>
> Have you checked any performance metrics with increasing internal counts?
I haven't yet. I don't have an IPU3 platform that I can test on anymore.
I'll test it on imx8mp at least.
> The comments (and this series) overwhelming says "without frame drops"
> throughout - but I am not sure if that's the case in reality. But the
> direction is more or less correct in my opinion.
>
> (The feedback applies to other platforms as well, regarding pivoting
> constants the MinimumRequests property, so I rather not comment on them
> individually)
Thanks,
Paul
> >
> > 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_);
> > 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;
>
More information about the libcamera-devel
mailing list