Message ID | 20241009083556.330325-1-mzamazal@redhat.com |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
Milan Zamazal <mzamazal@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@redhat.com> Changes in v2: - The request is additionally canceled. - New helper method PipelineHandler::cancelRequest() introduced. Robert, could you please test v2? > --- > 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 > + * > + * 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
Hi, thanks for the patch! On 09.10.24 10:39, Milan Zamazal wrote: > Milan Zamazal <mzamazal@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@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()) >> --- >> 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
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@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@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 >
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@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 >
Hi Robert, thank you for testing and review. Robert Mader <robert.mader@collabora.com> writes: > Hi, thanks for the patch! > > On 09.10.24 10:39, Milan Zamazal wrote: >> Milan Zamazal <mzamazal@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@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()) > >>> --- >>> 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 Sorry, I can't spot the typo, what typo exactly? >>> + * >>> + * 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
On 09.10.24 16:05, Milan Zamazal wrote: > Hi Robert, > > thank you for testing and review. > > Robert Mader <robert.mader@collabora.com> writes: > >> Hi, thanks for the patch! >> >> On 09.10.24 10:39, Milan Zamazal wrote: >>> Milan Zamazal <mzamazal@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@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()) >>>> --- >>>> 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 > Sorry, I can't spot the typo, what typo exactly? Urgh, never mind, I though the "request The request.." part was a typo - but that's the arg name 🤦 > >>>> + * >>>> + * 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
On Wed, Oct 09, 2024 at 10:35:56AM +0200, Milan Zamazal wrote: > 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@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); Aren't you cancelling the same request multiple times ? > + } > + 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 > + * > + * 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
Kieran Bingham <kieran.bingham@ideasonboard.com> writes: > 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@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_? I don't think so -- conversionBuffers_ contains buffers obtained from V4L2VideoDevice while clearIncompleteRequests examines conversionQueue_, which contains buffers from requests. I.e. they are input x output buffers if I understand the things correctly. I can swap the two lines to avoid any doubts or future mistakes. > 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. Yes, I'll split 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... Good deal. :-) > -- > 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 >>
Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > On Wed, Oct 09, 2024 at 10:35:56AM +0200, Milan Zamazal wrote: >> 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@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); > > Aren't you cancelling the same request multiple times ? Possibly yes. I'll add a check for RequestPending status. >> + } >> + 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 >> + * >> + * 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
On Wed, Oct 09, 2024 at 06:51:36PM +0200, Milan Zamazal wrote: > Laurent Pinchart writes: > > On Wed, Oct 09, 2024 at 10:35:56AM +0200, Milan Zamazal wrote: > >> 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@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); > > > > Aren't you cancelling the same request multiple times ? > > Possibly yes. I'll add a check for RequestPending status. How about modifying conversionQueue_ to store instances of a structure that contain a Request pointer in addition to the std::map ? > >> + } > >> + 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 > >> + * > >> + * 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 >
Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > On Wed, Oct 09, 2024 at 06:51:36PM +0200, Milan Zamazal wrote: >> Laurent Pinchart writes: >> > On Wed, Oct 09, 2024 at 10:35:56AM +0200, Milan Zamazal wrote: > >> >> 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@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); >> > >> > Aren't you cancelling the same request multiple times ? >> >> Possibly yes. I'll add a check for RequestPending status. > > How about modifying conversionQueue_ to store instances of a structure > that contain a Request pointer in addition to the std::map ? I prefer keeping data structures simple. What would be the advantage worth of maintaining the extra pointer? I'd be inclined to have it if it served more purposes, but is it worth just for the cleanup? >> >> + } >> >> + 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 >> >> + * >> >> + * 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 >>
Hi 2024. október 9., szerda 13:17 keltezéssel, Kieran Bingham <kieran.bingham@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@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@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 > >
On Wed, Oct 09, 2024 at 09:43:42PM +0200, Milan Zamazal wrote: > Laurent Pinchart writes: > > On Wed, Oct 09, 2024 at 06:51:36PM +0200, Milan Zamazal wrote: > >> Laurent Pinchart writes: > >> > On Wed, Oct 09, 2024 at 10:35:56AM +0200, Milan Zamazal wrote: > > > >> >> 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@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); > >> > > >> > Aren't you cancelling the same request multiple times ? > >> > >> Possibly yes. I'll add a check for RequestPending status. > > > > How about modifying conversionQueue_ to store instances of a structure > > that contain a Request pointer in addition to the std::map ? > > I prefer keeping data structures simple. What would be the advantage > worth of maintaining the extra pointer? I'd be inclined to have it if > it served more purposes, but is it worth just for the cleanup? As far as I understand, the conversionQueue_ contains a map of streams to output buffers for one request. If you look at SimpleCameraData::bufferReady(), you'll see, in the !FrameMetadata::FrameSuccess path at the top of the function, Request *request = nullptr; for (auto &item : conversionQueue_.front()) { FrameBuffer *outputBuffer = item.second; request = outputBuffer->request(); pipe->completeBuffer(request, outputBuffer); } conversionQueue_.pop(); if (request) pipe->completeRequest(request); All buffers need to be completed individually, but the request needs to be completed once only. All the output buffers in the map relate to the same request, so I think it makes sense to store the request pointer separately in the queue for easy access. Alternatively, you could have a code construct similar to the above, getting the request pointer from the buffer, and calling completeRequest() once only. It would be nice if we could share more code, replacing the above construct with something shared by the cancel path. > >> >> + } > >> >> + 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 > >> >> + * > >> >> + * 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
Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > On Wed, Oct 09, 2024 at 09:43:42PM +0200, Milan Zamazal wrote: >> Laurent Pinchart writes: >> > On Wed, Oct 09, 2024 at 06:51:36PM +0200, Milan Zamazal wrote: > >> >> Laurent Pinchart writes: >> >> > On Wed, Oct 09, 2024 at 10:35:56AM +0200, Milan Zamazal wrote: >> > >> >> >> 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@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); >> >> > >> >> > Aren't you cancelling the same request multiple times ? >> >> >> >> Possibly yes. I'll add a check for RequestPending status. >> > >> > How about modifying conversionQueue_ to store instances of a structure >> > that contain a Request pointer in addition to the std::map ? >> >> I prefer keeping data structures simple. What would be the advantage >> worth of maintaining the extra pointer? I'd be inclined to have it if >> it served more purposes, but is it worth just for the cleanup? > > As far as I understand, the conversionQueue_ contains a map of streams > to output buffers for one request. If you look at > SimpleCameraData::bufferReady(), you'll see, in the > !FrameMetadata::FrameSuccess path at the top of the function, > > Request *request = nullptr; > for (auto &item : conversionQueue_.front()) { > FrameBuffer *outputBuffer = item.second; > request = outputBuffer->request(); > pipe->completeBuffer(request, outputBuffer); > } > conversionQueue_.pop(); > > if (request) > pipe->completeRequest(request); > > All buffers need to be completed individually, but the request needs to > be completed once only. But it is completed only if: - There is at least one output buffer - AND the buffer refers to the given request. Those look like reasonable assumptions but I'm not sure there is nothing subtle behind. You authored the code structure above when adding support for multiple streams so can you confirm that something like std::queue<std::pair<Request *, std::map<const Stream *, FrameBuffer *>>> conversionQueue_; ... Request *request = conversionQueue_.front().first; for (auto &item : conversionQueue_.front().second) pipe->completeBuffer(request, item.second); pipe->completeRequest(request); conversionQueue_.pop(); would be equivalent under the right assumptions? > All the output buffers in the map relate to the same request, so I think > it makes sense to store the request pointer separately in the queue for > easy access. Alternatively, you could have a code construct similar to > the above, getting the request pointer from the buffer, and calling > completeRequest() once only. It would be nice if we could share more > code, replacing the above construct with something shared by the cancel > path. > >> >> >> + } >> >> >> + 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 >> >> >> + * >> >> >> + * 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
Milan Zamazal <mzamazal@redhat.com> writes: > Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > >> On Wed, Oct 09, 2024 at 09:43:42PM +0200, Milan Zamazal wrote: >>> Laurent Pinchart writes: >>> > On Wed, Oct 09, 2024 at 06:51:36PM +0200, Milan Zamazal wrote: >> >>> >> Laurent Pinchart writes: >>> >> > On Wed, Oct 09, 2024 at 10:35:56AM +0200, Milan Zamazal wrote: >>> > >>> >> >> 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@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); >>> >> > >>> >> > Aren't you cancelling the same request multiple times ? >>> >> >>> >> Possibly yes. I'll add a check for RequestPending status. >>> > >>> > How about modifying conversionQueue_ to store instances of a structure >>> > that contain a Request pointer in addition to the std::map ? >>> >>> I prefer keeping data structures simple. What would be the advantage >>> worth of maintaining the extra pointer? I'd be inclined to have it if >>> it served more purposes, but is it worth just for the cleanup? >> >> As far as I understand, the conversionQueue_ contains a map of streams >> to output buffers for one request. If you look at >> SimpleCameraData::bufferReady(), you'll see, in the >> !FrameMetadata::FrameSuccess path at the top of the function, >> >> Request *request = nullptr; >> for (auto &item : conversionQueue_.front()) { >> FrameBuffer *outputBuffer = item.second; >> request = outputBuffer->request(); >> pipe->completeBuffer(request, outputBuffer); >> } >> conversionQueue_.pop(); >> >> if (request) >> pipe->completeRequest(request); >> >> All buffers need to be completed individually, but the request needs to >> be completed once only. > > But it is completed only if: > > - There is at least one output buffer > - AND the buffer refers to the given request. > > Those look like reasonable assumptions but I'm not sure there is nothing > subtle behind. You authored the code structure above when adding > support for multiple streams so can you confirm that something like > > std::queue<std::pair<Request *, std::map<const Stream *, FrameBuffer *>>> conversionQueue_; > > ... > > Request *request = conversionQueue_.front().first; > for (auto &item : conversionQueue_.front().second) > pipe->completeBuffer(request, item.second); > pipe->completeRequest(request); > conversionQueue_.pop(); > > would be equivalent under the right assumptions? I take the silence as yes, so posted v4 with this change. >> All the output buffers in the map relate to the same request, so I think >> it makes sense to store the request pointer separately in the queue for >> easy access. Alternatively, you could have a code construct similar to >> the above, getting the request pointer from the buffer, and calling >> completeRequest() once only. It would be nice if we could share more >> code, replacing the above construct with something shared by the cancel >> path. >> >>> >> >> + } >>> >> >> + 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 >>> >> >> + * >>> >> >> + * 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
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 + * + * 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
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@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(-)