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

[Laurent Pinchart]

Hi Jacopo,

On Mon, Aug 19, 2019 at 09:32:36AM +0200, Jacopo Mondi wrote:
> On Sat, Aug 17, 2019 at 07:04:11PM +0300, Laurent Pinchart wrote:
> > 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);
> > 	...
> 
> It does and should not be hard to implement...
> 
> >>> +
> >>> +	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 :-)
> 
> That was my thinking, we should push drivers to use the APIs we need,
> but I agree it might be a bit harsh... However I'm not sure what we
> should default this property to to be honest...

If we require kernel drivers to implement that control, we don't need to
pick a default.

> >>> +	}
> >>> +
> >>>  	/* 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