[libcamera-devel] [PATCH] libcamera: camera: allocateBuffers: Pass the stream set
Niklas Söderlund
niklas.soderlund at ragnatech.se
Wed Mar 27 10:41:38 CET 2019
Hi Jacopo,
Thanks for your patch.
On 2019-03-26 17:50:11 +0100, Jacopo Mondi wrote:
> Pipeline handlers might need to perform allocation of internal buffers,
> setup operations, or simple sanity check before going into the
> per-stream buffer allocation.
>
> As of now, PipelineHandler::allocateBuffers() is called once per each
> active stream, leaving no space for stream-independent configuration.
>
> Change this by providing to the pipeline handler the full set of active
> stream, and ask them to loop over them to perform per-streams
> allocations after eventual stream-independent operations.
>
> The same rational applies to PipelineHandler::freeBuffers() and eventual
> stream-independent clean up operations.
>
> While at it, change the return type of freeBuffers() to void as it was
> not checked by the Camera class method calling it.
>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> As noted with the development of multi stream support for the IPU3 platform,
> pipeline handlers might need room to perform pre and post stream buffer memory
> allocation operations.
>
> I initally implemented pre and post hooks, but as Laurent suggested, changing
> the interface between Camera and Pipeline Handlers is a much nicer solution.
> ---
> src/libcamera/camera.cpp | 15 +++++++--------
> src/libcamera/include/pipeline_handler.h | 6 ++++--
> src/libcamera/pipeline/ipu3/ipu3.cpp | 19 ++++++++++---------
> src/libcamera/pipeline/uvcvideo.cpp | 15 ++++++++++-----
> src/libcamera/pipeline/vimc.cpp | 15 ++++++++++-----
> src/libcamera/pipeline_handler.cpp | 8 ++++----
> 6 files changed, 45 insertions(+), 33 deletions(-)
>
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 8ee9cc086616..99eb0e6876f0 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -467,13 +467,11 @@ int Camera::allocateBuffers()
> return -EINVAL;
> }
>
> - for (Stream *stream : activeStreams_) {
> - int ret = pipe_->allocateBuffers(this, stream);
> - if (ret) {
> - LOG(Camera, Error) << "Failed to allocate buffers";
> - freeBuffers();
> - return ret;
> - }
> + int ret = pipe_->allocateBuffers(this, activeStreams_);
> + if (ret) {
> + LOG(Camera, Error) << "Failed to allocate buffers";
> + freeBuffers();
> + return ret;
> }
>
> state_ = CameraPrepared;
> @@ -503,9 +501,10 @@ int Camera::freeBuffers()
> * by the V4L2 device that has allocated them.
> */
> stream->bufferPool().destroyBuffers();
> - pipe_->freeBuffers(this, stream);
> }
>
> + pipe_->freeBuffers(this, activeStreams_);
> +
> state_ = CameraConfigured;
>
> return 0;
> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> index acb376e07030..920b57609470 100644
> --- a/src/libcamera/include/pipeline_handler.h
> +++ b/src/libcamera/include/pipeline_handler.h
> @@ -57,8 +57,10 @@ public:
> virtual int configureStreams(Camera *camera,
> std::map<Stream *, StreamConfiguration> &config) = 0;
>
> - virtual int allocateBuffers(Camera *camera, Stream *stream) = 0;
> - virtual int freeBuffers(Camera *camera, Stream *stream) = 0;
> + virtual int allocateBuffers(Camera *camera,
> + const std::set<Stream *> &streams) = 0;
> + virtual void freeBuffers(Camera *camera,
> + const std::set<Stream *> &streams) = 0;
>
> virtual int start(Camera *camera) = 0;
> virtual void stop(Camera *camera);
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 55489c31df2d..c13252b6f77e 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -36,8 +36,10 @@ public:
> int configureStreams(Camera *camera,
> std::map<Stream *, StreamConfiguration> &config) override;
>
> - int allocateBuffers(Camera *camera, Stream *stream) override;
> - int freeBuffers(Camera *camera, Stream *stream) override;
> + int allocateBuffers(Camera *camera,
> + const std::set<Stream *> &streams) override;
> + void freeBuffers(Camera *camera,
> + const std::set<Stream *> &streams) override;
>
> int start(Camera *camera) override;
> void stop(Camera *camera) override;
> @@ -188,9 +190,11 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,
> return 0;
> }
>
> -int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream)
> +int PipelineHandlerIPU3::allocateBuffers(Camera *camera,
> + const std::set<Stream *> &streams)
> {
> IPU3CameraData *data = cameraData(camera);
> + Stream *stream = *streams.begin();
> const StreamConfiguration &cfg = stream->configuration();
>
> if (!cfg.bufferCount)
> @@ -205,17 +209,14 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream)
> return 0;
> }
>
> -int PipelineHandlerIPU3::freeBuffers(Camera *camera, Stream *stream)
> +void PipelineHandlerIPU3::freeBuffers(Camera *camera,
> + const std::set<Stream *> &streams)
> {
> IPU3CameraData *data = cameraData(camera);
>
> int ret = data->cio2_->releaseBuffers();
> - if (ret) {
> + if (ret)
> LOG(IPU3, Error) << "Failed to release memory";
> - return ret;
> - }
> -
> - return 0;
> }
>
> int PipelineHandlerIPU3::start(Camera *camera)
> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> index cc3e0cd9afab..128f0c49dba3 100644
> --- a/src/libcamera/pipeline/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo.cpp
> @@ -32,8 +32,10 @@ public:
> int configureStreams(Camera *camera,
> std::map<Stream *, StreamConfiguration> &config) override;
>
> - int allocateBuffers(Camera *camera, Stream *stream) override;
> - int freeBuffers(Camera *camera, Stream *stream) override;
> + int allocateBuffers(Camera *camera,
> + const std::set<Stream *> &streams) override;
> + void freeBuffers(Camera *camera,
> + const std::set<Stream *> &streams) override;
>
> int start(Camera *camera) override;
> void stop(Camera *camera) override;
> @@ -129,9 +131,11 @@ int PipelineHandlerUVC::configureStreams(Camera *camera,
> return 0;
> }
>
> -int PipelineHandlerUVC::allocateBuffers(Camera *camera, Stream *stream)
> +int PipelineHandlerUVC::allocateBuffers(Camera *camera,
> + const std::set<Stream *> &streams)
> {
> UVCCameraData *data = cameraData(camera);
> + Stream *stream = *streams.begin();
> const StreamConfiguration &cfg = stream->configuration();
>
> LOG(UVC, Debug) << "Requesting " << cfg.bufferCount << " buffers";
> @@ -139,10 +143,11 @@ int PipelineHandlerUVC::allocateBuffers(Camera *camera, Stream *stream)
> return data->video_->exportBuffers(&stream->bufferPool());
> }
>
> -int PipelineHandlerUVC::freeBuffers(Camera *camera, Stream *stream)
> +void PipelineHandlerUVC::freeBuffers(Camera *camera,
> + const std::set<Stream *> &streams)
> {
> UVCCameraData *data = cameraData(camera);
> - return data->video_->releaseBuffers();
> + data->video_->releaseBuffers();
> }
>
> int PipelineHandlerUVC::start(Camera *camera)
> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> index 2e8c26fb7c0b..6735940799d8 100644
> --- a/src/libcamera/pipeline/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc.cpp
> @@ -32,8 +32,10 @@ public:
> int configureStreams(Camera *camera,
> std::map<Stream *, StreamConfiguration> &config) override;
>
> - int allocateBuffers(Camera *camera, Stream *stream) override;
> - int freeBuffers(Camera *camera, Stream *stream) override;
> + int allocateBuffers(Camera *camera,
> + const std::set<Stream *> &streams) override;
> + void freeBuffers(Camera *camera,
> + const std::set<Stream *> &streams) override;
>
> int start(Camera *camera) override;
> void stop(Camera *camera) override;
> @@ -129,9 +131,11 @@ int PipelineHandlerVimc::configureStreams(Camera *camera,
> return 0;
> }
>
> -int PipelineHandlerVimc::allocateBuffers(Camera *camera, Stream *stream)
> +int PipelineHandlerVimc::allocateBuffers(Camera *camera,
> + const std::set<Stream *> &streams)
> {
> VimcCameraData *data = cameraData(camera);
> + Stream *stream = *streams.begin();
> const StreamConfiguration &cfg = stream->configuration();
>
> LOG(VIMC, Debug) << "Requesting " << cfg.bufferCount << " buffers";
> @@ -139,10 +143,11 @@ int PipelineHandlerVimc::allocateBuffers(Camera *camera, Stream *stream)
> return data->video_->exportBuffers(&stream->bufferPool());
> }
>
> -int PipelineHandlerVimc::freeBuffers(Camera *camera, Stream *stream)
> +void PipelineHandlerVimc::freeBuffers(Camera *camera,
> + const std::set<Stream *> &streams)
> {
> VimcCameraData *data = cameraData(camera);
> - return data->video_->releaseBuffers();
> + data->video_->releaseBuffers();
> }
>
> int PipelineHandlerVimc::start(Camera *camera)
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 1a858f2638ce..d14d463cf610 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -193,10 +193,10 @@ PipelineHandler::~PipelineHandler()
> * \fn PipelineHandler::allocateBuffers()
> * \brief Allocate buffers for a stream
> * \param[in] camera The camera the \a stream belongs to
> - * \param[in] stream The stream to allocate buffers for
> + * \param[in] streams The group of active streams to allocate buffers for
I would s/active// here and bellow, the pipeline handler cares not that
the camera calls this interface using the active streams. The parameter
is an array of streams to allocate/free buffers from.
With this fixed,
Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> *
> * This method allocates buffers internally in the pipeline handler and
> - * associates them with the stream's buffer pool.
> + * associates them with each stream's buffer pool.
> *
> * The intended caller of this method is the Camera class.
> *
> @@ -207,9 +207,9 @@ PipelineHandler::~PipelineHandler()
> * \fn PipelineHandler::freeBuffers()
> * \brief Free all buffers associated with a stream
> * \param[in] camera The camera the \a stream belongs to
> - * \param[in] stream The stream to free buffers from
> + * \param[in] streams The group of active streams to free buffers from
> *
> - * After a capture session has been stopped all buffers associated with the
> + * 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.
> --
> 2.21.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list