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

[Jacopo Mondi]

Hi Laurent,

On Sat, Aug 17, 2019 at 07:04:11PM +0300, Laurent Pinchart wrote:
> 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);
> 	...
>

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

> > > +	}
> > > +
> > >  	/* 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
-------------- 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/20190819/a0570596/attachment.sig>