[libcamera-devel] [PATCH v6 01/10] rkisp1: Add camera lens to PH and expose it to the IPA

[Daniel Semkowicz]

Hello Michael,

On Fri, May 12, 2023 at 12:08 PM Michael Riesch
<michael.riesch at wolfvision.net> wrote:
>
> Hi Daniel,
>
> On 5/11/23 14:14, Daniel Semkowicz wrote:
> > Hi Michael,
> >
> > On Wed, Apr 26, 2023 at 10:18 AM Michael Riesch
> > <michael.riesch at wolfvision.net> wrote:
> >>
> >> Hi Daniel,
> >>
> >> On 3/31/23 10:19, Daniel Semkowicz via libcamera-devel wrote:
> >>> Extend the IPA init() function by additional lensControls input
> >>> parameter. Check in pipeline handler if camera lens exists, and expose
> >>> its controls to the IPA using the new parameter.
> >>>
> >>> Signed-off-by: Daniel Semkowicz <dse at thaumatec.com>
> >>> ---
> >>>  include/libcamera/ipa/rkisp1.mojom       | 3 ++-
> >>>  src/ipa/rkisp1/rkisp1.cpp                | 7 +++++++
> >>>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++++++++-
> >>>  3 files changed, 17 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
> >>> index 1009e970..d4ff1230 100644
> >>> --- a/include/libcamera/ipa/rkisp1.mojom
> >>> +++ b/include/libcamera/ipa/rkisp1.mojom
> >>> @@ -17,7 +17,8 @@ interface IPARkISP1Interface {
> >>>       init(libcamera.IPASettings settings,
> >>>            uint32 hwRevision,
> >>>            libcamera.IPACameraSensorInfo sensorInfo,
> >>> -          libcamera.ControlInfoMap sensorControls)
> >>> +          libcamera.ControlInfoMap sensorControls,
> >>> +          libcamera.ControlInfoMap lensControls)
> >>
> >> One could imagine that in future other subdevices are supported. For
> >> example, I expect that soon we'll have a "flashControls" parameter.
> >> Would it make sense to gather all those controls in a single parameter
> >> "hwControls" (name to be bikeshedded)? (In #libcamera the notion of
> >> internal controls has been introduced recently. Maybe simply
> >> "internalControls"?)
> >>
> >> BTW I also could imagine that a "lensInfo" parameter is necessary for
> >> complex lens controllers (e.g., does the lens have a zoom function, how
> >> many zoom lenses does it have, model of the lens controller, ...). But
> >> for simple VCM controllers this may not be necessary, so feel free to
> >> ignore this at this point.
> >
> > It would probably make sense, but I would like not to do everything at once,
> > but introduce the changes gradually. I would suggest to extended it with the
> > introduction of next optional subdevice.
>
> Going one step at a time makes sense, of course. As I already said, the
> "lensInfo" parameter can be introduced once it is actually required and
> used.
>
> On the other hand, it probably makes sense to discuss the
> "sensorControls", "lensControls", ... -> "internalControls" issue right
> now. Of course I can wait until your series is applied and then propose
> a change, but this would be a bit back-and-forth. If we agree that there
> should be a single parameter (instead of one parameter per subdev), then
> this change should be incorporated in this series already IMHO.
>
> Of course, there may be perfectly fine reasons to have one parameter per
> subdev. I can't be the judge of that, but it would be great to see a
> verdict on that case.

I agree We need to find a good way to handle multiple subdevices,
but there are already a lot of changes in this series. That is why I am
reluctant to add even more.  I would suggest starting a separate thread
to find a solution to this problem. What others think about it?

Is it safe to gather all controls in a single parameter?
Can we guarantee the same control will not show up in two different
subdevices? Is it possible there will be two subdevices of the same
type, e.g. 2x flashlight?

>
> >>>               => (int32 ret, libcamera.ControlInfoMap ipaControls);
> >>>       start() => (int32 ret);
> >>>       stop();
> >>
> >> Shouldn't processStatsBuffer have an additional "lensControls"
> >> parameter? Of course, the comment above would apply here as well and we
> >> could get away with simply adjusting the name "sensorControls" ->
> >> "internalControls".
> >
> > Right now, We do not report any current values of lens controls to the IPA,
> > so such parameter would be left unused.
>
> Right, this should be proposed/introduced when it is actually required/used.
>
> Best regards,
> Michael
>
> >
> > Best regards
> > Daniel
> >
> >>
> >> Best regards,
> >> Michael
> >>
> >>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> >>> index 6544c925..d338d374 100644
> >>> --- a/src/ipa/rkisp1/rkisp1.cpp
> >>> +++ b/src/ipa/rkisp1/rkisp1.cpp
> >>> @@ -7,6 +7,7 @@
> >>>
> >>>  #include <algorithm>
> >>>  #include <math.h>
> >>> +#include <optional>
> >>>  #include <queue>
> >>>  #include <stdint.h>
> >>>  #include <string.h>
> >>> @@ -52,6 +53,7 @@ public:
> >>>       int init(const IPASettings &settings, unsigned int hwRevision,
> >>>                const IPACameraSensorInfo &sensorInfo,
> >>>                const ControlInfoMap &sensorControls,
> >>> +              const ControlInfoMap &lensControls,
> >>>                ControlInfoMap *ipaControls) override;
> >>>       int start() override;
> >>>       void stop() override;
> >>> @@ -80,6 +82,7 @@ private:
> >>>       std::map<unsigned int, MappedFrameBuffer> mappedBuffers_;
> >>>
> >>>       ControlInfoMap sensorControls_;
> >>> +     std::optional<ControlInfoMap> lensControls_;
> >>>
> >>>       /* revision-specific data */
> >>>       rkisp1_cif_isp_version hwRevision_;
> >>> @@ -123,6 +126,7 @@ std::string IPARkISP1::logPrefix() const
> >>>  int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
> >>>                   const IPACameraSensorInfo &sensorInfo,
> >>>                   const ControlInfoMap &sensorControls,
> >>> +                 const ControlInfoMap &lensControls,
> >>>                   ControlInfoMap *ipaControls)
> >>>  {
> >>>       /* \todo Add support for other revisions */
> >>> @@ -160,6 +164,9 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
> >>>       context_.configuration.sensor.lineDuration = sensorInfo.minLineLength
> >>>                                                  * 1.0s / sensorInfo.pixelRate;
> >>>
> >>> +     if (!lensControls.empty())
> >>> +             lensControls_ = lensControls;
> >>> +
> >>>       /* Load the tuning data file. */
> >>>       File file(settings.configurationFile);
> >>>       if (!file.open(File::OpenModeFlag::ReadOnly)) {
> >>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> >>> index 8a30fe06..f966254a 100644
> >>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> >>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> >>> @@ -32,6 +32,7 @@
> >>>  #include <libcamera/ipa/rkisp1_ipa_proxy.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"
> >>> @@ -367,8 +368,14 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)
> >>>               return ret;
> >>>       }
> >>>
> >>> +     ControlInfoMap lensControls;
> >>> +     CameraLens *lens = sensor_->focusLens();
> >>> +     if (lens)
> >>> +             lensControls = lens->controls();
> >>> +
> >>>       ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,
> >>> -                      sensorInfo, sensor_->controls(), &controlInfo_);
> >>> +                      sensorInfo, sensor_->controls(), lensControls,
> >>> +                      &controlInfo_);
> >>>       if (ret < 0) {
> >>>               LOG(RkISP1, Error) << "IPA initialization failure";
> >>>               return ret;

Best regards
Daniel