[libcamera-devel] [PATCH 4/4] ipa: rkisp1: add FrameDurationLimits control
Nicholas Roth
nicholas at rothemail.net
Sat Nov 19 19:33:16 CET 2022
Following up-- can we merge this?
On Mon, Nov 7, 2022 at 5:08 AM Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> Hi Kieran
>
> On Fri, Nov 04, 2022 at 12:37:34PM +0000, Kieran Bingham wrote:
> > Quoting Jacopo Mondi via libcamera-devel (2022-11-04 10:50:04)
> > > From: Nicholas Roth <nicholas at rothemail.net>
> > >
> > > Currently, the Android HAL does not work on rkisp1-based devices because
> > > required FrameDurationLimits metadata is missing from the IPA
> > > implementation.
> > >
> > > This change sets FrameDurationLimits for rkisp1 based on the existing
> > > ipu3 implementation, using the sensor's reported range of vertical
> > > blanking intervals with the minimum reported horizontal blanking
> > > interval.
> > >
> > > Signed-off-by: Nicholas Roth <nicholas at rothemail.net>
> > > ---
> > > include/libcamera/ipa/rkisp1.mojom | 6 ++-
> > > src/ipa/rkisp1/rkisp1.cpp | 57 +++++++++++++++++++++---
> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 11 +++--
> > > 3 files changed, 64 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
> > > index 36bf291e8a51..1009e970a1b5 100644
> > > --- a/include/libcamera/ipa/rkisp1.mojom
> > > +++ b/include/libcamera/ipa/rkisp1.mojom
> > > @@ -15,14 +15,16 @@ struct IPAConfigInfo {
> > >
> > > interface IPARkISP1Interface {
> > > init(libcamera.IPASettings settings,
> > > - uint32 hwRevision)
> > > + uint32 hwRevision,
> > > + libcamera.IPACameraSensorInfo sensorInfo,
> > > + libcamera.ControlInfoMap sensorControls)
> > > => (int32 ret, libcamera.ControlInfoMap ipaControls);
> > > start() => (int32 ret);
> > > stop();
> > >
> > > configure(IPAConfigInfo configInfo,
> > > map<uint32, libcamera.IPAStream> streamConfig)
> > > - => (int32 ret);
> > > + => (int32 ret, libcamera.ControlInfoMap ipaControls);
> > >
> > > mapBuffers(array<libcamera.IPABuffer> buffers);
> > > unmapBuffers(array<uint32> ids);
> > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > index e1ede62ead3a..6d1238d54f57 100644
> > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > @@ -49,12 +49,15 @@ public:
> > > IPARkISP1();
> > >
> > > int init(const IPASettings &settings, unsigned int hwRevision,
> > > + const IPACameraSensorInfo &sensorInfo,
> > > + const ControlInfoMap &sensorControls,
> > > ControlInfoMap *ipaControls) override;
> > > int start() override;
> > > void stop() override;
> > >
> > > int configure(const IPAConfigInfo &ipaConfig,
> > > - const std::map<uint32_t, IPAStream> &streamConfig) override;
> > > + const std::map<uint32_t, IPAStream> &streamConfig,
> > > + ControlInfoMap *ipaControls) override;
> > > void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> > > void unmapBuffers(const std::vector<unsigned int> &ids) override;
> > >
> > > @@ -67,6 +70,9 @@ protected:
> > > std::string logPrefix() const override;
> > >
> > > private:
> > > + void updateControls(const IPACameraSensorInfo &sensorInfo,
> > > + const ControlInfoMap &sensorControls,
> > > + ControlInfoMap *ipaControls);
> > > void setControls(unsigned int frame);
> > >
> > > std::map<unsigned int, FrameBuffer> buffers_;
> > > @@ -114,6 +120,8 @@ std::string IPARkISP1::logPrefix() const
> > > }
> > >
> > > int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
> > > + const IPACameraSensorInfo &sensorInfo,
> > > + const ControlInfoMap &sensorControls,
> > > ControlInfoMap *ipaControls)
> > > {
> > > /* \todo Add support for other revisions */
> > > @@ -179,9 +187,8 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
> > > if (ret)
> > > return ret;
> > >
> > > - /* Return the controls handled by the IPA. */
> > > - ControlInfoMap::Map ctrlMap = rkisp1Controls;
> > > - *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);
> > > + /* Initialize controls. */
> > > + updateControls(sensorInfo, sensorControls, ipaControls);
> > >
> > > return 0;
> > > }
> > > @@ -199,7 +206,8 @@ void IPARkISP1::stop()
> > > }
> > >
> > > int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,
> > > - [[maybe_unused]] const std::map<uint32_t, IPAStream> &streamConfig)
> > > + [[maybe_unused]] const std::map<uint32_t, IPAStream> &streamConfig,
> > > + ControlInfoMap *ipaControls)
> > > {
> > > sensorControls_ = ipaConfig.sensorControls;
> > >
> > > @@ -229,6 +237,9 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,
> > > context_.configuration.sensor.size = info.outputSize;
> > > context_.configuration.sensor.lineDuration = info.minLineLength * 1.0s / info.pixelRate;
> > >
> > > + /* Update the camera controls using the new sensor settings. */
> > > + updateControls(info, sensorControls_, ipaControls);
> > > +
> > > /*
> > > * When the AGC computes the new exposure values for a frame, it needs
> > > * to know the limits for shutter speed and analogue gain.
> > > @@ -329,6 +340,42 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId
> > > metadataReady.emit(frame, metadata);
> > > }
> > >
> > > +void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,
> > > + const ControlInfoMap &sensorControls,
> > > + ControlInfoMap *ipaControls)
> > > +{
> > > + ControlInfoMap::Map ctrlMap = rkisp1Controls;
> > > +
> > > + /*
> > > + * Compute the frame duration limits.
> > > + *
> > > + * The frame length is computed assuming a fixed line length combined
> > > + * with the vertical frame sizes.
> > > + */
> >
> > Does the CameraSensor class have enough information to do all this in a
> > common location?
>
> I presume so... Actually as this depends on the current configuration
> CameraSensorInfo seems an ideal place..
>
> However, it can safely be don on top, after we have aligned both
> implementations ?
>
> >
> > Yakety Yak - but it seems like it might be nicer to have this code in
> > CameraSensor class, and used by both IPU3 and RKISP1... (and therefore
> > any other platform using CameraSensor class).
> >
> > Although - perhaps it's reliant upon the desired configuration too - but
> > maybe it's still something that could live in libipa ?
> >
> > ControlInfo limits = libipa::frameDurationLimits(sensorControls,
> > sensorInfo.outputSise);
> >
> > ?
> >
> > --
> > Kieran
> >
> >
> > > + const ControlInfo &v4l2HBlank = sensorControls.find(V4L2_CID_HBLANK)->second;
> > > + uint32_t hblank = v4l2HBlank.def().get<int32_t>();
> > > + uint32_t lineLength = sensorInfo.outputSize.width + hblank;
> > > +
> > > + const ControlInfo &v4l2VBlank = sensorControls.find(V4L2_CID_VBLANK)->second;
> > > + std::array<uint32_t, 3> frameHeights{
> > > + v4l2VBlank.min().get<int32_t>() + sensorInfo.outputSize.height,
> > > + v4l2VBlank.max().get<int32_t>() + sensorInfo.outputSize.height,
> > > + v4l2VBlank.def().get<int32_t>() + sensorInfo.outputSize.height,
> > > + };
> > > +
> > > + std::array<int64_t, 3> frameDurations;
> > > + for (unsigned int i = 0; i < frameHeights.size(); ++i) {
> > > + uint64_t frameSize = lineLength * frameHeights[i];
> > > + frameDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);
> > > + }
> > > +
> > > + ctrlMap[&controls::FrameDurationLimits] = ControlInfo(frameDurations[0],
> > > + frameDurations[1],
> > > + frameDurations[2]);
> >
> > Or
> > ctrlMap[&controls::FrameDurationLimits] =
> > libipa::frameDurationLimits(sensorControls,
> > sensorInfo.outputSize);
> >
> > directly even?
> >
> > > +
> > > + *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);
> > > +}
> > > +
> > > void IPARkISP1::setControls(unsigned int frame)
> > > {
> > > /*
> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > index 023d1fd4744f..8fbeed8ca800 100644
> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > @@ -347,8 +347,13 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)
> > > ipaTuningFile = std::string(configFromEnv);
> > > }
> > >
> > > - int ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,
> > > - &controlInfo_);
> > > + IPACameraSensorInfo sensorInfo{};
> > > + int ret = sensor_->sensorInfo(&sensorInfo);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,
> > > + sensorInfo, sensor_->controls(), &controlInfo_);
> > > if (ret < 0) {
> > > LOG(RkISP1, Error) << "IPA initialization failure";
> > > return ret;
> > > @@ -724,7 +729,7 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
> > >
> > > ipaConfig.sensorControls = data->sensor_->controls();
> > >
> > > - ret = data->ipa_->configure(ipaConfig, streamConfig);
> > > + ret = data->ipa_->configure(ipaConfig, streamConfig, &data->controlInfo_);
> > > if (ret) {
> > > LOG(RkISP1, Error) << "failed configuring IPA (" << ret << ")";
> > > return ret;
> > > --
> > > 2.38.1
> > >
More information about the libcamera-devel
mailing list