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

[Kieran Bingham]

Quoting Robert Mader (2024-10-09 10:41:00)
> Hi, thanks for the patch!
> 
> On 09.10.24 10:39, Milan Zamazal wrote:
> > Milan Zamazal <mzamazal at redhat.com> writes:
> >
> >> 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>
> > Changes in v2:
> > - The request is additionally canceled.
> > - New helper method PipelineHandler::cancelRequest() introduced.
> >
> > Robert, could you please test v2?
> 
> I gave it a quick go for 5 minutes - generally it seems to work great, 
> however trying enough quick camera switching I still run into the assert.
> 
> So probably, on top of this, we'll need something implementing what 
> Kieran suggested:
> 
>  > (Though I think we should reject any incoming requests as soon as we 
> hit stop())

Sorry - there I was saying that should /already/ be happening. The state
machine should put the camera in to a stopping state where new incoming
requests will be rejected...

https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/camera.cpp#n1401

Unless this is still somehow racy ;-(

--
Kieran


> 
> >> ---
> >>   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();
> >>   
> >>      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
> >> @@ -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
> 
> Small typo here
> 
> 
> >> + *
> >> + * 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
> 
> -- 
> Robert Mader
> Consultant Software Developer
> 
> Collabora Ltd.
> Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK
> Registered in England & Wales, no. 5513718
>