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

[Kieran Bingham]

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 ?

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());

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?

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 ?

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