[PATCH v2 00/13] media: imx274: cleanups, improvements and SELECTION API support

[PATCH v2 00/13] 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 v1 this patchset adds the "media: " prefix to commits
and fixes a warning in patch 9.

Patches 1-6 clean up and restructure code in various places and are
pretty much independent from the cropping feature.

Patches 7-11 are further restructuring which are mostly useful to
implement cropping API in a cleaner way.

Patch 12 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.

Patch 13 implements cropping in the set_selection pad operation. On
the v4l2 side there is nothing special. The most tricky part was
respecting all the device constraints on the horizontal crop.

Regards,
Luca

Luca Ceresoli (13):
  media: imx274: document reset delays more clearly
  media: imx274: fix typo in comment
  media: imx274: slightly simplify code
  media: imx274: remove unused data from struct imx274_frmfmt
  media: imx274: rename and reorder register address definitions
  media: imx274: remove non-indexed pointers from mode_table
  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: add SELECTION support for cropping

 drivers/media/i2c/imx274.c | 598 ++++++++++++++++++++++++++-------------------
 1 file changed, 348 insertions(+), 250 deletions(-)

-- 
2.7.4

[PATCH v2 01/13] media: imx274: document reset delays more clearly

[Luca Ceresoli]

Document the unit to avoid having to look through the code to compute it.
Also clarify that these are min and max values.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>

---
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 daec33f4196a..5e425db9204d 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -87,7 +87,7 @@
 #define IMX274_SHR_LIMIT_CONST			(4)
 
 /*
- * Constants for sensor reset delay
+ * Min and max sensor reset delay (microseconds)
  */
 #define IMX274_RESET_DELAY1			(2000)
 #define IMX274_RESET_DELAY2			(2200)
-- 
2.7.4

[PATCH v2 02/13] media: imx274: fix typo in comment

[Luca Ceresoli]

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>

---
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 5e425db9204d..dfd04edcdd48 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -971,7 +971,7 @@ static int imx274_s_frame_interval(struct v4l2_subdev *sd,
 	if (!ret) {
 		/*
 		 * exposure time range is decided by frame interval
-		 * need to update it after frame interal changes
+		 * need to update it after frame interval changes
 		 */
 		min = IMX274_MIN_EXPOSURE_TIME;
 		max = fi->interval.numerator * 1000000
-- 
2.7.4

[PATCH v2 03/13] media: imx274: slightly simplify code

[Luca Ceresoli]

imx274_s_frame_interval() already has a direct pointer to the v4l2
exposure control, so reuse it to simplify code.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>

---
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 dfd04edcdd48..c5d00ade4d64 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -984,7 +984,7 @@ static int imx274_s_frame_interval(struct v4l2_subdev *sd,
 		}
 
 		/* update exposure time accordingly */
-		imx274_set_exposure(imx274, imx274->ctrls.exposure->val);
+		imx274_set_exposure(imx274, ctrl->val);
 
 		dev_dbg(&imx274->client->dev, "set frame interval to %uus\n",
 			fi->interval.numerator * 1000000
-- 
2.7.4

[PATCH v2 04/13] media: imx274: remove unused data from struct imx274_frmfmt

[Luca Ceresoli]

struct imx274_frmfmt is instantiated only in the imx274_formats[]
array, where imx274_formats[N].mode always equals N (via enum
imx274_mode).  So .mode carries no information, and unsurprisingly it
is never used.

mbus_code is never used because the 12 bit modes are not implemented.

The colorspace member is also never used, which is normal since the
imx274 sensor can output only one colorspace.

Let's get rid of all of them.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>

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

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index c5d00ade4d64..9203d377cfe2 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -156,10 +156,7 @@ enum imx274_mode {
  * imx274 format related structure
  */
 struct imx274_frmfmt {
-	u32 mbus_code;
-	enum v4l2_colorspace colorspace;
 	struct v4l2_frmsize_discrete size;
-	enum imx274_mode mode;
 };
 
 /*
@@ -501,12 +498,9 @@ static const struct reg_8 *mode_table[] = {
  * imx274 format related structure
  */
 static const struct imx274_frmfmt imx274_formats[] = {
-	{MEDIA_BUS_FMT_SRGGB10_1X10, V4L2_COLORSPACE_SRGB, {3840, 2160},
-		IMX274_MODE_3840X2160},
-	{MEDIA_BUS_FMT_SRGGB10_1X10, V4L2_COLORSPACE_SRGB, {1920, 1080},
-		IMX274_MODE_1920X1080},
-	{MEDIA_BUS_FMT_SRGGB10_1X10, V4L2_COLORSPACE_SRGB, {1280, 720},
-		IMX274_MODE_1280X720},
+	{ {3840, 2160} },
+	{ {1920, 1080} },
+	{ {1280,  720} },
 };
 
 /*
@@ -890,9 +884,8 @@ static int imx274_set_fmt(struct v4l2_subdev *sd,
 	int index;
 
 	dev_dbg(&client->dev,
-		"%s: width = %d height = %d code = %d mbus_code = %d\n",
-		__func__, fmt->width, fmt->height, fmt->code,
-		imx274_formats[imx274->mode_index].mbus_code);
+		"%s: width = %d height = %d code = %d\n",
+		__func__, fmt->width, fmt->height, fmt->code);
 
 	mutex_lock(&imx274->lock);
 
-- 
2.7.4

[PATCH v2 05/13] media: imx274: rename and reorder register address definitions

[Luca Ceresoli]

Most registers are defined using the name used in the datasheet.
E.g. the defines for the HMAX register are IMX274_HMAX_REG_*.

Rename the SHR and VMAX register accordingly. Also move them close to
related registers: SHR close to SVR, VMAX close to HMAX.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>

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

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 9203d377cfe2..62a0d7af0e51 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -107,15 +107,15 @@
 /*
  * IMX274 register definitions
  */
-#define IMX274_FRAME_LENGTH_ADDR_1		0x30FA /* VMAX, MSB */
-#define IMX274_FRAME_LENGTH_ADDR_2		0x30F9 /* VMAX */
-#define IMX274_FRAME_LENGTH_ADDR_3		0x30F8 /* VMAX, LSB */
+#define IMX274_SHR_REG_MSB			0x300D /* SHR */
+#define IMX274_SHR_REG_LSB			0x300C /* SHR */
 #define IMX274_SVR_REG_MSB			0x300F /* SVR */
 #define IMX274_SVR_REG_LSB			0x300E /* SVR */
+#define IMX274_VMAX_REG_1			0x30FA /* VMAX, MSB */
+#define IMX274_VMAX_REG_2			0x30F9 /* VMAX */
+#define IMX274_VMAX_REG_3			0x30F8 /* VMAX, LSB */
 #define IMX274_HMAX_REG_MSB			0x30F7 /* HMAX */
 #define IMX274_HMAX_REG_LSB			0x30F6 /* HMAX */
-#define IMX274_COARSE_TIME_ADDR_MSB		0x300D /* SHR */
-#define IMX274_COARSE_TIME_ADDR_LSB		0x300C /* SHR */
 #define IMX274_ANALOG_GAIN_ADDR_LSB		0x300A /* ANALOG GAIN LSB */
 #define IMX274_ANALOG_GAIN_ADDR_MSB		0x300B /* ANALOG GAIN MSB */
 #define IMX274_DIGITAL_GAIN_REG			0x3012 /* Digital Gain */
@@ -1126,15 +1126,15 @@ static int imx274_get_frame_length(struct stimx274 *priv, u32 *val)
 	svr = (reg_val[1] << IMX274_SHIFT_8_BITS) + reg_val[0];
 
 	/* vmax */
-	err = imx274_read_reg(priv, IMX274_FRAME_LENGTH_ADDR_3, &reg_val[0]);
+	err = imx274_read_reg(priv, IMX274_VMAX_REG_3, &reg_val[0]);
 	if (err)
 		goto fail;
 
-	err = imx274_read_reg(priv, IMX274_FRAME_LENGTH_ADDR_2, &reg_val[1]);
+	err = imx274_read_reg(priv, IMX274_VMAX_REG_2, &reg_val[1]);
 	if (err)
 		goto fail;
 
-	err = imx274_read_reg(priv, IMX274_FRAME_LENGTH_ADDR_1, &reg_val[2]);
+	err = imx274_read_reg(priv, IMX274_VMAX_REG_1, &reg_val[2]);
 	if (err)
 		goto fail;
 
@@ -1293,10 +1293,10 @@ static int imx274_set_gain(struct stimx274 *priv, struct v4l2_ctrl *ctrl)
 static inline void imx274_calculate_coarse_time_regs(struct reg_8 regs[2],
 						     u32 coarse_time)
 {
-	regs->addr = IMX274_COARSE_TIME_ADDR_MSB;
+	regs->addr = IMX274_SHR_REG_MSB;
 	regs->val = (coarse_time >> IMX274_SHIFT_8_BITS)
 			& IMX274_MASK_LSB_8_BITS;
-	(regs + 1)->addr = IMX274_COARSE_TIME_ADDR_LSB;
+	(regs + 1)->addr = IMX274_SHR_REG_LSB;
 	(regs + 1)->val = (coarse_time) & IMX274_MASK_LSB_8_BITS;
 }
 
@@ -1464,13 +1464,13 @@ static int imx274_set_test_pattern(struct stimx274 *priv, int val)
 static inline void imx274_calculate_frame_length_regs(struct reg_8 regs[3],
 						      u32 frame_length)
 {
-	regs->addr = IMX274_FRAME_LENGTH_ADDR_1;
+	regs->addr = IMX274_VMAX_REG_1;
 	regs->val = (frame_length >> IMX274_SHIFT_16_BITS)
 			& IMX274_MASK_LSB_4_BITS;
-	(regs + 1)->addr = IMX274_FRAME_LENGTH_ADDR_2;
+	(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_FRAME_LENGTH_ADDR_3;
+	(regs + 2)->addr = IMX274_VMAX_REG_3;
 	(regs + 2)->val = (frame_length) & IMX274_MASK_LSB_8_BITS;
 }
 
-- 
2.7.4

[PATCH v2 06/13] media: imx274: remove non-indexed pointers from mode_table

[Luca Ceresoli]

mode_table[] has 3 members that are accessed based on their index, which
makes worth using an array.

The other members are always accessed with a constant index. This added
indirection gives no improvement and only makes code more verbose.

Remove these pointers from the array and access them directly.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>

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

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 62a0d7af0e51..63fb94e7da37 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -144,12 +144,6 @@ enum imx274_mode {
 	IMX274_MODE_3840X2160,
 	IMX274_MODE_1920X1080,
 	IMX274_MODE_1280X720,
-
-	IMX274_MODE_START_STREAM_1,
-	IMX274_MODE_START_STREAM_2,
-	IMX274_MODE_START_STREAM_3,
-	IMX274_MODE_START_STREAM_4,
-	IMX274_MODE_STOP_STREAM
 };
 
 /*
@@ -486,12 +480,6 @@ 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_MODE_START_STREAM_1]	= imx274_start_1,
-	[IMX274_MODE_START_STREAM_2]	= imx274_start_2,
-	[IMX274_MODE_START_STREAM_3]	= imx274_start_3,
-	[IMX274_MODE_START_STREAM_4]	= imx274_start_4,
-	[IMX274_MODE_STOP_STREAM]	= imx274_stop,
 };
 
 /*
@@ -731,11 +719,11 @@ static int imx274_mode_regs(struct stimx274 *priv, int mode)
 {
 	int err = 0;
 
-	err = imx274_write_table(priv, mode_table[IMX274_MODE_START_STREAM_1]);
+	err = imx274_write_table(priv, imx274_start_1);
 	if (err)
 		return err;
 
-	err = imx274_write_table(priv, mode_table[IMX274_MODE_START_STREAM_2]);
+	err = imx274_write_table(priv, imx274_start_2);
 	if (err)
 		return err;
 
@@ -760,7 +748,7 @@ static int imx274_start_stream(struct stimx274 *priv)
 	 * give it 1 extra ms for margin
 	 */
 	msleep_range(11);
-	err = imx274_write_table(priv, mode_table[IMX274_MODE_START_STREAM_3]);
+	err = imx274_write_table(priv, imx274_start_3);
 	if (err)
 		return err;
 
@@ -770,7 +758,7 @@ static int imx274_start_stream(struct stimx274 *priv)
 	 * give it 1 extra ms for margin
 	 */
 	msleep_range(8);
-	err = imx274_write_table(priv, mode_table[IMX274_MODE_START_STREAM_4]);
+	err = imx274_write_table(priv, imx274_start_4);
 	if (err)
 		return err;
 
@@ -1081,8 +1069,7 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on)
 			goto fail;
 	} else {
 		/* stop stream */
-		ret = imx274_write_table(imx274,
-					 mode_table[IMX274_MODE_STOP_STREAM]);
+		ret = imx274_write_table(imx274, imx274_stop);
 		if (ret)
 			goto fail;
 	}
@@ -1779,7 +1766,7 @@ static int imx274_remove(struct i2c_client *client)
 	struct stimx274 *imx274 = to_imx274(sd);
 
 	/* stop stream */
-	imx274_write_table(imx274, mode_table[IMX274_MODE_STOP_STREAM]);
+	imx274_write_table(imx274, imx274_stop);
 
 	v4l2_async_unregister_subdev(sd);
 	v4l2_ctrl_handler_free(&imx274->ctrls.handler);
-- 
2.7.4

[PATCH v2 07/13] 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>

---
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 v2 08/13] 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>

---
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 v2 09/13] 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 a 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>

---
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 | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 2ec31ae4e60d..69fdc82d4214 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)
@@ -1029,7 +1025,8 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on)
 	int ret = 0;
 
 	dev_dbg(&imx274->client->dev, "%s : %s, mode index = %d\n", __func__,
-		on ? "Stream Start" : "Stream Stop", imx274->mode_index);
+		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 v2 10/13] 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>

