[libcamera-devel] [PATCH 8/9] libcamera: pipeline_handler: Fold buffer management with start/stop
Niklas Söderlund
niklas.soderlund at ragnatech.se
Mon Mar 16 16:48:41 CET 2020
Hi Laurent,
Thanks for your patch.
On 2020-03-15 01:57:27 +0200, Laurent Pinchart wrote:
> There's no need anymore to have the Camera object control how and when
> pipeline handlers allocate and free the buffers for the
> application-facing video devices. Fold those operations, currently
> performed by importFrameBuffers() and freeFrameBuffers(), into the
> start() and stop() functions. This simplifies the pipeline handler API,
> its implementation, and the implementation of the Camera class.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> src/libcamera/camera.cpp | 11 --
> src/libcamera/include/pipeline_handler.h | 2 -
> src/libcamera/pipeline/ipu3/ipu3.cpp | 146 +++++++++++------------
> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 20 ++--
> src/libcamera/pipeline/uvcvideo.cpp | 28 ++---
> src/libcamera/pipeline/vimc.cpp | 28 ++---
> src/libcamera/pipeline_handler.cpp | 41 +------
> 7 files changed, 101 insertions(+), 175 deletions(-)
>
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 3192dfb42d01..5593c1b317a0 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -915,13 +915,6 @@ int Camera::start()
>
> LOG(Camera, Debug) << "Starting capture";
>
> - for (Stream *stream : p_->activeStreams_) {
> - ret = p_->pipe_->invokeMethod(&PipelineHandler::importFrameBuffers,
> - ConnectionTypeDirect, this, stream);
> - if (ret < 0)
> - return ret;
> - }
> -
> ret = p_->pipe_->invokeMethod(&PipelineHandler::start,
> ConnectionTypeBlocking, this);
> if (ret)
> @@ -959,10 +952,6 @@ int Camera::stop()
> p_->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking,
> this);
>
> - for (Stream *stream : p_->activeStreams_)
> - p_->pipe_->invokeMethod(&PipelineHandler::freeFrameBuffers,
> - ConnectionTypeBlocking, this, stream);
> -
> return 0;
> }
>
> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> index db6c3104d812..3fcfeda4bfee 100644
> --- a/src/libcamera/include/pipeline_handler.h
> +++ b/src/libcamera/include/pipeline_handler.h
> @@ -76,8 +76,6 @@ public:
>
> virtual int exportFrameBuffers(Camera *camera, Stream *stream,
> std::vector<std::unique_ptr<FrameBuffer>> *buffers) = 0;
> - virtual int importFrameBuffers(Camera *camera, Stream *stream) = 0;
> - virtual void freeFrameBuffers(Camera *camera, Stream *stream) = 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 b6db8d567ea4..6b93c50978a7 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -72,6 +72,7 @@ public:
> int configureOutput(ImgUOutput *output,
> const StreamConfiguration &cfg);
>
> + int allocateBuffers(IPU3CameraData *data, unsigned int bufferCount);
> void freeBuffers(IPU3CameraData *data);
>
> int start();
> @@ -208,8 +209,6 @@ public:
>
> int exportFrameBuffers(Camera *camera, Stream *stream,
> std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> - int importFrameBuffers(Camera *camera, Stream *stream) override;
> - void freeFrameBuffers(Camera *camera, Stream *stream) override;
>
> int start(Camera *camera) override;
> void stop(Camera *camera) override;
> @@ -625,23 +624,6 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,
> return video->exportBuffers(count, buffers);
> }
>
> -int PipelineHandlerIPU3::importFrameBuffers(Camera *camera, Stream *stream)
> -{
> - IPU3Stream *ipu3stream = static_cast<IPU3Stream *>(stream);
> - V4L2VideoDevice *video = ipu3stream->device_->dev;
> - unsigned int count = stream->configuration().bufferCount;
> -
> - return video->importBuffers(count);
> -}
> -
> -void PipelineHandlerIPU3::freeFrameBuffers(Camera *camera, Stream *stream)
> -{
> - IPU3Stream *ipu3stream = static_cast<IPU3Stream *>(stream);
> - V4L2VideoDevice *video = ipu3stream->device_->dev;
> -
> - video->releaseBuffers();
> -}
> -
> /**
> * \todo Clarify if 'viewfinder' and 'stat' nodes have to be set up and
> * started even if not in use. As of now, if not properly configured and
> @@ -653,69 +635,24 @@ void PipelineHandlerIPU3::freeFrameBuffers(Camera *camera, Stream *stream)
> int PipelineHandlerIPU3::allocateBuffers(Camera *camera)
> {
> IPU3CameraData *data = cameraData(camera);
> - IPU3Stream *outStream = &data->outStream_;
> - IPU3Stream *vfStream = &data->vfStream_;
> CIO2Device *cio2 = &data->cio2_;
> ImgUDevice *imgu = data->imgu_;
> unsigned int bufferCount;
> int ret;
>
> - /* Share buffers between CIO2 output and ImgU input. */
> ret = cio2->allocateBuffers();
> if (ret < 0)
> return ret;
>
> bufferCount = ret;
>
> - ret = imgu->input_->importBuffers(bufferCount);
> - if (ret) {
> - LOG(IPU3, Error) << "Failed to import ImgU input buffers";
> - goto error;
> - }
> -
> - /*
> - * Use for the stat's internal pool the same number of buffers as for
> - * the input pool.
> - * \todo To be revised when we'll actually use the stat node.
> - */
> - ret = imgu->stat_.dev->allocateBuffers(bufferCount, &imgu->stat_.buffers);
> + ret = imgu->allocateBuffers(data, bufferCount);
> if (ret < 0) {
> - LOG(IPU3, Error) << "Failed to allocate ImgU stat buffers";
> - goto error;
> - }
> -
> - /*
> - * Allocate buffers also on non-active outputs; use the same number
> - * of buffers as the active ones.
> - */
> - if (!outStream->active_) {
> - ImgUDevice::ImgUOutput *output = outStream->device_;
> -
> - ret = output->dev->allocateBuffers(bufferCount, &output->buffers);
> - if (ret < 0) {
> - LOG(IPU3, Error) << "Failed to allocate ImgU "
> - << output->name << " buffers";
> - goto error;
> - }
> - }
> -
> - if (!vfStream->active_) {
> - ImgUDevice::ImgUOutput *output = vfStream->device_;
> -
> - ret = output->dev->allocateBuffers(bufferCount, &output->buffers);
> - if (ret < 0) {
> - LOG(IPU3, Error) << "Failed to allocate ImgU "
> - << output->name << " buffers";
> - goto error;
> - }
> + cio2->freeBuffers();
> + return ret;
> }
>
> return 0;
> -
> -error:
> - freeBuffers(camera);
> -
> - return ret;
> }
>
> int PipelineHandlerIPU3::freeBuffers(Camera *camera)
> @@ -1156,6 +1093,65 @@ int ImgUDevice::configureOutput(ImgUOutput *output,
> return 0;
> }
>
> +/**
> + * \brief Allocate buffers for all the ImgU video devices
> + */
> +int ImgUDevice::allocateBuffers(IPU3CameraData *data, unsigned int bufferCount)
> +{
> + IPU3Stream *outStream = &data->outStream_;
> + IPU3Stream *vfStream = &data->vfStream_;
> +
> + /* Share buffers between CIO2 output and ImgU input. */
> + int ret = input_->importBuffers(bufferCount);
> + if (ret) {
> + LOG(IPU3, Error) << "Failed to import ImgU input buffers";
> + return ret;
> + }
> +
> + /*
> + * Use for the stat's internal pool the same number of buffers as for
> + * the input pool.
> + * \todo To be revised when we'll actually use the stat node.
> + */
> + ret = stat_.dev->allocateBuffers(bufferCount, &stat_.buffers);
> + if (ret < 0) {
> + LOG(IPU3, Error) << "Failed to allocate ImgU stat buffers";
> + goto error;
> + }
> +
> + /*
> + * Allocate buffers for both outputs. If an output is active, prepare
> + * for buffer import, otherwise allocate internal buffers. Use the same
> + * number of buffers in either case.
> + */
> + if (outStream->active_)
> + ret = output_.dev->importBuffers(bufferCount);
> + else
> + ret = output_.dev->allocateBuffers(bufferCount,
> + &output_.buffers);
> + if (ret < 0) {
> + LOG(IPU3, Error) << "Failed to allocate ImgU output buffers";
> + goto error;
> + }
> +
> + if (vfStream->active_)
> + ret = viewfinder_.dev->importBuffers(bufferCount);
> + else
> + ret = viewfinder_.dev->allocateBuffers(bufferCount,
> + &viewfinder_.buffers);
> + if (ret < 0) {
> + LOG(IPU3, Error) << "Failed to allocate ImgU viewfinder buffers";
> + goto error;
> + }
> +
> + return 0;
> +
> +error:
> + freeBuffers(data);
> +
> + return ret;
> +}
> +
> /**
> * \brief Release buffers for all the ImgU video devices
> */
> @@ -1163,21 +1159,17 @@ void ImgUDevice::freeBuffers(IPU3CameraData *data)
> {
> int ret;
>
> - if (!data->outStream_.active_) {
> - ret = output_.dev->releaseBuffers();
> - if (ret)
> - LOG(IPU3, Error) << "Failed to release ImgU output buffers";
> - }
> + ret = output_.dev->releaseBuffers();
> + if (ret)
> + LOG(IPU3, Error) << "Failed to release ImgU output buffers";
>
> ret = stat_.dev->releaseBuffers();
> if (ret)
> LOG(IPU3, Error) << "Failed to release ImgU stat buffers";
>
> - if (!data->vfStream_.active_) {
> - ret = viewfinder_.dev->releaseBuffers();
> - if (ret)
> - LOG(IPU3, Error) << "Failed to release ImgU viewfinder buffers";
> - }
> + ret = viewfinder_.dev->releaseBuffers();
> + if (ret)
> + LOG(IPU3, Error) << "Failed to release ImgU viewfinder buffers";
>
> ret = input_->releaseBuffers();
> if (ret)
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 1ad53fbde112..01977ad697a9 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -175,8 +175,6 @@ public:
>
> int exportFrameBuffers(Camera *camera, Stream *stream,
> std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> - int importFrameBuffers(Camera *camera, Stream *stream) override;
> - void freeFrameBuffers(Camera *camera, Stream *stream) override;
>
> int start(Camera *camera) override;
> void stop(Camera *camera) override;
> @@ -668,17 +666,6 @@ int PipelineHandlerRkISP1::exportFrameBuffers(Camera *camera, Stream *stream,
> return video_->exportBuffers(count, buffers);
> }
>
> -int PipelineHandlerRkISP1::importFrameBuffers(Camera *camera, Stream *stream)
> -{
> - unsigned int count = stream->configuration().bufferCount;
> - return video_->importBuffers(count);
> -}
> -
> -void PipelineHandlerRkISP1::freeFrameBuffers(Camera *camera, Stream *stream)
> -{
> - video_->releaseBuffers();
> -}
> -
> int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)
> {
> RkISP1CameraData *data = cameraData(camera);
> @@ -689,6 +676,10 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)
> for (const Stream *s : camera->streams())
> maxBuffers = std::max(maxBuffers, s->configuration().bufferCount);
>
> + ret = video_->importBuffers(count);
> + if (ret < 0)
> + goto error;
> +
> ret = param_->allocateBuffers(maxBuffers, ¶mBuffers_);
> if (ret < 0)
> goto error;
> @@ -749,6 +740,9 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera)
> if (stat_->releaseBuffers())
> LOG(RkISP1, Error) << "Failed to release stat buffers";
>
> + if (video_->releaseBuffers())
> + LOG(RkISP1, Error) << "Failed to release video buffers";
> +
> return 0;
> }
>
> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> index 29afb121aa46..40cc3ee7d098 100644
> --- a/src/libcamera/pipeline/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo.cpp
> @@ -67,8 +67,6 @@ public:
>
> int exportFrameBuffers(Camera *camera, Stream *stream,
> std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> - int importFrameBuffers(Camera *camera, Stream *stream) override;
> - void freeFrameBuffers(Camera *camera, Stream *stream) override;
>
> int start(Camera *camera) override;
> void stop(Camera *camera) override;
> @@ -202,31 +200,29 @@ int PipelineHandlerUVC::exportFrameBuffers(Camera *camera, Stream *stream,
> return data->video_->exportBuffers(count, buffers);
> }
>
> -int PipelineHandlerUVC::importFrameBuffers(Camera *camera, Stream *stream)
> +int PipelineHandlerUVC::start(Camera *camera)
> {
> UVCCameraData *data = cameraData(camera);
> - unsigned int count = stream->configuration().bufferCount;
> + unsigned int count = data->stream_.configuration().bufferCount;
>
> - return data->video_->importBuffers(count);
> -}
> -
> -void PipelineHandlerUVC::freeFrameBuffers(Camera *camera, Stream *stream)
> -{
> - UVCCameraData *data = cameraData(camera);
> + int ret = data->video_->importBuffers(count);
> + if (ret < 0)
> + return ret;
>
> - data->video_->releaseBuffers();
> -}
> + ret = data->video_->streamOn();
> + if (ret < 0) {
> + data->video_->releaseBuffers();
> + return ret;
> + }
>
> -int PipelineHandlerUVC::start(Camera *camera)
> -{
> - UVCCameraData *data = cameraData(camera);
> - return data->video_->streamOn();
> + return 0;
> }
>
> void PipelineHandlerUVC::stop(Camera *camera)
> {
> UVCCameraData *data = cameraData(camera);
> data->video_->streamOff();
> + data->video_->releaseBuffers();
> }
>
> int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)
> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> index 5d3d12fef30b..eceb16d5586a 100644
> --- a/src/libcamera/pipeline/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc.cpp
> @@ -84,8 +84,6 @@ public:
>
> int exportFrameBuffers(Camera *camera, Stream *stream,
> std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> - int importFrameBuffers(Camera *camera, Stream *stream) override;
> - void freeFrameBuffers(Camera *camera, Stream *stream) override;
>
> int start(Camera *camera) override;
> void stop(Camera *camera) override;
> @@ -268,31 +266,29 @@ int PipelineHandlerVimc::exportFrameBuffers(Camera *camera, Stream *stream,
> return data->video_->exportBuffers(count, buffers);
> }
>
> -int PipelineHandlerVimc::importFrameBuffers(Camera *camera, Stream *stream)
> +int PipelineHandlerVimc::start(Camera *camera)
> {
> VimcCameraData *data = cameraData(camera);
> - unsigned int count = stream->configuration().bufferCount;
> + unsigned int count = data->stream_.configuration().bufferCount;
>
> - return data->video_->importBuffers(count);
> -}
> -
> -void PipelineHandlerVimc::freeFrameBuffers(Camera *camera, Stream *stream)
> -{
> - VimcCameraData *data = cameraData(camera);
> + int ret = data->video_->importBuffers(count);
> + if (ret < 0)
> + return ret;
>
> - data->video_->releaseBuffers();
> -}
> + ret = data->video_->streamOn();
> + if (ret < 0) {
> + data->video_->releaseBuffers();
> + return ret;
> + }
>
> -int PipelineHandlerVimc::start(Camera *camera)
> -{
> - VimcCameraData *data = cameraData(camera);
> - return data->video_->streamOn();
> + return 0;
> }
>
> void PipelineHandlerVimc::stop(Camera *camera)
> {
> VimcCameraData *data = cameraData(camera);
> data->video_->streamOff();
> + data->video_->releaseBuffers();
> }
>
> int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request)
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index e5034c54e2fb..254d341fb8a4 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -325,7 +325,7 @@ const ControlList &PipelineHandler::properties(Camera *camera)
>
> /**
> * \fn PipelineHandler::exportFrameBuffers()
> - * \brief Allocate buffers for \a stream
> + * \brief Allocate and export buffers for \a stream
> * \param[in] camera The camera
> * \param[in] stream The stream to allocate buffers for
> * \param[out] buffers Array of buffers successfully allocated
> @@ -347,45 +347,6 @@ const ControlList &PipelineHandler::properties(Camera *camera)
> * otherwise
> */
>
> -/**
> - * \fn PipelineHandler::importFrameBuffers()
> - * \brief Prepare \a stream to use external buffers
> - * \param[in] camera The camera
> - * \param[in] stream The stream to prepare for import
> - *
> - * This method prepares the pipeline handler to use buffers provided by the
> - * application for the \a stream.
> - *
> - * The method may only be called after the Camera has been configured and before
> - * it gets started, or after it gets stopped. It shall be called only for
> - * streams that are part of the active camera configuration, and at most once
> - * per stream until buffers for the stream are freed with freeFrameBuffers().
> - *
> - * importFrameBuffers() shall also allocate all other resources required by the
> - * pipeline handler for the stream to prepare for starting the Camera.
> - *
> - * The only intended caller is Camera::start().
> - *
> - * \context This function is called from the CameraManager thread.
> - *
> - * \return 0 on success or a negative error code otherwise
> - */
> -
> -/**
> - * \fn PipelineHandler::freeFrameBuffers()
> - * \brief Free buffers allocated from the stream
> - * \param[in] camera The camera
> - * \param[in] stream The stream to free buffers for
> - *
> - * This method shall release all resources allocated for the \a stream by
> - * importFrameBuffers(). It shall be called only after a successful call that
> - * method, and only once per stream.
> - *
> - * The only intended callers are Camera::stop() and Camera::freeFrameBuffers().
> - *
> - * \context This function is called from the CameraManager thread.
> - */
> -
> /**
> * \fn PipelineHandler::start()
> * \brief Start capturing from a group of streams
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> 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