[libcamera-devel,05/11] libcamera: ipu3: cio2: Return the FrameBuffer pointer used
diff mbox series

Message ID 20201105001546.1690179-6-niklas.soderlund@ragnatech.se
State Superseded
Delegated to: Niklas Söderlund
Headers show
Series
  • libcamera: ipu3: Attach to an skeleton IPA
Related show

Commit Message

Niklas Söderlund Nov. 5, 2020, 12:15 a.m. UTC 
When adding IPA plumbing to the IPU3 pipeline handler it will be needed
to track witch raw buffer was queued to the CIO2 for a specific request.
Currently if using an internally allocated raw buffer the FrameBuffer is
not exposed outside the CIO2Device as it was not needed. Prepare for the
IPA by exposing which infernal raw buffer is picked for each request,
later changes will associate this with a Request.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 src/libcamera/pipeline/ipu3/cio2.cpp | 10 +++++++---
 src/libcamera/pipeline/ipu3/cio2.h   |  2 +-
 src/libcamera/pipeline/ipu3/ipu3.cpp |  8 ++++----
 3 files changed, 12 insertions(+), 8 deletions(-)

Comments

Kieran Bingham Nov. 5, 2020, 11:33 a.m. UTC | #1 
Hi Niklas,

On 05/11/2020 00:15, Niklas Söderlund wrote:
> When adding IPA plumbing to the IPU3 pipeline handler it will be needed
> to track witch raw buffer was queued to the CIO2 for a specific request.
> Currently if using an internally allocated raw buffer the FrameBuffer is
> not exposed outside the CIO2Device as it was not needed. Prepare for the
> IPA by exposing which infernal raw buffer is picked for each request,
> later changes will associate this with a Request.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  src/libcamera/pipeline/ipu3/cio2.cpp | 10 +++++++---
>  src/libcamera/pipeline/ipu3/cio2.h   |  2 +-
>  src/libcamera/pipeline/ipu3/ipu3.cpp |  8 ++++----
>  3 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> index e43ec70fe3e4e0db..1c7b9676f52abdb6 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> @@ -262,7 +262,7 @@ int CIO2Device::stop()
>  	return ret;
>  }
>  
> -int CIO2Device::queueBuffer(Request *request, FrameBuffer *rawBuffer)
> +FrameBuffer *CIO2Device::queueBuffer(Request *request, FrameBuffer *rawBuffer)
>  {
>  	FrameBuffer *buffer = rawBuffer;
>  
> @@ -270,7 +270,7 @@ int CIO2Device::queueBuffer(Request *request, FrameBuffer *rawBuffer)
>  	if (!buffer) {
>  		if (availableBuffers_.empty()) {
>  			LOG(IPU3, Error) << "CIO2 buffer underrun";
> -			return -EINVAL;
> +			return nullptr;
>  		}
>  
>  		buffer = availableBuffers_.front();
> @@ -279,7 +279,11 @@ int CIO2Device::queueBuffer(Request *request, FrameBuffer *rawBuffer)
>  
>  	buffer->setRequest(request);
>  
> -	return output_->queueBuffer(buffer);
> +	int ret = output_->queueBuffer(buffer);
> +	if (ret)
> +		return nullptr;
> +
> +	return buffer;
>  }
>  
>  void CIO2Device::tryReturnBuffer(FrameBuffer *buffer)
> diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h
> index fa813a989fd21aec..e8b491a0c104a3e2 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.h
> +++ b/src/libcamera/pipeline/ipu3/cio2.h
> @@ -52,7 +52,7 @@ public:
>  	CameraSensor *sensor() { return sensor_; }
>  	const CameraSensor *sensor() const { return sensor_; }
>  
> -	int queueBuffer(Request *request, FrameBuffer *rawBuffer);
> +	FrameBuffer *queueBuffer(Request *request, FrameBuffer *rawBuffer);
>  	void tryReturnBuffer(FrameBuffer *buffer);
>  	Signal<FrameBuffer *> &bufferReady() { return output_->bufferReady; }
>  
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index c559d160084f87e7..62e99a308a6136a7 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -643,10 +643,10 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)
>  	 * Queue a buffer on the CIO2, using the raw stream buffer provided in
>  	 * the request, if any, or a CIO2 internal buffer otherwise.
>  	 */
> -	FrameBuffer *rawBuffer = request->findBuffer(&data->rawStream_);
> -	error = data->cio2_.queueBuffer(request, rawBuffer);
> -	if (error)
> -		return error;
> +	FrameBuffer *reqRawBuffer = request->findBuffer(&data->rawStream_);
> +	FrameBuffer *rawBuffer = data->cio2_.queueBuffer(request, reqRawBuffer);
> +	if (!rawBuffer)
> +		return -ENOMEM;

