[PATCH v7 04/10] Documentation: uAPI: media: add V4L2_CID_FLASH_{DURATION,HW_STROBE_SIGNAL}

Richard Leitner posted 10 patches 1 month ago
[PATCH v7 04/10] Documentation: uAPI: media: add V4L2_CID_FLASH_{DURATION,HW_STROBE_SIGNAL}
Posted by Richard Leitner 1 month ago
Add the new strobe duration and hardware strobe signal control to v4l
uAPI documentation. Additionally add labels for cross-referencing v4l
controls.

Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
---
 .../userspace-api/media/v4l/ext-ctrls-flash.rst    | 29 ++++++++++++++++++++++
 1 file changed, 29 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..6254420a8ca95929d23ffdc65f40a6e53e30a635 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
@@ -57,6 +57,8 @@ Flash Control IDs
 ``V4L2_CID_FLASH_CLASS (class)``
     The FLASH class descriptor.
 
+.. _v4l2-cid-flash-led-mode:
+
 ``V4L2_CID_FLASH_LED_MODE (menu)``
     Defines the mode of the flash LED, the high-power white LED attached
     to the flash controller. Setting this control may not be possible in
@@ -80,6 +82,8 @@ Flash Control IDs
 
 
 
+.. _v4l2-cid-flash-strobe-source:
+
 ``V4L2_CID_FLASH_STROBE_SOURCE (menu)``
     Defines the source of the flash LED strobe.
 
@@ -186,3 +190,28 @@ 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:
+
+``V4L2_CID_FLASH_DURATION (integer)``
+    Duration of the flash strobe pulse generated by the strobe source,
+    typically a camera sensor. This method of controlling flash LED strobe
+    duration has three prerequisites: the strobe source's
+    :ref:`hardware strobe signal <v4l2-cid-flash-hw-strobe-signal>` must be
+    enabled, the flash LED driver's :ref:`flash LED mode <v4l2-cid-flash-led-mode>`
+    must be set to ``V4L2_FLASH_LED_MODE_FLASH``, and the
+    :ref:`strobe source <v4l2-cid-flash-strobe-source>` must be configured to
+    ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``. The unit should be microseconds (µs)
+    if possible.
+
+.. _v4l2-cid-flash-hw-strobe-signal:
+
+``V4L2_CID_FLASH_HW_STROBE_SIGNAL (boolean)``
+    Enables the output of a hardware strobe signal from the strobe source,
+    typically a camera sensor. To control a flash LED driver connected to this
+    hardware signal, the :ref:`flash LED mode <v4l2-cid-flash-led-mode>`
+    must be set to ``V4L2_FLASH_LED_MODE_FLASH`` and the
+    :ref:`strobe source <v4l2-cid-flash-strobe-source>` must be set to
+    ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``. Provided the flash LED driver
+    supports it, the length of the strobe signal can be configured by
+    adjusting its :ref:`flash duration <v4l2-cid-flash-duration>`.

-- 
2.47.2


Re: [PATCH v7 04/10] Documentation: uAPI: media: add V4L2_CID_FLASH_{DURATION,HW_STROBE_SIGNAL}
Posted by Laurent Pinchart 3 weeks, 4 days ago
Hi Richard,

Thank you for the patch.

On Mon, Sep 01, 2025 at 05:05:09PM +0200, Richard Leitner wrote:
> Add the new strobe duration and hardware strobe signal control to v4l
> uAPI documentation. Additionally add labels for cross-referencing v4l
> controls.
> 
> Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> ---
>  .../userspace-api/media/v4l/ext-ctrls-flash.rst    | 29 ++++++++++++++++++++++
>  1 file changed, 29 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..6254420a8ca95929d23ffdc65f40a6e53e30a635 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> @@ -57,6 +57,8 @@ Flash Control IDs
>  ``V4L2_CID_FLASH_CLASS (class)``
>      The FLASH class descriptor.
>  
> +.. _v4l2-cid-flash-led-mode:
> +
>  ``V4L2_CID_FLASH_LED_MODE (menu)``
>      Defines the mode of the flash LED, the high-power white LED attached
>      to the flash controller. Setting this control may not be possible in
> @@ -80,6 +82,8 @@ Flash Control IDs
>  
>  
>  
> +.. _v4l2-cid-flash-strobe-source:
> +
>  ``V4L2_CID_FLASH_STROBE_SOURCE (menu)``
>      Defines the source of the flash LED strobe.
>  
> @@ -186,3 +190,28 @@ 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:
> +
> +``V4L2_CID_FLASH_DURATION (integer)``
> +    Duration of the flash strobe pulse generated by the strobe source,
> +    typically a camera sensor. This method of controlling flash LED strobe
> +    duration has three prerequisites: the strobe source's
> +    :ref:`hardware strobe signal <v4l2-cid-flash-hw-strobe-signal>` must be
> +    enabled, the flash LED driver's :ref:`flash LED mode <v4l2-cid-flash-led-mode>`
> +    must be set to ``V4L2_FLASH_LED_MODE_FLASH``, and the
> +    :ref:`strobe source <v4l2-cid-flash-strobe-source>` must be configured to
> +    ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``. The unit should be microseconds (µs)
> +    if possible.

As mentioned in the review of 01/10, I think this needs to be clarified.
Ideally we should add a new document in
Documentation/userspace-api/media/v4l/ to explain the flash API, but in
the meantime let's at lets improve the description of the duration
control. Here's a proposal.