---
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 69fdc82d4214..ae90fb31416c 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 v2 11/13] 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>

---
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 ae90fb31416c..bc307eb65a10 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 v2 12/13] 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>

---
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 bc307eb65a10..b6c54712f2aa 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

[PATCH v2 13/13] 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 set_fmt callback chooses among these 3 configurations the one that
matches the requested format.

Adding support to VIDIOC_SUBDEV_G_SELECTION and
VIDIOC_SUBDEV_S_SELECTION involved a complete rewrite of set_fmt,
which now chooses the most appropriate binning based on the ratio
between the previously-set cropping size and the format size being
requested.

Note that the names in enum imx274_mode change from being
resolution-based to being binning-based. 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 anny
misunderstanding.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>

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

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index b6c54712f2aa..ceb5b3e498c6 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -19,6 +19,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 +75,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 +112,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 +156,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,
 };
 
 /*
@@ -215,31 +230,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,7 +271,7 @@ static const struct reg_8 imx274_mode3_1920x1080_raw10[] = {
 	{0x3004, 0x02},
 	{0x3005, 0x21},
 	{0x3006, 0x00},
-	{0x3007, 0x11},
+	{0x3007, 0xb1},
 
 	{0x3018, 0xA2}, /* output XVS, HVS */
 
@@ -283,22 +281,7 @@ static const struct reg_8 imx274_mode3_1920x1080_raw10[] = {
 	{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,7 +314,7 @@ static const struct reg_8 imx274_mode5_1280x720_raw10[] = {
 	{0x3004, 0x03},
 	{0x3005, 0x31},
 	{0x3006, 0x00},
-	{0x3007, 0x09},
+	{0x3007, 0xa9},
 
 	{0x3018, 0xA2}, /* output XVS, HVS */
 
@@ -341,21 +324,7 @@ static const struct reg_8 imx274_mode5_1280x720_raw10[] = {
 	{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},
@@ -561,6 +530,7 @@ struct stimx274 {
 	struct imx274_ctrls ctrls;
 	struct v4l2_mbus_framefmt format;
 	struct v4l2_fract frame_interval;
+	struct v4l2_rect crop;
 	struct regmap *regmap;
 	struct gpio_desc *reset_gpio;
 	struct mutex lock; /* mutex lock for operations */
@@ -901,36 +871,30 @@ 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;
+	struct device *dev = &imx274->client->dev;
+	unsigned int ratio; /* Binning ratio */
 
-	dev_dbg(&client->dev,
-		"%s: width = %d height = %d code = %d\n",
-		__func__, fmt->width, fmt->height, fmt->code);
+	dev_dbg(dev, "%s: request of  %dx%d (%s)\n",
+		__func__, fmt->width, fmt->height,
+		V4L2_SUBDEV_FORMAT_ACTIVE ? "ACTIVE" : "TRY");
 
 	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)
+	/* Find ratio (maximize output resolution). Fallback to 1:1. */
+	for (ratio = 3; ratio > 1; ratio--)
+		if (fmt->width <= DIV_ROUND_UP(imx274->crop.width, ratio) &&
+		    fmt->height <= DIV_ROUND_UP(imx274->crop.height, ratio))
 			break;
-	}
-
-	if (index >= ARRAY_SIZE(imx274_formats)) {
-		/* default to first format */
-		index = 0;
-	}
 
-	imx274->mode = &imx274_formats[index];
+	imx274->mode = &imx274_formats[ratio - 1];
 
-	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);
+	fmt->width = imx274->crop.width / ratio;
+	fmt->height = imx274->crop.height / ratio;
 	fmt->field = V4L2_FIELD_NONE;
 
+	dev_dbg(dev, "%s: adjusted to %dx%d (%d:1 binning)\n",
+		__func__, fmt->width, fmt->height, ratio);
+
 	if (format->which == V4L2_SUBDEV_FORMAT_TRY)
 		cfg->try_fmt = *fmt;
 	else
@@ -940,6 +904,152 @@ static int imx274_set_fmt(struct v4l2_subdev *sd,
 	return 0;
 }
 
+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);
+
+	if (sel->pad != 0)
+		return -EINVAL;
+
+	switch (sel->target) {
+	case 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;
+	case V4L2_SEL_TGT_CROP:
+		if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
+			mutex_lock(&imx274->lock);
+			sel->r = imx274->crop;
+			mutex_unlock(&imx274->lock);
+		} else {
+			sel->r = cfg->try_crop;
+		}
+		return 0;
+	default:
+		return -EINVAL;
+	}
+
+	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);
+	struct device *dev = &imx274->client->dev;
+	struct v4l2_mbus_framefmt *tgt_fmt;
+	struct v4l2_rect *tgt_crop;
+	struct v4l2_rect new_crop;
+
+	/*
+	 * 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");
+
+	if (sel->target != V4L2_SEL_TGT_CROP || sel->pad != 0)
+		return -EINVAL;
+
+	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);
+
+	if (sel->which == V4L2_SUBDEV_FORMAT_TRY) {
+		tgt_crop = &cfg->try_crop;
+		tgt_fmt = &cfg->try_fmt;
+	} else {
+		tgt_crop = &imx274->crop;
+		tgt_fmt = &imx274->format;
+	}
+
+	mutex_lock(&imx274->lock);
+
+	if (new_crop.width != tgt_crop->width ||
+	    new_crop.height != tgt_crop->height) {
+		/* crop size changed, reset the output image size */
+		tgt_fmt->width = new_crop.width;
+		tgt_fmt->height = new_crop.height;
+		if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE)
+			imx274->mode = &imx274_formats[IMX274_MODE_BINNING_OFF];
+	}
+
+	*tgt_crop = new_crop;
+
+	mutex_unlock(&imx274->lock);
+
+	sel->r = new_crop;
+
+	return 0;
+}
+
+static int imx274_set_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 +1190,10 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on)
 		if (ret)
 			goto fail;
 
