| Message ID | 20251015012251.17508-17-bryan.odonoghue@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Quoting Bryan O'Donoghue (2025-10-15 02:22:28) > EGL requires both configure() and process() to operate on the same > thread. As preparation for that, move current CPU configure into the > WorkerThread with a ConnectionTypeBlocking invocation of > &DebayerCpu::configure. > > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > --- > src/libcamera/software_isp/software_isp.cpp | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp > index 500858ab..df72b1c3 100644 > --- a/src/libcamera/software_isp/software_isp.cpp > +++ b/src/libcamera/software_isp/software_isp.cpp > @@ -266,7 +266,15 @@ int SoftwareIsp::configure(const StreamConfiguration &inputCfg, > > ispWorkerThread_.start(); > > - return debayer_->configure(inputCfg, outputCfgs, ccmEnabled_); > + ret = debayer_->invokeMethod(&Debayer::configure, > + ConnectionTypeBlocking, inputCfg, > + outputCfgs, ccmEnabled_); > + if (ret) { > + ispWorkerThread_.exit(); > + ispWorkerThread_.wait(); > + } Again, I think having the thread always available when the object is there is probably less overhead than starting and stopping the thread on each configure cycle. I don't "think" threads are expensive... > + > + return ret; > } > > /** > @@ -389,7 +397,7 @@ void SoftwareIsp::stop() > void SoftwareIsp::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output) > { > ipa_->computeParams(frame); > - debayer_->invokeMethod(&DebayerCpu::process, > + debayer_->invokeMethod(&Debayer::process, Is this a separate change? > ConnectionTypeQueued, frame, input, output, debayerParams_); > } > > -- > 2.51.0 >
On 15/10/2025 23:26, Kieran Bingham wrote: >> - debayer_->invokeMethod(&DebayerCpu::process, >> + debayer_->invokeMethod(&Debayer::process, > Is this a separate change? This should squash into the next patch and then the next patch should precede this patch. --- bod
Quoting Bryan O'Donoghue (2025-10-15 23:31:24) > On 15/10/2025 23:26, Kieran Bingham wrote: > >> - debayer_->invokeMethod(&DebayerCpu::process, > >> + debayer_->invokeMethod(&Debayer::process, > > Is this a separate change? > > This should squash into the next patch and then the next patch should > precede this patch. Agreed on squash. I was about to suggest that too. > > --- > bod
On 15/10/2025 23:26, Kieran Bingham wrote: >> + ret = debayer_->invokeMethod(&Debayer::configure, >> + ConnectionTypeBlocking, inputCfg, >> + outputCfgs, ccmEnabled_); >> + if (ret) { >> + ispWorkerThread_.exit(); >> + ispWorkerThread_.wait(); >> + } > Again, I think having the thread always available when the object is > there is probably less overhead than starting and stopping the thread on > each configure cycle. > > I don't "think" threads are expensive... So ... the point here is that all of this eGL/OpenGL stuff has no handle to the context. I've never looked at the code but, there is something like a handle that gets associated with the PID/TID that sets up the initial GL state. So long as the constructor and destructor run in the same thead id - gettid(); then this will work, if not then this is not technically feasible. Hoops must be jumped through because you can't get a pointer/handle to your eGL context and pass that handle between threads. When we say Mesa/OpenGL/GLES isn't thread safe, its not that it has no synchronisation primitives, its that the implicit handle - I really must look into the code to speak about this more authoritatively - the implicit handle can't be passed from one thread to another. TL;DR I get what you're saying nicer to start/stop in constructor/destructor but I'm not sure what the impact on thread-id and hence implicit GL "handles" will be in the end. Need to look further. --- bod
Quoting Bryan O'Donoghue (2025-10-15 23:40:20) > On 15/10/2025 23:26, Kieran Bingham wrote: > >> + ret = debayer_->invokeMethod(&Debayer::configure, > >> + ConnectionTypeBlocking, inputCfg, > >> + outputCfgs, ccmEnabled_); > >> + if (ret) { > >> + ispWorkerThread_.exit(); > >> + ispWorkerThread_.wait(); > >> + } > > Again, I think having the thread always available when the object is > > there is probably less overhead than starting and stopping the thread on > > each configure cycle. > > > > I don't "think" threads are expensive... > > So ... the point here is that all of this eGL/OpenGL stuff has no handle > to the context. > > I've never looked at the code but, there is something like a handle that > gets associated with the PID/TID that sets up the initial GL state. > > So long as the constructor and destructor run in the same thead id - > gettid(); then this will work, if not then this is not technically feasible. > But aren't you pushing all the work to happen on the ispWorkerThread ? Isn't that the point ? My only concern is around the lifetime of /that/ thread. I don't think calling thread->start() and then calling thread->start() again is a good thing - and applications are free to call configure as many times as they like before starting the camera. > Hoops must be jumped through because you can't get a pointer/handle to > your eGL context and pass that handle between threads. > > When we say Mesa/OpenGL/GLES isn't thread safe, its not that it has no > synchronisation primitives, its that the implicit handle - I really must > look into the code to speak about this more authoritatively - the > implicit handle can't be passed from one thread to another. > > TL;DR I get what you're saying nicer to start/stop in > constructor/destructor but I'm not sure what the impact on thread-id and > hence implicit GL "handles" will be in the end. As far as I understand - you need to create a thread to do all the work in a single place. I would expect you can start that thread as early as you like - and you can keep it around for as long as you think you might have work to do regarding eGL ... and if you keep the thread as long as the camera is constructed - I don't think that's a big expense ? > > Need to look further. > > --- > bod
On 15/10/2025 23:45, Kieran Bingham wrote: >> TL;DR I get what you're saying nicer to start/stop in >> constructor/destructor but I'm not sure what the impact on thread-id and >> hence implicit GL "handles" will be in the end. > As far as I understand - you need to create a thread to do all the work > in a single place. I would expect you can start that thread as early as > you like - and you can keep it around for as long as you think you might > have work to do regarding eGL ... and if you keep the thread as long as > the camera is constructed - I don't think that's a big expense ? In principle agreed. I can't actually think of why doing start in the constructor would be wrong or how it might affect the thread ownership as it should be the same thread... No, this _should_ work, agreed. --- bod
diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp index 500858ab..df72b1c3 100644 --- a/src/libcamera/software_isp/software_isp.cpp +++ b/src/libcamera/software_isp/software_isp.cpp @@ -266,7 +266,15 @@ int SoftwareIsp::configure(const StreamConfiguration &inputCfg, ispWorkerThread_.start(); - return debayer_->configure(inputCfg, outputCfgs, ccmEnabled_); + ret = debayer_->invokeMethod(&Debayer::configure, + ConnectionTypeBlocking, inputCfg, + outputCfgs, ccmEnabled_); + if (ret) { + ispWorkerThread_.exit(); + ispWorkerThread_.wait(); + } + + return ret; } /** @@ -389,7 +397,7 @@ void SoftwareIsp::stop() void SoftwareIsp::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output) { ipa_->computeParams(frame); - debayer_->invokeMethod(&DebayerCpu::process, + debayer_->invokeMethod(&Debayer::process, ConnectionTypeQueued, frame, input, output, debayerParams_); }
EGL requires both configure() and process() to operate on the same thread. As preparation for that, move current CPU configure into the WorkerThread with a ConnectionTypeBlocking invocation of &DebayerCpu::configure. Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- src/libcamera/software_isp/software_isp.cpp | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)