[PATCH] [PATCH] v4l: Introduce "index" attribute for persistent video4linux device nodes

[PATCH] [PATCH] v4l: Introduce "index" attribute for persistent video4linux device nodes

[brandon]

# HG changeset patch
# User Brandon Philips <brandon@ifup.org>
# 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 <bphilips@suse.de>
Signed-off-by: Kees Cook <kees@outflux.net>
Signed-off-by: Kay Sievers <kay.sievers@vrfy.org>

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

diff --git a/linux/drivers/media/video/videodev.c b/linux/drivers/media/video/videodev.c
--- a/linux/drivers/media/video/videodev.c
+++ b/linux/drivers/media/video/videodev.c
@@ -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.
+ */
+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)
+{
+	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)
 {
 	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
diff --git a/linux/include/media/v4l2-dev.h b/linux/include/media/v4l2-dev.h
--- a/linux/include/media/v4l2-dev.h
+++ b/linux/include/media/v4l2-dev.h
@@ -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:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] [PATCH] v4l: Introduce "index" attribute for persistent video4linux device nodes

[Hans Verkuil]

Hi Brandon,

I'm looking at this patch and I wonder whether it isn't rather 
inefficient. 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)?

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 >= 
2.6.25 check.

Regards,

	Hans

On Saturday 21 June 2008 00:58:53 brandon@ifup.org wrote:
> # HG changeset patch
> # User Brandon Philips <brandon@ifup.org>
> # 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 <bphilips@suse.de>
> Signed-off-by: Kees Cook <kees@outflux.net>
> Signed-off-by: Kay Sievers <kay.sievers@vrfy.org>
>
> ---
>  linux/drivers/media/video/videodev.c |   98
> ++++++++++++++++++++++++++++++++++- linux/include/media/v4l2-dev.h   
>    |    4 +
>  2 files changed, 100 insertions(+), 2 deletions(-)
>
> diff --git a/linux/drivers/media/video/videodev.c
> b/linux/drivers/media/video/videodev.c ---
> a/linux/drivers/media/video/videodev.c
> +++ b/linux/drivers/media/video/videodev.c
> @@ -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.
> + */
> +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) +{
> +	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)
>  {
>  	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
> diff --git a/linux/include/media/v4l2-dev.h
> b/linux/include/media/v4l2-dev.h --- a/linux/include/media/v4l2-dev.h
> +++ b/linux/include/media/v4l2-dev.h
> @@ -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:video4linux-list-request@redhat.com?subject=unsubscribe
> https://www.redhat.com/mailman/listinfo/video4linux-list


--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [v4l-dvb-maintainer] [PATCH] [PATCH] v4l: Introduce "index" attribute for persistent video4linux device nodes

[Trent Piepho]

On Sun, 22 Jun 2008, Hans Verkuil wrote:
> 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?

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] [PATCH] v4l: Introduce "index" attribute for persistent video4linux device nodes

[Brandon Philips]

On 13:34 Sun 22 Jun 2008, Hans Verkuil wrote:
> 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.

> 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.

> 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.

diff --git a/linux/drivers/media/video/videodev.c b/linux/drivers/media/video/videodev.c
--- a/linux/drivers/media/video/videodev.c
+++ b/linux/drivers/media/video/videodev.c
@@ -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.

> 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.

> 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
> >= 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

