[libcamera-devel] [PATCH v4 6/7] libcamera: camera_sensor: Add method to get sensor info

[Niklas Söderlund]

Hi Jacopo,

Thanks for your work.

On 2020-04-27 23:32:35 +0200, Jacopo Mondi wrote:
> Add method to retrieve the CameraSensorInfo to the CameraSensor class.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/camera_sensor.cpp       | 83 +++++++++++++++++++++++++++
>  src/libcamera/include/camera_sensor.h |  1 +
>  2 files changed, 84 insertions(+)
> 
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 4fd1ee84eb47..8b37f63dc04e 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -440,6 +440,89 @@ int CameraSensor::setControls(ControlList *ctrls)
>  	return subdev_->setControls(ctrls);
>  }
>  
> +/**
> + * \brief Assemble and return the camera sensor info
> + * \param[out] info The camera sensor info that reports the sensor information
> + *
> + * The CameraSensorInfo content is assembled by inspecting the currently
> + * applied sensor configuration and the sensor static properties.
> + *
> + * \return 0 on success, a negative error code otherwise
> + */
> +int CameraSensor::sensorInfo(CameraSensorInfo *info) const
> +{
> +	/*
> +	 * Construct the camera sensor name using the media entity name.
> +	 *
> +	 * \todo There is no standardized naming scheme for sensor entities
> +	 * in the Linux kernel at the moment. The most common naming scheme
> +	 * is the one obtained by associating the sensor name and its I2C
> +	 * device and bus addresses in the form of: "devname i2c-adapt:i2c-addr"
> +	 * Assume this is the standard naming scheme and parse the first part
> +	 * of the entity name to obtain "devname".
> +	 */
> +	std::string entityName = subdev_->entity()->name();
> +	info->model = entityName.substr(0, entityName.find(' '));

If we need to keep the model name I like the interface provided by 
CameraSensor::model() from '[PATCH] libcamera: camera_sensor: Add 
model() function'.

As stated in the patch mention above I have comments on the 
documentation and it's implementation.

> +
> +	/*
> +	 * Get the active area size from the ActiveAreas property. Do
> +	 * not use the content of properties::ActiveAreas but fetch it from the
> +	 * subdevice as the property registration is optional, but it's
> +	 * mandatory to construct the CameraSensorInfo.

I'm sorry but this paragraph confuses me.

    Get FOO from property. Do not use the content of properties::FOO.

> +	 *
> +	 * \todo The ActiveAreas property aims to describe multiple
> +	 * active area rectangles. At the moment only a single active
> +	 * area rectangle is exposed by the subdevice API. Use that single
> +	 * rectangle width and height to construct the ActiveAreaSize.
> +	 */
> +	Rectangle rect = {};
> +	int ret = subdev_->getSelection(0, V4L2_SEL_TGT_CROP_DEFAULT, &rect);
> +	if (ret) {
> +		LOG(CameraSensor, Error)

If the target is mandatory for libcamera shall this be a fatal error ?  
Here and bellow.

> +			<< "Failed to construct camera sensor info: "
> +			<< "the camera sensor does not report the active area";
> +
> +		return ret;
> +	}
> +	info->activeAreaSize = { rect.width, rect.height };
> +
> +	/* It's mandatory for the subdevice to report its crop rectangle. */
> +	ret = subdev_->getSelection(0, V4L2_SEL_TGT_CROP, &info->analogCrop);
> +	if (ret) {
> +		LOG(CameraSensor, Error)
> +			<< "Failed to construct camera sensor info: "
> +			<< "the camera sensor does not report the crop rectangle";
> +		return ret;
> +	}
> +
> +	/* The bit-depth and image size depend on the currently applied format. */
> +	V4L2SubdeviceFormat format{};
> +	ret = subdev_->getFormat(0, &format);
> +	if (ret)
> +		return ret;
> +	info->bitsPerPixel = format.bitsPerPixel();
> +	info->outputSize = format.size;
> +
> +	/*
> +	 * Retrieve the pixel rate and the line length through V4L2 controls.
> +	 * Support for the V4L2_CID_PIXEL_RATE and V4L2_CID_HBLANK controls is
> +	 * mandatory.
> +	 */
> +	ControlList ctrls = subdev_->getControls({ V4L2_CID_PIXEL_RATE,
> +						   V4L2_CID_HBLANK });
> +	if (ctrls.empty()) {

What if only one of the controls are implemented by the driver? Would it 
make more sens to check ctrls.size() != 2 ?

> +		LOG(CameraSensor, Error)
> +			<< "Failed to retrieve camera info controls";
> +		return -EINVAL;
> +	}
> +
> +	int32_t hblank = ctrls.get(V4L2_CID_HBLANK).get<int32_t>();
> +	info->lineLength = info->outputSize.width + hblank;
> +	info->pixelRate = ctrls.get(V4L2_CID_PIXEL_RATE).get<int64_t>();
> +
> +	return 0;
> +}
> +
>  std::string CameraSensor::logPrefix() const
>  {
>  	return "'" + subdev_->entity()->name() + "'";
> diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h
> index 28ebfaa30c22..91d270552d3f 100644
> --- a/src/libcamera/include/camera_sensor.h
> +++ b/src/libcamera/include/camera_sensor.h
> @@ -60,6 +60,7 @@ public:
>  	int setControls(ControlList *ctrls);
>  
>  	const ControlList &properties() const { return properties_; }
> +	int sensorInfo(CameraSensorInfo *info) const;
>  
>  protected:
>  	std::string logPrefix() const;
> -- 
> 2.26.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund