Message-ID: <7h6ni4u34hpmfpu56fbcx3sz7fxeqdskjkwbgatttycsljl2we@5wlox7llfa74>
Date: Wed, 17 Sep 2025 12:14:20 +0200
From: Richard Leitner <richard.leitner@...ux.dev>
To: Sakari Ailus <sakari.ailus@...ux.intel.com>, 
	Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Dave Stevenson <dave.stevenson@...pberrypi.com>, 
	Mauro Carvalho Chehab <mchehab@...nel.org>, Lee Jones <lee@...nel.org>, Pavel Machek <pavel@...nel.org>, 
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org, linux-leds@...r.kernel.org, 
	Hans Verkuil <hverkuil@...nel.org>
Subject: Re: [PATCH v7 04/10] Documentation: uAPI: media: add
 V4L2_CID_FLASH_{DURATION,HW_STROBE_SIGNAL}

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@...ux.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

Powered by blists - more mailing lists