v4l: Introduce "index" attribute for persistent video4linux device nodes

# HG changeset patch
# User Brandon Philips <[email protected]>
# Date 1214002727 25200
# Node ID 3dbf42455956d17b8aa65bf92319edc3c52b88b8
# Parent 4012ea1a6a06acad9509aee70ee6059af9000a37 [PATCH] v4l: Introduce "index" attribute for persistent video4linux device nodes

A number of V4L drivers have a mod param to specify their preferred minors. This is because it is often desirable for applications to have a static /dev name for a particular device. However, using minors has several disadvantages:

1) the requested minor may already be taken
2) using a mod param is driver specific
3) it requires every driver to add a param
4) requires configuration by hand

This patch introduces an "index" attribute that when combined with udev rules can create static device paths like this:

/dev/v4l/by-path/pci-0000\:00\:1d.2-usb-0\:1\:1.0-video0 /dev/v4l/by-path/pci-0000\:00\:1d.2-usb-0\:1\:1.0-video1 /dev/v4l/by-path/pci-0000\:00\:1d.2-usb-0\:1\:1.0-video2

# ls -la /dev/v4l/by-path/pci-0000\:00\:1d.2-usb-0\:1\:1.0-video0
lrwxrwxrwx 1 root root 12 2008-04-28 00:02
/dev/v4l/by-path/pci-0000:00:1d.2-usb-0:1:1.0-video0 -> ../../video1

These paths are steady across reboots and should be resistant to rearranging across Kernel versions.

video_register_device_index is available to drivers to request a specific index number.

Signed-off-by Brandon Philips <[email protected]>

Signed-off-by Kees Cook <[email protected]>

Signed-off-by Kay Sievers <[email protected]>

---
 linux/drivers/media/video/videodev.c |   98
++++++++++++++++++++++++++++++++++- linux/include/media/v4l2-dev.h   
   |    4 +
 2 files changed, 100 insertions(+), 2 deletions(-)

(quoted diff) +0 -0

b/linux/drivers/media/video/videodev.c ---
a/linux/drivers/media/video/videodev.c
+++ b/linux/drivers/media/video/videodev.c

linux/drivers/media/video/videodev.c +29 -0

@@ -429,6 +429,14 @@ EXPORT_SYMBOL(v4l_printk_ioctl);
  *	sysfs stuff
  */
 
+static ssize_t show_index(struct device *cd,
+			 struct device_attribute *attr, char *buf)
+{
+	struct video_device *vfd = container_of(cd, struct video_device,
+						class_dev);
+	return sprintf(buf, "%i\n", vfd->index);
+}
+
 #if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,13)
 static ssize_t show_name(struct class_device *cd, char *buf)
 #else

@@ -487,6 +495,7 @@ static void video_release(struct device
 #if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,13)
 static struct device_attribute video_device_attrs[] = {
 	__ATTR(name, S_IRUGO, show_name, NULL),
+	__ATTR(index, S_IRUGO, show_index, NULL),
 	__ATTR_NULL
 };
 #endif

@@ -2042,7 +2051,81 @@ out:
 }
 EXPORT_SYMBOL(video_ioctl2);
 
+struct index_info {
+	struct device *dev;
+	unsigned int used[VIDEO_NUM_DEVICES];
+};
+
+static int __fill_index_info(struct device *cd, void *data)
+{
+	struct index_info *info = data;
+	struct video_device *vfd = container_of(cd, struct video_device,
+						class_dev);
+
+	if (info->dev == vfd->dev)
+		info->used[vfd->index] = 1;
+
+	return 0;
+}
+
+/**
+ * assign_index - assign stream number based on parent device
+ * @vdev: video_device to assign index number to, vdev->dev should

be assigned + * @num: -1 if auto assign, requested number otherwise
+ *
+ *
+ * returns -ENFILE if num is already in use, a free index number if
+ * successful.
+ */

linux/drivers/media/video/videodev.c +44 -0

