> > > > > 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/4
> >e138fd2/attachment-0001.bin
>
> The patch looks sane to me.
Sane ? You must be kidding. Look at the overhead added to the open function.
Do you expect all drivers to implement that ? Compare it to this, which
doesn't require *any* change to the video drivers:
static inline void *video_get_drvdata(struct video_device *vdev)
{
return dev_get_drvdata(vdev->dev);
}
static inline void video_set_drvdata(struct video_device *vdev, void *data)
{
dev_set_drvdata(vdev->dev, data);
}
Doesn't that look saner ? Now, have a look at this:
static inline void *pci_get_drvdata (struct pci_dev *pdev)
{
return dev_get_drvdata(&pdev->dev);
}
static inline void pci_set_drvdata (struct pci_dev *pdev, void *data)
{
dev_set_drvdata(&pdev->dev, data);
}
Can't you see a similarity ?
[...]
> > > 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_repla
> >ced_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.
That's wrong. The video_device->priv field was from Linux 2.4 (and probably
earlier), where there were no proper kernel objects implementation and where
the device model we know today didn't exist. In 2.6 the device structures
includes a driver_data field which is intended to replace all priv field in
device structures (video_device and others). That's why the priv field in
video_device was removed. Instead of dirty hacks forcing all drivers to keep
a global list of private data for all the devices they handle, we must use
dev_{get|set}_drvdata. This function should be encapsulated in video_{get|
set}_drvdata to make transition easier.
> 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.
That doesn't matter. per-open data will go in file->private_data, per-device
data in device->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.
I've never advocated nor mentioned any complete redesign of the video API. You
doesn't seem to understand me.
Best regards,
Laurent Pinchart
_______________________________________________
Linux-uvc-devel mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/linux-uvc-devel
