[libcamera-devel] [PATCH v3 5/8] libcamera: camera_sensor: Expose a DelayedControls interface

[Jacopo Mondi]

Hi Niklas,

On Mon, Nov 23, 2020 at 11:12:31PM +0100, Niklas Söderlund wrote:
> Expose the optional DelayedControls interface to pipeline handlers. If
> used by a pipeline generic delays are used. In the future the delay
> values should be fetched to match the specific sensor module, either
> from a new kernel interface or worst case a sensors database.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  include/libcamera/internal/camera_sensor.h |  5 ++++
>  src/libcamera/camera_sensor.cpp            | 31 ++++++++++++++++++++++
>  2 files changed, 36 insertions(+)
>
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index b9eba89f00fba882..6be1cfd49134c534 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -14,6 +14,7 @@
>  #include <libcamera/controls.h>
>  #include <libcamera/geometry.h>
>
> +#include "libcamera/internal/delayed_controls.h"
>  #include "libcamera/internal/formats.h"
>  #include "libcamera/internal/log.h"
>  #include "libcamera/internal/v4l2_subdevice.h"
> @@ -61,6 +62,8 @@ public:
>  	ControlList getControls(const std::vector<uint32_t> &ids);
>  	int setControls(ControlList *ctrls);
>
> +	DelayedControls *delayedContols();
> +
>  	const ControlList &properties() const { return properties_; }
>  	int sensorInfo(CameraSensorInfo *info) const;
>
> @@ -83,6 +86,8 @@ private:
>  	std::vector<Size> sizes_;
>
>  	ControlList properties_;
> +
> +	std::unique_ptr<DelayedControls> delayedControls_;
>  };
>
>  } /* namespace libcamera */
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 935de528c4963453..6c92c97f4cc2547a 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -493,6 +493,37 @@ int CameraSensor::setControls(ControlList *ctrls)
>  	return subdev_->setControls(ctrls);
>  }
>
> +/**
> + * \brief Get the sensors delayed controls interface
> + *
> + * Access the sensors delayed controls interface. This interface aids pipelines
> + * keeping tack of controls that needs time to take effect. The interface is
> + * optional to use and does not interact with setControls() and getControls()
> + * that operates directly on the sensor device.
> + *
> + * \sa DelayedControls
> + * \return The delayed controls interface
> + */
> +DelayedControls *CameraSensor::delayedContols()
> +{
> +	if (!delayedControls_) {
> +		/*
> +		 * \todo Read dealy values from the sensor itself or from a
> +		 * a sensor database. For now use generic values taken from
> +		 * the Raspberry Pi and listed as generic values.
> +		 */
> +		std::unordered_map<uint32_t, unsigned int> delays = {
> +			{ V4L2_CID_ANALOGUE_GAIN, 1 },
> +			{ V4L2_CID_EXPOSURE, 2 },
> +		};

I'm debated.

As I see this, the current interface will have to evolve, on top of
this series. I think pipeline handler will not have to get a
'DelayedControls' object and interface with it, but rather the
CameraSensor will use it internally for either:
- all controls marked as delayed in the sensor database
- controls initialized as delayed by the pipeline handler (which won't
  be sensor-agnostic anymore)

The current implementation aims to be a drop-in replacement for
StaggeredCtrl so I'm more than fine with the proposed enanchement
being performed on top. At the same time as you know this specific
bits will break ov5647 capture on RPi, and I don't think that's right.

As a possible plan I would propose:
- replace staggered ctrls with this series and allow pipeline handler
  to pass in delays to maintain 1-to-1 compatibility with the existing
- grow the sensor database and the CameraSensor interface to remove
  direct pipeline->DelayedControl interactions on top

Am I missing a valid use case for pipeline handler having to deal with
DelayedControls directly ?

> +
> +		delayedControls_ =
> +			std::make_unique<DelayedControls>(subdev_.get(), delays);
> +	}
> +
> +	return delayedControls_.get();
> +}
> +
>  /**
>   * \brief Assemble and return the camera sensor info
>   * \param[out] info The camera sensor info
> --
> 2.29.2
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel