Re: [PATCH v5 02/10] media: imx219: Scale the vblank limits according to rate_factor

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Tue, Jun 09, 2026 at 11:31:54AM +0530, Jai Luthra wrote:
> Quoting Jai Luthra (2026-06-09 11:28:42)
> > Quoting Laurent Pinchart (2026-06-09 02:58:56)
> > > Hi Dave,
> > > 
> > > Thanks a lot for taking the time to investigate and provide very
> > > valuable insight. I really appreciate that.
> > > 
> > > On Mon, Jun 08, 2026 at 04:29:46PM +0100, Dave Stevenson wrote:
> > > > On Sun, 7 Jun 2026 at 22:54, 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.
> > > > 
> > > > Whilst that would be a logical interpretation, it doesn't match with
> > > > how the hardware performs, nor the docs.
> > > > 
> > > > The datasheet for register FRM_LENGTH_A 0x160 says
> > > > frame_length_lines
> > > > BINNING_MODE = 0,1,2
> > > > Unit: 1Lines
> > > > BINNING_MODE = 3
> > > > Units: 2Lines
> > > > 
> > > > That's not units of 2 lines for active lines only, that is units of 2
> > > > lines for ALL lines.
> > > > 
> > > > I have tested it, and the sensor works fine with FRM_LENGTH_A being
> > > > 0x278 in the 1640x1232 mode, and 0x100 in 640x480 modes with all the
> > > > tests I've thrown at it. Drop them any lower and it does stall or give
> > > > corrupt horizontal lines.
> > > 
> > > If I understand this correctly, it indicates that
> > > 
> > > - The frame length is expressed as a number of lines at the output of
> > >   the sensor (after binning).
> > > 
> > > - The minimum margin between the output height (after binning) and the
> > >   frame length is 32 lines.
> > > 
> > > - In special binning mode, the FRM_LENGTH_A register needs to be
> > >   programmed with frame_length_lines / 2.
> > > 
> > > The last constraint is device-specific, and as far as I understand it
> > > can be handled directly in the driver without affecting the userspace
> > > API by simply dividing the frame length value by 2 before writing it to
> > > the register.
> > > 
> > > It's getting a bit late, I'll test the "regular" binning mode
> > > (BINNING_MODE = 1) tomorrow to see how it compares (unless someone beats
> > > me to it). Unless I get very unexpected results, it seems that the
> > > existing implementation is correct and the patch should be dropped.
> > > 
> > > One thing that may not be implemented correctly is different binning
> > > modes horizontally and vertically. imx219_get_rate_factor() will return
> > > 2 only when both the horizontal and vertical binning modes are "special
> > > analog binning". When used to scale the frame length and exposure time,
> > > I wonder if only vertical binning should be taken into account.
> > > 
> > 
> > That does make sense.
> > 
> > When I was working on the binning and related issues in this driver, I had
> > decided to always keep `bin_h == bin_v` everywhere after some brief tests
> > with different values for them:
> > 
> > 1. Setting bin_h = 0x1 (normal x2 binning) and bin_v = 0x0 (or vice versa)
> >    results in the output image being stretched/squeezed, which is expected
> >    but not a typical usecase
> > 
> > 2. Setting bin_h = 0x3 (special x2 binning) and bin_v = 0x0 (or vice versa)
> >    results in both dimensions getting binned as opposed to only one, which
> 
> Ah sorry it was: "none of the dimensions getting binned" but the output was
> cropped. So in the above case (bin_h = 0x3), it would just drop the right
> half of the image as opposed to squeezing it when bin_h = 0x1.

If the sensor ignores horizontal special binning when vertical binning
is disabled, then cropping is expected, as the driver sets the output
size to the binned size.

> >    is unexpected, and it might be breaking other things with the sensor
> >    timing too
> > 
> > My analysis on the other sub-thread also suggests special x2 binning means
> > analogue-domain averaging in both horzizontal and vertical dimensions..
> > which matches up with this behaviour, so maybe we can't really separate
> > vertical and horizontal out for the "special" case even if the registers
> > allow us to.

Binning independently in the horizontal and vertical direction is a
niche use case, so I'm fine not supporting it in the imx219 driver.

> > > > (Please note that the sensor extends the frame length automatically to
> > > > accommodate the exposure time requested, so do ensure the exposure
> > > > time doesn't interact with the frame length if you're testing).
> > > > 
> > > > This patch drops the maximum frame rate from 81.07 to 79.07fps in
> > > > 1640x1232 (2.5%), and 200.1 to 188.39fps in 640x480 (6%) for no good
> > > > reason that I can see.
> > > > Unless anyone can produce a genuine situation where they see the
> > > > sensor behave incorrectly with the old setup, I'll be very sad to see
> > > > this merged.
> > > > 
> > > > > 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>
> > > > > ---
> > > > >  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 223d3753cc93..d8fe7db18b6c 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,

Laurent Pinchart