[PATCH v2] libcamera: software_isp: Clean up pending requests on stop
Barnabás Pőcze
pobrn at protonmail.com
Wed Oct 9 22:53:17 CEST 2024
Hi
2024. október 9., szerda 13:17 keltezéssel, Kieran Bingham <kieran.bingham at ideasonboard.com> írta:
> 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 ;-(
It seems to me that one thread calling Camera::stop() and another thread calling
Camera::queueRequest() can cause this issue. Specifically, if the thread executing
Camera::stop() is stopped e.g. after `LOG(Camera, Debug) << "Stopping capture";`,
then the thread running Camera::queueRequest() can progress and queue the request
without anything stopping it. Afterwards, if the thread running Camera::stop() continues
such that the request hasn't been completed, then the assertion will be hit
unless PipelineHandler::stopDevice() does something with the requests. At least
that what I can see, it is possible that I have overlooked something.
Slightly unrelated, but I think the state transitions of the Camera should be done
with atomic compare exchange operations, otherwise multiple threads could successfully
call e.g. Camera::stop() concurrently.
Regards,
Barnabás Pőcze
>
> --
> 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
> >
More information about the libcamera-devel
mailing list