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

[Kieran Bingham]

On 21/07/2020 14:09, Jacopo Mondi wrote:
> 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

That sounds like that would cause a very bad implementation to me.

Waiting for upstart to decide when to retry launching the camera-service
wouldn't be an acceptable time to wait IMO.

Either it tries every second, and is flooding the system, or it tries
every minute and it takes a minute before you can view your newly
attached UVC camera ...

I would expect event driven camera attachment, not polled ...

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

So, looking at the USB HAL in cros - it does look like they pre-allocate
some cameras or such and only activate them on device connection.

I fear we might have to do the same somehow in the future, as indeed it
might not be possible to tell android there is a 'new' camera. Only an
update to an existing one... not sure it will require more investigation.



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

Ok, but I don't think it's that difficult to conjur up a scenario where
there won't be a device


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


At least PipelineHandlerUVC will call PipelineHandler::match() once for
each camera attached.

But yes, perhaps due to the threading model, it won't be a possible race.

I was thinking of when a pipeline starts and only 'one' of it's media
devices is available.




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

I'm suggesting that when hotplug support is available, this patch, if
integrated, might need to be reverted in effect ... and thus a todo note
should be added to say that or such.

I believe we should be able to start the camera service successfully
with zero cameras if there are zero cameras at the time the service starts.


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


Hrm, so it disables the app completely?

Anyway, essentially I'm thinking - whatever happens with the existing
UVC stack is what we would need to be able to mirror.



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


I was going to ask how does it register camera's it doesn't know will
exist, but I think I saw that the UVC HAL just pre-allocates some
cameras or such, so that explains how that one works.





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

-- 
Regards
--
Kieran