Re: [PATCH v5 06/10] media: imx219: Fix vertical blanking and exposure for analogue binning

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Jai,

On Wed, Jun 10, 2026 at 03:59:36PM +0530, Jai Luthra wrote:
> > > > OPTION 1 (as proposed by Sakari):
> > > > 
> > > >     Fix PIXEL_RATE to 182400000 and allow **negative values** for HBLANK
> > > >     and VBLANK controls when using 2x2-special-binning mode.
> > > > 
> > > >     This will break any userspace tools, many libcamera pipelines included,
> > > >     that never expected those control values to be negative (even though
> > > >     the API has always permitted those)
> > 
> > That's a clear bug, but it only becomes apparent when the sign bit is set.
> > 
> 
> Agreed, I'll try to send patches for that in libcamera regardless of what
> we do here.

Thanks, this is much appreciated!

> 
> > > > 
> > > > OPTION 2 (something that struck me today morning discussing with Jacopo):
> > > >     
> > > >     Fix PIXEL_RATE to 182400000 but **adjust the HBLANK values** to go
> > > >     lower to compensate, which will diverge from the sensor registers which
> > > >     keep MIN_LINE_LENGTH fixed across both binning modes.
> > > > 
> > > >     This will make the driver quite more complicated, but userspace
> > > >     expectations of non-negative blankings will be met. And it's likely
> > > >     that the sensor is internally doing pre-ADC averaging horizontally as
> > > >     well, or so my best guess is.
> > > > 
> > > 
> > > Which makes me lean more on OPTION 2 now.
> > > 
> > > In our case above with 2x2-special-binning mode for 1640x1232, the register
> > > values are:
> > > 
> > >     LINE_LENGTH = 3560 pixels
> > >     FRAME_LENGTH = 632 lines
> > > 
> > > 
> > > So OPTION 1 would give us:
> > > 
> > >     HBLANK = 1920, VBLANK = -600
> > > 
> > > When actually the datasheet's "H Binning = Analog" would suggest to me that
> > > reality looks more like:
> > > 
> > >     LINE_LENGTH = 1780 pixels
> > >     FRAME_LENGTH = 1264 lines
> > > 
> > > So OPTION 2 would give us:
> > > 
> > >     HBLANK = 140, VBLANK = 32
> > 
> > This approach has the downsides that 1) it doesn't reflect what the sensor
> > apparently does and 2) you lose one bit of granularity on line length in
> > pixels.
> > 
> 
> I agree with you on 2)
> 
> On 1) though I don't think the sensor's register values are a good
> indicator of what the sensor apparently does.

Let's say the sensor still behaves as if it is using these values in the
control of its internal timing. And as that's the case, I think we can say
these are in fact used for internal sensor timing -- there's no suggestion
of anything else.

If 4x analogue binning was used for the "special binning" mode, then why
would the sensor vendor have written it's special, instead of just saying
it's "4x analogue binning"?

I tested the effect of setting the horizontal blanking and it seems the
step value appears to be 8, not 1 which is suggested by the driver and not
mentioned in the specification.

So even if you multiply FLL and divide LLP by 2 for the special binning
mode, no information is lost and the sensor configurability remains
unaffected. I still wouldn't do this as it's going to be a perpetual glitch
in a driver meant to be exemplary. An integer type change in libcamera
would be a passing issue, albeit it would inflict some intermediate pain.

In either case I'd fix the HBLANK step before changing the rest.

> 
> For example, FLL being programmed to ~ 1/4th of analogue crop height, while
> LLP being fixed to ~ analogue crop width would suggest the sensor is
> averaging 4 R/Gr/Gb/B pixels vertically in the analogue domain (possibly
> through common FD charge summing) before the ADC reads it out.
> 
> But the table in the datasheet clearly mentions horizontal binning is
> (also) done in the analogue domain. Which makes more sense as well, given
> the output has an average of a 2x2 block of pixels and not a 4x1 block of
> pixels.
> 
> > The advantage still is that it works around the sign bit issue. The new
> > controls still have their proper values but conversion between the two
> > becomes rather complicated. See
> > <URL:https://git.linuxtv.org/sailus/media_tree.git/commit/?h=metadata&id=366cb25c7d944ef2935668a07471e4576e7088ed>
> > for instance how it looks like without that.
> > 
> 
> I see.. well IMHO even the LLP and FLL controls should try to model what
> the sensor does (if it is known, at least) rather than being just a 1-to-1
> mapping of the sensor registers. Which as Dave and I have mentioned, is not
> the case in many sensors that scale the HMAX register units according to
> some lower frequency clock (so 1 clock cycle => multiple pixels)
> 
> In this particular case it would mean the new controls should scale like:
> 
> FRAME_LENGTH = FLL_REG / 2
> LINE_LENGTH = LLP_REG * 2
> 
> When analogue binning is used. Which should make your code slightly easier,
> even if not re-usable across all sensors.

-- 
Regards,

Sakari Ailus