[PATCH 0/2] media: i2c: imx219: Fix regressions introduced by 1x2/2x1 binning

[PATCH 0/2] media: i2c: imx219: Fix regressions introduced by 1x2/2x1 binning

[Jai Luthra]

This series fixes a regression introduced in commit 0af46fbc333d
("media: i2c: imx219: Calculate crop rectangle dynamically") that
started using vertical binning for 1920x1080 without binning
horizontally, causing the captured images to be stretched in one
dimension.

In a subsequent patch, simplify the binning mode calculation logic as
well. This is done separately, without a fixes tag, as it is cleaning up
code that was introduced much later to the regression commit, and
doesn't strictly require backporting.

Once Sakari's metadata series with binning controls [1] is merged, this
and other existing sensor drivers may be extended to support free
configuration of resolution, and give the userspace freedom to bin
pixels in only one dimension if required, of course, as long as it
doesn't cause regressions with exsiting userspace applications.

[1]: https://lore.kernel.org/all/20250825095107.1332313-44-sakari.ailus@linux.intel.com/

Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
---
Dave Stevenson (1):
      media: i2c: imx219: Fix 1920x1080 mode to use 1:1 pixel aspect ratio

Jai Luthra (1):
      media: i2c: imx219: Simplify imx219_get_binning() function

 drivers/media/i2c/imx219.c | 25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)
---
base-commit: 3a8660878839faadb4f1a6dd72c3179c1df56787
change-id: 20251016-imx219-1080p-ad3fbdce70d1

Best regards,
-- 
Jai Luthra <jai.luthra@ideasonboard.com>

[PATCH 1/2] media: i2c: imx219: Fix 1920x1080 mode to use 1:1 pixel aspect ratio

[Jai Luthra]

From: Dave Stevenson <dave.stevenson@raspberrypi.com>

