| Message ID | 20211111104958.312070-3-hanlinchen@chromium.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Han-Lin, On 11/11/2021 11:49, Han-Lin Chen wrote: > Allow IPA to apply controls to the lens device. > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > --- > meson.build | 6 ++++++ > src/libcamera/pipeline/ipu3/cio2.cpp | 30 ++++++++++++++++++++++++++++ > src/libcamera/pipeline/ipu3/cio2.h | 3 +++ > src/libcamera/pipeline/ipu3/ipu3.cpp | 9 +++++++-- > 4 files changed, 46 insertions(+), 2 deletions(-) > > diff --git a/meson.build b/meson.build > index 7892a9e3..2a4b68a2 100644 > --- a/meson.build > +++ b/meson.build > @@ -108,6 +108,12 @@ if cc.has_argument('-Wno-c99-designator') > ] > endif > > +if get_option('android_platform') == 'cros' > + common_arguments += [ > + '-DOS_CHROMEOS', > + ] > +endif I am not really aware of specifics regarding this, why can't we have something not dependent on the OS ? > + > c_arguments += common_arguments > cpp_arguments += common_arguments > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp > index 59dda56b..233553c2 100644 > --- a/src/libcamera/pipeline/ipu3/cio2.cpp > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp > @@ -16,6 +16,7 @@ > #include <libcamera/geometry.h> > #include <libcamera/stream.h> > > +#include "libcamera/internal/camera_lens.h" > #include "libcamera/internal/camera_sensor.h" > #include "libcamera/internal/framebuffer.h" > #include "libcamera/internal/media_device.h" > @@ -159,6 +160,35 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index) > return -EINVAL; > } > > +#if defined(OS_CHROMEOS) > + /* > + * \todo Read the lens model from the sensor itself or from a device database. > + * For now use default values taken from ChromeOS. > + */ > + static std::unordered_map<std::string, std::string> sensorLens = { > + { "ov13858", "dw9714" }, > + { "imx258", "dw9807" }, > + { "imx355", "ak7375" } > + }; So, we need a list giving the lens model associated to a sensor... How is this dependent on chromeos ? Imagine I want to use the sensor lens on my SGo2, which has a ov8865, what is needed to do that ? > + > + auto it = sensorLens.find(sensor_->model()); > + if (it != sensorLens.end()) { > + const std::vector<MediaEntity *> &entities = media->entities(); > + for (auto ent : entities) { > + if (ent->function() == MEDIA_ENT_F_LENS) { > + lens_ = std::make_unique<CameraLens>(ent); > + ret = lens_->init(); > + if (!ret && lens_->model() == it->second) { > + break; > + } > + lens_.reset(); > + } > + if (!lens_) > + LOG(IPU3, Warning) << "Lens device " << it->second << " not found"; > + } > + } > +#endif > + > /* > * \todo Define when to open and close video device nodes, as they > * might impact on power consumption. > diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h > index ba8f0052..635566c8 100644 > --- a/src/libcamera/pipeline/ipu3/cio2.h > +++ b/src/libcamera/pipeline/ipu3/cio2.h > @@ -18,6 +18,7 @@ > > namespace libcamera { > > +class CameraLens; > class CameraSensor; > class FrameBuffer; > class MediaDevice; > @@ -52,6 +53,7 @@ public: > int stop(); > > CameraSensor *sensor() { return sensor_.get(); } > + CameraLens *lens() { return lens_.get(); } > const CameraSensor *sensor() const { return sensor_.get(); } > > FrameBuffer *queueBuffer(Request *request, FrameBuffer *rawBuffer); > @@ -67,6 +69,7 @@ private: > void cio2BufferReady(FrameBuffer *buffer); > > std::unique_ptr<CameraSensor> sensor_; > + std::unique_ptr<CameraLens> lens_; > std::unique_ptr<V4L2Subdevice> csi2_; > std::unique_ptr<V4L2VideoDevice> output_; > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 97003681..88775f67 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -24,6 +24,7 @@ > #include <libcamera/stream.h> > > #include "libcamera/internal/camera.h" > +#include "libcamera/internal/camera_lens.h" > #include "libcamera/internal/camera_sensor.h" > #include "libcamera/internal/delayed_controls.h" > #include "libcamera/internal/device_enumerator.h" > @@ -1255,8 +1256,12 @@ void IPU3CameraData::queueFrameAction(unsigned int id, > { > switch (action.op) { > case ipa::ipu3::ActionSetSensorControls: { > - const ControlList &controls = action.sensorControls; > - delayedCtrls_->push(controls); > + const ControlList &sensorControls = action.sensorControls; > + delayedCtrls_->push(sensorControls); > + if (cio2_.lens()) { > + ControlList lensControls = action.lensControls; > + cio2_.lens()->setControls(&lensControls); > + } > break; > } > case ipa::ipu3::ActionParamFilled: { >
On Thu, Nov 11, 2021 at 05:12:52PM +0100, Jean-Michel Hautbois wrote: > Hi Han-Lin, > > On 11/11/2021 11:49, Han-Lin Chen wrote: > > Allow IPA to apply controls to the lens device. > > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > > --- > > meson.build | 6 ++++++ > > src/libcamera/pipeline/ipu3/cio2.cpp | 30 ++++++++++++++++++++++++++++ > > src/libcamera/pipeline/ipu3/cio2.h | 3 +++ > > src/libcamera/pipeline/ipu3/ipu3.cpp | 9 +++++++-- > > 4 files changed, 46 insertions(+), 2 deletions(-) > > > > diff --git a/meson.build b/meson.build > > index 7892a9e3..2a4b68a2 100644 > > --- a/meson.build > > +++ b/meson.build > > @@ -108,6 +108,12 @@ if cc.has_argument('-Wno-c99-designator') > > ] > > endif > > > > +if get_option('android_platform') == 'cros' > > + common_arguments += [ > > + '-DOS_CHROMEOS', > > + ] > > +endif > > I am not really aware of specifics regarding this, why can't we have > something not dependent on the OS ? There's a mail thread regarding VCM support on MS Surface machines (see https://lists.libcamera.org/pipermail/libcamera-devel/2021-November/026526.html). Raspberry Pi is also looking into VCM support. It would be best to cooperate with all parties involved to define a way to expose the relation between camera sensors and lens controllers to userspace, and use it here. I'm really not keen on having a CrOS-specific hack. > > + > > c_arguments += common_arguments > > cpp_arguments += common_arguments > > > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp > > index 59dda56b..233553c2 100644 > > --- a/src/libcamera/pipeline/ipu3/cio2.cpp > > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp > > @@ -16,6 +16,7 @@ > > #include <libcamera/geometry.h> > > #include <libcamera/stream.h> > > > > +#include "libcamera/internal/camera_lens.h" > > #include "libcamera/internal/camera_sensor.h" > > #include "libcamera/internal/framebuffer.h" > > #include "libcamera/internal/media_device.h" > > @@ -159,6 +160,35 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index) > > return -EINVAL; > > } > > > > +#if defined(OS_CHROMEOS) > > + /* > > + * \todo Read the lens model from the sensor itself or from a device database. > > + * For now use default values taken from ChromeOS. > > + */ > > + static std::unordered_map<std::string, std::string> sensorLens = { > > + { "ov13858", "dw9714" }, > > + { "imx258", "dw9807" }, > > + { "imx355", "ak7375" } > > + }; > > So, we need a list giving the lens model associated to a sensor... How > is this dependent on chromeos ? Imagine I want to use the sensor lens on > my SGo2, which has a ov8865, what is needed to do that ? > > > + > > + auto it = sensorLens.find(sensor_->model()); > > + if (it != sensorLens.end()) { > > + const std::vector<MediaEntity *> &entities = media->entities(); > > + for (auto ent : entities) { > > + if (ent->function() == MEDIA_ENT_F_LENS) { > > + lens_ = std::make_unique<CameraLens>(ent); > > + ret = lens_->init(); > > + if (!ret && lens_->model() == it->second) { > > + break; > > + } > > + lens_.reset(); > > + } > > + if (!lens_) > > + LOG(IPU3, Warning) << "Lens device " << it->second << " not found"; > > + } > > + } > > +#endif > > + > > /* > > * \todo Define when to open and close video device nodes, as they > > * might impact on power consumption. > > diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h > > index ba8f0052..635566c8 100644 > > --- a/src/libcamera/pipeline/ipu3/cio2.h > > +++ b/src/libcamera/pipeline/ipu3/cio2.h > > @@ -18,6 +18,7 @@ > > > > namespace libcamera { > > > > +class CameraLens; > > class CameraSensor; > > class FrameBuffer; > > class MediaDevice; > > @@ -52,6 +53,7 @@ public: > > int stop(); > > > > CameraSensor *sensor() { return sensor_.get(); } > > + CameraLens *lens() { return lens_.get(); } > > const CameraSensor *sensor() const { return sensor_.get(); } > > > > FrameBuffer *queueBuffer(Request *request, FrameBuffer *rawBuffer); > > @@ -67,6 +69,7 @@ private: > > void cio2BufferReady(FrameBuffer *buffer); > > > > std::unique_ptr<CameraSensor> sensor_; > > + std::unique_ptr<CameraLens> lens_; > > std::unique_ptr<V4L2Subdevice> csi2_; > > std::unique_ptr<V4L2VideoDevice> output_; > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index 97003681..88775f67 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -24,6 +24,7 @@ > > #include <libcamera/stream.h> > > > > #include "libcamera/internal/camera.h" > > +#include "libcamera/internal/camera_lens.h" > > #include "libcamera/internal/camera_sensor.h" > > #include "libcamera/internal/delayed_controls.h" > > #include "libcamera/internal/device_enumerator.h" > > @@ -1255,8 +1256,12 @@ void IPU3CameraData::queueFrameAction(unsigned int id, > > { > > switch (action.op) { > > case ipa::ipu3::ActionSetSensorControls: { > > - const ControlList &controls = action.sensorControls; > > - delayedCtrls_->push(controls); > > + const ControlList &sensorControls = action.sensorControls; > > + delayedCtrls_->push(sensorControls); > > + if (cio2_.lens()) { > > + ControlList lensControls = action.lensControls; > > + cio2_.lens()->setControls(&lensControls); > > + } > > break; > > } > > case ipa::ipu3::ActionParamFilled: { > >
Hi Jean-Michel, On Fri, Nov 12, 2021 at 12:12 AM Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> wrote: > > Hi Han-Lin, > > On 11/11/2021 11:49, Han-Lin Chen wrote: > > Allow IPA to apply controls to the lens device. > > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > > --- > > meson.build | 6 ++++++ > > src/libcamera/pipeline/ipu3/cio2.cpp | 30 ++++++++++++++++++++++++++++ > > src/libcamera/pipeline/ipu3/cio2.h | 3 +++ > > src/libcamera/pipeline/ipu3/ipu3.cpp | 9 +++++++-- > > 4 files changed, 46 insertions(+), 2 deletions(-) > > > > diff --git a/meson.build b/meson.build > > index 7892a9e3..2a4b68a2 100644 > > --- a/meson.build > > +++ b/meson.build > > @@ -108,6 +108,12 @@ if cc.has_argument('-Wno-c99-designator') > > ] > > endif > > > > +if get_option('android_platform') == 'cros' > > + common_arguments += [ > > + '-DOS_CHROMEOS', > > + ] > > +endif > > I am not really aware of specifics regarding this, why can't we have > something not dependent on the OS ? > > > + > > c_arguments += common_arguments > > cpp_arguments += common_arguments > > > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp > > index 59dda56b..233553c2 100644 > > --- a/src/libcamera/pipeline/ipu3/cio2.cpp > > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp > > @@ -16,6 +16,7 @@ > > #include <libcamera/geometry.h> > > #include <libcamera/stream.h> > > > > +#include "libcamera/internal/camera_lens.h" > > #include "libcamera/internal/camera_sensor.h" > > #include "libcamera/internal/framebuffer.h" > > #include "libcamera/internal/media_device.h" > > @@ -159,6 +160,35 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index) > > return -EINVAL; > > } > > > > +#if defined(OS_CHROMEOS) > > + /* > > + * \todo Read the lens model from the sensor itself or from a device database. > > + * For now use default values taken from ChromeOS. > > + */ > > + static std::unordered_map<std::string, std::string> sensorLens = { > > + { "ov13858", "dw9714" }, > > + { "imx258", "dw9807" }, > > + { "imx355", "ak7375" } > > + }; > > So, we need a list giving the lens model associated to a sensor... How > is this dependent on chromeos ? Imagine I want to use the sensor lens on > my SGo2, which has a ov8865, what is needed to do that ? You are right. Not only ChromeOS needs the mapping. I made it depend on CrOS simply because it's the mapping I can confirm, and I was worried to accidentally broken the other platforms. Maybe I was overthinking this... Will remove the directive. Besides, the mapping is considered as a temporary solution, which should be removed once we have a way to read the relation from the media-ctl or a database. > > > + > > + auto it = sensorLens.find(sensor_->model()); > > + if (it != sensorLens.end()) { > > + const std::vector<MediaEntity *> &entities = media->entities(); > > + for (auto ent : entities) { > > + if (ent->function() == MEDIA_ENT_F_LENS) { > > + lens_ = std::make_unique<CameraLens>(ent); > > + ret = lens_->init(); > > + if (!ret && lens_->model() == it->second) { > > + break; > > + } > > + lens_.reset(); > > + } > > + if (!lens_) > > + LOG(IPU3, Warning) << "Lens device " << it->second << " not found"; > > + } > > + } > > +#endif > > + > > /* > > * \todo Define when to open and close video device nodes, as they > > * might impact on power consumption. > > diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h > > index ba8f0052..635566c8 100644 > > --- a/src/libcamera/pipeline/ipu3/cio2.h > > +++ b/src/libcamera/pipeline/ipu3/cio2.h > > @@ -18,6 +18,7 @@ > > > > namespace libcamera { > > > > +class CameraLens; > > class CameraSensor; > > class FrameBuffer; > > class MediaDevice; > > @@ -52,6 +53,7 @@ public: > > int stop(); > > > > CameraSensor *sensor() { return sensor_.get(); } > > + CameraLens *lens() { return lens_.get(); } > > const CameraSensor *sensor() const { return sensor_.get(); } > > > > FrameBuffer *queueBuffer(Request *request, FrameBuffer *rawBuffer); > > @@ -67,6 +69,7 @@ private: > > void cio2BufferReady(FrameBuffer *buffer); > > > > std::unique_ptr<CameraSensor> sensor_; > > + std::unique_ptr<CameraLens> lens_; > > std::unique_ptr<V4L2Subdevice> csi2_; > > std::unique_ptr<V4L2VideoDevice> output_; > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index 97003681..88775f67 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -24,6 +24,7 @@ > > #include <libcamera/stream.h> > > > > #include "libcamera/internal/camera.h" > > +#include "libcamera/internal/camera_lens.h" > > #include "libcamera/internal/camera_sensor.h" > > #include "libcamera/internal/delayed_controls.h" > > #include "libcamera/internal/device_enumerator.h" > > @@ -1255,8 +1256,12 @@ void IPU3CameraData::queueFrameAction(unsigned int id, > > { > > switch (action.op) { > > case ipa::ipu3::ActionSetSensorControls: { > > - const ControlList &controls = action.sensorControls; > > - delayedCtrls_->push(controls); > > + const ControlList &sensorControls = action.sensorControls; > > + delayedCtrls_->push(sensorControls); > > + if (cio2_.lens()) { > > + ControlList lensControls = action.lensControls; > > + cio2_.lens()->setControls(&lensControls); > > + } > > break; > > } > > case ipa::ipu3::ActionParamFilled: { > >
On Fri, Nov 12, 2021 at 7:58 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Thu, Nov 11, 2021 at 05:12:52PM +0100, Jean-Michel Hautbois wrote: > > Hi Han-Lin, > > > > On 11/11/2021 11:49, Han-Lin Chen wrote: > > > Allow IPA to apply controls to the lens device. > > > > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > > > --- > > > meson.build | 6 ++++++ > > > src/libcamera/pipeline/ipu3/cio2.cpp | 30 ++++++++++++++++++++++++++++ > > > src/libcamera/pipeline/ipu3/cio2.h | 3 +++ > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 9 +++++++-- > > > 4 files changed, 46 insertions(+), 2 deletions(-) > > > > > > diff --git a/meson.build b/meson.build > > > index 7892a9e3..2a4b68a2 100644 > > > --- a/meson.build > > > +++ b/meson.build > > > @@ -108,6 +108,12 @@ if cc.has_argument('-Wno-c99-designator') > > > ] > > > endif > > > > > > +if get_option('android_platform') == 'cros' > > > + common_arguments += [ > > > + '-DOS_CHROMEOS', > > > + ] > > > +endif > > > > I am not really aware of specifics regarding this, why can't we have > > something not dependent on the OS ? > > There's a mail thread regarding VCM support on MS Surface machines (see > https://lists.libcamera.org/pipermail/libcamera-devel/2021-November/026526.html). > Raspberry Pi is also looking into VCM support. It would be best to > cooperate with all parties involved to define a way to expose the > relation between camera sensors and lens controllers to userspace, and > use it here. I'm really not keen on having a CrOS-specific hack. Looking forward to it :-). > > > > + > > > c_arguments += common_arguments > > > cpp_arguments += common_arguments > > > > > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp > > > index 59dda56b..233553c2 100644 > > > --- a/src/libcamera/pipeline/ipu3/cio2.cpp > > > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp > > > @@ -16,6 +16,7 @@ > > > #include <libcamera/geometry.h> > > > #include <libcamera/stream.h> > > > > > > +#include "libcamera/internal/camera_lens.h" > > > #include "libcamera/internal/camera_sensor.h" > > > #include "libcamera/internal/framebuffer.h" > > > #include "libcamera/internal/media_device.h" > > > @@ -159,6 +160,35 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index) > > > return -EINVAL; > > > } > > > > > > +#if defined(OS_CHROMEOS) > > > + /* > > > + * \todo Read the lens model from the sensor itself or from a device database. > > > + * For now use default values taken from ChromeOS. > > > + */ > > > + static std::unordered_map<std::string, std::string> sensorLens = { > > > + { "ov13858", "dw9714" }, > > > + { "imx258", "dw9807" }, > > > + { "imx355", "ak7375" } > > > + }; > > > > So, we need a list giving the lens model associated to a sensor... How > > is this dependent on chromeos ? Imagine I want to use the sensor lens on > > my SGo2, which has a ov8865, what is needed to do that ? > > > > > + > > > + auto it = sensorLens.find(sensor_->model()); > > > + if (it != sensorLens.end()) { > > > + const std::vector<MediaEntity *> &entities = media->entities(); > > > + for (auto ent : entities) { > > > + if (ent->function() == MEDIA_ENT_F_LENS) { > > > + lens_ = std::make_unique<CameraLens>(ent); > > > + ret = lens_->init(); > > > + if (!ret && lens_->model() == it->second) { > > > + break; > > > + } > > > + lens_.reset(); > > > + } > > > + if (!lens_) > > > + LOG(IPU3, Warning) << "Lens device " << it->second << " not found"; > > > + } > > > + } > > > +#endif > > > + > > > /* > > > * \todo Define when to open and close video device nodes, as they > > > * might impact on power consumption. > > > diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h > > > index ba8f0052..635566c8 100644 > > > --- a/src/libcamera/pipeline/ipu3/cio2.h > > > +++ b/src/libcamera/pipeline/ipu3/cio2.h > > > @@ -18,6 +18,7 @@ > > > > > > namespace libcamera { > > > > > > +class CameraLens; > > > class CameraSensor; > > > class FrameBuffer; > > > class MediaDevice; > > > @@ -52,6 +53,7 @@ public: > > > int stop(); > > > > > > CameraSensor *sensor() { return sensor_.get(); } > > > + CameraLens *lens() { return lens_.get(); } > > > const CameraSensor *sensor() const { return sensor_.get(); } > > > > > > FrameBuffer *queueBuffer(Request *request, FrameBuffer *rawBuffer); > > > @@ -67,6 +69,7 @@ private: > > > void cio2BufferReady(FrameBuffer *buffer); > > > > > > std::unique_ptr<CameraSensor> sensor_; > > > + std::unique_ptr<CameraLens> lens_; > > > std::unique_ptr<V4L2Subdevice> csi2_; > > > std::unique_ptr<V4L2VideoDevice> output_; > > > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > index 97003681..88775f67 100644 > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > @@ -24,6 +24,7 @@ > > > #include <libcamera/stream.h> > > > > > > #include "libcamera/internal/camera.h" > > > +#include "libcamera/internal/camera_lens.h" > > > #include "libcamera/internal/camera_sensor.h" > > > #include "libcamera/internal/delayed_controls.h" > > > #include "libcamera/internal/device_enumerator.h" > > > @@ -1255,8 +1256,12 @@ void IPU3CameraData::queueFrameAction(unsigned int id, > > > { > > > switch (action.op) { > > > case ipa::ipu3::ActionSetSensorControls: { > > > - const ControlList &controls = action.sensorControls; > > > - delayedCtrls_->push(controls); > > > + const ControlList &sensorControls = action.sensorControls; > > > + delayedCtrls_->push(sensorControls); > > > + if (cio2_.lens()) { > > > + ControlList lensControls = action.lensControls; > > > + cio2_.lens()->setControls(&lensControls); > > > + } > > > break; > > > } > > > case ipa::ipu3::ActionParamFilled: { > > > > > -- > Regards, > > Laurent Pinchart
Hi Han-Lin, On Tue, Nov 16, 2021 at 05:52:49PM +0800, Hanlin Chen wrote: > On Fri, Nov 12, 2021 at 7:58 AM Laurent Pinchart wrote: > > On Thu, Nov 11, 2021 at 05:12:52PM +0100, Jean-Michel Hautbois wrote: > > > On 11/11/2021 11:49, Han-Lin Chen wrote: > > > > Allow IPA to apply controls to the lens device. > > > > > > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > > > > --- > > > > meson.build | 6 ++++++ > > > > src/libcamera/pipeline/ipu3/cio2.cpp | 30 ++++++++++++++++++++++++++++ > > > > src/libcamera/pipeline/ipu3/cio2.h | 3 +++ > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 9 +++++++-- > > > > 4 files changed, 46 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/meson.build b/meson.build > > > > index 7892a9e3..2a4b68a2 100644 > > > > --- a/meson.build > > > > +++ b/meson.build > > > > @@ -108,6 +108,12 @@ if cc.has_argument('-Wno-c99-designator') > > > > ] > > > > endif > > > > > > > > +if get_option('android_platform') == 'cros' > > > > + common_arguments += [ > > > > + '-DOS_CHROMEOS', > > > > + ] > > > > +endif > > > > > > I am not really aware of specifics regarding this, why can't we have > > > something not dependent on the OS ? > > > > There's a mail thread regarding VCM support on MS Surface machines (see > > https://lists.libcamera.org/pipermail/libcamera-devel/2021-November/026526.html). > > Raspberry Pi is also looking into VCM support. It would be best to > > cooperate with all parties involved to define a way to expose the > > relation between camera sensors and lens controllers to userspace, and > > use it here. I'm really not keen on having a CrOS-specific hack. > > Looking forward to it :-). To make that happen, may I ask you to reply to David's proposal about autofocus controls (https://lists.libcamera.org/pipermail/libcamera-devel/2021-October/025986.html) ? > > > > + > > > > c_arguments += common_arguments > > > > cpp_arguments += common_arguments > > > > > > > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp > > > > index 59dda56b..233553c2 100644 > > > > --- a/src/libcamera/pipeline/ipu3/cio2.cpp > > > > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp > > > > @@ -16,6 +16,7 @@ > > > > #include <libcamera/geometry.h> > > > > #include <libcamera/stream.h> > > > > > > > > +#include "libcamera/internal/camera_lens.h" > > > > #include "libcamera/internal/camera_sensor.h" > > > > #include "libcamera/internal/framebuffer.h" > > > > #include "libcamera/internal/media_device.h" > > > > @@ -159,6 +160,35 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index) > > > > return -EINVAL; > > > > } > > > > > > > > +#if defined(OS_CHROMEOS) > > > > + /* > > > > + * \todo Read the lens model from the sensor itself or from a device database. > > > > + * For now use default values taken from ChromeOS. > > > > + */ > > > > + static std::unordered_map<std::string, std::string> sensorLens = { > > > > + { "ov13858", "dw9714" }, > > > > + { "imx258", "dw9807" }, > > > > + { "imx355", "ak7375" } > > > > + }; > > > > > > So, we need a list giving the lens model associated to a sensor... How > > > is this dependent on chromeos ? Imagine I want to use the sensor lens on > > > my SGo2, which has a ov8865, what is needed to do that ? > > > > > > > + > > > > + auto it = sensorLens.find(sensor_->model()); > > > > + if (it != sensorLens.end()) { > > > > + const std::vector<MediaEntity *> &entities = media->entities(); > > > > + for (auto ent : entities) { > > > > + if (ent->function() == MEDIA_ENT_F_LENS) { > > > > + lens_ = std::make_unique<CameraLens>(ent); > > > > + ret = lens_->init(); > > > > + if (!ret && lens_->model() == it->second) { > > > > + break; > > > > + } > > > > + lens_.reset(); > > > > + } > > > > + if (!lens_) > > > > + LOG(IPU3, Warning) << "Lens device " << it->second << " not found"; > > > > + } > > > > + } > > > > +#endif > > > > + > > > > /* > > > > * \todo Define when to open and close video device nodes, as they > > > > * might impact on power consumption. > > > > diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h > > > > index ba8f0052..635566c8 100644 > > > > --- a/src/libcamera/pipeline/ipu3/cio2.h > > > > +++ b/src/libcamera/pipeline/ipu3/cio2.h > > > > @@ -18,6 +18,7 @@ > > > > > > > > namespace libcamera { > > > > > > > > +class CameraLens; > > > > class CameraSensor; > > > > class FrameBuffer; > > > > class MediaDevice; > > > > @@ -52,6 +53,7 @@ public: > > > > int stop(); > > > > > > > > CameraSensor *sensor() { return sensor_.get(); } > > > > + CameraLens *lens() { return lens_.get(); } > > > > const CameraSensor *sensor() const { return sensor_.get(); } > > > > > > > > FrameBuffer *queueBuffer(Request *request, FrameBuffer *rawBuffer); > > > > @@ -67,6 +69,7 @@ private: > > > > void cio2BufferReady(FrameBuffer *buffer); > > > > > > > > std::unique_ptr<CameraSensor> sensor_; > > > > + std::unique_ptr<CameraLens> lens_; > > > > std::unique_ptr<V4L2Subdevice> csi2_; > > > > std::unique_ptr<V4L2VideoDevice> output_; > > > > > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > > index 97003681..88775f67 100644 > > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > > @@ -24,6 +24,7 @@ > > > > #include <libcamera/stream.h> > > > > > > > > #include "libcamera/internal/camera.h" > > > > +#include "libcamera/internal/camera_lens.h" > > > > #include "libcamera/internal/camera_sensor.h" > > > > #include "libcamera/internal/delayed_controls.h" > > > > #include "libcamera/internal/device_enumerator.h" > > > > @@ -1255,8 +1256,12 @@ void IPU3CameraData::queueFrameAction(unsigned int id, > > > > { > > > > switch (action.op) { > > > > case ipa::ipu3::ActionSetSensorControls: { > > > > - const ControlList &controls = action.sensorControls; > > > > - delayedCtrls_->push(controls); > > > > + const ControlList &sensorControls = action.sensorControls; > > > > + delayedCtrls_->push(sensorControls); > > > > + if (cio2_.lens()) { > > > > + ControlList lensControls = action.lensControls; > > > > + cio2_.lens()->setControls(&lensControls); > > > > + } > > > > break; > > > > } > > > > case ipa::ipu3::ActionParamFilled: { > > > >
On Tue, Nov 16, 2021 at 6:07 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Han-Lin, > > On Tue, Nov 16, 2021 at 05:52:49PM +0800, Hanlin Chen wrote: > > On Fri, Nov 12, 2021 at 7:58 AM Laurent Pinchart wrote: > > > On Thu, Nov 11, 2021 at 05:12:52PM +0100, Jean-Michel Hautbois wrote: > > > > On 11/11/2021 11:49, Han-Lin Chen wrote: > > > > > Allow IPA to apply controls to the lens device. > > > > > > > > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > > > > > --- > > > > > meson.build | 6 ++++++ > > > > > src/libcamera/pipeline/ipu3/cio2.cpp | 30 ++++++++++++++++++++++++++++ > > > > > src/libcamera/pipeline/ipu3/cio2.h | 3 +++ > > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 9 +++++++-- > > > > > 4 files changed, 46 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/meson.build b/meson.build > > > > > index 7892a9e3..2a4b68a2 100644 > > > > > --- a/meson.build > > > > > +++ b/meson.build > > > > > @@ -108,6 +108,12 @@ if cc.has_argument('-Wno-c99-designator') > > > > > ] > > > > > endif > > > > > > > > > > +if get_option('android_platform') == 'cros' > > > > > + common_arguments += [ > > > > > + '-DOS_CHROMEOS', > > > > > + ] > > > > > +endif > > > > > > > > I am not really aware of specifics regarding this, why can't we have > > > > something not dependent on the OS ? > > > > > > There's a mail thread regarding VCM support on MS Surface machines (see > > > https://lists.libcamera.org/pipermail/libcamera-devel/2021-November/026526.html). > > > Raspberry Pi is also looking into VCM support. It would be best to > > > cooperate with all parties involved to define a way to expose the > > > relation between camera sensors and lens controllers to userspace, and > > > use it here. I'm really not keen on having a CrOS-specific hack. > > > > Looking forward to it :-). > > To make that happen, may I ask you to reply to David's proposal about > autofocus controls (https://lists.libcamera.org/pipermail/libcamera-devel/2021-October/025986.html) ? > Ah yes, thanks for the notice :-) Will do. > > > > > + > > > > > c_arguments += common_arguments > > > > > cpp_arguments += common_arguments > > > > > > > > > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp > > > > > index 59dda56b..233553c2 100644 > > > > > --- a/src/libcamera/pipeline/ipu3/cio2.cpp > > > > > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp > > > > > @@ -16,6 +16,7 @@ > > > > > #include <libcamera/geometry.h> > > > > > #include <libcamera/stream.h> > > > > > > > > > > +#include "libcamera/internal/camera_lens.h" > > > > > #include "libcamera/internal/camera_sensor.h" > > > > > #include "libcamera/internal/framebuffer.h" > > > > > #include "libcamera/internal/media_device.h" > > > > > @@ -159,6 +160,35 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index) > > > > > return -EINVAL; > > > > > } > > > > > > > > > > +#if defined(OS_CHROMEOS) > > > > > + /* > > > > > + * \todo Read the lens model from the sensor itself or from a device database. > > > > > + * For now use default values taken from ChromeOS. > > > > > + */ > > > > > + static std::unordered_map<std::string, std::string> sensorLens = { > > > > > + { "ov13858", "dw9714" }, > > > > > + { "imx258", "dw9807" }, > > > > > + { "imx355", "ak7375" } > > > > > + }; > > > > > > > > So, we need a list giving the lens model associated to a sensor... How > > > > is this dependent on chromeos ? Imagine I want to use the sensor lens on > > > > my SGo2, which has a ov8865, what is needed to do that ? > > > > > > > > > + > > > > > + auto it = sensorLens.find(sensor_->model()); > > > > > + if (it != sensorLens.end()) { > > > > > + const std::vector<MediaEntity *> &entities = media->entities(); > > > > > + for (auto ent : entities) { > > > > > + if (ent->function() == MEDIA_ENT_F_LENS) { > > > > > + lens_ = std::make_unique<CameraLens>(ent); > > > > > + ret = lens_->init(); > > > > > + if (!ret && lens_->model() == it->second) { > > > > > + break; > > > > > + } > > > > > + lens_.reset(); > > > > > + } > > > > > + if (!lens_) > > > > > + LOG(IPU3, Warning) << "Lens device " << it->second << " not found"; > > > > > + } > > > > > + } > > > > > +#endif > > > > > + > > > > > /* > > > > > * \todo Define when to open and close video device nodes, as they > > > > > * might impact on power consumption. > > > > > diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h > > > > > index ba8f0052..635566c8 100644 > > > > > --- a/src/libcamera/pipeline/ipu3/cio2.h > > > > > +++ b/src/libcamera/pipeline/ipu3/cio2.h > > > > > @@ -18,6 +18,7 @@ > > > > > > > > > > namespace libcamera { > > > > > > > > > > +class CameraLens; > > > > > class CameraSensor; > > > > > class FrameBuffer; > > > > > class MediaDevice; > > > > > @@ -52,6 +53,7 @@ public: > > > > > int stop(); > > > > > > > > > > CameraSensor *sensor() { return sensor_.get(); } > > > > > + CameraLens *lens() { return lens_.get(); } > > > > > const CameraSensor *sensor() const { return sensor_.get(); } > > > > > > > > > > FrameBuffer *queueBuffer(Request *request, FrameBuffer *rawBuffer); > > > > > @@ -67,6 +69,7 @@ private: > > > > > void cio2BufferReady(FrameBuffer *buffer); > > > > > > > > > > std::unique_ptr<CameraSensor> sensor_; > > > > > + std::unique_ptr<CameraLens> lens_; > > > > > std::unique_ptr<V4L2Subdevice> csi2_; > > > > > std::unique_ptr<V4L2VideoDevice> output_; > > > > > > > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > > > index 97003681..88775f67 100644 > > > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > > > @@ -24,6 +24,7 @@ > > > > > #include <libcamera/stream.h> > > > > > > > > > > #include "libcamera/internal/camera.h" > > > > > +#include "libcamera/internal/camera_lens.h" > > > > > #include "libcamera/internal/camera_sensor.h" > > > > > #include "libcamera/internal/delayed_controls.h" > > > > > #include "libcamera/internal/device_enumerator.h" > > > > > @@ -1255,8 +1256,12 @@ void IPU3CameraData::queueFrameAction(unsigned int id, > > > > > { > > > > > switch (action.op) { > > > > > case ipa::ipu3::ActionSetSensorControls: { > > > > > - const ControlList &controls = action.sensorControls; > > > > > - delayedCtrls_->push(controls); > > > > > + const ControlList &sensorControls = action.sensorControls; > > > > > + delayedCtrls_->push(sensorControls); > > > > > + if (cio2_.lens()) { > > > > > + ControlList lensControls = action.lensControls; > > > > > + cio2_.lens()->setControls(&lensControls); > > > > > + } > > > > > break; > > > > > } > > > > > case ipa::ipu3::ActionParamFilled: { > > > > > > > -- > Regards, > > Laurent Pinchart
diff --git a/meson.build b/meson.build index 7892a9e3..2a4b68a2 100644 --- a/meson.build +++ b/meson.build @@ -108,6 +108,12 @@ if cc.has_argument('-Wno-c99-designator') ] endif +if get_option('android_platform') == 'cros' + common_arguments += [ + '-DOS_CHROMEOS', + ] +endif + c_arguments += common_arguments cpp_arguments += common_arguments diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp index 59dda56b..233553c2 100644 --- a/src/libcamera/pipeline/ipu3/cio2.cpp +++ b/src/libcamera/pipeline/ipu3/cio2.cpp @@ -16,6 +16,7 @@ #include <libcamera/geometry.h> #include <libcamera/stream.h> +#include "libcamera/internal/camera_lens.h" #include "libcamera/internal/camera_sensor.h" #include "libcamera/internal/framebuffer.h" #include "libcamera/internal/media_device.h" @@ -159,6 +160,35 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index) return -EINVAL; } +#if defined(OS_CHROMEOS) + /* + * \todo Read the lens model from the sensor itself or from a device database. + * For now use default values taken from ChromeOS. + */ + static std::unordered_map<std::string, std::string> sensorLens = { + { "ov13858", "dw9714" }, + { "imx258", "dw9807" }, + { "imx355", "ak7375" } + }; + + auto it = sensorLens.find(sensor_->model()); + if (it != sensorLens.end()) { + const std::vector<MediaEntity *> &entities = media->entities(); + for (auto ent : entities) { + if (ent->function() == MEDIA_ENT_F_LENS) { + lens_ = std::make_unique<CameraLens>(ent); + ret = lens_->init(); + if (!ret && lens_->model() == it->second) { + break; + } + lens_.reset(); + } + if (!lens_) + LOG(IPU3, Warning) << "Lens device " << it->second << " not found"; + } + } +#endif + /* * \todo Define when to open and close video device nodes, as they * might impact on power consumption. diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h index ba8f0052..635566c8 100644 --- a/src/libcamera/pipeline/ipu3/cio2.h +++ b/src/libcamera/pipeline/ipu3/cio2.h @@ -18,6 +18,7 @@ namespace libcamera { +class CameraLens; class CameraSensor; class FrameBuffer; class MediaDevice; @@ -52,6 +53,7 @@ public: int stop(); CameraSensor *sensor() { return sensor_.get(); } + CameraLens *lens() { return lens_.get(); } const CameraSensor *sensor() const { return sensor_.get(); } FrameBuffer *queueBuffer(Request *request, FrameBuffer *rawBuffer); @@ -67,6 +69,7 @@ private: void cio2BufferReady(FrameBuffer *buffer); std::unique_ptr<CameraSensor> sensor_; + std::unique_ptr<CameraLens> lens_; std::unique_ptr<V4L2Subdevice> csi2_; std::unique_ptr<V4L2VideoDevice> output_; diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 97003681..88775f67 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -24,6 +24,7 @@ #include <libcamera/stream.h> #include "libcamera/internal/camera.h" +#include "libcamera/internal/camera_lens.h" #include "libcamera/internal/camera_sensor.h" #include "libcamera/internal/delayed_controls.h" #include "libcamera/internal/device_enumerator.h" @@ -1255,8 +1256,12 @@ void IPU3CameraData::queueFrameAction(unsigned int id, { switch (action.op) { case ipa::ipu3::ActionSetSensorControls: { - const ControlList &controls = action.sensorControls; - delayedCtrls_->push(controls); + const ControlList &sensorControls = action.sensorControls; + delayedCtrls_->push(sensorControls); + if (cio2_.lens()) { + ControlList lensControls = action.lensControls; + cio2_.lens()->setControls(&lensControls); + } break; } case ipa::ipu3::ActionParamFilled: {
Allow IPA to apply controls to the lens device. Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> --- meson.build | 6 ++++++ src/libcamera/pipeline/ipu3/cio2.cpp | 30 ++++++++++++++++++++++++++++ src/libcamera/pipeline/ipu3/cio2.h | 3 +++ src/libcamera/pipeline/ipu3/ipu3.cpp | 9 +++++++-- 4 files changed, 46 insertions(+), 2 deletions(-)