Re: [PATCH v4 04/29] media: imx219: Scale the vblank limits according to rate_factor

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

On Fri, Apr 10, 2026 at 11:41:45AM +0300, Sakari Ailus wrote:
> Hi Jacopo,
>
> On Fri, Apr 10, 2026 at 10:28:27AM +0200, Jacopo Mondi wrote:
> > Hi Sakari
> >
> > On Wed, Apr 08, 2026 at 06:39:13PM +0300, Sakari Ailus wrote:
> > > The limits for vertical blanking (and frame length in pixels) is related
> > > to the properties of the hardware, it's not in half-line units the driver
> > > uses. Multiply the vertical blanking limits by the rate_factor to satisty
> > > hardware requirements.
> > >
> > > Fixes: f513997119f4 ("media: i2c: imx219: Scale the pixel rate for analog binning")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >
> > I'm not sure I understand this change.
> >
> > I think we have clarified the imx219 has a "special" binning mode
> > where the ADC consumes two lines at a time, allowing an higher
> > framerate.
> >
> > The driver accounts for that by doubling the PIXEL_RATE control value
> > and halving the VBLANK and EXPOSURE controls values when writing them
> > to registers.
> >
> > Userspace is not concerned with the special binning mode and is not
> > required to halve the values it writes to the VBLANK and EXPOSURE controls.
> >
> > Doesn't the same apply to the limits ? Also, I presume but special
> > binning mode is not well documented, the actual maximum register value
> > for the frame length is still 0xfffe.
> >
> > What have I missed ?
>
> This patch indeed changes the limits of the VBLANK control. The maximum
> frame length (in hardware) indeed is 0xfffe but the driver only allowed
> frames up to 0x7fff lines before this patch.
>

Isn't userspace still allowed to write values up to 0xfffe ?

What ends up in registers is halved because, again in my speculative
understanding of the special binning mode, when using this special
mode two lines at the time are sampled.

Anyway, provided you've tested all the use cases we tested when
implementing support for the special binning mode, and that Dave is
fine being him the maintainer of this driver, I'll be happy to shut up.

> >
> > > ---
> > >  drivers/media/i2c/imx219.c | 10 ++++++----
> > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > > index 62a23541b1dc..6819a2fa3262 100644
> > > --- a/drivers/media/i2c/imx219.c
> > > +++ b/drivers/media/i2c/imx219.c
> > > @@ -878,14 +878,17 @@ 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;
> > >  		int pixel_rate;
> > >
> > >  		/* Update limits and set FPS to default */
> > > -		ret = __v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
> > > -					       IMX219_FLL_MAX - mode->height, 1,
> > > +		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);
> > >  		if (ret)
> > >  			return ret;
> > > @@ -928,8 +931,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) *
> > > -			     imx219_get_rate_factor(state);
> > > +		pixel_rate = imx219_get_pixel_rate(imx219) * rate_factor;
> > >  		ret = __v4l2_ctrl_modify_range(imx219->pixel_rate, pixel_rate,
> > >  					       pixel_rate, 1, pixel_rate);
> > >  		if (ret)
>
> --
> Regards,
>
> Sakari Ailus