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

[Laurent Pinchart]

Hi Niklas and Jacopo,

On Tue, Apr 28, 2020 at 02:28:39AM +0200, Niklas Söderlund wrote:
> On 2020-04-27 23:32:35 +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       | 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

I would s/ that reports.*$// as it's redundant I think, up to you.

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

I've copied the code for that function from this patch :-) I think we
should then replace this by just a call to model(). Let's discuss your
comments as part of the other patch.

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

I agree with Niklas. I think you can just write

	/* Get the active area size. */

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

I don't think an abort() is required here, returning an error should
allow us to handle that gracefully and, for instance, display a message
to the user in the GUI instead of killing the process.

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

s/bit-depth/bit depth/

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

If any of the controls is unsupported, getControls() returns an empty
list. I'm fine with != 2 too though, up to Jacopo.

Assuming we call model() above and thus don't need to discuss how the
model is retrieved as part of this patch,

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

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

-- 
Regards,

Laurent Pinchart