V4L2 subdev and sensor control changes, SMIA++ driver and N9 camera board code

From: Sakari Ailus <[email protected]>

Create a new control type called V4L2_CTRL_TYPE_INTEGER_MENU. Integer menu controls are just like menu controls but the menu items are 64-bit integers rather than strings.

drivers/media/video/v4l2-ctrls.c +3 -1

-	if (ctrl->qmenu == NULL ||
+	if ((ctrl->type == V4L2_CTRL_TYPE_MENU && ctrl->qmenu == NULL) ||
+	    (ctrl->type == V4L2_CTRL_TYPE_INTEGER_MENU
+	     && ctrl->qmenu_int == NULL) ||
 	    i < ctrl->minimum || i > ctrl->maximum)
 		return -EINVAL;
 	/* Use mask to see if this menu item should be skipped */
 	if (ctrl->menu_skip_mask & (1 << i))

drivers/media/video/v4l2-ctrls.c +2 -0

+		qm->value = ctrl->qmenu_int[i];
+	}
 	return 0;
 }

EXPORT_SYMBOL(v4l2_querymenu);

Hi Sakari,

Thanks for the patch.

Quoted 5 lines

From: Sakari Ailus <[email protected]>

Create a new control type called V4L2_CTRL_TYPE_INTEGER_MENU. Integer menu controls are just like menu controls but the menu items are 64-bit integers rather than strings.

[snip]

Quoted 8 lines

(quoted diff) +0 -0

b/drivers/media/video/v4l2-ctrls.c index 0f415da..083bb79 100644

drivers/media/video/v4l2-ctrls.c +0 -0

@@ -1775,16 +1797,22 @@ int v4l2_querymenu(struct v4l2_ctrl_handler *hdl,
 struct v4l2_querymenu *qm)
 

drivers/media/video/v4l2-ctrls.c int v4l2_querymenu(struct v4l2_ctrl_handler *hdl, struct v4l2_querymenu *qm) · line 1799

Quoted 2 lines

drivers/media/video/v4l2-ctrls.c +0 -0

 	qm->reserved = 0;
 	/* Sanity checks */

You should return -EINVAL here if the control is not of a menu type. It was done implictly before by the ctrl->qmenu == NULL check, but that's now conditioned to the control type being V4L2_CTRL_TYPE_MENU.

Quoted 12 lines

drivers/media/video/v4l2-ctrls.c +3 -3

-	if (ctrl->qmenu == NULL ||
+	if ((ctrl->type == V4L2_CTRL_TYPE_MENU && ctrl->qmenu == NULL) ||
+	    (ctrl->type == V4L2_CTRL_TYPE_INTEGER_MENU
+	     && ctrl->qmenu_int == NULL) ||
 	    i < ctrl->minimum || i > ctrl->maximum)
 		return -EINVAL;
 	/* Use mask to see if this menu item should be skipped */
 	if (ctrl->menu_skip_mask & (1 << i))
 		return -EINVAL;
 	/* Empty menu items should also be skipped */
-	if (ctrl->qmenu[i] == NULL || ctrl->qmenu[i][0] == '\0')
-		return -EINVAL;

drivers/media/video/v4l2-ctrls.c int v4l2_querymenu(struct v4l2_ctrl_handler *hdl, struct v4l2_querymenu *qm) · line 1813

Quoted 6 lines

drivers/media/video/v4l2-ctrls.c +5 -1

-	strlcpy(qm->name, ctrl->qmenu[i], sizeof(qm->name));
+	if (ctrl->type == V4L2_CTRL_TYPE_MENU) {
+		if (ctrl->qmenu[i] == NULL || ctrl->qmenu[i][0] == '\0')
+			return -EINVAL;
+		strlcpy(qm->name, ctrl->qmenu[i], sizeof(qm->name));
+	} else if (ctrl->type == V4L2_CTRL_TYPE_INTEGER_MENU) {

And you can then replace the else if by an else.

Quoted 5 lines

drivers/media/video/v4l2-ctrls.c +2 -0

+		qm->value = ctrl->qmenu_int[i];
+	}
 	return 0;
 }

EXPORT_SYMBOL(v4l2_querymenu);

From: Sakari Ailus <[email protected]>

Signed-off-by Sakari Ailus <[email protected]>

---
 Documentation/DocBook/media/v4l/compat.xml         |   10 +++++
 Documentation/DocBook/media/v4l/v4l2.xml           |    7 ++++
 .../DocBook/media/v4l/vidioc-queryctrl.xml         |   39

+++++++++++++++++++- 3 files changed, 54 insertions(+), 2 deletions(-)

(quoted diff) +0 -0

b/Documentation/DocBook/media/v4l/compat.xml index b68698f..569efd1 100644

Documentation/DocBook/media/v4l/compat.xml +0 -0

@@ -2379,6 +2379,16 @@ that used it. It was originally scheduled for

removal in 2.6.35. </orderedlist>

     </section>

Documentation/DocBook/media/v4l/compat.xml +8 -0

+      <orderedlist>
+        <listitem>
+	  <para>Added integer menus, the new type will be
+	  V4L2_CTRL_TYPE_INTEGER_MENU.</para>
+        </listitem>
+      </orderedlist>
+    </section>
+
     <section id="other">
       <title>Relation of V4L2 to other Linux multimedia APIs</title>

Hi Sakari,

Thanks for the patch.

Quoted 17 lines · On Tuesday 20 December 2011 21:27:54 Sakari Ailus wrote:

From: Sakari Ailus <[email protected]>

Signed-off-by Sakari Ailus <[email protected]>

---
 Documentation/DocBook/media/v4l/compat.xml         |   10 +++++
 Documentation/DocBook/media/v4l/v4l2.xml           |    7 ++++
 .../DocBook/media/v4l/vidioc-queryctrl.xml         |   39

+++++++++++++++++++- 3 files changed, 54 insertions(+), 2 deletions(-)

(quoted diff) +0 -0

b/Documentation/DocBook/media/v4l/compat.xml index b68698f..569efd1 100644

Documentation/DocBook/media/v4l/compat.xml +0 -0

@@ -2379,6 +2379,16 @@ that used it. It was originally scheduled for

removal in 2.6.35. </orderedlist>

     </section>

Documentation/DocBook/media/v4l/compat.xml that used it. It was originally scheduled for removal in 2.6.35. · line 2383

Quoted 2 lines

Documentation/DocBook/media/v4l/compat.xml +2 -0

+    <section>
+      <title>V4L2 in Linux 3.3</title>

Seems it will be for 3.4 :-) Same for Documentation/DocBook/media/v4l/v4l2.xml

From: Sakari Ailus <[email protected]>

Add an integer menu test control for the vivi driver.

Signed-off-by Sakari Ailus <[email protected]>

---
 drivers/media/video/vivi.c |   21 +++++++++++++++++++++
 1 files changed, 21 insertions(+), 0 deletions(-)

drivers/media/video/vivi.c +1 -0

