[PATCH v2 0/8] Add strobe/flash duration v4l2 ctrl & use it for ov9282

[PATCH v2 0/8] Add strobe/flash duration v4l2 ctrl & use it for ov9282

[Richard Leitner]

This series adds a new v4l2 controls named "strobe duration" with id
V4L2_CID_FLASH_DURATION. This control enables setting a desired
flash/strobe length/duration in µs.

As a first user of this new control add basic flash/strobe support for
ov9282 sensors using their "hardware strobe output". The duration
calculation is only interpolated from various measurements, as no
documentation was found.

Further flash/strobe-related controls as well as a migration to v4l2-cci
helpers for ov9282 will likely be implemented in future series.

All register addresses/values are based on the OV9281 datasheet v1.53
(january 2019). This series was tested using an ov9281 VisionComponents
camera module.

Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
---
Changes in v2:
- remove not needed controls in struct ov9282 (thanks Dave)
- Fix commit message of 3/3 regarding framerate get/set (thanks Dave)
- Add V4L2_CID_FLASH_STROBE_SOURCE impementation to ov9282
- Add new V4L2_CID_FLASH_DURATION control (as suggested by Laurent)
- Use FLASH_DURATION instead of FLASH_TIMEOUT for ov9282
- Link to v1: https://lore.kernel.org/r/20250303-ov9282-flash-strobe-v1-0-0fd57a1564ba@linux.dev

---
Richard Leitner (8):
      media: v4l: ctrls: add a control for flash/strobe duration
      media: v4l2-flash: add support for flash/stobe duration
      media: v4l2-flash: fix flash_timeout comment
      Documentation: uAPI: media: add V4L2_CID_FLASH_DURATION
      media: i2c: ov9282: add output enable register definitions
      media: i2c: ov9282: add led_mode v4l2 control
      media: i2c: ov9282: add strobe_duration v4l2 control
      media: i2c: ov9282: add strobe_source v4l2 control

 .../userspace-api/media/v4l/ext-ctrls-flash.rst    |  5 ++
 drivers/leds/led-class-flash.c                     | 15 ++++
 drivers/media/i2c/ov9282.c                         | 92 +++++++++++++++++++++-
 drivers/media/v4l2-core/v4l2-ctrls-defs.c          |  1 +
 drivers/media/v4l2-core/v4l2-flash-led-class.c     | 13 +++
 include/linux/led-class-flash.h                    | 18 ++++-
 include/uapi/linux/v4l2-controls.h                 |  1 +
 7 files changed, 141 insertions(+), 4 deletions(-)
---
base-commit: 9fc81f92bc8a84772e4a4094649601e7665a6092
change-id: 20250303-ov9282-flash-strobe-ac6bd00c9de6
prerequisite-change-id: 20250225-b4-ov9282-gain-ef1cdaba5bfd:v1
prerequisite-patch-id: 86f2582378ff7095ab65ce4bb25a143eb639e840
prerequisite-patch-id: b06eb6ec697aaf0b3155b4b2370f171d0d304ae2
prerequisite-patch-id: b123047d71bfb9b93f743bbdd6893d5a98495801

Best regards,
-- 
Richard Leitner <richard.leitner@linux.dev>

[PATCH v2 1/8] media: v4l: ctrls: add a control for flash/strobe duration

[Richard Leitner]

Add a control V4L2_CID_FLASH_DURATION to set the duration of a
flash/strobe pulse. This is different to the V4L2_CID_FLASH_TIMEOUT
control, as the timeout defines a limit after which the flash is
"forcefully" turned off again.

On the other hand the new V4L2_CID_FLASH_DURATION is the desired length
of the flash/strobe pulse

Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
---
 drivers/media/v4l2-core/v4l2-ctrls-defs.c | 1 +
 include/uapi/linux/v4l2-controls.h        | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
index 1ea52011247accc51d0261f56eab1cf13c0624a0..f9ed7273a9f3eafe01c31b638e1c8d9fcf5424af 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
@@ -1135,6 +1135,7 @@ const char *v4l2_ctrl_get_name(u32 id)
 	case V4L2_CID_FLASH_FAULT:		return "Faults";
 	case V4L2_CID_FLASH_CHARGE:		return "Charge";
 	case V4L2_CID_FLASH_READY:		return "Ready to Strobe";
+	case V4L2_CID_FLASH_DURATION:		return "Strobe Duration";
 
 	/* JPEG encoder controls */
 	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index 974fd254e57309e6def95b4a4f8e4de13a3972a7..80050cadb8377e3070ebbadc493fcd08b2c12c0b 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -1173,6 +1173,7 @@ enum v4l2_flash_strobe_source {
 
 #define V4L2_CID_FLASH_CHARGE			(V4L2_CID_FLASH_CLASS_BASE + 11)
 #define V4L2_CID_FLASH_READY			(V4L2_CID_FLASH_CLASS_BASE + 12)
+#define V4L2_CID_FLASH_DURATION			(V4L2_CID_FLASH_CLASS_BASE + 13)
 
 
 /* JPEG-class control IDs */

-- 
2.47.2

[PATCH v2 2/8] media: v4l2-flash: add support for flash/stobe duration

[Richard Leitner]

Add support for the new V4L2_CID_FLASH_DURATION control to the v4l2
led flash class.

Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
---
 drivers/leds/led-class-flash.c                 | 15 +++++++++++++++
 drivers/media/v4l2-core/v4l2-flash-led-class.c | 13 +++++++++++++
 include/linux/led-class-flash.h                | 16 ++++++++++++++++
 3 files changed, 44 insertions(+)

diff --git a/drivers/leds/led-class-flash.c b/drivers/leds/led-class-flash.c
index f4e26ce84862c05092a9598e63ed301967852f13..165035a8826ca7d44a5cd265a5130a76c6e94347 100644
--- a/drivers/leds/led-class-flash.c
+++ b/drivers/leds/led-class-flash.c
@@ -440,6 +440,21 @@ int led_update_flash_brightness(struct led_classdev_flash *fled_cdev)
 }
 EXPORT_SYMBOL_GPL(led_update_flash_brightness);
 
+int led_set_flash_duration(struct led_classdev_flash *fled_cdev, u32 duration)
+{
+	struct led_classdev *led_cdev = &fled_cdev->led_cdev;
+	struct led_flash_setting *s = &fled_cdev->duration;
+
+	s->val = duration;
+	led_clamp_align(s);
+
+	if (!(led_cdev->flags & LED_SUSPENDED))
+		return call_flash_op(fled_cdev, duration_set, s->val);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(led_set_flash_duration);
+
 MODULE_AUTHOR("Jacek Anaszewski <j.anaszewski@samsung.com>");
 MODULE_DESCRIPTION("LED Flash class interface");
 MODULE_LICENSE("GPL v2");
diff --git a/drivers/media/v4l2-core/v4l2-flash-led-class.c b/drivers/media/v4l2-core/v4l2-flash-led-class.c
index 355595a0fefac72c2f6941a30fa430d37dbdccfe..963b549480f6eb3b9eb0d80696a764de7ffcc1a2 100644
--- a/drivers/media/v4l2-core/v4l2-flash-led-class.c
+++ b/drivers/media/v4l2-core/v4l2-flash-led-class.c
@@ -298,6 +298,12 @@ static int v4l2_flash_s_ctrl(struct v4l2_ctrl *c)
 		 * microamperes for flash intensity units.
 		 */
 		return led_set_flash_brightness(fled_cdev, c->val);
+	case V4L2_CID_FLASH_DURATION:
+		/*
+		 * No conversion is needed as LED Flash class also uses
+		 * microseconds for flash duration units.
+		 */
+		return led_set_flash_duration(fled_cdev, c->val);
 	}
 
 	return -EINVAL;
@@ -424,6 +430,13 @@ static void __fill_ctrl_init_data(struct v4l2_flash *v4l2_flash,
 		ctrl_cfg->flags = V4L2_CTRL_FLAG_VOLATILE |
 				  V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
 	}
+
+	/* Init FLASH_DURATION ctrl data */
+	if (has_flash_op(fled_cdev, timeout_set)) {
+		ctrl_init_data[FLASH_DURATION].cid = V4L2_CID_FLASH_DURATION;
+		ctrl_cfg = &ctrl_init_data[FLASH_DURATION].config;
+		__lfs_to_v4l2_ctrl_config(&fled_cdev->duration, ctrl_cfg);
+		ctrl_cfg->id = V4L2_CID_FLASH_DURATION;
 }
 
 static int v4l2_flash_init_controls(struct v4l2_flash *v4l2_flash,
diff --git a/include/linux/led-class-flash.h b/include/linux/led-class-flash.h
index 36df927ec4b7dcaf9074c6ef32ac8ce83a87a79d..21ec856c36bc67decda46aa8ff1c040ffdcf1181 100644
--- a/include/linux/led-class-flash.h
+++ b/include/linux/led-class-flash.h
@@ -45,6 +45,8 @@ struct led_flash_ops {
 	int (*timeout_set)(struct led_classdev_flash *fled_cdev, u32 timeout);
 	/* get the flash LED fault */
 	int (*fault_get)(struct led_classdev_flash *fled_cdev, u32 *fault);
+	/* set flash duration */
+	int (*duration_set)(struct led_classdev_flash *fled_cdev, u32 duration);
 };
 
 /*
@@ -75,6 +77,9 @@ struct led_classdev_flash {
 	/* flash timeout value in microseconds along with its constraints */
 	struct led_flash_setting timeout;
 
+	/* flash timeout value in microseconds along with its constraints */
+	struct led_flash_setting duration;
+
 	/* LED Flash class sysfs groups */
 	const struct attribute_group *sysfs_groups[LED_FLASH_SYSFS_GROUPS_SIZE];
 };
@@ -209,4 +214,15 @@ int led_set_flash_timeout(struct led_classdev_flash *fled_cdev, u32 timeout);
  */
 int led_get_flash_fault(struct led_classdev_flash *fled_cdev, u32 *fault);
 
+/**
+ * led_set_flash_duration - set flash LED duration
+ * @fled_cdev: the flash LED to set
+ * @timeout: the flash duration to set it to
+ *
+ * Set the flash strobe duration.
+ *
+ * Returns: 0 on success or negative error value on failure
+ */
+int led_set_flash_duration(struct led_classdev_flash *fled_cdev, u32 duration);
+
 #endif	/* __LINUX_FLASH_LEDS_H_INCLUDED */

-- 
2.47.2

[PATCH v2 3/8] media: v4l2-flash: fix flash_timeout comment

[Richard Leitner]

The comment for the flash_timeout setter mentioned it is the "flash
duration". Fix this by changing it to "flash timeout".

Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
---
 include/linux/led-class-flash.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/led-class-flash.h b/include/linux/led-class-flash.h
index 21ec856c36bc67decda46aa8ff1c040ffdcf1181..775a96217518936633541c7a5d394502dbf04f83 100644
--- a/include/linux/led-class-flash.h
+++ b/include/linux/led-class-flash.h
@@ -197,7 +197,7 @@ int led_update_flash_brightness(struct led_classdev_flash *fled_cdev);
  * @fled_cdev: the flash LED to set
  * @timeout: the flash timeout to set it to
  *
