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

[Milan Zamazal]

Hi Kieran,

thank you for review.

Kieran Bingham <kieran.bingham at ideasonboard.com> writes:

> Quoting Milan Zamazal (2024-10-08 16:09:16)
>> 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 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).
>
> Indeed, I think that's why I added the assertion to make sure we force
> Pipeline handlers to do 'the right thing for that handler'. Maybe we
> could relax that in the future if it becomes apparetnly safe ... but I
> think this patch is probably the right direction for now.
>
>
>> 
>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=234
>> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
>> ---
>>  src/libcamera/pipeline/simple/simple.cpp | 15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>> 
>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
>> index 3ddce71d3..4e8504922 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,19 @@ 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();
>
>
> I think somewhere here we should be marking the buffers as cancelled
> before we complete it:
> 			outputBuffer->_d()->cancel();

Ah, right.

>> +                       pipe()->completeBuffer(request, outputBuffer);
>> +                       pipe()->completeRequest(request);
>
> in fact both those lines might be better as:
>
>                 request->_d()->cancel();
>                 pipe()->completeRequest(request);
>
> as I think request->_d()->cancel(); will correctly mark all buffers as
> cancelled, and the request itself.

Yes, it should.  request->_d()->cancel() is private; but as those two
lines are already used twice in pipeline_handler.cpp, I'll extract them
to a separate method and will call it from here.

> With that handled, and retested:
>
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
>
>> +               }
>> +               conversionQueue_.pop();
>> +       }
>> +}
>> +
>>  void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)
>>  {
>>         swIsp_->processStats(frame, bufferId,
>> @@ -1407,6 +1421,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera)
>>         video->bufferReady.disconnect(data, &SimpleCameraData::bufferReady);
>>  
>>         data->conversionBuffers_.clear();
>> +       data->clearIncompleteRequests();
>>  
>>         releasePipeline(data);
>>  }
>> -- 
>> 2.44.1
>>