| Message ID | 20251015012251.17508-23-bryan.odonoghue@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Bryan, Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes: > The eGL class wants to be able to teardown its sync_ data member > properly but, doing so in the destructor means we can't make the eGL > context current and thus can't tear down the sync primitive properly. > > Introduce a stop() method to the debayer class which triggers from the > softisp's stop method, allowing a controlled and appropriate tear-down > of debayer-egl and egl class related data well before the destructors > get invoked. Looking around, I'm not sure stop() gets called only before the destructor, perhaps the instance survives camera stop-(re)start. But even then it looks like the correct thing to do. AFAICS: Reviewed-by: Milan Zamazal <mzamazal@redhat.com> > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > --- > src/libcamera/software_isp/debayer.h | 1 + > src/libcamera/software_isp/debayer_cpu.h | 1 + > src/libcamera/software_isp/software_isp.cpp | 3 +++ > 3 files changed, 5 insertions(+) > > diff --git a/src/libcamera/software_isp/debayer.h b/src/libcamera/software_isp/debayer.h > index c5eb0d38..8fa281c7 100644 > --- a/src/libcamera/software_isp/debayer.h > +++ b/src/libcamera/software_isp/debayer.h > @@ -48,6 +48,7 @@ public: > strideAndFrameSize(const PixelFormat &outputFormat, const Size &size) = 0; > > virtual void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params) = 0; > + virtual void stop() = 0; > > virtual SizeRange sizes(PixelFormat inputFormat, const Size &inputSize) = 0; > > diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h > index ff72eaba..3cc07028 100644 > --- a/src/libcamera/software_isp/debayer_cpu.h > +++ b/src/libcamera/software_isp/debayer_cpu.h > @@ -38,6 +38,7 @@ public: > std::tuple<unsigned int, unsigned int> > strideAndFrameSize(const PixelFormat &outputFormat, const Size &size); > void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params); > + void stop() {} > SizeRange sizes(PixelFormat inputFormat, const Size &inputSize); > > /** > diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp > index df72b1c3..1f984a52 100644 > --- a/src/libcamera/software_isp/software_isp.cpp > +++ b/src/libcamera/software_isp/software_isp.cpp > @@ -366,6 +366,9 @@ int SoftwareIsp::start() > */ > void SoftwareIsp::stop() > { > + debayer_->invokeMethod(&Debayer::stop, > + ConnectionTypeQueued); > + > ispWorkerThread_.exit(); > ispWorkerThread_.wait();
On 10/16/25 13:32, Milan Zamazal wrote: > Hi Bryan, > > Bryan O'Donoghue<bryan.odonoghue@linaro.org> writes: > >> The eGL class wants to be able to teardown its sync_ data member >> properly but, doing so in the destructor means we can't make the eGL >> context current and thus can't tear down the sync primitive properly. >> >> Introduce a stop() method to the debayer class which triggers from the >> softisp's stop method, allowing a controlled and appropriate tear-down >> of debayer-egl and egl class related data well before the destructors >> get invoked. > Looking around, I'm not sure stop() gets called only before the > destructor, perhaps the instance survives camera stop-(re)start. But > even then it looks like the correct thing to do. In fact I can reliably reproduce the assert in DebayerEGL::stop() -> eGL::cleanUp() when restarting a camera, switching back-and-forth between two cameras in Snapshot. IIRC that was not the case in v2 - did anything change here? > AFAICS: > > Reviewed-by: Milan Zamazal<mzamazal@redhat.com> > >> Signed-off-by: Bryan O'Donoghue<bryan.odonoghue@linaro.org> >> --- >> src/libcamera/software_isp/debayer.h | 1 + >> src/libcamera/software_isp/debayer_cpu.h | 1 + >> src/libcamera/software_isp/software_isp.cpp | 3 +++ >> 3 files changed, 5 insertions(+) >> >> diff --git a/src/libcamera/software_isp/debayer.h b/src/libcamera/software_isp/debayer.h >> index c5eb0d38..8fa281c7 100644 >> --- a/src/libcamera/software_isp/debayer.h >> +++ b/src/libcamera/software_isp/debayer.h >> @@ -48,6 +48,7 @@ public: >> strideAndFrameSize(const PixelFormat &outputFormat, const Size &size) = 0; >> >> virtual void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params) = 0; >> + virtual void stop() = 0; >> >> virtual SizeRange sizes(PixelFormat inputFormat, const Size &inputSize) = 0; >> >> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h >> index ff72eaba..3cc07028 100644 >> --- a/src/libcamera/software_isp/debayer_cpu.h >> +++ b/src/libcamera/software_isp/debayer_cpu.h >> @@ -38,6 +38,7 @@ public: >> std::tuple<unsigned int, unsigned int> >> strideAndFrameSize(const PixelFormat &outputFormat, const Size &size); >> void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params); >> + void stop() {} >> SizeRange sizes(PixelFormat inputFormat, const Size &inputSize); >> >> /** >> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp >> index df72b1c3..1f984a52 100644 >> --- a/src/libcamera/software_isp/software_isp.cpp >> +++ b/src/libcamera/software_isp/software_isp.cpp >> @@ -366,6 +366,9 @@ int SoftwareIsp::start() >> */ >> void SoftwareIsp::stop() >> { >> + debayer_->invokeMethod(&Debayer::stop, >> + ConnectionTypeQueued); >> + >> ispWorkerThread_.exit(); >> ispWorkerThread_.wait();
Hi 2025. 10. 15. 3:22 keltezéssel, Bryan O'Donoghue írta: > The eGL class wants to be able to teardown its sync_ data member > properly but, doing so in the destructor means we can't make the eGL > context current and thus can't tear down the sync primitive properly. > > Introduce a stop() method to the debayer class which triggers from the > softisp's stop method, allowing a controlled and appropriate tear-down > of debayer-egl and egl class related data well before the destructors > get invoked. > > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > --- > src/libcamera/software_isp/debayer.h | 1 + > src/libcamera/software_isp/debayer_cpu.h | 1 + > src/libcamera/software_isp/software_isp.cpp | 3 +++ > 3 files changed, 5 insertions(+) > > diff --git a/src/libcamera/software_isp/debayer.h b/src/libcamera/software_isp/debayer.h > index c5eb0d38..8fa281c7 100644 > --- a/src/libcamera/software_isp/debayer.h > +++ b/src/libcamera/software_isp/debayer.h > @@ -48,6 +48,7 @@ public: > strideAndFrameSize(const PixelFormat &outputFormat, const Size &size) = 0; > > virtual void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params) = 0; > + virtual void stop() = 0; > > virtual SizeRange sizes(PixelFormat inputFormat, const Size &inputSize) = 0; > > diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h > index ff72eaba..3cc07028 100644 > --- a/src/libcamera/software_isp/debayer_cpu.h > +++ b/src/libcamera/software_isp/debayer_cpu.h > @@ -38,6 +38,7 @@ public: > std::tuple<unsigned int, unsigned int> > strideAndFrameSize(const PixelFormat &outputFormat, const Size &size); > void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params); > + void stop() {} > SizeRange sizes(PixelFormat inputFormat, const Size &inputSize); > > /** > diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp > index df72b1c3..1f984a52 100644 > --- a/src/libcamera/software_isp/software_isp.cpp > +++ b/src/libcamera/software_isp/software_isp.cpp > @@ -366,6 +366,9 @@ int SoftwareIsp::start() > */ > void SoftwareIsp::stop() > { > + debayer_->invokeMethod(&Debayer::stop, > + ConnectionTypeQueued); This should be `ConnectionTypeBlocking` otherwise it's not guaranteed to run since the thread is immediately stopped. Regards, Barnabás Pőcze > + > ispWorkerThread_.exit(); > ispWorkerThread_.wait(); > > -- > 2.51.0 >
On 16.10.25 13:39, Barnabás Pőcze wrote: >> diff --git a/src/libcamera/software_isp/software_isp.cpp >> b/src/libcamera/software_isp/software_isp.cpp >> index df72b1c3..1f984a52 100644 >> --- a/src/libcamera/software_isp/software_isp.cpp >> +++ b/src/libcamera/software_isp/software_isp.cpp >> @@ -366,6 +366,9 @@ int SoftwareIsp::start() >> */ >> void SoftwareIsp::stop() >> { >> + debayer_->invokeMethod(&Debayer::stop, >> + ConnectionTypeQueued); > > This should be `ConnectionTypeBlocking` otherwise it's not > guaranteed to run since the thread is immediately stopped. Nice, can confirm that doing that fixes the assertions I saw in DebayerEGL::stop() -> eGL::cleanUp() when restarting a camera. > > > Regards, > Barnabás Pőcze > >> + >> ispWorkerThread_.exit(); >> ispWorkerThread_.wait(); >> >> -- >> 2.51.0 >> >
diff --git a/src/libcamera/software_isp/debayer.h b/src/libcamera/software_isp/debayer.h index c5eb0d38..8fa281c7 100644 --- a/src/libcamera/software_isp/debayer.h +++ b/src/libcamera/software_isp/debayer.h @@ -48,6 +48,7 @@ public: strideAndFrameSize(const PixelFormat &outputFormat, const Size &size) = 0; virtual void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params) = 0; + virtual void stop() = 0; virtual SizeRange sizes(PixelFormat inputFormat, const Size &inputSize) = 0; diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h index ff72eaba..3cc07028 100644 --- a/src/libcamera/software_isp/debayer_cpu.h +++ b/src/libcamera/software_isp/debayer_cpu.h @@ -38,6 +38,7 @@ public: std::tuple<unsigned int, unsigned int> strideAndFrameSize(const PixelFormat &outputFormat, const Size &size); void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params); + void stop() {} SizeRange sizes(PixelFormat inputFormat, const Size &inputSize); /** diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp index df72b1c3..1f984a52 100644 --- a/src/libcamera/software_isp/software_isp.cpp +++ b/src/libcamera/software_isp/software_isp.cpp @@ -366,6 +366,9 @@ int SoftwareIsp::start() */ void SoftwareIsp::stop() { + debayer_->invokeMethod(&Debayer::stop, + ConnectionTypeQueued); + ispWorkerThread_.exit(); ispWorkerThread_.wait();
The eGL class wants to be able to teardown its sync_ data member properly but, doing so in the destructor means we can't make the eGL context current and thus can't tear down the sync primitive properly. Introduce a stop() method to the debayer class which triggers from the softisp's stop method, allowing a controlled and appropriate tear-down of debayer-egl and egl class related data well before the destructors get invoked. Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- src/libcamera/software_isp/debayer.h | 1 + src/libcamera/software_isp/debayer_cpu.h | 1 + src/libcamera/software_isp/software_isp.cpp | 3 +++ 3 files changed, 5 insertions(+)