[v2,2/5] libcamera: software_isp: Handle queued output buffers on stop
diff mbox series

Message ID 20250224185235.43381-3-mzamazal@redhat.com
State Changes Requested
Headers show
Series
  • Fix occasional software ISP assertion error on stop
Related show

Commit Message

Milan Zamazal Feb. 24, 2025, 6:52 p.m. UTC 
When SoftwareIsp stops, input and output buffers queued to it may not
yet be fully processed.  They will be eventually returned but stop means
stop, there should be no processing related actions invoked afterwards.

Let's stop forwarding processed output buffers from the SoftwareIsp
slots once SoftwareIsp is stopped.  Let's track the queued output
buffers and mark those still pending as canceled in SoftwareIsp::stop
and return them to the pipeline handler.

Dealing with input buffers is addressed in a separate patch.

Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
---
 .../internal/software_isp/software_isp.h      |  1 +
 src/libcamera/software_isp/software_isp.cpp   | 22 ++++++++++++++++---
 2 files changed, 20 insertions(+), 3 deletions(-)

Comments

Laurent Pinchart Feb. 24, 2025, 7:49 p.m. UTC | #1 
Hi Milan,

Thank you for the patch.

On Mon, Feb 24, 2025 at 07:52:32PM +0100, Milan Zamazal wrote:
> When SoftwareIsp stops, input and output buffers queued to it may not
> yet be fully processed.  They will be eventually returned but stop means
> stop, there should be no processing related actions invoked afterwards.
> 
> Let's stop forwarding processed output buffers from the SoftwareIsp
> slots once SoftwareIsp is stopped.  Let's track the queued output
> buffers and mark those still pending as canceled in SoftwareIsp::stop
> and return them to the pipeline handler.
> 
> Dealing with input buffers is addressed in a separate patch.
> 
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>  .../internal/software_isp/software_isp.h      |  1 +
>  src/libcamera/software_isp/software_isp.cpp   | 22 ++++++++++++++++---
>  2 files changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h
> index af0dcc24..f2344355 100644
> --- a/include/libcamera/internal/software_isp/software_isp.h
> +++ b/include/libcamera/internal/software_isp/software_isp.h
> @@ -101,6 +101,7 @@ private:
>  
>  	std::unique_ptr<ipa::soft::IPAProxySoft> ipa_;
>  	bool running_;
> +	std::deque<FrameBuffer *> queuedOutputBuffers_;

#include <deque>

>  };
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
> index 1a39f4d8..4339e547 100644
> --- a/src/libcamera/software_isp/software_isp.cpp
> +++ b/src/libcamera/software_isp/software_isp.cpp
> @@ -17,6 +17,7 @@
>  #include <libcamera/formats.h>
>  #include <libcamera/stream.h>
>  
> +#include "libcamera/internal/framebuffer.h"
>  #include "libcamera/internal/ipa_manager.h"
>  #include "libcamera/internal/software_isp/debayer_params.h"
>  
> @@ -300,8 +301,11 @@ int SoftwareIsp::queueBuffers(uint32_t frame, FrameBuffer *input,
>  			return -EINVAL;
>  	}
>  
> -	for (auto iter = outputs.begin(); iter != outputs.end(); iter++)
> -		process(frame, input, iter->second);
> +	for (auto iter = outputs.begin(); iter != outputs.end(); iter++) {
> +		FrameBuffer *const buffer = iter->second;
> +		queuedOutputBuffers_.push_back(buffer);
> +		process(frame, input, buffer);
> +	}
>  
>  	return 0;
>  }
> @@ -331,6 +335,13 @@ void SoftwareIsp::stop()
>  
>  	running_ = false;
>  	ipa_->stop();
> +
> +	for (auto buffer : queuedOutputBuffers_) {
> +		FrameMetadata &metadata = buffer->_d()->metadata();
> +		metadata.status = FrameMetadata::FrameCancelled;
> +		outputBufferReady.emit(buffer);
> +	}
> +	queuedOutputBuffers_.clear();
>  }
>  
>  /**
> @@ -369,7 +380,12 @@ void SoftwareIsp::inputReady(FrameBuffer *input)
>  
>  void SoftwareIsp::outputReady(FrameBuffer *output)
>  {
> -	outputBufferReady.emit(output);
> +	if (running_) {
> +		queuedOutputBuffers_.erase(find(queuedOutputBuffers_.begin(),

s/find/std::find/

and #include <algorithm>.

This being said, the software ISP always processes buffers in order. Do
we need to call find(), can't we just call pop_front() with an assertion
to ensure the element matches ?

> +						queuedOutputBuffers_.end(),
> +						output));
> +		outputBufferReady.emit(output);
> +	}
>  }
>  
>  } /* namespace libcamera */
Milan Zamazal Feb. 24, 2025, 8:50 p.m. UTC | #2 
Hi Laurent,

thank you for review.

Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:

