[libcamera-devel,v4,8/9] libcamera: ipu3: cio2: Hide buffer allocation and freeing from users
diff mbox series

Message ID 20200625223900.1282164-9-niklas.soderlund@ragnatech.se
State Accepted
Headers show
Series
  • libcamera: ipu3: Allow zero-copy RAW stream
Related show

Commit Message

Niklas Söderlund June 25, 2020, 10:38 p.m. UTC 
The allocation and freeing of buffers for the CIO2 is handled in the
IPU3 pipeline handlers start() and stop() functions. These functions
also call CIO2Device start() and stop() at the appropriate times so
move the CIO2 buffer allocation/freeing inside the CIO2Device and reduce
the complexity of the exposed interface.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---
* Changes since v3
- Keep CIO2Device::freeBuffers() but make it private.

* Changes since v2
- Stop IMGU before CIO2

* Changes since v1
- Fix potential leaking of buffers if start fails.
---
 src/libcamera/pipeline/ipu3/cio2.cpp | 62 +++++++++++++---------------
 src/libcamera/pipeline/ipu3/cio2.h   |  4 +-
 src/libcamera/pipeline/ipu3/ipu3.cpp | 14 ++-----
 3 files changed, 33 insertions(+), 47 deletions(-)

Comments

Laurent Pinchart June 26, 2020, 12:43 a.m. UTC | #1 
Hi Niklas,

Thank you for the patch.

