[libcamera-devel] [PATCH v2 1/4] libcamera: pipeline: uvcvideo: Treat all UVC cameras as external

[Kieran Bingham]

On 13/08/2020 20:13, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Thu, Aug 13, 2020 at 08:11:18PM +0100, Kieran Bingham wrote:
>> On 13/08/2020 11:31, Niklas Söderlund wrote:
>>> On 2020-08-10 12:04:11 +0000, Umang Jain wrote:
>>>> Treat all UVC cameras as external for android framework to know that
>>>> they are external.
>>>
>>> nit: I would rewrite this to describe that as we currently have no way 
>>> to determine the location if a UVC camera mark it External to have a 
>>> starting point for the property.
>>>
>>> With or without this fixed but with the comments by Kieran and Laurent 
>>> fixed,
>>>
>>> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
>>
>> So, having seen the conflict this might cause with camera naming, I
>> wonder if we should introduce a new property called "hotpluggable" or
>> "removable" or such in a separate patch, and then set that property here.
> 
> I'm not sure to follow you, what's the conflict ?
> 
>> I'm not entirely sure yet, because I fear android expects the difference
>> to be 'internal/external' rather than 'hotpluggable/not-hotpluggable'.
> 
> Android expects all internal cameras to not be hotpluggable, and all
> external cameras to be hotpluggable.

You're right, it's not a 'conflict' as such, just very ambiguous naming
in cam: - this is the output I get with this change, and Niklas' naming
series:

./build/src/cam/cam -l

Available cameras:
1: External camera (\_SB_.PCI0.XHC_.RHUB.HS01-1:1.0-041e:406c)
2: External camera (\_SB_.PCI0.XHC_.RHUB.HS05-5:1.0-0408:5251)
3: External camera (\_SB_.PCI0.XHC_.RHUB.HS05-5:1.2-0408:5251)

However this is just the output rendered by cam, which is application
side, so I'm conflating the issue.

Perhaps it really means I think cam should also always display the model
when available ;-)

--
Kieran


> 
>> What do you think ?
>>
>>>> Signed-off-by: Umang Jain <email at uajain.com>
>>>> ---
>>>>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
>>>> index bc892ec..ed61d12 100644
>>>> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
>>>> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
>>>> @@ -14,6 +14,7 @@
>>>>  #include <libcamera/camera.h>
>>>>  #include <libcamera/control_ids.h>
>>>>  #include <libcamera/controls.h>
>>>> +#include <libcamera/property_ids.h>
>>>>  #include <libcamera/request.h>
>>>>  #include <libcamera/stream.h>
>>>>  
>>>> @@ -500,6 +501,9 @@ int UVCCameraData::init(MediaEntity *entity)
>>>>  
>>>>  	video_->bufferReady.connect(this, &UVCCameraData::bufferReady);
>>>>  
>>>> +	/* Initialise Camera Property. Treat all UVC cameras as external. */
>>>> +	properties_.set(properties::Location, properties::CameraLocationExternal);
>>>> +
>>>>  	/* Initialise the supported controls. */
>>>>  	ControlInfoMap::Map ctrls;
>>>>  
> 

-- 
Regards
--
Kieran