[libcamera-devel] [PATCH] android: camera_hal_manager: Fail on no cameras

[Kieran Bingham]

Hi Jacopo,

On 21/07/2020 12:26, Jacopo Mondi wrote:
> The CameraHalManager initialization tries to start the
> libcamera::CameraManager which enumerate the registered cameras.
> 
> When initialization is called too early during system boot, it might
> happen that the media graphs are still being registered to user-space
> preventing any pipeline handler to match and register cameras.
> 
> If that happens, the CameraHalManager silently accepts that no
> cameras are available in the system, reporting that information
> to the camera stack:
> 
> cros_camera_service[2054]: (5) StartOnThread(): Camera module "libcamera camera HALv3 module" has 0 built-in camera(s)
> cros_camera_service[2054]: (5) StartOnThread(): SuperHAL started with 1 modules and 0 built-in cameras
> CameraProviderManager: Camera provider legacy/0 ready with 0 camera devices
> 

Hrm ... should this be part of the hotplug work that as cameras become
available this gets updated somehow? - Even on *non* hotpluggable pipelines?


> Fix this by returning an error code if no camera is registered in the
> system at the time CameraHalManager::init() is called. The camera
> framework then tries to re-load the HAL module later in time, hopefully
> after the media device dependencies have been registered:
> 
> 2020-07-21T12:26:37.903456+02:00 INFO cros_camera_service[2054]: (5) StartOnThread(): Camera module "libcamera camera HALv3 module" has 0 built-in camera(s)
> 2020-07-21T12:26:37.903521+02:00 INFO cros_camera_service[2054]: (5) StartOnThread(): SuperHAL started with 1 modules and 0 built-in cameras
> ....
> 2020-07-21T12:30:36.662877+02:00 INFO cros_camera_service[5908]: (5910) StartOnThread(): Camera module "libcamera camera HALv3 module" has 2 built-in camera(s)
> 2020-07-21T12:30:36.663196+02:00 INFO cros_camera_service[5908]: (5910) StartOnThread(): SuperHAL started with 1 modules and 2 built-in cameras
> 
> Return -ENODEV as according to camera_common.h specification is the
> only supported error code. The CrOS HAL adapter does not distinguish
> between that and other negative values, so it does not really make
> a difference.

This feels a bit punchy/racey still. And what happens in the instance
that we really don't yet have any cameras? (I.e. a UVC only platform,
which doesn't yet have any cameras connected).

How many times will we defer? How long does it defer for for instance?

> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/android/camera_hal_manager.cpp | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp
> index 02b6418fb36d..e967d210e547 100644
> --- a/src/android/camera_hal_manager.cpp
> +++ b/src/android/camera_hal_manager.cpp
> @@ -73,6 +73,17 @@ int CameraHalManager::init()
>  		++index;
>  	}
>  
> +	/*
> +	 * If no pipeline has registered cameras, defer initialization to give
> +	 * time to media devices to register to user-space.
> +	 */
> +	if (index == 0) {
> +		LOG(HAL, Debug) << "Defer CameraHALManager initialization";

I think perhaps this needs a higher level than Debug to make sure it
always prints somewhere.

I fear this will be deflecting the issue, and introduces races (i.e.
this would go through if only one camera of two in the system is available).

..

Perhaps it can still go in if it solves an immediate problem, but in
that case I think it needs a \todo or a warning of some kind...


> +		delete cameraManager_;
> +		cameraManager_ = nullptr;
> +		return -ENODEV;
> +	}
> +
>  	return 0;
>  }
>  
> 

-- 
Regards
--
Kieran