+static int get_index(struct video_device *vdev, int num)
+{
+	struct index_info *info;
+	int i;
+	int ret = 0;
+
+	if (num >= VIDEO_NUM_DEVICES)
+		return -EINVAL;
+
+	info = kzalloc(sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+
+	info->dev = vdev->dev;
+
+	ret = class_for_each_device(&video_class, info,
+					__fill_index_info);
+
+	if (ret < 0)
+		goto out;
+
+	if (num >= 0) {
+		if (!info->used[num])
+			ret = num;
+		else
+			ret = -ENFILE;
+
+		goto out;
+	}
+
+	for (i = 0; i < VIDEO_NUM_DEVICES; i++) {
+		if (info->used[i])
+			continue;
+		ret = i;
+		goto out;
+	}
+
+out:
+	kfree(info);
+	return ret;
+}
+
 static const struct file_operations video_fops;
+
+int video_register_device(struct video_device *vfd, int type, int

nr) +{

linux/drivers/media/video/videodev.c +3 -1

+	return video_register_device_index(vfd, type, nr, -1);
+}
+EXPORT_SYMBOL(video_register_device);
 
 /**
  *	video_register_device - register video4linux devices

@@ -2069,7 +2152,8 @@ static const struct file_operations vide
  *	%VFL_TYPE_RADIO - A radio card
  */
 
-int video_register_device(struct video_device *vfd, int type, int

nr) +int video_register_device_index(struct video_device *vfd, int
type, int nr, + int index)
{

linux/drivers/media/video/videodev.c +11 -1

 	int i=0;
 	int base;

@@ -2126,6 +2210,16 @@ int video_register_device(struct video_d
 	}
 	video_device[i]=vfd;
 	vfd->minor=i;
+
+	ret = get_index(vfd, index);
+	if (ret < 0) {
+		printk(KERN_ERR "%s: get_index failed\n",
+		       __func__);
+		goto fail_minor;
+	}
+
+	vfd->index = ret;
+
 	mutex_unlock(&videodev_lock);
 	mutex_init(&vfd->lock);
 

@@ -2188,7 +2282,7 @@ fail_minor:
 	mutex_unlock(&videodev_lock);
 	return ret;
 }
-EXPORT_SYMBOL(video_register_device);
+EXPORT_SYMBOL(video_register_device_index);
 
 /**
  *	video_unregister_device - unregister a video4linux device

(quoted diff) +0 -0

b/linux/include/media/v4l2-dev.h --- a/linux/include/media/v4l2-dev.h +++ b/linux/include/media/v4l2-dev.h

linux/include/media/v4l2-dev.h +1 -1

@@ -112,6 +112,8 @@ struct video_device
 	int type;       /* v4l1 */
 	int type2;      /* v4l2 */
 	int minor;
+	/* attribute to diferentiate multiple indexs on one physical device
 */ +	int index;
 
 	int debug;	/* Activates debug level*/

@@ -377,6 +379,8 @@ void *priv;
 
 /* Version 2 functions */
 extern int video_register_device(struct video_device *vfd, int type,
 int nr); +int video_register_device_index(struct video_device *vfd,
 int type, int nr, +					int index);
 void video_unregister_device(struct video_device *);
 extern int video_ioctl2(struct inode *inode, struct file *file,
 			  unsigned int cmd, unsigned long arg);
 
--

video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Isn't it better to have a static bitarray in videodev that keeps track of which devices are in use? Or if that's not possible for some reason,

Quoted 2 lines · Hans Verkuil

Isn't it better to have a static bitarray in videodev that keeps track of which devices are in use? Or if that's not possible for some reason,

Isn't there already an array that has a entry for each minor that's in use?

I might misunderstand it, but it looks like for every v4l device that's created you temporarily allocate a structure, loop over all current v4l devices to find which indices are in use, determine the new index and free the structure again.

Isn't it better to have a static bitarray in videodev that keeps track of which devices are in use?

Or if that's not possible for some reason, at least avoid the expensive struct allocation and simply use a bitarray allocated on the stack (max 256 devices = 32 bytes)?

I'm looking at this patch and I wonder whether it isn't rather inefficient.

Note that class_for_each_device() didn't appear until 2.6.25, so I've a simple patch pending in my tree that puts all of this under a kernel

Quoted 1 line

= 2.6.25 check.

Quoted 212 lines

# HG changeset patch
# User Brandon Philips <[email protected]>
# Date 1214002727 25200
# Node ID 3dbf42455956d17b8aa65bf92319edc3c52b88b8
# Parent 4012ea1a6a06acad9509aee70ee6059af9000a37 [PATCH] v4l: Introduce "index" attribute for persistent video4linux device nodes

A number of V4L drivers have a mod param to specify their preferred minors. This is because it is often desirable for applications to have a static /dev name for a particular device. However, using minors has several disadvantages:

1) the requested minor may already be taken
2) using a mod param is driver specific
3) it requires every driver to add a param
4) requires configuration by hand

This patch introduces an "index" attribute that when combined with udev rules can create static device paths like this:

/dev/v4l/by-path/pci-0000\:00\:1d.2-usb-0\:1\:1.0-video0 /dev/v4l/by-path/pci-0000\:00\:1d.2-usb-0\:1\:1.0-video1 /dev/v4l/by-path/pci-0000\:00\:1d.2-usb-0\:1\:1.0-video2

# ls -la /dev/v4l/by-path/pci-0000\:00\:1d.2-usb-0\:1\:1.0-video0
lrwxrwxrwx 1 root root 12 2008-04-28 00:02
/dev/v4l/by-path/pci-0000:00:1d.2-usb-0:1:1.0-video0 -> ../../video1

These paths are steady across reboots and should be resistant to rearranging across Kernel versions.

video_register_device_index is available to drivers to request a specific index number.

Signed-off-by Brandon Philips <[email protected]>

Signed-off-by Kees Cook <[email protected]>

Signed-off-by Kay Sievers <[email protected]>

---
 linux/drivers/media/video/videodev.c |   98
++++++++++++++++++++++++++++++++++- linux/include/media/v4l2-dev.h   
   |    4 +
 2 files changed, 100 insertions(+), 2 deletions(-)

(quoted diff) +0 -0

b/linux/drivers/media/video/videodev.c ---
a/linux/drivers/media/video/videodev.c
+++ b/linux/drivers/media/video/videodev.c

(quoted diff) +29 -0

@@ -429,6 +429,14 @@ EXPORT_SYMBOL(v4l_printk_ioctl);
  *	sysfs stuff
  */
 
