[PATCH v4 0/8] media: imx274: cleanups, improvements and SELECTION API support

[PATCH v4 0/8] media: imx274: cleanups, improvements and SELECTION API support

[Luca Ceresoli]

Hi,

this patchset introduces cropping support for the Sony IMX274 sensor
using the SELECTION API.

With respect to v3, this version uses the SELECTION API with taget
V4L2_SEL_TGT_COMPOSE to change the output resolution. This is the
recommended API for cropping + downscaling. However for backward
compatibility the set_format callback is still supported and is
equivalent to setting the compose rect as far as resolutions are
concerned.

Patches 1-5 are overall improvements and restructuring, mostly useful
to implement the SELECTION API in a clean way.

Patch 6 introduces a helper to allow setting many registers computed
at runtime in a straightforward way. This would not have been very
useful before because all long register write sequences came from
const tables, but it's definitely a must for the cropping code where
several register values are computed at runtime.

Patch 7 is new in this series, it's a trivial typo fix that can be
applied independently.

Patch 8 implements the set_selection pad operation for cropping
(V4L2_SEL_TGT_CROP) and binning (V4L2_SEL_TGT_COMPOSE). The most
tricky part was respecting all the device constraints on the
horizontal crop.

Usage examples:

 * Capture the entire 4K area, downscaled to 1080p with 2:1 binning:
   media-ctl -V '"IMX274":0[crop:(0,0)/3840x2160]'
   media-ctl -V '"IMX274":0[compose:(0,0)/1920x1080]'

 * Capture the central 1080p area (no binning):
   media-ctl -V '"IMX274":0[crop:(960,540)/1920x1080]'
   (this also sets the compose and fmt rects to 1920x1080)

Regards,
Luca


Luca Ceresoli (8):
  media: imx274: initialize format before v4l2 controls
  media: imx274: consolidate per-mode data in imx274_frmfmt
  media: imx274: get rid of mode_index
  media: imx274: actually use IMX274_DEFAULT_MODE
  media: imx274: simplify imx274_write_table()
  media: imx274: add helper function to fill a reg_8 table chunk
  media: imx274: fix typo
  media: imx274: add SELECTION support for cropping

 drivers/media/i2c/imx274.c | 686 ++++++++++++++++++++++++++++++---------------
 1 file changed, 461 insertions(+), 225 deletions(-)

-- 
2.7.4

[PATCH v4 1/8] media: imx274: initialize format before v4l2 controls

[Luca Ceresoli]

The current probe function calls v4l2_ctrl_handler_setup() before
initializing the format info. This triggers call paths such as:
imx274_probe -> v4l2_ctrl_handler_setup -> imx274_s_ctrl ->
imx274_set_exposure, where priv->mode_index is accessed before being
assigned.

This is wrong but does not trigger a visible bug because priv is
zero-initialized and 0 is the default value for priv->mode_index. But
this would become a crash in follow-up commits when mode_index is
replaced by a pointer that must always be valid.

Fix the bug before it shows up by initializing struct members early.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>

---
Changed v3 -> v4: nothing

Changed v2 -> v3: nothing

Changed v1 -> v2:
 - add "media: " prefix to commit message
