[RFC,3/3,media] gspca - main: implement vidioc_g_ext_ctrls and vidioc_s_ext_ctrls
Commit Message
This makes it possible for applications to handle controls with a class
different than V4L2_CTRL_CLASS_USER for gspca subdevices, like for
instance V4L2_CID_EXPOSURE_AUTO which some subdrivers use and which
can't be controlled right now.
See
http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/47010
for an example of a problem fixed by this change.
NOTE: gspca currently won't handle control types like
V4L2_CTRL_TYPE_INTEGER64 or V4L2_CTRL_TYPE_STRING, so just the
__s32 field 'value' of 'struct v4l2_ext_control' is handled for now.
Signed-off-by: Antonio Ospite <ospite@studenti.unina.it>
---
drivers/media/video/gspca/gspca.c | 42 +++++++++++++++++++++++++++++++++++++
1 file changed, 42 insertions(+)
Comments
Hi Antonio,
My apologies for the late review, I missed this the first time around.
Since I am the 'control guy' :-) I thought I should give my view on this.
And that is the real long-term approach to implementing extended controls
in gspca is to add support in gspca for the control framework (see
Documentation/video4linux/v4l2-controls.txt). That will probably simplify
control handling in gspca anyway. The main problem is how to do it in such
a manner that we can convert gspca drivers to the control framework one by
one.
I am inclined to NACK code that adds a driver-specific extended control
implementation instead of going to the control framework because I know
from painful personal experience that it is very hard to get it right.
I might have some time (no guarantees yet) to help with this. It would
certainly be interesting to add support for the control framework in the
gspca core. Hmm, perhaps that's a job for the weekend...
Regards,
Hans
On Friday, April 20, 2012 17:19:11 Antonio Ospite wrote:
> This makes it possible for applications to handle controls with a class
> different than V4L2_CTRL_CLASS_USER for gspca subdevices, like for
> instance V4L2_CID_EXPOSURE_AUTO which some subdrivers use and which
> can't be controlled right now.
>
> See
> http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/47010
> for an example of a problem fixed by this change.
>
> NOTE: gspca currently won't handle control types like
> V4L2_CTRL_TYPE_INTEGER64 or V4L2_CTRL_TYPE_STRING, so just the
> __s32 field 'value' of 'struct v4l2_ext_control' is handled for now.
>
> Signed-off-by: Antonio Ospite <ospite@studenti.unina.it>
> ---
> drivers/media/video/gspca/gspca.c | 42 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 42 insertions(+)
>
> diff --git a/drivers/media/video/gspca/gspca.c b/drivers/media/video/gspca/gspca.c
> index ba1bda9..7906093 100644
> --- a/drivers/media/video/gspca/gspca.c
> +++ b/drivers/media/video/gspca/gspca.c
> @@ -1567,6 +1567,46 @@ static int vidioc_g_ctrl(struct file *file, void *priv,
> return gspca_get_ctrl(gspca_dev, ctrl->id, &ctrl->value);
> }
>
> +static int vidioc_s_ext_ctrls(struct file *file, void *priv,
> + struct v4l2_ext_controls *ext_ctrls)
> +{
> + struct gspca_dev *gspca_dev = priv;
> + int ret = 0;
> + int i;
> +
> + for (i = 0; i < ext_ctrls->count; i++) {
> + struct v4l2_ext_control *ctrl;
> +
> + ctrl = ext_ctrls->controls + i;
> + ret = gspca_set_ctrl(gspca_dev, ctrl->id, ctrl->value);
> + if (ret < 0) {
> + ext_ctrls->error_idx = i;
> + return ret;
> + }
> + }
> + return ret;
> +}
> +
> +static int vidioc_g_ext_ctrls(struct file *file, void *priv,
> + struct v4l2_ext_controls *ext_ctrls)
> +{
> + struct gspca_dev *gspca_dev = priv;
> + int i;
> + int ret = 0;
> +
> + for (i = 0; i < ext_ctrls->count; i++) {
> + struct v4l2_ext_control *ctrl;
> +
> + ctrl = ext_ctrls->controls + i;
> + ret = gspca_get_ctrl(gspca_dev, ctrl->id, &ctrl->value);
> + if (ret < 0) {
> + ext_ctrls->error_idx = i;
> + return ret;
> + }
> + }
> + return ret;
> +}
> +
> static int vidioc_querymenu(struct file *file, void *priv,
> struct v4l2_querymenu *qmenu)
> {
> @@ -2260,6 +2300,8 @@ static const struct v4l2_ioctl_ops dev_ioctl_ops = {
> .vidioc_queryctrl = vidioc_queryctrl,
> .vidioc_g_ctrl = vidioc_g_ctrl,
> .vidioc_s_ctrl = vidioc_s_ctrl,
> + .vidioc_g_ext_ctrls = vidioc_g_ext_ctrls,
> + .vidioc_s_ext_ctrls = vidioc_s_ext_ctrls,
> .vidioc_querymenu = vidioc_querymenu,
> .vidioc_enum_input = vidioc_enum_input,
> .vidioc_g_input = vidioc_g_input,
>
--
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
On Fri, 27 Apr 2012 10:20:23 +0200
Hans Verkuil <hverkuil@xs4all.nl> wrote:
> I might have some time (no guarantees yet) to help with this. It would
> certainly be interesting to add support for the control framework in the
> gspca core. Hmm, perhaps that's a job for the weekend...
Hi Hans,
I hope you will not do it! The way gspca treats the controls is static,
quick and very small. The controls in the subdrivers ask only for the
declaration of the controls and functions to send the values to the
webcams. Actually, not all subdrivers have been converted to the new
gspca control handling, but, when done, it will save more memory.
Best regards.
On Friday, April 27, 2012 11:24:43 Jean-Francois Moine wrote:
> On Fri, 27 Apr 2012 10:20:23 +0200
> Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> > I might have some time (no guarantees yet) to help with this. It would
> > certainly be interesting to add support for the control framework in the
> > gspca core. Hmm, perhaps that's a job for the weekend...
>
> Hi Hans,
>
> I hope you will not do it! The way gspca treats the controls is static,
> quick and very small. The controls in the subdrivers ask only for the
> declaration of the controls and functions to send the values to the
> webcams. Actually, not all subdrivers have been converted to the new
> gspca control handling, but, when done, it will save more memory.
And that is exactly what the control framework also does for you. Drivers
only have to declare the controls and have a function to set their value.
Everything else is handled by the control framework. And you get support
for the extended control API for free and also support for control events,
plus any future changes/enhancements to how controls are done will be
automatically added. Not to mention that it ensures consistent and correct
behavior of the control API towards applications.
Note that the control code is already part of videodev.ko, so you have that
code in memory anyway. So why not use it?
Regards,
Hans
--
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
Hi,
On 04/27/2012 11:24 AM, Jean-Francois Moine wrote:
> On Fri, 27 Apr 2012 10:20:23 +0200
> Hans Verkuil<hverkuil@xs4all.nl> wrote:
>
>> I might have some time (no guarantees yet) to help with this. It would
>> certainly be interesting to add support for the control framework in the
>> gspca core. Hmm, perhaps that's a job for the weekend...
>
> Hi Hans,
>
> I hope you will not do it! The way gspca treats the controls is static,
> quick and very small. The controls in the subdrivers ask only for the
> declaration of the controls and functions to send the values to the
> webcams. Actually, not all subdrivers have been converted to the new
> gspca control handling, but, when done, it will save more memory.
Actually I've moving gspca over to the control framework on my to-do
list. This will allows us to remove hacks like we have in sonixb.c for
coarse exposure / fine exposure controls. More in general it will allow
(in an easy way) to have per sensor control min/max/step values.
But the most important reason for me to want to use the control framework
in gspca is to get support for control events. Which allow a control-panel
app to dynamically update its display when settings are changed to some
other way (ie another control app, or software autogain).
I do plan to do this in way so that we can do this one subdriver at a
time. I've no idea when I'll get around to doing the first driver
though :)
Regards,
Hans
--
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
@@ -1567,6 +1567,46 @@ static int vidioc_g_ctrl(struct file *file, void *priv,
return gspca_get_ctrl(gspca_dev, ctrl->id, &ctrl->value);
}
+static int vidioc_s_ext_ctrls(struct file *file, void *priv,
+ struct v4l2_ext_controls *ext_ctrls)
+{
+ struct gspca_dev *gspca_dev = priv;
+ int ret = 0;
+ int i;
+
+ for (i = 0; i < ext_ctrls->count; i++) {
+ struct v4l2_ext_control *ctrl;
+
+ ctrl = ext_ctrls->controls + i;
+ ret = gspca_set_ctrl(gspca_dev, ctrl->id, ctrl->value);
+ if (ret < 0) {
+ ext_ctrls->error_idx = i;
+ return ret;
+ }
+ }
+ return ret;
+}
+
+static int vidioc_g_ext_ctrls(struct file *file, void *priv,
+ struct v4l2_ext_controls *ext_ctrls)
+{
+ struct gspca_dev *gspca_dev = priv;
+ int i;
+ int ret = 0;
+
+ for (i = 0; i < ext_ctrls->count; i++) {
+ struct v4l2_ext_control *ctrl;
+
+ ctrl = ext_ctrls->controls + i;
+ ret = gspca_get_ctrl(gspca_dev, ctrl->id, &ctrl->value);
+ if (ret < 0) {
+ ext_ctrls->error_idx = i;
+ return ret;
+ }
+ }
+ return ret;
+}
+
static int vidioc_querymenu(struct file *file, void *priv,
struct v4l2_querymenu *qmenu)
{
@@ -2260,6 +2300,8 @@ static const struct v4l2_ioctl_ops dev_ioctl_ops = {
.vidioc_queryctrl = vidioc_queryctrl,
.vidioc_g_ctrl = vidioc_g_ctrl,
.vidioc_s_ctrl = vidioc_s_ctrl,
+ .vidioc_g_ext_ctrls = vidioc_g_ext_ctrls,
+ .vidioc_s_ext_ctrls = vidioc_s_ext_ctrls,
.vidioc_querymenu = vidioc_querymenu,
.vidioc_enum_input = vidioc_enum_input,
.vidioc_g_input = vidioc_g_input,