[libcamera-devel] [PATCH 1/3] libcamera: rkisp1: Control the lens from pipeline

[Jacopo Mondi]

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 at 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 at 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