[libcamera-devel] [PATCH 03/12] libcamera: ipu3: Register Exposure control

[Jacopo Mondi]

Hi Niklas,

On Mon, Jan 18, 2021 at 04:16:19PM +0100, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your work.
>
> On 2021-01-05 20:05:13 +0100, Jacopo Mondi wrote:
> > Calculate the controls::Exposure limits at camera registration time
> > and register it in the list of camera supported controls.
> >
> > Cache the default exposure value to report it in the request metadata.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 67 ++++++++++++++++++++++++++--
> >  1 file changed, 63 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index f1151733d9fe..879057dab328 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -41,7 +41,7 @@ static constexpr unsigned int IMGU_OUTPUT_HEIGHT_ALIGN = 4;
> >  static constexpr unsigned int IMGU_OUTPUT_WIDTH_MARGIN = 64;
> >  static constexpr unsigned int IMGU_OUTPUT_HEIGHT_MARGIN = 32;
> >
> > -static const ControlInfoMap IPU3Controls = {
> > +static const ControlInfoMap::Map IPU3Controls = {
> >  	{ &controls::draft::PipelineDepth, ControlInfo(2, 3) },
> >  };
> >
> > @@ -49,7 +49,7 @@ class IPU3CameraData : public CameraData
> >  {
> >  public:
> >  	IPU3CameraData(PipelineHandler *pipe)
> > -		: CameraData(pipe)
> > +		: CameraData(pipe), exposureTime_(0)
> >  	{
> >  	}
> >
> > @@ -62,6 +62,8 @@ public:
> >  	Stream outStream_;
> >  	Stream vfStream_;
> >  	Stream rawStream_;
> > +
> > +	int32_t exposureTime_;
> >  };
> >
> >  class IPU3CameraConfiguration : public CameraConfiguration
> > @@ -119,6 +121,7 @@ private:
> >  			PipelineHandler::cameraData(camera));
> >  	}
> >
> > +	int initControls(IPU3CameraData *data);
> >  	int registerCameras();
> >
> >  	int allocateBuffers(Camera *camera);
> > @@ -731,6 +734,60 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)
> >  	return ret == 0;
> >  }
> >
> > +/*
> > + * \brief Initialize the camera controls
> > + * \param[in] data The camera data
> > + *
> > + * Initialize the camera controls by registering the pipeline handler
> > + * ones along with the controls assembled by inspecting the sensor
> > + * capabilities.
> > + *
> > + * \return 0 on success or a negative error code for error
> > + */
> > +int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
> > +{
> > +	const CameraSensor *sensor = data->cio2_.sensor();
> > +	CameraSensorInfo sensorInfo{};
> > +
> > +	int ret = sensor->sensorInfo(&sensorInfo);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ControlInfoMap::Map controls = IPU3Controls;
> > +
> > +	/*
> > +	 * Compute exposure time limits.
> > +	 *
> > +	 * \todo The exposure limits depend on the sensor configuration.
> > +	 * Initialize the control using the line lenght and pixel rate of the
> > +	 * current configurtion, as reported by the CameraSensorInfo. Use the
> > +	 * V4L2_CID_EXPOSURE control to get exposure min and max and convert it
> > +	 * from lines into micro-seconds.
> > +	 */
> > +	float pixelRate = sensorInfo.pixelRate / 1e6f;
> > +	const ControlInfoMap &sensorControls = sensor->controls();
> > +	const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;
> > +	int32_t minExposure = v4l2Exposure.min().get<int32_t>()
> > +			    * sensorInfo.lineLength / pixelRate;
> > +	int32_t maxExposure = v4l2Exposure.max().get<int32_t>()
> > +			    * sensorInfo.lineLength / pixelRate;
> > +	int32_t defExposure = v4l2Exposure.def().get<int32_t>()
> > +			    * sensorInfo.lineLength / pixelRate;
> > +
> > +	/*
> > +	 * \todo Report the actual exposure time, use the default for the
> > +	 * moment.
> > +	 */
> > +	data->exposureTime_ = defExposure;
> > +
> > +	controls[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure,
> > +							defExposure);
>
> nit: This is the only usage of of the local 'controls' variable, would
> it make sens to remove it and use 'data->controlInfo_' directly and
> avoid the std::move() below? While reading the code I thought there more
> things going on here then an advance optimization ;-)

If I'm not mistaken that's actually how the ControlInfoMap API
requires an instance to be intialized, to trigger the generation of
the numerical ids map and support access by numerical index.

>
> With or without this fix, but with the issues Laurent points out fixed,
>
> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
>

Thanks
  j

> > +
> > +	data->controlInfo_ = std::move(controls);
> > +
> > +	return 0;
> > +}
> > +
> >  /**
> >   * \brief Initialise ImgU and CIO2 devices associated with cameras
> >   *
> > @@ -776,8 +833,9 @@ int PipelineHandlerIPU3::registerCameras()
> >  		/* Initialize the camera properties. */
> >  		data->properties_ = cio2->sensor()->properties();
> >
> > -		/* Initialze the camera controls. */
> > -		data->controlInfo_ = IPU3Controls;
> > +		ret = initControls(data.get());
> > +		if (ret)
> > +			continue;
> >
> >  		/**
> >  		 * \todo Dynamically assign ImgU and output devices to each
> > @@ -842,6 +900,7 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)
> >
> >  	/* Mark the request as complete. */
> >  	request->metadata().set(controls::draft::PipelineDepth, 3);
> > +	request->metadata().set(controls::ExposureTime, exposureTime_);
> >  	pipe_->completeRequest(request);
> >  }
> >
> > --
> > 2.29.2
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund