libcamera: software_isp: Skip stop before worker start

28194 diff mbox series

The simple pipeline calls stop() if the capture device fails to startstreaming, before SoftwareIsp::start() has run. The blocking invocation
of Debayer::stop() then waits indefinitely for a worker thread that is
not running.

Return early when the worker has not started so that Camera::start()
can report the original capture error.

Bug: https://gitlab.freedesktop.org/camera/libcamera/-/issues/349
Signed-off-by: Birk Skyum <birk.skyum@pm.me>
---

Tested on a Lenovo Yoga Slim 7x with libcamera 0.7.2. Injecting an
ETIMEDOUT failure into VIDIOC_STREAMON causes the unpatched cam process
to hang until an eight-second timeout. With this patch, cam promptly
returns the capture-start error.

The private AArch64 build passed 46 tests, with one expected failure,
30 skips and no unexpected failures. Runtime testing was on 0.7.2;
the same unguarded stop path remains in current master.

Reproducer and detailed results:
https://gist.github.com/birkskyum/5ec156ca5654d7ac2251a275718c94b3

 src/libcamera/software_isp/software_isp.cpp | 3 +++
 1 file changed, 3 insertions(+)


base-commit: 191e202178f02430b5942397c70d215cdd2056fa

Quoting Birk Skyum (2026-09-06 14:44:18)
> The simple pipeline calls stop() if the capture device fails to startstreaming, before SoftwareIsp::start() has run. The blocking invocation
> of Debayer::stop() then waits indefinitely for a worker thread that is
> not running.
> 
> Return early when the worker has not started so that Camera::start()
> can report the original capture error.
> 
> Bug: https://gitlab.freedesktop.org/camera/libcamera/-/issues/349
> Signed-off-by: Birk Skyum <birk.skyum@pm.me>
> ---
> 
> Tested on a Lenovo Yoga Slim 7x with libcamera 0.7.2. Injecting an
> ETIMEDOUT failure into VIDIOC_STREAMON causes the unpatched cam process
> to hang until an eight-second timeout. With this patch, cam promptly
> returns the capture-start error.
> 
> The private AArch64 build passed 46 tests, with one expected failure,
> 30 skips and no unexpected failures. Runtime testing was on 0.7.2;
> the same unguarded stop path remains in current master.
> 
> Reproducer and detailed results:
> https://gist.github.com/birkskyum/5ec156ca5654d7ac2251a275718c94b3
> 
>  src/libcamera/software_isp/software_isp.cpp | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
> index c73a16c..c801034 100644
> --- a/src/libcamera/software_isp/software_isp.cpp
> +++ b/src/libcamera/software_isp/software_isp.cpp
> @@ -395,6 +395,9 @@ int SoftwareIsp::start()
>   */
>  void SoftwareIsp::stop()
>  {
> + if (!ispWorkerThread_.isRunning())
> + return;
> +

I know there is a resend with the indentation fixed. As Laurent metions,
we can't review an attachment. I look forward to a corrected patch
sent through git-send-email. That's the cleanest / easiest way to get
the patches delivered correctly.


Otherwise, I dug into this quickly, to see - at first I was worried that
if the thread isn't running we don't call or check other cleanup like
stopping the IPA below ... but I looking at 
   int SoftwareIsp::start()
then the I if the thread isn't running, I don't expect the IPA to have
been started so I think it's ok.

It feels a bit worrying using the thread as a marker to the cleanup
state, but given there's not much else to worry about there at the
moment I think it's ok.

Curious if anyone has any alternative suggestions. I don't think we need
to add a bool state explicitly so with the formatting fixed and the
patch sent to the list correctly:

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

You can already add that to your patch for the resend.

--
Kieran


>   debayer_->invokeMethod(&Debayer::stop,
>         ConnectionTypeBlocking);
> 
> 
> base-commit: 191e202178f02430b5942397c70d215cdd2056fa