+		ret = imx274_set_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 +1703,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 = {
@@ -1641,6 +1757,8 @@ static int imx274_probe(struct i2c_client *client,
 	imx274->format.colorspace = V4L2_COLORSPACE_SRGB;
 	imx274->frame_interval.numerator = 1;
 	imx274->frame_interval.denominator = IMX274_DEF_FRAME_RATE;
+	imx274->crop.width = imx274->mode->size.width;
+	imx274->crop.height = imx274->mode->size.height;
 
 	/* initialize regmap */
 	imx274->regmap = devm_regmap_init_i2c(client, &imx274_regmap_config);
-- 
2.7.4

Re: [PATCH v2 13/13] media: imx274: add SELECTION support for cropping

[Sakari Ailus]

Hi Luca,

Thank you for the patchset.

Some comments below... what I propose is that I apply the rest of the
patches and then the comments to this one could be addressed separately.
Would that work for you?

On Tue, Apr 24, 2018 at 10:24:18AM +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 set_fmt callback chooses among these 3 configurations the one that
> matches the requested format.
> 
> Adding support to VIDIOC_SUBDEV_G_SELECTION and
> VIDIOC_SUBDEV_S_SELECTION involved a complete rewrite of set_fmt,
> which now chooses the most appropriate binning based on the ratio
> between the previously-set cropping size and the format size being
> requested.
> 
> Note that the names in enum imx274_mode change from being
> resolution-based to being binning-based. 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 anny
> misunderstanding.
> 
> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
> 
> ---
> Changed v1 -> v2:
>  - add "media: " prefix to commit message
> ---
>  drivers/media/i2c/imx274.c | 266 ++++++++++++++++++++++++++++++++-------------
>  1 file changed, 192 insertions(+), 74 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
> index b6c54712f2aa..ceb5b3e498c6 100644
> --- a/drivers/media/i2c/imx274.c
> +++ b/drivers/media/i2c/imx274.c
> @@ -19,6 +19,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 +75,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 +112,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 +156,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,
>  };
>  
>  /*
> @@ -215,31 +230,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,7 +271,7 @@ static const struct reg_8 imx274_mode3_1920x1080_raw10[] = {
>  	{0x3004, 0x02},
>  	{0x3005, 0x21},
>  	{0x3006, 0x00},
> -	{0x3007, 0x11},
> +	{0x3007, 0xb1},
>  
>  	{0x3018, 0xA2}, /* output XVS, HVS */
>  
> @@ -283,22 +281,7 @@ static const struct reg_8 imx274_mode3_1920x1080_raw10[] = {
>  	{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,7 +314,7 @@ static const struct reg_8 imx274_mode5_1280x720_raw10[] = {
>  	{0x3004, 0x03},
>  	{0x3005, 0x31},
>  	{0x3006, 0x00},
> -	{0x3007, 0x09},
> +	{0x3007, 0xa9},
>  
>  	{0x3018, 0xA2}, /* output XVS, HVS */
>  
> @@ -341,21 +324,7 @@ static const struct reg_8 imx274_mode5_1280x720_raw10[] = {
>  	{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},
> @@ -561,6 +530,7 @@ struct stimx274 {
>  	struct imx274_ctrls ctrls;
>  	struct v4l2_mbus_framefmt format;
>  	struct v4l2_fract frame_interval;
> +	struct v4l2_rect crop;
>  	struct regmap *regmap;
>  	struct gpio_desc *reset_gpio;
>  	struct mutex lock; /* mutex lock for operations */
> @@ -901,36 +871,30 @@ 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;
> +	struct device *dev = &imx274->client->dev;
> +	unsigned int ratio; /* Binning ratio */
>  
> -	dev_dbg(&client->dev,
> -		"%s: width = %d height = %d code = %d\n",
> -		__func__, fmt->width, fmt->height, fmt->code);
> +	dev_dbg(dev, "%s: request of  %dx%d (%s)\n",
> +		__func__, fmt->width, fmt->height,
> +		V4L2_SUBDEV_FORMAT_ACTIVE ? "ACTIVE" : "TRY");
>  
>  	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)
> +	/* Find ratio (maximize output resolution). Fallback to 1:1. */
> +	for (ratio = 3; ratio > 1; ratio--)
> +		if (fmt->width <= DIV_ROUND_UP(imx274->crop.width, ratio) &&
> +		    fmt->height <= DIV_ROUND_UP(imx274->crop.height, ratio))
>  			break;
> -	}
> -
> -	if (index >= ARRAY_SIZE(imx274_formats)) {
> -		/* default to first format */
> -		index = 0;
> -	}
>  
> -	imx274->mode = &imx274_formats[index];
> +	imx274->mode = &imx274_formats[ratio - 1];
>  
> -	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);
> +	fmt->width = imx274->crop.width / ratio;
> +	fmt->height = imx274->crop.height / ratio;
>  	fmt->field = V4L2_FIELD_NONE;
>  
> +	dev_dbg(dev, "%s: adjusted to %dx%d (%d:1 binning)\n",
> +		__func__, fmt->width, fmt->height, ratio);
> +
>  	if (format->which == V4L2_SUBDEV_FORMAT_TRY)
>  		cfg->try_fmt = *fmt;
>  	else
> @@ -940,6 +904,152 @@ static int imx274_set_fmt(struct v4l2_subdev *sd,
>  	return 0;
>  }
>  
> +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);
> +
> +	if (sel->pad != 0)
> +		return -EINVAL;
> +
> +	switch (sel->target) {
> +	case 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;
> +	case V4L2_SEL_TGT_CROP:
> +		if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> +			mutex_lock(&imx274->lock);
> +			sel->r = imx274->crop;
> +			mutex_unlock(&imx274->lock);
> +		} else {
> +			sel->r = cfg->try_crop;
> +		}
> +		return 0;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	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);
> +	struct device *dev = &imx274->client->dev;
> +	struct v4l2_mbus_framefmt *tgt_fmt;
> +	struct v4l2_rect *tgt_crop;
> +	struct v4l2_rect new_crop;
> +

A lot of sensor drivers use the format IOCTLs to configure the output size
of the sensor and that's what it must be: the get_fmt() pad op has to
return the format of the image produced by the sensor. The size set by the
set_fmt() also must be obtainable using get_fmt().

What I propose is to move also the binning configuration to use selections,
i.e. the COMPOSE selection target. The NATIVE_SIZE target is the sensor's
native size, the largest that can be accessed in its pixel array.

The size in format related IOCTLs is thus derived from the NATIVE_SIZE,
CROP and COMPOSE configuration, and is no longer settable directly.

Btw. the crop here, is that taking place after binning as it would seem
like? Then, I presume it is digital cropping, and the CROP rectangle is
related to the COMPOSE rectangle (binning configuration).

> +	/*
> +	 * 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");
> +
> +	if (sel->target != V4L2_SEL_TGT_CROP || sel->pad != 0)
> +		return -EINVAL;
> +
> +	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);
> +
> +	if (sel->which == V4L2_SUBDEV_FORMAT_TRY) {
> +		tgt_crop = &cfg->try_crop;
> +		tgt_fmt = &cfg->try_fmt;
> +	} else {
> +		tgt_crop = &imx274->crop;
> +		tgt_fmt = &imx274->format;
> +	}
> +
> +	mutex_lock(&imx274->lock);
> +
> +	if (new_crop.width != tgt_crop->width ||
> +	    new_crop.height != tgt_crop->height) {
> +		/* crop size changed, reset the output image size */
> +		tgt_fmt->width = new_crop.width;
> +		tgt_fmt->height = new_crop.height;
> +		if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> +			imx274->mode = &imx274_formats[IMX274_MODE_BINNING_OFF];
> +	}
> +
> +	*tgt_crop = new_crop;
> +
> +	mutex_unlock(&imx274->lock);
> +
> +	sel->r = new_crop;
> +
> +	return 0;
> +}
> +
> +static int imx274_set_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 +1190,10 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on)
>  		if (ret)
>  			goto fail;
>  
> +		ret = imx274_set_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 +1703,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 = {
> @@ -1641,6 +1757,8 @@ static int imx274_probe(struct i2c_client *client,
>  	imx274->format.colorspace = V4L2_COLORSPACE_SRGB;
>  	imx274->frame_interval.numerator = 1;
>  	imx274->frame_interval.denominator = IMX274_DEF_FRAME_RATE;
> +	imx274->crop.width = imx274->mode->size.width;
> +	imx274->crop.height = imx274->mode->size.height;
>  
>  	/* initialize regmap */
>  	imx274->regmap = devm_regmap_init_i2c(client, &imx274_regmap_config);

-- 
Kind regards,

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

Re: [PATCH v2 13/13] media: imx274: add SELECTION support for cropping

[Luca Ceresoli]

Hi Sakari,

On 24/04/2018 11:59, Sakari Ailus wrote:
> Hi Luca,
> 
> Thank you for the patchset.
> 
> Some comments below... what I propose is that I apply the rest of the
> patches and then the comments to this one could be addressed separately.
> Would that work for you?

Yes, but I suggest you only apply patches 1-6. I noticed a warning in
patch 9, and patches 7-8 are not very useful without it. Will fix it in v3.

I'll wait for the outcome of the discussion below before sending v3.
Tell me if you prefer me to do it earlier.

> On Tue, Apr 24, 2018 at 10:24:18AM +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 set_fmt callback chooses among these 3 configurations the one that
>> matches the requested format.
>>
>> Adding support to VIDIOC_SUBDEV_G_SELECTION and
>> VIDIOC_SUBDEV_S_SELECTION involved a complete rewrite of set_fmt,
>> which now chooses the most appropriate binning based on the ratio
>> between the previously-set cropping size and the format size being
>> requested.
>>
>> Note that the names in enum imx274_mode change from being
>> resolution-based to being binning-based. 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 anny
>> misunderstanding.
>>
>> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
>>
>> ---
>> Changed v1 -> v2:
>>  - add "media: " prefix to commit message
>> ---
>>  drivers/media/i2c/imx274.c | 266 ++++++++++++++++++++++++++++++++-------------
>>  1 file changed, 192 insertions(+), 74 deletions(-)
>>
>> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
>> index b6c54712f2aa..ceb5b3e498c6 100644
>> --- a/drivers/media/i2c/imx274.c
>> +++ b/drivers/media/i2c/imx274.c
>> @@ -19,6 +19,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 +75,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 +112,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 +156,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,
>>  };
>>  
>>  /*
>> @@ -215,31 +230,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,7 +271,7 @@ static const struct reg_8 imx274_mode3_1920x1080_raw10[] = {
>>  	{0x3004, 0x02},
>>  	{0x3005, 0x21},
>>  	{0x3006, 0x00},
>> -	{0x3007, 0x11},
>> +	{0x3007, 0xb1},
>>  
>>  	{0x3018, 0xA2}, /* output XVS, HVS */
>>  
>> @@ -283,22 +281,7 @@ static const struct reg_8 imx274_mode3_1920x1080_raw10[] = {
>>  	{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,7 +314,7 @@ static const struct reg_8 imx274_mode5_1280x720_raw10[] = {
>>  	{0x3004, 0x03},
>>  	{0x3005, 0x31},
>>  	{0x3006, 0x00},
>> -	{0x3007, 0x09},
>> +	{0x3007, 0xa9},
>>  
>>  	{0x3018, 0xA2}, /* output XVS, HVS */
>>  
>> @@ -341,21 +324,7 @@ static const struct reg_8 imx274_mode5_1280x720_raw10[] = {
>>  	{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},
>> @@ -561,6 +530,7 @@ struct stimx274 {
>>  	struct imx274_ctrls ctrls;
>>  	struct v4l2_mbus_framefmt format;
>>  	struct v4l2_fract frame_interval;
>> +	struct v4l2_rect crop;
>>  	struct regmap *regmap;
>>  	struct gpio_desc *reset_gpio;
>>  	struct mutex lock; /* mutex lock for operations */
>> @@ -901,36 +871,30 @@ 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;
>> +	struct device *dev = &imx274->client->dev;
>> +	unsigned int ratio; /* Binning ratio */
>>  
>> -	dev_dbg(&client->dev,
>> -		"%s: width = %d height = %d code = %d\n",
>> -		__func__, fmt->width, fmt->height, fmt->code);
>> +	dev_dbg(dev, "%s: request of  %dx%d (%s)\n",
>> +		__func__, fmt->width, fmt->height,
>> +		V4L2_SUBDEV_FORMAT_ACTIVE ? "ACTIVE" : "TRY");
>>  
>>  	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)
>> +	/* Find ratio (maximize output resolution). Fallback to 1:1. */
>> +	for (ratio = 3; ratio > 1; ratio--)
>> +		if (fmt->width <= DIV_ROUND_UP(imx274->crop.width, ratio) &&
>> +		    fmt->height <= DIV_ROUND_UP(imx274->crop.height, ratio))
>>  			break;
>> -	}
>> -
>> -	if (index >= ARRAY_SIZE(imx274_formats)) {
>> -		/* default to first format */
>> -		index = 0;
>> -	}
>>  
>> -	imx274->mode = &imx274_formats[index];
>> +	imx274->mode = &imx274_formats[ratio - 1];
>>  
>> -	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);
>> +	fmt->width = imx274->crop.width / ratio;
>> +	fmt->height = imx274->crop.height / ratio;
>>  	fmt->field = V4L2_FIELD_NONE;
>>  
>> +	dev_dbg(dev, "%s: adjusted to %dx%d (%d:1 binning)\n",
>> +		__func__, fmt->width, fmt->height, ratio);
>> +
>>  	if (format->which == V4L2_SUBDEV_FORMAT_TRY)
>>  		cfg->try_fmt = *fmt;
>>  	else
>> @@ -940,6 +904,152 @@ static int imx274_set_fmt(struct v4l2_subdev *sd,
>>  	return 0;
>>  }
>>  
>> +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);
>> +
>> +	if (sel->pad != 0)
>> +		return -EINVAL;
>> +
>> +	switch (sel->target) {
>> +	case 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;
>> +	case V4L2_SEL_TGT_CROP:
>> +		if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
>> +			mutex_lock(&imx274->lock);
>> +			sel->r = imx274->crop;
>> +			mutex_unlock(&imx274->lock);
>> +		} else {
>> +			sel->r = cfg->try_crop;
>> +		}
>> +		return 0;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	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);
>> +	struct device *dev = &imx274->client->dev;
>> +	struct v4l2_mbus_framefmt *tgt_fmt;
>> +	struct v4l2_rect *tgt_crop;
>> +	struct v4l2_rect new_crop;
>> +
> 
> A lot of sensor drivers use the format IOCTLs to configure the output size
> of the sensor and that's what it must be: the get_fmt() pad op has to
> return the format of the image produced by the sensor. The size set by the
> set_fmt() also must be obtainable using get_fmt().
> 
> What I propose is to move also the binning configuration to use selections,
> i.e. the COMPOSE selection target. The NATIVE_SIZE target is the sensor's
> native size, the largest that can be accessed in its pixel array.
> 
> The size in format related IOCTLs is thus derived from the NATIVE_SIZE,
> CROP and COMPOSE configuration, and is no longer settable directly.

I'm OK with making any improvement, but I'm afraid I'm not understanding
what you mean. Do you mean we should stop responding to the VIDIOC_S_FMT
ioctl, aka v4l2_subdev_pad_ops.set_selection? Wouldn't this break
existing applications?

Also the meaning of COMPOSE in the context of a sensor subdev is unclear
to me. For a video node (which is typically paired with a DMA) it makes
sense: the DMA can write a sub-area of the destination memory buffer.
But the sensor subdev is the first element of a possibly long processing
chain, and as such it only produces a stream. The sensor does not know
what a "memory buffer" is.

What's wrong with my understanding?

> Btw. the crop here, is that taking place after binning as it would seem
> like? Then, I presume it is digital cropping, and the CROP rectangle is
> related to the COMPOSE rectangle (binning configuration).

Logically speaking, cropping here happens before binning, i.e. the crop
rectangle always refers to NATIVE_SIZE. The sensor just skips lines and
columns outside of that rectangle and optionally bins what's inside.

Examples of how it is currently implemented:
- native 3840x2160, crop 3840x2160, fmt 3840x2160 = don't crop,  1:1 bin
- native 3840x2160, crop 1920x1080, fmt 1920x1080 = crop to 50%, 1:1 bin
- native 3840x2160, crop 3840x2160, fmt 1920x1080 = don't crop,  2:1 bin
- native 3840x2160, crop 1920x1080, fmt  960x 540 = crop to 50%, 2:1 bin

Can you provide similar examples of how it would work when setting CROP
+ COMPOSE, and without setting the format?

Regards,
-- 
Luca

Re: [PATCH v2 09/13] media: imx274: get rid of mode_index

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 6492 bytes --]

