[libcamera-devel] libcamera: camera: allocateBuffers: Pass the stream set
diff mbox series

Message ID 20190326165011.10817-1-jacopo@jmondi.org
State Superseded
Delegated to: Jacopo Mondi
Headers show
Series
  • [libcamera-devel] libcamera: camera: allocateBuffers: Pass the stream set
Related show

Commit Message

Jacopo Mondi March 26, 2019, 4:50 p.m. UTC 
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@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(-)

--
2.21.0

Comments

Niklas Söderlund March 27, 2019, 9:41 a.m. UTC | #1 
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@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@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@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Patch
diff mbox series 

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
  *
  * 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.