Re: [PATCH 09/10] media: i2c: ov9282: fix flash duration control range

[Richard Leitner]

On Mon, Oct 05, 2026 at 05:05:54PM +0100, Dave Stevenson wrote:
> Hi Richard
> 
> On Mon, 5 Oct 2026 at 15:46, Richard Leitner <richard.leitner@xxxxxxxxx> wrote:
> >
> > On Tue, Sep 29, 2026 at 03:06:51PM +0100, Dave Stevenson wrote:
> > > On Mon, 28 Sept 2026 at 22:08, Richard Leitner
> > > <richard.leitner@xxxxxxxxx> wrote:
> > > >
> > > > On Mon, Sep 28, 2026 at 03:40:21PM +0100, Dave Stevenson wrote:
> > > > > On Mon, 28 Sept 2026 at 15:23, Richard Leitner
> > > > > <richard.leitner@xxxxxxxxx> wrote:
> > > > > >
> > > > > > Hi Dave,
> > > > > >
> > > > > > thanks for your feedback! Greatly appreciated!
> > > > > >
> > > > > > On Mon, Sep 28, 2026 at 02:47:36PM +0100, Dave Stevenson wrote:
> > > > > > > Hi Richard
> > > > > > >
> > > > > > > On Mon, 14 Sept 2026 at 20:21, Richard Leitner
> > > > > > > <richard.leitner@xxxxxxxxx> wrote:
> > > > > > > >
> > > > > > > > When updating the flash_duration range ensure the ceiling is at least as
> > > > > > > > long as the exposure time is. This may cause the calculated
> > > > > > > > flash_duration register value to be rounded up.
> > > > > > > >
> > > > > > > > This is done by introducing a new
> > > > > > > > ov9282_update_ctrl_range_flash_duration() function and using them
> > > > > > > > wherever possible.
> > > > > > > >
> > > > > > > > Signed-off-by: Richard Leitner <richard.leitner@xxxxxxxxx>
> > > > > > > > ---
> > > > > > > >  drivers/media/i2c/ov9282.c | 47 +++++++++++++++++++++++++++-------------------
> > > > > > > >  1 file changed, 28 insertions(+), 19 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > > > > > > > index f728709fcf0a6..be38ecfad8c82 100644
> > > > > > > > --- a/drivers/media/i2c/ov9282.c
> > > > > > > > +++ b/drivers/media/i2c/ov9282.c
> > > > > > > > @@ -541,6 +541,29 @@ static u32 ov9282_flash_duration_to_us(struct ov9282 *ov9282, u32 value)
> > > > > > > >                                 NSEC_PER_USEC);
> > > > > > > >  }
> > > > > > > >
> > > > > > > > +/**
> > > > > > > > + * ov9282_update_ctrl_range_flash_duration() - Update flash_duration control range
> > > > > > > > + * @ov9282: pointer to ov9282 device
> > > > > > > > + *
> > > > > > > > + * This may round up the ceiling to the microseconds representation of the
> > > > > > > > + * next flash_duration register value to make sure one can illuminate the whole
> > > > > > > > + * exposure time long.
> > > > > > > > + *
> > > > > > > > + * Return: 0 if successful, error code otherwise.
> > > > > > > > + */
> > > > > > > > +static int ov9282_update_ctrl_range_flash_duration(struct ov9282 *ov9282)
> > > > > > > > +{
> > > > > > > > +       u32 exposure_us = ov9282_exposure_to_us(ov9282, ov9282->exp_ctrl->val);
> > > > > > > > +       u32 fd_max = ov9282_us_to_flash_duration(ov9282, exposure_us);
> > > > > > > > +       u32 fd_max_us = ov9282_flash_duration_to_us(ov9282, fd_max);
> > > > > > > > +
> > > > > > > > +       if (fd_max_us < exposure_us)
> > > > > > > > +               fd_max_us = ov9282_flash_duration_to_us(ov9282, fd_max + 1);
> > > > > > >
> > > > > > > Reading the driver for how it handles flash duration, it all gets a
> > > > > > > bit convoluted. I see part of it comes from the units being usecs
> > > > > > > whilst natively it is in lines, but we've got this slightly odd calc
> > > > > > > and adding 1, and rounding in try_ctrl to get closest to the absolute
> > > > > > > value.
> > > > > >
> > > > > > That's true. I'm also not happy with this back-and-forth conversion between
> > > > > > u/n-secs and lines. From a V4l2 API user perspective I'd love to have
> > > > > > all "timing related" controls in the same unit (likely usecs). I was
> > > > > > already playing around with that approach, but as this would break the
> > > > > > current exporsure property I have not sent those patches...
> > > > > >
> > > > > > If you have any "mainline-acceptable" idea on how to do that I would
> > > > > > definitely love to hear it ;-)
> > > > >
> > > > > Sadly I think that there is a need for some of the back and forth in
> > > > > conversions :-(
> > > > >
> > > > > > > Seeing as this is the max flash_duration, is it actually limited by
> > > > > > > the exposure, or by the frame duration? The difference between those
> > > > > > > is a minimum of 25 lines (OV9282_EXPOSURE_OFFSET), which I think gives
> > > > > > > you up to another 9usecs to play with. That covers any of this
> > > > > > > rounding stuff. You can afford to always round that one down and never
> > > > > > > clip the range below the exposure time.
> > > > > >
> > > > > > From a hardware perspective this isn't limited at all AFAICT. But from a
> > > > > > "use-case" perspective a flash_duration (with a flash offset=0, which is
> > > > > > the hard-coded default case in this driver currently) longer than the
> > > > > > exposure time makes no sense, as it does not have any effect on the image.
> > > > > >
> > > > > > With this patch I want to make sure to be able to illuminate the frame
> > > > > > during the complete exposure time, but keep the maximum as short as
> > > > > > possible. So I was comparing/aiming this at the exposure time, not the
> > > > > > frame duration.
> > > > > >
> > > > > > As the frame duration is OV9282_EXPOSURE_OFFSET longer than the exposure
> > > > > > time: Would a "turned-on flash" during those 25 lines have any impact on
> > > > > > the frames brightness in your opinion?
> > > > > > I haven't explicitely tested/measured this with hardware, but I guess
> > > > > > this does not have any impact?
> > > > >
> > > > > No, as this is a global shutter sensor I wouldn't expect the flash to
> > > > > impact the image if a connected flash was on for more lines than the
> > > > > exposure.
> > > > >
> > > > > > Or do you mean when calculating against the frame duration the +/-1
> > > > > > stuff can be dropped and the code would be easier to read?
> > > > >
> > > > > Yes, I'm suggesting the driver handles the range of values that the
> > > > > hardware can support correctly. Whether that helps for your image
> > > > > quality requirements is a different question.
> > > > > The driver can pass the buck with at least some of the rounding issues
> > > > > to userspace, which potentially has use-case specific knowledge. And
> > > > > it simplifies the driver in the process.
> > > >
> > > > Talking about "range of values that the hardware can support": AFAICT
> > > > the strobe duration is (from a sensor hardware point of view) not
> > > > limited by the frame duration. At least I found nothing on this in the
> > > > datasheet and a quick test on hardware also showed no correlation.
> > >
> > > Interesting. So if you set it to a 1 second strobe pulse when
> > > streaming at 60fps, what behaviour do you get?
> >
> > I haven't tested the 1 second strobe explicitely. But the strobe in my
> > case is always on for the configured time. No matter if it exceeds the
> > frame time/trigger. After the configured strobe_duration passed it goes
> > low until the next trigger happens.
> >
> > Maybe the following visualization helps:
> >
> >
> > strobe duration < frame time:
> >                   __         __         __         __
> >                  |  |       |  |       |  |       |  |
> > strobe signal   _|  |_______|  |_______|  |_______|  |_____
> > strobe trigger   ^          ^          ^          ^
> >
> >
> > frame time < strobe duration < 2x frame time:
> >                   _____________         _____________
> >                  |             |       |             |
> > strobe signal   _|             |_______|             |_____
> > strobe trigger   ^          ^          ^          ^
> >
> >
> > 2x frame time < strobe duration < 3x frame time:
> >                   ________________________         ________
> >                  |                        |       |
> > strobe signal   _|                        |_______|
> > strobe trigger   ^          ^          ^          ^
> >
> > Therefore I would strongly suggest to set the maximum strobe duration to
> > be in every case shorter than the "frame time".
> >
> > For the reasons discussed earlier it the maximum strobe duration should
> > be the same or longer than the actual exposure time.
> >
> > What's your take on this Dave? Or Sakari?
> 
> Seeing as the behaviour is logical, personally I'd go with allowing
> whatever strobe period (up to the register max), and leave it up to
> userspace to keep the period below the exposure time if that is what
> is desired for the use case.

Fair approach. But if we do so it will become a pain in the ass to
calculate between the expsoure time (which is based on the line time)
and the strobe duration (which is currently microseconds) IMHO.. As we
don't have a "reasonable upper limit" easily available...

The problem I'm talking about is that it would be quite "easy" then to
mess up the units and configure a strobe duration longer than the frame
time, which then show up as "strangely illuminated frames" in the stream.

Do you see any chance to get the exposure time and strobe duration on
the same unit? Or is this basically impossible as we would "break"
userpace with this? If so, would a new "strobe_duration in line time" be
a possible approach?

Apart from that I found another problem in the current time calculation
of the flash duration... When changing the "line width" (e.g. HBLANK)
the register value of the flash_duration stays unchanged. This
results in a different strobe duration on hardware, as it "scales" with
the line time... So a change to e.g. HBLANK would need to trigger a
re-calculation and therefore register write of the strobe duration...

As you mentioned earlier, this back-and-forth conversion of
width/lines/microseconds is somehow just bloating the driver without any
real benefit... Unless all "time/duration" properties would be on
microseconds... IMHO

regards;rl

> 
>   Dave
> 

[... snip ...]