| Message ID | 20200721112633.103016-1-jacopo@jmondi.org |
|---|---|
| State | Superseded, archived |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
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? > 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). How many times will we defer? How long does it defer for for instance? > > Signed-off-by: Jacopo Mondi <jacopo@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 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). .. 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... > + delete cameraManager_; > + cameraManager_ = nullptr; > + return -ENODEV; > + } > + > return 0; > } > >
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 > > > 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 > > 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. > > > > Signed-off-by: Jacopo Mondi <jacopo@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 > > 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. > > .. > > 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. > > > + delete cameraManager_; > > + cameraManager_ = nullptr; > > + return -ENODEV; > > + } > > + > > return 0; > > } > > > > > > -- > Regards > -- > Kieran
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? 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. >> >>> 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@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? 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 ... But still ... I think this is stuff that will get dealt with by hotplug being added to the HAL. >> .. >> >> 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 they become available at which point they'll be registered through the same mechanisms as the hotplugging ... > >> >>> + delete cameraManager_; >>> + cameraManager_ = nullptr; >>> + return -ENODEV; >>> + } >>> + >>> return 0; >>> } >>> >>> >> >> -- >> Regards >> -- >> Kieran
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 > > 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. > > > > > >> > >>> 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@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. > > 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 ? > > 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. > > > > > >> .. > >> > >> 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). > 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) > > > > > > >> > >>> + delete cameraManager_; > >>> + cameraManager_ = nullptr; > >>> + return -ENODEV; > >>> + } > >>> + > >>> return 0; > >>> } > >>> > >>> > >> > >> -- > >> Regards > >> -- > >> Kieran > > -- > Regards > -- > Kieran
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@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; >>>>> }
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. 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 For non-pluggable cameras, as shall wait until the pipeline handler is matched and has registered cameras > >> 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 > > > >>>>> 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@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. > > > > > >> > >> 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 > > > > > >> 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. UVC, again, is a different story as it supports hotplug. > > > >>>> .. > >>>> > >>>> 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.
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@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)
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 :-( > >>>>>> 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@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. 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.
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@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.
On Wed, Jul 22, 2020 at 4:19 PM Kieran Bingham <kieran.bingham@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@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. > > 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
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. No worries, I think that helped clarifying what Android expects for external cameras and will help when we will support UVC hotplug, so it was indeed worth it. Thanks j
Hi Tomasz, On 22/07/2020 15:31, Tomasz Figa wrote: > On Wed, Jul 22, 2020 at 4:19 PM Kieran Bingham > <kieran.bingham@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@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
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@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.
Hi Tomasz, 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@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 ? > > 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.
On Wed, Jul 22, 2020 at 7:56 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Tomasz, > > 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@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. > > > 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
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 > <laurent.pinchart@ideasonboard.com> wrote: > > > > Hi Tomasz, > > snip > > > >>>>> 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. > Do you happen to know why I see this issue being triggered only for CTS which goes through the android services ? It goes through the cros_camera_service if I got the architecture right, but even without this patch, the camera service is always able to identify both cameras... It is still not clear to me if the android camera service (is this ARC++?) interacts with the cros_camera_service through the same mojo interface as stock ChromeOS camera application (and cros_camera_test?) use or there's a different interface for it.. Thanks j
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@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 ? > >>> 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.
On Thu, Jul 23, 2020 at 11:43 AM Jacopo Mondi <jacopo@jmondi.org> 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 > > <laurent.pinchart@ideasonboard.com> wrote: > > > > > > Hi Tomasz, > > > > > snip > > > > > >>>>> 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. > > > > Do you happen to know why I see this issue being triggered only for > CTS which goes through the android services ? It goes through the > cros_camera_service if I got the architecture right, but even without > this patch, the camera service is always able to identify both > cameras... > > It is still not clear to me if the android camera service (is this ARC++?) > interacts with the cros_camera_service through the same mojo interface > as stock ChromeOS camera application (and cros_camera_test?) use or > there's a different interface for it.. Yes, Android interacts with the cros_camera service using the same interface. I think it's just that Android camera frameworks cache the camera metadata when they query it the first time and would ignore any later changes. On the contrary, Chrome probably just queries the metadata every time some camera use case is executed. Best regards, Tomasz
On Fri, Jul 24, 2020 at 3:28 AM Laurent Pinchart <laurent.pinchart@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@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
Hi Tomasz, On Mon, Jul 27, 2020 at 11:30:16AM +0200, Tomasz Figa wrote: > On Thu, Jul 23, 2020 at 11:43 AM Jacopo Mondi wrote: > > 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: > > > > snip > > > >>>>>>>>> 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. > > > > Do you happen to know why I see this issue being triggered only for > > CTS which goes through the android services ? It goes through the > > cros_camera_service if I got the architecture right, but even without > > this patch, the camera service is always able to identify both > > cameras... > > > > It is still not clear to me if the android camera service (is this ARC++?) > > interacts with the cros_camera_service through the same mojo interface > > as stock ChromeOS camera application (and cros_camera_test?) use or > > there's a different interface for it.. > > Yes, Android interacts with the cros_camera service using the same > interface. I think it's just that Android camera frameworks cache the > camera metadata when they query it the first time and would ignore any > later changes. On the contrary, Chrome probably just queries the > metadata every time some camera use case is executed. IS there a service that could be restarted to reset the Android camera framework state instead of rebooting the device ?
On Mon, Jul 27, 2020 at 12:52 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Tomasz, > > On Mon, Jul 27, 2020 at 11:30:16AM +0200, Tomasz Figa wrote: > > On Thu, Jul 23, 2020 at 11:43 AM Jacopo Mondi wrote: > > > 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: > > > > > > snip > > > > > >>>>>>>>> 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. > > > > > > Do you happen to know why I see this issue being triggered only for > > > CTS which goes through the android services ? It goes through the > > > cros_camera_service if I got the architecture right, but even without > > > this patch, the camera service is always able to identify both > > > cameras... > > > > > > It is still not clear to me if the android camera service (is this ARC++?) > > > interacts with the cros_camera_service through the same mojo interface > > > as stock ChromeOS camera application (and cros_camera_test?) use or > > > there's a different interface for it.. > > > > Yes, Android interacts with the cros_camera service using the same > > interface. I think it's just that Android camera frameworks cache the > > camera metadata when they query it the first time and would ignore any > > later changes. On the contrary, Chrome probably just queries the > > metadata every time some camera use case is executed. > > IS there a service that could be restarted to reset the Android camera > framework state instead of rebooting the device ? I'm not sure unfortunately. I think logging out and in again in Chrome OS should restart the Android container, but not sure if that improves anything. Also given that inside the container is just plain Android, maybe there is some generic Android way to reload the camera HAL?
Hello, On Mon, Jul 27, 2020 at 01:00:12PM +0200, Tomasz Figa wrote: > On Mon, Jul 27, 2020 at 12:52 PM Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: > > > >> > > > >> 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. FWIW I tested https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/2239080 applied to libcamera and not to ipu6, with https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2239846/2/camera/hal_adapter/init/cros-camera.conf and I have two cameras registered at ARC++ start up. I think we can drop this patch and just keep it as a local fix to run CTS or Android camera applications, and drop it when the above change lands in upstream chromium > > > > > > > > Do you happen to know why I see this issue being triggered only for > > > > CTS which goes through the android services ? It goes through the > > > > cros_camera_service if I got the architecture right, but even without > > > > this patch, the camera service is always able to identify both > > > > cameras... > > > > > > > > It is still not clear to me if the android camera service (is this ARC++?) > > > > interacts with the cros_camera_service through the same mojo interface > > > > as stock ChromeOS camera application (and cros_camera_test?) use or > > > > there's a different interface for it.. > > > > > > Yes, Android interacts with the cros_camera service using the same > > > interface. I think it's just that Android camera frameworks cache the > > > camera metadata when they query it the first time and would ignore any > > > later changes. On the contrary, Chrome probably just queries the > > > metadata every time some camera use case is executed. > > > > IS there a service that could be restarted to reset the Android camera > > framework state instead of rebooting the device ? > > I'm not sure unfortunately. I think logging out and in again in Chrome > OS should restart the Android container, but not sure if that improves > anything. Also given that inside the container is just plain Android, > maybe there is some generic Android way to reload the camera HAL?
Hi Jacopo, On Tue, Jul 28, 2020 at 04:12:29PM +0200, Jacopo Mondi wrote: > On Mon, Jul 27, 2020 at 01:00:12PM +0200, Tomasz Figa wrote: > > On Mon, Jul 27, 2020 at 12:52 PM Laurent Pinchart wrote: > > > > >> > > > > >> 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. > > FWIW I tested > https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/2239080 > applied to libcamera and not to ipu6, with > https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2239846/2/camera/hal_adapter/init/cros-camera.conf > > and I have two cameras registered at ARC++ start up. > > I think we can drop this patch and just keep it as a local fix to run > CTS or Android camera applications, and drop it when the above change > lands in upstream chromium Nice !! Thank you for testing. > > > > > Do you happen to know why I see this issue being triggered only for > > > > > CTS which goes through the android services ? It goes through the > > > > > cros_camera_service if I got the architecture right, but even without > > > > > this patch, the camera service is always able to identify both > > > > > cameras... > > > > > > > > > > It is still not clear to me if the android camera service (is this ARC++?) > > > > > interacts with the cros_camera_service through the same mojo interface > > > > > as stock ChromeOS camera application (and cros_camera_test?) use or > > > > > there's a different interface for it.. > > > > > > > > Yes, Android interacts with the cros_camera service using the same > > > > interface. I think it's just that Android camera frameworks cache the > > > > camera metadata when they query it the first time and would ignore any > > > > later changes. On the contrary, Chrome probably just queries the > > > > metadata every time some camera use case is executed. > > > > > > IS there a service that could be restarted to reset the Android camera > > > framework state instead of rebooting the device ? > > > > I'm not sure unfortunately. I think logging out and in again in Chrome > > OS should restart the Android container, but not sure if that improves > > anything. Also given that inside the container is just plain Android, > > maybe there is some generic Android way to reload the camera HAL?
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"; + delete cameraManager_; + cameraManager_ = nullptr; + return -ENODEV; + } + return 0; }
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 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. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/android/camera_hal_manager.cpp | 11 +++++++++++ 1 file changed, 11 insertions(+)