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

[Jacopo Mondi]

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
> >