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

[Jacopo Mondi]

Hi David,

I admit I have not followed closely the recent discussions on this
topic, but this seems an acceptable solution to me, however I wonder
if it would not be worth taking it a little step forward (I know...)

The problem I see here is fundamentally one: we're missing a method
to update a (V4L2)ControlInfo. The method itself could be quite
trivial, a ControlInfo::update(ControlValue min, ...) and one
V4L2ControlInfo::update(v4l2_query_ext_ctrl ctrl).

The lack of a mechanism to update a control info has this
consequences:
1) V4L2 controls passed by the ph to the IPA could be stale after a
sensor format update
2) the Camera::controls_ might be stale after a Camera::configure()

If you introduce the above suggested update() (or whatever) methods in
(V4L2)ControlInfo, then they could be used as you're doing here below
to handle 1)

In order to handle 2) pipeline handlers that register controls would
have to get a freshened list of V4L2Controls from the CameraSensor (or
use a freshened CameraSensorInfo) to update the controls they register
as Camera::controls_, performing the opportune re-calculations (in
example the IPU3::initControls() function would need to be split in
calculateExposure(), calculateDurations() etc which will then be
called at configure() time to update Camera::controls_.

Do you think your proposed solution could work well with
ControlInfo::update() ?

A few minors on the patch

On Tue, Mar 30, 2021 at 05:57:43PM +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 refreshControls() method to perform
> this function. Note that not all pipeline handlers invoke the
> subdevice's setFormat method directly, so the new method must be made
> publicly available.

I would however call refreshControls() in CameraSensor::setFormat()

>
> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> ---
>  include/libcamera/internal/v4l2_device.h |  2 ++
>  src/libcamera/camera_sensor.cpp          |  7 +++++++
>  src/libcamera/v4l2_device.cpp            | 26 ++++++++++++++++++++++++
>  3 files changed, 35 insertions(+)
>
> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h
> index d006bf68..6efcb0db 100644
> --- a/include/libcamera/internal/v4l2_device.h
> +++ b/include/libcamera/internal/v4l2_device.h
> @@ -41,6 +41,8 @@ public:
>  	int setFrameStartEnabled(bool enable);
>  	Signal<uint32_t> frameStart;
>
> +	void refreshControls();
> +
>  protected:
>  	V4L2Device(const std::string &deviceNode);
>  	~V4L2Device();
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index f7ed91d9..9f99ce3e 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -796,6 +796,13 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const
>  	info->bitsPerPixel = format.bitsPerPixel();
>  	info->outputSize = format.size;
>
> +	/*
> +	 * Some controls, specifically the vblanking, may have different min
> +	 * or max values if the sensor format has been changed since it was
> +	 * opened, so be sure to update them first.
> +	 */
> +	subdev_->refreshControls();
> +

I feel a bit like it's up to the ph to be aware if controls need to be
refreshed or not. For 'regular' ph this would be transpared, calling
CameraSensor::setFormat() would refresh controls and it is guaranteed
that CameraSensorInfo is always up-to-date.

For ph like yours that use CameraSensor but do not set the format
directly on it, I think it's up to the ph to refresh controls after
you have successfully set a format on the Unicam::Image node (this
would require adding a refreshControls() method to CameraSensor
though, I'm not 100% sure it is worth it :/)

This would allow unnecessary refresh of controls, as keeping the state
in the V4L2Device class through a flag would require to tweak into the
subclasses' setFormat() methods, and it might get nasty, if even
doable in clean way.

All in all, I would be happy with this patch if it would provide a way
to update ControlInfo first, and then use it to update the
V4L2Controls during a refresh. Do you think it would be worth it ?

Thanks
   j

>  	/*
>  	 * Retrieve the pixel rate, line length and minimum/maximum frame
>  	 * duration through V4L2 controls. Support for the V4L2_CID_PIXEL_RATE,
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index decd19ef..6d396f89 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -514,6 +514,32 @@ void V4L2Device::listControls()
>  	controls_ = std::move(ctrls);
>  }
>
> +/*
> + * \brief Update the control information that was previously stored by
> + * listControls(). Some parts of this information, such as min and max
> + * values of some controls, are liable to change when a new format is set.
> + */
> +void V4L2Device::refreshControls()
> +{

And I would here add a flag to the class, something like needRefersh,
to be set to true in
> +	for (auto &controlId : controlIds_) {
> +		unsigned int id = controlId->id();
> +
> +		struct v4l2_query_ext_ctrl ctrl = {};
> +		ctrl.id = id;
> +
> +		if (ioctl(VIDIOC_QUERY_EXT_CTRL, &ctrl)) {
> +			LOG(V4L2, Debug)
> +				<< "Could not refresh control "
> +				<< utils::hex(ctrl.id);
> +			continue;
> +		}
> +
> +		/* Assume these are present - listControls() made them. */
> +		controlInfo_[id] = ctrl;
> +		controls_.at(controlId.get()) = 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 at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel