Em Seg, 2006-09-18 às 14:50 +0200, Laurent Pinchart escreveu: > Hi Mauro, > > > > > I am trying out linux-uvc with version 2.6.18-rc6-git3 and it reports > > > > unknown symbols for video_get_drvdata and video_set_drvdata. Based on > > > > another patch for this problem [1] I have fixed this for uvc. The fix > > > > works for me but please bear in mind that I have little kernel > > > > programming experience and thus it could be wrong. > > > > > > Thanks for the patch. It looks overly complicated to me, and it seems > > > that video_get_drvdata/video_set_drvdata should not have been removed, > > > but modified to use dev_get_drvdata/dev_set_drvdata instead. > > > > > > Mauro, could you have a look at that ? > > > > Sure! Can you email it to me? I'm not subscribed on linux-uvc-devel. > > You can find it at > > https://lists.berlios.de/pipermail/linux-uvc-devel/attachments/20060915/4e138fd2/attachment-0001.bin The patch looks sane to me. > > > > Why have those been removed ? I know > > > they were marked with OBSOLETE_OWNER, but that's becaused they accessed > > > the video_device structure's priv field, marked for removal. > > > > > > /* dev->driver_data will be used instead some day. > > > * Use the video_{get|set}_drvdata() helper functions, > > > * so the switch over will be transparent for you. > > > * Or use {pci|usb}_{get|set}_drvdata() directly. */ > > > void *priv; > > > > > > Shouldn't video_{get|set}_drvdata() have been kept ? > > > > This stuff just add more complexity and makes the code more opaque, and > > adds this additional pointer... > > I agree that video_device->priv should go. But it should be replaced by > something else, namely dev->driver_data. > > > For most cases, it will just turn on something like this (from > > cx88-video.c): > > > > static ssize_t > > video_read(struct file *file, char __user *data, size_t count, loff_t > > *ppos) > > { > > struct cx8800_fh *fh = file->private_data; > > This is an entirely different thing. file->private_data contains a *per-open* > pointer to private data. video_{get|set}_drvdata was used to retrieve private > data global to the device. > > Have a look at the changes to em2820_v4l2_open in > > http://www.linuxtv.org/downloads/video4linux/patches/2.6.14/v4l_813_replaced_obsolete_video_get_drvdata_and_video_set_drvdata.patch > > Do you really want to require *every* video driver to maintain a global list > of devices ? Think about overhead, locking issues, ... > > Drivers should use file->private_data to point to *per-open* private data, > not > per-device data. per-device data should be stored in device->driver_data > instead of video_device->priv (device->driver_data was made for exactly that > purpose), and the helper functions video_{get|set}_drvdata should be kept, > calling dev_{get|set}_drvdata instead of accessing video_device->priv. > > Things are quite simple. Video devices are class devices, so per-device > driver-private data is stored in device->driver_data, and per-open > driver-private data is stored in file->file_private. That's how the driver > model has been designed since the beginning of Linux 2.6. I can't imagine > requiring *every* driver to maintain a global list of devices just to store > private data. The obsolete get/set_drvdata were from the V4L1 time, where you cannot open a device more than once. V4L2 API does require multiple opens, in order that you can have one open to retrieve the streams, and another to control the device.
There is even more complex cases (probably not very useful for USB devices, due to USB traffic, but already supported by the core PCI drivers), where you are retrieving an stream to show at the screen, and another to store at disks (maybe with even different parameters). With such complex cases, you can't avoid having locks and per-open driver data. We might eventually replace this to a different model, but it would require redesigning videodev.c and all other drivers. I don't intend to do such core changes until we have all V4L1 drivers migrated to V4L2. Then, we may re-discuss this and propose a better model. Cheers, Mauro. _______________________________________________ Linux-uvc-devel mailing list [email protected] https://lists.berlios.de/mailman/listinfo/linux-uvc-devel