On Fri, Jun 26, 2020 at 12:38:59AM +0200, Niklas Söderlund wrote:
> The allocation and freeing of buffers for the CIO2 is handled in the
> IPU3 pipeline handlers start() and stop() functions. These functions
> also call CIO2Device start() and stop() at the appropriate times so
> move the CIO2 buffer allocation/freeing inside the CIO2Device and reduce
> the complexity of the exposed interface.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
> * Changes since v3
> - Keep CIO2Device::freeBuffers() but make it private.
> 
> * Changes since v2
> - Stop IMGU before CIO2
> 
> * Changes since v1
> - Fix potential leaking of buffers if start fails.
> ---
>  src/libcamera/pipeline/ipu3/cio2.cpp | 62 +++++++++++++---------------
>  src/libcamera/pipeline/ipu3/cio2.h   |  4 +-
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 14 ++-----
>  3 files changed, 33 insertions(+), 47 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> index 972787a83dede203..d698cd80cba212a1 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> @@ -207,44 +207,12 @@ CIO2Device::generateConfiguration(Size size) const
>  	return cfg;
>  }
>  
> -/**
> - * \brief Allocate frame buffers for the CIO2 output
> - *
> - * Allocate frame buffers in the CIO2 video device to be used to capture frames
> - * from the CIO2 output. The buffers are stored in the CIO2Device::buffers_
> - * vector.
> - *
> - * \return Number of buffers allocated or negative error code
> - */
> -int CIO2Device::allocateBuffers()
> -{
> -	int ret = output_->allocateBuffers(CIO2_BUFFER_COUNT, &buffers_);
> -	if (ret < 0)
> -		return ret;
> -
> -	for (std::unique_ptr<FrameBuffer> &buffer : buffers_)
> -		availableBuffers_.push(buffer.get());
> -
> -	return ret;
> -}
> -
>  int CIO2Device::exportBuffers(unsigned int count,
>  			      std::vector<std::unique_ptr<FrameBuffer>> *buffers)
>  {
>  	return output_->exportBuffers(count, buffers);
>  }
>  
> -void CIO2Device::freeBuffers()
> -{
> -	/* The default std::queue constructor is explicit with gcc 5 and 6. */
> -	availableBuffers_ = std::queue<FrameBuffer *>{};
> -
> -	buffers_.clear();
> -
> -	if (output_->releaseBuffers())
> -		LOG(IPU3, Error) << "Failed to release CIO2 buffers";
> -}
> -
>  FrameBuffer *CIO2Device::getBuffer()
>  {
>  	if (availableBuffers_.empty()) {
> @@ -266,12 +234,27 @@ void CIO2Device::putBuffer(FrameBuffer *buffer)
>  
>  int CIO2Device::start()
>  {
> -	return output_->streamOn();
> +	int ret = output_->allocateBuffers(CIO2_BUFFER_COUNT, &buffers_);
> +	if (ret < 0)
> +		return ret;
> +
> +	for (std::unique_ptr<FrameBuffer> &buffer : buffers_)
> +		availableBuffers_.push(buffer.get());
> +
> +	ret = output_->streamOn();
> +	if (ret)
> +		freeBuffers();
> +
> +	return ret;
>  }
>  
>  int CIO2Device::stop()
>  {
> -	return output_->streamOff();
> +	int ret = output_->streamOff();
> +
> +	freeBuffers();
> +
> +	return ret;
>  }
>  
>  int CIO2Device::queueBuffer(FrameBuffer *buffer)
> @@ -279,6 +262,17 @@ int CIO2Device::queueBuffer(FrameBuffer *buffer)
>  	return output_->queueBuffer(buffer);
>  }
>  
> +void CIO2Device::freeBuffers()
> +{
> +	/* The default std::queue constructor is explicit with gcc 5 and 6. */
> +	availableBuffers_ = std::queue<FrameBuffer *>{};
> +
> +	buffers_.clear();
> +
> +	if (output_->releaseBuffers())
> +		LOG(IPU3, Error) << "Failed to release CIO2 buffers";
> +}

You don't have to move this function.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +
>  void CIO2Device::cio2BufferReady(FrameBuffer *buffer)
>  {
>  	bufferReady.emit(buffer);
> diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h
> index 8cb90c9da7f19ecb..47c6f010eae6a64d 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.h
> +++ b/src/libcamera/pipeline/ipu3/cio2.h
> @@ -37,10 +37,8 @@ public:
>  
>  	StreamConfiguration generateConfiguration(Size size) const;
>  
> -	int allocateBuffers();
>  	int exportBuffers(unsigned int count,
>  			  std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> -	void freeBuffers();
>  
>  	FrameBuffer *getBuffer();
>  	void putBuffer(FrameBuffer *buffer);
> @@ -54,6 +52,8 @@ public:
>  	Signal<FrameBuffer *> bufferReady;
>  
>  private:
> +	void freeBuffers();
> +
>  	void cio2BufferReady(FrameBuffer *buffer);
>  
>  	CameraSensor *sensor_;
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 2d1ec707ea4b08fe..4818027e8db1f7a3 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -658,15 +658,10 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,
>  int PipelineHandlerIPU3::allocateBuffers(Camera *camera)
>  {
>  	IPU3CameraData *data = cameraData(camera);
> -	CIO2Device *cio2 = &data->cio2_;
>  	ImgUDevice *imgu = data->imgu_;
>  	unsigned int bufferCount;
>  	int ret;
>  
> -	ret = cio2->allocateBuffers();
> -	if (ret < 0)
> -		return ret;
> -
>  	bufferCount = std::max({
>  		data->outStream_.configuration().bufferCount,
>  		data->vfStream_.configuration().bufferCount,
> @@ -674,10 +669,8 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera)
>  	});
>  
>  	ret = imgu->allocateBuffers(data, bufferCount);
> -	if (ret < 0) {
> -		cio2->freeBuffers();
> +	if (ret < 0)
>  		return ret;
> -	}
>  
>  	return 0;
>  }
> @@ -686,7 +679,6 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera)
>  {
>  	IPU3CameraData *data = cameraData(camera);
>  
> -	data->cio2_.freeBuffers();
>  	data->imgu_->freeBuffers(data);
>  
>  	return 0;
> @@ -731,10 +723,10 @@ error:
>  void PipelineHandlerIPU3::stop(Camera *camera)
>  {
>  	IPU3CameraData *data = cameraData(camera);
> -	int ret;
> +	int ret = 0;
>  
> -	ret = data->cio2_.stop();
>  	ret |= data->imgu_->stop();
> +	ret |= data->cio2_.stop();
>  	if (ret)
>  		LOG(IPU3, Warning) << "Failed to stop camera "
>  				   << camera->name();

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
index 972787a83dede203..d698cd80cba212a1 100644
--- a/src/libcamera/pipeline/ipu3/cio2.cpp
+++ b/src/libcamera/pipeline/ipu3/cio2.cpp
@@ -207,44 +207,12 @@  CIO2Device::generateConfiguration(Size size) const
 	return cfg;
 }
 
-/**
- * \brief Allocate frame buffers for the CIO2 output
- *
- * Allocate frame buffers in the CIO2 video device to be used to capture frames
- * from the CIO2 output. The buffers are stored in the CIO2Device::buffers_
- * vector.
- *
- * \return Number of buffers allocated or negative error code
- */
-int CIO2Device::allocateBuffers()
-{
-	int ret = output_->allocateBuffers(CIO2_BUFFER_COUNT, &buffers_);
-	if (ret < 0)
-		return ret;
-
-	for (std::unique_ptr<FrameBuffer> &buffer : buffers_)
-		availableBuffers_.push(buffer.get());
-
-	return ret;
-}
-
 int CIO2Device::exportBuffers(unsigned int count,
 			      std::vector<std::unique_ptr<FrameBuffer>> *buffers)
 {
 	return output_->exportBuffers(count, buffers);
 }
 
-void CIO2Device::freeBuffers()
-{
-	/* The default std::queue constructor is explicit with gcc 5 and 6. */
-	availableBuffers_ = std::queue<FrameBuffer *>{};
-
-	buffers_.clear();
-
-	if (output_->releaseBuffers())
-		LOG(IPU3, Error) << "Failed to release CIO2 buffers";
-}
-
 FrameBuffer *CIO2Device::getBuffer()
 {
 	if (availableBuffers_.empty()) {
@@ -266,12 +234,27 @@  void CIO2Device::putBuffer(FrameBuffer *buffer)
 
 int CIO2Device::start()
 {
-	return output_->streamOn();
+	int ret = output_->allocateBuffers(CIO2_BUFFER_COUNT, &buffers_);
+	if (ret < 0)
+		return ret;
+
+	for (std::unique_ptr<FrameBuffer> &buffer : buffers_)
+		availableBuffers_.push(buffer.get());
+
+	ret = output_->streamOn();
+	if (ret)
+		freeBuffers();
+
+	return ret;
 }
 
 int CIO2Device::stop()
 {
-	return output_->streamOff();
+	int ret = output_->streamOff();
+
+	freeBuffers();
+
+	return ret;
 }
 
 int CIO2Device::queueBuffer(FrameBuffer *buffer)
@@ -279,6 +262,17 @@  int CIO2Device::queueBuffer(FrameBuffer *buffer)
 	return output_->queueBuffer(buffer);
 }
 
+void CIO2Device::freeBuffers()
+{
+	/* The default std::queue constructor is explicit with gcc 5 and 6. */
+	availableBuffers_ = std::queue<FrameBuffer *>{};
+
+	buffers_.clear();
+
+	if (output_->releaseBuffers())
+		LOG(IPU3, Error) << "Failed to release CIO2 buffers";
+}
+
 void CIO2Device::cio2BufferReady(FrameBuffer *buffer)
 {
 	bufferReady.emit(buffer);
diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h
index 8cb90c9da7f19ecb..47c6f010eae6a64d 100644
--- a/src/libcamera/pipeline/ipu3/cio2.h
+++ b/src/libcamera/pipeline/ipu3/cio2.h
@@ -37,10 +37,8 @@  public:
 
 	StreamConfiguration generateConfiguration(Size size) const;
 
-	int allocateBuffers();
 	int exportBuffers(unsigned int count,
 			  std::vector<std::unique_ptr<FrameBuffer>> *buffers);
-	void freeBuffers();
 
 	FrameBuffer *getBuffer();
 	void putBuffer(FrameBuffer *buffer);
@@ -54,6 +52,8 @@  public:
 	Signal<FrameBuffer *> bufferReady;
 
 private:
+	void freeBuffers();
+
 	void cio2BufferReady(FrameBuffer *buffer);
 
 	CameraSensor *sensor_;
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index 2d1ec707ea4b08fe..4818027e8db1f7a3 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -658,15 +658,10 @@  int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,
 int PipelineHandlerIPU3::allocateBuffers(Camera *camera)
 {
 	IPU3CameraData *data = cameraData(camera);
-	CIO2Device *cio2 = &data->cio2_;
 	ImgUDevice *imgu = data->imgu_;
 	unsigned int bufferCount;
 	int ret;
 
-	ret = cio2->allocateBuffers();
-	if (ret < 0)
-		return ret;
-
 	bufferCount = std::max({
 		data->outStream_.configuration().bufferCount,
 		data->vfStream_.configuration().bufferCount,
@@ -674,10 +669,8 @@  int PipelineHandlerIPU3::allocateBuffers(Camera *camera)
 	});
 
 	ret = imgu->allocateBuffers(data, bufferCount);
-	if (ret < 0) {
-		cio2->freeBuffers();
+	if (ret < 0)
 		return ret;
-	}
 
 	return 0;
 }
@@ -686,7 +679,6 @@  int PipelineHandlerIPU3::freeBuffers(Camera *camera)
 {
 	IPU3CameraData *data = cameraData(camera);
 
-	data->cio2_.freeBuffers();
 	data->imgu_->freeBuffers(data);
 
 	return 0;
@@ -731,10 +723,10 @@  error:
 void PipelineHandlerIPU3::stop(Camera *camera)
 {
 	IPU3CameraData *data = cameraData(camera);
-	int ret;
+	int ret = 0;
 
-	ret = data->cio2_.stop();
 	ret |= data->imgu_->stop();
+	ret |= data->cio2_.stop();
 	if (ret)
 		LOG(IPU3, Warning) << "Failed to stop camera "
 				   << camera->name();