libcamera: delayed_controls: Make VBLANK priority by default
diff mbox series

Message ID 20251102-rkisp1-vblank-v1-1-d2d6458696aa@ideasonboard.com
State New
Headers show
Series
  • libcamera: delayed_controls: Make VBLANK priority by default
Related show

Commit Message

Jacopo Mondi Nov. 2, 2025, 7:19 p.m. UTC 
The DelayedControls class works around a limitation of the V4L2
controls API by assigning to controls that modify the limits of
other controls a 'priority' flag.

'Priority' controls are written to the device before others
to make sure the limits of dependent controls are correctly
updated.

A typical example of a priority control is VBLANK, whose value changes
the limits of the EXPOSURE control. This doesn't apply to a specific
hardware platform but to all V4L2 sensors.

It's currently up to users of delayed controls to apply to
each control the correct flag by populating a ControlParams instance.
However, this is error prone, and the RkISP1 pipeline handler
wrongly sets the VBLANK priority to false.

Instead of simply fixing the RkISP1 implementation, change how the
priorty flag is set, by making DelayedControls automatically flag
VBLANK as priority.

This limits what controls the pipelines can set as priority, but so
far only VBLANK needs that flag to be set.

Split the ControlParams type into an external one that only associates
delays to control ids and an internal one that is populated at class
creation time.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
Tested on rkisp1:
delayed_controls.cpp:100 Set a delay of 2 and priority write flag 1 for Vertical Blanking
---
 include/libcamera/internal/delayed_controls.h | 14 ++++++---
 src/libcamera/delayed_controls.cpp            | 41 +++++++++++++--------------
 src/libcamera/pipeline/ipu3/ipu3.cpp          |  6 ++--
 src/libcamera/pipeline/mali-c55/mali-c55.cpp  |  6 ++--
 src/libcamera/pipeline/rkisp1/rkisp1.cpp      | 10 +++----
 src/libcamera/pipeline/simple/simple.cpp      |  6 ++--
 test/delayed_controls.cpp                     | 20 ++++++-------
 7 files changed, 53 insertions(+), 50 deletions(-)


---
base-commit: b1f09c013a01a82c739f0e30b71fd8d000ef5655
change-id: 20251102-rkisp1-vblank-ba2084646ba7

Best regards,

Patch
diff mbox series 

