Re: [RFCv1 PATCH 1/7] gspca: allow subdrivers to use the control framework.
On Sat, 05 May 2012 17:05:28 +0200 Hans de Goede wrote: > > Now I see that we are doing exactly that in for example vidioc_g_jpegcomp > > in gspca.c, so > > we should stop doing that. We can make vidioc_g/s_jpegcomp only do the usb > > locking if > > gspca_dev->vdev.ctrl_handler == NULL, and once all sub drivers are > > converted simply remove > > it. Actually I'm thinking about making the jpegqual control part of the > > gspca_dev struct > > itself and move all handling of vidioc_g/s_jpegcomp out of the sub drivers > > and into > > the core. > > Here is an updated version of this patch implementing this approach for > vidioc_g/s_jpegcomp. We may need to do something similar in other places, > although I cannot > think of any such places atm, As the JPEG parameters have been redefined as standard controls, and as there should be only a very few applications which use it, I think the vidioc_g/s_jpegcomp code could be fully removed. -- 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: [RFCv1 PATCH 1/7] gspca: allow subdrivers to use the control framework.
On Sat, 05 May 2012 16:59:30 +0200 Hans de Goede wrote: > > Unless there is another good reason for doing the probing in sd_init I > > prefer > > to move it to sd_config. > > Sensor probing does more then just sensor probing, it also configures > things like the i2c clockrate, and if the bus between bridge and sensor > is spi / i2c or 3-wire, or whatever ... > > After a suspend resume all bets are of wrt bridge state, so we prefer to > always do a full re-init as we do on initial probe, so that we (hopefully) > will put the bridge back in a sane state. > > I think moving the probing from init to config is a bad idea, the chance > that we will get regressions (after a suspend/resume) from this are too > big IMHO. Moving the sensor probing to sd_config is normally safe because the init sequences which are sent actually after probing do all the re-initialization job. An easy way to know it in zc3xx is to force the sensor via the module parameter. -- 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: [RFCv1 PATCH 1/7] gspca: allow subdrivers to use the control framework.
Hi, On 05/05/2012 05:02 PM, Hans Verkuil wrote: BTW, this is getting messy: both of us producing patches for the same driver :-) For those reading along on the mailinglist, we've got together on irc and coordinated how we are going to handle this there. 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: [RFCv1 PATCH 1/7] gspca: allow subdrivers to use the control framework.
Hi,
Oops, forgot the attachment it is here now...
Regards,
Hans
On 05/05/2012 05:05 PM, Hans de Goede wrote:
Hi,
On 05/05/2012 04:44 PM, Hans de Goede wrote:
Hi,
On 05/05/2012 11:14 AM, Hans Verkuil wrote:
So you get:
vidioc_foo()
lock(mylock)
v4l2_ctrl_s_ctrl(ctrl, val)
s_ctrl(ctrl, val)
lock(mylock)
Easy solution here, remove the first lock(mylock), since we are not using
v4l2-dev's
locking, we are the one doing the first lock, and if we are going to call
v4l2_ctrl_s_ctrl
we should simply not do that!
Now I see that we are doing exactly that in for example vidioc_g_jpegcomp in
gspca.c, so
we should stop doing that. We can make vidioc_g/s_jpegcomp only do the usb
locking if
gspca_dev->vdev.ctrl_handler == NULL, and once all sub drivers are converted
simply remove
it. Actually I'm thinking about making the jpegqual control part of the
gspca_dev struct
itself and move all handling of vidioc_g/s_jpegcomp out of the sub drivers and
into
the core.
Here is an updated version of this patch implementing this approach for
vidioc_g/s_jpegcomp. We may need to do something similar in other places,
although I cannot
think of any such places atm,
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
>From eb7eb7c63156c1c040a7fddaeddcf1b1891f0fb7 Mon Sep 17 00:00:00 2001
From: Hans Verkuil
Date: Sat, 28 Apr 2012 17:09:50 +0200
Subject: [PATCH 1/8] gspca: allow subdrivers to use the control framework.
Make the necessary changes to allow subdrivers to use the control framework.
This does not add control event support, that needs more work.
Signed-off-by: Hans Verkuil
Signed-off-by: Hans de Goede
---
drivers/media/video/gspca/gspca.c | 47 ++---
1 file changed, 33 insertions(+), 14 deletions(-)
diff --git a/drivers/media/video/gspca/gspca.c b/drivers/media/video/gspca/gspca.c
index ca5a2b1..dfe2e8a 100644
--- a/drivers/media/video/gspca/gspca.c
+++ b/drivers/media/video/gspca/gspca.c
@@ -38,6 +38,7 @@
#include
#include
#include
+#include
#include "gspca.h"
@@ -1006,6 +1007,8 @@ static void gspca_set_default_mode(struct gspca_dev *gspca_dev)
/* set the current control values to their default values
* which may have changed in sd_init() */
+ /* does nothing if ctrl_handler == NULL */
+ v4l2_ctrl_handler_setup(gspca_dev->vdev.ctrl_handler);
ctrl = gspca_dev->cam.ctrls;
if (ctrl != NULL) {
for (i = 0;
@@ -1323,6 +1326,7 @@ static void gspca_release(struct video_device *vfd)
PDEBUG(D_PROBE, "%s released",
video_device_node_name(&gspca_dev->vdev));
+ v4l2_ctrl_handler_free(gspca_dev->vdev.ctrl_handler);
kfree(gspca_dev->usb_buf);
kfree(gspca_dev);
}
@@ -1771,14 +1775,21 @@ static int vidioc_g_jpegcomp(struct file *file, void *priv,
if (!gspca_dev->sd_desc->get_jcomp)
return -EINVAL;
- if (mutex_lock_interruptible(&gspca_dev->usb_lock))
- return -ERESTARTSYS;
+
+ /* Don't take the usb_lock when using the v4l2-ctrls framework */
+ if (gspca_dev->vdev.ctrl_handler == NULL)
+ if (mutex_lock_interruptible(&gspca_dev->usb_lock))
+ return -ERESTARTSYS;
+
gspca_dev->usb_err = 0;
if (gspca_dev->present)
ret = gspca_dev->sd_desc->get_jcomp(gspca_dev, jpegcomp);
else
ret = -ENODEV;
- mutex_unlock(&gspca_dev->usb_lock);
+
+ if (gspca_dev->vdev.ctrl_handler == NULL)
+ mutex_unlock(&gspca_dev->usb_lock);
+
return ret;
}
@@ -1790,14 +1801,21 @@ static int vidioc_s_jpegcomp(struct file *file, void *priv,
if (!gspca_dev->sd_desc->set_jcomp)
return -EINVAL;
- if (mutex_lock_interruptible(&gspca_dev->usb_lock))
- return -ERESTARTSYS;
+
+ /* Don't take the usb_lock when using the v4l2-ctrls framework */
+ if (gspca_dev->vdev.ctrl_handler == NULL)
+ if (mutex_lock_interruptible(&gspca_dev->usb_lock))
+ return -ERESTARTSYS;
+
gspca_dev->usb_err = 0;
if (gspca_dev->present)
ret = gspca_dev->sd_desc->set_jcomp(gspca_dev, jpegcomp);
else
ret = -ENODEV;
- mutex_unlock(&gspca_dev->usb_lock);
+
+ if (gspca_dev->vdev.ctrl_handler == NULL)
+ mutex_unlock(&gspca_dev->usb_lock);
+
return ret;
}
@@ -2347,6 +2365,14 @@ int gspca_dev_probe2(struct usb_interface *intf,
gspca_dev->sd_desc = sd_desc;
gspca_dev->nbufread = 2;
gspca_dev->empty_packet = -1; /* don't check the empty packets */
+ gspca_dev->vdev = gspca_template;
+ gspca_dev->vdev.parent = &intf->dev;
+ gspca_dev->module = module;
+ gspca_dev->present = 1;
+
+ mutex_init(&gspca_dev->usb_lock);
+ mutex_init(&gspca_dev->queue_lock);
+ init_waitqueue_head(&gspca_dev->wq);
/* configure the subdriver and initialize the USB device */
ret = sd_desc->config(gspca_dev, id);
@@ -2363,15 +2389,7 @@ int gspca_dev_probe2(struct usb_interface *intf,
if (ret)
goto out;
- mutex_init(&gspca_dev->usb_lock);
- mutex_init(&gspca_dev->queue_lock);
- init_waitqueue_head(&gspca_dev->wq);
-
/* i
Re: [RFCv1 PATCH 1/7] gspca: allow subdrivers to use the control framework.
Hi, On 05/05/2012 04:44 PM, Hans de Goede wrote: Hi, On 05/05/2012 11:14 AM, Hans Verkuil wrote: So you get: vidioc_foo() lock(mylock) v4l2_ctrl_s_ctrl(ctrl, val) s_ctrl(ctrl, val) lock(mylock) Easy solution here, remove the first lock(mylock), since we are not using v4l2-dev's locking, we are the one doing the first lock, and if we are going to call v4l2_ctrl_s_ctrl we should simply not do that! Now I see that we are doing exactly that in for example vidioc_g_jpegcomp in gspca.c, so we should stop doing that. We can make vidioc_g/s_jpegcomp only do the usb locking if gspca_dev->vdev.ctrl_handler == NULL, and once all sub drivers are converted simply remove it. Actually I'm thinking about making the jpegqual control part of the gspca_dev struct itself and move all handling of vidioc_g/s_jpegcomp out of the sub drivers and into the core. Here is an updated version of this patch implementing this approach for vidioc_g/s_jpegcomp. We may need to do something similar in other places, although I cannot think of any such places atm, 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: [RFCv1 PATCH 1/7] gspca: allow subdrivers to use the control framework.
On Sat May 5 2012 16:44:34 Hans de Goede wrote:
> Hi,
>
> On 05/05/2012 11:14 AM, Hans Verkuil wrote:
> > On Sat May 5 2012 09:43:01 Hans de Goede wrote:
> >> Hi,
> >>
> >> I'm slowly working my way though this series today (both review, as well
> >> as some tweaks and testing).
> >>
> >> More comments inline...
> >>
> >> On 04/28/2012 05:09 PM, Hans Verkuil wrote:
> >>> From: Hans Verkuil
> >>>
> >>> Make the necessary changes to allow subdrivers to use the control
> >>> framework.
> >>> This does not add control event support, that needs more work.
> >>>
> >>> Signed-off-by: Hans Verkuil
> >>> ---
> >>>drivers/media/video/gspca/gspca.c | 13 +
> >>>1 file changed, 9 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/drivers/media/video/gspca/gspca.c
> >>> b/drivers/media/video/gspca/gspca.c
> >>> index ca5a2b1..56dff10 100644
> >>> --- a/drivers/media/video/gspca/gspca.c
> >>> +++ b/drivers/media/video/gspca/gspca.c
> >>> @@ -38,6 +38,7 @@
> >>>#include
> >>>#include
> >>>#include
> >>> +#include
> >>>
> >>>#include "gspca.h"
> >>>
> >>> @@ -1006,6 +1007,8 @@ static void gspca_set_default_mode(struct gspca_dev
> >>> *gspca_dev)
> >>>
> >>> /* set the current control values to their default values
> >>>* which may have changed in sd_init() */
> >>> + /* does nothing if ctrl_handler == NULL */
> >>> + v4l2_ctrl_handler_setup(gspca_dev->vdev.ctrl_handler);
> >>> ctrl = gspca_dev->cam.ctrls;
> >>> if (ctrl != NULL) {
> >>> for (i = 0;
> >>> @@ -1323,6 +1326,7 @@ static void gspca_release(struct video_device *vfd)
> >>> PDEBUG(D_PROBE, "%s released",
> >>> video_device_node_name(&gspca_dev->vdev));
> >>>
> >>> + v4l2_ctrl_handler_free(gspca_dev->vdev.ctrl_handler);
> >>> kfree(gspca_dev->usb_buf);
> >>> kfree(gspca_dev);
> >>>}
> >>> @@ -2347,6 +2351,10 @@ int gspca_dev_probe2(struct usb_interface *intf,
> >>> gspca_dev->sd_desc = sd_desc;
> >>> gspca_dev->nbufread = 2;
> >>> gspca_dev->empty_packet = -1; /* don't check the empty
> >>> packets */
> >>> + gspca_dev->vdev = gspca_template;
> >>> + gspca_dev->vdev.parent =&intf->dev;
> >>> + gspca_dev->module = module;
> >>> + gspca_dev->present = 1;
> >>>
> >>> /* configure the subdriver and initialize the USB device */
> >>> ret = sd_desc->config(gspca_dev, id);
> >>
> >> You also need to move the initialization of the mutexes here, as the
> >> v4l2_ctrl_handler_setup will call s_ctrl on all the controls, and s_ctrl
> >> should take the usb_lock (see my review of the next patch in this series),
> >> I'll make this change myself and merge it into your patch.
> >
> > Looking at how usb_lock is used I am inclined to just set video_device->lock
> > to it and let the v4l2 core do all the locking for me, which will
> > automatically
> > fix the missing s_ctrl lock too.
>
> Please don't I've really bad experience with this with pwc (large latencies
> on dqbuf),
Can you refresh my memory on that? I remember that you had problems with it
but I don't think I ever followed up on it (i.e. trying to find a solution
that works with core locking).
> also gspca uses worker-threads which issue usb control requests
> in various places, currently these take usb_lock to avoid them fighting with
> sd_start or controls, these won't know about the global device lock and if
> these start taking that lock too things will become pretty messy pretty
> quickly.
Well, gspca takes usb_lock at the same places the core lock is taken. And it
actually fails to take the lock in the suspend and resume functions which can
lead to an oops (reproduced). Those worker threads do not handle suspend/resume
well at the moment (I have patches for that).
>
> >
> > I've realized that there is a problem if you do your own locking *and* use
> > the
> > control framework: if you need to set a control from within the driver, then
> > you do that using v4l2_ctrl_s_ctrl. But if s_ctrl has to take the driver's
> > lock,
> > then you can't call v4l2_ctrl_s_ctrl with that lock already taken!
> >
> > So you get:
> >
> > vidioc_foo()
> > lock(mylock)
> > v4l2_ctrl_s_ctrl(ctrl, val)
> > s_ctrl(ctrl, val)
> > lock(mylock)
>
> Easy solution here, remove the first lock(mylock), since we are not using
> v4l2-dev's
> locking, we are the one doing the first lock, and if we are going to call
> v4l2_ctrl_s_ctrl
> we should simply not do that!
It's really ugly that way: you basically take a low-level lock (that of the
control handler which is held whenever s_ctrl et al is called) first, then take
a high-level lock (usb_lock). That's asking for problems.
> Now I see that we are doing exactly that in for example vidioc_g_jpegcomp in
> gspca.c, so
> we should stop doing that. We can make vidioc_g/s_jpegcomp only do the usb
> locking if
> gspca_dev->vdev.ctrl_handler =
Re: [RFCv1 PATCH 1/7] gspca: allow subdrivers to use the control framework.
Hi, On 05/05/2012 04:50 PM, Hans Verkuil wrote: On Sat May 5 2012 16:46:32 Hans de Goede wrote: Hi, On 05/05/2012 10:34 AM, Hans Verkuil wrote: On Sat May 5 2012 09:43:01 Hans de Goede wrote: Hi, I'm slowly working my way though this series today (both review, as well as some tweaks and testing). Thanks for that! One note: I initialized the controls in sd_init. That's wrong, it should be sd_config. sd_init is also called on resume, so that would initialize the controls twice. You cannot move the initializing of the controls to sd_config, since in many cases the sensor probing is done in sd_init, and we need to know the sensor type to init the controls. Or you move the sensor probing to sd_config as I did. It makes no sense anyway to do sensor probing every time you resume. Unless there is another good reason for doing the probing in sd_init I prefer to move it to sd_config. Sensor probing does more then just sensor probing, it also configures things like the i2c clockrate, and if the bus between bridge and sensor is spi / i2c or 3-wire, or whatever ... After a suspend resume all bets are of wrt bridge state, so we prefer to always do a full re-init as we do on initial probe, so that we (hopefully) will put the bridge back in a sane state. I think moving the probing from init to config is a bad idea, the chance that we will get regressions (after a suspend/resume) from this are too big IMHO. 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: [RFCv1 PATCH 1/7] gspca: allow subdrivers to use the control framework.
On Sat May 5 2012 16:46:32 Hans de Goede wrote: > Hi, > > On 05/05/2012 10:34 AM, Hans Verkuil wrote: > > On Sat May 5 2012 09:43:01 Hans de Goede wrote: > >> Hi, > >> > >> I'm slowly working my way though this series today (both review, as well > >> as some tweaks and testing). > > > > Thanks for that! > > > > One note: I initialized the controls in sd_init. That's wrong, it should be > > sd_config. sd_init is also called on resume, so that would initialize the > > controls twice. > > You cannot move the initializing of the controls to sd_config, since in many > cases the sensor probing is done in sd_init, and we need to know the sensor > type to init the controls. Or you move the sensor probing to sd_config as I did. It makes no sense anyway to do sensor probing every time you resume. Unless there is another good reason for doing the probing in sd_init I prefer to move it to sd_config. Regards, Hans > I suggest that instead you give the sd_init > function a resume parameter and only init the controls if the resume parameter > is false. > > > I'm working on this as well today, together with finishing the stv06xx and > > mars conversion. > > Cool! > > 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: [RFCv1 PATCH 1/7] gspca: allow subdrivers to use the control framework.
Hi, On 05/05/2012 10:34 AM, Hans Verkuil wrote: On Sat May 5 2012 09:43:01 Hans de Goede wrote: Hi, I'm slowly working my way though this series today (both review, as well as some tweaks and testing). Thanks for that! One note: I initialized the controls in sd_init. That's wrong, it should be sd_config. sd_init is also called on resume, so that would initialize the controls twice. You cannot move the initializing of the controls to sd_config, since in many cases the sensor probing is done in sd_init, and we need to know the sensor type to init the controls. I suggest that instead you give the sd_init function a resume parameter and only init the controls if the resume parameter is false. I'm working on this as well today, together with finishing the stv06xx and mars conversion. Cool! 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: [RFCv1 PATCH 1/7] gspca: allow subdrivers to use the control framework.
Hi,
On 05/05/2012 11:14 AM, Hans Verkuil wrote:
On Sat May 5 2012 09:43:01 Hans de Goede wrote:
Hi,
I'm slowly working my way though this series today (both review, as well
as some tweaks and testing).
More comments inline...
On 04/28/2012 05:09 PM, Hans Verkuil wrote:
From: Hans Verkuil
Make the necessary changes to allow subdrivers to use the control framework.
This does not add control event support, that needs more work.
Signed-off-by: Hans Verkuil
---
drivers/media/video/gspca/gspca.c | 13 +
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/media/video/gspca/gspca.c
b/drivers/media/video/gspca/gspca.c
index ca5a2b1..56dff10 100644
--- a/drivers/media/video/gspca/gspca.c
+++ b/drivers/media/video/gspca/gspca.c
@@ -38,6 +38,7 @@
#include
#include
#include
+#include
#include "gspca.h"
@@ -1006,6 +1007,8 @@ static void gspca_set_default_mode(struct gspca_dev
*gspca_dev)
/* set the current control values to their default values
* which may have changed in sd_init() */
+ /* does nothing if ctrl_handler == NULL */
+ v4l2_ctrl_handler_setup(gspca_dev->vdev.ctrl_handler);
ctrl = gspca_dev->cam.ctrls;
if (ctrl != NULL) {
for (i = 0;
@@ -1323,6 +1326,7 @@ static void gspca_release(struct video_device *vfd)
PDEBUG(D_PROBE, "%s released",
video_device_node_name(&gspca_dev->vdev));
+ v4l2_ctrl_handler_free(gspca_dev->vdev.ctrl_handler);
kfree(gspca_dev->usb_buf);
kfree(gspca_dev);
}
@@ -2347,6 +2351,10 @@ int gspca_dev_probe2(struct usb_interface *intf,
gspca_dev->sd_desc = sd_desc;
gspca_dev->nbufread = 2;
gspca_dev->empty_packet = -1;/* don't check the empty packets */
+ gspca_dev->vdev = gspca_template;
+ gspca_dev->vdev.parent =&intf->dev;
+ gspca_dev->module = module;
+ gspca_dev->present = 1;
/* configure the subdriver and initialize the USB device */
ret = sd_desc->config(gspca_dev, id);
You also need to move the initialization of the mutexes here, as the
v4l2_ctrl_handler_setup will call s_ctrl on all the controls, and s_ctrl
should take the usb_lock (see my review of the next patch in this series),
I'll make this change myself and merge it into your patch.
Looking at how usb_lock is used I am inclined to just set video_device->lock
to it and let the v4l2 core do all the locking for me, which will automatically
fix the missing s_ctrl lock too.
Please don't I've really bad experience with this with pwc (large latencies
on dqbuf), also gspca uses worker-threads which issue usb control requests
in various places, currently these take usb_lock to avoid them fighting with
sd_start or controls, these won't know about the global device lock and if
these start taking that lock too things will become pretty messy pretty quickly.
I've realized that there is a problem if you do your own locking *and* use the
control framework: if you need to set a control from within the driver, then
you do that using v4l2_ctrl_s_ctrl. But if s_ctrl has to take the driver's lock,
then you can't call v4l2_ctrl_s_ctrl with that lock already taken!
So you get:
vidioc_foo()
lock(mylock)
v4l2_ctrl_s_ctrl(ctrl, val)
s_ctrl(ctrl, val)
lock(mylock)
Easy solution here, remove the first lock(mylock), since we are not using
v4l2-dev's
locking, we are the one doing the first lock, and if we are going to call
v4l2_ctrl_s_ctrl
we should simply not do that!
Now I see that we are doing exactly that in for example vidioc_g_jpegcomp in
gspca.c, so
we should stop doing that. We can make vidioc_g/s_jpegcomp only do the usb
locking if
gspca_dev->vdev.ctrl_handler == NULL, and once all sub drivers are converted
simply remove
it. Actually I'm thinking about making the jpegqual control part of the
gspca_dev struct
itself and move all handling of vidioc_g/s_jpegcomp out of the sub drivers and
into
the core.
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: [RFCv1 PATCH 1/7] gspca: allow subdrivers to use the control framework.
On Sat May 5 2012 09:43:01 Hans de Goede wrote:
> Hi,
>
> I'm slowly working my way though this series today (both review, as well
> as some tweaks and testing).
>
> More comments inline...
>
> On 04/28/2012 05:09 PM, Hans Verkuil wrote:
> > From: Hans Verkuil
> >
> > Make the necessary changes to allow subdrivers to use the control framework.
> > This does not add control event support, that needs more work.
> >
> > Signed-off-by: Hans Verkuil
> > ---
> > drivers/media/video/gspca/gspca.c | 13 +
> > 1 file changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/video/gspca/gspca.c
> > b/drivers/media/video/gspca/gspca.c
> > index ca5a2b1..56dff10 100644
> > --- a/drivers/media/video/gspca/gspca.c
> > +++ b/drivers/media/video/gspca/gspca.c
> > @@ -38,6 +38,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #include "gspca.h"
> >
> > @@ -1006,6 +1007,8 @@ static void gspca_set_default_mode(struct gspca_dev
> > *gspca_dev)
> >
> > /* set the current control values to their default values
> > * which may have changed in sd_init() */
> > + /* does nothing if ctrl_handler == NULL */
> > + v4l2_ctrl_handler_setup(gspca_dev->vdev.ctrl_handler);
> > ctrl = gspca_dev->cam.ctrls;
> > if (ctrl != NULL) {
> > for (i = 0;
> > @@ -1323,6 +1326,7 @@ static void gspca_release(struct video_device *vfd)
> > PDEBUG(D_PROBE, "%s released",
> > video_device_node_name(&gspca_dev->vdev));
> >
> > + v4l2_ctrl_handler_free(gspca_dev->vdev.ctrl_handler);
> > kfree(gspca_dev->usb_buf);
> > kfree(gspca_dev);
> > }
> > @@ -2347,6 +2351,10 @@ int gspca_dev_probe2(struct usb_interface *intf,
> > gspca_dev->sd_desc = sd_desc;
> > gspca_dev->nbufread = 2;
> > gspca_dev->empty_packet = -1; /* don't check the empty packets */
> > + gspca_dev->vdev = gspca_template;
> > + gspca_dev->vdev.parent =&intf->dev;
> > + gspca_dev->module = module;
> > + gspca_dev->present = 1;
> >
> > /* configure the subdriver and initialize the USB device */
> > ret = sd_desc->config(gspca_dev, id);
>
> You also need to move the initialization of the mutexes here, as the
> v4l2_ctrl_handler_setup will call s_ctrl on all the controls, and s_ctrl
> should take the usb_lock (see my review of the next patch in this series),
> I'll make this change myself and merge it into your patch.
Looking at how usb_lock is used I am inclined to just set video_device->lock
to it and let the v4l2 core do all the locking for me, which will automatically
fix the missing s_ctrl lock too.
I've realized that there is a problem if you do your own locking *and* use the
control framework: if you need to set a control from within the driver, then
you do that using v4l2_ctrl_s_ctrl. But if s_ctrl has to take the driver's lock,
then you can't call v4l2_ctrl_s_ctrl with that lock already taken!
So you get:
vidioc_foo()
lock(mylock)
v4l2_ctrl_s_ctrl(ctrl, val)
s_ctrl(ctrl, val)
lock(mylock)
If the core takes care of locking then everything is fine.
All the current drivers that use v4l2_ctrl_g/s_ctrl use core locking. But this
can be a problem in the future. The only way to resolve this is to tell
v4l2-ioctl.c
about your own lock so it can take it for you when calling into the control
framework.
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: [RFCv1 PATCH 1/7] gspca: allow subdrivers to use the control framework.
On Sat May 5 2012 09:43:01 Hans de Goede wrote:
> Hi,
>
> I'm slowly working my way though this series today (both review, as well
> as some tweaks and testing).
Thanks for that!
One note: I initialized the controls in sd_init. That's wrong, it should be
sd_config. sd_init is also called on resume, so that would initialize the
controls twice.
I'm working on this as well today, together with finishing the stv06xx and
mars conversion.
I will also do some suspend/resume testing today to check whether the controls
are put back correctly after a resume.
Regards,
Hans
>
> More comments inline...
>
> On 04/28/2012 05:09 PM, Hans Verkuil wrote:
> > From: Hans Verkuil
> >
> > Make the necessary changes to allow subdrivers to use the control framework.
> > This does not add control event support, that needs more work.
> >
> > Signed-off-by: Hans Verkuil
> > ---
> > drivers/media/video/gspca/gspca.c | 13 +
> > 1 file changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/video/gspca/gspca.c
> > b/drivers/media/video/gspca/gspca.c
> > index ca5a2b1..56dff10 100644
> > --- a/drivers/media/video/gspca/gspca.c
> > +++ b/drivers/media/video/gspca/gspca.c
> > @@ -38,6 +38,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #include "gspca.h"
> >
> > @@ -1006,6 +1007,8 @@ static void gspca_set_default_mode(struct gspca_dev
> > *gspca_dev)
> >
> > /* set the current control values to their default values
> > * which may have changed in sd_init() */
> > + /* does nothing if ctrl_handler == NULL */
> > + v4l2_ctrl_handler_setup(gspca_dev->vdev.ctrl_handler);
> > ctrl = gspca_dev->cam.ctrls;
> > if (ctrl != NULL) {
> > for (i = 0;
> > @@ -1323,6 +1326,7 @@ static void gspca_release(struct video_device *vfd)
> > PDEBUG(D_PROBE, "%s released",
> > video_device_node_name(&gspca_dev->vdev));
> >
> > + v4l2_ctrl_handler_free(gspca_dev->vdev.ctrl_handler);
> > kfree(gspca_dev->usb_buf);
> > kfree(gspca_dev);
> > }
> > @@ -2347,6 +2351,10 @@ int gspca_dev_probe2(struct usb_interface *intf,
> > gspca_dev->sd_desc = sd_desc;
> > gspca_dev->nbufread = 2;
> > gspca_dev->empty_packet = -1; /* don't check the empty packets */
> > + gspca_dev->vdev = gspca_template;
> > + gspca_dev->vdev.parent =&intf->dev;
> > + gspca_dev->module = module;
> > + gspca_dev->present = 1;
> >
> > /* configure the subdriver and initialize the USB device */
> > ret = sd_desc->config(gspca_dev, id);
>
> You also need to move the initialization of the mutexes here, as the
> v4l2_ctrl_handler_setup will call s_ctrl on all the controls, and s_ctrl
> should take the usb_lock (see my review of the next patch in this series),
> I'll make this change myself and merge it into your patch.
>
> > @@ -2368,10 +2376,6 @@ int gspca_dev_probe2(struct usb_interface *intf,
> > init_waitqueue_head(&gspca_dev->wq);
> >
> > /* init video stuff */
> > - memcpy(&gspca_dev->vdev,&gspca_template, sizeof gspca_template);
> > - gspca_dev->vdev.parent =&intf->dev;
> > - gspca_dev->module = module;
> > - gspca_dev->present = 1;
> > ret = video_register_device(&gspca_dev->vdev,
> > VFL_TYPE_GRABBER,
> > -1);
> > @@ -2391,6 +2395,7 @@ out:
> > if (gspca_dev->input_dev)
> > input_unregister_device(gspca_dev->input_dev);
> > #endif
> > + v4l2_ctrl_handler_free(gspca_dev->vdev.ctrl_handler);
> > kfree(gspca_dev->usb_buf);
> > kfree(gspca_dev);
> > return ret;
>
> Otherwise looks good, I've added it to my local tree (with the
> described change), and will include it in my next pullreq.
>
> 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: [RFCv1 PATCH 1/7] gspca: allow subdrivers to use the control framework.
Hi,
I'm slowly working my way though this series today (both review, as well
as some tweaks and testing).
More comments inline...
On 04/28/2012 05:09 PM, Hans Verkuil wrote:
From: Hans Verkuil
Make the necessary changes to allow subdrivers to use the control framework.
This does not add control event support, that needs more work.
Signed-off-by: Hans Verkuil
---
drivers/media/video/gspca/gspca.c | 13 +
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/media/video/gspca/gspca.c
b/drivers/media/video/gspca/gspca.c
index ca5a2b1..56dff10 100644
--- a/drivers/media/video/gspca/gspca.c
+++ b/drivers/media/video/gspca/gspca.c
@@ -38,6 +38,7 @@
#include
#include
#include
+#include
#include "gspca.h"
@@ -1006,6 +1007,8 @@ static void gspca_set_default_mode(struct gspca_dev
*gspca_dev)
/* set the current control values to their default values
* which may have changed in sd_init() */
+ /* does nothing if ctrl_handler == NULL */
+ v4l2_ctrl_handler_setup(gspca_dev->vdev.ctrl_handler);
ctrl = gspca_dev->cam.ctrls;
if (ctrl != NULL) {
for (i = 0;
@@ -1323,6 +1326,7 @@ static void gspca_release(struct video_device *vfd)
PDEBUG(D_PROBE, "%s released",
video_device_node_name(&gspca_dev->vdev));
+ v4l2_ctrl_handler_free(gspca_dev->vdev.ctrl_handler);
kfree(gspca_dev->usb_buf);
kfree(gspca_dev);
}
@@ -2347,6 +2351,10 @@ int gspca_dev_probe2(struct usb_interface *intf,
gspca_dev->sd_desc = sd_desc;
gspca_dev->nbufread = 2;
gspca_dev->empty_packet = -1;/* don't check the empty packets */
+ gspca_dev->vdev = gspca_template;
+ gspca_dev->vdev.parent =&intf->dev;
+ gspca_dev->module = module;
+ gspca_dev->present = 1;
/* configure the subdriver and initialize the USB device */
ret = sd_desc->config(gspca_dev, id);
You also need to move the initialization of the mutexes here, as the
v4l2_ctrl_handler_setup will call s_ctrl on all the controls, and s_ctrl
should take the usb_lock (see my review of the next patch in this series),
I'll make this change myself and merge it into your patch.
@@ -2368,10 +2376,6 @@ int gspca_dev_probe2(struct usb_interface *intf,
init_waitqueue_head(&gspca_dev->wq);
/* init video stuff */
- memcpy(&gspca_dev->vdev,&gspca_template, sizeof gspca_template);
- gspca_dev->vdev.parent =&intf->dev;
- gspca_dev->module = module;
- gspca_dev->present = 1;
ret = video_register_device(&gspca_dev->vdev,
VFL_TYPE_GRABBER,
-1);
@@ -2391,6 +2395,7 @@ out:
if (gspca_dev->input_dev)
input_unregister_device(gspca_dev->input_dev);
#endif
+ v4l2_ctrl_handler_free(gspca_dev->vdev.ctrl_handler);
kfree(gspca_dev->usb_buf);
kfree(gspca_dev);
return ret;
Otherwise looks good, I've added it to my local tree (with the
described change), and will include it in my next pullreq.
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
[RFCv1 PATCH 1/7] gspca: allow subdrivers to use the control framework.
From: Hans Verkuil
Make the necessary changes to allow subdrivers to use the control framework.
This does not add control event support, that needs more work.
Signed-off-by: Hans Verkuil
---
drivers/media/video/gspca/gspca.c | 13 +
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/media/video/gspca/gspca.c
b/drivers/media/video/gspca/gspca.c
index ca5a2b1..56dff10 100644
--- a/drivers/media/video/gspca/gspca.c
+++ b/drivers/media/video/gspca/gspca.c
@@ -38,6 +38,7 @@
#include
#include
#include
+#include
#include "gspca.h"
@@ -1006,6 +1007,8 @@ static void gspca_set_default_mode(struct gspca_dev
*gspca_dev)
/* set the current control values to their default values
* which may have changed in sd_init() */
+ /* does nothing if ctrl_handler == NULL */
+ v4l2_ctrl_handler_setup(gspca_dev->vdev.ctrl_handler);
ctrl = gspca_dev->cam.ctrls;
if (ctrl != NULL) {
for (i = 0;
@@ -1323,6 +1326,7 @@ static void gspca_release(struct video_device *vfd)
PDEBUG(D_PROBE, "%s released",
video_device_node_name(&gspca_dev->vdev));
+ v4l2_ctrl_handler_free(gspca_dev->vdev.ctrl_handler);
kfree(gspca_dev->usb_buf);
kfree(gspca_dev);
}
@@ -2347,6 +2351,10 @@ int gspca_dev_probe2(struct usb_interface *intf,
gspca_dev->sd_desc = sd_desc;
gspca_dev->nbufread = 2;
gspca_dev->empty_packet = -1; /* don't check the empty packets */
+ gspca_dev->vdev = gspca_template;
+ gspca_dev->vdev.parent = &intf->dev;
+ gspca_dev->module = module;
+ gspca_dev->present = 1;
/* configure the subdriver and initialize the USB device */
ret = sd_desc->config(gspca_dev, id);
@@ -2368,10 +2376,6 @@ int gspca_dev_probe2(struct usb_interface *intf,
init_waitqueue_head(&gspca_dev->wq);
/* init video stuff */
- memcpy(&gspca_dev->vdev, &gspca_template, sizeof gspca_template);
- gspca_dev->vdev.parent = &intf->dev;
- gspca_dev->module = module;
- gspca_dev->present = 1;
ret = video_register_device(&gspca_dev->vdev,
VFL_TYPE_GRABBER,
-1);
@@ -2391,6 +2395,7 @@ out:
if (gspca_dev->input_dev)
input_unregister_device(gspca_dev->input_dev);
#endif
+ v4l2_ctrl_handler_free(gspca_dev->vdev.ctrl_handler);
kfree(gspca_dev->usb_buf);
kfree(gspca_dev);
return ret;
--
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
