Hi Jacopo, Naush, On Feb 07, 2025 at 17:49:19 +0100, Jacopo Mondi wrote: > Hi Jai > > On Tue, Feb 04, 2025 at 12:34:40PM +0530, Jai Luthra wrote: > > When the analog binning mode is used for high framerate operation, the > > pixel rate is effectively doubled. Account for this when setting up the > > pixel clock rate, and applying the vblank and exposure controls. > > > > The previous logic only used analog binning for RAW8, but normal binning > > limits the framerate on RAW10 480p [1]. So with this patch we switch to > > using special binning (with 2x pixel rate) wherever possible. > > > > [1]: https://github.com/raspberrypi/linux/issues/5493 > > > > Co-developed-by: Naushir Patuck <naush@xxxxxxxxxxxxxxx> > > Signed-off-by: Naushir Patuck <naush@xxxxxxxxxxxxxxx> > > Co-developed-by: Vinay Varma <varmavinaym@xxxxxxxxx> > > Signed-off-by: Vinay Varma <varmavinaym@xxxxxxxxx> > > Signed-off-by: Jai Luthra <jai.luthra@xxxxxxxxxxxxxxxx> > > --- [snip] > > @@ -367,10 +426,12 @@ 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(imx219); > > > > if (ctrl->id == V4L2_CID_VBLANK) { > > int exposure_max, exposure_def; > > @@ -399,7 +460,7 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl) > > break; > > case V4L2_CID_EXPOSURE: > > cci_write(imx219->regmap, IMX219_REG_EXPOSURE, > > - ctrl->val, &ret); > > + ctrl->val / rate_factor, &ret); > > break; > > case V4L2_CID_DIGITAL_GAIN: > > cci_write(imx219->regmap, IMX219_REG_DIGITAL_GAIN, > > @@ -416,7 +477,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, &ret); > > + (format->height + ctrl->val) / rate_factor, &ret); > > > Isn't this (and exposure) compensatd by the doubled pixel rate ? > The datasheet mentions that FRM_LENGTH_A register is in units of 2xLines when analog binning mode is selected. And the exposure is also usually in unit of lines, so I assume that is why the same division was made in the original commit by Naush [1] [1] https://github.com/raspberrypi/linux/commit/caebe4fe817b5079 > Applications use the pixel rate to compute the line duration and from > there transform the frame duration and the exposure in lines, don't > they ? While this change doesn't update the user-side of the control values, only the register values, I wonder if there is a clean way to handle this without updating some assumptions in the application. The IMX219 pixel clock behaves differently with analog binning compared to (most of our) intuitions, where rather than doubling the horizontal pixel clock, each line is still read-out in the same time but the number of lines read are halved.. at least that's the best explanation I have from these results: https://lore.kernel.org/linux-media/zla2sogd7ov3yz2k2je6zrgh3uzeermowlaixt3qkcioturppo@tswbw354tpdk/ And that is why the total pixel rate is doubled, but the actual line duration should be the same as a digitally binned or cropped format. > > Overall, very nice to be able to double the achievable frame rate > without any artifacts! Good job! > > Thanks > j > Thanks, Jai > > break; > > case V4L2_CID_HBLANK: > > cci_write(imx219->regmap, IMX219_REG_LINE_LENGTH_A, [snip]