- * Set the flash strobe duration.
+ * Set the flash strobe timeout.
  *
  * Returns: 0 on success or negative error value on failure
  */

-- 
2.47.2

[PATCH v2 4/8] Documentation: uAPI: media: add V4L2_CID_FLASH_DURATION

[Richard Leitner]

Add the new strobe_duration control to v4l uAPI documentation.

Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
---
 Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
index d22c5efb806a183a3ad67ec3e6550b002a51659a..03a58ef94be7c870f55d5a9bb09503995dbfb402 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
@@ -186,3 +186,8 @@ Flash Control IDs
     charged before strobing. LED flashes often require a cooldown period
     after strobe during which another strobe will not be possible. This
     is a read-only control.
+
+``V4L2_CID_FLASH_DURATION (integer)``
+    Duration the flash should be on when the flash LED is in flash mode
+    (V4L2_FLASH_LED_MODE_FLASH). The unit should be microseconds (µs)
+    if possible.

-- 
2.47.2

[PATCH v2 5/8] media: i2c: ov9282: add output enable register definitions

[Richard Leitner]

Add #define's for the output enable registers (0x3004, 0x3005, 0x3006),
also known as SC_CTRL_04, SC_CTRL_05, SC_CTRL_04. Use those register
definitions instead of the raw values in the `common_regs` struct.

All values are based on the OV9281 datasheet v1.53 (january 2019).

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
---
 drivers/media/i2c/ov9282.c | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index c882a021cf18852237bf9b9524d3de0c5b48cbcb..f42e0d439753e74d14e3a3592029e48f49234927 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -37,6 +37,29 @@
 #define OV9282_REG_ID		0x300a
 #define OV9282_ID		0x9281
 
+/* Output enable registers */
+#define OV9282_REG_OUTPUT_ENABLE4	0x3004
+#define OV9282_OUTPUT_ENABLE4_GPIO2	BIT(1)
+#define OV9282_OUTPUT_ENABLE4_D9	BIT(0)
+
+#define OV9282_REG_OUTPUT_ENABLE5	0x3005
+#define OV9282_OUTPUT_ENABLE5_D8	BIT(7)
+#define OV9282_OUTPUT_ENABLE5_D7	BIT(6)
+#define OV9282_OUTPUT_ENABLE5_D6	BIT(5)
+#define OV9282_OUTPUT_ENABLE5_D5	BIT(4)
+#define OV9282_OUTPUT_ENABLE5_D4	BIT(3)
+#define OV9282_OUTPUT_ENABLE5_D3	BIT(2)
+#define OV9282_OUTPUT_ENABLE5_D2	BIT(1)
+#define OV9282_OUTPUT_ENABLE5_D1	BIT(0)
+
+#define OV9282_REG_OUTPUT_ENABLE6	0x3006
+#define OV9282_OUTPUT_ENABLE6_D0	BIT(7)
+#define OV9282_OUTPUT_ENABLE6_PCLK	BIT(6)
+#define OV9282_OUTPUT_ENABLE6_HREF	BIT(5)
+#define OV9282_OUTPUT_ENABLE6_STROBE	BIT(3)
+#define OV9282_OUTPUT_ENABLE6_ILPWM	BIT(2)
+#define OV9282_OUTPUT_ENABLE6_VSYNC	BIT(1)
+
 /* Exposure control */
 #define OV9282_REG_EXPOSURE	0x3500
 #define OV9282_EXPOSURE_MIN	1
@@ -213,9 +236,9 @@ static const struct ov9282_reg common_regs[] = {
 	{0x0302, 0x32},
 	{0x030e, 0x02},
 	{0x3001, 0x00},
-	{0x3004, 0x00},
-	{0x3005, 0x00},
-	{0x3006, 0x04},
+	{OV9282_REG_OUTPUT_ENABLE4, 0x00},
+	{OV9282_REG_OUTPUT_ENABLE5, 0x00},
+	{OV9282_REG_OUTPUT_ENABLE6, OV9282_OUTPUT_ENABLE6_ILPWM},
 	{0x3011, 0x0a},
 	{0x3013, 0x18},
 	{0x301c, 0xf0},

-- 
2.47.2

[PATCH v2 6/8] media: i2c: ov9282: add led_mode v4l2 control

[Richard Leitner]

Add V4L2_CID_FLASH_LED_MODE support using the "strobe output enable"
feature of the sensor. This implements following modes:

 - V4L2_FLASH_LED_MODE_NONE, which disables the strobe output
 - V4L2_FLASH_LED_MODE_FLASH, which enables the strobe output

All values are based on the OV9281 datasheet v1.53 (january 2019) and
tested using an ov9281 VisionComponents module.

Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
---
 drivers/media/i2c/ov9282.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index f42e0d439753e74d14e3a3592029e48f49234927..35edbe1e0efb61a0e03da0ed817c4c4ec0ae9c35 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -670,6 +670,23 @@ static int ov9282_set_ctrl_vflip(struct ov9282 *ov9282, int value)
 				current_val);
 }
 
+static int ov9282_set_ctrl_flash_led_mode(struct ov9282 *ov9282, int mode)
+{
+	u32 current_val;
+	int ret = ov9282_read_reg(ov9282, OV9282_REG_OUTPUT_ENABLE6, 1,
+				  &current_val);
+	if (ret)
+		return ret;
+
+	if (mode == V4L2_FLASH_LED_MODE_FLASH)
+		current_val |= OV9282_OUTPUT_ENABLE6_STROBE;
+	else
+		current_val &= ~OV9282_OUTPUT_ENABLE6_STROBE;
+
+	return ov9282_write_reg(ov9282, OV9282_REG_OUTPUT_ENABLE6, 1,
+				current_val);
+}
+
 /**
  * ov9282_set_ctrl() - Set subdevice control
  * @ctrl: pointer to v4l2_ctrl structure
@@ -736,6 +753,9 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
 		ret = ov9282_write_reg(ov9282, OV9282_REG_TIMING_HTS, 2,
 				       (ctrl->val + ov9282->cur_mode->width) >> 1);
 		break;
+	case V4L2_CID_FLASH_LED_MODE:
+		ret = ov9282_set_ctrl_flash_led_mode(ov9282, ctrl->val);
+		break;
 	default:
 		dev_err(ov9282->dev, "Invalid control %d", ctrl->id);
 		ret = -EINVAL;
@@ -1391,6 +1411,13 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
 						OV9282_TIMING_HTS_MAX - mode->width,
 						1, hblank_min);
 
+	/* Flash/Strobe controls */
+	v4l2_ctrl_new_std_menu(ctrl_hdlr, &ov9282_ctrl_ops,
+			       V4L2_CID_FLASH_LED_MODE,
+			       V4L2_FLASH_LED_MODE_TORCH,
+			       (1 << V4L2_FLASH_LED_MODE_TORCH),
+			       V4L2_FLASH_LED_MODE_NONE);
+
 	ret = v4l2_fwnode_device_parse(ov9282->dev, &props);
 	if (!ret) {
 		/* Failure sets ctrl_hdlr->error, which we check afterwards anyway */

-- 
2.47.2

[PATCH v2 7/8] media: i2c: ov9282: add strobe_duration v4l2 control

[Richard Leitner]

Add V4L2_CID_FLASH_DURATION support using the "strobe_frame_span"
feature of the sensor. This is implemented by transforming the given µs
value by an interpolated formula to a "span step width" value and
writing it to register PWM_CTRL_25, PWM_CTRL_26, PWM_CTRL_27,
PWM_CTRL_28 (0x3925, 0x3926, 0x3927, 0x3928).

The maximum control value is set to the period of the current default
framerate.

All register values are based on the OV9281 datasheet v1.53 (jan 2019)
and tested using an ov9281 VisionComponents module.

Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
---
 drivers/media/i2c/ov9282.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index 35edbe1e0efb61a0e03da0ed817c4c4ec0ae9c35..5ddbfc51586111fbd2e17b739fb3d28bfb0aee1e 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -97,6 +97,10 @@
 #define OV9282_REG_MIPI_CTRL00	0x4800
 #define OV9282_GATED_CLOCK	BIT(5)
 
+/* Flash/Strobe control registers */
+#define OV9282_REG_FLASH_DURATION	0x3925
+#define OV9282_FLASH_DURATION_DEFAULT	0x0000001A
+
 /* Input clock rate */
 #define OV9282_INCLK_RATE	24000000
 
@@ -687,6 +691,24 @@ static int ov9282_set_ctrl_flash_led_mode(struct ov9282 *ov9282, int mode)
 				current_val);
 }
 
+static int ov9282_set_ctrl_flash_duration(struct ov9282 *ov9282, int value)
+{
+	/* Calculate "strobe_frame_span" increments from a given value (µs).
+	 * This is quite tricky as "The step width of shift and span is
+	 * programmable under system clock domain.", but it's not documented
+	 * how to program this step width (at least in the datasheet available
+	 * to the author at time of writing).
+	 * The formula below is interpolated from different modes/framerates
+	 * and should work quite well for most settings.
+	 */
+	u32 val = value * 192 / (ov9282->cur_mode->width + ov9282->hblank_ctrl->val);
+
+	ov9282_write_reg(ov9282, OV9282_REG_FLASH_DURATION, 1, (val >> 24) & 0xff);
+	ov9282_write_reg(ov9282, OV9282_REG_FLASH_DURATION + 1, 1, (val >> 16) & 0xff);
+	ov9282_write_reg(ov9282, OV9282_REG_FLASH_DURATION + 2, 1, (val >> 8) & 0xff);
+	return ov9282_write_reg(ov9282, OV9282_REG_FLASH_DURATION + 3, 1, val & 0xff);
+}
+
 /**
  * ov9282_set_ctrl() - Set subdevice control
  * @ctrl: pointer to v4l2_ctrl structure
@@ -756,6 +778,9 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
 	case V4L2_CID_FLASH_LED_MODE:
 		ret = ov9282_set_ctrl_flash_led_mode(ov9282, ctrl->val);
 		break;
+	case V4L2_CID_FLASH_DURATION:
+		ret = ov9282_set_ctrl_flash_duration(ov9282, ctrl->val);
+		break;
 	default:
 		dev_err(ov9282->dev, "Invalid control %d", ctrl->id);
 		ret = -EINVAL;
@@ -1418,6 +1443,9 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
 			       (1 << V4L2_FLASH_LED_MODE_TORCH),
 			       V4L2_FLASH_LED_MODE_NONE);
 
+	v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
+			  0, 13900, 1, 8);
+
 	ret = v4l2_fwnode_device_parse(ov9282->dev, &props);
 	if (!ret) {
 		/* Failure sets ctrl_hdlr->error, which we check afterwards anyway */

-- 
2.47.2

[PATCH v2 8/8] media: i2c: ov9282: add strobe_source v4l2 control

