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

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Jan 05, 2021 at 08:05:13PM +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_;

This can't be negative, unsigned int would do.

>  };
>  
>  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.

Should this mention IPU3Controls ? Maybe the following ?

 * 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 for error

s/for error/otherwise/

> + */
> +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

s/lenght/length/

> +	 * current configurtion, as reported by the CameraSensorInfo. Use the

s/configurtion/configuration/

> +	 * V4L2_CID_EXPOSURE control to get exposure min and max and convert it
> +	 * from lines into micro-seconds.

s/into/to/
s/micro-seconds/microseconds/

> +	 */
> +	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;

For long exposure time this will overflow as the first two operands are
32-bit integers. How about the following ?

	double lineDuration = sensorInfo.pixelRate / 1e6f /pixelRate;
	...
	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);
> +
> +	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);

	/* \todo Move the ExposureTime control to the IPA */

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +	request->metadata().set(controls::ExposureTime, exposureTime_);
>  	pipe_->completeRequest(request);
>  }
>  

-- 
Regards,

Laurent Pinchart