> On Saturday 21 June 2008 00:58:53 brandon@ifup.org wrote:
> > # HG changeset patch
> > # User Brandon Philips <brandon@ifup.org>
> > # 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 <bphilips@suse.de>
> > Signed-off-by: Kees Cook <kees@outflux.net>
> > Signed-off-by: Kay Sievers <kay.sievers@vrfy.org>
> >
> > ---
> >  linux/drivers/media/video/videodev.c |   98
> > ++++++++++++++++++++++++++++++++++- linux/include/media/v4l2-dev.h   
> >    |    4 +
> >  2 files changed, 100 insertions(+), 2 deletions(-)
> >
> > diff --git a/linux/drivers/media/video/videodev.c
> > b/linux/drivers/media/video/videodev.c ---
> > a/linux/drivers/media/video/videodev.c
> > +++ b/linux/drivers/media/video/videodev.c
> > @@ -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.
> > + */
> > +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) +{
> > +	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)
> >  {
> >  	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
> > diff --git a/linux/include/media/v4l2-dev.h
> > b/linux/include/media/v4l2-dev.h --- a/linux/include/media/v4l2-dev.h
> > +++ b/linux/include/media/v4l2-dev.h
> > @@ -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:video4linux-list-request@redhat.com?subject=unsubscribe
> > https://www.redhat.com/mailman/listinfo/video4linux-list
> 
> 

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [v4l-dvb-maintainer] [PATCH] [PATCH] v4l: Introduce "index" attribute for persistent video4linux device nodes

[Brandon Philips]

On 00:05 Mon 23 Jun 2008, Trent Piepho wrote:
> On Sun, 22 Jun 2008, Hans Verkuil wrote:
> > 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?

Wait, how would that help in this case?  The index is not a minor.

The index is a number that increases for each videodev added to a
physical device.

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [v4l-dvb-maintainer] [PATCH] [PATCH] v4l: Introduce "index" attribute for persistent video4linux device nodes

[Hans Verkuil]

On Monday 23 June 2008 17:07:34 Brandon Philips wrote:
> On 13:34 Sun 22 Jun 2008, Hans Verkuil wrote:
> > 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.
>
> > 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.

I hadn't realized it's indexed per physical device. But we don't need 
VIDEO_NUM_DEVICES for that, an u32 is enough for now. There are no 
physical devices at the moment that register more than 9 devices I 
think.

> > 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.

The cost of using set_bit etc. is close to zero compared to the cost of 
doing a kzalloc/kfree each time.

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:

static int get_index(struct video_device *vdev, int num)
{
	u32 used = 0;
        int i;

        if (num >= 32)
                return -EINVAL;

	for (i = 0; i < VIDEO_NUM_DEVICES; i++) {
		if (video_device[i] != NULL &&
		    video_device[i] != vdev &&
		    video_device[i]->dev == vdev->dev) {
			used |= 1 << video_device[i]->index;
		}
	}

        if (num >= 0) {
                if (used & (1 << num))
                        return -ENFILE;
		return num;
        }

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

Untested code, but it looks much simpler to me.

> I think changing "used" to a uchar is a good choice though.
>
> diff --git a/linux/drivers/media/video/videodev.c
> b/linux/drivers/media/video/videodev.c ---
> a/linux/drivers/media/video/videodev.c
> +++ b/linux/drivers/media/video/videodev.c
> @@ -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.
>
> > 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.

I agree, but I think it is too complicated for what it should do.

> > 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
> >
> > >= 2.6.25 check.
>
> Backporting class_for_each_device should be trivial since it just
> uses list_for_each_entry and get/put device.

Let me know what you think of my approach (and if it works!).

Regards,

	Hans

>
> Cheers,
>
> 	Brandon
>
> > On Saturday 21 June 2008 00:58:53 brandon@ifup.org wrote:
> > > # HG changeset patch
> > > # User Brandon Philips <brandon@ifup.org>
> > > # 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 <bphilips@suse.de>
> > > Signed-off-by: Kees Cook <kees@outflux.net>
> > > Signed-off-by: Kay Sievers <kay.sievers@vrfy.org>
> > >
> > > ---
> > >  linux/drivers/media/video/videodev.c |   98
> > > ++++++++++++++++++++++++++++++++++-
> > > linux/include/media/v4l2-dev.h
> > >
> > >    |    4 +
> > >
> > >  2 files changed, 100 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/linux/drivers/media/video/videodev.c
> > > b/linux/drivers/media/video/videodev.c ---
> > > a/linux/drivers/media/video/videodev.c
> > > +++ b/linux/drivers/media/video/videodev.c
> > > @@ -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.
> > > + */
> > > +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) +{
> > > +	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)
> > >  {
> > >  	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
> > > diff --git a/linux/include/media/v4l2-dev.h
> > > b/linux/include/media/v4l2-dev.h ---
> > > a/linux/include/media/v4l2-dev.h +++
> > > b/linux/include/media/v4l2-dev.h
> > > @@ -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:video4linux-list-request@redhat.com?subject=unsubscribe
> > > https://www.redhat.com/mailman/listinfo/video4linux-list
>
> _______________________________________________
> v4l-dvb-maintainer mailing list
> v4l-dvb-maintainer@linuxtv.org
> http://www.linuxtv.org/cgi-bin/mailman/listinfo/v4l-dvb-maintainer


--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [v4l-dvb-maintainer] [PATCH] [PATCH] v4l: Introduce "index" attribute for persistent video4linux device nodes

[Trent Piepho]

On Mon, 23 Jun 2008, Brandon Philips wrote:
> On 13:34 Sun 22 Jun 2008, Hans Verkuil wrote:
> > 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.

unsigned long used[VIDEO_NUM_DEVICES / (sizeof(used[0])*8)];

for_each_device {
    set_bit(index, used);
}

empty = find_first_zero_bit(used, VIDEO_NUM_DEVICES);

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [v4l-dvb-maintainer] [PATCH] [PATCH] v4l: Introduce "index" attribute for?persistent video4linux device nodes

[Brandon Philips]

On 18:00 Mon 23 Jun 2008, Hans Verkuil wrote:
> 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:

Your code compiles, looks correct and is simpler.  It tested OK on my
single node device but I don't have any multiple node devices like a
ivtv.  Testers welcome ;)

Mauro had already committed my patch to his branch so I just made the
change on top of that.

Mauro you can pull this patch from here: http://ifup.org/hg/v4l-dvb

Cheers,

	Brandon

changeset:   8111:e39be24dd6a0
tag:         tip
user:        Brandon Philips <brandon@ifup.org>
date:        Mon Jun 23 16:33:06 2008 -0700
files:       linux/drivers/media/video/videodev.c
description:
videodev: simplify get_index()

Use Hans Verkuil's suggested method of implementing get_index which doesn't
depend on class_for_each_device and instead uses the video_device array.  This
simplifies the code and reduces its memory footprint.

Signed-off-by: Brandon Philips <bphilips@suse.de>

diff --git a/linux/drivers/media/video/videodev.c b/linux/drivers/media/video/videodev.c
--- a/linux/drivers/media/video/videodev.c
+++ b/linux/drivers/media/video/videodev.c
@@ -1989,26 +1989,8 @@ out:
 }
 EXPORT_SYMBOL(video_ioctl2);
 
