Hi Jai On Mon, Feb 17, 2025 at 06:57:35PM +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> > --- > drivers/media/i2c/imx219.c | 80 +++++++++++++++++++++++++++++++--------------- > 1 file changed, 54 insertions(+), 26 deletions(-) > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c > index 418f88386659d494ff674d64ed69b8441d1ee2cd..f97abcc5703ea32f1d6f19a4c0a671a7e978a974 100644 > --- a/drivers/media/i2c/imx219.c > +++ b/drivers/media/i2c/imx219.c > @@ -296,13 +296,13 @@ static const struct imx219_mode supported_modes[] = { > .fll_def = 1763, > }, > { > - /* 2x2 binned 30fps mode */ > + /* 2x2 binned 60fps mode */ > .width = 1640, > .height = 1232, > .fll_def = 1707, > }, > { > - /* 640x480 30fps mode */ > + /* 640x480 60fps mode */ > .width = 640, > .height = 480, > .fll_def = 1707, > @@ -357,6 +357,45 @@ static u32 imx219_get_format_code(struct imx219 *imx219, u32 code) > return imx219_mbus_formats[i]; > } > > +static void imx219_get_binning(struct imx219 *imx219, u8 *bin_h, u8 *bin_v) > +{ > + struct v4l2_subdev_state *state = > + v4l2_subdev_get_locked_active_state(&imx219->sd); > + const struct v4l2_mbus_framefmt *format = > + v4l2_subdev_state_get_format(state, 0); > + const struct v4l2_rect *crop = v4l2_subdev_state_get_crop(state, 0); > + u32 hbin = crop->width / format->width; > + u32 vbin = crop->height / format->height; > + > + *bin_h = IMX219_BINNING_NONE; > + *bin_v = IMX219_BINNING_NONE; > + > + /* > + * Use analog binning only if both dimensions are binned, as it crops > + * the other dimension. > + */ > + if (hbin == 2 && vbin == 2) { > + *bin_h = IMX219_BINNING_X2_ANALOG; > + *bin_v = IMX219_BINNING_X2_ANALOG; > + > + return; > + } > + > + if (hbin == 2) > + *bin_h = IMX219_BINNING_X2; > + if (vbin == 2) > + *bin_v = IMX219_BINNING_X2; > +} > + > +static inline u32 imx219_get_rate_factor(struct imx219 *imx219) > +{ > + u8 bin_h, bin_v; > + > + imx219_get_binning(imx219, &bin_h, &bin_v); > + > + return (bin_h & bin_v) == IMX219_BINNING_X2_ANALOG ? 2 : 1; > +} > + > /* ----------------------------------------------------------------------------- > * Controls > */ > @@ -368,10 +407,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; > @@ -400,7 +441,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, > @@ -417,7 +458,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); > break; > case V4L2_CID_HBLANK: > cci_write(imx219->regmap, IMX219_REG_LINE_LENGTH_A, > @@ -589,7 +630,7 @@ static int imx219_set_framefmt(struct imx219 *imx219, > const struct v4l2_mbus_framefmt *format; > const struct v4l2_rect *crop; > unsigned int bpp; > - u64 bin_h, bin_v; > + u8 bin_h, bin_v; > int ret = 0; > > format = v4l2_subdev_state_get_format(state, 0); > @@ -602,7 +643,6 @@ static int imx219_set_framefmt(struct imx219 *imx219, > case MEDIA_BUS_FMT_SBGGR8_1X8: > bpp = 8; > break; > - > case MEDIA_BUS_FMT_SRGGB10_1X10: > case MEDIA_BUS_FMT_SGRBG10_1X10: > case MEDIA_BUS_FMT_SGBRG10_1X10: > @@ -621,26 +661,7 @@ static int imx219_set_framefmt(struct imx219 *imx219, > cci_write(imx219->regmap, IMX219_REG_Y_ADD_END_A, > crop->top - IMX219_PIXEL_ARRAY_TOP + crop->height - 1, &ret); > > - switch (crop->width / format->width) { > - case 1: > - default: > - bin_h = IMX219_BINNING_NONE; > - break; > - case 2: > - bin_h = bpp == 8 ? IMX219_BINNING_X2_ANALOG : IMX219_BINNING_X2; > - break; > - } > - > - switch (crop->height / format->height) { > - case 1: > - default: > - bin_v = IMX219_BINNING_NONE; > - break; > - case 2: > - bin_v = bpp == 8 ? IMX219_BINNING_X2_ANALOG : IMX219_BINNING_X2; > - break; > - } > - > + imx219_get_binning(imx219, &bin_h, &bin_v); > cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_H, bin_h, &ret); > cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_V, bin_v, &ret); > > @@ -847,6 +868,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd, > int exposure_max; > int exposure_def; > int hblank, llp_min; > + int pixel_rate; > > /* Update limits and set FPS to default */ > __v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN, > @@ -882,6 +904,12 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd, > */ > hblank = prev_line_len - mode->width; > __v4l2_ctrl_s_ctrl(imx219->hblank, hblank); > + > + /* Scale the pixel rate based on the mode specific factor */ > + pixel_rate = imx219_get_pixel_rate(imx219) * > + imx219_get_rate_factor(imx219); Is this correct ? imx219_set_pad_format() operates a subdev state, while imx219_get_rate_factor() always operate on the active state. It could be argued that controls are always 'active' so using the 'active' state for both imx219_get_pixel_rate() and imx219_get_rate_factor() is ok. However I would rather pass to imx219_get_rate_factor() the current subdev state and pass it to imx219_get_binning() as well. With this Reviewed-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx> Thanks j > + __v4l2_ctrl_modify_range(imx219->pixel_rate, pixel_rate, > + pixel_rate, 1, pixel_rate); > } > > return 0; > > -- > 2.48.1 > >
