Message-ID: <20250907194953.GA19568@pendragon.ideasonboard.com>
Date: Sun, 7 Sep 2025 21:49:53 +0200
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Richard Leitner <richard.leitner@...ux.dev>
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 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.

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

Powered by blists - more mailing lists