Hi Luca,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v4.17-rc2 next-20180424]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Luca-Ceresoli/media-imx274-cleanups-improvements-and-SELECTION-API-support/20180424-220137
base:   git://linuxtv.org/media_tree.git master
config: sparc64-allmodconfig (attached as .config)
compiler: sparc64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=sparc64 

All warnings (new ones prefixed by >>):

   In file included from include/linux/printk.h:332:0,
                    from include/linux/kernel.h:14,
                    from include/linux/clk.h:16,
                    from drivers/media//i2c/imx274.c:22:
   drivers/media//i2c/imx274.c: In function 'imx274_s_stream':
>> drivers/media//i2c/imx274.c:1027:32: warning: format '%d' expects argument of type 'int', but argument 6 has type 'long int' [-Wformat=]
     dev_dbg(&imx274->client->dev, "%s : %s, mode index = %d\n", __func__,
                                   ^
   drivers/media//i2c/imx274.c:1029:3:
      imx274->mode - &imx274_formats[0]);
      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:135:39: note: in definition of macro 'dynamic_dev_dbg'
      __dynamic_dev_dbg(&descriptor, dev, fmt, \
                                          ^~~
   drivers/media//i2c/imx274.c:1027:2: note: in expansion of macro 'dev_dbg'
     dev_dbg(&imx274->client->dev, "%s : %s, mode index = %d\n", __func__,
     ^~~~~~~

vim +1027 drivers/media//i2c/imx274.c

0985dd30 Leon Luo      2017-10-05  1011  
0985dd30 Leon Luo      2017-10-05  1012  /**
0985dd30 Leon Luo      2017-10-05  1013   * imx274_s_stream - It is used to start/stop the streaming.
0985dd30 Leon Luo      2017-10-05  1014   * @sd: V4L2 Sub device
0985dd30 Leon Luo      2017-10-05  1015   * @on: Flag (True / False)
0985dd30 Leon Luo      2017-10-05  1016   *
0985dd30 Leon Luo      2017-10-05  1017   * This function controls the start or stop of streaming for the
0985dd30 Leon Luo      2017-10-05  1018   * imx274 sensor.
0985dd30 Leon Luo      2017-10-05  1019   *
0985dd30 Leon Luo      2017-10-05  1020   * Return: 0 on success, errors otherwise
0985dd30 Leon Luo      2017-10-05  1021   */
0985dd30 Leon Luo      2017-10-05  1022  static int imx274_s_stream(struct v4l2_subdev *sd, int on)
0985dd30 Leon Luo      2017-10-05  1023  {
0985dd30 Leon Luo      2017-10-05  1024  	struct stimx274 *imx274 = to_imx274(sd);
0985dd30 Leon Luo      2017-10-05  1025  	int ret = 0;
0985dd30 Leon Luo      2017-10-05  1026  
0985dd30 Leon Luo      2017-10-05 @1027  	dev_dbg(&imx274->client->dev, "%s : %s, mode index = %d\n", __func__,
f5180bf1 Luca Ceresoli 2018-04-24  1028  		on ? "Stream Start" : "Stream Stop",
f5180bf1 Luca Ceresoli 2018-04-24  1029  		imx274->mode - &imx274_formats[0]);
0985dd30 Leon Luo      2017-10-05  1030  
0985dd30 Leon Luo      2017-10-05  1031  	mutex_lock(&imx274->lock);
0985dd30 Leon Luo      2017-10-05  1032  
0985dd30 Leon Luo      2017-10-05  1033  	if (on) {
0985dd30 Leon Luo      2017-10-05  1034  		/* load mode registers */
da5bbae7 Luca Ceresoli 2018-04-24  1035  		ret = imx274_mode_regs(imx274);
0985dd30 Leon Luo      2017-10-05  1036  		if (ret)
0985dd30 Leon Luo      2017-10-05  1037  			goto fail;
0985dd30 Leon Luo      2017-10-05  1038  
0985dd30 Leon Luo      2017-10-05  1039  		/*
0985dd30 Leon Luo      2017-10-05  1040  		 * update frame rate & expsoure. if the last mode is different,
0985dd30 Leon Luo      2017-10-05  1041  		 * HMAX could be changed. As the result, frame rate & exposure
0985dd30 Leon Luo      2017-10-05  1042  		 * are changed.
0985dd30 Leon Luo      2017-10-05  1043  		 * gain is not affected.
0985dd30 Leon Luo      2017-10-05  1044  		 */
0985dd30 Leon Luo      2017-10-05  1045  		ret = imx274_set_frame_interval(imx274,
0985dd30 Leon Luo      2017-10-05  1046  						imx274->frame_interval);
0985dd30 Leon Luo      2017-10-05  1047  		if (ret)
0985dd30 Leon Luo      2017-10-05  1048  			goto fail;
0985dd30 Leon Luo      2017-10-05  1049  
0985dd30 Leon Luo      2017-10-05  1050  		/* update exposure time */
0985dd30 Leon Luo      2017-10-05  1051  		ret = __v4l2_ctrl_s_ctrl(imx274->ctrls.exposure,
0985dd30 Leon Luo      2017-10-05  1052  					 imx274->ctrls.exposure->val);
0985dd30 Leon Luo      2017-10-05  1053  		if (ret)
0985dd30 Leon Luo      2017-10-05  1054  			goto fail;
0985dd30 Leon Luo      2017-10-05  1055  
0985dd30 Leon Luo      2017-10-05  1056  		/* start stream */
0985dd30 Leon Luo      2017-10-05  1057  		ret = imx274_start_stream(imx274);
0985dd30 Leon Luo      2017-10-05  1058  		if (ret)
0985dd30 Leon Luo      2017-10-05  1059  			goto fail;
0985dd30 Leon Luo      2017-10-05  1060  	} else {
0985dd30 Leon Luo      2017-10-05  1061  		/* stop stream */
cf2a54e2 Luca Ceresoli 2018-04-24  1062  		ret = imx274_write_table(imx274, imx274_stop);
0985dd30 Leon Luo      2017-10-05  1063  		if (ret)
0985dd30 Leon Luo      2017-10-05  1064  			goto fail;
0985dd30 Leon Luo      2017-10-05  1065  	}
0985dd30 Leon Luo      2017-10-05  1066  
0985dd30 Leon Luo      2017-10-05  1067  	mutex_unlock(&imx274->lock);
f5180bf1 Luca Ceresoli 2018-04-24  1068  	dev_dbg(&imx274->client->dev, "%s : Done\n", __func__);
0985dd30 Leon Luo      2017-10-05  1069  	return 0;
0985dd30 Leon Luo      2017-10-05  1070  
0985dd30 Leon Luo      2017-10-05  1071  fail:
0985dd30 Leon Luo      2017-10-05  1072  	mutex_unlock(&imx274->lock);
0985dd30 Leon Luo      2017-10-05  1073  	dev_err(&imx274->client->dev, "s_stream failed\n");
0985dd30 Leon Luo      2017-10-05  1074  	return ret;
0985dd30 Leon Luo      2017-10-05  1075  }
0985dd30 Leon Luo      2017-10-05  1076  

:::::: The code at line 1027 was first introduced by commit
:::::: 0985dd306f727df6c0e71cd8a8eda93e8fa5206e media: imx274: V4l2 driver for Sony imx274 CMOS sensor

:::::: TO: Leon Luo <leonl@leopardimaging.com>
:::::: CC: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 53493 bytes --]

Re: [PATCH v2 13/13] media: imx274: add SELECTION support for cropping

[Sakari Ailus]

Hi Luca,

On Tue, Apr 24, 2018 at 04:30:38PM +0200, Luca Ceresoli wrote:
> Hi Sakari,
> 
> On 24/04/2018 11:59, Sakari Ailus wrote:
> > Hi Luca,
> > 
> > Thank you for the patchset.
> > 
> > Some comments below... what I propose is that I apply the rest of the
> > patches and then the comments to this one could be addressed separately.
> > Would that work for you?
> 
> Yes, but I suggest you only apply patches 1-6. I noticed a warning in
> patch 9, and patches 7-8 are not very useful without it. Will fix it in v3.

Ack. I'll apply 1--6 then.

> 
> I'll wait for the outcome of the discussion below before sending v3.
> Tell me if you prefer me to do it earlier.
> 
> > On Tue, Apr 24, 2018 at 10:24:18AM +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 set_fmt callback chooses among these 3 configurations the one that
> >> matches the requested format.
> >>
> >> Adding support to VIDIOC_SUBDEV_G_SELECTION and
> >> VIDIOC_SUBDEV_S_SELECTION involved a complete rewrite of set_fmt,
> >> which now chooses the most appropriate binning based on the ratio
> >> between the previously-set cropping size and the format size being
> >> requested.
> >>
> >> Note that the names in enum imx274_mode change from being
> >> resolution-based to being binning-based. 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 anny
> >> misunderstanding.
> >>
> >> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
> >>
> >> ---
> >> Changed v1 -> v2:
> >>  - add "media: " prefix to commit message
> >> ---
> >>  drivers/media/i2c/imx274.c | 266 ++++++++++++++++++++++++++++++++-------------
> >>  1 file changed, 192 insertions(+), 74 deletions(-)
> >>
> >> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
> >> index b6c54712f2aa..ceb5b3e498c6 100644
> >> --- a/drivers/media/i2c/imx274.c
> >> +++ b/drivers/media/i2c/imx274.c
> >> @@ -19,6 +19,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 +75,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 +112,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 +156,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,
> >>  };
> >>  
> >>  /*
> >> @@ -215,31 +230,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,7 +271,7 @@ static const struct reg_8 imx274_mode3_1920x1080_raw10[] = {
> >>  	{0x3004, 0x02},
> >>  	{0x3005, 0x21},
> >>  	{0x3006, 0x00},
> >> -	{0x3007, 0x11},
> >> +	{0x3007, 0xb1},
> >>  
> >>  	{0x3018, 0xA2}, /* output XVS, HVS */
> >>  
> >> @@ -283,22 +281,7 @@ static const struct reg_8 imx274_mode3_1920x1080_raw10[] = {
> >>  	{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,7 +314,7 @@ static const struct reg_8 imx274_mode5_1280x720_raw10[] = {
> >>  	{0x3004, 0x03},
> >>  	{0x3005, 0x31},
> >>  	{0x3006, 0x00},
> >> -	{0x3007, 0x09},
> >> +	{0x3007, 0xa9},
> >>  
> >>  	{0x3018, 0xA2}, /* output XVS, HVS */
> >>  
> >> @@ -341,21 +324,7 @@ static const struct reg_8 imx274_mode5_1280x720_raw10[] = {
> >>  	{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},
> >> @@ -561,6 +530,7 @@ struct stimx274 {
> >>  	struct imx274_ctrls ctrls;
> >>  	struct v4l2_mbus_framefmt format;
> >>  	struct v4l2_fract frame_interval;
> >> +	struct v4l2_rect crop;
> >>  	struct regmap *regmap;
> >>  	struct gpio_desc *reset_gpio;
> >>  	struct mutex lock; /* mutex lock for operations */
> >> @@ -901,36 +871,30 @@ 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;
> >> +	struct device *dev = &imx274->client->dev;
> >> +	unsigned int ratio; /* Binning ratio */
> >>  
> >> -	dev_dbg(&client->dev,
> >> -		"%s: width = %d height = %d code = %d\n",
> >> -		__func__, fmt->width, fmt->height, fmt->code);
> >> +	dev_dbg(dev, "%s: request of  %dx%d (%s)\n",
> >> +		__func__, fmt->width, fmt->height,
> >> +		V4L2_SUBDEV_FORMAT_ACTIVE ? "ACTIVE" : "TRY");
> >>  
> >>  	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)
> >> +	/* Find ratio (maximize output resolution). Fallback to 1:1. */
> >> +	for (ratio = 3; ratio > 1; ratio--)
> >> +		if (fmt->width <= DIV_ROUND_UP(imx274->crop.width, ratio) &&
> >> +		    fmt->height <= DIV_ROUND_UP(imx274->crop.height, ratio))
> >>  			break;
> >> -	}
> >> -
> >> -	if (index >= ARRAY_SIZE(imx274_formats)) {
> >> -		/* default to first format */
> >> -		index = 0;
> >> -	}
> >>  
> >> -	imx274->mode = &imx274_formats[index];
> >> +	imx274->mode = &imx274_formats[ratio - 1];
> >>  
> >> -	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);
> >> +	fmt->width = imx274->crop.width / ratio;
> >> +	fmt->height = imx274->crop.height / ratio;
> >>  	fmt->field = V4L2_FIELD_NONE;
> >>  
> >> +	dev_dbg(dev, "%s: adjusted to %dx%d (%d:1 binning)\n",
> >> +		__func__, fmt->width, fmt->height, ratio);
> >> +
> >>  	if (format->which == V4L2_SUBDEV_FORMAT_TRY)
> >>  		cfg->try_fmt = *fmt;
> >>  	else
> >> @@ -940,6 +904,152 @@ static int imx274_set_fmt(struct v4l2_subdev *sd,
> >>  	return 0;
> >>  }
> >>  
> >> +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);
> >> +
> >> +	if (sel->pad != 0)
> >> +		return -EINVAL;
> >> +
> >> +	switch (sel->target) {
> >> +	case 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;
> >> +	case V4L2_SEL_TGT_CROP:
> >> +		if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> >> +			mutex_lock(&imx274->lock);
> >> +			sel->r = imx274->crop;
> >> +			mutex_unlock(&imx274->lock);
> >> +		} else {
> >> +			sel->r = cfg->try_crop;
> >> +		}
> >> +		return 0;
> >> +	default:
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	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);
> >> +	struct device *dev = &imx274->client->dev;
> >> +	struct v4l2_mbus_framefmt *tgt_fmt;
> >> +	struct v4l2_rect *tgt_crop;
> >> +	struct v4l2_rect new_crop;
> >> +
> > 
> > A lot of sensor drivers use the format IOCTLs to configure the output size
> > of the sensor and that's what it must be: the get_fmt() pad op has to
> > return the format of the image produced by the sensor. The size set by the
> > set_fmt() also must be obtainable using get_fmt().
> > 
> > What I propose is to move also the binning configuration to use selections,
> > i.e. the COMPOSE selection target. The NATIVE_SIZE target is the sensor's
> > native size, the largest that can be accessed in its pixel array.
> > 
> > The size in format related IOCTLs is thus derived from the NATIVE_SIZE,
> > CROP and COMPOSE configuration, and is no longer settable directly.
> 
> I'm OK with making any improvement, but I'm afraid I'm not understanding
> what you mean. Do you mean we should stop responding to the VIDIOC_S_FMT
> ioctl, aka v4l2_subdev_pad_ops.set_selection? Wouldn't this break
> existing applications?

