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

[Kieran Bingham]

Hi Tomasz,

On 22/07/2020 15:31, Tomasz Figa wrote:
> On Wed, Jul 22, 2020 at 4:19 PM Kieran Bingham
> <kieran.bingham at ideasonboard.com> wrote:
>>
>> Hi Laurent, Jacopo,
>>
>> Jacopo, I'm sorry for the noise, it seems my opinions were based on an
>> invalid assumption about what is reasonable :-( and a lack of
>> understanding of how CrOS implemented working around that assumption.
>>
>> So my whole premise has been completely wrong and should be ignored.
>>
>>
>> On 22/07/2020 14:49, Laurent Pinchart wrote:
>>> Hello,
>>>
>>> (CC'ing Tomasz)
>>>
>>> On Wed, Jul 22, 2020 at 12:24:56PM +0100, Kieran Bingham wrote:
>>>> 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.
>>>
>>> I don't think we can do this, as, as far as I know, Android doesn't
>>> support status changes for internal cameras. Only cameras reported as
>>> external can generate a status change event. Quoting from
>>> camera/provider/2.4/ICameraProviderCallback.hal in
>>> https://android.googlesource.com/platform/hardware/interfaces:
>>>
>>>      * On camera service startup, when ICameraProvider::setCallback is invoked,
>>>      * the camera service must assume that all internal camera devices are in
>>>      * the CAMERA_DEVICE_STATUS_PRESENT state.
>>>      *
>>>      * The provider must call this method to inform the camera service of any
>>>      * initially NOT_PRESENT devices, and of any external camera devices that
>>>      * are already present, as soon as the callbacks are available through
>>>      * setCallback.
>>>
>>>> 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 ?
>>>
>>> Yes, on the libcamera side hotplug should work perfectly fine for
>>> internal cameras, but unfortunately not on the Android side.
>>>
>>>>>>>> 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 ...
>>>
>>> This is actually how the camera HALs in Chrome OS handle this issue :-(
>>
>>
>> Oh dear :-( Well I guess that invalidates all my assumptions
>>
>> /me throws toys out of the pram.
>>
>>
>>>>>>>>>> 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.
>>>
>>> I also don't see any way around this *for UVC*.
>>>
>>>>>>>> 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)
>>>
>>> As explained above, this is unfortunately not an option. To make this
>>> matter more complicated, we need to wait for *all* built-in cameras to
>>> be available before initializing, as otherwise the system will be
>>> permanently stuck with only part of the cameras available.
>>>
>>> I think this isn't something we should solve, the system should ensure
>>> that device nodes have been created before starting the camera service.
>>
>> Yes, given what you've clarified, then I agree - the service should be
>> dependent upon the devices being brought up. I think systemd can do
>> that, but CrOS uses upstart, so I don't know what that might support.
> 
> As ugly as it sounds, we have an udev rule which restarts the camera
> service when devices matching the internal cameras show up. I believe
> we match them by the "video4linux" class and particular hardware
> subsystems, e.g. PCI or I2C.

That's actually slightly better than what I had imagined was happening ;-)

I had got the impression there was some arbitrary delay between retries
... so at least having some event driving the retry is better than a
time based interval.


>> But that's out of scope (and control) for us I think ...
>>
>>
>>> I've expressed this before in a conversation with Tomasz (not sure if it
>>> was on the public mailing list, IRC, or in private), but we didn't reach
>>> an agreement at the time. One option that was discussed was a platform
>>> configuration file that would list the cameras expected to be present (I
>>> think everybody knows how much I like that :-)). Another option that has
>>> been experimented was
>>> https://lists.libcamera.org/pipermail/libcamera-devel/2019-September/004850.html.
>>
>>
>> --
>> Regards
>> --
>> Kieran

-- 
Regards
--
Kieran