Hi Dave On Feb 10, 2025 at 19:05:52 +0000, Dave Stevenson wrote: > Hi Jai > > Thanks for the patch > > On Tue, 4 Feb 2025 at 07:05, Jai Luthra <jai.luthra@xxxxxxxxxxxxxxxx> wrote: > > > > The sensor's internal ADC supports a minimum line length of 3448 pixels, > > which may be too small to use with analog binning, where ADC operates on > > two lines together. Switch to a higher minimum line length of 3560 > > pixels to fix the blocky artefacts seen with analog binning [1]. > > > > To keep the same default framerate as before for all the modes, lower > > the default fll value to compensate for the increase in llp. > > Extending the line length as you've done reduces the maximum frame > rate of all modes by ~3%. > Why do we need to reduce the max frame rate for non-binned modes which > have no artefacts? > > Default frame rate is a relatively useless setting - any sensible > application should be setting the blanking controls to configure the > frame rate that it wants. > > The datasheet does list line_length_pix default value as 3448 (0xd78), > and 3448 is mentioned numerous times throughout the datasheet for > calculations. Deviating from it for all modes is a brave move IMHO. > I changed the minimum because 3% didn't seem like a big difference and it kept the driver simple. But if there are usecases that rely on the maximum framerate, or the default 3448 value, I think it would make sense to just change the minimum for binned modes. I can do that in the next revision. > Dave > > > [1]: https://github.com/raspberrypi/rpicam-apps/issues/281#issuecomment-1082894118 > > > > Signed-off-by: Jai Luthra <jai.luthra@xxxxxxxxxxxxxxxx> > > --- > > drivers/media/i2c/imx219.c | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c > > index 8239e7ea8ec03849b339c1f314485266d4c4d8bd..e4aa6e66b673bb7a8942bf8daf27267c2884ec95 100644 > > --- a/drivers/media/i2c/imx219.c > > +++ b/drivers/media/i2c/imx219.c > > @@ -74,7 +74,7 @@ > > #define IMX219_FLL_MAX 0xffff > > #define IMX219_VBLANK_MIN 32 > > #define IMX219_REG_LINE_LENGTH_A CCI_REG16(0x0162) > > -#define IMX219_LLP_MIN 0x0d78 > > +#define IMX219_LLP_MIN 0x0de8 > > #define IMX219_LLP_MAX 0x7ff0 > > > > #define IMX219_REG_X_ADD_STA_A CCI_REG16(0x0164) > > @@ -286,25 +286,25 @@ static const struct imx219_mode supported_modes[] = { > > /* 8MPix 15fps mode */ > > .width = 3280, > > .height = 2464, > > - .fll_def = 3526, > > + .fll_def = 3415, > > }, > > { > > /* 1080P 30fps cropped */ > > .width = 1920, > > .height = 1080, > > - .fll_def = 1763, > > + .fll_def = 1707, > > }, > > { > > /* 2x2 binned 30fps mode */ > > .width = 1640, > > .height = 1232, > > - .fll_def = 1763, > > + .fll_def = 1707, > > }, > > { > > /* 640x480 30fps mode */ > > .width = 640, > > .height = 480, > > - .fll_def = 1763, > > + .fll_def = 1707, > > }, > > }; > > > > > > -- > > 2.48.1 > >
