| Message ID | 20220630133902.321099-22-jacopo@jmondi.org |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series |
|
| Related | show |
Quoting Jacopo Mondi via libcamera-devel (2022-06-30 14:39:00) > Add to the internal controls LensFocusAbsolute as a draft control > as it is currently identical to V4L2_CID_FOCUS_ABSOLUTE. > > Use the newly introduced control in the IPU3 pipeline handler and IPA > module. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > include/libcamera/ipa/ipu3.mojom | 3 +-- > src/ipa/ipu3/ipu3.cpp | 7 ++----- > src/libcamera/internal_control_ids.yaml | 13 +++++++++++++ > src/libcamera/pipeline/ipu3/ipu3.cpp | 13 +++++-------- > 4 files changed, 21 insertions(+), 15 deletions(-) > > diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom > index 64fe65fdd5fc..bfeadc58c52a 100644 > --- a/include/libcamera/ipa/ipu3.mojom > +++ b/include/libcamera/ipa/ipu3.mojom > @@ -35,8 +35,7 @@ interface IPAIPU3Interface { > }; > > interface IPAIPU3EventInterface { > - setSensorControls(uint32 frame, libcamera.ControlList sensorControls, > - libcamera.ControlList lensControls); > + setSensorControls(uint32 frame, libcamera.ControlList sensorControls); > paramsBufferReady(uint32 frame); > metadataReady(uint32 frame, libcamera.ControlList metadata); > }; > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > index 6d622b4c290b..2f158df367a8 100644 > --- a/src/ipa/ipu3/ipu3.cpp > +++ b/src/ipa/ipu3/ipu3.cpp > @@ -541,12 +541,9 @@ void IPAIPU3::setControls(unsigned int frame, const IPAActiveState &state) > ControlList ctrls(controls::internal::controls); > ctrls.set(controls::internal::ExposureTime, state.agc.exposure); > ctrls.set(controls::internal::AnalogueGain, static_cast<float>(state.agc.gain)); > + ctrls.set(controls::internal::draft::LensFocusAbsolute, state.af.focus); > > - ControlList lensCtrls(controls::controls); > - lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE, > - static_cast<int32_t>(state.af.focus)); > - > - setSensorControls.emit(frame, ctrls, lensCtrls); > + setSensorControls.emit(frame, ctrls); > } > > } /* namespace ipa::ipu3 */ > diff --git a/src/libcamera/internal_control_ids.yaml b/src/libcamera/internal_control_ids.yaml > index 6d3775afcf67..804c227213a7 100644 > --- a/src/libcamera/internal_control_ids.yaml > +++ b/src/libcamera/internal_control_ids.yaml > @@ -24,4 +24,17 @@ controls: > description: | > The sensor analogue gain value. > > + # ---------------------------------------------------------------------------- > + # Draft controls section > + > + - LensFocusAbsolute: > + type: int32_t > + draft: true > + description: | > + This control sets the focal point of the camera to the specified > + position. The unit is undefined. Positive values set the focus closer to > + the camera, negative values towards infinity. I think this is something that we should be returning in request->metadata(). So that means it can't be an internal only control ? We should define the units too. I thought David had a definition based around a calibrated/known infinity point? > + > + Currently identical to V4L2_CID_FOCUS_ABSOLUTE. > + > ... > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 55b96137f065..1dbcdd0b56e6 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -86,8 +86,7 @@ public: > private: > void metadataReady(unsigned int id, const ControlList &metadata); > void paramsBufferReady(unsigned int id); > - void setSensorControls(unsigned int id, const ControlList &sensorControls, > - const ControlList &lensControls); > + void setSensorControls(unsigned int id, const ControlList &sensorControls); > }; > > class IPU3CameraConfiguration : public CameraConfiguration > @@ -1247,8 +1246,7 @@ int IPU3CameraData::loadIPA() > } > > void IPU3CameraData::setSensorControls([[maybe_unused]] unsigned int id, > - const ControlList &sensorControls, > - const ControlList &lensControls) > + const ControlList &sensorControls) > { > CameraSensor *sensor = cio2_.sensor(); > > @@ -1258,12 +1256,11 @@ void IPU3CameraData::setSensorControls([[maybe_unused]] unsigned int id, > if (!focusLens) > return; > > - if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE)) > + if (!sensorControls.contains(controls::internal::draft::LensFocusAbsolute)) > return; > > - const ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE); > - > - focusLens->setFocusPosition(focusValue.get<int32_t>()); > + int32_t focusAbsolute = sensorControls.get(controls::internal::draft::LensFocusAbsolute); > + focusLens->setFocusPosition(focusAbsolute); And I think conversion between our defined units and the units specific to a lens driver should then be handled in the CameraLens class, so passing in our 'defined' libcamera units type here should be fine. > } > > void IPU3CameraData::paramsBufferReady(unsigned int id) > -- > 2.36.1 >
Hi Kieran On Thu, Jun 30, 2022 at 11:14:13PM +0100, Kieran Bingham wrote: > Quoting Jacopo Mondi via libcamera-devel (2022-06-30 14:39:00) > > Add to the internal controls LensFocusAbsolute as a draft control > > as it is currently identical to V4L2_CID_FOCUS_ABSOLUTE. > > > > Use the newly introduced control in the IPU3 pipeline handler and IPA > > module. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > include/libcamera/ipa/ipu3.mojom | 3 +-- > > src/ipa/ipu3/ipu3.cpp | 7 ++----- > > src/libcamera/internal_control_ids.yaml | 13 +++++++++++++ > > src/libcamera/pipeline/ipu3/ipu3.cpp | 13 +++++-------- > > 4 files changed, 21 insertions(+), 15 deletions(-) > > > > diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom > > index 64fe65fdd5fc..bfeadc58c52a 100644 > > --- a/include/libcamera/ipa/ipu3.mojom > > +++ b/include/libcamera/ipa/ipu3.mojom > > @@ -35,8 +35,7 @@ interface IPAIPU3Interface { > > }; > > > > interface IPAIPU3EventInterface { > > - setSensorControls(uint32 frame, libcamera.ControlList sensorControls, > > - libcamera.ControlList lensControls); > > + setSensorControls(uint32 frame, libcamera.ControlList sensorControls); > > paramsBufferReady(uint32 frame); > > metadataReady(uint32 frame, libcamera.ControlList metadata); > > }; > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > > index 6d622b4c290b..2f158df367a8 100644 > > --- a/src/ipa/ipu3/ipu3.cpp > > +++ b/src/ipa/ipu3/ipu3.cpp > > @@ -541,12 +541,9 @@ void IPAIPU3::setControls(unsigned int frame, const IPAActiveState &state) > > ControlList ctrls(controls::internal::controls); > > ctrls.set(controls::internal::ExposureTime, state.agc.exposure); > > ctrls.set(controls::internal::AnalogueGain, static_cast<float>(state.agc.gain)); > > + ctrls.set(controls::internal::draft::LensFocusAbsolute, state.af.focus); > > > > - ControlList lensCtrls(controls::controls); > > - lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE, > > - static_cast<int32_t>(state.af.focus)); > > - > > - setSensorControls.emit(frame, ctrls, lensCtrls); > > + setSensorControls.emit(frame, ctrls); > > } > > > > } /* namespace ipa::ipu3 */ > > diff --git a/src/libcamera/internal_control_ids.yaml b/src/libcamera/internal_control_ids.yaml > > index 6d3775afcf67..804c227213a7 100644 > > --- a/src/libcamera/internal_control_ids.yaml > > +++ b/src/libcamera/internal_control_ids.yaml > > @@ -24,4 +24,17 @@ controls: > > description: | > > The sensor analogue gain value. > > > > + # ---------------------------------------------------------------------------- > > + # Draft controls section > > + > > + - LensFocusAbsolute: > > + type: int32_t > > + draft: true > > + description: | > > + This control sets the focal point of the camera to the specified > > + position. The unit is undefined. Positive values set the focus closer to > > + the camera, negative values towards infinity. > > I think this is something that we should be returning in > request->metadata(). > > So that means it can't be an internal only control ? > > We should define the units too. I thought David had a definition based > around a calibrated/known infinity point? > We're missing a lot of pieces yet. We have defined a unit for LensPosition which requires a clear way to measure/calibrate the lens, and for internal usage we have no way yet to translate the generic control to the sensor-specific value. So this is identical to the V4L2 version, as that's what the IPA uses at the moment. > > + > > + Currently identical to V4L2_CID_FOCUS_ABSOLUTE. > > + > > ... > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index 55b96137f065..1dbcdd0b56e6 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -86,8 +86,7 @@ public: > > private: > > void metadataReady(unsigned int id, const ControlList &metadata); > > void paramsBufferReady(unsigned int id); > > - void setSensorControls(unsigned int id, const ControlList &sensorControls, > > - const ControlList &lensControls); > > + void setSensorControls(unsigned int id, const ControlList &sensorControls); > > }; > > > > class IPU3CameraConfiguration : public CameraConfiguration > > @@ -1247,8 +1246,7 @@ int IPU3CameraData::loadIPA() > > } > > > > void IPU3CameraData::setSensorControls([[maybe_unused]] unsigned int id, > > - const ControlList &sensorControls, > > - const ControlList &lensControls) > > + const ControlList &sensorControls) > > { > > CameraSensor *sensor = cio2_.sensor(); > > > > @@ -1258,12 +1256,11 @@ void IPU3CameraData::setSensorControls([[maybe_unused]] unsigned int id, > > if (!focusLens) > > return; > > > > - if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE)) > > + if (!sensorControls.contains(controls::internal::draft::LensFocusAbsolute)) > > return; > > > > - const ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE); > > - > > - focusLens->setFocusPosition(focusValue.get<int32_t>()); > > + int32_t focusAbsolute = sensorControls.get(controls::internal::draft::LensFocusAbsolute); > > + focusLens->setFocusPosition(focusAbsolute); > > And I think conversion between our defined units and the units specific to a > lens driver should then be handled in the CameraLens class, so passing > in our 'defined' libcamera units type here should be fine. > > > > } > > > > void IPU3CameraData::paramsBufferReady(unsigned int id) > > -- > > 2.36.1 > >
diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom index 64fe65fdd5fc..bfeadc58c52a 100644 --- a/include/libcamera/ipa/ipu3.mojom +++ b/include/libcamera/ipa/ipu3.mojom @@ -35,8 +35,7 @@ interface IPAIPU3Interface { }; interface IPAIPU3EventInterface { - setSensorControls(uint32 frame, libcamera.ControlList sensorControls, - libcamera.ControlList lensControls); + setSensorControls(uint32 frame, libcamera.ControlList sensorControls); paramsBufferReady(uint32 frame); metadataReady(uint32 frame, libcamera.ControlList metadata); }; diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp index 6d622b4c290b..2f158df367a8 100644 --- a/src/ipa/ipu3/ipu3.cpp +++ b/src/ipa/ipu3/ipu3.cpp @@ -541,12 +541,9 @@ void IPAIPU3::setControls(unsigned int frame, const IPAActiveState &state) ControlList ctrls(controls::internal::controls); ctrls.set(controls::internal::ExposureTime, state.agc.exposure); ctrls.set(controls::internal::AnalogueGain, static_cast<float>(state.agc.gain)); + ctrls.set(controls::internal::draft::LensFocusAbsolute, state.af.focus); - ControlList lensCtrls(controls::controls); - lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE, - static_cast<int32_t>(state.af.focus)); - - setSensorControls.emit(frame, ctrls, lensCtrls); + setSensorControls.emit(frame, ctrls); } } /* namespace ipa::ipu3 */ diff --git a/src/libcamera/internal_control_ids.yaml b/src/libcamera/internal_control_ids.yaml index 6d3775afcf67..804c227213a7 100644 --- a/src/libcamera/internal_control_ids.yaml +++ b/src/libcamera/internal_control_ids.yaml @@ -24,4 +24,17 @@ controls: description: | The sensor analogue gain value. + # ---------------------------------------------------------------------------- + # Draft controls section + + - LensFocusAbsolute: + type: int32_t + draft: true + description: | + This control sets the focal point of the camera to the specified + position. The unit is undefined. Positive values set the focus closer to + the camera, negative values towards infinity. + + Currently identical to V4L2_CID_FOCUS_ABSOLUTE. + ... diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 55b96137f065..1dbcdd0b56e6 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -86,8 +86,7 @@ public: private: void metadataReady(unsigned int id, const ControlList &metadata); void paramsBufferReady(unsigned int id); - void setSensorControls(unsigned int id, const ControlList &sensorControls, - const ControlList &lensControls); + void setSensorControls(unsigned int id, const ControlList &sensorControls); }; class IPU3CameraConfiguration : public CameraConfiguration @@ -1247,8 +1246,7 @@ int IPU3CameraData::loadIPA() } void IPU3CameraData::setSensorControls([[maybe_unused]] unsigned int id, - const ControlList &sensorControls, - const ControlList &lensControls) + const ControlList &sensorControls) { CameraSensor *sensor = cio2_.sensor(); @@ -1258,12 +1256,11 @@ void IPU3CameraData::setSensorControls([[maybe_unused]] unsigned int id, if (!focusLens) return; - if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE)) + if (!sensorControls.contains(controls::internal::draft::LensFocusAbsolute)) return; - const ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE); - - focusLens->setFocusPosition(focusValue.get<int32_t>()); + int32_t focusAbsolute = sensorControls.get(controls::internal::draft::LensFocusAbsolute); + focusLens->setFocusPosition(focusAbsolute); } void IPU3CameraData::paramsBufferReady(unsigned int id)
Add to the internal controls LensFocusAbsolute as a draft control as it is currently identical to V4L2_CID_FOCUS_ABSOLUTE. Use the newly introduced control in the IPU3 pipeline handler and IPA module. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- include/libcamera/ipa/ipu3.mojom | 3 +-- src/ipa/ipu3/ipu3.cpp | 7 ++----- src/libcamera/internal_control_ids.yaml | 13 +++++++++++++ src/libcamera/pipeline/ipu3/ipu3.cpp | 13 +++++-------- 4 files changed, 21 insertions(+), 15 deletions(-)