Hi Dave,
On Tue, Aug 22, 2023 at 07:15:21PM +0100, Dave Stevenson wrote:
> On Mon, 21 Aug 2023 at 23:30, 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@xxxxxxxxxxxxxxxx>
> > ---
> > 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 646d73d1e6a3..4140d9b78e4c 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>
> > @@ -152,9 +153,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,
> > },
> > };
> > @@ -830,6 +804,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;
> >
> > mode = v4l2_find_nearest_size(supported_modes,
> > ARRAY_SIZE(supported_modes),
> > @@ -839,10 +814,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 by the same factor horizontally and vertically.
> > + */
> > + bin = min3(IMX219_PIXEL_ARRAY_WIDTH / format->width,
> > + IMX219_PIXEL_ARRAY_HEIGHT / format->height, 2U);
>
> If you're insisting that binning is by the same factor horizontally
> and vertically, why consider both in the min? Either only look at one,
> or we start looking at making H & V binning independent.
I was considering making them independent. It should be fairly easy. If
you're fine with that, I think that would be my preference.
> > +
> > + crop = v4l2_subdev_get_pad_crop(sd, sd_state, 0);
> > + crop->width = format->width * bin;
> > + crop->height = format->height * bin;
> > + 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