Commit 0af46fbc333d ("media: i2c: imx219: Calculate crop rectangle
dynamically") meant that the 1920x1080 mode switched from using no
binning to using vertical binning but no horizontal binning, which
resulted in stretched pixels.

Until proper controls are available to independently select horizontal
and vertical binning, restore the original 1:1 pixel aspect ratio by
forcing binning to be uniform in both directions.

Cc: stable@vger.kernel.org
Fixes: 0af46fbc333d ("media: i2c: imx219: Calculate crop rectangle dynamically")
Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
[Add comment & reword commit message]
Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
---
 drivers/media/i2c/imx219.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index c680aa6c3a55a9d865e79ad337b258cb681f98fe..300935b1ef2497050fe2808e4ceedda389a75b50 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -856,7 +856,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
 	const struct imx219_mode *mode;
 	struct v4l2_mbus_framefmt *format;
 	struct v4l2_rect *crop;
-	u8 bin_h, bin_v;
+	u8 bin_h, bin_v, binning;
 	u32 prev_line_len;
 
 	format = v4l2_subdev_state_get_format(state, 0);
@@ -877,9 +877,12 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
 	bin_h = min(IMX219_PIXEL_ARRAY_WIDTH / format->width, 2U);
 	bin_v = min(IMX219_PIXEL_ARRAY_HEIGHT / format->height, 2U);
 
+	/* Ensure bin_h and bin_v are same to avoid 1:2 or 2:1 stretching */
+	binning = min(bin_h, bin_v);
+
 	crop = v4l2_subdev_state_get_crop(state, 0);
-	crop->width = format->width * bin_h;
-	crop->height = format->height * bin_v;
+	crop->width = format->width * binning;
+	crop->height = format->height * binning;
 	crop->left = (IMX219_NATIVE_WIDTH - crop->width) / 2;
 	crop->top = (IMX219_NATIVE_HEIGHT - crop->height) / 2;
 

-- 
2.51.0

Re: [PATCH 1/2] media: i2c: imx219: Fix 1920x1080 mode to use 1:1 pixel aspect ratio

[Jacopo Mondi]

Hi Jai

On Fri, Oct 17, 2025 at 01:43:49PM +0530, Jai Luthra wrote:
> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
>
> Commit 0af46fbc333d ("media: i2c: imx219: Calculate crop rectangle
> dynamically") meant that the 1920x1080 mode switched from using no
> binning to using vertical binning but no horizontal binning, which
> resulted in stretched pixels.
>
> Until proper controls are available to independently select horizontal
> and vertical binning, restore the original 1:1 pixel aspect ratio by
> forcing binning to be uniform in both directions.

I think it makes sense and I wonder if binning in one direction and
not in the other will ever be needed in the general case.

For this driver indeed, this fixes a visible regression
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j
>
> Cc: stable@vger.kernel.org
> Fixes: 0af46fbc333d ("media: i2c: imx219: Calculate crop rectangle dynamically")
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> [Add comment & reword commit message]
> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> ---
>  drivers/media/i2c/imx219.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index c680aa6c3a55a9d865e79ad337b258cb681f98fe..300935b1ef2497050fe2808e4ceedda389a75b50 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -856,7 +856,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>  	const struct imx219_mode *mode;
>  	struct v4l2_mbus_framefmt *format;
>  	struct v4l2_rect *crop;
> -	u8 bin_h, bin_v;
> +	u8 bin_h, bin_v, binning;
>  	u32 prev_line_len;
>
>  	format = v4l2_subdev_state_get_format(state, 0);
> @@ -877,9 +877,12 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>  	bin_h = min(IMX219_PIXEL_ARRAY_WIDTH / format->width, 2U);
>  	bin_v = min(IMX219_PIXEL_ARRAY_HEIGHT / format->height, 2U);
>
> +	/* Ensure bin_h and bin_v are same to avoid 1:2 or 2:1 stretching */
> +	binning = min(bin_h, bin_v);
> +
>  	crop = v4l2_subdev_state_get_crop(state, 0);
> -	crop->width = format->width * bin_h;
> -	crop->height = format->height * bin_v;
> +	crop->width = format->width * binning;
> +	crop->height = format->height * binning;
>  	crop->left = (IMX219_NATIVE_WIDTH - crop->width) / 2;
>  	crop->top = (IMX219_NATIVE_HEIGHT - crop->height) / 2;
>
>
> --
> 2.51.0
>

[PATCH 2/2] media: i2c: imx219: Simplify imx219_get_binning() function

[Jai Luthra]

In imx219_set_pad_format() there is now a constraint to enforce hbin ==
vbin. So, simplify the logic in imx219_get_binning() function by
removing dead code that handles the case where hbin != vbin.

Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
---
 drivers/media/i2c/imx219.c | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index 300935b1ef2497050fe2808e4ceedda389a75b50..48efdcd2a8f96b678f9819223e0f9895fb4025ea 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -409,24 +409,14 @@ static void imx219_get_binning(struct v4l2_subdev_state *state, u8 *bin_h,
 	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;
+	} else {
+		*bin_h = IMX219_BINNING_NONE;
+		*bin_v = IMX219_BINNING_NONE;
 	}
 
-	if (hbin == 2)
-		*bin_h = IMX219_BINNING_X2;
-	if (vbin == 2)
-		*bin_v = IMX219_BINNING_X2;
 }
 
 static inline u32 imx219_get_rate_factor(struct v4l2_subdev_state *state)

-- 
2.51.0

Re: [PATCH 2/2] media: i2c: imx219: Simplify imx219_get_binning() function

[Jacopo Mondi]

On Fri, Oct 17, 2025 at 01:43:50PM +0530, Jai Luthra wrote:
> In imx219_set_pad_format() there is now a constraint to enforce hbin ==
> vbin. So, simplify the logic in imx219_get_binning() function by
> removing dead code that handles the case where hbin != vbin.
>
> Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> ---
>  drivers/media/i2c/imx219.c | 16 +++-------------
>  1 file changed, 3 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index 300935b1ef2497050fe2808e4ceedda389a75b50..48efdcd2a8f96b678f9819223e0f9895fb4025ea 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -409,24 +409,14 @@ static void imx219_get_binning(struct v4l2_subdev_state *state, u8 *bin_h,
>  	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;

So we're always going for BINNING_ANALOG_X2 whenever we bin now

I tested the binned mode 1640x1232 and 640x480 (which should bin then
crop) and both works fine.

I was wondering if we should then just rename ANALOG_X2 to X2 but the
datasheet actually defines that mode as "x2 analog (special) binning"
so I would keep the current name.

I didn't know, but the sensor can also x4 bin!

Anyway, for this patch, I would keep a comment around that says we
always use the special analog binning mode which is now the default.

This little nit apart
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Tested-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

> -
> -		return;
> +	} else {
> +		*bin_h = IMX219_BINNING_NONE;
> +		*bin_v = IMX219_BINNING_NONE;
>  	}
>
> -	if (hbin == 2)
> -		*bin_h = IMX219_BINNING_X2;
> -	if (vbin == 2)
> -		*bin_v = IMX219_BINNING_X2;
>  }
>
>  static inline u32 imx219_get_rate_factor(struct v4l2_subdev_state *state)
>
> --
> 2.51.0
>

