[RFC PATCH 0/5] Matrix and Motion Detection support

[RFC PATCH 0/5] Matrix and Motion Detection support

[Hans Verkuil]

This patch series adds support for matrices and motion detection and
converts the solo6x10 driver to use these new APIs.

See the RFCv2 for details on the motion detection API:

http://www.mail-archive.com/linux-media@vger.kernel.org/msg62085.html

And this RFC for details on the matrix API (which superseeds the v4l2_md_blocks
in the RFC above):

http://permalink.gmane.org/gmane.linux.drivers.video-input-infrastructure/65195

I have tested this with the solo card, both global motion detection and
regional motion detection, and it works well.

There is no documentation for the new APIs yet (other than the RFCs). I would
like to know what others think of this proposal before I start work on the
DocBook documentation.

My tentative goal is to get this in for 3.12. Once this is in place the solo
and go7007 drivers can be moved out of staging into the mainline since this is
the only thing holding them back.

Regards,

	Hans

[RFC PATCH 1/5] v4l2: add matrix support.

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

This patch adds core support for matrices: querying, getting and setting.

Two initial matrix types are defined for motion detection (defining regions
and thresholds).

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/v4l2-dev.c   |  3 ++
 drivers/media/v4l2-core/v4l2-ioctl.c | 23 ++++++++++++-
 include/media/v4l2-ioctl.h           |  8 +++++
 include/uapi/linux/videodev2.h       | 64 ++++++++++++++++++++++++++++++++++++
 4 files changed, 97 insertions(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index c8859d6..5e58df6 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -598,6 +598,9 @@ static void determine_valid_ioctls(struct video_device *vdev)
 	SET_VALID_IOCTL(ops, VIDIOC_UNSUBSCRIBE_EVENT, vidioc_unsubscribe_event);
 	if (ops->vidioc_enum_freq_bands || ops->vidioc_g_tuner || ops->vidioc_g_modulator)
 		set_bit(_IOC_NR(VIDIOC_ENUM_FREQ_BANDS), valid_ioctls);
+	SET_VALID_IOCTL(ops, VIDIOC_QUERY_MATRIX, vidioc_query_matrix);
+	SET_VALID_IOCTL(ops, VIDIOC_G_MATRIX, vidioc_g_matrix);
+	SET_VALID_IOCTL(ops, VIDIOC_S_MATRIX, vidioc_s_matrix);
 
 	if (is_vid) {
 		/* video specific ioctls */
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 68e6b5e..47debfc 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -549,7 +549,7 @@ static void v4l_print_cropcap(const void *arg, bool write_only)
 	const struct v4l2_cropcap *p = arg;
 
 	pr_cont("type=%s, bounds wxh=%dx%d, x,y=%d,%d, "
-		"defrect wxh=%dx%d, x,y=%d,%d\n, "
+		"defrect wxh=%dx%d, x,y=%d,%d, "
 		"pixelaspect %d/%d\n",
 		prt_names(p->type, v4l2_type_names),
 		p->bounds.width, p->bounds.height,
@@ -831,6 +831,24 @@ static void v4l_print_freq_band(const void *arg, bool write_only)
 			p->rangehigh, p->modulation);
 }
 
+static void v4l_print_query_matrix(const void *arg, bool write_only)
+{
+	const struct v4l2_query_matrix *p = arg;
+
+	pr_cont("type=0x%x, columns=%u, rows=%u, elem_min=%lld, elem_max=%lld, elem_size=%u\n",
+			p->type, p->columns, p->rows,
+			p->elem_min.val, p->elem_max.val, p->elem_size);
+}
+
+static void v4l_print_matrix(const void *arg, bool write_only)
+{
+	const struct v4l2_matrix *p = arg;
+
+	pr_cont("type=0x%x, wxh=%dx%d, x,y=%d,%d, matrix=%p\n",
+			p->type, p->rect.width, p->rect.height,
+			p->rect.top, p->rect.left, p->matrix);
+}
+
 static void v4l_print_u32(const void *arg, bool write_only)
 {
 	pr_cont("value=%u\n", *(const u32 *)arg);
@@ -2055,6 +2073,9 @@ static struct v4l2_ioctl_info v4l2_ioctls[] = {
 	IOCTL_INFO_STD(VIDIOC_DV_TIMINGS_CAP, vidioc_dv_timings_cap, v4l_print_dv_timings_cap, INFO_FL_CLEAR(v4l2_dv_timings_cap, type)),
 	IOCTL_INFO_FNC(VIDIOC_ENUM_FREQ_BANDS, v4l_enum_freq_bands, v4l_print_freq_band, 0),
 	IOCTL_INFO_FNC(VIDIOC_DBG_G_CHIP_INFO, v4l_dbg_g_chip_info, v4l_print_dbg_chip_info, INFO_FL_CLEAR(v4l2_dbg_chip_info, match)),
+	IOCTL_INFO_STD(VIDIOC_QUERY_MATRIX, vidioc_query_matrix, v4l_print_query_matrix, INFO_FL_CLEAR(v4l2_query_matrix, ref)),
+	IOCTL_INFO_STD(VIDIOC_G_MATRIX, vidioc_g_matrix, v4l_print_matrix, INFO_FL_CLEAR(v4l2_matrix, matrix)),
+	IOCTL_INFO_STD(VIDIOC_S_MATRIX, vidioc_s_matrix, v4l_print_matrix, INFO_FL_PRIO | INFO_FL_CLEAR(v4l2_matrix, matrix)),
 };
 #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
 
diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
index e0b74a4..7e4538e 100644
--- a/include/media/v4l2-ioctl.h
+++ b/include/media/v4l2-ioctl.h
@@ -271,6 +271,14 @@ struct v4l2_ioctl_ops {
 	int (*vidioc_unsubscribe_event)(struct v4l2_fh *fh,
 					const struct v4l2_event_subscription *sub);
 
+	/* Matrix ioctls */
+	int (*vidioc_query_matrix) (struct file *file, void *fh,
+				    struct v4l2_query_matrix *qmatrix);
+	int (*vidioc_g_matrix) (struct file *file, void *fh,
+				    struct v4l2_matrix *matrix);
+	int (*vidioc_s_matrix) (struct file *file, void *fh,
+				    struct v4l2_matrix *matrix);
+
 	/* For other private ioctls */
 	long (*vidioc_default)	       (struct file *file, void *fh,
 					bool valid_prio, unsigned int cmd, void *arg);
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 95ef455..5cbe815 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -1838,6 +1838,64 @@ struct v4l2_create_buffers {
 	__u32			reserved[8];
 };
 
+/* Define to which motion detection region each element belongs.
+ * Each element is a __u8. */
+#define V4L2_MATRIX_TYPE_MD_REGION     (1)
+/* Define the motion detection threshold for each element.
+ * Each element is a __u16. */
+#define V4L2_MATRIX_TYPE_MD_THRESHOLD  (2)
+
+/**
+ * struct v4l2_query_matrix - VIDIOC_QUERY_MATRIX argument
+ * @type:	matrix type
+ * @ref:	reference to some object (if any) owning the matrix
+ * @columns:	number of columns in the matrix
+ * @rows:	number of rows in the matrix
+ * @elem_min:	minimum matrix element value
+ * @elem_max:	maximum matrix element value
+ * @elem_size:	size in bytes each matrix element
+ * @reserved:	future extensions, applications and drivers must zero this.
+ */
+struct v4l2_query_matrix {
+	__u32 type;
+	union {
+		__u32 reserved[4];
+	} ref;
+	__u32 columns;
+	__u32 rows;
+	union {
+		__s64 val;
+		__u64 uval;
+		__u32 reserved[4];
+	} elem_min;
+	union {
+		__s64 val;
+		__u64 uval;
+		__u32 reserved[4];
+	} elem_max;
+	__u32 elem_size;
+	__u32 reserved[12];
+} __attribute__ ((packed));
+
+/**
+ * struct v4l2_matrix - VIDIOC_G/S_MATRIX argument
+ * @type:	matrix type
+ * @ref:	reference to some object (if any) owning the matrix
+ * @rect:	which part of the matrix to get/set
+ * @matrix:	pointer to the matrix of size (in bytes):
+ *		elem_size * rect.width * rect.height
+ * @reserved:	future extensions, applications and drivers must zero this.
+ */
+struct v4l2_matrix {
+	__u32 type;
+	union {
+		__u32 reserved[4];
+	} ref;
+	struct v4l2_rect rect;
+	void __user *matrix;
+	__u32 reserved[12];
+} __attribute__ ((packed));
+
 /*
  *	I O C T L   C O D E S   F O R   V I D E O   D E V I C E S
  *
@@ -1946,6 +2004,12 @@ struct v4l2_create_buffers {
    Never use these in applications! */
 #define VIDIOC_DBG_G_CHIP_INFO  _IOWR('V', 102, struct v4l2_dbg_chip_info)
 
+/* Experimental, these three ioctls may change over the next couple of kernel
+   versions. */
+#define VIDIOC_QUERY_MATRIX	_IOWR('V', 103, struct v4l2_query_matrix)
+#define VIDIOC_G_MATRIX		_IOWR('V', 104, struct v4l2_matrix)
+#define VIDIOC_S_MATRIX		_IOWR('V', 105, struct v4l2_matrix)
+
 /* Reminder: when adding new ioctls please add support for them to
    drivers/media/video/v4l2-compat-ioctl32.c as well! */
 
-- 
1.8.3.1

[RFC PATCH 2/5] v4l2-compat-ioctl32: add g/s_matrix support.

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 55 +++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index 8f7a6a4..64155b1 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -777,6 +777,44 @@ static int put_v4l2_subdev_edid32(struct v4l2_subdev_edid *kp, struct v4l2_subde
 	return 0;
 }
 
+struct v4l2_matrix32 {
+	__u32 type;
+	union {
+		__u32 reserved[4];
+	} ref;
+	struct v4l2_rect rect;
+	compat_caddr_t matrix;
+	__u32 reserved[12];
+} __attribute__ ((packed));
+
+static int get_v4l2_matrix32(struct v4l2_matrix *kp, struct v4l2_matrix32 __user *up)
+{
+	u32 tmp;
+
+	if (!access_ok(VERIFY_READ, up, sizeof(struct v4l2_matrix32)) ||
+		get_user(kp->type, &up->type) ||
+		copy_from_user(&kp->ref, &up->ref, sizeof(up->ref)) ||
+		copy_from_user(&kp->rect, &up->rect, sizeof(up->rect)) ||
+		get_user(tmp, &up->matrix) ||
+		copy_from_user(kp->reserved, up->reserved, sizeof(kp->reserved)))
+			return -EFAULT;
+	kp->matrix = compat_ptr(tmp);
+	return 0;
+}
+
+static int put_v4l2_matrix32(struct v4l2_matrix *kp, struct v4l2_matrix32 __user *up)
+{
+	u32 tmp = (u32)((unsigned long)kp->matrix);
+
+	if (!access_ok(VERIFY_WRITE, up, sizeof(struct v4l2_matrix32)) ||
+		put_user(kp->type, &up->type) ||
+		copy_to_user(&kp->ref, &up->ref, sizeof(kp->ref)) ||
+		copy_to_user(&kp->rect, &up->rect, sizeof(kp->rect)) ||
+		put_user(tmp, &up->matrix) ||
+		copy_to_user(kp->reserved, up->reserved, sizeof(kp->reserved)))
+			return -EFAULT;
+	return 0;
+}
 
 #define VIDIOC_G_FMT32		_IOWR('V',  4, struct v4l2_format32)
 #define VIDIOC_S_FMT32		_IOWR('V',  5, struct v4l2_format32)
@@ -796,6 +834,8 @@ static int put_v4l2_subdev_edid32(struct v4l2_subdev_edid *kp, struct v4l2_subde
 #define	VIDIOC_DQEVENT32	_IOR ('V', 89, struct v4l2_event32)
 #define VIDIOC_CREATE_BUFS32	_IOWR('V', 92, struct v4l2_create_buffers32)
 #define VIDIOC_PREPARE_BUF32	_IOWR('V', 93, struct v4l2_buffer32)
+#define VIDIOC_G_MATRIX32	_IOWR('V', 104, struct v4l2_matrix32)
+#define VIDIOC_S_MATRIX32	_IOWR('V', 105, struct v4l2_matrix32)
 
 #define VIDIOC_OVERLAY32	_IOW ('V', 14, s32)
 #define VIDIOC_STREAMON32	_IOW ('V', 18, s32)
@@ -817,6 +857,7 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 		struct v4l2_event v2ev;
 		struct v4l2_create_buffers v2crt;
 		struct v4l2_subdev_edid v2edid;
+		struct v4l2_matrix v2matrix;
 		unsigned long vx;
 		int vi;
 	} karg;
@@ -851,6 +892,8 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 	case VIDIOC_PREPARE_BUF32: cmd = VIDIOC_PREPARE_BUF; break;
 	case VIDIOC_SUBDEV_G_EDID32: cmd = VIDIOC_SUBDEV_G_EDID; break;
 	case VIDIOC_SUBDEV_S_EDID32: cmd = VIDIOC_SUBDEV_S_EDID; break;
+	case VIDIOC_G_MATRIX32: cmd = VIDIOC_G_MATRIX; break;
+	case VIDIOC_S_MATRIX32: cmd = VIDIOC_S_MATRIX; break;
 	}
 
 	switch (cmd) {
@@ -922,6 +965,12 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 	case VIDIOC_DQEVENT:
 		compatible_arg = 0;
 		break;
+
+	case VIDIOC_G_MATRIX:
+	case VIDIOC_S_MATRIX:
+		err = get_v4l2_matrix32(&karg.v2matrix, up);
+		compatible_arg = 0;
+		break;
 	}
 	if (err)
 		return err;
@@ -994,6 +1043,11 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 	case VIDIOC_ENUMINPUT:
 		err = put_v4l2_input32(&karg.v2i, up);
 		break;
+
+	case VIDIOC_G_MATRIX:
+	case VIDIOC_S_MATRIX:
+		err = put_v4l2_matrix32(&karg.v2matrix, up);
+		break;
 	}
 	return err;
 }