index 7d754fb..763ec23 100644
@@ -177,6 +177,7 @@ struct vivi_dev {
 	struct v4l2_ctrl	   *menu;
 	struct v4l2_ctrl	   *string;
 	struct v4l2_ctrl	   *bitmask;
+	struct v4l2_ctrl	   *int_menu;
 
 	spinlock_t                 slock;
 	struct mutex		   mutex;

drivers/media/video/vivi.c +2 -0

+	gen_text(dev, vbuf, line++ * 16, 16, str);
 	mutex_unlock(&dev->ctrl_handler.lock);
 	gen_text(dev, vbuf, line++ * 16, 16, str);
 	if (dev->button_pressed) {

@@ -1183,6 +1188,22 @@ static const struct v4l2_ctrl_config
 vivi_ctrl_bitmask = { .step = 0,
 };
 
+static const s64 * const vivi_ctrl_int_menu_values[] = {

drivers/media/video/vivi.c +5 -0

+	.def = 4,
+	.menu_skip_mask = 0x02,
+	.qmenu_int = &vivi_ctrl_int_menu_values,
+};
+
 static const struct v4l2_file_operations vivi_fops = {
 	.owner		= THIS_MODULE,
 	.open           = v4l2_fh_open,

Hi Sakari,

Thanks for the patch.

Quoted 19 lines

From: Sakari Ailus <[email protected]>

Add an integer menu test control for the vivi driver.

Signed-off-by Sakari Ailus <[email protected]>

---
 drivers/media/video/vivi.c |   21 +++++++++++++++++++++
 1 files changed, 21 insertions(+), 0 deletions(-)

drivers/media/video/vivi.c +1 -0

index 7d754fb..763ec23 100644
@@ -177,6 +177,7 @@ struct vivi_dev {
 	struct v4l2_ctrl	   *menu;
 	struct v4l2_ctrl	   *string;
 	struct v4l2_ctrl	   *bitmask;
+	struct v4l2_ctrl	   *int_menu;
 

drivers/media/video/vivi.c static void vivi_fillbuff(struct vivi_dev *dev, struct · line 508

Quoted 8 lines

drivers/media/video/vivi.c +2 -0

 	spinlock_t                 slock;
 	struct mutex		   mutex;

@@ -503,6 +504,10 @@ static void vivi_fillbuff(struct vivi_dev *dev, struct
 vivi_buffer *buf) dev->boolean->cur.val,
 			dev->menu->qmenu[dev->menu->cur.val],
 			dev->string->cur.string);
+	snprintf(str, sizeof(str), " integer_menu %s, value %lld ",
+			dev->int_menu->qmenu[dev->int_menu->cur.val],

Shouldn't you print the content of qmenu_int as a 64-bit integer instead ?

drivers/media/video/vivi.c static void vivi_fillbuff(struct vivi_dev *dev, struct vivi_buffer *buf) · line 509

Quoted 1 line

drivers/media/video/vivi.c +1 -0

+			dev->int64->cur.val64);

Shouldn't this be dev->int_menu->cur.val ?

Quoted 8 lines

drivers/media/video/vivi.c +1 -0

+	gen_text(dev, vbuf, line++ * 16, 16, str);
 	mutex_unlock(&dev->ctrl_handler.lock);
 	gen_text(dev, vbuf, line++ * 16, 16, str);
 	if (dev->button_pressed) {

@@ -1183,6 +1188,22 @@ static const struct v4l2_ctrl_config
 vivi_ctrl_bitmask = { .step = 0,
 };
 

drivers/media/video/vivi.c static const struct v4l2_ctrl_config vivi_ctrl_bitmask = { · line 1201

Quoted 11 lines

drivers/media/video/vivi.c +11 -0

+static const s64 * const vivi_ctrl_int_menu_values[] = {
+	1, 1, 2, 3, 5, 8, 13, 21, 42,
+};
+
+static const struct v4l2_ctrl_config vivi_ctrl_string = {
+	.ops = &vivi_ctrl_ops,
+	.id = VIDI_CID_CUSTOM_BASE + 7
+	.name = "Integer menu",
+	.type = V4L2_CTRL_TYPE_INTEGER_MENU,
+	.min = 1,
+	.max = 8,

There are 9 values in your vivi_ctrl_int_menu_values array. Is 8 on purpose here ?

Quoted 8 lines

drivers/media/video/vivi.c +5 -0

+	.def = 4,
+	.menu_skip_mask = 0x02,
+	.qmenu_int = &vivi_ctrl_int_menu_values,
+};
+
 static const struct v4l2_file_operations vivi_fops = {
 	.owner		= THIS_MODULE,
 	.open           = v4l2_fh_open,

I put it there to limit the maximum to 8 instead of 9, but 9 would be equally good. I'll change it.

Sakari Ailus wrote:
...

Quoted 2 lines

I put it there to limit the maximum to 8 instead of 9, but 9 would be equally good. I'll change it.

Or not. 8 is still the index of the last value. min is one to start the menu from the second item. Would you like that to be changed to zero?

From: Sakari Ailus <[email protected]>

Add support for VIDIOC_SUBDEV_S_SELECTION and VIDIOC_SUBDEV_G_SELECTION IOCTLs. They replace functionality provided by VIDIOC_SUBDEV_S_CROP and VIDIOC_SUBDEV_G_CROP IOCTLs and also add new functionality (composing).

VIDIOC_SUBDEV_G_CROP and VIDIOC_SUBDEV_S_CROP continue to be supported.

Signed-off-by Sakari Ailus <[email protected]>

---
 drivers/media/video/v4l2-subdev.c |   26 ++++++++++++++++++++-
 include/linux/v4l2-subdev.h       |   45 ++++++++++++++++++++++++++++++++++
 include/media/v4l2-subdev.h       |    5 ++++
 3 files changed, 75 insertions(+), 1 deletions(-)

(quoted diff) +0 -0

b/drivers/media/video/v4l2-subdev.c index 65ade5f..e8ae098 100644

drivers/media/video/v4l2-subdev.c +3 -0

 	if (fh->try_fmt == NULL)
 		return -ENOMEM;
 
 	fh->try_crop = (struct v4l2_rect *)
 		(fh->try_fmt + sd->entity.num_pads);
+
+	fh->try_compose = (struct v4l2_rect *)
+		(fh->try_crop + sd->entity.num_pads);
 #endif
 	return 0;
 }

drivers/media/video/v4l2-subdev.c +6 -0

+		if (sel->pad >= sd->entity.num_pads)
+			return -EINVAL;
+
+		return v4l2_subdev_call(
+			sd, pad, set_selection, subdev_fh, sel);
+	}
 #endif
 	default:
 		return v4l2_subdev_call(sd, core, ioctl, cmd, arg);

include/linux/v4l2-subdev.h +27 -0

index ed29cbb..d53d775 100644
@@ -123,6 +123,47 @@ struct v4l2_subdev_frame_interval_enum {
 	__u32 reserved[9];
 };
 
+#define V4L2_SUBDEV_SEL_FLAG_SIZE_GE			(1 << 0)
+#define V4L2_SUBDEV_SEL_FLAG_SIZE_LE			(1 << 1)
+#define V4L2_SUBDEV_SEL_FLAG_KEEP_CONFIG		(1 << 2)
+
+/* active cropping area */
+#define V4L2_SUBDEV_SEL_TGT_CROP_ACTIVE			0
+/* default cropping area */
+#define V4L2_SUBDEV_SEL_TGT_CROP_DEFAULT		1
+/* cropping bounds */
+#define V4L2_SUBDEV_SEL_TGT_CROP_BOUNDS			2
+/* current composing area */
+#define V4L2_SUBDEV_SEL_TGT_COMPOSE_ACTIVE		256
+/* default composing area */
+#define V4L2_SUBDEV_SEL_TGT_COMPOSE_DEFAULT		257
+/* composing bounds */
+#define V4L2_SUBDEV_SEL_TGT_COMPOSE_BOUNDS		258
+
+
+/**
+ * struct v4l2_subdev_selection - selection info
+ *
+ * @which: either V4L2_SUBDEV_FORMAT_ACTIVE or V4L2_SUBDEV_FORMAT_TRY
+ * @pad: pad number, as reported by the media API
+ * @target: selection target, used to choose one of possible rectangles
+ * @flags: constraints flags
+ * @r: coordinates of selection window
+ * @reserved: for future use, rounds structure size to 64 bytes, set to

zero + *
+ * Hardware may use multiple helper window to process a video stream.
+ * The structure is used to exchange this selection areas between
+ * an application and a driver.
+ */

include/linux/v4l2-subdev.h +13 -0

+struct v4l2_subdev_selection {
+	__u32 which;
+	__u32 pad;
+	__u32 target;
+	__u32 flags;
+	struct v4l2_rect r;
+	__u32 reserved[8];
+};
+
 #define VIDIOC_SUBDEV_G_FMT	_IOWR('V',  4, struct v4l2_subdev_format)
 #define VIDIOC_SUBDEV_S_FMT	_IOWR('V',  5, struct v4l2_subdev_format)
 #define VIDIOC_SUBDEV_G_FRAME_INTERVAL \

@@ -137,5 +178,9 @@ struct v4l2_subdev_frame_interval_enum {
 			_IOWR('V', 75, struct v4l2_subdev_frame_interval_enum)
 #define VIDIOC_SUBDEV_G_CROP	_IOWR('V', 59, struct v4l2_subdev_crop)
 #define VIDIOC_SUBDEV_S_CROP	_IOWR('V', 60, struct v4l2_subdev_crop)
+#define VIDIOC_SUBDEV_G_SELECTION \
+	_IOWR('V', 61, struct v4l2_subdev_selection)
+#define VIDIOC_SUBDEV_S_SELECTION \
+	_IOWR('V', 62, struct v4l2_subdev_selection)
 
 #endif

include/media/v4l2-subdev.h +5 -0

index f0f3358..26eeaa4 100644
@@ -466,6 +466,10 @@ struct v4l2_subdev_pad_ops {
 		       struct v4l2_subdev_crop *crop);
 	int (*get_crop)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
 		       struct v4l2_subdev_crop *crop);
+	int (*get_selection)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
+			     struct v4l2_subdev_selection *sel);
+	int (*set_selection)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
+			     struct v4l2_subdev_selection *sel);
 };
 
 struct v4l2_subdev_ops {

@@ -551,6 +555,7 @@ struct v4l2_subdev_fh {
 #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
 	struct v4l2_mbus_framefmt *try_fmt;
 	struct v4l2_rect *try_crop;
+	struct v4l2_rect *try_compose;
 #endif
 };

Quoted 7 lines · On Tuesday 20 December 2011 21:27:56 Sakari Ailus wrote:

From: Sakari Ailus <[email protected]>

Add support for VIDIOC_SUBDEV_S_SELECTION and VIDIOC_SUBDEV_G_SELECTION IOCTLs. They replace functionality provided by VIDIOC_SUBDEV_S_CROP and VIDIOC_SUBDEV_G_CROP IOCTLs and also add new functionality (composing).

VIDIOC_SUBDEV_G_CROP and VIDIOC_SUBDEV_S_CROP continue to be supported.

As those ioctls are experimental, should we deprecate them ?

Quoted 12 lines

Signed-off-by Sakari Ailus <[email protected]>

---
 drivers/media/video/v4l2-subdev.c |   26 ++++++++++++++++++++-
 include/linux/v4l2-subdev.h       |   45 ++++++++++++++++++++++++++++++++++
 include/media/v4l2-subdev.h       |    5 ++++
 3 files changed, 75 insertions(+), 1 deletions(-)

(quoted diff) +0 -0

b/drivers/media/video/v4l2-subdev.c index 65ade5f..e8ae098 100644

drivers/media/video/v4l2-subdev.c static int subdev_fh_init(struct v4l2_subdev_fh *fh, · line 41

Quoted 7 lines

drivers/media/video/v4l2-subdev.c +2 -1

@@ -36,13 +36,17 @@ static int subdev_fh_init(struct v4l2_subdev_fh *fh,
 struct v4l2_subdev *sd) {
 #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
 	/* Allocate try format and crop in the same memory block */
-	fh->try_fmt = kzalloc((sizeof(*fh->try_fmt) + sizeof(*fh->try_crop))
+	fh->try_fmt = kzalloc((sizeof(*fh->try_fmt) + sizeof(*fh->try_crop)
+			       + sizeof(*fh->try_compose))
 			      * sd->entity.num_pads, GFP_KERNEL);

Could you check how the 3 structures are aligned on 64-bit platforms ? I'm a bit worried about the compiler expecting a 64-bit alignment for one of them, and getting only a 32-bit alignment in the end.

What about using kcalloc ?

Quoted 12 lines

drivers/media/video/v4l2-subdev.c +3 -0

 	if (fh->try_fmt == NULL)
 		return -ENOMEM;
 
 	fh->try_crop = (struct v4l2_rect *)
 		(fh->try_fmt + sd->entity.num_pads);
+
+	fh->try_compose = (struct v4l2_rect *)
+		(fh->try_crop + sd->entity.num_pads);
 #endif
 	return 0;
 }

@@ -281,6 +285,26 @@ static long subdev_do_ioctl(struct file *file,

drivers/media/video/v4l2-subdev.c static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) · line 290

Quoted 6 lines

drivers/media/video/v4l2-subdev.c +3 -0

 unsigned int cmd, void *arg) return v4l2_subdev_call(sd, pad,
 enum_frame_interval, subdev_fh, fie);
 	}
+
+	case VIDIOC_SUBDEV_G_SELECTION: {
+		struct v4l2_subdev_selection *sel = arg;

Shouldn't you check sel->which ?

drivers/media/video/v4l2-subdev.c static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) · line 300

Quoted 9 lines

drivers/media/video/v4l2-subdev.c +9 -0

+		if (sel->pad >= sd->entity.num_pads)
+			return -EINVAL;
+
+		return v4l2_subdev_call(
+			sd, pad, get_selection, subdev_fh, sel);
+	}
+
+	case VIDIOC_SUBDEV_S_SELECTION: {
+		struct v4l2_subdev_selection *sel = arg;

And here too ?

Quoted 93 lines

drivers/media/video/v4l2-subdev.c +6 -0

+		if (sel->pad >= sd->entity.num_pads)
+			return -EINVAL;
+
+		return v4l2_subdev_call(
+			sd, pad, set_selection, subdev_fh, sel);
+	}
 #endif
 	default:
 		return v4l2_subdev_call(sd, core, ioctl, cmd, arg);

include/linux/v4l2-subdev.h +27 -0

index ed29cbb..d53d775 100644
@@ -123,6 +123,47 @@ struct v4l2_subdev_frame_interval_enum {
 	__u32 reserved[9];
 };
 
+#define V4L2_SUBDEV_SEL_FLAG_SIZE_GE			(1 << 0)
+#define V4L2_SUBDEV_SEL_FLAG_SIZE_LE			(1 << 1)
+#define V4L2_SUBDEV_SEL_FLAG_KEEP_CONFIG		(1 << 2)
+
+/* active cropping area */
+#define V4L2_SUBDEV_SEL_TGT_CROP_ACTIVE			0
+/* default cropping area */
+#define V4L2_SUBDEV_SEL_TGT_CROP_DEFAULT		1
+/* cropping bounds */
+#define V4L2_SUBDEV_SEL_TGT_CROP_BOUNDS			2
+/* current composing area */
+#define V4L2_SUBDEV_SEL_TGT_COMPOSE_ACTIVE		256
+/* default composing area */
+#define V4L2_SUBDEV_SEL_TGT_COMPOSE_DEFAULT		257
+/* composing bounds */
+#define V4L2_SUBDEV_SEL_TGT_COMPOSE_BOUNDS		258
+
+
+/**
+ * struct v4l2_subdev_selection - selection info
+ *
+ * @which: either V4L2_SUBDEV_FORMAT_ACTIVE or V4L2_SUBDEV_FORMAT_TRY
+ * @pad: pad number, as reported by the media API
+ * @target: selection target, used to choose one of possible rectangles
+ * @flags: constraints flags
+ * @r: coordinates of selection window
+ * @reserved: for future use, rounds structure size to 64 bytes, set to

zero + *
+ * Hardware may use multiple helper window to process a video stream.
+ * The structure is used to exchange this selection areas between
+ * an application and a driver.
+ */

include/linux/v4l2-subdev.h +13 -0

+struct v4l2_subdev_selection {
+	__u32 which;
+	__u32 pad;
+	__u32 target;
+	__u32 flags;
+	struct v4l2_rect r;
+	__u32 reserved[8];
+};
+
 #define VIDIOC_SUBDEV_G_FMT	_IOWR('V',  4, struct v4l2_subdev_format)
 #define VIDIOC_SUBDEV_S_FMT	_IOWR('V',  5, struct v4l2_subdev_format)
 #define VIDIOC_SUBDEV_G_FRAME_INTERVAL \

@@ -137,5 +178,9 @@ struct v4l2_subdev_frame_interval_enum {
 			_IOWR('V', 75, struct v4l2_subdev_frame_interval_enum)
 #define VIDIOC_SUBDEV_G_CROP	_IOWR('V', 59, struct v4l2_subdev_crop)
 #define VIDIOC_SUBDEV_S_CROP	_IOWR('V', 60, struct v4l2_subdev_crop)
+#define VIDIOC_SUBDEV_G_SELECTION \
+	_IOWR('V', 61, struct v4l2_subdev_selection)
+#define VIDIOC_SUBDEV_S_SELECTION \
+	_IOWR('V', 62, struct v4l2_subdev_selection)
 
 #endif

include/media/v4l2-subdev.h +5 -0

index f0f3358..26eeaa4 100644
@@ -466,6 +466,10 @@ struct v4l2_subdev_pad_ops {
 		       struct v4l2_subdev_crop *crop);
 	int (*get_crop)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
 		       struct v4l2_subdev_crop *crop);
+	int (*get_selection)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
+			     struct v4l2_subdev_selection *sel);
+	int (*set_selection)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
+			     struct v4l2_subdev_selection *sel);
 };
 
 struct v4l2_subdev_ops {

@@ -551,6 +555,7 @@ struct v4l2_subdev_fh {
 #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
 	struct v4l2_mbus_framefmt *try_fmt;
 	struct v4l2_rect *try_crop;
+	struct v4l2_rect *try_compose;
 #endif
 };

Quoted 10 lines · Laurent Pinchart

Quoted 7 lines · On Tuesday 20 December 2011 21:27:56 Sakari Ailus wrote:

From: Sakari Ailus <[email protected]>

Add support for VIDIOC_SUBDEV_S_SELECTION and VIDIOC_SUBDEV_G_SELECTION IOCTLs. They replace functionality provided by VIDIOC_SUBDEV_S_CROP and VIDIOC_SUBDEV_G_CROP IOCTLs and also add new functionality (composing).

VIDIOC_SUBDEV_G_CROP and VIDIOC_SUBDEV_S_CROP continue to be supported.

As those ioctls are experimental, should we deprecate them ?

I'm also in favour of doing that. But I'll make it a separate patch.

Quoted 30 lines

Quoted 15 lines

Signed-off-by Sakari Ailus <[email protected]>

---

 drivers/media/video/v4l2-subdev.c |   26 ++++++++++++++++++++-
 include/linux/v4l2-subdev.h       |   45
 ++++++++++++++++++++++++++++++++++ include/media/v4l2-subdev.h       |
    5 ++++
 3 files changed, 75 insertions(+), 1 deletions(-)

(quoted diff) +0 -0

b/drivers/media/video/v4l2-subdev.c index 65ade5f..e8ae098 100644

drivers/media/video/v4l2-subdev.c +0 -0

@@ -36,13 +36,17 @@ static int subdev_fh_init(struct v4l2_subdev_fh *fh,
 struct v4l2_subdev *sd) {

drivers/media/video/v4l2-subdev.c static int subdev_fh_init(struct v4l2_subdev_fh *fh, · line 45

Quoted 10 lines

drivers/media/video/v4l2-subdev.c +2 -1

 
 #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
 
 	/* Allocate try format and crop in the same memory block */
 
-	fh->try_fmt = kzalloc((sizeof(*fh->try_fmt) + sizeof(*fh->try_crop))
+	fh->try_fmt = kzalloc((sizeof(*fh->try_fmt) + sizeof(*fh->try_crop)
+			       + sizeof(*fh->try_compose))
 
 			      * sd->entity.num_pads, GFP_KERNEL);

Could you check how the 3 structures are aligned on 64-bit platforms ? I'm a bit worried about the compiler expecting a 64-bit alignment for one of them, and getting only a 32-bit alignment in the end.

Quoted 1 line

What about using kcalloc ?

kcalloc won't make a difference --- see the implementation. Do you think this is really an issue in practice?

If we want to ensure alignment I'd just allocate them separately. Or create a struct out of them locally, and get the pointers from that struct --- then the alignment would be the same as if those were part of a single struct. That achieves the desired result and also keeps error handling trivial.

I wouldn't want to start relying on the alignment based on the sizes of these structures.

Hi Sakari,

Quoted 44 lines · On Friday 06 January 2012 12:27:03 Sakari Ailus wrote:

Quoted 10 lines · Laurent Pinchart

Quoted 7 lines · On Tuesday 20 December 2011 21:27:56 Sakari Ailus wrote:

From: Sakari Ailus <[email protected]>

Add support for VIDIOC_SUBDEV_S_SELECTION and VIDIOC_SUBDEV_G_SELECTION IOCTLs. They replace functionality provided by VIDIOC_SUBDEV_S_CROP and VIDIOC_SUBDEV_G_CROP IOCTLs and also add new functionality (composing).

VIDIOC_SUBDEV_G_CROP and VIDIOC_SUBDEV_S_CROP continue to be supported.

As those ioctls are experimental, should we deprecate them ?

I'm also in favour of doing that. But I'll make it a separate patch.

Quoted 30 lines

Quoted 15 lines

Signed-off-by Sakari Ailus <[email protected]>

---

 drivers/media/video/v4l2-subdev.c |   26 ++++++++++++++++++++-
 include/linux/v4l2-subdev.h       |   45
 ++++++++++++++++++++++++++++++++++ include/media/v4l2-subdev.h       |
    5 ++++
 3 files changed, 75 insertions(+), 1 deletions(-)

(quoted diff) +0 -0

b/drivers/media/video/v4l2-subdev.c index 65ade5f..e8ae098 100644

drivers/media/video/v4l2-subdev.c +0 -0

@@ -36,13 +36,17 @@ static int subdev_fh_init(struct v4l2_subdev_fh *fh,
 struct v4l2_subdev *sd) {

drivers/media/video/v4l2-subdev.c static int subdev_fh_init(struct v4l2_subdev_fh *fh, · line 45

Quoted 10 lines

drivers/media/video/v4l2-subdev.c +2 -1

 
 #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
 
 	/* Allocate try format and crop in the same memory block */
 
-	fh->try_fmt = kzalloc((sizeof(*fh->try_fmt) + sizeof(*fh->try_crop))
+	fh->try_fmt = kzalloc((sizeof(*fh->try_fmt) + sizeof(*fh->try_crop)
+			       + sizeof(*fh->try_compose))
 
 			      * sd->entity.num_pads, GFP_KERNEL);

Could you check how the 3 structures are aligned on 64-bit platforms ? I'm a bit worried about the compiler expecting a 64-bit alignment for one of them, and getting only a 32-bit alignment in the end.

Quoted 4 lines

Quoted 1 line

What about using kcalloc ?

kcalloc won't make a difference --- see the implementation. Do you think this is really an issue in practice?

It won't make a difference for the alignment, it's just that we allocate an array, so kcalloc seemed right.

Quoted 6 lines

If we want to ensure alignment I'd just allocate them separately. Or create a struct out of them locally, and get the pointers from that struct --- then the alignment would be the same as if those were part of a single struct. That achieves the desired result and also keeps error handling trivial.

Quoted 2 lines

I wouldn't want to start relying on the alignment based on the sizes of these structures.

Sounds good to me. Allocating them as part of a bigger structure internally could be more efficient than separate allocations, but I'm fine with both.

On second thought, I think I'll combine them into a new anonymous struct the field name of which I call "pad", unless that requires too intrusive changes in other drivers. How about that?

Sakari Ailus wrote:
...

Quoted 3 lines

On second thought, I think I'll combine them into a new anonymous struct the field name of which I call "pad", unless that requires too intrusive changes in other drivers. How about that?

And the answer to that is "no". The smia++ driver does store the format, crop and compose values in arrays indexed by pad numbers which I think is a natural thing for the driver to do. In many functiona the driver uses internally it's trivial to choose the array either from driver's internal data structure (V4L2_SUBDEV_FORMAT_ACTIVE) or the file handle (V4L2_SUBDEV_FORMAT_TRY).

Alternatively a named struct could be created for the same, but the drivers might not need all the fields at all, or choose to store them in a different form.

Hi Sakari,

Quoted 14 lines · On Saturday 07 January 2012 12:09:29 Sakari Ailus wrote:

Sakari Ailus wrote:
...

Quoted 3 lines

On second thought, I think I'll combine them into a new anonymous struct the field name of which I call "pad", unless that requires too intrusive changes in other drivers. How about that?

And the answer to that is "no". The smia++ driver does store the format, crop and compose values in arrays indexed by pad numbers which I think is a natural thing for the driver to do. In many functiona the driver uses internally it's trivial to choose the array either from driver's internal data structure (V4L2_SUBDEV_FORMAT_ACTIVE) or the file handle (V4L2_SUBDEV_FORMAT_TRY).

Quoted 3 lines

Alternatively a named struct could be created for the same, but the drivers might not need all the fields at all, or choose to store them in a different form.

Drivers should use the v4l2_subdev_get_try_format(), v4l2_subdev_get_try_crop() and v4l2_subdev_get_try_compose() functions to access TRY formats and selection rectangles on file handles, so they shouldn't care about the allocation details.

From: Sakari Ailus <[email protected]>

Revert to s_selection if s_crop isn't implemented by a driver. Same for g_selection / g_crop.

Signed-off-by Sakari Ailus <[email protected]>

---
 drivers/media/video/v4l2-subdev.c |   37

+++++++++++++++++++++++++++++++++++-- 1 files changed, 35 insertions(+), 2 deletions(-)

(quoted diff) +0 -0

b/drivers/media/video/v4l2-subdev.c index e8ae098..f8de551 100644

drivers/media/video/v4l2-subdev.c +2 -0

@@ -226,6 +226,8 @@ static long subdev_do_ioctl(struct file *file, unsigned
 int cmd, void *arg)
 
 	case VIDIOC_SUBDEV_G_CROP: {
 		struct v4l2_subdev_crop *crop = arg;
+		struct v4l2_subdev_selection sel;
+		int rval;
 
 		if (crop->which != V4L2_SUBDEV_FORMAT_TRY &&

drivers/media/video/v4l2-subdev.c +0 -0

 		    crop->which != V4L2_SUBDEV_FORMAT_ACTIVE)

drivers/media/video/v4l2-subdev.c +11 -0

+		sel.pad = crop->pad;
+		sel.target = V4L2_SUBDEV_SEL_TGT_CROP_ACTIVE;
+
+		rval = v4l2_subdev_call(
+			sd, pad, get_selection, subdev_fh, &sel);
+
+		crop->rect = sel.r;
+
+		return rval;
 	}
 
 	case VIDIOC_SUBDEV_S_CROP: {
 		struct v4l2_subdev_crop *crop = arg;
+		struct v4l2_subdev_selection sel;
+		int rval;
 
 		if (crop->which != V4L2_SUBDEV_FORMAT_TRY &&
 		    crop->which != V4L2_SUBDEV_FORMAT_ACTIVE)

drivers/media/video/v4l2-subdev.c +10 -0

+		sel.pad = crop->pad;
+		sel.target = V4L2_SUBDEV_SEL_TGT_CROP_ACTIVE;
+		sel.r = crop->rect;
+
+		rval = v4l2_subdev_call(
+			sd, pad, set_selection, subdev_fh, &sel);
+
+		crop->rect = sel.r;
+
+		return rval;
 	}
 
 	case VIDIOC_SUBDEV_ENUM_MBUS_CODE: {

Hi Sakari,

Thanks for the patch.

Quoted 4 lines · On Tuesday 20 December 2011 21:27:57 Sakari Ailus wrote:

From: Sakari Ailus <[email protected]>

Revert to s_selection if s_crop isn't implemented by a driver. Same for g_selection / g_crop.

Shouldn't this say "Fall back" instead of "Revert" ?

Quoted 21 lines

Signed-off-by Sakari Ailus <[email protected]>

---
 drivers/media/video/v4l2-subdev.c |   37

+++++++++++++++++++++++++++++++++++-- 1 files changed, 35 insertions(+), 2 deletions(-)

(quoted diff) +0 -0

b/drivers/media/video/v4l2-subdev.c index e8ae098..f8de551 100644

drivers/media/video/v4l2-subdev.c +2 -0

@@ -226,6 +226,8 @@ static long subdev_do_ioctl(struct file *file, unsigned
 int cmd, void *arg)
 
 	case VIDIOC_SUBDEV_G_CROP: {
 		struct v4l2_subdev_crop *crop = arg;
+		struct v4l2_subdev_selection sel;
+		int rval;
 
 		if (crop->which != V4L2_SUBDEV_FORMAT_TRY &&

drivers/media/video/v4l2-subdev.c +0 -0

 		    crop->which != V4L2_SUBDEV_FORMAT_ACTIVE)

drivers/media/video/v4l2-subdev.c static long subdev_do_ioctl(struct file *file, · line 244

Quoted 10 lines

drivers/media/video/v4l2-subdev.c +6 -1

@@ -234,11 +236,27 @@ static long subdev_do_ioctl(struct file *file,
 unsigned int cmd, void *arg) if (crop->pad >= sd->entity.num_pads)
 			return -EINVAL;
 
-		return v4l2_subdev_call(sd, pad, get_crop, subdev_fh, crop);
+		rval = v4l2_subdev_call(sd, pad, get_crop, subdev_fh, crop);
+		if (rval != -ENOIOCTLCMD)
+			return rval;
+
+		memset(&sel, 0, sizeof(sel));
+		sel.which = V4L2_SUBDEV_FORMAT_ACTIVE;

Shouldn't sel.which be set to crop->which ?

Quoted 19 lines

drivers/media/video/v4l2-subdev.c +11 -0

+		sel.pad = crop->pad;
+		sel.target = V4L2_SUBDEV_SEL_TGT_CROP_ACTIVE;
+
+		rval = v4l2_subdev_call(
+			sd, pad, get_selection, subdev_fh, &sel);
+
+		crop->rect = sel.r;
+
+		return rval;
 	}
 
 	case VIDIOC_SUBDEV_S_CROP: {
 		struct v4l2_subdev_crop *crop = arg;
+		struct v4l2_subdev_selection sel;
+		int rval;
 
 		if (crop->which != V4L2_SUBDEV_FORMAT_TRY &&
 		    crop->which != V4L2_SUBDEV_FORMAT_ACTIVE)

drivers/media/video/v4l2-subdev.c static long subdev_do_ioctl(struct file *file, · line 273

Quoted 10 lines

drivers/media/video/v4l2-subdev.c +6 -1

@@ -247,7 +265,22 @@ static long subdev_do_ioctl(struct file *file,
 unsigned int cmd, void *arg) if (crop->pad >= sd->entity.num_pads)
 			return -EINVAL;
 
-		return v4l2_subdev_call(sd, pad, set_crop, subdev_fh, crop);
+		rval = v4l2_subdev_call(sd, pad, set_crop, subdev_fh, crop);
+		if (rval != -ENOIOCTLCMD)
+			return rval;
+
+		memset(&sel, 0, sizeof(sel));
+		sel.which = V4L2_SUBDEV_FORMAT_ACTIVE;

Same here.

Quoted 13 lines

drivers/media/video/v4l2-subdev.c +10 -0

+		sel.pad = crop->pad;
+		sel.target = V4L2_SUBDEV_SEL_TGT_CROP_ACTIVE;
+		sel.r = crop->rect;
+
+		rval = v4l2_subdev_call(
+			sd, pad, set_selection, subdev_fh, &sel);
+
+		crop->rect = sel.r;
+
+		return rval;
 	}
 
 	case VIDIOC_SUBDEV_ENUM_MBUS_CODE: {

(quoted diff) +0 -0

b/Documentation/DocBook/media/v4l/dev-subdev.xml index 0916a73..722db60 100644

Documentation/DocBook/media/v4l/dev-subdev.xml +0 -0

Documentation/DocBook/media/v4l/dev-subdev.xml +0 -0

@@ -288,26 +288,81 @@

Documentation/DocBook/media/v4l/dev-subdev.xml +23 -0

+      <para>Scaling operation changes the size of the image by scaling
+      it to new dimensions. Some sub-devices support it. The scaled
+      size (width and height) is represented by &v4l2-rect;. In the
+      case of scaling, top and left will always be zero. Scaling is
+      configured using &sub-subdev-g-selection; and
+      <constant>V4L2_SUBDEV_SEL_COMPOSE_ACTIVE</constant> selection
+      target on the sink pad of the subdev. The scaling is performed
+      related to the width and height of the crop rectangle on the
+      subdev's sink pad.</para>
+
+      <para>As for pad formats, drivers store try and active
+      rectangles for the selection targets of ACTIVE type <xref
+      linkend="v4l2-subdev-selection-targets">.</xref></para>
+
+      <para>On sink pads, cropping is applied relatively to the
+      current pad format. The pad format represents the image size as
+      received by the sub-device from the previous block in the
+      pipeline, and the crop rectangle represents the sub-image that
+      will be transmitted further inside the sub-device for
+      processing.</para>
+
+      <para>On source pads, cropping is similar to sink pads, with the
+      exception that the source size from which the cropping is

Documentation/DocBook/media/v4l/dev-subdev.xml +25 -1

+      <orderedlist>
+	<listitem>Sink pad format. The user configures the sink pad
+	format. This format defines the parameters of the image the
+	entity receives through the pad for further processing.</listitem>
 
-      <para>Cropping behaviour on output pads is not defined.</para>
+	<listitem>Sink pad active crop selection. The sink pad crop
+	defines the performed to the sink pad format.</listitem>
 
+	<listitem>Sink pad active compose selection. The sink pad compose
+	rectangle defines the scaling ratio compared to the size of
+	the sink pad crop rectangle.</listitem>
+
+	<listitem>Source pad active crop selection. Crop on the source
+	pad defines crop performed to the image scaled according to
+	the sink pad compose rectangle.</listitem>
+
+	<listitem>Source pad active compose selection. The source pad
+	compose defines the size and location of the compose
+	rectangle.</listitem>
+
+	<listitem>Source pad format. The source pad format defines the
+	output pixel format of the subdev, as well as the other
+	parameters with the exception of the image width and
+	height.</listitem>
+
+      </orderedlist>
     </section>
+
   </section>
 
   &sub-subdev-formats;

(quoted diff) +0 -0

b/Documentation/DocBook/media/v4l/vidioc-subdev-g-selection.xml new file mode 100644
index 0000000..5fbcd65

Documentation/DocBook/media/v4l/vidioc-subdev-g-selection.xml new file +0 -0

@@ -0,0 +1,226 @@

Documentation/DocBook/media/v4l/vidioc-subdev-g-selection.xml new file +20 -0

+  <refsect1>
+    <title>Description</title>
+
+    <note>
+      <title>Experimental</title>
+      <para>This is an <link linkend="experimental">experimental</link>
+      interface and may change in the future.</para>
+    </note>
+
+    <para>The selections are used to configure various image
+    processing functionality performed by the subdevs which affect the
+    image size. This currently includes cropping, scaling and
+    composition.</para>
+
+    <para>The selections replace the crop API &sub-subdev-g-crop;. All
+    the function of the crop API, and more, are supported by the
+    selections API.</para>
+
+    <para>See <xref linkend="subdev">Sub-device interface</xref> for
+    more information on how each selection target affects the image

The PADDED target
+ provides the width and height for the padded image,

Documentation/DocBook/media/v4l/vidioc-subdev-g-selection.xml new file +25 -0

+    </section>
+
+    <table pgwide="1" frame="none" id="v4l2-subdev-selection-targets">
+      <title>V4L2 subdev selection targets</title>
+      <tgroup cols="3">
+        &cs-def;
+	<tbody valign="top">
+	  <row>
+	    <entry><constant>V4L2_SUBDEV_SEL_TGT_CROP_ACTIVE</constant></entry>
+	    <entry>0</entry>
+	    <entry>Active crop. Defines the cropping
+	    performed by the processing step.</entry>
+	  </row>
+	  <row>
+	    <entry><constant>V4L2_SUBDEV_SEL_TGT_CROP_DEFAULT</constant></entry>
+	    <entry>1</entry>
+	    <entry>Default crop rectangle.</entry>
+	  </row>
+	  <row>
+	    <entry><constant>V4L2_SUBDEV_SEL_TGT_CROP_BOUNDS</constant></entry>
+	    <entry>2</entry>
+	    <entry>Bounds of the crop rectangle.</entry>
+	  </row>
+	  <row>
+	   

<entry><constant>V4L2_SUBDEV_SEL_TGT_COMPOSE_ACTIVE</constant></entry> +
<entry>256</entry>

Documentation/DocBook/media/v4l/vidioc-subdev-g-selection.xml new file +5 -0

+	    <entry>Active compose rectangle. Used to configure scaling
+	    on sink pads and composition on source pads.</entry>
+	  </row>
+	  <row>
+	   

<entry><constant>V4L2_SUBDEV_SEL_TGT_COMPOSE_DEFAULT</constant></entry> +
<entry>257</entry>

Documentation/DocBook/media/v4l/vidioc-subdev-g-selection.xml new file +4 -0

+	    <entry>Default compose rectangle.</entry>
+	  </row>
+	  <row>
+	   

<entry><constant>V4L2_SUBDEV_SEL_TGT_COMPOSE_BOUNDS</constant></entry> +
<entry>258</entry>

Documentation/DocBook/media/v4l/vidioc-subdev-g-selection.xml new file +108 -0

+	    <entry>Bounds of the compose rectangle.</entry>
+	  </row>
+	</tbody>
+      </tgroup>
+    </table>
+
+    <table pgwide="1" frame="none" id="v4l2-subdev-selection-flags">
+      <title>V4L2 subdev selection flags</title>
+      <tgroup cols="3">
+        &cs-def;
+	<tbody valign="top">
+	  <row>
+	    <entry><constant>V4L2_SUBDEV_SEL_FLAG_SIZE_GE</constant></entry>
+	    <entry>(1 &lt;&lt; 0)</entry>
+	    <entry>Suggest the driver it should choose greater or
+	    equal rectangle (in size) than was requested.</entry>
+	  </row>
+	  <row>
+	    <entry><constant>V4L2_SUBDEV_SEL_FLAG_SIZE_LE</constant></entry>
+	    <entry>(1 &lt;&lt; 1)</entry>
+	    <entry>Suggest the driver it should choose lesser or
+	    equal rectangle (in size) than was requested.</entry>
+	  </row>
+	  <row>
+	    <entry><constant>V4L2_SUBDEV_SEL_FLAG_KEEP_CONFIG</constant></entry>
+	    <entry>(1 &lt;&lt; 2)</entry>
+	    <entry>The configuration should not be propagated to any
+	    further processing steps. If this flag is not given, the
+	    configuration is propagated inside the subdevice to all
+	    further processing steps.</entry>
+	  </row>
+	</tbody>
+      </tgroup>
+    </table>
+
+    <table pgwide="1" frame="none" id="v4l2-subdev-selection">
+      <title>struct <structname>v4l2_subdev_selection</structname></title>
+      <tgroup cols="3">
+        &cs-str;
+	<tbody valign="top">
+	  <row>
+	    <entry>__u32</entry>
+	    <entry><structfield>which</structfield></entry>
+	    <entry>Active or try selection, from
+	    &v4l2-subdev-format-whence;.</entry>
+	  </row>
+	  <row>
+	    <entry>__u32</entry>
+	    <entry><structfield>pad</structfield></entry>
+	    <entry>Pad number as reported by the media framework.</entry>
+	  </row>
+	  <row>
+	    <entry>__u32</entry>
+	    <entry><structfield>target</structfield></entry>
+	    <entry>Target selection rectangle. See
+	    <xref linkend="v4l2-subdev-selection-targets">.</xref>.</entry>
+	  </row>
+	  <row>
+	    <entry>__u32</entry>
+	    <entry><structfield>flags</structfield></entry>
+	    <entry>Flags. See
+	    <xref linkend="v4l2-subdev-selection-flags">.</xref></entry>
+	  </row>
+	  <row>
+	    <entry>&v4l2-rect;</entry>
+	    <entry><structfield>rect</structfield></entry>
+	    <entry>Crop rectangle boundaries, in pixels.</entry>
+	  </row>
+	  <row>
+	    <entry>__u32</entry>
+	    <entry><structfield>reserved</structfield>[8]</entry>
+	    <entry>Reserved for future extensions. Applications and drivers must
+	    set the array to zero.</entry>
+	  </row>
+	</tbody>
+      </tgroup>
+    </table>
+
+  </refsect1>
+
+  <refsect1>
+    &return-value;
+
+    <variablelist>
+      <varlistentry>
+	<term><errorcode>EBUSY</errorcode></term>
+	<listitem>
+	  <para>The selection rectangle can't be changed because the
+	  pad is currently busy. This can be caused, for instance, by
+	  an active video stream on the pad. The ioctl must not be
+	  retried without performing another action to fix the problem
+	  first. Only returned by
+	  <constant>VIDIOC_SUBDEV_S_SELECTION</constant></para>
+	</listitem>
+      </varlistentry>
+      <varlistentry>
+	<term><errorcode>EINVAL</errorcode></term>
+	<listitem>
+	  <para>The &v4l2-subdev-selection;
+	  <structfield>pad</structfield> references a non-existing
+	  pad, the <structfield>which</structfield> field references a
+	  non-existing format, or the selection target is not
+	  supported on the given subdev pad.</para>
+	</listitem>
+      </varlistentry>
+    </variablelist>
+  </refsect1>
+</refentry>

On Tuesday 20 December 2011 21:27:58 Sakari Ailus wrote:

[snip]

Quoted 5 lines

(quoted diff) +0 -0

b/Documentation/DocBook/media/v4l/dev-subdev.xml index 0916a73..722db60 100644

Documentation/DocBook/media/v4l/dev-subdev.xml +0 -0

[snip]

Quoted 1 line

Documentation/DocBook/media/v4l/dev-subdev.xml +0 -0

@@ -288,26 +288,81 @@

[snip]

Quoted 30 lines

Documentation/DocBook/media/v4l/dev-subdev.xml +30 -0

+      <para>Scaling operation changes the size of the image by scaling
+      it to new dimensions. Some sub-devices support it. The scaled
+      size (width and height) is represented by &v4l2-rect;. In the
+      case of scaling, top and left will always be zero. Scaling is
+      configured using &sub-subdev-g-selection; and
+      <constant>V4L2_SUBDEV_SEL_COMPOSE_ACTIVE</constant> selection
+      target on the sink pad of the subdev. The scaling is performed
+      related to the width and height of the crop rectangle on the
+      subdev's sink pad.</para>
+
+      <para>As for pad formats, drivers store try and active
+      rectangles for the selection targets of ACTIVE type <xref
+      linkend="v4l2-subdev-selection-targets">.</xref></para>
+
+      <para>On sink pads, cropping is applied relatively to the
+      current pad format. The pad format represents the image size as
+      received by the sub-device from the previous block in the
+      pipeline, and the crop rectangle represents the sub-image that
+      will be transmitted further inside the sub-device for
+      processing.</para>
+
+      <para>On source pads, cropping is similar to sink pads, with the
+      exception that the source size from which the cropping is
+      performed, is the COMPOSE rectangle on the sink pad. In both
+      sink and source pads, the crop rectangle must be entirely
+      containted inside the source image size for the crop
+      operation.</para>
+
+      <para>The drivers should always use the closest possible
+      rectangle the user requests on all selection targets, unless

Documentation/DocBook/media/v4l/dev-subdev.xml line 323

Quoted 3 lines

Documentation/DocBook/media/v4l/dev-subdev.xml +3 -0

+      specificly told otherwise<xref
+      linkend="v4l2-subdev-selection-flags">.</xref></para>
+    </section>

This sounds a bit confusing to me. One issue is that composing is not formally defined. I think it would help if you could draw a diagram that shows how the operations are applied, and modify the text to describe the diagram, using the natural order of the compose and crop operations on sink and source pads.

Documentation/DocBook/media/v4l/dev-subdev.xml line 336

Quoted 12 lines

Documentation/DocBook/media/v4l/dev-subdev.xml +12 -0

+    <section>
+      <title>Order of configuration and format propagation</title>
+
+      <para>The order of image processing steps will always be from
+      the sink pad towards the source pad. This is also reflected in
+      the order in which the configuration must be performed by the
+      user. The format is propagated within the subdev along the later
+      processing steps. For example, setting the sink pad format
+      causes all the selection rectangles and the source pad format to
+      be set to sink pad format --- if allowed by the hardware, and if
+      not, then closest possible. The coordinates to a step always
+      refer to the active size of the previous step.</para>

This also sounds a bit ambiguous if I try to ignore the fact that I know how it works :-) You should at least make it explicit that propagation inside subdevs is performed by the driver(s), and that propagation outside subdevs is to be handled by userspace.

Quoted 32 lines

Documentation/DocBook/media/v4l/dev-subdev.xml +25 -1

+      <orderedlist>
+	<listitem>Sink pad format. The user configures the sink pad
+	format. This format defines the parameters of the image the
+	entity receives through the pad for further processing.</listitem>
 
-      <para>Cropping behaviour on output pads is not defined.</para>
+	<listitem>Sink pad active crop selection. The sink pad crop
+	defines the performed to the sink pad format.</listitem>
 
+	<listitem>Sink pad active compose selection. The sink pad compose
+	rectangle defines the scaling ratio compared to the size of
+	the sink pad crop rectangle.</listitem>
+
+	<listitem>Source pad active crop selection. Crop on the source
+	pad defines crop performed to the image scaled according to
+	the sink pad compose rectangle.</listitem>
+
+	<listitem>Source pad active compose selection. The source pad
+	compose defines the size and location of the compose
+	rectangle.</listitem>
+
+	<listitem>Source pad format. The source pad format defines the
+	output pixel format of the subdev, as well as the other
+	parameters with the exception of the image width and
+	height.</listitem>
+
+      </orderedlist>
     </section>
+
   </section>
 
   &sub-subdev-formats;

[snip]

Quoted 7 lines

(quoted diff) +0 -0

b/Documentation/DocBook/media/v4l/vidioc-subdev-g-selection.xml new file mode 100644
index 0000000..5fbcd65

Documentation/DocBook/media/v4l/vidioc-subdev-g-selection.xml new file +0 -0

@@ -0,0 +1,226 @@

[snip]

Quoted 24 lines

Documentation/DocBook/media/v4l/vidioc-subdev-g-selection.xml new file +24 -0

+  <refsect1>
+    <title>Description</title>
+
+    <note>
+      <title>Experimental</title>
+      <para>This is an <link linkend="experimental">experimental</link>
+      interface and may change in the future.</para>
+    </note>
+
+    <para>The selections are used to configure various image
+    processing functionality performed by the subdevs which affect the
+    image size. This currently includes cropping, scaling and
+    composition.</para>
+
+    <para>The selections replace the crop API &sub-subdev-g-crop;. All
+    the function of the crop API, and more, are supported by the
+    selections API.</para>
+
+    <para>See <xref linkend="subdev">Sub-device interface</xref> for
+    more information on how each selection target affects the image
+    processing pipeline inside the subdevice.</para>
+
+    <section>
+      <title>Types of selection targets</title>

Documentation/DocBook/media/v4l/vidioc-subdev-g-selection.xml

Quoted 6 lines

Documentation/DocBook/media/v4l/vidioc-subdev-g-selection.xml new file +6 -0

+
+      <para>The are four types of selection targets: active, default,
+      bounds and padding. The ACTIVE targets are the targets which
+      configure the hardware. The DEFAULT target provides the default
+      for the ACTIVE selection. The BOUNDS target will return the
+      maximum width and height of the target.

What about the minimum ?

Quoted 2 lines

The PADDED target
+ provides the width and height for the padded image,

Is it valid for both crop and compose rectangles ?

Documentation/DocBook/media/v4l/vidioc-subdev-g-selection.xml line 81

Quoted 3 lines

and is

Documentation/DocBook/media/v4l/vidioc-subdev-g-selection.xml new file +2 -0

+      directly affected by the ACTIVE target. The PADDED targets may
+      be configurable depending on the hardware.</para>

If that's configurable drivers will need a way to store it in the file handle.

Quoted 148 lines

Documentation/DocBook/media/v4l/vidioc-subdev-g-selection.xml new file +25 -0

+    </section>
+
+    <table pgwide="1" frame="none" id="v4l2-subdev-selection-targets">
+      <title>V4L2 subdev selection targets</title>
+      <tgroup cols="3">
+        &cs-def;
+	<tbody valign="top">
+	  <row>
+	    <entry><constant>V4L2_SUBDEV_SEL_TGT_CROP_ACTIVE</constant></entry>
+	    <entry>0</entry>
+	    <entry>Active crop. Defines the cropping
+	    performed by the processing step.</entry>
+	  </row>
+	  <row>
+	    <entry><constant>V4L2_SUBDEV_SEL_TGT_CROP_DEFAULT</constant></entry>
+	    <entry>1</entry>
+	    <entry>Default crop rectangle.</entry>
+	  </row>
+	  <row>
+	    <entry><constant>V4L2_SUBDEV_SEL_TGT_CROP_BOUNDS</constant></entry>
+	    <entry>2</entry>
+	    <entry>Bounds of the crop rectangle.</entry>
+	  </row>
+	  <row>
+	   

<entry><constant>V4L2_SUBDEV_SEL_TGT_COMPOSE_ACTIVE</constant></entry> +
<entry>256</entry>

Documentation/DocBook/media/v4l/vidioc-subdev-g-selection.xml new file +5 -0

+	    <entry>Active compose rectangle. Used to configure scaling
+	    on sink pads and composition on source pads.</entry>
+	  </row>
+	  <row>
+	   

<entry><constant>V4L2_SUBDEV_SEL_TGT_COMPOSE_DEFAULT</constant></entry> +
<entry>257</entry>

Documentation/DocBook/media/v4l/vidioc-subdev-g-selection.xml new file +4 -0

+	    <entry>Default compose rectangle.</entry>
+	  </row>
+	  <row>
+	   

<entry><constant>V4L2_SUBDEV_SEL_TGT_COMPOSE_BOUNDS</constant></entry> +
<entry>258</entry>

Documentation/DocBook/media/v4l/vidioc-subdev-g-selection.xml new file +108 -0

+	    <entry>Bounds of the compose rectangle.</entry>
+	  </row>
+	</tbody>
+      </tgroup>
+    </table>
+
+    <table pgwide="1" frame="none" id="v4l2-subdev-selection-flags">
+      <title>V4L2 subdev selection flags</title>
+      <tgroup cols="3">
+        &cs-def;
+	<tbody valign="top">
+	  <row>
+	    <entry><constant>V4L2_SUBDEV_SEL_FLAG_SIZE_GE</constant></entry>
+	    <entry>(1 &lt;&lt; 0)</entry>
+	    <entry>Suggest the driver it should choose greater or
+	    equal rectangle (in size) than was requested.</entry>
+	  </row>
+	  <row>
+	    <entry><constant>V4L2_SUBDEV_SEL_FLAG_SIZE_LE</constant></entry>
+	    <entry>(1 &lt;&lt; 1)</entry>
+	    <entry>Suggest the driver it should choose lesser or
+	    equal rectangle (in size) than was requested.</entry>
+	  </row>
+	  <row>
+	    <entry><constant>V4L2_SUBDEV_SEL_FLAG_KEEP_CONFIG</constant></entry>
+	    <entry>(1 &lt;&lt; 2)</entry>
+	    <entry>The configuration should not be propagated to any
+	    further processing steps. If this flag is not given, the
+	    configuration is propagated inside the subdevice to all
+	    further processing steps.</entry>
+	  </row>
+	</tbody>
+      </tgroup>
+    </table>
+
+    <table pgwide="1" frame="none" id="v4l2-subdev-selection">
+      <title>struct <structname>v4l2_subdev_selection</structname></title>
+      <tgroup cols="3">
+        &cs-str;
+	<tbody valign="top">
+	  <row>
+	    <entry>__u32</entry>
+	    <entry><structfield>which</structfield></entry>
+	    <entry>Active or try selection, from
+	    &v4l2-subdev-format-whence;.</entry>
+	  </row>
+	  <row>
+	    <entry>__u32</entry>
+	    <entry><structfield>pad</structfield></entry>
+	    <entry>Pad number as reported by the media framework.</entry>
+	  </row>
+	  <row>
+	    <entry>__u32</entry>
+	    <entry><structfield>target</structfield></entry>
+	    <entry>Target selection rectangle. See
+	    <xref linkend="v4l2-subdev-selection-targets">.</xref>.</entry>
+	  </row>
+	  <row>
+	    <entry>__u32</entry>
+	    <entry><structfield>flags</structfield></entry>
+	    <entry>Flags. See
+	    <xref linkend="v4l2-subdev-selection-flags">.</xref></entry>
+	  </row>
+	  <row>
+	    <entry>&v4l2-rect;</entry>
+	    <entry><structfield>rect</structfield></entry>
+	    <entry>Crop rectangle boundaries, in pixels.</entry>
+	  </row>
+	  <row>
+	    <entry>__u32</entry>
+	    <entry><structfield>reserved</structfield>[8]</entry>
+	    <entry>Reserved for future extensions. Applications and drivers must
+	    set the array to zero.</entry>
+	  </row>
+	</tbody>
+      </tgroup>
+    </table>
+
+  </refsect1>
+
+  <refsect1>
+    &return-value;
+
+    <variablelist>
+      <varlistentry>
+	<term><errorcode>EBUSY</errorcode></term>
+	<listitem>
+	  <para>The selection rectangle can't be changed because the
+	  pad is currently busy. This can be caused, for instance, by
+	  an active video stream on the pad. The ioctl must not be
+	  retried without performing another action to fix the problem
+	  first. Only returned by
+	  <constant>VIDIOC_SUBDEV_S_SELECTION</constant></para>
+	</listitem>
+      </varlistentry>
+      <varlistentry>
+	<term><errorcode>EINVAL</errorcode></term>
+	<listitem>
+	  <para>The &v4l2-subdev-selection;
+	  <structfield>pad</structfield> references a non-existing
+	  pad, the <structfield>which</structfield> field references a
+	  non-existing format, or the selection target is not
+	  supported on the given subdev pad.</para>
+	</listitem>
+      </varlistentry>
+    </variablelist>
+  </refsect1>
+</refentry>

Hi Laurent,

Thanks for the comments!

Quoted 9 lines · Laurent Pinchart

On Tuesday 20 December 2011 21:27:58 Sakari Ailus wrote:

[snip]

Quoted 5 lines

(quoted diff) +0 -0

b/Documentation/DocBook/media/v4l/dev-subdev.xml index 0916a73..722db60 100644

Documentation/DocBook/media/v4l/dev-subdev.xml +0 -0

Quoted 5 lines

This sounds a bit confusing to me. One issue is that composing is not formally defined. I think it would help if you could draw a diagram that shows how the operations are applied, and modify the text to describe the diagram, using the natural order of the compose and crop operations on sink and source pads.

I drew a diagram based on your suggestion, but I'd prefer the formal definition would come from someone who needs composition and better understands the use cases.

Also cc Tomasz.

Quoted 13 lines

Documentation/DocBook/media/v4l/dev-subdev.xml line 336

Quoted 12 lines

+<section>
+<title>Order of configuration and format propagation</title>

Documentation/DocBook/media/v4l/dev-subdev.xml +10 -0

+
+<para>The order of image processing steps will always be from
+      the sink pad towards the source pad. This is also reflected in
+      the order in which the configuration must be performed by the
+      user. The format is propagated within the subdev along the later
+      processing steps. For example, setting the sink pad format
+      causes all the selection rectangles and the source pad format to
+      be set to sink pad format --- if allowed by the hardware, and if
+      not, then closest possible. The coordinates to a step always
+      refer to the active size of the previous step.</para>

Quoted 4 lines

This also sounds a bit ambiguous if I try to ignore the fact that I know how it works :-) You should at least make it explicit that propagation inside subdevs is performed by the driver(s), and that propagation outside subdevs is to be handled by userspace.

Agreed. I'll reword it.

Quoted 7 lines

Documentation/DocBook/media/v4l/dev-subdev.xml

Quoted 5 lines

+<para>The are four types of selection targets: active, default,

Documentation/DocBook/media/v4l/dev-subdev.xml +4 -0

+      bounds and padding. The ACTIVE targets are the targets which
+      configure the hardware. The DEFAULT target provides the default
+      for the ACTIVE selection. The BOUNDS target will return the
+      maximum width and height of the target.

What about the minimum ?

Good question. We could also specify that the minimum is obtained by using the V4L2_SUBDEV_SEL_FLAG_LE flag with the BOUNDS target.

Quoted 4 lines

Quoted 2 lines

The PADDED target
+ provides the width and height for the padded image,

Is it valid for both crop and compose rectangles ?

I think all targets are valid for all rectangles. Should I mention that?

The practical use cases may be more limited, though. I wonder if I should remove the padded targets until we get use cases for them. I included them for the reason that they also exist in the V4L2.

Tomasz, Sylwester: do you have use for the PADDED targets?

I think we also must define what will be done in cases where crop (on either sink or source) / scaling / composition is not supported by the subdev. That's currently undefined. I think it'd be most clear to return an error code but I'm not sure which one --- EINVAL is an obvious candidate but that is also returned when the pad is wrong. It looks still like the best choice to me.

Quoted 6 lines

Documentation/DocBook/media/v4l/dev-subdev.xml

Quoted 4 lines

and is

Documentation/DocBook/media/v4l/dev-subdev.xml +2 -0

+      directly affected by the ACTIVE target. The PADDED targets may
+      be configurable depending on the hardware.</para>

If that's configurable drivers will need a way to store it in the file handle.

Good point. I'll add it if we end up defining the padded targets now.

Quoted 2 lines

Quoted 2 lines

+</section>
+

Quoted 10 lines

Quoted 9 lines · Laurent Pinchart

On Tuesday 20 December 2011 21:27:58 Sakari Ailus wrote:

[snip]

Quoted 5 lines

(quoted diff) +0 -0

b/Documentation/DocBook/media/v4l/dev-subdev.xml index 0916a73..722db60 100644

Documentation/DocBook/media/v4l/dev-subdev.xml +0 -0

[snip]

Quoted 39 lines

Quoted 9 lines

This sounds a bit confusing to me. One issue is that composing is not formally
defined. I think it would help if you could draw a diagram that shows how the
operations are applied, and modify the text to describe the diagram, using the
natural order of the compose and crop operations on sink and source pads.

I drew a diagram based on your suggestion, but I'd prefer the formal definition would come from someone who needs composition and better understands the use cases.

Also cc Tomasz.

Quoted 13 lines

Documentation/DocBook/media/v4l/dev-subdev.xml line 336

Quoted 12 lines

+<section>
+<title>Order of configuration and format propagation</title>

Documentation/DocBook/media/v4l/dev-subdev.xml +10 -0

+
+<para>The order of image processing steps will always be from
+      the sink pad towards the source pad. This is also reflected in
+      the order in which the configuration must be performed by the
+      user. The format is propagated within the subdev along the later
+      processing steps. For example, setting the sink pad format
+      causes all the selection rectangles and the source pad format to
+      be set to sink pad format --- if allowed by the hardware, and if
+      not, then closest possible. The coordinates to a step always
+      refer to the active size of the previous step.</para>

Quoted 7 lines

This also sounds a bit ambiguous if I try to ignore the fact that I know how
it works :-) You should at least make it explicit that propagation inside
subdevs is performed by the driver(s), and that propagation outside subdevs is
to be handled by userspace.

Agreed. I'll reword it.

[snip]

Quoted 7 lines

Quoted 7 lines

Documentation/DocBook/media/v4l/dev-subdev.xml

Quoted 5 lines

+<para>The are four types of selection targets: active, default,

Documentation/DocBook/media/v4l/dev-subdev.xml +4 -0

+      bounds and padding. The ACTIVE targets are the targets which
+      configure the hardware. The DEFAULT target provides the default
+      for the ACTIVE selection. The BOUNDS target will return the
+      maximum width and height of the target.

What about the minimum ?

There are multiple problems with idea of the maximal rectangle because one has to provide definition how to compare size of two rectangles. If you had to compare rectangles 10x1, 1x10, 5x5 which would you choose as the largest? Such problems may appear if HW has width, height and size constraints. One encounters similar problem with definition of the smallest rectangle. I prefer to define "is larger" as "contains". Rectangle 10x10 contains all three overmentioned rectangles therefore it is the maximal rectangle. The problem with such a definition is that such a rectangle may not be accepted by HW.

Quoted 1 line

Quoted 3 lines

Good question. We could also specify that the minimum is obtained by using the V4L2_SUBDEV_SEL_FLAG_LE flag with the BOUNDS target.

As I remember bounds rectangle is fixed and there is no such a thing as minimal/maximal bounds. For V4L2 video node API, the bounds rectangle is read-only value describing rectangle that contains all pixels. Could you describe the use case that utilities minimal bounds rectangle?

Quoted 5 lines

Quoted 4 lines

Quoted 2 lines

The PADDED target
+ provides the width and height for the padded image,

Is it valid for both crop and compose rectangles ?

Quoted 7 lines

I think all targets are valid for all rectangles. Should I mention that?

The practical use cases may be more limited, though. I wonder if I should remove the padded targets until we get use cases for them. I included them for the reason that they also exist in the V4L2.

Tomasz, Sylwester: do you have use for the PADDED targets?

S5P-TV and S5P-JPEG (by Andrzej Pietrasiewicz) makes use of PADDED target. The padded target is very hardware/application dependent parameter. It is defined only for composing targets. Basically it refers to all pixels that are modified by the hardware. In S5P-TV the padded rectangle is equal to active rectangle. However, if having no good idea about padded area, then it is always safe/consistent to make padded rectangle equal to bounds one.

Quoted 6 lines

I think we also must define what will be done in cases where crop (on either sink or source) / scaling / composition is not supported by the subdev. That's currently undefined. I think it'd be most clear to return an error code but I'm not sure which one --- EINVAL is an obvious candidate but that is also returned when the pad is wrong. It looks still like the best choice to me.

Maybe one should return EPERM or EACCES if S_SELECTION is called on read-only target. Other idea is to introduce V4L2_SEL_FLAG_RDONLY flag which is set by VIDIOC_G_SELECTION for a given target. The driver may return EINVAL if target is not supported. It would imply that support for the target is not implemented in the driver.

Quoted 1 line · Laurent Pinchart

On Tuesday 20 December 2011 21:27:58 Sakari Ailus wrote:

Quoted 4 lines

Quoted 2 lines

The PADDED target
+ provides the width and height for the padded image,

Is it valid for both crop and compose rectangles ?

I think all targets are valid for all rectangles. Should I mention that?

The practical use cases may be more limited, though. I wonder if I should remove the padded targets until we get use cases for them. I included them for the reason that they also exist in the V4L2.

Tomasz, Sylwester: do you have use for the PADDED targets?

I think we also must define what will be done in cases where crop (on either sink or source) / scaling / composition is not supported by the subdev. That's currently undefined. I think it'd be most clear to return an error code but I'm not sure which one --- EINVAL is an obvious candidate but that is also returned when the pad is wrong. It looks still like the best choice to me.

From: Sakari Ailus <[email protected]>

Pixelrate is an essential part of the image data parameters. Add this. Together, the current parameters also define the frame rate.

Sensors do not have a concept of frame rate; pixelrate is much more meaningful in this context. Also, it is best to combine the pixelrate with the other format parameters since there are dependencies between them.

Signed-off-by Sakari Ailus <[email protected]>

---
 Documentation/DocBook/media/v4l/subdev-formats.xml |   10 +++++++++-
 include/linux/v4l2-mediabus.h                      |    4 +++-
 2 files changed, 12 insertions(+), 2 deletions(-)

(quoted diff) +0 -0

b/Documentation/DocBook/media/v4l/subdev-formats.xml index 49c532e..a6a6630 100644

This clock is the maximum rate at

Documentation/DocBook/media/v4l/subdev-formats.xml +4 -0

+	</row>
+	<row>
+	  <entry>__u32</entry>
+	  <entry><structfield>reserved</structfield>[6]</entry>
 	  <entry>Reserved for future extensions. Applications and drivers must
 	  set the array to zero.</entry>
 	</row>

include/linux/v4l2-mediabus.h +0 -0

index 5ea7f75..35c6b96 100644

include/linux/v4l2-mediabus.h +2 -1

  */
 struct v4l2_mbus_framefmt {
 	__u32			width;

@@ -108,7 +109,8 @@ struct v4l2_mbus_framefmt {
 	__u32			code;
 	__u32			field;
 	__u32			colorspace;
-	__u32			reserved[7];
+	__u32			pixelrate;
+	__u32			reserved[6];
 };
 
 #endif

Quoted 15 lines · On Tuesday 20 December 2011 21:27:59 Sakari Ailus wrote:

From: Sakari Ailus <[email protected]>

Pixelrate is an essential part of the image data parameters. Add this. Together, the current parameters also define the frame rate.

Sensors do not have a concept of frame rate; pixelrate is much more meaningful in this context. Also, it is best to combine the pixelrate with the other format parameters since there are dependencies between them.

Signed-off-by Sakari Ailus <[email protected]>

---
 Documentation/DocBook/media/v4l/subdev-formats.xml |   10 +++++++++-
 include/linux/v4l2-mediabus.h                      |    4 +++-
 2 files changed, 12 insertions(+), 2 deletions(-)

Documentation/DocBook/media/v4l/subdev-formats.xml line 39

Quoted 12 lines

(quoted diff) +0 -0

b/Documentation/DocBook/media/v4l/subdev-formats.xml index 49c532e..a6a6630 100644

Documentation/DocBook/media/v4l/subdev-formats.xml +2 -1

@@ -35,7 +35,15 @@
 	</row>
 	<row>
 	  <entry>__u32</entry>
-	  <entry><structfield>reserved</structfield>[7]</entry>
+	  <entry><structfield>pixelrate</structfield></entry>
+	  <entry>Pixel rate in kp/s.

kPix/s or kPixel/s ?

Quoted 1 line

This clock is the maximum rate at

Is it really a clock ?

Documentation/DocBook/media/v4l/subdev-formats.xml line 42

Quoted 3 lines

Documentation/DocBook/media/v4l/subdev-formats.xml +3 -0

+	  which pixels are transferred on the bus. The
+	  <structfield>pixelrate</structfield> field is
+	  read-only.</entry>

Does that mean that userspace isn't required to propagate the value down the pipeline when configuring it ? I'm fine with that, but it should probably be documented explictly somewhere to make sure that drivers don't rely on it.

Quoted 12 lines

Documentation/DocBook/media/v4l/subdev-formats.xml +4 -0

+	</row>
+	<row>
+	  <entry>__u32</entry>
+	  <entry><structfield>reserved</structfield>[6]</entry>
 	  <entry>Reserved for future extensions. Applications and drivers must
 	  set the array to zero.</entry>
 	</row>

include/linux/v4l2-mediabus.h +0 -0

index 5ea7f75..35c6b96 100644

include/linux/v4l2-mediabus.h enum v4l2_mbus_pixelcode { · line 104

Quoted 4 lines

include/linux/v4l2-mediabus.h +1 -0

@@ -101,6 +101,7 @@ enum v4l2_mbus_pixelcode {
  * @code:	data format code (from enum v4l2_mbus_pixelcode)
  * @field:	used interlacing type (from enum v4l2_field)
  * @colorspace:	colorspace of the data (from enum v4l2_colorspace)
+ * @pixel_clock: pixel clock, in kHz

I think you forgot to update the comment.

Quoted 13 lines

include/linux/v4l2-mediabus.h +2 -1

  */
 struct v4l2_mbus_framefmt {
 	__u32			width;

@@ -108,7 +109,8 @@ struct v4l2_mbus_framefmt {
 	__u32			code;
 	__u32			field;
 	__u32			colorspace;
-	__u32			reserved[7];
+	__u32			pixelrate;
+	__u32			reserved[6];
 };
 
 #endif

From: Sakari Ailus <[email protected]>

Add image source control class. This control class is intended to contain low level controls which deal with control of the image capture process --- the A/D converter in image sensors, for example.

Signed-off-by Sakari Ailus <[email protected]>

---
 Documentation/DocBook/media/v4l/controls.xml       |  101 +++++++++++++++++
 .../DocBook/media/v4l/vidioc-g-ext-ctrls.xml       |    6 +
 drivers/media/video/v4l2-ctrls.c                   |   10 ++
 include/linux/videodev2.h                          |   10 ++
 4 files changed, 127 insertions(+), 0 deletions(-)

(quoted diff) +0 -0

b/Documentation/DocBook/media/v4l/controls.xml index 3bc5ee8..69ede83 100644

Documentation/DocBook/media/v4l/controls.xml +6 -0

@@ -3356,6 +3356,107 @@ interface and may change in the future.</para>
       </table>
 
     </section>
+
+    <section id="image-source-controls">
+      <title>Image Source Control Reference</title>
+
+      <note>
+	<title>Experimental</title>

Documentation/DocBook/media/v4l/controls.xml +18 -0

+	transmitter to transmit the image data out of the device.
+      </para>
+
+      <table pgwide="1" frame="none" id="image-source-control-id">
+      <title>Image Source Control IDs</title>
+
+      <tgroup cols="4">
+	<colspec colname="c1" colwidth="1*" />
+	<colspec colname="c2" colwidth="6*" />
+	<colspec colname="c3" colwidth="2*" />
+	<colspec colname="c4" colwidth="6*" />
+	<spanspec namest="c1" nameend="c2" spanname="id" />
+	<spanspec namest="c2" nameend="c4" spanname="descr" />
+	<thead>
+	  <row>
+	    <entry spanname="id" align="left">ID</entry>
+	    <entry align="left">Type</entry>
+	  </row><row rowsep="1"><entry spanname="descr"

align="left">Description</entry> + </row>

Documentation/DocBook/media/v4l/controls.xml +5 -0

+	</thead>
+	<tbody valign="top">
+	  <row><entry></entry></row>
+	  <row>
+	    <entry

spanname="id"><constant>V4L2_CID_IMAGE_SOURCE_CLASS</constant></entry> +
<entry>class</entry>

Documentation/DocBook/media/v4l/controls.xml +6 -0

+	  </row>
+	  <row>
+	    <entry spanname="descr">The IMAGE_SOURCE class descriptor.</entry>
+	  </row>
+	  <row>
+	    <entry

spanname="id"><constant>V4L2_CID_IMAGE_SOURCE_VBLANK</constant></entry> +
<entry>integer</entry>

Documentation/DocBook/media/v4l/controls.xml +8 -0

+	    produced. The unit of vertical blanking is a line. Every
+	    line has length of the image width plus horizontal
+	    blanking at the pixel clock specified by struct
+	    v4l2_mbus_framefmt <xref linkend="v4l2-mbus-framefmt"
+	    />.</entry>
+	  </row>
+	  <row>
+	    <entry

spanname="id"><constant>V4L2_CID_IMAGE_SOURCE_HBLANK</constant></entry> +
<entry>integer</entry>

Documentation/DocBook/media/v4l/controls.xml +11 -0

+	    Together with the media bus pixel code, bus type (clock
+	    cycles per sample), the data bus frequency defines the
+	    pixel clock. <xref linkend="v4l2-mbus-framefmt" /> The
+	    frame rate can be calculated from the pixel clock, image
+	    width and height and horizontal and vertical blanking. The
+	    frame rate control is performed by selecting the desired
+	    horizontal and vertical blanking.
+	    </entry>
+	  </row>
+	  <row>
+	    <entry

spanname="id"><constant>V4L2_CID_IMAGE_SOURCE_ANALOGUE_GAIN</constant></en
try> + <entry>integer</entry>

Documentation/DocBook/media/v4l/controls.xml +9 -0

+	    </entry>
+	  </row>
+	  <row><entry></entry></row>
+	</tbody>
+      </tgroup>
+      </table>
+
+    </section>
+
 </section>
 
   <!--

(quoted diff) +0 -0

b/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml index 5122ce8..250c1cf 100644

Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml +6 -0

@@ -257,6 +257,12 @@ These controls are described in <xref
 These controls are described in <xref
 		linkend="flash-controls" />.</entry>
 	  </row>
+	  <row>
+	    <entry><constant>V4L2_CTRL_CLASS_IMAGE_SOURCE</constant></entry>
+	    <entry>0x9d0000</entry> <entry>The class containing image
+	    source controls. These controls are described in <xref
+	    linkend="image-source-controls" />.</entry>
+	  </row>
 	</tbody>
       </tgroup>
     </table>

(quoted diff) +0 -0

b/drivers/media/video/v4l2-ctrls.c index 083bb79..da1ec52 100644

drivers/media/video/v4l2-ctrls.c +0 -0

 	case V4L2_CID_RDS_TX_PS_NAME:
 	case V4L2_CID_RDS_TX_RADIO_TEXT:
 		*type = V4L2_CTRL_TYPE_STRING;

@@ -703,6 +712,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum

v4l2_ctrl_type *type, case V4L2_CID_MPEG_CLASS:

drivers/media/video/v4l2-ctrls.c +1 -0

 	case V4L2_CID_FM_TX_CLASS:
 	case V4L2_CID_FLASH_CLASS:
+	case V4L2_CID_IMAGE_SOURCE_CLASS:
 		*type = V4L2_CTRL_TYPE_CTRL_CLASS;
 		/* You can neither read not write these */
 		*flags |= V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_WRITE_ONLY;

include/linux/videodev2.h +1 -0

index 9633c69..0f8f904 100644
@@ -1080,6 +1080,7 @@ struct v4l2_ext_controls {
 #define V4L2_CTRL_CLASS_CAMERA 0x009a0000	/* Camera class controls */
 #define V4L2_CTRL_CLASS_FM_TX 0x009b0000	/* FM Modulator control class */
 #define V4L2_CTRL_CLASS_FLASH 0x009c0000	/* Camera flash controls */
+#define V4L2_CTRL_CLASS_IMAGE_SOURCE 0x009d0000	/* Image source flash

controls */

#define V4L2_CTRL_ID_MASK (0x0fffffff)
#define V4L2_CTRL_ID2CLASS(id) ((id) & 0x0fff0000UL)

include/linux/videodev2.h +2 -0

@@ -1690,6 +1691,15 @@ enum v4l2_flash_strobe_source {
 #define V4L2_CID_FLASH_CHARGE			(V4L2_CID_FLASH_CLASS_BASE + 11)
 #define V4L2_CID_FLASH_READY			(V4L2_CID_FLASH_CLASS_BASE + 12)
 
+/* Image source controls */
+#define V4L2_CID_IMAGE_SOURCE_CLASS_BASE	(V4L2_CTRL_CLASS_IMAGE_SOURCE |

0x900) +#define V4L2_CID_IMAGE_SOURCE_CLASS

| 1) +
+#define V4L2_CID_IMAGE_SOURCE_VBLANK (V4L2_CID_IMAGE_SOURCE_CLASS_BASE

1) +#define
V4L2_CID_IMAGE_SOURCE_HBLANK (V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 2)
+#define
V4L2_CID_IMAGE_SOURCE_LINK_FREQ (V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 3)
+#define
V4L2_CID_IMAGE_SOURCE_ANALOGUE_GAIN (V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 4)
+
/*
* T U N I N G
*/

Quoted 34 lines · On Tuesday 20 December 2011 21:28:00 Sakari Ailus wrote:

From: Sakari Ailus <[email protected]>

Add image source control class. This control class is intended to contain low level controls which deal with control of the image capture process --- the A/D converter in image sensors, for example.

Signed-off-by Sakari Ailus <[email protected]>

---
 Documentation/DocBook/media/v4l/controls.xml       |  101 +++++++++++++++++
 .../DocBook/media/v4l/vidioc-g-ext-ctrls.xml       |    6 +
 drivers/media/video/v4l2-ctrls.c                   |   10 ++
 include/linux/videodev2.h                          |   10 ++
 4 files changed, 127 insertions(+), 0 deletions(-)

(quoted diff) +0 -0

b/Documentation/DocBook/media/v4l/controls.xml index 3bc5ee8..69ede83 100644

Documentation/DocBook/media/v4l/controls.xml +11 -0

@@ -3356,6 +3356,107 @@ interface and may change in the future.</para>
       </table>
 
     </section>
+
+    <section id="image-source-controls">
+      <title>Image Source Control Reference</title>
+
+      <note>
+	<title>Experimental</title>
+
+	<para>This is an <link
+	linkend="experimental">experimental</link> interface and may
+	change in the future.</para>
+      </note>

Documentation/DocBook/media/v4l/controls.xml interface and may change in the future.</para> · line 3374

Quoted 5 lines

Documentation/DocBook/media/v4l/controls.xml +5 -0

+
+      <para>
+	The Image Source control class is intended for low-level
+	control of image source devices such as image sensors. The
+	devices feature an analogue to digital converter and a bus

Is the V4L2 documentation written in US or UK English ? US uses analog, UK uses analogue. Analog would be shorter in control names.

Quoted 34 lines

Documentation/DocBook/media/v4l/controls.xml +18 -0

+	transmitter to transmit the image data out of the device.
+      </para>
+
+      <table pgwide="1" frame="none" id="image-source-control-id">
+      <title>Image Source Control IDs</title>
+
+      <tgroup cols="4">
+	<colspec colname="c1" colwidth="1*" />
+	<colspec colname="c2" colwidth="6*" />
+	<colspec colname="c3" colwidth="2*" />
+	<colspec colname="c4" colwidth="6*" />
+	<spanspec namest="c1" nameend="c2" spanname="id" />
+	<spanspec namest="c2" nameend="c4" spanname="descr" />
+	<thead>
+	  <row>
+	    <entry spanname="id" align="left">ID</entry>
+	    <entry align="left">Type</entry>
+	  </row><row rowsep="1"><entry spanname="descr"

align="left">Description</entry> + </row>

Documentation/DocBook/media/v4l/controls.xml +5 -0

+	</thead>
+	<tbody valign="top">
+	  <row><entry></entry></row>
+	  <row>
+	    <entry

spanname="id"><constant>V4L2_CID_IMAGE_SOURCE_CLASS</constant></entry> +
<entry>class</entry>

Documentation/DocBook/media/v4l/controls.xml +6 -0

+	  </row>
+	  <row>
+	    <entry spanname="descr">The IMAGE_SOURCE class descriptor.</entry>
+	  </row>
+	  <row>
+	    <entry

spanname="id"><constant>V4L2_CID_IMAGE_SOURCE_VBLANK</constant></entry> +
<entry>integer</entry>

Documentation/DocBook/media/v4l/controls.xml interface and may change in the future.</para> · line 3410

Quoted 4 lines

Documentation/DocBook/media/v4l/controls.xml +4 -0

+	  </row>
+	  <row>
+	    <entry spanname="descr">Vertical blanking. The idle
+	    preriod after every frame during which no image data is

s/preriod/period/

Quoted 10 lines

Documentation/DocBook/media/v4l/controls.xml +8 -0

+	    produced. The unit of vertical blanking is a line. Every
+	    line has length of the image width plus horizontal
+	    blanking at the pixel clock specified by struct
+	    v4l2_mbus_framefmt <xref linkend="v4l2-mbus-framefmt"
+	    />.</entry>
+	  </row>
+	  <row>
+	    <entry

spanname="id"><constant>V4L2_CID_IMAGE_SOURCE_HBLANK</constant></entry> +
<entry>integer</entry>

Documentation/DocBook/media/v4l/controls.xml interface and may change in the future.</para> · line 3423

Quoted 4 lines

Documentation/DocBook/media/v4l/controls.xml +4 -0

+	  </row>
+	  <row>
+	    <entry spanname="descr">Horizontal blanking. The idle
+	    preriod after every line of image data during which no

s/preriod/period/

Documentation/DocBook/media/v4l/controls.xml interface and may change in the future.</para> · line 3432

Quoted 10 lines

Documentation/DocBook/media/v4l/controls.xml +5 -0

+	    image data is produced. The unit of horizontal blanking is
+	    pixels.</entry>
+	  </row>
+	  <row>
+	    <entry

spanname="id"><constant>V4L2_CID_IMAGE_SOURCE_LINK_FREQ</constant></entry>

Documentation/DocBook/media/v4l/controls.xml +4 -0

+	    <entry>integer menu</entry>
+	  </row>
+	  <row>
+	    <entry spanname="descr">Image source's data bus frequency.

What's the frequency unit ? Sample/second ?

Quoted 13 lines

Documentation/DocBook/media/v4l/controls.xml +11 -0

+	    Together with the media bus pixel code, bus type (clock
+	    cycles per sample), the data bus frequency defines the
+	    pixel clock. <xref linkend="v4l2-mbus-framefmt" /> The
+	    frame rate can be calculated from the pixel clock, image
+	    width and height and horizontal and vertical blanking. The
+	    frame rate control is performed by selecting the desired
+	    horizontal and vertical blanking.
+	    </entry>
+	  </row>
+	  <row>
+	    <entry

spanname="id"><constant>V4L2_CID_IMAGE_SOURCE_ANALOGUE_GAIN</constant></en
try> + <entry>integer</entry>

Documentation/DocBook/media/v4l/controls.xml interface and may change in the future.</para> · line 3450

Quoted 6 lines

Documentation/DocBook/media/v4l/controls.xml +6 -0

+	  </row>
+	  <row>
+	    <entry spanname="descr">Analogue gain is gain affecting
+	    all colour components in the pixel matrix. The gain
+	    operation is performed in the analogue domain before A/D
+	    conversion.

Should we define one gain per color component ?

Quoted 38 lines

Documentation/DocBook/media/v4l/controls.xml +9 -0

+	    </entry>
+	  </row>
+	  <row><entry></entry></row>
+	</tbody>
+      </tgroup>
+      </table>
+
+    </section>
+
 </section>
 
   <!--

(quoted diff) +0 -0

b/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml index 5122ce8..250c1cf 100644

Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml +6 -0

@@ -257,6 +257,12 @@ These controls are described in <xref
 These controls are described in <xref
 		linkend="flash-controls" />.</entry>
 	  </row>
+	  <row>
+	    <entry><constant>V4L2_CTRL_CLASS_IMAGE_SOURCE</constant></entry>
+	    <entry>0x9d0000</entry> <entry>The class containing image
+	    source controls. These controls are described in <xref
+	    linkend="image-source-controls" />.</entry>
+	  </row>
 	</tbody>
       </tgroup>
     </table>

(quoted diff) +0 -0

b/drivers/media/video/v4l2-ctrls.c index 083bb79..da1ec52 100644

drivers/media/video/v4l2-ctrls.c +0 -0

@@ -606,6 +606,12 @@ const char *v4l2_ctrl_get_name(u32 id)
 	case V4L2_CID_FLASH_CHARGE:		return "Charge";
 	case V4L2_CID_FLASH_READY:		return "Ready to strobe";
 

drivers/media/video/v4l2-ctrls.c const char *v4l2_ctrl_get_name(u32 id) · line 613

Quoted 5 lines

drivers/media/video/v4l2-ctrls.c +5 -0

+	case V4L2_CID_IMAGE_SOURCE_CLASS:	return "Image source controls";
+	case V4L2_CID_IMAGE_SOURCE_VBLANK:	return "Vertical blanking";
+	case V4L2_CID_IMAGE_SOURCE_HBLANK:	return "Horizontal blanking";
+	case V4L2_CID_IMAGE_SOURCE_LINK_FREQ:	return "Link frequency";
+	case V4L2_CID_IMAGE_SOURCE_ANALOGUE_GAIN: return "Analogue gain";

Please capitalize each word, as done for the other controls.

Quoted 5 lines

drivers/media/video/v4l2-ctrls.c +0 -0

 	default:
 		return NULL;
 	}

@@ -694,6 +700,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum

v4l2_ctrl_type *type, case V4L2_CID_MPEG_VIDEO_MPEG4_PROFILE:

drivers/media/video/v4l2-ctrls.c void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type, · line 705

Quoted 5 lines

drivers/media/video/v4l2-ctrls.c +3 -0

 		*type = V4L2_CTRL_TYPE_MENU;
 		break;
+	case V4L2_CID_IMAGE_SOURCE_LINK_FREQ:
+		*type = V4L2_CTRL_TYPE_INTEGER_MENU;
+		break;

Will this always be an integer menu control, or can you foresee cases where the range would be continuous ?

We already have drivers for Aptina parallel sensors that include a configurable PLL. The pixel clock can be configured more or less freely in that case, although I'm not sure if that's useful in practice.

Quoted 30 lines

drivers/media/video/v4l2-ctrls.c +0 -0

 	case V4L2_CID_RDS_TX_PS_NAME:
 	case V4L2_CID_RDS_TX_RADIO_TEXT:
 		*type = V4L2_CTRL_TYPE_STRING;

@@ -703,6 +712,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum

v4l2_ctrl_type *type, case V4L2_CID_MPEG_CLASS:

drivers/media/video/v4l2-ctrls.c +1 -0

 	case V4L2_CID_FM_TX_CLASS:
 	case V4L2_CID_FLASH_CLASS:
+	case V4L2_CID_IMAGE_SOURCE_CLASS:
 		*type = V4L2_CTRL_TYPE_CTRL_CLASS;
 		/* You can neither read not write these */
 		*flags |= V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_WRITE_ONLY;

include/linux/videodev2.h +1 -0

index 9633c69..0f8f904 100644
@@ -1080,6 +1080,7 @@ struct v4l2_ext_controls {
 #define V4L2_CTRL_CLASS_CAMERA 0x009a0000	/* Camera class controls */
 #define V4L2_CTRL_CLASS_FM_TX 0x009b0000	/* FM Modulator control class */
 #define V4L2_CTRL_CLASS_FLASH 0x009c0000	/* Camera flash controls */
+#define V4L2_CTRL_CLASS_IMAGE_SOURCE 0x009d0000	/* Image source flash

controls */

#define V4L2_CTRL_ID_MASK (0x0fffffff)
#define V4L2_CTRL_ID2CLASS(id) ((id) & 0x0fff0000UL)

include/linux/videodev2.h +2 -0

@@ -1690,6 +1691,15 @@ enum v4l2_flash_strobe_source {
 #define V4L2_CID_FLASH_CHARGE			(V4L2_CID_FLASH_CLASS_BASE + 11)
 #define V4L2_CID_FLASH_READY			(V4L2_CID_FLASH_CLASS_BASE + 12)
 
+/* Image source controls */
+#define V4L2_CID_IMAGE_SOURCE_CLASS_BASE	(V4L2_CTRL_CLASS_IMAGE_SOURCE |

Quoted 1 line

0x900) +#define V4L2_CID_IMAGE_SOURCE_CLASS

(V4L2_CTRL_CLASS_IMAGE_SOURCE

Quoted 2 lines

| 1) +
+#define V4L2_CID_IMAGE_SOURCE_VBLANK (V4L2_CID_IMAGE_SOURCE_CLASS_BASE

+

Quoted 10 lines

1) +#define
V4L2_CID_IMAGE_SOURCE_HBLANK (V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 2)
+#define
V4L2_CID_IMAGE_SOURCE_LINK_FREQ (V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 3)
+#define
V4L2_CID_IMAGE_SOURCE_ANALOGUE_GAIN (V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 4)
+
/*
* T U N I N G
*/

Quoted 39 lines · Laurent Pinchart

Quoted 38 lines · On Tuesday 20 December 2011 21:28:00 Sakari Ailus wrote:

From: Sakari Ailus <[email protected]>

Add image source control class. This control class is intended to contain low level controls which deal with control of the image capture process --- the A/D converter in image sensors, for example.

Signed-off-by Sakari Ailus <[email protected]>

---

 Documentation/DocBook/media/v4l/controls.xml       |  101
 +++++++++++++++++ .../DocBook/media/v4l/vidioc-g-ext-ctrls.xml       |
    6 +
 drivers/media/video/v4l2-ctrls.c                   |   10 ++
 include/linux/videodev2.h                          |   10 ++
 4 files changed, 127 insertions(+), 0 deletions(-)

(quoted diff) +0 -0

b/Documentation/DocBook/media/v4l/controls.xml index 3bc5ee8..69ede83 100644

Documentation/DocBook/media/v4l/controls.xml +11 -0

@@ -3356,6 +3356,107 @@ interface and may change in the future.</para>
 
       </table>
 
     </section>
 
+
+    <section id="image-source-controls">
+      <title>Image Source Control Reference</title>
+
+      <note>
+	<title>Experimental</title>
+
+	<para>This is an <link
+	linkend="experimental">experimental</link> interface and may
+	change in the future.</para>
+      </note>

Quoted 8 lines

Documentation/DocBook/media/v4l/controls.xml interface and may change in the future.</para> · line 3374

Quoted 5 lines

Documentation/DocBook/media/v4l/controls.xml +5 -0

+
+      <para>
+	The Image Source control class is intended for low-level
+	control of image source devices such as image sensors. The
+	devices feature an analogue to digital converter and a bus

Is the V4L2 documentation written in US or UK English ? US uses analog, UK uses analogue. Analog would be shorter in control names.

Both appear to be used, but the US English appears to be more commonly used. I guess it's mostly chosen by whatever happened to be used by the author of the patch. I prefer UK spelling which you might have noticed already. :-)

I remember there was a discussion on this topic years ago within the kernel community but I don't remember how it ended up with... LWN.net appears to remember better than I do, but by a quick check I can't find any definitive conclusion.

<URL:http://lwn.net/Articles/44262/>
<URL:http://lkml.org/lkml/2003/8/7/245>

Quoted 66 lines

Quoted 35 lines

Documentation/DocBook/media/v4l/controls.xml +18 -0

+	transmitter to transmit the image data out of the device.
+      </para>
+
+      <table pgwide="1" frame="none" id="image-source-control-id">
+      <title>Image Source Control IDs</title>
+
+      <tgroup cols="4">
+	<colspec colname="c1" colwidth="1*" />
+	<colspec colname="c2" colwidth="6*" />
+	<colspec colname="c3" colwidth="2*" />
+	<colspec colname="c4" colwidth="6*" />
+	<spanspec namest="c1" nameend="c2" spanname="id" />
+	<spanspec namest="c2" nameend="c4" spanname="descr" />
+	<thead>
+	  <row>
+	    <entry spanname="id" align="left">ID</entry>
+	    <entry align="left">Type</entry>
+	  </row><row rowsep="1"><entry spanname="descr"

align="left">Description</entry> + </row>

Documentation/DocBook/media/v4l/controls.xml +5 -0

+	</thead>
+	<tbody valign="top">
+	  <row><entry></entry></row>
+	  <row>
+	    <entry

spanname="id"><constant>V4L2_CID_IMAGE_SOURCE_CLASS</constant></entry> +

<entry>class</entry>

Documentation/DocBook/media/v4l/controls.xml +6 -0

+	  </row>
+	  <row>
+	    <entry spanname="descr">The IMAGE_SOURCE class descriptor.</entry>
+	  </row>
+	  <row>
+	    <entry

spanname="id"><constant>V4L2_CID_IMAGE_SOURCE_VBLANK</constant></entry>

Documentation/DocBook/media/v4l/controls.xml interface and may change in the future.</para> · line 3410

Quoted 8 lines

Documentation/DocBook/media/v4l/controls.xml +5 -0

+
 
   <entry>integer</entry>
 
+	  </row>
+	  <row>
+	    <entry spanname="descr">Vertical blanking. The idle
+	    preriod after every frame during which no image data is

s/preriod/period/

Quoted 9 lines

Documentation/DocBook/media/v4l/controls.xml +8 -0

+	    produced. The unit of vertical blanking is a line. Every
+	    line has length of the image width plus horizontal
+	    blanking at the pixel clock specified by struct
+	    v4l2_mbus_framefmt <xref linkend="v4l2-mbus-framefmt"
+	    />.</entry>
+	  </row>
+	  <row>
+	    <entry

spanname="id"><constant>V4L2_CID_IMAGE_SOURCE_HBLANK</constant></entry>

Documentation/DocBook/media/v4l/controls.xml interface and may change in the future.</para> · line 3423

Quoted 8 lines

Documentation/DocBook/media/v4l/controls.xml +5 -0

+
 
   <entry>integer</entry>
 
+	  </row>
+	  <row>
+	    <entry spanname="descr">Horizontal blanking. The idle
+	    preriod after every line of image data during which no

s/preriod/period/

Quoted 12 lines

Documentation/DocBook/media/v4l/controls.xml interface and may change in the future.</para> · line 3432

Quoted 10 lines

Documentation/DocBook/media/v4l/controls.xml +5 -0

+	    image data is produced. The unit of horizontal blanking is
+	    pixels.</entry>
+	  </row>
+	  <row>
+	    <entry

spanname="id"><constant>V4L2_CID_IMAGE_SOURCE_LINK_FREQ</constant></entr
y> + <entry>integer menu</entry>

Documentation/DocBook/media/v4l/controls.xml +3 -0

+	  </row>
+	  <row>
+	    <entry spanname="descr">Image source's data bus frequency.

What's the frequency unit ? Sample/second ?

Hz --- that's the unit of frequency. I fixed that in the new version.

Quoted 13 lines

Quoted 13 lines

Documentation/DocBook/media/v4l/controls.xml +11 -0

+	    Together with the media bus pixel code, bus type (clock
+	    cycles per sample), the data bus frequency defines the
+	    pixel clock. <xref linkend="v4l2-mbus-framefmt" /> The
+	    frame rate can be calculated from the pixel clock, image
+	    width and height and horizontal and vertical blanking. The
+	    frame rate control is performed by selecting the desired
+	    horizontal and vertical blanking.
+	    </entry>
+	  </row>
+	  <row>
+	    <entry

spanname="id"><constant>V4L2_CID_IMAGE_SOURCE_ANALOGUE_GAIN</constant></
en try> + <entry>integer</entry>

Quoted 8 lines

Documentation/DocBook/media/v4l/controls.xml interface and may change in the future.</para> · line 3450

Quoted 6 lines

Documentation/DocBook/media/v4l/controls.xml +6 -0

+	  </row>
+	  <row>
+	    <entry spanname="descr">Analogue gain is gain affecting
+	    all colour components in the pixel matrix. The gain
+	    operation is performed in the analogue domain before A/D
+	    conversion.

Should we define one gain per color component ?

I think that in the end we may need up to six analogue gains:

- Gain for all components

- Blue gain
- Red gain
- Green gain (for both greens)
- Gr gain
- Gb gain

It may be debatable whether Gr / Gb gain will always be the same or not. I'm not fully certain about that. As Hans G. suggested, it might be possible to go with just one for green.

Quoted 49 lines

Quoted 49 lines

Documentation/DocBook/media/v4l/controls.xml +9 -0

+	    </entry>
+	  </row>
+	  <row><entry></entry></row>
+	</tbody>
+      </tgroup>
+      </table>
+
+    </section>
+
 
 </section>
 
   <!--

(quoted diff) +0 -0

b/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml index 5122ce8..250c1cf 100644

Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml +6 -0

@@ -257,6 +257,12 @@ These controls are described in <xref
 
 These controls are described in <xref
 
 		linkend="flash-controls" />.</entry>
 
 	  </row>
 
+	  <row>
+	    <entry><constant>V4L2_CTRL_CLASS_IMAGE_SOURCE</constant></entry>
+	    <entry>0x9d0000</entry> <entry>The class containing image
+	    source controls. These controls are described in <xref
+	    linkend="image-source-controls" />.</entry>
+	  </row>
 
 	</tbody>
 
       </tgroup>
 
     </table>

(quoted diff) +0 -0

b/drivers/media/video/v4l2-ctrls.c index 083bb79..da1ec52 100644

drivers/media/video/v4l2-ctrls.c +0 -0

@@ -606,6 +606,12 @@ const char *v4l2_ctrl_get_name(u32 id)
 
 	case V4L2_CID_FLASH_CHARGE:		return "Charge";
 	case V4L2_CID_FLASH_READY:		return "Ready to strobe";
 

Quoted 7 lines

drivers/media/video/v4l2-ctrls.c const char *v4l2_ctrl_get_name(u32 id) · line 613

Quoted 5 lines

drivers/media/video/v4l2-ctrls.c +5 -0

+	case V4L2_CID_IMAGE_SOURCE_CLASS:	return "Image source controls";
+	case V4L2_CID_IMAGE_SOURCE_VBLANK:	return "Vertical blanking";
+	case V4L2_CID_IMAGE_SOURCE_HBLANK:	return "Horizontal blanking";
+	case V4L2_CID_IMAGE_SOURCE_LINK_FREQ:	return "Link frequency";
+	case V4L2_CID_IMAGE_SOURCE_ANALOGUE_GAIN: return "Analogue gain";

Please capitalize each word, as done for the other controls.

This isn't done for the flash controls either, have you noticed that?

Well, I guess I have to admit that they were added by myself. ;-)

I can fix this for the next patchset.

Quoted 11 lines

Quoted 11 lines

drivers/media/video/v4l2-ctrls.c +0 -0

 	default:
 		return NULL;
 
 	}

@@ -694,6 +700,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum
 

v4l2_ctrl_type *type, case V4L2_CID_MPEG_VIDEO_MPEG4_PROFILE:

drivers/media/video/v4l2-ctrls.c +0 -0

 		*type = V4L2_CTRL_TYPE_MENU;
 		break;

Quoted 6 lines

drivers/media/video/v4l2-ctrls.c void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type, · line 705

Quoted 3 lines

drivers/media/video/v4l2-ctrls.c +3 -0

+	case V4L2_CID_IMAGE_SOURCE_LINK_FREQ:
+		*type = V4L2_CTRL_TYPE_INTEGER_MENU;
+		break;

Will this always be an integer menu control, or can you foresee cases where the range would be continuous ?

Good question. On some hardware this definitely is an integer menu, but hardware may exist where more selections would be available.