Re: [GIT PULL FOR 2.6.37] uvcvideo: BKL removal
On Thursday 23 December 2010 09:34:27 Laurent Pinchart wrote: Hi Mauro, On Tuesday 21 December 2010 12:10:18 Mauro Carvalho Chehab wrote: You didn't understand me: uvcvideo is returning -EBUSY to format changes with buffers freed. As explained in my answer to Hans, that's on purpose. The uvcvideo driver releases buffers when calling REQBUFS(0). However, the file handle is still marked as owning the device for streaming purpose, so other applications can't change the format or request buffers. The reason for that is to avoid race conditions when an application wants to change the resolution during capture. As the application has to stop capture, call REQBUFS(0), change the format, request buffers and restart capture, it prevents another application from racing it after REQBUFS(0). And it's actually not correct. I've just tested the latest uvcvideo version, and I can start capture in a second application after calling REQBUFS(0) in the first one. As Hans correctly pointed out, this should be implemented using the priority ioctls. I will fix that. -- Regards, Laurent Pinchart -- 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: [GIT PULL FOR 2.6.37] uvcvideo: BKL removal
Hi Hans, On Monday 20 December 2010 14:09:40 Hans Verkuil wrote: On Monday, December 20, 2010 13:48:51 Laurent Pinchart wrote: What if the application wants to change the resolution during capture ? It will have to stop capture, call REQBUFS(0), change the format, request buffers and restart capture. If filehandle ownership is dropped after REQBUFS(0) that will open the door to a race condition. That's why S_PRIORITY was invented. Right, I should implement that. I think the documentation isn't clear though. What is the background priority for exactly ? And the unset priority ? Are other applications allowed to change controls when an application has the record priority ? In general I find the priority ioctls underspecified, that's why I haven't implemented them yet. On a side note, I've just tested the latest uvcvideo driver, and I've successfully captured video using a second application after calling REQBUFS(0) in a first application. -- Regards, Laurent Pinchart -- 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: [GIT PULL FOR 2.6.37] uvcvideo: BKL removal
On Thursday, December 23, 2010 10:02:33 Laurent Pinchart wrote: Hi Hans, On Monday 20 December 2010 14:09:40 Hans Verkuil wrote: On Monday, December 20, 2010 13:48:51 Laurent Pinchart wrote: What if the application wants to change the resolution during capture ? It will have to stop capture, call REQBUFS(0), change the format, request buffers and restart capture. If filehandle ownership is dropped after REQBUFS(0) that will open the door to a race condition. That's why S_PRIORITY was invented. Right, I should implement that. I think the documentation isn't clear though. What is the background priority for exactly ? As the documentation mentions, it can be used for background processes monitoring VBI (e.g. teletext) transmissions. I'm not aware of any such applications, though. PRIORITY_DEFAULT and PRIORITY_RECORD are the only two relevant prios in practice. And the unset priority ? Internal prio only. I think it's the value when no file handle is open. Are other applications allowed to change controls when an application has the record priority ? No. Only read-only ioctls can be executed. In general I find the priority ioctls underspecified, that's why I haven't implemented them yet. Use the prio support functions in v4l2-common. They are easy to use and do the job. Regards, Hans On a side note, I've just tested the latest uvcvideo driver, and I've successfully captured video using a second application after calling REQBUFS(0) in a first application. -- Hans Verkuil - video4linux developer - sponsored by Cisco -- 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: [GIT PULL FOR 2.6.37] uvcvideo: BKL removal
Hi Hans, On Thursday 23 December 2010 10:20:17 Hans Verkuil wrote: On Thursday, December 23, 2010 10:02:33 Laurent Pinchart wrote: On Monday 20 December 2010 14:09:40 Hans Verkuil wrote: On Monday, December 20, 2010 13:48:51 Laurent Pinchart wrote: What if the application wants to change the resolution during capture ? It will have to stop capture, call REQBUFS(0), change the format, request buffers and restart capture. If filehandle ownership is dropped after REQBUFS(0) that will open the door to a race condition. That's why S_PRIORITY was invented. Right, I should implement that. I think the documentation isn't clear though. What is the background priority for exactly ? As the documentation mentions, it can be used for background processes monitoring VBI (e.g. teletext) transmissions. I'm not aware of any such applications, though. PRIORITY_DEFAULT and PRIORITY_RECORD are the only two relevant prios in practice. And the unset priority ? Internal prio only. I think it's the value when no file handle is open. Aren't priorities associated with file handles ? Are other applications allowed to change controls when an application has the record priority ? No. Only read-only ioctls can be executed. Then we got an issue here. I want an application to be able to acquire exclusive streaming rights on the device (so that there won't be race conditions when changing the resolution), but still allow other applications to change controls. In general I find the priority ioctls underspecified, that's why I haven't implemented them yet. Use the prio support functions in v4l2-common. They are easy to use and do the job. -- Regards, Laurent Pinchart -- 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: [GIT PULL FOR 2.6.37] uvcvideo: BKL removal
On Thursday, December 23, 2010 10:27:25 Laurent Pinchart wrote: Hi Hans, On Thursday 23 December 2010 10:20:17 Hans Verkuil wrote: On Thursday, December 23, 2010 10:02:33 Laurent Pinchart wrote: On Monday 20 December 2010 14:09:40 Hans Verkuil wrote: On Monday, December 20, 2010 13:48:51 Laurent Pinchart wrote: What if the application wants to change the resolution during capture ? It will have to stop capture, call REQBUFS(0), change the format, request buffers and restart capture. If filehandle ownership is dropped after REQBUFS(0) that will open the door to a race condition. That's why S_PRIORITY was invented. Right, I should implement that. I think the documentation isn't clear though. What is the background priority for exactly ? As the documentation mentions, it can be used for background processes monitoring VBI (e.g. teletext) transmissions. I'm not aware of any such applications, though. PRIORITY_DEFAULT and PRIORITY_RECORD are the only two relevant prios in practice. And the unset priority ? Internal prio only. I think it's the value when no file handle is open. Aren't priorities associated with file handles ? Yes, but there is also a global prio. Are other applications allowed to change controls when an application has the record priority ? No. Only read-only ioctls can be executed. Then we got an issue here. I want an application to be able to acquire exclusive streaming rights on the device (so that there won't be race conditions when changing the resolution), but still allow other applications to change controls. Why? The whole idea of prios is that once you are PRIO_RECORD no one else can mess with settings. Allowing other apps access to controls will make it possible to e.g. change the contrast to some crappy value. Not acceptable. If the only thing you want to use PRIO_RECORD for in apps is to prevent this 'race condition' (I don't really see this as a real race, to be honest), then the app can raise the prio to RECORD just before STREAMOFF, change the resolution, start streaming again and lower it to the default prio. Regards, Hans In general I find the priority ioctls underspecified, that's why I haven't implemented them yet. Use the prio support functions in v4l2-common. They are easy to use and do the job. -- Hans Verkuil - video4linux developer - sponsored by Cisco -- 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: [GIT PULL FOR 2.6.37] uvcvideo: BKL removal
On Thu, 2010-12-23 at 10:02 +0100, Laurent Pinchart wrote:
Hi Hans,
On Monday 20 December 2010 14:09:40 Hans Verkuil wrote:
On Monday, December 20, 2010 13:48:51 Laurent Pinchart wrote:
What if the application wants to change the resolution during capture ?
It will have to stop capture, call REQBUFS(0), change the format,
request buffers and restart capture. If filehandle ownership is dropped
after REQBUFS(0) that will open the door to a race condition.
That's why S_PRIORITY was invented.
Right, I should implement that. I think the documentation isn't clear though.
What is the background priority for exactly ?
http://linuxtv.org/downloads/v4l-dvb-apis/app-pri.html
Another objective is to permit low priority applications working in
background, which can be preempted by user controlled applications and
automatically regain control of the device at a later time.
contrived examples
A use case would be for a daemon that does background channel scanning
or VBI data collection when the user isn't doing something else with the
video device.
For a camera, maybe it would be an application that does device health
monitoring, some sort of continuous calibration, or motion detection
when the device isn't in use by the user for viewing.
/contrived examples
And the unset priority ?
When checking for the maximum priority in use, it indicates that no
nodes have any priorities. See v4l2_prio_max(). For a driver that
supports priorities, it means no device nodes are opened, since any
device node being open would get a priority of
V4L2_PRIORITY_INTERACTIVE/_DEFAULT by default in v4l2_prio_open().
V4L2_PRIORITY_UNSET is also used to indicate to the v4l2_prio_*()
functions that a struct v4l2_prio_state hasn't been initialized (i.e.
has just been kzalloc()'ed).
Are
other applications allowed to change controls when an application has the
record priority ?
According to the spec, no:
V4L2_PRIORITY_RECORD 3 Highest priority. Only one file descriptor can
have this priority, it blocks any other fd from changing device
properties.
Once an automated, scheduled-recording process is recording, one really
wouldn't want another process changing them. I personally would not
like unpredictable volume level variations on my TV show recordings.
In cx18-controls.c one can find an implementation example for the old
control framework:
int cx18_s_ext_ctrls(struct file *file, void *fh, struct
v4l2_ext_controls *c)
{
struct cx18_open_id *id = fh;
struct cx18 *cx = id-cx;
int ret;
struct v4l2_control ctrl;
ret = v4l2_prio_check(cx-prio, id-prio);
if (ret)
return ret;
[...]
However, the new control framework in v4l2-ctrl.c seems to be missing
any v4l2_prio_check() calls. The ioctl() handling in v4l2-ioctl.c
doesn't have any either.
In general I find the priority ioctls underspecified, that's why I haven't
implemented them yet.
Yes, it appears some of the details should be gleaned from the existing
driver implementations before they are/were converted to the new control
framework.
Regards,
Andy
--
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: [GIT PULL FOR 2.6.37] uvcvideo: BKL removal
On Thursday, December 23, 2010 14:04:38 Andy Walls wrote:
On Thu, 2010-12-23 at 10:02 +0100, Laurent Pinchart wrote:
Hi Hans,
On Monday 20 December 2010 14:09:40 Hans Verkuil wrote:
On Monday, December 20, 2010 13:48:51 Laurent Pinchart wrote:
What if the application wants to change the resolution during capture ?
It will have to stop capture, call REQBUFS(0), change the format,
request buffers and restart capture. If filehandle ownership is dropped
after REQBUFS(0) that will open the door to a race condition.
That's why S_PRIORITY was invented.
Right, I should implement that. I think the documentation isn't clear
though.
What is the background priority for exactly ?
http://linuxtv.org/downloads/v4l-dvb-apis/app-pri.html
Another objective is to permit low priority applications working in
background, which can be preempted by user controlled applications and
automatically regain control of the device at a later time.
contrived examples
A use case would be for a daemon that does background channel scanning
or VBI data collection when the user isn't doing something else with the
video device.
For a camera, maybe it would be an application that does device health
monitoring, some sort of continuous calibration, or motion detection
when the device isn't in use by the user for viewing.
/contrived examples
And the unset priority ?
When checking for the maximum priority in use, it indicates that no
nodes have any priorities. See v4l2_prio_max(). For a driver that
supports priorities, it means no device nodes are opened, since any
device node being open would get a priority of
V4L2_PRIORITY_INTERACTIVE/_DEFAULT by default in v4l2_prio_open().
V4L2_PRIORITY_UNSET is also used to indicate to the v4l2_prio_*()
functions that a struct v4l2_prio_state hasn't been initialized (i.e.
has just been kzalloc()'ed).
Are
other applications allowed to change controls when an application has the
record priority ?
According to the spec, no:
V4L2_PRIORITY_RECORD 3 Highest priority. Only one file descriptor can
have this priority, it blocks any other fd from changing device
properties.
Once an automated, scheduled-recording process is recording, one really
wouldn't want another process changing them. I personally would not
like unpredictable volume level variations on my TV show recordings.
In cx18-controls.c one can find an implementation example for the old
control framework:
int cx18_s_ext_ctrls(struct file *file, void *fh, struct
v4l2_ext_controls *c)
{
struct cx18_open_id *id = fh;
struct cx18 *cx = id-cx;
int ret;
struct v4l2_control ctrl;
ret = v4l2_prio_check(cx-prio, id-prio);
if (ret)
return ret;
[...]
However, the new control framework in v4l2-ctrl.c seems to be missing
any v4l2_prio_check() calls. The ioctl() handling in v4l2-ioctl.c
doesn't have any either.
Oops, I'd forgotten about that. The priority support really should be
moved into the v4l2 core framework where it belongs. I'll see if I can spend
some time on that after Christmas.
Regards,
Hans
--
Hans Verkuil - video4linux developer - sponsored by Cisco
--
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: [GIT PULL FOR 2.6.37] uvcvideo: BKL removal
Em 20-12-2010 10:28, Hans Verkuil escreveu: On Monday, December 20, 2010 13:10:32 Mauro Carvalho Chehab wrote: Em 18-12-2010 08:45, Hans Verkuil escreveu: On Saturday, December 18, 2010 01:54:41 Laurent Pinchart wrote: Hi Mauro, On Friday 17 December 2010 18:09:39 Mauro Carvalho Chehab wrote: Em 14-12-2010 08:55, Laurent Pinchart escreveu: Hi Mauro, Please don't forget this pull request for 2.6.37. Pull request for upstream sent today. Thank you. I didn't find any regressions at the BKL removal patches, but I noticed a few issues with qv4l2, not all related to uvcvideo. The remaining of this email is an attempt to document them for later fixes. They don't seem to be regressions caused by BKL removal, but the better would be to fix them later. - with uvcvideo and two video apps, if qv4l2 is started first, the second application doesn't start/capture. I suspect that REQBUFS (used by qv4l2 to probe mmap/userptr capabilities) create some resource locking at uvcvideo. The proper way is to lock the resources only if the driver is streaming, as other drivers and videobuf do. I don't agree with that. The uvcvideo driver has one buffer queue per device, so if an application requests buffers on one file handle it will lock other applications out. If the driver didn't it would be subject to race conditions. I agree with Laurent. Once an application calls REQBUFS with non-zero count, then it should lock the resources needed for streaming. The reason behind that is that REQBUFS also locks the current selected format in place, since the format determines the amount of memory needed for the buffers. qv4l2 calls REQBUFS(1), then REQBUFS(0). Well, this is currently wrong, as most drivers will only release buffers at VIDIOC_STREAMOFF. qv4l2 first calls STREAMOFF, then REQBUFS(1), then REQBUFS(0). In the hope that one of these will actually free any buffers. It's random at the moment when drivers release buffers, one of the reasons for using vb2. This won't free buffers. REQBUFS(0) is not honored. so, what you're doing is really STREAMOFF/REQBUFS(1). It would work fine if you change the order to: REQBUFS(1) REQBUFS(0) STREAMOFF Anyway, even replacing REQBUFS(0) with VIDIOC_STREAMOFF at qv4l2 won't help with uvcvideo. It seems that, once buffers are requested at uvcvideo, they will release only at close(). One consequence on the way uvcvideo is currently doing it is that, if you use qv4l2, it is impossible to change the video size, as it returns -EBUSY, if you ask it to select a different format (even without streaming): $ ./qv4l2 Set Capture Format: Device or resource busy The reason a lot of drivers don't do this is partially because for many TV capture drivers it is highly unlikely that the buffer size will change after calling REQBUFS (there are basically only two formats: 720x480 or 720x576 and users will normally never change between the two). However, this is much more likely to happen for webcams and embedded systems supporting HDTV. What applications do, when they need to change the formats, is to call REQBUFS again. The other reason is probably because driver developers simple do not realize they need to lock the resources on REQBUFS. I'm sure many existing drivers will fail miserably if you change the format after calling REQBUFS (particularly with mmap streaming mode). I didn't make any test, but I don't think they'll fail (at least, on the drivers that use videobuf), as streaming format will be stored at the videobuf handling (at buffer_prepare callback). So, if you change the format, the change will be applied only at the next call to REQBUFS. This behavior isn't according to the spec. G/S_FMT relate to the current format, not to some future format. Most non-videobuf drivers will not support this behavior. It should be simple, really: STREAMOFF REQBUFS(0) That's all that should be needed to stop streaming and return all buffers to the app. This is what uvc should also support (and I actually thought it did). Attempts to change formats while buffers have been requested should be blocked with EBUSY. It's all perfectly reasonable. Well, perhaps next year we might succeed in having all drivers behave consistently... You didn't understand me: uvcvideo is returning -EBUSY to format changes with buffers freed. If I understood well, during probe time, qv4l2 calls REQBUFS(1)/REQBUFS(0) for both userptr and mmap, in order to detect what the device supports. This takes some locking schema inside uvcvideo, but, as uvcvideo is only releasing such lock at close(), it is impossible to change the video format. I even added an extra STREAMOFF at qv4l2 code, but this didn't solve the issue. Well, maybe better than explaining it, you should try to change the video image size or format with qv4l2 and an uvc camera ;) Cheers, Mauro -- To unsubscribe from this list: send the
Re: [GIT PULL FOR 2.6.37] uvcvideo: BKL removal
Em 18-12-2010 08:45, Hans Verkuil escreveu: On Saturday, December 18, 2010 01:54:41 Laurent Pinchart wrote: Hi Mauro, On Friday 17 December 2010 18:09:39 Mauro Carvalho Chehab wrote: Em 14-12-2010 08:55, Laurent Pinchart escreveu: Hi Mauro, Please don't forget this pull request for 2.6.37. Pull request for upstream sent today. Thank you. I didn't find any regressions at the BKL removal patches, but I noticed a few issues with qv4l2, not all related to uvcvideo. The remaining of this email is an attempt to document them for later fixes. They don't seem to be regressions caused by BKL removal, but the better would be to fix them later. - with uvcvideo and two video apps, if qv4l2 is started first, the second application doesn't start/capture. I suspect that REQBUFS (used by qv4l2 to probe mmap/userptr capabilities) create some resource locking at uvcvideo. The proper way is to lock the resources only if the driver is streaming, as other drivers and videobuf do. I don't agree with that. The uvcvideo driver has one buffer queue per device, so if an application requests buffers on one file handle it will lock other applications out. If the driver didn't it would be subject to race conditions. I agree with Laurent. Once an application calls REQBUFS with non-zero count, then it should lock the resources needed for streaming. The reason behind that is that REQBUFS also locks the current selected format in place, since the format determines the amount of memory needed for the buffers. qv4l2 calls REQBUFS(1), then REQBUFS(0). Well, this is currently wrong, as most drivers will only release buffers at VIDIOC_STREAMOFF. Anyway, even replacing REQBUFS(0) with VIDIOC_STREAMOFF at qv4l2 won't help with uvcvideo. It seems that, once buffers are requested at uvcvideo, they will release only at close(). One consequence on the way uvcvideo is currently doing it is that, if you use qv4l2, it is impossible to change the video size, as it returns -EBUSY, if you ask it to select a different format (even without streaming): $ ./qv4l2 Set Capture Format: Device or resource busy The reason a lot of drivers don't do this is partially because for many TV capture drivers it is highly unlikely that the buffer size will change after calling REQBUFS (there are basically only two formats: 720x480 or 720x576 and users will normally never change between the two). However, this is much more likely to happen for webcams and embedded systems supporting HDTV. What applications do, when they need to change the formats, is to call REQBUFS again. The other reason is probably because driver developers simple do not realize they need to lock the resources on REQBUFS. I'm sure many existing drivers will fail miserably if you change the format after calling REQBUFS (particularly with mmap streaming mode). I didn't make any test, but I don't think they'll fail (at least, on the drivers that use videobuf), as streaming format will be stored at the videobuf handling (at buffer_prepare callback). So, if you change the format, the change will be applied only at the next call to REQBUFS. Cheers, Mauro -- 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: [GIT PULL FOR 2.6.37] uvcvideo: BKL removal
On Monday, December 20, 2010 13:10:32 Mauro Carvalho Chehab wrote: Em 18-12-2010 08:45, Hans Verkuil escreveu: On Saturday, December 18, 2010 01:54:41 Laurent Pinchart wrote: Hi Mauro, On Friday 17 December 2010 18:09:39 Mauro Carvalho Chehab wrote: Em 14-12-2010 08:55, Laurent Pinchart escreveu: Hi Mauro, Please don't forget this pull request for 2.6.37. Pull request for upstream sent today. Thank you. I didn't find any regressions at the BKL removal patches, but I noticed a few issues with qv4l2, not all related to uvcvideo. The remaining of this email is an attempt to document them for later fixes. They don't seem to be regressions caused by BKL removal, but the better would be to fix them later. - with uvcvideo and two video apps, if qv4l2 is started first, the second application doesn't start/capture. I suspect that REQBUFS (used by qv4l2 to probe mmap/userptr capabilities) create some resource locking at uvcvideo. The proper way is to lock the resources only if the driver is streaming, as other drivers and videobuf do. I don't agree with that. The uvcvideo driver has one buffer queue per device, so if an application requests buffers on one file handle it will lock other applications out. If the driver didn't it would be subject to race conditions. I agree with Laurent. Once an application calls REQBUFS with non-zero count, then it should lock the resources needed for streaming. The reason behind that is that REQBUFS also locks the current selected format in place, since the format determines the amount of memory needed for the buffers. qv4l2 calls REQBUFS(1), then REQBUFS(0). Well, this is currently wrong, as most drivers will only release buffers at VIDIOC_STREAMOFF. qv4l2 first calls STREAMOFF, then REQBUFS(1), then REQBUFS(0). In the hope that one of these will actually free any buffers. It's random at the moment when drivers release buffers, one of the reasons for using vb2. Anyway, even replacing REQBUFS(0) with VIDIOC_STREAMOFF at qv4l2 won't help with uvcvideo. It seems that, once buffers are requested at uvcvideo, they will release only at close(). One consequence on the way uvcvideo is currently doing it is that, if you use qv4l2, it is impossible to change the video size, as it returns -EBUSY, if you ask it to select a different format (even without streaming): $ ./qv4l2 Set Capture Format: Device or resource busy The reason a lot of drivers don't do this is partially because for many TV capture drivers it is highly unlikely that the buffer size will change after calling REQBUFS (there are basically only two formats: 720x480 or 720x576 and users will normally never change between the two). However, this is much more likely to happen for webcams and embedded systems supporting HDTV. What applications do, when they need to change the formats, is to call REQBUFS again. The other reason is probably because driver developers simple do not realize they need to lock the resources on REQBUFS. I'm sure many existing drivers will fail miserably if you change the format after calling REQBUFS (particularly with mmap streaming mode). I didn't make any test, but I don't think they'll fail (at least, on the drivers that use videobuf), as streaming format will be stored at the videobuf handling (at buffer_prepare callback). So, if you change the format, the change will be applied only at the next call to REQBUFS. This behavior isn't according to the spec. G/S_FMT relate to the current format, not to some future format. Most non-videobuf drivers will not support this behavior. It should be simple, really: STREAMOFF REQBUFS(0) That's all that should be needed to stop streaming and return all buffers to the app. This is what uvc should also support (and I actually thought it did). Attempts to change formats while buffers have been requested should be blocked with EBUSY. It's all perfectly reasonable. Well, perhaps next year we might succeed in having all drivers behave consistently... Regards, Hans -- Hans Verkuil - video4linux developer - sponsored by Cisco -- 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: [GIT PULL FOR 2.6.37] uvcvideo: BKL removal
Hi Hans, On Monday 20 December 2010 13:28:06 Hans Verkuil wrote: On Monday, December 20, 2010 13:10:32 Mauro Carvalho Chehab wrote: Em 18-12-2010 08:45, Hans Verkuil escreveu: On Saturday, December 18, 2010 01:54:41 Laurent Pinchart wrote: On Friday 17 December 2010 18:09:39 Mauro Carvalho Chehab wrote: I didn't find any regressions at the BKL removal patches, but I noticed a few issues with qv4l2, not all related to uvcvideo. The remaining of this email is an attempt to document them for later fixes. They don't seem to be regressions caused by BKL removal, but the better would be to fix them later. - with uvcvideo and two video apps, if qv4l2 is started first, the second application doesn't start/capture. I suspect that REQBUFS (used by qv4l2 to probe mmap/userptr capabilities) create some resource locking at uvcvideo. The proper way is to lock the resources only if the driver is streaming, as other drivers and videobuf do. I don't agree with that. The uvcvideo driver has one buffer queue per device, so if an application requests buffers on one file handle it will lock other applications out. If the driver didn't it would be subject to race conditions. I agree with Laurent. Once an application calls REQBUFS with non-zero count, then it should lock the resources needed for streaming. The reason behind that is that REQBUFS also locks the current selected format in place, since the format determines the amount of memory needed for the buffers. qv4l2 calls REQBUFS(1), then REQBUFS(0). Well, this is currently wrong, as most drivers will only release buffers at VIDIOC_STREAMOFF. qv4l2 first calls STREAMOFF, then REQBUFS(1), then REQBUFS(0). In the hope that one of these will actually free any buffers. It's random at the moment when drivers release buffers, one of the reasons for using vb2. Anyway, even replacing REQBUFS(0) with VIDIOC_STREAMOFF at qv4l2 won't help with uvcvideo. It seems that, once buffers are requested at uvcvideo, they will release only at close(). That's not correct. Buffers are released when calling REQBUFS(0). However, the file handle is still marked as owning the device for streaming purpose, so other applications can't change the format or request buffers. One consequence on the way uvcvideo is currently doing it is that, if you use qv4l2, it is impossible to change the video size, as it returns -EBUSY, if you ask it to select a different format (even without streaming): $ ./qv4l2 Set Capture Format: Device or resource busy The reason a lot of drivers don't do this is partially because for many TV capture drivers it is highly unlikely that the buffer size will change after calling REQBUFS (there are basically only two formats: 720x480 or 720x576 and users will normally never change between the two). However, this is much more likely to happen for webcams and embedded systems supporting HDTV. What applications do, when they need to change the formats, is to call REQBUFS again. The other reason is probably because driver developers simple do not realize they need to lock the resources on REQBUFS. I'm sure many existing drivers will fail miserably if you change the format after calling REQBUFS (particularly with mmap streaming mode). I didn't make any test, but I don't think they'll fail (at least, on the drivers that use videobuf), as streaming format will be stored at the videobuf handling (at buffer_prepare callback). So, if you change the format, the change will be applied only at the next call to REQBUFS. This behavior isn't according to the spec. G/S_FMT relate to the current format, not to some future format. Most non-videobuf drivers will not support this behavior. It should be simple, really: STREAMOFF REQBUFS(0) That's all that should be needed to stop streaming and return all buffers to the app. This is what uvc should also support (and I actually thought it did). That's what uvcvideo does. Attempts to change formats while buffers have been requested should be blocked with EBUSY. It's all perfectly reasonable. Well, perhaps next year we might succeed in having all drivers behave consistently... -- Regards, Laurent Pinchart -- 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: [GIT PULL FOR 2.6.37] uvcvideo: BKL removal
On Monday, December 20, 2010 13:35:28 Laurent Pinchart wrote: Hi Hans, On Monday 20 December 2010 13:28:06 Hans Verkuil wrote: On Monday, December 20, 2010 13:10:32 Mauro Carvalho Chehab wrote: Em 18-12-2010 08:45, Hans Verkuil escreveu: On Saturday, December 18, 2010 01:54:41 Laurent Pinchart wrote: On Friday 17 December 2010 18:09:39 Mauro Carvalho Chehab wrote: I didn't find any regressions at the BKL removal patches, but I noticed a few issues with qv4l2, not all related to uvcvideo. The remaining of this email is an attempt to document them for later fixes. They don't seem to be regressions caused by BKL removal, but the better would be to fix them later. - with uvcvideo and two video apps, if qv4l2 is started first, the second application doesn't start/capture. I suspect that REQBUFS (used by qv4l2 to probe mmap/userptr capabilities) create some resource locking at uvcvideo. The proper way is to lock the resources only if the driver is streaming, as other drivers and videobuf do. I don't agree with that. The uvcvideo driver has one buffer queue per device, so if an application requests buffers on one file handle it will lock other applications out. If the driver didn't it would be subject to race conditions. I agree with Laurent. Once an application calls REQBUFS with non-zero count, then it should lock the resources needed for streaming. The reason behind that is that REQBUFS also locks the current selected format in place, since the format determines the amount of memory needed for the buffers. qv4l2 calls REQBUFS(1), then REQBUFS(0). Well, this is currently wrong, as most drivers will only release buffers at VIDIOC_STREAMOFF. qv4l2 first calls STREAMOFF, then REQBUFS(1), then REQBUFS(0). In the hope that one of these will actually free any buffers. It's random at the moment when drivers release buffers, one of the reasons for using vb2. Anyway, even replacing REQBUFS(0) with VIDIOC_STREAMOFF at qv4l2 won't help with uvcvideo. It seems that, once buffers are requested at uvcvideo, they will release only at close(). That's not correct. Buffers are released when calling REQBUFS(0). However, the file handle is still marked as owning the device for streaming purpose, so other applications can't change the format or request buffers. Why? After REQBUFS(0) any filehandle ownership should be dropped. Regards, Hans -- Hans Verkuil - video4linux developer - sponsored by Cisco -- 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: [GIT PULL FOR 2.6.37] uvcvideo: BKL removal
Hi Hans, On Monday 20 December 2010 13:45:45 Hans Verkuil wrote: On Monday, December 20, 2010 13:35:28 Laurent Pinchart wrote: On Monday 20 December 2010 13:28:06 Hans Verkuil wrote: On Monday, December 20, 2010 13:10:32 Mauro Carvalho Chehab wrote: Em 18-12-2010 08:45, Hans Verkuil escreveu: On Saturday, December 18, 2010 01:54:41 Laurent Pinchart wrote: On Friday 17 December 2010 18:09:39 Mauro Carvalho Chehab wrote: I didn't find any regressions at the BKL removal patches, but I noticed a few issues with qv4l2, not all related to uvcvideo. The remaining of this email is an attempt to document them for later fixes. They don't seem to be regressions caused by BKL removal, but the better would be to fix them later. - with uvcvideo and two video apps, if qv4l2 is started first, the second application doesn't start/capture. I suspect that REQBUFS (used by qv4l2 to probe mmap/userptr capabilities) create some resource locking at uvcvideo. The proper way is to lock the resources only if the driver is streaming, as other drivers and videobuf do. I don't agree with that. The uvcvideo driver has one buffer queue per device, so if an application requests buffers on one file handle it will lock other applications out. If the driver didn't it would be subject to race conditions. I agree with Laurent. Once an application calls REQBUFS with non-zero count, then it should lock the resources needed for streaming. The reason behind that is that REQBUFS also locks the current selected format in place, since the format determines the amount of memory needed for the buffers. qv4l2 calls REQBUFS(1), then REQBUFS(0). Well, this is currently wrong, as most drivers will only release buffers at VIDIOC_STREAMOFF. qv4l2 first calls STREAMOFF, then REQBUFS(1), then REQBUFS(0). In the hope that one of these will actually free any buffers. It's random at the moment when drivers release buffers, one of the reasons for using vb2. Anyway, even replacing REQBUFS(0) with VIDIOC_STREAMOFF at qv4l2 won't help with uvcvideo. It seems that, once buffers are requested at uvcvideo, they will release only at close(). That's not correct. Buffers are released when calling REQBUFS(0). However, the file handle is still marked as owning the device for streaming purpose, so other applications can't change the format or request buffers. Why? After REQBUFS(0) any filehandle ownership should be dropped. What if the application wants to change the resolution during capture ? It will have to stop capture, call REQBUFS(0), change the format, request buffers and restart capture. If filehandle ownership is dropped after REQBUFS(0) that will open the door to a race condition. -- Regards, Laurent Pinchart -- 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: [GIT PULL FOR 2.6.37] uvcvideo: BKL removal
On Monday, December 20, 2010 13:48:51 Laurent Pinchart wrote: Hi Hans, On Monday 20 December 2010 13:45:45 Hans Verkuil wrote: On Monday, December 20, 2010 13:35:28 Laurent Pinchart wrote: On Monday 20 December 2010 13:28:06 Hans Verkuil wrote: On Monday, December 20, 2010 13:10:32 Mauro Carvalho Chehab wrote: Em 18-12-2010 08:45, Hans Verkuil escreveu: On Saturday, December 18, 2010 01:54:41 Laurent Pinchart wrote: On Friday 17 December 2010 18:09:39 Mauro Carvalho Chehab wrote: I didn't find any regressions at the BKL removal patches, but I noticed a few issues with qv4l2, not all related to uvcvideo. The remaining of this email is an attempt to document them for later fixes. They don't seem to be regressions caused by BKL removal, but the better would be to fix them later. - with uvcvideo and two video apps, if qv4l2 is started first, the second application doesn't start/capture. I suspect that REQBUFS (used by qv4l2 to probe mmap/userptr capabilities) create some resource locking at uvcvideo. The proper way is to lock the resources only if the driver is streaming, as other drivers and videobuf do. I don't agree with that. The uvcvideo driver has one buffer queue per device, so if an application requests buffers on one file handle it will lock other applications out. If the driver didn't it would be subject to race conditions. I agree with Laurent. Once an application calls REQBUFS with non-zero count, then it should lock the resources needed for streaming. The reason behind that is that REQBUFS also locks the current selected format in place, since the format determines the amount of memory needed for the buffers. qv4l2 calls REQBUFS(1), then REQBUFS(0). Well, this is currently wrong, as most drivers will only release buffers at VIDIOC_STREAMOFF. qv4l2 first calls STREAMOFF, then REQBUFS(1), then REQBUFS(0). In the hope that one of these will actually free any buffers. It's random at the moment when drivers release buffers, one of the reasons for using vb2. Anyway, even replacing REQBUFS(0) with VIDIOC_STREAMOFF at qv4l2 won't help with uvcvideo. It seems that, once buffers are requested at uvcvideo, they will release only at close(). That's not correct. Buffers are released when calling REQBUFS(0). However, the file handle is still marked as owning the device for streaming purpose, so other applications can't change the format or request buffers. Why? After REQBUFS(0) any filehandle ownership should be dropped. What if the application wants to change the resolution during capture ? It will have to stop capture, call REQBUFS(0), change the format, request buffers and restart capture. If filehandle ownership is dropped after REQBUFS(0) that will open the door to a race condition. That's why S_PRIORITY was invented. One of the nice properties of V4L2 is the ability to allow multiple processes to access the same device at the same time and even make modifications where possible. To prevent unwanted modifications the priority scheme was created. Unfortunately, too many drivers do not support it. It really should be core functionality. I did work on it earlier in the year, but never finished it, although it shouldn't be hard to do. Regards, Hans -- Hans Verkuil - video4linux developer - sponsored by Cisco -- 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: [GIT PULL FOR 2.6.37] uvcvideo: BKL removal
On Saturday, December 18, 2010 01:54:41 Laurent Pinchart wrote: Hi Mauro, On Friday 17 December 2010 18:09:39 Mauro Carvalho Chehab wrote: Em 14-12-2010 08:55, Laurent Pinchart escreveu: Hi Mauro, Please don't forget this pull request for 2.6.37. Pull request for upstream sent today. Thank you. I didn't find any regressions at the BKL removal patches, but I noticed a few issues with qv4l2, not all related to uvcvideo. The remaining of this email is an attempt to document them for later fixes. They don't seem to be regressions caused by BKL removal, but the better would be to fix them later. - with uvcvideo and two video apps, if qv4l2 is started first, the second application doesn't start/capture. I suspect that REQBUFS (used by qv4l2 to probe mmap/userptr capabilities) create some resource locking at uvcvideo. The proper way is to lock the resources only if the driver is streaming, as other drivers and videobuf do. I don't agree with that. The uvcvideo driver has one buffer queue per device, so if an application requests buffers on one file handle it will lock other applications out. If the driver didn't it would be subject to race conditions. I agree with Laurent. Once an application calls REQBUFS with non-zero count, then it should lock the resources needed for streaming. The reason behind that is that REQBUFS also locks the current selected format in place, since the format determines the amount of memory needed for the buffers. The reason a lot of drivers don't do this is partially because for many TV capture drivers it is highly unlikely that the buffer size will change after calling REQBUFS (there are basically only two formats: 720x480 or 720x576 and users will normally never change between the two). However, this is much more likely to happen for webcams and embedded systems supporting HDTV. The other reason is probably because driver developers simple do not realize they need to lock the resources on REQBUFS. I'm sure many existing drivers will fail miserably if you change the format after calling REQBUFS (particularly with mmap streaming mode). Regards, Hans -- Hans Verkuil - video4linux developer - sponsored by Cisco -- 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: [GIT PULL FOR 2.6.37] uvcvideo: BKL removal
Laurent, Em 14-12-2010 08:55, Laurent Pinchart escreveu: Hi Mauro, Please don't forget this pull request for 2.6.37. Pull request for upstream sent today. I didn't find any regressions at the BKL removal patches, but I noticed a few issues with qv4l2, not all related to uvcvideo. The remaining of this email is an attempt to document them for later fixes. They don't seem to be regressions caused by BKL removal, but the better would be to fix them later. - with uvcvideo and two video apps, if qv4l2 is started first, the second application doesn't start/capture. I suspect that REQBUFS (used by qv4l2 to probe mmap/userptr capabilities) create some resource locking at uvcvideo. The proper way is to lock the resources only if the driver is streaming, as other drivers and videobuf do. - with saa7134 and qv4l2 (and after a fix for input capabilities): saa7134 and/or qv4l2 doesn't seem to work fine if video format is changed to a 60HZ format (NTSC or PAL/M). It keeps trying to use 576 lines, but the driver only works with 480 lines for those formats. So, if qv4l2 tries to capture with STD/M, it fails, except if the number of lines is manually fixed by the user. - at least with the saa7134 board I used for test, video capture fails on some conditions. This is not related to BKL patches. I suspect it may be some initialization failure with the tuner (tda8275/tda8290), but I didn't have time to dig into it, nor to test with a simpler saa7134 device. The device I used was an Avermedia m135. On Monday 29 November 2010 11:15:10 Laurent Pinchart wrote: Hi Mauro, The following changes since commit c796e203229c8c08250f9d372ae4e10c466b1787: [media] kconfig: add an option to determine a menu's visibility (2010-11-22 10:37:56 -0200) are available in the git repository at: git://linuxtv.org/pinchartl/uvcvideo.git uvcvideo-stable They complete the BKL removal from the uvcvideo driver. Feedback received from Hans during review has been integrated. Laurent Pinchart (5): uvcvideo: Lock controls mutex when querying menus uvcvideo: Move mutex lock/unlock inside uvc_free_buffers uvcvideo: Move mmap() handler to uvc_queue.c uvcvideo: Lock stream mutex when accessing format-related information uvcvideo: Convert to unlocked_ioctl drivers/media/video/uvc/uvc_ctrl.c | 48 +- drivers/media/video/uvc/uvc_queue.c | 133 +- drivers/media/video/uvc/uvc_v4l2.c | 185 +++--- drivers/media/video/uvc/uvc_video.c | 3 - drivers/media/video/uvc/uvcvideo.h | 10 ++- 5 files changed, 222 insertions(+), 157 deletions(-) -- 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: [GIT PULL FOR 2.6.37] uvcvideo: BKL removal
Hi Mauro, On Friday 17 December 2010 18:09:39 Mauro Carvalho Chehab wrote: Em 14-12-2010 08:55, Laurent Pinchart escreveu: Hi Mauro, Please don't forget this pull request for 2.6.37. Pull request for upstream sent today. Thank you. I didn't find any regressions at the BKL removal patches, but I noticed a few issues with qv4l2, not all related to uvcvideo. The remaining of this email is an attempt to document them for later fixes. They don't seem to be regressions caused by BKL removal, but the better would be to fix them later. - with uvcvideo and two video apps, if qv4l2 is started first, the second application doesn't start/capture. I suspect that REQBUFS (used by qv4l2 to probe mmap/userptr capabilities) create some resource locking at uvcvideo. The proper way is to lock the resources only if the driver is streaming, as other drivers and videobuf do. I don't agree with that. The uvcvideo driver has one buffer queue per device, so if an application requests buffers on one file handle it will lock other applications out. If the driver didn't it would be subject to race conditions. - with saa7134 and qv4l2 (and after a fix for input capabilities): saa7134 and/or qv4l2 doesn't seem to work fine if video format is changed to a 60HZ format (NTSC or PAL/M). It keeps trying to use 576 lines, but the driver only works with 480 lines for those formats. So, if qv4l2 tries to capture with STD/M, it fails, except if the number of lines is manually fixed by the user. - at least with the saa7134 board I used for test, video capture fails on some conditions. This is not related to BKL patches. I suspect it may be some initialization failure with the tuner (tda8275/tda8290), but I didn't have time to dig into it, nor to test with a simpler saa7134 device. The device I used was an Avermedia m135. -- Regards, Laurent Pinchart -- 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: [GIT PULL FOR 2.6.37] uvcvideo: BKL removal
Hi Mauro, Please don't forget this pull request for 2.6.37. On Monday 29 November 2010 11:15:10 Laurent Pinchart wrote: Hi Mauro, The following changes since commit c796e203229c8c08250f9d372ae4e10c466b1787: [media] kconfig: add an option to determine a menu's visibility (2010-11-22 10:37:56 -0200) are available in the git repository at: git://linuxtv.org/pinchartl/uvcvideo.git uvcvideo-stable They complete the BKL removal from the uvcvideo driver. Feedback received from Hans during review has been integrated. Laurent Pinchart (5): uvcvideo: Lock controls mutex when querying menus uvcvideo: Move mutex lock/unlock inside uvc_free_buffers uvcvideo: Move mmap() handler to uvc_queue.c uvcvideo: Lock stream mutex when accessing format-related information uvcvideo: Convert to unlocked_ioctl drivers/media/video/uvc/uvc_ctrl.c | 48 +- drivers/media/video/uvc/uvc_queue.c | 133 +- drivers/media/video/uvc/uvc_v4l2.c | 185 +++--- drivers/media/video/uvc/uvc_video.c | 3 - drivers/media/video/uvc/uvcvideo.h | 10 ++- 5 files changed, 222 insertions(+), 157 deletions(-) -- Regards, Laurent Pinchart -- 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
[GIT PULL FOR 2.6.37] uvcvideo: BKL removal
Hi Mauro, The following changes since commit c796e203229c8c08250f9d372ae4e10c466b1787: [media] kconfig: add an option to determine a menu's visibility (2010-11-22 10:37:56 -0200) are available in the git repository at: git://linuxtv.org/pinchartl/uvcvideo.git uvcvideo-stable They complete the BKL removal from the uvcvideo driver. Feedback received from Hans during review has been integrated. Laurent Pinchart (5): uvcvideo: Lock controls mutex when querying menus uvcvideo: Move mutex lock/unlock inside uvc_free_buffers uvcvideo: Move mmap() handler to uvc_queue.c uvcvideo: Lock stream mutex when accessing format-related information uvcvideo: Convert to unlocked_ioctl drivers/media/video/uvc/uvc_ctrl.c | 48 +- drivers/media/video/uvc/uvc_queue.c | 133 +- drivers/media/video/uvc/uvc_v4l2.c | 185 +++--- drivers/media/video/uvc/uvc_video.c |3 - drivers/media/video/uvc/uvcvideo.h | 10 ++- 5 files changed, 222 insertions(+), 157 deletions(-) -- Regards, Laurent Pinchart -- 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