Hmm. The driver supports sub-device uAPI. Which bridge (or ISP) driver are
you using the sensor with?

> 
> Also the meaning of COMPOSE in the context of a sensor subdev is unclear
> to me. For a video node (which is typically paired with a DMA) it makes
> sense: the DMA can write a sub-area of the destination memory buffer.
> But the sensor subdev is the first element of a possibly long processing
> chain, and as such it only produces a stream. The sensor does not know
> what a "memory buffer" is.
> 
> What's wrong with my understanding?

The COMPOSE target is also used for configuring scaling:

<URL:https://hverkuil.home.xs4all.nl/spec/uapi/v4l/dev-subdev.html#selections-cropping-scaling-and-composition>

And binning is effectively scaling.

> 
> > Btw. the crop here, is that taking place after binning as it would seem
> > like? Then, I presume it is digital cropping, and the CROP rectangle is
> > related to the COMPOSE rectangle (binning configuration).
> 
> Logically speaking, cropping here happens before binning, i.e. the crop
> rectangle always refers to NATIVE_SIZE. The sensor just skips lines and
> columns outside of that rectangle and optionally bins what's inside.
> 
> Examples of how it is currently implemented:
> - native 3840x2160, crop 3840x2160, fmt 3840x2160 = don't crop,  1:1 bin
> - native 3840x2160, crop 1920x1080, fmt 1920x1080 = crop to 50%, 1:1 bin
> - native 3840x2160, crop 3840x2160, fmt 1920x1080 = don't crop,  2:1 bin
> - native 3840x2160, crop 1920x1080, fmt  960x 540 = crop to 50%, 2:1 bin
> 
> Can you provide similar examples of how it would work when setting CROP
> + COMPOSE, and without setting the format?
> 
> Regards,