-#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,25)
-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
+ * get_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
  *
@@ -2018,46 +2000,35 @@ static int __fill_index_info(struct devi
  */
 static int get_index(struct video_device *vdev, int num)
 {
-	struct index_info *info;
+	u32 used = 0;
 	int i;
-	int ret = 0;
 
-	if (num >= VIDEO_NUM_DEVICES)
+	if (num >= 32) {
+		printk(KERN_ERR "videodev: %s num is too large\n", __func__);
 		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;
+		if (video_device[i] != NULL &&
+		    video_device[i] != vdev &&
+		    video_device[i]->dev == vdev->dev) {
+			used |= 1 << video_device[i]->index;
+		}
 	}
 
-out:
-	kfree(info);
-	return ret;
+	if (num >= 0) {
+		if (used & (1 << num))
+			return -ENFILE;
+		return num;
+	}
+
+	for (i = 0; i < 32; i++) {
+		if (used & (1 << i))
+			continue;
+		return i;
+	}
+	return -ENFILE;
 }
-#endif
 
 static const struct file_operations video_fops;
 
@@ -2151,11 +2122,7 @@ int video_register_device_index(struct v
 	video_device[i]=vfd;
 	vfd->minor=i;
 
-#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,25)
 	ret = get_index(vfd, index);
-#else
-	ret = 0;
-#endif
 	if (ret < 0) {
 		printk(KERN_ERR "%s: get_index failed\n",
 		       __func__);

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [v4l-dvb-maintainer] [PATCH] [PATCH] v4l: Introduce "index" attribute for?persistent video4linux device nodes

[Trent Piepho]

On Mon, 23 Jun 2008, Brandon Philips wrote:
> +	for (i = 0; i < 32; i++) {
> +		if (used & (1 << i))
> +			continue;
> +		return i;
> +	}

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

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [v4l-dvb-maintainer] [PATCH] [PATCH] v4l: Introduce "index" attribute for?persistent video4linux device nodes

[Brandon Philips]

On 00:34 Tue 24 Jun 2008, Trent Piepho wrote:
> On Mon, 23 Jun 2008, Brandon Philips wrote:
> > +	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

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [v4l-dvb-maintainer] [PATCH] [PATCH] v4l: Introduce "index" attribute for?persistent video4linux device nodes

[Hans Verkuil]

On Wednesday 25 June 2008 00:59:51 Brandon Philips wrote:
> On 00:34 Tue 24 Jun 2008, Trent Piepho wrote:
> > On Mon, 23 Jun 2008, Brandon Philips wrote:
> > > +	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


Hi Mauro,

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

Thanks,

	Hans

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [v4l-dvb-maintainer] [PATCH] [PATCH] v4l: Introduce "index" attribute for?persistent video4linux device nodes

[Mauro Carvalho Chehab]

On Thu, 17 Jul 2008, Hans Verkuil wrote:

> On Wednesday 25 June 2008 00:59:51 Brandon Philips wrote:
>> On 00:34 Tue 24 Jun 2008, Trent Piepho wrote:
>>> On Mon, 23 Jun 2008, Brandon Philips wrote:
>>>> +	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
>
>
> 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:

-       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?

-- 
Cheers,
Mauro Carvalho Chehab
http://linuxtv.org
mchehab@infradead.org

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [v4l-dvb-maintainer] [PATCH] [PATCH] v4l: Introduce "index" attribute for?persistent video4linux device nodes

[Hans Verkuil]

On Thursday 17 July 2008 18:40:47 Mauro Carvalho Chehab wrote:
> On Thu, 17 Jul 2008, Hans Verkuil wrote:
> > On Wednesday 25 June 2008 00:59:51 Brandon Philips wrote:
> >> On 00:34 Tue 24 Jun 2008, Trent Piepho wrote:
> >>> On Mon, 23 Jun 2008, Brandon Philips wrote:
> >>>> +	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
> >
> > 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:
>
> -       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?

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.

Regards,

	Hans

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [v4l-dvb-maintainer] [PATCH] [PATCH] v4l: Introduce "index" attribute for?persistent video4linux device nodes

[Hans Verkuil]

On Thursday 17 July 2008 18:44:23 Hans Verkuil wrote:
> On Thursday 17 July 2008 18:40:47 Mauro Carvalho Chehab wrote:
> > On Thu, 17 Jul 2008, Hans Verkuil wrote:
> > > On Wednesday 25 June 2008 00:59:51 Brandon Philips wrote:
> > >> On 00:34 Tue 24 Jun 2008, Trent Piepho wrote:
> > >>> On Mon, 23 Jun 2008, Brandon Philips wrote:
> > >>>> +	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
> > >
> > > 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:
> >
> > -       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?
>
> 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.

Regards,

	Hans

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [v4l-dvb-maintainer] [PATCH] [PATCH] v4l: Introduce "index" attribute for?persistent video4linux device nodes

[Mauro Carvalho Chehab]

On Thu, 17 Jul 2008, Hans Verkuil wrote:

> On Thursday 17 July 2008 18:44:23 Hans Verkuil wrote:
>> On Thursday 17 July 2008 18:40:47 Mauro Carvalho Chehab wrote:
>>> On Thu, 17 Jul 2008, Hans Verkuil wrote:
>>>> On Wednesday 25 June 2008 00:59:51 Brandon Philips wrote:
>>>>> On 00:34 Tue 24 Jun 2008, Trent Piepho wrote:
>>>>>> On Mon, 23 Jun 2008, Brandon Philips wrote:
>>>>>>> +	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
>>>>
>>>> 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:
>>>
>>> -       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?
>>
>> 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.

> 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.

-- 
Cheers,
Mauro Carvalho Chehab
http://linuxtv.org
mchehab@infradead.org

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [v4l-dvb-maintainer] [PATCH] [PATCH] v4l: Introduce "index" attribute for?persistent video4linux device nodes

[Hans Verkuil]

On Thursday 17 July 2008 18:57:49 Mauro Carvalho Chehab wrote:
> On Thu, 17 Jul 2008, Hans Verkuil wrote:
> > On Thursday 17 July 2008 18:44:23 Hans Verkuil wrote:
> >> On Thursday 17 July 2008 18:40:47 Mauro Carvalho Chehab wrote:
> >>> On Thu, 17 Jul 2008, Hans Verkuil wrote:
> >>>> On Wednesday 25 June 2008 00:59:51 Brandon Philips wrote:
> >>>>> On 00:34 Tue 24 Jun 2008, Trent Piepho wrote:
> >>>>>> On Mon, 23 Jun 2008, Brandon Philips wrote:
> >>>>>>> +	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
> >>>>
> >>>> 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:
> >>>
> >>> -       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?
> >>
> >> 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.
>
> > 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?

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

> 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.

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [v4l-dvb-maintainer] [PATCH] [PATCH] v4l: Introduce "index" attribute for?persistent video4linux device nodes

[Mauro Carvalho Chehab]

On Thu, 17 Jul 2008, Hans Verkuil wrote:

> On Thursday 17 July 2008 18:57:49 Mauro Carvalho Chehab wrote:
>> On Thu, 17 Jul 2008, Hans Verkuil wrote:
>>> On Thursday 17 July 2008 18:44:23 Hans Verkuil wrote:
>>>> On Thursday 17 July 2008 18:40:47 Mauro Carvalho Chehab wrote:
>>>>> On Thu, 17 Jul 2008, Hans Verkuil wrote:
>>>>>> On Wednesday 25 June 2008 00:59:51 Brandon Philips wrote:
>>>>>>> On 00:34 Tue 24 Jun 2008, Trent Piepho wrote:
>>>>>>>> On Mon, 23 Jun 2008, Brandon Philips wrote:
>>>>>>>>> +	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
>>>>>>
>>>>>> 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:
>>>>>
>>>>> -       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?
>>>>
>>>> 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.
>>
>>> 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?
>
> 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.

>
> 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
>
>> 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.
>
>

-- 
Cheers,
Mauro Carvalho Chehab
http://linuxtv.org
mchehab@infradead.org

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [v4l-dvb-maintainer] [PATCH] [PATCH] v4l: Introduce "index" attribute for?persistent video4linux device nodes

[Hans Verkuil]

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 :-)

Regards,

	Hans

On Thursday 17 July 2008 19:35:55 Mauro Carvalho Chehab wrote:
> On Thu, 17 Jul 2008, Hans Verkuil wrote:
> > On Thursday 17 July 2008 18:57:49 Mauro Carvalho Chehab wrote:
> >> On Thu, 17 Jul 2008, Hans Verkuil wrote:
> >>> On Thursday 17 July 2008 18:44:23 Hans Verkuil wrote:
> >>>> On Thursday 17 July 2008 18:40:47 Mauro Carvalho Chehab wrote:
> >>>>> On Thu, 17 Jul 2008, Hans Verkuil wrote:
> >>>>>> On Wednesday 25 June 2008 00:59:51 Brandon Philips wrote:
> >>>>>>> On 00:34 Tue 24 Jun 2008, Trent Piepho wrote:
> >>>>>>>> On Mon, 23 Jun 2008, Brandon Philips wrote:
> >>>>>>>>> +	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
> >>>>>>
> >>>>>> 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:
> >>>>>
> >>>>> -       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?
> >>>>
> >>>> 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.
> >>
> >>> 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?
> >
> > 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.
>
> > 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
> >
> >> 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.


--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [v4l-dvb-maintainer] [PATCH] [PATCH] v4l: Introduce "index" attribute for?persistent video4linux device nodes

[Mauro Carvalho Chehab]

On Thu, 17 Jul 2008, Hans Verkuil wrote:

> 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 :-)

Seems fine to me.

As videodev is the core for V4L devices, it is never an overkill to make 
the code as readable as possible.

-- 
Cheers,
Mauro Carvalho Chehab
http://linuxtv.org
mchehab@infradead.org

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list