[libcamera-devel] [PATCH v3 1/2] libcamera: camera_sensor: Fix frame lengths calculated by sensorInfo()

[Laurent Pinchart]

Hi David,

Thank you for the patch.

On Wed, May 05, 2021 at 02:53:07PM +0100, David Plowman wrote:
> The minimum and maximum vblanking can change when a new format is
> applied to the sensor subdevice, so be sure to retrieve up-to-date
> values.
> 
> The V4L2Device acquires the new updateControlInfo() method to perform
> this function, and which the CameraSensor calls automatically if its
> setFormat method is used to update the sensor.
> 
> However, not all pipeline handlers invoke the setFormat method
> directly, so the new method must be made publicly available for
> pipeline handlers to call if they need to.
> 
> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> Reviewed-by: Kieran Bingham <kieran.bingham at raspberrypi.com>
> ---
>  include/libcamera/internal/camera_sensor.h |  2 ++
>  include/libcamera/internal/v4l2_device.h   |  2 ++
>  src/libcamera/camera_sensor.cpp            | 32 ++++++++++++++++++-
>  src/libcamera/v4l2_device.cpp              | 37 ++++++++++++++++++++++
>  4 files changed, 72 insertions(+), 1 deletion(-)
> 
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index 3e98f71b..6ce513b7 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -68,6 +68,8 @@ public:
>  	const ControlList &properties() const { return properties_; }
>  	int sensorInfo(CameraSensorInfo *info) const;
>  
> +	void updateControlInfo();
> +
>  protected:
>  	std::string logPrefix() const override;
>  
> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h
> index 087f07e7..5ba9b23b 100644
> --- a/include/libcamera/internal/v4l2_device.h
> +++ b/include/libcamera/internal/v4l2_device.h
> @@ -42,6 +42,8 @@ public:
>  	int setFrameStartEnabled(bool enable);
>  	Signal<uint32_t> frameStart;
>  
> +	void updateControlInfo();
> +
>  protected:
>  	V4L2Device(const std::string &deviceNode);
>  	~V4L2Device();
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 2887bb69..322678c8 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -656,15 +656,30 @@ V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector<unsigned int> &mbu
>  /**
>   * \brief Set the sensor output format
>   * \param[in] format The desired sensor output format
> + *
> + * The ranges of any controls associated with the sensor are also updated.
> + *
>   * \return 0 on success or a negative error code otherwise
>   */
>  int CameraSensor::setFormat(V4L2SubdeviceFormat *format)
>  {
> -	return subdev_->setFormat(pad_, format);
> +	int ret = subdev_->setFormat(pad_, format);
> +	if (ret)
> +		return ret;
> +
> +	updateControlInfo();
> +	return 0;
>  }
>  
>  /**
>   * \brief Retrieve the supported V4L2 controls and their information
> + *
> + * Control information is updated automatically to reflect the current sensor
> + * configuration when the setFormat() function is called, without invalidating
> + * any iterator on the ControlInfoMap. A manual update can also be forced by
> + * calling the updateControlInfo() function for pipeline handlers that change
> + * the sensor configuration wihtout using setFormat().
> + *
>   * \return A map of the V4L2 controls supported by the sensor
>   */
>  const ControlInfoMap &CameraSensor::controls() const
> @@ -750,6 +765,11 @@ int CameraSensor::setControls(ControlList *ctrls)
>   * Sensor information is only available for raw sensors. When called for a YUV
>   * sensor, this function returns -EINVAL.
>   *
> + * Pipeline handlers that do not change the sensor format using the
> + * CameraSensor::setFormat method may need to call
> + * CameraSensor::updateControlInfo beforehand, to ensure all the control

You can write setFormat() and updateControlInfo(), no need for a class
name qualifier when the documentation refers to functions in the same
class. No need to resend, I'll update this when applying.

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

> + * ranges are up to date.
> + *
>   * \return 0 on success, a negative error code otherwise
>   */
>  int CameraSensor::sensorInfo(CameraSensorInfo *info) const
> @@ -821,6 +841,16 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const
>  	return 0;
>  }
>  
> +/**
> + * \fn void CameraSensor::updateControlInfo()
> + * \brief Update the sensor's ControlInfoMap in case they have changed
> + * \sa V4L2Device::updateControlInfo()
> + */
> +void CameraSensor::updateControlInfo()
> +{
> +	subdev_->updateControlInfo();
> +}
> +
>  std::string CameraSensor::logPrefix() const
>  {
>  	return "'" + entity_->name() + "'";
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index 397029ac..515cbdfe 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -511,6 +511,43 @@ void V4L2Device::listControls()
>  	controls_ = std::move(ctrls);
>  }
>  
> +/**
> +* \brief Update the information for all device controls
> + *
> + * The V4L2Device class caches information about all controls supported by the
> + * device and exposes it through the controls() and controlInfo() functions.
> + * Control information may change at runtime, for instance when formats on a
> + * subdev are modified. When this occurs, this function can be used to refresh
> + * control information. The information is refreshed in-place, all pointers to
> + * v4l2_query_ext_ctrl instances previously returned by controlInfo() and
> + * iterators to the ControlInfoMap returned by controls() remain valid.
> + *
> + * Note that control information isn't refreshed automatically is it may be an
> + * expensive operation. The V4L2Device users are responsible for calling this
> + * function when required, based on their usage pattern of the class.
> + */
> +void V4L2Device::updateControlInfo()
> +{
> +	for (auto &[controlId, info] : controls_) {
> +		unsigned int id = controlId->id();
> +
> +		/*
> +		 * Assume controlInfo_ has a corresponding entry, as it has been
> +		 * generated by listControls().
> +		 */
> +		struct v4l2_query_ext_ctrl &ctrl = controlInfo_[id];
> +
> +		if (ioctl(VIDIOC_QUERY_EXT_CTRL, &ctrl)) {
> +			LOG(V4L2, Debug)
> +				<< "Could not refresh control "
> +				<< utils::hex(id);
> +			continue;
> +		}
> +
> +		info = V4L2ControlInfo(ctrl);
> +	}
> +}
> +
>  /*
>   * \brief Update the value of the first \a count V4L2 controls in \a ctrls using
>   * values in \a v4l2Ctrls

-- 
Regards,

Laurent Pinchart