[libcamera-devel] [PATCH 3/5] libcamera: camera_sensor: Store the camera location

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Sat, Aug 17, 2019 at 05:57:17PM +0200, Niklas Söderlund wrote:
> On 2019-08-17 12:59:35 +0200, Jacopo Mondi wrote:
> > Store the camera sensor location by parsing the
> > V4L2_CID_CAMERA_SENSOR_LOCATION V4L2 control.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/libcamera/camera_sensor.cpp       | 25 +++++++++++++++++++++++++
> >  src/libcamera/include/camera_sensor.h |  3 +++
> >  2 files changed, 28 insertions(+)
> > 
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index a7670b449b31..2703d10c719e 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -89,6 +89,31 @@ int CameraSensor::init()
> >  	if (ret < 0)
> >  		return ret;
> >  
> > +	/* Retrieve and store the camera sensor location. */
> > +	V4L2ControlList controls;
> > +	controls.add(V4L2_CID_CAMERA_SENSOR_LOCATION);

Unrelated to this patch, would it make sense to have a V4L2ControlList
constructor that would take a list of control ids ?

	V4L2ControlList controls{ V4L2_CID_CAMERA_SENSOR_LOCATION };
	ret = subdev_->getControls(&controls);
	...

> > +
> > +	ret = subdev_->getControls(&controls);
> > +	if (ret) {
> > +		LOG(CameraSensor, Error)
> > +			<< "Failed to get camera sensor controls: " << ret;
> > +		return ret;
> > +	}
> > +
> > +	V4L2Control *locationControl = controls[V4L2_CID_CAMERA_SENSOR_LOCATION];
> > +	int64_t v4l2Location = locationControl->value();
> 
> I would skip the temporary variable here, but it's just bikeshedding ;-)
> 
> > +	switch (v4l2Location) {
> > +	case V4L2_LOCATION_FRONT:
> > +		location_ = CAMERA_LOCATION_FRONT;
> > +		break;
> > +	case V4L2_LOCATION_BACK:
> > +		location_ = CAMERA_LOCATION_BACK;
> > +		break;
> 
> Should we not handle V4L2_LOCATION_EXTERNAL also?
> 
> > +	default:
> > +		LOG(CameraSensor, Error) << "Unsupported camera location";
> > +		return -EINVAL;
> 
> Is it not a bit harsh to fail if location information is not available?  
> Is it possible to set it to V4L2_LOCATION_UNKOWN and move on. For the 
> Andriod HAL we could map UNKWON to EXTERNAL and print a warning, or if 
> we require location information at that point skip camers which do not 
> provided it.

That's one option, but I also like the idea of libcamera pushing for the
kernel drivers to implement the required APIs :-)

> > +	}
> > +
> >  	/* 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..a237a1684605 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,8 @@ private:
> >  
> >  	std::vector<unsigned int> mbusCodes_;
> >  	std::vector<Size> sizes_;
> > +
> > +	CameraLocation location_;
> >  };
> >  
> >  } /* namespace libcamera */

-- 
Regards,

Laurent Pinchart