1. Patchew
  2. linux
  3. [PATCH v6 0/5] media: i2c: imx219: Fixes for blanking, pixel rate and binning
  4. View patch

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

Jai Luthra posted 5 patches 10 months, 1 week ago
There is a newer version of this series
[PATCH v6 4/5] media: i2c: imx219: Increase minimum LLP to fix blocky artefacts
Posted by Jai Luthra 10 months, 1 week ago 
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.

[1]: https://github.com/raspberrypi/rpicam-apps/issues/281#issuecomment-1082894118

Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.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
Re: [PATCH v6 4/5] media: i2c: imx219: Increase minimum LLP to fix blocky artefacts
Posted by Dave Stevenson 10 months ago 
Hi Jai

Thanks for the patch

On Tue, 4 Feb 2025 at 07:05, Jai Luthra <jai.luthra@ideasonboard.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.

  Dave

> [1]: https://github.com/raspberrypi/rpicam-apps/issues/281#issuecomment-1082894118
>
> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.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
>
Re: [PATCH v6 4/5] media: i2c: imx219: Increase minimum LLP to fix blocky artefacts
Posted by Jai Luthra 10 months ago 
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@ideasonboard.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@ideasonboard.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
> >
Re: [PATCH v6 4/5] media: i2c: imx219: Increase minimum LLP to fix blocky artefacts
Posted by Jacopo Mondi 10 months, 1 week ago 
Hi Jai

On Tue, Feb 04, 2025 at 12:34:39PM +0530, Jai Luthra 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].
>

Very nice finding!

If the new value differs from the default register value in the
datahseet, can we record the reason with a comment ?

> To keep the same default framerate as before for all the modes, lower
> the default fll value to compensate for the increase in llp.

Thanks for handling this

This seems fine to me
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

>
> [1]: https://github.com/raspberrypi/rpicam-apps/issues/281#issuecomment-1082894118
>
> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.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
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.