[libcamera-devel] [PATCH] android: nautilus: Add camera HAL configuration

[Umang Jain]

Hi Laurent,

On 5/26/21 8:29 PM, Laurent Pinchart wrote:
> Hi Umang,
>
> On Wed, May 26, 2021 at 03:42:53PM +0530, Umang Jain wrote:
>> On 5/26/21 3:27 PM, Jacopo Mondi wrote:
>>> On Wed, May 26, 2021 at 02:51:19PM +0530, Umang Jain wrote:
>>>> On 5/26/21 1:35 PM, Jacopo Mondi wrote:
>>>>> On Wed, May 26, 2021 at 12:45:42PM +0530, Umang Jain wrote:
>>>>>> nautilus has two internal cameras, one UVC and one for the IPU3.
>>>>>> However, libcamera assumes all UVC cameras as 'external' hence, mark
>>>>>> the location of UVC camera in HAL configuration as 'external' too.
>>>>>>
>>>>>> Note that the presence of UVC camera in camera HAL will complain:
>>>>>>
>>>>>>> ERROR HAL camera_device.cpp:701 '\_SB_.PCI0.XHCI.RHUB.HS09-9:1.0-04f2:b647':
>>>>>>      Failed to map mandatory Android format IMPLEMENTATION_DEFINED (0x00000022): aborting
>>>>>>> ERROR HAL camera_hal_manager.cpp:153 Failed to initialize camera:
>>>>>>      \_SB_.PCI0.XHCI.RHUB.HS09-9:1.0-04f2:b647
>>>>>>
>>>>>> This is related to a known issue of UVC cameras on HAL which do not
>>>>>> provide NV12 by default.
>>>>> Does it work without configuration file support ?
>>>> No, it won't, since it's UVC. And cannot work until there's a
>>>> format-convertor in place that can convert the stream to NV12 provided by
>>>> that camera, I suppose.
>>> Right, it was not working before the configuration file series went
>>> in, right ? What I'm interested about is that there's no regression
>>> due to the series :)
>> Oh yea, no regression. I just went and wrote a verbose commit message
>> about what's "fine"
>>
>>>>>> Also, if we don't mention the UVC camera in the config:
>>>>>>
>>>>>>> ERROR HALConfig camera_hal_config.cpp:393 Camera
>>>>>>      '\_SB_.PCI0.XHCI.RHUB.HS09-9:1.0-04f2:b647' not described in the HAL
>>>>>>      configuration file
>>> This is "fine".. If you look at the caller of
>>> CameraHalConfig::cameraConfigData() in camera_hal_manager.cpp
>>>
>>> 	const CameraConfigData *cameraConfigData = halConfig_.cameraConfigData(cam->id());
>>> 	if (!isCameraExternal && !cameraConfigData) {
>>> 		LOG(HAL, Error)
>>> 			<< "HAL configuration entry for internal camera "
>>> 			<< cam->id() << " is missing";
>>> 		return;
>>> 	}
>>>
>>> You'll see that the configuration entry for external cameras is not
>>> mandatory, and the UVC camera should be correctly identified as
>>> EXTERNAL.
>>>
>>> We spam the log enough with worrying errors which are not really
>>> errors, so we might want to suppress this one for external cameras ?
>> Won't supress it entirely, but maybe not keep it as ERROR. WARN or DEBUG
>> maybe?
> Given that the UVC camera is internal, what happens if you set its
> location to front in the configuration file ?
It complains :-)

There is a location check of what libcamera reports vs what's provided 
in camera hal.

[0:16:15.090460200] [4680]  WARN HAL camera_device.cpp:481 
'\_SB_.PCI0.XHCI.RHUB.HS09-9:1.0-04f2:b647': Camera location does not 
match configuration file. Using 2

Oh, I can't un-see "2" now ;-)
>
>>>>>>> ERROR HAL camera_device.cpp:701 '\_SB_.PCI0.XHCI.RHUB.HS09-9:1.0-04f2:b647':
>>>>>>      Failed to map mandatory Android format IMPLEMENTATION_DEFINED (0x00000022): aborting
>>>>>>> ERROR HAL camera_hal_manager.cpp:153 Failed to initialize camera:
>>>>>>      \_SB_.PCI0.XHCI.RHUB.HS09-9:1.0-04f2:b647
>>>>>>
>>>>>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>>>>>> ---
>>>>>>     src/android/data/nautilus/camera_hal.yaml | 8 ++++++++
>>>>>>     1 file changed, 8 insertions(+)
>>>>>>     create mode 100644 src/android/data/nautilus/camera_hal.yaml
>>>>>>
>>>>>> diff --git a/src/android/data/nautilus/camera_hal.yaml b/src/android/data/nautilus/camera_hal.yaml
>>>>>> new file mode 100644
>>>>>> index 00000000..f2d31e1c
>>>>>> --- /dev/null
>>>>>> +++ b/src/android/data/nautilus/camera_hal.yaml
>>>>>> @@ -0,0 +1,8 @@
>>>>>> +cameras:
>>>>>> +  "\\_SB_.PCI0.I2C2.CAM0":
>>>>>> +    location: back
>>>>>> +    rotation: 0
>>>>>> +
>>>>>> +  "\\_SB_.PCI0.XHCI.RHUB.HS09-9:1.0-04f2:b647":
>>>>>> +    location: external
>>>>>> +    rotation: 0