@@ -1089,6 +1143,7 @@ long v4l2_compat_ioctl32(struct file *file, unsigned int cmd, unsigned long arg)
 	case VIDIOC_ENUM_FREQ_BANDS:
 	case VIDIOC_SUBDEV_G_EDID32:
 	case VIDIOC_SUBDEV_S_EDID32:
+	case VIDIOC_QUERY_MATRIX:
 		ret = do_video_ioctl(file, cmd, arg);
 		break;
 
-- 
1.8.3.1

[RFC PATCH 3/5] solo: implement the new matrix ioctls instead of the custom ones.

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c | 102 ++++++++++++++++++---
 drivers/staging/media/solo6x10/solo6x10.h          |  10 +-
 2 files changed, 89 insertions(+), 23 deletions(-)

diff --git a/drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c b/drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c
index a4c5896..2058f4d 100644
--- a/drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c
+++ b/drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c
@@ -1033,29 +1033,98 @@ static int solo_s_parm(struct file *file, void *priv,
 	return solo_g_parm(file, priv, sp);
 }
 
-static long solo_enc_default(struct file *file, void *fh,
-			bool valid_prio, unsigned int cmd, void *arg)
+static int solo_query_matrix(struct file *file, void *fh,
+			struct v4l2_query_matrix *qm)
+{
+	qm->columns = 45;
+	qm->rows = 36;
+	switch (qm->type) {
+	case V4L2_MATRIX_TYPE_MD_REGION:
+		qm->elem_size = 1;
+		break;
+	case V4L2_MATRIX_TYPE_MD_THRESHOLD:
+		qm->elem_max.val = 65535;
+		qm->elem_size = 2;
+		break;
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static int solo_g_matrix(struct file *file, void *fh,
+			struct v4l2_matrix *m)
+{
+	struct solo_enc_dev *solo_enc = video_drvdata(file);
+	int w = m->rect.width;
+	int h = m->rect.height;
+	u16 *mt;
+	int y;
+
+	if (m->rect.top < 0 || m->rect.top + h > 35 || h < 0 || w < 0 ||
+	    m->rect.left < 0 || m->rect.left + w >= SOLO_MOTION_SZ)
+		return -EINVAL;
+	if (h == 0 || w == 0)
+		return 0;
+
+	switch (m->type) {
+	case V4L2_MATRIX_TYPE_MD_REGION:
+		return clear_user(m->matrix, w * h);
+	case V4L2_MATRIX_TYPE_MD_THRESHOLD:
+		mt = &solo_enc->motion_thresholds.thresholds[m->rect.top][m->rect.left];
+		for (y = 0; y < h; y++, mt += SOLO_MOTION_SZ)
+			if (copy_to_user(m->matrix + y * w * 2, mt, w * 2))
+				return -EFAULT;
+		break;
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static int solo_s_matrix(struct file *file, void *fh,
+			struct v4l2_matrix *m)
 {
 	struct solo_enc_dev *solo_enc = video_drvdata(file);
 	struct solo_dev *solo_dev = solo_enc->solo_dev;
-	struct solo_motion_thresholds *thresholds = arg;
+	int w = m->rect.width;
+	int h = m->rect.height;
+	u16 *mt;
+	int y;
 
-	switch (cmd) {
-	case SOLO_IOC_G_MOTION_THRESHOLDS:
-		*thresholds = solo_enc->motion_thresholds;
+	if (m->rect.top < 0 || m->rect.top + h > 35 || h < 0 || w < 0 ||
+	    m->rect.left < 0 || m->rect.left + w >= SOLO_MOTION_SZ)
+		return -EINVAL;
+	if (h == 0 || w == 0)
 		return 0;
 
-	case SOLO_IOC_S_MOTION_THRESHOLDS:
-		if (!valid_prio)
-			return -EBUSY;
-		solo_enc->motion_thresholds = *thresholds;
-		if (solo_enc->motion_enabled && !solo_enc->motion_global)
-			return solo_set_motion_block(solo_dev, solo_enc->ch,
-						&solo_enc->motion_thresholds);
+	switch (m->type) {
+	case V4L2_MATRIX_TYPE_MD_REGION:
+		/* Check that the region matrix is all zeroes */
+		for (y = 0; y < h; y++) {
+			u8 region[SOLO_MOTION_SZ];
+			static const u8 zeroes[SOLO_MOTION_SZ];
+
+			if (copy_from_user(region, m->matrix + y * w, w))
+				return -EFAULT;
+			if (memcmp(region, zeroes, w))
+				return -EINVAL;
+		}
 		return 0;
+	case V4L2_MATRIX_TYPE_MD_THRESHOLD:
+		mt = &solo_enc->motion_thresholds.thresholds[m->rect.top][m->rect.left];
+		for (y = 0; y < h; y++, mt += SOLO_MOTION_SZ)
+			if (copy_from_user(mt, m->matrix + y * w * 2, w * 2))
+				return -EFAULT;
+		break;
 	default:
-		return -ENOTTY;
+		return -EINVAL;
 	}
+
+	if (solo_enc->motion_enabled && !solo_enc->motion_global)
+		return solo_set_motion_block(solo_dev, solo_enc->ch,
+				&solo_enc->motion_thresholds);
+	return 0;
 }
 
 static int solo_s_ctrl(struct v4l2_ctrl *ctrl)
@@ -1141,11 +1210,14 @@ static const struct v4l2_ioctl_ops solo_enc_ioctl_ops = {
 	/* Video capture parameters */
 	.vidioc_s_parm			= solo_s_parm,
 	.vidioc_g_parm			= solo_g_parm,
+	/* Motion Detection matrices */
+	.vidioc_query_matrix		= solo_query_matrix,
+	.vidioc_g_matrix		= solo_g_matrix,
+	.vidioc_s_matrix		= solo_s_matrix,
 	/* Logging and events */
 	.vidioc_log_status		= v4l2_ctrl_log_status,
 	.vidioc_subscribe_event		= v4l2_ctrl_subscribe_event,
 	.vidioc_unsubscribe_event	= v4l2_event_unsubscribe,
-	.vidioc_default			= solo_enc_default,
 };
 
 static const struct video_device solo_enc_template = {
diff --git a/drivers/staging/media/solo6x10/solo6x10.h b/drivers/staging/media/solo6x10/solo6x10.h
index 6f91d2e..01c8655 100644
--- a/drivers/staging/media/solo6x10/solo6x10.h
+++ b/drivers/staging/media/solo6x10/solo6x10.h
@@ -114,20 +114,14 @@
  * effect, 44x30 samples are used for NTSC, and 44x36 for PAL.
  * The 5th sample on the 10th row is (10*64)+5 = 645.
  *
- * Using a 64x64 array will result in a problem on some architectures like
- * the powerpc where the size of the argument is limited to 13 bits.
- * Since both PAL and NTSC do not use the full table anyway I've chosen
- * to limit the array to 45x45 (45*16 = 720, which is the maximum PAL/NTSC
- * width).
+ * Internally it is stored as a 45x45 array (45*16 = 720, which is the
+ * maximum PAL/NTSC width).
  */
 #define SOLO_MOTION_SZ (45)
 struct solo_motion_thresholds {
 	__u16	thresholds[SOLO_MOTION_SZ][SOLO_MOTION_SZ];
 };
 
-#define SOLO_IOC_G_MOTION_THRESHOLDS	_IOR('V', BASE_VIDIOC_PRIVATE+0, struct solo_motion_thresholds)
-#define SOLO_IOC_S_MOTION_THRESHOLDS	_IOW('V', BASE_VIDIOC_PRIVATE+1, struct solo_motion_thresholds)
-
 enum SOLO_I2C_STATE {
 	IIC_STATE_IDLE,
 	IIC_STATE_START,
-- 
1.8.3.1

[RFC PATCH 4/5] v4l2: add a motion detection event.

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 include/uapi/linux/videodev2.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 5cbe815..f926209 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -1721,6 +1721,7 @@ struct v4l2_streamparm {
 #define V4L2_EVENT_EOS				2
 #define V4L2_EVENT_CTRL				3
 #define V4L2_EVENT_FRAME_SYNC			4
+#define V4L2_EVENT_MOTION_DET			5
 #define V4L2_EVENT_PRIVATE_START		0x08000000
 
 /* Payload for V4L2_EVENT_VSYNC */
@@ -1752,12 +1753,28 @@ struct v4l2_event_frame_sync {
 	__u32 frame_sequence;
 };
 
+#define V4L2_EVENT_MD_FL_HAVE_FRAME_SEQ	(1 << 0)
+
+/**
+ * struct v4l2_event_motion_det - motion detection event
+ * @flags:             if V4L2_EVENT_MD_FL_HAVE_FRAME_SEQ is set, then the
+ *                     frame_sequence field is valid.
+ * @frame_sequence:    the frame sequence number associated with this event.
+ * @region_mask:       which regions detected motion.
+ */
+struct v4l2_event_motion_det {
+	__u32 flags;
+	__u32 frame_sequence;
+	__u32 region_mask;
+};
+
 struct v4l2_event {
 	__u32				type;
 	union {
 		struct v4l2_event_vsync		vsync;
 		struct v4l2_event_ctrl		ctrl;
 		struct v4l2_event_frame_sync	frame_sync;
+		struct v4l2_event_motion_det	motion_det;
 		__u8				data[64];
 	} u;
 	__u32				pending;
-- 
1.8.3.1

[RFC PATCH 5/5] solo6x10: implement motion detection events and controls.

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c | 117 +++++++++++++--------
 drivers/staging/media/solo6x10/solo6x10.h          |   9 +-
 2 files changed, 74 insertions(+), 52 deletions(-)

diff --git a/drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c b/drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c
index 2058f4d..6e8025c 100644
--- a/drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c
+++ b/drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c
@@ -270,6 +270,8 @@ static int solo_enc_on(struct solo_enc_dev *solo_enc)
 	if (solo_enc->bw_weight > solo_dev->enc_bw_remain)
 		return -EBUSY;
 	solo_enc->sequence = 0;
+	solo_enc->motion_last_state = false;
+	solo_enc->frames_since_last_motion = 0;
 	solo_dev->enc_bw_remain -= solo_enc->bw_weight;
 
 	if (solo_enc->type == SOLO_ENC_TYPE_EXT)
@@ -510,15 +512,6 @@ static int solo_enc_fillbuf(struct solo_enc_dev *solo_enc,
 	struct vop_header *vh = enc_buf->vh;
 	int ret;
 
-	/* Check for motion flags */
-	vb->v4l2_buf.flags &= ~(V4L2_BUF_FLAG_MOTION_ON |
-				V4L2_BUF_FLAG_MOTION_DETECTED);
-	if (solo_is_motion_on(solo_enc)) {
-		vb->v4l2_buf.flags |= V4L2_BUF_FLAG_MOTION_ON;
-		if (enc_buf->motion)
-			vb->v4l2_buf.flags |= V4L2_BUF_FLAG_MOTION_DETECTED;
-	}
-
 	switch (solo_enc->fmt) {
 	case V4L2_PIX_FMT_MPEG4:
 	case V4L2_PIX_FMT_H264:
@@ -530,9 +523,49 @@ static int solo_enc_fillbuf(struct solo_enc_dev *solo_enc,
 	}
 
 	if (!ret) {
+		bool send_event = false;
+
 		vb->v4l2_buf.sequence = solo_enc->sequence++;
 		vb->v4l2_buf.timestamp.tv_sec = vh->sec;
 		vb->v4l2_buf.timestamp.tv_usec = vh->usec;
+
+		/* Check for motion flags */
+		if (solo_is_motion_on(solo_enc)) {
+			/* It takes a few frames for the hardware to detect
+			 * motion. Once it does it clears the motion detection
+			 * register and it takes again a few frames before
+			 * motion is seen. This means in practice that when the
+			 * motion field is 1, it will go back to 0 for the next
+			 * frame. This leads to motion detection event being
+			 * sent all the time, which is not what we want.
+			 * Instead wait a few frames before deciding that the
+			 * motion has halted. After some experimentation it
+			 * turns out that waiting for 5 frames works well.
+			 */
+			if (enc_buf->motion == 0 &&
+			    solo_enc->motion_last_state &&
+			    solo_enc->frames_since_last_motion++ > 5)
+				send_event = true;
+			else if (enc_buf->motion) {
+				solo_enc->frames_since_last_motion = 0;
+				send_event = !solo_enc->motion_last_state;
+			}
+		}
+
+		if (send_event) {
+			struct v4l2_event ev = {
+				.type = V4L2_EVENT_MOTION_DET,
+				.u.motion_det = {
+					.flags = V4L2_EVENT_MD_FL_HAVE_FRAME_SEQ,
+					.frame_sequence = vb->v4l2_buf.sequence,
+					.region_mask = enc_buf->motion ? 1 : 0,
+				},
+			};
+
+			solo_enc->motion_last_state = enc_buf->motion;
+			solo_enc->frames_since_last_motion = 0;
+			v4l2_event_queue(solo_enc->vfd, &ev);
+		}
 	}
 
 	vb2_buffer_done(vb, ret ? VB2_BUF_STATE_ERROR : VB2_BUF_STATE_DONE);
@@ -1145,14 +1178,15 @@ static int solo_s_ctrl(struct v4l2_ctrl *ctrl)
 	case V4L2_CID_MPEG_VIDEO_GOP_SIZE:
 		solo_enc->gop = ctrl->val;
 		return 0;
-	case V4L2_CID_MOTION_THRESHOLD:
-		solo_enc->motion_thresh = ctrl->val;
+	case V4L2_CID_DETECT_MOTION_THRESHOLD:
+		solo_enc->motion_thresh = ctrl->val << 8;
 		if (!solo_enc->motion_global || !solo_enc->motion_enabled)
 			return 0;
-		return solo_set_motion_threshold(solo_dev, solo_enc->ch, ctrl->val);
-	case V4L2_CID_MOTION_MODE:
-		solo_enc->motion_global = ctrl->val == 1;
-		solo_enc->motion_enabled = ctrl->val > 0;
+		return solo_set_motion_threshold(solo_dev, solo_enc->ch,
+				solo_enc->motion_thresh);
+	case V4L2_CID_DETECT_MOTION_MODE:
+		solo_enc->motion_global = ctrl->val == V4L2_DETECT_MOTION_GLOBAL;
+		solo_enc->motion_enabled = ctrl->val > V4L2_DETECT_MOTION_DISABLED;
 		if (ctrl->val) {
 			if (solo_enc->motion_global)
 				solo_set_motion_threshold(solo_dev, solo_enc->ch,
@@ -1174,6 +1208,21 @@ static int solo_s_ctrl(struct v4l2_ctrl *ctrl)
 	return 0;
 }
 
+static int solo_subscribe_event(struct v4l2_fh *fh,
+				const struct v4l2_event_subscription *sub)
+{
+
+	switch (sub->type) {
+	case V4L2_EVENT_CTRL:
+		return v4l2_ctrl_subscribe_event(fh, sub);
+	case V4L2_EVENT_MOTION_DET:
+		/* Allow for up to 30 events (1 second for NTSC) to be
+		 * stored. */
+		return v4l2_event_subscribe(fh, sub, 30, NULL);
+	}
+	return -EINVAL;
+}
+
 static const struct v4l2_file_operations solo_enc_fops = {
 	.owner			= THIS_MODULE,
 	.open			= v4l2_fh_open,
@@ -1216,7 +1265,7 @@ static const struct v4l2_ioctl_ops solo_enc_ioctl_ops = {
 	.vidioc_s_matrix		= solo_s_matrix,
 	/* Logging and events */
 	.vidioc_log_status		= v4l2_ctrl_log_status,
-	.vidioc_subscribe_event		= v4l2_ctrl_subscribe_event,
+	.vidioc_subscribe_event		= solo_subscribe_event,
 	.vidioc_unsubscribe_event	= v4l2_event_unsubscribe,
 };
 
@@ -1233,33 +1282,6 @@ static const struct v4l2_ctrl_ops solo_ctrl_ops = {
 	.s_ctrl = solo_s_ctrl,
 };
 
-static const struct v4l2_ctrl_config solo_motion_threshold_ctrl = {
-	.ops = &solo_ctrl_ops,
-	.id = V4L2_CID_MOTION_THRESHOLD,
-	.name = "Motion Detection Threshold",
-	.type = V4L2_CTRL_TYPE_INTEGER,
-	.max = 0xffff,
-	.def = SOLO_DEF_MOT_THRESH,
-	.step = 1,
-	.flags = V4L2_CTRL_FLAG_SLIDER,
-};
-
-static const char * const solo_motion_mode_menu[] = {
-	"Disabled",
-	"Global Threshold",
-	"Regional Threshold",
-	NULL
-};
-
-static const struct v4l2_ctrl_config solo_motion_enable_ctrl = {
-	.ops = &solo_ctrl_ops,
-	.id = V4L2_CID_MOTION_MODE,
-	.name = "Motion Detection Mode",
-	.type = V4L2_CTRL_TYPE_MENU,
-	.qmenu = solo_motion_mode_menu,
-	.max = 2,
-};
-
 static const struct v4l2_ctrl_config solo_osd_text_ctrl = {
 	.ops = &solo_ctrl_ops,
 	.id = V4L2_CID_OSD_TEXT,
@@ -1296,8 +1318,13 @@ static struct solo_enc_dev *solo_enc_alloc(struct solo_dev *solo_dev,
 			V4L2_CID_SHARPNESS, 0, 15, 1, 0);
 	v4l2_ctrl_new_std(hdl, &solo_ctrl_ops,
 			V4L2_CID_MPEG_VIDEO_GOP_SIZE, 1, 255, 1, solo_dev->fps);
-	v4l2_ctrl_new_custom(hdl, &solo_motion_threshold_ctrl, NULL);
-	v4l2_ctrl_new_custom(hdl, &solo_motion_enable_ctrl, NULL);
+	v4l2_ctrl_new_std_menu(hdl, &solo_ctrl_ops,
+			V4L2_CID_DETECT_MOTION_MODE,
+			V4L2_DETECT_MOTION_REGIONAL, 0,
+			V4L2_DETECT_MOTION_DISABLED);
+	v4l2_ctrl_new_std(hdl, &solo_ctrl_ops,
+			V4L2_CID_DETECT_MOTION_THRESHOLD, 0, 0xff, 1,
+			SOLO_DEF_MOT_THRESH >> 8);
 	v4l2_ctrl_new_custom(hdl, &solo_osd_text_ctrl, NULL);
 	if (hdl->error) {
 		ret = hdl->error;
diff --git a/drivers/staging/media/solo6x10/solo6x10.h b/drivers/staging/media/solo6x10/solo6x10.h
index 01c8655..df34a31 100644
--- a/drivers/staging/media/solo6x10/solo6x10.h
+++ b/drivers/staging/media/solo6x10/solo6x10.h
@@ -97,14 +97,7 @@
 #define SOLO_DEFAULT_GOP		30
 #define SOLO_DEFAULT_QP			3
 
-#ifndef V4L2_BUF_FLAG_MOTION_ON
-#define V4L2_BUF_FLAG_MOTION_ON		0x10000
-#define V4L2_BUF_FLAG_MOTION_DETECTED	0x20000
-#endif
-
 #define SOLO_CID_CUSTOM_BASE		(V4L2_CID_USER_BASE | 0xf000)
-#define V4L2_CID_MOTION_MODE		(SOLO_CID_CUSTOM_BASE+0)
-#define V4L2_CID_MOTION_THRESHOLD	(SOLO_CID_CUSTOM_BASE+1)
 #define V4L2_CID_MOTION_TRACE		(SOLO_CID_CUSTOM_BASE+2)
 #define V4L2_CID_OSD_TEXT		(SOLO_CID_CUSTOM_BASE+3)
 
@@ -174,6 +167,8 @@ struct solo_enc_dev {
 	struct solo_motion_thresholds motion_thresholds;
 	bool			motion_global;
 	bool			motion_enabled;
+	bool			motion_last_state;
+	u8			frames_since_last_motion;
 	u16			width;
 	u16			height;
 
-- 
1.8.3.1

Re: [RFC PATCH 0/5] Matrix and Motion Detection support

[Sylwester Nawrocki]

Hi Hans,

On 06/28/2013 02:27 PM, Hans Verkuil wrote:
> This patch series adds support for matrices and motion detection and
> converts the solo6x10 driver to use these new APIs.
>
> See the RFCv2 for details on the motion detection API:
>
> http://www.mail-archive.com/linux-media@vger.kernel.org/msg62085.html
>
> And this RFC for details on the matrix API (which superseeds the v4l2_md_blocks
> in the RFC above):
>
> http://permalink.gmane.org/gmane.linux.drivers.video-input-infrastructure/65195
>
> I have tested this with the solo card, both global motion detection and
> regional motion detection, and it works well.
>
> There is no documentation for the new APIs yet (other than the RFCs). I would
> like to know what others think of this proposal before I start work on the
> DocBook documentation.

These 3 ioctls look pretty generic and will likely allow us to handle wide
range of functionalities, similarly to what the controls framework does 
today.

What I don't like in the current trend of the V4L2 API development 
though is
that we have seemingly separate APIs for configuring integers, rectangles,
matrices, etc. And interactions between those APIs sometimes happen to be
not well defined.

I'm not opposed to having this matrix API, but I would _much_ more like to
see it as a starting point of a more powerful API, that would allow to 
model
dependencies between parameters being configured and the objects more
explicitly and freely (e.g. case of the per buffer controls), that would
allow to pass a list of commands to the hardware for atomic 
re-configurations,
that would allow to create hardware configuration contexts, etc., etc.

But it's all song of future, requires lots of effort, founding and takes
engineers with significant experience.

As it likely won't happen soon I guess we can proceed with the matrix API
for now.

> My tentative goal is to get this in for 3.12. Once this is in place the solo
> and go7007 drivers can be moved out of staging into the mainline since this is
> the only thing holding them back.

--
Regards,
Sylwester

Re: [RFC PATCH 1/5] v4l2: add matrix support.

[Sylwester Nawrocki]

On 06/28/2013 02:27 PM, Hans Verkuil wrote:
> From: Hans Verkuil<hans.verkuil@cisco.com>
>
> This patch adds core support for matrices: querying, getting and setting.
>
> Two initial matrix types are defined for motion detection (defining regions
> and thresholds).
>
> Signed-off-by: Hans Verkuil<hans.verkuil@cisco.com>
> ---
>   drivers/media/v4l2-core/v4l2-dev.c   |  3 ++
>   drivers/media/v4l2-core/v4l2-ioctl.c | 23 ++++++++++++-
>   include/media/v4l2-ioctl.h           |  8 +++++
>   include/uapi/linux/videodev2.h       | 64 ++++++++++++++++++++++++++++++++++++
>   4 files changed, 97 insertions(+), 1 deletion(-)

[...]

> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
> index e0b74a4..7e4538e 100644
> --- a/include/media/v4l2-ioctl.h
> +++ b/include/media/v4l2-ioctl.h
> @@ -271,6 +271,14 @@ struct v4l2_ioctl_ops {
>   	int (*vidioc_unsubscribe_event)(struct v4l2_fh *fh,
>   					const struct v4l2_event_subscription *sub);
>
> +	/* Matrix ioctls */
> +	int (*vidioc_query_matrix) (struct file *file, void *fh,
> +				    struct v4l2_query_matrix *qmatrix);
> +	int (*vidioc_g_matrix) (struct file *file, void *fh,
> +				    struct v4l2_matrix *matrix);
> +	int (*vidioc_s_matrix) (struct file *file, void *fh,
> +				    struct v4l2_matrix *matrix);
> +
>   	/* For other private ioctls */
>   	long (*vidioc_default)	       (struct file *file, void *fh,
>   					bool valid_prio, unsigned int cmd, void *arg);
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 95ef455..5cbe815 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -1838,6 +1838,64 @@ struct v4l2_create_buffers {
>   	__u32			reserved[8];
>   };
>
> +/* Define to which motion detection region each element belongs.
> + * Each element is a __u8. */
> +#define V4L2_MATRIX_TYPE_MD_REGION     (1)
> +/* Define the motion detection threshold for each element.
> + * Each element is a __u16. */
> +#define V4L2_MATRIX_TYPE_MD_THRESHOLD  (2)
> +
> +/**
> + * struct v4l2_query_matrix - VIDIOC_QUERY_MATRIX argument
> + * @type:	matrix type
> + * @ref:	reference to some object (if any) owning the matrix
> + * @columns:	number of columns in the matrix
> + * @rows:	number of rows in the matrix
> + * @elem_min:	minimum matrix element value
> + * @elem_max:	maximum matrix element value
> + * @elem_size:	size in bytes each matrix element
> + * @reserved:	future extensions, applications and drivers must zero this.
> + */
> +struct v4l2_query_matrix {
> +	__u32 type;
> +	union {
> +		__u32 reserved[4];
> +	} ref;
> +	__u32 columns;
> +	__u32 rows;
> +	union {
> +		__s64 val;
> +		__u64 uval;
> +		__u32 reserved[4];
> +	} elem_min;
> +	union {
> +		__s64 val;
> +		__u64 uval;
> +		__u32 reserved[4];
> +	} elem_max;
> +	__u32 elem_size;

How about reordering it to something like:

	struct {
		union {
			__s64 val;
			__u64 uval;
			__u32 reserved[4];
		} min;
		union {
			__s64 val;
			__u64 uval;
			__u32 reserved[4];
		} max;
		__u32 size;
	} element;

?

--
Regards,
Sylwester

Re: [RFC PATCH 1/5] v4l2: add matrix support.

[Hans Verkuil]

On Sun July 7 2013 23:50:51 Sylwester Nawrocki wrote:
> On 06/28/2013 02:27 PM, Hans Verkuil wrote:
> > From: Hans Verkuil<hans.verkuil@cisco.com>
> >
> > This patch adds core support for matrices: querying, getting and setting.
> >
> > Two initial matrix types are defined for motion detection (defining regions
> > and thresholds).
> >
> > Signed-off-by: Hans Verkuil<hans.verkuil@cisco.com>
> > ---
> >   drivers/media/v4l2-core/v4l2-dev.c   |  3 ++
> >   drivers/media/v4l2-core/v4l2-ioctl.c | 23 ++++++++++++-
> >   include/media/v4l2-ioctl.h           |  8 +++++
> >   include/uapi/linux/videodev2.h       | 64 ++++++++++++++++++++++++++++++++++++
> >   4 files changed, 97 insertions(+), 1 deletion(-)
> 
> [...]
> 
> > diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
> > index e0b74a4..7e4538e 100644
> > --- a/include/media/v4l2-ioctl.h
> > +++ b/include/media/v4l2-ioctl.h
> > @@ -271,6 +271,14 @@ struct v4l2_ioctl_ops {
> >   	int (*vidioc_unsubscribe_event)(struct v4l2_fh *fh,
> >   					const struct v4l2_event_subscription *sub);
> >
> > +	/* Matrix ioctls */
> > +	int (*vidioc_query_matrix) (struct file *file, void *fh,
> > +				    struct v4l2_query_matrix *qmatrix);
> > +	int (*vidioc_g_matrix) (struct file *file, void *fh,
> > +				    struct v4l2_matrix *matrix);
> > +	int (*vidioc_s_matrix) (struct file *file, void *fh,
> > +				    struct v4l2_matrix *matrix);
> > +
> >   	/* For other private ioctls */
> >   	long (*vidioc_default)	       (struct file *file, void *fh,
> >   					bool valid_prio, unsigned int cmd, void *arg);
> > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > index 95ef455..5cbe815 100644
> > --- a/include/uapi/linux/videodev2.h
> > +++ b/include/uapi/linux/videodev2.h
> > @@ -1838,6 +1838,64 @@ struct v4l2_create_buffers {
> >   	__u32			reserved[8];
> >   };
> >
> > +/* Define to which motion detection region each element belongs.
> > + * Each element is a __u8. */
> > +#define V4L2_MATRIX_TYPE_MD_REGION     (1)
> > +/* Define the motion detection threshold for each element.
> > + * Each element is a __u16. */
> > +#define V4L2_MATRIX_TYPE_MD_THRESHOLD  (2)
> > +
> > +/**
> > + * struct v4l2_query_matrix - VIDIOC_QUERY_MATRIX argument
> > + * @type:	matrix type
> > + * @ref:	reference to some object (if any) owning the matrix
> > + * @columns:	number of columns in the matrix
> > + * @rows:	number of rows in the matrix
> > + * @elem_min:	minimum matrix element value
> > + * @elem_max:	maximum matrix element value
> > + * @elem_size:	size in bytes each matrix element
> > + * @reserved:	future extensions, applications and drivers must zero this.
> > + */
> > +struct v4l2_query_matrix {
> > +	__u32 type;
> > +	union {
> > +		__u32 reserved[4];
> > +	} ref;
> > +	__u32 columns;
> > +	__u32 rows;
> > +	union {
> > +		__s64 val;
> > +		__u64 uval;
> > +		__u32 reserved[4];
> > +	} elem_min;
> > +	union {
> > +		__s64 val;
> > +		__u64 uval;
> > +		__u32 reserved[4];
> > +	} elem_max;
> > +	__u32 elem_size;
> 
> How about reordering it to something like:
> 
> 	struct {
> 		union {
> 			__s64 val;
> 			__u64 uval;
> 			__u32 reserved[4];
> 		} min;
> 		union {
> 			__s64 val;
> 			__u64 uval;
> 			__u32 reserved[4];
> 		} max;
> 		__u32 size;
> 	} element;
> 
> ?

Makes sense, although I prefer 'elem' over the longer 'element'. Would that
be OK with you?

Regards,

	Hans

Re: [RFC PATCH 0/5] Matrix and Motion Detection support

[Hans Verkuil]

On Sun July 7 2013 23:50:30 Sylwester Nawrocki wrote:
> Hi Hans,
> 
> On 06/28/2013 02:27 PM, Hans Verkuil wrote:
> > This patch series adds support for matrices and motion detection and
> > converts the solo6x10 driver to use these new APIs.
> >
> > See the RFCv2 for details on the motion detection API:
> >
> > http://www.mail-archive.com/linux-media@vger.kernel.org/msg62085.html
> >
> > And this RFC for details on the matrix API (which superseeds the v4l2_md_blocks
> > in the RFC above):
> >
> > http://permalink.gmane.org/gmane.linux.drivers.video-input-infrastructure/65195
> >
> > I have tested this with the solo card, both global motion detection and
> > regional motion detection, and it works well.
> >
> > There is no documentation for the new APIs yet (other than the RFCs). I would
> > like to know what others think of this proposal before I start work on the
> > DocBook documentation.
> 
> These 3 ioctls look pretty generic and will likely allow us to handle wide
> range of functionalities, similarly to what the controls framework does 
> today.
> 
> What I don't like in the current trend of the V4L2 API development 
> though is
> that we have seemingly separate APIs for configuring integers, rectangles,
> matrices, etc. And interactions between those APIs sometimes happen to be
> not well defined.
> 
> I'm not opposed to having this matrix API, but I would _much_ more like to
> see it as a starting point of a more powerful API, that would allow to 
> model
> dependencies between parameters being configured and the objects more
> explicitly and freely (e.g. case of the per buffer controls), that would
> allow to pass a list of commands to the hardware for atomic 
> re-configurations,
> that would allow to create hardware configuration contexts, etc., etc.
> 
> But it's all song of future, requires lots of effort, founding and takes
> engineers with significant experience.
> 
> As it likely won't happen soon I guess we can proceed with the matrix API
> for now.

Do you attend the LPC in New Orleans? I would like to discuss this further,
but it is easier to do so face-to-face with a whiteboard. Alternatively, we
could set up a brainstorm session somewhere. This discussion keeps cropping
up time and again, perhaps we should start to do something about it :-)

Regards,

	Hans

Re: [RFC PATCH 1/5] v4l2: add matrix support.

[Sylwester Nawrocki]

On 07/08/2013 09:15 AM, Hans Verkuil wrote:
> On Sun July 7 2013 23:50:51 Sylwester Nawrocki wrote:
>> On 06/28/2013 02:27 PM, Hans Verkuil wrote:
>>> From: Hans Verkuil<hans.verkuil@cisco.com>
>>>
>>> This patch adds core support for matrices: querying, getting and setting.
>>>
>>> Two initial matrix types are defined for motion detection (defining regions
>>> and thresholds).
>>>
>>> Signed-off-by: Hans Verkuil<hans.verkuil@cisco.com>
>>> ---
>>>   drivers/media/v4l2-core/v4l2-dev.c   |  3 ++
>>>   drivers/media/v4l2-core/v4l2-ioctl.c | 23 ++++++++++++-
>>>   include/media/v4l2-ioctl.h           |  8 +++++
>>>   include/uapi/linux/videodev2.h       | 64 ++++++++++++++++++++++++++++++++++++
>>>   4 files changed, 97 insertions(+), 1 deletion(-)
>>
[...]
>>> +/* Define to which motion detection region each element belongs.
>>> + * Each element is a __u8. */
>>> +#define V4L2_MATRIX_TYPE_MD_REGION     (1)
>>> +/* Define the motion detection threshold for each element.
>>> + * Each element is a __u16. */
>>> +#define V4L2_MATRIX_TYPE_MD_THRESHOLD  (2)
>>> +
>>> +/**
>>> + * struct v4l2_query_matrix - VIDIOC_QUERY_MATRIX argument
>>> + * @type:	matrix type
>>> + * @ref:	reference to some object (if any) owning the matrix
>>> + * @columns:	number of columns in the matrix
>>> + * @rows:	number of rows in the matrix
>>> + * @elem_min:	minimum matrix element value
>>> + * @elem_max:	maximum matrix element value
>>> + * @elem_size:	size in bytes each matrix element
>>> + * @reserved:	future extensions, applications and drivers must zero this.
>>> + */
>>> +struct v4l2_query_matrix {
>>> +	__u32 type;
>>> +	union {
>>> +		__u32 reserved[4];
>>> +	} ref;
>>> +	__u32 columns;
>>> +	__u32 rows;
>>> +	union {
>>> +		__s64 val;
>>> +		__u64 uval;
>>> +		__u32 reserved[4];
>>> +	} elem_min;
>>> +	union {
>>> +		__s64 val;
>>> +		__u64 uval;
>>> +		__u32 reserved[4];
>>> +	} elem_max;
>>> +	__u32 elem_size;
>>
>> How about reordering it to something like:
>>
>> 	struct {
>> 		union {
>> 			__s64 val;
>> 			__u64 uval;
>> 			__u32 reserved[4];
>> 		} min;
>> 		union {
>> 			__s64 val;
>> 			__u64 uval;
>> 			__u32 reserved[4];
>> 		} max;
>> 		__u32 size;
>> 	} element;
>>
>> ?
> 
> Makes sense, although I prefer 'elem' over the longer 'element'. Would that
> be OK with you?

Yes, I'm fine with that. Just thought using full words where sensible is
a good practice. But the shorter form seems better indeed in this case.

Regards,
Sylwester

Re: [RFC PATCH 1/5] v4l2: add matrix support.

[Sakari Ailus]

Hi Hans,

Thanks for the patchset!

On Fri, Jun 28, 2013 at 02:27:30PM +0200, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
>
> This patch adds core support for matrices: querying, getting and setting.
>
> Two initial matrix types are defined for motion detection (defining regions
> and thresholds).
>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/v4l2-core/v4l2-dev.c   |  3 ++
>  drivers/media/v4l2-core/v4l2-ioctl.c | 23 ++++++++++++-
>  include/media/v4l2-ioctl.h           |  8 +++++
>  include/uapi/linux/videodev2.h       | 64 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 97 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index c8859d6..5e58df6 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -598,6 +598,9 @@ static void determine_valid_ioctls(struct video_device *vdev)
>  	SET_VALID_IOCTL(ops, VIDIOC_UNSUBSCRIBE_EVENT, vidioc_unsubscribe_event);
>  	if (ops->vidioc_enum_freq_bands || ops->vidioc_g_tuner || ops->vidioc_g_modulator)
>  		set_bit(_IOC_NR(VIDIOC_ENUM_FREQ_BANDS), valid_ioctls);
> +	SET_VALID_IOCTL(ops, VIDIOC_QUERY_MATRIX, vidioc_query_matrix);
> +	SET_VALID_IOCTL(ops, VIDIOC_G_MATRIX, vidioc_g_matrix);
> +	SET_VALID_IOCTL(ops, VIDIOC_S_MATRIX, vidioc_s_matrix);
>
>  	if (is_vid) {
>  		/* video specific ioctls */
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 68e6b5e..47debfc 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -549,7 +549,7 @@ static void v4l_print_cropcap(const void *arg, bool write_only)
>  	const struct v4l2_cropcap *p = arg;
>
>  	pr_cont("type=%s, bounds wxh=%dx%d, x,y=%d,%d, "
> -		"defrect wxh=%dx%d, x,y=%d,%d\n, "
> +		"defrect wxh=%dx%d, x,y=%d,%d, "
>  		"pixelaspect %d/%d\n",
>  		prt_names(p->type, v4l2_type_names),
>  		p->bounds.width, p->bounds.height,
> @@ -831,6 +831,24 @@ static void v4l_print_freq_band(const void *arg, bool write_only)
>  			p->rangehigh, p->modulation);
>  }
>
> +static void v4l_print_query_matrix(const void *arg, bool write_only)
> +{
> +	const struct v4l2_query_matrix *p = arg;
> +
> +	pr_cont("type=0x%x, columns=%u, rows=%u, elem_min=%lld, elem_max=%lld, elem_size=%u\n",
> +			p->type, p->columns, p->rows,
> +			p->elem_min.val, p->elem_max.val, p->elem_size);
> +}
> +
> +static void v4l_print_matrix(const void *arg, bool write_only)
> +{
> +	const struct v4l2_matrix *p = arg;
> +
> +	pr_cont("type=0x%x, wxh=%dx%d, x,y=%d,%d, matrix=%p\n",
> +			p->type, p->rect.width, p->rect.height,
> +			p->rect.top, p->rect.left, p->matrix);
> +}
> +
>  static void v4l_print_u32(const void *arg, bool write_only)
>  {
>  	pr_cont("value=%u\n", *(const u32 *)arg);
> @@ -2055,6 +2073,9 @@ static struct v4l2_ioctl_info v4l2_ioctls[] = {
>  	IOCTL_INFO_STD(VIDIOC_DV_TIMINGS_CAP, vidioc_dv_timings_cap, v4l_print_dv_timings_cap, INFO_FL_CLEAR(v4l2_dv_timings_cap, type)),
>  	IOCTL_INFO_FNC(VIDIOC_ENUM_FREQ_BANDS, v4l_enum_freq_bands, v4l_print_freq_band, 0),
>  	IOCTL_INFO_FNC(VIDIOC_DBG_G_CHIP_INFO, v4l_dbg_g_chip_info, v4l_print_dbg_chip_info, INFO_FL_CLEAR(v4l2_dbg_chip_info, match)),
> +	IOCTL_INFO_STD(VIDIOC_QUERY_MATRIX, vidioc_query_matrix, v4l_print_query_matrix, INFO_FL_CLEAR(v4l2_query_matrix, ref)),
> +	IOCTL_INFO_STD(VIDIOC_G_MATRIX, vidioc_g_matrix, v4l_print_matrix, INFO_FL_CLEAR(v4l2_matrix, matrix)),
> +	IOCTL_INFO_STD(VIDIOC_S_MATRIX, vidioc_s_matrix, v4l_print_matrix, INFO_FL_PRIO | INFO_FL_CLEAR(v4l2_matrix, matrix)),
>  };
>  #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
>
> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
> index e0b74a4..7e4538e 100644
> --- a/include/media/v4l2-ioctl.h
> +++ b/include/media/v4l2-ioctl.h
> @@ -271,6 +271,14 @@ struct v4l2_ioctl_ops {
>  	int (*vidioc_unsubscribe_event)(struct v4l2_fh *fh,
>  					const struct v4l2_event_subscription *sub);
>
> +	/* Matrix ioctls */
> +	int (*vidioc_query_matrix) (struct file *file, void *fh,
> +				    struct v4l2_query_matrix *qmatrix);
> +	int (*vidioc_g_matrix) (struct file *file, void *fh,
> +				    struct v4l2_matrix *matrix);
> +	int (*vidioc_s_matrix) (struct file *file, void *fh,
> +				    struct v4l2_matrix *matrix);
> +
>  	/* For other private ioctls */
>  	long (*vidioc_default)	       (struct file *file, void *fh,
>  					bool valid_prio, unsigned int cmd, void *arg);
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 95ef455..5cbe815 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -1838,6 +1838,64 @@ struct v4l2_create_buffers {
>  	__u32			reserved[8];
>  };
>
> +/* Define to which motion detection region each element belongs.
> + * Each element is a __u8. */
> +#define V4L2_MATRIX_TYPE_MD_REGION     (1)
> +/* Define the motion detection threshold for each element.
> + * Each element is a __u16. */
> +#define V4L2_MATRIX_TYPE_MD_THRESHOLD  (2)

How about V4L2_MATRIX_T_... instead? These are bound to be quite long.

> +
> +/**
> + * struct v4l2_query_matrix - VIDIOC_QUERY_MATRIX argument
> + * @type:	matrix type
> + * @ref:	reference to some object (if any) owning the matrix
> + * @columns:	number of columns in the matrix
> + * @rows:	number of rows in the matrix
> + * @elem_min:	minimum matrix element value
> + * @elem_max:	maximum matrix element value
> + * @elem_size:	size in bytes each matrix element
> + * @reserved:	future extensions, applications and drivers must zero this.
> + */
> +struct v4l2_query_matrix {
> +	__u32 type;
> +	union {
> +		__u32 reserved[4];
> +	} ref;
> +	__u32 columns;
> +	__u32 rows;

You're assuming two-dimensional matrices. How about array dim[3 or 4] 
and ndim instead of columns and rows?

> +	union {
> +		__s64 val;
> +		__u64 uval;
> +		__u32 reserved[4];

How about "raw" as in e.g. v4l2_format? Reserved suggests to me the 
field is unused, which is not the case.

> +	} elem_min;
> +	union {
> +		__s64 val;
> +		__u64 uval;
> +		__u32 reserved[4];
> +	} elem_max;
> +	__u32 elem_size;
> +	__u32 reserved[12];
> +} __attribute__ ((packed));
> +
> +/**
> + * struct v4l2_matrix - VIDIOC_G/S_MATRIX argument
> + * @type:	matrix type
> + * @ref:	reference to some object (if any) owning the matrix
> + * @rect:	which part of the matrix to get/set
> + * @matrix:	pointer to the matrix of size (in bytes):
> + *		elem_size * rect.width * rect.height
> + * @reserved:	future extensions, applications and drivers must zero this.
> + */
> +struct v4l2_matrix {
> +	__u32 type;
> +	union {
> +		__u32 reserved[4];
> +	} ref;
> +	struct v4l2_rect rect;
> +	void __user *matrix;

This is an interesting idea. Do you have use cases in mind for this?

> +	__u32 reserved[12];
> +} __attribute__ ((packed));
> +
>  /*
>   *	I O C T L   C O D E S   F O R   V I D E O   D E V I C E S
>   *
> @@ -1946,6 +2004,12 @@ struct v4l2_create_buffers {
>     Never use these in applications! */
>  #define VIDIOC_DBG_G_CHIP_INFO  _IOWR('V', 102, struct v4l2_dbg_chip_info)
>
> +/* Experimental, these three ioctls may change over the next couple of kernel
> +   versions. */
> +#define VIDIOC_QUERY_MATRIX	_IOWR('V', 103, struct v4l2_query_matrix)
> +#define VIDIOC_G_MATRIX		_IOWR('V', 104, struct v4l2_matrix)
> +#define VIDIOC_S_MATRIX		_IOWR('V', 105, struct v4l2_matrix)
> +
>  /* Reminder: when adding new ioctls please add support for them to
>     drivers/media/video/v4l2-compat-ioctl32.c as well! */

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [RFC PATCH 0/5] Matrix and Motion Detection support

[Sylwester Nawrocki]

Hi Hans,

On 07/08/2013 09:22 AM, Hans Verkuil wrote:
> On Sun July 7 2013 23:50:30 Sylwester Nawrocki wrote:
>> On 06/28/2013 02:27 PM, Hans Verkuil wrote:
>>> This patch series adds support for matrices and motion detection and
>>> converts the solo6x10 driver to use these new APIs.
>>>
>>> See the RFCv2 for details on the motion detection API:
>>>
>>> http://www.mail-archive.com/linux-media@vger.kernel.org/msg62085.html
>>>
>>> And this RFC for details on the matrix API (which superseeds the v4l2_md_blocks
>>> in the RFC above):
>>>
>>> http://permalink.gmane.org/gmane.linux.drivers.video-input-infrastructure/65195
>>>
>>> I have tested this with the solo card, both global motion detection and
>>> regional motion detection, and it works well.
>>>
>>> There is no documentation for the new APIs yet (other than the RFCs). I would
>>> like to know what others think of this proposal before I start work on the
>>> DocBook documentation.
>>
>> These 3 ioctls look pretty generic and will likely allow us to handle wide
>> range of functionalities, similarly to what the controls framework does 
>> today.
>>
>> What I don't like in the current trend of the V4L2 API development 
>> though is
>> that we have seemingly separate APIs for configuring integers, rectangles,
>> matrices, etc. And interactions between those APIs sometimes happen to be
>> not well defined.
>>
>> I'm not opposed to having this matrix API, but I would _much_ more like to
>> see it as a starting point of a more powerful API, that would allow to 
>> model
>> dependencies between parameters being configured and the objects more
>> explicitly and freely (e.g. case of the per buffer controls), that would
>> allow to pass a list of commands to the hardware for atomic 
>> re-configurations,
>> that would allow to create hardware configuration contexts, etc., etc.
>>
>> But it's all song of future, requires lots of effort, founding and takes
>> engineers with significant experience.
>>
>> As it likely won't happen soon I guess we can proceed with the matrix API
>> for now.
> 
> Do you attend the LPC in New Orleans? I would like to discuss this further,
> but it is easier to do so face-to-face with a whiteboard. Alternatively, we
> could set up a brainstorm session somewhere. This discussion keeps cropping
> up time and again, perhaps we should start to do something about it :-)

My apologies for the delay. I'm not planning to attend LPC, certainly
discussing this in person sounds like a good idea. I will be most likely
attending ELCE in Edinburg though, perhaps we could have some meeting
organized there, if there are other persons interested in that.

--
Regards,
Sylwester

Re: [RFC PATCH 0/5] Matrix and Motion Detection support

[Laurent Pinchart]

Hello,

On Sunday 07 July 2013 23:50:30 Sylwester Nawrocki wrote:
> On 06/28/2013 02:27 PM, Hans Verkuil wrote:
> > This patch series adds support for matrices and motion detection and
> > converts the solo6x10 driver to use these new APIs.
> > 
> > See the RFCv2 for details on the motion detection API:
> > 
> > http://www.mail-archive.com/linux-media@vger.kernel.org/msg62085.html
> > 
> > And this RFC for details on the matrix API (which superseeds the
> > v4l2_md_blocks in the RFC above):
> > 
> > http://permalink.gmane.org/gmane.linux.drivers.video-input-infrastructure/
> > 65195
> > 
> > I have tested this with the solo card, both global motion detection and
> > regional motion detection, and it works well.
> > 
> > There is no documentation for the new APIs yet (other than the RFCs). I
> > would like to know what others think of this proposal before I start work
> > on the DocBook documentation.
> 
> These 3 ioctls look pretty generic and will likely allow us to handle wide
> range of functionalities, similarly to what the controls framework does
> today.
> 
> What I don't like in the current trend of the V4L2 API development
> though is that we have seemingly separate APIs for configuring integers,
> rectangles, matrices, etc. And interactions between those APIs sometimes
> happen to be not well defined.
> 
> I'm not opposed to having this matrix API, but I would _much_ more like to
> see it as a starting point of a more powerful API, that would allow to
> model dependencies between parameters being configured and the objects more
> explicitly and freely (e.g. case of the per buffer controls), that would
> allow to pass a list of commands to the hardware for atomic re-
> configurations, that would allow to create hardware configuration contexts,
> etc., etc.
> 
> But it's all song of future, requires lots of effort, founding and takes
> engineers with significant experience.
> 
> As it likely won't happen soon I guess we can proceed with the matrix API
> for now.

Just for the record, I second that point of view. A matrix API, even as an 
interim solution for the problems at hand, would be welcome. I would use it to 
configure various kinds of LUTs (such as gamma tables). I'm all for going to a 
property-based model (or at least seriously brainstorming it), but we're 
looking at a too long time frame.

> > My tentative goal is to get this in for 3.12. Once this is in place the
> > solo and go7007 drivers can be moved out of staging into the mainline
> > since this is the only thing holding them back.

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 4/5] v4l2: add a motion detection event.

[Laurent Pinchart]

Hi Hans,

Thanks for the patch.

On Friday 28 June 2013 14:27:33 Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  include/uapi/linux/videodev2.h | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 5cbe815..f926209 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -1721,6 +1721,7 @@ struct v4l2_streamparm {
>  #define V4L2_EVENT_EOS				2
>  #define V4L2_EVENT_CTRL				3
>  #define V4L2_EVENT_FRAME_SYNC			4
> +#define V4L2_EVENT_MOTION_DET			5
>  #define V4L2_EVENT_PRIVATE_START		0x08000000
> 
>  /* Payload for V4L2_EVENT_VSYNC */
> @@ -1752,12 +1753,28 @@ struct v4l2_event_frame_sync {
>  	__u32 frame_sequence;
>  };
> 
> +#define V4L2_EVENT_MD_FL_HAVE_FRAME_SEQ	(1 << 0)
> +
> +/**
> + * struct v4l2_event_motion_det - motion detection event
> + * @flags:             if V4L2_EVENT_MD_FL_HAVE_FRAME_SEQ is set, then the
> + *                     frame_sequence field is valid.
> + * @frame_sequence:    the frame sequence number associated with this
> event.
> + * @region_mask:       which regions detected motion.
> + */
> +struct v4l2_event_motion_det {
> +	__u32 flags;
> +	__u32 frame_sequence;
> +	__u32 region_mask;

Will a 32-bit region mask be extensible enough ? What about hardware that 
could report motion detection as a (possibly low resolution) binary image ?

> +};
> +
>  struct v4l2_event {
>  	__u32				type;
>  	union {
>  		struct v4l2_event_vsync		vsync;
>  		struct v4l2_event_ctrl		ctrl;
>  		struct v4l2_event_frame_sync	frame_sync;
> +		struct v4l2_event_motion_det	motion_det;
>  		__u8				data[64];
>  	} u;
>  	__u32				pending;
-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 2/5] v4l2-compat-ioctl32: add g/s_matrix support.

[Laurent Pinchart]

Hi Hans,

Thanks for the patch.

On Friday 28 June 2013 14:27:31 Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 55 ++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c index 8f7a6a4..64155b1
> 100644
> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> @@ -777,6 +777,44 @@ static int put_v4l2_subdev_edid32(struct
> v4l2_subdev_edid *kp, struct v4l2_subde return 0;
>  }
> 
> +struct v4l2_matrix32 {
> +	__u32 type;
> +	union {
> +		__u32 reserved[4];
> +	} ref;
> +	struct v4l2_rect rect;
> +	compat_caddr_t matrix;
> +	__u32 reserved[12];
> +} __attribute__ ((packed));
> +
> +static int get_v4l2_matrix32(struct v4l2_matrix *kp, struct v4l2_matrix32
> __user *up) +{
> +	u32 tmp;
> +
> +	if (!access_ok(VERIFY_READ, up, sizeof(struct v4l2_matrix32)) ||
> +		get_user(kp->type, &up->type) ||
> +		copy_from_user(&kp->ref, &up->ref, sizeof(up->ref)) ||
> +		copy_from_user(&kp->rect, &up->rect, sizeof(up->rect)) ||
> +		get_user(tmp, &up->matrix) ||
> +		copy_from_user(kp->reserved, up->reserved, sizeof(kp->reserved)))
> +			return -EFAULT;

A bit of nit-picking here, the return is aligned too far right according to 
the kernel coding style (same for put_v4l2_matrix32() below).

> +	kp->matrix = compat_ptr(tmp);
> +	return 0;
> +}
> +
> +static int put_v4l2_matrix32(struct v4l2_matrix *kp, struct v4l2_matrix32
> __user *up)
> +{
> +	u32 tmp = (u32)((unsigned long)kp->matrix);
> +
> +	if (!access_ok(VERIFY_WRITE, up, sizeof(struct v4l2_matrix32)) ||
> +		put_user(kp->type, &up->type) ||
> +		copy_to_user(&kp->ref, &up->ref, sizeof(kp->ref)) ||
> +		copy_to_user(&kp->rect, &up->rect, sizeof(kp->rect)) ||
> +		put_user(tmp, &up->matrix) ||

Given that drivers shouldn't be allowed to modify the matrix pointer, could we 
get rid of the put_user() here as a small optimization ? The same could be 
done for all read-only (from a driver point of view) fields in the various 
put_v4l2_* functions.


> +		copy_to_user(kp->reserved, up->reserved, sizeof(kp->reserved)))
> +			return -EFAULT;
> +	return 0;
> +}
> 
>  #define VIDIOC_G_FMT32		_IOWR('V',  4, struct v4l2_format32)
>  #define VIDIOC_S_FMT32		_IOWR('V',  5, struct v4l2_format32)
> @@ -796,6 +834,8 @@ static int put_v4l2_subdev_edid32(struct
> v4l2_subdev_edid *kp, struct v4l2_subde #define	VIDIOC_DQEVENT32	_IOR ('V',
> 89, struct v4l2_event32)
>  #define VIDIOC_CREATE_BUFS32	_IOWR('V', 92, struct v4l2_create_buffers32)
>  #define VIDIOC_PREPARE_BUF32	_IOWR('V', 93, struct v4l2_buffer32)
> +#define VIDIOC_G_MATRIX32	_IOWR('V', 104, struct v4l2_matrix32)
> +#define VIDIOC_S_MATRIX32	_IOWR('V', 105, struct v4l2_matrix32)
> 
>  #define VIDIOC_OVERLAY32	_IOW ('V', 14, s32)
>  #define VIDIOC_STREAMON32	_IOW ('V', 18, s32)
> @@ -817,6 +857,7 @@ static long do_video_ioctl(struct file *file, unsigned
> int cmd, unsigned long ar struct v4l2_event v2ev;
>  		struct v4l2_create_buffers v2crt;
>  		struct v4l2_subdev_edid v2edid;
> +		struct v4l2_matrix v2matrix;
>  		unsigned long vx;
>  		int vi;
>  	} karg;
> @@ -851,6 +892,8 @@ static long do_video_ioctl(struct file *file, unsigned
> int cmd, unsigned long ar case VIDIOC_PREPARE_BUF32: cmd =
> VIDIOC_PREPARE_BUF; break;
>  	case VIDIOC_SUBDEV_G_EDID32: cmd = VIDIOC_SUBDEV_G_EDID; break;
>  	case VIDIOC_SUBDEV_S_EDID32: cmd = VIDIOC_SUBDEV_S_EDID; break;
> +	case VIDIOC_G_MATRIX32: cmd = VIDIOC_G_MATRIX; break;
> +	case VIDIOC_S_MATRIX32: cmd = VIDIOC_S_MATRIX; break;
>  	}
> 
>  	switch (cmd) {
> @@ -922,6 +965,12 @@ static long do_video_ioctl(struct file *file, unsigned
> int cmd, unsigned long ar case VIDIOC_DQEVENT:
>  		compatible_arg = 0;
>  		break;
> +
> +	case VIDIOC_G_MATRIX:
> +	case VIDIOC_S_MATRIX:
> +		err = get_v4l2_matrix32(&karg.v2matrix, up);
> +		compatible_arg = 0;
> +		break;
>  	}
>  	if (err)
>  		return err;
> @@ -994,6 +1043,11 @@ static long do_video_ioctl(struct file *file, unsigned
> int cmd, unsigned long ar case VIDIOC_ENUMINPUT:
>  		err = put_v4l2_input32(&karg.v2i, up);
>  		break;
> +
> +	case VIDIOC_G_MATRIX:
> +	case VIDIOC_S_MATRIX:
> +		err = put_v4l2_matrix32(&karg.v2matrix, up);
> +		break;
>  	}
>  	return err;
>  }
> @@ -1089,6 +1143,7 @@ long v4l2_compat_ioctl32(struct file *file, unsigned
> int cmd, unsigned long arg) case VIDIOC_ENUM_FREQ_BANDS:
>  	case VIDIOC_SUBDEV_G_EDID32:
>  	case VIDIOC_SUBDEV_S_EDID32:
> +	case VIDIOC_QUERY_MATRIX:
>  		ret = do_video_ioctl(file, cmd, arg);
>  		break;
-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 4/5] v4l2: add a motion detection event.

[Hans Verkuil]

On Thu 18 July 2013 02:14:28 Laurent Pinchart wrote:
> Hi Hans,
> 
> Thanks for the patch.
> 
> On Friday 28 June 2013 14:27:33 Hans Verkuil wrote:
> > From: Hans Verkuil <hans.verkuil@cisco.com>
> > 
> > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > ---
> >  include/uapi/linux/videodev2.h | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> > 
> > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > index 5cbe815..f926209 100644
> > --- a/include/uapi/linux/videodev2.h
> > +++ b/include/uapi/linux/videodev2.h
> > @@ -1721,6 +1721,7 @@ struct v4l2_streamparm {
> >  #define V4L2_EVENT_EOS				2
> >  #define V4L2_EVENT_CTRL				3
> >  #define V4L2_EVENT_FRAME_SYNC			4
> > +#define V4L2_EVENT_MOTION_DET			5
> >  #define V4L2_EVENT_PRIVATE_START		0x08000000
> > 
> >  /* Payload for V4L2_EVENT_VSYNC */
> > @@ -1752,12 +1753,28 @@ struct v4l2_event_frame_sync {
> >  	__u32 frame_sequence;
> >  };
> > 
> > +#define V4L2_EVENT_MD_FL_HAVE_FRAME_SEQ	(1 << 0)
> > +
> > +/**
> > + * struct v4l2_event_motion_det - motion detection event
> > + * @flags:             if V4L2_EVENT_MD_FL_HAVE_FRAME_SEQ is set, then the
> > + *                     frame_sequence field is valid.
> > + * @frame_sequence:    the frame sequence number associated with this
> > event.
> > + * @region_mask:       which regions detected motion.
> > + */
> > +struct v4l2_event_motion_det {
> > +	__u32 flags;
> > +	__u32 frame_sequence;
> > +	__u32 region_mask;
> 
> Will a 32-bit region mask be extensible enough ? What about hardware that 
> could report motion detection as a (possibly low resolution) binary image ?

I'm not sure whether we should be bothered about this. The struct can easily
be extended later.

Also, in your particular example I would actually expect that that would either
need support for 'large payload events', or you would call G_MATRIX to get that
image.

> > +};
> > +
> >  struct v4l2_event {
> >  	__u32				type;
> >  	union {
> >  		struct v4l2_event_vsync		vsync;
> >  		struct v4l2_event_ctrl		ctrl;
> >  		struct v4l2_event_frame_sync	frame_sync;
> > +		struct v4l2_event_motion_det	motion_det;
> >  		__u8				data[64];
> >  	} u;
> >  	__u32				pending;
> 

Regards,

	Hans

Re: [RFC PATCH 2/5] v4l2-compat-ioctl32: add g/s_matrix support.

[Hans Verkuil]

On Thu 18 July 2013 02:22:05 Laurent Pinchart wrote:
> Hi Hans,
> 
> Thanks for the patch.
> 
> On Friday 28 June 2013 14:27:31 Hans Verkuil wrote:
> > From: Hans Verkuil <hans.verkuil@cisco.com>
> > 
> > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > ---
> >  drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 55 ++++++++++++++++++++++++
> >  1 file changed, 55 insertions(+)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> > b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c index 8f7a6a4..64155b1
> > 100644
> > --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> > +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> > @@ -777,6 +777,44 @@ static int put_v4l2_subdev_edid32(struct
> > v4l2_subdev_edid *kp, struct v4l2_subde return 0;
> >  }
> > 
> > +struct v4l2_matrix32 {
> > +	__u32 type;
> > +	union {
> > +		__u32 reserved[4];
> > +	} ref;
> > +	struct v4l2_rect rect;
> > +	compat_caddr_t matrix;
> > +	__u32 reserved[12];
> > +} __attribute__ ((packed));
> > +
> > +static int get_v4l2_matrix32(struct v4l2_matrix *kp, struct v4l2_matrix32
> > __user *up) +{
> > +	u32 tmp;
> > +
> > +	if (!access_ok(VERIFY_READ, up, sizeof(struct v4l2_matrix32)) ||
> > +		get_user(kp->type, &up->type) ||
> > +		copy_from_user(&kp->ref, &up->ref, sizeof(up->ref)) ||
> > +		copy_from_user(&kp->rect, &up->rect, sizeof(up->rect)) ||
> > +		get_user(tmp, &up->matrix) ||
> > +		copy_from_user(kp->reserved, up->reserved, sizeof(kp->reserved)))
> > +			return -EFAULT;
> 
> A bit of nit-picking here, the return is aligned too far right according to 
> the kernel coding style (same for put_v4l2_matrix32() below).

Hmm, I copied-and-pasted this from another get/put function. I guess this is
wrong in more places than just this one.

> 
> > +	kp->matrix = compat_ptr(tmp);
> > +	return 0;
> > +}
> > +
> > +static int put_v4l2_matrix32(struct v4l2_matrix *kp, struct v4l2_matrix32
> > __user *up)
> > +{
> > +	u32 tmp = (u32)((unsigned long)kp->matrix);
> > +
> > +	if (!access_ok(VERIFY_WRITE, up, sizeof(struct v4l2_matrix32)) ||
> > +		put_user(kp->type, &up->type) ||
> > +		copy_to_user(&kp->ref, &up->ref, sizeof(kp->ref)) ||
> > +		copy_to_user(&kp->rect, &up->rect, sizeof(kp->rect)) ||
> > +		put_user(tmp, &up->matrix) ||
> 
> Given that drivers shouldn't be allowed to modify the matrix pointer, could we 
> get rid of the put_user() here as a small optimization ? The same could be 
> done for all read-only (from a driver point of view) fields in the various 
> put_v4l2_* functions.

Ditto. But it's a good idea. I'll look at it.

	Hans

> 
> 
> > +		copy_to_user(kp->reserved, up->reserved, sizeof(kp->reserved)))
> > +			return -EFAULT;
> > +	return 0;
> > +}
> > 
> >  #define VIDIOC_G_FMT32		_IOWR('V',  4, struct v4l2_format32)
> >  #define VIDIOC_S_FMT32		_IOWR('V',  5, struct v4l2_format32)
> > @@ -796,6 +834,8 @@ static int put_v4l2_subdev_edid32(struct
> > v4l2_subdev_edid *kp, struct v4l2_subde #define	VIDIOC_DQEVENT32	_IOR ('V',
> > 89, struct v4l2_event32)
> >  #define VIDIOC_CREATE_BUFS32	_IOWR('V', 92, struct v4l2_create_buffers32)
> >  #define VIDIOC_PREPARE_BUF32	_IOWR('V', 93, struct v4l2_buffer32)
> > +#define VIDIOC_G_MATRIX32	_IOWR('V', 104, struct v4l2_matrix32)
> > +#define VIDIOC_S_MATRIX32	_IOWR('V', 105, struct v4l2_matrix32)
> > 
> >  #define VIDIOC_OVERLAY32	_IOW ('V', 14, s32)
> >  #define VIDIOC_STREAMON32	_IOW ('V', 18, s32)
> > @@ -817,6 +857,7 @@ static long do_video_ioctl(struct file *file, unsigned
> > int cmd, unsigned long ar struct v4l2_event v2ev;
> >  		struct v4l2_create_buffers v2crt;
> >  		struct v4l2_subdev_edid v2edid;
> > +		struct v4l2_matrix v2matrix;
> >  		unsigned long vx;
> >  		int vi;
> >  	} karg;
> > @@ -851,6 +892,8 @@ static long do_video_ioctl(struct file *file, unsigned
> > int cmd, unsigned long ar case VIDIOC_PREPARE_BUF32: cmd =
> > VIDIOC_PREPARE_BUF; break;
> >  	case VIDIOC_SUBDEV_G_EDID32: cmd = VIDIOC_SUBDEV_G_EDID; break;
> >  	case VIDIOC_SUBDEV_S_EDID32: cmd = VIDIOC_SUBDEV_S_EDID; break;
> > +	case VIDIOC_G_MATRIX32: cmd = VIDIOC_G_MATRIX; break;
> > +	case VIDIOC_S_MATRIX32: cmd = VIDIOC_S_MATRIX; break;
> >  	}
> > 
> >  	switch (cmd) {
> > @@ -922,6 +965,12 @@ static long do_video_ioctl(struct file *file, unsigned
> > int cmd, unsigned long ar case VIDIOC_DQEVENT:
> >  		compatible_arg = 0;
> >  		break;
> > +
> > +	case VIDIOC_G_MATRIX:
> > +	case VIDIOC_S_MATRIX:
> > +		err = get_v4l2_matrix32(&karg.v2matrix, up);
> > +		compatible_arg = 0;
> > +		break;
> >  	}
> >  	if (err)
> >  		return err;
> > @@ -994,6 +1043,11 @@ static long do_video_ioctl(struct file *file, unsigned
> > int cmd, unsigned long ar case VIDIOC_ENUMINPUT:
> >  		err = put_v4l2_input32(&karg.v2i, up);
> >  		break;
> > +
> > +	case VIDIOC_G_MATRIX:
> > +	case VIDIOC_S_MATRIX:
> > +		err = put_v4l2_matrix32(&karg.v2matrix, up);
> > +		break;
> >  	}
> >  	return err;
> >  }
> > @@ -1089,6 +1143,7 @@ long v4l2_compat_ioctl32(struct file *file, unsigned
> > int cmd, unsigned long arg) case VIDIOC_ENUM_FREQ_BANDS:
> >  	case VIDIOC_SUBDEV_G_EDID32:
> >  	case VIDIOC_SUBDEV_S_EDID32:
> > +	case VIDIOC_QUERY_MATRIX:
> >  		ret = do_video_ioctl(file, cmd, arg);
> >  		break;
> 

Re: [RFC PATCH 0/5] Matrix and Motion Detection support

[Hans Verkuil]

Hi Laurent,

Thanks for your reviews!

On Thu 18 July 2013 02:12:49 Laurent Pinchart wrote:
> Hello,
> 
> On Sunday 07 July 2013 23:50:30 Sylwester Nawrocki wrote:
> > On 06/28/2013 02:27 PM, Hans Verkuil wrote:
> > > This patch series adds support for matrices and motion detection and
> > > converts the solo6x10 driver to use these new APIs.
> > > 
> > > See the RFCv2 for details on the motion detection API:
> > > 
> > > http://www.mail-archive.com/linux-media@vger.kernel.org/msg62085.html
> > > 
> > > And this RFC for details on the matrix API (which superseeds the
> > > v4l2_md_blocks in the RFC above):
> > > 
> > > http://permalink.gmane.org/gmane.linux.drivers.video-input-infrastructure/
> > > 65195
> > > 
> > > I have tested this with the solo card, both global motion detection and
> > > regional motion detection, and it works well.
> > > 
> > > There is no documentation for the new APIs yet (other than the RFCs). I
> > > would like to know what others think of this proposal before I start work
> > > on the DocBook documentation.
> > 
> > These 3 ioctls look pretty generic and will likely allow us to handle wide
> > range of functionalities, similarly to what the controls framework does
> > today.
> > 
> > What I don't like in the current trend of the V4L2 API development
> > though is that we have seemingly separate APIs for configuring integers,
> > rectangles, matrices, etc. And interactions between those APIs sometimes
> > happen to be not well defined.
> > 
> > I'm not opposed to having this matrix API, but I would _much_ more like to
> > see it as a starting point of a more powerful API, that would allow to
> > model dependencies between parameters being configured and the objects more
> > explicitly and freely (e.g. case of the per buffer controls), that would
> > allow to pass a list of commands to the hardware for atomic re-
> > configurations, that would allow to create hardware configuration contexts,
> > etc., etc.
> > 
> > But it's all song of future, requires lots of effort, founding and takes
> > engineers with significant experience.
> > 
> > As it likely won't happen soon I guess we can proceed with the matrix API
> > for now.
> 
> Just for the record, I second that point of view. A matrix API, even as an 
> interim solution for the problems at hand, would be welcome. I would use it to 
> configure various kinds of LUTs (such as gamma tables). I'm all for going to a 
> property-based model (or at least seriously brainstorming it), but we're 
> looking at a too long time frame.

OK. Good to know. In that case I will proceed with this and start writing the
documentation part as well.

Regards,

	Hans