-- 
Kind regards,

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

Re: [PATCH v2 13/13] media: imx274: add SELECTION support for cropping

[Luca Ceresoli]

Hi Sakari,

thanks for your feedback, see below my replies.

On 26/04/2018 10:13, Sakari Ailus wrote:
> Hi Luca,
> 
> On Tue, Apr 24, 2018 at 04:30:38PM +0200, Luca Ceresoli wrote:
>> Hi Sakari,
>>
>> On 24/04/2018 11:59, Sakari Ailus wrote:
>>> Hi Luca,
>>>
>>> Thank you for the patchset.
>>>
>>> Some comments below... what I propose is that I apply the rest of the
>>> patches and then the comments to this one could be addressed separately.
>>> Would that work for you?
>>
>> Yes, but I suggest you only apply patches 1-6. I noticed a warning in
>> patch 9, and patches 7-8 are not very useful without it. Will fix it in v3.
> 
> Ack. I'll apply 1--6 then.

Thanks.

>> I'll wait for the outcome of the discussion below before sending v3.
>> Tell me if you prefer me to do it earlier.
>>
>>> On Tue, Apr 24, 2018 at 10:24:18AM +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 set_fmt callback chooses among these 3 configurations the one that
>>>> matches the requested format.
>>>>
>>>> Adding support to VIDIOC_SUBDEV_G_SELECTION and
>>>> VIDIOC_SUBDEV_S_SELECTION involved a complete rewrite of set_fmt,
>>>> which now chooses the most appropriate binning based on the ratio
>>>> between the previously-set cropping size and the format size being
>>>> requested.
>>>>
>>>> Note that the names in enum imx274_mode change from being
>>>> resolution-based to being binning-based. 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 anny
>>>> misunderstanding.
>>>>
>>>> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
>>>>
>>>> ---
>>>> Changed v1 -> v2:
>>>>  - add "media: " prefix to commit message
>>>> ---
>>>>  drivers/media/i2c/imx274.c | 266 ++++++++++++++++++++++++++++++++-------------
>>>>  1 file changed, 192 insertions(+), 74 deletions(-)
>>>>
>>>> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
>>>> index b6c54712f2aa..ceb5b3e498c6 100644
>>>> --- a/drivers/media/i2c/imx274.c
>>>> +++ b/drivers/media/i2c/imx274.c
>>>> @@ -19,6 +19,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 +75,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 +112,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 +156,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,
>>>>  };
>>>>  
>>>>  /*
>>>> @@ -215,31 +230,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,7 +271,7 @@ static const struct reg_8 imx274_mode3_1920x1080_raw10[] = {
>>>>  	{0x3004, 0x02},
>>>>  	{0x3005, 0x21},
>>>>  	{0x3006, 0x00},
>>>> -	{0x3007, 0x11},
>>>> +	{0x3007, 0xb1},
>>>>  
>>>>  	{0x3018, 0xA2}, /* output XVS, HVS */
>>>>  
>>>> @@ -283,22 +281,7 @@ static const struct reg_8 imx274_mode3_1920x1080_raw10[] = {
>>>>  	{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,7 +314,7 @@ static const struct reg_8 imx274_mode5_1280x720_raw10[] = {
>>>>  	{0x3004, 0x03},
>>>>  	{0x3005, 0x31},
>>>>  	{0x3006, 0x00},
>>>> -	{0x3007, 0x09},
>>>> +	{0x3007, 0xa9},
>>>>  
>>>>  	{0x3018, 0xA2}, /* output XVS, HVS */
>>>>  
>>>> @@ -341,21 +324,7 @@ static const struct reg_8 imx274_mode5_1280x720_raw10[] = {
>>>>  	{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},
>>>> @@ -561,6 +530,7 @@ struct stimx274 {
>>>>  	struct imx274_ctrls ctrls;
>>>>  	struct v4l2_mbus_framefmt format;
>>>>  	struct v4l2_fract frame_interval;
>>>> +	struct v4l2_rect crop;
>>>>  	struct regmap *regmap;
>>>>  	struct gpio_desc *reset_gpio;
>>>>  	struct mutex lock; /* mutex lock for operations */
>>>> @@ -901,36 +871,30 @@ 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;
>>>> +	struct device *dev = &imx274->client->dev;
>>>> +	unsigned int ratio; /* Binning ratio */
>>>>  
>>>> -	dev_dbg(&client->dev,
>>>> -		"%s: width = %d height = %d code = %d\n",
>>>> -		__func__, fmt->width, fmt->height, fmt->code);
>>>> +	dev_dbg(dev, "%s: request of  %dx%d (%s)\n",
>>>> +		__func__, fmt->width, fmt->height,
>>>> +		V4L2_SUBDEV_FORMAT_ACTIVE ? "ACTIVE" : "TRY");
>>>>  
>>>>  	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)
>>>> +	/* Find ratio (maximize output resolution). Fallback to 1:1. */
>>>> +	for (ratio = 3; ratio > 1; ratio--)
>>>> +		if (fmt->width <= DIV_ROUND_UP(imx274->crop.width, ratio) &&
>>>> +		    fmt->height <= DIV_ROUND_UP(imx274->crop.height, ratio))
>>>>  			break;
>>>> -	}
>>>> -
>>>> -	if (index >= ARRAY_SIZE(imx274_formats)) {
>>>> -		/* default to first format */
>>>> -		index = 0;
>>>> -	}
>>>>  
>>>> -	imx274->mode = &imx274_formats[index];
>>>> +	imx274->mode = &imx274_formats[ratio - 1];
>>>>  
>>>> -	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);
>>>> +	fmt->width = imx274->crop.width / ratio;
>>>> +	fmt->height = imx274->crop.height / ratio;
>>>>  	fmt->field = V4L2_FIELD_NONE;
>>>>  
>>>> +	dev_dbg(dev, "%s: adjusted to %dx%d (%d:1 binning)\n",
>>>> +		__func__, fmt->width, fmt->height, ratio);
>>>> +
>>>>  	if (format->which == V4L2_SUBDEV_FORMAT_TRY)
>>>>  		cfg->try_fmt = *fmt;
>>>>  	else
>>>> @@ -940,6 +904,152 @@ static int imx274_set_fmt(struct v4l2_subdev *sd,
>>>>  	return 0;
>>>>  }
>>>>  
>>>> +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);
>>>> +
>>>> +	if (sel->pad != 0)
>>>> +		return -EINVAL;
>>>> +
>>>> +	switch (sel->target) {
>>>> +	case 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;
>>>> +	case V4L2_SEL_TGT_CROP:
>>>> +		if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
>>>> +			mutex_lock(&imx274->lock);
>>>> +			sel->r = imx274->crop;
>>>> +			mutex_unlock(&imx274->lock);
>>>> +		} else {
>>>> +			sel->r = cfg->try_crop;
>>>> +		}
>>>> +		return 0;
>>>> +	default:
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	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);
>>>> +	struct device *dev = &imx274->client->dev;
>>>> +	struct v4l2_mbus_framefmt *tgt_fmt;
>>>> +	struct v4l2_rect *tgt_crop;
>>>> +	struct v4l2_rect new_crop;
>>>> +
>>>
>>> A lot of sensor drivers use the format IOCTLs to configure the output size
>>> of the sensor and that's what it must be: the get_fmt() pad op has to
>>> return the format of the image produced by the sensor. The size set by the
>>> set_fmt() also must be obtainable using get_fmt().
>>>
>>> What I propose is to move also the binning configuration to use selections,
>>> i.e. the COMPOSE selection target. The NATIVE_SIZE target is the sensor's
>>> native size, the largest that can be accessed in its pixel array.
>>>
>>> The size in format related IOCTLs is thus derived from the NATIVE_SIZE,
>>> CROP and COMPOSE configuration, and is no longer settable directly.
>>
>> I'm OK with making any improvement, but I'm afraid I'm not understanding
>> what you mean. Do you mean we should stop responding to the VIDIOC_S_FMT
>> ioctl, aka v4l2_subdev_pad_ops.set_selection? Wouldn't this break
                                      ^
