Hi Jai Thanks for the patch. On Wed, 28 Dec 2022 at 11:15, Jai Luthra <j-luthra@xxxxxx> wrote: > > 2x2 binning works fine for RAW10 capture, but for RAW8 1232p mode it > leads to corrupted frames [1][2]. > > Using the special 2x2 analog binning mode fixes the issue, but causes > artefacts for RAW10 1232p capture. So here we choose the binning mode > depending upon the frame format selected. > > As both binning modes work fine for 480p RAW8 and RAW10 capture, it can > share the same code path as 1232p for selecting binning mode. > > [1] https://forums.raspberrypi.com/viewtopic.php?t=332103 > [2] https://github.com/raspberrypi/libcamera-apps/issues/281 > > Fixes: 22da1d56e982 ("media: i2c: imx219: Add support for RAW8 bit bayer format") > Signed-off-by: Jai Luthra <j-luthra@xxxxxx> I've checked with David Plowman who did the investigation on [2]. He doesn't recall the issue, but your patch does appear to fix the described issue. Reviewed-by: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx> There are minor conflicts against https://patchwork.linuxtv.org/project/linux-media/list/?series=9503 that I've just added a R-B to. I'll leave it to Sakari as to whether he wants you to resubmit or not. Dave > --- > > v2: > - Removed extra newline found by checkpatch.pl --strict > - Add Fixes tag with the commit introducing RAW8 support > - Reword the commit description > > drivers/media/i2c/imx219.c | 57 ++++++++++++++++++++++++++++++++------ > 1 file changed, 49 insertions(+), 8 deletions(-) > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c > index 77bd79a5954e..4d358da25af2 100644 > --- a/drivers/media/i2c/imx219.c > +++ b/drivers/media/i2c/imx219.c > @@ -89,6 +89,12 @@ > > #define IMX219_REG_ORIENTATION 0x0172 > > +/* Binning Mode */ > +#define IMX219_REG_BINNING_MODE 0x0174 > +#define IMX219_BINNING_NONE 0x0000 > +#define IMX219_BINNING_2X2 0x0101 > +#define IMX219_BINNING_2X2_ANALOG 0x0303 > + > /* Test Pattern Control */ > #define IMX219_REG_TEST_PATTERN 0x0600 > #define IMX219_TEST_PATTERN_DISABLE 0 > @@ -143,6 +149,9 @@ struct imx219_mode { > > /* Default register values */ > struct imx219_reg_list reg_list; > + > + /* 2x2 binning is used */ > + bool binning; > }; > > /* > @@ -176,8 +185,6 @@ static const struct imx219_reg mode_3280x2464_regs[] = { > {0x016f, 0xa0}, > {0x0170, 0x01}, > {0x0171, 0x01}, > - {0x0174, 0x00}, > - {0x0175, 0x00}, > {0x0301, 0x05}, > {0x0303, 0x01}, > {0x0304, 0x03}, > @@ -235,8 +242,6 @@ static const struct imx219_reg mode_1920_1080_regs[] = { > {0x016f, 0x38}, > {0x0170, 0x01}, > {0x0171, 0x01}, > - {0x0174, 0x00}, > - {0x0175, 0x00}, > {0x0301, 0x05}, > {0x0303, 0x01}, > {0x0304, 0x03}, > @@ -290,8 +295,6 @@ static const struct imx219_reg mode_1640_1232_regs[] = { > {0x016f, 0xd0}, > {0x0170, 0x01}, > {0x0171, 0x01}, > - {0x0174, 0x01}, > - {0x0175, 0x01}, > {0x0301, 0x05}, > {0x0303, 0x01}, > {0x0304, 0x03}, > @@ -349,8 +352,6 @@ static const struct imx219_reg mode_640_480_regs[] = { > {0x016f, 0xe0}, > {0x0170, 0x01}, > {0x0171, 0x01}, > - {0x0174, 0x03}, > - {0x0175, 0x03}, > {0x0301, 0x05}, > {0x0303, 0x01}, > {0x0304, 0x03}, > @@ -485,6 +486,7 @@ static const struct imx219_mode supported_modes[] = { > .num_of_regs = ARRAY_SIZE(mode_3280x2464_regs), > .regs = mode_3280x2464_regs, > }, > + .binning = false, > }, > { > /* 1080P 30fps cropped */ > @@ -501,6 +503,7 @@ static const struct imx219_mode supported_modes[] = { > .num_of_regs = ARRAY_SIZE(mode_1920_1080_regs), > .regs = mode_1920_1080_regs, > }, > + .binning = false, > }, > { > /* 2x2 binned 30fps mode */ > @@ -517,6 +520,7 @@ static const struct imx219_mode supported_modes[] = { > .num_of_regs = ARRAY_SIZE(mode_1640_1232_regs), > .regs = mode_1640_1232_regs, > }, > + .binning = true, > }, > { > /* 640x480 30fps mode */ > @@ -533,6 +537,7 @@ static const struct imx219_mode supported_modes[] = { > .num_of_regs = ARRAY_SIZE(mode_640_480_regs), > .regs = mode_640_480_regs, > }, > + .binning = true, > }, > }; > > @@ -979,6 +984,35 @@ static int imx219_set_framefmt(struct imx219 *imx219) > return -EINVAL; > } > > +static int imx219_set_binning(struct imx219 *imx219) > +{ > + if (!imx219->mode->binning) { > + return imx219_write_reg(imx219, IMX219_REG_BINNING_MODE, > + IMX219_REG_VALUE_16BIT, > + IMX219_BINNING_NONE); > + } > + > + switch (imx219->fmt.code) { > + case MEDIA_BUS_FMT_SRGGB8_1X8: > + case MEDIA_BUS_FMT_SGRBG8_1X8: > + case MEDIA_BUS_FMT_SGBRG8_1X8: > + case MEDIA_BUS_FMT_SBGGR8_1X8: > + return imx219_write_reg(imx219, IMX219_REG_BINNING_MODE, > + IMX219_REG_VALUE_16BIT, > + IMX219_BINNING_2X2_ANALOG); > + > + case MEDIA_BUS_FMT_SRGGB10_1X10: > + case MEDIA_BUS_FMT_SGRBG10_1X10: > + case MEDIA_BUS_FMT_SGBRG10_1X10: > + case MEDIA_BUS_FMT_SBGGR10_1X10: > + return imx219_write_reg(imx219, IMX219_REG_BINNING_MODE, > + IMX219_REG_VALUE_16BIT, > + IMX219_BINNING_2X2); > + } > + > + return -EINVAL; > +} > + > static const struct v4l2_rect * > __imx219_get_pad_crop(struct imx219 *imx219, > struct v4l2_subdev_state *sd_state, > @@ -1056,6 +1090,13 @@ static int imx219_start_streaming(struct imx219 *imx219) > goto err_rpm_put; > } > > + ret = imx219_set_binning(imx219); > + if (ret) { > + dev_err(&client->dev, "%s failed to set binning: %d\n", > + __func__, ret); > + goto err_rpm_put; > + } > + > /* Apply customized values from user */ > ret = __v4l2_ctrl_handler_setup(imx219->sd.ctrl_handler); > if (ret) > -- > 2.17.1 >
