[libcamera-devel] [PATCH 29/30] libcamera: pipeline: Remove explicit buffer handling
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Dec 15 15:01:03 CET 2019
Hi Niklas,
One additional comment.
On Sun, Dec 15, 2019 at 11:28:42AM +0200, Laurent Pinchart wrote:
> On Wed, Nov 27, 2019 at 12:36:19AM +0100, Niklas Söderlund wrote:
> > With FrameBuffer interface in place there is no need for the Camera to
>
> s/FrameBuffer/the FrameBuffer/
>
> > call into the specific pipelines allocation and freeing of buffers as it
> > no longer needs to be synced with buffer allocation by the application.
>
> s/synced/synchronized/
>
> > Remove the function prototypes in the pipeline handler base class and
> > fold the functionality in the pipelines start() and stop() functions
> > where needed. A follow up patch will remove the now no-op
> > Camera::allocateBuffers() and Camera::freeBuffers().
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> > src/libcamera/camera.cpp | 8 +------
> > src/libcamera/include/pipeline_handler.h | 5 ----
> > src/libcamera/pipeline/ipu3/ipu3.cpp | 21 +++++++++--------
> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 21 ++++++++++-------
> > src/libcamera/pipeline/uvcvideo.cpp | 17 --------------
> > src/libcamera/pipeline/vimc.cpp | 17 --------------
> > src/libcamera/pipeline_handler.cpp | 29 ------------------------
> > 7 files changed, 26 insertions(+), 92 deletions(-)
> >
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index 05fdcaab8f918afa..12878a1c2e6076d3 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -709,12 +709,6 @@ int Camera::allocateBuffers()
> > return -EINVAL;
> > }
> >
> > - int ret = pipe_->allocateBuffers(this, activeStreams_);
> > - if (ret) {
> > - LOG(Camera, Error) << "Failed to allocate buffers";
> > - return ret;
> > - }
> > -
> > state_ = CameraPrepared;
> >
> > return 0;
> > @@ -735,7 +729,7 @@ int Camera::freeBuffers()
> >
> > state_ = CameraConfigured;
> >
> > - return pipe_->freeBuffers(this, activeStreams_);
> > + return 0;
> > }
> >
> > /**
> > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> > index e4588f10bac0228c..9e5dca894aff45ae 100644
> > --- a/src/libcamera/include/pipeline_handler.h
> > +++ b/src/libcamera/include/pipeline_handler.h
> > @@ -69,11 +69,6 @@ public:
> > const StreamRoles &roles) = 0;
> > virtual int configure(Camera *camera, CameraConfiguration *config) = 0;
> >
> > - virtual int allocateBuffers(Camera *camera,
> > - const std::set<Stream *> &streams) = 0;
> > - virtual int freeBuffers(Camera *camera,
> > - const std::set<Stream *> &streams) = 0;
> > -
> > virtual int start(Camera *camera) = 0;
> > virtual void stop(Camera *camera) = 0;
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 8d9420aea3eb0b21..6d6ecb511777ec49 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -215,10 +215,8 @@ public:
> > const StreamRoles &roles) override;
> > int configure(Camera *camera, CameraConfiguration *config) override;
> >
> > - int allocateBuffers(Camera *camera,
> > - const std::set<Stream *> &streams) override;
> > - int freeBuffers(Camera *camera,
> > - const std::set<Stream *> &streams) override;
> > + int allocateBuffers(Camera *camera);
> > + int freeBuffers(Camera *camera);
>
> Shouldn't those methods be made private ?
>
> >
> > int start(Camera *camera) override;
> > void stop(Camera *camera) override;
> > @@ -629,8 +627,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> > * In order to be able to start the 'viewfinder' and 'stat' nodes, we need
> > * memory to be reserved.
> > */
> > -int PipelineHandlerIPU3::allocateBuffers(Camera *camera,
> > - const std::set<Stream *> &streams)
> > +int PipelineHandlerIPU3::allocateBuffers(Camera *camera)
> > {
> > IPU3CameraData *data = cameraData(camera);
> > unsigned int bufferCount;
> > @@ -670,13 +667,12 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera,
> >
> > return 0;
> > error:
> > - freeBuffers(camera, streams);
> > + freeBuffers(camera);
> >
> > return ret;
> > }
> >
> > -int PipelineHandlerIPU3::freeBuffers(Camera *camera,
> > - const std::set<Stream *> &streams)
> > +int PipelineHandlerIPU3::freeBuffers(Camera *camera)
> > {
> > IPU3CameraData *data = cameraData(camera);
> >
> > @@ -693,6 +689,10 @@ int PipelineHandlerIPU3::start(Camera *camera)
> > ImgUDevice *imgu = data->imgu_;
> > int ret;
> >
I think we need a comment here to explain why buffers need to be
allocated here, and what buffers are being allocated. This code will be
used as a reference implementation, so it should have detailed comments.
Same for the RkISP1 pipeline handler.
> > + ret = allocateBuffers(camera);
> > + if (ret)
> > + return ret;
> > +
> > /*
> > * Start the ImgU video devices, buffers will be queued to the
> > * ImgU output and viewfinder when requests will be queued.
> > @@ -711,6 +711,7 @@ int PipelineHandlerIPU3::start(Camera *camera)
> > return 0;
> >
> > error:
> > + freeBuffers(camera);
> > LOG(IPU3, Error) << "Failed to start camera " << camera->name();
> >
> > return ret;
> > @@ -726,6 +727,8 @@ void PipelineHandlerIPU3::stop(Camera *camera)
> > if (ret)
> > LOG(IPU3, Warning) << "Failed to stop camera "
> > << camera->name();
> > +
> > + freeBuffers(camera);
> > }
> >
> > int PipelineHandlerIPU3::queueRequestHardware(Camera *camera, Request *request)
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 96e863f0208fa748..a56c7022f57ce771 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -174,10 +174,8 @@ public:
> > const StreamRoles &roles) override;
> > int configure(Camera *camera, CameraConfiguration *config) override;
> >
> > - int allocateBuffers(Camera *camera,
> > - const std::set<Stream *> &streams) override;
> > - int freeBuffers(Camera *camera,
> > - const std::set<Stream *> &streams) override;
> > + int allocateBuffers(Camera *camera);
> > + int freeBuffers(Camera *camera);
>
> Same here, shouldn't these methods be made private ?
>
> With this fixed,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> >
> > int start(Camera *camera) override;
> > void stop(Camera *camera) override;
> > @@ -651,8 +649,7 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
> > return 0;
> > }
> >
> > -int PipelineHandlerRkISP1::allocateBuffers(Camera *camera,
> > - const std::set<Stream *> &streams)
> > +int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)
> > {
> > RkISP1CameraData *data = cameraData(camera);
> > std::vector<FrameBuffer *> paramBuffers, statBuffers;
> > @@ -695,8 +692,7 @@ err:
> > return ret;
> > }
> >
> > -int PipelineHandlerRkISP1::freeBuffers(Camera *camera,
> > - const std::set<Stream *> &streams)
> > +int PipelineHandlerRkISP1::freeBuffers(Camera *camera)
> > {
> > RkISP1CameraData *data = cameraData(camera);
> >
> > @@ -731,10 +727,15 @@ int PipelineHandlerRkISP1::start(Camera *camera)
> > RkISP1CameraData *data = cameraData(camera);
> > int ret;
> >
> > + ret = allocateBuffers(camera);
> > + if (ret)
> > + return ret;
> > +
> > data->frame_ = 0;
> >
> > ret = param_->streamOn();
> > if (ret) {
> > + freeBuffers(camera);
> > LOG(RkISP1, Error)
> > << "Failed to start parameters " << camera->name();
> > return ret;
> > @@ -743,6 +744,7 @@ int PipelineHandlerRkISP1::start(Camera *camera)
> > ret = stat_->streamOn();
> > if (ret) {
> > param_->streamOff();
> > + freeBuffers(camera);
> > LOG(RkISP1, Error)
> > << "Failed to start statistics " << camera->name();
> > return ret;
> > @@ -752,6 +754,7 @@ int PipelineHandlerRkISP1::start(Camera *camera)
> > if (ret) {
> > param_->streamOff();
> > stat_->streamOff();
> > + freeBuffers(camera);
> >
> > LOG(RkISP1, Error)
> > << "Failed to start camera " << camera->name();
> > @@ -796,6 +799,8 @@ void PipelineHandlerRkISP1::stop(Camera *camera)
> >
> > data->timeline_.reset();
> >
> > + freeBuffers(camera);
> > +
> > activeCamera_ = nullptr;
> > }
> >
> > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> > index c275b9f0f75429bd..2be7864ef59e932b 100644
> > --- a/src/libcamera/pipeline/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo.cpp
> > @@ -65,11 +65,6 @@ public:
> > const StreamRoles &roles) override;
> > int configure(Camera *camera, CameraConfiguration *config) override;
> >
> > - int allocateBuffers(Camera *camera,
> > - const std::set<Stream *> &streams) override;
> > - int freeBuffers(Camera *camera,
> > - const std::set<Stream *> &streams) override;
> > -
> > int start(Camera *camera) override;
> > void stop(Camera *camera) override;
> >
> > @@ -193,18 +188,6 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config)
> > return 0;
> > }
> >
> > -int PipelineHandlerUVC::allocateBuffers(Camera *camera,
> > - const std::set<Stream *> &streams)
> > -{
> > - return 0;
> > -}
> > -
> > -int PipelineHandlerUVC::freeBuffers(Camera *camera,
> > - const std::set<Stream *> &streams)
> > -{
> > - return 0;
> > -}
> > -
> > int PipelineHandlerUVC::start(Camera *camera)
> > {
> > UVCCameraData *data = cameraData(camera);
> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> > index 64793788c752c791..cacf8bfe3ff9cc45 100644
> > --- a/src/libcamera/pipeline/vimc.cpp
> > +++ b/src/libcamera/pipeline/vimc.cpp
> > @@ -84,11 +84,6 @@ public:
> > const StreamRoles &roles) override;
> > int configure(Camera *camera, CameraConfiguration *config) override;
> >
> > - int allocateBuffers(Camera *camera,
> > - const std::set<Stream *> &streams) override;
> > - int freeBuffers(Camera *camera,
> > - const std::set<Stream *> &streams) override;
> > -
> > int start(Camera *camera) override;
> > void stop(Camera *camera) override;
> >
> > @@ -261,18 +256,6 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config)
> > return 0;
> > }
> >
> > -int PipelineHandlerVimc::allocateBuffers(Camera *camera,
> > - const std::set<Stream *> &streams)
> > -{
> > - return 0;
> > -}
> > -
> > -int PipelineHandlerVimc::freeBuffers(Camera *camera,
> > - const std::set<Stream *> &streams)
> > -{
> > - return 0;
> > -}
> > -
> > int PipelineHandlerVimc::start(Camera *camera)
> > {
> > VimcCameraData *data = cameraData(camera);
> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > index b107e23258dee692..67758f3f1ebc39e5 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -287,35 +287,6 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera)
> > * \return 0 on success or a negative error code otherwise
> > */
> >
> > -/**
> > - * \fn PipelineHandler::allocateBuffers()
> > - * \brief Allocate buffers for a stream
> > - * \param[in] camera The camera the \a stream belongs to
> > - * \param[in] streams The set of streams to allocate buffers for
> > - *
> > - * This method allocates buffers internally in the pipeline handler for each
> > - * stream in the \a streams buffer set, and associates them with the stream's
> > - * buffer pool.
> > - *
> > - * The intended caller of this method is the Camera class.
> > - *
> > - * \return 0 on success or a negative error code otherwise
> > - */
> > -
> > -/**
> > - * \fn PipelineHandler::freeBuffers()
> > - * \brief Free all buffers associated with a stream
> > - * \param[in] camera The camera the \a stream belongs to
> > - * \param[in] streams The set of streams to free buffers from
> > - *
> > - * After a capture session has been stopped all buffers associated with each
> > - * stream shall be freed.
> > - *
> > - * The intended caller of this method is the Camera class.
> > - *
> > - * \return 0 on success or a negative error code otherwise
> > - */
> > -
> > /**
> > * \fn PipelineHandler::start()
> > * \brief Start capturing from a group of streams
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list