My typo here, it should be v4l2_subdev_pad_ops.set_fmt.

>> existing applications?
> 
> Hmm. The driver supports sub-device uAPI. Which bridge (or ISP) driver are
> you using the sensor with?

In my current setup I have an AXI-stream chain made of Xilinx IPs:
imx274, demosaic, color correction matrix, rgb2yuv, framebuffer writer
(=DMA).

>> Also the meaning of COMPOSE in the context of a sensor subdev is unclear
>> to me. For a video node (which is typically paired with a DMA) it makes
>> sense: the DMA can write a sub-area of the destination memory buffer.
>> But the sensor subdev is the first element of a possibly long processing
>> chain, and as such it only produces a stream. The sensor does not know
>> what a "memory buffer" is.
>>
>> What's wrong with my understanding?
> 
> The COMPOSE target is also used for configuring scaling:
> 
> <URL:https://hverkuil.home.xs4all.nl/spec/uapi/v4l/dev-subdev.html#selections-cropping-scaling-and-composition>

Thanks for the link, that was useful. I'm now convinced COMPOSE makes
sense on the _sink_ pads of a subdev. The last example with 2 sinks and
2 sources was particularly enlightening to me. The logical abstraction
is that there's a "compose buffer" inside the subdev where 1+ sinks are
composed, then 1+ source pads pick a sub-portion (crop) from this buffer
and send it out. Of course the "compose buffer" might not exist
physically, but the transfer function is the same.

