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

[Kieran Bingham]

Hi Jacopo,

On 22/07/2020 12:12, Jacopo Mondi wrote:
> Hi Kieran,
>    I think the conversation diverged, as I was clearly overlooking
> the UVC camera use case and you're missing the intended use case of
> this patch. I'll try to reply and summarize at the end.

My interpretation is that this patch fixes the issue you are
experiencing, (which is a valid thing to do, and this is likely a
suitable solution for the time being)

I was indeed trying to highlight that it would cause breakage for the
UVC style use cases, and I believe system degradation for systems with
no cameras attached, and which is why I thought a \todo is required.

> On Tue, Jul 21, 2020 at 09:59:10PM +0100, Kieran Bingham wrote:
>> 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 ...
>>
> 
> For UVC we will have to register non-active cameras and notify to
> android they're available when hotplug is detected


Yes, this is what I think I see that the platform2/camera/hal/usb*
implementation does in CrOS.


> For non-pluggable cameras, as shall wait until the pipeline handler is
> matched and has registered cameras

The issue for us is that *we support UVC* ... so we're now a UVC capable
stack.

I believe that means we will in some form or another have to also do the
same (some sort of pre-allocation if that's what is needed to make
android happy). I dislike the idea that we would then have a 'maximum
supported cameras' but maybe it is set arbitrarily high so it doesn't
get hit without an extreme use case.


Now ... given that we (if we support UVC) must support dynamically
adding cameras when they are available - I anticipate that the hotplug
work will do that.

When we do that, We could also use it for -non hotplug cameras, and
simply allow cameras to be registered correctly using the same methods
as they are discovered/notified by udev/hotplug mechanisms.

Because even if they are fixed CSI2 busses, we're still going to get
udev events when they appear as a media-device right ?



>>>> 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.
>>
> 
> Yes, but that's for UVC. Built-in cameras are different, and Android
> bets on the fact if you have a camera service running then your system
> has cameras which are expected to be registered.
> 
> Keep in mind so far there's been an HAL for UVC and one for built-in
> cameras. We're handling both and that's a new use case afaict

Precisely - and that's what I've been trying to discuss on this patch
;-) I'm sorry my thoughts didn't come across clearly.




>>>>>>> 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
> 
> Not for what Android has been thought to work on. You integrate a
> system with cameras, it's fair to defer until those cameras are
> registered. You have no cameras, either you disable the camera stack
> or you register 0 cameras and then the camera subsystem is silent and
> not accessible.

Android is wrong - It should always support UVC hotplug ;-) hehehe.

I've been annoyed I haven't been able to connect a UVC camera to my
phone before ;-)



>>>> 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.
> 
> Yes, for UVC that's different I agree. A call to
> CameraManager::start() might match one uvc media device but miss a
> second one which still have to appear to userspace.
> 
> As said, UVC needs some special handling and pre-register cameras

Yes, and we will therefore /have/ to do that.



>>>> 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.
> 
> I don't think so for built-in cameras. We start the service when
> cameras are registered, otherwise if you have to pre-allocate cameras
> you would need to know how many of them the pipeline handler expects
> to register and that's not known before it has been matched.


Yes, not knowing in advance is a pain point.


> UVC, again, is a different story as it supports hotplug.

But equally we don't know how many UVC cameras someone could connect,
but we can define a max. The question would then be if that 'max'
includes static cameras, as well as dynamic UVC cameras, or if it's 'on
top' of the static cameras.

Including it in the max, means we can just preallocate the camera
registrations for Android sake, and register the (static) cameras in
that list as soon as they are available through the udev notifications
system.


>>>>>> ..
>>>>>>
>>>>>> 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.
>>
> 
> I see three cases:
> 1) No uvc support
>    Defer until a pipeline handler is matched and has registered
>    cameras
> 
> 2) UVC only
>    Need to pre-register cameras and activate on hot-plug. Knowing when
>    this has to happen is tricky, as the HAL needs to know it should
>    not wait for built-in cameras and can proceed to register
>    non-active USB cameras
> 
> 3) built-in + UVC
>    Simmilarly, knowing we have to wait for built-in to appear, we
>    defer until cameras are available, then pre-register UVC cameras.
> 
> The hard part I think it is to define how to instruct the HAL it has
> to wait for built-in to appear or not before registering UVC
> non-active cameras.


My suggestion is (not blocking this patch, but on top) to do so by
treating static cameras and hotpluggable cameras in the same way.

It's just that static cameras will never 'unplug' (unless someone
unbinds them? but we all know how bad that mess would get with V4L2 anyway)

-- 
Regards
--
Kieran