| Message ID | 20250224185235.43381-4-mzamazal@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series |
|
| Related | show |
Hi Milan, Thank you for the patch. On Mon, Feb 24, 2025 at 07:52:33PM +0100, Milan Zamazal wrote: > When SoftwareIsp stops, input and output buffers queued to it may not > yet be fully processed. They will be eventually returned but stop means > stop, there should be no processing related actions invoked afterwards. > > Let's stop forwarding processed input buffers from SoftwareIsp slots > when SoftwareIsp is stopped. Let's track the queued input buffers and > return them back for capture in SoftwareIsp::stop(). > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > .../internal/software_isp/software_isp.h | 1 + > src/libcamera/software_isp/software_isp.cpp | 16 +++++++++++++++- > 2 files changed, 16 insertions(+), 1 deletion(-) > > diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h > index f2344355..bfe34725 100644 > --- a/include/libcamera/internal/software_isp/software_isp.h > +++ b/include/libcamera/internal/software_isp/software_isp.h > @@ -102,6 +102,7 @@ private: > std::unique_ptr<ipa::soft::IPAProxySoft> ipa_; > bool running_; > std::deque<FrameBuffer *> queuedOutputBuffers_; > + std::deque<FrameBuffer *> queuedInputBuffers_; I'd move input before output. > }; > > } /* namespace libcamera */ > diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp > index 4339e547..3a605ab2 100644 > --- a/src/libcamera/software_isp/software_isp.cpp > +++ b/src/libcamera/software_isp/software_isp.cpp > @@ -301,6 +301,8 @@ int SoftwareIsp::queueBuffers(uint32_t frame, FrameBuffer *input, > return -EINVAL; > } > > + queuedInputBuffers_.push_back(input); > + > for (auto iter = outputs.begin(); iter != outputs.end(); iter++) { > FrameBuffer *const buffer = iter->second; > queuedOutputBuffers_.push_back(buffer); > @@ -327,6 +329,9 @@ int SoftwareIsp::start() > > /** > * \brief Stops the Software ISP streaming operation > + * > + * All pending buffers are returned back (output buffers as canceled) before > + * this method finishes. Shouldn't the input buffer be cancelled too ? > */ > void SoftwareIsp::stop() > { > @@ -342,6 +347,10 @@ void SoftwareIsp::stop() > outputBufferReady.emit(buffer); > } > queuedOutputBuffers_.clear(); > + > + for (auto buffer : queuedInputBuffers_) > + inputBufferReady.emit(buffer); > + queuedInputBuffers_.clear(); > } > > /** > @@ -375,7 +384,12 @@ void SoftwareIsp::statsReady(uint32_t frame, uint32_t bufferId) > > void SoftwareIsp::inputReady(FrameBuffer *input) > { > - inputBufferReady.emit(input); > + if (running_) { > + queuedInputBuffers_.erase(find(queuedInputBuffers_.begin(), > + queuedInputBuffers_.end(), > + input)); Same comment as for 2/5. > + inputBufferReady.emit(input); > + } > } > > void SoftwareIsp::outputReady(FrameBuffer *output)
Hi Laurent, thank you for review. Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > Hi Milan, > > Thank you for the patch. > > On Mon, Feb 24, 2025 at 07:52:33PM +0100, Milan Zamazal wrote: >> When SoftwareIsp stops, input and output buffers queued to it may not >> yet be fully processed. They will be eventually returned but stop means >> stop, there should be no processing related actions invoked afterwards. >> >> Let's stop forwarding processed input buffers from SoftwareIsp slots >> when SoftwareIsp is stopped. Let's track the queued input buffers and >> return them back for capture in SoftwareIsp::stop(). >> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> .../internal/software_isp/software_isp.h | 1 + >> src/libcamera/software_isp/software_isp.cpp | 16 +++++++++++++++- >> 2 files changed, 16 insertions(+), 1 deletion(-) >> >> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h >> index f2344355..bfe34725 100644 >> --- a/include/libcamera/internal/software_isp/software_isp.h >> +++ b/include/libcamera/internal/software_isp/software_isp.h >> @@ -102,6 +102,7 @@ private: >> std::unique_ptr<ipa::soft::IPAProxySoft> ipa_; >> bool running_; >> std::deque<FrameBuffer *> queuedOutputBuffers_; >> + std::deque<FrameBuffer *> queuedInputBuffers_; > > I'd move input before output. > >> }; >> >> } /* namespace libcamera */ >> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp >> index 4339e547..3a605ab2 100644 >> --- a/src/libcamera/software_isp/software_isp.cpp >> +++ b/src/libcamera/software_isp/software_isp.cpp >> @@ -301,6 +301,8 @@ int SoftwareIsp::queueBuffers(uint32_t frame, FrameBuffer *input, >> return -EINVAL; >> } >> >> + queuedInputBuffers_.push_back(input); >> + >> for (auto iter = outputs.begin(); iter != outputs.end(); iter++) { >> FrameBuffer *const buffer = iter->second; >> queuedOutputBuffers_.push_back(buffer); >> @@ -327,6 +329,9 @@ int SoftwareIsp::start() >> >> /** >> * \brief Stops the Software ISP streaming operation >> + * >> + * All pending buffers are returned back (output buffers as canceled) before >> + * this method finishes. > > Shouldn't the input buffer be cancelled too ? Is it necessary to cancel input buffers? Aren't they just returned for re-use regardless of their contents (unlike the output buffers where the validity of the contents matters)? The input buffer lands in V4L2VideoDevice::queueBuffer() and I can't see any use of the status there. >> */ >> void SoftwareIsp::stop() >> { >> @@ -342,6 +347,10 @@ void SoftwareIsp::stop() >> outputBufferReady.emit(buffer); >> } >> queuedOutputBuffers_.clear(); >> + >> + for (auto buffer : queuedInputBuffers_) >> + inputBufferReady.emit(buffer); >> + queuedInputBuffers_.clear(); >> } >> >> /** >> @@ -375,7 +384,12 @@ void SoftwareIsp::statsReady(uint32_t frame, uint32_t bufferId) >> >> void SoftwareIsp::inputReady(FrameBuffer *input) >> { >> - inputBufferReady.emit(input); >> + if (running_) { >> + queuedInputBuffers_.erase(find(queuedInputBuffers_.begin(), >> + queuedInputBuffers_.end(), >> + input)); > > Same comment as for 2/5. > >> + inputBufferReady.emit(input); >> + } >> } >> >> void SoftwareIsp::outputReady(FrameBuffer *output)
On Mon, Feb 24, 2025 at 09:19:53PM +0100, Milan Zamazal wrote: > Laurent Pinchart writes: > > On Mon, Feb 24, 2025 at 07:52:33PM +0100, Milan Zamazal wrote: > >> When SoftwareIsp stops, input and output buffers queued to it may not > >> yet be fully processed. They will be eventually returned but stop means > >> stop, there should be no processing related actions invoked afterwards. > >> > >> Let's stop forwarding processed input buffers from SoftwareIsp slots > >> when SoftwareIsp is stopped. Let's track the queued input buffers and > >> return them back for capture in SoftwareIsp::stop(). > >> > >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > >> --- > >> .../internal/software_isp/software_isp.h | 1 + > >> src/libcamera/software_isp/software_isp.cpp | 16 +++++++++++++++- > >> 2 files changed, 16 insertions(+), 1 deletion(-) > >> > >> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h > >> index f2344355..bfe34725 100644 > >> --- a/include/libcamera/internal/software_isp/software_isp.h > >> +++ b/include/libcamera/internal/software_isp/software_isp.h > >> @@ -102,6 +102,7 @@ private: > >> std::unique_ptr<ipa::soft::IPAProxySoft> ipa_; > >> bool running_; > >> std::deque<FrameBuffer *> queuedOutputBuffers_; > >> + std::deque<FrameBuffer *> queuedInputBuffers_; > > > > I'd move input before output. > > > >> }; > >> > >> } /* namespace libcamera */ > >> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp > >> index 4339e547..3a605ab2 100644 > >> --- a/src/libcamera/software_isp/software_isp.cpp > >> +++ b/src/libcamera/software_isp/software_isp.cpp > >> @@ -301,6 +301,8 @@ int SoftwareIsp::queueBuffers(uint32_t frame, FrameBuffer *input, > >> return -EINVAL; > >> } > >> > >> + queuedInputBuffers_.push_back(input); > >> + > >> for (auto iter = outputs.begin(); iter != outputs.end(); iter++) { > >> FrameBuffer *const buffer = iter->second; > >> queuedOutputBuffers_.push_back(buffer); > >> @@ -327,6 +329,9 @@ int SoftwareIsp::start() > >> > >> /** > >> * \brief Stops the Software ISP streaming operation > >> + * > >> + * All pending buffers are returned back (output buffers as canceled) before > >> + * this method finishes. > > > > Shouldn't the input buffer be cancelled too ? > > Is it necessary to cancel input buffers? Aren't they just returned for > re-use regardless of their contents (unlike the output buffers where the > validity of the contents matters)? The input buffer lands in > V4L2VideoDevice::queueBuffer() and I can't see any use of the status > there. At the moment we don't use the status, but I think it would be nicer to still mark it appropriately. If the input buffer hasn't been fully consumed, marking it as cancelled will let pipeline handlers deal with this if they need to. If marking it as cancelled causes other issues then we can reconsider that, but if it's easy I'd do so for completeness. > >> */ > >> void SoftwareIsp::stop() > >> { > >> @@ -342,6 +347,10 @@ void SoftwareIsp::stop() > >> outputBufferReady.emit(buffer); > >> } > >> queuedOutputBuffers_.clear(); > >> + > >> + for (auto buffer : queuedInputBuffers_) > >> + inputBufferReady.emit(buffer); > >> + queuedInputBuffers_.clear(); > >> } > >> > >> /** > >> @@ -375,7 +384,12 @@ void SoftwareIsp::statsReady(uint32_t frame, uint32_t bufferId) > >> > >> void SoftwareIsp::inputReady(FrameBuffer *input) > >> { > >> - inputBufferReady.emit(input); > >> + if (running_) { > >> + queuedInputBuffers_.erase(find(queuedInputBuffers_.begin(), > >> + queuedInputBuffers_.end(), > >> + input)); > > > > Same comment as for 2/5. > > > >> + inputBufferReady.emit(input); > >> + } > >> } > >> > >> void SoftwareIsp::outputReady(FrameBuffer *output)
Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > On Mon, Feb 24, 2025 at 09:19:53PM +0100, Milan Zamazal wrote: >> Laurent Pinchart writes: >> > On Mon, Feb 24, 2025 at 07:52:33PM +0100, Milan Zamazal wrote: > >> >> When SoftwareIsp stops, input and output buffers queued to it may not >> >> yet be fully processed. They will be eventually returned but stop means >> >> stop, there should be no processing related actions invoked afterwards. >> >> >> >> Let's stop forwarding processed input buffers from SoftwareIsp slots >> >> when SoftwareIsp is stopped. Let's track the queued input buffers and >> >> return them back for capture in SoftwareIsp::stop(). >> >> >> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> >> --- >> >> .../internal/software_isp/software_isp.h | 1 + >> >> src/libcamera/software_isp/software_isp.cpp | 16 +++++++++++++++- >> >> 2 files changed, 16 insertions(+), 1 deletion(-) >> >> >> >> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h >> >> index f2344355..bfe34725 100644 >> >> --- a/include/libcamera/internal/software_isp/software_isp.h >> >> +++ b/include/libcamera/internal/software_isp/software_isp.h >> >> @@ -102,6 +102,7 @@ private: >> >> std::unique_ptr<ipa::soft::IPAProxySoft> ipa_; >> >> bool running_; >> >> std::deque<FrameBuffer *> queuedOutputBuffers_; >> >> + std::deque<FrameBuffer *> queuedInputBuffers_; >> > >> > I'd move input before output. >> > >> >> }; >> >> >> >> } /* namespace libcamera */ >> >> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp >> >> index 4339e547..3a605ab2 100644 >> >> --- a/src/libcamera/software_isp/software_isp.cpp >> >> +++ b/src/libcamera/software_isp/software_isp.cpp >> >> @@ -301,6 +301,8 @@ int SoftwareIsp::queueBuffers(uint32_t frame, FrameBuffer *input, >> >> return -EINVAL; >> >> } >> >> >> >> + queuedInputBuffers_.push_back(input); >> >> + >> >> for (auto iter = outputs.begin(); iter != outputs.end(); iter++) { >> >> FrameBuffer *const buffer = iter->second; >> >> queuedOutputBuffers_.push_back(buffer); >> >> @@ -327,6 +329,9 @@ int SoftwareIsp::start() >> >> >> >> /** >> >> * \brief Stops the Software ISP streaming operation >> >> + * >> >> + * All pending buffers are returned back (output buffers as canceled) before >> >> + * this method finishes. >> > >> > Shouldn't the input buffer be cancelled too ? >> >> Is it necessary to cancel input buffers? Aren't they just returned for >> re-use regardless of their contents (unlike the output buffers where the >> validity of the contents matters)? The input buffer lands in >> V4L2VideoDevice::queueBuffer() and I can't see any use of the status >> there. > > At the moment we don't use the status, but I think it would be nicer to > still mark it appropriately. If the input buffer hasn't been fully > consumed, marking it as cancelled will let pipeline handlers deal with > this if they need to. OK. > If marking it as cancelled causes other issues then we can reconsider > that, but if it's easy I'd do so for completeness. Works for me. But I'm not much comfortable with the fact that then the buffer may keep FrameCancelled status forever and float with it around. Should SimpleCameraData::conversionInputDone() or something else reset the status? >> >> */ >> >> void SoftwareIsp::stop() >> >> { >> >> @@ -342,6 +347,10 @@ void SoftwareIsp::stop() >> >> outputBufferReady.emit(buffer); >> >> } >> >> queuedOutputBuffers_.clear(); >> >> + >> >> + for (auto buffer : queuedInputBuffers_) >> >> + inputBufferReady.emit(buffer); >> >> + queuedInputBuffers_.clear(); >> >> } >> >> >> >> /** >> >> @@ -375,7 +384,12 @@ void SoftwareIsp::statsReady(uint32_t frame, uint32_t bufferId) >> >> >> >> void SoftwareIsp::inputReady(FrameBuffer *input) >> >> { >> >> - inputBufferReady.emit(input); >> >> + if (running_) { >> >> + queuedInputBuffers_.erase(find(queuedInputBuffers_.begin(), >> >> + queuedInputBuffers_.end(), >> >> + input)); >> > >> > Same comment as for 2/5. >> > >> >> + inputBufferReady.emit(input); >> >> + } >> >> } >> >> >> >> void SoftwareIsp::outputReady(FrameBuffer *output)
On Mon, Feb 24, 2025 at 10:07:43PM +0100, Milan Zamazal wrote: > Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > > > On Mon, Feb 24, 2025 at 09:19:53PM +0100, Milan Zamazal wrote: > >> Laurent Pinchart writes: > >> > On Mon, Feb 24, 2025 at 07:52:33PM +0100, Milan Zamazal wrote: > > > >> >> When SoftwareIsp stops, input and output buffers queued to it may not > >> >> yet be fully processed. They will be eventually returned but stop means > >> >> stop, there should be no processing related actions invoked afterwards. > >> >> > >> >> Let's stop forwarding processed input buffers from SoftwareIsp slots > >> >> when SoftwareIsp is stopped. Let's track the queued input buffers and > >> >> return them back for capture in SoftwareIsp::stop(). > >> >> > >> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > >> >> --- > >> >> .../internal/software_isp/software_isp.h | 1 + > >> >> src/libcamera/software_isp/software_isp.cpp | 16 +++++++++++++++- > >> >> 2 files changed, 16 insertions(+), 1 deletion(-) > >> >> > >> >> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h > >> >> index f2344355..bfe34725 100644 > >> >> --- a/include/libcamera/internal/software_isp/software_isp.h > >> >> +++ b/include/libcamera/internal/software_isp/software_isp.h > >> >> @@ -102,6 +102,7 @@ private: > >> >> std::unique_ptr<ipa::soft::IPAProxySoft> ipa_; > >> >> bool running_; > >> >> std::deque<FrameBuffer *> queuedOutputBuffers_; > >> >> + std::deque<FrameBuffer *> queuedInputBuffers_; > >> > > >> > I'd move input before output. > >> > > >> >> }; > >> >> > >> >> } /* namespace libcamera */ > >> >> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp > >> >> index 4339e547..3a605ab2 100644 > >> >> --- a/src/libcamera/software_isp/software_isp.cpp > >> >> +++ b/src/libcamera/software_isp/software_isp.cpp > >> >> @@ -301,6 +301,8 @@ int SoftwareIsp::queueBuffers(uint32_t frame, FrameBuffer *input, > >> >> return -EINVAL; > >> >> } > >> >> > >> >> + queuedInputBuffers_.push_back(input); > >> >> + > >> >> for (auto iter = outputs.begin(); iter != outputs.end(); iter++) { > >> >> FrameBuffer *const buffer = iter->second; > >> >> queuedOutputBuffers_.push_back(buffer); > >> >> @@ -327,6 +329,9 @@ int SoftwareIsp::start() > >> >> > >> >> /** > >> >> * \brief Stops the Software ISP streaming operation > >> >> + * > >> >> + * All pending buffers are returned back (output buffers as canceled) before > >> >> + * this method finishes. > >> > > >> > Shouldn't the input buffer be cancelled too ? > >> > >> Is it necessary to cancel input buffers? Aren't they just returned for > >> re-use regardless of their contents (unlike the output buffers where the > >> validity of the contents matters)? The input buffer lands in > >> V4L2VideoDevice::queueBuffer() and I can't see any use of the status > >> there. > > > > At the moment we don't use the status, but I think it would be nicer to > > still mark it appropriately. If the input buffer hasn't been fully > > consumed, marking it as cancelled will let pipeline handlers deal with > > this if they need to. > > OK. > > > If marking it as cancelled causes other issues then we can reconsider > > that, but if it's easy I'd do so for completeness. > > Works for me. But I'm not much comfortable with the fact that then the > buffer may keep FrameCancelled status forever and float with it around. > Should SimpleCameraData::conversionInputDone() or something else reset > the status? Aren't the buffers freed in SimplePipelineHandler::stopDevice() by data->conversionBuffers_.clear(); ? > >> >> */ > >> >> void SoftwareIsp::stop() > >> >> { > >> >> @@ -342,6 +347,10 @@ void SoftwareIsp::stop() > >> >> outputBufferReady.emit(buffer); > >> >> } > >> >> queuedOutputBuffers_.clear(); > >> >> + > >> >> + for (auto buffer : queuedInputBuffers_) > >> >> + inputBufferReady.emit(buffer); > >> >> + queuedInputBuffers_.clear(); > >> >> } > >> >> > >> >> /** > >> >> @@ -375,7 +384,12 @@ void SoftwareIsp::statsReady(uint32_t frame, uint32_t bufferId) > >> >> > >> >> void SoftwareIsp::inputReady(FrameBuffer *input) > >> >> { > >> >> - inputBufferReady.emit(input); > >> >> + if (running_) { > >> >> + queuedInputBuffers_.erase(find(queuedInputBuffers_.begin(), > >> >> + queuedInputBuffers_.end(), > >> >> + input)); > >> > > >> > Same comment as for 2/5. > >> > > >> >> + inputBufferReady.emit(input); > >> >> + } > >> >> } > >> >> > >> >> void SoftwareIsp::outputReady(FrameBuffer *output)
diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h index f2344355..bfe34725 100644 --- a/include/libcamera/internal/software_isp/software_isp.h +++ b/include/libcamera/internal/software_isp/software_isp.h @@ -102,6 +102,7 @@ private: std::unique_ptr<ipa::soft::IPAProxySoft> ipa_; bool running_; std::deque<FrameBuffer *> queuedOutputBuffers_; + std::deque<FrameBuffer *> queuedInputBuffers_; }; } /* namespace libcamera */ diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp index 4339e547..3a605ab2 100644 --- a/src/libcamera/software_isp/software_isp.cpp +++ b/src/libcamera/software_isp/software_isp.cpp @@ -301,6 +301,8 @@ int SoftwareIsp::queueBuffers(uint32_t frame, FrameBuffer *input, return -EINVAL; } + queuedInputBuffers_.push_back(input); + for (auto iter = outputs.begin(); iter != outputs.end(); iter++) { FrameBuffer *const buffer = iter->second; queuedOutputBuffers_.push_back(buffer); @@ -327,6 +329,9 @@ int SoftwareIsp::start() /** * \brief Stops the Software ISP streaming operation + * + * All pending buffers are returned back (output buffers as canceled) before + * this method finishes. */ void SoftwareIsp::stop() { @@ -342,6 +347,10 @@ void SoftwareIsp::stop() outputBufferReady.emit(buffer); } queuedOutputBuffers_.clear(); + + for (auto buffer : queuedInputBuffers_) + inputBufferReady.emit(buffer); + queuedInputBuffers_.clear(); } /** @@ -375,7 +384,12 @@ void SoftwareIsp::statsReady(uint32_t frame, uint32_t bufferId) void SoftwareIsp::inputReady(FrameBuffer *input) { - inputBufferReady.emit(input); + if (running_) { + queuedInputBuffers_.erase(find(queuedInputBuffers_.begin(), + queuedInputBuffers_.end(), + input)); + inputBufferReady.emit(input); + } } void SoftwareIsp::outputReady(FrameBuffer *output)
When SoftwareIsp stops, input and output buffers queued to it may not yet be fully processed. They will be eventually returned but stop means stop, there should be no processing related actions invoked afterwards. Let's stop forwarding processed input buffers from SoftwareIsp slots when SoftwareIsp is stopped. Let's track the queued input buffers and return them back for capture in SoftwareIsp::stop(). Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- .../internal/software_isp/software_isp.h | 1 + src/libcamera/software_isp/software_isp.cpp | 16 +++++++++++++++- 2 files changed, 16 insertions(+), 1 deletion(-)