[libcamera-devel] [PATCH 29/30] libcamera: pipeline: Remove explicit buffer handling
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Dec 15 10:28:42 CET 2019
Hi Niklas,
Thank you for the patch.
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;
>
> + 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