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

[Jacopo Mondi <jacopo.mondi@ideasonboard.com>]

Hi Sakari

On Fri, Apr 10, 2026 at 12:04:10PM +0300, Sakari Ailus wrote:
> Hi Jacopo,
>
> On Fri, Apr 10, 2026 at 10:56:46AM +0200, Jacopo Mondi wrote:
> > Hi Sakari
> >
> > On Fri, Apr 10, 2026 at 11:46:49AM +0300, Sakari Ailus wrote:
> > > 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.
> >
> > Are you suggesting doubling the PIXEL_RATE is not accurate as the PLL
> > config doesn't change ? I might be very wrong, but in my understanding
> > this is partially why the "special" binning mode is special. We don't
> > have documentation for it and I'm maybe speculating here.
>
> I guess this comes down to the definition of PIXEL_RATE. I checked the
> control's documentation and I think it's indeed possible to understand it
> the way you suggest above.
>
> The control's value indeed was meant to reflect the actual frequency the
> pixels are read in the sensor's pixel array. The fact that the sensor uses
> analogue binning to sample two (or in some cases more) pixels into one
> output pixel in a single cycle doesn't change the frequency.
>

Not sure, I feel like we're talking details :) To be honest I
value consistency towards userspace more than exposing the exact
details of the sensor's configuration. It might be argued that the
sole purpose of PIXEL_RATE is to allow userspace to calculate the
frame duration instead of reporting the exact PLL configuration.

I'll defer the call to you and Dave, but I suggst to test this
carefully because getting it right hasn't been easy as Jai can confirm

> >
> > Is doubling the VBLANK limit to a number larger of the register
> > maximum value more accurate ?
> >
> > >
> > > > 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.
> > > >
> >
> > still not sure 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
> > >
>
> --
> Regards,
>
> Sakari Ailus
>