| Message ID | 20251210003139.43606-16-bryan.odonoghue@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes: > In order to initialise and deinitialise gpuisp we need to be able to setup > EGL in the same thread as Debayer::process() happens in. > > In order to do this we need to extend the Debayer object to provide start > and stop methods which are triggered through invokeMethod in the same way > as process() is. > > Introduce start() and stop() methods to the debayer class. Trigger those > methods as described above via invokeMethod. The debayer_egl class will > take care of initialising and de-initialising as necessary. Debayer CPU > sees no functional change. > > [bod: Made method blocking not queued per Robert's bugfixes] > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > --- > src/libcamera/software_isp/debayer.cpp | 18 ++++++++++++++++++ > src/libcamera/software_isp/debayer.h | 2 ++ > src/libcamera/software_isp/software_isp.cpp | 8 ++++++-- > 3 files changed, 26 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp > index 1d135b278..32a2c8378 100644 > --- a/src/libcamera/software_isp/debayer.cpp > +++ b/src/libcamera/software_isp/debayer.cpp > @@ -336,6 +336,24 @@ Debayer::~Debayer() > * debayer processing. > */ > > +/** > + * \fn int Debayer::start() > + * \brief Execute a start signal in the debayer object from workerthread context. > + * > + * In the DebayerCPU case this is an empty stub function but > + * for the GPU case this does something useful. The stub here is to > + * ensure the right version of the virtual gets called. I think this paragraph can be omitted. Instead, the purpose and the return value of the method should be described; it's unclear what "start signal" is. Perhaps something similar to SoftwareIsp::start() docstring could be used. > + */ > + > +/** > + * \fn void Debayer::stop() > + * \brief Stop the debayering process and perform cleanup > + * > + * In the DebayerCPU case this is an empty stub function but > + * for the GPU case this does something useful. The stub here is to > + * ensure the right version of the virtual gets called. Similarly, let's replace this paragraph with something else. > + */ > + > /** > * \fn void Debayer::setParams(DebayerParams ¶ms) > * \brief Select the bayer params to use for the next frame debayer > diff --git a/src/libcamera/software_isp/debayer.h b/src/libcamera/software_isp/debayer.h > index ff4a92c15..5c0cb3914 100644 > --- a/src/libcamera/software_isp/debayer.h > +++ b/src/libcamera/software_isp/debayer.h > @@ -48,6 +48,8 @@ public: > strideAndFrameSize(const PixelFormat &outputFormat, const Size &size) = 0; > > virtual void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params) = 0; > + virtual int start() { return 0; } > + virtual void stop() { } > > virtual SizeRange sizes(PixelFormat inputFormat, const Size &inputSize) = 0; > > diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp > index 928a2520c..7c9ad9160 100644 > --- a/src/libcamera/software_isp/software_isp.cpp > +++ b/src/libcamera/software_isp/software_isp.cpp > @@ -347,7 +347,9 @@ int SoftwareIsp::start() > return ret; > > ispWorkerThread_.start(); > - return 0; > + > + return debayer_->invokeMethod(&DebayerCpu::start, > + ConnectionTypeBlocking); DebayerCpu or Debayer? > } > > /** > @@ -358,9 +360,11 @@ int SoftwareIsp::start() > */ > void SoftwareIsp::stop() > { > + debayer_->invokeMethod(&DebayerCpu::stop, > + ConnectionTypeBlocking); DebayerCpu or Debayer? > + > ispWorkerThread_.exit(); > ispWorkerThread_.wait(); > - ispWorkerThread_.removeMessages(debayer_.get()); Why removing this here? > > Thread::current()->dispatchMessages(Message::Type::InvokeMessage, this);
On 10/12/2025 09:13, Milan Zamazal wrote: > Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes: > >> In order to initialise and deinitialise gpuisp we need to be able to setup >> EGL in the same thread as Debayer::process() happens in. >> >> In order to do this we need to extend the Debayer object to provide start >> and stop methods which are triggered through invokeMethod in the same way >> as process() is. >> >> Introduce start() and stop() methods to the debayer class. Trigger those >> methods as described above via invokeMethod. The debayer_egl class will >> take care of initialising and de-initialising as necessary. Debayer CPU >> sees no functional change. >> >> [bod: Made method blocking not queued per Robert's bugfixes] >> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> >> --- >> src/libcamera/software_isp/debayer.cpp | 18 ++++++++++++++++++ >> src/libcamera/software_isp/debayer.h | 2 ++ >> src/libcamera/software_isp/software_isp.cpp | 8 ++++++-- >> 3 files changed, 26 insertions(+), 2 deletions(-) >> >> diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp >> index 1d135b278..32a2c8378 100644 >> --- a/src/libcamera/software_isp/debayer.cpp >> +++ b/src/libcamera/software_isp/debayer.cpp >> @@ -336,6 +336,24 @@ Debayer::~Debayer() >> * debayer processing. >> */ >> >> +/** >> + * \fn int Debayer::start() >> + * \brief Execute a start signal in the debayer object from workerthread context. >> + * >> + * In the DebayerCPU case this is an empty stub function but >> + * for the GPU case this does something useful. The stub here is to >> + * ensure the right version of the virtual gets called. > > I think this paragraph can be omitted. Instead, the purpose and the > return value of the method should be described; it's unclear what "start > signal" is. Perhaps something similar to SoftwareIsp::start() > docstring could be used. > >> + */ >> + >> +/** >> + * \fn void Debayer::stop() >> + * \brief Stop the debayering process and perform cleanup >> + * >> + * In the DebayerCPU case this is an empty stub function but >> + * for the GPU case this does something useful. The stub here is to >> + * ensure the right version of the virtual gets called. > > Similarly, let's replace this paragraph with something else. Sure. > >> + */ >> + >> /** >> * \fn void Debayer::setParams(DebayerParams ¶ms) >> * \brief Select the bayer params to use for the next frame debayer >> diff --git a/src/libcamera/software_isp/debayer.h b/src/libcamera/software_isp/debayer.h >> index ff4a92c15..5c0cb3914 100644 >> --- a/src/libcamera/software_isp/debayer.h >> +++ b/src/libcamera/software_isp/debayer.h >> @@ -48,6 +48,8 @@ public: >> strideAndFrameSize(const PixelFormat &outputFormat, const Size &size) = 0; >> >> virtual void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params) = 0; >> + virtual int start() { return 0; } >> + virtual void stop() { } >> >> virtual SizeRange sizes(PixelFormat inputFormat, const Size &inputSize) = 0; >> >> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp >> index 928a2520c..7c9ad9160 100644 >> --- a/src/libcamera/software_isp/software_isp.cpp >> +++ b/src/libcamera/software_isp/software_isp.cpp >> @@ -347,7 +347,9 @@ int SoftwareIsp::start() >> return ret; >> >> ispWorkerThread_.start(); >> - return 0; >> + >> + return debayer_->invokeMethod(&DebayerCpu::start, >> + ConnectionTypeBlocking); > > DebayerCpu or Debayer? DebayerCPU. The next patch changes the type to virtual base class. This patch adds the methods. > >> } >> >> /** >> @@ -358,9 +360,11 @@ int SoftwareIsp::start() >> */ >> void SoftwareIsp::stop() >> { >> + debayer_->invokeMethod(&DebayerCpu::stop, >> + ConnectionTypeBlocking); > > DebayerCpu or Debayer? > >> + >> ispWorkerThread_.exit(); >> ispWorkerThread_.wait(); >> - ispWorkerThread_.removeMessages(debayer_.get()); > > Why removing this here? > >> >> Thread::current()->dispatchMessages(Message::Type::InvokeMessage, this); > Per Barnabas comment in the previous version, making the call blocking, means we should remove the removeMessages() call. --- bod
Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes: > On 10/12/2025 09:13, Milan Zamazal wrote: >> Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes: >> > >>> In order to initialise and deinitialise gpuisp we need to be able to setup >>> EGL in the same thread as Debayer::process() happens in. >>> >>> In order to do this we need to extend the Debayer object to provide start >>> and stop methods which are triggered through invokeMethod in the same way >>> as process() is. >>> >>> Introduce start() and stop() methods to the debayer class. Trigger those >>> methods as described above via invokeMethod. The debayer_egl class will >>> take care of initialising and de-initialising as necessary. Debayer CPU >>> sees no functional change. >>> >>> [bod: Made method blocking not queued per Robert's bugfixes] >>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> >>> --- >>> src/libcamera/software_isp/debayer.cpp | 18 ++++++++++++++++++ >>> src/libcamera/software_isp/debayer.h | 2 ++ >>> src/libcamera/software_isp/software_isp.cpp | 8 ++++++-- >>> 3 files changed, 26 insertions(+), 2 deletions(-) >>> >>> diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp >>> index 1d135b278..32a2c8378 100644 >>> --- a/src/libcamera/software_isp/debayer.cpp >>> +++ b/src/libcamera/software_isp/debayer.cpp >>> @@ -336,6 +336,24 @@ Debayer::~Debayer() >>> * debayer processing. >>> */ >>> +/** >>> + * \fn int Debayer::start() >>> + * \brief Execute a start signal in the debayer object from workerthread context. >>> + * >>> + * In the DebayerCPU case this is an empty stub function but >>> + * for the GPU case this does something useful. The stub here is to >>> + * ensure the right version of the virtual gets called. >> I think this paragraph can be omitted. Instead, the purpose and the >> return value of the method should be described; it's unclear what "start >> signal" is. Perhaps something similar to SoftwareIsp::start() >> docstring could be used. >> >>> + */ >>> + >>> +/** >>> + * \fn void Debayer::stop() >>> + * \brief Stop the debayering process and perform cleanup >>> + * >>> + * In the DebayerCPU case this is an empty stub function but >>> + * for the GPU case this does something useful. The stub here is to >>> + * ensure the right version of the virtual gets called. >> Similarly, let's replace this paragraph with something else. > > Sure. > >> >>> + */ >>> + >>> /** >>> * \fn void Debayer::setParams(DebayerParams ¶ms) >>> * \brief Select the bayer params to use for the next frame debayer >>> diff --git a/src/libcamera/software_isp/debayer.h b/src/libcamera/software_isp/debayer.h >>> index ff4a92c15..5c0cb3914 100644 >>> --- a/src/libcamera/software_isp/debayer.h >>> +++ b/src/libcamera/software_isp/debayer.h >>> @@ -48,6 +48,8 @@ public: >>> strideAndFrameSize(const PixelFormat &outputFormat, const Size &size) = 0; >>> virtual void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params) = 0; >>> + virtual int start() { return 0; } >>> + virtual void stop() { } >>> virtual SizeRange sizes(PixelFormat inputFormat, const Size &inputSize) = 0; >>> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp >>> index 928a2520c..7c9ad9160 100644 >>> --- a/src/libcamera/software_isp/software_isp.cpp >>> +++ b/src/libcamera/software_isp/software_isp.cpp >>> @@ -347,7 +347,9 @@ int SoftwareIsp::start() >>> return ret; >>> ispWorkerThread_.start(); >>> - return 0; >>> + >>> + return debayer_->invokeMethod(&DebayerCpu::start, >>> + ConnectionTypeBlocking); >> DebayerCpu or Debayer? > > DebayerCPU. > > The next patch changes the type to virtual base class. This patch adds the methods. > >> >>> } >>> /** >>> @@ -358,9 +360,11 @@ int SoftwareIsp::start() >>> */ >>> void SoftwareIsp::stop() >>> { >>> + debayer_->invokeMethod(&DebayerCpu::stop, >>> + ConnectionTypeBlocking); >> DebayerCpu or Debayer? >> >>> + >>> ispWorkerThread_.exit(); >>> ispWorkerThread_.wait(); >>> - ispWorkerThread_.removeMessages(debayer_.get()); >> Why removing this here? >> >>> Thread::current()->dispatchMessages(Message::Type::InvokeMessage, this); >> > > Per Barnabas comment in the previous version, making the call blocking, means we should remove the removeMessages() call. I see. It would be worth to mention this in the commit message. > --- > bod
diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp index 1d135b278..32a2c8378 100644 --- a/src/libcamera/software_isp/debayer.cpp +++ b/src/libcamera/software_isp/debayer.cpp @@ -336,6 +336,24 @@ Debayer::~Debayer() * debayer processing. */ +/** + * \fn int Debayer::start() + * \brief Execute a start signal in the debayer object from workerthread context. + * + * In the DebayerCPU case this is an empty stub function but + * for the GPU case this does something useful. The stub here is to + * ensure the right version of the virtual gets called. + */ + +/** + * \fn void Debayer::stop() + * \brief Stop the debayering process and perform cleanup + * + * In the DebayerCPU case this is an empty stub function but + * for the GPU case this does something useful. The stub here is to + * ensure the right version of the virtual gets called. + */ + /** * \fn void Debayer::setParams(DebayerParams ¶ms) * \brief Select the bayer params to use for the next frame debayer diff --git a/src/libcamera/software_isp/debayer.h b/src/libcamera/software_isp/debayer.h index ff4a92c15..5c0cb3914 100644 --- a/src/libcamera/software_isp/debayer.h +++ b/src/libcamera/software_isp/debayer.h @@ -48,6 +48,8 @@ public: strideAndFrameSize(const PixelFormat &outputFormat, const Size &size) = 0; virtual void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params) = 0; + virtual int start() { return 0; } + virtual void stop() { } virtual SizeRange sizes(PixelFormat inputFormat, const Size &inputSize) = 0; diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp index 928a2520c..7c9ad9160 100644 --- a/src/libcamera/software_isp/software_isp.cpp +++ b/src/libcamera/software_isp/software_isp.cpp @@ -347,7 +347,9 @@ int SoftwareIsp::start() return ret; ispWorkerThread_.start(); - return 0; + + return debayer_->invokeMethod(&DebayerCpu::start, + ConnectionTypeBlocking); } /** @@ -358,9 +360,11 @@ int SoftwareIsp::start() */ void SoftwareIsp::stop() { + debayer_->invokeMethod(&DebayerCpu::stop, + ConnectionTypeBlocking); + ispWorkerThread_.exit(); ispWorkerThread_.wait(); - ispWorkerThread_.removeMessages(debayer_.get()); Thread::current()->dispatchMessages(Message::Type::InvokeMessage, this);
In order to initialise and deinitialise gpuisp we need to be able to setup EGL in the same thread as Debayer::process() happens in. In order to do this we need to extend the Debayer object to provide start and stop methods which are triggered through invokeMethod in the same way as process() is. Introduce start() and stop() methods to the debayer class. Trigger those methods as described above via invokeMethod. The debayer_egl class will take care of initialising and de-initialising as necessary. Debayer CPU sees no functional change. [bod: Made method blocking not queued per Robert's bugfixes] Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- src/libcamera/software_isp/debayer.cpp | 18 ++++++++++++++++++ src/libcamera/software_isp/debayer.h | 2 ++ src/libcamera/software_isp/software_isp.cpp | 8 ++++++-- 3 files changed, 26 insertions(+), 2 deletions(-)