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

[Laurent Pinchart]

Hi Jacopo,

On Sun, Apr 26, 2020 at 12:03:58AM +0300, Laurent Pinchart wrote:
> 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;

Another comment, to improve efficiency, would it make sense to read the
two controls in one go ? I've just sent a small series that simplifies
the API of V4L2Device::getControls(), and you can then write

	ControlList ctrls = subdev_->getControls({ V4L2_CID_PIXEL_RATE,
						   V4L2_CID_HBLANK });
	if (ctrls.empty()) {
		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->pixelClock = ctrls.get(V4L2_CID_PIXEL_RATE).get<int64_t>();

Do you think the API would be good enough ?

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