It feels a bit opaque or odd to return a buffer from queueBuffer, but
given that one of the parameters is 'a raw buffer to use', returning
'which one actaully got used' doesn't feel that bad.

It just seems a bit odd that's all ... but I haven't seen how it's all
used yet.

I'll come back to this one I guess ;-)



>  
>  	/* Queue all buffers from the request aimed for the ImgU. */
>  	for (auto it : request->buffers()) {
>
Jacopo Mondi Nov. 9, 2020, 10:54 a.m. UTC | #2 
Hi Niklas,

On Thu, Nov 05, 2020 at 01:15:40AM +0100, Niklas Söderlund wrote:
> When adding IPA plumbing to the IPU3 pipeline handler it will be needed
> to track witch raw buffer was queued to the CIO2 for a specific request.

s/witch/which

> Currently if using an internally allocated raw buffer the FrameBuffer is
> not exposed outside the CIO2Device as it was not needed. Prepare for the
> IPA by exposing which infernal raw buffer is picked for each request,

s/infernal/internal

With 'witch raw buffers' and 'infernal raw buffers' I'm sure libcamera
would gain much more esoteric super-powers. I like it!

> later changes will associate this with a Request.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  src/libcamera/pipeline/ipu3/cio2.cpp | 10 +++++++---
>  src/libcamera/pipeline/ipu3/cio2.h   |  2 +-
>  src/libcamera/pipeline/ipu3/ipu3.cpp |  8 ++++----
>  3 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> index e43ec70fe3e4e0db..1c7b9676f52abdb6 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> @@ -262,7 +262,7 @@ int CIO2Device::stop()
>  	return ret;
>  }
>
> -int CIO2Device::queueBuffer(Request *request, FrameBuffer *rawBuffer)
> +FrameBuffer *CIO2Device::queueBuffer(Request *request, FrameBuffer *rawBuffer)
>  {
>  	FrameBuffer *buffer = rawBuffer;
>
> @@ -270,7 +270,7 @@ int CIO2Device::queueBuffer(Request *request, FrameBuffer *rawBuffer)
>  	if (!buffer) {
>  		if (availableBuffers_.empty()) {
>  			LOG(IPU3, Error) << "CIO2 buffer underrun";
> -			return -EINVAL;
> +			return nullptr;
>  		}
>
>  		buffer = availableBuffers_.front();
> @@ -279,7 +279,11 @@ int CIO2Device::queueBuffer(Request *request, FrameBuffer *rawBuffer)
>
>  	buffer->setRequest(request);
>
> -	return output_->queueBuffer(buffer);
> +	int ret = output_->queueBuffer(buffer);
> +	if (ret)
> +		return nullptr;
> +
> +	return buffer;
>  }
>
>  void CIO2Device::tryReturnBuffer(FrameBuffer *buffer)
> diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h
> index fa813a989fd21aec..e8b491a0c104a3e2 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.h
> +++ b/src/libcamera/pipeline/ipu3/cio2.h
> @@ -52,7 +52,7 @@ public:
>  	CameraSensor *sensor() { return sensor_; }
>  	const CameraSensor *sensor() const { return sensor_; }
>
> -	int queueBuffer(Request *request, FrameBuffer *rawBuffer);
> +	FrameBuffer *queueBuffer(Request *request, FrameBuffer *rawBuffer);
>  	void tryReturnBuffer(FrameBuffer *buffer);
>  	Signal<FrameBuffer *> &bufferReady() { return output_->bufferReady; }
>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index c559d160084f87e7..62e99a308a6136a7 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -643,10 +643,10 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)
>  	 * Queue a buffer on the CIO2, using the raw stream buffer provided in
>  	 * the request, if any, or a CIO2 internal buffer otherwise.
>  	 */
> -	FrameBuffer *rawBuffer = request->findBuffer(&data->rawStream_);
> -	error = data->cio2_.queueBuffer(request, rawBuffer);
> -	if (error)
> -		return error;
> +	FrameBuffer *reqRawBuffer = request->findBuffer(&data->rawStream_);
> +	FrameBuffer *rawBuffer = data->cio2_.queueBuffer(request, reqRawBuffer);

