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

Sakari Ailus
2026-04-10T08:47:06Z

Hi Jacopo,

On Fri, Apr 10, 2026 at 10:42:22AM +0200, Jacopo Mondi wrote:
> Hi Sakari
>
> On Wed, Apr 08, 2026 at 06:39:14PM +0300, Sakari Ailus wrote:
> > 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.
>
> I don't think this was a workaround. Doubling the pixel rate and

You could also call it a bug. :-) If you look what the driver does with
PLL, you can see the PLL configuration is always the same independently of
any binning configuration.

> halving the values written to registers for EXPOSURE and VBLANK
> allowed userspace to maintain a consistent view while the driver
> accounts for the special binning mode where, at least in my latest
> understanding, the sensor averages two lines before passing them to
> the ADC.
>
> I am missing in which case, with the current driver implementation,
> the vertical blanking can be negative.
>
> >
> > 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>
>
> Jai, and others including Dave and me, have spent quite some time
> testing and implementing proper support for the special analogue
> binning mode for this sensor. Of course we might have missed something
> obvious, but I'm still missing what you're trying to fix here.
>
> > ---
> > 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),
> > + 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)

--
Regards,

Sakari Ailus