Message-ID: <aMrXNXexGBXCxbKd@kekkonen.localdomain>
Date: Wed, 17 Sep 2025 18:43:49 +0300
From: Sakari Ailus <sakari.ailus@...ux.intel.com>
To: Richard Leitner <richard.leitner@...ux.dev>
Cc: Laurent Pinchart <laurent.pinchart@...asonboard.com>,
	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 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@...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

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

Powered by blists - more mailing lists