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

[Niklas Söderlund]

Hi Kieran,

On 2020-08-13 20:19:13 +0100, Kieran Bingham wrote:
> 
> 
> 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 ;-)

I agree with you, camera names printed with both these series are 
unsuitable for UVC cameras that are hot-pluggable. I think the name for 
UVC cameras that are hot-pluggable should be the model name.

As the exact requirements for how the HAL should select if a camera is 
hot-pluggable are still under debate I think we should merge the series 
as soon as they become ready. The fix for the name generation rule 
change introduce in this series is small and easy to squash in or fix 
on-top, depending on the order the series are ready.

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

-- 
Regards,
Niklas Söderlund