[libcamera-devel] [PATCH v6 01/10] rkisp1: Add camera lens to PH and expose it to the IPA
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed May 31 20:39:45 CEST 2023
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.
> > >>> => (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
More information about the libcamera-devel
mailing list