[libcamera-devel] [PATCH v5 13/19] libcamera: ipu3: Implement memory handling
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Apr 2 12:24:38 CEST 2019
Hi Jacopo,
Thank you for the patch.
On Tue, Mar 26, 2019 at 09:38:56AM +0100, Jacopo Mondi wrote:
> Implement buffer allocation and relase in IPU3 pipeline handlers.
s/relase/release/
> As the pipeline handler supports a single stream, provide two internal
> buffer pools for 'viewfinder' and 'stat' video devices, and export
> the 'output' video device buffers to the Stream's pool.
I would add "This works around the fact that the ImgU requires buffers
to be queued on all its outputs, even when they are not in use."
> Share buffers between the CIO2 output and the ImgU input video devices,
> as the output of the former should immediately be provided to the
> latter for further processing.
>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> src/libcamera/pipeline/ipu3/ipu3.cpp | 174 ++++++++++++++++++++++++---
> 1 file changed, 160 insertions(+), 14 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 63b84706b9b2..d3519bb1d536 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -64,6 +64,7 @@ public:
> V4L2Device *dev;
> unsigned int pad;
> std::string name;
> + BufferPool *pool;
> };
>
> ImgUDevice()
> @@ -94,6 +95,10 @@ public:
> {
> return outputMap[id].name;
> }
> + BufferPool *outputPool(enum OutputId id)
> + {
> + return outputMap[id].pool;
> + }
>
> int init(MediaDevice *media, unsigned int index);
> int configureInput(const StreamConfiguration &config,
> @@ -101,6 +106,11 @@ public:
> int configureOutput(enum OutputId id,
> const StreamConfiguration &config);
>
> + int importBuffers(BufferPool *pool);
> + int exportBuffers(enum OutputId id, BufferPool *pool);
> + int exportBuffers(enum OutputId id, unsigned int count);
> + void freeBuffers();
> +
> unsigned int index_;
> std::string name_;
> MediaDevice *media_;
> @@ -114,11 +124,16 @@ public:
>
> /* ImgU output map: associate an output id with its descriptor. */
> std::map<enum OutputId, struct outputDesc> outputMap;
> +
> + BufferPool vfPool;
> + BufferPool statPool;
> };
>
> class CIO2Device
> {
> public:
> + static constexpr unsigned int CIO2_BUFFER_COUNT = 4;
> +
> CIO2Device()
> : output_(nullptr), csi2_(nullptr), sensor_(nullptr)
> {
> @@ -136,12 +151,17 @@ public:
> int configure(const StreamConfiguration &config,
> V4L2SubdeviceFormat *format);
>
> + BufferPool *exportBuffers();
> + void freeBuffers();
> +
> V4L2Device *output_;
> V4L2Subdevice *csi2_;
> V4L2Subdevice *sensor_;
>
> /* Maximum sizes and the mbus code used to produce them. */
> std::pair<unsigned int, SizeRange> maxSizes_;
> +
> + BufferPool pool_;
> };
>
> class PipelineHandlerIPU3 : public PipelineHandler
> @@ -301,18 +321,39 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,
>
> int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream)
> {
> - const StreamConfiguration &cfg = stream->configuration();
> IPU3CameraData *data = cameraData(camera);
> - V4L2Device *cio2 = data->cio2_.output_;
> + CIO2Device *cio2 = &data->cio2_;
> + ImgUDevice *imgu = data->imgu_;
> + int ret;
>
> - if (!cfg.bufferCount)
> - return -EINVAL;
> + /* Share buffers between CIO2 output and ImgU input. */
> + BufferPool *pool = cio2->exportBuffers();
> + if (!pool)
> + return -ENOMEM;
>
> - int ret = cio2->exportBuffers(&stream->bufferPool());
> - if (ret) {
> - LOG(IPU3, Error) << "Failed to request memory";
> + ret = imgu->importBuffers(pool);
> + if (ret)
Please print error an message in ImgUDevice::importBuffers or this error
path will be silent.
> + return ret;
> +
> + /* Export ImgU output buffers to the stream's pool. */
> + ret = imgu->exportBuffers(ImgUDevice::MAIN_OUTPUT,
> + &stream->bufferPool());
> + if (ret)
> + return ret;
> +
> + /*
> + * Reserve memory in viewfinder and stat output devices. Use the
> + * same number of buffers as the ones requested for the output
> + * stream.
> + */
> + unsigned int bufferCount = stream->bufferPool().count();
> + ret = imgu->exportBuffers(ImgUDevice::SECONDARY_OUTPUT, bufferCount);
> + if (ret)
> + return ret;
> +
> + ret = imgu->exportBuffers(ImgUDevice::STAT, bufferCount);
> + if (ret)
> return ret;
> - }
>
> return 0;
> }
> @@ -320,13 +361,9 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream)
> int PipelineHandlerIPU3::freeBuffers(Camera *camera, Stream *stream)
> {
> IPU3CameraData *data = cameraData(camera);
> - V4L2Device *cio2 = data->cio2_.output_;
>
> - int ret = cio2->releaseBuffers();
> - if (ret) {
> - LOG(IPU3, Error) << "Failed to release memory";
> - return ret;
> - }
> + data->cio2_.freeBuffers();
> + data->imgu_->freeBuffers();
>
> return 0;
> }
> @@ -592,6 +629,7 @@ int ImgUDevice::init(MediaDevice *media, unsigned int index)
> desc.dev = viewfinder_;
> desc.pad = PAD_VF;
> desc.name = "viewfinder";
> + desc.pool = &vfPool;
> outputMap[SECONDARY_OUTPUT] = desc;
>
> stat_ = V4L2Device::fromEntityName(media, name_ + " 3a stat");
> @@ -603,6 +641,7 @@ int ImgUDevice::init(MediaDevice *media, unsigned int index)
> desc.dev = stat_;
> desc.pad = PAD_STAT;
> desc.name = "stat";
> + desc.pool = &statPool;
> outputMap[STAT] = desc;
>
> return 0;
> @@ -714,6 +753,86 @@ int ImgUDevice::configureOutput(enum OutputId id,
> return 0;
> }
>
> +/**
> + * \brief Import buffers from CIO2 device into the ImgU
> + * \param[in] pool The buffer pool to import
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +int ImgUDevice::importBuffers(BufferPool *pool)
> +{
> + return input_->importBuffers(pool);
> +}
> +
> +/**
> + * \brief Export buffers from output \a id to the provided \a pool
> + * \param[in] id The ImgU output identifier
> + * \param[in] pool The buffer pool where to export buffers
> + *
> + * Export memory buffers reserved in the video device memory associated with
> + * output \a id to the buffer pool provided as argument.
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +int ImgUDevice::exportBuffers(enum OutputId id, BufferPool *pool)
> +{
> + V4L2Device *output = outputDevice(id);
> + const std::string &name = outputName(id);
The second variable could be inlined, the first probably as well.
> +
> + int ret = output->exportBuffers(pool);
> + if (ret) {
> + LOG(IPU3, Error) << "Failed to reserve ImgU "
> + << name << " buffers";
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * \brief Export buffers from output \a id to the an internal buffer pool
> + * \param[in] id The ImgU output identifier
> + * \param[in] count The number of buffers to reserve
> + *
> + * Export memory buffers reserved in the video device memory associated with
> + * output \a id to the output internal buffer pool.
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +int ImgUDevice::exportBuffers(enum OutputId id, unsigned int count)
> +{
> + BufferPool *pool = outputPool(id);
> +
> + /* Internal buffer pools needs memory to be allocated first. */
> + pool->createBuffers(count);
> +
> + return exportBuffers(id, pool);
> +}
I don't like this function too much. I think the could would end up
being easier to follow if you called createBuffers() in the caller, and
always used the first variant of exportBuffers().
> +
> +/**
> + * \brief Release buffers of the ImgU devices
"Release buffers for all the ImgU video devices" ?
> + */
> +void ImgUDevice::freeBuffers()
> +{
> + int ret;
> +
> + ret = output_->releaseBuffers();
> + if (ret)
> + LOG(IPU3, Error) << "Failed to release ImgU output buffers";
> +
> + ret = stat_->releaseBuffers();
> + if (ret)
> + LOG(IPU3, Error) << "Failed to release ImgU stat buffers";
> +
> + ret = viewfinder_->releaseBuffers();
> + if (ret)
> + LOG(IPU3, Error) << "Failed to release ImgU viewfinder buffers";
> +
> + ret = input_->releaseBuffers();
> + if (ret)
> + LOG(IPU3, Error) << "Failed to release ImgU input buffers";
> +}
> +
> /*------------------------------------------------------------------------------
> * CIO2 Device
> */
> @@ -880,6 +999,33 @@ int CIO2Device::configure(const StreamConfiguration &config,
> return 0;
> }
>
> +/**
> + * \brief Reserve CIO2 memory buffers and export them in a BufferPool
Maybe s/Reserve/Allocate/ ?
> + *
> + * Allocate memory buffers in the CIO2 video device and export them to
> + * a buffer pool that will be later imported by the ImgU device.
"that can then be imported by another device" to keep the CIO2Device and
ImgUDevice self-contained ?
> + *
> + * \return The buffer pool with export buffers on success or nullptr otherwise
> + */
> +BufferPool *CIO2Device::exportBuffers()
> +{
> + pool_.createBuffers(CIO2_BUFFER_COUNT);
> +
> + int ret = output_->exportBuffers(&pool_);
> + if (ret) {
> + LOG(IPU3, Error) << "Failed to export CIO2 buffers";
> + return nullptr;
> + }
> +
> + return &pool_;
> +}
> +
> +void CIO2Device::freeBuffers()
> +{
> + if (output_->releaseBuffers())
> + LOG(IPU3, Error) << "Failed to release CIO2 buffers";
> +}
> +
> REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3);
>
> } /* namespace libcamera */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list