Message-ID: <j337fpaqahmee3qutgtkavud6rbqyn4lpsj4yaha2xmvcvfhli@z67twdhybvqp>
Date: Mon, 8 Sep 2025 14:37:15 +0200
From: Richard Leitner <richard.leitner@...ux.dev>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Sakari Ailus <sakari.ailus@...ux.intel.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 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@...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.

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

Powered by blists - more mailing lists