[libcamera-devel] [PATCH v6 2/5] ipa: rkisp1: add FrameDurationLimits control

[Nicholas Roth]

Thanks-- I saw that. I appreciate it.


On Fri, Nov 4, 2022 at 5:54 AM Jacopo Mondi <jacopo at jmondi.org> wrote:

> Hello
>
> On Mon, Oct 31, 2022 at 12:42:33PM +0100, Jacopo Mondi via libcamera-devel
> wrote:
> > Hi Nicholas
> >
> > On Sun, Oct 30, 2022 at 06:04:57PM -0500, Nicholas Roth via
> libcamera-devel wrote:
>
> [snip]
>
> >
> > To make it more messy, we have this validation in CameraSensor class
> >
> >       static constexpr uint32_t mandatoryControls[] = {
> >               V4L2_CID_EXPOSURE,
> >               V4L2_CID_HBLANK,
> >               V4L2_CID_PIXEL_RATE,
> >               V4L2_CID_VBLANK,
> >       };
> >
> > The optimal path would be:
> > 1) Decide how much to validate on the pipeline handler side vs IPA
> >    side. If have to validate on IPA side for $reasons:
> >    2) Move validateSensorControls() to IPU3 init()
> >    3) Copy validateSensorControls in RkISP1
> >    4) Call validateSensorControls() in RkISP1::init()
> >    5) Drop custom validation code from RkISP1::configure()
> >
> > As this is more work for you, and we need to finally make a call on
> > where sensor control validation should happen, I would be fine merging
> > this as it is or, if you feel particularly motived, with a check
> > for HBLANK/VBLANK before calling updateControls() in init() as a
> > safety measure, with a pinky promise from our side we fix this mess on
> > both platforms as soon as these patches go in. (not something I'm
> > generally confortable with as accepting technical debt with the
> > promise that we'll eventually fix it is usually a recipe for disaster,
> > but as we're keeping a close eye on the series and we want to progress
> > it as soon as possible, I think we can take the risk here)
>
> I've taken this patch in the "[PATCH 0/4] ipa: Validate controls in
> CameraSensor" series to hopefully shorten the list of pending items in
> this series.
>
> Thanks
>    j
> >
> >
> > > +   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]);
> > > +
> > > +   *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 455ee2a0..dae29a2c 100644
> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > @@ -340,15 +340,19 @@ int RkISP1CameraData::loadIPA(unsigned int
> hwRevision)
> > >             /*
> > >              * If the tuning file isn't found, fall back to the
> > >              * 'uncalibrated' configuration file.
> > > -            */
> > > +    */
> > >             if (ipaTuningFile.empty())
> > >                     ipaTuningFile =
> ipa_->configurationFile("uncalibrated.yaml");
> > >     } else {
> > >             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;
> > > @@ -725,7 +729,7 @@ int PipelineHandlerRkISP1::configure(Camera
> *camera, CameraConfiguration *c)
> > >     std::map<uint32_t, ControlInfoMap> entityControls;
> > >     entityControls.emplace(0, data->sensor_->controls());
> > >
> > > -   ret = data->ipa_->configure(sensorInfo, streamConfig,
> entityControls);
> > > +   ret = data->ipa_->configure(sensorInfo, streamConfig,
> entityControls, &data->controlInfo_);
> > >     if (ret) {
> > >             LOG(RkISP1, Error) << "failed configuring IPA (" << ret <<
> ")";
> > >             return ret;
> > > --
> > > 2.34.1
> > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20221105/1e77ce39/attachment.htm>