| Message ID | 20251015012251.17508-16-bryan.odonoghue@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Quoting Bryan O'Donoghue (2025-10-15 02:22:27) > EGL is not thread-safe and in fact associates invisible handles with the > threadid of the calling context. > > As a result we need to make Deabyer::configure() and Debayer::process() in SoftISP execute on s/Deabyer/Debayer/ > the same thread. > > When we call Debayer::configure() in the egl class this will setup and egl context > for us which is associated with the calling thread context. Hence when > Debayer::process(); runs it must run in the same thread as > Debayer::configure() or the hidden Gegl context handles will not point Gegl? or egl ? > to the same place. > > Move start thread into configure() as a first step towards this. > > Co-developed-by: Robert Mader <robert.mader@collabora.com> > Signed-off-by: Robert Mader <robert.mader@collabora.com> > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > --- > src/libcamera/software_isp/software_isp.cpp | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp > index 6f1f88fe..500858ab 100644 > --- a/src/libcamera/software_isp/software_isp.cpp > +++ b/src/libcamera/software_isp/software_isp.cpp > @@ -264,6 +264,8 @@ int SoftwareIsp::configure(const StreamConfiguration &inputCfg, > if (ret < 0) > return ret; > > + ispWorkerThread_.start(); > + I don't yet know when ispWorkerThread is stopped ... but I'm not sure if this is a good place to call start(). I might assume SoftwareIsp::stop() is calling ispWorkerThread_stop() ... but we can generally assume that a camera pipeline could have the following call pattern: Camera() // construct new camera Camera->Configure(A) /// Yeah make it like this. Camera->Configure(B) /// Sorry I changed my mind. Camera->Start(); Camera->Stop(); So I think if you're moving something out of start it would have to go into the constructor and be alive for the 'lifetime' of the object ? But I haven't looked at anything else around this so all the above is purely comments based on seeing a thread operation moving from start to configure. -- Kieran > return debayer_->configure(inputCfg, outputCfgs, ccmEnabled_); > } > > @@ -345,7 +347,6 @@ int SoftwareIsp::start() > if (ret) > return ret; > > - ispWorkerThread_.start(); > return 0; > } > > -- > 2.51.0 >
On 15/10/2025 23:23, Kieran Bingham wrote:
> Gegl? or egl ?
Yes I'm noticing I didn't follow my _own_ feedback from a previous
review cycle.
eGL to match the class name in the commit log.
---
bod
diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp index 6f1f88fe..500858ab 100644 --- a/src/libcamera/software_isp/software_isp.cpp +++ b/src/libcamera/software_isp/software_isp.cpp @@ -264,6 +264,8 @@ int SoftwareIsp::configure(const StreamConfiguration &inputCfg, if (ret < 0) return ret; + ispWorkerThread_.start(); + return debayer_->configure(inputCfg, outputCfgs, ccmEnabled_); } @@ -345,7 +347,6 @@ int SoftwareIsp::start() if (ret) return ret; - ispWorkerThread_.start(); return 0; }