+static ssize_t show_index(struct device *cd,
+			 struct device_attribute *attr, char *buf)
+{
+	struct video_device *vfd = container_of(cd, struct video_device,
+						class_dev);
+	return sprintf(buf, "%i\n", vfd->index);
+}
+
 #if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,13)
 static ssize_t show_name(struct class_device *cd, char *buf)
 #else

@@ -487,6 +495,7 @@ static void video_release(struct device
 #if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,13)
 static struct device_attribute video_device_attrs[] = {
 	__ATTR(name, S_IRUGO, show_name, NULL),
+	__ATTR(index, S_IRUGO, show_index, NULL),
 	__ATTR_NULL
 };
 #endif

@@ -2042,7 +2051,81 @@ out:
 }
 EXPORT_SYMBOL(video_ioctl2);
 
+struct index_info {
+	struct device *dev;
+	unsigned int used[VIDEO_NUM_DEVICES];
+};
+
+static int __fill_index_info(struct device *cd, void *data)
+{
+	struct index_info *info = data;
+	struct video_device *vfd = container_of(cd, struct video_device,
+						class_dev);
+
+	if (info->dev == vfd->dev)
+		info->used[vfd->index] = 1;
+
+	return 0;
+}
+
+/**
+ * assign_index - assign stream number based on parent device
+ * @vdev: video_device to assign index number to, vdev->dev should

be assigned + * @num: -1 if auto assign, requested number otherwise
+ *
+ *
+ * returns -ENFILE if num is already in use, a free index number if
+ * successful.
+ */

(quoted diff) +44 -0

+static int get_index(struct video_device *vdev, int num)
+{
+	struct index_info *info;
+	int i;
+	int ret = 0;
+
+	if (num >= VIDEO_NUM_DEVICES)
+		return -EINVAL;
+
+	info = kzalloc(sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+
+	info->dev = vdev->dev;
+
+	ret = class_for_each_device(&video_class, info,
+					__fill_index_info);
+
+	if (ret < 0)
+		goto out;
+
+	if (num >= 0) {
+		if (!info->used[num])
+			ret = num;
+		else
+			ret = -ENFILE;
+
+		goto out;
+	}
+
+	for (i = 0; i < VIDEO_NUM_DEVICES; i++) {
+		if (info->used[i])
+			continue;
+		ret = i;
+		goto out;
+	}
+
+out:
+	kfree(info);
+	return ret;
+}
+
 static const struct file_operations video_fops;
+
+int video_register_device(struct video_device *vfd, int type, int

nr) +{

(quoted diff) +3 -1

+	return video_register_device_index(vfd, type, nr, -1);
+}
+EXPORT_SYMBOL(video_register_device);
 
 /**
  *	video_register_device - register video4linux devices

@@ -2069,7 +2152,8 @@ static const struct file_operations vide
  *	%VFL_TYPE_RADIO - A radio card
  */
 
-int video_register_device(struct video_device *vfd, int type, int

nr) +int video_register_device_index(struct video_device *vfd, int
type, int nr, + int index)
{

(quoted diff) +11 -1

 	int i=0;
 	int base;

@@ -2126,6 +2210,16 @@ int video_register_device(struct video_d
 	}
 	video_device[i]=vfd;
 	vfd->minor=i;
+
+	ret = get_index(vfd, index);
+	if (ret < 0) {
+		printk(KERN_ERR "%s: get_index failed\n",
+		       __func__);
+		goto fail_minor;
+	}
+
+	vfd->index = ret;
+
 	mutex_unlock(&videodev_lock);
 	mutex_init(&vfd->lock);
 

@@ -2188,7 +2282,7 @@ fail_minor:
 	mutex_unlock(&videodev_lock);
 	return ret;
 }
-EXPORT_SYMBOL(video_register_device);
+EXPORT_SYMBOL(video_register_device_index);
 
 /**
  *	video_unregister_device - unregister a video4linux device

(quoted diff) +0 -0

b/linux/include/media/v4l2-dev.h --- a/linux/include/media/v4l2-dev.h +++ b/linux/include/media/v4l2-dev.h

(quoted diff) +1 -0

@@ -112,6 +112,8 @@ struct video_device
 	int type;       /* v4l1 */
 	int type2;      /* v4l2 */
 	int minor;
+	/* attribute to diferentiate multiple indexs on one physical device
 */ +	int index;
 
 	int debug;	/* Activates debug level*/

@@ -377,6 +379,8 @@ void *priv;
 
 /* Version 2 functions */
 extern int video_register_device(struct video_device *vfd, int type,
 int nr); +int video_register_device_index(struct video_device *vfd,
 int type, int nr, +					int index);
 void video_unregister_device(struct video_device *);
 extern int video_ioctl2(struct inode *inode, struct file *file,
 			  unsigned int cmd, unsigned long arg);

Quoted 5 lines

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Quoted 4 lines · Hans Verkuil

I might misunderstand it, but it looks like for every v4l device that's created you temporarily allocate a structure, loop over all current v4l devices to find which indices are in use, determine the new index and free the structure again.

Yes, that is the way it currently works. Note that this searches for indices that are in use by the physical device that we are adding. Index is not videodev global.

Quoted 2 lines

Isn't it better to have a static bitarray in videodev that keeps track of which devices are in use?

A simple global bit array wouldn't work. You need to have a separate bit array for every physical device that could possibly be plugged in since the indices are per-physical device.

Quoted 3 lines

Or if that's not possible for some reason, at least avoid the expensive struct allocation and simply use a bitarray allocated on the stack (max 256 devices = 32 bytes)?

Using ffz and set_bit would be an option (since bitfields can't be used on arrays) but I don't think the savings would be worth the effort since we would need to use division and an array if VIDEO_NUM_DEVICES grows past 256.

I think changing "used" to a uchar is a good choice though.

(quoted diff) +0 -0

b/linux/drivers/media/video/videodev.c ---
a/linux/drivers/media/video/videodev.c
+++ b/linux/drivers/media/video/videodev.c

linux/drivers/media/video/videodev.c +1 -1

@@ -2053,7 +2053,7 @@ EXPORT_SYMBOL(video_ioctl2);
 
 struct index_info {
 	struct device *dev;
-	unsigned int used[VIDEO_NUM_DEVICES];
+	unsigned char used[VIDEO_NUM_DEVICES];
 };
 
 static int __fill_index_info(struct device *cd, void *data)

I will send a new patch to Mauro with this change.

Quoted 2 lines

I'm looking at this patch and I wonder whether it isn't rather inefficient.

IMHO, probe isn't a fast path and simple code beats complex fast code in this case.

Quoted 5 lines

Note that class_for_each_device() didn't appear until 2.6.25, so I've a simple patch pending in my tree that puts all of this under a kernel

Quoted 1 line

= 2.6.25 check.

Backporting class_for_each_device should be trivial since it just uses list_for_each_entry and get/put device.

Cheers,

Brandon

Quoted 218 lines · On Saturday 21 June 2008 00:58:53 [email protected] wrote:

Quoted 217 lines

# HG changeset patch
# User Brandon Philips <[email protected]>
# Date 1214002727 25200
# Node ID 3dbf42455956d17b8aa65bf92319edc3c52b88b8 # Parent 4012ea1a6a06acad9509aee70ee6059af9000a37 [PATCH] v4l: Introduce "index" attribute for persistent video4linux device nodes

A number of V4L drivers have a mod param to specify their preferred minors. This is because it is often desirable for applications to have a static /dev name for a particular device. However, using minors has several disadvantages:

1) the requested minor may already be taken
2) using a mod param is driver specific
3) it requires every driver to add a param
4) requires configuration by hand

