[PATCH v2 1/8] media: v4l: ctrls: add a control for flash/strobe duration

Richard Leitner posted 8 patches 9 months ago
There is a newer version of this series
[PATCH v2 1/8] media: v4l: ctrls: add a control for flash/strobe duration
Posted by Richard Leitner 9 months ago
Add a control V4L2_CID_FLASH_DURATION to set the duration of a
flash/strobe pulse. This is different to the V4L2_CID_FLASH_TIMEOUT
control, as the timeout defines a limit after which the flash is
"forcefully" turned off again.

On the other hand the new V4L2_CID_FLASH_DURATION is the desired length
of the flash/strobe pulse

Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
---
 drivers/media/v4l2-core/v4l2-ctrls-defs.c | 1 +
 include/uapi/linux/v4l2-controls.h        | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
index 1ea52011247accc51d0261f56eab1cf13c0624a0..f9ed7273a9f3eafe01c31b638e1c8d9fcf5424af 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
@@ -1135,6 +1135,7 @@ const char *v4l2_ctrl_get_name(u32 id)
 	case V4L2_CID_FLASH_FAULT:		return "Faults";
 	case V4L2_CID_FLASH_CHARGE:		return "Charge";
 	case V4L2_CID_FLASH_READY:		return "Ready to Strobe";
