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

[Richard Leitner <richard.leitner@linux.dev>]

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@linux.dev> 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@linux.dev> 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@linux.dev>
> > > > ---
> > > >  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.

So IMHO the real maximum flash duration range the sensor can support is
the maximum strobe_frame_span register value.

I'm not saying this makes any sense from a use-case perspective, but if
we require the driver to support the hardware's maximum values wouldn't
that be the correct approach?

Or am I misunderstanding something?

regards;rl

> 
> > Sorry If misunderstood your feedback somehow.
> >
> > >
> > > There is reference in the docs to a different behaviour if "the
> > > vertical blanking period is long", but with a bit to set to give
> > > stable behaviour (0x3017 bit 1).
> >
> > I've read that paragraph in the datasheet, yes. But as this was not
> > affecting my use-case I haven't included this in this series or my
> > downstream kernel. Do you think it's worth adding support for this? Do
> > you know of any reports complaining about this power saving feature?
> 
> It only matters if you allow flash duration to be significantly
> greater than the exposure time, which I was effectively proposing.
> 
>   Dave
> 

[... snip ...]