Re: [PATCH 2/2] media: i2c: imx219: Simplify imx219_get_binning() function

[Jai Luthra]

Hi Jacopo

Thanks for the review,

Quoting Jacopo Mondi (2025-10-17 15:19:37)
> On Fri, Oct 17, 2025 at 01:43:50PM +0530, Jai Luthra wrote:
> > In imx219_set_pad_format() there is now a constraint to enforce hbin ==
> > vbin. So, simplify the logic in imx219_get_binning() function by
> > removing dead code that handles the case where hbin != vbin.
> >
> > Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
> > ---
> >  drivers/media/i2c/imx219.c | 16 +++-------------
> >  1 file changed, 3 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > index 300935b1ef2497050fe2808e4ceedda389a75b50..48efdcd2a8f96b678f9819223e0f9895fb4025ea 100644
> > --- a/drivers/media/i2c/imx219.c
> > +++ b/drivers/media/i2c/imx219.c
> > @@ -409,24 +409,14 @@ static void imx219_get_binning(struct v4l2_subdev_state *state, u8 *bin_h,
> >       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;
> 
> So we're always going for BINNING_ANALOG_X2 whenever we bin now
> 
> I tested the binned mode 1640x1232 and 640x480 (which should bin then
> crop) and both works fine.
> 
> I was wondering if we should then just rename ANALOG_X2 to X2 but the
> datasheet actually defines that mode as "x2 analog (special) binning"
> so I would keep the current name.
> 
> I didn't know, but the sensor can also x4 bin!

I actually tested the x4 binning with 640x480 sometime ago, it's available
here:
https://github.com/jailuthra/linux/tree/imx219_binning_4x

Will hopefully be upstream support for both x4 and x2 modes once Sakari's
series with binning controls is merged :-)

> 
> Anyway, for this patch, I would keep a comment around that says we
> always use the special analog binning mode which is now the default.
> 

Indeed, will add this in v2.

> This little nit apart
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Tested-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> 
> Thanks
>   j
> 
> > -
> > -             return;
> > +     } else {
> > +             *bin_h = IMX219_BINNING_NONE;
> > +             *bin_v = IMX219_BINNING_NONE;
> >       }
> >
> > -     if (hbin == 2)
> > -             *bin_h = IMX219_BINNING_X2;
> > -     if (vbin == 2)
> > -             *bin_v = IMX219_BINNING_X2;
> >  }
> >
> >  static inline u32 imx219_get_rate_factor(struct v4l2_subdev_state *state)
> >
> > --
> > 2.51.0
> >

Thanks,
    Jai