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

[Jacopo Mondi]

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

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
-------------- 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/20190828/6f54efa9/attachment-0001.sig>