[libcamera-devel] [PATCH v3 11/13] libcamera: camera_sensor: Add method to get sensor info

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Fri, Apr 24, 2020 at 11:53:02PM +0200, Jacopo Mondi wrote:
> Add method to retrieve the CameraSensorInfo to the CameraSensor class.

s/method/a method/

> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/camera_sensor.cpp       | 84 +++++++++++++++++++++++++++
>  src/libcamera/include/camera_sensor.h |  1 +
>  2 files changed, 85 insertions(+)
> 
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index e39f277622ae..c1ef4adb579c 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -492,6 +492,90 @@ int CameraSensor::setControls(ControlList *ctrls)
>   * \return The list of camera sensor properties
>   */
>  
> +/**
> + * \brief Assemble and return the camera sensor info
> + * \param[out] info The camera sensor info that report the sensor information

s/report/reports/

> + *
> + * 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
> +{
> +	/*
> +	 * Make sure the sub-device exports all the controls we need to operate.
> +	 *
> +	 * Currently V4L2_CID_PIXEL_RATE and V4L2_CID_HBLANK are required.
> +	 *
> +	 * \todo This is an hard policy that all sensors that want to export
> +	 * a properly populated CameraSensorInfo have to adhere to. Consider if
> +	 * it's worth relaxing it and providing default values instead.

I like such hard policies :-) I don't see a way to provide a sane
default, so I think this is fine. It's about time to put a bit of order
in the V4L2 sensor drivers anyway.

> +	 */
> +	const ControlInfoMap &controlMap = subdev_->controls();
> +	if (controlMap.find(V4L2_CID_PIXEL_RATE) == controlMap.end()) {
> +		LOG(CameraSensor, Error) << "'Pixel rate' control not available";

You can drop the quotes here and below. I would also either write
PIXEL_RATE here, or Horizontal blanking below.

> +		return -EINVAL;
> +	}
> +
> +	if (controlMap.find(V4L2_CID_HBLANK) == controlMap.end()) {
> +		LOG(CameraSensor, Error) << "'HBLANK' control not available";
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * 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->name = *utils::split(entityName, " ").begin();

	info->name = entityName.substr(0, entityName.find(' '));

will be more efficient (and would avoid me the need to check if the
temporary object returned by utils::split() will last until the
assignement operator is executed).

> +
> +	/*
> +	 * Get the active area size from the ActiveAreas property.
> +	 *
> +	 * \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.
> +	 */
> +	Span<const int> activeArea = properties_.get(properties::ActiveAreas);
> +	info->activeAreaSize = { static_cast<unsigned int>(activeArea[2]),
> +				 static_cast<unsigned int>(activeArea[3]) };
> +
> +	/* The bit-depth and image size depend on the currently applied format. */
> +	V4L2SubdeviceFormat format{};
> +	int ret = subdev_->getFormat(0, &format);
> +	if (ret)
> +		return ret;

It doesn't have to be done as part of this series, but do you think it
would make sense to cache the format in the CameraSensor class ? We'll
likely have to rework the CameraSensor class when supporting sensors
that expose multiple subdevs, so we can rework this at that time.

> +	info->bitsPerPixel = format.bitsPerPixel();
> +	info->outputSize = format.size;
> +
> +	/* It's mandatory for the subdevice to report its crop rectangle. */
> +	ret = subdev_->getCropRectangle(0, &info->analogCrop);
> +	if (ret) {
> +		LOG(CameraSensor, Error)
> +			<< "Failed to construct camera sensor info: "
> +			<< "the camera sensor does not report the crop rectangle";
> +		return ret;
> +	}
> +
> +	int64_t pixelRate;
> +	subdev_->getControl(V4L2_CID_PIXEL_RATE, &pixelRate);

Should we check for errors here ? The control is guaranteed to exist,
but other errors could occur. And if we add an error check here, is it
worth it checking if the controls exist at the top ?

> +	info->pixelClock = pixelRate;

Should the pixelClock field already be extended to 64 bits ?

> +
> +	int32_t hblank;
> +	subdev_->getControl(V4L2_CID_HBLANK, &hblank);

Same here, error checking is needed.

> +	info->lineLength = info->outputSize.width + hblank;
> +
> +	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 e19852d4cabe..b162c3886b1d 100644
> --- a/src/libcamera/include/camera_sensor.h
> +++ b/src/libcamera/include/camera_sensor.h
> @@ -61,6 +61,7 @@ public:
>  	int setControls(ControlList *ctrls);
>  
>  	const ControlList &properties() const { return properties_; }
> +	int sensorInfo(CameraSensorInfo *info) const;
>  
>  protected:
>  	std::string logPrefix() const;

-- 
Regards,

Laurent Pinchart