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

[Jacopo Mondi]

Hello Jean-Michel,

On Thu, Jan 21, 2021 at 03:17:18PM +0100, Jean-Michel Hautbois wrote:
> Hi Jacopo,
>
> On 19/01/2021 15:37, 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.
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 66 ++++++++++++++++++++++++++--
> >  1 file changed, 62 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 73304ea73050..f928af4d92a2 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_;
> > +
> > +	uint32_t exposureTime_;
> >  };
> >
> >  class IPU3CameraConfiguration : public CameraConfiguration
> > @@ -119,6 +121,7 @@ private:
> >  			PipelineHandler::cameraData(camera));
> >  	}
> >
> > +	int initControls(IPU3CameraData *data);
> >  	int registerCameras();
> >
> >  	int allocateBuffers(Camera *camera);
> > @@ -730,6 +733,58 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)
> >  	return ret == 0;
> >  }
> >
> > +/**
> > + * \brief Initialize the camera controls
> > + * \param[in] data The camera data
> > + *
> > + * Initialize the camera controls as the union of the static pipeline handler
> > + * controls (IPU3Controls) and controls created dynamically from the sensor
> > + * capabilities.
> > + *
> > + * \return 0 on success or a negative error code otherwise
> > + */
> > +int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
>
> Probably more a cosmetic and "can wait" thing: initControls is used for
> exposure only...
> If there is more controls in the future, shouldn't we create a
> initExposureControl and call it from initControls (which will ease its
> reading if more are added) ?
>

Possibly. This series adds already one more control (ScalerCrop) and
more will come.

So far this didn't bother me for being too long as a function, but we
can consider splitting already indeed.

> > +{
> > +	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 length and pixel rate of the
> > +	 * current configuration converted to microseconds. Use the
> > +	 * V4L2_CID_EXPOSURE control to get exposure min, max and default and
> > +	 * convert it from lines to microseconds.
> > +	 */
> > +	double lineDuration = sensorInfo.lineLength
> > +			    / (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>() * lineDuration;
> > +	int32_t maxExposure = v4l2Exposure.max().get<int32_t>() * lineDuration;
> > +	int32_t defExposure = v4l2Exposure.def().get<int32_t>() * lineDuration;
> > +
> > +	/*
> > +	 * \todo Report the actual exposure time, use the default for the
> > +	 * moment.
> > +	 */
> > +	data->exposureTime_ = defExposure;
> > +
> > +	controls[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure,
> > +							defExposure);
>
> Basically, we are transforming an undefined exposure value into an
> exposureTime value in a new ControlInfo... ?

Why 'undefined' ? The thing here is that all these controls are
dependent on the sensor configuration, so establishing a good initial
value can only be done by implementing a policy. Here I've chose to
use the current configuration, but there might be better options.

Or are you referring to the todo note for data->exposureTime_ ?

> Meaning the camera_sensor could be able to use it to convert from time
> to exposure maybe ?

It could indeed. The thing is that we haven't yet established an
interface for the controls the CameraSensor should expose.

Right now the class reports V4L2 controls, and operates on the same
set of controls in set/getControls(). Also, sensor controls are transported
to the IPA as V4L2 controls, not libcamera and this has to be taken into
account.

Adding an interface to report libcamera Control[Info] created by
inspecting V4L2 control limits might be an option, but IPU3 is the
sole user of that interface so we might need a few more data point to
actually see what interface is better suited.

In brief, yes, the initialization of the ControlInfoMap accessible by
applications through libcamera::Camera::controls() could be
centralized or moved to helpers, but a few more things have to be
straighten up first to make the right (or the less wrong, if you
prefer) call.

> I am wondering if it could not be used to generalize the CamHelper from
> raspberrypi to a more general cam_helper class used through the pipeline ?
> Not sure my question is crystal clear :-p.

The CamHelper functionalities should be generalized, I agree. The part
that serves to report static sensor information (ie the controls'
delay) will probably be moved to a CameraSensorDatabase which has been
on the list already. Afaict CamHelper won't help with controls
initialization, but in general yes, we should aim to generalize those
components.

Thanks
  j

>
> > +	data->controlInfo_ = std::move(controls);
> > +
> > +	return 0;
> > +}
> > +
> >  /**
> >   * \brief Initialise ImgU and CIO2 devices associated with cameras
> >   *
> > @@ -775,8 +830,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
> > @@ -841,6 +897,8 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)
> >
> >  	/* Mark the request as complete. */
> >  	request->metadata().set(controls::draft::PipelineDepth, 3);
> > +	/* \todo Move the ExposureTime control to the IPA. */
> > +	request->metadata().set(controls::ExposureTime, exposureTime_);
> >  	pipe_->completeRequest(request);
> >  }
> >
> >