| Message ID | 20251211232246.31330-16-bryan.odonoghue@linaro.org |
|---|---|
| State | Accepted |
| 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. > > This requires extending 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. > > Per feedback from Barnabas the stop method using blocking synchronisation ... is using ... ? > and thus we drop ispWorkerThread_.removeMessages(). > > [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 | 25 +++++++++++++++++++++ > src/libcamera/software_isp/debayer.h | 2 ++ > src/libcamera/software_isp/software_isp.cpp | 8 +++++-- > 3 files changed, 33 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp > index 1d135b278..8b4b6cd74 100644 > --- a/src/libcamera/software_isp/debayer.cpp > +++ b/src/libcamera/software_isp/debayer.cpp > @@ -336,6 +336,31 @@ Debayer::~Debayer() > * debayer processing. > */ > > +/** > + * \fn int Debayer::start() > + * \brief Execute a start signal in the debayer object from workerthread context. s/\.$// Reviewed-by: Milan Zamazal <mzamazal@redhat.com> > + * > + * The start() method is invoked so that a Debayer object can initialise > + * internal variables or data. It is called from the software_isp::start > + * method. > + * > + * This method is particularly useful with DebayerEGL as it allows for the > + * initialisation of the EGL stack after configure in a thread-safe manner. > + */ > + > +/** > + * \fn void Debayer::stop() > + * \brief Stop the debayering process and perform cleanup > + * > + * The stop() method is invoked as the logically corollary of start(). > + * Debayer::stop() will be called by software_isp::stop() allowing for any > + * cleanup which should happend with stop(). > + * > + * The stop method similar to start() is useful for DebayerEGL as it allows > + * for cleanup of EGL context and/or data that happens in > + * DebayerEGL::start. > + */ > + > /** > * \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);
On 12/12/2025 13:29, Milan Zamazal wrote: >> Per feedback from Barnabas the stop method using blocking synchronisation > ... is using ... ? Eh sure. I don't think an entire v6 for one typo is a bit much. Perhaps Kieran would be kind enough to fix it up on application. --- bod
Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes: > On 12/12/2025 13:29, Milan Zamazal wrote: >>> Per feedback from Barnabas the stop method using blocking synchronisation >> ... is using ... ? > > Eh sure. > > I don't think an entire v6 for one typo is a bit much. Certainly. The precursor series seems to work fine for me and my Reviewed-By is now everywhere where appropriate. > Perhaps Kieran would be kind enough to fix it up on application. Yep, a good reason to apply the patches. :-)
Quoting Milan Zamazal (2025-12-12 18:36:50) > Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes: > > > On 12/12/2025 13:29, Milan Zamazal wrote: > >>> Per feedback from Barnabas the stop method using blocking synchronisation > >> ... is using ... ? > > > > Eh sure. > > > > I don't think an entire v6 for one typo is a bit much. > > Certainly. The precursor series seems to work fine for me and my > Reviewed-By is now everywhere where appropriate. > > > Perhaps Kieran would be kind enough to fix it up on application. Fixed. Also see below > Yep, a good reason to apply the patches. :-) Applied - anything to reduce the number of patches repeating in my inbox hey ;-) Bryan - there are /a lot/ of trivial checkpatch issues coming up in these patches. Please make sure you set up the checkstyle post-commit hook. cp utils/hooks/post-commit .git/hooks/post-commit These are all trivial - and easy to fix when you see them with "./utils/checkpatch | patch -p0" and then 'git add -p' the fixes to squash as you like. I've done that in this series to stop another iteration on trivials, with the following diff: (changes squahsed into their relevant commits) diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp index b5b99d39027e..b33f818a7829 100644 --- a/src/libcamera/software_isp/debayer.cpp +++ b/src/libcamera/software_isp/debayer.cpp @@ -205,23 +205,23 @@ Debayer::~Debayer() /** * \fn const SharedFD &Debayer::getStatsFD() - * \brief Get the file descriptor for the statistics. + * \brief Get the file descriptor for the statistics * * This file descriptor provides access to the output statistics buffer * associated with the current debayering process. * - * \return The file descriptor pointing to the statistics data. + * \return The file descriptor pointing to the statistics data */ /** * \fn unsigned int Debayer::frameSize() - * \brief Get the output frame size. + * \brief Get the output frame size * * \return The total output frame size in bytes as configured for the * current stream. */ - /** +/** * \var Signal<FrameBuffer *> Debayer::inputBufferReady * \brief Signals when the input buffer is ready */ @@ -233,7 +233,7 @@ Debayer::~Debayer() /** * \struct Debayer::DebayerInputConfig - * \brief Structure describing the incoming Bayer parameters. + * \brief Structure describing the incoming Bayer parameters * * The DebayerInputConfig structure defines the characteristics of the raw * Bayer frame being processed, including: @@ -258,7 +258,7 @@ Debayer::~Debayer() /** * \struct Debayer::DebayerOutputConfig - * \brief Structure describing the output frame configuration. + * \brief Structure describing the output frame configuration * * Defines how the output of the debayer process is laid out in memory. * It includes per-pixel size, stride, and total frame size. @@ -275,7 +275,7 @@ Debayer::~Debayer() /** * \var Debayer::inputConfig_ - * \brief Input configuration parameters for the current debayer operation. + * \brief Input configuration parameters for the current debayer operation * * Holds metadata describing the incoming Bayer image layout, including * pattern size, bytes per pixel, stride, and supported output formats. @@ -284,7 +284,7 @@ Debayer::~Debayer() /** * \var Debayer::outputConfig_ - * \brief Output configuration data for the debayered frame. + * \brief Output configuration data for the debayered frame * * Contains bytes per pixel, stride, and total frame size for the * output image buffer. Set during stream configuration. @@ -292,7 +292,7 @@ Debayer::~Debayer() /** * \var Debayer::red_ - * \brief Lookup table for red channel gain and correction values. + * \brief Lookup table for red channel gain and correction values * * This table provides precomputed per-pixel or per-intensity * correction values for the red color channel used during debayering. @@ -300,7 +300,7 @@ Debayer::~Debayer() /** * \var Debayer::green_ - * \brief Lookup table for green channel gain and correction values. + * \brief Lookup table for green channel gain and correction values * * This table provides precomputed per-pixel or per-intensity * correction values for the green color channel used during debayering. @@ -308,7 +308,7 @@ Debayer::~Debayer() /** * \var Debayer::blue_ - * \brief Lookup table for blue channel gain and correction values. + * \brief Lookup table for blue channel gain and correction values * * This table provides precomputed per-pixel or per-intensity * correction values for the blue color channel used during debayering. @@ -316,33 +316,33 @@ Debayer::~Debayer() /** * \var Debayer::redCcm_ - * \brief Red channel Color Correction Matrix (CCM) lookup table. + * \brief Red channel Color Correction Matrix (CCM) lookup table * * Contains coefficients for green channel color correction. */ /** * \var Debayer::greenCcm_ - * \brief Green channel Color Correction Matrix (CCM) lookup table. + * \brief Green channel Color Correction Matrix (CCM) lookup table * * Contains coefficients for green channel color correction. */ /** * \var Debayer::blueCcm_ - * \brief Blue channel Color Correction Matrix (CCM) lookup table. + * \brief Blue channel Color Correction Matrix (CCM) lookup table * * Contains coefficients for blue channel color correction. */ /** * \var Debayer::gammaLut_ - * \brief Gamma correction lookup table. + * \brief Gamma correction lookup table */ /** * \var Debayer::swapRedBlueGains_ - * \brief Flag indicating whether red and blue channel gains should be swapped. + * \brief Flag indicating whether red and blue channel gains should be swapped * * Used when the Bayer pattern order indicates that red/blue color channels are * reversed. @@ -350,7 +350,7 @@ Debayer::~Debayer() /** * \var Debayer::bench_ - * \brief Benchmarking utility instance for performance measurements. + * \brief Benchmarking utility instance for performance measurements * * Used internally to track timing and performance metrics during * debayer processing. @@ -358,7 +358,7 @@ Debayer::~Debayer() /** * \fn int Debayer::start() - * \brief Execute a start signal in the debayer object from workerthread context. + * \brief Execute a start signal in the debayer object from workerthread context * * The start() method is invoked so that a Debayer object can initialise * internal variables or data. It is called from the software_isp::start @@ -426,7 +426,7 @@ void Debayer::dmaSyncBegin(std::vector<DmaSyncer> &dmaSyncers, FrameBuffer *inpu /** * \fn void Debayer::isStandardBayerOrder(BayerFormat::Order order) - * \brief Common method to validate standard 2x2 Bayer pattern of 2 Green, 1 Blue, 1 Red pixels. + * \brief Common method to validate standard 2x2 Bayer pattern of 2 Green, 1 Blue, 1 Red pixels */ bool Debayer::isStandardBayerOrder(BayerFormat::Order order) { diff --git a/src/libcamera/software_isp/debayer.h b/src/libcamera/software_isp/debayer.h index 5c0cb3914052..b8287166e1f9 100644 --- a/src/libcamera/software_isp/debayer.h +++ b/src/libcamera/software_isp/debayer.h @@ -35,7 +35,7 @@ LOG_DECLARE_CATEGORY(Debayer) class Debayer : public Object { public: - Debayer (const GlobalConfiguration &configuration); + Debayer(const GlobalConfiguration &configuration); virtual ~Debayer() = 0; virtual int configure(const StreamConfiguration &inputCfg, @@ -49,7 +49,7 @@ public: virtual void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params) = 0; virtual int start() { return 0; } - virtual void stop() { } + virtual void stop() {} virtual SizeRange sizes(PixelFormat inputFormat, const Size &inputSize) = 0; -- Thanks Kieran
On 12/12/2025 23:43, Kieran Bingham wrote: > Bryan - there are/a lot/ of trivial checkpatch issues coming up in > these patches. Please make sure you set up the checkstyle post-commit > hook. > > cp utils/hooks/post-commit .git/hooks/post-commit Ah, I wasn't aware of that, the only thing I've been aware-of/running is reset; ninja -C builds/build.master.dbg -D documentation=enabled -D doc_werror=true Thank you. --- bod
diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp index 1d135b278..8b4b6cd74 100644 --- a/src/libcamera/software_isp/debayer.cpp +++ b/src/libcamera/software_isp/debayer.cpp @@ -336,6 +336,31 @@ Debayer::~Debayer() * debayer processing. */ +/** + * \fn int Debayer::start() + * \brief Execute a start signal in the debayer object from workerthread context. + * + * The start() method is invoked so that a Debayer object can initialise + * internal variables or data. It is called from the software_isp::start + * method. + * + * This method is particularly useful with DebayerEGL as it allows for the + * initialisation of the EGL stack after configure in a thread-safe manner. + */ + +/** + * \fn void Debayer::stop() + * \brief Stop the debayering process and perform cleanup + * + * The stop() method is invoked as the logically corollary of start(). + * Debayer::stop() will be called by software_isp::stop() allowing for any + * cleanup which should happend with stop(). + * + * The stop method similar to start() is useful for DebayerEGL as it allows + * for cleanup of EGL context and/or data that happens in + * DebayerEGL::start. + */ + /** * \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. This requires extending 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. Per feedback from Barnabas the stop method using blocking synchronisation and thus we drop ispWorkerThread_.removeMessages(). [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 | 25 +++++++++++++++++++++ src/libcamera/software_isp/debayer.h | 2 ++ src/libcamera/software_isp/software_isp.cpp | 8 +++++-- 3 files changed, 33 insertions(+), 2 deletions(-)