[libcamera-devel] libcamera: ipu3: Get request from information structure instead of buffer
diff mbox series

Message ID 20210205204303.394105-1-niklas.soderlund@ragnatech.se
State Accepted
Headers show
Series
  • [libcamera-devel] libcamera: ipu3: Get request from information structure instead of buffer
Related show

Commit Message

Niklas Söderlund Feb. 5, 2021, 8:43 p.m. UTC 
Buffers dequeued from the ImgU may not be associated with a Request as
they are internal (statistics, parameters and some RAW buffers). Fetch
the request they are used for from the frame information instead.

Fixes: 9708f49fecf2f9ee ("libcamera: ipu3: Share parameter and statistic buffers with IPA")
Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 src/libcamera/pipeline/ipu3/ipu3.cpp | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

Comments

Kieran Bingham Feb. 8, 2021, 1:43 p.m. UTC | #1 
Hi Niklas,

On 05/02/2021 20:43, Niklas Söderlund wrote:
> Buffers dequeued from the ImgU may not be associated with a Request as
> they are internal (statistics, parameters and some RAW buffers). Fetch
> the request they are used for from the frame information instead.
> 
> Fixes: 9708f49fecf2f9ee ("libcamera: ipu3: Share parameter and statistic buffers with IPA")

So far I've done quite a few start/stops with this patch and no failures*

*except for one, very early on which I therefore currently attribute to
probably not testing the updated code.

So at least for now a tentative:

Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

And the change seems to make sense so:

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>



> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index c3763507c40a5d53..48f0a81f0634d424 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -1109,14 +1109,14 @@ void IPU3CameraData::queueFrameAction(unsigned int id,
>   */
>  void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)
>  {
> -	Request *request = buffer->request();
> +	IPU3Frames::Info *info = frameInfos_.find(buffer);
> +	if (!info)
> +		return;
> +
> +	Request *request = info->request;
>  
>  	pipe_->completeBuffer(request, buffer);
>  
> -	IPU3Frames::Info *info = frameInfos_.find(buffer);
> -	if (!info)
> -		return;
> -
>  	request->metadata().set(controls::draft::PipelineDepth, 3);
>  	/* \todo Move the ExposureTime control to the IPA. */
>  	request->metadata().set(controls::ExposureTime, exposureTime_);
> @@ -1146,7 +1146,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)
>  	if (!info)
>  		return;
>  
> -	Request *request = buffer->request();
> +	Request *request = info->request;
>  
>  	/* If the buffer is cancelled force a complete of the whole request. */
>  	if (buffer->metadata().status == FrameMetadata::FrameCancelled) {
> @@ -1175,7 +1175,7 @@ void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)
>  
>  	info->paramDequeued = true;
>  	if (frameInfos_.tryComplete(info))
> -		pipe_->completeRequest(buffer->request());
> +		pipe_->completeRequest(info->request);
>  }
>  
>  void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
> @@ -1187,7 +1187,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
>  	if (buffer->metadata().status == FrameMetadata::FrameCancelled) {
>  		info->metadataProcessed = true;
>  		if (frameInfos_.tryComplete(info))
> -			pipe_->completeRequest(buffer->request());
> +			pipe_->completeRequest(info->request);
>  		return;
>  	}
>  
>
Laurent Pinchart Feb. 8, 2021, 3:23 p.m. UTC | #2 
Hi Niklas,

Thank you for the patch.

On Fri, Feb 05, 2021 at 09:43:03PM +0100, Niklas Söderlund wrote:
> Buffers dequeued from the ImgU may not be associated with a Request as
> they are internal (statistics, parameters and some RAW buffers). Fetch
> the request they are used for from the frame information instead.
> 
> Fixes: 9708f49fecf2f9ee ("libcamera: ipu3: Share parameter and statistic buffers with IPA")
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index c3763507c40a5d53..48f0a81f0634d424 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -1109,14 +1109,14 @@ void IPU3CameraData::queueFrameAction(unsigned int id,
>   */
>  void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)
>  {
> -	Request *request = buffer->request();
> +	IPU3Frames::Info *info = frameInfos_.find(buffer);
> +	if (!info)
> +		return;
> +
> +	Request *request = info->request;
>  
>  	pipe_->completeBuffer(request, buffer);
>  
> -	IPU3Frames::Info *info = frameInfos_.find(buffer);
> -	if (!info)
> -		return;
> -

Do I understand correctly that this change isn't strictly required since
the ImgU output buffers are always part of a request ? Still, it's good
to keep the implementation consistent.

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

>  	request->metadata().set(controls::draft::PipelineDepth, 3);
>  	/* \todo Move the ExposureTime control to the IPA. */
>  	request->metadata().set(controls::ExposureTime, exposureTime_);
> @@ -1146,7 +1146,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)
>  	if (!info)
>  		return;
>  
> -	Request *request = buffer->request();
> +	Request *request = info->request;
>  
>  	/* If the buffer is cancelled force a complete of the whole request. */
>  	if (buffer->metadata().status == FrameMetadata::FrameCancelled) {
> @@ -1175,7 +1175,7 @@ void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)
>  
>  	info->paramDequeued = true;
>  	if (frameInfos_.tryComplete(info))
> -		pipe_->completeRequest(buffer->request());
> +		pipe_->completeRequest(info->request);
>  }
>  
>  void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
> @@ -1187,7 +1187,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
>  	if (buffer->metadata().status == FrameMetadata::FrameCancelled) {
>  		info->metadataProcessed = true;
>  		if (frameInfos_.tryComplete(info))
> -			pipe_->completeRequest(buffer->request());
> +			pipe_->completeRequest(info->request);
>  		return;
>  	}
>
Niklas Söderlund Feb. 8, 2021, 7:13 p.m. UTC | #3 
Hi Laurent,