diff --git a/include/libcamera/internal/delayed_controls.h b/include/libcamera/internal/delayed_controls.h
index b64d8bba7cf73e10491d13d64bb4b3f7db80681b..c966fbedb1b832bcb50d0931204a011a334e6514 100644
--- a/include/libcamera/internal/delayed_controls.h
+++ b/include/libcamera/internal/delayed_controls.h
@@ -21,13 +21,14 @@  class V4L2Device;
 class DelayedControls : public Object
 {
 public:
-	struct ControlParams {
+	struct Controls {
+		uint32_t id;
 		unsigned int delay;
-		bool priorityWrite;
 	};
 
-	DelayedControls(V4L2Device *device,
-			const std::unordered_map<uint32_t, ControlParams> &controlParams);
+	using Params = std::vector<Controls>;
+
+	DelayedControls(V4L2Device *device, const Params &controlParams);
 
 	void reset();
 
@@ -37,6 +38,11 @@  public:
 	void applyControls(uint32_t sequence);
 
 private:
+	struct ControlParams {
+		unsigned int delay;
+		bool priorityWrite;
+	};
+
 	class Info : public ControlValue
 	{
 	public:
diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp
index 94d0a575b01b72416bdf05ea595f1c8e0ab6082c..f3092481ed804077dedd2cce0cda72f22d3563f8 100644
--- a/src/libcamera/delayed_controls.cpp
+++ b/src/libcamera/delayed_controls.cpp
@@ -39,40 +39,35 @@  LOG_DEFINE_CATEGORY(DelayedControls)
  */
 
 /**
- * \struct DelayedControls::ControlParams
- * \brief Parameters associated with controls handled by the \a DelayedControls
+ * \struct DelayedControls::Controls
+ * \brief Delays associated with controls handled by the \a DelayedControls
  * helper class
  *
- * \var ControlParams::delay
+ * \var Controls::id
+ * \brief The control id
+ *
+ * \var Controls::delay
  * \brief Frame delay from setting the control on a sensor device to when it is
  * consumed during framing.
- *
- * \var ControlParams::priorityWrite
- * \brief Flag to indicate that this control must be applied ahead of, and
- * separately from the other controls.
- *
- * Typically set for the \a V4L2_CID_VBLANK control so that the device driver
- * does not reject \a V4L2_CID_EXPOSURE control values that may be outside of
- * the existing vertical blanking specified bounds, but are within the new
- * blanking bounds.
+ */
+
+/**
+ * \typedef DelayedControls::Params
+ * \brief Vector of DelayedControls::Controls
  */
 
 /**
  * \brief Construct a DelayedControls instance
  * \param[in] device The V4L2 device the controls have to be applied to
- * \param[in] controlParams Map of the numerical V4L2 control ids to their
- * associated control parameters.
+ * \param[in] controlParams Control ids and delays
  *
- * The control parameters comprise of delays (in frames) and a priority write
- * flag. If this flag is set, the relevant control is written separately from,
- * and ahead of the rest of the batched controls.
+ * The control parameters associate a delay (in frames) to control ids.
  *
  * Only controls specified in \a controlParams are handled. If it's desired to
  * mix delayed controls and controls that take effect immediately the immediate
  * controls must be listed in the \a controlParams map with a delay value of 0.
  */
-DelayedControls::DelayedControls(V4L2Device *device,
-				 const std::unordered_map<uint32_t, ControlParams> &controlParams)
+DelayedControls::DelayedControls(V4L2Device *device, const Params &controlParams)
 	: device_(device), maxDelay_(0)
 {
 	const ControlInfoMap &controls = device_->controls();
@@ -82,11 +77,11 @@  DelayedControls::DelayedControls(V4L2Device *device,
 	 * device.
 	 */
 	for (auto const &param : controlParams) {
-		auto it = controls.find(param.first);
+		auto it = controls.find(param.id);
 		if (it == controls.end()) {
 			LOG(DelayedControls, Error)
 				<< "Delay request for control id "
-				<< utils::hex(param.first)
+				<< utils::hex(param.id)
 				<< " but control is not exposed by device "
 				<< device_->deviceNode();
 			continue;
@@ -94,7 +89,9 @@  DelayedControls::DelayedControls(V4L2Device *device,
 
 		const ControlId *id = it->first;
 
-		controlParams_[id] = param.second;
+		controlParams_[id].delay = param.delay;
+		if (param.id == V4L2_CID_VBLANK)
+			controlParams_[id].priorityWrite = true;
 
 		LOG(DelayedControls, Debug)
 			<< "Set a delay of " << controlParams_[id].delay
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index d6b7edcb5a7f9fcb4083d4105a193978e265ef67..0d6d2e30065e03defbc47ae45fd67a22f6bff25a 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -1083,9 +1083,9 @@  int PipelineHandlerIPU3::registerCameras()
 			continue;
 
 		const CameraSensorProperties::SensorDelays &delays = cio2->sensor()->sensorDelays();
-		std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {
-			{ V4L2_CID_ANALOGUE_GAIN, { delays.gainDelay, false } },
-			{ V4L2_CID_EXPOSURE, { delays.exposureDelay, false } },
+		DelayedControls::Params params = {
+			{ V4L2_CID_ANALOGUE_GAIN, delays.gainDelay },
+			{ V4L2_CID_EXPOSURE, delays.exposureDelay },
 		};
 
 		data->delayedCtrls_ =
diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
index 38bdc6138ed167a2c05f43ca30e60ed549fd953c..85f87ab979b23117c47bda4ca9418e4cb50c878d 100644
--- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp
+++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
@@ -1607,9 +1607,9 @@  bool PipelineHandlerMaliC55::registerSensorCamera(MediaLink *ispLink)
 		data->properties_ = data->sensor_->properties();
 
 		const CameraSensorProperties::SensorDelays &delays = data->sensor_->sensorDelays();
-		std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {
-			{ V4L2_CID_ANALOGUE_GAIN, { delays.gainDelay, false } },
-			{ V4L2_CID_EXPOSURE, { delays.exposureDelay, false } },
+		DelayedControls::Params params = {
+			{ V4L2_CID_ANALOGUE_GAIN, delays.gainDelay },
+			{ V4L2_CID_EXPOSURE, delays.exposureDelay },
 		};
 
 		data->delayedCtrls_ =
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index ecd13831539fdf5cb79da2ea4b33a353514328ae..525c8be6f19282e96df47ead602b67f6a74a8f96 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -1345,12 +1345,12 @@  int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
 	scalerMaxCrop_ = Rectangle(data->sensor_->resolution());
 
 	const CameraSensorProperties::SensorDelays &delays = data->sensor_->sensorDelays();
-	std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {
-		{ V4L2_CID_ANALOGUE_GAIN, { delays.gainDelay, false } },
-		{ V4L2_CID_EXPOSURE, { delays.exposureDelay, false } },
-		{ V4L2_CID_VBLANK, { delays.vblankDelay, false } },
-	};
+	DelayedControls::Params params = {
+		{ V4L2_CID_ANALOGUE_GAIN, delays.gainDelay },
+		{ V4L2_CID_EXPOSURE, delays.exposureDelay },
+		{ V4L2_CID_VBLANK, delays.vblankDelay },
 
+	};
 	data->delayedCtrls_ =
 		std::make_unique<DelayedControls>(data->sensor_->device(),
 						  params);
diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
index 91715b7f8afd2058c42856ef38144847f81bd7b0..617ffa8eef490a1301920b3f30ab1f13b3ff62ff 100644
--- a/src/libcamera/pipeline/simple/simple.cpp
+++ b/src/libcamera/pipeline/simple/simple.cpp
@@ -553,9 +553,9 @@  SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe,
 		return;
 
 	const CameraSensorProperties::SensorDelays &delays = sensor_->sensorDelays();
-	std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {
-		{ V4L2_CID_ANALOGUE_GAIN, { delays.gainDelay, false } },
-		{ V4L2_CID_EXPOSURE, { delays.exposureDelay, false } },
+	DelayedControls::Params params = {
+		{ V4L2_CID_ANALOGUE_GAIN, delays.gainDelay },
+		{ V4L2_CID_EXPOSURE, delays.exposureDelay },
 	};
 	delayedCtrls_ = std::make_unique<DelayedControls>(sensor_->device(), params);
 
diff --git a/test/delayed_controls.cpp b/test/delayed_controls.cpp
index 7bd30e7aead87f9350f1e0b48203f32804cfeef0..88425abc829a83ab7212775a8cab2165692a0a52 100644
--- a/test/delayed_controls.cpp
+++ b/test/delayed_controls.cpp
@@ -72,8 +72,8 @@  protected:
 
 	int singleControlNoDelay()
 	{
-		std::unordered_map<uint32_t, DelayedControls::ControlParams> delays = {
-			{ V4L2_CID_BRIGHTNESS, { 0, false } },
+		DelayedControls::Params delays = {
+			{ V4L2_CID_BRIGHTNESS, 0 },
 		};
 		std::unique_ptr<DelayedControls> delayed =
 			std::make_unique<DelayedControls>(dev_.get(), delays);
@@ -113,8 +113,8 @@  protected:
 
 	int singleControlWithDelay()
 	{
-		std::unordered_map<uint32_t, DelayedControls::ControlParams> delays = {
-			{ V4L2_CID_BRIGHTNESS, { 1, false } },
+		DelayedControls::Params delays = {
+			{ V4L2_CID_BRIGHTNESS, 1 },
 		};
 		std::unique_ptr<DelayedControls> delayed =
 			std::make_unique<DelayedControls>(dev_.get(), delays);
@@ -159,9 +159,9 @@  protected:
 	{
 		static const unsigned int maxDelay = 2;
 
-		std::unordered_map<uint32_t, DelayedControls::ControlParams> delays = {
-			{ V4L2_CID_BRIGHTNESS, { 1, false } },
-			{ V4L2_CID_CONTRAST, { maxDelay, false } },
+		DelayedControls::Params delays = {
+			{ V4L2_CID_BRIGHTNESS, 1 },
+			{ V4L2_CID_CONTRAST, maxDelay },
 		};
 		std::unique_ptr<DelayedControls> delayed =
 			std::make_unique<DelayedControls>(dev_.get(), delays);
@@ -210,9 +210,9 @@  protected:
 	{
 		static const unsigned int maxDelay = 2;
 
-		std::unordered_map<uint32_t, DelayedControls::ControlParams> delays = {
-			{ V4L2_CID_BRIGHTNESS, { 1, false } },
-			{ V4L2_CID_CONTRAST, { maxDelay, false } }
+		DelayedControls::Params delays = {
+			{ V4L2_CID_BRIGHTNESS, 1 },
+			{ V4L2_CID_CONTRAST, maxDelay }
 		};
 		std::unique_ptr<DelayedControls> delayed =
 			std::make_unique<DelayedControls>(dev_.get(), delays);