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

[Tomasz Figa]

On Fri, Jul 24, 2020 at 3:28 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Tomasz,
>
> On Wed, Jul 22, 2020 at 08:10:03PM +0200, Tomasz Figa wrote:
> > On Wed, Jul 22, 2020 at 7:56 PM Laurent Pinchart wrote:
> > > On Wed, Jul 22, 2020 at 04:31:42PM +0200, Tomasz Figa wrote:
> > >> On Wed, Jul 22, 2020 at 4:19 PM Kieran Bingham 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.
> > >
> > > Has it always worked like that, or is it fairly recent ? If the service
> > > is restarted when new devices appear, everything should be fine even if
> > > the HAL initializes successfully with zero cameras, shouldn't it ?
> >
> > Yes, it's a recent change for a new platform where we ended up with
> > dynamic detection of components. Actually it hasn't landed yet. The CL
> > for reference is
> > https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/2239080.
>
> Does this mean that if we integrate that change manually (including
> possible dependencies), then no further action should be needed on the
> HAL side ?

Yes, I believe so, although I recall some changes have been requested
in the review, so it might change a bit.

Best regards,
Tomasz