+	case V4L2_CID_FLASH_DURATION:		return "Strobe Duration";
 
 	/* JPEG encoder controls */
 	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index 974fd254e57309e6def95b4a4f8e4de13a3972a7..80050cadb8377e3070ebbadc493fcd08b2c12c0b 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -1173,6 +1173,7 @@ enum v4l2_flash_strobe_source {
 
 #define V4L2_CID_FLASH_CHARGE			(V4L2_CID_FLASH_CLASS_BASE + 11)
 #define V4L2_CID_FLASH_READY			(V4L2_CID_FLASH_CLASS_BASE + 12)
+#define V4L2_CID_FLASH_DURATION			(V4L2_CID_FLASH_CLASS_BASE + 13)
 
 
 /* JPEG-class control IDs */

-- 
2.47.2
Re: [PATCH v2 1/8] media: v4l: ctrls: add a control for flash/strobe duration
Posted by Sakari Ailus 9 months ago
Hi Richard,

Thanks for the set.

On Fri, Mar 14, 2025 at 09:49:55AM +0100, Richard Leitner wrote:
> Add a control V4L2_CID_FLASH_DURATION to set the duration of a
> flash/strobe pulse. This is different to the V4L2_CID_FLASH_TIMEOUT
> control, as the timeout defines a limit after which the flash is
> "forcefully" turned off again.
> 
> On the other hand the new V4L2_CID_FLASH_DURATION is the desired length
> of the flash/strobe pulse

What's the actual difference between the two? To me they appear the same,
just expressed in a different way.

> 
> Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> ---
>  drivers/media/v4l2-core/v4l2-ctrls-defs.c | 1 +
>  include/uapi/linux/v4l2-controls.h        | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> index 1ea52011247accc51d0261f56eab1cf13c0624a0..f9ed7273a9f3eafe01c31b638e1c8d9fcf5424af 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> @@ -1135,6 +1135,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_FLASH_FAULT:		return "Faults";
>  	case V4L2_CID_FLASH_CHARGE:		return "Charge";
>  	case V4L2_CID_FLASH_READY:		return "Ready to Strobe";
> +	case V4L2_CID_FLASH_DURATION:		return "Strobe Duration";
>  
>  	/* JPEG encoder controls */
>  	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index 974fd254e57309e6def95b4a4f8e4de13a3972a7..80050cadb8377e3070ebbadc493fcd08b2c12c0b 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -1173,6 +1173,7 @@ enum v4l2_flash_strobe_source {
>  
>  #define V4L2_CID_FLASH_CHARGE			(V4L2_CID_FLASH_CLASS_BASE + 11)
>  #define V4L2_CID_FLASH_READY			(V4L2_CID_FLASH_CLASS_BASE + 12)
> +#define V4L2_CID_FLASH_DURATION			(V4L2_CID_FLASH_CLASS_BASE + 13)
>  
>  
>  /* JPEG-class control IDs */
> 

-- 
Regards,

Sakari Ailus
Re: [PATCH v2 1/8] media: v4l: ctrls: add a control for flash/strobe duration
Posted by Richard Leitner 9 months ago
On Fri, Mar 14, 2025 at 09:20:23AM +0000, Sakari Ailus wrote:
> Hi Richard,
> 
> Thanks for the set.

Hi Sakari,
thanks for the quick response!

> 
> On Fri, Mar 14, 2025 at 09:49:55AM +0100, Richard Leitner wrote:
> > Add a control V4L2_CID_FLASH_DURATION to set the duration of a
> > flash/strobe pulse. This is different to the V4L2_CID_FLASH_TIMEOUT
> > control, as the timeout defines a limit after which the flash is
> > "forcefully" turned off again.
> > 
> > On the other hand the new V4L2_CID_FLASH_DURATION is the desired length
> > of the flash/strobe pulse
> 
> What's the actual difference between the two? To me they appear the same,
> just expressed in a different way.

According to FLASH_TIMEOUT documentation:

	Hardware timeout for flash. The flash strobe is stopped after this
	period of time has passed from the start of the strobe. [1]

This is a little bit unspecific, but as also discussed with Dave [2]
according to the documentation of V4L2_FLASH_FAULT_TIMEOUT it seems to
be targeted at providing a "real timeout" control, not settings the
desired duration:

	The flash strobe was still on when the timeout set by the user
	--- V4L2_CID_FLASH_TIMEOUT control --- has expired. Not all flash
	controllers may set this in all such conditions. [1]

If I understood that wrong, I'm also happy to use FLASH_TIMEOUT for this
use-case. But tbh I think FLASH_DURATION would be more specific.

As this still seems unclear: Should the documentation be
changed/rewritten if we stick with the FLASH_DURATION approach?

[1] https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-flash.html
[2] https://lore.kernel.org/lkml/CAPY8ntB8i4OyUWAL8k899yUd5QsRifJXiOfWXKceGQ7TNZ4OUw@mail.gmail.com/

> 
> > 
> > Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> > ---
> >  drivers/media/v4l2-core/v4l2-ctrls-defs.c | 1 +
> >  include/uapi/linux/v4l2-controls.h        | 1 +
> >  2 files changed, 2 insertions(+)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > index 1ea52011247accc51d0261f56eab1cf13c0624a0..f9ed7273a9f3eafe01c31b638e1c8d9fcf5424af 100644
> > --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > @@ -1135,6 +1135,7 @@ const char *v4l2_ctrl_get_name(u32 id)
> >  	case V4L2_CID_FLASH_FAULT:		return "Faults";
> >  	case V4L2_CID_FLASH_CHARGE:		return "Charge";
> >  	case V4L2_CID_FLASH_READY:		return "Ready to Strobe";
> > +	case V4L2_CID_FLASH_DURATION:		return "Strobe Duration";
> >  
> >  	/* JPEG encoder controls */
> >  	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
> > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> > index 974fd254e57309e6def95b4a4f8e4de13a3972a7..80050cadb8377e3070ebbadc493fcd08b2c12c0b 100644
> > --- a/include/uapi/linux/v4l2-controls.h
> > +++ b/include/uapi/linux/v4l2-controls.h
> > @@ -1173,6 +1173,7 @@ enum v4l2_flash_strobe_source {
> >  
> >  #define V4L2_CID_FLASH_CHARGE			(V4L2_CID_FLASH_CLASS_BASE + 11)
> >  #define V4L2_CID_FLASH_READY			(V4L2_CID_FLASH_CLASS_BASE + 12)
> > +#define V4L2_CID_FLASH_DURATION			(V4L2_CID_FLASH_CLASS_BASE + 13)
> >  
> >  
> >  /* JPEG-class control IDs */
> > 
> 
> -- 
> Regards,
> 
> Sakari Ailus

Thanks!
Richard
Re: [PATCH v2 1/8] media: v4l: ctrls: add a control for flash/strobe duration
Posted by Sakari Ailus 9 months ago
Hi Richard,

On Fri, Mar 14, 2025 at 11:25:09AM +0100, Richard Leitner wrote:
> On Fri, Mar 14, 2025 at 09:20:23AM +0000, Sakari Ailus wrote:
> > Hi Richard,
> > 
> > Thanks for the set.
> 
> Hi Sakari,
> thanks for the quick response!
> 
> > 
> > On Fri, Mar 14, 2025 at 09:49:55AM +0100, Richard Leitner wrote:
> > > Add a control V4L2_CID_FLASH_DURATION to set the duration of a
> > > flash/strobe pulse. This is different to the V4L2_CID_FLASH_TIMEOUT
> > > control, as the timeout defines a limit after which the flash is
> > > "forcefully" turned off again.
> > > 
> > > On the other hand the new V4L2_CID_FLASH_DURATION is the desired length
> > > of the flash/strobe pulse
> > 
> > What's the actual difference between the two? To me they appear the same,
> > just expressed in a different way.
> 
> According to FLASH_TIMEOUT documentation:
> 
> 	Hardware timeout for flash. The flash strobe is stopped after this
> 	period of time has passed from the start of the strobe. [1]
> 
> This is a little bit unspecific, but as also discussed with Dave [2]
> according to the documentation of V4L2_FLASH_FAULT_TIMEOUT it seems to
> be targeted at providing a "real timeout" control, not settings the
> desired duration:
> 
> 	The flash strobe was still on when the timeout set by the user
> 	--- V4L2_CID_FLASH_TIMEOUT control --- has expired. Not all flash
> 	controllers may set this in all such conditions. [1]
> 
> If I understood that wrong, I'm also happy to use FLASH_TIMEOUT for this
> use-case. But tbh I think FLASH_DURATION would be more specific.
> 
> As this still seems unclear: Should the documentation be
> changed/rewritten if we stick with the FLASH_DURATION approach?
> 
> [1] https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-flash.html
> [2] https://lore.kernel.org/lkml/CAPY8ntB8i4OyUWAL8k899yUd5QsRifJXiOfWXKceGQ7TNZ4OUw@mail.gmail.com/

Right, I think I can see what you're after.

How does the sensor determine when to start the strobe, i.e. on which frame
and which part of the exposure of that frame?

> 
> > 
> > > 
> > > Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> > > ---
> > >  drivers/media/v4l2-core/v4l2-ctrls-defs.c | 1 +
> > >  include/uapi/linux/v4l2-controls.h        | 1 +
> > >  2 files changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > > index 1ea52011247accc51d0261f56eab1cf13c0624a0..f9ed7273a9f3eafe01c31b638e1c8d9fcf5424af 100644
> > > --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > > +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > > @@ -1135,6 +1135,7 @@ const char *v4l2_ctrl_get_name(u32 id)
> > >  	case V4L2_CID_FLASH_FAULT:		return "Faults";
> > >  	case V4L2_CID_FLASH_CHARGE:		return "Charge";
> > >  	case V4L2_CID_FLASH_READY:		return "Ready to Strobe";
> > > +	case V4L2_CID_FLASH_DURATION:		return "Strobe Duration";
> > >  
> > >  	/* JPEG encoder controls */
> > >  	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
> > > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> > > index 974fd254e57309e6def95b4a4f8e4de13a3972a7..80050cadb8377e3070ebbadc493fcd08b2c12c0b 100644
> > > --- a/include/uapi/linux/v4l2-controls.h
> > > +++ b/include/uapi/linux/v4l2-controls.h
> > > @@ -1173,6 +1173,7 @@ enum v4l2_flash_strobe_source {
> > >  
> > >  #define V4L2_CID_FLASH_CHARGE			(V4L2_CID_FLASH_CLASS_BASE + 11)
> > >  #define V4L2_CID_FLASH_READY			(V4L2_CID_FLASH_CLASS_BASE + 12)
> > > +#define V4L2_CID_FLASH_DURATION			(V4L2_CID_FLASH_CLASS_BASE + 13)
> > >  
> > >  
> > >  /* JPEG-class control IDs */
> > > 
> > 

-- 
Regards,

Sakari Ailus
Re: [PATCH v2 1/8] media: v4l: ctrls: add a control for flash/strobe duration
Posted by Richard Leitner 9 months ago
Hi Sakari,

On Fri, Mar 14, 2025 at 01:34:07PM +0000, Sakari Ailus wrote:
> Hi Richard,
> 
> On Fri, Mar 14, 2025 at 11:25:09AM +0100, Richard Leitner wrote:
> > On Fri, Mar 14, 2025 at 09:20:23AM +0000, Sakari Ailus wrote:
[...]
> > > On Fri, Mar 14, 2025 at 09:49:55AM +0100, Richard Leitner wrote:
> > > > Add a control V4L2_CID_FLASH_DURATION to set the duration of a
> > > > flash/strobe pulse. This is different to the V4L2_CID_FLASH_TIMEOUT
> > > > control, as the timeout defines a limit after which the flash is
> > > > "forcefully" turned off again.
> > > > 
> > > > On the other hand the new V4L2_CID_FLASH_DURATION is the desired length
> > > > of the flash/strobe pulse
> > > 
> > > What's the actual difference between the two? To me they appear the same,
> > > just expressed in a different way.
> > 
> > According to FLASH_TIMEOUT documentation:
> > 
> > 	Hardware timeout for flash. The flash strobe is stopped after this
> > 	period of time has passed from the start of the strobe. [1]
> > 
> > This is a little bit unspecific, but as also discussed with Dave [2]
> > according to the documentation of V4L2_FLASH_FAULT_TIMEOUT it seems to
> > be targeted at providing a "real timeout" control, not settings the
> > desired duration:
> > 
> > 	The flash strobe was still on when the timeout set by the user
> > 	--- V4L2_CID_FLASH_TIMEOUT control --- has expired. Not all flash
> > 	controllers may set this in all such conditions. [1]
> > 
> > If I understood that wrong, I'm also happy to use FLASH_TIMEOUT for this
> > use-case. But tbh I think FLASH_DURATION would be more specific.
> > 
> > As this still seems unclear: Should the documentation be
> > changed/rewritten if we stick with the FLASH_DURATION approach?
> > 
> > [1] https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-flash.html
> > [2] https://lore.kernel.org/lkml/CAPY8ntB8i4OyUWAL8k899yUd5QsRifJXiOfWXKceGQ7TNZ4OUw@mail.gmail.com/
> 
> Right, I think I can see what you're after.
> 
> How does the sensor determine when to start the strobe, i.e. on which frame
> and which part of the exposure of that frame?

In general I think it's not part of V4L2_CID_FLASH_DURATION to take any
assumptions on that, as that's sensor/flash specific IMHO.

In case of the ov9282 sensor driver (which is also part of this series)
the strobe is started synchronously with the exposure on each frame
start.
Being even more specific on the ov9292, the sensor also offers the
possibility to shift that strobe start in in either direction using a
register. Implementing this "flash shift" (as it's called in the sensors
datasheet) is currently under test on my side. I will likely send a
series for that in the coming weeks.

> > > > Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> > > > ---
> > > >  drivers/media/v4l2-core/v4l2-ctrls-defs.c | 1 +
> > > >  include/uapi/linux/v4l2-controls.h        | 1 +
> > > >  2 files changed, 2 insertions(+)
[...]
> 
> -- 
> Regards,
> 
> Sakari Ailus

Thanks!
Richard
Re: [PATCH v2 1/8] media: v4l: ctrls: add a control for flash/strobe duration
Posted by Sakari Ailus 9 months ago
Hi Richard,

On Fri, Mar 14, 2025 at 05:08:16PM +0100, Richard Leitner wrote:
> Hi Sakari,
> 
> On Fri, Mar 14, 2025 at 01:34:07PM +0000, Sakari Ailus wrote:
> > Hi Richard,
> > 
> > On Fri, Mar 14, 2025 at 11:25:09AM +0100, Richard Leitner wrote:
> > > On Fri, Mar 14, 2025 at 09:20:23AM +0000, Sakari Ailus wrote:
> [...]
> > > > On Fri, Mar 14, 2025 at 09:49:55AM +0100, Richard Leitner wrote:
> > > > > Add a control V4L2_CID_FLASH_DURATION to set the duration of a
> > > > > flash/strobe pulse. This is different to the V4L2_CID_FLASH_TIMEOUT
> > > > > control, as the timeout defines a limit after which the flash is
> > > > > "forcefully" turned off again.
> > > > > 
> > > > > On the other hand the new V4L2_CID_FLASH_DURATION is the desired length
> > > > > of the flash/strobe pulse
> > > > 
> > > > What's the actual difference between the two? To me they appear the same,
> > > > just expressed in a different way.
> > > 
> > > According to FLASH_TIMEOUT documentation:
> > > 
> > > 	Hardware timeout for flash. The flash strobe is stopped after this
> > > 	period of time has passed from the start of the strobe. [1]
> > > 
> > > This is a little bit unspecific, but as also discussed with Dave [2]
> > > according to the documentation of V4L2_FLASH_FAULT_TIMEOUT it seems to
> > > be targeted at providing a "real timeout" control, not settings the
> > > desired duration:
> > > 
> > > 	The flash strobe was still on when the timeout set by the user
> > > 	--- V4L2_CID_FLASH_TIMEOUT control --- has expired. Not all flash
> > > 	controllers may set this in all such conditions. [1]
> > > 
> > > If I understood that wrong, I'm also happy to use FLASH_TIMEOUT for this
> > > use-case. But tbh I think FLASH_DURATION would be more specific.
> > > 
> > > As this still seems unclear: Should the documentation be
> > > changed/rewritten if we stick with the FLASH_DURATION approach?
> > > 
> > > [1] https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-flash.html
> > > [2] https://lore.kernel.org/lkml/CAPY8ntB8i4OyUWAL8k899yUd5QsRifJXiOfWXKceGQ7TNZ4OUw@mail.gmail.com/
> > 
> > Right, I think I can see what you're after.
> > 
> > How does the sensor determine when to start the strobe, i.e. on which frame
> > and which part of the exposure of that frame?
> 
> In general I think it's not part of V4L2_CID_FLASH_DURATION to take any
> assumptions on that, as that's sensor/flash specific IMHO.
> 
> In case of the ov9282 sensor driver (which is also part of this series)
> the strobe is started synchronously with the exposure on each frame
> start.
> Being even more specific on the ov9292, the sensor also offers the
> possibility to shift that strobe start in in either direction using a
> register. Implementing this "flash shift" (as it's called in the sensors
> datasheet) is currently under test on my side. I will likely send a
> series for that in the coming weeks.

Ok, so you get a single frame exposed with a flash when you start
streaming, is that correct?

> 
> > > > > Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> > > > > ---
> > > > >  drivers/media/v4l2-core/v4l2-ctrls-defs.c | 1 +
> > > > >  include/uapi/linux/v4l2-controls.h        | 1 +
> > > > >  2 files changed, 2 insertions(+)
> [...]
> > 

-- 
Regards,

Sakari Ailus
Re: [PATCH v2 1/8] media: v4l: ctrls: add a control for flash/strobe duration
Posted by Richard Leitner 9 months ago
On Tue, Mar 18, 2025 at 01:28:01PM +0000, Sakari Ailus wrote:
> Hi Richard,
> 
> On Fri, Mar 14, 2025 at 05:08:16PM +0100, Richard Leitner wrote:
> > Hi Sakari,
> > 
> > On Fri, Mar 14, 2025 at 01:34:07PM +0000, Sakari Ailus wrote:
> > > Hi Richard,
> > > 
> > > On Fri, Mar 14, 2025 at 11:25:09AM +0100, Richard Leitner wrote:
> > > > On Fri, Mar 14, 2025 at 09:20:23AM +0000, Sakari Ailus wrote:
> > [...]
> > > > > On Fri, Mar 14, 2025 at 09:49:55AM +0100, Richard Leitner wrote:
> > > > > > Add a control V4L2_CID_FLASH_DURATION to set the duration of a
> > > > > > flash/strobe pulse. This is different to the V4L2_CID_FLASH_TIMEOUT
> > > > > > control, as the timeout defines a limit after which the flash is
> > > > > > "forcefully" turned off again.
> > > > > > 
> > > > > > On the other hand the new V4L2_CID_FLASH_DURATION is the desired length
> > > > > > of the flash/strobe pulse
> > > > > 
> > > > > What's the actual difference between the two? To me they appear the same,
> > > > > just expressed in a different way.
> > > > 
> > > > According to FLASH_TIMEOUT documentation:
> > > > 
> > > > 	Hardware timeout for flash. The flash strobe is stopped after this
> > > > 	period of time has passed from the start of the strobe. [1]
> > > > 
> > > > This is a little bit unspecific, but as also discussed with Dave [2]
> > > > according to the documentation of V4L2_FLASH_FAULT_TIMEOUT it seems to
> > > > be targeted at providing a "real timeout" control, not settings the
> > > > desired duration:
> > > > 
> > > > 	The flash strobe was still on when the timeout set by the user
> > > > 	--- V4L2_CID_FLASH_TIMEOUT control --- has expired. Not all flash
> > > > 	controllers may set this in all such conditions. [1]
> > > > 
> > > > If I understood that wrong, I'm also happy to use FLASH_TIMEOUT for this
> > > > use-case. But tbh I think FLASH_DURATION would be more specific.
> > > > 
> > > > As this still seems unclear: Should the documentation be
> > > > changed/rewritten if we stick with the FLASH_DURATION approach?
> > > > 
> > > > [1] https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-flash.html
> > > > [2] https://lore.kernel.org/lkml/CAPY8ntB8i4OyUWAL8k899yUd5QsRifJXiOfWXKceGQ7TNZ4OUw@mail.gmail.com/
> > > 
> > > Right, I think I can see what you're after.
> > > 
> > > How does the sensor determine when to start the strobe, i.e. on which frame
> > > and which part of the exposure of that frame?
> > 
> > In general I think it's not part of V4L2_CID_FLASH_DURATION to take any
> > assumptions on that, as that's sensor/flash specific IMHO.
> > 
> > In case of the ov9282 sensor driver (which is also part of this series)
> > the strobe is started synchronously with the exposure on each frame
> > start.
> > Being even more specific on the ov9292, the sensor also offers the
> > possibility to shift that strobe start in in either direction using a
> > register. Implementing this "flash shift" (as it's called in the sensors
> > datasheet) is currently under test on my side. I will likely send a
> > series for that in the coming weeks.
> 
> Ok, so you get a single frame exposed with a flash when you start
> streaming, is that correct?

Correct. The flash is switched on for the configured duration at every
frame exposure (the sensor has a global shutter) as long as the camera is
streaming.

Maybe to following visualization of configured flash and exposure times help:

             _________        _________        _________
exposure: __|         |______|         |______|         |__

             __               __               __
flash:    __|  |_____________|  |_____________|  |_________
            ^^^^
      strobe_duration


regards;rl

> 
> > 
> > > > > > Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
> > > > > > ---
> > > > > >  drivers/media/v4l2-core/v4l2-ctrls-defs.c | 1 +
> > > > > >  include/uapi/linux/v4l2-controls.h        | 1 +
> > > > > >  2 files changed, 2 insertions(+)
> > [...]
> > > 
> 
> -- 
> Regards,
> 
> Sakari Ailus
Re: [PATCH v2 1/8] media: v4l: ctrls: add a control for flash/strobe duration
Posted by Sakari Ailus 9 months ago
Hi Richard,

On Tue, Mar 18, 2025 at 02:42:53PM +0100, Richard Leitner wrote:
> On Tue, Mar 18, 2025 at 01:28:01PM +0000, Sakari Ailus wrote:
> > Hi Richard,
> > 
> > On Fri, Mar 14, 2025 at 05:08:16PM +0100, Richard Leitner wrote:
> > > Hi Sakari,
> > > 
> > > On Fri, Mar 14, 2025 at 01:34:07PM +0000, Sakari Ailus wrote:
> > > > Hi Richard,
> > > > 
> > > > On Fri, Mar 14, 2025 at 11:25:09AM +0100, Richard Leitner wrote:
> > > > > On Fri, Mar 14, 2025 at 09:20:23AM +0000, Sakari Ailus wrote:
> > > [...]
> > > > > > On Fri, Mar 14, 2025 at 09:49:55AM +0100, Richard Leitner wrote:
> > > > > > > Add a control V4L2_CID_FLASH_DURATION to set the duration of a
> > > > > > > flash/strobe pulse. This is different to the V4L2_CID_FLASH_TIMEOUT
> > > > > > > control, as the timeout defines a limit after which the flash is
> > > > > > > "forcefully" turned off again.
> > > > > > > 
> > > > > > > On the other hand the new V4L2_CID_FLASH_DURATION is the desired length
> > > > > > > of the flash/strobe pulse
> > > > > > 
> > > > > > What's the actual difference between the two? To me they appear the same,
> > > > > > just expressed in a different way.
> > > > > 
> > > > > According to FLASH_TIMEOUT documentation:
> > > > > 
> > > > > 	Hardware timeout for flash. The flash strobe is stopped after this
> > > > > 	period of time has passed from the start of the strobe. [1]
> > > > > 
> > > > > This is a little bit unspecific, but as also discussed with Dave [2]
> > > > > according to the documentation of V4L2_FLASH_FAULT_TIMEOUT it seems to
> > > > > be targeted at providing a "real timeout" control, not settings the
> > > > > desired duration:
> > > > > 
> > > > > 	The flash strobe was still on when the timeout set by the user
> > > > > 	--- V4L2_CID_FLASH_TIMEOUT control --- has expired. Not all flash
> > > > > 	controllers may set this in all such conditions. [1]
> > > > > 
> > > > > If I understood that wrong, I'm also happy to use FLASH_TIMEOUT for this
> > > > > use-case. But tbh I think FLASH_DURATION would be more specific.
> > > > > 
> > > > > As this still seems unclear: Should the documentation be
> > > > > changed/rewritten if we stick with the FLASH_DURATION approach?
> > > > > 
> > > > > [1] https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-flash.html
> > > > > [2] https://lore.kernel.org/lkml/CAPY8ntB8i4OyUWAL8k899yUd5QsRifJXiOfWXKceGQ7TNZ4OUw@mail.gmail.com/
> > > > 
> > > > Right, I think I can see what you're after.
> > > > 
> > > > How does the sensor determine when to start the strobe, i.e. on which frame
> > > > and which part of the exposure of that frame?
> > > 
> > > In general I think it's not part of V4L2_CID_FLASH_DURATION to take any
> > > assumptions on that, as that's sensor/flash specific IMHO.
> > > 
> > > In case of the ov9282 sensor driver (which is also part of this series)
> > > the strobe is started synchronously with the exposure on each frame
> > > start.
> > > Being even more specific on the ov9292, the sensor also offers the
> > > possibility to shift that strobe start in in either direction using a
> > > register. Implementing this "flash shift" (as it's called in the sensors
> > > datasheet) is currently under test on my side. I will likely send a
> > > series for that in the coming weeks.
> > 
> > Ok, so you get a single frame exposed with a flash when you start
> > streaming, is that correct?
> 
> Correct. The flash is switched on for the configured duration at every
> frame exposure (the sensor has a global shutter) as long as the camera is
> streaming.
> 
> Maybe to following visualization of configured flash and exposure times help:
> 
>              _________        _________        _________
> exposure: __|         |______|         |______|         |__
> 
>              __               __               __
> flash:    __|  |_____________|  |_____________|  |_________
>             ^^^^
>       strobe_duration

That diagram would work for global shutter but not for the much, much more
common rolling shutter operation. Does the driver use the sensor in rolling
shutter mode? This isn't very common with LED flashes.

-- 
Regards,

Sakari Ailus
Re: [PATCH v2 1/8] media: v4l: ctrls: add a control for flash/strobe duration
Posted by Richard Leitner 9 months ago
On Tue, Mar 18, 2025 at 02:06:50PM +0000, Sakari Ailus wrote:
> Hi Richard,
> 
> On Tue, Mar 18, 2025 at 02:42:53PM +0100, Richard Leitner wrote:
> > On Tue, Mar 18, 2025 at 01:28:01PM +0000, Sakari Ailus wrote:
> > > Hi Richard,
> > > 
> > > On Fri, Mar 14, 2025 at 05:08:16PM +0100, Richard Leitner wrote:
> > > > Hi Sakari,
> > > > 
> > > > On Fri, Mar 14, 2025 at 01:34:07PM +0000, Sakari Ailus wrote:
> > > > > Hi Richard,
> > > > > 
> > > > > On Fri, Mar 14, 2025 at 11:25:09AM +0100, Richard Leitner wrote:
> > > > > > On Fri, Mar 14, 2025 at 09:20:23AM +0000, Sakari Ailus wrote:
> > > > [...]
> > > > > > > On Fri, Mar 14, 2025 at 09:49:55AM +0100, Richard Leitner wrote:
> > > > > > > > Add a control V4L2_CID_FLASH_DURATION to set the duration of a
> > > > > > > > flash/strobe pulse. This is different to the V4L2_CID_FLASH_TIMEOUT
> > > > > > > > control, as the timeout defines a limit after which the flash is
> > > > > > > > "forcefully" turned off again.
> > > > > > > > 
> > > > > > > > On the other hand the new V4L2_CID_FLASH_DURATION is the desired length
> > > > > > > > of the flash/strobe pulse
> > > > > > > 
> > > > > > > What's the actual difference between the two? To me they appear the same,
> > > > > > > just expressed in a different way.
> > > > > > 
> > > > > > According to FLASH_TIMEOUT documentation:
> > > > > > 
> > > > > > 	Hardware timeout for flash. The flash strobe is stopped after this
> > > > > > 	period of time has passed from the start of the strobe. [1]
> > > > > > 
> > > > > > This is a little bit unspecific, but as also discussed with Dave [2]
> > > > > > according to the documentation of V4L2_FLASH_FAULT_TIMEOUT it seems to
> > > > > > be targeted at providing a "real timeout" control, not settings the
> > > > > > desired duration:
> > > > > > 
> > > > > > 	The flash strobe was still on when the timeout set by the user
> > > > > > 	--- V4L2_CID_FLASH_TIMEOUT control --- has expired. Not all flash
> > > > > > 	controllers may set this in all such conditions. [1]
> > > > > > 
> > > > > > If I understood that wrong, I'm also happy to use FLASH_TIMEOUT for this
> > > > > > use-case. But tbh I think FLASH_DURATION would be more specific.
> > > > > > 
> > > > > > As this still seems unclear: Should the documentation be
> > > > > > changed/rewritten if we stick with the FLASH_DURATION approach?
> > > > > > 
> > > > > > [1] https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-flash.html
> > > > > > [2] https://lore.kernel.org/lkml/CAPY8ntB8i4OyUWAL8k899yUd5QsRifJXiOfWXKceGQ7TNZ4OUw@mail.gmail.com/
> > > > > 
> > > > > Right, I think I can see what you're after.
> > > > > 
> > > > > How does the sensor determine when to start the strobe, i.e. on which frame
> > > > > and which part of the exposure of that frame?
> > > > 
> > > > In general I think it's not part of V4L2_CID_FLASH_DURATION to take any
> > > > assumptions on that, as that's sensor/flash specific IMHO.
> > > > 
> > > > In case of the ov9282 sensor driver (which is also part of this series)
> > > > the strobe is started synchronously with the exposure on each frame
> > > > start.
> > > > Being even more specific on the ov9292, the sensor also offers the
> > > > possibility to shift that strobe start in in either direction using a
> > > > register. Implementing this "flash shift" (as it's called in the sensors
> > > > datasheet) is currently under test on my side. I will likely send a
> > > > series for that in the coming weeks.
> > > 
> > > Ok, so you get a single frame exposed with a flash when you start
> > > streaming, is that correct?
> > 
> > Correct. The flash is switched on for the configured duration at every
> > frame exposure (the sensor has a global shutter) as long as the camera is
> > streaming.
> > 
> > Maybe to following visualization of configured flash and exposure times help:
> > 
> >              _________        _________        _________
> > exposure: __|         |______|         |______|         |__
> > 
> >              __               __               __
> > flash:    __|  |_____________|  |_____________|  |_________
> >             ^^^^
> >       strobe_duration
> 
> That diagram would work for global shutter but not for the much, much more
> common rolling shutter operation. Does the driver use the sensor in rolling
> shutter mode? This isn't very common with LED flashes.

The ov9282 driver uses the sensor in global shutter mode.

I totally agree with your statement. This pattern is only useful for
global shutter operation.

> 
> -- 
> Regards,
> 
> Sakari Ailus
Re: [PATCH v2 1/8] media: v4l: ctrls: add a control for flash/strobe duration
Posted by Sakari Ailus 9 months ago
Hi Richard,

On Tue, Mar 18, 2025 at 03:46:18PM +0100, Richard Leitner wrote:
> On Tue, Mar 18, 2025 at 02:06:50PM +0000, Sakari Ailus wrote:
> > Hi Richard,
> > 
> > On Tue, Mar 18, 2025 at 02:42:53PM +0100, Richard Leitner wrote:
> > > On Tue, Mar 18, 2025 at 01:28:01PM +0000, Sakari Ailus wrote:
> > > > Hi Richard,
> > > > 
> > > > On Fri, Mar 14, 2025 at 05:08:16PM +0100, Richard Leitner wrote:
> > > > > Hi Sakari,
> > > > > 
> > > > > On Fri, Mar 14, 2025 at 01:34:07PM +0000, Sakari Ailus wrote:
> > > > > > Hi Richard,
> > > > > > 
> > > > > > On Fri, Mar 14, 2025 at 11:25:09AM +0100, Richard Leitner wrote:
> > > > > > > On Fri, Mar 14, 2025 at 09:20:23AM +0000, Sakari Ailus wrote:
> > > > > [...]
> > > > > > > > On Fri, Mar 14, 2025 at 09:49:55AM +0100, Richard Leitner wrote:
> > > > > > > > > Add a control V4L2_CID_FLASH_DURATION to set the duration of a
> > > > > > > > > flash/strobe pulse. This is different to the V4L2_CID_FLASH_TIMEOUT
> > > > > > > > > control, as the timeout defines a limit after which the flash is
> > > > > > > > > "forcefully" turned off again.
> > > > > > > > > 
> > > > > > > > > On the other hand the new V4L2_CID_FLASH_DURATION is the desired length
> > > > > > > > > of the flash/strobe pulse
> > > > > > > > 
> > > > > > > > What's the actual difference between the two? To me they appear the same,
> > > > > > > > just expressed in a different way.
> > > > > > > 
> > > > > > > According to FLASH_TIMEOUT documentation:
> > > > > > > 
> > > > > > > 	Hardware timeout for flash. The flash strobe is stopped after this
> > > > > > > 	period of time has passed from the start of the strobe. [1]
> > > > > > > 
> > > > > > > This is a little bit unspecific, but as also discussed with Dave [2]
> > > > > > > according to the documentation of V4L2_FLASH_FAULT_TIMEOUT it seems to
> > > > > > > be targeted at providing a "real timeout" control, not settings the
> > > > > > > desired duration:
> > > > > > > 
> > > > > > > 	The flash strobe was still on when the timeout set by the user
> > > > > > > 	--- V4L2_CID_FLASH_TIMEOUT control --- has expired. Not all flash
> > > > > > > 	controllers may set this in all such conditions. [1]
> > > > > > > 
> > > > > > > If I understood that wrong, I'm also happy to use FLASH_TIMEOUT for this
> > > > > > > use-case. But tbh I think FLASH_DURATION would be more specific.
> > > > > > > 
> > > > > > > As this still seems unclear: Should the documentation be
> > > > > > > changed/rewritten if we stick with the FLASH_DURATION approach?
> > > > > > > 
> > > > > > > [1] https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-flash.html
> > > > > > > [2] https://lore.kernel.org/lkml/CAPY8ntB8i4OyUWAL8k899yUd5QsRifJXiOfWXKceGQ7TNZ4OUw@mail.gmail.com/
> > > > > > 
> > > > > > Right, I think I can see what you're after.
> > > > > > 
> > > > > > How does the sensor determine when to start the strobe, i.e. on which frame
> > > > > > and which part of the exposure of that frame?
> > > > > 
> > > > > In general I think it's not part of V4L2_CID_FLASH_DURATION to take any
> > > > > assumptions on that, as that's sensor/flash specific IMHO.
> > > > > 
> > > > > In case of the ov9282 sensor driver (which is also part of this series)
> > > > > the strobe is started synchronously with the exposure on each frame
> > > > > start.
> > > > > Being even more specific on the ov9292, the sensor also offers the
> > > > > possibility to shift that strobe start in in either direction using a
> > > > > register. Implementing this "flash shift" (as it's called in the sensors
> > > > > datasheet) is currently under test on my side. I will likely send a
> > > > > series for that in the coming weeks.
> > > > 
> > > > Ok, so you get a single frame exposed with a flash when you start
> > > > streaming, is that correct?
> > > 
> > > Correct. The flash is switched on for the configured duration at every
> > > frame exposure (the sensor has a global shutter) as long as the camera is
> > > streaming.
> > > 
> > > Maybe to following visualization of configured flash and exposure times help:
> > > 
> > >              _________        _________        _________
> > > exposure: __|         |______|         |______|         |__
> > > 
> > >              __               __               __
> > > flash:    __|  |_____________|  |_____________|  |_________
> > >             ^^^^
> > >       strobe_duration
> > 
> > That diagram would work for global shutter but not for the much, much more
> > common rolling shutter operation. Does the driver use the sensor in rolling
> > shutter mode? This isn't very common with LED flashes.
> 
> The ov9282 driver uses the sensor in global shutter mode.
> 
> I totally agree with your statement. This pattern is only useful for
> global shutter operation.

I think (nearly?) all supported sensors use a rolling shutter.

Could you include a comment on this to the driver?

I wonder what Laurent thinks.

-- 
Kind regards,

Sakari Ailus
Re: [PATCH v2 1/8] media: v4l: ctrls: add a control for flash/strobe duration
Posted by Dave Stevenson 9 months ago
Hi Sakari

On Tue, 18 Mar 2025 at 15:11, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
>
> Hi Richard,
>
> On Tue, Mar 18, 2025 at 03:46:18PM +0100, Richard Leitner wrote:
> > On Tue, Mar 18, 2025 at 02:06:50PM +0000, Sakari Ailus wrote:
> > > Hi Richard,
> > >
> > > On Tue, Mar 18, 2025 at 02:42:53PM +0100, Richard Leitner wrote:
> > > > On Tue, Mar 18, 2025 at 01:28:01PM +0000, Sakari Ailus wrote:
> > > > > Hi Richard,
> > > > >
> > > > > On Fri, Mar 14, 2025 at 05:08:16PM +0100, Richard Leitner wrote:
> > > > > > Hi Sakari,
> > > > > >
> > > > > > On Fri, Mar 14, 2025 at 01:34:07PM +0000, Sakari Ailus wrote:
> > > > > > > Hi Richard,
> > > > > > >
> > > > > > > On Fri, Mar 14, 2025 at 11:25:09AM +0100, Richard Leitner wrote:
> > > > > > > > On Fri, Mar 14, 2025 at 09:20:23AM +0000, Sakari Ailus wrote:
> > > > > > [...]
> > > > > > > > > On Fri, Mar 14, 2025 at 09:49:55AM +0100, Richard Leitner wrote:
> > > > > > > > > > Add a control V4L2_CID_FLASH_DURATION to set the duration of a
> > > > > > > > > > flash/strobe pulse. This is different to the V4L2_CID_FLASH_TIMEOUT
> > > > > > > > > > control, as the timeout defines a limit after which the flash is
> > > > > > > > > > "forcefully" turned off again.
> > > > > > > > > >
> > > > > > > > > > On the other hand the new V4L2_CID_FLASH_DURATION is the desired length
> > > > > > > > > > of the flash/strobe pulse
> > > > > > > > >
> > > > > > > > > What's the actual difference between the two? To me they appear the same,
> > > > > > > > > just expressed in a different way.
> > > > > > > >
> > > > > > > > According to FLASH_TIMEOUT documentation:
> > > > > > > >
> > > > > > > >   Hardware timeout for flash. The flash strobe is stopped after this
> > > > > > > >   period of time has passed from the start of the strobe. [1]
> > > > > > > >
> > > > > > > > This is a little bit unspecific, but as also discussed with Dave [2]
> > > > > > > > according to the documentation of V4L2_FLASH_FAULT_TIMEOUT it seems to
> > > > > > > > be targeted at providing a "real timeout" control, not settings the
> > > > > > > > desired duration:
> > > > > > > >
> > > > > > > >   The flash strobe was still on when the timeout set by the user
> > > > > > > >   --- V4L2_CID_FLASH_TIMEOUT control --- has expired. Not all flash
> > > > > > > >   controllers may set this in all such conditions. [1]
> > > > > > > >
> > > > > > > > If I understood that wrong, I'm also happy to use FLASH_TIMEOUT for this
> > > > > > > > use-case. But tbh I think FLASH_DURATION would be more specific.
> > > > > > > >
> > > > > > > > As this still seems unclear: Should the documentation be
> > > > > > > > changed/rewritten if we stick with the FLASH_DURATION approach?
> > > > > > > >
> > > > > > > > [1] https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-flash.html
> > > > > > > > [2] https://lore.kernel.org/lkml/CAPY8ntB8i4OyUWAL8k899yUd5QsRifJXiOfWXKceGQ7TNZ4OUw@mail.gmail.com/
> > > > > > >
> > > > > > > Right, I think I can see what you're after.
> > > > > > >
> > > > > > > How does the sensor determine when to start the strobe, i.e. on which frame
> > > > > > > and which part of the exposure of that frame?
> > > > > >
> > > > > > In general I think it's not part of V4L2_CID_FLASH_DURATION to take any
> > > > > > assumptions on that, as that's sensor/flash specific IMHO.
> > > > > >
> > > > > > In case of the ov9282 sensor driver (which is also part of this series)
> > > > > > the strobe is started synchronously with the exposure on each frame
> > > > > > start.
> > > > > > Being even more specific on the ov9292, the sensor also offers the
> > > > > > possibility to shift that strobe start in in either direction using a
> > > > > > register. Implementing this "flash shift" (as it's called in the sensors
> > > > > > datasheet) is currently under test on my side. I will likely send a
> > > > > > series for that in the coming weeks.
> > > > >
> > > > > Ok, so you get a single frame exposed with a flash when you start
> > > > > streaming, is that correct?
> > > >
> > > > Correct. The flash is switched on for the configured duration at every
> > > > frame exposure (the sensor has a global shutter) as long as the camera is
> > > > streaming.
> > > >
> > > > Maybe to following visualization of configured flash and exposure times help:
> > > >
> > > >              _________        _________        _________
> > > > exposure: __|         |______|         |______|         |__
> > > >
> > > >              __               __               __
> > > > flash:    __|  |_____________|  |_____________|  |_________
> > > >             ^^^^
> > > >       strobe_duration
> > >
> > > That diagram would work for global shutter but not for the much, much more
> > > common rolling shutter operation. Does the driver use the sensor in rolling
> > > shutter mode? This isn't very common with LED flashes.
> >
> > The ov9282 driver uses the sensor in global shutter mode.
> >
> > I totally agree with your statement. This pattern is only useful for
> > global shutter operation.
>
> I think (nearly?) all supported sensors use a rolling shutter.

You've got at least two other global shutter sensors supported in
mainline - Omnivision ov7251 and Sony imx296.
Patches have been posted for OnSemi ar0144 (Laurent) and ar0234
(Dongcheng), which are also both global shutter sensors.

So yes they are in the minority, but not that uncommon.

  Dave



> Could you include a comment on this to the driver?
>
> I wonder what Laurent thinks.
>
> --
> Kind regards,
>
> Sakari Ailus
Re: [PATCH v2 1/8] media: v4l: ctrls: add a control for flash/strobe duration
Posted by Sakari Ailus 9 months ago
Hi Dave,

On Tue, Mar 18, 2025 at 04:39:05PM +0000, Dave Stevenson wrote:
> Hi Sakari
> 
> On Tue, 18 Mar 2025 at 15:11, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Richard,
> >
> > On Tue, Mar 18, 2025 at 03:46:18PM +0100, Richard Leitner wrote:
> > > On Tue, Mar 18, 2025 at 02:06:50PM +0000, Sakari Ailus wrote:
> > > > Hi Richard,
> > > >
> > > > On Tue, Mar 18, 2025 at 02:42:53PM +0100, Richard Leitner wrote:
> > > > > On Tue, Mar 18, 2025 at 01:28:01PM +0000, Sakari Ailus wrote:
> > > > > > Hi Richard,
> > > > > >
> > > > > > On Fri, Mar 14, 2025 at 05:08:16PM +0100, Richard Leitner wrote:
> > > > > > > Hi Sakari,
> > > > > > >
> > > > > > > On Fri, Mar 14, 2025 at 01:34:07PM +0000, Sakari Ailus wrote:
> > > > > > > > Hi Richard,
> > > > > > > >
> > > > > > > > On Fri, Mar 14, 2025 at 11:25:09AM +0100, Richard Leitner wrote:
> > > > > > > > > On Fri, Mar 14, 2025 at 09:20:23AM +0000, Sakari Ailus wrote:
> > > > > > > [...]
> > > > > > > > > > On Fri, Mar 14, 2025 at 09:49:55AM +0100, Richard Leitner wrote:
> > > > > > > > > > > Add a control V4L2_CID_FLASH_DURATION to set the duration of a
> > > > > > > > > > > flash/strobe pulse. This is different to the V4L2_CID_FLASH_TIMEOUT
> > > > > > > > > > > control, as the timeout defines a limit after which the flash is
> > > > > > > > > > > "forcefully" turned off again.
> > > > > > > > > > >
> > > > > > > > > > > On the other hand the new V4L2_CID_FLASH_DURATION is the desired length
> > > > > > > > > > > of the flash/strobe pulse
> > > > > > > > > >
> > > > > > > > > > What's the actual difference between the two? To me they appear the same,
> > > > > > > > > > just expressed in a different way.
> > > > > > > > >
> > > > > > > > > According to FLASH_TIMEOUT documentation:
> > > > > > > > >
> > > > > > > > >   Hardware timeout for flash. The flash strobe is stopped after this
> > > > > > > > >   period of time has passed from the start of the strobe. [1]
> > > > > > > > >
> > > > > > > > > This is a little bit unspecific, but as also discussed with Dave [2]
> > > > > > > > > according to the documentation of V4L2_FLASH_FAULT_TIMEOUT it seems to
> > > > > > > > > be targeted at providing a "real timeout" control, not settings the
> > > > > > > > > desired duration:
> > > > > > > > >
> > > > > > > > >   The flash strobe was still on when the timeout set by the user
> > > > > > > > >   --- V4L2_CID_FLASH_TIMEOUT control --- has expired. Not all flash
> > > > > > > > >   controllers may set this in all such conditions. [1]
> > > > > > > > >
> > > > > > > > > If I understood that wrong, I'm also happy to use FLASH_TIMEOUT for this
> > > > > > > > > use-case. But tbh I think FLASH_DURATION would be more specific.
> > > > > > > > >
> > > > > > > > > As this still seems unclear: Should the documentation be
> > > > > > > > > changed/rewritten if we stick with the FLASH_DURATION approach?
> > > > > > > > >
> > > > > > > > > [1] https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-flash.html
> > > > > > > > > [2] https://lore.kernel.org/lkml/CAPY8ntB8i4OyUWAL8k899yUd5QsRifJXiOfWXKceGQ7TNZ4OUw@mail.gmail.com/
> > > > > > > >
> > > > > > > > Right, I think I can see what you're after.
> > > > > > > >
> > > > > > > > How does the sensor determine when to start the strobe, i.e. on which frame
> > > > > > > > and which part of the exposure of that frame?
> > > > > > >
> > > > > > > In general I think it's not part of V4L2_CID_FLASH_DURATION to take any
> > > > > > > assumptions on that, as that's sensor/flash specific IMHO.
> > > > > > >
> > > > > > > In case of the ov9282 sensor driver (which is also part of this series)
> > > > > > > the strobe is started synchronously with the exposure on each frame
> > > > > > > start.
> > > > > > > Being even more specific on the ov9292, the sensor also offers the
> > > > > > > possibility to shift that strobe start in in either direction using a
> > > > > > > register. Implementing this "flash shift" (as it's called in the sensors
> > > > > > > datasheet) is currently under test on my side. I will likely send a
> > > > > > > series for that in the coming weeks.
> > > > > >
> > > > > > Ok, so you get a single frame exposed with a flash when you start
> > > > > > streaming, is that correct?
> > > > >
> > > > > Correct. The flash is switched on for the configured duration at every
> > > > > frame exposure (the sensor has a global shutter) as long as the camera is
> > > > > streaming.
> > > > >
> > > > > Maybe to following visualization of configured flash and exposure times help:
> > > > >
> > > > >              _________        _________        _________
> > > > > exposure: __|         |______|         |______|         |__
> > > > >
> > > > >              __               __               __
> > > > > flash:    __|  |_____________|  |_____________|  |_________
> > > > >             ^^^^
> > > > >       strobe_duration
> > > >
> > > > That diagram would work for global shutter but not for the much, much more
> > > > common rolling shutter operation. Does the driver use the sensor in rolling
> > > > shutter mode? This isn't very common with LED flashes.
> > >
> > > The ov9282 driver uses the sensor in global shutter mode.
> > >
> > > I totally agree with your statement. This pattern is only useful for
> > > global shutter operation.
> >
> > I think (nearly?) all supported sensors use a rolling shutter.
> 
> You've got at least two other global shutter sensors supported in
> mainline - Omnivision ov7251 and Sony imx296.
> Patches have been posted for OnSemi ar0144 (Laurent) and ar0234
> (Dongcheng), which are also both global shutter sensors.
> 
> So yes they are in the minority, but not that uncommon.

Thanks for the list. I briefly discussed this with Laurent and I agree with
him this information would probably be best kept with userspace (libcamera)
without trying to enumerate it from the kernel (albeit CCS might be an
exception, but that's an entirely different discussion then). Only changing
the global/rolling configuration would require a control.

-- 
Regards,

Sakari Ailus
Re: [PATCH v2 1/8] media: v4l: ctrls: add a control for flash/strobe duration
Posted by Richard Leitner 8 months, 3 weeks ago
Hi Dave, Hi Sakari

On Wed, Mar 19, 2025 at 10:06:28AM +0000, Sakari Ailus wrote:
> Hi Dave,
> 
> On Tue, Mar 18, 2025 at 04:39:05PM +0000, Dave Stevenson wrote:
> > Hi Sakari
> > 
> > On Tue, 18 Mar 2025 at 15:11, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> > >
> > > Hi Richard,
> > >
> > > On Tue, Mar 18, 2025 at 03:46:18PM +0100, Richard Leitner wrote:

...

> > > > The ov9282 driver uses the sensor in global shutter mode.
> > > >
> > > > I totally agree with your statement. This pattern is only useful for
> > > > global shutter operation.
> > >
> > > I think (nearly?) all supported sensors use a rolling shutter.
> > 
> > You've got at least two other global shutter sensors supported in
> > mainline - Omnivision ov7251 and Sony imx296.
> > Patches have been posted for OnSemi ar0144 (Laurent) and ar0234
> > (Dongcheng), which are also both global shutter sensors.
> > 
> > So yes they are in the minority, but not that uncommon.
> 
> Thanks for the list. I briefly discussed this with Laurent and I agree with
> him this information would probably be best kept with userspace (libcamera)
> without trying to enumerate it from the kernel (albeit CCS might be an
> exception, but that's an entirely different discussion then). Only changing
> the global/rolling configuration would require a control.

Thanks for the feeback and clarification!

So am I understanding this correctly that the flash/strobe duration
approach in this series is basically fine?

If so I will send a v3 later today.

regards;rl

> 
> -- 
> Regards,
> 
> Sakari Ailus
Re: [PATCH v2 1/8] media: v4l: ctrls: add a control for flash/strobe duration
Posted by Sakari Ailus 8 months, 2 weeks ago
Hi Richard, Dave,

On Tue, Mar 25, 2025 at 09:20:29AM +0100, Richard Leitner wrote:
> Hi Dave, Hi Sakari
> 
> On Wed, Mar 19, 2025 at 10:06:28AM +0000, Sakari Ailus wrote:
> > Hi Dave,
> > 
> > On Tue, Mar 18, 2025 at 04:39:05PM +0000, Dave Stevenson wrote:
> > > Hi Sakari
> > > 
> > > On Tue, 18 Mar 2025 at 15:11, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> > > >
> > > > Hi Richard,
> > > >
> > > > On Tue, Mar 18, 2025 at 03:46:18PM +0100, Richard Leitner wrote:
> 
> ...
> 
> > > > > The ov9282 driver uses the sensor in global shutter mode.
> > > > >
> > > > > I totally agree with your statement. This pattern is only useful for
> > > > > global shutter operation.
> > > >
> > > > I think (nearly?) all supported sensors use a rolling shutter.
> > > 
> > > You've got at least two other global shutter sensors supported in
> > > mainline - Omnivision ov7251 and Sony imx296.
> > > Patches have been posted for OnSemi ar0144 (Laurent) and ar0234
> > > (Dongcheng), which are also both global shutter sensors.
> > > 
> > > So yes they are in the minority, but not that uncommon.
> > 
> > Thanks for the list. I briefly discussed this with Laurent and I agree with
> > him this information would probably be best kept with userspace (libcamera)
> > without trying to enumerate it from the kernel (albeit CCS might be an
> > exception, but that's an entirely different discussion then). Only changing
> > the global/rolling configuration would require a control.
> 
> Thanks for the feeback and clarification!
> 
> So am I understanding this correctly that the flash/strobe duration
> approach in this series is basically fine?

I'd think so, yes.

> 
> If so I will send a v3 later today.

-- 
Regards,

Sakari Ailus