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

[Robert Mader]

On 09.10.24 16:05, Milan Zamazal wrote:
> Hi Robert,
>
> thank you for testing and review.
>
> Robert Mader <robert.mader at collabora.com> writes:
>
>> Hi, thanks for the patch!
>>
>> On 09.10.24 10:39, Milan Zamazal wrote:
>>> Milan Zamazal <mzamazal at redhat.com> writes:
>>>
>>>> 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>
>>> Changes in v2:
>>> - The request is additionally canceled.
>>> - New helper method PipelineHandler::cancelRequest() introduced.
>>>
>>> Robert, could you please test v2?
>> I gave it a quick go for 5 minutes - generally it seems to work great, however trying enough quick camera
>> switching I still run into the assert.
>>
>> So probably, on top of this, we'll need something implementing what Kieran suggested:
>>
>>> (Though I think we should reject any incoming requests as soon as we hit stop())
>>>> ---
>>>>    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);
>>>> +		}
>>>> +		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
>> Small typo here
> Sorry, I can't spot the typo, what typo exactly?

Urgh, never mind, I though the "request The request.." part was a typo - 
but that's the arg name 🤦

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

-- 
Robert Mader
Consultant Software Developer

Collabora Ltd.
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK
Registered in England & Wales, no. 5513718