* [RFCv3 PATCH 02/18] v4l2-ctrls: simplify error_idx handling.
2011-06-07 15:05 ` [RFCv3 PATCH 01/18] v4l2-ctrls: introduce call_op define Hans Verkuil
@ 2011-06-07 15:05 ` Hans Verkuil
2011-06-07 15:05 ` [RFCv3 PATCH 03/18] v4l2-ctrls: drivers should be able to ignore the READ_ONLY flag Hans Verkuil
` (15 subsequent siblings)
16 siblings, 0 replies; 42+ messages in thread
From: Hans Verkuil @ 2011-06-07 15:05 UTC (permalink / raw)
To: linux-media; +Cc: Hans Verkuil
From: Hans Verkuil <hans.verkuil@cisco.com>
The lower-level prepare functions just set error_idx for each control that
might have an error. The high-level functions will override this with
cs->count in the get and set cases. Only try will keep the error_idx.
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
drivers/media/video/v4l2-ctrls.c | 24 +++++++++---------------
1 files changed, 9 insertions(+), 15 deletions(-)
diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
index 3f2a0c5..d9e0439 100644
--- a/drivers/media/video/v4l2-ctrls.c
+++ b/drivers/media/video/v4l2-ctrls.c
@@ -1450,8 +1450,7 @@ EXPORT_SYMBOL(v4l2_subdev_querymenu);
Find the controls in the control array and do some basic checks. */
static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
struct v4l2_ext_controls *cs,
- struct ctrl_helper *helpers,
- bool try)
+ struct ctrl_helper *helpers)
{
u32 i;
@@ -1460,8 +1459,7 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
struct v4l2_ctrl *ctrl;
u32 id = c->id & V4L2_CTRL_ID_MASK;
- if (try)
- cs->error_idx = i;
+ cs->error_idx = i;
if (cs->ctrl_class && V4L2_CTRL_ID2CLASS(id) != cs->ctrl_class)
return -EINVAL;
@@ -1554,7 +1552,8 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *cs
return -ENOMEM;
}
- ret = prepare_ext_ctrls(hdl, cs, helpers, false);
+ ret = prepare_ext_ctrls(hdl, cs, helpers);
+ cs->error_idx = cs->count;
for (i = 0; !ret && i < cs->count; i++)
if (helpers[i].ctrl->flags & V4L2_CTRL_FLAG_WRITE_ONLY)
@@ -1701,12 +1700,10 @@ static int try_or_set_ext_ctrls(struct v4l2_ctrl_handler *hdl,
unsigned i, j;
int ret = 0;
- cs->error_idx = cs->count;
for (i = 0; i < cs->count; i++) {
struct v4l2_ctrl *ctrl = helpers[i].ctrl;
- if (!set)
- cs->error_idx = i;
+ cs->error_idx = i;
if (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY)
return -EACCES;
@@ -1724,11 +1721,10 @@ static int try_or_set_ext_ctrls(struct v4l2_ctrl_handler *hdl,
struct v4l2_ctrl *ctrl = helpers[i].ctrl;
struct v4l2_ctrl *master = ctrl->cluster[0];
- cs->error_idx = i;
-
if (helpers[i].handled)
continue;
+ cs->error_idx = i;
v4l2_ctrl_lock(ctrl);
/* Reset the 'is_new' flags of the cluster */
@@ -1777,12 +1773,11 @@ static int try_set_ext_ctrls(struct v4l2_ctrl_handler *hdl,
if (!helpers)
return -ENOMEM;
}
- ret = prepare_ext_ctrls(hdl, cs, helpers, !set);
- if (ret)
- goto free;
+ ret = prepare_ext_ctrls(hdl, cs, helpers);
/* First 'try' all controls and abort on error */
- ret = try_or_set_ext_ctrls(hdl, cs, helpers, false);
+ if (!ret)
+ ret = try_or_set_ext_ctrls(hdl, cs, helpers, false);
/* If this is a 'set' operation and the initial 'try' failed,
then set error_idx to count to tell the application that no
controls changed value yet. */
@@ -1795,7 +1790,6 @@ static int try_set_ext_ctrls(struct v4l2_ctrl_handler *hdl,
ret = try_or_set_ext_ctrls(hdl, cs, helpers, true);
}
-free:
if (cs->count > ARRAY_SIZE(helper))
kfree(helpers);
return ret;
--
1.7.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [RFCv3 PATCH 03/18] v4l2-ctrls: drivers should be able to ignore the READ_ONLY flag
2011-06-07 15:05 ` [RFCv3 PATCH 01/18] v4l2-ctrls: introduce call_op define Hans Verkuil
2011-06-07 15:05 ` [RFCv3 PATCH 02/18] v4l2-ctrls: simplify error_idx handling Hans Verkuil
@ 2011-06-07 15:05 ` Hans Verkuil
2011-06-07 15:05 ` [RFCv3 PATCH 04/18] v4l2-ioctl: add ctrl_handler to v4l2_fh Hans Verkuil
` (14 subsequent siblings)
16 siblings, 0 replies; 42+ messages in thread
From: Hans Verkuil @ 2011-06-07 15:05 UTC (permalink / raw)
To: linux-media; +Cc: Hans Verkuil
From: Hans Verkuil <hans.verkuil@cisco.com>
When applications try to set READ_ONLY controls an error should
be returned. However, when drivers do that it should be accepted.
Those controls could reflect some driver status which the application
can't change but the driver obviously has to be able to change it.
This is needed among others for future HDMI status controls.
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
drivers/media/video/v4l2-ctrls.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
index d9e0439..0b1b30f 100644
--- a/drivers/media/video/v4l2-ctrls.c
+++ b/drivers/media/video/v4l2-ctrls.c
@@ -1826,9 +1826,6 @@ static int set_ctrl(struct v4l2_ctrl *ctrl, s32 *val)
int ret;
int i;
- if (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY)
- return -EACCES;
-
v4l2_ctrl_lock(ctrl);
/* Reset the 'is_new' flags of the cluster */
@@ -1853,6 +1850,9 @@ int v4l2_s_ctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_control *control)
if (ctrl == NULL || !type_is_int(ctrl))
return -EINVAL;
+ if (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY)
+ return -EACCES;
+
return set_ctrl(ctrl, &control->value);
}
EXPORT_SYMBOL(v4l2_s_ctrl);
--
1.7.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [RFCv3 PATCH 04/18] v4l2-ioctl: add ctrl_handler to v4l2_fh
2011-06-07 15:05 ` [RFCv3 PATCH 01/18] v4l2-ctrls: introduce call_op define Hans Verkuil
2011-06-07 15:05 ` [RFCv3 PATCH 02/18] v4l2-ctrls: simplify error_idx handling Hans Verkuil
2011-06-07 15:05 ` [RFCv3 PATCH 03/18] v4l2-ctrls: drivers should be able to ignore the READ_ONLY flag Hans Verkuil
@ 2011-06-07 15:05 ` Hans Verkuil
2011-06-07 15:05 ` [RFCv3 PATCH 05/18] v4l2-subdev: implement per-filehandle control handlers Hans Verkuil
` (13 subsequent siblings)
16 siblings, 0 replies; 42+ messages in thread
From: Hans Verkuil @ 2011-06-07 15:05 UTC (permalink / raw)
To: linux-media
This is required to implement control events and is also needed to allow
for per-filehandle control handlers.
Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
drivers/media/video/v4l2-fh.c | 2 ++
drivers/media/video/v4l2-ioctl.c | 36 +++++++++++++++++++++++++++---------
include/media/v4l2-fh.h | 2 ++
3 files changed, 31 insertions(+), 9 deletions(-)
diff --git a/drivers/media/video/v4l2-fh.c b/drivers/media/video/v4l2-fh.c
index 717f71e..8635011 100644
--- a/drivers/media/video/v4l2-fh.c
+++ b/drivers/media/video/v4l2-fh.c
@@ -32,6 +32,8 @@
int v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev)
{
fh->vdev = vdev;
+ /* Inherit from video_device. May be overridden by the driver. */
+ fh->ctrl_handler = vdev->ctrl_handler;
INIT_LIST_HEAD(&fh->list);
set_bit(V4L2_FL_USES_V4L2_FH, &fh->vdev->flags);
fh->prio = V4L2_PRIORITY_UNSET;
diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
index 213ba7d..9811b1e 100644
--- a/drivers/media/video/v4l2-ioctl.c
+++ b/drivers/media/video/v4l2-ioctl.c
@@ -1418,7 +1418,9 @@ static long __video_do_ioctl(struct file *file,
{
struct v4l2_queryctrl *p = arg;
- if (vfd->ctrl_handler)
+ if (vfh && vfh->ctrl_handler)
+ ret = v4l2_queryctrl(vfh->ctrl_handler, p);
+ else if (vfd->ctrl_handler)
ret = v4l2_queryctrl(vfd->ctrl_handler, p);
else if (ops->vidioc_queryctrl)
ret = ops->vidioc_queryctrl(file, fh, p);
@@ -1438,7 +1440,9 @@ static long __video_do_ioctl(struct file *file,
{
struct v4l2_control *p = arg;
- if (vfd->ctrl_handler)
+ if (vfh && vfh->ctrl_handler)
+ ret = v4l2_g_ctrl(vfh->ctrl_handler, p);
+ else if (vfd->ctrl_handler)
ret = v4l2_g_ctrl(vfd->ctrl_handler, p);
else if (ops->vidioc_g_ctrl)
ret = ops->vidioc_g_ctrl(file, fh, p);
@@ -1470,12 +1474,16 @@ static long __video_do_ioctl(struct file *file,
struct v4l2_ext_controls ctrls;
struct v4l2_ext_control ctrl;
- if (!vfd->ctrl_handler &&
+ if (!(vfh && vfh->ctrl_handler) && !vfd->ctrl_handler &&
!ops->vidioc_s_ctrl && !ops->vidioc_s_ext_ctrls)
break;
dbgarg(cmd, "id=0x%x, value=%d\n", p->id, p->value);
+ if (vfh && vfh->ctrl_handler) {
+ ret = v4l2_s_ctrl(vfh->ctrl_handler, p);
+ break;
+ }
if (vfd->ctrl_handler) {
ret = v4l2_s_ctrl(vfd->ctrl_handler, p);
break;
@@ -1501,7 +1509,9 @@ static long __video_do_ioctl(struct file *file,
struct v4l2_ext_controls *p = arg;
p->error_idx = p->count;
- if (vfd->ctrl_handler)
+ if (vfh && vfh->ctrl_handler)
+ ret = v4l2_g_ext_ctrls(vfh->ctrl_handler, p);
+ else if (vfd->ctrl_handler)
ret = v4l2_g_ext_ctrls(vfd->ctrl_handler, p);
else if (ops->vidioc_g_ext_ctrls && check_ext_ctrls(p, 0))
ret = ops->vidioc_g_ext_ctrls(file, fh, p);
@@ -1515,10 +1525,13 @@ static long __video_do_ioctl(struct file *file,
struct v4l2_ext_controls *p = arg;
p->error_idx = p->count;
- if (!vfd->ctrl_handler && !ops->vidioc_s_ext_ctrls)
+ if (!(vfh && vfh->ctrl_handler) && !vfd->ctrl_handler &&
+ !ops->vidioc_s_ext_ctrls)
break;
v4l_print_ext_ctrls(cmd, vfd, p, 1);
- if (vfd->ctrl_handler)
+ if (vfh && vfh->ctrl_handler)
+ ret = v4l2_s_ext_ctrls(vfh->ctrl_handler, p);
+ else if (vfd->ctrl_handler)
ret = v4l2_s_ext_ctrls(vfd->ctrl_handler, p);
else if (check_ext_ctrls(p, 0))
ret = ops->vidioc_s_ext_ctrls(file, fh, p);
@@ -1529,10 +1542,13 @@ static long __video_do_ioctl(struct file *file,
struct v4l2_ext_controls *p = arg;
p->error_idx = p->count;
- if (!vfd->ctrl_handler && !ops->vidioc_try_ext_ctrls)
+ if (!(vfh && vfh->ctrl_handler) && !vfd->ctrl_handler &&
+ !ops->vidioc_try_ext_ctrls)
break;
v4l_print_ext_ctrls(cmd, vfd, p, 1);
- if (vfd->ctrl_handler)
+ if (vfh && vfh->ctrl_handler)
+ ret = v4l2_try_ext_ctrls(vfh->ctrl_handler, p);
+ else if (vfd->ctrl_handler)
ret = v4l2_try_ext_ctrls(vfd->ctrl_handler, p);
else if (check_ext_ctrls(p, 0))
ret = ops->vidioc_try_ext_ctrls(file, fh, p);
@@ -1542,7 +1558,9 @@ static long __video_do_ioctl(struct file *file,
{
struct v4l2_querymenu *p = arg;
- if (vfd->ctrl_handler)
+ if (vfh && vfh->ctrl_handler)
+ ret = v4l2_querymenu(vfh->ctrl_handler, p);
+ else if (vfd->ctrl_handler)
ret = v4l2_querymenu(vfd->ctrl_handler, p);
else if (ops->vidioc_querymenu)
ret = ops->vidioc_querymenu(file, fh, p);
diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
index 0206aa5..d247111 100644
--- a/include/media/v4l2-fh.h
+++ b/include/media/v4l2-fh.h
@@ -30,11 +30,13 @@
struct video_device;
struct v4l2_events;
+struct v4l2_ctrl_handler;
struct v4l2_fh {
struct list_head list;
struct video_device *vdev;
struct v4l2_events *events; /* events, pending and subscribed */
+ struct v4l2_ctrl_handler *ctrl_handler;
enum v4l2_priority prio;
};
--
1.7.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [RFCv3 PATCH 05/18] v4l2-subdev: implement per-filehandle control handlers.
2011-06-07 15:05 ` [RFCv3 PATCH 01/18] v4l2-ctrls: introduce call_op define Hans Verkuil
` (2 preceding siblings ...)
2011-06-07 15:05 ` [RFCv3 PATCH 04/18] v4l2-ioctl: add ctrl_handler to v4l2_fh Hans Verkuil
@ 2011-06-07 15:05 ` Hans Verkuil
2011-06-07 15:05 ` [RFCv3 PATCH 06/18] v4l2-ctrls: fix and improve volatile control handling Hans Verkuil
` (12 subsequent siblings)
16 siblings, 0 replies; 42+ messages in thread
From: Hans Verkuil @ 2011-06-07 15:05 UTC (permalink / raw)
To: linux-media; +Cc: Hans Verkuil
From: Hans Verkuil <hans.verkuil@cisco.com>
To be consistent with v4l2-ioctl.c.
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
drivers/media/video/v4l2-device.c | 1 +
drivers/media/video/v4l2-subdev.c | 14 +++++++-------
2 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/media/video/v4l2-device.c b/drivers/media/video/v4l2-device.c
index 4aae501..c72856c 100644
--- a/drivers/media/video/v4l2-device.c
+++ b/drivers/media/video/v4l2-device.c
@@ -209,6 +209,7 @@ int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev)
vdev->v4l2_dev = v4l2_dev;
vdev->fops = &v4l2_subdev_fops;
vdev->release = video_device_release_empty;
+ vdev->ctrl_handler = sd->ctrl_handler;
err = __video_register_device(vdev, VFL_TYPE_SUBDEV, -1, 1,
sd->owner);
if (err < 0)
diff --git a/drivers/media/video/v4l2-subdev.c b/drivers/media/video/v4l2-subdev.c
index 812729e..f396cc3 100644
--- a/drivers/media/video/v4l2-subdev.c
+++ b/drivers/media/video/v4l2-subdev.c
@@ -155,25 +155,25 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
switch (cmd) {
case VIDIOC_QUERYCTRL:
- return v4l2_queryctrl(sd->ctrl_handler, arg);
+ return v4l2_queryctrl(vfh->ctrl_handler, arg);
case VIDIOC_QUERYMENU:
- return v4l2_querymenu(sd->ctrl_handler, arg);
+ return v4l2_querymenu(vfh->ctrl_handler, arg);
case VIDIOC_G_CTRL:
- return v4l2_g_ctrl(sd->ctrl_handler, arg);
+ return v4l2_g_ctrl(vfh->ctrl_handler, arg);
case VIDIOC_S_CTRL:
- return v4l2_s_ctrl(sd->ctrl_handler, arg);
+ return v4l2_s_ctrl(vfh->ctrl_handler, arg);
case VIDIOC_G_EXT_CTRLS:
- return v4l2_g_ext_ctrls(sd->ctrl_handler, arg);
+ return v4l2_g_ext_ctrls(vfh->ctrl_handler, arg);
case VIDIOC_S_EXT_CTRLS:
- return v4l2_s_ext_ctrls(sd->ctrl_handler, arg);
+ return v4l2_s_ext_ctrls(vfh->ctrl_handler, arg);
case VIDIOC_TRY_EXT_CTRLS:
- return v4l2_try_ext_ctrls(sd->ctrl_handler, arg);
+ return v4l2_try_ext_ctrls(vfh->ctrl_handler, arg);
case VIDIOC_DQEVENT:
if (!(sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS))
--
1.7.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [RFCv3 PATCH 06/18] v4l2-ctrls: fix and improve volatile control handling.
2011-06-07 15:05 ` [RFCv3 PATCH 01/18] v4l2-ctrls: introduce call_op define Hans Verkuil
` (3 preceding siblings ...)
2011-06-07 15:05 ` [RFCv3 PATCH 05/18] v4l2-subdev: implement per-filehandle control handlers Hans Verkuil
@ 2011-06-07 15:05 ` Hans Verkuil
2011-06-07 15:05 ` [RFCv3 PATCH 07/18] v4l2-controls.txt: update to latest v4l2-ctrl.c changes Hans Verkuil
` (11 subsequent siblings)
16 siblings, 0 replies; 42+ messages in thread
From: Hans Verkuil @ 2011-06-07 15:05 UTC (permalink / raw)
To: linux-media; +Cc: Hans Verkuil
From: Hans Verkuil <hans.verkuil@cisco.com>
If you have a cluster of controls that is a mix of volatile and non-volatile
controls, then requesting the value of the volatile control would fail if the
master control of that cluster was non-volatile. The code assumed that the
volatile state of the master control was the same for all other controls in
the cluster.
This is now fixed.
In addition, it was clear from bugs in some drivers that it was confusing that
the ctrl->cur union had to be used in g_volatile_ctrl. Several drivers used the
'new' values instead. The framework was changed so that drivers now set the new
value instead of the current value.
This has an additional benefit as well: the volatile values are now only stored
in the 'new' value, leaving the current value alone. This is useful for
autofoo/foo control clusters where you want to have a 'foo' control act like a
volatile control if 'autofoo' is on, but as a normal control when it is off.
Since with this change the cur value is no longer overwritten when g_volatile_ctrl
is called, you can use it to remember the original 'foo' value. For example:
autofoo = 0, foo = 10 and foo is non-volatile.
Now autofoo is set to 1 and foo is marked volatile. Retrieving the foo value
will get the volatile value. Set autofoo back to 0, which marks foo as non-
volatile again, and retrieving foo will get the old current value of 10.
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
drivers/media/radio/radio-wl1273.c | 2 +-
drivers/media/radio/wl128x/fmdrv_v4l2.c | 2 +-
drivers/media/video/saa7115.c | 4 +-
drivers/media/video/v4l2-ctrls.c | 52 ++++++++++++++++++++++++++-----
4 files changed, 48 insertions(+), 12 deletions(-)
diff --git a/drivers/media/radio/radio-wl1273.c b/drivers/media/radio/radio-wl1273.c
index 459f727..46cacf8 100644
--- a/drivers/media/radio/radio-wl1273.c
+++ b/drivers/media/radio/radio-wl1273.c
@@ -1382,7 +1382,7 @@ static int wl1273_fm_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
switch (ctrl->id) {
case V4L2_CID_TUNE_ANTENNA_CAPACITOR:
- ctrl->cur.val = wl1273_fm_get_tx_ctune(radio);
+ ctrl->val = wl1273_fm_get_tx_ctune(radio);
break;
default:
diff --git a/drivers/media/radio/wl128x/fmdrv_v4l2.c b/drivers/media/radio/wl128x/fmdrv_v4l2.c
index 8701072..d50e5ac 100644
--- a/drivers/media/radio/wl128x/fmdrv_v4l2.c
+++ b/drivers/media/radio/wl128x/fmdrv_v4l2.c
@@ -191,7 +191,7 @@ static int fm_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
switch (ctrl->id) {
case V4L2_CID_TUNE_ANTENNA_CAPACITOR:
- ctrl->cur.val = fm_tx_get_tune_cap_val(fmdev);
+ ctrl->val = fm_tx_get_tune_cap_val(fmdev);
break;
default:
fmwarn("%s: Unknown IOCTL: %d\n", __func__, ctrl->id);
diff --git a/drivers/media/video/saa7115.c b/drivers/media/video/saa7115.c
index 0db9092..f2ae405 100644
--- a/drivers/media/video/saa7115.c
+++ b/drivers/media/video/saa7115.c
@@ -757,8 +757,8 @@ static int saa711x_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
switch (ctrl->id) {
case V4L2_CID_CHROMA_AGC:
/* chroma gain cluster */
- if (state->agc->cur.val)
- state->gain->cur.val =
+ if (state->agc->val)
+ state->gain->val =
saa711x_read(sd, R_0F_CHROMA_GAIN_CNTL) & 0x7f;
break;
}
diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
index 0b1b30f..a46d5c1 100644
--- a/drivers/media/video/v4l2-ctrls.c
+++ b/drivers/media/video/v4l2-ctrls.c
@@ -25,8 +25,10 @@
#include <media/v4l2-ctrls.h>
#include <media/v4l2-dev.h>
+#define has_op(master, op) \
+ (master->ops && master->ops->op)
#define call_op(master, op) \
- ((master->ops && master->ops->op) ? master->ops->op(master) : 0)
+ (has_op(master, op) ? master->ops->op(master) : 0)
/* Internal temporary helper struct, one for each v4l2_ext_control */
struct ctrl_helper {
@@ -626,6 +628,20 @@ static int new_to_user(struct v4l2_ext_control *c,
return 0;
}
+static int ctrl_to_user(struct v4l2_ext_control *c,
+ struct v4l2_ctrl *ctrl)
+{
+ if (ctrl->is_volatile)
+ return new_to_user(c, ctrl);
+ return cur_to_user(c, ctrl);
+}
+
+static int ctrl_is_volatile(struct v4l2_ext_control *c,
+ struct v4l2_ctrl *ctrl)
+{
+ return ctrl->is_volatile;
+}
+
/* Copy the new value to the current value. */
static void new_to_cur(struct v4l2_ctrl *ctrl)
{
@@ -1535,7 +1551,7 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *cs
struct ctrl_helper helper[4];
struct ctrl_helper *helpers = helper;
int ret;
- int i;
+ int i, j;
cs->error_idx = cs->count;
cs->ctrl_class = V4L2_CTRL_ID2CLASS(cs->ctrl_class);
@@ -1562,6 +1578,7 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *cs
for (i = 0; !ret && i < cs->count; i++) {
struct v4l2_ctrl *ctrl = helpers[i].ctrl;
struct v4l2_ctrl *master = ctrl->cluster[0];
+ bool has_volatiles;
if (helpers[i].handled)
continue;
@@ -1569,12 +1586,25 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *cs
cs->error_idx = i;
v4l2_ctrl_lock(master);
- /* g_volatile_ctrl will update the current control values */
- if (ctrl->is_volatile)
+
+ /* Any volatile controls requested from this cluster? */
+ has_volatiles = ctrl->is_volatile;
+ if (!has_volatiles && has_op(master, g_volatile_ctrl) &&
+ master->ncontrols > 1)
+ has_volatiles = cluster_walk(i + 1, cs, helpers,
+ ctrl_is_volatile);
+
+ /* g_volatile_ctrl will update the new control values */
+ if (has_volatiles) {
+ for (j = 0; j < master->ncontrols; j++)
+ cur_to_new(master->cluster[j]);
ret = call_op(master, g_volatile_ctrl);
- /* If OK, then copy the current control values to the caller */
+ }
+ /* If OK, then copy the current (for non-volatile controls)
+ or the new (for volatile controls) control values to the
+ caller */
if (!ret)
- ret = cluster_walk(i, cs, helpers, cur_to_user);
+ ret = cluster_walk(i, cs, helpers, ctrl_to_user);
v4l2_ctrl_unlock(master);
cluster_done(i, cs, helpers);
}
@@ -1596,15 +1626,21 @@ static int get_ctrl(struct v4l2_ctrl *ctrl, s32 *val)
{
struct v4l2_ctrl *master = ctrl->cluster[0];
int ret = 0;
+ int i;
if (ctrl->flags & V4L2_CTRL_FLAG_WRITE_ONLY)
return -EACCES;
v4l2_ctrl_lock(master);
/* g_volatile_ctrl will update the current control values */
- if (ctrl->is_volatile)
+ if (ctrl->is_volatile) {
+ for (i = 0; i < master->ncontrols; i++)
+ cur_to_new(master->cluster[i]);
ret = call_op(master, g_volatile_ctrl);
- *val = ctrl->cur.val;
+ *val = ctrl->val;
+ } else {
+ *val = ctrl->cur.val;
+ }
v4l2_ctrl_unlock(master);
return ret;
}
--
1.7.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [RFCv3 PATCH 07/18] v4l2-controls.txt: update to latest v4l2-ctrl.c changes.
2011-06-07 15:05 ` [RFCv3 PATCH 01/18] v4l2-ctrls: introduce call_op define Hans Verkuil
` (4 preceding siblings ...)
2011-06-07 15:05 ` [RFCv3 PATCH 06/18] v4l2-ctrls: fix and improve volatile control handling Hans Verkuil
@ 2011-06-07 15:05 ` Hans Verkuil
2011-06-07 15:05 ` [RFCv3 PATCH 08/18] v4l2-ctrls: add v4l2_ctrl_auto_cluster to simplify autogain/gain scenarios Hans Verkuil
` (10 subsequent siblings)
16 siblings, 0 replies; 42+ messages in thread
From: Hans Verkuil @ 2011-06-07 15:05 UTC (permalink / raw)
To: linux-media; +Cc: Hans Verkuil
From: Hans Verkuil <hans.verkuil@cisco.com>
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
Documentation/video4linux/v4l2-controls.txt | 13 ++++---------
1 files changed, 4 insertions(+), 9 deletions(-)
diff --git a/Documentation/video4linux/v4l2-controls.txt b/Documentation/video4linux/v4l2-controls.txt
index 881e7f4..95a3813 100644
--- a/Documentation/video4linux/v4l2-controls.txt
+++ b/Documentation/video4linux/v4l2-controls.txt
@@ -277,16 +277,13 @@ implement g_volatile_ctrl like this:
{
switch (ctrl->id) {
case V4L2_CID_BRIGHTNESS:
- ctrl->cur.val = read_reg(0x123);
+ ctrl->val = read_reg(0x123);
break;
}
}
-The 'new value' union is not used in g_volatile_ctrl. In general controls
-that need to implement g_volatile_ctrl are read-only controls.
-
-Note that if one or more controls in a control cluster are marked as volatile,
-then all the controls in the cluster are seen as volatile.
+Note that you use the 'new value' union as well in g_volatile_ctrl. In general
+controls that need to implement g_volatile_ctrl are read-only controls.
To mark a control as volatile you have to set the is_volatile flag:
@@ -636,9 +633,7 @@ button controls are write-only controls.
-EINVAL as the spec says.
5) The spec does not mention what should happen when you try to set/get a
-control class controls. ivtv currently returns -EINVAL (indicating that the
-control ID does not exist) while the framework will return -EACCES, which
-makes more sense.
+control class controls. The framework will return -EACCES.
Proposals for Extensions
--
1.7.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [RFCv3 PATCH 08/18] v4l2-ctrls: add v4l2_ctrl_auto_cluster to simplify autogain/gain scenarios
2011-06-07 15:05 ` [RFCv3 PATCH 01/18] v4l2-ctrls: introduce call_op define Hans Verkuil
` (5 preceding siblings ...)
2011-06-07 15:05 ` [RFCv3 PATCH 07/18] v4l2-controls.txt: update to latest v4l2-ctrl.c changes Hans Verkuil
@ 2011-06-07 15:05 ` Hans Verkuil
2011-06-20 13:05 ` Laurent Pinchart
` (2 more replies)
2011-06-07 15:05 ` [RFCv3 PATCH 09/18] DocBook: Improve cluster documentation and document the new autoclusters Hans Verkuil
` (9 subsequent siblings)
16 siblings, 3 replies; 42+ messages in thread
From: Hans Verkuil @ 2011-06-07 15:05 UTC (permalink / raw)
To: linux-media; +Cc: Hans Verkuil
From: Hans Verkuil <hans.verkuil@cisco.com>
It is a bit tricky to handle autogain/gain type scenerios correctly. Such
controls need to be clustered and the V4L2_CTRL_FLAG_UPDATE should be set on
the autofoo controls. In addition, the manual controls should be marked
inactive when the automatic mode is on, and active when the manual mode is on.
This also requires specialized volatile handling.
The chances of drivers doing all these things correctly are pretty remote.
So a new v4l2_ctrl_auto_cluster function was added that takes care of these
issues.
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
drivers/media/video/v4l2-ctrls.c | 69 +++++++++++++++++++++++++++++++------
include/media/v4l2-ctrls.h | 45 ++++++++++++++++++++++++
2 files changed, 102 insertions(+), 12 deletions(-)
diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
index a46d5c1..c39ab0c 100644
--- a/drivers/media/video/v4l2-ctrls.c
+++ b/drivers/media/video/v4l2-ctrls.c
@@ -39,6 +39,20 @@ struct ctrl_helper {
bool handled;
};
+/* Small helper function to determine if the autocluster is set to manual
+ mode. In that case the is_volatile flag should be ignored. */
+static bool is_cur_manual(const struct v4l2_ctrl *master)
+{
+ return master->is_auto && master->cur.val == master->manual_mode_value;
+}
+
+/* Same as above, but this checks the against the new value instead of the
+ current value. */
+static bool is_new_manual(const struct v4l2_ctrl *master)
+{
+ return master->is_auto && master->val == master->manual_mode_value;
+}
+
/* Returns NULL or a character pointer array containing the menu for
the given control ID. The pointer array ends with a NULL pointer.
An empty string signifies a menu entry that is invalid. This allows
@@ -643,7 +657,7 @@ static int ctrl_is_volatile(struct v4l2_ext_control *c,
}
/* Copy the new value to the current value. */
-static void new_to_cur(struct v4l2_ctrl *ctrl)
+static void new_to_cur(struct v4l2_ctrl *ctrl, bool update_inactive)
{
if (ctrl == NULL)
return;
@@ -659,6 +673,11 @@ static void new_to_cur(struct v4l2_ctrl *ctrl)
ctrl->cur.val = ctrl->val;
break;
}
+ if (update_inactive) {
+ ctrl->flags &= ~V4L2_CTRL_FLAG_INACTIVE;
+ if (!is_cur_manual(ctrl->cluster[0]))
+ ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE;
+ }
}
/* Copy the current value to the new value */
@@ -1166,7 +1185,7 @@ void v4l2_ctrl_cluster(unsigned ncontrols, struct v4l2_ctrl **controls)
int i;
/* The first control is the master control and it must not be NULL */
- BUG_ON(controls[0] == NULL);
+ BUG_ON(ncontrols == 0 || controls[0] == NULL);
for (i = 0; i < ncontrols; i++) {
if (controls[i]) {
@@ -1177,6 +1196,28 @@ void v4l2_ctrl_cluster(unsigned ncontrols, struct v4l2_ctrl **controls)
}
EXPORT_SYMBOL(v4l2_ctrl_cluster);
+void v4l2_ctrl_auto_cluster(unsigned ncontrols, struct v4l2_ctrl **controls,
+ u8 manual_val, bool set_volatile)
+{
+ struct v4l2_ctrl *master = controls[0];
+ u32 flag;
+ int i;
+
+ v4l2_ctrl_cluster(ncontrols, controls);
+ WARN_ON(ncontrols <= 1);
+ master->is_auto = true;
+ master->manual_mode_value = manual_val;
+ master->flags |= V4L2_CTRL_FLAG_UPDATE;
+ flag = is_cur_manual(master) ? 0 : V4L2_CTRL_FLAG_INACTIVE;
+
+ for (i = 1; i < ncontrols; i++)
+ if (controls[i]) {
+ controls[i]->is_volatile = set_volatile;
+ controls[i]->flags |= flag;
+ }
+}
+EXPORT_SYMBOL(v4l2_ctrl_auto_cluster);
+
/* Activate/deactivate a control. */
void v4l2_ctrl_activate(struct v4l2_ctrl *ctrl, bool active)
{
@@ -1595,7 +1636,7 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *cs
ctrl_is_volatile);
/* g_volatile_ctrl will update the new control values */
- if (has_volatiles) {
+ if (has_volatiles && !is_cur_manual(master)) {
for (j = 0; j < master->ncontrols; j++)
cur_to_new(master->cluster[j]);
ret = call_op(master, g_volatile_ctrl);
@@ -1633,7 +1674,7 @@ static int get_ctrl(struct v4l2_ctrl *ctrl, s32 *val)
v4l2_ctrl_lock(master);
/* g_volatile_ctrl will update the current control values */
- if (ctrl->is_volatile) {
+ if (ctrl->is_volatile && !is_cur_manual(master)) {
for (i = 0; i < master->ncontrols; i++)
cur_to_new(master->cluster[i]);
ret = call_op(master, g_volatile_ctrl);
@@ -1678,6 +1719,7 @@ EXPORT_SYMBOL(v4l2_ctrl_g_ctrl);
Must be called with ctrl->handler->lock held. */
static int try_or_set_control_cluster(struct v4l2_ctrl *master, bool set)
{
+ bool update_flag;
bool try = !set;
int ret = 0;
int i;
@@ -1717,14 +1759,17 @@ static int try_or_set_control_cluster(struct v4l2_ctrl *master, bool set)
ret = call_op(master, try_ctrl);
/* Don't set if there is no change */
- if (!ret && set && cluster_changed(master)) {
- ret = call_op(master, s_ctrl);
- /* If OK, then make the new values permanent. */
- if (!ret)
- for (i = 0; i < master->ncontrols; i++)
- new_to_cur(master->cluster[i]);
- }
- return ret;
+ if (ret || !set || !cluster_changed(master))
+ return ret;
+ ret = call_op(master, s_ctrl);
+ /* If OK, then make the new values permanent. */
+ if (ret)
+ return ret;
+
+ update_flag = is_cur_manual(master) != is_new_manual(master);
+ for (i = 0; i < master->ncontrols; i++)
+ new_to_cur(master->cluster[i], update_flag && i > 0);
+ return 0;
}
/* Try or set controls. */
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 97d0638..56323e3 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -65,6 +65,15 @@ struct v4l2_ctrl_ops {
* control's current value cannot be cached and needs to be
* retrieved through the g_volatile_ctrl op. Drivers can set
* this flag.
+ * @is_auto: If set, then this control selects whether the other cluster
+ * members are in 'automatic' mode or 'manual' mode. This is
+ * used for autogain/gain type clusters. Drivers should never
+ * set this flag directly.
+ * @manual_mode_value: If the is_auto flag is set, then this is the value
+ * of the auto control that determines if that control is in
+ * manual mode. So if the value of the auto control equals this
+ * value, then the whole cluster is in manual mode. Drivers should
+ * never set this flag directly.
* @ops: The control ops.
* @id: The control ID.
* @name: The control name.
@@ -105,6 +114,8 @@ struct v4l2_ctrl {
unsigned int is_new:1;
unsigned int is_private:1;
unsigned int is_volatile:1;
+ unsigned int is_auto:1;
+ unsigned int manual_mode_value:5;
const struct v4l2_ctrl_ops *ops;
u32 id;
@@ -363,6 +374,40 @@ int v4l2_ctrl_add_handler(struct v4l2_ctrl_handler *hdl,
void v4l2_ctrl_cluster(unsigned ncontrols, struct v4l2_ctrl **controls);
+/** v4l2_ctrl_auto_cluster() - Mark all controls in the cluster as belonging to
+ * that cluster and set it up for autofoo/foo-type handling.
+ * @ncontrols: The number of controls in this cluster.
+ * @controls: The cluster control array of size @ncontrols. The first control
+ * must be the 'auto' control (e.g. autogain, autoexposure, etc.)
+ * @manual_val: The value for the first control in the cluster that equals the
+ * manual setting.
+ * @set_volatile: If true, then all controls except the first auto control will
+ * have is_volatile set to true. If false, then is_volatile will not
+ * be touched.
+ *
+ * Use for control groups where one control selects some automatic feature and
+ * the other controls are only active whenever the automatic feature is turned
+ * off (manual mode). Typical examples: autogain vs gain, auto-whitebalance vs
+ * red and blue balance, etc.
+ *
+ * The behavior of such controls is as follows:
+ *
+ * When the autofoo control is set to automatic, then any manual controls
+ * are set to inactive and any reads will call g_volatile_ctrl (if the control
+ * was marked volatile).
+ *
+ * When the autofoo control is set to manual, then any manual controls will
+ * be marked active, and any reads will just return the current value without
+ * going through g_volatile_ctrl.
+ *
+ * In addition, this function will set the V4L2_CTRL_FLAG_UPDATE flag
+ * on the autofoo control and V4L2_CTRL_FLAG_INACTIVE on the foo control(s)
+ * if autofoo is in auto mode.
+ */
+void v4l2_ctrl_auto_cluster(unsigned ncontrols, struct v4l2_ctrl **controls,
+ u8 manual_val, bool set_volatile);
+
+
/** v4l2_ctrl_find() - Find a control with the given ID.
* @hdl: The control handler.
* @id: The control ID to find.
--
1.7.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [RFCv3 PATCH 08/18] v4l2-ctrls: add v4l2_ctrl_auto_cluster to simplify autogain/gain scenarios
2011-06-07 15:05 ` [RFCv3 PATCH 08/18] v4l2-ctrls: add v4l2_ctrl_auto_cluster to simplify autogain/gain scenarios Hans Verkuil
@ 2011-06-20 13:05 ` Laurent Pinchart
2011-06-20 13:16 ` Hans Verkuil
2011-06-27 20:57 ` Mauro Carvalho Chehab
2011-06-27 21:10 ` Mauro Carvalho Chehab
2 siblings, 1 reply; 42+ messages in thread
From: Laurent Pinchart @ 2011-06-20 13:05 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, Hans Verkuil
Hi Hans,
Thanks for the patch.
On Tuesday 07 June 2011 17:05:13 Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
>
> It is a bit tricky to handle autogain/gain type scenerios correctly. Such
> controls need to be clustered and the V4L2_CTRL_FLAG_UPDATE should be set
> on the autofoo controls. In addition, the manual controls should be marked
> inactive when the automatic mode is on, and active when the manual mode is
> on. This also requires specialized volatile handling.
>
> The chances of drivers doing all these things correctly are pretty remote.
> So a new v4l2_ctrl_auto_cluster function was added that takes care of these
> issues.
Sorry for being a killjoy, but how is this supposed to handle the auto-
exposure control ? Auto-exposure can be in complete auto mode, where both
exposure time and aperture are controlled automatically, in exposure- or
aperture-priority mode, where only one the exposure time and aperture is
controlled automatically, or in manual mode.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFCv3 PATCH 08/18] v4l2-ctrls: add v4l2_ctrl_auto_cluster to simplify autogain/gain scenarios
2011-06-20 13:05 ` Laurent Pinchart
@ 2011-06-20 13:16 ` Hans Verkuil
0 siblings, 0 replies; 42+ messages in thread
From: Hans Verkuil @ 2011-06-20 13:16 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: linux-media, Hans Verkuil
On Monday, June 20, 2011 15:05:47 Laurent Pinchart wrote:
> Hi Hans,
>
> Thanks for the patch.
>
> On Tuesday 07 June 2011 17:05:13 Hans Verkuil wrote:
> > From: Hans Verkuil <hans.verkuil@cisco.com>
> >
> > It is a bit tricky to handle autogain/gain type scenerios correctly. Such
> > controls need to be clustered and the V4L2_CTRL_FLAG_UPDATE should be set
> > on the autofoo controls. In addition, the manual controls should be marked
> > inactive when the automatic mode is on, and active when the manual mode is
> > on. This also requires specialized volatile handling.
> >
> > The chances of drivers doing all these things correctly are pretty remote.
> > So a new v4l2_ctrl_auto_cluster function was added that takes care of these
> > issues.
>
> Sorry for being a killjoy, but how is this supposed to handle the auto-
> exposure control ? Auto-exposure can be in complete auto mode, where both
> exposure time and aperture are controlled automatically, in exposure- or
> aperture-priority mode, where only one the exposure time and aperture is
> controlled automatically, or in manual mode.
That you will have to implement yourself. This may need some additional
support from the framework. v4l2_ctrl_auto_cluster() is meant to cater
to most, but not necessarily all, use cases. This particular case clearly
falls out of the scope of that function.
Hmm, perhaps it should be extended with an optional callback function.
That would be the most general approach.
But let's deal with that when we get there.
Regards,
Hans
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFCv3 PATCH 08/18] v4l2-ctrls: add v4l2_ctrl_auto_cluster to simplify autogain/gain scenarios
2011-06-07 15:05 ` [RFCv3 PATCH 08/18] v4l2-ctrls: add v4l2_ctrl_auto_cluster to simplify autogain/gain scenarios Hans Verkuil
2011-06-20 13:05 ` Laurent Pinchart
@ 2011-06-27 20:57 ` Mauro Carvalho Chehab
2011-06-28 6:08 ` Hans Verkuil
2011-06-27 21:10 ` Mauro Carvalho Chehab
2 siblings, 1 reply; 42+ messages in thread
From: Mauro Carvalho Chehab @ 2011-06-27 20:57 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, Hans Verkuil
Em 07-06-2011 12:05, Hans Verkuil escreveu:
> From: Hans Verkuil <hans.verkuil@cisco.com>
>
> It is a bit tricky to handle autogain/gain type scenerios correctly. Such
> controls need to be clustered and the V4L2_CTRL_FLAG_UPDATE should be set on
> the autofoo controls. In addition, the manual controls should be marked
> inactive when the automatic mode is on, and active when the manual mode is on.
> This also requires specialized volatile handling.
>
> The chances of drivers doing all these things correctly are pretty remote.
> So a new v4l2_ctrl_auto_cluster function was added that takes care of these
> issues.
>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
> drivers/media/video/v4l2-ctrls.c | 69 +++++++++++++++++++++++++++++++------
> include/media/v4l2-ctrls.h | 45 ++++++++++++++++++++++++
> 2 files changed, 102 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
> index a46d5c1..c39ab0c 100644
> --- a/drivers/media/video/v4l2-ctrls.c
> +++ b/drivers/media/video/v4l2-ctrls.c
> @@ -39,6 +39,20 @@ struct ctrl_helper {
> bool handled;
> };
>
> +/* Small helper function to determine if the autocluster is set to manual
> + mode. In that case the is_volatile flag should be ignored. */
> +static bool is_cur_manual(const struct v4l2_ctrl *master)
> +{
> + return master->is_auto && master->cur.val == master->manual_mode_value;
> +}
> +
> +/* Same as above, but this checks the against the new value instead of the
> + current value. */
> +static bool is_new_manual(const struct v4l2_ctrl *master)
> +{
> + return master->is_auto && master->val == master->manual_mode_value;
> +}
> +
> /* Returns NULL or a character pointer array containing the menu for
> the given control ID. The pointer array ends with a NULL pointer.
> An empty string signifies a menu entry that is invalid. This allows
> @@ -643,7 +657,7 @@ static int ctrl_is_volatile(struct v4l2_ext_control *c,
> }
>
> /* Copy the new value to the current value. */
> -static void new_to_cur(struct v4l2_ctrl *ctrl)
> +static void new_to_cur(struct v4l2_ctrl *ctrl, bool update_inactive)
> {
> if (ctrl == NULL)
> return;
> @@ -659,6 +673,11 @@ static void new_to_cur(struct v4l2_ctrl *ctrl)
> ctrl->cur.val = ctrl->val;
> break;
> }
> + if (update_inactive) {
> + ctrl->flags &= ~V4L2_CTRL_FLAG_INACTIVE;
> + if (!is_cur_manual(ctrl->cluster[0]))
> + ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE;
> + }
> }
>
> /* Copy the current value to the new value */
> @@ -1166,7 +1185,7 @@ void v4l2_ctrl_cluster(unsigned ncontrols, struct v4l2_ctrl **controls)
> int i;
>
> /* The first control is the master control and it must not be NULL */
> - BUG_ON(controls[0] == NULL);
> + BUG_ON(ncontrols == 0 || controls[0] == NULL);
>
> for (i = 0; i < ncontrols; i++) {
> if (controls[i]) {
> @@ -1177,6 +1196,28 @@ void v4l2_ctrl_cluster(unsigned ncontrols, struct v4l2_ctrl **controls)
> }
> EXPORT_SYMBOL(v4l2_ctrl_cluster);
>
> +void v4l2_ctrl_auto_cluster(unsigned ncontrols, struct v4l2_ctrl **controls,
> + u8 manual_val, bool set_volatile)
> +{
> + struct v4l2_ctrl *master = controls[0];
> + u32 flag;
> + int i;
> +
> + v4l2_ctrl_cluster(ncontrols, controls);
> + WARN_ON(ncontrols <= 1);
> + master->is_auto = true;
> + master->manual_mode_value = manual_val;
You'll have an overflow if manual_val is higher than 31 here.
> + master->flags |= V4L2_CTRL_FLAG_UPDATE;
> + flag = is_cur_manual(master) ? 0 : V4L2_CTRL_FLAG_INACTIVE;
> +
> + for (i = 1; i < ncontrols; i++)
> + if (controls[i]) {
> + controls[i]->is_volatile = set_volatile;
> + controls[i]->flags |= flag;
> + }
> +}
> +EXPORT_SYMBOL(v4l2_ctrl_auto_cluster);
> +
> /* Activate/deactivate a control. */
> void v4l2_ctrl_activate(struct v4l2_ctrl *ctrl, bool active)
> {
> @@ -1595,7 +1636,7 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *cs
> ctrl_is_volatile);
>
> /* g_volatile_ctrl will update the new control values */
> - if (has_volatiles) {
> + if (has_volatiles && !is_cur_manual(master)) {
> for (j = 0; j < master->ncontrols; j++)
> cur_to_new(master->cluster[j]);
> ret = call_op(master, g_volatile_ctrl);
> @@ -1633,7 +1674,7 @@ static int get_ctrl(struct v4l2_ctrl *ctrl, s32 *val)
>
> v4l2_ctrl_lock(master);
> /* g_volatile_ctrl will update the current control values */
> - if (ctrl->is_volatile) {
> + if (ctrl->is_volatile && !is_cur_manual(master)) {
> for (i = 0; i < master->ncontrols; i++)
> cur_to_new(master->cluster[i]);
> ret = call_op(master, g_volatile_ctrl);
> @@ -1678,6 +1719,7 @@ EXPORT_SYMBOL(v4l2_ctrl_g_ctrl);
> Must be called with ctrl->handler->lock held. */
> static int try_or_set_control_cluster(struct v4l2_ctrl *master, bool set)
> {
> + bool update_flag;
> bool try = !set;
> int ret = 0;
> int i;
> @@ -1717,14 +1759,17 @@ static int try_or_set_control_cluster(struct v4l2_ctrl *master, bool set)
> ret = call_op(master, try_ctrl);
>
> /* Don't set if there is no change */
> - if (!ret && set && cluster_changed(master)) {
> - ret = call_op(master, s_ctrl);
> - /* If OK, then make the new values permanent. */
> - if (!ret)
> - for (i = 0; i < master->ncontrols; i++)
> - new_to_cur(master->cluster[i]);
> - }
> - return ret;
> + if (ret || !set || !cluster_changed(master))
> + return ret;
> + ret = call_op(master, s_ctrl);
> + /* If OK, then make the new values permanent. */
> + if (ret)
> + return ret;
> +
> + update_flag = is_cur_manual(master) != is_new_manual(master);
> + for (i = 0; i < master->ncontrols; i++)
> + new_to_cur(master->cluster[i], update_flag && i > 0);
> + return 0;
> }
>
> /* Try or set controls. */
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index 97d0638..56323e3 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -65,6 +65,15 @@ struct v4l2_ctrl_ops {
> * control's current value cannot be cached and needs to be
> * retrieved through the g_volatile_ctrl op. Drivers can set
> * this flag.
> + * @is_auto: If set, then this control selects whether the other cluster
> + * members are in 'automatic' mode or 'manual' mode. This is
> + * used for autogain/gain type clusters. Drivers should never
> + * set this flag directly.
> + * @manual_mode_value: If the is_auto flag is set, then this is the value
> + * of the auto control that determines if that control is in
> + * manual mode. So if the value of the auto control equals this
> + * value, then the whole cluster is in manual mode. Drivers should
> + * never set this flag directly.
> * @ops: The control ops.
> * @id: The control ID.
> * @name: The control name.
> @@ -105,6 +114,8 @@ struct v4l2_ctrl {
> unsigned int is_new:1;
> unsigned int is_private:1;
> unsigned int is_volatile:1;
> + unsigned int is_auto:1;
> + unsigned int manual_mode_value:5;
Why are you using 5 bits for it? It seems too arbitrary to limit it to 32, especially
since I failed to see any rationale for that in the documentation.
>
> const struct v4l2_ctrl_ops *ops;
> u32 id;
> @@ -363,6 +374,40 @@ int v4l2_ctrl_add_handler(struct v4l2_ctrl_handler *hdl,
> void v4l2_ctrl_cluster(unsigned ncontrols, struct v4l2_ctrl **controls);
>
>
> +/** v4l2_ctrl_auto_cluster() - Mark all controls in the cluster as belonging to
> + * that cluster and set it up for autofoo/foo-type handling.
> + * @ncontrols: The number of controls in this cluster.
> + * @controls: The cluster control array of size @ncontrols. The first control
> + * must be the 'auto' control (e.g. autogain, autoexposure, etc.)
> + * @manual_val: The value for the first control in the cluster that equals the
> + * manual setting.
> + * @set_volatile: If true, then all controls except the first auto control will
> + * have is_volatile set to true. If false, then is_volatile will not
> + * be touched.
> + *
> + * Use for control groups where one control selects some automatic feature and
> + * the other controls are only active whenever the automatic feature is turned
> + * off (manual mode). Typical examples: autogain vs gain, auto-whitebalance vs
> + * red and blue balance, etc.
> + *
> + * The behavior of such controls is as follows:
> + *
> + * When the autofoo control is set to automatic, then any manual controls
> + * are set to inactive and any reads will call g_volatile_ctrl (if the control
> + * was marked volatile).
> + *
> + * When the autofoo control is set to manual, then any manual controls will
> + * be marked active, and any reads will just return the current value without
> + * going through g_volatile_ctrl.
> + *
> + * In addition, this function will set the V4L2_CTRL_FLAG_UPDATE flag
> + * on the autofoo control and V4L2_CTRL_FLAG_INACTIVE on the foo control(s)
> + * if autofoo is in auto mode.
> + */
> +void v4l2_ctrl_auto_cluster(unsigned ncontrols, struct v4l2_ctrl **controls,
> + u8 manual_val, bool set_volatile);
If you're limiting it to 5 bits, why are you allowing 8 bits here?
> +
> +
> /** v4l2_ctrl_find() - Find a control with the given ID.
> * @hdl: The control handler.
> * @id: The control ID to find.
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [RFCv3 PATCH 08/18] v4l2-ctrls: add v4l2_ctrl_auto_cluster to simplify autogain/gain scenarios
2011-06-27 20:57 ` Mauro Carvalho Chehab
@ 2011-06-28 6:08 ` Hans Verkuil
2011-06-28 10:25 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 42+ messages in thread
From: Hans Verkuil @ 2011-06-28 6:08 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: linux-media, Hans Verkuil
On Monday, June 27, 2011 22:57:34 Mauro Carvalho Chehab wrote:
> Em 07-06-2011 12:05, Hans Verkuil escreveu:
> > From: Hans Verkuil <hans.verkuil@cisco.com>
> >
> > It is a bit tricky to handle autogain/gain type scenerios correctly. Such
> > controls need to be clustered and the V4L2_CTRL_FLAG_UPDATE should be set on
> > the autofoo controls. In addition, the manual controls should be marked
> > inactive when the automatic mode is on, and active when the manual mode is on.
> > This also requires specialized volatile handling.
> >
> > The chances of drivers doing all these things correctly are pretty remote.
> > So a new v4l2_ctrl_auto_cluster function was added that takes care of these
> > issues.
> >
> > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > ---
> > drivers/media/video/v4l2-ctrls.c | 69 +++++++++++++++++++++++++++++++------
> > include/media/v4l2-ctrls.h | 45 ++++++++++++++++++++++++
> > 2 files changed, 102 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
> > index a46d5c1..c39ab0c 100644
> > --- a/drivers/media/video/v4l2-ctrls.c
> > +++ b/drivers/media/video/v4l2-ctrls.c
> > @@ -39,6 +39,20 @@ struct ctrl_helper {
> > bool handled;
> > };
> >
> > +/* Small helper function to determine if the autocluster is set to manual
> > + mode. In that case the is_volatile flag should be ignored. */
> > +static bool is_cur_manual(const struct v4l2_ctrl *master)
> > +{
> > + return master->is_auto && master->cur.val == master->manual_mode_value;
> > +}
> > +
> > +/* Same as above, but this checks the against the new value instead of the
> > + current value. */
> > +static bool is_new_manual(const struct v4l2_ctrl *master)
> > +{
> > + return master->is_auto && master->val == master->manual_mode_value;
> > +}
> > +
> > /* Returns NULL or a character pointer array containing the menu for
> > the given control ID. The pointer array ends with a NULL pointer.
> > An empty string signifies a menu entry that is invalid. This allows
> > @@ -643,7 +657,7 @@ static int ctrl_is_volatile(struct v4l2_ext_control *c,
> > }
> >
> > /* Copy the new value to the current value. */
> > -static void new_to_cur(struct v4l2_ctrl *ctrl)
> > +static void new_to_cur(struct v4l2_ctrl *ctrl, bool update_inactive)
> > {
> > if (ctrl == NULL)
> > return;
> > @@ -659,6 +673,11 @@ static void new_to_cur(struct v4l2_ctrl *ctrl)
> > ctrl->cur.val = ctrl->val;
> > break;
> > }
> > + if (update_inactive) {
> > + ctrl->flags &= ~V4L2_CTRL_FLAG_INACTIVE;
> > + if (!is_cur_manual(ctrl->cluster[0]))
> > + ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE;
> > + }
> > }
> >
> > /* Copy the current value to the new value */
> > @@ -1166,7 +1185,7 @@ void v4l2_ctrl_cluster(unsigned ncontrols, struct v4l2_ctrl **controls)
> > int i;
> >
> > /* The first control is the master control and it must not be NULL */
> > - BUG_ON(controls[0] == NULL);
> > + BUG_ON(ncontrols == 0 || controls[0] == NULL);
> >
> > for (i = 0; i < ncontrols; i++) {
> > if (controls[i]) {
> > @@ -1177,6 +1196,28 @@ void v4l2_ctrl_cluster(unsigned ncontrols, struct v4l2_ctrl **controls)
> > }
> > EXPORT_SYMBOL(v4l2_ctrl_cluster);
> >
> > +void v4l2_ctrl_auto_cluster(unsigned ncontrols, struct v4l2_ctrl **controls,
> > + u8 manual_val, bool set_volatile)
> > +{
> > + struct v4l2_ctrl *master = controls[0];
> > + u32 flag;
> > + int i;
> > +
> > + v4l2_ctrl_cluster(ncontrols, controls);
> > + WARN_ON(ncontrols <= 1);
> > + master->is_auto = true;
> > + master->manual_mode_value = manual_val;
>
> You'll have an overflow if manual_val is higher than 31 here.
>
> > + master->flags |= V4L2_CTRL_FLAG_UPDATE;
> > + flag = is_cur_manual(master) ? 0 : V4L2_CTRL_FLAG_INACTIVE;
> > +
> > + for (i = 1; i < ncontrols; i++)
> > + if (controls[i]) {
> > + controls[i]->is_volatile = set_volatile;
> > + controls[i]->flags |= flag;
> > + }
> > +}
> > +EXPORT_SYMBOL(v4l2_ctrl_auto_cluster);
> > +
> > /* Activate/deactivate a control. */
> > void v4l2_ctrl_activate(struct v4l2_ctrl *ctrl, bool active)
> > {
> > @@ -1595,7 +1636,7 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *cs
> > ctrl_is_volatile);
> >
> > /* g_volatile_ctrl will update the new control values */
> > - if (has_volatiles) {
> > + if (has_volatiles && !is_cur_manual(master)) {
> > for (j = 0; j < master->ncontrols; j++)
> > cur_to_new(master->cluster[j]);
> > ret = call_op(master, g_volatile_ctrl);
> > @@ -1633,7 +1674,7 @@ static int get_ctrl(struct v4l2_ctrl *ctrl, s32 *val)
> >
> > v4l2_ctrl_lock(master);
> > /* g_volatile_ctrl will update the current control values */
> > - if (ctrl->is_volatile) {
> > + if (ctrl->is_volatile && !is_cur_manual(master)) {
> > for (i = 0; i < master->ncontrols; i++)
> > cur_to_new(master->cluster[i]);
> > ret = call_op(master, g_volatile_ctrl);
> > @@ -1678,6 +1719,7 @@ EXPORT_SYMBOL(v4l2_ctrl_g_ctrl);
> > Must be called with ctrl->handler->lock held. */
> > static int try_or_set_control_cluster(struct v4l2_ctrl *master, bool set)
> > {
> > + bool update_flag;
> > bool try = !set;
> > int ret = 0;
> > int i;
> > @@ -1717,14 +1759,17 @@ static int try_or_set_control_cluster(struct v4l2_ctrl *master, bool set)
> > ret = call_op(master, try_ctrl);
> >
> > /* Don't set if there is no change */
> > - if (!ret && set && cluster_changed(master)) {
> > - ret = call_op(master, s_ctrl);
> > - /* If OK, then make the new values permanent. */
> > - if (!ret)
> > - for (i = 0; i < master->ncontrols; i++)
> > - new_to_cur(master->cluster[i]);
> > - }
> > - return ret;
> > + if (ret || !set || !cluster_changed(master))
> > + return ret;
> > + ret = call_op(master, s_ctrl);
> > + /* If OK, then make the new values permanent. */
> > + if (ret)
> > + return ret;
> > +
> > + update_flag = is_cur_manual(master) != is_new_manual(master);
> > + for (i = 0; i < master->ncontrols; i++)
> > + new_to_cur(master->cluster[i], update_flag && i > 0);
> > + return 0;
> > }
> >
> > /* Try or set controls. */
> > diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> > index 97d0638..56323e3 100644
> > --- a/include/media/v4l2-ctrls.h
> > +++ b/include/media/v4l2-ctrls.h
> > @@ -65,6 +65,15 @@ struct v4l2_ctrl_ops {
> > * control's current value cannot be cached and needs to be
> > * retrieved through the g_volatile_ctrl op. Drivers can set
> > * this flag.
> > + * @is_auto: If set, then this control selects whether the other cluster
> > + * members are in 'automatic' mode or 'manual' mode. This is
> > + * used for autogain/gain type clusters. Drivers should never
> > + * set this flag directly.
> > + * @manual_mode_value: If the is_auto flag is set, then this is the value
> > + * of the auto control that determines if that control is in
> > + * manual mode. So if the value of the auto control equals this
> > + * value, then the whole cluster is in manual mode. Drivers should
> > + * never set this flag directly.
> > * @ops: The control ops.
> > * @id: The control ID.
> > * @name: The control name.
> > @@ -105,6 +114,8 @@ struct v4l2_ctrl {
> > unsigned int is_new:1;
> > unsigned int is_private:1;
> > unsigned int is_volatile:1;
> > + unsigned int is_auto:1;
> > + unsigned int manual_mode_value:5;
>
> Why are you using 5 bits for it? It seems too arbitrary to limit it to 32, especially
> since I failed to see any rationale for that in the documentation.
This value is for a boolean or menu control. The menu control is currently
limited to 32 values, hence the 5 bits. However, it is probably better to
make it 8 bits and add a check above for whether the manual_mode_value >
ctrl->maximum (or < ctrl->minimum while I'm at it).
That's a lot more general.
> >
> > const struct v4l2_ctrl_ops *ops;
> > u32 id;
> > @@ -363,6 +374,40 @@ int v4l2_ctrl_add_handler(struct v4l2_ctrl_handler *hdl,
> > void v4l2_ctrl_cluster(unsigned ncontrols, struct v4l2_ctrl **controls);
> >
> >
> > +/** v4l2_ctrl_auto_cluster() - Mark all controls in the cluster as belonging to
> > + * that cluster and set it up for autofoo/foo-type handling.
> > + * @ncontrols: The number of controls in this cluster.
> > + * @controls: The cluster control array of size @ncontrols. The first control
> > + * must be the 'auto' control (e.g. autogain, autoexposure, etc.)
> > + * @manual_val: The value for the first control in the cluster that equals the
> > + * manual setting.
> > + * @set_volatile: If true, then all controls except the first auto control will
> > + * have is_volatile set to true. If false, then is_volatile will not
> > + * be touched.
> > + *
> > + * Use for control groups where one control selects some automatic feature and
> > + * the other controls are only active whenever the automatic feature is turned
> > + * off (manual mode). Typical examples: autogain vs gain, auto-whitebalance vs
> > + * red and blue balance, etc.
> > + *
> > + * The behavior of such controls is as follows:
> > + *
> > + * When the autofoo control is set to automatic, then any manual controls
> > + * are set to inactive and any reads will call g_volatile_ctrl (if the control
> > + * was marked volatile).
> > + *
> > + * When the autofoo control is set to manual, then any manual controls will
> > + * be marked active, and any reads will just return the current value without
> > + * going through g_volatile_ctrl.
> > + *
> > + * In addition, this function will set the V4L2_CTRL_FLAG_UPDATE flag
> > + * on the autofoo control and V4L2_CTRL_FLAG_INACTIVE on the foo control(s)
> > + * if autofoo is in auto mode.
> > + */
> > +void v4l2_ctrl_auto_cluster(unsigned ncontrols, struct v4l2_ctrl **controls,
> > + u8 manual_val, bool set_volatile);
>
> If you're limiting it to 5 bits, why are you allowing 8 bits here?
There is no u5?
Anyway, I'll change it to 8 bits.
Regards,
Hans
>
> > +
> > +
> > /** v4l2_ctrl_find() - Find a control with the given ID.
> > * @hdl: The control handler.
> > * @id: The control ID to find.
>
>
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [RFCv3 PATCH 08/18] v4l2-ctrls: add v4l2_ctrl_auto_cluster to simplify autogain/gain scenarios
2011-06-28 6:08 ` Hans Verkuil
@ 2011-06-28 10:25 ` Mauro Carvalho Chehab
0 siblings, 0 replies; 42+ messages in thread
From: Mauro Carvalho Chehab @ 2011-06-28 10:25 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, Hans Verkuil
Em 28-06-2011 03:08, Hans Verkuil escreveu:
> On Monday, June 27, 2011 22:57:34 Mauro Carvalho Chehab wrote:
>> Em 07-06-2011 12:05, Hans Verkuil escreveu:
>>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>>
>>> It is a bit tricky to handle autogain/gain type scenerios correctly. Such
>>> controls need to be clustered and the V4L2_CTRL_FLAG_UPDATE should be set on
>>> the autofoo controls. In addition, the manual controls should be marked
>>> inactive when the automatic mode is on, and active when the manual mode is on.
>>> This also requires specialized volatile handling.
>>>
>>> The chances of drivers doing all these things correctly are pretty remote.
>>> So a new v4l2_ctrl_auto_cluster function was added that takes care of these
>>> issues.
>>>
>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>>> ---
>>> drivers/media/video/v4l2-ctrls.c | 69 +++++++++++++++++++++++++++++++------
>>> include/media/v4l2-ctrls.h | 45 ++++++++++++++++++++++++
>>> 2 files changed, 102 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
>>> index a46d5c1..c39ab0c 100644
>>> --- a/drivers/media/video/v4l2-ctrls.c
>>> +++ b/drivers/media/video/v4l2-ctrls.c
>>> @@ -39,6 +39,20 @@ struct ctrl_helper {
>>> bool handled;
>>> };
>>>
>>> +/* Small helper function to determine if the autocluster is set to manual
>>> + mode. In that case the is_volatile flag should be ignored. */
>>> +static bool is_cur_manual(const struct v4l2_ctrl *master)
>>> +{
>>> + return master->is_auto && master->cur.val == master->manual_mode_value;
>>> +}
>>> +
>>> +/* Same as above, but this checks the against the new value instead of the
>>> + current value. */
>>> +static bool is_new_manual(const struct v4l2_ctrl *master)
>>> +{
>>> + return master->is_auto && master->val == master->manual_mode_value;
>>> +}
>>> +
>>> /* Returns NULL or a character pointer array containing the menu for
>>> the given control ID. The pointer array ends with a NULL pointer.
>>> An empty string signifies a menu entry that is invalid. This allows
>>> @@ -643,7 +657,7 @@ static int ctrl_is_volatile(struct v4l2_ext_control *c,
>>> }
>>>
>>> /* Copy the new value to the current value. */
>>> -static void new_to_cur(struct v4l2_ctrl *ctrl)
>>> +static void new_to_cur(struct v4l2_ctrl *ctrl, bool update_inactive)
>>> {
>>> if (ctrl == NULL)
>>> return;
>>> @@ -659,6 +673,11 @@ static void new_to_cur(struct v4l2_ctrl *ctrl)
>>> ctrl->cur.val = ctrl->val;
>>> break;
>>> }
>>> + if (update_inactive) {
>>> + ctrl->flags &= ~V4L2_CTRL_FLAG_INACTIVE;
>>> + if (!is_cur_manual(ctrl->cluster[0]))
>>> + ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE;
>>> + }
>>> }
>>>
>>> /* Copy the current value to the new value */
>>> @@ -1166,7 +1185,7 @@ void v4l2_ctrl_cluster(unsigned ncontrols, struct v4l2_ctrl **controls)
>>> int i;
>>>
>>> /* The first control is the master control and it must not be NULL */
>>> - BUG_ON(controls[0] == NULL);
>>> + BUG_ON(ncontrols == 0 || controls[0] == NULL);
>>>
>>> for (i = 0; i < ncontrols; i++) {
>>> if (controls[i]) {
>>> @@ -1177,6 +1196,28 @@ void v4l2_ctrl_cluster(unsigned ncontrols, struct v4l2_ctrl **controls)
>>> }
>>> EXPORT_SYMBOL(v4l2_ctrl_cluster);
>>>
>>> +void v4l2_ctrl_auto_cluster(unsigned ncontrols, struct v4l2_ctrl **controls,
>>> + u8 manual_val, bool set_volatile)
>>> +{
>>> + struct v4l2_ctrl *master = controls[0];
>>> + u32 flag;
>>> + int i;
>>> +
>>> + v4l2_ctrl_cluster(ncontrols, controls);
>>> + WARN_ON(ncontrols <= 1);
>>> + master->is_auto = true;
>>> + master->manual_mode_value = manual_val;
>>
>> You'll have an overflow if manual_val is higher than 31 here.
>>
>>> + master->flags |= V4L2_CTRL_FLAG_UPDATE;
>>> + flag = is_cur_manual(master) ? 0 : V4L2_CTRL_FLAG_INACTIVE;
>>> +
>>> + for (i = 1; i < ncontrols; i++)
>>> + if (controls[i]) {
>>> + controls[i]->is_volatile = set_volatile;
>>> + controls[i]->flags |= flag;
>>> + }
>>> +}
>>> +EXPORT_SYMBOL(v4l2_ctrl_auto_cluster);
>>> +
>>> /* Activate/deactivate a control. */
>>> void v4l2_ctrl_activate(struct v4l2_ctrl *ctrl, bool active)
>>> {
>>> @@ -1595,7 +1636,7 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *cs
>>> ctrl_is_volatile);
>>>
>>> /* g_volatile_ctrl will update the new control values */
>>> - if (has_volatiles) {
>>> + if (has_volatiles && !is_cur_manual(master)) {
>>> for (j = 0; j < master->ncontrols; j++)
>>> cur_to_new(master->cluster[j]);
>>> ret = call_op(master, g_volatile_ctrl);
>>> @@ -1633,7 +1674,7 @@ static int get_ctrl(struct v4l2_ctrl *ctrl, s32 *val)
>>>
>>> v4l2_ctrl_lock(master);
>>> /* g_volatile_ctrl will update the current control values */
>>> - if (ctrl->is_volatile) {
>>> + if (ctrl->is_volatile && !is_cur_manual(master)) {
>>> for (i = 0; i < master->ncontrols; i++)
>>> cur_to_new(master->cluster[i]);
>>> ret = call_op(master, g_volatile_ctrl);
>>> @@ -1678,6 +1719,7 @@ EXPORT_SYMBOL(v4l2_ctrl_g_ctrl);
>>> Must be called with ctrl->handler->lock held. */
>>> static int try_or_set_control_cluster(struct v4l2_ctrl *master, bool set)
>>> {
>>> + bool update_flag;
>>> bool try = !set;
>>> int ret = 0;
>>> int i;
>>> @@ -1717,14 +1759,17 @@ static int try_or_set_control_cluster(struct v4l2_ctrl *master, bool set)
>>> ret = call_op(master, try_ctrl);
>>>
>>> /* Don't set if there is no change */
>>> - if (!ret && set && cluster_changed(master)) {
>>> - ret = call_op(master, s_ctrl);
>>> - /* If OK, then make the new values permanent. */
>>> - if (!ret)
>>> - for (i = 0; i < master->ncontrols; i++)
>>> - new_to_cur(master->cluster[i]);
>>> - }
>>> - return ret;
>>> + if (ret || !set || !cluster_changed(master))
>>> + return ret;
>>> + ret = call_op(master, s_ctrl);
>>> + /* If OK, then make the new values permanent. */
>>> + if (ret)
>>> + return ret;
>>> +
>>> + update_flag = is_cur_manual(master) != is_new_manual(master);
>>> + for (i = 0; i < master->ncontrols; i++)
>>> + new_to_cur(master->cluster[i], update_flag && i > 0);
>>> + return 0;
>>> }
>>>
>>> /* Try or set controls. */
>>> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
>>> index 97d0638..56323e3 100644
>>> --- a/include/media/v4l2-ctrls.h
>>> +++ b/include/media/v4l2-ctrls.h
>>> @@ -65,6 +65,15 @@ struct v4l2_ctrl_ops {
>>> * control's current value cannot be cached and needs to be
>>> * retrieved through the g_volatile_ctrl op. Drivers can set
>>> * this flag.
>>> + * @is_auto: If set, then this control selects whether the other cluster
>>> + * members are in 'automatic' mode or 'manual' mode. This is
>>> + * used for autogain/gain type clusters. Drivers should never
>>> + * set this flag directly.
>>> + * @manual_mode_value: If the is_auto flag is set, then this is the value
>>> + * of the auto control that determines if that control is in
>>> + * manual mode. So if the value of the auto control equals this
>>> + * value, then the whole cluster is in manual mode. Drivers should
>>> + * never set this flag directly.
>>> * @ops: The control ops.
>>> * @id: The control ID.
>>> * @name: The control name.
>>> @@ -105,6 +114,8 @@ struct v4l2_ctrl {
>>> unsigned int is_new:1;
>>> unsigned int is_private:1;
>>> unsigned int is_volatile:1;
>>> + unsigned int is_auto:1;
>>> + unsigned int manual_mode_value:5;
>>
>> Why are you using 5 bits for it? It seems too arbitrary to limit it to 32, especially
>> since I failed to see any rationale for that in the documentation.
>
> This value is for a boolean or menu control. The menu control is currently
> limited to 32 values, hence the 5 bits. However, it is probably better to
> make it 8 bits and add a check above for whether the manual_mode_value >
> ctrl->maximum (or < ctrl->minimum while I'm at it).
>
> That's a lot more general.
Agreed.
>
>>>
>>> const struct v4l2_ctrl_ops *ops;
>>> u32 id;
>>> @@ -363,6 +374,40 @@ int v4l2_ctrl_add_handler(struct v4l2_ctrl_handler *hdl,
>>> void v4l2_ctrl_cluster(unsigned ncontrols, struct v4l2_ctrl **controls);
>>>
>>>
>>> +/** v4l2_ctrl_auto_cluster() - Mark all controls in the cluster as belonging to
>>> + * that cluster and set it up for autofoo/foo-type handling.
>>> + * @ncontrols: The number of controls in this cluster.
>>> + * @controls: The cluster control array of size @ncontrols. The first control
>>> + * must be the 'auto' control (e.g. autogain, autoexposure, etc.)
>>> + * @manual_val: The value for the first control in the cluster that equals the
>>> + * manual setting.
>>> + * @set_volatile: If true, then all controls except the first auto control will
>>> + * have is_volatile set to true. If false, then is_volatile will not
>>> + * be touched.
>>> + *
>>> + * Use for control groups where one control selects some automatic feature and
>>> + * the other controls are only active whenever the automatic feature is turned
>>> + * off (manual mode). Typical examples: autogain vs gain, auto-whitebalance vs
>>> + * red and blue balance, etc.
>>> + *
>>> + * The behavior of such controls is as follows:
>>> + *
>>> + * When the autofoo control is set to automatic, then any manual controls
>>> + * are set to inactive and any reads will call g_volatile_ctrl (if the control
>>> + * was marked volatile).
>>> + *
>>> + * When the autofoo control is set to manual, then any manual controls will
>>> + * be marked active, and any reads will just return the current value without
>>> + * going through g_volatile_ctrl.
>>> + *
>>> + * In addition, this function will set the V4L2_CTRL_FLAG_UPDATE flag
>>> + * on the autofoo control and V4L2_CTRL_FLAG_INACTIVE on the foo control(s)
>>> + * if autofoo is in auto mode.
>>> + */
>>> +void v4l2_ctrl_auto_cluster(unsigned ncontrols, struct v4l2_ctrl **controls,
>>> + u8 manual_val, bool set_volatile);
>>
>> If you're limiting it to 5 bits, why are you allowing 8 bits here?
>
> There is no u5?
Yes, but also there is not check for values >31
>
> Anyway, I'll change it to 8 bits.
It sounds the better approach for me.
Thanks!
Mauro
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFCv3 PATCH 08/18] v4l2-ctrls: add v4l2_ctrl_auto_cluster to simplify autogain/gain scenarios
2011-06-07 15:05 ` [RFCv3 PATCH 08/18] v4l2-ctrls: add v4l2_ctrl_auto_cluster to simplify autogain/gain scenarios Hans Verkuil
2011-06-20 13:05 ` Laurent Pinchart
2011-06-27 20:57 ` Mauro Carvalho Chehab
@ 2011-06-27 21:10 ` Mauro Carvalho Chehab
2011-06-28 6:11 ` Hans Verkuil
2 siblings, 1 reply; 42+ messages in thread
From: Mauro Carvalho Chehab @ 2011-06-27 21:10 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, Hans Verkuil
Em 07-06-2011 12:05, Hans Verkuil escreveu:
> From: Hans Verkuil <hans.verkuil@cisco.com>
>
> It is a bit tricky to handle autogain/gain type scenerios correctly. Such
> controls need to be clustered and the V4L2_CTRL_FLAG_UPDATE should be set on
> the autofoo controls. In addition, the manual controls should be marked
> inactive when the automatic mode is on, and active when the manual mode is on.
> This also requires specialized volatile handling.
>
> The chances of drivers doing all these things correctly are pretty remote.
> So a new v4l2_ctrl_auto_cluster function was added that takes care of these
> issues.
>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
> drivers/media/video/v4l2-ctrls.c | 69 +++++++++++++++++++++++++++++++------
> include/media/v4l2-ctrls.h | 45 ++++++++++++++++++++++++
> 2 files changed, 102 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
> index a46d5c1..c39ab0c 100644
> --- a/drivers/media/video/v4l2-ctrls.c
> +++ b/drivers/media/video/v4l2-ctrls.c
> @@ -39,6 +39,20 @@ struct ctrl_helper {
> bool handled;
> };
>
> +/* Small helper function to determine if the autocluster is set to manual
> + mode. In that case the is_volatile flag should be ignored. */
> +static bool is_cur_manual(const struct v4l2_ctrl *master)
> +{
> + return master->is_auto && master->cur.val == master->manual_mode_value;
> +}
> +
> +/* Same as above, but this checks the against the new value instead of the
> + current value. */
> +static bool is_new_manual(const struct v4l2_ctrl *master)
> +{
> + return master->is_auto && master->val == master->manual_mode_value;
> +}
> +
> /* Returns NULL or a character pointer array containing the menu for
> the given control ID. The pointer array ends with a NULL pointer.
> An empty string signifies a menu entry that is invalid. This allows
> @@ -643,7 +657,7 @@ static int ctrl_is_volatile(struct v4l2_ext_control *c,
> }
>
> /* Copy the new value to the current value. */
> -static void new_to_cur(struct v4l2_ctrl *ctrl)
> +static void new_to_cur(struct v4l2_ctrl *ctrl, bool update_inactive)
> {
> if (ctrl == NULL)
> return;
> @@ -659,6 +673,11 @@ static void new_to_cur(struct v4l2_ctrl *ctrl)
> ctrl->cur.val = ctrl->val;
> break;
> }
> + if (update_inactive) {
> + ctrl->flags &= ~V4L2_CTRL_FLAG_INACTIVE;
> + if (!is_cur_manual(ctrl->cluster[0]))
> + ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE;
> + }
> }
>
> /* Copy the current value to the new value */
> @@ -1166,7 +1185,7 @@ void v4l2_ctrl_cluster(unsigned ncontrols, struct v4l2_ctrl **controls)
> int i;
>
> /* The first control is the master control and it must not be NULL */
> - BUG_ON(controls[0] == NULL);
> + BUG_ON(ncontrols == 0 || controls[0] == NULL);
>
> for (i = 0; i < ncontrols; i++) {
> if (controls[i]) {
> @@ -1177,6 +1196,28 @@ void v4l2_ctrl_cluster(unsigned ncontrols, struct v4l2_ctrl **controls)
> }
> EXPORT_SYMBOL(v4l2_ctrl_cluster);
>
> +void v4l2_ctrl_auto_cluster(unsigned ncontrols, struct v4l2_ctrl **controls,
> + u8 manual_val, bool set_volatile)
> +{
> + struct v4l2_ctrl *master = controls[0];
> + u32 flag;
> + int i;
> +
> + v4l2_ctrl_cluster(ncontrols, controls);
> + WARN_ON(ncontrols <= 1);
> + master->is_auto = true;
> + master->manual_mode_value = manual_val;
> + master->flags |= V4L2_CTRL_FLAG_UPDATE;
> + flag = is_cur_manual(master) ? 0 : V4L2_CTRL_FLAG_INACTIVE;
> +
> + for (i = 1; i < ncontrols; i++)
Hmm... the first control _should_ be the autogain one. This is documented at the ABI
description, but it would be good to have a comment about there at the *.h file.
> + if (controls[i]) {
> + controls[i]->is_volatile = set_volatile;
> + controls[i]->flags |= flag;
> + }
> +}
> +EXPORT_SYMBOL(v4l2_ctrl_auto_cluster);
> +
> /* Activate/deactivate a control. */
> void v4l2_ctrl_activate(struct v4l2_ctrl *ctrl, bool active)
> {
> @@ -1595,7 +1636,7 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *cs
> ctrl_is_volatile);
>
> /* g_volatile_ctrl will update the new control values */
> - if (has_volatiles) {
> + if (has_volatiles && !is_cur_manual(master)) {
> for (j = 0; j < master->ncontrols; j++)
> cur_to_new(master->cluster[j]);
> ret = call_op(master, g_volatile_ctrl);
> @@ -1633,7 +1674,7 @@ static int get_ctrl(struct v4l2_ctrl *ctrl, s32 *val)
>
> v4l2_ctrl_lock(master);
> /* g_volatile_ctrl will update the current control values */
> - if (ctrl->is_volatile) {
> + if (ctrl->is_volatile && !is_cur_manual(master)) {
> for (i = 0; i < master->ncontrols; i++)
> cur_to_new(master->cluster[i]);
> ret = call_op(master, g_volatile_ctrl);
> @@ -1678,6 +1719,7 @@ EXPORT_SYMBOL(v4l2_ctrl_g_ctrl);
> Must be called with ctrl->handler->lock held. */
> static int try_or_set_control_cluster(struct v4l2_ctrl *master, bool set)
> {
> + bool update_flag;
> bool try = !set;
> int ret = 0;
> int i;
> @@ -1717,14 +1759,17 @@ static int try_or_set_control_cluster(struct v4l2_ctrl *master, bool set)
> ret = call_op(master, try_ctrl);
>
> /* Don't set if there is no change */
> - if (!ret && set && cluster_changed(master)) {
> - ret = call_op(master, s_ctrl);
> - /* If OK, then make the new values permanent. */
> - if (!ret)
> - for (i = 0; i < master->ncontrols; i++)
> - new_to_cur(master->cluster[i]);
> - }
> - return ret;
> + if (ret || !set || !cluster_changed(master))
> + return ret;
> + ret = call_op(master, s_ctrl);
> + /* If OK, then make the new values permanent. */
> + if (ret)
> + return ret;
> +
> + update_flag = is_cur_manual(master) != is_new_manual(master);
> + for (i = 0; i < master->ncontrols; i++)
> + new_to_cur(master->cluster[i], update_flag && i > 0);
> + return 0;
> }
>
> /* Try or set controls. */
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index 97d0638..56323e3 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -65,6 +65,15 @@ struct v4l2_ctrl_ops {
> * control's current value cannot be cached and needs to be
> * retrieved through the g_volatile_ctrl op. Drivers can set
> * this flag.
> + * @is_auto: If set, then this control selects whether the other cluster
> + * members are in 'automatic' mode or 'manual' mode. This is
> + * used for autogain/gain type clusters. Drivers should never
> + * set this flag directly.
> + * @manual_mode_value: If the is_auto flag is set, then this is the value
> + * of the auto control that determines if that control is in
> + * manual mode. So if the value of the auto control equals this
> + * value, then the whole cluster is in manual mode. Drivers should
> + * never set this flag directly.
> * @ops: The control ops.
> * @id: The control ID.
> * @name: The control name.
> @@ -105,6 +114,8 @@ struct v4l2_ctrl {
> unsigned int is_new:1;
> unsigned int is_private:1;
> unsigned int is_volatile:1;
> + unsigned int is_auto:1;
> + unsigned int manual_mode_value:5;
>
> const struct v4l2_ctrl_ops *ops;
> u32 id;
> @@ -363,6 +374,40 @@ int v4l2_ctrl_add_handler(struct v4l2_ctrl_handler *hdl,
> void v4l2_ctrl_cluster(unsigned ncontrols, struct v4l2_ctrl **controls);
>
>
> +/** v4l2_ctrl_auto_cluster() - Mark all controls in the cluster as belonging to
> + * that cluster and set it up for autofoo/foo-type handling.
> + * @ncontrols: The number of controls in this cluster.
> + * @controls: The cluster control array of size @ncontrols. The first control
> + * must be the 'auto' control (e.g. autogain, autoexposure, etc.)
> + * @manual_val: The value for the first control in the cluster that equals the
> + * manual setting.
> + * @set_volatile: If true, then all controls except the first auto control will
> + * have is_volatile set to true. If false, then is_volatile will not
> + * be touched.
> + *
> + * Use for control groups where one control selects some automatic feature and
> + * the other controls are only active whenever the automatic feature is turned
> + * off (manual mode). Typical examples: autogain vs gain, auto-whitebalance vs
> + * red and blue balance, etc.
> + *
> + * The behavior of such controls is as follows:
> + *
> + * When the autofoo control is set to automatic, then any manual controls
> + * are set to inactive and any reads will call g_volatile_ctrl (if the control
> + * was marked volatile).
> + *
> + * When the autofoo control is set to manual, then any manual controls will
> + * be marked active, and any reads will just return the current value without
> + * going through g_volatile_ctrl.
> + *
> + * In addition, this function will set the V4L2_CTRL_FLAG_UPDATE flag
> + * on the autofoo control and V4L2_CTRL_FLAG_INACTIVE on the foo control(s)
> + * if autofoo is in auto mode.
> + */
> +void v4l2_ctrl_auto_cluster(unsigned ncontrols, struct v4l2_ctrl **controls,
> + u8 manual_val, bool set_volatile);
> +
> +
> /** v4l2_ctrl_find() - Find a control with the given ID.
> * @hdl: The control handler.
> * @id: The control ID to find.
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [RFCv3 PATCH 08/18] v4l2-ctrls: add v4l2_ctrl_auto_cluster to simplify autogain/gain scenarios
2011-06-27 21:10 ` Mauro Carvalho Chehab
@ 2011-06-28 6:11 ` Hans Verkuil
0 siblings, 0 replies; 42+ messages in thread
From: Hans Verkuil @ 2011-06-28 6:11 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: linux-media, Hans Verkuil
On Monday, June 27, 2011 23:10:49 Mauro Carvalho Chehab wrote:
> Em 07-06-2011 12:05, Hans Verkuil escreveu:
> > From: Hans Verkuil <hans.verkuil@cisco.com>
> >
> > It is a bit tricky to handle autogain/gain type scenerios correctly. Such
> > controls need to be clustered and the V4L2_CTRL_FLAG_UPDATE should be set on
> > the autofoo controls. In addition, the manual controls should be marked
> > inactive when the automatic mode is on, and active when the manual mode is on.
> > This also requires specialized volatile handling.
> >
> > The chances of drivers doing all these things correctly are pretty remote.
> > So a new v4l2_ctrl_auto_cluster function was added that takes care of these
> > issues.
> >
> > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > ---
> > drivers/media/video/v4l2-ctrls.c | 69 +++++++++++++++++++++++++++++++------
> > include/media/v4l2-ctrls.h | 45 ++++++++++++++++++++++++
> > 2 files changed, 102 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
> > index a46d5c1..c39ab0c 100644
> > --- a/drivers/media/video/v4l2-ctrls.c
> > +++ b/drivers/media/video/v4l2-ctrls.c
> > @@ -39,6 +39,20 @@ struct ctrl_helper {
> > bool handled;
> > };
> >
> > +/* Small helper function to determine if the autocluster is set to manual
> > + mode. In that case the is_volatile flag should be ignored. */
> > +static bool is_cur_manual(const struct v4l2_ctrl *master)
> > +{
> > + return master->is_auto && master->cur.val == master->manual_mode_value;
> > +}
> > +
> > +/* Same as above, but this checks the against the new value instead of the
> > + current value. */
> > +static bool is_new_manual(const struct v4l2_ctrl *master)
> > +{
> > + return master->is_auto && master->val == master->manual_mode_value;
> > +}
> > +
> > /* Returns NULL or a character pointer array containing the menu for
> > the given control ID. The pointer array ends with a NULL pointer.
> > An empty string signifies a menu entry that is invalid. This allows
> > @@ -643,7 +657,7 @@ static int ctrl_is_volatile(struct v4l2_ext_control *c,
> > }
> >
> > /* Copy the new value to the current value. */
> > -static void new_to_cur(struct v4l2_ctrl *ctrl)
> > +static void new_to_cur(struct v4l2_ctrl *ctrl, bool update_inactive)
> > {
> > if (ctrl == NULL)
> > return;
> > @@ -659,6 +673,11 @@ static void new_to_cur(struct v4l2_ctrl *ctrl)
> > ctrl->cur.val = ctrl->val;
> > break;
> > }
> > + if (update_inactive) {
> > + ctrl->flags &= ~V4L2_CTRL_FLAG_INACTIVE;
> > + if (!is_cur_manual(ctrl->cluster[0]))
> > + ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE;
> > + }
> > }
> >
> > /* Copy the current value to the new value */
> > @@ -1166,7 +1185,7 @@ void v4l2_ctrl_cluster(unsigned ncontrols, struct v4l2_ctrl **controls)
> > int i;
> >
> > /* The first control is the master control and it must not be NULL */
> > - BUG_ON(controls[0] == NULL);
> > + BUG_ON(ncontrols == 0 || controls[0] == NULL);
> >
> > for (i = 0; i < ncontrols; i++) {
> > if (controls[i]) {
> > @@ -1177,6 +1196,28 @@ void v4l2_ctrl_cluster(unsigned ncontrols, struct v4l2_ctrl **controls)
> > }
> > EXPORT_SYMBOL(v4l2_ctrl_cluster);
> >
> > +void v4l2_ctrl_auto_cluster(unsigned ncontrols, struct v4l2_ctrl **controls,
> > + u8 manual_val, bool set_volatile)
> > +{
> > + struct v4l2_ctrl *master = controls[0];
> > + u32 flag;
> > + int i;
> > +
> > + v4l2_ctrl_cluster(ncontrols, controls);
> > + WARN_ON(ncontrols <= 1);
> > + master->is_auto = true;
> > + master->manual_mode_value = manual_val;
> > + master->flags |= V4L2_CTRL_FLAG_UPDATE;
> > + flag = is_cur_manual(master) ? 0 : V4L2_CTRL_FLAG_INACTIVE;
> > +
> > + for (i = 1; i < ncontrols; i++)
>
> Hmm... the first control _should_ be the autogain one. This is documented at the ABI
> description, but it would be good to have a comment about there at the *.h file.
Huh? It's there already:
* @controls: The cluster control array of size @ncontrols. The first control
* must be the 'auto' control (e.g. autogain, autoexposure, etc.)
Regards,
Hans
^ permalink raw reply [flat|nested] 42+ messages in thread
* [RFCv3 PATCH 09/18] DocBook: Improve cluster documentation and document the new autoclusters.
2011-06-07 15:05 ` [RFCv3 PATCH 01/18] v4l2-ctrls: introduce call_op define Hans Verkuil
` (6 preceding siblings ...)
2011-06-07 15:05 ` [RFCv3 PATCH 08/18] v4l2-ctrls: add v4l2_ctrl_auto_cluster to simplify autogain/gain scenarios Hans Verkuil
@ 2011-06-07 15:05 ` Hans Verkuil
2011-06-07 15:05 ` [RFCv3 PATCH 10/18] vivi: add autogain/gain support to test the autocluster functionality Hans Verkuil
` (8 subsequent siblings)
16 siblings, 0 replies; 42+ messages in thread
From: Hans Verkuil @ 2011-06-07 15:05 UTC (permalink / raw)
To: linux-media; +Cc: Hans Verkuil
From: Hans Verkuil <hans.verkuil@cisco.com>
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
Documentation/video4linux/v4l2-controls.txt | 56 +++++++++++++++++++++++++++
1 files changed, 56 insertions(+), 0 deletions(-)
diff --git a/Documentation/video4linux/v4l2-controls.txt b/Documentation/video4linux/v4l2-controls.txt
index 95a3813..9346fc8 100644
--- a/Documentation/video4linux/v4l2-controls.txt
+++ b/Documentation/video4linux/v4l2-controls.txt
@@ -450,6 +450,25 @@ In the example above the following are equivalent for the VOLUME case:
ctrl == ctrl->cluster[AUDIO_CL_VOLUME] == state->audio_cluster[AUDIO_CL_VOLUME]
ctrl->cluster[AUDIO_CL_MUTE] == state->audio_cluster[AUDIO_CL_MUTE]
+In practice using cluster arrays like this becomes very tiresome. So instead
+the following equivalent method is used:
+
+ struct {
+ /* audio cluster */
+ struct v4l2_ctrl *volume;
+ struct v4l2_ctrl *mute;
+ };
+
+The anonymous struct is used to clearly 'cluster' these two control pointers,
+but it serves no other purpose. The effect is the same as creating an
+array with two control pointers. So you can just do:
+
+ state->volume = v4l2_ctrl_new_std(&state->ctrl_handler, ...);
+ state->mute = v4l2_ctrl_new_std(&state->ctrl_handler, ...);
+ v4l2_ctrl_cluster(2, &state->volume);
+
+And in foo_s_ctrl you can use these pointers directly: state->mute->val.
+
Note that controls in a cluster may be NULL. For example, if for some
reason mute was never added (because the hardware doesn't support that
particular feature), then mute will be NULL. So in that case we have a
@@ -472,6 +491,43 @@ controls, then the 'is_new' flag would be 1 for both controls.
The 'is_new' flag is always 1 when called from v4l2_ctrl_handler_setup().
+Handling autogain/gain-type Controls with Auto Clusters
+=======================================================
+
+A common type of control cluster is one that handles 'auto-foo/foo'-type
+controls. Typical examples are autogain/gain, autoexposure/exposure,
+autowhitebalance/red balance/blue balance. In all cases you have one controls
+that determines whether another control is handled automatically by the hardware,
+or whether it is under manual control from the user.
+
+If the cluster is in automatic mode, then the manual controls should be
+marked inactive. When the volatile controls are read the g_volatile_ctrl
+operation should return the value that the hardware's automatic mode set up
+automatically.
+
+If the cluster is put in manual mode, then the manual controls should become
+active again and the is_volatile flag should be ignored (so g_volatile_ctrl is
+no longer called while in manual mode).
+
+Finally the V4L2_CTRL_FLAG_UPDATE should be set for the auto control since
+changing that control affects the control flags of the manual controls.
+
+In order to simplify this a special variation of v4l2_ctrl_cluster was
+introduced:
+
+void v4l2_ctrl_auto_cluster(unsigned ncontrols, struct v4l2_ctrl **controls,
+ u8 manual_val, bool set_volatile);
+
+The first two arguments are identical to v4l2_ctrl_cluster. The third argument
+tells the framework which value switches the cluster into manual mode. The
+last argument will optionally set the is_volatile flag for the non-auto controls.
+
+The first control of the cluster is assumed to be the 'auto' control.
+
+Using this function will ensure that you don't need to handle all the complex
+flag and volatile handling.
+
+
VIDIOC_LOG_STATUS Support
=========================
--
1.7.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [RFCv3 PATCH 10/18] vivi: add autogain/gain support to test the autocluster functionality.
2011-06-07 15:05 ` [RFCv3 PATCH 01/18] v4l2-ctrls: introduce call_op define Hans Verkuil
` (7 preceding siblings ...)
2011-06-07 15:05 ` [RFCv3 PATCH 09/18] DocBook: Improve cluster documentation and document the new autoclusters Hans Verkuil
@ 2011-06-07 15:05 ` Hans Verkuil
2011-06-07 15:05 ` [RFCv3 PATCH 11/18] v4l2-ctrls: add v4l2_fh pointer to the set control functions Hans Verkuil
` (7 subsequent siblings)
16 siblings, 0 replies; 42+ messages in thread
From: Hans Verkuil @ 2011-06-07 15:05 UTC (permalink / raw)
To: linux-media; +Cc: Hans Verkuil
From: Hans Verkuil <hans.verkuil@cisco.com>
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
drivers/media/video/vivi.c | 25 ++++++++++++++++++++++++-
1 files changed, 24 insertions(+), 1 deletions(-)
diff --git a/drivers/media/video/vivi.c b/drivers/media/video/vivi.c
index 2238a61..6e62ddf 100644
--- a/drivers/media/video/vivi.c
+++ b/drivers/media/video/vivi.c
@@ -167,6 +167,11 @@ struct vivi_dev {
struct v4l2_ctrl *contrast;
struct v4l2_ctrl *saturation;
struct v4l2_ctrl *hue;
+ struct {
+ /* autogain/gain cluster */
+ struct v4l2_ctrl *autogain;
+ struct v4l2_ctrl *gain;
+ };
struct v4l2_ctrl *volume;
struct v4l2_ctrl *button;
struct v4l2_ctrl *boolean;
@@ -457,6 +462,7 @@ static void vivi_fillbuff(struct vivi_dev *dev, struct vivi_buffer *buf)
unsigned ms;
char str[100];
int h, line = 1;
+ s32 gain;
if (!vbuf)
return;
@@ -479,6 +485,7 @@ static void vivi_fillbuff(struct vivi_dev *dev, struct vivi_buffer *buf)
dev->width, dev->height, dev->input);
gen_text(dev, vbuf, line++ * 16, 16, str);
+ gain = v4l2_ctrl_g_ctrl(dev->gain);
mutex_lock(&dev->ctrl_handler.lock);
snprintf(str, sizeof(str), " brightness %3d, contrast %3d, saturation %3d, hue %d ",
dev->brightness->cur.val,
@@ -486,7 +493,8 @@ static void vivi_fillbuff(struct vivi_dev *dev, struct vivi_buffer *buf)
dev->saturation->cur.val,
dev->hue->cur.val);
gen_text(dev, vbuf, line++ * 16, 16, str);
- snprintf(str, sizeof(str), " volume %3d ", dev->volume->cur.val);
+ snprintf(str, sizeof(str), " autogain %d, gain %3d, volume %3d ",
+ dev->autogain->cur.val, gain, dev->volume->cur.val);
gen_text(dev, vbuf, line++ * 16, 16, str);
snprintf(str, sizeof(str), " int32 %d, int64 %lld ",
dev->int32->cur.val,
@@ -983,6 +991,15 @@ static int vidioc_s_input(struct file *file, void *priv, unsigned int i)
/* --- controls ---------------------------------------------- */
+static int vivi_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
+{
+ struct vivi_dev *dev = container_of(ctrl->handler, struct vivi_dev, ctrl_handler);
+
+ if (ctrl == dev->autogain)
+ dev->gain->val = jiffies & 0xff;
+ return 0;
+}
+
static int vivi_s_ctrl(struct v4l2_ctrl *ctrl)
{
struct vivi_dev *dev = container_of(ctrl->handler, struct vivi_dev, ctrl_handler);
@@ -1045,6 +1062,7 @@ static int vivi_mmap(struct file *file, struct vm_area_struct *vma)
}
static const struct v4l2_ctrl_ops vivi_ctrl_ops = {
+ .g_volatile_ctrl = vivi_g_volatile_ctrl,
.s_ctrl = vivi_s_ctrl,
};
@@ -1213,6 +1231,10 @@ static int __init vivi_create_instance(int inst)
V4L2_CID_SATURATION, 0, 255, 1, 127);
dev->hue = v4l2_ctrl_new_std(hdl, &vivi_ctrl_ops,
V4L2_CID_HUE, -128, 127, 1, 0);
+ dev->autogain = v4l2_ctrl_new_std(hdl, &vivi_ctrl_ops,
+ V4L2_CID_AUTOGAIN, 0, 1, 1, 1);
+ dev->gain = v4l2_ctrl_new_std(hdl, &vivi_ctrl_ops,
+ V4L2_CID_GAIN, 0, 255, 1, 100);
dev->button = v4l2_ctrl_new_custom(hdl, &vivi_ctrl_button, NULL);
dev->int32 = v4l2_ctrl_new_custom(hdl, &vivi_ctrl_int32, NULL);
dev->int64 = v4l2_ctrl_new_custom(hdl, &vivi_ctrl_int64, NULL);
@@ -1223,6 +1245,7 @@ static int __init vivi_create_instance(int inst)
ret = hdl->error;
goto unreg_dev;
}
+ v4l2_ctrl_auto_cluster(2, &dev->autogain, 0, true);
dev->v4l2_dev.ctrl_handler = hdl;
/* initialize locks */
--
1.7.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [RFCv3 PATCH 11/18] v4l2-ctrls: add v4l2_fh pointer to the set control functions.
2011-06-07 15:05 ` [RFCv3 PATCH 01/18] v4l2-ctrls: introduce call_op define Hans Verkuil
` (8 preceding siblings ...)
2011-06-07 15:05 ` [RFCv3 PATCH 10/18] vivi: add autogain/gain support to test the autocluster functionality Hans Verkuil
@ 2011-06-07 15:05 ` Hans Verkuil
2011-06-27 21:20 ` Mauro Carvalho Chehab
2011-06-07 15:05 ` [RFCv3 PATCH 12/18] vb2_poll: don't start DMA, leave that to the first read() Hans Verkuil
` (6 subsequent siblings)
16 siblings, 1 reply; 42+ messages in thread
From: Hans Verkuil @ 2011-06-07 15:05 UTC (permalink / raw)
To: linux-media; +Cc: Hans Verkuil
From: Hans Verkuil <hans.verkuil@cisco.com>
When an application changes a control you want to generate an event.
However, you want to avoid sending such an event back to the application
(file handle) that caused the change.
Add the filehandle to the various set control functions.
The filehandle isn't used yet, but the control event patches will need
this.
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
drivers/media/video/v4l2-ctrls.c | 45 ++++++++++++++++++++----------------
drivers/media/video/v4l2-ioctl.c | 8 +++---
drivers/media/video/v4l2-subdev.c | 4 +-
include/media/v4l2-ctrls.h | 8 ++++--
4 files changed, 36 insertions(+), 29 deletions(-)
diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
index c39ab0c..a38bdf9 100644
--- a/drivers/media/video/v4l2-ctrls.c
+++ b/drivers/media/video/v4l2-ctrls.c
@@ -657,7 +657,8 @@ static int ctrl_is_volatile(struct v4l2_ext_control *c,
}
/* Copy the new value to the current value. */
-static void new_to_cur(struct v4l2_ctrl *ctrl, bool update_inactive)
+static void new_to_cur(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl,
+ bool update_inactive)
{
if (ctrl == NULL)
return;
@@ -1717,7 +1718,8 @@ EXPORT_SYMBOL(v4l2_ctrl_g_ctrl);
/* Core function that calls try/s_ctrl and ensures that the new value is
copied to the current value on a set.
Must be called with ctrl->handler->lock held. */
-static int try_or_set_control_cluster(struct v4l2_ctrl *master, bool set)
+static int try_or_set_control_cluster(struct v4l2_fh *fh,
+ struct v4l2_ctrl *master, bool set)
{
bool update_flag;
bool try = !set;
@@ -1768,12 +1770,13 @@ static int try_or_set_control_cluster(struct v4l2_ctrl *master, bool set)
update_flag = is_cur_manual(master) != is_new_manual(master);
for (i = 0; i < master->ncontrols; i++)
- new_to_cur(master->cluster[i], update_flag && i > 0);
+ new_to_cur(fh, master->cluster[i], update_flag && i > 0);
return 0;
}
/* Try or set controls. */
-static int try_or_set_ext_ctrls(struct v4l2_ctrl_handler *hdl,
+static int try_or_set_ext_ctrls(struct v4l2_fh *fh,
+ struct v4l2_ctrl_handler *hdl,
struct v4l2_ext_controls *cs,
struct ctrl_helper *helpers,
bool set)
@@ -1818,7 +1821,7 @@ static int try_or_set_ext_ctrls(struct v4l2_ctrl_handler *hdl,
ret = cluster_walk(i, cs, helpers, user_to_new);
if (!ret)
- ret = try_or_set_control_cluster(master, set);
+ ret = try_or_set_control_cluster(fh, master, set);
/* Copy the new values back to userspace. */
if (!ret)
@@ -1831,7 +1834,7 @@ static int try_or_set_ext_ctrls(struct v4l2_ctrl_handler *hdl,
}
/* Try or try-and-set controls */
-static int try_set_ext_ctrls(struct v4l2_ctrl_handler *hdl,
+static int try_set_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
struct v4l2_ext_controls *cs,
bool set)
{
@@ -1858,7 +1861,7 @@ static int try_set_ext_ctrls(struct v4l2_ctrl_handler *hdl,
/* First 'try' all controls and abort on error */
if (!ret)
- ret = try_or_set_ext_ctrls(hdl, cs, helpers, false);
+ ret = try_or_set_ext_ctrls(NULL, hdl, cs, helpers, false);
/* If this is a 'set' operation and the initial 'try' failed,
then set error_idx to count to tell the application that no
controls changed value yet. */
@@ -1868,7 +1871,7 @@ static int try_set_ext_ctrls(struct v4l2_ctrl_handler *hdl,
/* Reset 'handled' state */
for (i = 0; i < cs->count; i++)
helpers[i].handled = false;
- ret = try_or_set_ext_ctrls(hdl, cs, helpers, true);
+ ret = try_or_set_ext_ctrls(fh, hdl, cs, helpers, true);
}
if (cs->count > ARRAY_SIZE(helper))
@@ -1878,30 +1881,31 @@ static int try_set_ext_ctrls(struct v4l2_ctrl_handler *hdl,
int v4l2_try_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *cs)
{
- return try_set_ext_ctrls(hdl, cs, false);
+ return try_set_ext_ctrls(NULL, hdl, cs, false);
}
EXPORT_SYMBOL(v4l2_try_ext_ctrls);
-int v4l2_s_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *cs)
+int v4l2_s_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
+ struct v4l2_ext_controls *cs)
{
- return try_set_ext_ctrls(hdl, cs, true);
+ return try_set_ext_ctrls(fh, hdl, cs, true);
}
EXPORT_SYMBOL(v4l2_s_ext_ctrls);
int v4l2_subdev_try_ext_ctrls(struct v4l2_subdev *sd, struct v4l2_ext_controls *cs)
{
- return try_set_ext_ctrls(sd->ctrl_handler, cs, false);
+ return try_set_ext_ctrls(NULL, sd->ctrl_handler, cs, false);
}
EXPORT_SYMBOL(v4l2_subdev_try_ext_ctrls);
int v4l2_subdev_s_ext_ctrls(struct v4l2_subdev *sd, struct v4l2_ext_controls *cs)
{
- return try_set_ext_ctrls(sd->ctrl_handler, cs, true);
+ return try_set_ext_ctrls(NULL, sd->ctrl_handler, cs, true);
}
EXPORT_SYMBOL(v4l2_subdev_s_ext_ctrls);
/* Helper function for VIDIOC_S_CTRL compatibility */
-static int set_ctrl(struct v4l2_ctrl *ctrl, s32 *val)
+static int set_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, s32 *val)
{
struct v4l2_ctrl *master = ctrl->cluster[0];
int ret;
@@ -1916,15 +1920,16 @@ static int set_ctrl(struct v4l2_ctrl *ctrl, s32 *val)
ctrl->val = *val;
ctrl->is_new = 1;
- ret = try_or_set_control_cluster(master, false);
+ ret = try_or_set_control_cluster(NULL, master, false);
if (!ret)
- ret = try_or_set_control_cluster(master, true);
+ ret = try_or_set_control_cluster(fh, master, true);
*val = ctrl->cur.val;
v4l2_ctrl_unlock(ctrl);
return ret;
}
-int v4l2_s_ctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_control *control)
+int v4l2_s_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
+ struct v4l2_control *control)
{
struct v4l2_ctrl *ctrl = v4l2_ctrl_find(hdl, control->id);
@@ -1934,13 +1939,13 @@ int v4l2_s_ctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_control *control)
if (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY)
return -EACCES;
- return set_ctrl(ctrl, &control->value);
+ return set_ctrl(fh, ctrl, &control->value);
}
EXPORT_SYMBOL(v4l2_s_ctrl);
int v4l2_subdev_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *control)
{
- return v4l2_s_ctrl(sd->ctrl_handler, control);
+ return v4l2_s_ctrl(NULL, sd->ctrl_handler, control);
}
EXPORT_SYMBOL(v4l2_subdev_s_ctrl);
@@ -1948,6 +1953,6 @@ int v4l2_ctrl_s_ctrl(struct v4l2_ctrl *ctrl, s32 val)
{
/* It's a driver bug if this happens. */
WARN_ON(!type_is_int(ctrl));
- return set_ctrl(ctrl, &val);
+ return set_ctrl(NULL, ctrl, &val);
}
EXPORT_SYMBOL(v4l2_ctrl_s_ctrl);
diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
index 9811b1e..32107de 100644
--- a/drivers/media/video/v4l2-ioctl.c
+++ b/drivers/media/video/v4l2-ioctl.c
@@ -1481,11 +1481,11 @@ static long __video_do_ioctl(struct file *file,
dbgarg(cmd, "id=0x%x, value=%d\n", p->id, p->value);
if (vfh && vfh->ctrl_handler) {
- ret = v4l2_s_ctrl(vfh->ctrl_handler, p);
+ ret = v4l2_s_ctrl(vfh, vfh->ctrl_handler, p);
break;
}
if (vfd->ctrl_handler) {
- ret = v4l2_s_ctrl(vfd->ctrl_handler, p);
+ ret = v4l2_s_ctrl(NULL, vfd->ctrl_handler, p);
break;
}
if (ops->vidioc_s_ctrl) {
@@ -1530,9 +1530,9 @@ static long __video_do_ioctl(struct file *file,
break;
v4l_print_ext_ctrls(cmd, vfd, p, 1);
if (vfh && vfh->ctrl_handler)
- ret = v4l2_s_ext_ctrls(vfh->ctrl_handler, p);
+ ret = v4l2_s_ext_ctrls(vfh, vfh->ctrl_handler, p);
else if (vfd->ctrl_handler)
- ret = v4l2_s_ext_ctrls(vfd->ctrl_handler, p);
+ ret = v4l2_s_ext_ctrls(NULL, vfd->ctrl_handler, p);
else if (check_ext_ctrls(p, 0))
ret = ops->vidioc_s_ext_ctrls(file, fh, p);
break;
diff --git a/drivers/media/video/v4l2-subdev.c b/drivers/media/video/v4l2-subdev.c
index f396cc3..fd5dcca 100644
--- a/drivers/media/video/v4l2-subdev.c
+++ b/drivers/media/video/v4l2-subdev.c
@@ -164,13 +164,13 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
return v4l2_g_ctrl(vfh->ctrl_handler, arg);
case VIDIOC_S_CTRL:
- return v4l2_s_ctrl(vfh->ctrl_handler, arg);
+ return v4l2_s_ctrl(vfh, vfh->ctrl_handler, arg);
case VIDIOC_G_EXT_CTRLS:
return v4l2_g_ext_ctrls(vfh->ctrl_handler, arg);
case VIDIOC_S_EXT_CTRLS:
- return v4l2_s_ext_ctrls(vfh->ctrl_handler, arg);
+ return v4l2_s_ext_ctrls(vfh, vfh->ctrl_handler, arg);
case VIDIOC_TRY_EXT_CTRLS:
return v4l2_try_ext_ctrls(vfh->ctrl_handler, arg);
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 56323e3..e720f11 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -28,6 +28,7 @@
/* forward references */
struct v4l2_ctrl_handler;
struct v4l2_ctrl;
+struct v4l2_fh;
struct video_device;
struct v4l2_subdev;
@@ -485,15 +486,16 @@ s32 v4l2_ctrl_g_ctrl(struct v4l2_ctrl *ctrl);
*/
int v4l2_ctrl_s_ctrl(struct v4l2_ctrl *ctrl, s32 val);
-
/* Helpers for ioctl_ops. If hdl == NULL then they will all return -EINVAL. */
int v4l2_queryctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_queryctrl *qc);
int v4l2_querymenu(struct v4l2_ctrl_handler *hdl, struct v4l2_querymenu *qm);
int v4l2_g_ctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_control *ctrl);
-int v4l2_s_ctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_control *ctrl);
+int v4l2_s_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
+ struct v4l2_control *ctrl);
int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *c);
int v4l2_try_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *c);
-int v4l2_s_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *c);
+int v4l2_s_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
+ struct v4l2_ext_controls *c);
/* Helpers for subdevices. If the associated ctrl_handler == NULL then they
will all return -EINVAL. */
--
1.7.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [RFCv3 PATCH 11/18] v4l2-ctrls: add v4l2_fh pointer to the set control functions.
2011-06-07 15:05 ` [RFCv3 PATCH 11/18] v4l2-ctrls: add v4l2_fh pointer to the set control functions Hans Verkuil
@ 2011-06-27 21:20 ` Mauro Carvalho Chehab
2011-06-28 6:22 ` Hans Verkuil
0 siblings, 1 reply; 42+ messages in thread
From: Mauro Carvalho Chehab @ 2011-06-27 21:20 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, Hans Verkuil
Em 07-06-2011 12:05, Hans Verkuil escreveu:
> From: Hans Verkuil <hans.verkuil@cisco.com>
>
> When an application changes a control you want to generate an event.
> However, you want to avoid sending such an event back to the application
> (file handle) that caused the change.
Why?
I can see two usecases for an event-triggered control change:
1) when two applications are used, and one changed a value that could
affect the other;
2) as a way to implement async changes.
However, it seems, from your comments, that you're covering only case (1).
There are several reasons why we need to support case (2):
Some controls may be associated to a servo mechanism (like zoom, optical
focus, etc), or may require some time to happen (like charging a flash device).
So, it makes sense to have events back to the application that caused the change.
Kernel should not assume that the application that requested a change on a control
doesn't want to receive the notification back when the event actually happened.
This way, both cases will be covered.
Yet, I failed to see where, in the code, such restriction were imposed.
>
> Add the filehandle to the various set control functions.
>
> The filehandle isn't used yet, but the control event patches will need
> this.
>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
> drivers/media/video/v4l2-ctrls.c | 45 ++++++++++++++++++++----------------
> drivers/media/video/v4l2-ioctl.c | 8 +++---
> drivers/media/video/v4l2-subdev.c | 4 +-
> include/media/v4l2-ctrls.h | 8 ++++--
> 4 files changed, 36 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
> index c39ab0c..a38bdf9 100644
> --- a/drivers/media/video/v4l2-ctrls.c
> +++ b/drivers/media/video/v4l2-ctrls.c
> @@ -657,7 +657,8 @@ static int ctrl_is_volatile(struct v4l2_ext_control *c,
> }
>
> /* Copy the new value to the current value. */
> -static void new_to_cur(struct v4l2_ctrl *ctrl, bool update_inactive)
> +static void new_to_cur(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl,
> + bool update_inactive)
> {
> if (ctrl == NULL)
> return;
> @@ -1717,7 +1718,8 @@ EXPORT_SYMBOL(v4l2_ctrl_g_ctrl);
> /* Core function that calls try/s_ctrl and ensures that the new value is
> copied to the current value on a set.
> Must be called with ctrl->handler->lock held. */
> -static int try_or_set_control_cluster(struct v4l2_ctrl *master, bool set)
> +static int try_or_set_control_cluster(struct v4l2_fh *fh,
> + struct v4l2_ctrl *master, bool set)
> {
> bool update_flag;
> bool try = !set;
> @@ -1768,12 +1770,13 @@ static int try_or_set_control_cluster(struct v4l2_ctrl *master, bool set)
>
> update_flag = is_cur_manual(master) != is_new_manual(master);
> for (i = 0; i < master->ncontrols; i++)
> - new_to_cur(master->cluster[i], update_flag && i > 0);
> + new_to_cur(fh, master->cluster[i], update_flag && i > 0);
> return 0;
> }
>
> /* Try or set controls. */
> -static int try_or_set_ext_ctrls(struct v4l2_ctrl_handler *hdl,
> +static int try_or_set_ext_ctrls(struct v4l2_fh *fh,
> + struct v4l2_ctrl_handler *hdl,
> struct v4l2_ext_controls *cs,
> struct ctrl_helper *helpers,
> bool set)
> @@ -1818,7 +1821,7 @@ static int try_or_set_ext_ctrls(struct v4l2_ctrl_handler *hdl,
> ret = cluster_walk(i, cs, helpers, user_to_new);
>
> if (!ret)
> - ret = try_or_set_control_cluster(master, set);
> + ret = try_or_set_control_cluster(fh, master, set);
>
> /* Copy the new values back to userspace. */
> if (!ret)
> @@ -1831,7 +1834,7 @@ static int try_or_set_ext_ctrls(struct v4l2_ctrl_handler *hdl,
> }
>
> /* Try or try-and-set controls */
> -static int try_set_ext_ctrls(struct v4l2_ctrl_handler *hdl,
> +static int try_set_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
> struct v4l2_ext_controls *cs,
> bool set)
> {
> @@ -1858,7 +1861,7 @@ static int try_set_ext_ctrls(struct v4l2_ctrl_handler *hdl,
>
> /* First 'try' all controls and abort on error */
> if (!ret)
> - ret = try_or_set_ext_ctrls(hdl, cs, helpers, false);
> + ret = try_or_set_ext_ctrls(NULL, hdl, cs, helpers, false);
> /* If this is a 'set' operation and the initial 'try' failed,
> then set error_idx to count to tell the application that no
> controls changed value yet. */
> @@ -1868,7 +1871,7 @@ static int try_set_ext_ctrls(struct v4l2_ctrl_handler *hdl,
> /* Reset 'handled' state */
> for (i = 0; i < cs->count; i++)
> helpers[i].handled = false;
> - ret = try_or_set_ext_ctrls(hdl, cs, helpers, true);
> + ret = try_or_set_ext_ctrls(fh, hdl, cs, helpers, true);
> }
>
> if (cs->count > ARRAY_SIZE(helper))
> @@ -1878,30 +1881,31 @@ static int try_set_ext_ctrls(struct v4l2_ctrl_handler *hdl,
>
> int v4l2_try_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *cs)
> {
> - return try_set_ext_ctrls(hdl, cs, false);
> + return try_set_ext_ctrls(NULL, hdl, cs, false);
> }
> EXPORT_SYMBOL(v4l2_try_ext_ctrls);
>
> -int v4l2_s_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *cs)
> +int v4l2_s_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
> + struct v4l2_ext_controls *cs)
> {
> - return try_set_ext_ctrls(hdl, cs, true);
> + return try_set_ext_ctrls(fh, hdl, cs, true);
> }
> EXPORT_SYMBOL(v4l2_s_ext_ctrls);
>
> int v4l2_subdev_try_ext_ctrls(struct v4l2_subdev *sd, struct v4l2_ext_controls *cs)
> {
> - return try_set_ext_ctrls(sd->ctrl_handler, cs, false);
> + return try_set_ext_ctrls(NULL, sd->ctrl_handler, cs, false);
> }
> EXPORT_SYMBOL(v4l2_subdev_try_ext_ctrls);
>
> int v4l2_subdev_s_ext_ctrls(struct v4l2_subdev *sd, struct v4l2_ext_controls *cs)
> {
> - return try_set_ext_ctrls(sd->ctrl_handler, cs, true);
> + return try_set_ext_ctrls(NULL, sd->ctrl_handler, cs, true);
> }
> EXPORT_SYMBOL(v4l2_subdev_s_ext_ctrls);
>
> /* Helper function for VIDIOC_S_CTRL compatibility */
> -static int set_ctrl(struct v4l2_ctrl *ctrl, s32 *val)
> +static int set_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, s32 *val)
> {
> struct v4l2_ctrl *master = ctrl->cluster[0];
> int ret;
> @@ -1916,15 +1920,16 @@ static int set_ctrl(struct v4l2_ctrl *ctrl, s32 *val)
>
> ctrl->val = *val;
> ctrl->is_new = 1;
> - ret = try_or_set_control_cluster(master, false);
> + ret = try_or_set_control_cluster(NULL, master, false);
> if (!ret)
> - ret = try_or_set_control_cluster(master, true);
> + ret = try_or_set_control_cluster(fh, master, true);
> *val = ctrl->cur.val;
> v4l2_ctrl_unlock(ctrl);
> return ret;
> }
>
> -int v4l2_s_ctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_control *control)
> +int v4l2_s_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
> + struct v4l2_control *control)
> {
> struct v4l2_ctrl *ctrl = v4l2_ctrl_find(hdl, control->id);
>
> @@ -1934,13 +1939,13 @@ int v4l2_s_ctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_control *control)
> if (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY)
> return -EACCES;
>
> - return set_ctrl(ctrl, &control->value);
> + return set_ctrl(fh, ctrl, &control->value);
> }
> EXPORT_SYMBOL(v4l2_s_ctrl);
>
> int v4l2_subdev_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *control)
> {
> - return v4l2_s_ctrl(sd->ctrl_handler, control);
> + return v4l2_s_ctrl(NULL, sd->ctrl_handler, control);
> }
> EXPORT_SYMBOL(v4l2_subdev_s_ctrl);
>
> @@ -1948,6 +1953,6 @@ int v4l2_ctrl_s_ctrl(struct v4l2_ctrl *ctrl, s32 val)
> {
> /* It's a driver bug if this happens. */
> WARN_ON(!type_is_int(ctrl));
> - return set_ctrl(ctrl, &val);
> + return set_ctrl(NULL, ctrl, &val);
> }
> EXPORT_SYMBOL(v4l2_ctrl_s_ctrl);
> diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
> index 9811b1e..32107de 100644
> --- a/drivers/media/video/v4l2-ioctl.c
> +++ b/drivers/media/video/v4l2-ioctl.c
> @@ -1481,11 +1481,11 @@ static long __video_do_ioctl(struct file *file,
> dbgarg(cmd, "id=0x%x, value=%d\n", p->id, p->value);
>
> if (vfh && vfh->ctrl_handler) {
> - ret = v4l2_s_ctrl(vfh->ctrl_handler, p);
> + ret = v4l2_s_ctrl(vfh, vfh->ctrl_handler, p);
> break;
> }
> if (vfd->ctrl_handler) {
> - ret = v4l2_s_ctrl(vfd->ctrl_handler, p);
> + ret = v4l2_s_ctrl(NULL, vfd->ctrl_handler, p);
> break;
> }
> if (ops->vidioc_s_ctrl) {
> @@ -1530,9 +1530,9 @@ static long __video_do_ioctl(struct file *file,
> break;
> v4l_print_ext_ctrls(cmd, vfd, p, 1);
> if (vfh && vfh->ctrl_handler)
> - ret = v4l2_s_ext_ctrls(vfh->ctrl_handler, p);
> + ret = v4l2_s_ext_ctrls(vfh, vfh->ctrl_handler, p);
> else if (vfd->ctrl_handler)
> - ret = v4l2_s_ext_ctrls(vfd->ctrl_handler, p);
> + ret = v4l2_s_ext_ctrls(NULL, vfd->ctrl_handler, p);
> else if (check_ext_ctrls(p, 0))
> ret = ops->vidioc_s_ext_ctrls(file, fh, p);
> break;
> diff --git a/drivers/media/video/v4l2-subdev.c b/drivers/media/video/v4l2-subdev.c
> index f396cc3..fd5dcca 100644
> --- a/drivers/media/video/v4l2-subdev.c
> +++ b/drivers/media/video/v4l2-subdev.c
> @@ -164,13 +164,13 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> return v4l2_g_ctrl(vfh->ctrl_handler, arg);
>
> case VIDIOC_S_CTRL:
> - return v4l2_s_ctrl(vfh->ctrl_handler, arg);
> + return v4l2_s_ctrl(vfh, vfh->ctrl_handler, arg);
>
> case VIDIOC_G_EXT_CTRLS:
> return v4l2_g_ext_ctrls(vfh->ctrl_handler, arg);
>
> case VIDIOC_S_EXT_CTRLS:
> - return v4l2_s_ext_ctrls(vfh->ctrl_handler, arg);
> + return v4l2_s_ext_ctrls(vfh, vfh->ctrl_handler, arg);
>
> case VIDIOC_TRY_EXT_CTRLS:
> return v4l2_try_ext_ctrls(vfh->ctrl_handler, arg);
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index 56323e3..e720f11 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -28,6 +28,7 @@
> /* forward references */
> struct v4l2_ctrl_handler;
> struct v4l2_ctrl;
> +struct v4l2_fh;
> struct video_device;
> struct v4l2_subdev;
>
> @@ -485,15 +486,16 @@ s32 v4l2_ctrl_g_ctrl(struct v4l2_ctrl *ctrl);
> */
> int v4l2_ctrl_s_ctrl(struct v4l2_ctrl *ctrl, s32 val);
>
> -
> /* Helpers for ioctl_ops. If hdl == NULL then they will all return -EINVAL. */
> int v4l2_queryctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_queryctrl *qc);
> int v4l2_querymenu(struct v4l2_ctrl_handler *hdl, struct v4l2_querymenu *qm);
> int v4l2_g_ctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_control *ctrl);
> -int v4l2_s_ctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_control *ctrl);
> +int v4l2_s_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
> + struct v4l2_control *ctrl);
> int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *c);
> int v4l2_try_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *c);
> -int v4l2_s_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *c);
> +int v4l2_s_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
> + struct v4l2_ext_controls *c);
>
> /* Helpers for subdevices. If the associated ctrl_handler == NULL then they
> will all return -EINVAL. */
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [RFCv3 PATCH 11/18] v4l2-ctrls: add v4l2_fh pointer to the set control functions.
2011-06-27 21:20 ` Mauro Carvalho Chehab
@ 2011-06-28 6:22 ` Hans Verkuil
2011-06-28 10:27 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 42+ messages in thread
From: Hans Verkuil @ 2011-06-28 6:22 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: linux-media, Hans Verkuil
On Monday, June 27, 2011 23:20:07 Mauro Carvalho Chehab wrote:
> Em 07-06-2011 12:05, Hans Verkuil escreveu:
> > From: Hans Verkuil <hans.verkuil@cisco.com>
> >
> > When an application changes a control you want to generate an event.
> > However, you want to avoid sending such an event back to the application
> > (file handle) that caused the change.
>
> Why?
>
> I can see two usecases for an event-triggered control change:
> 1) when two applications are used, and one changed a value that could
> affect the other;
> 2) as a way to implement async changes.
>
> However, it seems, from your comments, that you're covering only case (1).
>
> There are several reasons why we need to support case (2):
>
> Some controls may be associated to a servo mechanism (like zoom, optical
> focus, etc), or may require some time to happen (like charging a flash device).
> So, it makes sense to have events back to the application that caused the change.
>
> Kernel should not assume that the application that requested a change on a control
> doesn't want to receive the notification back when the event actually happened.
> This way, both cases will be covered.
>
> Yet, I failed to see where, in the code, such restriction were imposed.
Async changes are triggered by the driver, not an application. Any changes
made by the driver will be sent to all applications.
That said, I think I should add a flag like V4L2_EVENT_SUB_FL_NO_FEEDBACK
to explicitly let applications decide.
That's easy enough.
Regards,
Hans
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFCv3 PATCH 11/18] v4l2-ctrls: add v4l2_fh pointer to the set control functions.
2011-06-28 6:22 ` Hans Verkuil
@ 2011-06-28 10:27 ` Mauro Carvalho Chehab
0 siblings, 0 replies; 42+ messages in thread
From: Mauro Carvalho Chehab @ 2011-06-28 10:27 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, Hans Verkuil
Em 28-06-2011 03:22, Hans Verkuil escreveu:
> On Monday, June 27, 2011 23:20:07 Mauro Carvalho Chehab wrote:
>> Em 07-06-2011 12:05, Hans Verkuil escreveu:
>>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>>
>>> When an application changes a control you want to generate an event.
>>> However, you want to avoid sending such an event back to the application
>>> (file handle) that caused the change.
>>
>> Why?
>>
>> I can see two usecases for an event-triggered control change:
>> 1) when two applications are used, and one changed a value that could
>> affect the other;
>> 2) as a way to implement async changes.
>>
>> However, it seems, from your comments, that you're covering only case (1).
>>
>> There are several reasons why we need to support case (2):
>>
>> Some controls may be associated to a servo mechanism (like zoom, optical
>> focus, etc), or may require some time to happen (like charging a flash device).
>> So, it makes sense to have events back to the application that caused the change.
>>
>> Kernel should not assume that the application that requested a change on a control
>> doesn't want to receive the notification back when the event actually happened.
>> This way, both cases will be covered.
>>
>> Yet, I failed to see where, in the code, such restriction were imposed.
>
> Async changes are triggered by the driver, not an application. Any changes
> made by the driver will be sent to all applications.
>
> That said, I think I should add a flag like V4L2_EVENT_SUB_FL_NO_FEEDBACK
> to explicitly let applications decide.
Agreed. it makes the code more generic.
>
> That's easy enough.
>
> Regards,
>
> Hans
^ permalink raw reply [flat|nested] 42+ messages in thread
* [RFCv3 PATCH 12/18] vb2_poll: don't start DMA, leave that to the first read().
2011-06-07 15:05 ` [RFCv3 PATCH 01/18] v4l2-ctrls: introduce call_op define Hans Verkuil
` (9 preceding siblings ...)
2011-06-07 15:05 ` [RFCv3 PATCH 11/18] v4l2-ctrls: add v4l2_fh pointer to the set control functions Hans Verkuil
@ 2011-06-07 15:05 ` Hans Verkuil
2011-06-27 21:52 ` Mauro Carvalho Chehab
2011-06-07 15:05 ` [RFCv3 PATCH 13/18] v4l2-ctrls: add control events Hans Verkuil
` (5 subsequent siblings)
16 siblings, 1 reply; 42+ messages in thread
From: Hans Verkuil @ 2011-06-07 15:05 UTC (permalink / raw)
To: linux-media; +Cc: Hans Verkuil
From: Hans Verkuil <hans.verkuil@cisco.com>
The vb2_poll function would start read-DMA if called without any streaming
in progress. This unfortunately does not work if the application just wants
to poll for exceptions. This information of what the application is polling
for is sadly unavailable in the driver.
Andy Walls suggested to just return POLLIN | POLLRDNORM and let the first
call to read() start the DMA. This initial read() call will return EAGAIN
since no actual data is available yet, but it does start the DMA.
Applications must handle EAGAIN in any case since there can be other reasons
for EAGAIN as well.
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
drivers/media/video/videobuf2-core.c | 17 +++--------------
1 files changed, 3 insertions(+), 14 deletions(-)
diff --git a/drivers/media/video/videobuf2-core.c b/drivers/media/video/videobuf2-core.c
index 6ba1461..ad75c95 100644
--- a/drivers/media/video/videobuf2-core.c
+++ b/drivers/media/video/videobuf2-core.c
@@ -1372,27 +1372,16 @@ static int __vb2_cleanup_fileio(struct vb2_queue *q);
unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait)
{
unsigned long flags;
- unsigned int ret;
struct vb2_buffer *vb = NULL;
/*
* Start file I/O emulator only if streaming API has not been used yet.
*/
if (q->num_buffers == 0 && q->fileio == NULL) {
- if (!V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_READ)) {
- ret = __vb2_init_fileio(q, 1);
- if (ret)
- return POLLERR;
- }
- if (V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_WRITE)) {
- ret = __vb2_init_fileio(q, 0);
- if (ret)
- return POLLERR;
- /*
- * Write to OUTPUT queue can be done immediately.
- */
+ if (!V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_READ))
+ return POLLIN | POLLRDNORM;
+ if (V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_WRITE))
return POLLOUT | POLLWRNORM;
- }
}
/*
--
1.7.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [RFCv3 PATCH 12/18] vb2_poll: don't start DMA, leave that to the first read().
2011-06-07 15:05 ` [RFCv3 PATCH 12/18] vb2_poll: don't start DMA, leave that to the first read() Hans Verkuil
@ 2011-06-27 21:52 ` Mauro Carvalho Chehab
2011-06-28 7:33 ` Hans Verkuil
0 siblings, 1 reply; 42+ messages in thread
From: Mauro Carvalho Chehab @ 2011-06-27 21:52 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, Hans Verkuil
Em 07-06-2011 12:05, Hans Verkuil escreveu:
> From: Hans Verkuil <hans.verkuil@cisco.com>
>
> The vb2_poll function would start read-DMA if called without any streaming
> in progress. This unfortunately does not work if the application just wants
> to poll for exceptions. This information of what the application is polling
> for is sadly unavailable in the driver.
>
> Andy Walls suggested to just return POLLIN | POLLRDNORM and let the first
> call to read() start the DMA. This initial read() call will return EAGAIN
> since no actual data is available yet, but it does start the DMA.
>
> Applications must handle EAGAIN in any case since there can be other reasons
> for EAGAIN as well.
>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
> drivers/media/video/videobuf2-core.c | 17 +++--------------
> 1 files changed, 3 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/media/video/videobuf2-core.c b/drivers/media/video/videobuf2-core.c
> index 6ba1461..ad75c95 100644
> --- a/drivers/media/video/videobuf2-core.c
> +++ b/drivers/media/video/videobuf2-core.c
> @@ -1372,27 +1372,16 @@ static int __vb2_cleanup_fileio(struct vb2_queue *q);
> unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait)
> {
> unsigned long flags;
> - unsigned int ret;
> struct vb2_buffer *vb = NULL;
>
> /*
> * Start file I/O emulator only if streaming API has not been used yet.
> */
> if (q->num_buffers == 0 && q->fileio == NULL) {
> - if (!V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_READ)) {
> - ret = __vb2_init_fileio(q, 1);
> - if (ret)
> - return POLLERR;
> - }
> - if (V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_WRITE)) {
> - ret = __vb2_init_fileio(q, 0);
> - if (ret)
> - return POLLERR;
> - /*
> - * Write to OUTPUT queue can be done immediately.
> - */
> + if (!V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_READ))
> + return POLLIN | POLLRDNORM;
> + if (V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_WRITE))
> return POLLOUT | POLLWRNORM;
> - }
> }
There is one behavior change on this patchset: __vb2_init_fileio() checks for some
troubles that may happen during device streaming initialization, returning POLLERR
if there is a problem there.
By moving this code to be called only at read, it means the poll() will not fail
anymore, but the first read() will fail. The man page for read() doesn't tell that
-EBUSY or -ENOMEM could happen there. The same happens with V4L2 specs. So, it is
clearly violating kernel ABI.
NACK.
Mauro.
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [RFCv3 PATCH 12/18] vb2_poll: don't start DMA, leave that to the first read().
2011-06-27 21:52 ` Mauro Carvalho Chehab
@ 2011-06-28 7:33 ` Hans Verkuil
2011-06-28 9:01 ` Hans Verkuil
2011-06-28 11:20 ` Mauro Carvalho Chehab
0 siblings, 2 replies; 42+ messages in thread
From: Hans Verkuil @ 2011-06-28 7:33 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: linux-media, Hans Verkuil
On Monday, June 27, 2011 23:52:37 Mauro Carvalho Chehab wrote:
> Em 07-06-2011 12:05, Hans Verkuil escreveu:
> > From: Hans Verkuil <hans.verkuil@cisco.com>
> >
> > The vb2_poll function would start read-DMA if called without any streaming
> > in progress. This unfortunately does not work if the application just wants
> > to poll for exceptions. This information of what the application is polling
> > for is sadly unavailable in the driver.
> >
> > Andy Walls suggested to just return POLLIN | POLLRDNORM and let the first
> > call to read() start the DMA. This initial read() call will return EAGAIN
> > since no actual data is available yet, but it does start the DMA.
> >
> > Applications must handle EAGAIN in any case since there can be other reasons
> > for EAGAIN as well.
> >
> > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > ---
> > drivers/media/video/videobuf2-core.c | 17 +++--------------
> > 1 files changed, 3 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/media/video/videobuf2-core.c b/drivers/media/video/videobuf2-core.c
> > index 6ba1461..ad75c95 100644
> > --- a/drivers/media/video/videobuf2-core.c
> > +++ b/drivers/media/video/videobuf2-core.c
> > @@ -1372,27 +1372,16 @@ static int __vb2_cleanup_fileio(struct vb2_queue *q);
> > unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait)
> > {
> > unsigned long flags;
> > - unsigned int ret;
> > struct vb2_buffer *vb = NULL;
> >
> > /*
> > * Start file I/O emulator only if streaming API has not been used yet.
> > */
> > if (q->num_buffers == 0 && q->fileio == NULL) {
> > - if (!V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_READ)) {
> > - ret = __vb2_init_fileio(q, 1);
> > - if (ret)
> > - return POLLERR;
> > - }
> > - if (V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_WRITE)) {
> > - ret = __vb2_init_fileio(q, 0);
> > - if (ret)
> > - return POLLERR;
> > - /*
> > - * Write to OUTPUT queue can be done immediately.
> > - */
> > + if (!V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_READ))
> > + return POLLIN | POLLRDNORM;
> > + if (V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_WRITE))
> > return POLLOUT | POLLWRNORM;
> > - }
> > }
>
> There is one behavior change on this patchset: __vb2_init_fileio() checks for some
> troubles that may happen during device streaming initialization, returning POLLERR
> if there is a problem there.
>
> By moving this code to be called only at read, it means the poll() will not fail
> anymore, but the first read() will fail. The man page for read() doesn't tell that
> -EBUSY or -ENOMEM could happen there. The same happens with V4L2 specs. So, it is
> clearly violating kernel ABI.
>
> NACK.
Actually, almost nothing changes in the ABI. It's counterintuitive, but
this is what happens:
1) The poll() function in a driver does not actually return any error codes.
It never did. It returns a poll mask, which is expected to be POLLERR in case
of any error. All that it does is that select() returns if it waits for reading
or writing. Any actual error codes are never propagated. This means BTW that
our poll manual page is wrong (what a surprise), most of those error codes can
never be returned.
2) Suppose we try to start streaming in poll. If this works, then poll returns,
with POLLIN set, and the next read() will succeed (actually, it can return an
error as well, but for other error conditions in case of e.g. hardware errors).
The problem with this is of course that this will also start the streaming if
all we wanted to wait for was an exception. That's not what we want at all.
Ideally we could inspect in the driver what the caller wanted to wait for, but
that information is not available, unfortunately.
3) The other case is that we try to start streaming in poll and it fails.
In that case any errors are lost and poll returns POLLERR (note that the poll(2)
manual page says that POLLERR is for output only, but clearly in the linux
kernel it is accepted both input and output).
But for the select() call this POLLERR information is lost as well and the
application will call read() anyway, which now will attempt to start the
streaming (again after poll() tried it the first time) and this time it
returns the actual error code.
Just try this with capture-example: start it in mmap mode, then try to run
it in read mode from a second console. The EBUSY error comes from the read()
command, not from select(). With or without this patch, capture-example
behaves exactly the same.
The only ABI change I see is with poll(2) and epoll(7) where POLLERR is no
longer returned. Since this isn't documented at all anyway (and the poll(2)
manual page is actually unclear about whether you can expect it), I am
actually quite happy with this change. After this analysis I realized it is
even better than expected.
I never liked that poll starts streaming, poll should be a fast function,
not something that does a lot of other things.
It's actually a very nice change, but I admit that it is tricky to analyze.
Regards,
Hans
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [RFCv3 PATCH 12/18] vb2_poll: don't start DMA, leave that to the first read().
2011-06-28 7:33 ` Hans Verkuil
@ 2011-06-28 9:01 ` Hans Verkuil
2011-06-28 11:20 ` Mauro Carvalho Chehab
1 sibling, 0 replies; 42+ messages in thread
From: Hans Verkuil @ 2011-06-28 9:01 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: linux-media, Hans Verkuil
On Tuesday, June 28, 2011 09:33:57 Hans Verkuil wrote:
> On Monday, June 27, 2011 23:52:37 Mauro Carvalho Chehab wrote:
> > Em 07-06-2011 12:05, Hans Verkuil escreveu:
> > > From: Hans Verkuil <hans.verkuil@cisco.com>
> > >
> > > The vb2_poll function would start read-DMA if called without any streaming
> > > in progress. This unfortunately does not work if the application just wants
> > > to poll for exceptions. This information of what the application is polling
> > > for is sadly unavailable in the driver.
> > >
> > > Andy Walls suggested to just return POLLIN | POLLRDNORM and let the first
> > > call to read() start the DMA. This initial read() call will return EAGAIN
> > > since no actual data is available yet, but it does start the DMA.
> > >
> > > Applications must handle EAGAIN in any case since there can be other reasons
> > > for EAGAIN as well.
> > >
> > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > > ---
> > > drivers/media/video/videobuf2-core.c | 17 +++--------------
> > > 1 files changed, 3 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/drivers/media/video/videobuf2-core.c b/drivers/media/video/videobuf2-core.c
> > > index 6ba1461..ad75c95 100644
> > > --- a/drivers/media/video/videobuf2-core.c
> > > +++ b/drivers/media/video/videobuf2-core.c
> > > @@ -1372,27 +1372,16 @@ static int __vb2_cleanup_fileio(struct vb2_queue *q);
> > > unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait)
> > > {
> > > unsigned long flags;
> > > - unsigned int ret;
> > > struct vb2_buffer *vb = NULL;
> > >
> > > /*
> > > * Start file I/O emulator only if streaming API has not been used yet.
> > > */
> > > if (q->num_buffers == 0 && q->fileio == NULL) {
> > > - if (!V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_READ)) {
> > > - ret = __vb2_init_fileio(q, 1);
> > > - if (ret)
> > > - return POLLERR;
> > > - }
> > > - if (V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_WRITE)) {
> > > - ret = __vb2_init_fileio(q, 0);
> > > - if (ret)
> > > - return POLLERR;
> > > - /*
> > > - * Write to OUTPUT queue can be done immediately.
> > > - */
> > > + if (!V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_READ))
> > > + return POLLIN | POLLRDNORM;
> > > + if (V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_WRITE))
> > > return POLLOUT | POLLWRNORM;
> > > - }
> > > }
> >
> > There is one behavior change on this patchset: __vb2_init_fileio() checks for some
> > troubles that may happen during device streaming initialization, returning POLLERR
> > if there is a problem there.
> >
> > By moving this code to be called only at read, it means the poll() will not fail
> > anymore, but the first read() will fail. The man page for read() doesn't tell that
> > -EBUSY or -ENOMEM could happen there. The same happens with V4L2 specs. So, it is
> > clearly violating kernel ABI.
> >
> > NACK.
>
> Actually, almost nothing changes in the ABI. It's counterintuitive, but
> this is what happens:
>
> 1) The poll() function in a driver does not actually return any error codes.
> It never did. It returns a poll mask, which is expected to be POLLERR in case
> of any error. All that it does is that select() returns if it waits for reading
> or writing. Any actual error codes are never propagated. This means BTW that
> our poll manual page is wrong (what a surprise), most of those error codes can
> never be returned.
>
> 2) Suppose we try to start streaming in poll. If this works, then poll returns,
> with POLLIN set, and the next read() will succeed (actually, it can return an
> error as well, but for other error conditions in case of e.g. hardware errors).
>
> The problem with this is of course that this will also start the streaming if
> all we wanted to wait for was an exception. That's not what we want at all.
> Ideally we could inspect in the driver what the caller wanted to wait for, but
> that information is not available, unfortunately.
>
> 3) The other case is that we try to start streaming in poll and it fails.
> In that case any errors are lost and poll returns POLLERR (note that the poll(2)
> manual page says that POLLERR is for output only, but clearly in the linux
> kernel it is accepted both input and output).
>
> But for the select() call this POLLERR information is lost as well and the
> application will call read() anyway, which now will attempt to start the
> streaming (again after poll() tried it the first time) and this time it
> returns the actual error code.
>
> Just try this with capture-example: start it in mmap mode, then try to run
> it in read mode from a second console. The EBUSY error comes from the read()
> command, not from select(). With or without this patch, capture-example
> behaves exactly the same.
>
> The only ABI change I see is with poll(2) and epoll(7) where POLLERR is no
> longer returned. Since this isn't documented at all anyway (and the poll(2)
> manual page is actually unclear about whether you can expect it), I am
> actually quite happy with this change. After this analysis I realized it is
> even better than expected.
>
> I never liked that poll starts streaming, poll should be a fast function,
> not something that does a lot of other things.
>
> It's actually a very nice change, but I admit that it is tricky to analyze.
Just two more remarks: POLLERR as a poll return code is pretty useless:
applications can't use it to determine what the error is. There is a big
difference between a ENOMEM or a EBUSY. The only way to find out is to do
a read().
Another problem I have with starting streaming in poll() is that starting
streaming can take a long time depending on the hardware. That's really not
what poll() is for. If this fd is part of a larger set with e.g. socket fds,
then you really don't want to have the event polling thread block for 10s of
milliseconds.
It's a very poor design. Changing this part doesn't affect applications as
far as I can see, but it makes poll() much simpler and usable again when you
just want to poll for events.
Note also that this change only affects applications that use the poll API
as opposed to select(), test for POLLERR in revents, use read() instead of
the streaming API and do not test for read() errors.
That's one API change I'm happy to make.
Regards,
Hans
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [RFCv3 PATCH 12/18] vb2_poll: don't start DMA, leave that to the first read().
2011-06-28 7:33 ` Hans Verkuil
2011-06-28 9:01 ` Hans Verkuil
@ 2011-06-28 11:20 ` Mauro Carvalho Chehab
2011-06-28 12:21 ` Andy Walls
1 sibling, 1 reply; 42+ messages in thread
From: Mauro Carvalho Chehab @ 2011-06-28 11:20 UTC (permalink / raw)
To: Hans Verkuil
Cc: linux-media, Hans Verkuil, Pawel Osciak, Marek Szyprowski,
Kyungmin Park
Em 28-06-2011 04:33, Hans Verkuil escreveu:
> On Monday, June 27, 2011 23:52:37 Mauro Carvalho Chehab wrote:
>> Em 07-06-2011 12:05, Hans Verkuil escreveu:
>>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>>
>>> The vb2_poll function would start read-DMA if called without any streaming
>>> in progress. This unfortunately does not work if the application just wants
>>> to poll for exceptions. This information of what the application is polling
>>> for is sadly unavailable in the driver.
>>>
>>> Andy Walls suggested to just return POLLIN | POLLRDNORM and let the first
>>> call to read() start the DMA. This initial read() call will return EAGAIN
>>> since no actual data is available yet, but it does start the DMA.
>>>
>>> Applications must handle EAGAIN in any case since there can be other reasons
>>> for EAGAIN as well.
>>>
>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>>> ---
>>> drivers/media/video/videobuf2-core.c | 17 +++--------------
>>> 1 files changed, 3 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/media/video/videobuf2-core.c b/drivers/media/video/videobuf2-core.c
>>> index 6ba1461..ad75c95 100644
>>> --- a/drivers/media/video/videobuf2-core.c
>>> +++ b/drivers/media/video/videobuf2-core.c
>>> @@ -1372,27 +1372,16 @@ static int __vb2_cleanup_fileio(struct vb2_queue *q);
>>> unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait)
>>> {
>>> unsigned long flags;
>>> - unsigned int ret;
>>> struct vb2_buffer *vb = NULL;
>>>
>>> /*
>>> * Start file I/O emulator only if streaming API has not been used yet.
>>> */
>>> if (q->num_buffers == 0 && q->fileio == NULL) {
>>> - if (!V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_READ)) {
>>> - ret = __vb2_init_fileio(q, 1);
>>> - if (ret)
>>> - return POLLERR;
>>> - }
>>> - if (V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_WRITE)) {
>>> - ret = __vb2_init_fileio(q, 0);
>>> - if (ret)
>>> - return POLLERR;
>>> - /*
>>> - * Write to OUTPUT queue can be done immediately.
>>> - */
>>> + if (!V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_READ))
>>> + return POLLIN | POLLRDNORM;
>>> + if (V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_WRITE))
>>> return POLLOUT | POLLWRNORM;
>>> - }
>>> }
>>
>> There is one behavior change on this patchset: __vb2_init_fileio() checks for some
>> troubles that may happen during device streaming initialization, returning POLLERR
>> if there is a problem there.
>>
>> By moving this code to be called only at read, it means the poll() will not fail
>> anymore, but the first read() will fail. The man page for read() doesn't tell that
>> -EBUSY or -ENOMEM could happen there. The same happens with V4L2 specs. So, it is
>> clearly violating kernel ABI.
>>
>> NACK.
>
> Actually, almost nothing changes in the ABI. It's counterintuitive, but
> this is what happens:
>
> 1) The poll() function in a driver does not actually return any error codes.
> It never did. It returns a poll mask, which is expected to be POLLERR in case
> of any error. All that it does is that select() returns if it waits for reading
> or writing. Any actual error codes are never propagated.
Yes, but POLLERR will return error on the vb2 cases that return -EBUSY or -ENOMEM.
This doesn't violate the ioctl ABI.
> This means BTW that
> our poll manual page is wrong (what a surprise), most of those error codes can
> never be returned.
True. The DocBook needs a fix. Posix describes it as:
http://pubs.opengroup.org/onlinepubs/009695399/functions/poll.html
>
> 2) Suppose we try to start streaming in poll. If this works, then poll returns,
> with POLLIN set, and the next read() will succeed (actually, it can return an
> error as well, but for other error conditions in case of e.g. hardware errors).
>
> The problem with this is of course that this will also start the streaming if
> all we wanted to wait for was an exception. That's not what we want at all.
> Ideally we could inspect in the driver what the caller wanted to wait for, but
> that information is not available, unfortunately.
>
> 3) The other case is that we try to start streaming in poll and it fails.
> In that case any errors are lost and poll returns POLLERR (note that the poll(2)
> manual page says that POLLERR is for output only, but clearly in the linux
> kernel it is accepted both input and output).
Posix doesn't impose such restriction.
> But for the select() call this POLLERR information is lost as well and the
> application will call read() anyway, which now will attempt to start the
> streaming (again after poll() tried it the first time) and this time it
> returns the actual error code.
The posix list of acceptable errors are:
http://pubs.opengroup.org/onlinepubs/009695399/functions/read.html
On that list, ENOMEM seems to be acceptable, but EBUSY doesn't.
We should not use anything outside the acceptable error codes there, as otherwise,
applications like cat, more, less, etc may not work. The read interface should
be simple enough to allow applications that are not aware of the V4L2 api to
work. So, it should follow whatever supported by standard files.
> Just try this with capture-example: start it in mmap mode, then try to run
> it in read mode from a second console. The EBUSY error comes from the read()
> command, not from select(). With or without this patch, capture-example
> behaves exactly the same.
>
> The only ABI change I see is with poll(2) and epoll(7) where POLLERR is no
> longer returned. Since this isn't documented at all anyway (and the poll(2)
> manual page is actually unclear about whether you can expect it),
Posix seems clear enough to me.
> I am
> actually quite happy with this change. After this analysis I realized it is
> even better than expected.
> I never liked that poll starts streaming, poll should be a fast function,
> not something that does a lot of other things.
>
> It's actually a very nice change, but I admit that it is tricky to analyze.
I'm not very comfortable with vb2 returning unexpected errors there. Also,
for me it is clear that, if read will fail, POLLERR should be rised.
Mauro.
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [RFCv3 PATCH 12/18] vb2_poll: don't start DMA, leave that to the first read().
2011-06-28 11:20 ` Mauro Carvalho Chehab
@ 2011-06-28 12:21 ` Andy Walls
2011-06-28 12:43 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 42+ messages in thread
From: Andy Walls @ 2011-06-28 12:21 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Hans Verkuil
Cc: linux-media, Hans Verkuil, Pawel Osciak, Marek Szyprowski,
Kyungmin Park
Mauro Carvalho Chehab <mchehab@redhat.com> wrote:
>Em 28-06-2011 04:33, Hans Verkuil escreveu:
>> On Monday, June 27, 2011 23:52:37 Mauro Carvalho Chehab wrote:
>>> Em 07-06-2011 12:05, Hans Verkuil escreveu:
>>>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>>>
>>>> The vb2_poll function would start read-DMA if called without any
>streaming
>>>> in progress. This unfortunately does not work if the application
>just wants
>>>> to poll for exceptions. This information of what the application is
>polling
>>>> for is sadly unavailable in the driver.
>>>>
>>>> Andy Walls suggested to just return POLLIN | POLLRDNORM and let the
>first
>>>> call to read() start the DMA. This initial read() call will return
>EAGAIN
>>>> since no actual data is available yet, but it does start the DMA.
>>>>
>>>> Applications must handle EAGAIN in any case since there can be
>other reasons
>>>> for EAGAIN as well.
>>>>
>>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>>>> ---
>>>> drivers/media/video/videobuf2-core.c | 17 +++--------------
>>>> 1 files changed, 3 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/drivers/media/video/videobuf2-core.c
>b/drivers/media/video/videobuf2-core.c
>>>> index 6ba1461..ad75c95 100644
>>>> --- a/drivers/media/video/videobuf2-core.c
>>>> +++ b/drivers/media/video/videobuf2-core.c
>>>> @@ -1372,27 +1372,16 @@ static int __vb2_cleanup_fileio(struct
>vb2_queue *q);
>>>> unsigned int vb2_poll(struct vb2_queue *q, struct file *file,
>poll_table *wait)
>>>> {
>>>> unsigned long flags;
>>>> - unsigned int ret;
>>>> struct vb2_buffer *vb = NULL;
>>>>
>>>> /*
>>>> * Start file I/O emulator only if streaming API has not been
>used yet.
>>>> */
>>>> if (q->num_buffers == 0 && q->fileio == NULL) {
>>>> - if (!V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_READ)) {
>>>> - ret = __vb2_init_fileio(q, 1);
>>>> - if (ret)
>>>> - return POLLERR;
>>>> - }
>>>> - if (V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_WRITE)) {
>>>> - ret = __vb2_init_fileio(q, 0);
>>>> - if (ret)
>>>> - return POLLERR;
>>>> - /*
>>>> - * Write to OUTPUT queue can be done immediately.
>>>> - */
>>>> + if (!V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_READ))
>>>> + return POLLIN | POLLRDNORM;
>>>> + if (V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_WRITE))
>>>> return POLLOUT | POLLWRNORM;
>>>> - }
>>>> }
>>>
>>> There is one behavior change on this patchset: __vb2_init_fileio()
>checks for some
>>> troubles that may happen during device streaming initialization,
>returning POLLERR
>>> if there is a problem there.
>>>
>>> By moving this code to be called only at read, it means the poll()
>will not fail
>>> anymore, but the first read() will fail. The man page for read()
>doesn't tell that
>>> -EBUSY or -ENOMEM could happen there. The same happens with V4L2
>specs. So, it is
>>> clearly violating kernel ABI.
>>>
>>> NACK.
>>
>> Actually, almost nothing changes in the ABI. It's counterintuitive,
>but
>> this is what happens:
>>
>> 1) The poll() function in a driver does not actually return any error
>codes.
>> It never did. It returns a poll mask, which is expected to be POLLERR
>in case
>> of any error. All that it does is that select() returns if it waits
>for reading
>> or writing. Any actual error codes are never propagated.
>
>Yes, but POLLERR will return error on the vb2 cases that return -EBUSY
>or -ENOMEM.
>This doesn't violate the ioctl ABI.
>
>> This means BTW that
>> our poll manual page is wrong (what a surprise), most of those error
>codes can
>> never be returned.
>
>True. The DocBook needs a fix. Posix describes it as:
> http://pubs.opengroup.org/onlinepubs/009695399/functions/poll.html
>
>>
>> 2) Suppose we try to start streaming in poll. If this works, then
>poll returns,
>> with POLLIN set, and the next read() will succeed (actually, it can
>return an
>> error as well, but for other error conditions in case of e.g.
>hardware errors).
>>
>> The problem with this is of course that this will also start the
>streaming if
>> all we wanted to wait for was an exception. That's not what we want
>at all.
>> Ideally we could inspect in the driver what the caller wanted to wait
>for, but
>> that information is not available, unfortunately.
>>
>> 3) The other case is that we try to start streaming in poll and it
>fails.
>> In that case any errors are lost and poll returns POLLERR (note that
>the poll(2)
>> manual page says that POLLERR is for output only, but clearly in the
>linux
>> kernel it is accepted both input and output).
>
>Posix doesn't impose such restriction.
>
>> But for the select() call this POLLERR information is lost as well
>and the
>> application will call read() anyway, which now will attempt to start
>the
>> streaming (again after poll() tried it the first time) and this time
>it
>> returns the actual error code.
>
>The posix list of acceptable errors are:
> http://pubs.opengroup.org/onlinepubs/009695399/functions/read.html
>On that list, ENOMEM seems to be acceptable, but EBUSY doesn't.
>
>We should not use anything outside the acceptable error codes there, as
>otherwise,
>applications like cat, more, less, etc may not work. The read interface
>should
>be simple enough to allow applications that are not aware of the V4L2
>api to
>work. So, it should follow whatever supported by standard files.
>
>> Just try this with capture-example: start it in mmap mode, then try
>to run
>> it in read mode from a second console. The EBUSY error comes from the
>read()
>> command, not from select(). With or without this patch,
>capture-example
>> behaves exactly the same.
>>
>> The only ABI change I see is with poll(2) and epoll(7) where POLLERR
>is no
>> longer returned. Since this isn't documented at all anyway (and the
>poll(2)
>> manual page is actually unclear about whether you can expect it),
>
>Posix seems clear enough to me.
>
>> I am
>> actually quite happy with this change. After this analysis I realized
>it is
>> even better than expected.
>
>> I never liked that poll starts streaming, poll should be a fast
>function,
>> not something that does a lot of other things.
>>
>> It's actually a very nice change, but I admit that it is tricky to
>analyze.
>
>I'm not very comfortable with vb2 returning unexpected errors there.
>Also,
>for me it is clear that, if read will fail, POLLERR should be rised.
>
>Mauro.
>--
>To unsubscribe from this list: send the line "unsubscribe linux-media"
>in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at http://vger.kernel.org/majordomo-info.html
It is also the case that a driver's poll method should never sleep.
I will try to find the conversation I had with laurent on interpreting the POSIX spec on error returns from select() and poll(). I will also try to find links to previos discussion with Hans on this.
One issue is how to start streaming with apps that:
- Open /dev/video/ in a nonblocking mode, and
- Use the read() method
while doing it in a way that is POSIX compliant and doesn't break existing apps.
The other constraint is to ensure when only poll()-ing for exception conditions, not having significant IO side effects.
I'm pretty sure sleeping in a driver's poll() method, or having significant side effects, is not ine the spirit of the POSIX select() and poll(), even if the letter of POSIX says nothing about it.
The method I suggested to Hans is completely POSIX compliant for apps using read() and select() and was checked against MythTV as having no bad side effects. (And by thought experiment doesn't break any sensible app using nonblocking IO with select() and read().)
I did not do analysis for apps that use mmap(), which I guess is the current concern.
Regards,
Andy
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [RFCv3 PATCH 12/18] vb2_poll: don't start DMA, leave that to the first read().
2011-06-28 12:21 ` Andy Walls
@ 2011-06-28 12:43 ` Mauro Carvalho Chehab
2011-06-28 13:58 ` Hans Verkuil
2011-06-28 23:14 ` Andy Walls
0 siblings, 2 replies; 42+ messages in thread
From: Mauro Carvalho Chehab @ 2011-06-28 12:43 UTC (permalink / raw)
To: Andy Walls
Cc: Hans Verkuil, linux-media, Hans Verkuil, Pawel Osciak,
Marek Szyprowski, Kyungmin Park
Em 28-06-2011 09:21, Andy Walls escreveu:
> Mauro Carvalho Chehab <mchehab@redhat.com> wrote:
>> I'm not very comfortable with vb2 returning unexpected errors there.
>> Also,
>> for me it is clear that, if read will fail, POLLERR should be rised.
>>
>> Mauro.
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-media"
>> in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> It is also the case that a driver's poll method should never sleep.
True.
> I will try to find the conversation I had with laurent on interpreting the POSIX spec on error returns from select() and poll(). I will also try to find links to previos discussion with Hans on this.
>
> One issue is how to start streaming with apps that:
> - Open /dev/video/ in a nonblocking mode, and
> - Use the read() method
>
> while doing it in a way that is POSIX compliant and doesn't break existing apps.
Well, a first call for poll() may rise a thread that will prepare the buffers, and
return with 0 while there's no data available.
> The other constraint is to ensure when only poll()-ing for exception conditions, not having significant IO side effects.
>
> I'm pretty sure sleeping in a driver's poll() method, or having significant side effects, is not ine the spirit of the POSIX select() and poll(), even if the letter of POSIX says nothing about it.
>
> The method I suggested to Hans is completely POSIX compliant for apps using read() and select() and was checked against MythTV as having no bad side effects. (And by thought experiment doesn't break any sensible app using nonblocking IO with select() and read().)
>
> I did not do analysis for apps that use mmap(), which I guess is the current concern.
The concern is that it is pointing that there are available data, even when there is an error.
This looks like a POSIX violation for me.
Cheers,
Mauro.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFCv3 PATCH 12/18] vb2_poll: don't start DMA, leave that to the first read().
2011-06-28 12:43 ` Mauro Carvalho Chehab
@ 2011-06-28 13:58 ` Hans Verkuil
2011-06-29 6:30 ` Hans Verkuil
2011-06-28 23:14 ` Andy Walls
1 sibling, 1 reply; 42+ messages in thread
From: Hans Verkuil @ 2011-06-28 13:58 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Andy Walls, linux-media, Hans Verkuil, Pawel Osciak,
Marek Szyprowski, Kyungmin Park
On Tuesday, June 28, 2011 14:43:22 Mauro Carvalho Chehab wrote:
> Em 28-06-2011 09:21, Andy Walls escreveu:
> > Mauro Carvalho Chehab <mchehab@redhat.com> wrote:
>
> >> I'm not very comfortable with vb2 returning unexpected errors there.
> >> Also,
> >> for me it is clear that, if read will fail, POLLERR should be rised.
> >>
> >> Mauro.
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-media"
> >> in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
> > It is also the case that a driver's poll method should never sleep.
>
> True.
Actually, it is allowed, but only since kernel 2.6.29 (before that it could
apparently give rise to busy looping if you were unlucky). But the main use
case is userspace file systems like fuse. Not so much in regular drivers.
Since drivers can sleep when starting streaming (ivtv will do that, in any
case), we were in violation of the poll kernel API for a long time :-)
> > I will try to find the conversation I had with laurent on interpreting the POSIX spec on error returns from select() and poll(). I will also try to find links to previos discussion with Hans on this.
> >
> > One issue is how to start streaming with apps that:
> > - Open /dev/video/ in a nonblocking mode, and
> > - Use the read() method
> >
> > while doing it in a way that is POSIX compliant and doesn't break existing apps.
>
> Well, a first call for poll() may rise a thread that will prepare the buffers, and
> return with 0 while there's no data available.
There is actually no guarantee whatsoever that if poll says you can read(), that that
read also has to succeed. Other threads can have read the data already, and errors may
have occured. And in fact, just starting streaming gives no guarantee that there is
anything to read. For example, starting the DMA engine when there is no valid input
signal. Many drivers (certainly those dealing with digital interfaces as opposed to
analog) will just sit and wait. A non-blocking read will just return 0 without
reading anything.
So the current poll implementation (and that includes the one in videobuf-core.c as
well) actually does *not* give any guarantee about whether data will be available
in read().
And from the same POSIX link you posted:
"The poll() function shall support regular files, terminal and pseudo-terminal devices,
FIFOs, pipes, sockets and [XSR] [Option Start] STREAMS-based files. [Option End]
The behavior of poll() on elements of fds that refer to other types of file is unspecified."
Note the last line: we do not fall under this posix document.
> > The other constraint is to ensure when only poll()-ing for exception conditions, not having significant IO side effects.
> >
> > I'm pretty sure sleeping in a driver's poll() method, or having significant side effects, is not ine the spirit of the POSIX select() and poll(), even if the letter of POSIX says nothing about it.
> >
> > The method I suggested to Hans is completely POSIX compliant for apps using read() and select() and was checked against MythTV as having no bad side effects. (And by thought experiment doesn't break any sensible app using nonblocking IO with select() and read().)
> >
> > I did not do analysis for apps that use mmap(), which I guess is the current concern.
There isn't a problem with mmap(). For the stream I/O API you have to call STREAMON
explicitly in order to start streaming. poll() will not do that for you.
I was thinking that one improvement that could be realized is that vb2_poll could
do some basic checks, such as checking whether streaming was already in progress
(EBUSY), but then I realized that it already does that: this code is only active
if there is no streaming in progress anyway.
Regards,
Hans
>
> The concern is that it is pointing that there are available data, even when there is an error.
> This looks like a POSIX violation for me.
>
> Cheers,
> Mauro.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFCv3 PATCH 12/18] vb2_poll: don't start DMA, leave that to the first read().
2011-06-28 13:58 ` Hans Verkuil
@ 2011-06-29 6:30 ` Hans Verkuil
0 siblings, 0 replies; 42+ messages in thread
From: Hans Verkuil @ 2011-06-29 6:30 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Andy Walls, linux-media, Hans Verkuil, Pawel Osciak,
Marek Szyprowski, Kyungmin Park
On Tuesday, June 28, 2011 15:58:36 Hans Verkuil wrote:
> On Tuesday, June 28, 2011 14:43:22 Mauro Carvalho Chehab wrote:
> > Em 28-06-2011 09:21, Andy Walls escreveu:
> > > Mauro Carvalho Chehab <mchehab@redhat.com> wrote:
> >
> > >> I'm not very comfortable with vb2 returning unexpected errors there.
> > >> Also,
> > >> for me it is clear that, if read will fail, POLLERR should be rised.
> > >>
> > >> Mauro.
> > >> --
> > >> To unsubscribe from this list: send the line "unsubscribe linux-media"
> > >> in
> > >> the body of a message to majordomo@vger.kernel.org
> > >> More majordomo info at http://vger.kernel.org/majordomo-info.html
> > >
> > > It is also the case that a driver's poll method should never sleep.
> >
> > True.
>
> Actually, it is allowed, but only since kernel 2.6.29 (before that it could
> apparently give rise to busy looping if you were unlucky). But the main use
> case is userspace file systems like fuse. Not so much in regular drivers.
>
> Since drivers can sleep when starting streaming (ivtv will do that, in any
> case), we were in violation of the poll kernel API for a long time :-)
>
> > > I will try to find the conversation I had with laurent on interpreting the POSIX spec on error returns from select() and poll(). I will also try to find links to previos discussion with Hans on this.
> > >
> > > One issue is how to start streaming with apps that:
> > > - Open /dev/video/ in a nonblocking mode, and
> > > - Use the read() method
> > >
> > > while doing it in a way that is POSIX compliant and doesn't break existing apps.
> >
> > Well, a first call for poll() may rise a thread that will prepare the buffers, and
> > return with 0 while there's no data available.
>
> There is actually no guarantee whatsoever that if poll says you can read(), that that
> read also has to succeed. Other threads can have read the data already, and errors may
> have occured. And in fact, just starting streaming gives no guarantee that there is
> anything to read. For example, starting the DMA engine when there is no valid input
> signal. Many drivers (certainly those dealing with digital interfaces as opposed to
> analog) will just sit and wait. A non-blocking read will just return 0 without
> reading anything.
>
> So the current poll implementation (and that includes the one in videobuf-core.c as
> well) actually does *not* give any guarantee about whether data will be available
> in read().
Never mind this. I didn't look carefully enough: it does start the DMA and then poll
waits for data to arrive. So when poll says there is data, then there is really data.
Although applications should always handle EAGAIN anyway: some drivers do return it,
even when data is supposed to be available (I had to add that to qv4l2 at one time).
Regards,
Hans
>
> And from the same POSIX link you posted:
>
> "The poll() function shall support regular files, terminal and pseudo-terminal devices,
> FIFOs, pipes, sockets and [XSR] [Option Start] STREAMS-based files. [Option End]
> The behavior of poll() on elements of fds that refer to other types of file is unspecified."
>
> Note the last line: we do not fall under this posix document.
>
> > > The other constraint is to ensure when only poll()-ing for exception conditions, not having significant IO side effects.
> > >
> > > I'm pretty sure sleeping in a driver's poll() method, or having significant side effects, is not ine the spirit of the POSIX select() and poll(), even if the letter of POSIX says nothing about it.
> > >
> > > The method I suggested to Hans is completely POSIX compliant for apps using read() and select() and was checked against MythTV as having no bad side effects. (And by thought experiment doesn't break any sensible app using nonblocking IO with select() and read().)
> > >
> > > I did not do analysis for apps that use mmap(), which I guess is the current concern.
>
> There isn't a problem with mmap(). For the stream I/O API you have to call STREAMON
> explicitly in order to start streaming. poll() will not do that for you.
>
> I was thinking that one improvement that could be realized is that vb2_poll could
> do some basic checks, such as checking whether streaming was already in progress
> (EBUSY), but then I realized that it already does that: this code is only active
> if there is no streaming in progress anyway.
>
> Regards,
>
> Hans
>
> >
> > The concern is that it is pointing that there are available data, even when there is an error.
> > This looks like a POSIX violation for me.
> >
> > Cheers,
> > Mauro.
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-media" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFCv3 PATCH 12/18] vb2_poll: don't start DMA, leave that to the first read().
2011-06-28 12:43 ` Mauro Carvalho Chehab
2011-06-28 13:58 ` Hans Verkuil
@ 2011-06-28 23:14 ` Andy Walls
2011-06-29 0:00 ` Mauro Carvalho Chehab
1 sibling, 1 reply; 42+ messages in thread
From: Andy Walls @ 2011-06-28 23:14 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Hans Verkuil, linux-media, Hans Verkuil, Pawel Osciak,
Marek Szyprowski, Kyungmin Park
On Tue, 2011-06-28 at 09:43 -0300, Mauro Carvalho Chehab wrote:
> Em 28-06-2011 09:21, Andy Walls escreveu:
> > It is also the case that a driver's poll method should never sleep.
>
> True.
> > One issue is how to start streaming with apps that:
> > - Open /dev/video/ in a nonblocking mode, and
> > - Use the read() method
> >
> > while doing it in a way that is POSIX compliant and doesn't break existing apps.
>
> Well, a first call for poll() may rise a thread that will prepare the buffers, and
> return with 0 while there's no data available.
Sure, but that doesn't solve the problem of an app only select()-ing or
poll()-ing for exception fd's and not starting any IO.
> > The other constraint is to ensure when only poll()-ing for exception
> conditions, not having significant IO side effects.
> >
> > I'm pretty sure sleeping in a driver's poll() method, or having
> significant side effects, is not ine the spirit of the POSIX select()
> and poll(), even if the letter of POSIX says nothing about it.
> >
> > The method I suggested to Hans is completely POSIX compliant for
> apps using read() and select() and was checked against MythTV as
> having no bad side effects. (And by thought experiment doesn't break
> any sensible app using nonblocking IO with select() and read().)
> >
> > I did not do analysis for apps that use mmap(), which I guess is the
> current concern.
>
> The concern is that it is pointing that there are available data, even
> when there is an error.
> This looks like a POSIX violation for me.
It isn't.
>From the specification for select():
http://pubs.opengroup.org/onlinepubs/009695399/functions/select.html
"A descriptor shall be considered ready for reading when a call to an
input function with O_NONBLOCK clear would not block, whether or not the
function would transfer data successfully. (The function might return
data, an end-of-file indication, or an error other than one indicating
that it is blocked, and in each of these cases the descriptor shall be
considered ready for reading.)"
To a userspace app, a non-blocking read() can always return an error,
regarless of the previous select() or poll() result. And all
applications that use select() or poll() folowed by a nonblocking read()
should be prepared to handle an errno from the read().
However, that excerpt from the select() specification does imply, that
perhaps, the driver should probably start streaming using a work item
and one of the CMWQ workers, so that the read() doesn't block.
Regards,
Andy
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFCv3 PATCH 12/18] vb2_poll: don't start DMA, leave that to the first read().
2011-06-28 23:14 ` Andy Walls
@ 2011-06-29 0:00 ` Mauro Carvalho Chehab
2011-06-29 5:08 ` Andy Walls
0 siblings, 1 reply; 42+ messages in thread
From: Mauro Carvalho Chehab @ 2011-06-29 0:00 UTC (permalink / raw)
To: Andy Walls
Cc: Hans Verkuil, linux-media, Hans Verkuil, Pawel Osciak,
Marek Szyprowski, Kyungmin Park
Em 28-06-2011 20:14, Andy Walls escreveu:
> On Tue, 2011-06-28 at 09:43 -0300, Mauro Carvalho Chehab wrote:
>> Em 28-06-2011 09:21, Andy Walls escreveu:
>
>>> It is also the case that a driver's poll method should never sleep.
>>
>> True.
>
>>> One issue is how to start streaming with apps that:
>>> - Open /dev/video/ in a nonblocking mode, and
>>> - Use the read() method
>>>
>>> while doing it in a way that is POSIX compliant and doesn't break existing apps.
>>
>> Well, a first call for poll() may rise a thread that will prepare the buffers, and
>> return with 0 while there's no data available.
>
> Sure, but that doesn't solve the problem of an app only select()-ing or
> poll()-ing for exception fd's and not starting any IO.
Well, a file descriptor can be used only for one thing: or it is a stream file
descriptor, or it is an event descriptor. You can't have both for the same
file descriptor. If an application need to check for both, the standard Unix way is:
fd_set set;
FD_ZERO (&set);
FD_SET (fd_stream, &set);
FD_SET (fd_event, &set);
select (FD_SETSIZE, &set, NULL, NULL, &timeout);
In other words, or the events nodes need to be different, or an ioctl is needed
in order to tell the Kernel that the associated file descriptor will be used
for an event, and that vb2 should not bother with it.
>>> The other constraint is to ensure when only poll()-ing for exception
>> conditions, not having significant IO side effects.
>>>
>>> I'm pretty sure sleeping in a driver's poll() method, or having
>> significant side effects, is not ine the spirit of the POSIX select()
>> and poll(), even if the letter of POSIX says nothing about it.
>>>
>>> The method I suggested to Hans is completely POSIX compliant for
>> apps using read() and select() and was checked against MythTV as
>> having no bad side effects. (And by thought experiment doesn't break
>> any sensible app using nonblocking IO with select() and read().)
>>>
>>> I did not do analysis for apps that use mmap(), which I guess is the
>> current concern.
>>
>> The concern is that it is pointing that there are available data, even
>> when there is an error.
>> This looks like a POSIX violation for me.
>
> It isn't.
>
> From the specification for select():
> http://pubs.opengroup.org/onlinepubs/009695399/functions/select.html
>
> "A descriptor shall be considered ready for reading when a call to an
> input function with O_NONBLOCK clear would not block, whether or not the
> function would transfer data successfully. (The function might return
> data, an end-of-file indication, or an error other than one indicating
> that it is blocked, and in each of these cases the descriptor shall be
> considered ready for reading.)"
My understanding from the above is that "ready" means:
- data;
- EOF;
- error.
if POLLIN (or POLLOUT) is returned, it should mean one of the above, e. g.
the device is ready to provide data or some error occurred.
Btw, we're talking about poll(), and not select(). The poll() doc is clearer
(http://pubs.opengroup.org/onlinepubs/009695399/functions/poll.html):
POLLIN
Data other than high-priority data may be read without blocking.
POLLOUT
Normal data may be written without blocking.
Saying that a -EAGAIN means that data is "ready" is an API abuse. This
can actually work, but it will effectively mean that poll or select won't
do anything, except for wasting a some CPU cycles.
> To a userspace app, a non-blocking read() can always return an error,
> regarless of the previous select() or poll() result. And all
> applications that use select() or poll() folowed by a nonblocking read()
> should be prepared to handle an errno from the read().
>
> However, that excerpt from the select() specification does imply, that
> perhaps, the driver should probably start streaming using a work item
> and one of the CMWQ workers, so that the read() doesn't block.
Cheers,
Mauro.
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [RFCv3 PATCH 12/18] vb2_poll: don't start DMA, leave that to the first read().
2011-06-29 0:00 ` Mauro Carvalho Chehab
@ 2011-06-29 5:08 ` Andy Walls
2011-06-29 11:37 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 42+ messages in thread
From: Andy Walls @ 2011-06-29 5:08 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Hans Verkuil, linux-media, Hans Verkuil, Pawel Osciak,
Marek Szyprowski, Kyungmin Park
On Tue, 2011-06-28 at 21:00 -0300, Mauro Carvalho Chehab wrote:
> Em 28-06-2011 20:14, Andy Walls escreveu:
> > On Tue, 2011-06-28 at 09:43 -0300, Mauro Carvalho Chehab wrote:
> >> Em 28-06-2011 09:21, Andy Walls escreveu:
> >
> >>> It is also the case that a driver's poll method should never sleep.
> >>
> >> True.
> >
> >>> One issue is how to start streaming with apps that:
> >>> - Open /dev/video/ in a nonblocking mode, and
> >>> - Use the read() method
> >>>
> >>> while doing it in a way that is POSIX compliant and doesn't break existing apps.
> >>
> >> Well, a first call for poll() may rise a thread that will prepare the buffers, and
> >> return with 0 while there's no data available.
> >
> > Sure, but that doesn't solve the problem of an app only select()-ing or
> > poll()-ing for exception fd's and not starting any IO.
>
> Well, a file descriptor can be used only for one thing: or it is a stream file
> descriptor, or it is an event descriptor. You can't have both for the same
> file descriptor. If an application need to check for both, the standard Unix way is:
>
> fd_set set;
>
> FD_ZERO (&set);
> FD_SET (fd_stream, &set);
> FD_SET (fd_event, &set);
>
> select (FD_SETSIZE, &set, NULL, NULL, &timeout);
>
> In other words, or the events nodes need to be different, or an ioctl is needed
> in order to tell the Kernel that the associated file descriptor will be used
> for an event, and that vb2 should not bother with it.
Um, no, that is not correct for Unix fd's and socket descriptors in
general. I realize that v4l2 events need to be enabled with an ioctl(),
but do we have a restriction that that can't happen on the same fd as
the one used for streaming?
Back in the days before threads were commonly available on Unix systems,
a process would use a single thread calling select() to handle I/O on a
serial port:
fd_set rfds, wfds;
int ttyfd;
...
FD_ZERO(&rfds);
FD_SET(ttyfd, &rfds);
FD_ZERO(&wfds);
FD_SET(ttyfd, &wfds);
n = select(ttyfd+1, &rfds, &wfds, NULL, NULL);
Or TCP socket
fd_set rfds, wfds, efds;
int sockd;
...
FD_ZERO(&rfds);
FD_SET(sockd, &rfds);
FD_ZERO(&wfds);
FD_SET(sockd, &wfds);
FD_ZERO(&efds);
FD_SET(sockd, &efds);
n = select(sockd+1, &rfds, &wfds, &efds, NULL);
> >>> The other constraint is to ensure when only poll()-ing for exception
> >> conditions, not having significant IO side effects.
> >>>
> >>> I'm pretty sure sleeping in a driver's poll() method, or having
> >> significant side effects, is not ine the spirit of the POSIX select()
> >> and poll(), even if the letter of POSIX says nothing about it.
> >>>
> >>> The method I suggested to Hans is completely POSIX compliant for
> >> apps using read() and select() and was checked against MythTV as
> >> having no bad side effects. (And by thought experiment doesn't break
> >> any sensible app using nonblocking IO with select() and read().)
> >>>
> >>> I did not do analysis for apps that use mmap(), which I guess is the
> >> current concern.
> >>
> >> The concern is that it is pointing that there are available data, even
> >> when there is an error.
> >> This looks like a POSIX violation for me.
> >
> > It isn't.
> >
> > From the specification for select():
> > http://pubs.opengroup.org/onlinepubs/009695399/functions/select.html
> >
> > "A descriptor shall be considered ready for reading when a call to an
> > input function with O_NONBLOCK clear would not block, whether or not the
> > function would transfer data successfully. (The function might return
> > data, an end-of-file indication, or an error other than one indicating
> > that it is blocked, and in each of these cases the descriptor shall be
> > considered ready for reading.)"
>
> My understanding from the above is that "ready" means:
> - data;
> - EOF;
> - error.
Waiting for data to arrive on an fd, while not streaming is an error
condition for select() should return. Something has to be done about
that fd in that case, or select()-ing on it is futile.
> if POLLIN (or POLLOUT) is returned, it should mean one of the above, e. g.
> the device is ready to provide data or some error occurred.
>
> Btw, we're talking about poll(), and not select(). The poll() doc is clearer
> (http://pubs.opengroup.org/onlinepubs/009695399/functions/poll.html):
>
> POLLIN
> Data other than high-priority data may be read without blocking.
>
> POLLOUT
> Normal data may be written without blocking.
>
> Saying that a -EAGAIN means that data is "ready" is an API abuse.
EAGAIN is the correct errno fo the read that returns no data. The abuse
is the revent value returned from poll(). However, for any application
that open()s a device node that supports mutliple open()s, it is a valid
circumstance that can happen with POSIX.
One way to avoid abusing the revent value returned by poll(), for the
initial poll() call where streaming has not started, is to modify the
kernel infrastructure that calls the driver poll() methods. If the
kernel passes down to the driver for what the caller is polling, the
driver poll() method could know that only the exception set for an fd
was being poll()-ed. In that case, the driver would know not to start
streaming and use a return value that made sense.
That solution seems like a lot of work and a large perturbation to the
kernel.
The only other solution I can think of is to do nothing. Then
v4l2-event monitoring will always have the unfortunate side effect of
starting the stream.
> This
> can actually work, but it will effectively mean that poll or select won't
> do anything, except for wasting a some CPU cycles.
The wasted CPU cycles are an insignificant, one-time penalty during the
first iteration of a loop that will use several orders of magnitude more
CPU cycles to fetch and process video data.
It avoids unecessarily prohibiting modification of v4l2 controls or
other resources that cannot be modified while streaming, for an
application that is in a phase where it only wishes to poll() for v4l2
events without starting a stream.
It mainatins backward compatability with existing programs. Those
existing programs *must* handle read() errors after select() or poll()
indicates that the fd won't block on the next read(), and the call to
read() will start the stream.
Regards,
Andy
> Cheers,
> Mauro.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [RFCv3 PATCH 12/18] vb2_poll: don't start DMA, leave that to the first read().
2011-06-29 5:08 ` Andy Walls
@ 2011-06-29 11:37 ` Mauro Carvalho Chehab
0 siblings, 0 replies; 42+ messages in thread
From: Mauro Carvalho Chehab @ 2011-06-29 11:37 UTC (permalink / raw)
To: Andy Walls
Cc: Hans Verkuil, linux-media, Hans Verkuil, Pawel Osciak,
Marek Szyprowski, Kyungmin Park
Em 29-06-2011 02:08, Andy Walls escreveu:
> On Tue, 2011-06-28 at 21:00 -0300, Mauro Carvalho Chehab wrote:
>> Em 28-06-2011 20:14, Andy Walls escreveu:
>>> On Tue, 2011-06-28 at 09:43 -0300, Mauro Carvalho Chehab wrote:
>>>> Em 28-06-2011 09:21, Andy Walls escreveu:
>>>
>>>>> It is also the case that a driver's poll method should never sleep.
>>>>
>>>> True.
>>>
>>>>> One issue is how to start streaming with apps that:
>>>>> - Open /dev/video/ in a nonblocking mode, and
>>>>> - Use the read() method
>>>>>
>>>>> while doing it in a way that is POSIX compliant and doesn't break existing apps.
>>>>
>>>> Well, a first call for poll() may rise a thread that will prepare the buffers, and
>>>> return with 0 while there's no data available.
>>>
>>> Sure, but that doesn't solve the problem of an app only select()-ing or
>>> poll()-ing for exception fd's and not starting any IO.
>>
>> Well, a file descriptor can be used only for one thing: or it is a stream file
>> descriptor, or it is an event descriptor. You can't have both for the same
>> file descriptor. If an application need to check for both, the standard Unix way is:
>>
>> fd_set set;
>>
>> FD_ZERO (&set);
>> FD_SET (fd_stream, &set);
>> FD_SET (fd_event, &set);
>>
>> select (FD_SETSIZE, &set, NULL, NULL, &timeout);
>>
>> In other words, or the events nodes need to be different, or an ioctl is needed
>> in order to tell the Kernel that the associated file descriptor will be used
>> for an event, and that vb2 should not bother with it.
>
> Um, no, that is not correct for Unix fd's and socket descriptors in
> general. I realize that v4l2 events need to be enabled with an ioctl(),
> but do we have a restriction that that can't happen on the same fd as
> the one used for streaming?
>
> Back in the days before threads were commonly available on Unix systems,
> a process would use a single thread calling select() to handle I/O on a
> serial port:
>
> fd_set rfds, wfds;
> int ttyfd;
> ...
> FD_ZERO(&rfds);
> FD_SET(ttyfd, &rfds);
> FD_ZERO(&wfds);
> FD_SET(ttyfd, &wfds);
>
> n = select(ttyfd+1, &rfds, &wfds, NULL, NULL);
>
> Or TCP socket
>
> fd_set rfds, wfds, efds;
> int sockd;
> ...
> FD_ZERO(&rfds);
> FD_SET(sockd, &rfds);
> FD_ZERO(&wfds);
> FD_SET(sockd, &wfds);
> FD_ZERO(&efds);
> FD_SET(sockd, &efds);
>
> n = select(sockd+1, &rfds, &wfds, &efds, NULL);
On both serial and socket devices, if select returns a file descriptor,
the data is there or the device/socket got disconnected.
> Waiting for data to arrive on an fd, while not streaming is an error
> condition for select() should return.
Yes, but poll() starts the streaming, if the mmap mode were not started,
according with the V4L2 spec:
http://linuxtv.org/downloads/v4l-dvb-apis/func-poll.html
Thanks,
Mauro.
^ permalink raw reply [flat|nested] 42+ messages in thread
* [RFCv3 PATCH 13/18] v4l2-ctrls: add control events.
2011-06-07 15:05 ` [RFCv3 PATCH 01/18] v4l2-ctrls: introduce call_op define Hans Verkuil
` (10 preceding siblings ...)
2011-06-07 15:05 ` [RFCv3 PATCH 12/18] vb2_poll: don't start DMA, leave that to the first read() Hans Verkuil
@ 2011-06-07 15:05 ` Hans Verkuil
2011-06-20 13:33 ` Laurent Pinchart
2011-06-07 15:05 ` [RFCv3 PATCH 14/18] v4l2-ctrls: simplify event subscription Hans Verkuil
` (4 subsequent siblings)
16 siblings, 1 reply; 42+ messages in thread
From: Hans Verkuil @ 2011-06-07 15:05 UTC (permalink / raw)
To: linux-media; +Cc: Hans Verkuil
From: Hans Verkuil <hans.verkuil@cisco.com>
Whenever a control changes value or state an event is sent to anyone
that subscribed to it.
This functionality is useful for control panels but also for applications
that need to wait for (usually status) controls to change value.
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
drivers/media/video/v4l2-ctrls.c | 115 ++++++++++++++++++++++++++++++++--
drivers/media/video/v4l2-event.c | 130 +++++++++++++++++++++++++++----------
drivers/media/video/v4l2-fh.c | 4 +-
include/linux/videodev2.h | 29 ++++++++-
include/media/v4l2-ctrls.h | 23 +++++--
include/media/v4l2-event.h | 2 +
6 files changed, 253 insertions(+), 50 deletions(-)
diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
index a38bdf9..99987fe 100644
--- a/drivers/media/video/v4l2-ctrls.c
+++ b/drivers/media/video/v4l2-ctrls.c
@@ -23,6 +23,7 @@
#include <media/v4l2-ioctl.h>
#include <media/v4l2-device.h>
#include <media/v4l2-ctrls.h>
+#include <media/v4l2-event.h>
#include <media/v4l2-dev.h>
#define has_op(master, op) \
@@ -556,6 +557,41 @@ static bool type_is_int(const struct v4l2_ctrl *ctrl)
}
}
+static void fill_event(struct v4l2_event *ev, struct v4l2_ctrl *ctrl, u32 changes)
+{
+ memset(ev->reserved, 0, sizeof(ev->reserved));
+ ev->type = V4L2_EVENT_CTRL;
+ ev->id = ctrl->id;
+ ev->u.ctrl.changes = changes;
+ ev->u.ctrl.type = ctrl->type;
+ ev->u.ctrl.flags = ctrl->flags;
+ if (ctrl->type == V4L2_CTRL_TYPE_STRING)
+ ev->u.ctrl.value64 = 0;
+ else
+ ev->u.ctrl.value64 = ctrl->cur.val64;
+ ev->u.ctrl.minimum = ctrl->minimum;
+ ev->u.ctrl.maximum = ctrl->maximum;
+ if (ctrl->type == V4L2_CTRL_TYPE_MENU)
+ ev->u.ctrl.step = 1;
+ else
+ ev->u.ctrl.step = ctrl->step;
+ ev->u.ctrl.default_value = ctrl->default_value;
+}
+
+static void send_event(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, u32 changes)
+{
+ struct v4l2_event ev;
+ struct v4l2_ctrl_fh *pos;
+
+ if (list_empty(&ctrl->fhs))
+ return;
+ fill_event(&ev, ctrl, changes);
+
+ list_for_each_entry(pos, &ctrl->fhs, node)
+ if (pos->fh != fh)
+ v4l2_event_queue_fh(pos->fh, &ev);
+}
+
/* Helper function: copy the current control value back to the caller */
static int cur_to_user(struct v4l2_ext_control *c,
struct v4l2_ctrl *ctrl)
@@ -660,17 +696,25 @@ static int ctrl_is_volatile(struct v4l2_ext_control *c,
static void new_to_cur(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl,
bool update_inactive)
{
+ bool changed = false;
+
if (ctrl == NULL)
return;
switch (ctrl->type) {
+ case V4L2_CTRL_TYPE_BUTTON:
+ changed = true;
+ break;
case V4L2_CTRL_TYPE_STRING:
/* strings are always 0-terminated */
+ changed = strcmp(ctrl->string, ctrl->cur.string);
strcpy(ctrl->cur.string, ctrl->string);
break;
case V4L2_CTRL_TYPE_INTEGER64:
+ changed = ctrl->val64 != ctrl->cur.val64;
ctrl->cur.val64 = ctrl->val64;
break;
default:
+ changed = ctrl->val != ctrl->cur.val;
ctrl->cur.val = ctrl->val;
break;
}
@@ -679,6 +723,10 @@ static void new_to_cur(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl,
if (!is_cur_manual(ctrl->cluster[0]))
ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE;
}
+ if (changed || update_inactive)
+ send_event(fh, ctrl,
+ (changed ? V4L2_EVENT_CTRL_CH_VALUE : 0) |
+ (update_inactive ? V4L2_EVENT_CTRL_CH_FLAGS : 0));
}
/* Copy the current value to the new value */
@@ -819,6 +867,7 @@ void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl)
{
struct v4l2_ctrl_ref *ref, *next_ref;
struct v4l2_ctrl *ctrl, *next_ctrl;
+ struct v4l2_ctrl_fh *ctrl_fh, *next_ctrl_fh;
if (hdl == NULL || hdl->buckets == NULL)
return;
@@ -832,6 +881,10 @@ void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl)
/* Free all controls owned by the handler */
list_for_each_entry_safe(ctrl, next_ctrl, &hdl->ctrls, node) {
list_del(&ctrl->node);
+ list_for_each_entry_safe(ctrl_fh, next_ctrl_fh, &ctrl->fhs, node) {
+ list_del(&ctrl_fh->node);
+ kfree(ctrl_fh);
+ }
kfree(ctrl);
}
kfree(hdl->buckets);
@@ -1030,6 +1083,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
}
INIT_LIST_HEAD(&ctrl->node);
+ INIT_LIST_HEAD(&ctrl->fhs);
ctrl->handler = hdl;
ctrl->ops = ops;
ctrl->id = id;
@@ -1171,6 +1225,9 @@ int v4l2_ctrl_add_handler(struct v4l2_ctrl_handler *hdl,
/* Skip handler-private controls. */
if (ctrl->is_private)
continue;
+ /* And control classes */
+ if (ctrl->type == V4L2_CTRL_TYPE_CTRL_CLASS)
+ continue;
ret = handler_new_ref(hdl, ctrl);
if (ret)
break;
@@ -1222,15 +1279,21 @@ EXPORT_SYMBOL(v4l2_ctrl_auto_cluster);
/* Activate/deactivate a control. */
void v4l2_ctrl_activate(struct v4l2_ctrl *ctrl, bool active)
{
+ /* invert since the actual flag is called 'inactive' */
+ bool inactive = !active;
+ bool old;
+
if (ctrl == NULL)
return;
- if (!active)
+ if (inactive)
/* set V4L2_CTRL_FLAG_INACTIVE */
- set_bit(4, &ctrl->flags);
+ old = test_and_set_bit(4, &ctrl->flags);
else
/* clear V4L2_CTRL_FLAG_INACTIVE */
- clear_bit(4, &ctrl->flags);
+ old = test_and_clear_bit(4, &ctrl->flags);
+ if (old != inactive)
+ send_event(NULL, ctrl, V4L2_EVENT_CTRL_CH_FLAGS);
}
EXPORT_SYMBOL(v4l2_ctrl_activate);
@@ -1242,15 +1305,21 @@ EXPORT_SYMBOL(v4l2_ctrl_activate);
these controls. */
void v4l2_ctrl_grab(struct v4l2_ctrl *ctrl, bool grabbed)
{
+ bool old;
+
if (ctrl == NULL)
return;
+ v4l2_ctrl_lock(ctrl);
if (grabbed)
/* set V4L2_CTRL_FLAG_GRABBED */
- set_bit(1, &ctrl->flags);
+ old = test_and_set_bit(1, &ctrl->flags);
else
/* clear V4L2_CTRL_FLAG_GRABBED */
- clear_bit(1, &ctrl->flags);
+ old = test_and_clear_bit(1, &ctrl->flags);
+ if (old != grabbed)
+ send_event(NULL, ctrl, V4L2_EVENT_CTRL_CH_FLAGS);
+ v4l2_ctrl_unlock(ctrl);
}
EXPORT_SYMBOL(v4l2_ctrl_grab);
@@ -1956,3 +2025,39 @@ int v4l2_ctrl_s_ctrl(struct v4l2_ctrl *ctrl, s32 val)
return set_ctrl(NULL, ctrl, &val);
}
EXPORT_SYMBOL(v4l2_ctrl_s_ctrl);
+
+void v4l2_ctrl_add_fh(struct v4l2_ctrl_handler *hdl,
+ struct v4l2_ctrl_fh *ctrl_fh,
+ struct v4l2_event_subscription *sub)
+{
+ struct v4l2_ctrl *ctrl = v4l2_ctrl_find(hdl, sub->id);
+
+ v4l2_ctrl_lock(ctrl);
+ list_add_tail(&ctrl_fh->node, &ctrl->fhs);
+ if (ctrl->type != V4L2_CTRL_TYPE_CTRL_CLASS &&
+ (sub->flags & V4L2_EVENT_SUB_FL_SEND_INITIAL)) {
+ struct v4l2_event ev;
+
+ fill_event(&ev, ctrl, V4L2_EVENT_CTRL_CH_VALUE |
+ V4L2_EVENT_CTRL_CH_FLAGS);
+ v4l2_event_queue_fh(ctrl_fh->fh, &ev);
+ }
+ v4l2_ctrl_unlock(ctrl);
+}
+EXPORT_SYMBOL(v4l2_ctrl_add_fh);
+
+void v4l2_ctrl_del_fh(struct v4l2_ctrl *ctrl, struct v4l2_fh *fh)
+{
+ struct v4l2_ctrl_fh *pos;
+
+ v4l2_ctrl_lock(ctrl);
+ list_for_each_entry(pos, &ctrl->fhs, node) {
+ if (pos->fh == fh) {
+ list_del(&pos->node);
+ kfree(pos);
+ break;
+ }
+ }
+ v4l2_ctrl_unlock(ctrl);
+}
+EXPORT_SYMBOL(v4l2_ctrl_del_fh);
diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
index 69fd343..670f2f8 100644
--- a/drivers/media/video/v4l2-event.c
+++ b/drivers/media/video/v4l2-event.c
@@ -25,10 +25,13 @@
#include <media/v4l2-dev.h>
#include <media/v4l2-fh.h>
#include <media/v4l2-event.h>
+#include <media/v4l2-ctrls.h>
#include <linux/sched.h>
#include <linux/slab.h>
+static void v4l2_event_unsubscribe_all(struct v4l2_fh *fh);
+
int v4l2_event_init(struct v4l2_fh *fh)
{
fh->events = kzalloc(sizeof(*fh->events), GFP_KERNEL);
@@ -91,7 +94,7 @@ void v4l2_event_free(struct v4l2_fh *fh)
list_kfree(&events->free, struct v4l2_kevent, list);
list_kfree(&events->available, struct v4l2_kevent, list);
- list_kfree(&events->subscribed, struct v4l2_subscribed_event, list);
+ v4l2_event_unsubscribe_all(fh);
kfree(events);
fh->events = NULL;
@@ -154,9 +157,9 @@ int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event,
}
EXPORT_SYMBOL_GPL(v4l2_event_dequeue);
-/* Caller must hold fh->event->lock! */
+/* Caller must hold fh->vdev->fh_lock! */
static struct v4l2_subscribed_event *v4l2_event_subscribed(
- struct v4l2_fh *fh, u32 type)
+ struct v4l2_fh *fh, u32 type, u32 id)
{
struct v4l2_events *events = fh->events;
struct v4l2_subscribed_event *sev;
@@ -164,13 +167,46 @@ static struct v4l2_subscribed_event *v4l2_event_subscribed(
assert_spin_locked(&fh->vdev->fh_lock);
list_for_each_entry(sev, &events->subscribed, list) {
- if (sev->type == type)
+ if (sev->type == type && sev->id == id)
return sev;
}
return NULL;
}
+static void __v4l2_event_queue_fh(struct v4l2_fh *fh, const struct v4l2_event *ev,
+ const struct timespec *ts)
+{
+ struct v4l2_events *events = fh->events;
+ struct v4l2_subscribed_event *sev;
+ struct v4l2_kevent *kev;
+
+ /* Are we subscribed? */
+ sev = v4l2_event_subscribed(fh, ev->type, ev->id);
+ if (sev == NULL)
+ return;
+
+ /* Increase event sequence number on fh. */
+ events->sequence++;
+
+ /* Do we have any free events? */
+ if (list_empty(&events->free))
+ return;
+
+ /* Take one and fill it. */
+ kev = list_first_entry(&events->free, struct v4l2_kevent, list);
+ kev->event.type = ev->type;
+ kev->event.u = ev->u;
+ kev->event.id = ev->id;
+ kev->event.timestamp = *ts;
+ kev->event.sequence = events->sequence;
+ list_move_tail(&kev->list, &events->available);
+
+ events->navailable++;
+
+ wake_up_all(&events->wait);
+}
+
void v4l2_event_queue(struct video_device *vdev, const struct v4l2_event *ev)
{
struct v4l2_fh *fh;
@@ -182,37 +218,26 @@ void v4l2_event_queue(struct video_device *vdev, const struct v4l2_event *ev)
spin_lock_irqsave(&vdev->fh_lock, flags);
list_for_each_entry(fh, &vdev->fh_list, list) {
- struct v4l2_events *events = fh->events;
- struct v4l2_kevent *kev;
-
- /* Are we subscribed? */
- if (!v4l2_event_subscribed(fh, ev->type))
- continue;
-
- /* Increase event sequence number on fh. */
- events->sequence++;
-
- /* Do we have any free events? */
- if (list_empty(&events->free))
- continue;
-
- /* Take one and fill it. */
- kev = list_first_entry(&events->free, struct v4l2_kevent, list);
- kev->event.type = ev->type;
- kev->event.u = ev->u;
- kev->event.timestamp = timestamp;
- kev->event.sequence = events->sequence;
- list_move_tail(&kev->list, &events->available);
-
- events->navailable++;
-
- wake_up_all(&events->wait);
+ __v4l2_event_queue_fh(fh, ev, ×tamp);
}
spin_unlock_irqrestore(&vdev->fh_lock, flags);
}
EXPORT_SYMBOL_GPL(v4l2_event_queue);
+void v4l2_event_queue_fh(struct v4l2_fh *fh, const struct v4l2_event *ev)
+{
+ unsigned long flags;
+ struct timespec timestamp;
+
+ ktime_get_ts(×tamp);
+
+ spin_lock_irqsave(&fh->vdev->fh_lock, flags);
+ __v4l2_event_queue_fh(fh, ev, ×tamp);
+ spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
+}
+EXPORT_SYMBOL_GPL(v4l2_event_queue_fh);
+
int v4l2_event_pending(struct v4l2_fh *fh)
{
return fh->events->navailable;
@@ -223,7 +248,9 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
struct v4l2_event_subscription *sub)
{
struct v4l2_events *events = fh->events;
- struct v4l2_subscribed_event *sev;
+ struct v4l2_subscribed_event *sev, *found_ev;
+ struct v4l2_ctrl *ctrl = NULL;
+ struct v4l2_ctrl_fh *ctrl_fh = NULL;
unsigned long flags;
if (fh->events == NULL) {
@@ -231,15 +258,31 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
return -ENOMEM;
}
+ if (sub->type == V4L2_EVENT_CTRL) {
+ ctrl = v4l2_ctrl_find(fh->ctrl_handler, sub->id);
+ if (ctrl == NULL)
+ return -EINVAL;
+ }
+
sev = kmalloc(sizeof(*sev), GFP_KERNEL);
if (!sev)
return -ENOMEM;
+ if (ctrl) {
+ ctrl_fh = kzalloc(sizeof(*ctrl_fh), GFP_KERNEL);
+ if (!ctrl_fh) {
+ kfree(sev);
+ return -ENOMEM;
+ }
+ ctrl_fh->fh = fh;
+ }
spin_lock_irqsave(&fh->vdev->fh_lock, flags);
- if (v4l2_event_subscribed(fh, sub->type) == NULL) {
+ found_ev = v4l2_event_subscribed(fh, sub->type, sub->id);
+ if (!found_ev) {
INIT_LIST_HEAD(&sev->list);
sev->type = sub->type;
+ sev->id = sub->id;
list_add(&sev->list, &events->subscribed);
sev = NULL;
@@ -247,6 +290,14 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
+ /* v4l2_ctrl_add_fh uses a mutex, so do this outside the spin lock */
+ if (ctrl) {
+ if (found_ev)
+ kfree(ctrl_fh);
+ else
+ v4l2_ctrl_add_fh(fh->ctrl_handler, ctrl_fh, sub);
+ }
+
kfree(sev);
return 0;
@@ -256,6 +307,7 @@ EXPORT_SYMBOL_GPL(v4l2_event_subscribe);
static void v4l2_event_unsubscribe_all(struct v4l2_fh *fh)
{
struct v4l2_events *events = fh->events;
+ struct v4l2_event_subscription sub;
struct v4l2_subscribed_event *sev;
unsigned long flags;
@@ -265,11 +317,13 @@ static void v4l2_event_unsubscribe_all(struct v4l2_fh *fh)
spin_lock_irqsave(&fh->vdev->fh_lock, flags);
if (!list_empty(&events->subscribed)) {
sev = list_first_entry(&events->subscribed,
- struct v4l2_subscribed_event, list);
- list_del(&sev->list);
+ struct v4l2_subscribed_event, list);
+ sub.type = sev->type;
+ sub.id = sev->id;
}
spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
- kfree(sev);
+ if (sev)
+ v4l2_event_unsubscribe(fh, &sub);
} while (sev);
}
@@ -286,11 +340,17 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
spin_lock_irqsave(&fh->vdev->fh_lock, flags);
- sev = v4l2_event_subscribed(fh, sub->type);
+ sev = v4l2_event_subscribed(fh, sub->type, sub->id);
if (sev != NULL)
list_del(&sev->list);
spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
+ if (sev->type == V4L2_EVENT_CTRL) {
+ struct v4l2_ctrl *ctrl = v4l2_ctrl_find(fh->ctrl_handler, sev->id);
+
+ if (ctrl)
+ v4l2_ctrl_del_fh(ctrl, fh);
+ }
kfree(sev);
diff --git a/drivers/media/video/v4l2-fh.c b/drivers/media/video/v4l2-fh.c
index 8635011..c6aef84 100644
--- a/drivers/media/video/v4l2-fh.c
+++ b/drivers/media/video/v4l2-fh.c
@@ -93,10 +93,8 @@ void v4l2_fh_exit(struct v4l2_fh *fh)
{
if (fh->vdev == NULL)
return;
-
- fh->vdev = NULL;
-
v4l2_event_free(fh);
+ fh->vdev = NULL;
}
EXPORT_SYMBOL_GPL(v4l2_fh_exit);
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index 8a4c309..baafe2f 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -1791,6 +1791,7 @@ struct v4l2_streamparm {
#define V4L2_EVENT_ALL 0
#define V4L2_EVENT_VSYNC 1
#define V4L2_EVENT_EOS 2
+#define V4L2_EVENT_CTRL 3
#define V4L2_EVENT_PRIVATE_START 0x08000000
/* Payload for V4L2_EVENT_VSYNC */
@@ -1799,21 +1800,45 @@ struct v4l2_event_vsync {
__u8 field;
} __attribute__ ((packed));
+/* Payload for V4L2_EVENT_CTRL */
+#define V4L2_EVENT_CTRL_CH_VALUE (1 << 0)
+#define V4L2_EVENT_CTRL_CH_FLAGS (1 << 1)
+
+struct v4l2_event_ctrl {
+ __u32 changes;
+ __u32 type;
+ union {
+ __s32 value;
+ __s64 value64;
+ };
+ __u32 flags;
+ __s32 minimum;
+ __s32 maximum;
+ __s32 step;
+ __s32 default_value;
+};
+
struct v4l2_event {
__u32 type;
union {
struct v4l2_event_vsync vsync;
+ struct v4l2_event_ctrl ctrl;
__u8 data[64];
} u;
__u32 pending;
__u32 sequence;
struct timespec timestamp;
- __u32 reserved[9];
+ __u32 id;
+ __u32 reserved[8];
};
+#define V4L2_EVENT_SUB_FL_SEND_INITIAL (1 << 0)
+
struct v4l2_event_subscription {
__u32 type;
- __u32 reserved[7];
+ __u32 id;
+ __u32 flags;
+ __u32 reserved[5];
};
/*
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index e720f11..c45bf40 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -28,9 +28,10 @@
/* forward references */
struct v4l2_ctrl_handler;
struct v4l2_ctrl;
-struct v4l2_fh;
struct video_device;
struct v4l2_subdev;
+struct v4l2_event_subscription;
+struct v4l2_fh;
/** struct v4l2_ctrl_ops - The control operations that the driver has to provide.
* @g_volatile_ctrl: Get a new value for this control. Generally only relevant
@@ -107,6 +108,7 @@ struct v4l2_ctrl_ops {
struct v4l2_ctrl {
/* Administrative fields */
struct list_head node;
+ struct list_head fhs;
struct v4l2_ctrl_handler *handler;
struct v4l2_ctrl **cluster;
unsigned ncontrols;
@@ -180,6 +182,11 @@ struct v4l2_ctrl_handler {
int error;
};
+struct v4l2_ctrl_fh {
+ struct list_head node;
+ struct v4l2_fh *fh;
+};
+
/** struct v4l2_ctrl_config - Control configuration structure.
* @ops: The control ops.
* @id: The control ID.
@@ -425,9 +432,9 @@ struct v4l2_ctrl *v4l2_ctrl_find(struct v4l2_ctrl_handler *hdl, u32 id);
* This sets or clears the V4L2_CTRL_FLAG_INACTIVE flag atomically.
* Does nothing if @ctrl == NULL.
* This will usually be called from within the s_ctrl op.
+ * The V4L2_EVENT_CTRL event will be generated afterwards.
*
- * This function can be called regardless of whether the control handler
- * is locked or not.
+ * This function assumes that the control handler is locked.
*/
void v4l2_ctrl_activate(struct v4l2_ctrl *ctrl, bool active);
@@ -437,11 +444,12 @@ void v4l2_ctrl_activate(struct v4l2_ctrl *ctrl, bool active);
*
* This sets or clears the V4L2_CTRL_FLAG_GRABBED flag atomically.
* Does nothing if @ctrl == NULL.
+ * The V4L2_EVENT_CTRL event will be generated afterwards.
* This will usually be called when starting or stopping streaming in the
* driver.
*
- * This function can be called regardless of whether the control handler
- * is locked or not.
+ * This function assumes that the control handler is not locked and will
+ * take the lock itself.
*/
void v4l2_ctrl_grab(struct v4l2_ctrl *ctrl, bool grabbed);
@@ -486,6 +494,11 @@ s32 v4l2_ctrl_g_ctrl(struct v4l2_ctrl *ctrl);
*/
int v4l2_ctrl_s_ctrl(struct v4l2_ctrl *ctrl, s32 val);
+void v4l2_ctrl_add_fh(struct v4l2_ctrl_handler *hdl,
+ struct v4l2_ctrl_fh *ctrl_fh,
+ struct v4l2_event_subscription *sub);
+void v4l2_ctrl_del_fh(struct v4l2_ctrl *ctrl, struct v4l2_fh *fh);
+
/* Helpers for ioctl_ops. If hdl == NULL then they will all return -EINVAL. */
int v4l2_queryctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_queryctrl *qc);
int v4l2_querymenu(struct v4l2_ctrl_handler *hdl, struct v4l2_querymenu *qm);
diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
index 3b86177..45e9c1e 100644
--- a/include/media/v4l2-event.h
+++ b/include/media/v4l2-event.h
@@ -40,6 +40,7 @@ struct v4l2_kevent {
struct v4l2_subscribed_event {
struct list_head list;
u32 type;
+ u32 id;
};
struct v4l2_events {
@@ -58,6 +59,7 @@ void v4l2_event_free(struct v4l2_fh *fh);
int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event,
int nonblocking);
void v4l2_event_queue(struct video_device *vdev, const struct v4l2_event *ev);
+void v4l2_event_queue_fh(struct v4l2_fh *fh, const struct v4l2_event *ev);
int v4l2_event_pending(struct v4l2_fh *fh);
int v4l2_event_subscribe(struct v4l2_fh *fh,
struct v4l2_event_subscription *sub);
--
1.7.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [RFCv3 PATCH 13/18] v4l2-ctrls: add control events.
2011-06-07 15:05 ` [RFCv3 PATCH 13/18] v4l2-ctrls: add control events Hans Verkuil
@ 2011-06-20 13:33 ` Laurent Pinchart
0 siblings, 0 replies; 42+ messages in thread
From: Laurent Pinchart @ 2011-06-20 13:33 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, Hans Verkuil
Hi Hans,
Thanks for the patch, and sorry for the late review.
On Tuesday 07 June 2011 17:05:18 Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
>
> Whenever a control changes value or state an event is sent to anyone
> that subscribed to it.
>
> This functionality is useful for control panels but also for applications
> that need to wait for (usually status) controls to change value.
[snip]
> +static void send_event(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, u32
> changes) +{
> + struct v4l2_event ev;
> + struct v4l2_ctrl_fh *pos;
> +
> + if (list_empty(&ctrl->fhs))
> + return;
There's one extra tab here.
> + fill_event(&ev, ctrl, changes);
> +
> + list_for_each_entry(pos, &ctrl->fhs, node)
> + if (pos->fh != fh)
> + v4l2_event_queue_fh(pos->fh, &ev);
> +}
[snip]
> @@ -1222,15 +1279,21 @@ EXPORT_SYMBOL(v4l2_ctrl_auto_cluster);
> /* Activate/deactivate a control. */
> void v4l2_ctrl_activate(struct v4l2_ctrl *ctrl, bool active)
> {
> + /* invert since the actual flag is called 'inactive' */
> + bool inactive = !active;
> + bool old;
> +
> if (ctrl == NULL)
> return;
>
> - if (!active)
> + if (inactive)
> /* set V4L2_CTRL_FLAG_INACTIVE */
> - set_bit(4, &ctrl->flags);
> + old = test_and_set_bit(4, &ctrl->flags);
I've never been found of hardcoded constants. What about
ffs(V4L2_CTRL_FLAG_INACTIVE) - 1 instead ? gcc will optimize that to a
constant.
> else
> /* clear V4L2_CTRL_FLAG_INACTIVE */
> - clear_bit(4, &ctrl->flags);
> + old = test_and_clear_bit(4, &ctrl->flags);
> + if (old != inactive)
> + send_event(NULL, ctrl, V4L2_EVENT_CTRL_CH_FLAGS);
> }
> EXPORT_SYMBOL(v4l2_ctrl_activate);
[snip]
> @@ -182,37 +218,26 @@ void v4l2_event_queue(struct video_device *vdev,
> const struct v4l2_event *ev) spin_lock_irqsave(&vdev->fh_lock, flags);
>
> list_for_each_entry(fh, &vdev->fh_list, list) {
> - struct v4l2_events *events = fh->events;
> - struct v4l2_kevent *kev;
> -
> - /* Are we subscribed? */
> - if (!v4l2_event_subscribed(fh, ev->type))
> - continue;
> -
> - /* Increase event sequence number on fh. */
> - events->sequence++;
> -
> - /* Do we have any free events? */
> - if (list_empty(&events->free))
> - continue;
> -
> - /* Take one and fill it. */
> - kev = list_first_entry(&events->free, struct v4l2_kevent, list);
> - kev->event.type = ev->type;
> - kev->event.u = ev->u;
> - kev->event.timestamp = timestamp;
> - kev->event.sequence = events->sequence;
> - list_move_tail(&kev->list, &events->available);
> -
> - events->navailable++;
> -
> - wake_up_all(&events->wait);
> + __v4l2_event_queue_fh(fh, ev, ×tamp);
> }
You can remove the braces.
>
> spin_unlock_irqrestore(&vdev->fh_lock, flags);
> }
> EXPORT_SYMBOL_GPL(v4l2_event_queue);
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 42+ messages in thread
* [RFCv3 PATCH 14/18] v4l2-ctrls: simplify event subscription.
2011-06-07 15:05 ` [RFCv3 PATCH 01/18] v4l2-ctrls: introduce call_op define Hans Verkuil
` (11 preceding siblings ...)
2011-06-07 15:05 ` [RFCv3 PATCH 13/18] v4l2-ctrls: add control events Hans Verkuil
@ 2011-06-07 15:05 ` Hans Verkuil
2011-06-07 15:05 ` [RFCv3 PATCH 15/18] V4L2 spec: document control events Hans Verkuil
` (3 subsequent siblings)
16 siblings, 0 replies; 42+ messages in thread
From: Hans Verkuil @ 2011-06-07 15:05 UTC (permalink / raw)
To: linux-media; +Cc: Hans Verkuil
From: Hans Verkuil <hans.verkuil@cisco.com>
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
drivers/media/video/v4l2-ctrls.c | 27 +++++++++++++++++++++++++++
include/media/v4l2-ctrls.h | 28 ++++++++++++++++++++++++++++
2 files changed, 55 insertions(+), 0 deletions(-)
diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
index 99987fe..bf649cf 100644
--- a/drivers/media/video/v4l2-ctrls.c
+++ b/drivers/media/video/v4l2-ctrls.c
@@ -862,6 +862,13 @@ int v4l2_ctrl_handler_init(struct v4l2_ctrl_handler *hdl,
}
EXPORT_SYMBOL(v4l2_ctrl_handler_init);
+/* Count the number of controls */
+unsigned v4l2_ctrl_handler_cnt(const struct v4l2_ctrl_handler *hdl)
+{
+ return hdl ? hdl->nr_of_refs : 0;
+}
+EXPORT_SYMBOL(v4l2_ctrl_handler_cnt);
+
/* Free all controls and control refs */
void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl)
{
@@ -1013,6 +1020,7 @@ static int handler_new_ref(struct v4l2_ctrl_handler *hdl,
insertion is an O(1) operation. */
if (list_empty(&hdl->ctrl_refs) || id > node2id(hdl->ctrl_refs.prev)) {
list_add_tail(&new_ref->node, &hdl->ctrl_refs);
+ hdl->nr_of_refs++;
goto insert_in_hash;
}
@@ -2061,3 +2069,22 @@ void v4l2_ctrl_del_fh(struct v4l2_ctrl *ctrl, struct v4l2_fh *fh)
v4l2_ctrl_unlock(ctrl);
}
EXPORT_SYMBOL(v4l2_ctrl_del_fh);
+
+int v4l2_ctrl_subscribe_fh(struct v4l2_fh *fh,
+ struct v4l2_event_subscription *sub, unsigned n)
+{
+ struct v4l2_ctrl_handler *hdl = fh->ctrl_handler;
+ int ret = 0;
+
+ if (!fh->events)
+ ret = v4l2_event_init(fh);
+ if (!ret) {
+ if (v4l2_ctrl_handler_cnt(hdl) * 2 > n)
+ n = v4l2_ctrl_handler_cnt(hdl) * 2;
+ ret = v4l2_event_alloc(fh, n);
+ }
+ if (!ret)
+ ret = v4l2_event_subscribe(fh, sub);
+ return ret;
+}
+EXPORT_SYMBOL(v4l2_ctrl_subscribe_fh);
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index c45bf40..72dafcf 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -169,6 +169,7 @@ struct v4l2_ctrl_ref {
* control is needed multiple times, so this is a simple
* optimization.
* @buckets: Buckets for the hashing. Allows for quick control lookup.
+ * @nr_of_refs: Total number of control references in the list.
* @nr_of_buckets: Total number of buckets in the array.
* @error: The error code of the first failed control addition.
*/
@@ -178,6 +179,7 @@ struct v4l2_ctrl_handler {
struct list_head ctrl_refs;
struct v4l2_ctrl_ref *cached;
struct v4l2_ctrl_ref **buckets;
+ u16 nr_of_refs;
u16 nr_of_buckets;
int error;
};
@@ -263,6 +265,14 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
int v4l2_ctrl_handler_init(struct v4l2_ctrl_handler *hdl,
unsigned nr_of_controls_hint);
+/** v4l2_ctrl_handler_cnt() - Count the number of controls in the handler.
+ * @hdl: The control handler.
+ *
+ * Returns the number of controls referenced by this handler.
+ * Returns 0 if @hdl == NULL.
+ */
+unsigned v4l2_ctrl_handler_cnt(const struct v4l2_ctrl_handler *hdl);
+
/** v4l2_ctrl_handler_free() - Free all controls owned by the handler and free
* the control list.
* @hdl: The control handler.
@@ -494,11 +504,29 @@ s32 v4l2_ctrl_g_ctrl(struct v4l2_ctrl *ctrl);
*/
int v4l2_ctrl_s_ctrl(struct v4l2_ctrl *ctrl, s32 val);
+/* Internal helper functions that deal with control events. */
void v4l2_ctrl_add_fh(struct v4l2_ctrl_handler *hdl,
struct v4l2_ctrl_fh *ctrl_fh,
struct v4l2_event_subscription *sub);
void v4l2_ctrl_del_fh(struct v4l2_ctrl *ctrl, struct v4l2_fh *fh);
+/** v4l2_ctrl_subscribe_fh() - Helper function that subscribes a control event.
+ * @fh: The file handler that subscribed the control event.
+ * @sub: The event to subscribe (type must be V4L2_EVENT_CTRL).
+ * @n: How many events should be allocated? (Passed to v4l2_event_alloc).
+ * Recommended to set to twice the number of controls plus whatever
+ * is needed for other events. This function will set n to
+ * max(n, 2 * v4l2_ctrl_handler_cnt(fh->ctrl_handler)).
+ *
+ * A helper function that initializes the fh for events, allocates the
+ * list of events and subscribes the control event.
+ *
+ * Typically called in the handler of VIDIOC_SUBSCRIBE_EVENT in the
+ * V4L2_EVENT_CTRL case.
+ */
+int v4l2_ctrl_subscribe_fh(struct v4l2_fh *fh,
+ struct v4l2_event_subscription *sub, unsigned n);
+
/* Helpers for ioctl_ops. If hdl == NULL then they will all return -EINVAL. */
int v4l2_queryctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_queryctrl *qc);
int v4l2_querymenu(struct v4l2_ctrl_handler *hdl, struct v4l2_querymenu *qm);
--
1.7.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [RFCv3 PATCH 15/18] V4L2 spec: document control events.
2011-06-07 15:05 ` [RFCv3 PATCH 01/18] v4l2-ctrls: introduce call_op define Hans Verkuil
` (12 preceding siblings ...)
2011-06-07 15:05 ` [RFCv3 PATCH 14/18] v4l2-ctrls: simplify event subscription Hans Verkuil
@ 2011-06-07 15:05 ` Hans Verkuil
2011-06-07 15:05 ` [RFCv3 PATCH 16/18] vivi: support " Hans Verkuil
` (2 subsequent siblings)
16 siblings, 0 replies; 42+ messages in thread
From: Hans Verkuil @ 2011-06-07 15:05 UTC (permalink / raw)
To: linux-media; +Cc: Hans Verkuil
From: Hans Verkuil <hans.verkuil@cisco.com>
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
Documentation/DocBook/media/v4l/vidioc-dqevent.xml | 17 +++-
.../DocBook/media/v4l/vidioc-subscribe-event.xml | 142 +++++++++++++++++++-
2 files changed, 157 insertions(+), 2 deletions(-)
diff --git a/Documentation/DocBook/media/v4l/vidioc-dqevent.xml b/Documentation/DocBook/media/v4l/vidioc-dqevent.xml
index 4e0a7cc..b8c4f76 100644
--- a/Documentation/DocBook/media/v4l/vidioc-dqevent.xml
+++ b/Documentation/DocBook/media/v4l/vidioc-dqevent.xml
@@ -81,6 +81,13 @@
</row>
<row>
<entry></entry>
+ <entry>&v4l2-event-ctrl;</entry>
+ <entry><structfield>ctrl</structfield></entry>
+ <entry>Event data for event V4L2_EVENT_CTRL.
+ </entry>
+ </row>
+ <row>
+ <entry></entry>
<entry>__u8</entry>
<entry><structfield>data</structfield>[64]</entry>
<entry>Event data. Defined by the event type. The union
@@ -110,8 +117,16 @@
<entry>Event timestamp.</entry>
</row>
<row>
+ <entry>u32</entry>
+ <entry><structfield>id</structfield></entry>
+ <entry></entry>
+ <entry>The ID associated with the event source. If the event does not
+ have an associated ID (this depends on the event type), then this
+ is 0.</entry>
+ </row>
+ <row>
<entry>__u32</entry>
- <entry><structfield>reserved</structfield>[9]</entry>
+ <entry><structfield>reserved</structfield>[8]</entry>
<entry></entry>
<entry>Reserved for future extensions. Drivers must set
the array to zero.</entry>
diff --git a/Documentation/DocBook/media/v4l/vidioc-subscribe-event.xml b/Documentation/DocBook/media/v4l/vidioc-subscribe-event.xml
index 8b50179..975f603 100644
--- a/Documentation/DocBook/media/v4l/vidioc-subscribe-event.xml
+++ b/Documentation/DocBook/media/v4l/vidioc-subscribe-event.xml
@@ -64,7 +64,19 @@
</row>
<row>
<entry>__u32</entry>
- <entry><structfield>reserved</structfield>[7]</entry>
+ <entry><structfield>id</structfield></entry>
+ <entry>ID of the event source. If there is no ID associated with
+ the event source, then set this to 0. Whether or not an event
+ needs an ID depends on the event type.</entry>
+ </row>
+ <row>
+ <entry>__u32</entry>
+ <entry><structfield>flags</structfield></entry>
+ <entry>Event flags, see <xref linkend="event-flags" />.</entry>
+ </row>
+ <row>
+ <entry>__u32</entry>
+ <entry><structfield>reserved</structfield>[5]</entry>
<entry>Reserved for future extensions. Drivers and applications
must set the array to zero.</entry>
</row>
@@ -100,6 +112,23 @@
</entry>
</row>
<row>
+ <entry><constant>V4L2_EVENT_CTRL</constant></entry>
+ <entry>3</entry>
+ <entry>This event requires that the <structfield>id</structfield>
+ matches the control ID from which you want to receive events.
+ This event is triggered if the control's value changes, if a
+ button control is pressed or if the control's flags change.
+ This event has &v4l2-event-ctrl; associated with it. This struct
+ contains much of the same information as &v4l2-queryctrl; and
+ &v4l2-control;.
+
+ If the event is generated due to a call to &VIDIOC-S-CTRL; or
+ &VIDIOC-S-EXT-CTRLS;, then the event will not be sent to
+ the file handle that called the ioctl function. This prevents
+ nasty feedback loops.
+ </entry>
+ </row>
+ <row>
<entry><constant>V4L2_EVENT_PRIVATE_START</constant></entry>
<entry>0x08000000</entry>
<entry>Base event number for driver-private events.</entry>
@@ -108,6 +137,23 @@
</tgroup>
</table>
+ <table pgwide="1" frame="none" id="event-flags">
+ <title>Event Flags</title>
+ <tgroup cols="3">
+ &cs-def;
+ <tbody valign="top">
+ <row>
+ <entry><constant>V4L2_EVENT_SUB_FL_SEND_INITIAL</constant></entry>
+ <entry>0x0001</entry>
+ <entry>When this event is subscribed an initial event will be sent
+ containing the current status. This only makes sense for events
+ that are triggered by a status change. Other events will ignore
+ this flag.</entry>
+ </row>
+ </tbody>
+ </tgroup>
+ </table>
+
<table frame="none" pgwide="1" id="v4l2-event-vsync">
<title>struct <structname>v4l2_event_vsync</structname></title>
<tgroup cols="3">
@@ -122,6 +168,100 @@
</tgroup>
</table>
+ <table frame="none" pgwide="1" id="v4l2-event-ctrl">
+ <title>struct <structname>v4l2_event_ctrl</structname></title>
+ <tgroup cols="4">
+ &cs-str;
+ <tbody valign="top">
+ <row>
+ <entry>__u32</entry>
+ <entry><structfield>changes</structfield></entry>
+ <entry></entry>
+ <entry>A bitmask that tells what has changed. See <xref linkend="changes-flags" />.</entry>
+ </row>
+ <row>
+ <entry>__u32</entry>
+ <entry><structfield>type</structfield></entry>
+ <entry></entry>
+ <entry>The type of the control. See &v4l2-ctrl-type;.</entry>
+ </row>
+ <row>
+ <entry>union (anonymous)</entry>
+ <entry></entry>
+ <entry></entry>
+ <entry></entry>
+ </row>
+ <row>
+ <entry></entry>
+ <entry>__s32</entry>
+ <entry><structfield>value</structfield></entry>
+ <entry>The 32-bit value of the control for 32-bit control types.
+ This is 0 for string controls since the value of a string
+ cannot be passed using &VIDIOC-DQEVENT;.</entry>
+ </row>
+ <row>
+ <entry></entry>
+ <entry>__s64</entry>
+ <entry><structfield>value64</structfield></entry>
+ <entry>The 64-bit value of the control for 64-bit control types.</entry>
+ </row>
+ <row>
+ <entry>__u32</entry>
+ <entry><structfield>flags</structfield></entry>
+ <entry></entry>
+ <entry>The control flags. See <xref linkend="control-flags" />.</entry>
+ </row>
+ <row>
+ <entry>__s32</entry>
+ <entry><structfield>minimum</structfield></entry>
+ <entry></entry>
+ <entry>The minimum value of the control. See &v4l2-queryctrl;.</entry>
+ </row>
+ <row>
+ <entry>__s32</entry>
+ <entry><structfield>maximum</structfield></entry>
+ <entry></entry>
+ <entry>The maximum value of the control. See &v4l2-queryctrl;.</entry>
+ </row>
+ <row>
+ <entry>__s32</entry>
+ <entry><structfield>step</structfield></entry>
+ <entry></entry>
+ <entry>The step value of the control. See &v4l2-queryctrl;.</entry>
+ </row>
+ <row>
+ <entry>__s32</entry>
+ <entry><structfield>default_value</structfield></entry>
+ <entry></entry>
+ <entry>The default value value of the control. See &v4l2-queryctrl;.</entry>
+ </row>
+ </tbody>
+ </tgroup>
+ </table>
+
+ <table pgwide="1" frame="none" id="changes-flags">
+ <title>Changes</title>
+ <tgroup cols="3">
+ &cs-def;
+ <tbody valign="top">
+ <row>
+ <entry><constant>V4L2_EVENT_CTRL_CH_VALUE</constant></entry>
+ <entry>0x0001</entry>
+ <entry>This control event was triggered because the value of the control
+ changed. Special case: if a button control is pressed, then this
+ event is sent as well, even though there is not explicit value
+ associated with a button control.</entry>
+ </row>
+ <row>
+ <entry><constant>V4L2_EVENT_CTRL_CH_FLAGS</constant></entry>
+ <entry>0x0002</entry>
+ <entry>This control event was triggered because the control flags
+ changed.</entry>
+ </row>
+ </tbody>
+ </tgroup>
+ </table>
+
</refsect1>
</refentry>
<!--
--
1.7.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [RFCv3 PATCH 16/18] vivi: support control events.
2011-06-07 15:05 ` [RFCv3 PATCH 01/18] v4l2-ctrls: introduce call_op define Hans Verkuil
` (13 preceding siblings ...)
2011-06-07 15:05 ` [RFCv3 PATCH 15/18] V4L2 spec: document control events Hans Verkuil
@ 2011-06-07 15:05 ` Hans Verkuil
2011-06-07 15:05 ` [RFCv3 PATCH 17/18] ivtv: add control event support Hans Verkuil
2011-06-07 15:05 ` [RFCv3 PATCH 18/18] v4l2-compat-ioctl32: add VIDIOC_DQEVENT support Hans Verkuil
16 siblings, 0 replies; 42+ messages in thread
From: Hans Verkuil @ 2011-06-07 15:05 UTC (permalink / raw)
To: linux-media; +Cc: Hans Verkuil
From: Hans Verkuil <hans.verkuil@cisco.com>
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
drivers/media/video/vivi.c | 28 ++++++++++++++++++++++++++--
1 files changed, 26 insertions(+), 2 deletions(-)
diff --git a/drivers/media/video/vivi.c b/drivers/media/video/vivi.c
index 6e62ddf..f4f599a 100644
--- a/drivers/media/video/vivi.c
+++ b/drivers/media/video/vivi.c
@@ -32,6 +32,7 @@
#include <media/v4l2-ioctl.h>
#include <media/v4l2-ctrls.h>
#include <media/v4l2-fh.h>
+#include <media/v4l2-event.h>
#include <media/v4l2-common.h>
#define VIVI_MODULE_NAME "vivi"
@@ -983,12 +984,26 @@ static int vidioc_s_input(struct file *file, void *priv, unsigned int i)
if (i >= NUM_INPUTS)
return -EINVAL;
+ if (i == dev->input)
+ return 0;
+
dev->input = i;
precalculate_bars(dev);
precalculate_line(dev);
return 0;
}
+static int vidioc_subscribe_event(struct v4l2_fh *fh,
+ struct v4l2_event_subscription *sub)
+{
+ switch (sub->type) {
+ case V4L2_EVENT_CTRL:
+ return v4l2_ctrl_subscribe_fh(fh, sub, 0);
+ default:
+ return -EINVAL;
+ }
+}
+
/* --- controls ---------------------------------------------- */
static int vivi_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
@@ -1027,10 +1042,17 @@ static unsigned int
vivi_poll(struct file *file, struct poll_table_struct *wait)
{
struct vivi_dev *dev = video_drvdata(file);
+ struct v4l2_fh *fh = file->private_data;
struct vb2_queue *q = &dev->vb_vidq;
+ unsigned int res;
dprintk(dev, 1, "%s\n", __func__);
- return vb2_poll(q, file, wait);
+ res = vb2_poll(q, file, wait);
+ if (v4l2_event_pending(fh))
+ res |= POLLPRI;
+ else
+ poll_wait(file, &fh->events->wait, wait);
+ return res;
}
static int vivi_close(struct file *file)
@@ -1137,7 +1159,7 @@ static const struct v4l2_ctrl_config vivi_ctrl_string = {
static const struct v4l2_file_operations vivi_fops = {
.owner = THIS_MODULE,
- .open = v4l2_fh_open,
+ .open = v4l2_fh_open,
.release = vivi_close,
.read = vivi_read,
.poll = vivi_poll,
@@ -1161,6 +1183,8 @@ static const struct v4l2_ioctl_ops vivi_ioctl_ops = {
.vidioc_s_input = vidioc_s_input,
.vidioc_streamon = vidioc_streamon,
.vidioc_streamoff = vidioc_streamoff,
+ .vidioc_subscribe_event = vidioc_subscribe_event,
+ .vidioc_unsubscribe_event = v4l2_event_unsubscribe,
};
static struct video_device vivi_template = {
--
1.7.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [RFCv3 PATCH 17/18] ivtv: add control event support.
2011-06-07 15:05 ` [RFCv3 PATCH 01/18] v4l2-ctrls: introduce call_op define Hans Verkuil
` (14 preceding siblings ...)
2011-06-07 15:05 ` [RFCv3 PATCH 16/18] vivi: support " Hans Verkuil
@ 2011-06-07 15:05 ` Hans Verkuil
2011-06-07 15:05 ` [RFCv3 PATCH 18/18] v4l2-compat-ioctl32: add VIDIOC_DQEVENT support Hans Verkuil
16 siblings, 0 replies; 42+ messages in thread
From: Hans Verkuil @ 2011-06-07 15:05 UTC (permalink / raw)
To: linux-media; +Cc: Hans Verkuil
From: Hans Verkuil <hans.verkuil@cisco.com>
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
drivers/media/video/ivtv/ivtv-fileops.c | 34 +++++++++++++-----------------
drivers/media/video/ivtv/ivtv-ioctl.c | 2 +
2 files changed, 17 insertions(+), 19 deletions(-)
diff --git a/drivers/media/video/ivtv/ivtv-fileops.c b/drivers/media/video/ivtv/ivtv-fileops.c
index a7f54b0..75c0354 100644
--- a/drivers/media/video/ivtv/ivtv-fileops.c
+++ b/drivers/media/video/ivtv/ivtv-fileops.c
@@ -750,31 +750,27 @@ unsigned int ivtv_v4l2_enc_poll(struct file *filp, poll_table * wait)
struct ivtv *itv = id->itv;
struct ivtv_stream *s = &itv->streams[id->type];
int eof = test_bit(IVTV_F_S_STREAMOFF, &s->s_flags);
+ unsigned res = 0;
- /* Start a capture if there is none */
- if (!eof && !test_bit(IVTV_F_S_STREAMING, &s->s_flags)) {
- int rc;
+ IVTV_DEBUG_HI_FILE("Encoder poll\n");
- mutex_lock(&itv->serialize_lock);
- rc = ivtv_start_capture(id);
- mutex_unlock(&itv->serialize_lock);
- if (rc) {
- IVTV_DEBUG_INFO("Could not start capture for %s (%d)\n",
- s->name, rc);
- return POLLERR;
- }
- IVTV_DEBUG_FILE("Encoder poll started capture\n");
- }
+ /* Start a capture if there is none */
+ if (!eof && !test_bit(IVTV_F_S_STREAMING, &s->s_flags))
+ res = POLLIN | POLLRDNORM;
- /* add stream's waitq to the poll list */
- IVTV_DEBUG_HI_FILE("Encoder poll\n");
- poll_wait(filp, &s->waitq, wait);
+ if (v4l2_event_pending(&id->fh))
+ res |= POLLPRI;
+ else
+ poll_wait(filp, &id->fh.events->wait, wait);
if (s->q_full.length || s->q_io.length)
- return POLLIN | POLLRDNORM;
+ return res | POLLIN | POLLRDNORM;
if (eof)
- return POLLHUP;
- return 0;
+ return res | POLLHUP;
+
+ /* add stream's waitq to the poll list */
+ poll_wait(filp, &s->waitq, wait);
+ return res;
}
void ivtv_stop_capture(struct ivtv_open_id *id, int gop_end)
diff --git a/drivers/media/video/ivtv/ivtv-ioctl.c b/drivers/media/video/ivtv/ivtv-ioctl.c
index 1689783..a81b4be 100644
--- a/drivers/media/video/ivtv/ivtv-ioctl.c
+++ b/drivers/media/video/ivtv/ivtv-ioctl.c
@@ -1445,6 +1445,8 @@ static int ivtv_subscribe_event(struct v4l2_fh *fh, struct v4l2_event_subscripti
case V4L2_EVENT_VSYNC:
case V4L2_EVENT_EOS:
break;
+ case V4L2_EVENT_CTRL:
+ return v4l2_ctrl_subscribe_fh(fh, sub, 0);
default:
return -EINVAL;
}
--
1.7.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [RFCv3 PATCH 18/18] v4l2-compat-ioctl32: add VIDIOC_DQEVENT support.
2011-06-07 15:05 ` [RFCv3 PATCH 01/18] v4l2-ctrls: introduce call_op define Hans Verkuil
` (15 preceding siblings ...)
2011-06-07 15:05 ` [RFCv3 PATCH 17/18] ivtv: add control event support Hans Verkuil
@ 2011-06-07 15:05 ` Hans Verkuil
16 siblings, 0 replies; 42+ messages in thread
From: Hans Verkuil @ 2011-06-07 15:05 UTC (permalink / raw)
To: linux-media; +Cc: Hans Verkuil
From: Hans Verkuil <hans.verkuil@cisco.com>
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
drivers/media/video/v4l2-compat-ioctl32.c | 37 +++++++++++++++++++++++++++++
kernel/compat.c | 1 +
2 files changed, 38 insertions(+), 0 deletions(-)
diff --git a/drivers/media/video/v4l2-compat-ioctl32.c b/drivers/media/video/v4l2-compat-ioctl32.c
index 7c26947..61979b7 100644
--- a/drivers/media/video/v4l2-compat-ioctl32.c
+++ b/drivers/media/video/v4l2-compat-ioctl32.c
@@ -662,6 +662,32 @@ static int put_v4l2_ext_controls32(struct v4l2_ext_controls *kp, struct v4l2_ext
return 0;
}
+struct v4l2_event32 {
+ __u32 type;
+ union {
+ __u8 data[64];
+ } u;
+ __u32 pending;
+ __u32 sequence;
+ struct compat_timespec timestamp;
+ __u32 id;
+ __u32 reserved[8];
+};
+
+static int put_v4l2_event32(struct v4l2_event *kp, struct v4l2_event32 __user *up)
+{
+ if (!access_ok(VERIFY_WRITE, up, sizeof(struct v4l2_event32)) ||
+ put_user(kp->type, &up->type) ||
+ copy_to_user(&up->u, &kp->u, sizeof(kp->u)) ||
+ put_user(kp->pending, &up->pending) ||
+ put_user(kp->sequence, &up->sequence) ||
+ put_compat_timespec(&kp->timestamp, &up->timestamp) ||
+ put_user(kp->id, &up->id) ||
+ copy_to_user(up->reserved, kp->reserved, 8 * sizeof(__u32)))
+ return -EFAULT;
+ return 0;
+}
+
#define VIDIOC_G_FMT32 _IOWR('V', 4, struct v4l2_format32)
#define VIDIOC_S_FMT32 _IOWR('V', 5, struct v4l2_format32)
#define VIDIOC_QUERYBUF32 _IOWR('V', 9, struct v4l2_buffer32)
@@ -675,6 +701,7 @@ static int put_v4l2_ext_controls32(struct v4l2_ext_controls *kp, struct v4l2_ext
#define VIDIOC_G_EXT_CTRLS32 _IOWR('V', 71, struct v4l2_ext_controls32)
#define VIDIOC_S_EXT_CTRLS32 _IOWR('V', 72, struct v4l2_ext_controls32)
#define VIDIOC_TRY_EXT_CTRLS32 _IOWR('V', 73, struct v4l2_ext_controls32)
+#define VIDIOC_DQEVENT32 _IOR ('V', 89, struct v4l2_event32)
#define VIDIOC_OVERLAY32 _IOW ('V', 14, s32)
#define VIDIOC_STREAMON32 _IOW ('V', 18, s32)
@@ -693,6 +720,7 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
struct v4l2_input v2i;
struct v4l2_standard v2s;
struct v4l2_ext_controls v2ecs;
+ struct v4l2_event v2ev;
unsigned long vx;
int vi;
} karg;
@@ -715,6 +743,7 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
case VIDIOC_G_EXT_CTRLS32: cmd = VIDIOC_G_EXT_CTRLS; break;
case VIDIOC_S_EXT_CTRLS32: cmd = VIDIOC_S_EXT_CTRLS; break;
case VIDIOC_TRY_EXT_CTRLS32: cmd = VIDIOC_TRY_EXT_CTRLS; break;
+ case VIDIOC_DQEVENT32: cmd = VIDIOC_DQEVENT; break;
case VIDIOC_OVERLAY32: cmd = VIDIOC_OVERLAY; break;
case VIDIOC_STREAMON32: cmd = VIDIOC_STREAMON; break;
case VIDIOC_STREAMOFF32: cmd = VIDIOC_STREAMOFF; break;
@@ -778,6 +807,9 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
err = get_v4l2_ext_controls32(&karg.v2ecs, up);
compatible_arg = 0;
break;
+ case VIDIOC_DQEVENT:
+ compatible_arg = 0;
+ break;
}
if (err)
return err;
@@ -818,6 +850,10 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
err = put_v4l2_framebuffer32(&karg.v2fb, up);
break;
+ case VIDIOC_DQEVENT:
+ err = put_v4l2_event32(&karg.v2ev, up);
+ break;
+
case VIDIOC_G_FMT:
case VIDIOC_S_FMT:
case VIDIOC_TRY_FMT:
@@ -920,6 +956,7 @@ long v4l2_compat_ioctl32(struct file *file, unsigned int cmd, unsigned long arg)
case VIDIOC_S_DV_TIMINGS:
case VIDIOC_G_DV_TIMINGS:
case VIDIOC_DQEVENT:
+ case VIDIOC_DQEVENT32:
case VIDIOC_SUBSCRIBE_EVENT:
case VIDIOC_UNSUBSCRIBE_EVENT:
ret = do_video_ioctl(file, cmd, arg);
diff --git a/kernel/compat.c b/kernel/compat.c
index fc9eb09..d4abc5b 100644
--- a/kernel/compat.c
+++ b/kernel/compat.c
@@ -158,6 +158,7 @@ int put_compat_timespec(const struct timespec *ts, struct compat_timespec __user
__put_user(ts->tv_sec, &cts->tv_sec) ||
__put_user(ts->tv_nsec, &cts->tv_nsec)) ? -EFAULT : 0;
}
+EXPORT_SYMBOL_GPL(put_compat_timespec);
static long compat_nanosleep_restart(struct restart_block *restart)
{
--
1.7.1
^ permalink raw reply related [flat|nested] 42+ messages in thread