Hi Hans,
On Thursday 01 April 2010 13:11:51 Hans Verkuil wrote:
> On Thursday 01 April 2010 11:23:30 Laurent Pinchart wrote:
> > On Thursday 01 April 2010 10:01:10 Hans Verkuil wrote:
> > > Hi all,
> > >
> > > I just read on LWN that the core kernel guys are putting more effort
> > > into removing the BKL. We are still using it in our own drivers,
> > > mostly V4L.
> > >
> > > I added a BKL column to my driver list:
> > >
> > > http://www.linuxtv.org/wiki/index.php/V4L_framework_progress#Bridge_Dri
> > > vers
> > >
> > > If you 'own' one of these drivers that still use BKL, then it would be
> > > nice if you can try and remove the use of the BKL from those drivers.
> > >
> > > The other part that needs to be done is to move from using the .ioctl
> > > file op to using .unlocked_ioctl. Very few drivers do that, but I
> > > suspect almost no driver actually needs to use .ioctl.
> >
> > What about something like this patch as a first step ?
>
> That doesn't fix anything. You just move the BKL from one place to another.
> I don't see any benefit from that.
Removing the BKL is a long-term project that basically pushes the BKL from
core code to subsystems and drivers, and then replace it on a case by case
basis. This patch (along with a replacement of lock_kernel/unlock_kernel by a
V4L-specific lock) goes into that direction and removes the BKL usage from V4L
ioctls. The V4L lock would then need to be pushed into individual drivers.
> > diff --git a/drivers/media/video/v4l2-dev.c
> > b/drivers/media/video/v4l2-dev.c index 7090699..14e1b1c 100644
> > --- a/drivers/media/video/v4l2-dev.c
> > +++ b/drivers/media/video/v4l2-dev.c
> > @@ -25,6 +25,7 @@
> >
> > #include <linux/init.h>
> > #include <linux/kmod.h>
> > #include <linux/slab.h>
> >
> > +#include <linux/smp_lock.h>
> >
> > #include <asm/uaccess.h>
> > #include <asm/system.h>
> >
> > @@ -215,28 +216,22 @@ static unsigned int v4l2_poll(struct file *filp,
> > struct poll_table_struct *poll)
> >
> > return vdev->fops->poll(filp, poll);
> >
> > }
> >
> > -static int v4l2_ioctl(struct inode *inode, struct file *filp,
> > - unsigned int cmd, unsigned long arg)
> > -{
> > - struct video_device *vdev = video_devdata(filp);
> > -
> > - if (!vdev->fops->ioctl)
> > - return -ENOTTY;
> > - /* Allow ioctl to continue even if the device was unregistered.
> > - Things like dequeueing buffers might still be useful. */
> > - return vdev->fops->ioctl(filp, cmd, arg);
> > -}
> > -
> >
> > static long v4l2_unlocked_ioctl(struct file *filp,
> >
> > unsigned int cmd, unsigned long arg)
> >
> > {
> >
> > struct video_device *vdev = video_devdata(filp);
> >
> > + int ret = -ENOTTY;
> >
> > - if (!vdev->fops->unlocked_ioctl)
> > - return -ENOTTY;
> >
> > /* Allow ioctl to continue even if the device was unregistered.
> >
> > Things like dequeueing buffers might still be useful. */
> >
> > - return vdev->fops->unlocked_ioctl(filp, cmd, arg);
> > + if (vdev->fops->ioctl) {
> > + lock_kernel();
> > + ret = vdev->fops->ioctl(filp, cmd, arg);
> > + unlock_kernel();
> > + } else if (vdev->fops->unlocked_ioctl)
> > + ret = vdev->fops->unlocked_ioctl(filp, cmd, arg);
> > +
> > + return ret;
> >
> > }
> >
> > #ifdef CONFIG_MMU
> >
> > @@ -323,22 +318,6 @@ static const struct file_operations
> > v4l2_unlocked_fops = {
> >
> > .llseek = no_llseek,
> >
> > };
> >
> > -static const struct file_operations v4l2_fops = {
> > - .owner = THIS_MODULE,
> > - .read = v4l2_read,
> > - .write = v4l2_write,
> > - .open = v4l2_open,
> > - .get_unmapped_area = v4l2_get_unmapped_area,
> > - .mmap = v4l2_mmap,
> > - .ioctl = v4l2_ioctl,
> > -#ifdef CONFIG_COMPAT
> > - .compat_ioctl = v4l2_compat_ioctl32,
> > -#endif
> > - .release = v4l2_release,
> > - .poll = v4l2_poll,
> > - .llseek = no_llseek,
> > -};
> > -
> >
> > /**
> >
> > * get_index - assign stream index number based on parent device
> > * @vdev: video_device to assign index number to, vdev->parent should be
> > assigned
> >
> > @@ -517,10 +496,7 @@ static int __video_register_device(struct
> > video_device *vdev, int type, int nr,
> >
> > ret = -ENOMEM;
> > goto cleanup;
> >
> > }
> >
> > - if (vdev->fops->unlocked_ioctl)
> > - vdev->cdev->ops = &v4l2_unlocked_fops;
> > - else
> > - vdev->cdev->ops = &v4l2_fops;
> > + vdev->cdev->ops = &v4l2_unlocked_fops;
> >
> > vdev->cdev->owner = vdev->fops->owner;
> > ret = cdev_add(vdev->cdev, MKDEV(VIDEO_MAJOR, vdev->minor), 1);
> > if (ret < 0) {
> >
> > A second step would be to replace lock_kernel/unlock_kernel with a
> > V4L-specific lock, and the third step to push the lock into drivers.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
