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

[Kieran Bingham]

Quoting Milan Zamazal (2024-10-09 09:35:56)
> 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);
> +               }
> +               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();

Do the requests have any existing reference to anything in teh
conversionBuffers_ that would need to make sure we cancel/clear before
releasing the conversionBuffers_? 

Probably not - but just checking.

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

Sorry to be a pain on nitpicking - but the changes to pipeline_handler
are core - not SoftISP and the changes below will function standalone,
so this should be a separate patch.

Then the softISP can use it!

Something like "libcamera: pipeline_handler: Provide cancelRequest"

is definitely something I want to be visible in the changelogs, and
clear that other pipeline handlers can/should use it.

Once split to two - I'd probably say this already goes in the right
direction to get merged even if there is possible still another race to
investigate on top...

--
Kieran

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