[PATCH v2 3/5] libcamera: software_isp: Handle queued input buffers on stop

[Laurent Pinchart]

On Mon, Feb 24, 2025 at 10:07:43PM +0100, Milan Zamazal wrote:
> Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:
> 
> > On Mon, Feb 24, 2025 at 09:19:53PM +0100, Milan Zamazal wrote:
> >> Laurent Pinchart writes:
> >> > On Mon, Feb 24, 2025 at 07:52:33PM +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().
> >> >> 
> >> >> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
> >> >> ---
> >> >>  .../internal/software_isp/software_isp.h         |  1 +
> >> >>  src/libcamera/software_isp/software_isp.cpp      | 16 +++++++++++++++-
> >> >>  2 files changed, 16 insertions(+), 1 deletion(-)
> >> >> 
> >> >> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h
> >> >> index f2344355..bfe34725 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 *> queuedOutputBuffers_;
> >> >> +	std::deque<FrameBuffer *> queuedInputBuffers_;
> >> >
> >> > I'd move input before output.
> >> >
> >> >>  };
> >> >>  
> >> >>  } /* namespace libcamera */
> >> >> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
> >> >> index 4339e547..3a605ab2 100644
> >> >> --- a/src/libcamera/software_isp/software_isp.cpp
> >> >> +++ b/src/libcamera/software_isp/software_isp.cpp
> >> >> @@ -301,6 +301,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);
> >> >> @@ -327,6 +329,9 @@ int SoftwareIsp::start()
> >> >>  
> >> >>  /**
> >> >>   * \brief Stops the Software ISP streaming operation
> >> >> + *
> >> >> + * All pending buffers are returned back (output buffers as canceled) before
> >> >> + * this method finishes.
> >> >
> >> > Shouldn't the input buffer be cancelled too ?
> >> 
> >> Is it necessary to cancel input buffers?  Aren't they just returned for
> >> re-use regardless of their contents (unlike the output buffers where the
> >> validity of the contents matters)?  The input buffer lands in
> >> V4L2VideoDevice::queueBuffer() and I can't see any use of the status
> >> there.
> >
> > At the moment we don't use the status, but I think it would be nicer to
> > still mark it appropriately. If the input buffer hasn't been fully
> > consumed, marking it as cancelled will let pipeline handlers deal with
> > this if they need to.
> 
> OK.
> 
> > If marking it as cancelled causes other issues then we can reconsider
> > that, but if it's easy I'd do so for completeness.
> 
> Works for me.  But I'm not much comfortable with the fact that then the
> buffer may keep FrameCancelled status forever and float with it around.
> Should SimpleCameraData::conversionInputDone() or something else reset
> the status?

Aren't the buffers freed in SimplePipelineHandler::stopDevice() by

	data->conversionBuffers_.clear();

?

> >> >>   */
> >> >>  void SoftwareIsp::stop()
> >> >>  {
> >> >> @@ -342,6 +347,10 @@ void SoftwareIsp::stop()
> >> >>  		outputBufferReady.emit(buffer);
> >> >>  	}
> >> >>  	queuedOutputBuffers_.clear();
> >> >> +
> >> >> +	for (auto buffer : queuedInputBuffers_)
> >> >> +		inputBufferReady.emit(buffer);
> >> >> +	queuedInputBuffers_.clear();
> >> >>  }
> >> >>  
> >> >>  /**
> >> >> @@ -375,7 +384,12 @@ void SoftwareIsp::statsReady(uint32_t frame, uint32_t bufferId)
> >> >>  
> >> >>  void SoftwareIsp::inputReady(FrameBuffer *input)
> >> >>  {
> >> >> -	inputBufferReady.emit(input);
> >> >> +	if (running_) {
> >> >> +		queuedInputBuffers_.erase(find(queuedInputBuffers_.begin(),
> >> >> +					       queuedInputBuffers_.end(),
> >> >> +					       input));
> >> >
> >> > Same comment as for 2/5.
> >> >
> >> >> +		inputBufferReady.emit(input);
> >> >> +	}
> >> >>  }
> >> >>  
> >> >>  void SoftwareIsp::outputReady(FrameBuffer *output)

-- 
Regards,

Laurent Pinchart