| Message ID | 20220628090656.19572-2-dse@thaumatec.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Daniel, On Tue, Jun 28, 2022 at 11:06:54AM +0200, Daniel Semkowicz via libcamera-devel wrote: > Control lens focus from rkisp1 pipeline, using CameraLens controller > and expose lens controls to the IPA during configure(). > > Signed-off-by: Daniel Semkowicz <dse@thaumatec.com> > --- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index 7cf36524..363273b2 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -28,6 +28,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" > @@ -107,6 +108,7 @@ private: > void paramFilled(unsigned int frame); > void setSensorControls(unsigned int frame, > const ControlList &sensorControls); > + void setLensControls(const ControlList &lensControls); > > void metadataReady(unsigned int frame, const ControlList &metadata); > }; > @@ -353,6 +355,20 @@ void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame, > delayedCtrls_->push(sensorControls); > } > > +void RkISP1CameraData::setLensControls(const ControlList &lensControls) > +{ > + CameraLens *focusLens = sensor_->focusLens(); > + if (!focusLens) > + return; > + > + if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE)) > + return; I would rather do so in the CameraLens class. The class validates that V4L2_CID_FOCUS_ABSOLUTE is available in CameraLens::init(). I would store a flag there and return immediately from setFocusPosition() if not available. > + > + const ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE); > + > + focusLens->setFocusPosition(focusValue.get<int32_t>()); > +} > + > void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata) > { > RkISP1FrameInfo *info = frameInfo_.find(frame); > @@ -654,6 +670,10 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > std::map<uint32_t, ControlInfoMap> entityControls; > entityControls.emplace(0, data->sensor_->controls()); > > + CameraLens *lens = data->sensor_->focusLens(); > + if (lens) > + entityControls.emplace(1, lens->controls()); > + I would have made 1 patch for the PH/IPA initialization, and one patch for the IPA/PH control set phase instead of making one patch for PH and one for IPA. Hope this makes sense :P The patch direction looks good! Thanks j > ret = data->ipa_->configure(sensorInfo, streamConfig, entityControls); > if (ret) { > LOG(RkISP1, Error) << "failed configuring IPA (" << ret << ")"; > -- > 2.34.1 >
Hi Jacopo, On Thu, Jul 14, 2022 at 4:53 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > Hi Daniel, > > On Tue, Jun 28, 2022 at 11:06:54AM +0200, Daniel Semkowicz via libcamera-devel wrote: > > Control lens focus from rkisp1 pipeline, using CameraLens controller > > and expose lens controls to the IPA during configure(). > > > > Signed-off-by: Daniel Semkowicz <dse@thaumatec.com> > > --- > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 20 ++++++++++++++++++++ > > 1 file changed, 20 insertions(+) > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > index 7cf36524..363273b2 100644 > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > @@ -28,6 +28,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" > > @@ -107,6 +108,7 @@ private: > > void paramFilled(unsigned int frame); > > void setSensorControls(unsigned int frame, > > const ControlList &sensorControls); > > + void setLensControls(const ControlList &lensControls); > > > > void metadataReady(unsigned int frame, const ControlList &metadata); > > }; > > @@ -353,6 +355,20 @@ void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame, > > delayedCtrls_->push(sensorControls); > > } > > > > +void RkISP1CameraData::setLensControls(const ControlList &lensControls) > > +{ > > + CameraLens *focusLens = sensor_->focusLens(); > > + if (!focusLens) > > + return; > > + > > + if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE)) > > + return; > > I would rather do so in the CameraLens class. > > The class validates that V4L2_CID_FOCUS_ABSOLUTE is available in > CameraLens::init(). I would store a flag there and return immediately > from setFocusPosition() if not available. Good point! Now I wonder if the CameraSensor::focusLens() should return the valid pointer at all if the CameraLens::init() failed. If it is specified that V4L2_CID_FOCUS_ABSOLUTE is mandatory and init fails, then I would expect to not receive the CameraLens object if it is not valid. Maybe We should return nullptr in such case? Then We can forget about additional checks in PH and IPA. If the CameraLens does not meet the mandatory requirements then I would treat this situation as if the lens are not available at all. > > > + > > + const ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE); > > + > > + focusLens->setFocusPosition(focusValue.get<int32_t>()); > > +} > > + > > void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata) > > { > > RkISP1FrameInfo *info = frameInfo_.find(frame); > > @@ -654,6 +670,10 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > > std::map<uint32_t, ControlInfoMap> entityControls; > > entityControls.emplace(0, data->sensor_->controls()); > > > > + CameraLens *lens = data->sensor_->focusLens(); > > + if (lens) > > + entityControls.emplace(1, lens->controls()); > > + > > I would have made 1 patch for the PH/IPA initialization, and one patch > for the IPA/PH control set phase instead of making one patch for PH > and one for IPA. Hope this makes sense :P Yes, this makes sense. I will try to arrange the next patchset this way :) > > The patch direction looks good! > > Thanks > j > > ret = data->ipa_->configure(sensorInfo, streamConfig, entityControls); > > if (ret) { > > LOG(RkISP1, Error) << "failed configuring IPA (" << ret << ")"; > > -- > > 2.34.1 > > Best regards Daniel
Hi Daniel On Mon, Jul 18, 2022 at 03:56:07PM +0200, Daniel Semkowicz wrote: > Hi Jacopo, > > On Thu, Jul 14, 2022 at 4:53 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > Hi Daniel, > > > > On Tue, Jun 28, 2022 at 11:06:54AM +0200, Daniel Semkowicz via libcamera-devel wrote: > > > Control lens focus from rkisp1 pipeline, using CameraLens controller > > > and expose lens controls to the IPA during configure(). > > > > > > Signed-off-by: Daniel Semkowicz <dse@thaumatec.com> > > > --- > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 20 ++++++++++++++++++++ > > > 1 file changed, 20 insertions(+) > > > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > index 7cf36524..363273b2 100644 > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > @@ -28,6 +28,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" > > > @@ -107,6 +108,7 @@ private: > > > void paramFilled(unsigned int frame); > > > void setSensorControls(unsigned int frame, > > > const ControlList &sensorControls); > > > + void setLensControls(const ControlList &lensControls); > > > > > > void metadataReady(unsigned int frame, const ControlList &metadata); > > > }; > > > @@ -353,6 +355,20 @@ void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame, > > > delayedCtrls_->push(sensorControls); > > > } > > > > > > +void RkISP1CameraData::setLensControls(const ControlList &lensControls) > > > +{ > > > + CameraLens *focusLens = sensor_->focusLens(); > > > + if (!focusLens) > > > + return; > > > + > > > + if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE)) > > > + return; > > > > I would rather do so in the CameraLens class. > > > > The class validates that V4L2_CID_FOCUS_ABSOLUTE is available in > > CameraLens::init(). I would store a flag there and return immediately > > from setFocusPosition() if not available. > > Good point! Now I wonder if the CameraSensor::focusLens() should return > the valid pointer at all if the CameraLens::init() failed. If it is > specified that V4L2_CID_FOCUS_ABSOLUTE is mandatory and init fails, then > I would expect to not receive the CameraLens object if it is not valid. > > Maybe We should return nullptr in such case? Then We can forget about > additional checks in PH and IPA. If the CameraLens does not meet the > mandatory requirements then I would treat this situation as if the lens > are not available at all. > It feels rather dangerous to return a nullptr because a feature is missing, it's a recipe for lazy callers to get into a segfault. Imagine you connect a different camera to your platform and the lens driver does not support the mandatory control while the one you were using did. You would have a segfault. True, there's plenty of messages in the log that could suggest you what went wrong, but segfaults are never nice. It's true that if the lens is not registered in DTS right now you get a nullptr, so the pipeline handler should already be prepared to handle that situation... Please note that if the CameraLens class validation fails then CameraSensor::init() fails as well, so if the pipeline handler is educated enough it could bail out soon enough or simply ignore the lens. Also, if we call CameraLens::setFocusPosition() and the lens does not support V4L2_CID_FOCUS_ABSOLUTE the V4L2Subdevice::setControl() call would complain loudly but no crashes are expected. All in all, I would not perform any check here (IPU3 should probably be updated too) but rather let the CameraLens::setFocusPosition() fail at setControls() time, or catch it before even getting there (which might be better as we can express a more precise error message instead of relying on the: LOG(V4L2, Error) << "Control " << utils::hex(id) << " not found"; Error message in V4L2Device::setControls() which reports the numerical control id, which we know it's not nice to decipher. Does this make sense to you ? Thanks j > > > > > + > > > + const ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE); > > > + > > > + focusLens->setFocusPosition(focusValue.get<int32_t>()); > > > +} > > > + > > > void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata) > > > { > > > RkISP1FrameInfo *info = frameInfo_.find(frame); > > > @@ -654,6 +670,10 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > > > std::map<uint32_t, ControlInfoMap> entityControls; > > > entityControls.emplace(0, data->sensor_->controls()); > > > > > > + CameraLens *lens = data->sensor_->focusLens(); > > > + if (lens) > > > + entityControls.emplace(1, lens->controls()); > > > + > > > > I would have made 1 patch for the PH/IPA initialization, and one patch > > for the IPA/PH control set phase instead of making one patch for PH > > and one for IPA. Hope this makes sense :P > > Yes, this makes sense. I will try to arrange the next patchset this way :) > > > > > The patch direction looks good! > > > > Thanks > > j > > > ret = data->ipa_->configure(sensorInfo, streamConfig, entityControls); > > > if (ret) { > > > LOG(RkISP1, Error) << "failed configuring IPA (" << ret << ")"; > > > -- > > > 2.34.1 > > > > > Best regards > Daniel
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index 7cf36524..363273b2 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -28,6 +28,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" @@ -107,6 +108,7 @@ private: void paramFilled(unsigned int frame); void setSensorControls(unsigned int frame, const ControlList &sensorControls); + void setLensControls(const ControlList &lensControls); void metadataReady(unsigned int frame, const ControlList &metadata); }; @@ -353,6 +355,20 @@ void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame, delayedCtrls_->push(sensorControls); } +void RkISP1CameraData::setLensControls(const ControlList &lensControls) +{ + CameraLens *focusLens = sensor_->focusLens(); + if (!focusLens) + return; + + if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE)) + return; + + const ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE); + + focusLens->setFocusPosition(focusValue.get<int32_t>()); +} + void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata) { RkISP1FrameInfo *info = frameInfo_.find(frame); @@ -654,6 +670,10 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) std::map<uint32_t, ControlInfoMap> entityControls; entityControls.emplace(0, data->sensor_->controls()); + CameraLens *lens = data->sensor_->focusLens(); + if (lens) + entityControls.emplace(1, lens->controls()); + ret = data->ipa_->configure(sensorInfo, streamConfig, entityControls); if (ret) { LOG(RkISP1, Error) << "failed configuring IPA (" << ret << ")";
Control lens focus from rkisp1 pipeline, using CameraLens controller and expose lens controls to the IPA during configure(). Signed-off-by: Daniel Semkowicz <dse@thaumatec.com> --- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)