<div dir="ltr"><div><div dir="ltr" class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div dir="ltr"><div style="font-size:12.8px"><div style="color:rgb(136,136,136);font-size:12.8px"><div style="font-size:small"><font face="arial, helvetica, sans-serif" color="#000000">Thanks-- I saw that. I appreciate it.</font></div></div></div></div></div></div></div><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Nov 4, 2022 at 5:54 AM Jacopo Mondi <<a href="mailto:jacopo@jmondi.org">jacopo@jmondi.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hello<br>
<br>
On Mon, Oct 31, 2022 at 12:42:33PM +0100, Jacopo Mondi via libcamera-devel wrote:<br>
> Hi Nicholas<br>
><br>
> On Sun, Oct 30, 2022 at 06:04:57PM -0500, Nicholas Roth via libcamera-devel wrote:<br>
<br>
[snip]<br>
<br>
><br>
> To make it more messy, we have this validation in CameraSensor class<br>
><br>
> static constexpr uint32_t mandatoryControls[] = {<br>
> V4L2_CID_EXPOSURE,<br>
> V4L2_CID_HBLANK,<br>
> V4L2_CID_PIXEL_RATE,<br>
> V4L2_CID_VBLANK,<br>
> };<br>
><br>
> The optimal path would be:<br>
> 1) Decide how much to validate on the pipeline handler side vs IPA<br>
> side. If have to validate on IPA side for $reasons:<br>
> 2) Move validateSensorControls() to IPU3 init()<br>
> 3) Copy validateSensorControls in RkISP1<br>
> 4) Call validateSensorControls() in RkISP1::init()<br>
> 5) Drop custom validation code from RkISP1::configure()<br>
><br>
> As this is more work for you, and we need to finally make a call on<br>
> where sensor control validation should happen, I would be fine merging<br>
> this as it is or, if you feel particularly motived, with a check<br>
> for HBLANK/VBLANK before calling updateControls() in init() as a<br>
> safety measure, with a pinky promise from our side we fix this mess on<br>
> both platforms as soon as these patches go in. (not something I'm<br>
> generally confortable with as accepting technical debt with the<br>
> promise that we'll eventually fix it is usually a recipe for disaster,<br>
> but as we're keeping a close eye on the series and we want to progress<br>
> it as soon as possible, I think we can take the risk here)<br>
<br>
I've taken this patch in the "[PATCH 0/4] ipa: Validate controls in<br>
CameraSensor" series to hopefully shorten the list of pending items in<br>
this series.<br>
<br>
Thanks<br>
j<br>
><br>
><br>
> > + uint32_t lineLength = sensorInfo.outputSize.width + hblank;<br>
> > +<br>
> > + const ControlInfo &v4l2VBlank = sensorControls.find(V4L2_CID_VBLANK)->second;<br>
> > + std::array<uint32_t, 3> frameHeights{<br>
> > + v4l2VBlank.min().get<int32_t>() + sensorInfo.outputSize.height,<br>
> > + v4l2VBlank.max().get<int32_t>() + sensorInfo.outputSize.height,<br>
> > + v4l2VBlank.def().get<int32_t>() + sensorInfo.outputSize.height,<br>
> > + };<br>
> > +<br>
> > + std::array<int64_t, 3> frameDurations;<br>
> > + for (unsigned int i = 0; i < frameHeights.size(); ++i) {<br>
> > + uint64_t frameSize = lineLength * frameHeights[i];<br>
> > + frameDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);<br>
> > + }<br>
> > +<br>
> > + ctrlMap[&controls::FrameDurationLimits] = ControlInfo(frameDurations[0],<br>
> > + frameDurations[1],<br>
> > + frameDurations[2]);<br>
> > +<br>
> > + *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);<br>
> > +}<br>
> > +<br>
> > void IPARkISP1::setControls(unsigned int frame)<br>
> > {<br>
> > /*<br>
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp<br>
> > index 455ee2a0..dae29a2c 100644<br>
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp<br>
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp<br>
> > @@ -340,15 +340,19 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)<br>
> > /*<br>
> > * If the tuning file isn't found, fall back to the<br>
> > * 'uncalibrated' configuration file.<br>
> > - */<br>
> > + */<br>
> > if (ipaTuningFile.empty())<br>
> > ipaTuningFile = ipa_->configurationFile("uncalibrated.yaml");<br>
> > } else {<br>
> > ipaTuningFile = std::string(configFromEnv);<br>
> > }<br>
> ><br>
> > - int ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,<br>
> > - &controlInfo_);<br>
> > + IPACameraSensorInfo sensorInfo{};<br>
> > + int ret = sensor_->sensorInfo(&sensorInfo);<br>
> > + if (ret)<br>
> > + return ret;<br>
> > + ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,<br>
> > + sensorInfo, sensor_->controls(), &controlInfo_);<br>
> > if (ret < 0) {<br>
> > LOG(RkISP1, Error) << "IPA initialization failure";<br>
> > return ret;<br>
> > @@ -725,7 +729,7 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)<br>
> > std::map<uint32_t, ControlInfoMap> entityControls;<br>
> > entityControls.emplace(0, data->sensor_->controls());<br>
> ><br>
> > - ret = data->ipa_->configure(sensorInfo, streamConfig, entityControls);<br>
> > + ret = data->ipa_->configure(sensorInfo, streamConfig, entityControls, &data->controlInfo_);<br>
> > if (ret) {<br>
> > LOG(RkISP1, Error) << "failed configuring IPA (" << ret << ")";<br>
> > return ret;<br>
> > --<br>
> > 2.34.1<br>
> ><br>
</blockquote></div>