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

[Jacopo Mondi]

On Tue, Jul 21, 2020 at 01:47:29PM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 21/07/2020 13:13, Jacopo Mondi wrote:
> > Hi Kieran,
> >
> > On Tue, Jul 21, 2020 at 12:41:16PM +0100, Kieran Bingham wrote:
> >> 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?
> >>
> >
> > I don't get what your comment is about here, I'm sorry
>
>
> Your patch prevents loading of libcamera completely unless there is at
> least one camera right ?
>
> What happens on say an x86 platform which will only have UVC cameras,
> yet will use libcamera...
>
> When will it be acceptable to load the camera service.
>
> Should the service itself continually fail, and respawn until someone
> plugs in a camera?
>

That's my understanding

>
> So taking that further, what I'm saying is, perhaps this should be
> perfectly able to launch with zero cameras, and /when/ cameras are
> created they should notify android that a new camera is now available.

Do you know if that's even possible ?

I think Android expects the number of possible cameras to be
statically defined, there's a callback to notify a change of status of
a device, but we're dealing here with -creating- cameras based on the
number of cameras detected by android.

>
>
>
>
> >>
> >>> 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).
> >
> > You keep defferring ? I think the framework handles that
>
> I would be surprised if it would be expected behaviour to just poll
> launching of the camera service until there is a camera there to satisfy
> it ...
>
>
> >> How many times will we defer? How long does it defer for for instance?
> >>
> >
> > Usually 1 is enough from my testing. I don't think we have to care
> > what is the timeout in the framework, as afaict android services are
> > restarted by default.
>
>
> Ok, so this is mostly dealing with the race at startup ...
>
>
>
> >>> 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 think that's kind of expected to happen, I don't think this should
> > be an error
>
>
> Info? Warning?
>
> If libcamera has prevented loading, I think it would be good to know
> about it by default...
>
>
> >> 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).
> >
> > If the pipeline handler match (ie the device enumerator matches the
> > dependencies) than the match() function continues and register
> > cameras. I don't think we can found ourselves in a semi-initialized
> > state.
>
>
> Ok, so it really is about having at least one camera available?

It's about at least a pipeline being matched.

>
> I think looking at CameraHalManager::init(), as it only creates a
> CameraDevice for cameras available in cameraManager_->cameras(), it
> /could/ be a race, it's just that creating two cameras is fast, so we're
> unlikely to get in here between the point that one has been available
> and another is still loading ...

I don't think that's possible.

If a all the dependencies of a pipeline handler are matched, the
camera are registered. What is the code flow that registers one
camera, returns to the camera manager, then calls the pipeline handler
again to register more ?

>
> But still ... I think this is stuff that will get dealt with by hotplug
> being added to the HAL.

I agree, we should support inserting and removing new cameras, but I
think at this point, we should really start only if cameras are
registered and any pipeline handler is matched.

I'm not sure I got what other alternative approach you are proposing
to be honest.

>
>
>
>
> >> ..
> >>
> >> 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...
> >>
> >
> > A \todo item to record this should be fixed how ?
> >
> > Unless we expect the CameraManager to stall until any of the pipeline
> > handler it tries match, but this seems not a good idea to me.
>
> No, i don't expect it to stall ... I expect it to complete successfully,
> and if there are only 0 cameras, it will only have zero cameras - until

That means Android/CrOS starts without the camera being active (ie.
the camera application icon is not available, in CrOS in example).

> they become available at which point they'll be registered through the
> same mechanisms as the hotplugging ...

Still I don't get if you are talking about libcamera hotplug or
android hotplug, which, to my understanding, expects anyway a camera
device to be registered but marked as 'not present' (looking at the
camera_device_status enumeration)

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