---
 drivers/media/i2c/imx274.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 63fb94e7da37..8a8a11b8d75d 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -1632,6 +1632,16 @@ static int imx274_probe(struct i2c_client *client,
 
 	mutex_init(&imx274->lock);
 
+	/* initialize format */
+	imx274->mode_index = IMX274_MODE_3840X2160;
+	imx274->format.width = imx274_formats[0].size.width;
+	imx274->format.height = imx274_formats[0].size.height;
+	imx274->format.field = V4L2_FIELD_NONE;
+	imx274->format.code = MEDIA_BUS_FMT_SRGGB10_1X10;
+	imx274->format.colorspace = V4L2_COLORSPACE_SRGB;
+	imx274->frame_interval.numerator = 1;
+	imx274->frame_interval.denominator = IMX274_DEF_FRAME_RATE;
+
 	/* initialize regmap */
 	imx274->regmap = devm_regmap_init_i2c(client, &imx274_regmap_config);
 	if (IS_ERR(imx274->regmap)) {
@@ -1720,16 +1730,6 @@ static int imx274_probe(struct i2c_client *client,
 		goto err_ctrls;
 	}
 
-	/* initialize format */
-	imx274->mode_index = IMX274_MODE_3840X2160;
-	imx274->format.width = imx274_formats[0].size.width;
-	imx274->format.height = imx274_formats[0].size.height;
-	imx274->format.field = V4L2_FIELD_NONE;
-	imx274->format.code = MEDIA_BUS_FMT_SRGGB10_1X10;
-	imx274->format.colorspace = V4L2_COLORSPACE_SRGB;
-	imx274->frame_interval.numerator = 1;
-	imx274->frame_interval.denominator = IMX274_DEF_FRAME_RATE;
-
 	/* load default control values */
 	ret = imx274_load_default(imx274);
 	if (ret) {
-- 
2.7.4

[PATCH v4 2/8] media: imx274: consolidate per-mode data in imx274_frmfmt

[Luca Ceresoli]

Data about the implemented readout modes is partially stored in
imx274_formats[], the rest is scattered in several arrays. The latter
are then accessed using the mode index, e.g.:

  min_frame_len[priv->mode_index]

Consolidate all these data in imx274_formats[], and store a pointer to
the selected mode (i.e. imx274_formats[priv->mode_index]) in the main
device struct. This way code to use these data becomes more readable,
e.g.:

  priv->mode->min_frame_len

This removes lots of scaffolding code and keeps data about each mode
in a unique place.

Also remove a parameter to imx274_mode_regs() that is now unused.

While this adds the mode pointer to the device struct, it does not
remove the mode_index from it because mode_index is still used in two
dev_dbg() calls.  This will be handled in a follow-up commit.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>

---
Changed v3 -> v4: nothing

Changed v2 -> v3: nothing

Changed v1 -> v2:
 - add "media: " prefix to commit message
---
 drivers/media/i2c/imx274.c | 139 +++++++++++++++++++++------------------------
 1 file changed, 66 insertions(+), 73 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 8a8a11b8d75d..2ec31ae4e60d 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -147,10 +147,28 @@ enum imx274_mode {
 };
 
 /*
- * imx274 format related structure
+ * Parameters for each imx274 readout mode.
+ *
+ * These are the values to configure the sensor in one of the
+ * implemented modes.
+ *
+ * @size: recommended recording pixels
+ * @init_regs: registers to initialize the mode
+ * @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
+ *           datasheet)
+ * @max_fps: Maximum frames per second
+ * @nocpiop: Number of clocks per internal offset period (see "Integration Time
+ *           in Each Readout Drive Mode (CSI-2)" in the datasheet)
  */
 struct imx274_frmfmt {
 	struct v4l2_frmsize_discrete size;
+	const struct reg_8 *init_regs;
+	int min_frame_len;
+	int min_SHR;
+	int max_fps;
+	int nocpiop;
 };
 
 /*
@@ -476,58 +494,35 @@ static const struct reg_8 imx274_tp_regs[] = {
 	{IMX274_TABLE_END, 0x00}
 };
 
-static const struct reg_8 *mode_table[] = {
-	[IMX274_MODE_3840X2160]		= imx274_mode1_3840x2160_raw10,
-	[IMX274_MODE_1920X1080]		= imx274_mode3_1920x1080_raw10,
-	[IMX274_MODE_1280X720]		= imx274_mode5_1280x720_raw10,
-};
-
-/*
- * imx274 format related structure
- */
+/* nocpiop happens to be the same number for the implemented modes */
 static const struct imx274_frmfmt imx274_formats[] = {
-	{ {3840, 2160} },
-	{ {1920, 1080} },
-	{ {1280,  720} },
-};
-
-/*
- * minimal frame length for each mode
- * refer to datasheet section "Frame Rate Adjustment (CSI-2)"
- */
-static const int min_frame_len[] = {
-	4550, /* mode 1, 4K */
-	2310, /* mode 3, 1080p */
-	2310 /* mode 5, 720p */
-};
-
-/*
- * minimal numbers of SHR register
- * refer to datasheet table "Shutter Setting (CSI-2)"
- */
-static const int min_SHR[] = {
-	12, /* mode 1, 4K */
-	8, /* mode 3, 1080p */
-	8 /* mode 5, 720p */
-};
-
-static const int max_frame_rate[] = {
-	60, /* mode 1 , 4K */
-	120, /* mode 3, 1080p */
-	120 /* mode 5, 720p */
-};
-
-/*
- * Number of clocks per internal offset period
- * a constant based on mode
- * refer to section "Integration Time in Each Readout Drive Mode (CSI-2)"
- * in the datasheet
- * for the implemented 3 modes, it happens to be the same number
- */
-static const int nocpiop[] = {
-	112, /* mode 1 , 4K */
-	112, /* mode 3, 1080p */
-	112 /* mode 5, 720p */
+	{
+		/* mode 1, 4K */
+		.size = {3840, 2160},
+		.init_regs = imx274_mode1_3840x2160_raw10,
+		.min_frame_len = 4550,
+		.min_SHR = 12,
+		.max_fps = 60,
+		.nocpiop = 112,
+	},
+	{
+		/* mode 3, 1080p */
+		.size = {1920, 1080},
+		.init_regs = imx274_mode3_1920x1080_raw10,
+		.min_frame_len = 2310,
+		.min_SHR = 8,
+		.max_fps = 120,
+		.nocpiop = 112,
+	},
+	{
+		/* mode 5, 720p */
+		.size = {1280, 720},
+		.init_regs = imx274_mode5_1280x720_raw10,
+		.min_frame_len = 2310,
+		.min_SHR = 8,
+		.max_fps = 120,
+		.nocpiop = 112,
+	},
 };
 
 /*
@@ -557,6 +552,8 @@ struct imx274_ctrls {
  * @regmap: Pointer to regmap structure
  * @reset_gpio: Pointer to reset gpio
  * @lock: Mutex structure
+ * @mode: Parameters for the selected readout mode
+ *        (points to imx274_formats[mode_index])
  * @mode_index: Resolution mode index
  */
 struct stimx274 {
@@ -569,6 +566,7 @@ struct stimx274 {
 	struct regmap *regmap;
 	struct gpio_desc *reset_gpio;
 	struct mutex lock; /* mutex lock for operations */
+	const struct imx274_frmfmt *mode;
 	u32 mode_index;
 };
 
@@ -704,18 +702,12 @@ static int imx274_write_table(struct stimx274 *priv, const struct reg_8 table[])
 }
 
 /*
- * imx274_mode_regs - Function for set mode registers per mode index
+ * Set mode registers to start stream.
  * @priv: Pointer to device structure
- * @mode: Mode index value
- *
- * This is used to start steam per mode index.
- * mode = 0, start stream for sensor Mode 1: 4K/raw10
- * mode = 1, start stream for sensor Mode 3: 1080p/raw10
- * mode = 2, start stream for sensor Mode 5: 720p/raw10
  *
  * Return: 0 on success, errors otherwise
  */
-static int imx274_mode_regs(struct stimx274 *priv, int mode)
+static int imx274_mode_regs(struct stimx274 *priv)
 {
 	int err = 0;
 
@@ -727,7 +719,7 @@ static int imx274_mode_regs(struct stimx274 *priv, int mode)
 	if (err)
 		return err;
 
-	err = imx274_write_table(priv, mode_table[mode]);
+	err = imx274_write_table(priv, priv->mode->init_regs);
 
 	return err;
 }
@@ -889,6 +881,7 @@ static int imx274_set_fmt(struct v4l2_subdev *sd,
 	}
 
 	imx274->mode_index = index;
+	imx274->mode = &imx274_formats[index];
 
 	if (fmt->width > IMX274_MAX_WIDTH)
 		fmt->width = IMX274_MAX_WIDTH;
@@ -1042,7 +1035,7 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on)
 
 	if (on) {
 		/* load mode registers */
-		ret = imx274_mode_regs(imx274, imx274->mode_index);
+		ret = imx274_mode_regs(imx274);
 		if (ret)
 			goto fail;
 
@@ -1146,14 +1139,14 @@ static int imx274_clamp_coarse_time(struct stimx274 *priv, u32 *val,
 	if (err)
 		return err;
 
-	if (*frame_length < min_frame_len[priv->mode_index])
-		*frame_length = min_frame_len[priv->mode_index];
+	if (*frame_length < priv->mode->min_frame_len)
+		*frame_length =  priv->mode->min_frame_len;
 
 	*val = *frame_length - *val; /* convert to raw shr */
 	if (*val > *frame_length - IMX274_SHR_LIMIT_CONST)
 		*val = *frame_length - IMX274_SHR_LIMIT_CONST;
-	else if (*val < min_SHR[priv->mode_index])
-		*val = min_SHR[priv->mode_index];
+	else if (*val < priv->mode->min_SHR)
+		*val = priv->mode->min_SHR;
 
 	return 0;
 }
@@ -1365,7 +1358,7 @@ static int imx274_set_exposure(struct stimx274 *priv, int val)
 	}
 
 	coarse_time = (IMX274_PIXCLK_CONST1 / IMX274_PIXCLK_CONST2 * val
-			- nocpiop[priv->mode_index]) / hmax;
+			- priv->mode->nocpiop) / hmax;
 
 	/* step 2: convert exposure_time into SHR value */
 
@@ -1375,7 +1368,7 @@ static int imx274_set_exposure(struct stimx274 *priv, int val)
 		goto fail;
 
 	priv->ctrls.exposure->val =
-			(coarse_time * hmax + nocpiop[priv->mode_index])
+			(coarse_time * hmax + priv->mode->nocpiop)
 			/ (IMX274_PIXCLK_CONST1 / IMX274_PIXCLK_CONST2);
 
 	dev_dbg(&priv->client->dev,
@@ -1529,10 +1522,9 @@ static int imx274_set_frame_interval(struct stimx274 *priv,
 				/ frame_interval.numerator);
 
 	/* boundary check */
-	if (req_frame_rate > max_frame_rate[priv->mode_index]) {
+	if (req_frame_rate > priv->mode->max_fps) {
 		frame_interval.numerator = 1;
-		frame_interval.denominator =
-					max_frame_rate[priv->mode_index];
+		frame_interval.denominator = priv->mode->max_fps;
 	} else if (req_frame_rate < IMX274_MIN_FRAME_RATE) {
 		frame_interval.numerator = 1;
 		frame_interval.denominator = IMX274_MIN_FRAME_RATE;
@@ -1634,8 +1626,9 @@ static int imx274_probe(struct i2c_client *client,
 
 	/* initialize format */
 	imx274->mode_index = IMX274_MODE_3840X2160;
-	imx274->format.width = imx274_formats[0].size.width;
-	imx274->format.height = imx274_formats[0].size.height;
+	imx274->mode = &imx274_formats[imx274->mode_index];
+	imx274->format.width = imx274->mode->size.width;
+	imx274->format.height = imx274->mode->size.height;
 	imx274->format.field = V4L2_FIELD_NONE;
 	imx274->format.code = MEDIA_BUS_FMT_SRGGB10_1X10;
 	imx274->format.colorspace = V4L2_COLORSPACE_SRGB;
-- 
2.7.4

[PATCH v4 3/8] media: imx274: get rid of mode_index

[Luca Ceresoli]

After restructuring struct imx274_frmfmt, the mode_index field is
still in use only for two dev_dbg() calls in imx274_s_stream(). Let's
remove it and avoid duplicated information.

Replacing the first usage requires some rather annoying but trivial
pointer math. The other one can be removed entirely since it would
print the same value anyway.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>

---
Changed v3 -> v4: nothing

Changed v2 -> v3:
 - really fix dev_dbg() format mismatch warning for both 32 and 64 bit

Changed v1 -> v2:
 - add "media: " prefix to commit message
 - fix dev_dbg() format mismatch warning
   ("warning: format ‘%ld’ expects argument of type ‘long int’, but argument 6 has type ‘int’")
 - slightly improve commit message
---
 drivers/media/i2c/imx274.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 2ec31ae4e60d..f075715ffced 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -553,8 +553,6 @@ struct imx274_ctrls {
  * @reset_gpio: Pointer to reset gpio
  * @lock: Mutex structure
  * @mode: Parameters for the selected readout mode
- *        (points to imx274_formats[mode_index])
- * @mode_index: Resolution mode index
  */
 struct stimx274 {
 	struct v4l2_subdev sd;
@@ -567,7 +565,6 @@ struct stimx274 {
 	struct gpio_desc *reset_gpio;
 	struct mutex lock; /* mutex lock for operations */
 	const struct imx274_frmfmt *mode;
-	u32 mode_index;
 };
 
 /*
@@ -880,7 +877,6 @@ static int imx274_set_fmt(struct v4l2_subdev *sd,
 		index = 0;
 	}
 
-	imx274->mode_index = index;
 	imx274->mode = &imx274_formats[index];
 
 	if (fmt->width > IMX274_MAX_WIDTH)
@@ -1028,8 +1024,9 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on)
 	struct stimx274 *imx274 = to_imx274(sd);
 	int ret = 0;
 
-	dev_dbg(&imx274->client->dev, "%s : %s, mode index = %d\n", __func__,
-		on ? "Stream Start" : "Stream Stop", imx274->mode_index);
+	dev_dbg(&imx274->client->dev, "%s : %s, mode index = %td\n", __func__,
+		on ? "Stream Start" : "Stream Stop",
+		imx274->mode - &imx274_formats[0]);
 
 	mutex_lock(&imx274->lock);
 
@@ -1068,8 +1065,7 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on)
 	}
 
 	mutex_unlock(&imx274->lock);
-	dev_dbg(&imx274->client->dev,
-		"%s : Done: mode = %d\n", __func__, imx274->mode_index);
+	dev_dbg(&imx274->client->dev, "%s : Done\n", __func__);
 	return 0;
 
 fail:
@@ -1625,8 +1621,7 @@ static int imx274_probe(struct i2c_client *client,
 	mutex_init(&imx274->lock);
 
 	/* initialize format */
-	imx274->mode_index = IMX274_MODE_3840X2160;
-	imx274->mode = &imx274_formats[imx274->mode_index];
+	imx274->mode = &imx274_formats[IMX274_MODE_3840X2160];
 	imx274->format.width = imx274->mode->size.width;
 	imx274->format.height = imx274->mode->size.height;
 	imx274->format.field = V4L2_FIELD_NONE;
-- 
2.7.4

[PATCH v4 4/8] media: imx274: actually use IMX274_DEFAULT_MODE

[Luca Ceresoli]

IMX274_DEFAULT_MODE is defined but not used. Start using it, so the
default can be more easily changed without digging into the code.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>

---
Changed v3 -> v4: nothing

Changed v2 -> v3: nothing

Changed v1 -> v2:
 - add "media: " prefix to commit message
---
 drivers/media/i2c/imx274.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index f075715ffced..ceeec97cd330 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -1621,7 +1621,7 @@ static int imx274_probe(struct i2c_client *client,
 	mutex_init(&imx274->lock);
 
 	/* initialize format */
-	imx274->mode = &imx274_formats[IMX274_MODE_3840X2160];
+	imx274->mode = &imx274_formats[IMX274_DEFAULT_MODE];
 	imx274->format.width = imx274->mode->size.width;
 	imx274->format.height = imx274->mode->size.height;
 	imx274->format.field = V4L2_FIELD_NONE;
-- 
2.7.4

[PATCH v4 5/8] media: imx274: simplify imx274_write_table()

[Luca Ceresoli]

imx274_write_table() is a mere wrapper (and the only user) to
imx274_regmap_util_write_table_8(). Remove this useless indirection by
merging the two functions into one.

Also get rid of the wait_ms_addr and end_addr parameters since it does
not make any sense to give them any values other than
IMX274_TABLE_WAIT_MS and IMX274_TABLE_END.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>

---
Changed v3 -> v4: nothing

Changed v2 -> v3: nothing

Changed v1 -> v2:
 - add "media: " prefix to commit message
---
 drivers/media/i2c/imx274.c | 28 ++++++++++------------------
 1 file changed, 10 insertions(+), 18 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index ceeec97cd330..48343c2ade83 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -597,20 +597,18 @@ static inline struct stimx274 *to_imx274(struct v4l2_subdev *sd)
 }
 
 /*
- * imx274_regmap_util_write_table_8 - Function for writing register table
- * @regmap: Pointer to device reg map structure
- * @table: Table containing register values
- * @wait_ms_addr: Flag for performing delay
- * @end_addr: Flag for incating end of table
+ * Writing a register table
+ *
+ * @priv: Pointer to device
+ * @table: Table containing register values (with optional delays)
  *
  * This is used to write register table into sensor's reg map.
  *
  * Return: 0 on success, errors otherwise
  */
-static int imx274_regmap_util_write_table_8(struct regmap *regmap,
-					    const struct reg_8 table[],
-					    u16 wait_ms_addr, u16 end_addr)
+static int imx274_write_table(struct stimx274 *priv, const struct reg_8 table[])
 {
+	struct regmap *regmap = priv->regmap;
 	int err = 0;
 	const struct reg_8 *next;
 	u8 val;
@@ -622,8 +620,8 @@ static int imx274_regmap_util_write_table_8(struct regmap *regmap,
 
 	for (next = table;; next++) {
 		if ((next->addr != range_start + range_count) ||
-		    (next->addr == end_addr) ||
-		    (next->addr == wait_ms_addr) ||
+		    (next->addr == IMX274_TABLE_END) ||
+		    (next->addr == IMX274_TABLE_WAIT_MS) ||
 		    (range_count == max_range_vals)) {
 			if (range_count == 1)
 				err = regmap_write(regmap,
@@ -642,10 +640,10 @@ static int imx274_regmap_util_write_table_8(struct regmap *regmap,
 			range_count = 0;
 
 			/* Handle special address values */
-			if (next->addr == end_addr)
+			if (next->addr == IMX274_TABLE_END)
 				break;
 
-			if (next->addr == wait_ms_addr) {
+			if (next->addr == IMX274_TABLE_WAIT_MS) {
 				msleep_range(next->val);
 				continue;
 			}
@@ -692,12 +690,6 @@ static inline int imx274_write_reg(struct stimx274 *priv, u16 addr, u8 val)
 	return err;
 }
 
-static int imx274_write_table(struct stimx274 *priv, const struct reg_8 table[])
-{
-	return imx274_regmap_util_write_table_8(priv->regmap,
-		table, IMX274_TABLE_WAIT_MS, IMX274_TABLE_END);
-}
-
 /*
  * Set mode registers to start stream.
  * @priv: Pointer to device structure
-- 
2.7.4

[PATCH v4 6/8] media: imx274: add helper function to fill a reg_8 table chunk

[Luca Ceresoli]

Tables of struct reg_8 are used to simplify multi-byte register
assignment. However filling these snippets with values computed at
runtime is currently implemented by very similar functions doing the
needed shift & mask manipulation.

Replace all those functions with a unique helper function to fill
reg_8 tables in a simple and clean way.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>

---
Changed v3 -> v4: nothing

Changed v2 -> v3:
 - minor reformatting in prepare_reg() documentation

Changed v1 -> v2:
 - add "media: " prefix to commit message
---
 drivers/media/i2c/imx274.c | 90 ++++++++++++++++++++++++++++------------------
 1 file changed, 55 insertions(+), 35 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 48343c2ade83..e5ba19b97083 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -597,6 +597,58 @@ static inline struct stimx274 *to_imx274(struct v4l2_subdev *sd)
 }
 
 /*
+ * Fill consecutive reg_8 items in order to set a multibyte register.
+ *
+ * The sensor has many 2-bytes registers and a 3-byte register. This
+ * simplifies code to set them by filling consecutive entries of a
+ * struct reg_8 table with the data to set a register.
+ *
+ * The number of table entries that is filled is the minimum needed
+ * for the given number of bits (i.e. nbits / 8, rounded up). If nbits
+ * is not a multiple of 8, extra bits in the most significant byte are
+ * zeroed.
+ *
+ * Example:
+ *   Calling prepare_reg(&regs[10], 0x3000, 0xcafe, 12) will set:
+ *   regs[10] = { .addr = 0x3000, .val = 0xfe }
+ *   regs[11] = { .addr = 0x3001, .val = 0x0a }
+ *
+ * @table_base: Pointer to the first reg_8 struct to be filled.  The
+ *              following entries will also be set, make sure they are
+ *              properly allocated!
+ * @addr_lsb: Address of the LSB register.  Other registers must be
+ *            consecutive, least-to-most significant.
+ * @value: Value to be written to the register.
+ * @nbits: Number of bits to write (range: [1..24])
+ */
+static void prepare_reg(struct reg_8 *table_base,
+			u16 addr_lsb,
+			u32 value,
+			size_t nbits)
+{
+	struct reg_8 *cmd = table_base;
+	u16 addr = addr_lsb;
+	size_t bits; /* how many bits at this round */
+
+	WARN_ON(nbits > 24);
+
+	if (nbits > 24)
+		nbits = 24;
+
+	while (nbits > 0) {
+		bits = min_t(size_t, 8, nbits);
+
+		cmd->addr = addr;
+		cmd->val = value & ((1 << bits) - 1);
+
+		nbits -= bits;
+		value >>= 8;
+		addr++;
+		cmd++;
+	}
+}
+
+/*
  * Writing a register table
  *
  * @priv: Pointer to device
@@ -1163,15 +1215,6 @@ static int imx274_set_digital_gain(struct stimx274 *priv, u32 dgain)
 				reg_val & IMX274_MASK_LSB_4_BITS);
 }
 
-static inline void imx274_calculate_gain_regs(struct reg_8 regs[2], u16 gain)
-{
-	regs->addr = IMX274_ANALOG_GAIN_ADDR_MSB;
-	regs->val = (gain >> IMX274_SHIFT_8_BITS) & IMX274_MASK_LSB_3_BITS;
-
-	(regs + 1)->addr = IMX274_ANALOG_GAIN_ADDR_LSB;
-	(regs + 1)->val = (gain) & IMX274_MASK_LSB_8_BITS;
-}
-
 /*
  * imx274_set_gain - Function called when setting gain
  * @priv: Pointer to device structure
@@ -1229,7 +1272,7 @@ static int imx274_set_gain(struct stimx274 *priv, struct v4l2_ctrl *ctrl)
 	if (gain_reg > IMX274_GAIN_REG_MAX)
 		gain_reg = IMX274_GAIN_REG_MAX;
 
-	imx274_calculate_gain_regs(reg_list, (u16)gain_reg);
+	prepare_reg(reg_list, IMX274_ANALOG_GAIN_ADDR_LSB, gain_reg, 11);
 
 	for (i = 0; i < ARRAY_SIZE(reg_list); i++) {
 		err = imx274_write_reg(priv, reg_list[i].addr,
@@ -1258,16 +1301,6 @@ static int imx274_set_gain(struct stimx274 *priv, struct v4l2_ctrl *ctrl)
 	return err;
 }
 
-static inline void imx274_calculate_coarse_time_regs(struct reg_8 regs[2],
-						     u32 coarse_time)
-{
-	regs->addr = IMX274_SHR_REG_MSB;
-	regs->val = (coarse_time >> IMX274_SHIFT_8_BITS)
-			& IMX274_MASK_LSB_8_BITS;
-	(regs + 1)->addr = IMX274_SHR_REG_LSB;
-	(regs + 1)->val = (coarse_time) & IMX274_MASK_LSB_8_BITS;
-}
-
 /*
  * imx274_set_coarse_time - Function called when setting SHR value
  * @priv: Pointer to device structure
@@ -1292,7 +1325,7 @@ static int imx274_set_coarse_time(struct stimx274 *priv, u32 *val)
 		goto fail;
 
 	/* prepare SHR registers */
-	imx274_calculate_coarse_time_regs(reg_list, coarse_time);
+	prepare_reg(reg_list, IMX274_SHR_REG_LSB, coarse_time, 16);
 
 	/* write to SHR registers */
 	for (i = 0; i < ARRAY_SIZE(reg_list); i++) {
@@ -1429,19 +1462,6 @@ static int imx274_set_test_pattern(struct stimx274 *priv, int val)
 	return err;
 }
 
-static inline void imx274_calculate_frame_length_regs(struct reg_8 regs[3],
-						      u32 frame_length)
-{
-	regs->addr = IMX274_VMAX_REG_1;
-	regs->val = (frame_length >> IMX274_SHIFT_16_BITS)
-			& IMX274_MASK_LSB_4_BITS;
-	(regs + 1)->addr = IMX274_VMAX_REG_2;
-	(regs + 1)->val = (frame_length >> IMX274_SHIFT_8_BITS)
-			& IMX274_MASK_LSB_8_BITS;
-	(regs + 2)->addr = IMX274_VMAX_REG_3;
-	(regs + 2)->val = (frame_length) & IMX274_MASK_LSB_8_BITS;
-}
-
 /*
  * imx274_set_frame_length - Function called when setting frame length
  * @priv: Pointer to device structure
@@ -1463,7 +1483,7 @@ static int imx274_set_frame_length(struct stimx274 *priv, u32 val)
 
 	frame_length = (u32)val;
 
-	imx274_calculate_frame_length_regs(reg_list, frame_length);
+	prepare_reg(reg_list, IMX274_VMAX_REG_3, frame_length, 20);
 	for (i = 0; i < ARRAY_SIZE(reg_list); i++) {
 		err = imx274_write_reg(priv, reg_list[i].addr,
 				       reg_list[i].val);
-- 
2.7.4

Re: [PATCH v4 6/8] media: imx274: add helper function to fill a reg_8 table chunk

[Sakari Ailus]

Hi Luca,

On Mon, Jun 11, 2018 at 01:35:37PM +0200, Luca Ceresoli wrote:
> Tables of struct reg_8 are used to simplify multi-byte register
> assignment. However filling these snippets with values computed at
> runtime is currently implemented by very similar functions doing the
> needed shift & mask manipulation.
> 
> Replace all those functions with a unique helper function to fill
> reg_8 tables in a simple and clean way.

What's the purpose of writing these registers as multiple I²C writes, when
this can be done as a single write (i.e. the address followed by two or
three octets of data)?

> 
> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> ---
> Changed v3 -> v4: nothing
> 
> Changed v2 -> v3:
>  - minor reformatting in prepare_reg() documentation
> 
> Changed v1 -> v2:
>  - add "media: " prefix to commit message
> ---
>  drivers/media/i2c/imx274.c | 90 ++++++++++++++++++++++++++++------------------
>  1 file changed, 55 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
> index 48343c2ade83..e5ba19b97083 100644
> --- a/drivers/media/i2c/imx274.c
> +++ b/drivers/media/i2c/imx274.c
> @@ -597,6 +597,58 @@ static inline struct stimx274 *to_imx274(struct v4l2_subdev *sd)
>  }
>  
>  /*
> + * Fill consecutive reg_8 items in order to set a multibyte register.
> + *
> + * The sensor has many 2-bytes registers and a 3-byte register. This
> + * simplifies code to set them by filling consecutive entries of a
> + * struct reg_8 table with the data to set a register.
> + *
> + * The number of table entries that is filled is the minimum needed
> + * for the given number of bits (i.e. nbits / 8, rounded up). If nbits
> + * is not a multiple of 8, extra bits in the most significant byte are
> + * zeroed.
> + *
> + * Example:
> + *   Calling prepare_reg(&regs[10], 0x3000, 0xcafe, 12) will set:
> + *   regs[10] = { .addr = 0x3000, .val = 0xfe }
> + *   regs[11] = { .addr = 0x3001, .val = 0x0a }
> + *
> + * @table_base: Pointer to the first reg_8 struct to be filled.  The
> + *              following entries will also be set, make sure they are
> + *              properly allocated!
> + * @addr_lsb: Address of the LSB register.  Other registers must be
> + *            consecutive, least-to-most significant.
> + * @value: Value to be written to the register.
> + * @nbits: Number of bits to write (range: [1..24])
> + */
> +static void prepare_reg(struct reg_8 *table_base,
> +			u16 addr_lsb,
> +			u32 value,
> +			size_t nbits)
> +{
> +	struct reg_8 *cmd = table_base;
> +	u16 addr = addr_lsb;
> +	size_t bits; /* how many bits at this round */
> +
> +	WARN_ON(nbits > 24);
> +
> +	if (nbits > 24)
> +		nbits = 24;
> +
> +	while (nbits > 0) {
> +		bits = min_t(size_t, 8, nbits);
> +
> +		cmd->addr = addr;
> +		cmd->val = value & ((1 << bits) - 1);
> +
> +		nbits -= bits;
> +		value >>= 8;
> +		addr++;
> +		cmd++;
> +	}
> +}
> +
> +/*
>   * Writing a register table
>   *
>   * @priv: Pointer to device
> @@ -1163,15 +1215,6 @@ static int imx274_set_digital_gain(struct stimx274 *priv, u32 dgain)
>  				reg_val & IMX274_MASK_LSB_4_BITS);
>  }
>  
> -static inline void imx274_calculate_gain_regs(struct reg_8 regs[2], u16 gain)
> -{
> -	regs->addr = IMX274_ANALOG_GAIN_ADDR_MSB;
> -	regs->val = (gain >> IMX274_SHIFT_8_BITS) & IMX274_MASK_LSB_3_BITS;
> -
> -	(regs + 1)->addr = IMX274_ANALOG_GAIN_ADDR_LSB;
> -	(regs + 1)->val = (gain) & IMX274_MASK_LSB_8_BITS;
> -}
> -
>  /*
>   * imx274_set_gain - Function called when setting gain
>   * @priv: Pointer to device structure
> @@ -1229,7 +1272,7 @@ static int imx274_set_gain(struct stimx274 *priv, struct v4l2_ctrl *ctrl)
>  	if (gain_reg > IMX274_GAIN_REG_MAX)
>  		gain_reg = IMX274_GAIN_REG_MAX;
>  
> -	imx274_calculate_gain_regs(reg_list, (u16)gain_reg);
> +	prepare_reg(reg_list, IMX274_ANALOG_GAIN_ADDR_LSB, gain_reg, 11);
>  
>  	for (i = 0; i < ARRAY_SIZE(reg_list); i++) {
>  		err = imx274_write_reg(priv, reg_list[i].addr,
> @@ -1258,16 +1301,6 @@ static int imx274_set_gain(struct stimx274 *priv, struct v4l2_ctrl *ctrl)
>  	return err;
>  }
>  
> -static inline void imx274_calculate_coarse_time_regs(struct reg_8 regs[2],
> -						     u32 coarse_time)
> -{
> -	regs->addr = IMX274_SHR_REG_MSB;
> -	regs->val = (coarse_time >> IMX274_SHIFT_8_BITS)
> -			& IMX274_MASK_LSB_8_BITS;
> -	(regs + 1)->addr = IMX274_SHR_REG_LSB;
> -	(regs + 1)->val = (coarse_time) & IMX274_MASK_LSB_8_BITS;
> -}
> -
>  /*
>   * imx274_set_coarse_time - Function called when setting SHR value
>   * @priv: Pointer to device structure
> @@ -1292,7 +1325,7 @@ static int imx274_set_coarse_time(struct stimx274 *priv, u32 *val)
>  		goto fail;
>  
>  	/* prepare SHR registers */
> -	imx274_calculate_coarse_time_regs(reg_list, coarse_time);
> +	prepare_reg(reg_list, IMX274_SHR_REG_LSB, coarse_time, 16);
>  
>  	/* write to SHR registers */
>  	for (i = 0; i < ARRAY_SIZE(reg_list); i++) {
> @@ -1429,19 +1462,6 @@ static int imx274_set_test_pattern(struct stimx274 *priv, int val)
>  	return err;
>  }
>  
> -static inline void imx274_calculate_frame_length_regs(struct reg_8 regs[3],
> -						      u32 frame_length)
> -{
> -	regs->addr = IMX274_VMAX_REG_1;
> -	regs->val = (frame_length >> IMX274_SHIFT_16_BITS)
> -			& IMX274_MASK_LSB_4_BITS;
> -	(regs + 1)->addr = IMX274_VMAX_REG_2;
> -	(regs + 1)->val = (frame_length >> IMX274_SHIFT_8_BITS)
> -			& IMX274_MASK_LSB_8_BITS;
> -	(regs + 2)->addr = IMX274_VMAX_REG_3;
> -	(regs + 2)->val = (frame_length) & IMX274_MASK_LSB_8_BITS;
> -}
> -
>  /*
>   * imx274_set_frame_length - Function called when setting frame length
>   * @priv: Pointer to device structure
> @@ -1463,7 +1483,7 @@ static int imx274_set_frame_length(struct stimx274 *priv, u32 val)
>  
>  	frame_length = (u32)val;
>  
> -	imx274_calculate_frame_length_regs(reg_list, frame_length);
> +	prepare_reg(reg_list, IMX274_VMAX_REG_3, frame_length, 20);
>  	for (i = 0; i < ARRAY_SIZE(reg_list); i++) {
>  		err = imx274_write_reg(priv, reg_list[i].addr,
>  				       reg_list[i].val);
> -- 
> 2.7.4
> 

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCH v4 6/8] media: imx274: add helper function to fill a reg_8 table chunk

[Luca Ceresoli]

Hi Sakari,

On 26/06/2018 14:20, Sakari Ailus wrote:
> Hi Luca,
> 
> On Mon, Jun 11, 2018 at 01:35:37PM +0200, Luca Ceresoli wrote:
>> Tables of struct reg_8 are used to simplify multi-byte register
>> assignment. However filling these snippets with values computed at
>> runtime is currently implemented by very similar functions doing the
>> needed shift & mask manipulation.
>>
>> Replace all those functions with a unique helper function to fill
>> reg_8 tables in a simple and clean way.
> 
> What's the purpose of writing these registers as multiple I²C writes, when
> this can be done as a single write (i.e. the address followed by two or
> three octets of data)?

Good point. The for loops applying the register values (the lines just
after those changed by my patch) defuse the regmap bulk write capability.

I guess this could be improved not filling any table, but directly
calling regmap_bulk_write(), passing the u16 or u32 register value with
proper endianness. No tables, less code. This would replace the present
patch with a shorter and more effective one. Is it what you was suggesting?

I'll try that.
-- 
Luca

Re: [PATCH v4 6/8] media: imx274: add helper function to fill a reg_8 table chunk

[Sakari Ailus]

On Wed, Jun 27, 2018 at 10:13:12AM +0200, Luca Ceresoli wrote:
> Hi Sakari,
> 
> On 26/06/2018 14:20, Sakari Ailus wrote:
> > Hi Luca,
> > 
> > On Mon, Jun 11, 2018 at 01:35:37PM +0200, Luca Ceresoli wrote:
> >> Tables of struct reg_8 are used to simplify multi-byte register
> >> assignment. However filling these snippets with values computed at
> >> runtime is currently implemented by very similar functions doing the
> >> needed shift & mask manipulation.
> >>
> >> Replace all those functions with a unique helper function to fill
> >> reg_8 tables in a simple and clean way.
> > 
> > What's the purpose of writing these registers as multiple I²C writes, when
> > this can be done as a single write (i.e. the address followed by two or
> > three octets of data)?
> 
> Good point. The for loops applying the register values (the lines just
> after those changed by my patch) defuse the regmap bulk write capability.
> 
> I guess this could be improved not filling any table, but directly
> calling regmap_bulk_write(), passing the u16 or u32 register value with
> proper endianness. No tables, less code. This would replace the present
> patch with a shorter and more effective one. Is it what you was suggesting?

Yes, please.

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCH v4 6/8] media: imx274: add helper function to fill a reg_8 table chunk

[Luca Ceresoli]

Hi Sakari,

On 27/06/2018 11:30, Sakari Ailus wrote:
> On Wed, Jun 27, 2018 at 10:13:12AM +0200, Luca Ceresoli wrote:
>> Hi Sakari,
>>
>> On 26/06/2018 14:20, Sakari Ailus wrote:
>>> Hi Luca,
>>>
>>> On Mon, Jun 11, 2018 at 01:35:37PM +0200, Luca Ceresoli wrote:
>>>> Tables of struct reg_8 are used to simplify multi-byte register
>>>> assignment. However filling these snippets with values computed at
>>>> runtime is currently implemented by very similar functions doing the
>>>> needed shift & mask manipulation.
>>>>
>>>> Replace all those functions with a unique helper function to fill
>>>> reg_8 tables in a simple and clean way.
>>>
>>> What's the purpose of writing these registers as multiple I²C writes, when
>>> this can be done as a single write (i.e. the address followed by two or
>>> three octets of data)?
>>
>> Good point. The for loops applying the register values (the lines just
>> after those changed by my patch) defuse the regmap bulk write capability.
>>
>> I guess this could be improved not filling any table, but directly
>> calling regmap_bulk_write(), passing the u16 or u32 register value with
>> proper endianness. No tables, less code. This would replace the present
>> patch with a shorter and more effective one. Is it what you was suggesting?
> 
> Yes, please.

Ok, will do. I think this will cut many lines of code, wow!

Patch 8 is only marginally related to this change, so it can be reviewed
independently except for the few lines in imx274_apply_trimming() where
registers are written.

-- 
Luca

[PATCH v4 7/8] media: imx274: fix typo

[Luca Ceresoli]

pd -> pad

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>

---
Changed v3 -> v4: nothing
---
 drivers/media/i2c/imx274.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index e5ba19b97083..2c13961e9764 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -544,7 +544,7 @@ struct imx274_ctrls {
 /*
  * struct stim274 - imx274 device structure
  * @sd: V4L2 subdevice structure
- * @pd: Media pad structure
+ * @pad: Media pad structure
  * @client: Pointer to I2C client
  * @ctrls: imx274 control structure
  * @format: V4L2 media bus frame format structure
-- 
2.7.4

[PATCH v4 8/8] media: imx274: add SELECTION support for cropping

[Luca Ceresoli]

Currently this driver does not support cropping. The supported modes
are the following, all capturing the entire area:

 - 3840x2160, 1:1 binning (native sensor resolution)
 - 1920x1080, 2:1 binning
 - 1280x720,  3:1 binning

The VIDIOC_SUBDEV_S_FMT ioctl chooses among these 3 configurations the
one that matches the requested format.

Add cropping support via VIDIOC_SUBDEV_S_SELECTION: with target
V4L2_SEL_TGT_CROP to choose the captured area, with
V4L2_SEL_TGT_COMPOSE to choose the output resolution.

To maintain backward compatibility we also allow setting the compose
format via VIDIOC_SUBDEV_S_FMT. To obtain this, compose rect and
output format are computed in the common helper function
__imx274_change_compose(), which sets both to the same width/height
values.

Cropping also calls __imx274_change_compose() whenever cropping rect
size changes in order to reset the compose rect (and output format
size) for 1:1 binning.

Also change the names in enum imx274_mode from being resolution-based
to being binning-based (e.g. from IMX274_MODE_1920X1080 to
IMX274_MODE_BINNING_2_1). Without cropping, the two naming are
equivalent. With cropping, the resolution could be different,
e.g. using 2:1 binning mode to crop 1200x960 and output a 600x480
format. Using binning in the names avoids any misunderstanding.  For
the same reason, replace the 'size' field in struct imx274_frmfmt with
'bin_ratio'.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>

---
Changed v3 -> v4:
 - Set the binning via the SELECTION API (COMPOSE rect), but still
   allow using VIDIOC_SUBDEV_S_FMT for backward compatibility.
 - rename imx274_set_trimming -> imx274_apply_trimming for clarity

Changed v2 -> v3:
 - Remove hard-coded HMAX reg setting from all modes, not only the
   first one. HMAX is always computed and set in s_stream now.
 - Reword commit log message.

Changed v1 -> v2:
 - add "media: " prefix to commit message
---
 drivers/media/i2c/imx274.c | 416 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 326 insertions(+), 90 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 2c13961e9764..8bfc20a46b3d 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -5,6 +5,7 @@
  *
  * Leon Luo <leonl@leopardimaging.com>
  * Edwin Zou <edwinz@leopardimaging.com>
+ * Luca Ceresoli <luca@lucaceresoli.net>
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms and conditions of the GNU General Public License,
@@ -19,6 +20,7 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <linux/kernel.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/gpio.h>
@@ -74,7 +76,7 @@
  */
 #define IMX274_MIN_EXPOSURE_TIME		(4 * 260 / 72)
 
-#define IMX274_DEFAULT_MODE			IMX274_MODE_3840X2160
+#define IMX274_DEFAULT_MODE			IMX274_MODE_BINNING_OFF
 #define IMX274_MAX_WIDTH			(3840)
 #define IMX274_MAX_HEIGHT			(2160)
 #define IMX274_MAX_FRAME_RATE			(120)
@@ -111,6 +113,20 @@
 #define IMX274_SHR_REG_LSB			0x300C /* SHR */
 #define IMX274_SVR_REG_MSB			0x300F /* SVR */
 #define IMX274_SVR_REG_LSB			0x300E /* SVR */
+#define IMX274_HTRIM_EN_REG			0x3037
+#define IMX274_HTRIM_START_REG_LSB		0x3038
+#define IMX274_HTRIM_START_REG_MSB		0x3039
+#define IMX274_HTRIM_END_REG_LSB		0x303A
+#define IMX274_HTRIM_END_REG_MSB		0x303B
+#define IMX274_VWIDCUTEN_REG			0x30DD
+#define IMX274_VWIDCUT_REG_LSB			0x30DE
+#define IMX274_VWIDCUT_REG_MSB			0x30DF
+#define IMX274_VWINPOS_REG_LSB			0x30E0
+#define IMX274_VWINPOS_REG_MSB			0x30E1
+#define IMX274_WRITE_VSIZE_REG_LSB		0x3130
+#define IMX274_WRITE_VSIZE_REG_MSB		0x3131
+#define IMX274_Y_OUT_SIZE_REG_LSB		0x3132
+#define IMX274_Y_OUT_SIZE_REG_MSB		0x3133
 #define IMX274_VMAX_REG_1			0x30FA /* VMAX, MSB */
 #define IMX274_VMAX_REG_2			0x30F9 /* VMAX */
 #define IMX274_VMAX_REG_3			0x30F8 /* VMAX, LSB */
@@ -141,9 +157,9 @@ static const struct regmap_config imx274_regmap_config = {
 };
 
 enum imx274_mode {
-	IMX274_MODE_3840X2160,
-	IMX274_MODE_1920X1080,
-	IMX274_MODE_1280X720,
+	IMX274_MODE_BINNING_OFF,
+	IMX274_MODE_BINNING_2_1,
+	IMX274_MODE_BINNING_3_1,
 };
 
 /*
@@ -152,8 +168,8 @@ enum imx274_mode {
  * These are the values to configure the sensor in one of the
  * implemented modes.
  *
- * @size: recommended recording pixels
  * @init_regs: registers to initialize the mode
+ * @bin_ratio: downscale factor (e.g. 3 for 3:1 binning)
  * @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
@@ -163,8 +179,8 @@ enum imx274_mode {
  *           in Each Readout Drive Mode (CSI-2)" in the datasheet)
  */
 struct imx274_frmfmt {
-	struct v4l2_frmsize_discrete size;
 	const struct reg_8 *init_regs;
+	unsigned int bin_ratio;
 	int min_frame_len;
 	int min_SHR;
 	int max_fps;
@@ -215,31 +231,14 @@ static const struct reg_8 imx274_mode1_3840x2160_raw10[] = {
 	{0x3004, 0x01},
 	{0x3005, 0x01},
 	{0x3006, 0x00},
-	{0x3007, 0x02},
+	{0x3007, 0xa2},
 
 	{0x3018, 0xA2}, /* output XVS, HVS */
 
 	{0x306B, 0x05},
 	{0x30E2, 0x01},
-	{0x30F6, 0x07}, /* HMAX, 263 */
-	{0x30F7, 0x01}, /* HMAX */
-
-	{0x30dd, 0x01}, /* crop to 2160 */
-	{0x30de, 0x06},
-	{0x30df, 0x00},
-	{0x30e0, 0x12},
-	{0x30e1, 0x00},
-	{0x3037, 0x01}, /* to crop to 3840 */
-	{0x3038, 0x0c},
-	{0x3039, 0x00},
-	{0x303a, 0x0c},
-	{0x303b, 0x0f},
 
 	{0x30EE, 0x01},
-	{0x3130, 0x86},
-	{0x3131, 0x08},
-	{0x3132, 0x7E},
-	{0x3133, 0x08},
 	{0x3342, 0x0A},
 	{0x3343, 0x00},
 	{0x3344, 0x16},
@@ -273,32 +272,14 @@ static const struct reg_8 imx274_mode3_1920x1080_raw10[] = {
 	{0x3004, 0x02},
 	{0x3005, 0x21},
 	{0x3006, 0x00},
-	{0x3007, 0x11},
+	{0x3007, 0xb1},
 
 	{0x3018, 0xA2}, /* output XVS, HVS */
 
 	{0x306B, 0x05},
 	{0x30E2, 0x02},
 
-	{0x30F6, 0x04}, /* HMAX, 260 */
-	{0x30F7, 0x01}, /* HMAX */
-
-	{0x30dd, 0x01}, /* to crop to 1920x1080 */
-	{0x30de, 0x05},
-	{0x30df, 0x00},
-	{0x30e0, 0x04},
-	{0x30e1, 0x00},
-	{0x3037, 0x01},
-	{0x3038, 0x0c},
-	{0x3039, 0x00},
-	{0x303a, 0x0c},
-	{0x303b, 0x0f},
-
 	{0x30EE, 0x01},
-	{0x3130, 0x4E},
-	{0x3131, 0x04},
-	{0x3132, 0x46},
-	{0x3133, 0x04},
 	{0x3342, 0x0A},
 	{0x3343, 0x00},
 	{0x3344, 0x1A},
@@ -331,31 +312,14 @@ static const struct reg_8 imx274_mode5_1280x720_raw10[] = {
 	{0x3004, 0x03},
 	{0x3005, 0x31},
 	{0x3006, 0x00},
-	{0x3007, 0x09},
+	{0x3007, 0xa9},
 
 	{0x3018, 0xA2}, /* output XVS, HVS */
 
 	{0x306B, 0x05},
 	{0x30E2, 0x03},
 
-	{0x30F6, 0x04}, /* HMAX, 260 */
-	{0x30F7, 0x01}, /* HMAX */
-
-	{0x30DD, 0x01},
-	{0x30DE, 0x07},
-	{0x30DF, 0x00},
-	{0x40E0, 0x04},
-	{0x30E1, 0x00},
-	{0x3030, 0xD4},
-	{0x3031, 0x02},
-	{0x3032, 0xD0},
-	{0x3033, 0x02},
-
 	{0x30EE, 0x01},
-	{0x3130, 0xE2},
-	{0x3131, 0x02},
-	{0x3132, 0xDE},
-	{0x3133, 0x02},
 	{0x3342, 0x0A},
 	{0x3343, 0x00},
 	{0x3344, 0x1B},
@@ -498,7 +462,7 @@ static const struct reg_8 imx274_tp_regs[] = {
 static const struct imx274_frmfmt imx274_formats[] = {
 	{
 		/* mode 1, 4K */
-		.size = {3840, 2160},
+		.bin_ratio = 1,
 		.init_regs = imx274_mode1_3840x2160_raw10,
 		.min_frame_len = 4550,
 		.min_SHR = 12,
@@ -507,7 +471,7 @@ static const struct imx274_frmfmt imx274_formats[] = {
 	},
 	{
 		/* mode 3, 1080p */
-		.size = {1920, 1080},
+		.bin_ratio = 2,
 		.init_regs = imx274_mode3_1920x1080_raw10,
 		.min_frame_len = 2310,
 		.min_SHR = 8,
@@ -516,7 +480,7 @@ static const struct imx274_frmfmt imx274_formats[] = {
 	},
 	{
 		/* mode 5, 720p */
-		.size = {1280, 720},
+		.bin_ratio = 3,
 		.init_regs = imx274_mode5_1280x720_raw10,
 		.min_frame_len = 2310,
 		.min_SHR = 8,
@@ -547,7 +511,10 @@ struct imx274_ctrls {
  * @pad: Media pad structure
  * @client: Pointer to I2C client
  * @ctrls: imx274 control structure
+ * @crop: rect to be captured
+ * @compose: compose rect, i.e. output resolution
  * @format: V4L2 media bus frame format structure
+ *          (width and height are in sync with the compose rect)
  * @frame_rate: V4L2 frame rate structure
  * @regmap: Pointer to regmap structure
  * @reset_gpio: Pointer to reset gpio
@@ -559,6 +526,8 @@ struct stimx274 {
 	struct media_pad pad;
 	struct i2c_client *client;
 	struct imx274_ctrls ctrls;
+	struct v4l2_rect crop;
+	struct v4l2_rect compose;
 	struct v4l2_mbus_framefmt format;
 	struct v4l2_fract frame_interval;
 	struct regmap *regmap;
@@ -864,6 +833,80 @@ static int imx274_s_ctrl(struct v4l2_ctrl *ctrl)
 }
 
 /**
+ * Helper function to change binning and set both compose and format.
+ *
+ * We have two entry points to change binning: set_fmt and
+ * set_selection(COMPOSE). Both have to compute the new output size
+ * and set it in both the compose rect and the frame format size. We
+ * also need to do the same things after setting cropping to restore
+ * 1:1 binning.
+ *
+ * This function contains the common code for these three cases, it
+ * has many arguments in order to accomodate the needs of all of them.
+ *
+ * Must be called with imx274->lock locked.
+ *
+ * @imx274 The device object
+ * @cfg    The pad config we are editing for TRY requests
+ * @which  V4L2_SUBDEV_FORMAT_ACTIVE or V4L2_SUBDEV_FORMAT_TRY from the caller
+ * @width  Input-output parameter: set to the desired width before
+ *         the call, contains the chosen value after returning successfully
+ * @height Input-output parameter for height (see @width)
+ */
+static int __imx274_change_compose(struct stimx274 *imx274,
+				   struct v4l2_subdev_pad_config *cfg,
+				   u32 which,
+				   u32 *width,
+				   u32 *height)
+{
+	struct device *dev = &imx274->client->dev;
+	const struct v4l2_rect *cur_crop;
+	struct v4l2_mbus_framefmt *tgt_fmt;
+	struct v4l2_rect *tgt_compose;
+	unsigned int ratio; /* Binning ratio */
+
+	dev_dbg(dev, "%s: request of  %dx%d (%s)\n",
+		__func__, *width, *height,
+		(which == V4L2_SUBDEV_FORMAT_ACTIVE) ? "ACTIVE" : "TRY");
+
+	if (which == V4L2_SUBDEV_FORMAT_TRY) {
+		cur_crop = &cfg->try_crop;
+		tgt_compose = &cfg->try_compose;
+		tgt_fmt = &cfg->try_fmt;
+	} else {
+		cur_crop = &imx274->crop;
+		tgt_compose = &imx274->compose;
+		tgt_fmt = &imx274->format;
+	}
+
+	/* Find ratio (maximize output resolution). Fallback to 1:1. */
+	for (ratio = 3; ratio > 1; ratio--)
+		if (*width <= DIV_ROUND_UP(cur_crop->width, ratio) &&
+		    *height <= DIV_ROUND_UP(cur_crop->height, ratio))
+			break;
+
+	*width = cur_crop->width / ratio;
+	*height = cur_crop->height / ratio;
+
+	if (which == V4L2_SUBDEV_FORMAT_ACTIVE)
+		imx274->mode = &imx274_formats[ratio - 1];
+
+	dev_dbg(dev, "%s: adjusted to %dx%d (%d:1 binning)\n",
+		__func__, *width, *height, ratio);
+
+	tgt_compose->top = 0;
+	tgt_compose->left = 0;
+	tgt_compose->width = *width;
+	tgt_compose->height = *height;
+
+	tgt_fmt->width = *width;
+	tgt_fmt->height = *height;
+	tgt_fmt->field = V4L2_FIELD_NONE;
+
+	return 0;
+}
+
+/**
  * imx274_get_fmt - Get the pad format
  * @sd: Pointer to V4L2 Sub device structure
  * @cfg: Pointer to sub device pad information structure
@@ -901,45 +944,228 @@ static int imx274_set_fmt(struct v4l2_subdev *sd,
 {
 	struct v4l2_mbus_framefmt *fmt = &format->format;
 	struct stimx274 *imx274 = to_imx274(sd);
-	struct i2c_client *client = imx274->client;
-	int index;
-
-	dev_dbg(&client->dev,
-		"%s: width = %d height = %d code = %d\n",
-		__func__, fmt->width, fmt->height, fmt->code);
+	int err = 0;
 
 	mutex_lock(&imx274->lock);
 
-	for (index = 0; index < ARRAY_SIZE(imx274_formats); index++) {
-		if (imx274_formats[index].size.width == fmt->width &&
-		    imx274_formats[index].size.height == fmt->height)
-			break;
-	}
-
-	if (index >= ARRAY_SIZE(imx274_formats)) {
-		/* default to first format */
-		index = 0;
-	}
+	err = __imx274_change_compose(imx274, cfg, format->which,
+				      &fmt->width, &fmt->height);
 
-	imx274->mode = &imx274_formats[index];
+	if (err)
+		goto out;
 
-	if (fmt->width > IMX274_MAX_WIDTH)
-		fmt->width = IMX274_MAX_WIDTH;
-	if (fmt->height > IMX274_MAX_HEIGHT)
-		fmt->height = IMX274_MAX_HEIGHT;
-	fmt->width = fmt->width & (~IMX274_MASK_LSB_2_BITS);
-	fmt->height = fmt->height & (~IMX274_MASK_LSB_2_BITS);
+	/*
+	 * __imx274_change_compose already set width and height in the
+	 * applicable format, but we need to keep all other format
+	 * values, so do a full copy here
+	 */
 	fmt->field = V4L2_FIELD_NONE;
-
 	if (format->which == V4L2_SUBDEV_FORMAT_TRY)
 		cfg->try_fmt = *fmt;
 	else
 		imx274->format = *fmt;
 
+out:
 	mutex_unlock(&imx274->lock);
+
+	return err;
+}
+
+static int imx274_get_selection(struct v4l2_subdev *sd,
+				struct v4l2_subdev_pad_config *cfg,
+				struct v4l2_subdev_selection *sel)
+{
+	struct stimx274 *imx274 = to_imx274(sd);
+	const struct v4l2_rect *src_crop;
+	const struct v4l2_rect *src_compose;
+	int ret = 0;
+
+	if (sel->pad != 0)
+		return -EINVAL;
+
+	if (sel->target == V4L2_SEL_TGT_CROP_BOUNDS) {
+		sel->r.left = 0;
+		sel->r.top = 0;
+		sel->r.width = IMX274_MAX_WIDTH;
+		sel->r.height = IMX274_MAX_HEIGHT;
+		return 0;
+	}
+
+	if (sel->which == V4L2_SUBDEV_FORMAT_TRY) {
+		src_crop = &cfg->try_crop;
+		src_compose = &cfg->try_compose;
+	} else {
+		src_crop = &imx274->crop;
+		src_compose = &imx274->compose;
+	}
+
+	mutex_lock(&imx274->lock);
+
+	switch (sel->target) {
+	case V4L2_SEL_TGT_CROP:
+		sel->r = *src_crop;
+		break;
+	case V4L2_SEL_TGT_COMPOSE_BOUNDS:
+		sel->r.top = 0;
+		sel->r.left = 0;
+		sel->r.width = src_crop->width;
+		sel->r.height = src_crop->height;
+		break;
+	case V4L2_SEL_TGT_COMPOSE:
+		sel->r = *src_compose;
+		break;
+	default:
+		ret = -EINVAL;
+	}
+
+	mutex_unlock(&imx274->lock);
+
+	return ret;
+}
+
+static int imx274_set_selection_crop(struct stimx274 *imx274,
+				     struct v4l2_subdev_pad_config *cfg,
+				     struct v4l2_subdev_selection *sel)
+{
+	struct device *dev = &imx274->client->dev;
+	struct v4l2_rect *tgt_crop;
+	struct v4l2_rect new_crop;
+	bool size_changed;
+
+	/*
+	 * h_step could be 12 or 24 depending on the binning. But we
+	 * won't know the binning until we choose the mode later in
+	 * imx274_set_fmt(). Thus let's be safe and use the most
+	 * conservative value in all cases.
+	 */
+	const int h_step = 24;
+
+	dev_dbg(dev, "%s: request of  (%d,%d)%dx%d (%s)\n", __func__,
+		sel->r.left, sel->r.top, sel->r.width, sel->r.height,
+		V4L2_SUBDEV_FORMAT_ACTIVE ? "ACTIVE" : "TRY");
+
+	new_crop.width = rounddown(min_t(u32, sel->r.width, IMX274_MAX_WIDTH),
+				   h_step);
+
+	/* Constraint: HTRIMMING_END - HTRIMMING_START >= 144 */
+	if (new_crop.width < 144)
+		new_crop.width = 144;
+
+	new_crop.left = min_t(u32, roundup(sel->r.left, h_step),
+			      IMX274_MAX_WIDTH - new_crop.width);
+
+	new_crop.height =
+		ALIGN(min_t(u32, sel->r.height, IMX274_MAX_HEIGHT), 2);
+
+	new_crop.top = min_t(u32, ALIGN(sel->r.top, 2),
+			     IMX274_MAX_HEIGHT - new_crop.height);
+
+	dev_dbg(dev, "%s: adjusted to (%d,%d)%dx%d", __func__,
+		new_crop.left, new_crop.top, new_crop.width, new_crop.height);
+
+	sel->r = new_crop;
+
+	if (sel->which == V4L2_SUBDEV_FORMAT_TRY)
+		tgt_crop = &cfg->try_crop;
+	else
+		tgt_crop = &imx274->crop;
+
+	mutex_lock(&imx274->lock);
+
+	size_changed = (new_crop.width != tgt_crop->width ||
+			new_crop.height != tgt_crop->height);
+
+	/* __imx274_change_compose needs the new size in *tgt_crop */
+	*tgt_crop = new_crop;
+
+	/* if crop size changed then reset the output image size */
+	if (size_changed)
+		__imx274_change_compose(imx274, cfg, sel->which,
+					&new_crop.width, &new_crop.height);
+
+	mutex_unlock(&imx274->lock);
+
 	return 0;
 }
 
+static int imx274_set_selection(struct v4l2_subdev *sd,
+				struct v4l2_subdev_pad_config *cfg,
+				struct v4l2_subdev_selection *sel)
+{
+	struct stimx274 *imx274 = to_imx274(sd);
+
+	if (sel->pad != 0)
+		return -EINVAL;
+
+	if (sel->target == V4L2_SEL_TGT_CROP)
+		return imx274_set_selection_crop(imx274, cfg, sel);
+
+	if (sel->target == V4L2_SEL_TGT_COMPOSE) {
+		int err;
+
+		mutex_lock(&imx274->lock);
+
+		err =  __imx274_change_compose(imx274, cfg, sel->which,
+					       &sel->r.width, &sel->r.height);
+		/*
+		 * __imx274_change_compose already set width and
+		 * height in set->r, we still need to set top-left
+		 */
+		if (!err) {
+			sel->r.top = 0;
+			sel->r.left = 0;
+		}
+
+		mutex_unlock(&imx274->lock);
+
+		return err;
+	}
+
+	return -EINVAL;
+}
+
+static int imx274_apply_trimming(struct stimx274 *imx274)
+{
+	u32 h_start;
+	u32 h_end;
+	u32 hmax;
+	u32 v_cut;
+	s32 v_pos;
+	u32 write_v_size;
+	u32 y_out_size;
+	struct reg_8 regs[17];
+
+	h_start = imx274->crop.left + 12;
+	h_end = h_start + imx274->crop.width;
+
+	/* Use the minimum allowed value of HMAX */
+	/* Note: except in mode 1, (width / 16 + 23) is always < hmax_min */
+	/* Note: 260 is the minimum HMAX in all implemented modes */
+	hmax = max_t(u32, 260, (imx274->crop.width) / 16 + 23);
+
+	/* invert v_pos if VFLIP */
+	v_pos = imx274->ctrls.vflip->cur.val ?
+		(-imx274->crop.top / 2) : (imx274->crop.top / 2);
+	v_cut = (IMX274_MAX_HEIGHT - imx274->crop.height) / 2;
+	write_v_size = imx274->crop.height + 22;
+	y_out_size   = imx274->crop.height + 14;
+
+	prepare_reg(&regs[0],  IMX274_HMAX_REG_LSB,        hmax,         16);
+	prepare_reg(&regs[2],  IMX274_HTRIM_EN_REG,        1,             1);
+	prepare_reg(&regs[3],  IMX274_HTRIM_START_REG_LSB, h_start,      13);
+	prepare_reg(&regs[5],  IMX274_HTRIM_END_REG_LSB,   h_end,        13);
+
+	prepare_reg(&regs[7],  IMX274_VWIDCUTEN_REG,       1,             1);
+	prepare_reg(&regs[8],  IMX274_VWIDCUT_REG_LSB,     v_cut,        11);
+	prepare_reg(&regs[10], IMX274_VWINPOS_REG_LSB,     v_pos,        12);
+	prepare_reg(&regs[12], IMX274_WRITE_VSIZE_REG_LSB, write_v_size, 13);
+	prepare_reg(&regs[14], IMX274_Y_OUT_SIZE_REG_LSB,  y_out_size,   13);
+
+	regs[16].addr = IMX274_TABLE_END;
+
+	return imx274_write_table(imx274, regs);
+}
+
 /**
  * imx274_g_frame_interval - Get the frame interval
  * @sd: Pointer to V4L2 Sub device structure
@@ -1080,6 +1306,10 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on)
 		if (ret)
 			goto fail;
 
+		ret = imx274_apply_trimming(imx274);
+		if (ret)
+			goto fail;
+
 		/*
 		 * update frame rate & expsoure. if the last mode is different,
 		 * HMAX could be changed. As the result, frame rate & exposure
@@ -1589,6 +1819,8 @@ static int imx274_set_frame_interval(struct stimx274 *priv,
 static const struct v4l2_subdev_pad_ops imx274_pad_ops = {
 	.get_fmt = imx274_get_fmt,
 	.set_fmt = imx274_set_fmt,
+	.get_selection = imx274_get_selection,
+	.set_selection = imx274_set_selection,
 };
 
 static const struct v4l2_subdev_video_ops imx274_video_ops = {
@@ -1634,8 +1866,12 @@ static int imx274_probe(struct i2c_client *client,
 
 	/* initialize format */
 	imx274->mode = &imx274_formats[IMX274_DEFAULT_MODE];
-	imx274->format.width = imx274->mode->size.width;
-	imx274->format.height = imx274->mode->size.height;
+	imx274->crop.width = IMX274_MAX_WIDTH;
+	imx274->crop.height = IMX274_MAX_HEIGHT;
+	imx274->compose.width = imx274->crop.width / imx274->mode->bin_ratio;
+	imx274->compose.height = imx274->crop.height / imx274->mode->bin_ratio;
+	imx274->format.width = imx274->compose.width;
+	imx274->format.height = imx274->compose.height;
 	imx274->format.field = V4L2_FIELD_NONE;
 	imx274->format.code = MEDIA_BUS_FMT_SRGGB10_1X10;
 	imx274->format.colorspace = V4L2_COLORSPACE_SRGB;
-- 
2.7.4

Re: [PATCH v4 8/8] media: imx274: add SELECTION support for cropping

[Sakari Ailus]

On Mon, Jun 11, 2018 at 01:35:39PM +0200, Luca Ceresoli wrote:
> Currently this driver does not support cropping. The supported modes
> are the following, all capturing the entire area:
> 
>  - 3840x2160, 1:1 binning (native sensor resolution)
>  - 1920x1080, 2:1 binning
>  - 1280x720,  3:1 binning
> 
> The VIDIOC_SUBDEV_S_FMT ioctl chooses among these 3 configurations the
> one that matches the requested format.
> 
> Add cropping support via VIDIOC_SUBDEV_S_SELECTION: with target
> V4L2_SEL_TGT_CROP to choose the captured area, with
> V4L2_SEL_TGT_COMPOSE to choose the output resolution.
> 
> To maintain backward compatibility we also allow setting the compose
> format via VIDIOC_SUBDEV_S_FMT. To obtain this, compose rect and
> output format are computed in the common helper function
> __imx274_change_compose(), which sets both to the same width/height
> values.
> 
> Cropping also calls __imx274_change_compose() whenever cropping rect
> size changes in order to reset the compose rect (and output format
> size) for 1:1 binning.
> 
> Also change the names in enum imx274_mode from being resolution-based
> to being binning-based (e.g. from IMX274_MODE_1920X1080 to
> IMX274_MODE_BINNING_2_1). Without cropping, the two naming are
> equivalent. With cropping, the resolution could be different,
> e.g. using 2:1 binning mode to crop 1200x960 and output a 600x480
> format. Using binning in the names avoids any misunderstanding.  For
> the same reason, replace the 'size' field in struct imx274_frmfmt with
> 'bin_ratio'.
> 
> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> ---
> Changed v3 -> v4:
>  - Set the binning via the SELECTION API (COMPOSE rect), but still
>    allow using VIDIOC_SUBDEV_S_FMT for backward compatibility.
>  - rename imx274_set_trimming -> imx274_apply_trimming for clarity
> 
> Changed v2 -> v3:
>  - Remove hard-coded HMAX reg setting from all modes, not only the
>    first one. HMAX is always computed and set in s_stream now.
>  - Reword commit log message.
> 
> Changed v1 -> v2:
>  - add "media: " prefix to commit message
> ---
>  drivers/media/i2c/imx274.c | 416 +++++++++++++++++++++++++++++++++++----------
>  1 file changed, 326 insertions(+), 90 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
> index 2c13961e9764..8bfc20a46b3d 100644
> --- a/drivers/media/i2c/imx274.c
> +++ b/drivers/media/i2c/imx274.c
> @@ -5,6 +5,7 @@
>   *
>   * Leon Luo <leonl@leopardimaging.com>
>   * Edwin Zou <edwinz@leopardimaging.com>
> + * Luca Ceresoli <luca@lucaceresoli.net>
>   *
>   * This program is free software; you can redistribute it and/or modify it
>   * under the terms and conditions of the GNU General Public License,
> @@ -19,6 +20,7 @@
>   * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> +#include <linux/kernel.h>

Alphabetical order, please.

>  #include <linux/clk.h>
>  #include <linux/delay.h>
>  #include <linux/gpio.h>
> @@ -74,7 +76,7 @@
>   */
>  #define IMX274_MIN_EXPOSURE_TIME		(4 * 260 / 72)
>  
> -#define IMX274_DEFAULT_MODE			IMX274_MODE_3840X2160
> +#define IMX274_DEFAULT_MODE			IMX274_MODE_BINNING_OFF
>  #define IMX274_MAX_WIDTH			(3840)
>  #define IMX274_MAX_HEIGHT			(2160)
>  #define IMX274_MAX_FRAME_RATE			(120)
> @@ -111,6 +113,20 @@
>  #define IMX274_SHR_REG_LSB			0x300C /* SHR */
>  #define IMX274_SVR_REG_MSB			0x300F /* SVR */
>  #define IMX274_SVR_REG_LSB			0x300E /* SVR */
> +#define IMX274_HTRIM_EN_REG			0x3037
> +#define IMX274_HTRIM_START_REG_LSB		0x3038
> +#define IMX274_HTRIM_START_REG_MSB		0x3039
> +#define IMX274_HTRIM_END_REG_LSB		0x303A
> +#define IMX274_HTRIM_END_REG_MSB		0x303B
> +#define IMX274_VWIDCUTEN_REG			0x30DD
> +#define IMX274_VWIDCUT_REG_LSB			0x30DE
> +#define IMX274_VWIDCUT_REG_MSB			0x30DF
> +#define IMX274_VWINPOS_REG_LSB			0x30E0
> +#define IMX274_VWINPOS_REG_MSB			0x30E1
> +#define IMX274_WRITE_VSIZE_REG_LSB		0x3130
> +#define IMX274_WRITE_VSIZE_REG_MSB		0x3131
> +#define IMX274_Y_OUT_SIZE_REG_LSB		0x3132
> +#define IMX274_Y_OUT_SIZE_REG_MSB		0x3133
>  #define IMX274_VMAX_REG_1			0x30FA /* VMAX, MSB */
>  #define IMX274_VMAX_REG_2			0x30F9 /* VMAX */
>  #define IMX274_VMAX_REG_3			0x30F8 /* VMAX, LSB */
> @@ -141,9 +157,9 @@ static const struct regmap_config imx274_regmap_config = {
>  };
>  
>  enum imx274_mode {

Is this still an appropriate name? imx274_binning?

> -	IMX274_MODE_3840X2160,
> -	IMX274_MODE_1920X1080,
> -	IMX274_MODE_1280X720,
> +	IMX274_MODE_BINNING_OFF,
> +	IMX274_MODE_BINNING_2_1,
> +	IMX274_MODE_BINNING_3_1,
>  };
>  
>  /*
> @@ -152,8 +168,8 @@ enum imx274_mode {
>   * These are the values to configure the sensor in one of the
>   * implemented modes.
>   *
> - * @size: recommended recording pixels
>   * @init_regs: registers to initialize the mode
> + * @bin_ratio: downscale factor (e.g. 3 for 3:1 binning)
>   * @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
> @@ -163,8 +179,8 @@ enum imx274_mode {
>   *           in Each Readout Drive Mode (CSI-2)" in the datasheet)
>   */
>  struct imx274_frmfmt {
> -	struct v4l2_frmsize_discrete size;
>  	const struct reg_8 *init_regs;
> +	unsigned int bin_ratio;
>  	int min_frame_len;
>  	int min_SHR;
>  	int max_fps;
> @@ -215,31 +231,14 @@ static const struct reg_8 imx274_mode1_3840x2160_raw10[] = {
>  	{0x3004, 0x01},
>  	{0x3005, 0x01},
>  	{0x3006, 0x00},
> -	{0x3007, 0x02},
> +	{0x3007, 0xa2},
>  
>  	{0x3018, 0xA2}, /* output XVS, HVS */
>  
>  	{0x306B, 0x05},
>  	{0x30E2, 0x01},
> -	{0x30F6, 0x07}, /* HMAX, 263 */
> -	{0x30F7, 0x01}, /* HMAX */
> -
> -	{0x30dd, 0x01}, /* crop to 2160 */
> -	{0x30de, 0x06},
> -	{0x30df, 0x00},
> -	{0x30e0, 0x12},
> -	{0x30e1, 0x00},
> -	{0x3037, 0x01}, /* to crop to 3840 */
> -	{0x3038, 0x0c},
> -	{0x3039, 0x00},
> -	{0x303a, 0x0c},
> -	{0x303b, 0x0f},
>  
>  	{0x30EE, 0x01},
> -	{0x3130, 0x86},
> -	{0x3131, 0x08},
> -	{0x3132, 0x7E},
> -	{0x3133, 0x08},
>  	{0x3342, 0x0A},
>  	{0x3343, 0x00},
>  	{0x3344, 0x16},
> @@ -273,32 +272,14 @@ static const struct reg_8 imx274_mode3_1920x1080_raw10[] = {
>  	{0x3004, 0x02},
>  	{0x3005, 0x21},
>  	{0x3006, 0x00},
> -	{0x3007, 0x11},
> +	{0x3007, 0xb1},
>  
>  	{0x3018, 0xA2}, /* output XVS, HVS */
>  
>  	{0x306B, 0x05},
>  	{0x30E2, 0x02},
>  
> -	{0x30F6, 0x04}, /* HMAX, 260 */
> -	{0x30F7, 0x01}, /* HMAX */
> -
> -	{0x30dd, 0x01}, /* to crop to 1920x1080 */
> -	{0x30de, 0x05},
> -	{0x30df, 0x00},
> -	{0x30e0, 0x04},
> -	{0x30e1, 0x00},
> -	{0x3037, 0x01},
> -	{0x3038, 0x0c},
> -	{0x3039, 0x00},
> -	{0x303a, 0x0c},
> -	{0x303b, 0x0f},
> -
>  	{0x30EE, 0x01},
> -	{0x3130, 0x4E},
> -	{0x3131, 0x04},
> -	{0x3132, 0x46},
> -	{0x3133, 0x04},
>  	{0x3342, 0x0A},
>  	{0x3343, 0x00},
>  	{0x3344, 0x1A},
> @@ -331,31 +312,14 @@ static const struct reg_8 imx274_mode5_1280x720_raw10[] = {
>  	{0x3004, 0x03},
>  	{0x3005, 0x31},
>  	{0x3006, 0x00},
> -	{0x3007, 0x09},
> +	{0x3007, 0xa9},
>  
>  	{0x3018, 0xA2}, /* output XVS, HVS */
>  
>  	{0x306B, 0x05},
>  	{0x30E2, 0x03},
>  
> -	{0x30F6, 0x04}, /* HMAX, 260 */
> -	{0x30F7, 0x01}, /* HMAX */
> -
> -	{0x30DD, 0x01},
> -	{0x30DE, 0x07},
> -	{0x30DF, 0x00},
> -	{0x40E0, 0x04},
> -	{0x30E1, 0x00},
> -	{0x3030, 0xD4},
> -	{0x3031, 0x02},
> -	{0x3032, 0xD0},
> -	{0x3033, 0x02},
> -
>  	{0x30EE, 0x01},
> -	{0x3130, 0xE2},
> -	{0x3131, 0x02},
> -	{0x3132, 0xDE},
> -	{0x3133, 0x02},
>  	{0x3342, 0x0A},
>  	{0x3343, 0x00},
>  	{0x3344, 0x1B},
> @@ -498,7 +462,7 @@ static const struct reg_8 imx274_tp_regs[] = {
>  static const struct imx274_frmfmt imx274_formats[] = {
>  	{
>  		/* mode 1, 4K */
> -		.size = {3840, 2160},
> +		.bin_ratio = 1,
>  		.init_regs = imx274_mode1_3840x2160_raw10,
>  		.min_frame_len = 4550,
>  		.min_SHR = 12,
> @@ -507,7 +471,7 @@ static const struct imx274_frmfmt imx274_formats[] = {
>  	},
>  	{
>  		/* mode 3, 1080p */
> -		.size = {1920, 1080},
> +		.bin_ratio = 2,
>  		.init_regs = imx274_mode3_1920x1080_raw10,
>  		.min_frame_len = 2310,
>  		.min_SHR = 8,
> @@ -516,7 +480,7 @@ static const struct imx274_frmfmt imx274_formats[] = {
>  	},
>  	{
>  		/* mode 5, 720p */
> -		.size = {1280, 720},
> +		.bin_ratio = 3,
>  		.init_regs = imx274_mode5_1280x720_raw10,
>  		.min_frame_len = 2310,
>  		.min_SHR = 8,
> @@ -547,7 +511,10 @@ struct imx274_ctrls {
>   * @pad: Media pad structure
>   * @client: Pointer to I2C client
>   * @ctrls: imx274 control structure
> + * @crop: rect to be captured
> + * @compose: compose rect, i.e. output resolution
>   * @format: V4L2 media bus frame format structure
> + *          (width and height are in sync with the compose rect)
>   * @frame_rate: V4L2 frame rate structure
>   * @regmap: Pointer to regmap structure
>   * @reset_gpio: Pointer to reset gpio
> @@ -559,6 +526,8 @@ struct stimx274 {
>  	struct media_pad pad;
>  	struct i2c_client *client;
>  	struct imx274_ctrls ctrls;
> +	struct v4l2_rect crop;
> +	struct v4l2_rect compose;
>  	struct v4l2_mbus_framefmt format;
>  	struct v4l2_fract frame_interval;
>  	struct regmap *regmap;
> @@ -864,6 +833,80 @@ static int imx274_s_ctrl(struct v4l2_ctrl *ctrl)
>  }
>  
>  /**
> + * Helper function to change binning and set both compose and format.
> + *
> + * We have two entry points to change binning: set_fmt and
> + * set_selection(COMPOSE). Both have to compute the new output size
> + * and set it in both the compose rect and the frame format size. We
> + * also need to do the same things after setting cropping to restore
> + * 1:1 binning.
> + *
> + * This function contains the common code for these three cases, it
> + * has many arguments in order to accomodate the needs of all of them.
> + *
> + * Must be called with imx274->lock locked.
> + *
> + * @imx274 The device object

Colon after the name of the argument. Same below.

> + * @cfg    The pad config we are editing for TRY requests
> + * @which  V4L2_SUBDEV_FORMAT_ACTIVE or V4L2_SUBDEV_FORMAT_TRY from the caller
> + * @width  Input-output parameter: set to the desired width before
> + *         the call, contains the chosen value after returning successfully
> + * @height Input-output parameter for height (see @width)
> + */
> +static int __imx274_change_compose(struct stimx274 *imx274,
> +				   struct v4l2_subdev_pad_config *cfg,
> +				   u32 which,
> +				   u32 *width,
> +				   u32 *height)
> +{
> +	struct device *dev = &imx274->client->dev;
> +	const struct v4l2_rect *cur_crop;
> +	struct v4l2_mbus_framefmt *tgt_fmt;
> +	struct v4l2_rect *tgt_compose;
> +	unsigned int ratio; /* Binning ratio */
> +
> +	dev_dbg(dev, "%s: request of  %dx%d (%s)\n",
> +		__func__, *width, *height,
> +		(which == V4L2_SUBDEV_FORMAT_ACTIVE) ? "ACTIVE" : "TRY");

If these's a need for such debug prints, I think they'd be better added to
the framework than individual drivers.

> +
> +	if (which == V4L2_SUBDEV_FORMAT_TRY) {
> +		cur_crop = &cfg->try_crop;
> +		tgt_compose = &cfg->try_compose;
> +		tgt_fmt = &cfg->try_fmt;
> +	} else {
> +		cur_crop = &imx274->crop;
> +		tgt_compose = &imx274->compose;
> +		tgt_fmt = &imx274->format;
> +	}
> +
> +	/* Find ratio (maximize output resolution). Fallback to 1:1. */
> +	for (ratio = 3; ratio > 1; ratio--)
> +		if (*width <= DIV_ROUND_UP(cur_crop->width, ratio) &&
> +		    *height <= DIV_ROUND_UP(cur_crop->height, ratio))
> +			break;

There are flags to direct the rounding behaviour. The default is the
nearest:

<URL:https://hverkuil.home.xs4all.nl/spec/uapi/v4l/v4l2-selection-flags.html>

The smiapp driver (as the only sensor driver) supports them.

> +
> +	*width = cur_crop->width / ratio;
> +	*height = cur_crop->height / ratio;
> +
> +	if (which == V4L2_SUBDEV_FORMAT_ACTIVE)
> +		imx274->mode = &imx274_formats[ratio - 1];
> +
> +	dev_dbg(dev, "%s: adjusted to %dx%d (%d:1 binning)\n",
> +		__func__, *width, *height, ratio);

Could be here, for binning. %d -> %u --- it's unsigned.

> +
> +	tgt_compose->top = 0;
> +	tgt_compose->left = 0;
> +	tgt_compose->width = *width;
> +	tgt_compose->height = *height;
> +
> +	tgt_fmt->width = *width;
> +	tgt_fmt->height = *height;
> +	tgt_fmt->field = V4L2_FIELD_NONE;
> +
> +	return 0;
> +}
> +
> +/**
>   * imx274_get_fmt - Get the pad format
>   * @sd: Pointer to V4L2 Sub device structure
>   * @cfg: Pointer to sub device pad information structure
> @@ -901,45 +944,228 @@ static int imx274_set_fmt(struct v4l2_subdev *sd,
>  {
>  	struct v4l2_mbus_framefmt *fmt = &format->format;
>  	struct stimx274 *imx274 = to_imx274(sd);
> -	struct i2c_client *client = imx274->client;
> -	int index;
> -
> -	dev_dbg(&client->dev,
> -		"%s: width = %d height = %d code = %d\n",
> -		__func__, fmt->width, fmt->height, fmt->code);
> +	int err = 0;
>  
>  	mutex_lock(&imx274->lock);
>  
> -	for (index = 0; index < ARRAY_SIZE(imx274_formats); index++) {
> -		if (imx274_formats[index].size.width == fmt->width &&
> -		    imx274_formats[index].size.height == fmt->height)
> -			break;
> -	}
> -
> -	if (index >= ARRAY_SIZE(imx274_formats)) {
> -		/* default to first format */
> -		index = 0;
> -	}
> +	err = __imx274_change_compose(imx274, cfg, format->which,
> +				      &fmt->width, &fmt->height);
>  
> -	imx274->mode = &imx274_formats[index];
> +	if (err)
> +		goto out;
>  
> -	if (fmt->width > IMX274_MAX_WIDTH)
> -		fmt->width = IMX274_MAX_WIDTH;
> -	if (fmt->height > IMX274_MAX_HEIGHT)
> -		fmt->height = IMX274_MAX_HEIGHT;
> -	fmt->width = fmt->width & (~IMX274_MASK_LSB_2_BITS);
> -	fmt->height = fmt->height & (~IMX274_MASK_LSB_2_BITS);
> +	/*
> +	 * __imx274_change_compose already set width and height in the
> +	 * applicable format, but we need to keep all other format
> +	 * values, so do a full copy here
> +	 */
>  	fmt->field = V4L2_FIELD_NONE;
> -
>  	if (format->which == V4L2_SUBDEV_FORMAT_TRY)
>  		cfg->try_fmt = *fmt;
>  	else
>  		imx274->format = *fmt;
>  
> +out:
>  	mutex_unlock(&imx274->lock);
> +
> +	return err;
> +}
> +
> +static int imx274_get_selection(struct v4l2_subdev *sd,
> +				struct v4l2_subdev_pad_config *cfg,
> +				struct v4l2_subdev_selection *sel)
> +{
> +	struct stimx274 *imx274 = to_imx274(sd);
> +	const struct v4l2_rect *src_crop;
> +	const struct v4l2_rect *src_compose;
> +	int ret = 0;
> +
> +	if (sel->pad != 0)
> +		return -EINVAL;
> +
> +	if (sel->target == V4L2_SEL_TGT_CROP_BOUNDS) {
> +		sel->r.left = 0;
> +		sel->r.top = 0;
> +		sel->r.width = IMX274_MAX_WIDTH;
> +		sel->r.height = IMX274_MAX_HEIGHT;
> +		return 0;
> +	}
> +
> +	if (sel->which == V4L2_SUBDEV_FORMAT_TRY) {
> +		src_crop = &cfg->try_crop;
> +		src_compose = &cfg->try_compose;
> +	} else {
> +		src_crop = &imx274->crop;
> +		src_compose = &imx274->compose;
> +	}
> +
> +	mutex_lock(&imx274->lock);
> +
> +	switch (sel->target) {
> +	case V4L2_SEL_TGT_CROP:
> +		sel->r = *src_crop;
> +		break;
> +	case V4L2_SEL_TGT_COMPOSE_BOUNDS:
> +		sel->r.top = 0;
> +		sel->r.left = 0;
> +		sel->r.width = src_crop->width;
> +		sel->r.height = src_crop->height;
> +		break;
> +	case V4L2_SEL_TGT_COMPOSE:
> +		sel->r = *src_compose;
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +
> +	mutex_unlock(&imx274->lock);
> +
> +	return ret;
> +}
> +
> +static int imx274_set_selection_crop(struct stimx274 *imx274,
> +				     struct v4l2_subdev_pad_config *cfg,
> +				     struct v4l2_subdev_selection *sel)
> +{
> +	struct device *dev = &imx274->client->dev;
> +	struct v4l2_rect *tgt_crop;
> +	struct v4l2_rect new_crop;
> +	bool size_changed;
> +
> +	/*
> +	 * h_step could be 12 or 24 depending on the binning. But we
> +	 * won't know the binning until we choose the mode later in
> +	 * imx274_set_fmt(). Thus let's be safe and use the most
> +	 * conservative value in all cases.
> +	 */
> +	const int h_step = 24;
> +
> +	dev_dbg(dev, "%s: request of  (%d,%d)%dx%d (%s)\n", __func__,
> +		sel->r.left, sel->r.top, sel->r.width, sel->r.height,
> +		V4L2_SUBDEV_FORMAT_ACTIVE ? "ACTIVE" : "TRY");
> +
> +	new_crop.width = rounddown(min_t(u32, sel->r.width, IMX274_MAX_WIDTH),
> +				   h_step);
> +
> +	/* Constraint: HTRIMMING_END - HTRIMMING_START >= 144 */
> +	if (new_crop.width < 144)
> +		new_crop.width = 144;
> +
> +	new_crop.left = min_t(u32, roundup(sel->r.left, h_step),
> +			      IMX274_MAX_WIDTH - new_crop.width);
> +
> +	new_crop.height =
> +		ALIGN(min_t(u32, sel->r.height, IMX274_MAX_HEIGHT), 2);
> +
> +	new_crop.top = min_t(u32, ALIGN(sel->r.top, 2),
> +			     IMX274_MAX_HEIGHT - new_crop.height);
> +
> +	dev_dbg(dev, "%s: adjusted to (%d,%d)%dx%d", __func__,
> +		new_crop.left, new_crop.top, new_crop.width, new_crop.height);
> +
> +	sel->r = new_crop;
> +
> +	if (sel->which == V4L2_SUBDEV_FORMAT_TRY)
> +		tgt_crop = &cfg->try_crop;
> +	else
> +		tgt_crop = &imx274->crop;
> +
> +	mutex_lock(&imx274->lock);
> +
> +	size_changed = (new_crop.width != tgt_crop->width ||
> +			new_crop.height != tgt_crop->height);
> +
> +	/* __imx274_change_compose needs the new size in *tgt_crop */
> +	*tgt_crop = new_crop;
> +
> +	/* if crop size changed then reset the output image size */
> +	if (size_changed)
> +		__imx274_change_compose(imx274, cfg, sel->which,
> +					&new_crop.width, &new_crop.height);
> +
> +	mutex_unlock(&imx274->lock);
> +
>  	return 0;
>  }
>  
> +static int imx274_set_selection(struct v4l2_subdev *sd,
> +				struct v4l2_subdev_pad_config *cfg,
> +				struct v4l2_subdev_selection *sel)
> +{
> +	struct stimx274 *imx274 = to_imx274(sd);
> +
> +	if (sel->pad != 0)
> +		return -EINVAL;
> +
> +	if (sel->target == V4L2_SEL_TGT_CROP)
> +		return imx274_set_selection_crop(imx274, cfg, sel);
> +
> +	if (sel->target == V4L2_SEL_TGT_COMPOSE) {
> +		int err;
> +
> +		mutex_lock(&imx274->lock);
> +
> +		err =  __imx274_change_compose(imx274, cfg, sel->which,
> +					       &sel->r.width, &sel->r.height);
> +		/*
> +		 * __imx274_change_compose already set width and
> +		 * height in set->r, we still need to set top-left
> +		 */
> +		if (!err) {
> +			sel->r.top = 0;
> +			sel->r.left = 0;
> +		}
> +
> +		mutex_unlock(&imx274->lock);
> +
> +		return err;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int imx274_apply_trimming(struct stimx274 *imx274)
> +{
> +	u32 h_start;
> +	u32 h_end;
> +	u32 hmax;
> +	u32 v_cut;
> +	s32 v_pos;
> +	u32 write_v_size;
> +	u32 y_out_size;
> +	struct reg_8 regs[17];
> +
> +	h_start = imx274->crop.left + 12;
> +	h_end = h_start + imx274->crop.width;
> +
> +	/* Use the minimum allowed value of HMAX */
> +	/* Note: except in mode 1, (width / 16 + 23) is always < hmax_min */
> +	/* Note: 260 is the minimum HMAX in all implemented modes */
> +	hmax = max_t(u32, 260, (imx274->crop.width) / 16 + 23);
> +
> +	/* invert v_pos if VFLIP */
> +	v_pos = imx274->ctrls.vflip->cur.val ?
> +		(-imx274->crop.top / 2) : (imx274->crop.top / 2);
> +	v_cut = (IMX274_MAX_HEIGHT - imx274->crop.height) / 2;
> +	write_v_size = imx274->crop.height + 22;
> +	y_out_size   = imx274->crop.height + 14;
> +
> +	prepare_reg(&regs[0],  IMX274_HMAX_REG_LSB,        hmax,         16);
> +	prepare_reg(&regs[2],  IMX274_HTRIM_EN_REG,        1,             1);
> +	prepare_reg(&regs[3],  IMX274_HTRIM_START_REG_LSB, h_start,      13);
> +	prepare_reg(&regs[5],  IMX274_HTRIM_END_REG_LSB,   h_end,        13);
> +
> +	prepare_reg(&regs[7],  IMX274_VWIDCUTEN_REG,       1,             1);
> +	prepare_reg(&regs[8],  IMX274_VWIDCUT_REG_LSB,     v_cut,        11);
> +	prepare_reg(&regs[10], IMX274_VWINPOS_REG_LSB,     v_pos,        12);
> +	prepare_reg(&regs[12], IMX274_WRITE_VSIZE_REG_LSB, write_v_size, 13);
> +	prepare_reg(&regs[14], IMX274_Y_OUT_SIZE_REG_LSB,  y_out_size,   13);
> +
> +	regs[16].addr = IMX274_TABLE_END;
> +
> +	return imx274_write_table(imx274, regs);
> +}
> +
>  /**
>   * imx274_g_frame_interval - Get the frame interval
>   * @sd: Pointer to V4L2 Sub device structure
> @@ -1080,6 +1306,10 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on)
>  		if (ret)
>  			goto fail;
>  
> +		ret = imx274_apply_trimming(imx274);
> +		if (ret)
> +			goto fail;
> +
>  		/*
>  		 * update frame rate & expsoure. if the last mode is different,
>  		 * HMAX could be changed. As the result, frame rate & exposure
> @@ -1589,6 +1819,8 @@ static int imx274_set_frame_interval(struct stimx274 *priv,
>  static const struct v4l2_subdev_pad_ops imx274_pad_ops = {
>  	.get_fmt = imx274_get_fmt,
>  	.set_fmt = imx274_set_fmt,
> +	.get_selection = imx274_get_selection,
> +	.set_selection = imx274_set_selection,
>  };
>  
>  static const struct v4l2_subdev_video_ops imx274_video_ops = {
> @@ -1634,8 +1866,12 @@ static int imx274_probe(struct i2c_client *client,
>  
>  	/* initialize format */
>  	imx274->mode = &imx274_formats[IMX274_DEFAULT_MODE];
> -	imx274->format.width = imx274->mode->size.width;
> -	imx274->format.height = imx274->mode->size.height;
> +	imx274->crop.width = IMX274_MAX_WIDTH;
> +	imx274->crop.height = IMX274_MAX_HEIGHT;
> +	imx274->compose.width = imx274->crop.width / imx274->mode->bin_ratio;
> +	imx274->compose.height = imx274->crop.height / imx274->mode->bin_ratio;
> +	imx274->format.width = imx274->compose.width;
> +	imx274->format.height = imx274->compose.height;
>  	imx274->format.field = V4L2_FIELD_NONE;
>  	imx274->format.code = MEDIA_BUS_FMT_SRGGB10_1X10;
>  	imx274->format.colorspace = V4L2_COLORSPACE_SRGB;
> -- 
> 2.7.4
> 

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v4 8/8] media: imx274: add SELECTION support for cropping

[Luca Ceresoli]

Hi Sakari,

thanks for the review.

On 29/06/2018 10:04, Sakari Ailus wrote:
> On Mon, Jun 11, 2018 at 01:35:39PM +0200, Luca Ceresoli wrote:
>> Currently this driver does not support cropping. The supported modes
>> are the following, all capturing the entire area:
>>
>>  - 3840x2160, 1:1 binning (native sensor resolution)
>>  - 1920x1080, 2:1 binning
>>  - 1280x720,  3:1 binning
>>
>> The VIDIOC_SUBDEV_S_FMT ioctl chooses among these 3 configurations the
>> one that matches the requested format.
>>
>> Add cropping support via VIDIOC_SUBDEV_S_SELECTION: with target
>> V4L2_SEL_TGT_CROP to choose the captured area, with
>> V4L2_SEL_TGT_COMPOSE to choose the output resolution.
>>
>> To maintain backward compatibility we also allow setting the compose
>> format via VIDIOC_SUBDEV_S_FMT. To obtain this, compose rect and
>> output format are computed in the common helper function
>> __imx274_change_compose(), which sets both to the same width/height
>> values.
>>
>> Cropping also calls __imx274_change_compose() whenever cropping rect
>> size changes in order to reset the compose rect (and output format
>> size) for 1:1 binning.
>>
>> Also change the names in enum imx274_mode from being resolution-based
>> to being binning-based (e.g. from IMX274_MODE_1920X1080 to
>> IMX274_MODE_BINNING_2_1). Without cropping, the two naming are
>> equivalent. With cropping, the resolution could be different,
>> e.g. using 2:1 binning mode to crop 1200x960 and output a 600x480
>> format. Using binning in the names avoids any misunderstanding.  For
>> the same reason, replace the 'size' field in struct imx274_frmfmt with
>> 'bin_ratio'.
>>
>> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
>> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
>>
>> ---
>> Changed v3 -> v4:
>>  - Set the binning via the SELECTION API (COMPOSE rect), but still
>>    allow using VIDIOC_SUBDEV_S_FMT for backward compatibility.
>>  - rename imx274_set_trimming -> imx274_apply_trimming for clarity
>>
>> Changed v2 -> v3:
>>  - Remove hard-coded HMAX reg setting from all modes, not only the
>>    first one. HMAX is always computed and set in s_stream now.
>>  - Reword commit log message.
>>
>> Changed v1 -> v2:
>>  - add "media: " prefix to commit message
>> ---
>>  drivers/media/i2c/imx274.c | 416 +++++++++++++++++++++++++++++++++++----------
>>  1 file changed, 326 insertions(+), 90 deletions(-)
>>
>> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
>> index 2c13961e9764..8bfc20a46b3d 100644
>> --- a/drivers/media/i2c/imx274.c
>> +++ b/drivers/media/i2c/imx274.c
>> @@ -5,6 +5,7 @@
>>   *
>>   * Leon Luo <leonl@leopardimaging.com>
>>   * Edwin Zou <edwinz@leopardimaging.com>
>> + * Luca Ceresoli <luca@lucaceresoli.net>
>>   *
>>   * This program is free software; you can redistribute it and/or modify it
>>   * under the terms and conditions of the GNU General Public License,
>> @@ -19,6 +20,7 @@
>>   * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>   */
>>  
>> +#include <linux/kernel.h>
> 
> Alphabetical order, please.

Ok.

>> @@ -141,9 +157,9 @@ static const struct regmap_config imx274_regmap_config = {
>>  };
>>  
>>  enum imx274_mode {
> 
> Is this still an appropriate name? imx274_binning?

Yes, that would be coherent with the renaming of the individual fields.

>> -	IMX274_MODE_3840X2160,
>> -	IMX274_MODE_1920X1080,
>> -	IMX274_MODE_1280X720,
>> +	IMX274_MODE_BINNING_OFF,
>> +	IMX274_MODE_BINNING_2_1,
>> +	IMX274_MODE_BINNING_3_1,
>>  };
>>  
>>  /*
[...]
>> @@ -559,6 +526,8 @@ struct stimx274 {
>>  	struct media_pad pad;
>>  	struct i2c_client *client;
>>  	struct imx274_ctrls ctrls;
>> +	struct v4l2_rect crop;
>> +	struct v4l2_rect compose;
>>  	struct v4l2_mbus_framefmt format;

I noticed the v4l2_rect compose can be removed. In this patch I keep
compose.width (and height) always equal to format.width (height). The
compose width (height) can be read from format.width (height). And since
compose.top and .left are always zero, compose carries no useful info.
Removing .compose will get less code to keep them in sync and no risk
for bugs that get them out of sync.

Note this is an implementation detail. No changes at the API level.

>> @@ -864,6 +833,80 @@ static int imx274_s_ctrl(struct v4l2_ctrl *ctrl)
>>  }
>>  
>>  /**
>> + * Helper function to change binning and set both compose and format.
>> + *
>> + * We have two entry points to change binning: set_fmt and
>> + * set_selection(COMPOSE). Both have to compute the new output size
>> + * and set it in both the compose rect and the frame format size. We
>> + * also need to do the same things after setting cropping to restore
>> + * 1:1 binning.
>> + *
>> + * This function contains the common code for these three cases, it
>> + * has many arguments in order to accomodate the needs of all of them.
>> + *
>> + * Must be called with imx274->lock locked.
>> + *
>> + * @imx274 The device object
> 
> Colon after the name of the argument. Same below.

Ouch. Thanks.

>> + * @cfg    The pad config we are editing for TRY requests
>> + * @which  V4L2_SUBDEV_FORMAT_ACTIVE or V4L2_SUBDEV_FORMAT_TRY from the caller
>> + * @width  Input-output parameter: set to the desired width before
>> + *         the call, contains the chosen value after returning successfully
>> + * @height Input-output parameter for height (see @width)
>> + */
>> +static int __imx274_change_compose(struct stimx274 *imx274,
>> +				   struct v4l2_subdev_pad_config *cfg,
>> +				   u32 which,
>> +				   u32 *width,
>> +				   u32 *height)
>> +{
>> +	struct device *dev = &imx274->client->dev;
>> +	const struct v4l2_rect *cur_crop;
>> +	struct v4l2_mbus_framefmt *tgt_fmt;
>> +	struct v4l2_rect *tgt_compose;
>> +	unsigned int ratio; /* Binning ratio */
>> +
>> +	dev_dbg(dev, "%s: request of  %dx%d (%s)\n",
>> +		__func__, *width, *height,
>> +		(which == V4L2_SUBDEV_FORMAT_ACTIVE) ? "ACTIVE" : "TRY");
> 
> If these's a need for such debug prints, I think they'd be better added to
> the framework than individual drivers.

Agreed, let's get rid of them.

>> +
>> +	if (which == V4L2_SUBDEV_FORMAT_TRY) {
>> +		cur_crop = &cfg->try_crop;
>> +		tgt_compose = &cfg->try_compose;
>> +		tgt_fmt = &cfg->try_fmt;
>> +	} else {
>> +		cur_crop = &imx274->crop;
>> +		tgt_compose = &imx274->compose;
>> +		tgt_fmt = &imx274->format;
>> +	}
>> +
>> +	/* Find ratio (maximize output resolution). Fallback to 1:1. */
>> +	for (ratio = 3; ratio > 1; ratio--)
>> +		if (*width <= DIV_ROUND_UP(cur_crop->width, ratio) &&
>> +		    *height <= DIV_ROUND_UP(cur_crop->height, ratio))
>> +			break;
> 
> There are flags to direct the rounding behaviour. The default is the
> nearest:
> 
> <URL:https://hverkuil.home.xs4all.nl/spec/uapi/v4l/v4l2-selection-flags.html>
> 
> The smiapp driver (as the only sensor driver) supports them.

Right, I will implement that.

>> +
>> +	*width = cur_crop->width / ratio;
>> +	*height = cur_crop->height / ratio;
>> +
>> +	if (which == V4L2_SUBDEV_FORMAT_ACTIVE)
>> +		imx274->mode = &imx274_formats[ratio - 1];
>> +
>> +	dev_dbg(dev, "%s: adjusted to %dx%d (%d:1 binning)\n",
>> +		__func__, *width, *height, ratio);
> 
> Could be here, for binning. %d -> %u --- it's unsigned.
Ok, in the nxt version this will look like:

    dev_dbg(dev, "%s: selected %u:1 binning\n", __func__, ratio);

Bye,
-- 
Luca

Re: [PATCH v4 0/8] media: imx274: cleanups, improvements and SELECTION API support

[Sakari Ailus]

Hi Luca,

On Mon, Jun 11, 2018 at 01:35:31PM +0200, Luca Ceresoli wrote:
> Hi,
> 
> this patchset introduces cropping support for the Sony IMX274 sensor
> using the SELECTION API.
> 
> With respect to v3, this version uses the SELECTION API with taget
> V4L2_SEL_TGT_COMPOSE to change the output resolution. This is the
> recommended API for cropping + downscaling. However for backward
> compatibility the set_format callback is still supported and is
> equivalent to setting the compose rect as far as resolutions are
> concerned.
> 
> Patches 1-5 are overall improvements and restructuring, mostly useful
> to implement the SELECTION API in a clean way.
> 
> Patch 6 introduces a helper to allow setting many registers computed
> at runtime in a straightforward way. This would not have been very
> useful before because all long register write sequences came from
> const tables, but it's definitely a must for the cropping code where
> several register values are computed at runtime.
> 
> Patch 7 is new in this series, it's a trivial typo fix that can be
> applied independently.
> 
> Patch 8 implements the set_selection pad operation for cropping
> (V4L2_SEL_TGT_CROP) and binning (V4L2_SEL_TGT_COMPOSE). The most
> tricky part was respecting all the device constraints on the
> horizontal crop.

My apologies for delays in reviewing the set. I'll take patches 1--5 and 7
and then I'll comment on patches 6 and 8 separately. Would that work for
you?

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCH v4 0/8] media: imx274: cleanups, improvements and SELECTION API support

[Luca Ceresoli]

Hi Sakari,

On 26/06/2018 14:19, Sakari Ailus wrote:
[...]
> My apologies for delays in reviewing the set. I'll take patches 1--5 and 7
> and then I'll comment on patches 6 and 8 separately. Would that work for
> you?

Sure.

I'll reply to your comment to patch 6 as soon as possible.

Bye,
-- 
Luca