Re: [PATCH v4 18/20] media: i2c: imx219: Calculate crop rectangle dynamically

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Dave,

On Tue, Apr 15, 2025 at 03:20:40PM +0100, Dave Stevenson wrote:
> On Sun, 24 Sept 2023 at 16:33, Laurent Pinchart wrote:
> >
> > Calculate the crop rectangle size and location dynamically when setting
> > the format, instead of storing it in the imx219_mode structure. This
> > removes duplicated information from the mode, to guarantee consistency.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> > Changes since v2:
> >
> > - Handle horizontal and vertical binning separately
> > ---
> >  drivers/media/i2c/imx219.c | 45 +++++++++++++-------------------------
> >  1 file changed, 15 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > index bf1c2a1dad95..2b88c5b8a7bf 100644
> > --- a/drivers/media/i2c/imx219.c
> > +++ b/drivers/media/i2c/imx219.c
> > @@ -18,6 +18,7 @@
> >  #include <linux/delay.h>
> >  #include <linux/gpio/consumer.h>
> >  #include <linux/i2c.h>
> > +#include <linux/minmax.h>
> >  #include <linux/module.h>
> >  #include <linux/pm_runtime.h>
> >  #include <linux/regulator/consumer.h>
> > @@ -153,9 +154,6 @@ struct imx219_mode {
> >         /* Frame height */
> >         unsigned int height;
> >
> > -       /* Analog crop rectangle. */
> > -       struct v4l2_rect crop;
> > -
> >         /* V-timing */
> >         unsigned int vts_def;
> >  };
> > @@ -292,48 +290,24 @@ static const struct imx219_mode supported_modes[] = {
> >                 /* 8MPix 15fps mode */
> >                 .width = 3280,
> >                 .height = 2464,
> > -               .crop = {
> > -                       .left = IMX219_PIXEL_ARRAY_LEFT,
> > -                       .top = IMX219_PIXEL_ARRAY_TOP,
> > -                       .width = 3280,
> > -                       .height = 2464
> > -               },
> >                 .vts_def = 3526,
> >         },
> >         {
> >                 /* 1080P 30fps cropped */
> >                 .width = 1920,
> >                 .height = 1080,
> > -               .crop = {
> > -                       .left = 688,
> > -                       .top = 700,
> > -                       .width = 1920,
> > -                       .height = 1080
> > -               },
> >                 .vts_def = 1763,
> >         },
> >         {
> >                 /* 2x2 binned 30fps mode */
> >                 .width = 1640,
> >                 .height = 1232,
> > -               .crop = {
> > -                       .left = IMX219_PIXEL_ARRAY_LEFT,
> > -                       .top = IMX219_PIXEL_ARRAY_TOP,
> > -                       .width = 3280,
> > -                       .height = 2464
> > -               },
> >                 .vts_def = 1763,
> >         },
> >         {
> >                 /* 640x480 30fps mode */
> >                 .width = 640,
> >                 .height = 480,
> > -               .crop = {
> > -                       .left = 1008,
> > -                       .top = 760,
> > -                       .width = 1280,
> > -                       .height = 960
> > -               },
> >                 .vts_def = 1763,
> >         },
> >  };
> > @@ -844,6 +818,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> >         int exposure_max, exposure_def, hblank;
> >         struct v4l2_mbus_framefmt *format;
> >         struct v4l2_rect *crop;
> > +       unsigned int bin_h, bin_v;
> >
> >         mode = v4l2_find_nearest_size(supported_modes,
> >                                       ARRAY_SIZE(supported_modes),
> > @@ -853,10 +828,20 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> >         imx219_update_pad_format(imx219, mode, &fmt->format, fmt->format.code);
> >
> >         format = v4l2_subdev_get_pad_format(sd, sd_state, 0);
> > -       crop = v4l2_subdev_get_pad_crop(sd, sd_state, 0);
> > -
> >         *format = fmt->format;
> > -       *crop = mode->crop;
> > +
> > +       /*
> > +        * Use binning to maximize the crop rectangle size, and centre it in the
> > +        * sensor.
> > +        */
> > +       bin_h = min(IMX219_PIXEL_ARRAY_WIDTH / format->width, 2U);
> > +       bin_v = min(IMX219_PIXEL_ARRAY_HEIGHT / format->height, 2U);
> 
> This patch missed 6.6 as the previous LTS, so we've only just noticed
> this regression with 6.12. It's also present in 6.15-rc2.
> 
> The 1920x1080 mode will give you bin_h of 1, but bin_v of 2 as 1080 <
> (2480/2). You therefore get a weirdly stretched image from the sensor.
> 
> I accept that having a nicely generic cropping/binning configuration
> is ideal, but currently this is a mode based driver, and there is no
> API to allow for configuring generic cropping/binning configuration.
> Until that API exists and can be implemented for previously mode-based
> drivers without regression, I see the only real options being either
> - move the binning setup back into struct imx219_mode and defined in
> supported modes
> - set the two binning values to be the same as the minimum of the two
> computed values.
> Which would be preferred?

Different horizontal and vertical binning factors seem to be a niche use
case to me, so I think I'd prefer the second option. It should also be
the simplest to implement. Would you like to send a patch ?

> > +
> > +       crop = v4l2_subdev_get_pad_crop(sd, sd_state, 0);
> > +       crop->width = format->width * bin_h;
> > +       crop->height = format->height * bin_v;
> > +       crop->left = (IMX219_NATIVE_WIDTH - crop->width) / 2;
> > +       crop->top = (IMX219_NATIVE_HEIGHT - crop->height) / 2;
> >
> >         if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> >                 /* Update limits and set FPS to default */

-- 
Regards,

Laurent Pinchart