Thanks for your feedback.

On 2021-02-08 17:23:40 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Fri, Feb 05, 2021 at 09:43:03PM +0100, Niklas Söderlund wrote:
> > Buffers dequeued from the ImgU may not be associated with a Request as
> > they are internal (statistics, parameters and some RAW buffers). Fetch
> > the request they are used for from the frame information instead.
> > 
> > Fixes: 9708f49fecf2f9ee ("libcamera: ipu3: Share parameter and statistic buffers with IPA")
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 16 ++++++++--------
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> > 
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index c3763507c40a5d53..48f0a81f0634d424 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -1109,14 +1109,14 @@ void IPU3CameraData::queueFrameAction(unsigned int id,
> >   */
> >  void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)
> >  {
> > -	Request *request = buffer->request();
> > +	IPU3Frames::Info *info = frameInfos_.find(buffer);
> > +	if (!info)
> > +		return;
> > +
> > +	Request *request = info->request;
> >  
> >  	pipe_->completeBuffer(request, buffer);
> >  
> > -	IPU3Frames::Info *info = frameInfos_.find(buffer);
> > -	if (!info)
> > -		return;
> > -
> 
> Do I understand correctly that this change isn't strictly required since
> the ImgU output buffers are always part of a request ? Still, it's good
> to keep the implementation consistent.

Yes it's only to keep the implementation consistent ad is not strictly 
needed for the output buffers.

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

Thanks!

> 
> >  	request->metadata().set(controls::draft::PipelineDepth, 3);
> >  	/* \todo Move the ExposureTime control to the IPA. */
> >  	request->metadata().set(controls::ExposureTime, exposureTime_);
> > @@ -1146,7 +1146,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)
> >  	if (!info)
> >  		return;
> >  
> > -	Request *request = buffer->request();
> > +	Request *request = info->request;
> >  
> >  	/* If the buffer is cancelled force a complete of the whole request. */
> >  	if (buffer->metadata().status == FrameMetadata::FrameCancelled) {
> > @@ -1175,7 +1175,7 @@ void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)
> >  
> >  	info->paramDequeued = true;
> >  	if (frameInfos_.tryComplete(info))
> > -		pipe_->completeRequest(buffer->request());
> > +		pipe_->completeRequest(info->request);
> >  }
> >  
> >  void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
> > @@ -1187,7 +1187,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
> >  	if (buffer->metadata().status == FrameMetadata::FrameCancelled) {
> >  		info->metadataProcessed = true;
> >  		if (frameInfos_.tryComplete(info))
> > -			pipe_->completeRequest(buffer->request());
> > +			pipe_->completeRequest(info->request);
> >  		return;
> >  	}
> >  
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index c3763507c40a5d53..48f0a81f0634d424 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -1109,14 +1109,14 @@  void IPU3CameraData::queueFrameAction(unsigned int id,
  */
 void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)
 {
-	Request *request = buffer->request();
+	IPU3Frames::Info *info = frameInfos_.find(buffer);
+	if (!info)
+		return;
+
+	Request *request = info->request;
 
 	pipe_->completeBuffer(request, buffer);
 
-	IPU3Frames::Info *info = frameInfos_.find(buffer);
-	if (!info)
-		return;
-
 	request->metadata().set(controls::draft::PipelineDepth, 3);
 	/* \todo Move the ExposureTime control to the IPA. */
 	request->metadata().set(controls::ExposureTime, exposureTime_);
@@ -1146,7 +1146,7 @@  void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)
 	if (!info)
 		return;
 
-	Request *request = buffer->request();
+	Request *request = info->request;
 
 	/* If the buffer is cancelled force a complete of the whole request. */
 	if (buffer->metadata().status == FrameMetadata::FrameCancelled) {
@@ -1175,7 +1175,7 @@  void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)
 
 	info->paramDequeued = true;
 	if (frameInfos_.tryComplete(info))
-		pipe_->completeRequest(buffer->request());
+		pipe_->completeRequest(info->request);
 }
 
 void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
@@ -1187,7 +1187,7 @@  void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
 	if (buffer->metadata().status == FrameMetadata::FrameCancelled) {
 		info->metadataProcessed = true;
 		if (frameInfos_.tryComplete(info))
-			pipe_->completeRequest(buffer->request());
+			pipe_->completeRequest(info->request);
 		return;
 	}