RFC: remove video_register_device_index, add video_register_device_range
Hi all, While looking at the video_register_device changes that broke ov511 I realized that the video_register_device_index function is never called from drivers. It will always assign a default index number. I also don't see a good use-case for giving it an explicit index. My proposal is to remove this API. Since it is never called, nothing will change effectively. I'm never happy with unused functions. However, I think we do need a new video_register_device_range function. This would be identical to video_register_device, but with an additional count argument: this allows drivers to select a kernel number in the range of nr to nr + count - 1. If cnt == -1, then the maximum is the compiled-in maximum. So video_register_device would call video_register_device_range(...nr, 1), thus restoring the original behavior, while ivtv and cx18 can call video_register_device_range(...nr, -1), thus keeping the current behavior. Comments? Regards, Hans -- Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom -- 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: remove video_register_device_index, add video_register_device_range
Em Mon, 15 Jun 2009 13:25:28 +0200
Hans Verkuil hverk...@xs4all.nl escreveu:
Hi all,
While looking at the video_register_device changes that broke ov511 I realized
that the video_register_device_index function is never called from drivers. It
will always assign a default index number. I also don't see a good use-case
for giving it an explicit index. My proposal is to remove this API. Since it
is never called, nothing will change effectively. I'm never happy with unused
functions.
It sounds ok to me.
However, I think we do need a new video_register_device_range function. This
would be identical to video_register_device, but with an additional count
argument: this allows drivers to select a kernel number in the range of
nr to nr + count - 1. If cnt == -1, then the maximum is the compiled-in
maximum.
So video_register_device would call video_register_device_range(...nr, 1),
thus restoring the original behavior, while ivtv and cx18 can call
video_register_device_range(...nr, -1), thus keeping the current behavior.
I don't think that this is needed. The issue with ov511 is that it was using
the error condition of video_register to identify how much ov511 devices were
already registered.
I already fixed it by adding an explicit device number count on it, just like
all the other drivers. With this, ov511 will behave like the other drivers.
With the current implementation, it should honor the explicit minor order
passed via modprobe parameter, as before.
There's one small change on the behavior that it is also present on all other
devices that allow users to explicit the desired minor order: instead of
failing if that device is already used, it will get the next available one.
IMO, this is better than just failing. So, the only remaining issue is that
video_register should print a warning if it had to get a different minor than
specified.
In other words, the enclosed patch seems to be a good approach to close this
issue
Cheers,
Mauro
---
v4l2-dev: Print a warning if need to use a different minor than specified
From: Mauro Carvalho Chehab mche...@redhat.com
video_register_device has two behaviors: dynamic minor attribution or
forced minor attribution. The latter mode is used to allow users to
force probing a device using a certain minor order. Even for the last
case, video_register_device won't fail if that minor is already used
but, instead, it will seek for the next available minor, without warning
users about that.
While don't fail due to a busy minor seems the better behavior, it
should be printing a warning when this happen.
While here, let's remove video_register_device_index(), since no driver
is using it.
Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com
---
linux/drivers/media/video/v4l2-dev.c | 31 ---
linux/include/media/v4l2-dev.h |5 ++---
2 files changed, 18 insertions(+), 18 deletions(-)
--- master.orig/linux/drivers/media/video/v4l2-dev.c
+++ master/linux/drivers/media/video/v4l2-dev.c
@@ -384,23 +384,18 @@ static int get_index(struct video_device
return i == VIDEO_NUM_DEVICES ? -ENFILE : i;
}
-int video_register_device(struct video_device *vdev, int type, int nr)
-{
- return video_register_device_index(vdev, type, nr, -1);
-}
-EXPORT_SYMBOL(video_register_device);
/**
- * video_register_device_index - register video4linux devices
+ * video_register_device - register video4linux devices
* @vdev: video device structure we want to register
* @type: type of device to register
* @nr: which device number (0 == /dev/video0, 1 == /dev/video1, ...
* -1 == first free)
- * @index: stream number based on parent device;
- * -1 if auto assign, requested number otherwise
*
* The registration code assigns minor numbers based on the type
- * requested. -ENFILE is returned in all the device slots for this
+ * requested. If the requested one is already used, get the next
+ * available one and prints a warning.
+ * -ENFILE is returned in all the device slots for this
* category are full. If not then the minor field is set and the
* driver initialize function is called (if non %NULL).
*
@@ -416,10 +411,10 @@ EXPORT_SYMBOL(video_register_device);
*
* %VFL_TYPE_RADIO - A radio card
*/
-int video_register_device_index(struct video_device *vdev, int type, int nr,
- int index)
+int video_register_device(struct video_device *vdev, const int type,
+ const int desirednr)
{
- int i = 0;
+ int i = 0, nr;
int ret;
int minor_offset = 0;
int minor_cnt = VIDEO_NUM_DEVICES;
@@ -493,7 +488,8 @@ int video_register_device_index(struct v
/* Pick a minor number */
mutex_lock(videodev_lock);
- nr = find_next_zero_bit(video_nums[type], minor_cnt, nr == -1 ? 0 : nr);
+ nr =
Re: RFC: remove video_register_device_index, add video_register_device_range
On Monday 15 June 2009 15:44:21 Mauro Carvalho Chehab wrote:
Em Mon, 15 Jun 2009 13:25:28 +0200
Hans Verkuil hverk...@xs4all.nl escreveu:
Hi all,
While looking at the video_register_device changes that broke ov511 I
realized
that the video_register_device_index function is never called from drivers.
It
will always assign a default index number. I also don't see a good use-case
for giving it an explicit index. My proposal is to remove this API. Since it
is never called, nothing will change effectively. I'm never happy with
unused
functions.
It sounds ok to me.
However, I think we do need a new video_register_device_range function. This
would be identical to video_register_device, but with an additional count
argument: this allows drivers to select a kernel number in the range of
nr to nr + count - 1. If cnt == -1, then the maximum is the compiled-in
maximum.
So video_register_device would call video_register_device_range(...nr, 1),
thus restoring the original behavior, while ivtv and cx18 can call
video_register_device_range(...nr, -1), thus keeping the current behavior.
I don't think that this is needed. The issue with ov511 is that it was using
the error condition of video_register to identify how much ov511 devices were
already registered.
I already fixed it by adding an explicit device number count on it, just like
all the other drivers. With this, ov511 will behave like the other drivers.
With the current implementation, it should honor the explicit minor order
passed via modprobe parameter, as before.
There's one small change on the behavior that it is also present on all other
devices that allow users to explicit the desired minor order: instead of
failing if that device is already used, it will get the next available one.
IMO, this is better than just failing. So, the only remaining issue is that
video_register should print a warning if it had to get a different minor than
specified.
The sticking point for me is that warning since for cx18/ivtv it is OK if you
get something else then you specified (since it is a starting index meant to
distinguish mpeg encoders from raw video inputs, from mpeg decoders, etc.).
So generating a warning for those two drivers is not correct.
Perhaps we should add a V4L2_FL_KNUM_OFFSET flag for the struct video_device
flags field that tell the register function that 'nr' should be interpreted
as a kernel number offset, and not as a preferred number. In the latter case
you generate a warning, in the first case you don't.
I think it isn't a bad idea to use a flag. It reflects the two possible use
cases: one for drivers that create multiple video (or vbi) devices and use the
kernel number to reflect the purpose of each video device, and the other where
the user wants a specific kernel number. In the latter case the driver creates
a single video device.
I don't want to see a lot of kernel warnings each time an ivtv or cx18 driver
is loaded. Those warnings do not apply to those drivers.
BTW: please note that my v4l-dvb-misc tree contains a patch to clean up the
comments/variable names in v4l2-dev.c. You might want to pull that in first.
Regards,
Hans
In other words, the enclosed patch seems to be a good approach to close this
issue
Cheers,
Mauro
---
v4l2-dev: Print a warning if need to use a different minor than specified
From: Mauro Carvalho Chehab mche...@redhat.com
video_register_device has two behaviors: dynamic minor attribution or
forced minor attribution. The latter mode is used to allow users to
force probing a device using a certain minor order. Even for the last
case, video_register_device won't fail if that minor is already used
but, instead, it will seek for the next available minor, without warning
users about that.
While don't fail due to a busy minor seems the better behavior, it
should be printing a warning when this happen.
While here, let's remove video_register_device_index(), since no driver
is using it.
Signed-off-by: Mauro Carvalho Chehab mche...@redhat.com
---
linux/drivers/media/video/v4l2-dev.c | 31 ---
linux/include/media/v4l2-dev.h |5 ++---
2 files changed, 18 insertions(+), 18 deletions(-)
--- master.orig/linux/drivers/media/video/v4l2-dev.c
+++ master/linux/drivers/media/video/v4l2-dev.c
@@ -384,23 +384,18 @@ static int get_index(struct video_device
return i == VIDEO_NUM_DEVICES ? -ENFILE : i;
}
-int video_register_device(struct video_device *vdev, int type, int nr)
-{
- return video_register_device_index(vdev, type, nr, -1);
-}
-EXPORT_SYMBOL(video_register_device);
/**
- * video_register_device_index - register video4linux devices
+ * video_register_device - register video4linux devices
* @vdev: video device structure we want to register
* @type: type of device to register
* @nr: which device number (0 ==
Re: RFC: remove video_register_device_index, add video_register_device_range
Em Mon, 15 Jun 2009 16:02:40 +0200 Hans Verkuil hverk...@xs4all.nl escreveu: The sticking point for me is that warning since for cx18/ivtv it is OK if you get something else then you specified (since it is a starting index meant to distinguish mpeg encoders from raw video inputs, from mpeg decoders, etc.). So generating a warning for those two drivers is not correct. Ok. Perhaps we should add a V4L2_FL_KNUM_OFFSET flag for the struct video_device flags field that tell the register function that 'nr' should be interpreted as a kernel number offset, and not as a preferred number. In the latter case you generate a warning, in the first case you don't. Hmm... V4L2_FL_KNUM_OFFSET seems a too obfuscated name. Also, such flag would be needed by just the register function, so, IMO, a parameter would work better. Also, am I wrong or is there anything wrong with the flags? The only place I'm seeing this being used is here: $ grep 'vdev-flags' v4l/*.[ch] v4l/v4l2-dev.c: set_bit(V4L2_FL_UNREGISTERED, vdev-flags); It is being set, but no code seems to actually test for it. So, IMO, we can just remove this field. One alternative would be to implement it as something like: +int __must_check __video_register_device(struct video_device *vdev, + const int type, const int nr, int warn_if_skip); #define video_register_device(vdev, type, nr) __video_register_device(vdev, type, nr, 0) I think it isn't a bad idea to use a flag. It reflects the two possible use cases: one for drivers that create multiple video (or vbi) devices and use the kernel number to reflect the purpose of each video device, and the other where the user wants a specific kernel number. In the latter case the driver creates a single video device. I don't want to see a lot of kernel warnings each time an ivtv or cx18 driver is loaded. Those warnings do not apply to those drivers. BTW: please note that my v4l-dvb-misc tree contains a patch to clean up the comments/variable names in v4l2-dev.c. You might want to pull that in first. Please see my comments for your v4l-dvb-misc tree pull request. Let's first work on that changeset, and then we can discuss better about the warning issue. Regards, Han 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: RFC: remove video_register_device_index, add video_register_device_range
On Monday 15 June 2009 21:51:13 Mauro Carvalho Chehab wrote:
Em Mon, 15 Jun 2009 16:02:40 +0200
Hans Verkuil hverk...@xs4all.nl escreveu:
The sticking point for me is that warning since for cx18/ivtv it is OK if
you
get something else then you specified (since it is a starting index meant to
distinguish mpeg encoders from raw video inputs, from mpeg decoders, etc.).
So generating a warning for those two drivers is not correct.
Ok.
Perhaps we should add a V4L2_FL_KNUM_OFFSET flag for the struct video_device
flags field that tell the register function that 'nr' should be interpreted
as a kernel number offset, and not as a preferred number. In the latter case
you generate a warning, in the first case you don't.
Hmm... V4L2_FL_KNUM_OFFSET seems a too obfuscated name. Also, such flag would
be needed by just the register function, so, IMO, a parameter would work
better.
That was the idea with the video_register_device_range() proposal. I don't want
to modify all existing video_register_device() calls.
Also, am I wrong or is there anything wrong with the flags?
The only place I'm seeing this being used is here:
$ grep 'vdev-flags' v4l/*.[ch]
v4l/v4l2-dev.c: set_bit(V4L2_FL_UNREGISTERED, vdev-flags);
It is being set, but no code seems to actually test for it. So, IMO, we can
just remove this field.
No, it is used a lot through the video_is_unregistered() inline in
media/v4l2-dev.h.
One alternative would be to implement it as something like:
+int __must_check __video_register_device(struct video_device *vdev,
+ const int type, const int nr, int
warn_if_skip);
#define video_register_device(vdev, type, nr) __video_register_device(vdev,
type, nr, 0)
Hmm, that's an option. Although I'd make it a static inline:
static inline int __must_check video_register_device(...)
{
return __video_register_device(vdev, type, nr, 1);
}
Regards,
Hans
I think it isn't a bad idea to use a flag. It reflects the two possible use
cases: one for drivers that create multiple video (or vbi) devices and use
the
kernel number to reflect the purpose of each video device, and the other
where
the user wants a specific kernel number. In the latter case the driver
creates
a single video device.
I don't want to see a lot of kernel warnings each time an ivtv or cx18
driver
is loaded. Those warnings do not apply to those drivers.
BTW: please note that my v4l-dvb-misc tree contains a patch to clean up the
comments/variable names in v4l2-dev.c. You might want to pull that in first.
Please see my comments for your v4l-dvb-misc tree pull request. Let's first
work on that changeset, and then we can discuss better about the warning
issue.
Regards,
Han
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
--
Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom
--
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
