[libcamera-devel] [PATCH v6 01/10] rkisp1: Add camera lens to PH and expose it to the IPA
Daniel Semkowicz
dse at thaumatec.com
Thu Jun 1 09:33:01 CEST 2023
Hello,
On Wed, May 31, 2023 at 8:39 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hello,
>
> On Mon, May 29, 2023 at 12:09:41PM +0200, Daniel Semkowicz via libcamera-devel wrote:
> > On Fri, May 12, 2023 at 12:08 PM Michael Riesch wrote:
> > > On 5/11/23 14:14, Daniel Semkowicz wrote:
> > > > On Wed, Apr 26, 2023 at 10:18 AM Michael Riesch wrote:
> > > >> 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?
>
> It's more about how we build abstraction layers inside libcamera than
> about the subdevices. We have CameraSensor and CameraLens classes, and
> those should expose an abstract interface towards the pipeline handler,
> regardless of how the kernel exposes the corresponding devices. Those
> two classes should move from using V4L2 controls to internal libcamera
> controls, and avoid exposing anything V4L2-specific. The interface
> between the IPA module and pipeline handler should use the same internal
> controls. The translation to V4L2 controls, and the mapping to
> particular subdevs, will be internal to the helper classes.
>
> I don't really envision a need for multiple flashes per camera. I also
> don't think we'll have to deal with multiple independent focus lenses,
> but we will have optical zoom in addition to focus.
This clarifies a lot to me. Right now, CameraLens just exposes
the V4L2 controls and I assumed this is the target approach, hence my
doubts.
Now, I need the decision how to extend the init() parameters, because it
impacts how I will handle it in the IPA. I am thinking about few options,
most of them already proposed in this thread:
1. Leave the current implementation and solve this problem later:
int init(..., const ControlInfoMap &sensorControls, const
ControlInfoMap &lensControls, ...)
2. Merge all controls into single parameter:
int init(..., const ControlInfoMap &internalControls, ...)
3. Leave the sensorControls intact, add auxiliaryDevicesControls
parameter and put there all controls other than sensor:
int init(..., const ControlInfoMap &sensorControls, const
ControlInfoMap &auxiliaryDevicesControls, ...)
4. Create a separate map for each auxiliary device. This will avoid
problems if there are multiple devices of the same type:
int init(..., const ControlInfoMap &sensorControls, const
Span<ControlInfoMap> &auxiliaryDevicesControls, ...)
What do you think?
>
> > > >>> => (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.
> > >
> > > >>> 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;
>
> --
> Regards,
>
> Laurent Pinchart
Best regards
Daniel
More information about the libcamera-devel
mailing list