[libcamera-devel] [PATCH v2 5/7] libcamera: camera_sensor: Collect camera properties

[Laurent Pinchart]

Hello,

On Thu, Aug 29, 2019 at 11:18:28AM +0200, Niklas Söderlund wrote:
> On 2019-08-29 11:03:34 +0200, Jacopo Mondi wrote:
> > On Thu, Aug 29, 2019 at 10:29:47AM +0200, Niklas Söderlund wrote:
> >> On 2019-08-28 18:31:01 +0200, Jacopo Mondi wrote:
> >>> On Wed, Aug 28, 2019 at 01:27:58PM +0200, Niklas Söderlund wrote:
> >>>> On 2019-08-27 11:50:05 +0200, Jacopo Mondi wrote:
> >>>>> Store the camera sensor location and rotation by parsing the
> >>>>> associated V4L2 controls.
> >>>>>
> >>>>> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> >>>>> ---
> >>>>>  src/libcamera/camera_sensor.cpp       | 39 +++++++++++++++++++++++++++
> >>>>>  src/libcamera/include/camera_sensor.h |  4 +++
> >>>>>  2 files changed, 43 insertions(+)
> >>>>>
> >>>>> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> >>>>> index a7670b449b31..fc7fdcdcaf5b 100644
> >>>>> --- a/src/libcamera/camera_sensor.cpp
> >>>>> +++ b/src/libcamera/camera_sensor.cpp
> >>>>> @@ -89,6 +89,45 @@ int CameraSensor::init()
> >>>>>  	if (ret < 0)
> >>>>>  		return ret;
> >>>>>
> >>>>> +	/* Retrieve and store the camera sensor properties. */
> >>>>> +	V4L2ControlList controls({
> >>>>> +		V4L2_CID_CAMERA_SENSOR_LOCATION,
> >>>>> +		V4L2_CID_CAMERA_SENSOR_ROTATION,
> >>>>> +	});
> >>>>> +	ret = subdev_->getControls(&controls);
> >>>>> +	if (ret) {
> >>>>> +		LOG(CameraSensor, Error)
> >>>>> +			<< "Failed to get camera sensor controls: " << ret;
> >>>>> +		return ret;
> >>>>> +	}
> >>>>
> >>>> I still feel this is a bit harsh for the moment. If the sensor don't
> >>>> support V4L2_CID_CAMERA_SENSOR_LOCATION and
> >>>> V4L2_CID_CAMERA_SENSOR_ROTATION this will fail and the camera is not
> >>>> registered with the system. I see two problems with this,
> >>>>
> >>>> 1. If the camera location is EXTERNAL is rotation really a mandatory
> >>>>    property?
> >>>>
> >>>> 2. I agree that in the future when the kernel support for
> >>>>    SENSOR_LOCATION have taken hold we can make it mandatory. For now
> >>>>    would it not be a good idea to set location to EXTERNAL if it's not
> >>>>    present and print a warning?
> >>>>
> >>>>    I fear we will block this series for a very long time without easing
> >>>>    the constraints for a while.
> >>>>
> >>>
> >>> Ideally we should have all of this properties set.
> >>
> >> So rotation should be a mandatory property for external cameras? How do
> >> you see that being implemented pluggable USB cameras such as UVC?
> > 
> > Set to 0? I don't know why one should produce a USB camera that works
> > upside down, but hey, the world is a strange place isn't it?
> 
> I agree it should be set to 0 inside libcamera, but do we wish for the 
> V4L2_CID_CAMERA_SENSOR_ROTATION control to be made mandatory for 
> EXTERNAL cameras? In my view we could look for the rotation control on 
> external cameras an use the value if it exists, else default to 0. While 
> on FRONT/BACK cameras we could error out if the control is not 
> supported.

UVC doesn't use the CameraSensor class... I expect other external
cameras not to use it either. Note that several laptops have an internal
UVC camera that is mounted upside-down, so we'll have to handle this at
some point.

> > I'm fine accepting to default it to 0 for external cameras, but for
> > internal ones, I think this should be mandatory. Question is, it is
> > not mandatory on kernel side, do we want libcamera to enforce it? I
> > don't think forcing it to be mandatory on kernel side will go much far
> > (probably rightfully).
> 
> For internal cameras I think libcamera should make the two controls 
> mandatory. But only once the upstream Linux support is available. I 
> share your view that making it mandatory in Linux might be a bit far 
> fetched.