This patch introduces an "index" attribute that when combined with udev rules can create static device paths like this:

/dev/v4l/by-path/pci-0000\:00\:1d.2-usb-0\:1\:1.0-video0 /dev/v4l/by-path/pci-0000\:00\:1d.2-usb-0\:1\:1.0-video1 /dev/v4l/by-path/pci-0000\:00\:1d.2-usb-0\:1\:1.0-video2

# ls -la /dev/v4l/by-path/pci-0000\:00\:1d.2-usb-0\:1\:1.0-video0
lrwxrwxrwx 1 root root 12 2008-04-28 00:02
/dev/v4l/by-path/pci-0000:00:1d.2-usb-0:1:1.0-video0 ->
../../video1

These paths are steady across reboots and should be resistant to rearranging across Kernel versions.

video_register_device_index is available to drivers to request a specific index number.

Signed-off-by Brandon Philips <[email protected]>

Signed-off-by Kees Cook <[email protected]>

Signed-off-by Kay Sievers <[email protected]>

---
 linux/drivers/media/video/videodev.c |   98

++++++++++++++++++++++++++++++++++-
linux/include/media/v4l2-dev.h

   |    4 +

 2 files changed, 100 insertions(+), 2 deletions(-)

(quoted diff) +0 -0

b/linux/drivers/media/video/videodev.c ---
a/linux/drivers/media/video/videodev.c
+++ b/linux/drivers/media/video/videodev.c

(quoted diff) +27 -0

@@ -429,6 +429,14 @@ EXPORT_SYMBOL(v4l_printk_ioctl);
  *	sysfs stuff
  */
 
+static ssize_t show_index(struct device *cd,
+			 struct device_attribute *attr, char *buf)
+{
+	struct video_device *vfd = container_of(cd, struct
 video_device, +						class_dev);
+	return sprintf(buf, "%i\n", vfd->index);
+}
+
 #if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,13)
 static ssize_t show_name(struct class_device *cd, char *buf)
 #else

@@ -487,6 +495,7 @@ static void video_release(struct device
 #if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,13)
 static struct device_attribute video_device_attrs[] = {
 	__ATTR(name, S_IRUGO, show_name, NULL),
+	__ATTR(index, S_IRUGO, show_index, NULL),
 	__ATTR_NULL
 };
 #endif

@@ -2042,7 +2051,81 @@ out:
 }
 EXPORT_SYMBOL(video_ioctl2);
 
+struct index_info {
+	struct device *dev;
+	unsigned int used[VIDEO_NUM_DEVICES];
+};
+
+static int __fill_index_info(struct device *cd, void *data)
+{
+	struct index_info *info = data;
+	struct video_device *vfd = container_of(cd, struct
 video_device, +						class_dev);
+
+	if (info->dev == vfd->dev)
+		info->used[vfd->index] = 1;
+
+	return 0;
+}
+
+/**
+ * assign_index - assign stream number based on parent device
+ * @vdev: video_device to assign index number to, vdev->dev

should be assigned + * @num: -1 if auto assign, requested number otherwise + *
+ *
+ * returns -ENFILE if num is already in use, a free index number if + * successful.
+ */

