linux-kernel - Re: [PATCH v6 4/5] media: i2c: imx219: Increase minimum LLP to fix blocky artefacts

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <vtxgyzvlp4tpxzeiblfi5rm25brvbgbfnfzequ3ln4ete7rigv@jit72nqfv3ya>
Date: Tue, 11 Feb 2025 12:29:44 +0530
From: Jai Luthra <jai.luthra@...asonboard.com>
To: Dave Stevenson <dave.stevenson@...pberrypi.com>
Cc: Sakari Ailus <sakari.ailus@...ux.intel.com>, 
	Mauro Carvalho Chehab <mchehab@...nel.org>, linux-media@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Jacopo Mondi <jacopo.mondi@...asonboard.com>, Laurent Pinchart <laurent.pinchart@...asonboard.com>
Subject: Re: [PATCH v6 4/5] media: i2c: imx219: Increase minimum LLP to fix
 blocky artefacts

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@...asonboard.com> 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@...asonboard.com>
> > ---
> >  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
> >