| Message ID | 20211203125359.25191-1-kieran.bingham@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | 294663eece8c067d268442724b969c9dfa081b0a |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thank you for the patch. On Fri, Dec 03, 2021 at 12:53:59PM +0000, Kieran Bingham wrote: > If there is no lens detected by the system, then we will not be able to > set the control, so we can skip processing of the list. > > Furthermore, if the IPA has not set a V4L2_CID_FOCUS_ABSOLUTE control, > then a warning will be printed as the lensControls.get() will not > succeed. > > Break out of the control parsing when there is no CameraLens > device, or if there is no absolute focus control set by the IPA. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 1215bdb84224..16380d2091b2 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -1242,13 +1242,19 @@ void IPU3CameraData::queueFrameAction(unsigned int id, > const ControlList &sensorControls = action.sensorControls; > delayedCtrls_->push(sensorControls); > > + CameraLens *focusLens = cio2_.sensor()->focusLens(); > + if (!focusLens) > + break; > + > const ControlList lensControls = action.lensControls; > + if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE)) > + break; > + > const ControlValue &focusValue = > lensControls.get(V4L2_CID_FOCUS_ABSOLUTE); > I'd drop this blank line. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > - CameraLens *focusLens = cio2_.sensor()->focusLens(); > - if (focusLens && !focusValue.isNone()) > - focusLens->setFocusPostion(focusValue.get<int32_t>()); > + focusLens->setFocusPostion(focusValue.get<int32_t>()); > + > break; > } > case ipa::ipu3::ActionParamFilled: {
Quoting Laurent Pinchart (2021-12-03 14:17:35) > Hi Kieran, > > Thank you for the patch. > > On Fri, Dec 03, 2021 at 12:53:59PM +0000, Kieran Bingham wrote: > > If there is no lens detected by the system, then we will not be able to > > set the control, so we can skip processing of the list. > > > > Furthermore, if the IPA has not set a V4L2_CID_FOCUS_ABSOLUTE control, > > then a warning will be printed as the lensControls.get() will not > > succeed. > > > > Break out of the control parsing when there is no CameraLens > > device, or if there is no absolute focus control set by the IPA. > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 12 +++++++++--- > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index 1215bdb84224..16380d2091b2 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -1242,13 +1242,19 @@ void IPU3CameraData::queueFrameAction(unsigned int id, > > const ControlList &sensorControls = action.sensorControls; > > delayedCtrls_->push(sensorControls); > > > > + CameraLens *focusLens = cio2_.sensor()->focusLens(); > > + if (!focusLens) > > + break; > > + > > const ControlList lensControls = action.lensControls; > > + if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE)) > > + break; > > + > > const ControlValue &focusValue = > > lensControls.get(V4L2_CID_FOCUS_ABSOLUTE); > > > > I'd drop this blank line. I can, I kind of prefer it when the line above is broken over two lines though: const ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE); focusLens->setFocusPostion(focusValue.get<int32_t>()); vs const ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE); focusLens->setFocusPostion(focusValue.get<int32_t>()); That takes us to 92 chars on the FOCUS_ABSOLUTE, so if you prefer without the blank line, I'll move that up. It's still within our hard-line-lenght-limit. > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thanks -- Kieran > > > - CameraLens *focusLens = cio2_.sensor()->focusLens(); > > - if (focusLens && !focusValue.isNone()) > > - focusLens->setFocusPostion(focusValue.get<int32_t>()); > > + focusLens->setFocusPostion(focusValue.get<int32_t>()); > > + > > break; > > } > > case ipa::ipu3::ActionParamFilled: { > > -- > Regards, > > Laurent Pinchart
Hi Kieran, On Fri, Dec 03, 2021 at 04:17:35PM +0200, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Fri, Dec 03, 2021 at 12:53:59PM +0000, Kieran Bingham wrote: > > If there is no lens detected by the system, then we will not be able to > > set the control, so we can skip processing of the list. > > > > Furthermore, if the IPA has not set a V4L2_CID_FOCUS_ABSOLUTE control, > > then a warning will be printed as the lensControls.get() will not > > succeed. > > > > Break out of the control parsing when there is no CameraLens > > device, or if there is no absolute focus control set by the IPA. > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 12 +++++++++--- > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index 1215bdb84224..16380d2091b2 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -1242,13 +1242,19 @@ void IPU3CameraData::queueFrameAction(unsigned int id, > > const ControlList &sensorControls = action.sensorControls; > > delayedCtrls_->push(sensorControls); > > > > + CameraLens *focusLens = cio2_.sensor()->focusLens(); > > + if (!focusLens) > > + break; > > + > > const ControlList lensControls = action.lensControls; > > + if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE)) > > + break; > > + > > const ControlValue &focusValue = > > lensControls.get(V4L2_CID_FOCUS_ABSOLUTE); > > > > I'd drop this blank line. Was about to suggest the same! Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > - CameraLens *focusLens = cio2_.sensor()->focusLens(); > > - if (focusLens && !focusValue.isNone()) > > - focusLens->setFocusPostion(focusValue.get<int32_t>()); > > + focusLens->setFocusPostion(focusValue.get<int32_t>()); > > + > > break; > > } > > case ipa::ipu3::ActionParamFilled: { > > -- > Regards, > > Laurent Pinchart
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 1215bdb84224..16380d2091b2 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -1242,13 +1242,19 @@ void IPU3CameraData::queueFrameAction(unsigned int id, const ControlList &sensorControls = action.sensorControls; delayedCtrls_->push(sensorControls); + CameraLens *focusLens = cio2_.sensor()->focusLens(); + if (!focusLens) + break; + const ControlList lensControls = action.lensControls; + if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE)) + break; + const ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE); - CameraLens *focusLens = cio2_.sensor()->focusLens(); - if (focusLens && !focusValue.isNone()) - focusLens->setFocusPostion(focusValue.get<int32_t>()); + focusLens->setFocusPostion(focusValue.get<int32_t>()); + break; } case ipa::ipu3::ActionParamFilled: {
If there is no lens detected by the system, then we will not be able to set the control, so we can skip processing of the list. Furthermore, if the IPA has not set a V4L2_CID_FOCUS_ABSOLUTE control, then a warning will be printed as the lensControls.get() will not succeed. Break out of the control parsing when there is no CameraLens device, or if there is no absolute focus control set by the IPA. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)