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

12194 diff mbox series

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@raspberrypi.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Reviewed-by: Kieran Bingham <kieran.bingham@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(-)

Hi

On Wed, 5 May 2021 at 14:53, David Plowman
<david.plowman@raspberrypi.com> 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@raspberrypi.com>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> Reviewed-by: Kieran Bingham <kieran.bingham@raspberrypi.com>

Oops, typo here! Sorry Kieran. Can that be fixed up while applying please?

Thanks
David

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

On 05/05/2021 14:54, David Plowman wrote:
> Hi
> 
> On Wed, 5 May 2021 at 14:53, David Plowman
> <david.plowman@raspberrypi.com> 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@raspberrypi.com>
>> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>> Reviewed-by: Kieran Bingham <kieran.bingham@raspberrypi.com>
> 
> Oops, typo here! Sorry Kieran. Can that be fixed up while applying please?

That depends, When will I receive my salary from RPi ;-)

(Otherwise, yes I think so :D)
--
Kieran

> 
> Thanks
> David
> 
>> ---
>>  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
>> + * 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
>> --
>> 2.20.1
>>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>

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@raspberrypi.com>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> Reviewed-by: Kieran Bingham <kieran.bingham@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@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