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

[Milan Zamazal]

Milan Zamazal <mzamazal at redhat.com> writes:

> 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?

I take the silence as yes, so posted v4 with this change.

>> 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