[Richard Leitner]

Add read-only V4L2_CID_FLASH_STROBE_SOURCE control. Its value is fixed
to V4L2_FLASH_STROBE_SOURCE_EXTERNAL as the camera sensor triggers the
strobe based on its register settings.

Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
---
 drivers/media/i2c/ov9282.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index 5ddbfc51586111fbd2e17b739fb3d28bfb0aee1e..34ea903a18dadeeebd497a4a8858abf12b598717 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -1367,6 +1367,7 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
 	struct v4l2_ctrl_handler *ctrl_hdlr = &ov9282->ctrl_handler;
 	const struct ov9282_mode *mode = ov9282->cur_mode;
 	struct v4l2_fwnode_device_properties props;
+	struct v4l2_ctrl *ctrl;
 	u32 hblank_min;
 	u32 lpfr;
 	int ret;
@@ -1446,6 +1447,13 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
 	v4l2_ctrl_new_std(ctrl_hdlr, &ov9282_ctrl_ops, V4L2_CID_FLASH_DURATION,
 			  0, 13900, 1, 8);
 
+	ctrl = v4l2_ctrl_new_std_menu(ctrl_hdlr, &ov9282_ctrl_ops,
+				      V4L2_CID_FLASH_STROBE_SOURCE,
+				      V4L2_FLASH_STROBE_SOURCE_EXTERNAL,
+				      ~(1 << V4L2_FLASH_STROBE_SOURCE_EXTERNAL),
+				      V4L2_FLASH_STROBE_SOURCE_EXTERNAL);
+	ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
 	ret = v4l2_fwnode_device_parse(ov9282->dev, &props);
 	if (!ret) {
 		/* Failure sets ctrl_hdlr->error, which we check afterwards anyway */

-- 
2.47.2

Re: [PATCH v2 1/8] media: v4l: ctrls: add a control for flash/strobe duration

[Sakari Ailus]

Hi Richard,

Thanks for the set.

On Fri, Mar 14, 2025 at 09:49:55AM +0100, Richard Leitner wrote:
> Add a control V4L2_CID_FLASH_DURATION to set the duration of a
> flash/strobe pulse. This is different to the V4L2_CID_FLASH_TIMEOUT
> control, as the timeout defines a limit after which the flash is
> "forcefully" turned off again.
> 
> On the other hand the new V4L2_CID_FLASH_DURATION is the desired length
> of the flash/strobe pulse

What's the actual difference between the two? To me they appear the same,
just expressed in a different way.

> 
> Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> ---
>  drivers/media/v4l2-core/v4l2-ctrls-defs.c | 1 +
>  include/uapi/linux/v4l2-controls.h        | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> index 1ea52011247accc51d0261f56eab1cf13c0624a0..f9ed7273a9f3eafe01c31b638e1c8d9fcf5424af 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> @@ -1135,6 +1135,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_FLASH_FAULT:		return "Faults";
>  	case V4L2_CID_FLASH_CHARGE:		return "Charge";
>  	case V4L2_CID_FLASH_READY:		return "Ready to Strobe";
> +	case V4L2_CID_FLASH_DURATION:		return "Strobe Duration";
>  
>  	/* JPEG encoder controls */
>  	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index 974fd254e57309e6def95b4a4f8e4de13a3972a7..80050cadb8377e3070ebbadc493fcd08b2c12c0b 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -1173,6 +1173,7 @@ enum v4l2_flash_strobe_source {
>  
>  #define V4L2_CID_FLASH_CHARGE			(V4L2_CID_FLASH_CLASS_BASE + 11)
>  #define V4L2_CID_FLASH_READY			(V4L2_CID_FLASH_CLASS_BASE + 12)
> +#define V4L2_CID_FLASH_DURATION			(V4L2_CID_FLASH_CLASS_BASE + 13)
>  
>  
>  /* JPEG-class control IDs */
> 

-- 
Regards,

Sakari Ailus

Re: [PATCH v2 4/8] Documentation: uAPI: media: add V4L2_CID_FLASH_DURATION

[Hans Verkuil]

On 14/03/2025 09:49, Richard Leitner wrote:
> Add the new strobe_duration control to v4l uAPI documentation.
> 
> Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> ---
>  Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> index d22c5efb806a183a3ad67ec3e6550b002a51659a..03a58ef94be7c870f55d5a9bb09503995dbfb402 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> @@ -186,3 +186,8 @@ Flash Control IDs
>      charged before strobing. LED flashes often require a cooldown period
>      after strobe during which another strobe will not be possible. This
>      is a read-only control.
> +
> +``V4L2_CID_FLASH_DURATION (integer)``
> +    Duration the flash should be on when the flash LED is in flash mode
> +    (V4L2_FLASH_LED_MODE_FLASH). The unit should be microseconds (µs)
> +    if possible.
> 

If this control is present, does that mean that the flash duration always have
to be set manually? Or can there be an 'Auto' mode as well? And if so, how is
that set?

Regards,

	Hans

Re: [PATCH v2 2/8] media: v4l2-flash: add support for flash/stobe duration

[Lee Jones]

On Fri, 14 Mar 2025, Richard Leitner wrote:

> Add support for the new V4L2_CID_FLASH_DURATION control to the v4l2
> led flash class.
> 
> Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> ---
>  drivers/leds/led-class-flash.c                 | 15 +++++++++++++++
>  include/linux/led-class-flash.h                | 16 ++++++++++++++++

This should be a separate patch.

Then Mauro and I will have to come up with a merge-plan for the series.

>  drivers/media/v4l2-core/v4l2-flash-led-class.c | 13 +++++++++++++
>  3 files changed, 44 insertions(+)

-- 
Lee Jones [李琼斯]

Re: [PATCH v2 1/8] media: v4l: ctrls: add a control for flash/strobe duration

[Richard Leitner]

On Fri, Mar 14, 2025 at 09:20:23AM +0000, Sakari Ailus wrote:
> Hi Richard,
> 
> Thanks for the set.

Hi Sakari,
thanks for the quick response!

> 
> On Fri, Mar 14, 2025 at 09:49:55AM +0100, Richard Leitner wrote:
> > Add a control V4L2_CID_FLASH_DURATION to set the duration of a
> > flash/strobe pulse. This is different to the V4L2_CID_FLASH_TIMEOUT
> > control, as the timeout defines a limit after which the flash is
> > "forcefully" turned off again.
> > 
> > On the other hand the new V4L2_CID_FLASH_DURATION is the desired length
> > of the flash/strobe pulse
> 
> What's the actual difference between the two? To me they appear the same,
> just expressed in a different way.

According to FLASH_TIMEOUT documentation:

	Hardware timeout for flash. The flash strobe is stopped after this
	period of time has passed from the start of the strobe. [1]

This is a little bit unspecific, but as also discussed with Dave [2]
according to the documentation of V4L2_FLASH_FAULT_TIMEOUT it seems to
be targeted at providing a "real timeout" control, not settings the
desired duration:

	The flash strobe was still on when the timeout set by the user
	--- V4L2_CID_FLASH_TIMEOUT control --- has expired. Not all flash
	controllers may set this in all such conditions. [1]

If I understood that wrong, I'm also happy to use FLASH_TIMEOUT for this
use-case. But tbh I think FLASH_DURATION would be more specific.

As this still seems unclear: Should the documentation be
changed/rewritten if we stick with the FLASH_DURATION approach?

[1] https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-flash.html
[2] https://lore.kernel.org/lkml/CAPY8ntB8i4OyUWAL8k899yUd5QsRifJXiOfWXKceGQ7TNZ4OUw@mail.gmail.com/

> 
> > 
> > Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> > ---
> >  drivers/media/v4l2-core/v4l2-ctrls-defs.c | 1 +
> >  include/uapi/linux/v4l2-controls.h        | 1 +
> >  2 files changed, 2 insertions(+)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > index 1ea52011247accc51d0261f56eab1cf13c0624a0..f9ed7273a9f3eafe01c31b638e1c8d9fcf5424af 100644
> > --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > @@ -1135,6 +1135,7 @@ const char *v4l2_ctrl_get_name(u32 id)
> >  	case V4L2_CID_FLASH_FAULT:		return "Faults";
> >  	case V4L2_CID_FLASH_CHARGE:		return "Charge";
> >  	case V4L2_CID_FLASH_READY:		return "Ready to Strobe";
> > +	case V4L2_CID_FLASH_DURATION:		return "Strobe Duration";
> >  
> >  	/* JPEG encoder controls */
> >  	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
> > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> > index 974fd254e57309e6def95b4a4f8e4de13a3972a7..80050cadb8377e3070ebbadc493fcd08b2c12c0b 100644
> > --- a/include/uapi/linux/v4l2-controls.h
> > +++ b/include/uapi/linux/v4l2-controls.h
> > @@ -1173,6 +1173,7 @@ enum v4l2_flash_strobe_source {
> >  
> >  #define V4L2_CID_FLASH_CHARGE			(V4L2_CID_FLASH_CLASS_BASE + 11)
> >  #define V4L2_CID_FLASH_READY			(V4L2_CID_FLASH_CLASS_BASE + 12)
> > +#define V4L2_CID_FLASH_DURATION			(V4L2_CID_FLASH_CLASS_BASE + 13)
> >  
> >  
> >  /* JPEG-class control IDs */
> > 
> 
> -- 
> Regards,
> 
> Sakari Ailus

Thanks!
Richard

Re: [PATCH v2 4/8] Documentation: uAPI: media: add V4L2_CID_FLASH_DURATION

[Richard Leitner]

Hi Hans,

thanks for your quick feedback!

On Fri, Mar 14, 2025 at 10:41:04AM +0100, Hans Verkuil wrote:
> On 14/03/2025 09:49, Richard Leitner wrote:
> > Add the new strobe_duration control to v4l uAPI documentation.
> > 
> > Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> > ---
> >  Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> > index d22c5efb806a183a3ad67ec3e6550b002a51659a..03a58ef94be7c870f55d5a9bb09503995dbfb402 100644
> > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> > @@ -186,3 +186,8 @@ Flash Control IDs
> >      charged before strobing. LED flashes often require a cooldown period
> >      after strobe during which another strobe will not be possible. This
> >      is a read-only control.
> > +
> > +``V4L2_CID_FLASH_DURATION (integer)``
> > +    Duration the flash should be on when the flash LED is in flash mode
> > +    (V4L2_FLASH_LED_MODE_FLASH). The unit should be microseconds (µs)
> > +    if possible.
> > 
> 
> If this control is present, does that mean that the flash duration always have
> to be set manually? Or can there be an 'Auto' mode as well? And if so, how is
> that set?

To be honest I haven't thought about automatic flash duration. Is this
something which is required?

At least for the ov9282 sensor (which I've implemented this control for
in this series) there is no "auto" mode AFAIK.

If it's required: What would be the best solution?
Extending V4L2_CID_FLASH_LED_MODE with a new menu option? E.g.
V4L2_FLASH_LED_MODE_FLASH_{MANUAL,AUTO}?

> 
> Regards,
> 
> 	Hans

Thanks!
Richard

Re: [PATCH v2 2/8] media: v4l2-flash: add support for flash/stobe duration

[Richard Leitner]

Hi Lee,
thanks for your quick feedback!

On Fri, Mar 14, 2025 at 09:51:46AM +0000, Lee Jones wrote:
> On Fri, 14 Mar 2025, Richard Leitner wrote:
> 
> > Add support for the new V4L2_CID_FLASH_DURATION control to the v4l2
> > led flash class.
> > 
> > Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> > ---
> >  drivers/leds/led-class-flash.c                 | 15 +++++++++++++++
> >  include/linux/led-class-flash.h                | 16 ++++++++++++++++
> 
> This should be a separate patch.
> 
> Then Mauro and I will have to come up with a merge-plan for the series.

Sure. Will create a separate patch for this in v3.

> 
> >  drivers/media/v4l2-core/v4l2-flash-led-class.c | 13 +++++++++++++
> >  3 files changed, 44 insertions(+)
> 
> -- 
> Lee Jones [李琼斯]

Thanks!
Richard

Re: [PATCH v2 4/8] Documentation: uAPI: media: add V4L2_CID_FLASH_DURATION

[Hans Verkuil]

On 14/03/2025 11:28, Richard Leitner wrote:
> Hi Hans,
> 
> thanks for your quick feedback!
> 
> On Fri, Mar 14, 2025 at 10:41:04AM +0100, Hans Verkuil wrote:
>> On 14/03/2025 09:49, Richard Leitner wrote:
>>> Add the new strobe_duration control to v4l uAPI documentation.
>>>
>>> Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
>>> ---
>>>  Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
>>> index d22c5efb806a183a3ad67ec3e6550b002a51659a..03a58ef94be7c870f55d5a9bb09503995dbfb402 100644
>>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
>>> @@ -186,3 +186,8 @@ Flash Control IDs
>>>      charged before strobing. LED flashes often require a cooldown period
>>>      after strobe during which another strobe will not be possible. This
>>>      is a read-only control.
>>> +
>>> +``V4L2_CID_FLASH_DURATION (integer)``
>>> +    Duration the flash should be on when the flash LED is in flash mode
>>> +    (V4L2_FLASH_LED_MODE_FLASH). The unit should be microseconds (µs)
>>> +    if possible.
>>>
>>
>> If this control is present, does that mean that the flash duration always have
>> to be set manually? Or can there be an 'Auto' mode as well? And if so, how is
>> that set?
> 
> To be honest I haven't thought about automatic flash duration. Is this
> something which is required?

No idea, it was just something I was wondering about. Sakari probably knows a lot
more about this.

Regards,

	Hans

> 
> At least for the ov9282 sensor (which I've implemented this control for
> in this series) there is no "auto" mode AFAIK.
> 
> If it's required: What would be the best solution?
> Extending V4L2_CID_FLASH_LED_MODE with a new menu option? E.g.
> V4L2_FLASH_LED_MODE_FLASH_{MANUAL,AUTO}?
> 
>>
>> Regards,
>>
>> 	Hans
> 
> Thanks!
> Richard

Re: [PATCH v2 1/8] media: v4l: ctrls: add a control for flash/strobe duration

[Sakari Ailus]

Hi Richard,

On Fri, Mar 14, 2025 at 11:25:09AM +0100, Richard Leitner wrote:
> On Fri, Mar 14, 2025 at 09:20:23AM +0000, Sakari Ailus wrote:
> > Hi Richard,
> > 
> > Thanks for the set.
> 
> Hi Sakari,
> thanks for the quick response!
> 
> > 
> > On Fri, Mar 14, 2025 at 09:49:55AM +0100, Richard Leitner wrote:
> > > Add a control V4L2_CID_FLASH_DURATION to set the duration of a
> > > flash/strobe pulse. This is different to the V4L2_CID_FLASH_TIMEOUT
> > > control, as the timeout defines a limit after which the flash is
> > > "forcefully" turned off again.
> > > 
> > > On the other hand the new V4L2_CID_FLASH_DURATION is the desired length
> > > of the flash/strobe pulse
> > 
> > What's the actual difference between the two? To me they appear the same,
> > just expressed in a different way.
> 
> According to FLASH_TIMEOUT documentation:
> 
> 	Hardware timeout for flash. The flash strobe is stopped after this
> 	period of time has passed from the start of the strobe. [1]
> 
> This is a little bit unspecific, but as also discussed with Dave [2]
> according to the documentation of V4L2_FLASH_FAULT_TIMEOUT it seems to
> be targeted at providing a "real timeout" control, not settings the
> desired duration:
> 
> 	The flash strobe was still on when the timeout set by the user
> 	--- V4L2_CID_FLASH_TIMEOUT control --- has expired. Not all flash
> 	controllers may set this in all such conditions. [1]
> 
> If I understood that wrong, I'm also happy to use FLASH_TIMEOUT for this
> use-case. But tbh I think FLASH_DURATION would be more specific.
> 
> As this still seems unclear: Should the documentation be
> changed/rewritten if we stick with the FLASH_DURATION approach?
> 
> [1] https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-flash.html
> [2] https://lore.kernel.org/lkml/CAPY8ntB8i4OyUWAL8k899yUd5QsRifJXiOfWXKceGQ7TNZ4OUw@mail.gmail.com/

Right, I think I can see what you're after.

How does the sensor determine when to start the strobe, i.e. on which frame
and which part of the exposure of that frame?

> 
> > 
> > > 
> > > Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> > > ---
> > >  drivers/media/v4l2-core/v4l2-ctrls-defs.c | 1 +
> > >  include/uapi/linux/v4l2-controls.h        | 1 +
> > >  2 files changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > > index 1ea52011247accc51d0261f56eab1cf13c0624a0..f9ed7273a9f3eafe01c31b638e1c8d9fcf5424af 100644
> > > --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > > +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > > @@ -1135,6 +1135,7 @@ const char *v4l2_ctrl_get_name(u32 id)
> > >  	case V4L2_CID_FLASH_FAULT:		return "Faults";
> > >  	case V4L2_CID_FLASH_CHARGE:		return "Charge";
> > >  	case V4L2_CID_FLASH_READY:		return "Ready to Strobe";
> > > +	case V4L2_CID_FLASH_DURATION:		return "Strobe Duration";
> > >  
> > >  	/* JPEG encoder controls */
> > >  	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
> > > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> > > index 974fd254e57309e6def95b4a4f8e4de13a3972a7..80050cadb8377e3070ebbadc493fcd08b2c12c0b 100644
> > > --- a/include/uapi/linux/v4l2-controls.h
> > > +++ b/include/uapi/linux/v4l2-controls.h
> > > @@ -1173,6 +1173,7 @@ enum v4l2_flash_strobe_source {
> > >  
> > >  #define V4L2_CID_FLASH_CHARGE			(V4L2_CID_FLASH_CLASS_BASE + 11)
> > >  #define V4L2_CID_FLASH_READY			(V4L2_CID_FLASH_CLASS_BASE + 12)
> > > +#define V4L2_CID_FLASH_DURATION			(V4L2_CID_FLASH_CLASS_BASE + 13)
> > >  
> > >  
> > >  /* JPEG-class control IDs */
> > > 
> > 

-- 
Regards,

Sakari Ailus

Re: [PATCH v2 1/8] media: v4l: ctrls: add a control for flash/strobe duration

[Richard Leitner]

Hi Sakari,

On Fri, Mar 14, 2025 at 01:34:07PM +0000, Sakari Ailus wrote:
> Hi Richard,
> 
> On Fri, Mar 14, 2025 at 11:25:09AM +0100, Richard Leitner wrote:
> > On Fri, Mar 14, 2025 at 09:20:23AM +0000, Sakari Ailus wrote:
[...]
> > > On Fri, Mar 14, 2025 at 09:49:55AM +0100, Richard Leitner wrote:
> > > > Add a control V4L2_CID_FLASH_DURATION to set the duration of a
> > > > flash/strobe pulse. This is different to the V4L2_CID_FLASH_TIMEOUT
> > > > control, as the timeout defines a limit after which the flash is
> > > > "forcefully" turned off again.
> > > > 
> > > > On the other hand the new V4L2_CID_FLASH_DURATION is the desired length
> > > > of the flash/strobe pulse
> > > 
> > > What's the actual difference between the two? To me they appear the same,
> > > just expressed in a different way.
> > 
> > According to FLASH_TIMEOUT documentation:
> > 
> > 	Hardware timeout for flash. The flash strobe is stopped after this
> > 	period of time has passed from the start of the strobe. [1]
> > 
> > This is a little bit unspecific, but as also discussed with Dave [2]
> > according to the documentation of V4L2_FLASH_FAULT_TIMEOUT it seems to
> > be targeted at providing a "real timeout" control, not settings the
> > desired duration:
> > 
> > 	The flash strobe was still on when the timeout set by the user
> > 	--- V4L2_CID_FLASH_TIMEOUT control --- has expired. Not all flash
> > 	controllers may set this in all such conditions. [1]
> > 
> > If I understood that wrong, I'm also happy to use FLASH_TIMEOUT for this
> > use-case. But tbh I think FLASH_DURATION would be more specific.
> > 
> > As this still seems unclear: Should the documentation be
> > changed/rewritten if we stick with the FLASH_DURATION approach?
> > 
> > [1] https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-flash.html
> > [2] https://lore.kernel.org/lkml/CAPY8ntB8i4OyUWAL8k899yUd5QsRifJXiOfWXKceGQ7TNZ4OUw@mail.gmail.com/
> 
> Right, I think I can see what you're after.
> 
> How does the sensor determine when to start the strobe, i.e. on which frame
> and which part of the exposure of that frame?

In general I think it's not part of V4L2_CID_FLASH_DURATION to take any
assumptions on that, as that's sensor/flash specific IMHO.

In case of the ov9282 sensor driver (which is also part of this series)
the strobe is started synchronously with the exposure on each frame
start.
Being even more specific on the ov9292, the sensor also offers the
possibility to shift that strobe start in in either direction using a
register. Implementing this "flash shift" (as it's called in the sensors
datasheet) is currently under test on my side. I will likely send a
series for that in the coming weeks.

> > > > Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> > > > ---
> > > >  drivers/media/v4l2-core/v4l2-ctrls-defs.c | 1 +
> > > >  include/uapi/linux/v4l2-controls.h        | 1 +
> > > >  2 files changed, 2 insertions(+)
[...]
> 
> -- 
> Regards,
> 
> Sakari Ailus

Thanks!
Richard

Re: [PATCH v2 1/8] media: v4l: ctrls: add a control for flash/strobe duration

[Sakari Ailus]

Hi Richard,

On Fri, Mar 14, 2025 at 05:08:16PM +0100, Richard Leitner wrote:
> Hi Sakari,
> 
> On Fri, Mar 14, 2025 at 01:34:07PM +0000, Sakari Ailus wrote:
> > Hi Richard,
> > 
> > On Fri, Mar 14, 2025 at 11:25:09AM +0100, Richard Leitner wrote:
> > > On Fri, Mar 14, 2025 at 09:20:23AM +0000, Sakari Ailus wrote:
> [...]
> > > > On Fri, Mar 14, 2025 at 09:49:55AM +0100, Richard Leitner wrote:
> > > > > Add a control V4L2_CID_FLASH_DURATION to set the duration of a
> > > > > flash/strobe pulse. This is different to the V4L2_CID_FLASH_TIMEOUT
> > > > > control, as the timeout defines a limit after which the flash is
> > > > > "forcefully" turned off again.
> > > > > 
> > > > > On the other hand the new V4L2_CID_FLASH_DURATION is the desired length
> > > > > of the flash/strobe pulse
> > > > 
> > > > What's the actual difference between the two? To me they appear the same,
> > > > just expressed in a different way.
> > > 
> > > According to FLASH_TIMEOUT documentation:
> > > 
> > > 	Hardware timeout for flash. The flash strobe is stopped after this
> > > 	period of time has passed from the start of the strobe. [1]
> > > 
> > > This is a little bit unspecific, but as also discussed with Dave [2]
> > > according to the documentation of V4L2_FLASH_FAULT_TIMEOUT it seems to
> > > be targeted at providing a "real timeout" control, not settings the
> > > desired duration:
> > > 
> > > 	The flash strobe was still on when the timeout set by the user
> > > 	--- V4L2_CID_FLASH_TIMEOUT control --- has expired. Not all flash
> > > 	controllers may set this in all such conditions. [1]
> > > 
> > > If I understood that wrong, I'm also happy to use FLASH_TIMEOUT for this
> > > use-case. But tbh I think FLASH_DURATION would be more specific.
> > > 
> > > As this still seems unclear: Should the documentation be
> > > changed/rewritten if we stick with the FLASH_DURATION approach?
> > > 
> > > [1] https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-flash.html
> > > [2] https://lore.kernel.org/lkml/CAPY8ntB8i4OyUWAL8k899yUd5QsRifJXiOfWXKceGQ7TNZ4OUw@mail.gmail.com/
> > 
> > Right, I think I can see what you're after.
> > 
> > How does the sensor determine when to start the strobe, i.e. on which frame
> > and which part of the exposure of that frame?
> 
> In general I think it's not part of V4L2_CID_FLASH_DURATION to take any
> assumptions on that, as that's sensor/flash specific IMHO.
> 
> In case of the ov9282 sensor driver (which is also part of this series)
> the strobe is started synchronously with the exposure on each frame
> start.
> Being even more specific on the ov9292, the sensor also offers the
> possibility to shift that strobe start in in either direction using a
> register. Implementing this "flash shift" (as it's called in the sensors
> datasheet) is currently under test on my side. I will likely send a
> series for that in the coming weeks.

Ok, so you get a single frame exposed with a flash when you start
streaming, is that correct?

> 
> > > > > Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> > > > > ---
> > > > >  drivers/media/v4l2-core/v4l2-ctrls-defs.c | 1 +
> > > > >  include/uapi/linux/v4l2-controls.h        | 1 +
> > > > >  2 files changed, 2 insertions(+)
> [...]
> > 

-- 
Regards,

Sakari Ailus

Re: [PATCH v2 1/8] media: v4l: ctrls: add a control for flash/strobe duration

[Richard Leitner]

On Tue, Mar 18, 2025 at 01:28:01PM +0000, Sakari Ailus wrote:
> Hi Richard,
> 
> On Fri, Mar 14, 2025 at 05:08:16PM +0100, Richard Leitner wrote:
> > Hi Sakari,
> > 
> > On Fri, Mar 14, 2025 at 01:34:07PM +0000, Sakari Ailus wrote:
> > > Hi Richard,
> > > 
> > > On Fri, Mar 14, 2025 at 11:25:09AM +0100, Richard Leitner wrote:
> > > > On Fri, Mar 14, 2025 at 09:20:23AM +0000, Sakari Ailus wrote:
> > [...]
> > > > > On Fri, Mar 14, 2025 at 09:49:55AM +0100, Richard Leitner wrote:
> > > > > > Add a control V4L2_CID_FLASH_DURATION to set the duration of a
> > > > > > flash/strobe pulse. This is different to the V4L2_CID_FLASH_TIMEOUT
> > > > > > control, as the timeout defines a limit after which the flash is
> > > > > > "forcefully" turned off again.
> > > > > > 
> > > > > > On the other hand the new V4L2_CID_FLASH_DURATION is the desired length
> > > > > > of the flash/strobe pulse
> > > > > 
> > > > > What's the actual difference between the two? To me they appear the same,
> > > > > just expressed in a different way.
> > > > 
> > > > According to FLASH_TIMEOUT documentation:
> > > > 
> > > > 	Hardware timeout for flash. The flash strobe is stopped after this
> > > > 	period of time has passed from the start of the strobe. [1]
> > > > 
> > > > This is a little bit unspecific, but as also discussed with Dave [2]
> > > > according to the documentation of V4L2_FLASH_FAULT_TIMEOUT it seems to
> > > > be targeted at providing a "real timeout" control, not settings the
> > > > desired duration:
> > > > 
> > > > 	The flash strobe was still on when the timeout set by the user
> > > > 	--- V4L2_CID_FLASH_TIMEOUT control --- has expired. Not all flash
> > > > 	controllers may set this in all such conditions. [1]
> > > > 
> > > > If I understood that wrong, I'm also happy to use FLASH_TIMEOUT for this
> > > > use-case. But tbh I think FLASH_DURATION would be more specific.
> > > > 
> > > > As this still seems unclear: Should the documentation be
> > > > changed/rewritten if we stick with the FLASH_DURATION approach?
> > > > 
> > > > [1] https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-flash.html
> > > > [2] https://lore.kernel.org/lkml/CAPY8ntB8i4OyUWAL8k899yUd5QsRifJXiOfWXKceGQ7TNZ4OUw@mail.gmail.com/
> > > 
> > > Right, I think I can see what you're after.
> > > 
> > > How does the sensor determine when to start the strobe, i.e. on which frame
> > > and which part of the exposure of that frame?
> > 
> > In general I think it's not part of V4L2_CID_FLASH_DURATION to take any
> > assumptions on that, as that's sensor/flash specific IMHO.
> > 
> > In case of the ov9282 sensor driver (which is also part of this series)
> > the strobe is started synchronously with the exposure on each frame
> > start.
> > Being even more specific on the ov9292, the sensor also offers the
> > possibility to shift that strobe start in in either direction using a
> > register. Implementing this "flash shift" (as it's called in the sensors
> > datasheet) is currently under test on my side. I will likely send a
> > series for that in the coming weeks.
> 
> Ok, so you get a single frame exposed with a flash when you start
> streaming, is that correct?

Correct. The flash is switched on for the configured duration at every
frame exposure (the sensor has a global shutter) as long as the camera is
streaming.

Maybe to following visualization of configured flash and exposure times help:

             _________        _________        _________
exposure: __|         |______|         |______|         |__

             __               __               __
flash:    __|  |_____________|  |_____________|  |_________
            ^^^^
      strobe_duration


regards;rl

> 
> > 
> > > > > > Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> > > > > > ---
> > > > > >  drivers/media/v4l2-core/v4l2-ctrls-defs.c | 1 +
> > > > > >  include/uapi/linux/v4l2-controls.h        | 1 +
> > > > > >  2 files changed, 2 insertions(+)
> > [...]
> > > 
> 
> -- 
> Regards,
> 
> Sakari Ailus

Re: [PATCH v2 1/8] media: v4l: ctrls: add a control for flash/strobe duration

[Sakari Ailus]

Hi Richard,

On Tue, Mar 18, 2025 at 02:42:53PM +0100, Richard Leitner wrote:
> On Tue, Mar 18, 2025 at 01:28:01PM +0000, Sakari Ailus wrote:
> > Hi Richard,
> > 
> > On Fri, Mar 14, 2025 at 05:08:16PM +0100, Richard Leitner wrote:
> > > Hi Sakari,
> > > 
> > > On Fri, Mar 14, 2025 at 01:34:07PM +0000, Sakari Ailus wrote:
> > > > Hi Richard,
> > > > 
> > > > On Fri, Mar 14, 2025 at 11:25:09AM +0100, Richard Leitner wrote:
> > > > > On Fri, Mar 14, 2025 at 09:20:23AM +0000, Sakari Ailus wrote:
> > > [...]
> > > > > > On Fri, Mar 14, 2025 at 09:49:55AM +0100, Richard Leitner wrote:
> > > > > > > Add a control V4L2_CID_FLASH_DURATION to set the duration of a
> > > > > > > flash/strobe pulse. This is different to the V4L2_CID_FLASH_TIMEOUT
> > > > > > > control, as the timeout defines a limit after which the flash is
> > > > > > > "forcefully" turned off again.
> > > > > > > 
> > > > > > > On the other hand the new V4L2_CID_FLASH_DURATION is the desired length
> > > > > > > of the flash/strobe pulse
> > > > > > 
> > > > > > What's the actual difference between the two? To me they appear the same,
> > > > > > just expressed in a different way.
> > > > > 
> > > > > According to FLASH_TIMEOUT documentation:
> > > > > 
> > > > > 	Hardware timeout for flash. The flash strobe is stopped after this
> > > > > 	period of time has passed from the start of the strobe. [1]
> > > > > 
> > > > > This is a little bit unspecific, but as also discussed with Dave [2]
> > > > > according to the documentation of V4L2_FLASH_FAULT_TIMEOUT it seems to
> > > > > be targeted at providing a "real timeout" control, not settings the
> > > > > desired duration:
> > > > > 
> > > > > 	The flash strobe was still on when the timeout set by the user
> > > > > 	--- V4L2_CID_FLASH_TIMEOUT control --- has expired. Not all flash
> > > > > 	controllers may set this in all such conditions. [1]
> > > > > 
> > > > > If I understood that wrong, I'm also happy to use FLASH_TIMEOUT for this
> > > > > use-case. But tbh I think FLASH_DURATION would be more specific.
> > > > > 
> > > > > As this still seems unclear: Should the documentation be
> > > > > changed/rewritten if we stick with the FLASH_DURATION approach?
> > > > > 
> > > > > [1] https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-flash.html
> > > > > [2] https://lore.kernel.org/lkml/CAPY8ntB8i4OyUWAL8k899yUd5QsRifJXiOfWXKceGQ7TNZ4OUw@mail.gmail.com/
> > > > 
> > > > Right, I think I can see what you're after.
> > > > 
> > > > How does the sensor determine when to start the strobe, i.e. on which frame
> > > > and which part of the exposure of that frame?
> > > 
> > > In general I think it's not part of V4L2_CID_FLASH_DURATION to take any
> > > assumptions on that, as that's sensor/flash specific IMHO.
> > > 
> > > In case of the ov9282 sensor driver (which is also part of this series)
> > > the strobe is started synchronously with the exposure on each frame
> > > start.
> > > Being even more specific on the ov9292, the sensor also offers the
> > > possibility to shift that strobe start in in either direction using a
> > > register. Implementing this "flash shift" (as it's called in the sensors
> > > datasheet) is currently under test on my side. I will likely send a
> > > series for that in the coming weeks.
> > 
> > Ok, so you get a single frame exposed with a flash when you start
> > streaming, is that correct?
> 
> Correct. The flash is switched on for the configured duration at every
> frame exposure (the sensor has a global shutter) as long as the camera is
> streaming.
> 
> Maybe to following visualization of configured flash and exposure times help:
> 
>              _________        _________        _________
> exposure: __|         |______|         |______|         |__
> 
>              __               __               __
> flash:    __|  |_____________|  |_____________|  |_________
>             ^^^^
>       strobe_duration

That diagram would work for global shutter but not for the much, much more
common rolling shutter operation. Does the driver use the sensor in rolling
shutter mode? This isn't very common with LED flashes.

-- 
Regards,

Sakari Ailus

Re: [PATCH v2 1/8] media: v4l: ctrls: add a control for flash/strobe duration

[Richard Leitner]

On Tue, Mar 18, 2025 at 02:06:50PM +0000, Sakari Ailus wrote:
> Hi Richard,
> 
> On Tue, Mar 18, 2025 at 02:42:53PM +0100, Richard Leitner wrote:
> > On Tue, Mar 18, 2025 at 01:28:01PM +0000, Sakari Ailus wrote:
> > > Hi Richard,
> > > 
> > > On Fri, Mar 14, 2025 at 05:08:16PM +0100, Richard Leitner wrote:
> > > > Hi Sakari,
> > > > 
> > > > On Fri, Mar 14, 2025 at 01:34:07PM +0000, Sakari Ailus wrote:
> > > > > Hi Richard,
> > > > > 
> > > > > On Fri, Mar 14, 2025 at 11:25:09AM +0100, Richard Leitner wrote:
> > > > > > On Fri, Mar 14, 2025 at 09:20:23AM +0000, Sakari Ailus wrote:
> > > > [...]
> > > > > > > On Fri, Mar 14, 2025 at 09:49:55AM +0100, Richard Leitner wrote:
> > > > > > > > Add a control V4L2_CID_FLASH_DURATION to set the duration of a
> > > > > > > > flash/strobe pulse. This is different to the V4L2_CID_FLASH_TIMEOUT
> > > > > > > > control, as the timeout defines a limit after which the flash is
> > > > > > > > "forcefully" turned off again.
> > > > > > > > 
> > > > > > > > On the other hand the new V4L2_CID_FLASH_DURATION is the desired length
> > > > > > > > of the flash/strobe pulse
> > > > > > > 
> > > > > > > What's the actual difference between the two? To me they appear the same,
> > > > > > > just expressed in a different way.
> > > > > > 
> > > > > > According to FLASH_TIMEOUT documentation:
> > > > > > 
> > > > > > 	Hardware timeout for flash. The flash strobe is stopped after this
> > > > > > 	period of time has passed from the start of the strobe. [1]
> > > > > > 
> > > > > > This is a little bit unspecific, but as also discussed with Dave [2]
> > > > > > according to the documentation of V4L2_FLASH_FAULT_TIMEOUT it seems to
> > > > > > be targeted at providing a "real timeout" control, not settings the
> > > > > > desired duration:
> > > > > > 
> > > > > > 	The flash strobe was still on when the timeout set by the user
> > > > > > 	--- V4L2_CID_FLASH_TIMEOUT control --- has expired. Not all flash
> > > > > > 	controllers may set this in all such conditions. [1]
> > > > > > 
> > > > > > If I understood that wrong, I'm also happy to use FLASH_TIMEOUT for this
> > > > > > use-case. But tbh I think FLASH_DURATION would be more specific.
> > > > > > 
> > > > > > As this still seems unclear: Should the documentation be
> > > > > > changed/rewritten if we stick with the FLASH_DURATION approach?
> > > > > > 
> > > > > > [1] https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-flash.html
> > > > > > [2] https://lore.kernel.org/lkml/CAPY8ntB8i4OyUWAL8k899yUd5QsRifJXiOfWXKceGQ7TNZ4OUw@mail.gmail.com/
> > > > > 
> > > > > Right, I think I can see what you're after.
> > > > > 
> > > > > How does the sensor determine when to start the strobe, i.e. on which frame
> > > > > and which part of the exposure of that frame?
> > > > 
> > > > In general I think it's not part of V4L2_CID_FLASH_DURATION to take any
> > > > assumptions on that, as that's sensor/flash specific IMHO.
> > > > 
> > > > In case of the ov9282 sensor driver (which is also part of this series)
> > > > the strobe is started synchronously with the exposure on each frame
> > > > start.
> > > > Being even more specific on the ov9292, the sensor also offers the
> > > > possibility to shift that strobe start in in either direction using a
> > > > register. Implementing this "flash shift" (as it's called in the sensors
> > > > datasheet) is currently under test on my side. I will likely send a
> > > > series for that in the coming weeks.
> > > 
> > > Ok, so you get a single frame exposed with a flash when you start
> > > streaming, is that correct?
> > 
> > Correct. The flash is switched on for the configured duration at every
> > frame exposure (the sensor has a global shutter) as long as the camera is
> > streaming.
> > 
> > Maybe to following visualization of configured flash and exposure times help:
> > 
> >              _________        _________        _________
> > exposure: __|         |______|         |______|         |__
> > 
> >              __               __               __
> > flash:    __|  |_____________|  |_____________|  |_________
> >             ^^^^
> >       strobe_duration
> 
> That diagram would work for global shutter but not for the much, much more
> common rolling shutter operation. Does the driver use the sensor in rolling
> shutter mode? This isn't very common with LED flashes.

The ov9282 driver uses the sensor in global shutter mode.

I totally agree with your statement. This pattern is only useful for
global shutter operation.

> 
> -- 
> Regards,
> 
> Sakari Ailus

Re: [PATCH v2 1/8] media: v4l: ctrls: add a control for flash/strobe duration

[Sakari Ailus]

Hi Richard,

On Tue, Mar 18, 2025 at 03:46:18PM +0100, Richard Leitner wrote:
> On Tue, Mar 18, 2025 at 02:06:50PM +0000, Sakari Ailus wrote:
> > Hi Richard,
> > 
> > On Tue, Mar 18, 2025 at 02:42:53PM +0100, Richard Leitner wrote:
> > > On Tue, Mar 18, 2025 at 01:28:01PM +0000, Sakari Ailus wrote:
> > > > Hi Richard,
> > > > 
> > > > On Fri, Mar 14, 2025 at 05:08:16PM +0100, Richard Leitner wrote:
> > > > > Hi Sakari,
> > > > > 
> > > > > On Fri, Mar 14, 2025 at 01:34:07PM +0000, Sakari Ailus wrote:
> > > > > > Hi Richard,
> > > > > > 
> > > > > > On Fri, Mar 14, 2025 at 11:25:09AM +0100, Richard Leitner wrote:
> > > > > > > On Fri, Mar 14, 2025 at 09:20:23AM +0000, Sakari Ailus wrote:
> > > > > [...]
> > > > > > > > On Fri, Mar 14, 2025 at 09:49:55AM +0100, Richard Leitner wrote:
> > > > > > > > > Add a control V4L2_CID_FLASH_DURATION to set the duration of a
> > > > > > > > > flash/strobe pulse. This is different to the V4L2_CID_FLASH_TIMEOUT
> > > > > > > > > control, as the timeout defines a limit after which the flash is
> > > > > > > > > "forcefully" turned off again.
> > > > > > > > > 
> > > > > > > > > On the other hand the new V4L2_CID_FLASH_DURATION is the desired length
> > > > > > > > > of the flash/strobe pulse
> > > > > > > > 
> > > > > > > > What's the actual difference between the two? To me they appear the same,
> > > > > > > > just expressed in a different way.
> > > > > > > 
> > > > > > > According to FLASH_TIMEOUT documentation:
> > > > > > > 
> > > > > > > 	Hardware timeout for flash. The flash strobe is stopped after this
> > > > > > > 	period of time has passed from the start of the strobe. [1]
> > > > > > > 
> > > > > > > This is a little bit unspecific, but as also discussed with Dave [2]
> > > > > > > according to the documentation of V4L2_FLASH_FAULT_TIMEOUT it seems to
> > > > > > > be targeted at providing a "real timeout" control, not settings the
> > > > > > > desired duration:
> > > > > > > 
> > > > > > > 	The flash strobe was still on when the timeout set by the user
> > > > > > > 	--- V4L2_CID_FLASH_TIMEOUT control --- has expired. Not all flash
> > > > > > > 	controllers may set this in all such conditions. [1]
> > > > > > > 
> > > > > > > If I understood that wrong, I'm also happy to use FLASH_TIMEOUT for this
> > > > > > > use-case. But tbh I think FLASH_DURATION would be more specific.
> > > > > > > 
> > > > > > > As this still seems unclear: Should the documentation be
> > > > > > > changed/rewritten if we stick with the FLASH_DURATION approach?
> > > > > > > 
> > > > > > > [1] https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-flash.html
> > > > > > > [2] https://lore.kernel.org/lkml/CAPY8ntB8i4OyUWAL8k899yUd5QsRifJXiOfWXKceGQ7TNZ4OUw@mail.gmail.com/
> > > > > > 
> > > > > > Right, I think I can see what you're after.
> > > > > > 
> > > > > > How does the sensor determine when to start the strobe, i.e. on which frame
> > > > > > and which part of the exposure of that frame?
> > > > > 
> > > > > In general I think it's not part of V4L2_CID_FLASH_DURATION to take any
> > > > > assumptions on that, as that's sensor/flash specific IMHO.
> > > > > 
> > > > > In case of the ov9282 sensor driver (which is also part of this series)
> > > > > the strobe is started synchronously with the exposure on each frame
> > > > > start.
> > > > > Being even more specific on the ov9292, the sensor also offers the
> > > > > possibility to shift that strobe start in in either direction using a
> > > > > register. Implementing this "flash shift" (as it's called in the sensors
> > > > > datasheet) is currently under test on my side. I will likely send a
> > > > > series for that in the coming weeks.
> > > > 
> > > > Ok, so you get a single frame exposed with a flash when you start
> > > > streaming, is that correct?
> > > 
> > > Correct. The flash is switched on for the configured duration at every
> > > frame exposure (the sensor has a global shutter) as long as the camera is
> > > streaming.
> > > 
> > > Maybe to following visualization of configured flash and exposure times help:
> > > 
> > >              _________        _________        _________
> > > exposure: __|         |______|         |______|         |__
> > > 
> > >              __               __               __
> > > flash:    __|  |_____________|  |_____________|  |_________
> > >             ^^^^
> > >       strobe_duration
> > 
> > That diagram would work for global shutter but not for the much, much more
> > common rolling shutter operation. Does the driver use the sensor in rolling
> > shutter mode? This isn't very common with LED flashes.
> 
> The ov9282 driver uses the sensor in global shutter mode.
> 
> I totally agree with your statement. This pattern is only useful for
> global shutter operation.

I think (nearly?) all supported sensors use a rolling shutter.

Could you include a comment on this to the driver?

I wonder what Laurent thinks.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v2 1/8] media: v4l: ctrls: add a control for flash/strobe duration

[Dave Stevenson]

Hi Sakari

On Tue, 18 Mar 2025 at 15:11, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
>
> Hi Richard,
>
> On Tue, Mar 18, 2025 at 03:46:18PM +0100, Richard Leitner wrote:
> > On Tue, Mar 18, 2025 at 02:06:50PM +0000, Sakari Ailus wrote:
> > > Hi Richard,
> > >
> > > On Tue, Mar 18, 2025 at 02:42:53PM +0100, Richard Leitner wrote:
> > > > On Tue, Mar 18, 2025 at 01:28:01PM +0000, Sakari Ailus wrote:
> > > > > Hi Richard,
> > > > >
> > > > > On Fri, Mar 14, 2025 at 05:08:16PM +0100, Richard Leitner wrote:
> > > > > > Hi Sakari,
> > > > > >
> > > > > > On Fri, Mar 14, 2025 at 01:34:07PM +0000, Sakari Ailus wrote:
> > > > > > > Hi Richard,
> > > > > > >
> > > > > > > On Fri, Mar 14, 2025 at 11:25:09AM +0100, Richard Leitner wrote:
> > > > > > > > On Fri, Mar 14, 2025 at 09:20:23AM +0000, Sakari Ailus wrote:
> > > > > > [...]
> > > > > > > > > On Fri, Mar 14, 2025 at 09:49:55AM +0100, Richard Leitner wrote:
> > > > > > > > > > Add a control V4L2_CID_FLASH_DURATION to set the duration of a
> > > > > > > > > > flash/strobe pulse. This is different to the V4L2_CID_FLASH_TIMEOUT
> > > > > > > > > > control, as the timeout defines a limit after which the flash is
> > > > > > > > > > "forcefully" turned off again.
> > > > > > > > > >
> > > > > > > > > > On the other hand the new V4L2_CID_FLASH_DURATION is the desired length
> > > > > > > > > > of the flash/strobe pulse
> > > > > > > > >
> > > > > > > > > What's the actual difference between the two? To me they appear the same,
> > > > > > > > > just expressed in a different way.
> > > > > > > >
> > > > > > > > According to FLASH_TIMEOUT documentation:
> > > > > > > >
> > > > > > > >   Hardware timeout for flash. The flash strobe is stopped after this
> > > > > > > >   period of time has passed from the start of the strobe. [1]
> > > > > > > >
> > > > > > > > This is a little bit unspecific, but as also discussed with Dave [2]
> > > > > > > > according to the documentation of V4L2_FLASH_FAULT_TIMEOUT it seems to
> > > > > > > > be targeted at providing a "real timeout" control, not settings the
> > > > > > > > desired duration:
> > > > > > > >
> > > > > > > >   The flash strobe was still on when the timeout set by the user
> > > > > > > >   --- V4L2_CID_FLASH_TIMEOUT control --- has expired. Not all flash
> > > > > > > >   controllers may set this in all such conditions. [1]
> > > > > > > >
> > > > > > > > If I understood that wrong, I'm also happy to use FLASH_TIMEOUT for this
> > > > > > > > use-case. But tbh I think FLASH_DURATION would be more specific.
> > > > > > > >
> > > > > > > > As this still seems unclear: Should the documentation be
> > > > > > > > changed/rewritten if we stick with the FLASH_DURATION approach?
> > > > > > > >
> > > > > > > > [1] https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-flash.html
> > > > > > > > [2] https://lore.kernel.org/lkml/CAPY8ntB8i4OyUWAL8k899yUd5QsRifJXiOfWXKceGQ7TNZ4OUw@mail.gmail.com/
> > > > > > >
> > > > > > > Right, I think I can see what you're after.
> > > > > > >
> > > > > > > How does the sensor determine when to start the strobe, i.e. on which frame
> > > > > > > and which part of the exposure of that frame?
> > > > > >
> > > > > > In general I think it's not part of V4L2_CID_FLASH_DURATION to take any
> > > > > > assumptions on that, as that's sensor/flash specific IMHO.
> > > > > >
> > > > > > In case of the ov9282 sensor driver (which is also part of this series)
> > > > > > the strobe is started synchronously with the exposure on each frame
> > > > > > start.
> > > > > > Being even more specific on the ov9292, the sensor also offers the
> > > > > > possibility to shift that strobe start in in either direction using a
> > > > > > register. Implementing this "flash shift" (as it's called in the sensors
> > > > > > datasheet) is currently under test on my side. I will likely send a
> > > > > > series for that in the coming weeks.
> > > > >
> > > > > Ok, so you get a single frame exposed with a flash when you start
> > > > > streaming, is that correct?
> > > >
> > > > Correct. The flash is switched on for the configured duration at every
> > > > frame exposure (the sensor has a global shutter) as long as the camera is
> > > > streaming.
> > > >
> > > > Maybe to following visualization of configured flash and exposure times help:
> > > >
> > > >              _________        _________        _________
> > > > exposure: __|         |______|         |______|         |__
> > > >
> > > >              __               __               __
> > > > flash:    __|  |_____________|  |_____________|  |_________
> > > >             ^^^^
> > > >       strobe_duration
> > >
> > > That diagram would work for global shutter but not for the much, much more
> > > common rolling shutter operation. Does the driver use the sensor in rolling
> > > shutter mode? This isn't very common with LED flashes.
> >
> > The ov9282 driver uses the sensor in global shutter mode.
> >
> > I totally agree with your statement. This pattern is only useful for
> > global shutter operation.
>
> I think (nearly?) all supported sensors use a rolling shutter.

You've got at least two other global shutter sensors supported in
mainline - Omnivision ov7251 and Sony imx296.
Patches have been posted for OnSemi ar0144 (Laurent) and ar0234
(Dongcheng), which are also both global shutter sensors.

So yes they are in the minority, but not that uncommon.

  Dave



> Could you include a comment on this to the driver?
>
> I wonder what Laurent thinks.
>
> --
> Kind regards,
>
> Sakari Ailus

Re: [PATCH v2 1/8] media: v4l: ctrls: add a control for flash/strobe duration

[Sakari Ailus]

Hi Dave,

On Tue, Mar 18, 2025 at 04:39:05PM +0000, Dave Stevenson wrote:
> Hi Sakari
> 
> On Tue, 18 Mar 2025 at 15:11, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Richard,
> >
> > On Tue, Mar 18, 2025 at 03:46:18PM +0100, Richard Leitner wrote:
> > > On Tue, Mar 18, 2025 at 02:06:50PM +0000, Sakari Ailus wrote:
> > > > Hi Richard,
> > > >
> > > > On Tue, Mar 18, 2025 at 02:42:53PM +0100, Richard Leitner wrote:
> > > > > On Tue, Mar 18, 2025 at 01:28:01PM +0000, Sakari Ailus wrote:
> > > > > > Hi Richard,
> > > > > >
> > > > > > On Fri, Mar 14, 2025 at 05:08:16PM +0100, Richard Leitner wrote:
> > > > > > > Hi Sakari,
> > > > > > >
> > > > > > > On Fri, Mar 14, 2025 at 01:34:07PM +0000, Sakari Ailus wrote:
> > > > > > > > Hi Richard,
> > > > > > > >
> > > > > > > > On Fri, Mar 14, 2025 at 11:25:09AM +0100, Richard Leitner wrote:
> > > > > > > > > On Fri, Mar 14, 2025 at 09:20:23AM +0000, Sakari Ailus wrote:
> > > > > > > [...]
> > > > > > > > > > On Fri, Mar 14, 2025 at 09:49:55AM +0100, Richard Leitner wrote:
> > > > > > > > > > > Add a control V4L2_CID_FLASH_DURATION to set the duration of a
> > > > > > > > > > > flash/strobe pulse. This is different to the V4L2_CID_FLASH_TIMEOUT
> > > > > > > > > > > control, as the timeout defines a limit after which the flash is
> > > > > > > > > > > "forcefully" turned off again.
> > > > > > > > > > >
> > > > > > > > > > > On the other hand the new V4L2_CID_FLASH_DURATION is the desired length
> > > > > > > > > > > of the flash/strobe pulse
> > > > > > > > > >
> > > > > > > > > > What's the actual difference between the two? To me they appear the same,
> > > > > > > > > > just expressed in a different way.
> > > > > > > > >
> > > > > > > > > According to FLASH_TIMEOUT documentation:
> > > > > > > > >
> > > > > > > > >   Hardware timeout for flash. The flash strobe is stopped after this
> > > > > > > > >   period of time has passed from the start of the strobe. [1]
> > > > > > > > >
> > > > > > > > > This is a little bit unspecific, but as also discussed with Dave [2]
> > > > > > > > > according to the documentation of V4L2_FLASH_FAULT_TIMEOUT it seems to
> > > > > > > > > be targeted at providing a "real timeout" control, not settings the
> > > > > > > > > desired duration:
> > > > > > > > >
> > > > > > > > >   The flash strobe was still on when the timeout set by the user
> > > > > > > > >   --- V4L2_CID_FLASH_TIMEOUT control --- has expired. Not all flash
> > > > > > > > >   controllers may set this in all such conditions. [1]
> > > > > > > > >
> > > > > > > > > If I understood that wrong, I'm also happy to use FLASH_TIMEOUT for this
> > > > > > > > > use-case. But tbh I think FLASH_DURATION would be more specific.
> > > > > > > > >
> > > > > > > > > As this still seems unclear: Should the documentation be
> > > > > > > > > changed/rewritten if we stick with the FLASH_DURATION approach?
> > > > > > > > >
> > > > > > > > > [1] https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-flash.html
> > > > > > > > > [2] https://lore.kernel.org/lkml/CAPY8ntB8i4OyUWAL8k899yUd5QsRifJXiOfWXKceGQ7TNZ4OUw@mail.gmail.com/
> > > > > > > >
> > > > > > > > Right, I think I can see what you're after.
> > > > > > > >
> > > > > > > > How does the sensor determine when to start the strobe, i.e. on which frame
> > > > > > > > and which part of the exposure of that frame?
> > > > > > >
> > > > > > > In general I think it's not part of V4L2_CID_FLASH_DURATION to take any
> > > > > > > assumptions on that, as that's sensor/flash specific IMHO.
> > > > > > >
> > > > > > > In case of the ov9282 sensor driver (which is also part of this series)
> > > > > > > the strobe is started synchronously with the exposure on each frame
> > > > > > > start.
> > > > > > > Being even more specific on the ov9292, the sensor also offers the
> > > > > > > possibility to shift that strobe start in in either direction using a
> > > > > > > register. Implementing this "flash shift" (as it's called in the sensors
> > > > > > > datasheet) is currently under test on my side. I will likely send a
> > > > > > > series for that in the coming weeks.
> > > > > >
> > > > > > Ok, so you get a single frame exposed with a flash when you start
> > > > > > streaming, is that correct?
> > > > >
> > > > > Correct. The flash is switched on for the configured duration at every
> > > > > frame exposure (the sensor has a global shutter) as long as the camera is
> > > > > streaming.
> > > > >
> > > > > Maybe to following visualization of configured flash and exposure times help:
> > > > >
> > > > >              _________        _________        _________
> > > > > exposure: __|         |______|         |______|         |__
> > > > >
> > > > >              __               __               __
> > > > > flash:    __|  |_____________|  |_____________|  |_________
> > > > >             ^^^^
> > > > >       strobe_duration
> > > >
> > > > That diagram would work for global shutter but not for the much, much more
> > > > common rolling shutter operation. Does the driver use the sensor in rolling
> > > > shutter mode? This isn't very common with LED flashes.
> > >
> > > The ov9282 driver uses the sensor in global shutter mode.
> > >
> > > I totally agree with your statement. This pattern is only useful for
> > > global shutter operation.
> >
> > I think (nearly?) all supported sensors use a rolling shutter.
> 
> You've got at least two other global shutter sensors supported in
> mainline - Omnivision ov7251 and Sony imx296.
> Patches have been posted for OnSemi ar0144 (Laurent) and ar0234
> (Dongcheng), which are also both global shutter sensors.
> 
> So yes they are in the minority, but not that uncommon.

Thanks for the list. I briefly discussed this with Laurent and I agree with
him this information would probably be best kept with userspace (libcamera)
without trying to enumerate it from the kernel (albeit CCS might be an
exception, but that's an entirely different discussion then). Only changing
the global/rolling configuration would require a control.

-- 
Regards,

Sakari Ailus

Re: [PATCH v2 1/8] media: v4l: ctrls: add a control for flash/strobe duration

[Richard Leitner]

Hi Dave, Hi Sakari

On Wed, Mar 19, 2025 at 10:06:28AM +0000, Sakari Ailus wrote:
> Hi Dave,
> 
> On Tue, Mar 18, 2025 at 04:39:05PM +0000, Dave Stevenson wrote:
> > Hi Sakari
> > 
> > On Tue, 18 Mar 2025 at 15:11, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> > >
> > > Hi Richard,
> > >
> > > On Tue, Mar 18, 2025 at 03:46:18PM +0100, Richard Leitner wrote:

...

> > > > The ov9282 driver uses the sensor in global shutter mode.
> > > >
> > > > I totally agree with your statement. This pattern is only useful for
> > > > global shutter operation.
> > >
> > > I think (nearly?) all supported sensors use a rolling shutter.
> > 
> > You've got at least two other global shutter sensors supported in
> > mainline - Omnivision ov7251 and Sony imx296.
> > Patches have been posted for OnSemi ar0144 (Laurent) and ar0234
> > (Dongcheng), which are also both global shutter sensors.
> > 
> > So yes they are in the minority, but not that uncommon.
> 
> Thanks for the list. I briefly discussed this with Laurent and I agree with
> him this information would probably be best kept with userspace (libcamera)
> without trying to enumerate it from the kernel (albeit CCS might be an
> exception, but that's an entirely different discussion then). Only changing
> the global/rolling configuration would require a control.

Thanks for the feeback and clarification!

So am I understanding this correctly that the flash/strobe duration
approach in this series is basically fine?

If so I will send a v3 later today.

regards;rl

> 
> -- 
> Regards,
> 
> Sakari Ailus

Re: [PATCH v2 4/8] Documentation: uAPI: media: add V4L2_CID_FLASH_DURATION

[Richard Leitner]

Hi Hans, Hi Sakari,

On Fri, Mar 14, 2025 at 11:36:12AM +0100, Hans Verkuil wrote:
> On 14/03/2025 11:28, Richard Leitner wrote:
> > Hi Hans,
> > 
> > thanks for your quick feedback!
> > 
> > On Fri, Mar 14, 2025 at 10:41:04AM +0100, Hans Verkuil wrote:
> >> On 14/03/2025 09:49, Richard Leitner wrote:
> >>> Add the new strobe_duration control to v4l uAPI documentation.
> >>>
> >>> Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> >>> ---
> >>>  Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst | 5 +++++
> >>>  1 file changed, 5 insertions(+)
> >>>
> >>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> >>> index d22c5efb806a183a3ad67ec3e6550b002a51659a..03a58ef94be7c870f55d5a9bb09503995dbfb402 100644
> >>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> >>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> >>> @@ -186,3 +186,8 @@ Flash Control IDs
> >>>      charged before strobing. LED flashes often require a cooldown period
> >>>      after strobe during which another strobe will not be possible. This
> >>>      is a read-only control.
> >>> +
> >>> +``V4L2_CID_FLASH_DURATION (integer)``
> >>> +    Duration the flash should be on when the flash LED is in flash mode
> >>> +    (V4L2_FLASH_LED_MODE_FLASH). The unit should be microseconds (µs)
> >>> +    if possible.
> >>>
> >>
> >> If this control is present, does that mean that the flash duration always have
> >> to be set manually? Or can there be an 'Auto' mode as well? And if so, how is
> >> that set?
> > 
> > To be honest I haven't thought about automatic flash duration. Is this
> > something which is required?
> 
> No idea, it was just something I was wondering about. Sakari probably knows a lot
> more about this.

Sakari, should I add something like an auto/manual flash duration
control to this series?

Personally I think as long as we have no user of such an "auto" control
it's not really necessary. Or are there any drivers doing "auto"
flash/strobe duration already?

Thanks!

regards;rl

> 
> Regards,
> 
> 	Hans
> 
> > 
> > At least for the ov9282 sensor (which I've implemented this control for
> > in this series) there is no "auto" mode AFAIK.
> > 
> > If it's required: What would be the best solution?
> > Extending V4L2_CID_FLASH_LED_MODE with a new menu option? E.g.
> > V4L2_FLASH_LED_MODE_FLASH_{MANUAL,AUTO}?
> > 
> >>
> >> Regards,
> >>
> >> 	Hans
> > 
> > Thanks!
> > Richard
> 

Re: [PATCH v2 4/8] Documentation: uAPI: media: add V4L2_CID_FLASH_DURATION

[Sakari Ailus]

Hi Richard,

On Tue, Mar 25, 2025 at 09:24:18AM +0100, Richard Leitner wrote:
> Hi Hans, Hi Sakari,
> 
> On Fri, Mar 14, 2025 at 11:36:12AM +0100, Hans Verkuil wrote:
> > On 14/03/2025 11:28, Richard Leitner wrote:
> > > Hi Hans,
> > > 
> > > thanks for your quick feedback!
> > > 
> > > On Fri, Mar 14, 2025 at 10:41:04AM +0100, Hans Verkuil wrote:
> > >> On 14/03/2025 09:49, Richard Leitner wrote:
> > >>> Add the new strobe_duration control to v4l uAPI documentation.
> > >>>
> > >>> Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> > >>> ---
> > >>>  Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst | 5 +++++
> > >>>  1 file changed, 5 insertions(+)
> > >>>
> > >>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> > >>> index d22c5efb806a183a3ad67ec3e6550b002a51659a..03a58ef94be7c870f55d5a9bb09503995dbfb402 100644
> > >>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> > >>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> > >>> @@ -186,3 +186,8 @@ Flash Control IDs
> > >>>      charged before strobing. LED flashes often require a cooldown period
> > >>>      after strobe during which another strobe will not be possible. This
> > >>>      is a read-only control.
> > >>> +
> > >>> +``V4L2_CID_FLASH_DURATION (integer)``
> > >>> +    Duration the flash should be on when the flash LED is in flash mode
> > >>> +    (V4L2_FLASH_LED_MODE_FLASH). The unit should be microseconds (µs)
> > >>> +    if possible.
> > >>>
> > >>
> > >> If this control is present, does that mean that the flash duration always have
> > >> to be set manually? Or can there be an 'Auto' mode as well? And if so, how is
> > >> that set?
> > > 
> > > To be honest I haven't thought about automatic flash duration. Is this
> > > something which is required?
> > 
> > No idea, it was just something I was wondering about. Sakari probably knows a lot
> > more about this.
> 
> Sakari, should I add something like an auto/manual flash duration
> control to this series?
> 
> Personally I think as long as we have no user of such an "auto" control
> it's not really necessary. Or are there any drivers doing "auto"
> flash/strobe duration already?

I think the only other drivers that support controlling the flash currently
are CCS and vgxy61. The CCS calculates the timing in the driver and the
vgxy61 appears to be doing that all in firmware.

I'd add a control to select between auto / manual if there's a need to
support both. CCS could be a case for this as the user space might know
better what it wants from the flash.

-- 
Regards,

Sakari Ailus

Re: [PATCH v2 1/8] media: v4l: ctrls: add a control for flash/strobe duration

[Sakari Ailus]

Hi Richard, Dave,

On Tue, Mar 25, 2025 at 09:20:29AM +0100, Richard Leitner wrote:
> Hi Dave, Hi Sakari
> 
> On Wed, Mar 19, 2025 at 10:06:28AM +0000, Sakari Ailus wrote:
> > Hi Dave,
> > 
> > On Tue, Mar 18, 2025 at 04:39:05PM +0000, Dave Stevenson wrote:
> > > Hi Sakari
> > > 
> > > On Tue, 18 Mar 2025 at 15:11, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> > > >
> > > > Hi Richard,
> > > >
> > > > On Tue, Mar 18, 2025 at 03:46:18PM +0100, Richard Leitner wrote:
> 
> ...
> 
> > > > > The ov9282 driver uses the sensor in global shutter mode.
> > > > >
> > > > > I totally agree with your statement. This pattern is only useful for
> > > > > global shutter operation.
> > > >
> > > > I think (nearly?) all supported sensors use a rolling shutter.
> > > 
> > > You've got at least two other global shutter sensors supported in
> > > mainline - Omnivision ov7251 and Sony imx296.
> > > Patches have been posted for OnSemi ar0144 (Laurent) and ar0234
> > > (Dongcheng), which are also both global shutter sensors.
> > > 
> > > So yes they are in the minority, but not that uncommon.
> > 
> > Thanks for the list. I briefly discussed this with Laurent and I agree with
> > him this information would probably be best kept with userspace (libcamera)
> > without trying to enumerate it from the kernel (albeit CCS might be an
> > exception, but that's an entirely different discussion then). Only changing
> > the global/rolling configuration would require a control.
> 
> Thanks for the feeback and clarification!
> 
> So am I understanding this correctly that the flash/strobe duration
> approach in this series is basically fine?

I'd think so, yes.

> 
> If so I will send a v3 later today.

-- 
Regards,

Sakari Ailus