Re: [RFC PATCH 3/3] [media] gspca - main: implement vidioc_g_ext_ctrls and vidioc_s_ext_ctrls
Hi, On 04/27/2012 11:24 AM, Jean-Francois Moine wrote: On Fri, 27 Apr 2012 10:20:23 +0200 Hans Verkuil 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 3/3] [media] gspca - main: implement vidioc_g_ext_ctrls and vidioc_s_ext_ctrls
On Friday, April 27, 2012 11:24:43 Jean-Francois Moine wrote: > On Fri, 27 Apr 2012 10:20:23 +0200 > Hans Verkuil 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 3/3] [media] gspca - main: implement vidioc_g_ext_ctrls and vidioc_s_ext_ctrls
On Fri, 27 Apr 2012 10:20:23 +0200 Hans Verkuil 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. -- Ken ar c'hentaƱ | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 3/3] [media] gspca - main: implement vidioc_g_ext_ctrls and vidioc_s_ext_ctrls
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 > --- > 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH 3/3] [media] gspca - main: implement vidioc_g_ext_ctrls and vidioc_s_ext_ctrls
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 --- 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, -- 1.7.10 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html