| Message ID | 20241009172108.2058320-3-mzamazal@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Milan Zamazal (2024-10-09 18:21:08) > PipelineHandler::stop() calls stopDevice() method to perform pipeline > specific cleanup and then completes waiting requests. If any queued > requests remain, an assertion error is raised. > > Software ISP stores request buffers in > SimpleCameraData::conversionQueue_ and queues them as V4L2 signals > bufferReady. stopDevice() cleanup forgets to clean up the buffers and > their requests from conversionQueue_, possibly resulting in the > assertion error. This patch fixes the omission. > > The problem wasn't very visible when > SimplePipelineHandler::kNumInternalBuffers (the number of buffers > allocated in V4L2) was equal to the number of buffers exported from > software ISP. But when the number of the exported buffers was increased > by one in commit abe2ec64f9e4e97bbdfe3a50372611bd7b5315c2, the assertion > error started pop up in some environments. Increasing the number of the > buffers much more, e.g. to 9, makes the problem very reproducible. > > Each pipeline uses its own mechanism to track the requests to clean up > and it can't be excluded that similar omissions are present in other > places. But there is no obvious way to make a common cleanup for all > the pipelines (except for doing it instead of raising the assertion > error, which is probably undesirable, in order not to hide incomplete > pipeline specific cleanups). > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=234 > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/libcamera/pipeline/simple/simple.cpp | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index 3ddce71d3..ff3859f18 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -284,6 +284,7 @@ public: > std::vector<std::unique_ptr<FrameBuffer>> conversionBuffers_; > std::queue<std::map<const Stream *, FrameBuffer *>> conversionQueue_; > bool useConversion_; > + void clearIncompleteRequests(); > > std::unique_ptr<Converter> converter_; > std::unique_ptr<SoftwareIsp> swIsp_; > @@ -897,6 +898,19 @@ void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer) > pipe->completeRequest(request); > } > > +void SimpleCameraData::clearIncompleteRequests() > +{ > + while (!conversionQueue_.empty()) { > + for (auto &item : conversionQueue_.front()) { > + FrameBuffer *outputBuffer = item.second; > + Request *request = outputBuffer->request(); > + if (request->status() == Request::RequestPending) > + pipe()->cancelRequest(request); Hrm. ... Is it possible to have a request on the conversionQueue_ which is not RequestPending ? I am worried that if we are selecting only Request::RequestPending requests to cancel - then at the end of this while loop we need an ASSERT(conversionQueue_.empty()); Although in fact, we're in a while loop that we can't leave until it's empty. Is this a potential for an infinite loop if there is a request in here which is request->status() != Request::RequestPending? Which would then take me back to really thinking we shouldn't have that check - we should cancel *all* requests in conversionQueue_ - or identify that there is a request that shouldn't be here ? > + } > + conversionQueue_.pop(); > + } > +} > + > void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId) > { > swIsp_->processStats(frame, bufferId, > @@ -1406,6 +1420,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera) > > video->bufferReady.disconnect(data, &SimpleCameraData::bufferReady); > > + data->clearIncompleteRequests(); > data->conversionBuffers_.clear(); > > releasePipeline(data); > -- > 2.44.1 >
Kieran Bingham <kieran.bingham@ideasonboard.com> writes: > Quoting Milan Zamazal (2024-10-09 18:21:08) >> PipelineHandler::stop() calls stopDevice() method to perform pipeline >> specific cleanup and then completes waiting requests. If any queued > >> requests remain, an assertion error is raised. >> >> Software ISP stores request buffers in >> SimpleCameraData::conversionQueue_ and queues them as V4L2 signals >> bufferReady. stopDevice() cleanup forgets to clean up the buffers and >> their requests from conversionQueue_, possibly resulting in the >> assertion error. This patch fixes the omission. >> >> The problem wasn't very visible when >> SimplePipelineHandler::kNumInternalBuffers (the number of buffers >> allocated in V4L2) was equal to the number of buffers exported from >> software ISP. But when the number of the exported buffers was increased >> by one in commit abe2ec64f9e4e97bbdfe3a50372611bd7b5315c2, the assertion >> error started pop up in some environments. Increasing the number of the >> buffers much more, e.g. to 9, makes the problem very reproducible. >> >> Each pipeline uses its own mechanism to track the requests to clean up >> and it can't be excluded that similar omissions are present in other >> places. But there is no obvious way to make a common cleanup for all >> the pipelines (except for doing it instead of raising the assertion >> error, which is probably undesirable, in order not to hide incomplete >> pipeline specific cleanups). >> >> Bug: https://bugs.libcamera.org/show_bug.cgi?id=234 >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> src/libcamera/pipeline/simple/simple.cpp | 15 +++++++++++++++ >> 1 file changed, 15 insertions(+) >> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp >> index 3ddce71d3..ff3859f18 100644 >> --- a/src/libcamera/pipeline/simple/simple.cpp >> +++ b/src/libcamera/pipeline/simple/simple.cpp >> @@ -284,6 +284,7 @@ public: >> std::vector<std::unique_ptr<FrameBuffer>> conversionBuffers_; >> std::queue<std::map<const Stream *, FrameBuffer *>> conversionQueue_; >> bool useConversion_; >> + void clearIncompleteRequests(); >> >> std::unique_ptr<Converter> converter_; >> std::unique_ptr<SoftwareIsp> swIsp_; >> @@ -897,6 +898,19 @@ void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer) >> pipe->completeRequest(request); >> } >> >> +void SimpleCameraData::clearIncompleteRequests() >> +{ >> + while (!conversionQueue_.empty()) { >> + for (auto &item : conversionQueue_.front()) { >> + FrameBuffer *outputBuffer = item.second; >> + Request *request = outputBuffer->request(); >> + if (request->status() == Request::RequestPending) >> + pipe()->cancelRequest(request); > > Hrm. ... Is it possible to have a request on the conversionQueue_ which > is not RequestPending ? The inner loop iterates over a map of streams and buffers. If there are multiple buffers in a request, the request gets processed repeatedly here. On the first run, it's status is pending, after being canceled, it's status is different and the next attempts to cancel it are skipped. I believe all `item's have the same request so it would be sufficient to look at the first `item' but I think it's better to avoid implicit assumptions. > I am worried that if we are selecting only Request::RequestPending > requests to cancel - then at the end of this while loop we need an > > ASSERT(conversionQueue_.empty()); I can add it. > Although in fact, we're in a while loop that we can't leave until it's > empty. Is this a potential for an infinite loop if there is a request in > here which is request->status() != Request::RequestPending? No, conversionQueue_.pop() below is called unconditionally. > Which would then take me back to really thinking we shouldn't have that > check - we should cancel *all* requests in conversionQueue_ - or > identify that there is a request that shouldn't be here ? The check is to avoid duplicate cancellation of a request. If there is a request with a different status originally and not completed/canceled, it will get caught by the assertion in the pipeline handler, I believe. It looks like this change introduces confusion. I'm not sure how to improve it. >> + } >> + conversionQueue_.pop(); >> + } >> +} >> + >> void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId) >> { >> swIsp_->processStats(frame, bufferId, >> @@ -1406,6 +1420,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera) >> >> video->bufferReady.disconnect(data, &SimpleCameraData::bufferReady); >> >> + data->clearIncompleteRequests(); >> data->conversionBuffers_.clear(); >> >> releasePipeline(data); >> -- >> 2.44.1 >>
Quoting Milan Zamazal (2024-10-10 08:35:00) > Kieran Bingham <kieran.bingham@ideasonboard.com> writes: > > > Quoting Milan Zamazal (2024-10-09 18:21:08) > >> PipelineHandler::stop() calls stopDevice() method to perform pipeline > >> specific cleanup and then completes waiting requests. If any queued > > > >> requests remain, an assertion error is raised. > >> > >> Software ISP stores request buffers in > >> SimpleCameraData::conversionQueue_ and queues them as V4L2 signals > >> bufferReady. stopDevice() cleanup forgets to clean up the buffers and > >> their requests from conversionQueue_, possibly resulting in the > >> assertion error. This patch fixes the omission. > >> > >> The problem wasn't very visible when > >> SimplePipelineHandler::kNumInternalBuffers (the number of buffers > >> allocated in V4L2) was equal to the number of buffers exported from > >> software ISP. But when the number of the exported buffers was increased > >> by one in commit abe2ec64f9e4e97bbdfe3a50372611bd7b5315c2, the assertion > >> error started pop up in some environments. Increasing the number of the > >> buffers much more, e.g. to 9, makes the problem very reproducible. > >> > >> Each pipeline uses its own mechanism to track the requests to clean up > >> and it can't be excluded that similar omissions are present in other > >> places. But there is no obvious way to make a common cleanup for all > >> the pipelines (except for doing it instead of raising the assertion > >> error, which is probably undesirable, in order not to hide incomplete > >> pipeline specific cleanups). > >> > >> Bug: https://bugs.libcamera.org/show_bug.cgi?id=234 > >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > >> --- > >> src/libcamera/pipeline/simple/simple.cpp | 15 +++++++++++++++ > >> 1 file changed, 15 insertions(+) > >> > >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > >> index 3ddce71d3..ff3859f18 100644 > >> --- a/src/libcamera/pipeline/simple/simple.cpp > >> +++ b/src/libcamera/pipeline/simple/simple.cpp > >> @@ -284,6 +284,7 @@ public: > >> std::vector<std::unique_ptr<FrameBuffer>> conversionBuffers_; > >> std::queue<std::map<const Stream *, FrameBuffer *>> conversionQueue_; > >> bool useConversion_; > >> + void clearIncompleteRequests(); > >> > >> std::unique_ptr<Converter> converter_; > >> std::unique_ptr<SoftwareIsp> swIsp_; > >> @@ -897,6 +898,19 @@ void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer) > >> pipe->completeRequest(request); > >> } > >> > >> +void SimpleCameraData::clearIncompleteRequests() > >> +{ > >> + while (!conversionQueue_.empty()) { > >> + for (auto &item : conversionQueue_.front()) { > >> + FrameBuffer *outputBuffer = item.second; > >> + Request *request = outputBuffer->request(); > >> + if (request->status() == Request::RequestPending) > >> + pipe()->cancelRequest(request); > > > > Hrm. ... Is it possible to have a request on the conversionQueue_ which > > is not RequestPending ? > > The inner loop iterates over a map of streams and buffers. If there are > multiple buffers in a request, the request gets processed repeatedly > here. On the first run, it's status is pending, after being canceled, > it's status is different and the next attempts to cancel it are skipped. > > I believe all `item's have the same request so it would be sufficient to > look at the first `item' but I think it's better to avoid implicit > assumptions. > > > I am worried that if we are selecting only Request::RequestPending > > requests to cancel - then at the end of this while loop we need an > > > > ASSERT(conversionQueue_.empty()); > > I can add it. I wouldn't now I understand the while loop better. There's no break, and no way to get out of while (!conversionQueue_.empty()) { } without the queue being emptied... > > Although in fact, we're in a while loop that we can't leave until it's > > empty. Is this a potential for an infinite loop if there is a request in > > here which is request->status() != Request::RequestPending? > > No, conversionQueue_.pop() below is called unconditionally. Ooops, I missed that I think. > > > Which would then take me back to really thinking we shouldn't have that > > check - we should cancel *all* requests in conversionQueue_ - or > > identify that there is a request that shouldn't be here ? > > The check is to avoid duplicate cancellation of a request. If there is > a request with a different status originally and not completed/canceled, > it will get caught by the assertion in the pipeline handler, I believe. > > It looks like this change introduces confusion. I'm not sure how to > improve it. It's probably just me not understanding things ;-) Ok - so perhaps the part I missed is that the queue will always be emptied, because ... > > >> + } ... I missed reading that } so the next line always happens ;-) > >> + conversionQueue_.pop(); I thought we were only popping off requests that were directly cancelled above. So it seems correct ... Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> + } > >> +} > >> + > >> void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId) > >> { > >> swIsp_->processStats(frame, bufferId, > >> @@ -1406,6 +1420,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera) > >> > >> video->bufferReady.disconnect(data, &SimpleCameraData::bufferReady); > >> > >> + data->clearIncompleteRequests(); > >> data->conversionBuffers_.clear(); > >> > >> releasePipeline(data); > >> -- > >> 2.44.1 > >> >
diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp index 3ddce71d3..ff3859f18 100644 --- a/src/libcamera/pipeline/simple/simple.cpp +++ b/src/libcamera/pipeline/simple/simple.cpp @@ -284,6 +284,7 @@ public: std::vector<std::unique_ptr<FrameBuffer>> conversionBuffers_; std::queue<std::map<const Stream *, FrameBuffer *>> conversionQueue_; bool useConversion_; + void clearIncompleteRequests(); std::unique_ptr<Converter> converter_; std::unique_ptr<SoftwareIsp> swIsp_; @@ -897,6 +898,19 @@ void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer) pipe->completeRequest(request); } +void SimpleCameraData::clearIncompleteRequests() +{ + while (!conversionQueue_.empty()) { + for (auto &item : conversionQueue_.front()) { + FrameBuffer *outputBuffer = item.second; + Request *request = outputBuffer->request(); + if (request->status() == Request::RequestPending) + pipe()->cancelRequest(request); + } + conversionQueue_.pop(); + } +} + void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId) { swIsp_->processStats(frame, bufferId, @@ -1406,6 +1420,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera) video->bufferReady.disconnect(data, &SimpleCameraData::bufferReady); + data->clearIncompleteRequests(); data->conversionBuffers_.clear(); releasePipeline(data);
PipelineHandler::stop() calls stopDevice() method to perform pipeline specific cleanup and then completes waiting requests. If any queued requests remain, an assertion error is raised. Software ISP stores request buffers in SimpleCameraData::conversionQueue_ and queues them as V4L2 signals bufferReady. stopDevice() cleanup forgets to clean up the buffers and their requests from conversionQueue_, possibly resulting in the assertion error. This patch fixes the omission. The problem wasn't very visible when SimplePipelineHandler::kNumInternalBuffers (the number of buffers allocated in V4L2) was equal to the number of buffers exported from software ISP. But when the number of the exported buffers was increased by one in commit abe2ec64f9e4e97bbdfe3a50372611bd7b5315c2, the assertion error started pop up in some environments. Increasing the number of the buffers much more, e.g. to 9, makes the problem very reproducible. Each pipeline uses its own mechanism to track the requests to clean up and it can't be excluded that similar omissions are present in other places. But there is no obvious way to make a common cleanup for all the pipelines (except for doing it instead of raising the assertion error, which is probably undesirable, in order not to hide incomplete pipeline specific cleanups). Bug: https://bugs.libcamera.org/show_bug.cgi?id=234 Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- src/libcamera/pipeline/simple/simple.cpp | 15 +++++++++++++++ 1 file changed, 15 insertions(+)