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

[Jai Luthra <jai.luthra@ideasonboard.com>]

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

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.

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 "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).

Jacopo & Laurent, what do you think?

Thanks,
    Jai

[1]: https://lore.kernel.org/all/20260225-media-fps-docs-v2-1-5cb222d41e4d@ideasonboard.com/

> +                                              IMX219_FLL_MAX - mode->height, 1,
> +                                              (int)(mode->fll_def / binning) -
> +                                              (int)mode->height);
>                 if (ret)
>                         return ret;
>  
>                 ret = __v4l2_ctrl_s_ctrl(imx219->vblank,
> -                                        mode->fll_def - mode->height);
> +                                        (int)(mode->fll_def / binning) -
> +                                        (int)mode->height);
>                 if (ret)
>                         return ret;
>  
> @@ -931,7 +920,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>                         return ret;
>  
>                 /* Scale the pixel rate based on the mode specific factor */
> -               pixel_rate = imx219_get_pixel_rate(imx219) * rate_factor;
> +               pixel_rate = imx219_get_pixel_rate(imx219);
>                 ret = __v4l2_ctrl_modify_range(imx219->pixel_rate, pixel_rate,
>                                                pixel_rate, 1, pixel_rate);
>                 if (ret)
> -- 
> 2.47.3
>