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

[Kieran Bingham]

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



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

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
>