But I still fail to understand COMPOSE on a _source_ pad. I understand
COMPOSE as: there's a "large" rectangle, but we write only a
sub-rectangle of it, leaving the surrounding area unmodified. But in the
stream between a sensor and the following subdev (e.g. demosaic) there
is only a flow of pixels, no notion of "surrounding unmodified pixels".
And no notione of "buffer", be it physical or not. Right?

This understanding of mine seems supported by the 5-item list in the
page you linked, where the parameters to set are listed as:

 1. Sink pad format
 2. Sink pad actual crop selection
 3. Sink pad actual compose selection
 4. Source pad actual crop selection
 5. Source pad format

And there is no "Source pad actual compose selection".

But clearly there is still something I don't understand. Any further
reading suggestion, or an existing sensor driver to look at, would be
welcome.

> And binning is effectively scaling.

Sure.

>>> Btw. the crop here, is that taking place after binning as it would seem
>>> like? Then, I presume it is digital cropping, and the CROP rectangle is
>>> related to the COMPOSE rectangle (binning configuration).
>>
>> Logically speaking, cropping here happens before binning, i.e. the crop
>> rectangle always refers to NATIVE_SIZE. The sensor just skips lines and
>> columns outside of that rectangle and optionally bins what's inside.
>>
>> Examples of how it is currently implemented:
>> - native 3840x2160, crop 3840x2160, fmt 3840x2160 = don't crop,  1:1 bin
>> - native 3840x2160, crop 1920x1080, fmt 1920x1080 = crop to 50%, 1:1 bin
>> - native 3840x2160, crop 3840x2160, fmt 1920x1080 = don't crop,  2:1 bin
>> - native 3840x2160, crop 1920x1080, fmt  960x 540 = crop to 50%, 2:1 bin
>>
>> Can you provide similar examples of how it would work when setting CROP
>> + COMPOSE, and without setting the format?
>>
>> Regards,

Thanks,
-- 
Luca