[v3,15/39] libcamera: software_isp: Start the ISP thread in configure

24660 diff mbox series

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
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
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(-)

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

Kieran Bingham <kieran.bingham@ideasonboard.com> writes:

> 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.

I can't find answers to this comment.  Also, what if

  Camera->start();

is called again, without calling configure()?  Then the thread wouldn't
be running.

> --
> 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
>>

Quoting Milan Zamazal (2025-12-03 14:09:20)
> Kieran Bingham <kieran.bingham@ideasonboard.com> writes:
> 
> > 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.
> 
> I can't find answers to this comment.  Also, what if
> 
>   Camera->start();
> 
> is called again, without calling configure()?  Then the thread wouldn't
> be running.
> 

Indeed, a

	Camera()
	Camera->Configure()
	Camera->Start()
	Camera->Stop()
	Camera->Start()
	Camera->Stop()

is a perfectly valid use of the API, so it's got to have it's resources
after a stop / restart cycle.

--
Kiran



> > --
> > 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 03/12/2025 14:52, Kieran Bingham wrote:
>> is called again, without calling configure()?  Then the thread wouldn't
>> be running.
>>
> Indeed, a
> 
> 	Camera()
> 	Camera->Configure()
> 	Camera->Start()
> 	Camera->Stop()
> 	Camera->Start()
> 	Camera->Stop()
> 
> is a perfectly valid use of the API, so it's got to have it's resources
> after a stop / restart cycle.
> 
> --
> Kiran

OK.

Let me see if the thread start/stop can be moved to the 
constructor/destructor.

I think we discussed this before but, I forget if it was tried and 
didn't work or more likely discussed and forgotten about.

---
bod

On Wed, Dec 03, 2025 at 03:00:58PM +0000, Bryan O'Donoghue wrote:
> On 03/12/2025 14:52, Kieran Bingham wrote:
> >> is called again, without calling configure()?  Then the thread wouldn't
> >> be running.
> >>
> > Indeed, a
> > 
> > 	Camera()
> > 	Camera->Configure()
> > 	Camera->Start()
> > 	Camera->Stop()
> > 	Camera->Start()
> > 	Camera->Stop()
> > 
> > is a perfectly valid use of the API, so it's got to have it's resources
> > after a stop / restart cycle.
> > 
> > --
> > Kiran
> 
> OK.
> 
> Let me see if the thread start/stop can be moved to the 
> constructor/destructor.
> 
> I think we discussed this before but, I forget if it was tried and 
> didn't work or more likely discussed and forgotten about.

Instead of starting the thread earlier, can we move EGL code from
Debayer::configure() to start ?

On 03/12/2025 15:12, Laurent Pinchart wrote:
> On Wed, Dec 03, 2025 at 03:00:58PM +0000, Bryan O'Donoghue wrote:
>> On 03/12/2025 14:52, Kieran Bingham wrote:
>>>> is called again, without calling configure()?  Then the thread wouldn't
>>>> be running.
>>>>
>>> Indeed, a
>>>
>>> 	Camera()
>>> 	Camera->Configure()
>>> 	Camera->Start()
>>> 	Camera->Stop()
>>> 	Camera->Start()
>>> 	Camera->Stop()
>>>
>>> is a perfectly valid use of the API, so it's got to have it's resources
>>> after a stop / restart cycle.
>>>
>>> --
>>> Kiran
>>
>> OK.
>>
>> Let me see if the thread start/stop can be moved to the
>> constructor/destructor.
>>
>> I think we discussed this before but, I forget if it was tried and
>> didn't work or more likely discussed and forgotten about.
> 
> Instead of starting the thread earlier, can we move EGL code from
> Debayer::configure() to start ?
> 

I believe it should be possible to add a start/stop method to the 
debayer base class - there is already a stop() introduced as part of 
this series.

So.

SoftwareISP::start() {
	ipa->start();
	invokemethod(&Debayer::start(); // instantiate egl context here
};


for i = 0 i < some_num; i++) {
	SoftwareISP::configure() {
		invokeMethod(&Deabyer::configure();
	};

	SoftwareISP::configure() {
		invokeMethod(&Deabyer::configure();
	};
}

SoftwareISP::process() {
	invokeMethod(&Debayer::process..);
}

SoftwareISP::start() {
	ipa->start();
	invokemethod(&Debayer::stop(); // instantiate egl context here
};

Looking at the code we have now, I _believe_ the egl context init could 
be moved to start.

Actually since configure() should be re-entrant, we will need to clean 
up eglImageBayerIn_ and eglBayerOut_.

Yes... I think this can be done and besides the egl object has some 
strict checking to make sure member functions are only called from the 
context which created the object to begin with.

Yes.. probably, I think this can be done.


---
bod

On 03/12/2025 15:33, Bryan O'Donoghue wrote:
> SoftwareISP::start() {
> 	ipa->start();
> 	invokemethod(&Debayer::stop(); // instantiate egl context here
> };

g:/start/s//stop/g

---
bod