| Message ID | mailman.202.1650534820.20186.libcamera-devel@lists.libcamera.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Yunke, Thank you for the patch. On Thu, Apr 21, 2022 at 06:53:20PM +0900, Yunke Cao via libcamera-devel wrote: > > Use vimc lens to test sensor's ability to discover ancillary lens. > > Tested with/without the recent kernel patch for vimc lens: > https://lore.kernel.org/linux-media/20220415023855.2568366-1-yunkec@google.com/ > > Signed-off-by: Yunke Cao <yunkec@google.com> > --- > Changelog since v1: > - Dont fail test when lens is not present. > - Remove lens from dm. > - Tested on both kernels with/without the vimc lens patch. > > src/libcamera/camera_sensor.cpp | 5 +++++ > test/camera-sensor.cpp | 12 ++++++++++++ > 2 files changed, 17 insertions(+) > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > index eaa2da6b..fe366267 100644 > --- a/src/libcamera/camera_sensor.cpp > +++ b/src/libcamera/camera_sensor.cpp > @@ -152,6 +152,11 @@ int CameraSensor::init() > */ > if (entity_->device()->driver() == "vimc") { > initVimcDefaultProperties(); > + > + ret = discoverAncillaryDevices(); > + if (ret) > + return ret; I would move this after initProperties() to keep the same order as for regular (non-vimc) sensors. > + > return initProperties(); > } > > diff --git a/test/camera-sensor.cpp b/test/camera-sensor.cpp > index 372ee4af..d1bb639a 100644 > --- a/test/camera-sensor.cpp > +++ b/test/camera-sensor.cpp > @@ -12,6 +12,7 @@ > > #include <libcamera/base/utils.h> > > +#include "libcamera/internal/camera_lens.h" > #include "libcamera/internal/camera_sensor.h" > #include "libcamera/internal/device_enumerator.h" > #include "libcamera/internal/media_device.h" > @@ -57,6 +58,11 @@ protected: > return TestFail; > } > > + lens_ = sensor_->focusLens(); > + if (lens_) { > + cout << "Found lens" << endl; Nitpicking, I'd write "Found lens controller" as we're dealing with the lens controller, not the lens itself. > + } No need for curly braces around a single statement. Other than that, the patch looks good. > + > return TestPass; > } > > @@ -104,6 +110,11 @@ protected: > return TestFail; > } > > + if (lens_ && lens_->setFocusPosition(10)) { > + cerr << "Failed to set lens focus position" << endl; > + return TestFail; > + } > + > return TestPass; > } > > @@ -116,6 +127,7 @@ private: > std::unique_ptr<DeviceEnumerator> enumerator_; > std::shared_ptr<MediaDevice> media_; > CameraSensor *sensor_; > + CameraLens *lens_; > }; > > TEST_REGISTER(CameraSensorTest)
Hi Yunke, On 25/04/2022 04:00, Yunke Cao wrote: > Use vimc lens to test sensor's ability to discover ancillary lens. > > Tested with the recent kernel patch for vimc lens: > https://lore.kernel.org/linux-media/20220415023855.2568366-1-yunkec@google.com/ > > Signed-off-by: Yunke Cao <yunkec@google.com> > --- > Changelog since v2: > - Flip the order of initProperties and discoverAncillaryDevices. > - Fix log string. > > Changelog since v1: > - Dont fail test when lens is not present. > - Remove lens from dm. > - Tested on both kernels with/without the vimc lens patch. > > src/libcamera/camera_sensor.cpp | 7 ++++++- > test/camera-sensor.cpp | 12 ++++++++++++ > 2 files changed, 18 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > index eaa2da6b..c56b8a60 100644 > --- a/src/libcamera/camera_sensor.cpp > +++ b/src/libcamera/camera_sensor.cpp > @@ -152,7 +152,12 @@ int CameraSensor::init() > */ > if (entity_->device()->driver() == "vimc") { > initVimcDefaultProperties(); > - return initProperties(); > + > + ret = initProperties(); > + if (ret) > + return ret; > + > + return discoverAncillaryDevices(); > } > > /* Get the color filter array pattern (only for RAW sensors). */ > diff --git a/test/camera-sensor.cpp b/test/camera-sensor.cpp > index 372ee4af..4473ca19 100644 > --- a/test/camera-sensor.cpp > +++ b/test/camera-sensor.cpp > @@ -12,6 +12,7 @@ > > #include <libcamera/base/utils.h> > > +#include "libcamera/internal/camera_lens.h" > #include "libcamera/internal/camera_sensor.h" > #include "libcamera/internal/device_enumerator.h" > #include "libcamera/internal/media_device.h" > @@ -57,6 +58,11 @@ protected: > return TestFail; > } > > + lens_ = sensor_->focusLens(); > + if (lens_) { > + cout << "Found lens controller" << endl; > + } Still don't need the curly braces, but they don't break the code so: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> And I'll let Laurent decide if he removes them while applying. > + > return TestPass; > } > > @@ -104,6 +110,11 @@ protected: > return TestFail; > } > > + if (lens_ && lens_->setFocusPosition(10)) { > + cerr << "Failed to set lens focus position" << endl; > + return TestFail; > + } > + > return TestPass; > } > > @@ -116,6 +127,7 @@ private: > std::unique_ptr<DeviceEnumerator> enumerator_; > std::shared_ptr<MediaDevice> media_; > CameraSensor *sensor_; > + CameraLens *lens_; > }; > > TEST_REGISTER(CameraSensorTest)
Quoting Yunke Cao (2022-04-25 10:27:10) > Use vimc lens to test sensor's ability to discover ancillary lens. > > Tested with the recent kernel patch for vimc lens: > https://lore.kernel.org/linux-media/20220415023855.2568366-1-yunkec@google.com/ > > Signed-off-by: Yunke Cao <yunkec@google.com> You can collect RB tags here when they are given to you for new versions too. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > Changelog since v3: > - Remove unnecessary brace. Sorry! Kept forgetting this. > > Changelog since v2: > - Flip the order of initProperties and discoverAncillaryDevices. > - Fix log string. > > Changelog since v1: > - Dont fail test when lens is not present. > - Remove lens from dm. > - Tested on both kernels with/without the vimc lens patch. > > src/libcamera/camera_sensor.cpp | 7 ++++++- > test/camera-sensor.cpp | 11 +++++++++++ > 2 files changed, 17 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > index eaa2da6b..c56b8a60 100644 > --- a/src/libcamera/camera_sensor.cpp > +++ b/src/libcamera/camera_sensor.cpp > @@ -152,7 +152,12 @@ int CameraSensor::init() > */ > if (entity_->device()->driver() == "vimc") { > initVimcDefaultProperties(); > - return initProperties(); > + > + ret = initProperties(); > + if (ret) > + return ret; > + > + return discoverAncillaryDevices(); > } > > /* Get the color filter array pattern (only for RAW sensors). */ > diff --git a/test/camera-sensor.cpp b/test/camera-sensor.cpp > index 372ee4af..dc99ab33 100644 > --- a/test/camera-sensor.cpp > +++ b/test/camera-sensor.cpp > @@ -12,6 +12,7 @@ > > #include <libcamera/base/utils.h> > > +#include "libcamera/internal/camera_lens.h" > #include "libcamera/internal/camera_sensor.h" > #include "libcamera/internal/device_enumerator.h" > #include "libcamera/internal/media_device.h" > @@ -57,6 +58,10 @@ protected: > return TestFail; > } > > + lens_ = sensor_->focusLens(); > + if (lens_) > + cout << "Found lens controller" << endl; > + > return TestPass; > } > > @@ -104,6 +109,11 @@ protected: > return TestFail; > } > > + if (lens_ && lens_->setFocusPosition(10)) { > + cerr << "Failed to set lens focus position" << endl; > + return TestFail; > + } > + > return TestPass; > } > > @@ -116,6 +126,7 @@ private: > std::unique_ptr<DeviceEnumerator> enumerator_; > std::shared_ptr<MediaDevice> media_; > CameraSensor *sensor_; > + CameraLens *lens_; > }; > > TEST_REGISTER(CameraSensorTest) > -- > 2.36.0.rc2.479.g8af0fa9b8e-goog >
Quoting Kieran Bingham (2022-04-25 15:24:42) > Quoting Yunke Cao (2022-04-25 10:27:10) > > Use vimc lens to test sensor's ability to discover ancillary lens. > > > > Tested with the recent kernel patch for vimc lens: > > https://lore.kernel.org/linux-media/20220415023855.2568366-1-yunkec@google.com/ > > > > Signed-off-by: Yunke Cao <yunkec@google.com> > > You can collect RB tags here when they are given to you for new versions > too. > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Argh, and because this was posted as a reply to the original - the tags didn't propogate to the correct version in patchwork either. This has resulted in it slipping through without being applied while it already could have. I'll try to apply / collect this now. -- Kieran > > > --- > > Changelog since v3: > > - Remove unnecessary brace. Sorry! Kept forgetting this. > > > > Changelog since v2: > > - Flip the order of initProperties and discoverAncillaryDevices. > > - Fix log string. > > > > Changelog since v1: > > - Dont fail test when lens is not present. > > - Remove lens from dm. > > - Tested on both kernels with/without the vimc lens patch. > > > > src/libcamera/camera_sensor.cpp | 7 ++++++- > > test/camera-sensor.cpp | 11 +++++++++++ > > 2 files changed, 17 insertions(+), 1 deletion(-) > > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > > index eaa2da6b..c56b8a60 100644 > > --- a/src/libcamera/camera_sensor.cpp > > +++ b/src/libcamera/camera_sensor.cpp > > @@ -152,7 +152,12 @@ int CameraSensor::init() > > */ > > if (entity_->device()->driver() == "vimc") { > > initVimcDefaultProperties(); > > - return initProperties(); > > + > > + ret = initProperties(); > > + if (ret) > > + return ret; > > + > > + return discoverAncillaryDevices(); > > } > > > > /* Get the color filter array pattern (only for RAW sensors). */ > > diff --git a/test/camera-sensor.cpp b/test/camera-sensor.cpp > > index 372ee4af..dc99ab33 100644 > > --- a/test/camera-sensor.cpp > > +++ b/test/camera-sensor.cpp > > @@ -12,6 +12,7 @@ > > > > #include <libcamera/base/utils.h> > > > > +#include "libcamera/internal/camera_lens.h" > > #include "libcamera/internal/camera_sensor.h" > > #include "libcamera/internal/device_enumerator.h" > > #include "libcamera/internal/media_device.h" > > @@ -57,6 +58,10 @@ protected: > > return TestFail; > > } > > > > + lens_ = sensor_->focusLens(); > > + if (lens_) > > + cout << "Found lens controller" << endl; > > + > > return TestPass; > > } > > > > @@ -104,6 +109,11 @@ protected: > > return TestFail; > > } > > > > + if (lens_ && lens_->setFocusPosition(10)) { > > + cerr << "Failed to set lens focus position" << endl; > > + return TestFail; > > + } > > + > > return TestPass; > > } > > > > @@ -116,6 +126,7 @@ private: > > std::unique_ptr<DeviceEnumerator> enumerator_; > > std::shared_ptr<MediaDevice> media_; > > CameraSensor *sensor_; > > + CameraLens *lens_; > > }; > > > > TEST_REGISTER(CameraSensorTest) > > -- > > 2.36.0.rc2.479.g8af0fa9b8e-goog > >
On Fri, Nov 11, 2022 at 04:50:56PM +0000, Kieran Bingham wrote: > Quoting Kieran Bingham (2022-04-25 15:24:42) > > Quoting Yunke Cao (2022-04-25 10:27:10) > > > Use vimc lens to test sensor's ability to discover ancillary lens. > > > > > > Tested with the recent kernel patch for vimc lens: > > > https://lore.kernel.org/linux-media/20220415023855.2568366-1-yunkec@google.com/ > > > > > > Signed-off-by: Yunke Cao <yunkec@google.com> > > > > You can collect RB tags here when they are given to you for new versions > > too. > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Argh, and because this was posted as a reply to the original - the tags > didn't propogate to the correct version in patchwork either. This has > resulted in it slipping through without being applied while it already > could have. > > I'll try to apply / collect this now. If it can help, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > Changelog since v3: > > > - Remove unnecessary brace. Sorry! Kept forgetting this. > > > > > > Changelog since v2: > > > - Flip the order of initProperties and discoverAncillaryDevices. > > > - Fix log string. > > > > > > Changelog since v1: > > > - Dont fail test when lens is not present. > > > - Remove lens from dm. > > > - Tested on both kernels with/without the vimc lens patch. > > > > > > src/libcamera/camera_sensor.cpp | 7 ++++++- > > > test/camera-sensor.cpp | 11 +++++++++++ > > > 2 files changed, 17 insertions(+), 1 deletion(-) > > > > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > > > index eaa2da6b..c56b8a60 100644 > > > --- a/src/libcamera/camera_sensor.cpp > > > +++ b/src/libcamera/camera_sensor.cpp > > > @@ -152,7 +152,12 @@ int CameraSensor::init() > > > */ > > > if (entity_->device()->driver() == "vimc") { > > > initVimcDefaultProperties(); > > > - return initProperties(); > > > + > > > + ret = initProperties(); > > > + if (ret) > > > + return ret; > > > + > > > + return discoverAncillaryDevices(); > > > } > > > > > > /* Get the color filter array pattern (only for RAW sensors). */ > > > diff --git a/test/camera-sensor.cpp b/test/camera-sensor.cpp > > > index 372ee4af..dc99ab33 100644 > > > --- a/test/camera-sensor.cpp > > > +++ b/test/camera-sensor.cpp > > > @@ -12,6 +12,7 @@ > > > > > > #include <libcamera/base/utils.h> > > > > > > +#include "libcamera/internal/camera_lens.h" > > > #include "libcamera/internal/camera_sensor.h" > > > #include "libcamera/internal/device_enumerator.h" > > > #include "libcamera/internal/media_device.h" > > > @@ -57,6 +58,10 @@ protected: > > > return TestFail; > > > } > > > > > > + lens_ = sensor_->focusLens(); > > > + if (lens_) > > > + cout << "Found lens controller" << endl; > > > + > > > return TestPass; > > > } > > > > > > @@ -104,6 +109,11 @@ protected: > > > return TestFail; > > > } > > > > > > + if (lens_ && lens_->setFocusPosition(10)) { > > > + cerr << "Failed to set lens focus position" << endl; > > > + return TestFail; > > > + } > > > + > > > return TestPass; > > > } > > > > > > @@ -116,6 +126,7 @@ private: > > > std::unique_ptr<DeviceEnumerator> enumerator_; > > > std::shared_ptr<MediaDevice> media_; > > > CameraSensor *sensor_; > > > + CameraLens *lens_; > > > }; > > > > > > TEST_REGISTER(CameraSensorTest)
diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp index eaa2da6b..fe366267 100644 --- a/src/libcamera/camera_sensor.cpp +++ b/src/libcamera/camera_sensor.cpp @@ -152,6 +152,11 @@ int CameraSensor::init() */ if (entity_->device()->driver() == "vimc") { initVimcDefaultProperties(); + + ret = discoverAncillaryDevices(); + if (ret) + return ret; + return initProperties(); } diff --git a/test/camera-sensor.cpp b/test/camera-sensor.cpp index 372ee4af..d1bb639a 100644 --- a/test/camera-sensor.cpp +++ b/test/camera-sensor.cpp @@ -12,6 +12,7 @@ #include <libcamera/base/utils.h> +#include "libcamera/internal/camera_lens.h" #include "libcamera/internal/camera_sensor.h" #include "libcamera/internal/device_enumerator.h" #include "libcamera/internal/media_device.h" @@ -57,6 +58,11 @@ protected: return TestFail; } + lens_ = sensor_->focusLens(); + if (lens_) { + cout << "Found lens" << endl; + } + return TestPass; } @@ -104,6 +110,11 @@ protected: return TestFail; } + if (lens_ && lens_->setFocusPosition(10)) { + cerr << "Failed to set lens focus position" << endl; + return TestFail; + } + return TestPass; } @@ -116,6 +127,7 @@ private: std::unique_ptr<DeviceEnumerator> enumerator_; std::shared_ptr<MediaDevice> media_; CameraSensor *sensor_; + CameraLens *lens_; }; TEST_REGISTER(CameraSensorTest)
Use vimc lens to test sensor's ability to discover ancillary lens. Tested with/without the recent kernel patch for vimc lens: https://lore.kernel.org/linux-media/20220415023855.2568366-1-yunkec@google.com/ Signed-off-by: Yunke Cao <yunkec@google.com> --- Changelog since v1: - Dont fail test when lens is not present. - Remove lens from dm. - Tested on both kernels with/without the vimc lens patch. src/libcamera/camera_sensor.cpp | 5 +++++ test/camera-sensor.cpp | 12 ++++++++++++ 2 files changed, 17 insertions(+)