Re: [PATCH v4 05/29] media: imx219: Fix vertical blanking and exposure for analogue binning

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

Hi Jai,

On Wed, Apr 15, 2026 at 10:45:31PM +0530, Jai Luthra wrote:
> Quoting Sakari Ailus (2026-04-08 21:09:14)
> > When vertical analogue binning is in use, the minimum frame length in
> > lines decreases to around half of the normal. In relation to the sensor's
> > output size this means vertical blanking can be negative but that's not an
> > issue as control values are signed. Remove the workaround for this
> > non-issue that doubled the pixel rate, frame length in lines and exposure
> > time.
> > 
> > The resulting change also fixes the minimum, the maximum and the step
> > values for the control.
> > 
> > Fixes: f513997119f4 ("media: i2c: imx219: Scale the pixel rate for analog binning")
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/media/i2c/imx219.c | 29 +++++++++--------------------
> >  1 file changed, 9 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > index 6819a2fa3262..a72630ad1561 100644
> > --- a/drivers/media/i2c/imx219.c
> > +++ b/drivers/media/i2c/imx219.c
> > @@ -420,15 +420,6 @@ static void imx219_get_binning(struct v4l2_subdev_state *state, u8 *bin_h,
> >  
> >  }
> >  
> > -static inline u32 imx219_get_rate_factor(struct v4l2_subdev_state *state)
> > -{
> > -       u8 bin_h, bin_v;
> > -
> > -       imx219_get_binning(state, &bin_h, &bin_v);
> > -
> > -       return (bin_h & bin_v) == IMX219_BINNING_X2_ANALOG ? 2 : 1;
> > -}
> > -
> >  /* -----------------------------------------------------------------------------
> >   * Controls
> >   */
> > @@ -440,12 +431,10 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
> >         struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> >         const struct v4l2_mbus_framefmt *format;
> >         struct v4l2_subdev_state *state;
> > -       u32 rate_factor;
> >         int ret = 0;
> >  
> >         state = v4l2_subdev_get_locked_active_state(&imx219->sd);
> >         format = v4l2_subdev_state_get_format(state, 0);
> > -       rate_factor = imx219_get_rate_factor(state);
> >  
> >         if (ctrl->id == V4L2_CID_VBLANK) {
> >                 int exposure_max, exposure_def;
> > @@ -478,7 +467,7 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
> >                 break;
> >         case V4L2_CID_EXPOSURE:
> >                 cci_write(imx219->regmap, IMX219_REG_EXPOSURE,
> > -                         ctrl->val / rate_factor, &ret);
> > +                         ctrl->val, &ret);
> >                 break;
> >         case V4L2_CID_DIGITAL_GAIN:
> >                 cci_write(imx219->regmap, IMX219_REG_DIGITAL_GAIN,
> > @@ -495,7 +484,7 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
> >                 break;
> >         case V4L2_CID_VBLANK:
> >                 cci_write(imx219->regmap, IMX219_REG_FRM_LENGTH_A,
> > -                         (format->height + ctrl->val) / rate_factor, &ret);
> > +                         format->height + ctrl->val, &ret);
> >                 break;
> >         case V4L2_CID_HBLANK:
> >                 cci_write(imx219->regmap, IMX219_REG_LINE_LENGTH_A,
> > @@ -878,7 +867,6 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> >         crop->top = (IMX219_NATIVE_HEIGHT - crop->height) / 2;
> >  
> >         if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > -               unsigned int rate_factor = imx219_get_rate_factor(state);
> >                 int exposure_max;
> >                 int exposure_def;
> >                 int llp_min;
> > @@ -886,15 +874,16 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> >  
> >                 /* Update limits and set FPS to default */
> >                 ret = __v4l2_ctrl_modify_range(imx219->vblank,
> > -                                              IMX219_VBLANK_MIN * rate_factor,
> > -                                              (IMX219_FLL_MAX - mode->height) *
> > -                                              rate_factor, rate_factor,
> > -                                              mode->fll_def - mode->height);
> > +                                              (int)(mode->height / binning),
> 
> This line seems to be the crux of the issue causing modes other than VGA to
> not work, and also restricting the FPS.
> 
> Vertical blanking control is defined relative to the output height. While
> the minimum value for FLL for analogue binning mode is:
> 
>     min_FLL = (mode->height / binning) + IMX219_VBLANK_MIN
> 
> So the minimum control value for vblank should be:
> 
>     vblank_min = min_FLL - mode->height

I had done most testing on top of the metadata series and it seems some
bugs were still hidden in the earlier patches. I took another look at the
patches and indeed there were issues in calculating the lower limit of
vertical blanking.

I've pushed the changes to metadata-pre (and metadata) branches to my
linuxtv.org tree while this series can be found in metadata-pre-v4 tag and
the recently posted metadata series in metadata-v12 tag.

The changes also resulted in a new patch, namely one that fixes the maximum
exposure limit before removing the rate_factor.

The table below contains the list of modes and their minimum vertical
blankings and the fps at that vertical blanking value. The patches between
references 1 and 2 make subtle changes to the frame rate due to changes
that ensure that the sensor is operated within its limits. The default mode
frame rate is 15, 30, 60 and 60 fps, respectively, beginning from reference
2.

\ ref		1		2		3		4
m		vb	fps	vb	fps	vb	fps	vb	fps
3280x2464	32	20.526	32	21.194	32	21.194	32	21.194
1920x1080	32	46.079	32	47.574	32	47.571	32	47.569
1640x1232	32	81.162	64	79.070	-584	79.064	-584	79.070
640x480		32	198.610	64	188.359	-208	188.359	-208	188.359

[1] current upstream state; default fps varies
[2] up to "media: imx219: Account rate_factor in setting upper exposure limit"
[3] up to "media: imx219: Fix vertical blanking and exposure for analogue binning"
[4] on top of the metadata branch

I can post the updates to the list after addressing the rest of the
comments.

> 
> Making that change I get the correct range for VBLANK control that leads to
> high FPS capture.
> 
> So we finally have something working. Now the debate is between these two
> models:
> 
> 1. PIXEL_RATE means the output rate of the PLL for the sensor's internal
> pixel clock. That would mean this special binning mode leads to negative
> blanking values, which breaks intuition for what blanking means, but lines
> up well with exposing sensor FLL/LLP registers directly for the new raw
> sensor model.

On the other hand, without removing the rate_factor we'd still need to
multiply the FLL (likely?) value, plus many others, by the same factor, as
the sensor only has a single PIXEL_RATE control value. Getting it right is
hard and I didn't even try address bugs in control step configuration, only
those that affected sensor timing.

> 
> 2. H/V_BLANK are always positive and defined w.r.t source pad width/height.
> Thus special binning mode would need to have "double" the PIXEL_RATE, even
> though the sensor PLL is configured the same way. This aligns with what
> applications expect today.

Even in this case the blanking values do not express blanking correctly as
they are bound to the output size, not the analogue crop rectangle. Thus
they are no more useful in determining actual blanking.

> 
> Even "normal" binning mode on IMX219 and many other sensors does binning in
> the Analogue domain for one dimension. As we discussed before in [1].
> 
> Which makes the former a cleaner model to adopt for future. Now what we
> should do for existing sensors is a question that needs more discussion.
> 
> I am fine with both approaches, but this does mean fixing libcamera to work
> with negative blanking values (which it doesn't currently at least in my
> short test today) and breaks intuition for what blanking means (which is
> annoying).

Ouch.

> 
> Jacopo & Laurent, what do you think?

-- 
Kind regards,

Sakari Ailus