> Hi Milan,
>
> Thank you for the patch.
>
> On Mon, Feb 24, 2025 at 07:52:32PM +0100, Milan Zamazal wrote:
>> When SoftwareIsp stops, input and output buffers queued to it may not
>> yet be fully processed.  They will be eventually returned but stop means
>> stop, there should be no processing related actions invoked afterwards.
>> 
>> Let's stop forwarding processed output buffers from the SoftwareIsp
>> slots once SoftwareIsp is stopped.  Let's track the queued output
>> buffers and mark those still pending as canceled in SoftwareIsp::stop
>> and return them to the pipeline handler.
>> 
>> Dealing with input buffers is addressed in a separate patch.
>> 
>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>> ---
>>  .../internal/software_isp/software_isp.h      |  1 +
>>  src/libcamera/software_isp/software_isp.cpp   | 22 ++++++++++++++++---
>>  2 files changed, 20 insertions(+), 3 deletions(-)
>> 
>> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h
>> index af0dcc24..f2344355 100644
>> --- a/include/libcamera/internal/software_isp/software_isp.h
>> +++ b/include/libcamera/internal/software_isp/software_isp.h
>> @@ -101,6 +101,7 @@ private:
>>  
>>  	std::unique_ptr<ipa::soft::IPAProxySoft> ipa_;
>>  	bool running_;
>> +	std::deque<FrameBuffer *> queuedOutputBuffers_;
>
> #include <deque>
>
>>  };
>>  
>>  } /* namespace libcamera */
>> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
>> index 1a39f4d8..4339e547 100644
>> --- a/src/libcamera/software_isp/software_isp.cpp
>> +++ b/src/libcamera/software_isp/software_isp.cpp
>> @@ -17,6 +17,7 @@
>>  #include <libcamera/formats.h>
>>  #include <libcamera/stream.h>
>>  
>> +#include "libcamera/internal/framebuffer.h"
>>  #include "libcamera/internal/ipa_manager.h"
>>  #include "libcamera/internal/software_isp/debayer_params.h"
>>  
>> @@ -300,8 +301,11 @@ int SoftwareIsp::queueBuffers(uint32_t frame, FrameBuffer *input,
>>  			return -EINVAL;
>>  	}
>>  
>> -	for (auto iter = outputs.begin(); iter != outputs.end(); iter++)
>> -		process(frame, input, iter->second);
>> +	for (auto iter = outputs.begin(); iter != outputs.end(); iter++) {
>> +		FrameBuffer *const buffer = iter->second;
>> +		queuedOutputBuffers_.push_back(buffer);
>> +		process(frame, input, buffer);
>> +	}
>>  
>>  	return 0;
>>  }
>> @@ -331,6 +335,13 @@ void SoftwareIsp::stop()
>>  
>>  	running_ = false;
>>  	ipa_->stop();
>> +
>> +	for (auto buffer : queuedOutputBuffers_) {
>> +		FrameMetadata &metadata = buffer->_d()->metadata();
>> +		metadata.status = FrameMetadata::FrameCancelled;
>> +		outputBufferReady.emit(buffer);
>> +	}
>> +	queuedOutputBuffers_.clear();
>>  }
>>  
>>  /**
>> @@ -369,7 +380,12 @@ void SoftwareIsp::inputReady(FrameBuffer *input)
>>  
>>  void SoftwareIsp::outputReady(FrameBuffer *output)
>>  {
>> -	outputBufferReady.emit(output);
>> +	if (running_) {
>> +		queuedOutputBuffers_.erase(find(queuedOutputBuffers_.begin(),
>
> s/find/std::find/
>
> and #include <algorithm>.
>
> This being said, the software ISP always processes buffers in order. Do
> we need to call find(), can't we just call pop_front() with an assertion
> to ensure the element matches ?

I can't see any contradiction to this and it works for me so I'll do it
this way in v3.

>> +						queuedOutputBuffers_.end(),
>> +						output));
>> +		outputBufferReady.emit(output);
>> +	}
>>  }
>>  
>>  } /* namespace libcamera */

Patch
diff mbox series 

diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h
index af0dcc24..f2344355 100644
--- a/include/libcamera/internal/software_isp/software_isp.h
+++ b/include/libcamera/internal/software_isp/software_isp.h
@@ -101,6 +101,7 @@  private:
 
 	std::unique_ptr<ipa::soft::IPAProxySoft> ipa_;
 	bool running_;
+	std::deque<FrameBuffer *> queuedOutputBuffers_;
 };
 
 } /* namespace libcamera */
diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
index 1a39f4d8..4339e547 100644
--- a/src/libcamera/software_isp/software_isp.cpp
+++ b/src/libcamera/software_isp/software_isp.cpp
@@ -17,6 +17,7 @@ 
 #include <libcamera/formats.h>
 #include <libcamera/stream.h>
 
+#include "libcamera/internal/framebuffer.h"
 #include "libcamera/internal/ipa_manager.h"
 #include "libcamera/internal/software_isp/debayer_params.h"
 
@@ -300,8 +301,11 @@  int SoftwareIsp::queueBuffers(uint32_t frame, FrameBuffer *input,
 			return -EINVAL;
 	}
 
-	for (auto iter = outputs.begin(); iter != outputs.end(); iter++)
-		process(frame, input, iter->second);
+	for (auto iter = outputs.begin(); iter != outputs.end(); iter++) {
+		FrameBuffer *const buffer = iter->second;
+		queuedOutputBuffers_.push_back(buffer);
+		process(frame, input, buffer);
+	}
 
 	return 0;
 }
@@ -331,6 +335,13 @@  void SoftwareIsp::stop()
 
 	running_ = false;
 	ipa_->stop();
+
+	for (auto buffer : queuedOutputBuffers_) {
+		FrameMetadata &metadata = buffer->_d()->metadata();
+		metadata.status = FrameMetadata::FrameCancelled;
+		outputBufferReady.emit(buffer);
+	}
+	queuedOutputBuffers_.clear();
 }
 
 /**
@@ -369,7 +380,12 @@  void SoftwareIsp::inputReady(FrameBuffer *input)
 
 void SoftwareIsp::outputReady(FrameBuffer *output)
 {
-	outputBufferReady.emit(output);
+	if (running_) {
+		queuedOutputBuffers_.erase(find(queuedOutputBuffers_.begin(),
+						queuedOutputBuffers_.end(),
+						output));
+		outputBufferReady.emit(output);
+	}
 }
 
 } /* namespace libcamera */