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

[Laurent Pinchart]

Hi Kieran,

On Wed, Jul 22, 2020 at 03:19:28PM +0100, 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.

Don't be too harsh on yourself. The devil is in the details here, it's
easy to overlook some parts, and I'm sure I'm missing info too.

> On 22/07/2020 14:49, Laurent Pinchart wrote:
> > 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:
> >>>>>> On 21/07/2020 13:13, Jacopo Mondi wrote:
> >>>>>>> On Tue, Jul 21, 2020 at 12:41:16PM +0100, Kieran Bingham wrote:
> >>>>>>>> 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 :-(

To be more accurate, it was done this way last time I checked, which
wasn't very recent. Tomasz has just commented they restart the service
when new devices are found, so maybe there's hope there. I'll reply to
his e-mail separately.

> Oh dear :-( Well I guess that invalidates all my assumptions
> 
> /me throws toys out of the pram.

Is this the step before rolling on the floor throwing a fit ? :-)

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

Laurent Pinchart