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

[Milan Zamazal]

Hi Laurent,

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

> Hi Milan,
>
> Discussing this fix today made me realized I forgot to reply to this
> e-mail. Sorry about that.
>
> On Fri, Oct 11, 2024 at 04:09:30PM +0200, Milan Zamazal wrote:
>> 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?
>
> My assumption (please tell me if I'm wrong) is that an entry in the
> conversion queue corresponds to one request, and holds a map of streams
> to frame buffers for that particular request. 

OK.

> We have code that needs to deal with those buffers, and also code that
> needs to deal with the request. Currently, the request is retrieved
> from the buffer. In the case of SimpleCameraData::bufferReady(), we
> use the request from the last buffer in the map, but any buffer would
> do, as they all belong to the same request.
>
> I find this code confusing, and I believe we should explicitly store the
> request pointer in the conversion queue entry, and retrieve it from
> there, instead of retrieving it from the buffer. It would make the code
> clearer. Your code snippet above looks fine, althouh I would probably
> create a new structure to hold the request pointer and map:
>
> 	struct ConversionJob {
> 		Request *request;
> 		std::map<const Stream *, FrameBuffer *> buffers;
> 	};
>
> 	std::queue<ConversionJob> conversionQueue_;
>
> (we can bikeshed the ConversionJob name). The code could then look like
>
>  	Request *request = nullptr;
> 	const ConversionJob &job = conversionQueue_.front();
>
>  	for (auto &[stream, outputBuffer] : job)
>  		pipe->completeBuffer(request, outputBuffer);
>
> 	pipe->completeRequest(job.request);
>  	conversionQueue_.pop();
>
> which I think is much more readable.

I think the eventual code in v6 is basically the same, minus naming and
omitting `request' variable.  And since it's based on a snippet you
wrote, I suppose it should be OK for you :-) but tell me in case any
further adjustments are needed.

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