I actually think it should be mandatory in Linux, as well as in
libcamera. New sensor drivers should always have those properties, and
existing drivers should get support for them when used on platform
supported by libcamera.

I however think they should be treated as optional in libcamera until
the kernel patches land upstream. We then have two options, either
delaying the merge of this series, or changing the code. If we change it
(which I think would be the best option), should we keep this patch
as-is and add a patch on top to make the controls optional, so we can
later just revert that patch ?

> >>> It's not a big work
> >>> to implement them, and I fear if we don't enforce them, people will
> >>> just ignore them, but I would rather defer policies decisions like this
> >>> one to Laurent...
> >>
> >> I agree that we should enforce it down the line, but for now I think we
> >> need to either hold of on this series until upstream Linux support is
> >> available for all our sensors used in our "supported" platforms. Or
> >> relax the constraint to a warning, merge this series in libcamera and
> >> once upstream Liunx support is available change the warning to an error.
> >>
> >> I'm not super happy to be forced to run kernels with out of tree code
> >> for libcamera to function :-)
> > 
> > Hopefully that code (at least for the devices we support) will get in
> > sooner or later, so I'm not too concerned for the devices we have, and
> > I think people wanting to run libcamera on their device should be
> > 'encouraged' to make their devices kernel interface as compliant as
> > possible. How bad of a showstopper would this be I cannot tell, that's
> > why I'm happy to defer the decision here to the group :)
> 
> I share your view, that the code hopefully will soon be upstream, but I 
> don't think we should depend or fail cameras until it's present in an 
> real upstream release.
> 
> >>>>> +
> >>>>> +	V4L2Control *control = controls[V4L2_CID_CAMERA_SENSOR_LOCATION];
> >>>>> +	int64_t value = control->value();
> >>>>> +	switch (value) {
> >>>>> +	case V4L2_LOCATION_EXTERNAL:
> >>>>> +		location_ = CAMERA_LOCATION_EXTERNAL;
> >>>>> +		break;
> >>>>> +	case V4L2_LOCATION_FRONT:
> >>>>> +		location_ = CAMERA_LOCATION_FRONT;
> >>>>> +		break;
> >>>>> +	case V4L2_LOCATION_BACK:
> >>>>> +		location_ = CAMERA_LOCATION_BACK;
> >>>>> +		break;
> >>>>> +	default:
> >>>>> +		LOG(CameraSensor, Error)
> >>>>> +			<< "Unsupported camera location: " << value;
> >>>>> +		return -EINVAL;
> >>>>> +	}
> >>>>> +
> >>>>> +	control = controls[V4L2_CID_CAMERA_SENSOR_ROTATION];
> >>>>> +	value = control->value();
> >>>>> +	if (value < 0 || value > 360) {

360 is equivalent to 0, so I would change this to value < 0 || value >=
360. Should we reject rotations other than 0, 90, 180 and 270 ?

> >>>>> +		LOG(CameraSensor, Error)
> >>>>> +			<< "Unsupported camera rotation: " << value;
> >>>>> +		return -EINVAL;
> >>>>> +	}
> >>>>> +	rotation_ = value;
> >>>>> +
> >>>>>  	/* Enumerate and cache media bus codes and sizes. */
> >>>>>  	const ImageFormats formats = subdev_->formats(0);
> >>>>>  	if (formats.isEmpty()) {
> >>>>> diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h
> >>>>> index fe033fb374c1..32d39127b275 100644
> >>>>> --- a/src/libcamera/include/camera_sensor.h
> >>>>> +++ b/src/libcamera/include/camera_sensor.h
> >>>>> @@ -10,6 +10,7 @@
> >>>>>  #include <string>
> >>>>>  #include <vector>
> >>>>>
> >>>>> +#include <libcamera/control_ids.h>
> >>>>>  #include <libcamera/geometry.h>
> >>>>>
> >>>>>  #include "log.h"
> >>>>> @@ -55,6 +56,9 @@ private:
> >>>>>
> >>>>>  	std::vector<unsigned int> mbusCodes_;
> >>>>>  	std::vector<Size> sizes_;
> >>>>> +
> >>>>> +	CameraLocation location_;
> >>>>> +	unsigned int rotation_;

Should we store this in a list of properties already ?

> >>>>>  };
> >>>>>
> >>>>>  } /* namespace libcamera */

-- 
Regards,

Laurent Pinchart