| Message ID | 20190817105937.29353-4-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thanks for your work. 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@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); > + > + 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. > + } > + > /* 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 */ > -- > 2.22.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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 */
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@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
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@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 */
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); + + 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(); + switch (v4l2Location) { + 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"; + return -EINVAL; + } + /* 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 */
Store the camera sensor location by parsing the V4L2_CID_CAMERA_SENSOR_LOCATION V4L2 control. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/camera_sensor.cpp | 25 +++++++++++++++++++++++++ src/libcamera/include/camera_sensor.h | 3 +++ 2 files changed, 28 insertions(+)