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

[Jacopo Mondi]

Hi Niklas,

On Thu, Aug 29, 2019 at 10:29:47AM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> On 2019-08-28 18:31:01 +0200, Jacopo Mondi wrote:
> > Hi Niklas,
> >
> > On Wed, Aug 28, 2019 at 01:27:58PM +0200, Niklas Söderlund wrote:
> > > Hi Jacopo,
> > >
> > > Thanks for your work.
> > >
> > > 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'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).

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

>
> >
> > Thanks
> >   j
> >
> > > > +
> > > > +	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) {
> > > > +		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_;
> > > >  };
> > > >
> > > >  } /* namespace libcamera */
> > > > --
> > > > 2.23.0
> > > >
> > > > _______________________________________________
> > > > libcamera-devel mailing list
> > > > libcamera-devel at lists.libcamera.org
> > > > https://lists.libcamera.org/listinfo/libcamera-devel
> > >
> > > --
> > > Regards,
> > > Niklas Söderlund
>
>
>
> --
> Regards,
> Niklas Söderlund
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190829/bbc63156/attachment-0001.sig>