``V4L2_CID_FLASH_DURATION (integer)``
    Duration of the flash strobe pulse generated by the strobe source, when
    using external strobe. This control shall be implemented by the device
    generating the hardware flash strobe signal, typically a camera sensor,
    connected to a flash controller. It must not be implemented by the flash
    controller.

    This method of controlling flash LED strobe duration has three
    prerequisites: the strobe source's :ref:`hardware strobe signal
    <v4l2-cid-flash-hw-strobe-signal>` must be enabled, the flash controller's
    :ref:`flash LED mode <v4l2-cid-flash-led-mode>` must be set to
    ``V4L2_FLASH_LED_MODE_FLASH``, and its :ref:`strobe source
    <v4l2-cid-flash-strobe-source>` must be configured to
    ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``.

    The unit should be microseconds (µs) if possible.


The second paragraph may be better replaced by expanding the
documentation of V4L2_FLASH_STROBE_SOURCE_EXTERNAL, it seems a better
place to document how external strobe works.

As for the unit, is microseconds really the best option ? I would expect
most sensors to express the strobe pulse width in unit of lines.

I think we also need to decide how to handle camera sensors whose flash
strobe pulse width can't be controlled. For instance, the AR0144 can
output a flash signal, and its width is always equal to the exposure
time. The most straightforward solution seems to implement
V4L2_CID_FLASH_HW_STROBE_SIGNAL but not V4L2_CID_FLASH_DURATION in the
sensor driver. Could this cause issues in any use case ? Is there a
better solution ? I would like this to be documented.

Finally, I think we also need to standardize the flash strobe offset.

> +
> +.. _v4l2-cid-flash-hw-strobe-signal:
> +
> +``V4L2_CID_FLASH_HW_STROBE_SIGNAL (boolean)``

Nitpicking a bit on the name, I would have called this
V4L2_CID_FLASH_STROBE_OUTPUT_ENABLE (or _OE).

> +    Enables the output of a hardware strobe signal from the strobe source,
> +    typically a camera sensor. To control a flash LED driver connected to this
> +    hardware signal, the :ref:`flash LED mode <v4l2-cid-flash-led-mode>`
> +    must be set to ``V4L2_FLASH_LED_MODE_FLASH`` and the
> +    :ref:`strobe source <v4l2-cid-flash-strobe-source>` must be set to
> +    ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``. Provided the flash LED driver
> +    supports it, the length of the strobe signal can be configured by
> +    adjusting its :ref:`flash duration <v4l2-cid-flash-duration>`.

The V4L2_CID_FLASH_HW_STROBE_SIGNAL documentation needs to be clarified
in a similar way as V4L2_CID_FLASH_DURATION.

-- 
Regards,

Laurent Pinchart
Re: [PATCH v7 04/10] Documentation: uAPI: media: add V4L2_CID_FLASH_{DURATION,HW_STROBE_SIGNAL}
Posted by Richard Leitner 3 weeks, 3 days ago
Hi Laurent,

thanks for reviewing this!

On Sun, Sep 07, 2025 at 09:49:53PM +0200, Laurent Pinchart wrote:
> Hi Richard,
> 
> Thank you for the patch.
> 
> On Mon, Sep 01, 2025 at 05:05:09PM +0200, Richard Leitner wrote:
> > Add the new strobe duration and hardware strobe signal control to v4l
> > uAPI documentation. Additionally add labels for cross-referencing v4l
> > controls.
> > 
> > Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> > ---
> >  .../userspace-api/media/v4l/ext-ctrls-flash.rst    | 29 ++++++++++++++++++++++
> >  1 file changed, 29 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..6254420a8ca95929d23ffdc65f40a6e53e30a635 100644
> > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> > @@ -57,6 +57,8 @@ Flash Control IDs
> >  ``V4L2_CID_FLASH_CLASS (class)``
> >      The FLASH class descriptor.
> >  
> > +.. _v4l2-cid-flash-led-mode:
> > +
> >  ``V4L2_CID_FLASH_LED_MODE (menu)``
> >      Defines the mode of the flash LED, the high-power white LED attached
> >      to the flash controller. Setting this control may not be possible in
> > @@ -80,6 +82,8 @@ Flash Control IDs
> >  
> >  
> >  
> > +.. _v4l2-cid-flash-strobe-source:
> > +
> >  ``V4L2_CID_FLASH_STROBE_SOURCE (menu)``
> >      Defines the source of the flash LED strobe.
> >  
> > @@ -186,3 +190,28 @@ 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:
> > +
> > +``V4L2_CID_FLASH_DURATION (integer)``
> > +    Duration of the flash strobe pulse generated by the strobe source,
> > +    typically a camera sensor. This method of controlling flash LED strobe
> > +    duration has three prerequisites: the strobe source's
> > +    :ref:`hardware strobe signal <v4l2-cid-flash-hw-strobe-signal>` must be
> > +    enabled, the flash LED driver's :ref:`flash LED mode <v4l2-cid-flash-led-mode>`
> > +    must be set to ``V4L2_FLASH_LED_MODE_FLASH``, and the
> > +    :ref:`strobe source <v4l2-cid-flash-strobe-source>` must be configured to
> > +    ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``. The unit should be microseconds (µs)
> > +    if possible.
> 
> As mentioned in the review of 01/10, I think this needs to be clarified.
> Ideally we should add a new document in
> Documentation/userspace-api/media/v4l/ to explain the flash API, but in
> the meantime let's at lets improve the description of the duration
> control. Here's a proposal.

Understood. Thank you for your proposal!

> 
> ``V4L2_CID_FLASH_DURATION (integer)``
>     Duration of the flash strobe pulse generated by the strobe source, when
>     using external strobe. This control shall be implemented by the device
>     generating the hardware flash strobe signal, typically a camera sensor,
>     connected to a flash controller. It must not be implemented by the flash
>     controller.
> 
>     This method of controlling flash LED strobe duration has three
>     prerequisites: the strobe source's :ref:`hardware strobe signal
>     <v4l2-cid-flash-hw-strobe-signal>` must be enabled, the flash controller's
>     :ref:`flash LED mode <v4l2-cid-flash-led-mode>` must be set to
>     ``V4L2_FLASH_LED_MODE_FLASH``, and its :ref:`strobe source
>     <v4l2-cid-flash-strobe-source>` must be configured to
>     ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``.
> 
>     The unit should be microseconds (µs) if possible.
> 
> 
> The second paragraph may be better replaced by expanding the
> documentation of V4L2_FLASH_STROBE_SOURCE_EXTERNAL, it seems a better
> place to document how external strobe works.

That's fine for me. I will adapt the V4L2_CID_FLASH_DURATION and
V4L2_FLASH_STROBE_SOURCE_EXTERNAL documentation accordingly and send in
v9.

> 
> As for the unit, is microseconds really the best option ? I would expect
> most sensors to express the strobe pulse width in unit of lines.

We had that discussion already somewhere during this series. Tbh for me
microseconds seems fine. Most (professional) flashes are configured with
s^-1, so that would also be an option, but as flash_timeout is
configured in microseconds, i chose it for flash_duration too.

Nonetheless technically it shouldn't be a problem to express it as
number of lines... Is there a reason to prefer this?

> 
> I think we also need to decide how to handle camera sensors whose flash
> strobe pulse width can't be controlled. For instance, the AR0144 can
> output a flash signal, and its width is always equal to the exposure
> time. The most straightforward solution seems to implement
> V4L2_CID_FLASH_HW_STROBE_SIGNAL but not V4L2_CID_FLASH_DURATION in the
> sensor driver. Could this cause issues in any use case ? Is there a
> better solution ? I would like this to be documented.

Sounds good to me. In this case the V4L2_CID_FLASH_DURATION could be
provided as a read-only property too. So userspace is explicitely aware
of the acutal value and doesn't have to make assumptions.

Should I add documentation on this topic to this patch?

> 
> Finally, I think we also need to standardize the flash strobe offset.

I guess I somewhere mentioned this already: I have some patches for
configuring the strobe offset of ov9282 and adding the corresponding
v4l2 control. But to keep this series simple I'm planning to send them
as soon as this one is "done".

IMHO the offset should then have the same unit as the flash_duration.

> 
> > +
> > +.. _v4l2-cid-flash-hw-strobe-signal:
> > +
> > +``V4L2_CID_FLASH_HW_STROBE_SIGNAL (boolean)``
> 
> Nitpicking a bit on the name, I would have called this
> V4L2_CID_FLASH_STROBE_OUTPUT_ENABLE (or _OE).

I'm always open to name-nitpicking ;-)

V4L2_CID_FLASH_STROBE_OE sounds great to me... It's clear and even
shorter than V4L2_CID_FLASH_HW_STROBE_SIGNAL.

> 
> > +    Enables the output of a hardware strobe signal from the strobe source,
> > +    typically a camera sensor. To control a flash LED driver connected to this
> > +    hardware signal, the :ref:`flash LED mode <v4l2-cid-flash-led-mode>`
> > +    must be set to ``V4L2_FLASH_LED_MODE_FLASH`` and the
> > +    :ref:`strobe source <v4l2-cid-flash-strobe-source>` must be set to
> > +    ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``. Provided the flash LED driver
> > +    supports it, the length of the strobe signal can be configured by
> > +    adjusting its :ref:`flash duration <v4l2-cid-flash-duration>`.
> 
> The V4L2_CID_FLASH_HW_STROBE_SIGNAL documentation needs to be clarified
> in a similar way as V4L2_CID_FLASH_DURATION.

Sure. I will adapt this for v9.

> 
> -- 
> Regards,
> 
> Laurent Pinchart

thanks again for your great review!

regards;rl
Re: [PATCH v7 04/10] Documentation: uAPI: media: add V4L2_CID_FLASH_{DURATION,HW_STROBE_SIGNAL}
Posted by Laurent Pinchart 3 weeks, 3 days ago
On Mon, Sep 08, 2025 at 02:37:15PM +0200, Richard Leitner wrote:
> On Sun, Sep 07, 2025 at 09:49:53PM +0200, Laurent Pinchart wrote:
> > On Mon, Sep 01, 2025 at 05:05:09PM +0200, Richard Leitner wrote:
> > > Add the new strobe duration and hardware strobe signal control to v4l
> > > uAPI documentation. Additionally add labels for cross-referencing v4l
> > > controls.
> > > 
> > > Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> > > ---
> > >  .../userspace-api/media/v4l/ext-ctrls-flash.rst    | 29 ++++++++++++++++++++++
> > >  1 file changed, 29 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..6254420a8ca95929d23ffdc65f40a6e53e30a635 100644
> > > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> > > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> > > @@ -57,6 +57,8 @@ Flash Control IDs
> > >  ``V4L2_CID_FLASH_CLASS (class)``
> > >      The FLASH class descriptor.
> > >  
> > > +.. _v4l2-cid-flash-led-mode:
> > > +
> > >  ``V4L2_CID_FLASH_LED_MODE (menu)``
> > >      Defines the mode of the flash LED, the high-power white LED attached
> > >      to the flash controller. Setting this control may not be possible in
> > > @@ -80,6 +82,8 @@ Flash Control IDs
> > >  
> > >  
> > >  
> > > +.. _v4l2-cid-flash-strobe-source:
> > > +
> > >  ``V4L2_CID_FLASH_STROBE_SOURCE (menu)``
> > >      Defines the source of the flash LED strobe.
> > >  
> > > @@ -186,3 +190,28 @@ 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:
> > > +
> > > +``V4L2_CID_FLASH_DURATION (integer)``
> > > +    Duration of the flash strobe pulse generated by the strobe source,
> > > +    typically a camera sensor. This method of controlling flash LED strobe
> > > +    duration has three prerequisites: the strobe source's
> > > +    :ref:`hardware strobe signal <v4l2-cid-flash-hw-strobe-signal>` must be
> > > +    enabled, the flash LED driver's :ref:`flash LED mode <v4l2-cid-flash-led-mode>`
> > > +    must be set to ``V4L2_FLASH_LED_MODE_FLASH``, and the
> > > +    :ref:`strobe source <v4l2-cid-flash-strobe-source>` must be configured to
> > > +    ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``. The unit should be microseconds (µs)
> > > +    if possible.
> > 
> > As mentioned in the review of 01/10, I think this needs to be clarified.
> > Ideally we should add a new document in
> > Documentation/userspace-api/media/v4l/ to explain the flash API, but in
> > the meantime let's at lets improve the description of the duration
> > control. Here's a proposal.
> 
> Understood. Thank you for your proposal!
> 
> > ``V4L2_CID_FLASH_DURATION (integer)``
> >     Duration of the flash strobe pulse generated by the strobe source, when
> >     using external strobe. This control shall be implemented by the device
> >     generating the hardware flash strobe signal, typically a camera sensor,
> >     connected to a flash controller. It must not be implemented by the flash
> >     controller.
> > 
> >     This method of controlling flash LED strobe duration has three
> >     prerequisites: the strobe source's :ref:`hardware strobe signal
> >     <v4l2-cid-flash-hw-strobe-signal>` must be enabled, the flash controller's
> >     :ref:`flash LED mode <v4l2-cid-flash-led-mode>` must be set to
> >     ``V4L2_FLASH_LED_MODE_FLASH``, and its :ref:`strobe source
> >     <v4l2-cid-flash-strobe-source>` must be configured to
> >     ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``.
> > 
> >     The unit should be microseconds (µs) if possible.
> > 
> > 
> > The second paragraph may be better replaced by expanding the
> > documentation of V4L2_FLASH_STROBE_SOURCE_EXTERNAL, it seems a better
> > place to document how external strobe works.
> 
> That's fine for me. I will adapt the V4L2_CID_FLASH_DURATION and
> V4L2_FLASH_STROBE_SOURCE_EXTERNAL documentation accordingly and send in
> v9.

Sakari, could you please check if you agree with the above ? Let's avoid
going back and forth with reviews (and I'll try my best to review the
next version quickly).

> > As for the unit, is microseconds really the best option ? I would expect
> > most sensors to express the strobe pulse width in unit of lines.
> 
> We had that discussion already somewhere during this series. Tbh for me
> microseconds seems fine. Most (professional) flashes are configured with
> s^-1, so that would also be an option, but as flash_timeout is
> configured in microseconds, i chose it for flash_duration too.
> 
> Nonetheless technically it shouldn't be a problem to express it as
> number of lines... Is there a reason to prefer this?

A few observations have confirmed my gut feeling that this is how
sensors typically express the pulse width. Expressing the value in its
hardware unit means we won't have rounding issues, and drivers will also
be simpler. We're missing data though, it would be nice to check a wider
variety of camera sensors.

> > I think we also need to decide how to handle camera sensors whose flash
> > strobe pulse width can't be controlled. For instance, the AR0144 can
> > output a flash signal, and its width is always equal to the exposure
> > time. The most straightforward solution seems to implement
> > V4L2_CID_FLASH_HW_STROBE_SIGNAL but not V4L2_CID_FLASH_DURATION in the
> > sensor driver. Could this cause issues in any use case ? Is there a
> > better solution ? I would like this to be documented.
> 
> Sounds good to me. In this case the V4L2_CID_FLASH_DURATION could be
> provided as a read-only property too. So userspace is explicitely aware
> of the acutal value and doesn't have to make assumptions.

The value would change depending on the exposure time. Given how control
change events are implemented that would be difficult to use from
userspace at best. I think not exposing the control would be as useful
as exposing a read-only value, and it would be simpler to implement in
kernel drivers.

> Should I add documentation on this topic to this patch?

That would be nice, thank you.

> > Finally, I think we also need to standardize the flash strobe offset.
> 
> I guess I somewhere mentioned this already: I have some patches for
> configuring the strobe offset of ov9282 and adding the corresponding
> v4l2 control. But to keep this series simple I'm planning to send them
> as soon as this one is "done".
> 
> IMHO the offset should then have the same unit as the flash_duration.

What's the unit for the OV9282 ? For AR0144, it's a 8-bit signed value
expressed in units of half a line.

> > > +
> > > +.. _v4l2-cid-flash-hw-strobe-signal:
> > > +
> > > +``V4L2_CID_FLASH_HW_STROBE_SIGNAL (boolean)``
> > 
> > Nitpicking a bit on the name, I would have called this
> > V4L2_CID_FLASH_STROBE_OUTPUT_ENABLE (or _OE).
> 
> I'm always open to name-nitpicking ;-)
> 
> V4L2_CID_FLASH_STROBE_OE sounds great to me... It's clear and even
> shorter than V4L2_CID_FLASH_HW_STROBE_SIGNAL.

Sakari, what's your opinion ?

> > > +    Enables the output of a hardware strobe signal from the strobe source,
> > > +    typically a camera sensor. To control a flash LED driver connected to this
> > > +    hardware signal, the :ref:`flash LED mode <v4l2-cid-flash-led-mode>`
> > > +    must be set to ``V4L2_FLASH_LED_MODE_FLASH`` and the
> > > +    :ref:`strobe source <v4l2-cid-flash-strobe-source>` must be set to
> > > +    ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``. Provided the flash LED driver
> > > +    supports it, the length of the strobe signal can be configured by
> > > +    adjusting its :ref:`flash duration <v4l2-cid-flash-duration>`.
> > 
> > The V4L2_CID_FLASH_HW_STROBE_SIGNAL documentation needs to be clarified
> > in a similar way as V4L2_CID_FLASH_DURATION.
> 
> Sure. I will adapt this for v9.

-- 
Regards,

Laurent Pinchart
Re: [PATCH v7 04/10] Documentation: uAPI: media: add V4L2_CID_FLASH_{DURATION,HW_STROBE_SIGNAL}
Posted by Richard Leitner 3 weeks, 3 days ago
Hi Laurent,

thanks for your great (and quick) feedback!

On Mon, Sep 08, 2025 at 05:59:17PM +0200, Laurent Pinchart wrote:
> On Mon, Sep 08, 2025 at 02:37:15PM +0200, Richard Leitner wrote:
> > On Sun, Sep 07, 2025 at 09:49:53PM +0200, Laurent Pinchart wrote:
> > > On Mon, Sep 01, 2025 at 05:05:09PM +0200, Richard Leitner wrote:
> > > > Add the new strobe duration and hardware strobe signal control to v4l
> > > > uAPI documentation. Additionally add labels for cross-referencing v4l
> > > > controls.
> > > > 
> > > > Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> > > > ---
> > > >  .../userspace-api/media/v4l/ext-ctrls-flash.rst    | 29 ++++++++++++++++++++++
> > > >  1 file changed, 29 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..6254420a8ca95929d23ffdc65f40a6e53e30a635 100644
> > > > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> > > > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> > > > @@ -57,6 +57,8 @@ Flash Control IDs
> > > >  ``V4L2_CID_FLASH_CLASS (class)``
> > > >      The FLASH class descriptor.
> > > >  
> > > > +.. _v4l2-cid-flash-led-mode:
> > > > +
> > > >  ``V4L2_CID_FLASH_LED_MODE (menu)``
> > > >      Defines the mode of the flash LED, the high-power white LED attached
> > > >      to the flash controller. Setting this control may not be possible in
> > > > @@ -80,6 +82,8 @@ Flash Control IDs
> > > >  
> > > >  
> > > >  
> > > > +.. _v4l2-cid-flash-strobe-source:
> > > > +
> > > >  ``V4L2_CID_FLASH_STROBE_SOURCE (menu)``
> > > >      Defines the source of the flash LED strobe.
> > > >  
> > > > @@ -186,3 +190,28 @@ 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:
> > > > +
> > > > +``V4L2_CID_FLASH_DURATION (integer)``
> > > > +    Duration of the flash strobe pulse generated by the strobe source,
> > > > +    typically a camera sensor. This method of controlling flash LED strobe
> > > > +    duration has three prerequisites: the strobe source's
> > > > +    :ref:`hardware strobe signal <v4l2-cid-flash-hw-strobe-signal>` must be
> > > > +    enabled, the flash LED driver's :ref:`flash LED mode <v4l2-cid-flash-led-mode>`
> > > > +    must be set to ``V4L2_FLASH_LED_MODE_FLASH``, and the
> > > > +    :ref:`strobe source <v4l2-cid-flash-strobe-source>` must be configured to
> > > > +    ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``. The unit should be microseconds (µs)
> > > > +    if possible.
> > > 
> > > As mentioned in the review of 01/10, I think this needs to be clarified.
> > > Ideally we should add a new document in
> > > Documentation/userspace-api/media/v4l/ to explain the flash API, but in
> > > the meantime let's at lets improve the description of the duration
> > > control. Here's a proposal.
> > 
> > Understood. Thank you for your proposal!
> > 
> > > ``V4L2_CID_FLASH_DURATION (integer)``
> > >     Duration of the flash strobe pulse generated by the strobe source, when
> > >     using external strobe. This control shall be implemented by the device
> > >     generating the hardware flash strobe signal, typically a camera sensor,
> > >     connected to a flash controller. It must not be implemented by the flash
> > >     controller.
> > > 
> > >     This method of controlling flash LED strobe duration has three
> > >     prerequisites: the strobe source's :ref:`hardware strobe signal
> > >     <v4l2-cid-flash-hw-strobe-signal>` must be enabled, the flash controller's
> > >     :ref:`flash LED mode <v4l2-cid-flash-led-mode>` must be set to
> > >     ``V4L2_FLASH_LED_MODE_FLASH``, and its :ref:`strobe source
> > >     <v4l2-cid-flash-strobe-source>` must be configured to
> > >     ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``.
> > > 
> > >     The unit should be microseconds (µs) if possible.
> > > 
> > > 
> > > The second paragraph may be better replaced by expanding the
> > > documentation of V4L2_FLASH_STROBE_SOURCE_EXTERNAL, it seems a better
> > > place to document how external strobe works.
> > 
> > That's fine for me. I will adapt the V4L2_CID_FLASH_DURATION and
> > V4L2_FLASH_STROBE_SOURCE_EXTERNAL documentation accordingly and send in
> > v9.
> 
> Sakari, could you please check if you agree with the above ? Let's avoid
> going back and forth with reviews (and I'll try my best to review the
> next version quickly).

My current proposal:

    * - ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``
      - The flash strobe is triggered by an external source. Typically
        this is a sensor, which makes it possible to synchronise the
        flash strobe start to exposure start.
        This method of controlling flash LED strobe has two additional
        prerequisites: the strobe source's :ref:`flash strobe output
        <v4l2-cid-flash-strobe-oe>` must be enabled (if available)
        and the flash controller's :ref:`flash LED mode
        <v4l2-cid-flash-led-mode>` must be set to
        ``V4L2_FLASH_LED_MODE_FLASH``. Additionally the :ref:`flash duration
	<v4l2-cid-flash-duration>` may be adjusted by the strobe source.


``V4L2_CID_FLASH_DURATION (integer)``
    Duration of the flash strobe pulse generated by the strobe source, when
    using external strobe. This control shall be implemented by the device
    generating the hardware flash strobe signal, typically a camera sensor,
    connected to a flash controller. It must not be implemented by the flash
    controller. Typically the flash strobe pulse needs to be activated by
    enabling the strobe source's :ref:`flash strobe output
    <v4l2-cid-flash-strobe-oe>`.

    The flash controllers :ref:`strobe source <v4l2-cid-flash-strobe-source>`
    must be configured to ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL`` for this
    mode of operation.

    The unit should be number of lines if possible.


``V4L2_CID_FLASH_STROBE_OE (boolean)``
    Enables the output of a hardware strobe signal from the strobe source,
    when using external strobe. This control shall be implemented by the device
    generating the hardware flash strobe signal, typically a camera sensor,
    connected to a flash controller.

    Provided the signal generating device driver supports it, the length of the
    strobe signal can be configured by adjusting its
    :ref:`flash duration <v4l2-cid-flash-duration>`. In case the device has a
    fixed strobe length, the flash duration control must not be implemented.

    The flash controllers :ref:`strobe source <v4l2-cid-flash-strobe-source>`
    must be configured to ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL`` for this
    mode of operation.

> 
> > > As for the unit, is microseconds really the best option ? I would expect
> > > most sensors to express the strobe pulse width in unit of lines.
> > 
> > We had that discussion already somewhere during this series. Tbh for me
> > microseconds seems fine. Most (professional) flashes are configured with
> > s^-1, so that would also be an option, but as flash_timeout is
> > configured in microseconds, i chose it for flash_duration too.
> > 
> > Nonetheless technically it shouldn't be a problem to express it as
> > number of lines... Is there a reason to prefer this?
> 
> A few observations have confirmed my gut feeling that this is how
> sensors typically express the pulse width. Expressing the value in its
> hardware unit means we won't have rounding issues, and drivers will also
> be simpler. We're missing data though, it would be nice to check a wider
> variety of camera sensors.

I have done some more measurements and calculation on this for ov9281.
It seems you are (somehow?) right. The strobe_frame_span (aka strobe
duration) register value seems to represent the duration of the strobe in
number of lines plus a constant and variable offset based on the hblank
value. Other settings (e.g. vblank, exposure, ...) have no influence on
the duration.

After about 50 measurements using different strobe_frame_span and hblank
values and 1280x800 as resolution I came up with the following formulas:

   line_factor = active_width + hblank * 1,04 + 56

   t_strobe = strobe_frame_span * line_factor / pixel_rate

Which matches all tested cased nicely...

Nonetheless I'm still unsure on what unit to use for flash duration...

The exposure time for ov9282 is set as "number of row periods, where the
low 4 bits are fraction bits" in the registers. The v4l2 control should
on the other hand accept 100 µs units as value.

From a user perspective it would make sense to me to configure exposure
time, flash duration and flash/strobe offset using the same base units.
On the other hand we may have rounding issues and formulas based on
assumptions or reverse-engineering when implementing this for a
sensor...

What's your opinion on this, Sakari, Laurent, Dave?

> 
> > > I think we also need to decide how to handle camera sensors whose flash
> > > strobe pulse width can't be controlled. For instance, the AR0144 can
> > > output a flash signal, and its width is always equal to the exposure
> > > time. The most straightforward solution seems to implement
> > > V4L2_CID_FLASH_HW_STROBE_SIGNAL but not V4L2_CID_FLASH_DURATION in the
> > > sensor driver. Could this cause issues in any use case ? Is there a
> > > better solution ? I would like this to be documented.
> > 
> > Sounds good to me. In this case the V4L2_CID_FLASH_DURATION could be
> > provided as a read-only property too. So userspace is explicitely aware
> > of the acutal value and doesn't have to make assumptions.
> 
> The value would change depending on the exposure time. Given how control
> change events are implemented that would be difficult to use from
> userspace at best. I think not exposing the control would be as useful
> as exposing a read-only value, and it would be simpler to implement in
> kernel drivers.

That's true. I guess keeping the drivers simple and moving this "logic"
to a possible client/userspace application (if needed) is fine with me.

As you may have seen above, I've tried to integrate this in the
documentation proposal already.

> 
> > Should I add documentation on this topic to this patch?
> 
> That would be nice, thank you.
> 
> > > Finally, I think we also need to standardize the flash strobe offset.
> > 
> > I guess I somewhere mentioned this already: I have some patches for
> > configuring the strobe offset of ov9282 and adding the corresponding
> > v4l2 control. But to keep this series simple I'm planning to send them
> > as soon as this one is "done".
> > 
> > IMHO the offset should then have the same unit as the flash_duration.
> 
> What's the unit for the OV9282 ? For AR0144, it's a 8-bit signed value
> expressed in units of half a line.
> 
> > > > +
> > > > +.. _v4l2-cid-flash-hw-strobe-signal:
> > > > +
> > > > +``V4L2_CID_FLASH_HW_STROBE_SIGNAL (boolean)``
> > > 
> > > Nitpicking a bit on the name, I would have called this
> > > V4L2_CID_FLASH_STROBE_OUTPUT_ENABLE (or _OE).
> > 
> > I'm always open to name-nitpicking ;-)
> > 
> > V4L2_CID_FLASH_STROBE_OE sounds great to me... It's clear and even
> > shorter than V4L2_CID_FLASH_HW_STROBE_SIGNAL.
> 
> Sakari, what's your opinion ?
> 
> > > > +    Enables the output of a hardware strobe signal from the strobe source,
> > > > +    typically a camera sensor. To control a flash LED driver connected to this
> > > > +    hardware signal, the :ref:`flash LED mode <v4l2-cid-flash-led-mode>`
> > > > +    must be set to ``V4L2_FLASH_LED_MODE_FLASH`` and the
> > > > +    :ref:`strobe source <v4l2-cid-flash-strobe-source>` must be set to
> > > > +    ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``. Provided the flash LED driver
> > > > +    supports it, the length of the strobe signal can be configured by
> > > > +    adjusting its :ref:`flash duration <v4l2-cid-flash-duration>`.
> > > 
> > > The V4L2_CID_FLASH_HW_STROBE_SIGNAL documentation needs to be clarified
> > > in a similar way as V4L2_CID_FLASH_DURATION.
> > 
> > Sure. I will adapt this for v9.
> 
> -- 
> Regards,
> 
> Laurent Pinchart

thanks & regards;rl
Re: [PATCH v7 04/10] Documentation: uAPI: media: add V4L2_CID_FLASH_{DURATION,HW_STROBE_SIGNAL}
Posted by Sakari Ailus 2 weeks, 1 day ago
Hi Richard,

On Tue, Sep 09, 2025 at 12:29:55PM +0200, Richard Leitner wrote:
> Hi Laurent,
> 
> thanks for your great (and quick) feedback!
> 
> On Mon, Sep 08, 2025 at 05:59:17PM +0200, Laurent Pinchart wrote:
> > On Mon, Sep 08, 2025 at 02:37:15PM +0200, Richard Leitner wrote:
> > > On Sun, Sep 07, 2025 at 09:49:53PM +0200, Laurent Pinchart wrote:
> > > > On Mon, Sep 01, 2025 at 05:05:09PM +0200, Richard Leitner wrote:
> > > > > Add the new strobe duration and hardware strobe signal control to v4l
> > > > > uAPI documentation. Additionally add labels for cross-referencing v4l
> > > > > controls.
> > > > > 
> > > > > Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> > > > > ---
> > > > >  .../userspace-api/media/v4l/ext-ctrls-flash.rst    | 29 ++++++++++++++++++++++
> > > > >  1 file changed, 29 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..6254420a8ca95929d23ffdc65f40a6e53e30a635 100644
> > > > > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> > > > > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> > > > > @@ -57,6 +57,8 @@ Flash Control IDs
> > > > >  ``V4L2_CID_FLASH_CLASS (class)``
> > > > >      The FLASH class descriptor.
> > > > >  
> > > > > +.. _v4l2-cid-flash-led-mode:
> > > > > +
> > > > >  ``V4L2_CID_FLASH_LED_MODE (menu)``
> > > > >      Defines the mode of the flash LED, the high-power white LED attached
> > > > >      to the flash controller. Setting this control may not be possible in
> > > > > @@ -80,6 +82,8 @@ Flash Control IDs
> > > > >  
> > > > >  
> > > > >  
> > > > > +.. _v4l2-cid-flash-strobe-source:
> > > > > +
> > > > >  ``V4L2_CID_FLASH_STROBE_SOURCE (menu)``
> > > > >      Defines the source of the flash LED strobe.
> > > > >  
> > > > > @@ -186,3 +190,28 @@ 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:
> > > > > +
> > > > > +``V4L2_CID_FLASH_DURATION (integer)``
> > > > > +    Duration of the flash strobe pulse generated by the strobe source,
> > > > > +    typically a camera sensor. This method of controlling flash LED strobe
> > > > > +    duration has three prerequisites: the strobe source's
> > > > > +    :ref:`hardware strobe signal <v4l2-cid-flash-hw-strobe-signal>` must be
> > > > > +    enabled, the flash LED driver's :ref:`flash LED mode <v4l2-cid-flash-led-mode>`
> > > > > +    must be set to ``V4L2_FLASH_LED_MODE_FLASH``, and the
> > > > > +    :ref:`strobe source <v4l2-cid-flash-strobe-source>` must be configured to
> > > > > +    ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``. The unit should be microseconds (µs)
> > > > > +    if possible.
> > > > 
> > > > As mentioned in the review of 01/10, I think this needs to be clarified.
> > > > Ideally we should add a new document in
> > > > Documentation/userspace-api/media/v4l/ to explain the flash API, but in
> > > > the meantime let's at lets improve the description of the duration
> > > > control. Here's a proposal.
> > > 
> > > Understood. Thank you for your proposal!
> > > 
> > > > ``V4L2_CID_FLASH_DURATION (integer)``
> > > >     Duration of the flash strobe pulse generated by the strobe source, when
> > > >     using external strobe. This control shall be implemented by the device
> > > >     generating the hardware flash strobe signal, typically a camera sensor,
> > > >     connected to a flash controller. It must not be implemented by the flash
> > > >     controller.
> > > > 
> > > >     This method of controlling flash LED strobe duration has three
> > > >     prerequisites: the strobe source's :ref:`hardware strobe signal
> > > >     <v4l2-cid-flash-hw-strobe-signal>` must be enabled, the flash controller's
> > > >     :ref:`flash LED mode <v4l2-cid-flash-led-mode>` must be set to
> > > >     ``V4L2_FLASH_LED_MODE_FLASH``, and its :ref:`strobe source
> > > >     <v4l2-cid-flash-strobe-source>` must be configured to
> > > >     ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``.
> > > > 
> > > >     The unit should be microseconds (µs) if possible.
> > > > 
> > > > 
> > > > The second paragraph may be better replaced by expanding the
> > > > documentation of V4L2_FLASH_STROBE_SOURCE_EXTERNAL, it seems a better
> > > > place to document how external strobe works.
> > > 
> > > That's fine for me. I will adapt the V4L2_CID_FLASH_DURATION and
> > > V4L2_FLASH_STROBE_SOURCE_EXTERNAL documentation accordingly and send in
> > > v9.
> > 
> > Sakari, could you please check if you agree with the above ? Let's avoid
> > going back and forth with reviews (and I'll try my best to review the
> > next version quickly).
> 
> My current proposal:
> 
>     * - ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``
>       - The flash strobe is triggered by an external source. Typically
>         this is a sensor, which makes it possible to synchronise the
>         flash strobe start to exposure start.
>         This method of controlling flash LED strobe has two additional
>         prerequisites: the strobe source's :ref:`flash strobe output
>         <v4l2-cid-flash-strobe-oe>` must be enabled (if available)
>         and the flash controller's :ref:`flash LED mode
>         <v4l2-cid-flash-led-mode>` must be set to
>         ``V4L2_FLASH_LED_MODE_FLASH``. Additionally the :ref:`flash duration
> 	<v4l2-cid-flash-duration>` may be adjusted by the strobe source.
> 
> 
> ``V4L2_CID_FLASH_DURATION (integer)``
>     Duration of the flash strobe pulse generated by the strobe source, when
>     using external strobe. This control shall be implemented by the device
>     generating the hardware flash strobe signal, typically a camera sensor,
>     connected to a flash controller. It must not be implemented by the flash
>     controller. Typically the flash strobe pulse needs to be activated by

I'd drop the sentence on flash controller as this is UAPI documentation.

>     enabling the strobe source's :ref:`flash strobe output
>     <v4l2-cid-flash-strobe-oe>`.
> 
>     The flash controllers :ref:`strobe source <v4l2-cid-flash-strobe-source>`
>     must be configured to ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL`` for this
>     mode of operation.
> 
>     The unit should be number of lines if possible.
> 
> 
> ``V4L2_CID_FLASH_STROBE_OE (boolean)``
>     Enables the output of a hardware strobe signal from the strobe source,
>     when using external strobe. This control shall be implemented by the device

I'd remove the comma.

>     generating the hardware flash strobe signal, typically a camera sensor,
>     connected to a flash controller.
> 
>     Provided the signal generating device driver supports it, the length of the
>     strobe signal can be configured by adjusting its
>     :ref:`flash duration <v4l2-cid-flash-duration>`. In case the device has a
>     fixed strobe length, the flash duration control must not be implemented.

I don't see why the duration control wouldn't be implemented in this case:
it's still relevant for the user space to know how long the duration is.
I'd simply drop this sentence.

> 
>     The flash controllers :ref:`strobe source <v4l2-cid-flash-strobe-source>`
>     must be configured to ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL`` for this
>     mode of operation.
> 
> > 
> > > > As for the unit, is microseconds really the best option ? I would expect
> > > > most sensors to express the strobe pulse width in unit of lines.
> > > 
> > > We had that discussion already somewhere during this series. Tbh for me
> > > microseconds seems fine. Most (professional) flashes are configured with
> > > s^-1, so that would also be an option, but as flash_timeout is
> > > configured in microseconds, i chose it for flash_duration too.
> > > 
> > > Nonetheless technically it shouldn't be a problem to express it as
> > > number of lines... Is there a reason to prefer this?
> > 
> > A few observations have confirmed my gut feeling that this is how
> > sensors typically express the pulse width. Expressing the value in its
> > hardware unit means we won't have rounding issues, and drivers will also
> > be simpler. We're missing data though, it would be nice to check a wider
> > variety of camera sensors.
> 
> I have done some more measurements and calculation on this for ov9281.
> It seems you are (somehow?) right. The strobe_frame_span (aka strobe
> duration) register value seems to represent the duration of the strobe in
> number of lines plus a constant and variable offset based on the hblank
> value. Other settings (e.g. vblank, exposure, ...) have no influence on
> the duration.
> 
> After about 50 measurements using different strobe_frame_span and hblank
> values and 1280x800 as resolution I came up with the following formulas:
> 
>    line_factor = active_width + hblank * 1,04 + 56
> 
>    t_strobe = strobe_frame_span * line_factor / pixel_rate
> 
> Which matches all tested cased nicely...
> 
> Nonetheless I'm still unsure on what unit to use for flash duration...
> 
> The exposure time for ov9282 is set as "number of row periods, where the
> low 4 bits are fraction bits" in the registers. The v4l2 control should
> on the other hand accept 100 µs units as value.
> 
> From a user perspective it would make sense to me to configure exposure
> time, flash duration and flash/strobe offset using the same base units.
> On the other hand we may have rounding issues and formulas based on
> assumptions or reverse-engineering when implementing this for a
> sensor...
> 
> What's your opinion on this, Sakari, Laurent, Dave?

I checked what CCS defines exposure time as a function of the external
clock frequency:

	exposure time = tFlash_strobe_width_ctrl / ext_clk_freq *
			flash_strobe_adjustment

The added accuracy is relevant for xenon (admittedly rare these days, but
depends on the device) flash but probably not so for LEDs.

So I'm fine with keeping this as-is and perhaps adding CCS specific private
controls separately.

> 
> > 
> > > > I think we also need to decide how to handle camera sensors whose flash
> > > > strobe pulse width can't be controlled. For instance, the AR0144 can
> > > > output a flash signal, and its width is always equal to the exposure
> > > > time. The most straightforward solution seems to implement
> > > > V4L2_CID_FLASH_HW_STROBE_SIGNAL but not V4L2_CID_FLASH_DURATION in the
> > > > sensor driver. Could this cause issues in any use case ? Is there a
> > > > better solution ? I would like this to be documented.
> > > 
> > > Sounds good to me. In this case the V4L2_CID_FLASH_DURATION could be
> > > provided as a read-only property too. So userspace is explicitely aware
> > > of the acutal value and doesn't have to make assumptions.
> > 
> > The value would change depending on the exposure time. Given how control
> > change events are implemented that would be difficult to use from
> > userspace at best. I think not exposing the control would be as useful
> > as exposing a read-only value, and it would be simpler to implement in
> > kernel drivers.
> 
> That's true. I guess keeping the drivers simple and moving this "logic"
> to a possible client/userspace application (if needed) is fine with me.
> 
> As you may have seen above, I've tried to integrate this in the
> documentation proposal already.
> 
> > 
> > > Should I add documentation on this topic to this patch?
> > 
> > That would be nice, thank you.
> > 
> > > > Finally, I think we also need to standardize the flash strobe offset.
> > > 
> > > I guess I somewhere mentioned this already: I have some patches for
> > > configuring the strobe offset of ov9282 and adding the corresponding
> > > v4l2 control. But to keep this series simple I'm planning to send them
> > > as soon as this one is "done".
> > > 
> > > IMHO the offset should then have the same unit as the flash_duration.
> > 
> > What's the unit for the OV9282 ? For AR0144, it's a 8-bit signed value
> > expressed in units of half a line.
> > 
> > > > > +
> > > > > +.. _v4l2-cid-flash-hw-strobe-signal:
> > > > > +
> > > > > +``V4L2_CID_FLASH_HW_STROBE_SIGNAL (boolean)``
> > > > 
> > > > Nitpicking a bit on the name, I would have called this
> > > > V4L2_CID_FLASH_STROBE_OUTPUT_ENABLE (or _OE).
> > > 
> > > I'm always open to name-nitpicking ;-)
> > > 
> > > V4L2_CID_FLASH_STROBE_OE sounds great to me... It's clear and even
> > > shorter than V4L2_CID_FLASH_HW_STROBE_SIGNAL.
> > 
> > Sakari, what's your opinion ?

I slightly prefer the former, too.

> > 
> > > > > +    Enables the output of a hardware strobe signal from the strobe source,
> > > > > +    typically a camera sensor. To control a flash LED driver connected to this
> > > > > +    hardware signal, the :ref:`flash LED mode <v4l2-cid-flash-led-mode>`
> > > > > +    must be set to ``V4L2_FLASH_LED_MODE_FLASH`` and the
> > > > > +    :ref:`strobe source <v4l2-cid-flash-strobe-source>` must be set to
> > > > > +    ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``. Provided the flash LED driver
> > > > > +    supports it, the length of the strobe signal can be configured by
> > > > > +    adjusting its :ref:`flash duration <v4l2-cid-flash-duration>`.
> > > > 
> > > > The V4L2_CID_FLASH_HW_STROBE_SIGNAL documentation needs to be clarified
> > > > in a similar way as V4L2_CID_FLASH_DURATION.
> > > 
> > > Sure. I will adapt this for v9.
> > 
> > -- 
> > Regards,
> > 
> > Laurent Pinchart
> 
> thanks & regards;rl

-- 
Kind regards,

Sakari Ailus
Re: [PATCH v7 04/10] Documentation: uAPI: media: add V4L2_CID_FLASH_{DURATION,HW_STROBE_SIGNAL}
Posted by Richard Leitner 2 weeks, 1 day ago
Hi Sakari,
thanks for your feedback!

On Wed, Sep 17, 2025 at 06:43:49PM +0300, Sakari Ailus wrote:
> Hi Richard,
> 
> On Tue, Sep 09, 2025 at 12:29:55PM +0200, Richard Leitner wrote:
> > Hi Laurent,
> > 
> > thanks for your great (and quick) feedback!
> > 
> > On Mon, Sep 08, 2025 at 05:59:17PM +0200, Laurent Pinchart wrote:
> > > On Mon, Sep 08, 2025 at 02:37:15PM +0200, Richard Leitner wrote:
> > > > On Sun, Sep 07, 2025 at 09:49:53PM +0200, Laurent Pinchart wrote:
> > > > > On Mon, Sep 01, 2025 at 05:05:09PM +0200, Richard Leitner wrote:
> > > > > > Add the new strobe duration and hardware strobe signal control to v4l
> > > > > > uAPI documentation. Additionally add labels for cross-referencing v4l
> > > > > > controls.
> > > > > > 
> > > > > > Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> > > > > > ---
> > > > > >  .../userspace-api/media/v4l/ext-ctrls-flash.rst    | 29 ++++++++++++++++++++++
> > > > > >  1 file changed, 29 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..6254420a8ca95929d23ffdc65f40a6e53e30a635 100644
> > > > > > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> > > > > > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> > > > > > @@ -57,6 +57,8 @@ Flash Control IDs
> > > > > >  ``V4L2_CID_FLASH_CLASS (class)``
> > > > > >      The FLASH class descriptor.
> > > > > >  
> > > > > > +.. _v4l2-cid-flash-led-mode:
> > > > > > +
> > > > > >  ``V4L2_CID_FLASH_LED_MODE (menu)``
> > > > > >      Defines the mode of the flash LED, the high-power white LED attached
> > > > > >      to the flash controller. Setting this control may not be possible in
> > > > > > @@ -80,6 +82,8 @@ Flash Control IDs
> > > > > >  
> > > > > >  
> > > > > >  
> > > > > > +.. _v4l2-cid-flash-strobe-source:
> > > > > > +
> > > > > >  ``V4L2_CID_FLASH_STROBE_SOURCE (menu)``
> > > > > >      Defines the source of the flash LED strobe.
> > > > > >  
> > > > > > @@ -186,3 +190,28 @@ 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:
> > > > > > +
> > > > > > +``V4L2_CID_FLASH_DURATION (integer)``
> > > > > > +    Duration of the flash strobe pulse generated by the strobe source,
> > > > > > +    typically a camera sensor. This method of controlling flash LED strobe
> > > > > > +    duration has three prerequisites: the strobe source's
> > > > > > +    :ref:`hardware strobe signal <v4l2-cid-flash-hw-strobe-signal>` must be
> > > > > > +    enabled, the flash LED driver's :ref:`flash LED mode <v4l2-cid-flash-led-mode>`
> > > > > > +    must be set to ``V4L2_FLASH_LED_MODE_FLASH``, and the
> > > > > > +    :ref:`strobe source <v4l2-cid-flash-strobe-source>` must be configured to
> > > > > > +    ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``. The unit should be microseconds (µs)
> > > > > > +    if possible.
> > > > > 
> > > > > As mentioned in the review of 01/10, I think this needs to be clarified.
> > > > > Ideally we should add a new document in
> > > > > Documentation/userspace-api/media/v4l/ to explain the flash API, but in
> > > > > the meantime let's at lets improve the description of the duration
> > > > > control. Here's a proposal.
> > > > 
> > > > Understood. Thank you for your proposal!
> > > > 
> > > > > ``V4L2_CID_FLASH_DURATION (integer)``
> > > > >     Duration of the flash strobe pulse generated by the strobe source, when
> > > > >     using external strobe. This control shall be implemented by the device
> > > > >     generating the hardware flash strobe signal, typically a camera sensor,
> > > > >     connected to a flash controller. It must not be implemented by the flash
> > > > >     controller.
> > > > > 
> > > > >     This method of controlling flash LED strobe duration has three
> > > > >     prerequisites: the strobe source's :ref:`hardware strobe signal
> > > > >     <v4l2-cid-flash-hw-strobe-signal>` must be enabled, the flash controller's
> > > > >     :ref:`flash LED mode <v4l2-cid-flash-led-mode>` must be set to
> > > > >     ``V4L2_FLASH_LED_MODE_FLASH``, and its :ref:`strobe source
> > > > >     <v4l2-cid-flash-strobe-source>` must be configured to
> > > > >     ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``.
> > > > > 
> > > > >     The unit should be microseconds (µs) if possible.
> > > > > 
> > > > > 
> > > > > The second paragraph may be better replaced by expanding the
> > > > > documentation of V4L2_FLASH_STROBE_SOURCE_EXTERNAL, it seems a better
> > > > > place to document how external strobe works.
> > > > 
> > > > That's fine for me. I will adapt the V4L2_CID_FLASH_DURATION and
> > > > V4L2_FLASH_STROBE_SOURCE_EXTERNAL documentation accordingly and send in
> > > > v9.
> > > 
> > > Sakari, could you please check if you agree with the above ? Let's avoid
> > > going back and forth with reviews (and I'll try my best to review the
> > > next version quickly).
> > 
> > My current proposal:
> > 
> >     * - ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``
> >       - The flash strobe is triggered by an external source. Typically
> >         this is a sensor, which makes it possible to synchronise the
> >         flash strobe start to exposure start.
> >         This method of controlling flash LED strobe has two additional
> >         prerequisites: the strobe source's :ref:`flash strobe output
> >         <v4l2-cid-flash-strobe-oe>` must be enabled (if available)
> >         and the flash controller's :ref:`flash LED mode
> >         <v4l2-cid-flash-led-mode>` must be set to
> >         ``V4L2_FLASH_LED_MODE_FLASH``. Additionally the :ref:`flash duration
> > 	<v4l2-cid-flash-duration>` may be adjusted by the strobe source.
> > 
> > 
> > ``V4L2_CID_FLASH_DURATION (integer)``
> >     Duration of the flash strobe pulse generated by the strobe source, when
> >     using external strobe. This control shall be implemented by the device
> >     generating the hardware flash strobe signal, typically a camera sensor,
> >     connected to a flash controller. It must not be implemented by the flash
> >     controller. Typically the flash strobe pulse needs to be activated by
> 
> I'd drop the sentence on flash controller as this is UAPI documentation.

Is there any other place where this can/should be documented. As this
was suggested by Laurent: What's your opinion on this?

> 
> >     enabling the strobe source's :ref:`flash strobe output
> >     <v4l2-cid-flash-strobe-oe>`.
> > 
> >     The flash controllers :ref:`strobe source <v4l2-cid-flash-strobe-source>`
> >     must be configured to ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL`` for this
> >     mode of operation.
> > 
> >     The unit should be number of lines if possible.
> > 
> > 
> > ``V4L2_CID_FLASH_STROBE_OE (boolean)``
> >     Enables the output of a hardware strobe signal from the strobe source,
> >     when using external strobe. This control shall be implemented by the device
> 
> I'd remove the comma.
> 
> >     generating the hardware flash strobe signal, typically a camera sensor,
> >     connected to a flash controller.
> > 
> >     Provided the signal generating device driver supports it, the length of the
> >     strobe signal can be configured by adjusting its
> >     :ref:`flash duration <v4l2-cid-flash-duration>`. In case the device has a
> >     fixed strobe length, the flash duration control must not be implemented.
> 
> I don't see why the duration control wouldn't be implemented in this case:
> it's still relevant for the user space to know how long the duration is.
> I'd simply drop this sentence.

Laurent and I were discussing this a few mails ago in this thread.

He suggested to do not expose the control on fixed strobe length to keep
the drivers simpler as this information is/should be available somewhere
else. I.e. defined by the hardware.

I suggested to add a read-only flash duration control for fixed strobe
length devices. This makes drivers more complicated and may add some
rounding issues when the control is given in (micro) seconds.

As mentioned previously: I have no strong opinion on this. So how to
proceed here?

> 
> > 
> >     The flash controllers :ref:`strobe source <v4l2-cid-flash-strobe-source>`
> >     must be configured to ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL`` for this
> >     mode of operation.
> > 
> > > 
> > > > > As for the unit, is microseconds really the best option ? I would expect
> > > > > most sensors to express the strobe pulse width in unit of lines.
> > > > 
> > > > We had that discussion already somewhere during this series. Tbh for me
> > > > microseconds seems fine. Most (professional) flashes are configured with
> > > > s^-1, so that would also be an option, but as flash_timeout is
> > > > configured in microseconds, i chose it for flash_duration too.
> > > > 
> > > > Nonetheless technically it shouldn't be a problem to express it as
> > > > number of lines... Is there a reason to prefer this?
> > > 
> > > A few observations have confirmed my gut feeling that this is how
> > > sensors typically express the pulse width. Expressing the value in its
> > > hardware unit means we won't have rounding issues, and drivers will also
> > > be simpler. We're missing data though, it would be nice to check a wider
> > > variety of camera sensors.
> > 
> > I have done some more measurements and calculation on this for ov9281.
> > It seems you are (somehow?) right. The strobe_frame_span (aka strobe
> > duration) register value seems to represent the duration of the strobe in
> > number of lines plus a constant and variable offset based on the hblank
> > value. Other settings (e.g. vblank, exposure, ...) have no influence on
> > the duration.
> > 
> > After about 50 measurements using different strobe_frame_span and hblank
> > values and 1280x800 as resolution I came up with the following formulas:
> > 
> >    line_factor = active_width + hblank * 1,04 + 56
> > 
> >    t_strobe = strobe_frame_span * line_factor / pixel_rate
> > 
> > Which matches all tested cased nicely...
> > 
> > Nonetheless I'm still unsure on what unit to use for flash duration...
> > 
> > The exposure time for ov9282 is set as "number of row periods, where the
> > low 4 bits are fraction bits" in the registers. The v4l2 control should
> > on the other hand accept 100 µs units as value.
> > 
> > From a user perspective it would make sense to me to configure exposure
> > time, flash duration and flash/strobe offset using the same base units.
> > On the other hand we may have rounding issues and formulas based on
> > assumptions or reverse-engineering when implementing this for a
> > sensor...
> > 
> > What's your opinion on this, Sakari, Laurent, Dave?
> 
> I checked what CCS defines exposure time as a function of the external
> clock frequency:
> 
> 	exposure time = tFlash_strobe_width_ctrl / ext_clk_freq *
> 			flash_strobe_adjustment
> 
> The added accuracy is relevant for xenon (admittedly rare these days, but
> depends on the device) flash but probably not so for LEDs.
> 
> So I'm fine with keeping this as-is and perhaps adding CCS specific private
> controls separately.

As mentioned previously I prefer (micro)seconds too for this control.
Yes, it results in more complicated drivers, but makes live for
userspace much easier IMHO.

> 
> > 
> > > 
> > > > > I think we also need to decide how to handle camera sensors whose flash
> > > > > strobe pulse width can't be controlled. For instance, the AR0144 can
> > > > > output a flash signal, and its width is always equal to the exposure
> > > > > time. The most straightforward solution seems to implement
> > > > > V4L2_CID_FLASH_HW_STROBE_SIGNAL but not V4L2_CID_FLASH_DURATION in the
> > > > > sensor driver. Could this cause issues in any use case ? Is there a
> > > > > better solution ? I would like this to be documented.
> > > > 
> > > > Sounds good to me. In this case the V4L2_CID_FLASH_DURATION could be
> > > > provided as a read-only property too. So userspace is explicitely aware
> > > > of the acutal value and doesn't have to make assumptions.
> > > 
> > > The value would change depending on the exposure time. Given how control
> > > change events are implemented that would be difficult to use from
> > > userspace at best. I think not exposing the control would be as useful
> > > as exposing a read-only value, and it would be simpler to implement in
> > > kernel drivers.
> > 
> > That's true. I guess keeping the drivers simple and moving this "logic"
> > to a possible client/userspace application (if needed) is fine with me.
> > 
> > As you may have seen above, I've tried to integrate this in the
> > documentation proposal already.
> > 
> > > 
> > > > Should I add documentation on this topic to this patch?
> > > 
> > > That would be nice, thank you.
> > > 
> > > > > Finally, I think we also need to standardize the flash strobe offset.
> > > > 
> > > > I guess I somewhere mentioned this already: I have some patches for
> > > > configuring the strobe offset of ov9282 and adding the corresponding
> > > > v4l2 control. But to keep this series simple I'm planning to send them
> > > > as soon as this one is "done".
> > > > 
> > > > IMHO the offset should then have the same unit as the flash_duration.
> > > 
> > > What's the unit for the OV9282 ? For AR0144, it's a 8-bit signed value
> > > expressed in units of half a line.
> > > 
> > > > > > +
> > > > > > +.. _v4l2-cid-flash-hw-strobe-signal:
> > > > > > +
> > > > > > +``V4L2_CID_FLASH_HW_STROBE_SIGNAL (boolean)``
> > > > > 
> > > > > Nitpicking a bit on the name, I would have called this
> > > > > V4L2_CID_FLASH_STROBE_OUTPUT_ENABLE (or _OE).
> > > > 
> > > > I'm always open to name-nitpicking ;-)
> > > > 
> > > > V4L2_CID_FLASH_STROBE_OE sounds great to me... It's clear and even
> > > > shorter than V4L2_CID_FLASH_HW_STROBE_SIGNAL.
> > > 
> > > Sakari, what's your opinion ?
> 
> I slightly prefer the former, too.

Thanks. Fine with me!
So I'll change the name to V4L2_CID_FLASH_STROBE_OE in the next version.

> 
> > > 
> > > > > > +    Enables the output of a hardware strobe signal from the strobe source,
> > > > > > +    typically a camera sensor. To control a flash LED driver connected to this
> > > > > > +    hardware signal, the :ref:`flash LED mode <v4l2-cid-flash-led-mode>`
> > > > > > +    must be set to ``V4L2_FLASH_LED_MODE_FLASH`` and the
> > > > > > +    :ref:`strobe source <v4l2-cid-flash-strobe-source>` must be set to
> > > > > > +    ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``. Provided the flash LED driver
> > > > > > +    supports it, the length of the strobe signal can be configured by
> > > > > > +    adjusting its :ref:`flash duration <v4l2-cid-flash-duration>`.
> > > > > 
> > > > > The V4L2_CID_FLASH_HW_STROBE_SIGNAL documentation needs to be clarified
> > > > > in a similar way as V4L2_CID_FLASH_DURATION.
> > > > 
> > > > Sure. I will adapt this for v9.
> > > 
> > > -- 
> > > Regards,
> > > 
> > > Laurent Pinchart
> > 
> > thanks & regards;rl
> 
> -- 
> Kind regards,
> 
> Sakari Ailus

regards;rl
Re: [PATCH v7 04/10] Documentation: uAPI: media: add V4L2_CID_FLASH_{DURATION,HW_STROBE_SIGNAL}
Posted by Richard Leitner 2 weeks, 2 days ago
Hi Laurent and Sakari,

just a friendly ping for the pending feedback on this series :-)

regards;rl

On Tue, Sep 09, 2025 at 12:29:59PM +0200, Richard Leitner wrote:
> Hi Laurent,
> 
> thanks for your great (and quick) feedback!
> 
> On Mon, Sep 08, 2025 at 05:59:17PM +0200, Laurent Pinchart wrote:
> > On Mon, Sep 08, 2025 at 02:37:15PM +0200, Richard Leitner wrote:
> > > On Sun, Sep 07, 2025 at 09:49:53PM +0200, Laurent Pinchart wrote:
> > > > On Mon, Sep 01, 2025 at 05:05:09PM +0200, Richard Leitner wrote:
> > > > > Add the new strobe duration and hardware strobe signal control to v4l
> > > > > uAPI documentation. Additionally add labels for cross-referencing v4l
> > > > > controls.
> > > > > 
> > > > > Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> > > > > ---
> > > > >  .../userspace-api/media/v4l/ext-ctrls-flash.rst    | 29 ++++++++++++++++++++++
> > > > >  1 file changed, 29 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..6254420a8ca95929d23ffdc65f40a6e53e30a635 100644
> > > > > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> > > > > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-flash.rst
> > > > > @@ -57,6 +57,8 @@ Flash Control IDs
> > > > >  ``V4L2_CID_FLASH_CLASS (class)``
> > > > >      The FLASH class descriptor.
> > > > >  
> > > > > +.. _v4l2-cid-flash-led-mode:
> > > > > +
> > > > >  ``V4L2_CID_FLASH_LED_MODE (menu)``
> > > > >      Defines the mode of the flash LED, the high-power white LED attached
> > > > >      to the flash controller. Setting this control may not be possible in
> > > > > @@ -80,6 +82,8 @@ Flash Control IDs
> > > > >  
> > > > >  
> > > > >  
> > > > > +.. _v4l2-cid-flash-strobe-source:
> > > > > +
> > > > >  ``V4L2_CID_FLASH_STROBE_SOURCE (menu)``
> > > > >      Defines the source of the flash LED strobe.
> > > > >  
> > > > > @@ -186,3 +190,28 @@ 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:
> > > > > +
> > > > > +``V4L2_CID_FLASH_DURATION (integer)``
> > > > > +    Duration of the flash strobe pulse generated by the strobe source,
> > > > > +    typically a camera sensor. This method of controlling flash LED strobe
> > > > > +    duration has three prerequisites: the strobe source's
> > > > > +    :ref:`hardware strobe signal <v4l2-cid-flash-hw-strobe-signal>` must be
> > > > > +    enabled, the flash LED driver's :ref:`flash LED mode <v4l2-cid-flash-led-mode>`
> > > > > +    must be set to ``V4L2_FLASH_LED_MODE_FLASH``, and the
> > > > > +    :ref:`strobe source <v4l2-cid-flash-strobe-source>` must be configured to
> > > > > +    ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``. The unit should be microseconds (µs)
> > > > > +    if possible.
> > > > 
> > > > As mentioned in the review of 01/10, I think this needs to be clarified.
> > > > Ideally we should add a new document in
> > > > Documentation/userspace-api/media/v4l/ to explain the flash API, but in
> > > > the meantime let's at lets improve the description of the duration
> > > > control. Here's a proposal.
> > > 
> > > Understood. Thank you for your proposal!
> > > 
> > > > ``V4L2_CID_FLASH_DURATION (integer)``
> > > >     Duration of the flash strobe pulse generated by the strobe source, when
> > > >     using external strobe. This control shall be implemented by the device
> > > >     generating the hardware flash strobe signal, typically a camera sensor,
> > > >     connected to a flash controller. It must not be implemented by the flash
> > > >     controller.
> > > > 
> > > >     This method of controlling flash LED strobe duration has three
> > > >     prerequisites: the strobe source's :ref:`hardware strobe signal
> > > >     <v4l2-cid-flash-hw-strobe-signal>` must be enabled, the flash controller's
> > > >     :ref:`flash LED mode <v4l2-cid-flash-led-mode>` must be set to
> > > >     ``V4L2_FLASH_LED_MODE_FLASH``, and its :ref:`strobe source
> > > >     <v4l2-cid-flash-strobe-source>` must be configured to
> > > >     ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``.
> > > > 
> > > >     The unit should be microseconds (µs) if possible.
> > > > 
> > > > 
> > > > The second paragraph may be better replaced by expanding the
> > > > documentation of V4L2_FLASH_STROBE_SOURCE_EXTERNAL, it seems a better
> > > > place to document how external strobe works.
> > > 
> > > That's fine for me. I will adapt the V4L2_CID_FLASH_DURATION and
> > > V4L2_FLASH_STROBE_SOURCE_EXTERNAL documentation accordingly and send in
> > > v9.
> > 
> > Sakari, could you please check if you agree with the above ? Let's avoid
> > going back and forth with reviews (and I'll try my best to review the
> > next version quickly).
> 
> My current proposal:
> 
>     * - ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``
>       - The flash strobe is triggered by an external source. Typically
>         this is a sensor, which makes it possible to synchronise the
>         flash strobe start to exposure start.
>         This method of controlling flash LED strobe has two additional
>         prerequisites: the strobe source's :ref:`flash strobe output
>         <v4l2-cid-flash-strobe-oe>` must be enabled (if available)
>         and the flash controller's :ref:`flash LED mode
>         <v4l2-cid-flash-led-mode>` must be set to
>         ``V4L2_FLASH_LED_MODE_FLASH``. Additionally the :ref:`flash duration
> 	<v4l2-cid-flash-duration>` may be adjusted by the strobe source.
> 
> 
> ``V4L2_CID_FLASH_DURATION (integer)``
>     Duration of the flash strobe pulse generated by the strobe source, when
>     using external strobe. This control shall be implemented by the device
>     generating the hardware flash strobe signal, typically a camera sensor,
>     connected to a flash controller. It must not be implemented by the flash
>     controller. Typically the flash strobe pulse needs to be activated by
>     enabling the strobe source's :ref:`flash strobe output
>     <v4l2-cid-flash-strobe-oe>`.
> 
>     The flash controllers :ref:`strobe source <v4l2-cid-flash-strobe-source>`
>     must be configured to ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL`` for this
>     mode of operation.
> 
>     The unit should be number of lines if possible.
> 
> 
> ``V4L2_CID_FLASH_STROBE_OE (boolean)``
>     Enables the output of a hardware strobe signal from the strobe source,
>     when using external strobe. This control shall be implemented by the device
>     generating the hardware flash strobe signal, typically a camera sensor,
>     connected to a flash controller.
> 
>     Provided the signal generating device driver supports it, the length of the
>     strobe signal can be configured by adjusting its
>     :ref:`flash duration <v4l2-cid-flash-duration>`. In case the device has a
>     fixed strobe length, the flash duration control must not be implemented.
> 
>     The flash controllers :ref:`strobe source <v4l2-cid-flash-strobe-source>`
>     must be configured to ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL`` for this
>     mode of operation.
> 
> > 
> > > > As for the unit, is microseconds really the best option ? I would expect
> > > > most sensors to express the strobe pulse width in unit of lines.
> > > 
> > > We had that discussion already somewhere during this series. Tbh for me
> > > microseconds seems fine. Most (professional) flashes are configured with
> > > s^-1, so that would also be an option, but as flash_timeout is
> > > configured in microseconds, i chose it for flash_duration too.
> > > 
> > > Nonetheless technically it shouldn't be a problem to express it as
> > > number of lines... Is there a reason to prefer this?
> > 
> > A few observations have confirmed my gut feeling that this is how
> > sensors typically express the pulse width. Expressing the value in its
> > hardware unit means we won't have rounding issues, and drivers will also
> > be simpler. We're missing data though, it would be nice to check a wider
> > variety of camera sensors.
> 
> I have done some more measurements and calculation on this for ov9281.
> It seems you are (somehow?) right. The strobe_frame_span (aka strobe
> duration) register value seems to represent the duration of the strobe in
> number of lines plus a constant and variable offset based on the hblank
> value. Other settings (e.g. vblank, exposure, ...) have no influence on
> the duration.
> 
> After about 50 measurements using different strobe_frame_span and hblank
> values and 1280x800 as resolution I came up with the following formulas:
> 
>    line_factor = active_width + hblank * 1,04 + 56
> 
>    t_strobe = strobe_frame_span * line_factor / pixel_rate
> 
> Which matches all tested cased nicely...
> 
> Nonetheless I'm still unsure on what unit to use for flash duration...
> 
> The exposure time for ov9282 is set as "number of row periods, where the
> low 4 bits are fraction bits" in the registers. The v4l2 control should
> on the other hand accept 100 µs units as value.
> 
> From a user perspective it would make sense to me to configure exposure
> time, flash duration and flash/strobe offset using the same base units.
> On the other hand we may have rounding issues and formulas based on
> assumptions or reverse-engineering when implementing this for a
> sensor...
> 
> What's your opinion on this, Sakari, Laurent, Dave?
> 
> > 
> > > > I think we also need to decide how to handle camera sensors whose flash
> > > > strobe pulse width can't be controlled. For instance, the AR0144 can
> > > > output a flash signal, and its width is always equal to the exposure
> > > > time. The most straightforward solution seems to implement
> > > > V4L2_CID_FLASH_HW_STROBE_SIGNAL but not V4L2_CID_FLASH_DURATION in the
> > > > sensor driver. Could this cause issues in any use case ? Is there a
> > > > better solution ? I would like this to be documented.
> > > 
> > > Sounds good to me. In this case the V4L2_CID_FLASH_DURATION could be
> > > provided as a read-only property too. So userspace is explicitely aware
> > > of the acutal value and doesn't have to make assumptions.
> > 
> > The value would change depending on the exposure time. Given how control
> > change events are implemented that would be difficult to use from
> > userspace at best. I think not exposing the control would be as useful
> > as exposing a read-only value, and it would be simpler to implement in
> > kernel drivers.
> 
> That's true. I guess keeping the drivers simple and moving this "logic"
> to a possible client/userspace application (if needed) is fine with me.
> 
> As you may have seen above, I've tried to integrate this in the
> documentation proposal already.
> 
> > 
> > > Should I add documentation on this topic to this patch?
> > 
> > That would be nice, thank you.
> > 
> > > > Finally, I think we also need to standardize the flash strobe offset.
> > > 
> > > I guess I somewhere mentioned this already: I have some patches for
> > > configuring the strobe offset of ov9282 and adding the corresponding
> > > v4l2 control. But to keep this series simple I'm planning to send them
> > > as soon as this one is "done".
> > > 
> > > IMHO the offset should then have the same unit as the flash_duration.
> > 
> > What's the unit for the OV9282 ? For AR0144, it's a 8-bit signed value
> > expressed in units of half a line.
> > 
> > > > > +
> > > > > +.. _v4l2-cid-flash-hw-strobe-signal:
> > > > > +
> > > > > +``V4L2_CID_FLASH_HW_STROBE_SIGNAL (boolean)``
> > > > 
> > > > Nitpicking a bit on the name, I would have called this
> > > > V4L2_CID_FLASH_STROBE_OUTPUT_ENABLE (or _OE).
> > > 
> > > I'm always open to name-nitpicking ;-)
> > > 
> > > V4L2_CID_FLASH_STROBE_OE sounds great to me... It's clear and even
> > > shorter than V4L2_CID_FLASH_HW_STROBE_SIGNAL.
> > 
> > Sakari, what's your opinion ?
> > 
> > > > > +    Enables the output of a hardware strobe signal from the strobe source,
> > > > > +    typically a camera sensor. To control a flash LED driver connected to this
> > > > > +    hardware signal, the :ref:`flash LED mode <v4l2-cid-flash-led-mode>`
> > > > > +    must be set to ``V4L2_FLASH_LED_MODE_FLASH`` and the
> > > > > +    :ref:`strobe source <v4l2-cid-flash-strobe-source>` must be set to
> > > > > +    ``V4L2_FLASH_STROBE_SOURCE_EXTERNAL``. Provided the flash LED driver
> > > > > +    supports it, the length of the strobe signal can be configured by
> > > > > +    adjusting its :ref:`flash duration <v4l2-cid-flash-duration>`.
> > > > 
> > > > The V4L2_CID_FLASH_HW_STROBE_SIGNAL documentation needs to be clarified
> > > > in a similar way as V4L2_CID_FLASH_DURATION.
> > > 
> > > Sure. I will adapt this for v9.
> > 
> > -- 
> > Regards,
> > 
> > Laurent Pinchart
> 
> thanks & regards;rl