Re: [PATCH] media: imx274: add support for sensor mode6, 1280x540
From: Sakari Ailus
Date: Fri Sep 11 2020 - 17:45:11 EST
Hi Eugen,
Thanks for the patch.
On Wed, Sep 09, 2020 at 01:53:28PM +0300, Eugen Hristev wrote:
> Add support for the mode 6 for the sensor, this mode uses
> 3/8 subsampling and 3 horizontal binning.
> Aspect ratio is changed.
> Split the bin_ratio variable into two parts, one for
> width and one for height, as the ratio is no longer preserved
> when doing subsampling in this mode.
>
> Signed-off-by: Eugen Hristev <eugen.hristev@xxxxxxxxxxxxx>
> ---
> drivers/media/i2c/imx274.c | 89 +++++++++++++++++++++++++++++++-------
> 1 file changed, 73 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
> index e6aa9f32b6a8..5adb11f036e2 100644
> --- a/drivers/media/i2c/imx274.c
> +++ b/drivers/media/i2c/imx274.c
> @@ -147,8 +147,9 @@ static const struct regmap_config imx274_regmap_config = {
>
> enum imx274_binning {
> IMX274_BINNING_OFF,
> - IMX274_BINNING_2_1,
> - IMX274_BINNING_3_1,
> + IMX274_BINNING_2_2,
> + IMX274_BINNING_3_3,
> + IMX274_BINNING_4_3,
This enum is unused (apart from the useless use of the first value). Please
remove it (may be a new patch).
> };
>
> /*
> @@ -158,7 +159,8 @@ enum imx274_binning {
> * implemented modes.
> *
> * @init_regs: registers to initialize the mode
> - * @bin_ratio: downscale factor (e.g. 3 for 3:1 binning)
> + * @wbin_ratio: width downscale factor (e.g. 3 for 1280; 3 = 3840/1280)
> + * @hbin_ratio: height downscale factor (e.g. 3 for 720; 3 = 2160/720)
> * @min_frame_len: Minimum frame length for each mode (see "Frame Rate
> * Adjustment (CSI-2)" in the datasheet)
> * @min_SHR: Minimum SHR register value (see "Shutter Setting (CSI-2)" in the
> @@ -169,7 +171,8 @@ enum imx274_binning {
> */
> struct imx274_mode {
> const struct reg_8 *init_regs;
> - unsigned int bin_ratio;
> + unsigned int wbin_ratio;
> + unsigned int hbin_ratio;
These could be char or short.
> int min_frame_len;
> int min_SHR;
> int max_fps;
> @@ -332,6 +335,46 @@ static const struct reg_8 imx274_mode5_1280x720_raw10[] = {
> {IMX274_TABLE_END, 0x00}
> };
>
> +/*
> + * Vertical 2/8 subsampling horizontal 3 binning
> + * imx274 mode6(refer to datasheet) register configuration with
> + * 1280x540 resolution, raw10 data and mipi four lane output
> + */
> +static const struct reg_8 imx274_mode6_1280x540_raw10[] = {
> + {0x3004, 0x04}, /* mode setting */
> + {0x3005, 0x31},
> + {0x3006, 0x00},
> + {0x3007, 0x02}, /* mode setting */
> +
> + {0x3018, 0xA2}, /* output XVS, HVS */
> +
> + {0x306B, 0x05},
> + {0x30E2, 0x04}, /* mode setting */
> +
> + {0x30EE, 0x01},
> + {0x3342, 0x0A},
> + {0x3343, 0x00},
> + {0x3344, 0x16},
> + {0x3345, 0x00},
> + {0x33A6, 0x01},
> + {0x3528, 0x0E},
> + {0x3554, 0x1F},
> + {0x3555, 0x01},
> + {0x3556, 0x01},
> + {0x3557, 0x01},
> + {0x3558, 0x01},
> + {0x3559, 0x00},
> + {0x355A, 0x00},
> + {0x35BA, 0x0E},
> + {0x366A, 0x1B},
> + {0x366B, 0x1A},
> + {0x366C, 0x19},
> + {0x366D, 0x17},
> + {0x3A41, 0x04},
> +
> + {IMX274_TABLE_END, 0x00}
> +};
> +
> /*
> * imx274 first step register configuration for
> * starting stream
> @@ -445,7 +488,8 @@ static const struct reg_8 imx274_tp_regs[] = {
> static const struct imx274_mode imx274_modes[] = {
> {
> /* mode 1, 4K */
> - .bin_ratio = 1,
> + .wbin_ratio = 1, /* 3840 */
> + .hbin_ratio = 1, /* 2160 */
> .init_regs = imx274_mode1_3840x2160_raw10,
> .min_frame_len = 4550,
> .min_SHR = 12,
> @@ -454,7 +498,8 @@ static const struct imx274_mode imx274_modes[] = {
> },
> {
> /* mode 3, 1080p */
> - .bin_ratio = 2,
> + .wbin_ratio = 2, /* 1620 */
> + .hbin_ratio = 2, /* 1080 */
> .init_regs = imx274_mode3_1920x1080_raw10,
> .min_frame_len = 2310,
> .min_SHR = 8,
> @@ -463,13 +508,24 @@ static const struct imx274_mode imx274_modes[] = {
> },
> {
> /* mode 5, 720p */
> - .bin_ratio = 3,
> + .wbin_ratio = 3, /* 1280 */
> + .hbin_ratio = 3, /* 720 */
> .init_regs = imx274_mode5_1280x720_raw10,
> .min_frame_len = 2310,
> .min_SHR = 8,
> .max_fps = 120,
> .nocpiop = 112,
> },
> + {
> + /* mode 6, 540p */
> + .wbin_ratio = 3, /* 1280 */
> + .hbin_ratio = 4, /* 540 */
> + .init_regs = imx274_mode6_1280x540_raw10,
> + .min_frame_len = 2310,
> + .min_SHR = 4,
> + .max_fps = 120,
> + .nocpiop = 112,
> + },
> };
>
> /*
> @@ -892,12 +948,13 @@ static int __imx274_change_compose(struct stimx274 *imx274,
> }
>
> for (i = 0; i < ARRAY_SIZE(imx274_modes); i++) {
> - unsigned int ratio = imx274_modes[i].bin_ratio;
> + unsigned int wratio = imx274_modes[i].wbin_ratio;
> + unsigned int hratio = imx274_modes[i].hbin_ratio;
>
> int goodness = imx274_binning_goodness(
> imx274,
> - cur_crop->width / ratio, *width,
> - cur_crop->height / ratio, *height,
> + cur_crop->width / wratio, *width,
> + cur_crop->height / hratio, *height,
> flags);
>
> if (goodness >= best_goodness) {
> @@ -906,14 +963,14 @@ static int __imx274_change_compose(struct stimx274 *imx274,
> }
> }
>
> - *width = cur_crop->width / best_mode->bin_ratio;
> - *height = cur_crop->height / best_mode->bin_ratio;
> + *width = cur_crop->width / best_mode->wbin_ratio;
> + *height = cur_crop->height / best_mode->hbin_ratio;
>
> if (which == V4L2_SUBDEV_FORMAT_ACTIVE)
> imx274->mode = best_mode;
>
> - dev_dbg(dev, "%s: selected %u:1 binning\n",
> - __func__, best_mode->bin_ratio);
> + dev_dbg(dev, "%s: selected %u:%u binning\n",
I think the earlier message suggested it was ratio. How about "x"
instead of a colon?
Apart from these looks good to me.
> + __func__, best_mode->wbin_ratio, best_mode->hbin_ratio);
>
> tgt_fmt->width = *width;
> tgt_fmt->height = *height;
> @@ -1840,8 +1897,8 @@ static int imx274_probe(struct i2c_client *client)
> imx274->mode = &imx274_modes[IMX274_DEFAULT_BINNING];
> imx274->crop.width = IMX274_MAX_WIDTH;
> imx274->crop.height = IMX274_MAX_HEIGHT;
> - imx274->format.width = imx274->crop.width / imx274->mode->bin_ratio;
> - imx274->format.height = imx274->crop.height / imx274->mode->bin_ratio;
> + imx274->format.width = imx274->crop.width / imx274->mode->wbin_ratio;
> + imx274->format.height = imx274->crop.height / imx274->mode->hbin_ratio;
> imx274->format.field = V4L2_FIELD_NONE;
> imx274->format.code = MEDIA_BUS_FMT_SRGGB10_1X10;
> imx274->format.colorspace = V4L2_COLORSPACE_SRGB;
--
Kind regards,
Sakari Ailus