I wonder if we should not better overload CIO2Device::queueBuffer(),
one version that accepts an externally provided buffer, and one that
uses an internal one. We can't overload on return value, so they
should be named differently, but the code here would look like:

        FrameBuffer *rawBuffer = request->findBuffer(&data->rawStream_);
        int ret;
        if (!rawBuffer) {
                rawBuffer = data->cio2_.queueInternalBuffer(request);
                ret = !!rawBuffer;
        } else {
                ret = data->cio2_.queueInternalBuffer(request, rawBuffer);
        }
        if (ret)
                return -ENOMEM;

It's less compact but more explicit. Up to you, what you have here is
good as well

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j

> +	if (!rawBuffer)
> +		return -ENOMEM;
>
>  	/* Queue all buffers from the request aimed for the ImgU. */
>  	for (auto it : request->buffers()) {
> --
> 2.29.2
>
> _______________________________________________
> 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/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
index e43ec70fe3e4e0db..1c7b9676f52abdb6 100644
--- a/src/libcamera/pipeline/ipu3/cio2.cpp
+++ b/src/libcamera/pipeline/ipu3/cio2.cpp
@@ -262,7 +262,7 @@  int CIO2Device::stop()
 	return ret;
 }
 
-int CIO2Device::queueBuffer(Request *request, FrameBuffer *rawBuffer)
+FrameBuffer *CIO2Device::queueBuffer(Request *request, FrameBuffer *rawBuffer)
 {
 	FrameBuffer *buffer = rawBuffer;
 
@@ -270,7 +270,7 @@  int CIO2Device::queueBuffer(Request *request, FrameBuffer *rawBuffer)
 	if (!buffer) {
 		if (availableBuffers_.empty()) {
 			LOG(IPU3, Error) << "CIO2 buffer underrun";
-			return -EINVAL;
+			return nullptr;
 		}
 
 		buffer = availableBuffers_.front();
@@ -279,7 +279,11 @@  int CIO2Device::queueBuffer(Request *request, FrameBuffer *rawBuffer)
 
 	buffer->setRequest(request);
 
-	return output_->queueBuffer(buffer);
+	int ret = output_->queueBuffer(buffer);
+	if (ret)
+		return nullptr;
+
+	return buffer;
 }
 
 void CIO2Device::tryReturnBuffer(FrameBuffer *buffer)
diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h
index fa813a989fd21aec..e8b491a0c104a3e2 100644
--- a/src/libcamera/pipeline/ipu3/cio2.h
+++ b/src/libcamera/pipeline/ipu3/cio2.h
@@ -52,7 +52,7 @@  public:
 	CameraSensor *sensor() { return sensor_; }
 	const CameraSensor *sensor() const { return sensor_; }
 
-	int queueBuffer(Request *request, FrameBuffer *rawBuffer);
+	FrameBuffer *queueBuffer(Request *request, FrameBuffer *rawBuffer);
 	void tryReturnBuffer(FrameBuffer *buffer);
 	Signal<FrameBuffer *> &bufferReady() { return output_->bufferReady; }
 
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index c559d160084f87e7..62e99a308a6136a7 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -643,10 +643,10 @@  int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)
 	 * Queue a buffer on the CIO2, using the raw stream buffer provided in
 	 * the request, if any, or a CIO2 internal buffer otherwise.
 	 */
-	FrameBuffer *rawBuffer = request->findBuffer(&data->rawStream_);
-	error = data->cio2_.queueBuffer(request, rawBuffer);
-	if (error)
-		return error;
+	FrameBuffer *reqRawBuffer = request->findBuffer(&data->rawStream_);
+	FrameBuffer *rawBuffer = data->cio2_.queueBuffer(request, reqRawBuffer);
+	if (!rawBuffer)
+		return -ENOMEM;
 
 	/* Queue all buffers from the request aimed for the ImgU. */
 	for (auto it : request->buffers()) {