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

[Milan Zamazal]

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

> Quoting Milan Zamazal (2024-10-09 18:21:08)
>> 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).
>> 
>> 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..ff3859f18 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();
>> +                       if (request->status() == Request::RequestPending)
>> +                               pipe()->cancelRequest(request);
>
> Hrm. ... Is it possible to have a request on the conversionQueue_ which
> is not RequestPending ?

The inner loop iterates over a map of streams and buffers.  If there are
multiple buffers in a request, the request gets processed repeatedly
here.  On the first run, it's status is pending, after being canceled,
it's status is different and the next attempts to cancel it are skipped.

I believe all `item's have the same request so it would be sufficient to
look at the first `item' but I think it's better to avoid implicit
assumptions.

> I am worried that if we are selecting only Request::RequestPending
> requests to cancel - then at the end of this while loop we need an
>
> ASSERT(conversionQueue_.empty());

I can add it.

> Although in fact, we're in a while loop that we can't leave until it's
> empty. Is this a potential for an infinite loop if there is a request in
> here which is request->status() != Request::RequestPending?

No, conversionQueue_.pop() below is called unconditionally.

> Which would then take me back to really thinking we shouldn't have that
> check - we should cancel *all* requests in conversionQueue_ - or
> identify that there is a request that shouldn't be here ?

The check is to avoid duplicate cancellation of a request.  If there is
a request with a different status originally and not completed/canceled,
it will get caught by the assertion in the pipeline handler, I believe.

It looks like this change introduces confusion.  I'm not sure how to
improve it.

>> +               }
>> +               conversionQueue_.pop();
>> +       }
>> +}
>> +
>>  void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)
>>  {
>>         swIsp_->processStats(frame, bufferId,
>> @@ -1406,6 +1420,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera)
>>  
>>         video->bufferReady.disconnect(data, &SimpleCameraData::bufferReady);
>>  
>> +       data->clearIncompleteRequests();
>>         data->conversionBuffers_.clear();
>>  
>>         releasePipeline(data);
>> -- 
>> 2.44.1
>>