[v3,3/6] libcamera: software_isp: Handle queued input buffers on stop
diff mbox series

Message ID 20250225150614.20195-4-mzamazal@redhat.com
State Accepted
Headers show
Series
  • Fix occasional software ISP assertion error on stop
Related show

Commit Message

Milan Zamazal Feb. 25, 2025, 3:06 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 input buffers from SoftwareIsp slots
when SoftwareIsp is stopped.  Let's track the queued input buffers and
return them back for capture in SoftwareIsp::stop().

The returned input buffers are marked as canceled.  This is not
necessary at the moment but it gives the pipeline handlers chance to
deal with this if they need to.

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

Comments

Laurent Pinchart Feb. 26, 2025, 12:03 a.m. UTC | #1 
Hi Milan,

Thank you for the patch.

On Tue, Feb 25, 2025 at 04:06:09PM +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 input buffers from SoftwareIsp slots
> when SoftwareIsp is stopped.  Let's track the queued input buffers and
> return them back for capture in SoftwareIsp::stop().
> 
> The returned input buffers are marked as canceled.  This is not
> necessary at the moment but it gives the pipeline handlers chance to
> deal with this if they need to.
> 
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>  .../internal/software_isp/software_isp.h       |  1 +
>  src/libcamera/software_isp/software_isp.cpp    | 18 +++++++++++++++++-
>  2 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h
> index 5073ce7a..400a4dc5 100644
> --- a/include/libcamera/internal/software_isp/software_isp.h
> +++ b/include/libcamera/internal/software_isp/software_isp.h
> @@ -102,6 +102,7 @@ private:
>  
>  	std::unique_ptr<ipa::soft::IPAProxySoft> ipa_;
>  	bool running_;
> +	std::deque<FrameBuffer *> queuedInputBuffers_;
>  	std::deque<FrameBuffer *> queuedOutputBuffers_;
>  };
>  
> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
> index 140cddf3..beac66fc 100644
> --- a/src/libcamera/software_isp/software_isp.cpp
> +++ b/src/libcamera/software_isp/software_isp.cpp
> @@ -303,6 +303,8 @@ int SoftwareIsp::queueBuffers(uint32_t frame, FrameBuffer *input,
>  			return -EINVAL;
>  	}
>  
> +	queuedInputBuffers_.push_back(input);
> +
>  	for (auto iter = outputs.begin(); iter != outputs.end(); iter++) {
>  		FrameBuffer *const buffer = iter->second;
>  		queuedOutputBuffers_.push_back(buffer);
> @@ -329,6 +331,9 @@ int SoftwareIsp::start()
>  
>  /**
>   * \brief Stops the Software ISP streaming operation
> + *
> + * All pending buffers are returned back as canceled before this method
> + * finishes.

s/emthod finishes/function returns/

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

>   */
>  void SoftwareIsp::stop()
>  {
> @@ -344,6 +349,13 @@ void SoftwareIsp::stop()
>  		outputBufferReady.emit(buffer);
>  	}
>  	queuedOutputBuffers_.clear();
> +
> +	for (auto buffer : queuedInputBuffers_) {
> +		FrameMetadata &metadata = buffer->_d()->metadata();
> +		metadata.status = FrameMetadata::FrameCancelled;
> +		inputBufferReady.emit(buffer);
> +	}
> +	queuedInputBuffers_.clear();
>  }
>  
>  /**
> @@ -377,7 +389,11 @@ void SoftwareIsp::statsReady(uint32_t frame, uint32_t bufferId)
>  
>  void SoftwareIsp::inputReady(FrameBuffer *input)
>  {
> -	inputBufferReady.emit(input);
> +	if (running_) {
> +		ASSERT(queuedInputBuffers_.front() == input);
> +		queuedInputBuffers_.pop_front();
> +		inputBufferReady.emit(input);
> +	}
>  }
>  
>  void SoftwareIsp::outputReady(FrameBuffer *output)
Kieran Bingham March 1, 2025, 11:04 p.m. UTC | #2 
Quoting Laurent Pinchart (2025-02-26 00:03:47)
> Hi Milan,
> 
> Thank you for the patch.
> 
> On Tue, Feb 25, 2025 at 04:06:09PM +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 input buffers from SoftwareIsp slots
> > when SoftwareIsp is stopped.  Let's track the queued input buffers and
> > return them back for capture in SoftwareIsp::stop().
> > 
> > The returned input buffers are marked as canceled.  This is not

s/canceled/cancelled/

> > necessary at the moment but it gives the pipeline handlers chance to
> > deal with this if they need to.
> > 
> > Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> > ---
> >  .../internal/software_isp/software_isp.h       |  1 +
> >  src/libcamera/software_isp/software_isp.cpp    | 18 +++++++++++++++++-
> >  2 files changed, 18 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h
> > index 5073ce7a..400a4dc5 100644
> > --- a/include/libcamera/internal/software_isp/software_isp.h
> > +++ b/include/libcamera/internal/software_isp/software_isp.h
> > @@ -102,6 +102,7 @@ private:
> >  
> >       std::unique_ptr<ipa::soft::IPAProxySoft> ipa_;
> >       bool running_;
> > +     std::deque<FrameBuffer *> queuedInputBuffers_;
> >       std::deque<FrameBuffer *> queuedOutputBuffers_;
> >  };
> >  
> > diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
> > index 140cddf3..beac66fc 100644
> > --- a/src/libcamera/software_isp/software_isp.cpp
> > +++ b/src/libcamera/software_isp/software_isp.cpp
> > @@ -303,6 +303,8 @@ int SoftwareIsp::queueBuffers(uint32_t frame, FrameBuffer *input,
> >                       return -EINVAL;
> >       }
> >  
> > +     queuedInputBuffers_.push_back(input);
> > +
> >       for (auto iter = outputs.begin(); iter != outputs.end(); iter++) {
> >               FrameBuffer *const buffer = iter->second;
> >               queuedOutputBuffers_.push_back(buffer);
> > @@ -329,6 +331,9 @@ int SoftwareIsp::start()
> >  
> >  /**
> >   * \brief Stops the Software ISP streaming operation
> > + *
> > + * All pending buffers are returned back as canceled before this method
> > + * finishes.
> 
> s/emthod finishes/function returns/
> 

Will fix.


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

> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> >   */
> >  void SoftwareIsp::stop()
> >  {
> > @@ -344,6 +349,13 @@ void SoftwareIsp::stop()
> >               outputBufferReady.emit(buffer);
> >       }
> >       queuedOutputBuffers_.clear();
> > +
> > +     for (auto buffer : queuedInputBuffers_) {
> > +             FrameMetadata &metadata = buffer->_d()->metadata();
> > +             metadata.status = FrameMetadata::FrameCancelled;
> > +             inputBufferReady.emit(buffer);
> > +     }
> > +     queuedInputBuffers_.clear();
> >  }
> >  
> >  /**
> > @@ -377,7 +389,11 @@ void SoftwareIsp::statsReady(uint32_t frame, uint32_t bufferId)
> >  
> >  void SoftwareIsp::inputReady(FrameBuffer *input)
> >  {
> > -     inputBufferReady.emit(input);
> > +     if (running_) {
> > +             ASSERT(queuedInputBuffers_.front() == input);
> > +             queuedInputBuffers_.pop_front();
> > +             inputBufferReady.emit(input);
> > +     }
> >  }
> >  
> >  void SoftwareIsp::outputReady(FrameBuffer *output)
> 
> -- 
> Regards,
> 
> Laurent Pinchart

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 5073ce7a..400a4dc5 100644
--- a/include/libcamera/internal/software_isp/software_isp.h
+++ b/include/libcamera/internal/software_isp/software_isp.h
@@ -102,6 +102,7 @@  private:
 
 	std::unique_ptr<ipa::soft::IPAProxySoft> ipa_;
 	bool running_;
+	std::deque<FrameBuffer *> queuedInputBuffers_;
 	std::deque<FrameBuffer *> queuedOutputBuffers_;
 };
 
diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
index 140cddf3..beac66fc 100644
--- a/src/libcamera/software_isp/software_isp.cpp
+++ b/src/libcamera/software_isp/software_isp.cpp
@@ -303,6 +303,8 @@  int SoftwareIsp::queueBuffers(uint32_t frame, FrameBuffer *input,
 			return -EINVAL;
 	}
 
+	queuedInputBuffers_.push_back(input);
+
 	for (auto iter = outputs.begin(); iter != outputs.end(); iter++) {
 		FrameBuffer *const buffer = iter->second;
 		queuedOutputBuffers_.push_back(buffer);
@@ -329,6 +331,9 @@  int SoftwareIsp::start()
 
 /**
  * \brief Stops the Software ISP streaming operation
+ *
+ * All pending buffers are returned back as canceled before this method
+ * finishes.
  */
 void SoftwareIsp::stop()
 {
@@ -344,6 +349,13 @@  void SoftwareIsp::stop()
 		outputBufferReady.emit(buffer);
 	}
 	queuedOutputBuffers_.clear();
+
+	for (auto buffer : queuedInputBuffers_) {
+		FrameMetadata &metadata = buffer->_d()->metadata();
+		metadata.status = FrameMetadata::FrameCancelled;
+		inputBufferReady.emit(buffer);
+	}
+	queuedInputBuffers_.clear();
 }
 
 /**
@@ -377,7 +389,11 @@  void SoftwareIsp::statsReady(uint32_t frame, uint32_t bufferId)
 
 void SoftwareIsp::inputReady(FrameBuffer *input)
 {
-	inputBufferReady.emit(input);
+	if (running_) {
+		ASSERT(queuedInputBuffers_.front() == input);
+		queuedInputBuffers_.pop_front();
+		inputBufferReady.emit(input);
+	}
 }
 
 void SoftwareIsp::outputReady(FrameBuffer *output)