(quoted diff) +44 -0

+static int get_index(struct video_device *vdev, int num)
+{
+	struct index_info *info;
+	int i;
+	int ret = 0;
+
+	if (num >= VIDEO_NUM_DEVICES)
+		return -EINVAL;
+
+	info = kzalloc(sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+
+	info->dev = vdev->dev;
+
+	ret = class_for_each_device(&video_class, info,
+					__fill_index_info);
+
+	if (ret < 0)
+		goto out;
+
+	if (num >= 0) {
+		if (!info->used[num])
+			ret = num;
+		else
+			ret = -ENFILE;
+
+		goto out;
+	}
+
+	for (i = 0; i < VIDEO_NUM_DEVICES; i++) {
+		if (info->used[i])
+			continue;
+		ret = i;
+		goto out;
+	}
+
+out:
+	kfree(info);
+	return ret;
+}
+
 static const struct file_operations video_fops;
+
+int video_register_device(struct video_device *vfd, int type,

int nr) +{

(quoted diff) +3 -1

+	return video_register_device_index(vfd, type, nr, -1);
+}
+EXPORT_SYMBOL(video_register_device);
 
 /**
  *	video_register_device - register video4linux devices

@@ -2069,7 +2152,8 @@ static const struct file_operations vide
  *	%VFL_TYPE_RADIO - A radio card
  */
 
-int video_register_device(struct video_device *vfd, int type,

int nr) +int video_register_device_index(struct video_device
*vfd, int type, int nr, + int index)
{

(quoted diff) +11 -1

 	int i=0;
 	int base;

@@ -2126,6 +2210,16 @@ int video_register_device(struct video_d
 	}
 	video_device[i]=vfd;
 	vfd->minor=i;
+
+	ret = get_index(vfd, index);
+	if (ret < 0) {
+		printk(KERN_ERR "%s: get_index failed\n",
+		       __func__);
+		goto fail_minor;
+	}
+
+	vfd->index = ret;
+
 	mutex_unlock(&videodev_lock);
 	mutex_init(&vfd->lock);
 

@@ -2188,7 +2282,7 @@ fail_minor:
 	mutex_unlock(&videodev_lock);
 	return ret;
 }
-EXPORT_SYMBOL(video_register_device);
+EXPORT_SYMBOL(video_register_device_index);
 
 /**
  *	video_unregister_device - unregister a video4linux device

(quoted diff) +0 -0

b/linux/include/media/v4l2-dev.h ---
a/linux/include/media/v4l2-dev.h +++
b/linux/include/media/v4l2-dev.h

(quoted diff) +1 -0

@@ -112,6 +112,8 @@ struct video_device
 	int type;       /* v4l1 */
 	int type2;      /* v4l2 */
 	int minor;
+	/* attribute to diferentiate multiple indexs on one physical
 device */ +	int index;
 
 	int debug;	/* Activates debug level*/

@@ -377,6 +379,8 @@ void *priv;
 
 /* Version 2 functions */
 extern int video_register_device(struct video_device *vfd, int

type, int nr); +int video_register_device_index(struct
video_device *vfd, int type, int nr, + int index);
void video_unregister_device(struct video_device *); extern int video_ioctl2(struct inode *inode, struct file *file,
unsigned int cmd, unsigned long arg);

Quoted 5 lines

Quoted 5 lines

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

_______________________________________________
v4l-dvb-maintainer mailing list
[email protected]
http://www.linuxtv.org/cgi-bin/mailman/listinfo/v4l-dvb-maintainer

On Monday 23 June 2008 17:07:34 Brandon Philips wrote:
There is also no need to use class_for_each_device(): you can also iterate over the video_device[] array.

So get_index would look something like this:

linux/drivers/media/video/videodev.c static int __fill_index_info(struct devi · line 2029

Quoted 5 lines · Brandon Philips

linux/drivers/media/video/videodev.c +5 -0

+	for (i = 0; i < 32; i++) {
+		if (used & (1 << i))
+			continue;
+		return i;
+	}

	i = ffz(used);
	return i >= 32 ? -ENFILE : i;

Quoted 9 lines · Trent Piepho

linux/drivers/media/video/videodev.c static int __fill_index_info(struct devi · line 2029

Quoted 5 lines · Brandon Philips

linux/drivers/media/video/videodev.c +5 -0

+	for (i = 0; i < 32; i++) {
+		if (used & (1 << i))
+			continue;
+		return i;
+	}

	i = ffz(used);
	return i >= 32 ? -ENFILE : i;

Err. Right :D Tested and pushed.

Mauro-

Updated http://ifup.org/hg/v4l-dvb to have Trent's improvement.

Cheers,

Brandon

Quoted 17 lines · On Wednesday 25 June 2008 00:59:51 Brandon Philips wrote:

Quoted 9 lines · Trent Piepho

linux/drivers/media/video/videodev.c static int __fill_index_info(struct devi · line 2029

Quoted 5 lines · Brandon Philips

linux/drivers/media/video/videodev.c +5 -0

+	for (i = 0; i < 32; i++) {
+		if (used & (1 << i))
+			continue;
+		return i;
+	}

	i = ffz(used);
	return i >= 32 ? -ENFILE : i;

Err. Right :D Tested and pushed.

Mauro-

Updated http://ifup.org/hg/v4l-dvb to have Trent's improvement.

Quoted 3 lines

Cheers,

Brandon

Hi Mauro,

I think you missed this pull request from Brandon. Can you merge this?

Quoted 18 lines · Hans Verkuil

Quoted 17 lines · On Wednesday 25 June 2008 00:59:51 Brandon Philips wrote:

Quoted 9 lines · Trent Piepho

linux/drivers/media/video/videodev.c static int __fill_index_info(struct devi · line 2029

Quoted 5 lines · Brandon Philips

linux/drivers/media/video/videodev.c +5 -0

+	for (i = 0; i < 32; i++) {
+		if (used & (1 << i))
+			continue;
+		return i;
+	}

	i = ffz(used);
	return i >= 32 ? -ENFILE : i;

Err. Right :D Tested and pushed.

Mauro-

Updated http://ifup.org/hg/v4l-dvb to have Trent's improvement.

Quoted 8 lines

Quoted 3 lines

Cheers,

Brandon

Hi Mauro,

I think you missed this pull request from Brandon. Can you merge this?

Yes, I missed that one.

Yet, I didn't like the usage of "32" magic numbers on those parts:

linux/drivers/media/video/videodev.c +3 -1

-       if (num >= VIDEO_NUM_DEVICES)
+
+       if (num >= 32) {
+               printk(KERN_ERR "videodev: %s num is too large\n",

__func__);

+ return i >= 32 ? -ENFILE : i;

It seems better to use VIDEO_NUM_DEVICES as the maximum limit on both usages of "32".

Brandon,

Could you fix and re-send me a pull request?

Quoted 46 lines · On Thursday 17 July 2008 18:40:47 Mauro Carvalho Chehab wrote:

Quoted 18 lines · Hans Verkuil

Quoted 17 lines · On Wednesday 25 June 2008 00:59:51 Brandon Philips wrote:

Quoted 9 lines · Trent Piepho

linux/drivers/media/video/videodev.c static int __fill_index_info(struct devi · line 2029

Quoted 5 lines · Brandon Philips

linux/drivers/media/video/videodev.c +5 -0

+	for (i = 0; i < 32; i++) {
+		if (used & (1 << i))
+			continue;
+		return i;
+	}

	i = ffz(used);
	return i >= 32 ? -ENFILE : i;

Err. Right :D Tested and pushed.

Mauro-

Updated http://ifup.org/hg/v4l-dvb to have Trent's improvement.

Quoted 8 lines

Quoted 3 lines

Cheers,

Brandon

Hi Mauro,

I think you missed this pull request from Brandon. Can you merge this?

Yes, I missed that one.

Yet, I didn't like the usage of "32" magic numbers on those parts:

linux/drivers/media/video/videodev.c +3 -1

-       if (num >= VIDEO_NUM_DEVICES)
+
+       if (num >= 32) {
+               printk(KERN_ERR "videodev: %s num is too large\n",

__func__);

+ return i >= 32 ? -ENFILE : i;

It seems better to use VIDEO_NUM_DEVICES as the maximum limit on both usages of "32".

Brandon,

Quoted 1 line

Could you fix and re-send me a pull request?

Mauro, Brandon,

If you do not mind, then I'll do this. I'm working on videodev.c anyway (making it compatible with kernels <2.6.19) so it's easy for me to do merge this and make the necessary adjustment. And I can test it with a 2.6.18 kernel at the same time.

Quoted 47 lines · On Thursday 17 July 2008 18:44:23 Hans Verkuil wrote:

Quoted 46 lines · On Thursday 17 July 2008 18:40:47 Mauro Carvalho Chehab wrote:

Quoted 18 lines · Hans Verkuil

Quoted 17 lines · On Wednesday 25 June 2008 00:59:51 Brandon Philips wrote:

Quoted 9 lines · Trent Piepho

Review

Quoted 5 lines · Brandon Philips

(quoted diff) +5 -0

+	for (i = 0; i < 32; i++) {
+		if (used & (1 << i))
+			continue;
+		return i;
+	}

	i = ffz(used);
	return i >= 32 ? -ENFILE : i;

Err. Right :D Tested and pushed.

Mauro-

Updated http://ifup.org/hg/v4l-dvb to have Trent's improvement.

Quoted 8 lines

Quoted 3 lines

Cheers,

Brandon

Hi Mauro,

I think you missed this pull request from Brandon. Can you merge this?

Yes, I missed that one.

Yet, I didn't like the usage of "32" magic numbers on those parts:

(quoted diff) +3 -1

-       if (num >= VIDEO_NUM_DEVICES)
+
+       if (num >= 32) {
+               printk(KERN_ERR "videodev: %s num is too large\n",

__func__);

+ return i >= 32 ? -ENFILE : i;

It seems better to use VIDEO_NUM_DEVICES as the maximum limit on both usages of "32".

Brandon,

Quoted 8 lines

Quoted 1 line

Could you fix and re-send me a pull request?

Mauro, Brandon,

If you do not mind, then I'll do this. I'm working on videodev.c anyway (making it compatible with kernels <2.6.19) so it's easy for me to do merge this and make the necessary adjustment. And I can test it with a 2.6.18 kernel at the same time.

Correction, the 32 refers to the number of bits in an u32, not to VIDEO_NUM_DEVICES. So I think you can just merge this patch as is. It does not conflict with my videodev.c changes (amazingly), so it is no problem if you merge this change.

Quoted 49 lines · Hans Verkuil

Quoted 48 lines · On Thursday 17 July 2008 18:44:23 Hans Verkuil wrote:

Quoted 47 lines · On Thursday 17 July 2008 18:40:47 Mauro Carvalho Chehab wrote:

Quoted 18 lines · Hans Verkuil

Quoted 17 lines · On Wednesday 25 June 2008 00:59:51 Brandon Philips wrote:

Quoted 9 lines · Trent Piepho

Review

Quoted 5 lines · Brandon Philips

(quoted diff) +5 -0

+	for (i = 0; i < 32; i++) {
+		if (used & (1 << i))
+			continue;
+		return i;
+	}

	i = ffz(used);
	return i >= 32 ? -ENFILE : i;

Err. Right :D Tested and pushed.

Mauro-

Updated http://ifup.org/hg/v4l-dvb to have Trent's improvement.

Quoted 8 lines

Quoted 3 lines

Cheers,

Brandon

Hi Mauro,

I think you missed this pull request from Brandon. Can you merge this?

Yes, I missed that one.

Yet, I didn't like the usage of "32" magic numbers on those parts:

(quoted diff) +3 -1

-       if (num >= VIDEO_NUM_DEVICES)
+
+       if (num >= 32) {
+               printk(KERN_ERR "videodev: %s num is too

large\n", __func__);

+ return i >= 32 ? -ENFILE : i;

It seems better to use VIDEO_NUM_DEVICES as the maximum limit on both usages of "32".

Brandon,

Quoted 8 lines

Quoted 8 lines

Quoted 1 line

Could you fix and re-send me a pull request?

Mauro, Brandon,

If you do not mind, then I'll do this. I'm working on videodev.c anyway (making it compatible with kernels <2.6.19) so it's easy for me to do merge this and make the necessary adjustment. And I can test it with a 2.6.18 kernel at the same time.

For me, it is OK if you want to touch on it.

Quoted 4 lines

Correction, the 32 refers to the number of bits in an u32, not to VIDEO_NUM_DEVICES. So I think you can just merge this patch as is. It does not conflict with my videodev.c changes (amazingly), so it is no problem if you merge this change.

Hmm... If I understood the patch, if you change VIDEO_NUM_DEVICES to a higher number, you'll still be limited on 32 max devices, right?

If I'm right, then, IMO, we should use VIDEO_NUM_DEVICES and put a notice that increasing this number will require changing some static var from u32 to u64.

Quoted 66 lines · On Thursday 17 July 2008 18:57:49 Mauro Carvalho Chehab wrote:

Quoted 49 lines · Hans Verkuil

Quoted 48 lines · On Thursday 17 July 2008 18:44:23 Hans Verkuil wrote:

Quoted 47 lines · On Thursday 17 July 2008 18:40:47 Mauro Carvalho Chehab wrote:

Quoted 18 lines · Hans Verkuil

Quoted 17 lines · On Wednesday 25 June 2008 00:59:51 Brandon Philips wrote:

Quoted 9 lines · Trent Piepho

Review

Quoted 5 lines · Brandon Philips

(quoted diff) +5 -0

+	for (i = 0; i < 32; i++) {
+		if (used & (1 << i))
+			continue;
+		return i;
+	}

	i = ffz(used);
	return i >= 32 ? -ENFILE : i;

Err. Right :D Tested and pushed.

Mauro-

Updated http://ifup.org/hg/v4l-dvb to have Trent's improvement.

Quoted 8 lines

Quoted 3 lines

Cheers,

Brandon

Hi Mauro,

I think you missed this pull request from Brandon. Can you merge this?

Yes, I missed that one.

Yet, I didn't like the usage of "32" magic numbers on those parts:

(quoted diff) +3 -1

-       if (num >= VIDEO_NUM_DEVICES)
+
+       if (num >= 32) {
+               printk(KERN_ERR "videodev: %s num is too

large\n", __func__);

+ return i >= 32 ? -ENFILE : i;

It seems better to use VIDEO_NUM_DEVICES as the maximum limit on both usages of "32".

Brandon,

Quoted 8 lines

Quoted 8 lines

Quoted 1 line

Could you fix and re-send me a pull request?

Mauro, Brandon,

If you do not mind, then I'll do this. I'm working on videodev.c anyway (making it compatible with kernels <2.6.19) so it's easy for me to do merge this and make the necessary adjustment. And I can test it with a 2.6.18 kernel at the same time.

For me, it is OK if you want to touch on it.

Quoted 4 lines

Correction, the 32 refers to the number of bits in an u32, not to VIDEO_NUM_DEVICES. So I think you can just merge this patch as is. It does not conflict with my videodev.c changes (amazingly), so it is no problem if you merge this change.

Quoted 2 lines

Hmm... If I understood the patch, if you change VIDEO_NUM_DEVICES to a higher number, you'll still be limited on 32 max devices, right?

No. The value of 32 refers to the maximum number of devices (/dev/vbiX, /dev/videoX, etc) created for ONE driver instance. E.g. my first ivtv card can create up to 32 devices, my second bttv card can create up to 32 devices, etc. etc.

The maximum currently in use is probably ivtv with 9 devices for a PVR-350 card. So we are safe with 32.

VIDEO_NUM_DEVICES refers to the TOTAL number of video4linux devices created. I have some future plans to enlarge that and possibly also getting rid of how the minor numbers are allocated. Currently this is very unfair with a range of 64 for radio devices and an equal number of video devices, although each ivtv card creates 3 video devices against one radio device (and 5 for a PVR-350 card!). Much better to just pick the first free minor number.

Regards,

Hans

Quoted 3 lines

If I'm right, then, IMO, we should use VIDEO_NUM_DEVICES and put a notice that increasing this number will require changing some static var from u32 to u64.

Quoted 68 lines · Hans Verkuil

Quoted 67 lines · On Thursday 17 July 2008 18:57:49 Mauro Carvalho Chehab wrote:

Quoted 50 lines · Hans Verkuil

Quoted 49 lines · On Thursday 17 July 2008 18:44:23 Hans Verkuil wrote:

Quoted 48 lines · On Thursday 17 July 2008 18:40:47 Mauro Carvalho Chehab wrote:

Quoted 19 lines · Hans Verkuil

Quoted 18 lines · On Wednesday 25 June 2008 00:59:51 Brandon Philips wrote:

Quoted 9 lines · Trent Piepho

Review

Quoted 5 lines · Brandon Philips

(quoted diff) +5 -0

+	for (i = 0; i < 32; i++) {
+		if (used & (1 << i))
+			continue;
+		return i;
+	}

	i = ffz(used);
	return i >= 32 ? -ENFILE : i;

Err. Right :D Tested and pushed.

Mauro-

Updated http://ifup.org/hg/v4l-dvb to have Trent's improvement.

Quoted 8 lines

Quoted 3 lines

Cheers,

Brandon

Hi Mauro,

I think you missed this pull request from Brandon. Can you merge this?

Yes, I missed that one.

Yet, I didn't like the usage of "32" magic numbers on those parts:

(quoted diff) +3 -1

-       if (num >= VIDEO_NUM_DEVICES)
+
+       if (num >= 32) {
+               printk(KERN_ERR "videodev: %s num is too

large\n", __func__);

+ return i >= 32 ? -ENFILE : i;

It seems better to use VIDEO_NUM_DEVICES as the maximum limit on both usages of "32".

Brandon,

Quoted 8 lines

Quoted 8 lines

Quoted 1 line

Could you fix and re-send me a pull request?

Mauro, Brandon,

If you do not mind, then I'll do this. I'm working on videodev.c anyway (making it compatible with kernels <2.6.19) so it's easy for me to do merge this and make the necessary adjustment. And I can test it with a 2.6.18 kernel at the same time.

For me, it is OK if you want to touch on it.

Quoted 4 lines

Correction, the 32 refers to the number of bits in an u32, not to VIDEO_NUM_DEVICES. So I think you can just merge this patch as is. It does not conflict with my videodev.c changes (amazingly), so it is no problem if you merge this change.

Quoted 8 lines

Quoted 3 lines

Hmm... If I understood the patch, if you change VIDEO_NUM_DEVICES to a higher number, you'll still be limited on 32 max devices, right?

No. The value of 32 refers to the maximum number of devices (/dev/vbiX, /dev/videoX, etc) created for ONE driver instance. E.g. my first ivtv card can create up to 32 devices, my second bttv card can create up to 32 devices, etc. etc.

So, IMO, the better would be to do something like this:
sizeof(u32) * 8

Instead of a magic number. I would also add some comments about this.

Quoted 18 lines

The maximum currently in use is probably ivtv with 9 devices for a PVR-350 card. So we are safe with 32.

VIDEO_NUM_DEVICES refers to the TOTAL number of video4linux devices created. I have some future plans to enlarge that and possibly also getting rid of how the minor numbers are allocated. Currently this is very unfair with a range of 64 for radio devices and an equal number of video devices, although each ivtv card creates 3 video devices against one radio device (and 5 for a PVR-350 card!). Much better to just pick the first free minor number.

Regards,

Hans

Quoted 3 lines

If I'm right, then, IMO, we should use VIDEO_NUM_DEVICES and put a notice that increasing this number will require changing some static var from u32 to u64.

Mauro,

I'll merge Brandon's patch and add a #define and comments afterwards. It's a bit overkill IMHO but discussing this takes longer than just putting in the code :-)

Quoted 3 lines · Hans Verkuil

Or if that's not possible for some reason, at least avoid the expensive struct allocation and simply use a bitarray allocated on the stack (max 256 devices = 32 bytes)?

Using ffz and set_bit would be an option (since bitfields can't be used on arrays) but I don't think the savings would be worth the effort since we would need to use division and an array if VIDEO_NUM_DEVICES grows past 256.