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

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