[libcamera-devel] [PATCH v4 03/12] libcamera: camera: allocateBuffers: Pass the stream set
Niklas Söderlund
niklas.soderlund at ragnatech.se
Sun Apr 14 21:53:58 CEST 2019
Hi Jacopo,
Thanks for your patch.
On 2019-04-09 21:25:39 +0200, 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 for each
> active stream, leaving no space for stream-independent configuration.
>
> Change this by providing to the pipeline handlers the full set of active
> streams, and ask them to loop over them to perform per-streams
> allocations.
>
> While at it, propagate the freeBuffer() error code to the applications
> in case of failure.
I think you should do this in a separate patch... yea I know you heard
it before. I'm going to keep pestering you with comments about this as
reviewing patches larger then ~40 lines which do more then one thing is
a huge pain for me ;-)
>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
I think you can put this patch at the beginning of the series or
potentially merge/post it as a separate patch as it solves a design
issue not strictly related to the IPU3 code.
I would drop 'allocateBuffers:' from the path subject as it's not our
convention to annotate using function names, how about
libcamera: camera: Pass all active streams in allocateBuffers()
With the subject fixed and with or without breaking the patch in two,
Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> src/libcamera/camera.cpp | 17 +++++++++--------
> src/libcamera/include/pipeline_handler.h | 6 ++++--
> src/libcamera/pipeline/ipu3/ipu3.cpp | 13 +++++++++----
> src/libcamera/pipeline/uvcvideo.cpp | 13 +++++++++----
> src/libcamera/pipeline/vimc.cpp | 13 +++++++++----
> src/libcamera/pipeline_handler.cpp | 11 ++++++-----
> 6 files changed, 46 insertions(+), 27 deletions(-)
>
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index bdf14b31d8ee..cb392ca3b7e7 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -647,13 +647,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;
> @@ -683,9 +681,12 @@ int Camera::freeBuffers()
> * by the V4L2 device that has allocated them.
> */
> stream->bufferPool().destroyBuffers();
> - pipe_->freeBuffers(this, stream);
> }
>
> + int ret = pipe_->freeBuffers(this, activeStreams_);
> + if (ret)
> + return ret;
> +
> state_ = CameraConfigured;
>
> return 0;
> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> index b6cbd3bae51b..253820e8eaf8 100644
> --- a/src/libcamera/include/pipeline_handler.h
> +++ b/src/libcamera/include/pipeline_handler.h
> @@ -57,8 +57,10 @@ public:
> streamConfiguration(Camera *camera, const std::vector<StreamUsage> &usages) = 0;
> virtual int configureStreams(Camera *camera, const CameraConfiguration &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 int 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 712e57c5a459..527213a8970a 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -159,8 +159,10 @@ public:
> int configureStreams(Camera *camera,
> const CameraConfiguration &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;
> + int freeBuffers(Camera *camera,
> + const std::set<Stream *> &streams) override;
>
> int start(Camera *camera) override;
> void stop(Camera *camera) override;
> @@ -378,9 +380,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();
> CIO2Device *cio2 = &data->cio2_;
> ImgUDevice *imgu = data->imgu_;
> int ret;
> @@ -419,7 +423,8 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream)
> return 0;
> }
>
> -int PipelineHandlerIPU3::freeBuffers(Camera *camera, Stream *stream)
> +int PipelineHandlerIPU3::freeBuffers(Camera *camera,
> + const std::set<Stream *> &streams)
> {
> IPU3CameraData *data = cameraData(camera);
>
> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> index cd472cfadd86..b8f634d88b46 100644
> --- a/src/libcamera/pipeline/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo.cpp
> @@ -32,8 +32,10 @@ public:
> int configureStreams(Camera *camera,
> const CameraConfiguration &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;
> + int freeBuffers(Camera *camera,
> + const std::set<Stream *> &streams) override;
>
> int start(Camera *camera) override;
> void stop(Camera *camera) override;
> @@ -127,9 +129,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";
> @@ -137,7 +141,8 @@ int PipelineHandlerUVC::allocateBuffers(Camera *camera, Stream *stream)
> return data->video_->exportBuffers(&stream->bufferPool());
> }
>
> -int PipelineHandlerUVC::freeBuffers(Camera *camera, Stream *stream)
> +int PipelineHandlerUVC::freeBuffers(Camera *camera,
> + const std::set<Stream *> &streams)
> {
> UVCCameraData *data = cameraData(camera);
> return data->video_->releaseBuffers();
> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> index c8bbe2a19847..22449e47bc2d 100644
> --- a/src/libcamera/pipeline/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc.cpp
> @@ -32,8 +32,10 @@ public:
> int configureStreams(Camera *camera,
> const CameraConfiguration &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;
> + int freeBuffers(Camera *camera,
> + const std::set<Stream *> &streams) override;
>
> int start(Camera *camera) override;
> void stop(Camera *camera) override;
> @@ -127,9 +129,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";
> @@ -137,7 +141,8 @@ int PipelineHandlerVimc::allocateBuffers(Camera *camera, Stream *stream)
> return data->video_->exportBuffers(&stream->bufferPool());
> }
>
> -int PipelineHandlerVimc::freeBuffers(Camera *camera, Stream *stream)
> +int PipelineHandlerVimc::freeBuffers(Camera *camera,
> + const std::set<Stream *> &streams)
> {
> VimcCameraData *data = cameraData(camera);
> return data->video_->releaseBuffers();
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 43550c0e0210..911d08448e69 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -193,10 +193,11 @@ 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 set of streams to allocate buffers for
> *
> - * This method allocates buffers internally in the pipeline handler and
> - * associates them with the stream's buffer pool.
> + * 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.
> *
> @@ -207,9 +208,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 set of 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