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

[Kieran Bingham]

Quoting Milan Zamazal (2024-10-09 08:59:07)
> Hi Kieran,
> 
> thank you for review.
> 
> Kieran Bingham <kieran.bingham at ideasonboard.com> writes:
> 
> > 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();
> 
> Ah, right.
> 
> >> +                       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.
> 
> Yes, it should.  request->_d()->cancel() is private; but as those two
> lines are already used twice in pipeline_handler.cpp, I'll extract them
> to a separate method and will call it from here.

Great, - indeed the first thing I went looking for was
"pipe()->cancelReqeust(request)" which isn't there (yet :D)

--
Kieran


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