Message-ID: <bhlm2hxkq5xojqokbhpk3ho6z7k37tzf7tw6zzm3223vi2wqbx@x3d5gq3yvups>
Date: Mon, 13 Oct 2025 11:57:37 +0200
From: Richard Leitner <richard.leitner@...ux.dev>
To: Sakari Ailus <sakari.ailus@...ux.intel.com>
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 Sakari, Hi Laurent,

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:

[snip]

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

That sentence was "requested" by Laurent. What's your opinion on this?
It would be great to have a wording for v8 that's fine for both of you ;-)

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

What's your opinion on this Laurent?

I'm currently preparing v8 and just want to make sure this aligns with
both of you?

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

Thanks for the feedback. That's fine with me :-)

[snip]

> > > > > > +
> > > > > > +.. _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.

Fine. I'll go for V4L2_CID_FLASH_STROBE_OE.

[snip]

regards; rl

Powered by blists - more mailing lists