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

[Niklas Söderlund]

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?

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

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