[libcamera-devel] [PATCH v4 21/31] RFC: libcamera: pipeline_handlers: Add preAllocateBuffers
Jacopo Mondi
jacopo at jmondi.org
Sat Mar 23 15:00:09 CET 2019
Hi Laurent,
On Sat, Mar 23, 2019 at 03:57:45PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Mar 20, 2019 at 05:30:45PM +0100, Jacopo Mondi wrote:
> > Add a preAllocateBuffers virtual method to the PipelineHandler base
> > class and call it before performing per-stream memory allocation.
> >
> > Implement the method in the IPU3 pipeline handler to perform memory
> > allocation on the CIO2 unit and the ImgU input and stat video devices.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> > src/libcamera/camera.cpp | 10 ++++-
> > src/libcamera/include/pipeline_handler.h | 5 +++
> > src/libcamera/pipeline/ipu3/ipu3.cpp | 54 ++++++++++++++----------
> > src/libcamera/pipeline_handler.cpp | 21 ++++++++-
> > 4 files changed, 66 insertions(+), 24 deletions(-)
> >
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index 8ee9cc086616..8020dff8f8ea 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -455,6 +455,8 @@ int Camera::configureStreams(std::map<Stream *, StreamConfiguration> &config)
> > */
> > int Camera::allocateBuffers()
> > {
> > + int ret;
> > +
> > if (disconnected_)
> > return -ENODEV;
> >
> > @@ -467,8 +469,14 @@ int Camera::allocateBuffers()
> > return -EINVAL;
> > }
> >
> > + ret = pipe_->preAllocateBuffers(this, activeStreams_);
> > + if (ret) {
> > + LOG(Camera, Error) << "Buffers pre-allocation failed";
> > + return ret;
> > + }
> > +
>
> I'm not a big fan of pre/post hooks. I think this should the
> PipelineHandler::allocateBuffers() and PipelineHandler::freeBuffers()
> methods are not correctly designed. It may be better to give them the
> list of active streams, moving the loop from Camera to the pipeline
> handlers. That way pipeline handlers could perform any extra step they
> require, and even handle dependencies between allocations for different
> streams if the need arises.
>
I agree, I'll do this in v5.
Thanks
j
> > for (Stream *stream : activeStreams_) {
> > - int ret = pipe_->allocateBuffers(this, stream);
> > + ret = pipe_->allocateBuffers(this, stream);
> > if (ret) {
> > LOG(Camera, Error) << "Failed to allocate buffers";
> > freeBuffers();
> > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> > index acb376e07030..7ce5b67cc7fc 100644
> > --- a/src/libcamera/include/pipeline_handler.h
> > +++ b/src/libcamera/include/pipeline_handler.h
> > @@ -57,6 +57,11 @@ public:
> > virtual int configureStreams(Camera *camera,
> > std::map<Stream *, StreamConfiguration> &config) = 0;
> >
> > + virtual int preAllocateBuffers(Camera *camera,
> > + const std::set<Stream *> &activeStreams)
> > + {
> > + return 0;
> > + }
> > virtual int allocateBuffers(Camera *camera, Stream *stream) = 0;
> > virtual int freeBuffers(Camera *camera, Stream *stream) = 0;
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 0507fc380e68..5f323888d84f 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -105,6 +105,8 @@ public:
> > int configureStreams(Camera *camera,
> > std::map<Stream *, StreamConfiguration> &config) override;
> >
> > + int preAllocateBuffers(Camera *camera,
> > + const std::set<Stream *> &activeStreams) override;
> > int allocateBuffers(Camera *camera, Stream *stream) override;
> > int freeBuffers(Camera *camera, Stream *stream) override;
> >
> > @@ -451,40 +453,48 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,
> > return 0;
> > }
> >
> > -int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream)
> > +int PipelineHandlerIPU3::preAllocateBuffers(Camera *camera,
> > + const std::set<Stream *> &activeStreams)
> > {
> > IPU3CameraData *data = cameraData(camera);
> > - V4L2Device *viewfinder = data->imgu->viewfinder;
> > - V4L2Device *output = data->imgu->output;
> > V4L2Device *input = data->imgu->input;
> > V4L2Device *cio2 = data->cio2.output;
> > V4L2Device *stat = data->imgu->stat;
> > ImgUDevice *imgu = data->imgu;
> > int ret;
> >
> > - if (data->cio2.pool.count() == 0) {
> > - /* Share buffers between CIO2 output and ImgU input. */
> > - data->cio2.pool.createBuffers(IPU3_CIO2_BUFFER_COUNT);
> > - ret = cio2->exportBuffers(&data->cio2.pool);
> > - if (ret) {
> > - LOG(IPU3, Error) << "Failed to reserve CIO2 memory";
> > - return ret;
> > - }
> > + /* Share buffers between CIO2 output and ImgU input. */
> > + data->cio2.pool.createBuffers(IPU3_CIO2_BUFFER_COUNT);
> > + ret = cio2->exportBuffers(&data->cio2.pool);
> > + if (ret) {
> > + LOG(IPU3, Error) << "Failed to reserve CIO2 memory";
> > + return ret;
> > + }
> >
> > - ret = input->importBuffers(&data->cio2.pool);
> > - if (ret) {
> > - LOG(IPU3, Error) << "Failed to import ImgU memory";
> > - return ret;
> > - }
> > + ret = input->importBuffers(&data->cio2.pool);
> > + if (ret) {
> > + LOG(IPU3, Error) << "Failed to import ImgU memory";
> > + return ret;
> > + }
> >
> > - imgu->statPool.createBuffers(IPU3_IMGU_BUFFER_COUNT);
> > - ret = stat->exportBuffers(&imgu->statPool);
> > - if (ret) {
> > - LOG(IPU3, Error) << "Failed to reserve ImgU stat memory";
> > - return ret;
> > - }
> > + imgu->statPool.createBuffers(IPU3_IMGU_BUFFER_COUNT);
> > + ret = stat->exportBuffers(&imgu->statPool);
> > + if (ret) {
> > + LOG(IPU3, Error) << "Failed to reserve ImgU stat memory";
> > + return ret;
> > }
> >
> > + return 0;
> > +}
> > +
> > +int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream)
> > +{
> > + IPU3CameraData *data = cameraData(camera);
> > + V4L2Device *viewfinder = data->imgu->viewfinder;
> > + V4L2Device *output = data->imgu->output;
> > + ImgUDevice *imgu = data->imgu;
> > + int ret;
> > +
> > if (isOutput(data, stream)) {
> > /* Export ImgU output buffers to the stream's pool. */
> > ret = output->exportBuffers(&stream->bufferPool());
> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > index 1a858f2638ce..1f556ed789b6 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -189,6 +189,24 @@ PipelineHandler::~PipelineHandler()
> > * \return 0 on success or a negative error code otherwise
> > */
> >
> > +/**
> > + * \fn PipelineHandler::preAllocateBuffers()
> > + * \brief Perform operations to prepare for memory allocation. Optional for
> > + * pipeline handlers to implement
> > + * \param[in] camera The camera the streams belongs to
> > + * \param[in] activeStreams The set of active streams
> > + *
> > + * If operations to prepare for the per-stream memory allocation are required,
> > + * this virtual method provides an entry point for pipeline handlers to do so.
> > + *
> > + * This method is optional to implement for pipeline handlers, and its intended
> > + * caller is the Camera class, which calls it once before performing per-stream
> > + * memory allocation by calling the PipelineHandler::allocateBuffers() method
> > + * once per each active stream.
> > + *
> > + * \return 0 on success or a negative error code otherwise
> > + */
> > +
> > /**
> > * \fn PipelineHandler::allocateBuffers()
> > * \brief Allocate buffers for a stream
> > @@ -198,7 +216,8 @@ PipelineHandler::~PipelineHandler()
> > * This method allocates buffers internally in the pipeline handler and
> > * associates them with the stream's buffer pool.
> > *
> > - * The intended caller of this method is the Camera class.
> > + * The intended caller of this method is the Camera class which calls it
> > + * once per each active stream.
> > *
> > * \return 0 on success or a negative error code otherwise
> > */
>
> --
> Regards,
>
> Laurent Pinchart
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190323/c36bfbf1/attachment.sig>
More information about the libcamera-devel
mailing list