[PATCH v2] libcamera: software_isp: Clean up pending requests on stop

[Milan Zamazal]

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

> On Wed, Oct 09, 2024 at 09:43:42PM +0200, Milan Zamazal wrote:
>> Laurent Pinchart writes:
>> > On Wed, Oct 09, 2024 at 06:51:36PM +0200, Milan Zamazal wrote:
>
>> >> Laurent Pinchart writes:
>> >> > On Wed, Oct 09, 2024 at 10:35:56AM +0200, Milan Zamazal wrote:
>> >
>> >> >> PipelineHandler::stop() calls stopDevice() method to perform pipeline
>> >> >> specific cleanup and then completes waiting requests.  If any queued
>> >> >
>> >> >> requests remain, an assertion error is raised.
>> >> >> 
>> >> >> Software ISP stores request buffers in
>> >> >> SimpleCameraData::conversionQueue_ and queues them as V4L2 signals
>> >> >> bufferReady.  stopDevice() cleanup forgets to clean up the buffers and
>> >> >> their requests from conversionQueue_, possibly resulting in the
>> >> >> assertion error.
>> >> >> 
>> >> >> This patch fixes the omission.  The request must be also canceled, which
>> >> >> requires introducing a little PipelineHandler::cancelRequest helper in
>> >> >> order to be able to access the private cancel() method.
>> >> >> 
>> >> >> The problem wasn't very visible when
>> >> >> SimplePipelineHandler::kNumInternalBuffers (the number of buffers
>> >> >> allocated in V4L2) was equal to the number of buffers exported from
>> >> >> software ISP.  But when the number of the exported buffers was increased
>> >> >> by one in commit abe2ec64f9e4e97bbdfe3a50372611bd7b5315c2, the assertion
>> >> >> error started pop up in some environments.  Increasing the number of the
>> >> >> buffers much more, e.g. to 9, makes the problem very reproducible.
>> >> >> 
>> >> >> Each pipeline uses its own mechanism to track the requests to clean up
>> >> >> and it can't be excluded that similar omissions are present in other
>> >> >> places.  But there is no obvious way to make a common cleanup for all
>> >> >> the pipelines (except for doing it instead of raising the assertion
>> >> >> error, which is probably undesirable, in order not to hide incomplete
>> >> >> pipeline specific cleanups).
>> >> >> 
>> >> >> Bug: https://bugs.libcamera.org/show_bug.cgi?id=234
>> >> >> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
>> >> >> ---
>> >> >>  include/libcamera/internal/pipeline_handler.h |  1 +
>> >> >>  src/libcamera/pipeline/simple/simple.cpp      | 14 +++++++++++
>> >> >>  src/libcamera/pipeline_handler.cpp            | 23 +++++++++++++------
>> >> >>  3 files changed, 31 insertions(+), 7 deletions(-)
>> >> >> 
>> >> >> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
>> >> >> index 0d3808036..fb28a18d0 100644
>> >> >> --- a/include/libcamera/internal/pipeline_handler.h
>> >> >> +++ b/include/libcamera/internal/pipeline_handler.h
>> >> >> @@ -60,6 +60,7 @@ public:
>> >> >>  
>> >> >>  	bool completeBuffer(Request *request, FrameBuffer *buffer);
>> >> >>  	void completeRequest(Request *request);
>> >> >> +	void cancelRequest(Request *request);
>> >> >>  
>> >> >>  	std::string configurationFile(const std::string &subdir,
>> >> >>  				      const std::string &name) const;
>> >> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
>> >> >> index 3ddce71d3..e862ef00f 100644
>> >> >> --- a/src/libcamera/pipeline/simple/simple.cpp
>> >> >> +++ b/src/libcamera/pipeline/simple/simple.cpp
>> >> >> @@ -284,6 +284,7 @@ public:
>> >> >>  	std::vector<std::unique_ptr<FrameBuffer>> conversionBuffers_;
>> >> >>  	std::queue<std::map<const Stream *, FrameBuffer *>> conversionQueue_;
>> >> >>  	bool useConversion_;
>> >> >> +	void clearIncompleteRequests();
>> >> >>  
>> >> >>  	std::unique_ptr<Converter> converter_;
>> >> >>  	std::unique_ptr<SoftwareIsp> swIsp_;
>> >> >> @@ -897,6 +898,18 @@ void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer)
>> >> >>  		pipe->completeRequest(request);
>> >> >>  }
>> >> >>  
>> >> >> +void SimpleCameraData::clearIncompleteRequests()
>> >> >> +{
>> >> >> +	while (!conversionQueue_.empty()) {
>> >> >> +		for (auto &item : conversionQueue_.front()) {
>> >> >> +			FrameBuffer *outputBuffer = item.second;
>> >> >> +			Request *request = outputBuffer->request();
>> >> >> +			pipe()->cancelRequest(request);
>> >> >
>> >> > Aren't you cancelling the same request multiple times ?
>> >> 
>> >> Possibly yes.  I'll add a check for RequestPending status.
>> >
>> > How about modifying conversionQueue_ to store instances of a structure
>> > that contain a Request pointer in addition to the std::map ?
>> 
>> I prefer keeping data structures simple.  What would be the advantage
>> worth of maintaining the extra pointer?  I'd be inclined to have it if
>> it served more purposes, but is it worth just for the cleanup?
>
> As far as I understand, the conversionQueue_ contains a map of streams
> to output buffers for one request. If you look at
> SimpleCameraData::bufferReady(), you'll see, in the
> !FrameMetadata::FrameSuccess path at the top of the function,
>
> 		Request *request = nullptr;
> 		for (auto &item : conversionQueue_.front()) {
> 			FrameBuffer *outputBuffer = item.second;
> 			request = outputBuffer->request();
> 			pipe->completeBuffer(request, outputBuffer);
> 		}
> 		conversionQueue_.pop();
>
> 		if (request)
> 			pipe->completeRequest(request);
>
> All buffers need to be completed individually, but the request needs to
> be completed once only.

But it is completed only if:

- There is at least one output buffer
- AND the buffer refers to the given request.

Those look like reasonable assumptions but I'm not sure there is nothing
subtle behind.  You authored the code structure above when adding
support for multiple streams so can you confirm that something like

  std::queue<std::pair<Request *, std::map<const Stream *, FrameBuffer *>>> conversionQueue_;

  ...

  Request *request = conversionQueue_.front().first;
  for (auto &item : conversionQueue_.front().second)
  	pipe->completeBuffer(request, item.second);
  pipe->completeRequest(request);
  conversionQueue_.pop();

would be equivalent under the right assumptions?

> All the output buffers in the map relate to the same request, so I think
> it makes sense to store the request pointer separately in the queue for
> easy access. Alternatively, you could have a code construct similar to
> the above, getting the request pointer from the buffer, and calling
> completeRequest() once only. It would be nice if we could share more
> code, replacing the above construct with something shared by the cancel
> path.
>
>> >> >> +		}
>> >> >> +		conversionQueue_.pop();
>> >> >> +	}
>> >> >> +}
>> >> >> +
>> >> >>  void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)
>> >> >>  {
>> >> >>  	swIsp_->processStats(frame, bufferId,
>> >> >> @@ -1407,6 +1420,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera)
>> >> >>  	video->bufferReady.disconnect(data, &SimpleCameraData::bufferReady);
>> >> >>  
>> >> >>  	data->conversionBuffers_.clear();
>> >> >> +	data->clearIncompleteRequests();
>> >> >>  
>> >> >>  	releasePipeline(data);
>> >> >>  }
>> >> >> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
>> >> >> index e59404691..c9cb11f0f 100644
>> >> >> --- a/src/libcamera/pipeline_handler.cpp
>> >> >> +++ b/src/libcamera/pipeline_handler.cpp
>> >> >> @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera)
>> >> >>  	while (!waitingRequests_.empty()) {
>> >> >>  		Request *request = waitingRequests_.front();
>> >> >>  		waitingRequests_.pop();
>> >> >> -
>> >> >> -		request->_d()->cancel();
>> >> >> -		completeRequest(request);
>> >> >> +		cancelRequest(request);
>> >> >>  	}
>> >> >>  
>> >> >>  	/* Make sure no requests are pending. */
>> >> >> @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request)
>> >> >>  	}
>> >> >>  
>> >> >>  	int ret = queueRequestDevice(camera, request);
>> >> >> -	if (ret) {
>> >> >> -		request->_d()->cancel();
>> >> >> -		completeRequest(request);
>> >> >> -	}
>> >> >> +	if (ret)
>> >> >> +		cancelRequest(request);
>> >> >>  }
>> >> >>  
>> >> >>  /**
>> >> >> @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request)
>> >> >>  	}
>> >> >>  }
>> >> >>  
>> >> >> +/**
>> >> >> + * \brief Cancel request and signal its completion
>> >> >> + * \param[in] request The request to cancel
>> >> >> + *
>> >> >> + * This function cancels the request in addition to its completion. The same
>> >> >> + * rules as for completeRequest() apply.
>> >> >> + */
>> >> >> +void PipelineHandler::cancelRequest(Request *request)
>> >> >> +{
>> >> >> +	request->_d()->cancel();
>> >> >> +	completeRequest(request);
>> >> >> +}
>> >> >> +
>> >> >>  /**
>> >> >>   * \brief Retrieve the absolute path to a platform configuration file
>> >> >>   * \param[in] subdir The pipeline handler specific subdirectory name