Message ID | 4E402D61.103@redhat.com (mailing list archive) |
---|---|
State | Not Applicable |
Headers | show |
On Mon, 8 Aug 2011, Mauro Carvalho Chehab wrote: > Em 08-08-2011 14:39, Theodore Kilgore escreveu: > > > > > > On Mon, 8 Aug 2011, Mauro Carvalho Chehab wrote: > > > >> Em 07-08-2011 23:26, Theodore Kilgore escreveu: > >>> > >>> (first of two replies to Adam's message; second reply deals with other > >>> topics) > >> In summary, there are currently two proposals: > >> > >> 1) a resource lock for USB interface between V4L and libusb; > >> > >> 2) a PTP-like USB driver, plus a resource lock between V4L and the PTP-like driver. > >> The same resource lock may also be implemented at libusb, in order to avoid > >> concurrency. > >> > >> As you said that streaming on some cameras may delete all pictures from it, > >> I suspect that (2) is the best alternative. > >> > >> Thanks, > >> Mauro > >> > > > > Mauro, > > > > In fact none of the currently known and supported cameras are using PTP. > > All of them are proprietary. They have a rather intimidating set of > > differences in functionality, too. Namely, some of them have an > > isochronous endpoint, and some of them rely exclusively upon bulk > > transport. Some of them have a well developed set of internal capabilities > > as far as handling still photos are concerned. I mean, such things as the > > ability to download a single photo, selected at random from the set of > > photos on the camera, and some do not, requiring that the "ability" to do > > this is emulated in software -- by first downloading all previously listed > > photos and sending the data to /dev/null, then downloading the desired > > photo and saving it. Some of them permit deletion of individual photos, or > > all photos, and some do not. For some of them it is even true, as I have > > previously mentioned, that the USB command string which will delete all > > photos is the same command used for starting the camera in streaming mode. > > > > But the point here is that these cameras are all different from one > > another, depending upon chipset and even, sometimes, upon firmware > > or chipset version. The still camera abilities and limitations of all of > > them are pretty much worked out in libgphoto2. My suggestion would be that > > the libgphoto2 support libraries for these cameras ought to be left the > > hell alone, except for some changes in, for example, how the camera is > > accessed in the first place (through libusb or through a kernel device) in > > order to address adequately the need to support both modes. I know what is > > in those libgphoto2 drivers because I wrote them. I can definitely promise > > that to move all of that functionality over into kernel modules would be a > > nightmare and would moreover greatly contribute to kernel bloat. You > > really don't want to go there. > > > > As to whether to use libusb or not to use libusb: > > > > It would be very nice to be able to keep using libusb to get access to > > these cameras, as then no change in the existing stillcam drivers would be > > required at all. Furthermore, if it were possible to solve all of the > > associated locking problems and to do it this way, it would be something > > that could be generalized to any analogous situation. > > > This would be very nice. I can also imagine, of course, that such an > > approach might require changes in libusb. For example, the current ability > > of libusb itself to switch off a kernel device might possibly be a step in > > the wrong direction, and it might possibly be needed to move that > > function, somehow, out of libusb and into the kernel support for affected > > hardware. > > > > In the alternative, it ought to be possible for a libgphoto2 driver to > > hook up directly to a kernel-created device without going through libusb, > > and, as I have said in earlier messages, some of our driver code (for > > digital picture frames in particular) does just that. Then, whatever /dev > > entries and associated locking problems are needed could be handled by the > > kernel, and libgphoto2 talks to the device. But if things are done this > > way I strongly suggest that as little of the internals of the libgphoto2 > > driver are put in the kernel as it is possible to do. Be very economical > > about that, else there will be a big mess. > > Doing an specific libusb-like approach just for those cams seems to be the > wrong direction, as such driver would be just a fork of an already existing > code. I'm all against duplicating it. Well, in practice the "fork" would presumably be carried out by yours truly. Presumably with the advice and help of concerned parties. too. Since I am involved on both the kernel side and the libgphoto2 side of the support for the same cameras, it would certainly shorten the lines of communication at the very least. Therefore this is not infeasible. > > So, either we need to move the code from libgphoto2 to kernel As I said, I think you don't want to do that. or work into > an approach that will make libusb (or an appropriate substitute) to return -EBUSY when a driver like V4L > is in usage, and vice-versa. > > I never took a look on how libusb works. It seems that the logic for it is > at drivers/usb/core/devio.c. Assuming that this is correct driver for libusb, > the locking patch would be similar to the enclosed one. > > Of course, more work will be needed. For example, in the specific case of > devices where starting stream will clean the memory data, the V4L driver > will need some additional logic to detect if the memory is filled and not > allowing stream (or requiring CAP_SYS_ADMIN, returning -EPERM otherwise). Yes, this is probably a good idea in any event. As far as I know, this would affect just one kernel driver. A complication is that it is only some of the cameras supported by that driver, and they need to be detected. > > Thanks, > Mauro > > - > > Add a hardware resource locking schema at the Kernel > > Sometimes, a hardware resource is used by more than one device driver. This > causes troubles, as sometimes, using the resource by one driver is mutually > exclusive than using the same resource by another driver. > > Adds a resource locking schema that will avoid such troubles. > > TODO: This is just a quick hack prototyping the a real solution. The > namespace there is not consistent, nor the actual code that locks the > resource is provided. > > NOTE: As the problem also happens with some PCI devices, instead of adding > such locking schema at usb_device, it seems better to bind whatever > solution into struct device. Interesting comment. > > > commit 7e4bd0a65c4b2f71157f42ce89ecd7df69480a4b > Author: Mauro Carvalho Chehab <mchehab@redhat.com> > Date: Mon Aug 8 15:26:50 2011 -0300 > > Add a hardware resource locking schema at the Kernel > > Sometimes, a hardware resource is used by more than one device driver. This > causes troubles, as sometimes, using the resource by one driver is mutually > exclusive than using the same resource by another driver. > > Adds a resource locking schema that will avoid such troubles. > > TODO: This is just a quick hack prototyping the a real solution. The > namespace there is not consistent, nor the actual code that locks the > resource is provided. > > NOTE: As the problem also happens with some PCI devices, instead of adding > such locking schema at usb_device, it seems better to bind whatever > solution into struct device. > > Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com> > > diff --git a/include/linux/resourcelock.h b/include/linux/resourcelock.h > new file mode 100644 > index 0000000..fc7238c > --- /dev/null > +++ b/include/linux/resourcelock.h > @@ -0,0 +1,27 @@ > +#include <linux/device.h> > + > +/** > + * enum hw_resources - type of resource to lock > + * LOCK_DEVICE - The complete device should be locked with exclusive access > + * > + * TODO: Add other types of resource locking, for example, to lock just a > + * tuner, or an I2C bus > + */ > + > +enum hw_resources { > + LOCK_DEVICE, > +}; > + > +static int get_resource_lock(struct device dev, enum hw_resources hw_rec) { > + /* > + * TODO: implement the actual code for the function, returning > + * -EBUSY if somebody else already allocated the needed resource > + */ > + return 0; > +} > + > +static void put_resource_lock(struct device dev, enum hw_resources hw_rec) { > + /* > + * TODO: implrement a function to release the resource > + */ > +} > diff --git a/drivers/media/video/gspca/gspca.c b/drivers/media/video/gspca/gspca.c > index 5da4879..d8da757 100644 > --- a/drivers/media/video/gspca/gspca.c > +++ b/drivers/media/video/gspca/gspca.c > @@ -35,6 +35,7 @@ > #include <asm/page.h> > #include <linux/uaccess.h> > #include <linux/ktime.h> > +#include <linux/resourcelock.h> > #include <media/v4l2-ioctl.h> > > #include "gspca.h" > @@ -1218,6 +1219,7 @@ static void gspca_release(struct video_device *vfd) > static int dev_open(struct file *file) > { > struct gspca_dev *gspca_dev; > + int ret; > > PDEBUG(D_STREAM, "[%s] open", current->comm); > gspca_dev = (struct gspca_dev *) video_devdata(file); > @@ -1228,6 +1230,10 @@ static int dev_open(struct file *file) > if (!try_module_get(gspca_dev->module)) > return -ENODEV; > > + ret = get_resource_lock(gspca_dev->dev->dev, LOCK_DEVICE); > + if (ret) > + return ret; > + > file->private_data = gspca_dev; > #ifdef GSPCA_DEBUG > /* activate the v4l2 debug */ > @@ -1260,6 +1266,9 @@ static int dev_close(struct file *file) > frame_free(gspca_dev); > } > file->private_data = NULL; > + > + put_resource_lock(gspca_dev->dev->dev, LOCK_DEVICE); > + > module_put(gspca_dev->module); > mutex_unlock(&gspca_dev->queue_lock); > > diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c > index 37518df..f94a6d5 100644 > --- a/drivers/usb/core/devio.c > +++ b/drivers/usb/core/devio.c > @@ -49,6 +49,7 @@ > #include <asm/uaccess.h> > #include <asm/byteorder.h> > #include <linux/moduleparam.h> > +#include <linux/resourcelock.h> > > #include "usb.h" > > @@ -693,6 +694,10 @@ static int usbdev_open(struct inode *inode, struct file *file) > if (dev->state == USB_STATE_NOTATTACHED) > goto out_unlock_device; > > + ret = get_resource_lock(dev->dev, LOCK_DEVICE); > + if (ret) > + goto out_unlock_device; > + > ret = usb_autoresume_device(dev); > if (ret) > goto out_unlock_device; > @@ -747,6 +752,7 @@ static int usbdev_release(struct inode *inode, struct file *file) > destroy_all_async(ps); > usb_autosuspend_device(dev); > usb_unlock_device(dev); > + put_resource_lock(dev->dev, LOCK_DEVICE); > usb_put_dev(dev); > put_pid(ps->disc_pid); > > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Em 08-08-2011 16:32, Theodore Kilgore escreveu: >> Doing an specific libusb-like approach just for those cams seems to be the >> wrong direction, as such driver would be just a fork of an already existing >> code. I'm all against duplicating it. > > Well, in practice the "fork" would presumably be carried out by yours > truly. Presumably with the advice and help of concerned parties. too. > Since I am involved on both the kernel side and the libgphoto2 side of the > support for the same cameras, it would certainly shorten the lines of > communication at the very least. Therefore this is not infeasible. Forking the code just because we have something "special" is the wrong thing to do (TM). I would not like to fork V4L core code due to some special need, but instead to add some glue there to cover the extra case. Maintaining a fork is bad in long term, as the same fixes/changes will likely be needed on both copies. Adding some sort of resource locking like the example I've pointed seems easy and will work just fine. >> So, either we need to move the code from libgphoto2 to kernel > > As I said, I think you don't want to do that. I don't have a strong opinion about that ATM. Both approaches have advantages and disadvantages. > or work into >> an approach that will make libusb > > (or an appropriate substitute) Something like what Hans proposed makes sense on my eyes, as an appropriate substitute, but it seems that this is exactly what you don't want. I can really see two alternatives there: 1) keep the libusb API, e. g. the driver for data access is on userspace, and a char device allows to communicate with USB in a transparent way; 2) create an API for camera, like Hans/Adam proposal. If we take (1), we should just use the already existing Kernel infrastructure, plus a resource locking, to put the USB device into "exclusive access" mode. > to return -EBUSY when a driver like V4L >> is in usage, and vice-versa. >> >> I never took a look on how libusb works. It seems that the logic for it is >> at drivers/usb/core/devio.c. Assuming that this is correct driver for libusb, >> the locking patch would be similar to the enclosed one. >> >> Of course, more work will be needed. For example, in the specific case of >> devices where starting stream will clean the memory data, the V4L driver >> will need some additional logic to detect if the memory is filled and not >> allowing stream (or requiring CAP_SYS_ADMIN, returning -EPERM otherwise). > > Yes, this is probably a good idea in any event. Agreed. > As far as I know, this > would affect just one kernel driver. A complication is that it > is only some of the cameras supported by that driver, and they need to be > detected. Yes. >> NOTE: As the problem also happens with some PCI devices, instead of adding >> such locking schema at usb_device, it seems better to bind whatever >> solution into struct device. > > Interesting comment. The problem with PCI devices is not exactly the same, but I tried to think on a way that could also work for those issues. Eventually, when actually implementing the code, we may come to a conclusion that this is the right thing to do, or to decide to address those cases with a different solution. The issue we have (and that it is bus-agnostic), is that some resources depend on or are mutually exclusive of another resource. For example, considering a single-tuner device that supports both analog and digital TV. Due to the analog TV, such device needs to have an ALSA module. However, accessing the ALSA input depends on having the hardware in analog mode, as, on almost all supported hardware, there's no MPEG decoder inside it. So, accessing the alsa device should return -EBUSY, if the device is on digital mode. On the other hand, as the device has just one tuner, the digital mode driver can't be used simultaneously with the analog mode one. So, what I'm seeing is that we need some kernel way to describe hardware resource dependencies. Regards, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 08 August 2011, Mauro Carvalho Chehab wrote: > > > > > > Well, in practice the "fork" would presumably be carried out by yours > > truly. Presumably with the advice and help of concerned parties. too. > > Since I am involved on both the kernel side and the libgphoto2 side of > > the support for the same cameras, it would certainly shorten the lines > > of communication at the very least. Therefore this is not infeasible. > > Forking the code just because we have something "special" is the wrong > thing to do (TM). I would not like to fork V4L core code due to some > special need, but instead to add some glue there to cover the extra case. > Maintaining a fork is bad in long term, as the same fixes/changes will > likely be needed on both copies. Unfortunately there is some difficulty with libusb in that respect. libgphoto relies upon libusb-0.1 becuase it is cross platform and Win32 support in libusb-1.0 is only just being integrated. The libusb developers consider the libusb-0.1 API frozen and are not willing to extend it to address our problem. libusb doesn't expose the file descriptor it uses to talk to the underlying device so it is hard to extend the interface without forking libusb (The best hope I can think of at the moment is to get the distros to accept a patch for it to add the extra required API call(s) and for libgphoto to use the extra features in that patch if it detects it is supported at compile time). Adam Baker -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 8 Aug 2011, Adam Baker wrote: > On Monday 08 August 2011, Mauro Carvalho Chehab wrote: > > > > > > > > > Well, in practice the "fork" would presumably be carried out by yours > > > truly. Presumably with the advice and help of concerned parties. too. > > > Since I am involved on both the kernel side and the libgphoto2 side of > > > the support for the same cameras, it would certainly shorten the lines > > > of communication at the very least. Therefore this is not infeasible. > > > > Forking the code just because we have something "special" is the wrong > > thing to do (TM). I would not like to fork V4L core code due to some > > special need, but instead to add some glue there to cover the extra case. > > Maintaining a fork is bad in long term, as the same fixes/changes will > > likely be needed on both copies. > > Unfortunately there is some difficulty with libusb in that respect. libgphoto > relies upon libusb-0.1 becuase it is cross platform and Win32 support in > libusb-1.0 is only just being integrated. The libusb developers consider the > libusb-0.1 API frozen and are not willing to extend it to address our problem. > libusb doesn't expose the file descriptor it uses to talk to the underlying > device so it is hard to extend the interface without forking libusb (The best > hope I can think of at the moment is to get the distros to accept a patch for > it to add the extra required API call(s) and for libgphoto to use the extra > features in that patch if it detects it is supported at compile time). Adam, Yes, you are quite correct about this. I was just on the way out of the house and remembered that this problem exists, decided to re-connect and add this point to the witches' brew that we are working on. What struck me was not the Windows support, though, it was the Mac support. And a number of people run Gphoto stuff on Mac, too. That just reinforces your point, of course. Gphoto is explicitly cross-platform. It is developed on Linux but it is supposed to compile on anyone's C compiler and run on any hardware platform or operating system which has available the minimal support require to make it work. You are right. We, basically, can not screw with the internals of libgphoto2. At the outside, one can not go to the point where any changes would break the support for other platforms. Theodore Kilgore -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/resourcelock.h b/include/linux/resourcelock.h new file mode 100644 index 0000000..fc7238c --- /dev/null +++ b/include/linux/resourcelock.h @@ -0,0 +1,27 @@ +#include <linux/device.h> + +/** + * enum hw_resources - type of resource to lock + * LOCK_DEVICE - The complete device should be locked with exclusive access + * + * TODO: Add other types of resource locking, for example, to lock just a + * tuner, or an I2C bus + */ + +enum hw_resources { + LOCK_DEVICE, +}; + +static int get_resource_lock(struct device dev, enum hw_resources hw_rec) { + /* + * TODO: implement the actual code for the function, returning + * -EBUSY if somebody else already allocated the needed resource + */ + return 0; +} + +static void put_resource_lock(struct device dev, enum hw_resources hw_rec) { + /* + * TODO: implrement a function to release the resource + */ +} diff --git a/drivers/media/video/gspca/gspca.c b/drivers/media/video/gspca/gspca.c index 5da4879..d8da757 100644 --- a/drivers/media/video/gspca/gspca.c +++ b/drivers/media/video/gspca/gspca.c @@ -35,6 +35,7 @@ #include <asm/page.h> #include <linux/uaccess.h> #include <linux/ktime.h> +#include <linux/resourcelock.h> #include <media/v4l2-ioctl.h> #include "gspca.h" @@ -1218,6 +1219,7 @@ static void gspca_release(struct video_device *vfd) static int dev_open(struct file *file) { struct gspca_dev *gspca_dev; + int ret; PDEBUG(D_STREAM, "[%s] open", current->comm); gspca_dev = (struct gspca_dev *) video_devdata(file); @@ -1228,6 +1230,10 @@ static int dev_open(struct file *file) if (!try_module_get(gspca_dev->module)) return -ENODEV; + ret = get_resource_lock(gspca_dev->dev->dev, LOCK_DEVICE); + if (ret) + return ret; + file->private_data = gspca_dev; #ifdef GSPCA_DEBUG /* activate the v4l2 debug */ @@ -1260,6 +1266,9 @@ static int dev_close(struct file *file) frame_free(gspca_dev); } file->private_data = NULL; + + put_resource_lock(gspca_dev->dev->dev, LOCK_DEVICE); + module_put(gspca_dev->module); mutex_unlock(&gspca_dev->queue_lock); diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c index 37518df..f94a6d5 100644 --- a/drivers/usb/core/devio.c +++ b/drivers/usb/core/devio.c @@ -49,6 +49,7 @@ #include <asm/uaccess.h> #include <asm/byteorder.h> #include <linux/moduleparam.h> +#include <linux/resourcelock.h> #include "usb.h" @@ -693,6 +694,10 @@ static int usbdev_open(struct inode *inode, struct file *file) if (dev->state == USB_STATE_NOTATTACHED) goto out_unlock_device; + ret = get_resource_lock(dev->dev, LOCK_DEVICE); + if (ret) + goto out_unlock_device; + ret = usb_autoresume_device(dev); if (ret) goto out_unlock_device; @@ -747,6 +752,7 @@ static int usbdev_release(struct inode *inode, struct file *file) destroy_all_async(ps); usb_autosuspend_device(dev); usb_unlock_device(dev); + put_resource_lock(dev->dev, LOCK_DEVICE); usb_put_dev(dev); put_pid(ps->disc_pid);