| Message ID | 20190326083902.26121-14-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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 */
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) + 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); + + 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); +} + +/** + * \brief Release buffers of the ImgU 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 + * + * Allocate memory buffers in the CIO2 video device and export them to + * a buffer pool that will be later imported by the ImgU device. + * + * \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 */
Implement buffer allocation and relase in IPU3 pipeline handlers. 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. 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@jmondi.org> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 174 ++++++++++++++++++++++++--- 1 file changed, 160 insertions(+), 14 deletions(-)