linux-kernel - Re: [PATCH 0/3] Add flash/strobe support for ov9282

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <2jaan6jm5abml3pve5hdesc5pj6kzbw4qaa5xofpwphxvp37rx@hyqq5ccn3n2h>
Date: Wed, 5 Mar 2025 07:49:50 +0100
From: Richard Leitner <richard.leitner@...ux.dev>
To: Dave Stevenson <dave.stevenson@...pberrypi.com>
Cc: Sakari Ailus <sakari.ailus@...ux.intel.com>, 
	Mauro Carvalho Chehab <mchehab@...nel.org>, Laurent Pinchart <laurent.pinchart@...asonboard.com>, 
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/3] Add flash/strobe support for ov9282

On Tue, Mar 04, 2025 at 05:46:34PM +0000, Dave Stevenson wrote:
> Hi Richard
> 
> Thanks for the series.

Hi Dave,

thanks for your quick and detailled review!

> 
> On Mon, 3 Mar 2025 at 22:59, Richard Leitner <richard.leitner@...ux.dev> wrote:
> >
> > This series adds basic flash/strobe support for ov9282 sensors using
> > their "hardware strobe output".
> >
> > Apart from en-/disabling the flash/strobe output, setting a timeout
> > (duration of activated strobe per frame) is implemented. The calculation
> > of this timeout is only interpolated from various measurements, as no
> > documentation was found.
> 
> The bigger picture question is whether using these flash controls is
> appropriate for controlling the strobe output on a sensor. That's a
> question for others (mainly Sakari and Laurent).

Thanks. So I'm looking forward to their response :-)

> V4L2_CID_FLASH_TIMEOUT feels wrong for setting the duration of the strobe pulse.
> Whilst the description in the docs [1] is a little brief, you then
> have the description of V4L2_FLASH_FAULT_TIMEOUT for
> V4L2_CID_FLASH_FAULT
> "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."
> which implies it is the hardware watchdog timeout to ensure the flash
> LEDs don't burn out, not configuring the duration of the flash pulse.
> Then again adp1653 adopts it as the flash duration.

I also thought of (and in fact did) implementing this using sensor
specific used CIDs, but then decided to go for FLASH_TIMEOUT.

If you think it's a better way to introduce either a completely new
"common control" or use another one I'm perfectly fine with that and
will try that for a v2.

> 
> Is there an expectation that V4L2_CID_FLASH_STROBE_SOURCE should also
> be implemented, even if it is fixed to
> V4L2_FLASH_STROBE_SOURCE_EXTERNAL?

I've already done this in my local tree, but was not sure if it "fits"
in this series...

So I guess I should include it in v2?

> 
> The one saving grace with this sensor is that it has a global shutter,
> so the strobe does correspond to the exposure period. With rolling
> shutter sensors, the flash duration is typically two frames to cover
> the exposure duration of all lines as the shutter rolls down.

Totally agree. Without global shutter configuring the flash duration
would not make that much sense.

Just to have mentioned it: I tested this quite heavily using an ov9281,
ran analysis on the resulting images and did lots of scope measurements
to make sure it really works as described.

regards;rl

> 
>   Dave
> 
> [1] https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-flash.html
> 
> > Further flash/strobe-related controls as well as a migration to v4l2-cci
> > helpers will likely be implemented in future series.
> >
> > All register addresses/values are based on the OV9281 datasheet v1.53
> > (january 2019). This series was tested using an ov9281 VisionComponents
> > camera module.
> >
> > Signed-off-by: Richard Leitner <richard.leitner@...ux.dev>
> > ---
> > Richard Leitner (3):
> >       media: i2c: ov9282: add output enable register definitions
> >       media: i2c: ov9282: add led_mode v4l2 control
> >       media: i2c: ov9282: add strobe_timeout v4l2 control
> >
> >  drivers/media/i2c/ov9282.c | 89 ++++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 86 insertions(+), 3 deletions(-)
> > ---
> > base-commit: f41427b3bdee7d9845b13a80c0d03882212f4b20
> > change-id: 20250303-ov9282-flash-strobe-ac6bd00c9de6
> > prerequisite-change-id: 20250225-b4-ov9282-gain-ef1cdaba5bfd:v1
> > prerequisite-patch-id: 86f2582378ff7095ab65ce4bb25a143eb639e840
> > prerequisite-patch-id: b06eb6ec697aaf0b3155b4b2370f171d0d304ae2
> > prerequisite-patch-id: b123047d71bfb9b93f743bbdd6893d5a98495801
> >
> > Best regards,
> > --
> > Richard Leitner <richard.leitner@...ux.dev>
> >
> >