Re: [RFC] Serialization flag example
Hans Verkuil wrote: >> Hans Verkuil wrote: >> - performance is important only for the ioctl's that directly handles >> the streaming (dbuf/dqbuf & friends); > > What performance? These calls just block waiting for a frame. How the hell > am I suppose to test performance anyway on something like that? They are called on the main loop for receiving buffers. As the userspace may be doing some video processing, by introducing latency here, you may affect the applications. By using perf subsystem, you should be able to test how much time is spent by an ioctl. > >> - videobuf has its own lock implementation; >> >> - a trivial mutex-based approach won't protect the stream to have >> some parameters modified by a VIDIOC_S_* ioctl (such protection should be >> provided by a resource locking); > > Generally once streaming starts drivers should return -EBUSY for such > calls. Nothing to do with locking in general. Yes, that's exactly what I said. This is a resource locking: you cannot change several stream properties while streaming (yet, you can change a few ones, like bright, contrast, etc). >> then, maybe the better would be to not call the hooks for those ioctls. >> It may be useful to do some perf tests and measure the real penalty before >> adding >> any extra code to exclude the buffer ioctls from the hook logic. > > Yuck. We want something easy to understand. Like: 'the hook is called for > every ioctl'. Not: 'the hook is called for every ioctl except this one and > this one and this one'. Then the driver might have to do different things > for different ioctls just because some behind-the-scene logic dictated > that the hook isn't called in some situations. > > I have said it before and I'll say it again: the problem with V4L2 drivers > has never been performance since all the heavy lifting is done with DMA, > the problem is complexity. Remember: premature optimization is the root of > all evil. If a driver really needs the last drop of performance (for what, > I wonder?) then it can choose to just not implement those hooks and do > everything itself. I tend to agree with you. All I'm saying is that it is valuable to double check the impacts before committing it. -- Cheers, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] Serialization flag example
> Hans Verkuil wrote: >> On Tuesday 06 April 2010 00:58:54 Hans Verkuil wrote: >>> On Tuesday 06 April 2010 00:46:11 Laurent Pinchart wrote: On Sunday 04 April 2010 05:14:17 David Ellingsworth wrote: > After looking at the proposed solution, I personally find the > suggestion for a serialization flag to be quite ridiculous. As others > have mentioned, the mere presence of the flag means that driver > writers will gloss over any concurrency issues that might exist in > their driver on the mere assumption that specifying the serialization > flag will handle it all for them. I happen to agree with this. Proper locking is difficult, but that's not a reason to introduce such a workaround. I'd much rather see proper documentation for driver developers on how to implement locking properly. >>> I've taken a different approach in another tree: >>> >>> http://linuxtv.org/hg/~hverkuil/v4l-dvb-ser2/ >>> >>> It adds two callbacks to ioctl_ops: pre_hook and post_hook. You can use >>> these >>> to do things like prio checking and taking your own mutex to serialize >>> the >>> ioctl call. >>> >>> This might be a good compromise between convenience and not hiding >>> anything. >> >> I realized that something like this is needed anyway if we go ahead with >> the >> new control framework. That exposes controls in sysfs, but if you set a >> control >> from sysfs, then that bypasses the ioctl completely. So you need a way >> to hook >> into whatever serialization scheme you have (if any). >> >> It is trivial to get to the video_device struct in the control handler >> and >> from there you can access ioctl_ops. So calling the pre/post hooks for >> the >> sysfs actions is very simple. >> >> The prototype for the hooks needs to change, though, since accesses from >> sysfs do not provide you with a struct file pointer. >> >> Something like this should work: >> >> int pre_hook(struct video_device *vdev, enum v4l2_priority prio, int >> cmd); >> void post_hook(struct video_device *vdev, int cmd); >> >> The prio is there to make priority checking possible. It will be >> initially >> unused, but as soon as the events API with the new v4l2_fh struct is >> merged >> we can add centralized support for this. > > I like this strategy. > > My only concern is about performance. Especially in the cases where we > need to > handle the command at the hooks, those methods will introduce two extra > jumps > to the driver and two extra switches. As the jump will go to a code > outside > V4L core, I suspect that they'll likely flush the L1 cache. > > If we consider that: > > - performance is important only for the ioctl's that directly handles > the streaming (dbuf/dqbuf & friends); What performance? These calls just block waiting for a frame. How the hell am I suppose to test performance anyway on something like that? > > - videobuf has its own lock implementation; > > - a trivial mutex-based approach won't protect the stream to have > some parameters modified by a VIDIOC_S_* ioctl (such protection should be > provided by a resource locking); Generally once streaming starts drivers should return -EBUSY for such calls. Nothing to do with locking in general. > then, maybe the better would be to not call the hooks for those ioctls. > It may be useful to do some perf tests and measure the real penalty before > adding > any extra code to exclude the buffer ioctls from the hook logic. Yuck. We want something easy to understand. Like: 'the hook is called for every ioctl'. Not: 'the hook is called for every ioctl except this one and this one and this one'. Then the driver might have to do different things for different ioctls just because some behind-the-scene logic dictated that the hook isn't called in some situations. I have said it before and I'll say it again: the problem with V4L2 drivers has never been performance since all the heavy lifting is done with DMA, the problem is complexity. Remember: premature optimization is the root of all evil. If a driver really needs the last drop of performance (for what, I wonder?) then it can choose to just not implement those hooks and do everything itself. Regards, Hans > > -- > > Cheers, > Mauro > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] Serialization flag example
Hans Verkuil wrote: > On Tuesday 06 April 2010 00:58:54 Hans Verkuil wrote: >> On Tuesday 06 April 2010 00:46:11 Laurent Pinchart wrote: >>> On Sunday 04 April 2010 05:14:17 David Ellingsworth wrote: After looking at the proposed solution, I personally find the suggestion for a serialization flag to be quite ridiculous. As others have mentioned, the mere presence of the flag means that driver writers will gloss over any concurrency issues that might exist in their driver on the mere assumption that specifying the serialization flag will handle it all for them. >>> I happen to agree with this. Proper locking is difficult, but that's not a >>> reason to introduce such a workaround. I'd much rather see proper >>> documentation for driver developers on how to implement locking properly. >> I've taken a different approach in another tree: >> >> http://linuxtv.org/hg/~hverkuil/v4l-dvb-ser2/ >> >> It adds two callbacks to ioctl_ops: pre_hook and post_hook. You can use these >> to do things like prio checking and taking your own mutex to serialize the >> ioctl call. >> >> This might be a good compromise between convenience and not hiding anything. > > I realized that something like this is needed anyway if we go ahead with the > new control framework. That exposes controls in sysfs, but if you set a > control > from sysfs, then that bypasses the ioctl completely. So you need a way to hook > into whatever serialization scheme you have (if any). > > It is trivial to get to the video_device struct in the control handler and > from there you can access ioctl_ops. So calling the pre/post hooks for the > sysfs actions is very simple. > > The prototype for the hooks needs to change, though, since accesses from > sysfs do not provide you with a struct file pointer. > > Something like this should work: > > int pre_hook(struct video_device *vdev, enum v4l2_priority prio, int cmd); > void post_hook(struct video_device *vdev, int cmd); > > The prio is there to make priority checking possible. It will be initially > unused, but as soon as the events API with the new v4l2_fh struct is merged > we can add centralized support for this. I like this strategy. My only concern is about performance. Especially in the cases where we need to handle the command at the hooks, those methods will introduce two extra jumps to the driver and two extra switches. As the jump will go to a code outside V4L core, I suspect that they'll likely flush the L1 cache. If we consider that: - performance is important only for the ioctl's that directly handles the streaming (dbuf/dqbuf & friends); - videobuf has its own lock implementation; - a trivial mutex-based approach won't protect the stream to have some parameters modified by a VIDIOC_S_* ioctl (such protection should be provided by a resource locking); then, maybe the better would be to not call the hooks for those ioctls. It may be useful to do some perf tests and measure the real penalty before adding any extra code to exclude the buffer ioctls from the hook logic. -- Cheers, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] Serialization flag example
On Tuesday 06 April 2010 00:58:54 Hans Verkuil wrote: > On Tuesday 06 April 2010 00:46:11 Laurent Pinchart wrote: > > On Sunday 04 April 2010 05:14:17 David Ellingsworth wrote: > > > After looking at the proposed solution, I personally find the > > > suggestion for a serialization flag to be quite ridiculous. As others > > > have mentioned, the mere presence of the flag means that driver > > > writers will gloss over any concurrency issues that might exist in > > > their driver on the mere assumption that specifying the serialization > > > flag will handle it all for them. > > > > I happen to agree with this. Proper locking is difficult, but that's not a > > reason to introduce such a workaround. I'd much rather see proper > > documentation for driver developers on how to implement locking properly. > > I've taken a different approach in another tree: > > http://linuxtv.org/hg/~hverkuil/v4l-dvb-ser2/ > > It adds two callbacks to ioctl_ops: pre_hook and post_hook. You can use these > to do things like prio checking and taking your own mutex to serialize the > ioctl call. > > This might be a good compromise between convenience and not hiding anything. I realized that something like this is needed anyway if we go ahead with the new control framework. That exposes controls in sysfs, but if you set a control from sysfs, then that bypasses the ioctl completely. So you need a way to hook into whatever serialization scheme you have (if any). It is trivial to get to the video_device struct in the control handler and from there you can access ioctl_ops. So calling the pre/post hooks for the sysfs actions is very simple. The prototype for the hooks needs to change, though, since accesses from sysfs do not provide you with a struct file pointer. Something like this should work: int pre_hook(struct video_device *vdev, enum v4l2_priority prio, int cmd); void post_hook(struct video_device *vdev, int cmd); The prio is there to make priority checking possible. It will be initially unused, but as soon as the events API with the new v4l2_fh struct is merged we can add centralized support for this. Regards, Hans > > Regards, > > Hans > > > -- Hans Verkuil - video4linux developer - sponsored by TANDBERG -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] Serialization flag example
On Tuesday 06 April 2010 00:46:11 Laurent Pinchart wrote: > On Sunday 04 April 2010 05:14:17 David Ellingsworth wrote: > > After looking at the proposed solution, I personally find the > > suggestion for a serialization flag to be quite ridiculous. As others > > have mentioned, the mere presence of the flag means that driver > > writers will gloss over any concurrency issues that might exist in > > their driver on the mere assumption that specifying the serialization > > flag will handle it all for them. > > I happen to agree with this. Proper locking is difficult, but that's not a > reason to introduce such a workaround. I'd much rather see proper > documentation for driver developers on how to implement locking properly. I've taken a different approach in another tree: http://linuxtv.org/hg/~hverkuil/v4l-dvb-ser2/ It adds two callbacks to ioctl_ops: pre_hook and post_hook. You can use these to do things like prio checking and taking your own mutex to serialize the ioctl call. This might be a good compromise between convenience and not hiding anything. Regards, Hans -- Hans Verkuil - video4linux developer - sponsored by TANDBERG -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] Serialization flag example
On Sunday 04 April 2010 05:14:17 David Ellingsworth wrote: > After looking at the proposed solution, I personally find the > suggestion for a serialization flag to be quite ridiculous. As others > have mentioned, the mere presence of the flag means that driver > writers will gloss over any concurrency issues that might exist in > their driver on the mere assumption that specifying the serialization > flag will handle it all for them. I happen to agree with this. Proper locking is difficult, but that's not a reason to introduce such a workaround. I'd much rather see proper documentation for driver developers on how to implement locking properly. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re: [RFC] Serialization flag example
After looking at the proposed solution, I personally find the suggestion for a serialization flag to be quite ridiculous. As others have mentioned, the mere presence of the flag means that driver writers will gloss over any concurrency issues that might exist in their driver on the mere assumption that specifying the serialization flag will handle it all for them. As the author of the most recent patches to radio-mr800, the proposed changes not only add additional complexity to the driver but also remove some of the fundamental error checking within the driver. Despite the fact the error check might not always be successful, it will still catch the majority of cases. I therefore NACK the proposed patches to this driver. The right thing to do is to actually correct the issue within all the drivers that need it. Is this a lot of work? Maybe, but it's a far better solution as each driver will be responsible for concurrency issues that it may or may not have. After all, wasn't this what the removal of the BKL was about in the first place? Regards, David Ellingsworth -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Aw: Re: [RFC] Serialization flag example
- Original Nachricht Von: Andy Walls An: Mauro Carvalho Chehab Datum: 03.04.2010 02:47 Betreff: Re: [RFC] Serialization flag example > On Fri, 2010-04-02 at 18:15 -0300, Mauro Carvalho Chehab wrote: > > Devin Heitmueller wrote: > > > In the case of a V4L x DVB type of lock, this is not to protect some > memory, but, > > instead, to limit the usage of a hardware that is not capable of > simultaneously > > provide V4L and DVB streams. This is a common case on almost all devices, > but, as > > Hermann pointed, there are a few devices that are capable of doing both > analog > > and digital streams at the same time, but saa7134 driver currently doesn't > support. Mauro, to do digital and analog at once is not restricted to a few devices. The only restriction, except those hybrid tuners do have, is that in dual mode only packed video formats are allowed for analog, since planar formats are using DMA channel five, which is already in use by the TS interface then. > I know a driver that does: Me too ;) and Trent tested on cx88xx, IIRC. > cx18 can handle simultaneous streaming of DTV and analog. Yup. Not to talk about recent PCIe devices. During I'm writing this I watch DVB-S, DVB-T and Composite at once on a single saa7231e on vista. Supports also S2 and HDTV, vbi and radio and can have a dual remote interface. http://www.creatix.de/produkte/multimedia/ctx1924.htm > Some cards have both an analog and digital tuner, so both DTV and analog > can come from an RF source simultaneously. (No locking needed really.) We have quite some such cards with two tuners on the saa7134 since 2005. Also such with three and even four tuners, two of them hybrid. Even the simplest variant, say with a single DVB-S only tuner, can still have external analog baseband at once from TV, STB, VCR or whatever. > Some cards only have one tuner, which means simultaneous streaming is > limited to DTV from RF and analog from baseband inputs. Streaming > analog from an RF source on these cards precludes streaming of DTV. (An > odd locking ruleset when you consider MPEG, YUV, PCM, and VBI from the > bridge chip can independently be streamed from the selected analog > source to 4 different device nodes.) On that mentioned Medion Quad md8080/CTX944 it becomes even more interesting. Each of the two PCI bridges can only handle one digital and one analog stream at once. That one must know. And if RF loopthrough is enabled or not is another important point for its usage configuration. For the two hybrid tuners it is a manual switch the driver doesn't know about. For the two DVB-S tuners it could be switchable in software and one of the LNB connectors could even be used as RF out to another device. (Has usage restrictions) The user can make a lot of decisions how to use such a card and for sure doesn't want to have any limitations only because of the hybrid tuners. Cheers, Hermann > Regards, > Andy > Frohe Ostern! Alles für's Fest der Hasen und Lämmer jetzt im Osterspecial auf Arcor.de: http://www.arcor.de/rd/footer.ostern -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] Serialization flag example
On Fri, 2010-04-02 at 18:15 -0300, Mauro Carvalho Chehab wrote: > Devin Heitmueller wrote: > In the case of a V4L x DVB type of lock, this is not to protect some memory, > but, > instead, to limit the usage of a hardware that is not capable of > simultaneously > provide V4L and DVB streams. This is a common case on almost all devices, > but, as > Hermann pointed, there are a few devices that are capable of doing both analog > and digital streams at the same time, but saa7134 driver currently doesn't > support. I know a driver that does: cx18 can handle simultaneous streaming of DTV and analog. Some cards have both an analog and digital tuner, so both DTV and analog can come from an RF source simultaneously. (No locking needed really.) Some cards only have one tuner, which means simultaneous streaming is limited to DTV from RF and analog from baseband inputs. Streaming analog from an RF source on these cards precludes streaming of DTV. (An odd locking ruleset when you consider MPEG, YUV, PCM, and VBI from the bridge chip can independently be streamed from the selected analog source to 4 different device nodes.) Regards, Andy -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] Serialization flag example
On Fri, 2010-04-02 at 10:57 +0200, Hans Verkuil wrote: > If needed one could allow drivers to override this function. But again, start > simple and only make it more complex if we really need to. Overengineering is > one of the worst mistakes one can make. I have seen too many projects fail > because of that. Gall's Law! http://en.wikipedia.org/wiki/Gall%27s_law Regards, Andy -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] Serialization flag example
Devin Heitmueller wrote: > On Fri, Apr 2, 2010 at 1:43 PM, Manu Abraham wrote: >> IMO, A framework shouldn't lock. Current DVB framework is locked with BKL. I agree that an unconditional lock like this is very bad. We need to get rid of it as soon as possible. > Hello Manu, > > The argument I am trying to make is that there are numerous cases > where you should not be able to use both a particular DVB and V4L > device at the same time. The implementation of such locking should be > handled by the v4l-dvb core, but the definition of the relationships > dictating which devices cannot be used in parallel is still the > responsibility of the driver. > > This way, a bridge driver can say "this DVB device cannot be used at > the same time as this V4L device" but the actual enforcement of the > locking is done in the core. For cases where the devices can be used > in parallel, the bridge driver doesn't have to do anything. Although both are some sort of locks, the BKL replacement lock is basically a memory barrier to serialize data, avoiding that the driver's internal structs to be corrupted or to return a wrong value. Only the driver really knows what should be protected. In the case of V4L/DVB devices, the struct to be protected is the struct *_dev (struct core, on cx88, struct em28xx on em28xx, struct saa7134_dev, and so on). Of course, if everything got serialized by the core, and assuming that the driver doesn't have IRQ's and/or other threads accessing the same data, the problem disappears, at the expense of a performance reduction that may or may not be relevant. That's said, except for the most simple drivers, like radio ones, there's always some sort of IRQ and/or threads evolved, touching on struct *_dev. For example, both cx88 and saa7134 have (depending on the hardware): - IRQ to announce that a data buffer was filled for video, vbi, alsa; - IRQ for IR; - IR polling; - video audio standard detection thread; A lock to protect the internal data structs should protect all the above. Even the most pedantic core-only lock won't solve it. Yet, a lock, on core, for ioctl/poll/open/close may be part of a protection, if the same lock is used also to protect the other usages of struct *_dev. So, I'm OK on having an optional mutex parameter to be passed to V4L core, to provide the BKL removal. In the case of a V4L x DVB type of lock, this is not to protect some memory, but, instead, to limit the usage of a hardware that is not capable of simultaneously provide V4L and DVB streams. This is a common case on almost all devices, but, as Hermann pointed, there are a few devices that are capable of doing both analog and digital streams at the same time, but saa7134 driver currently doesn't support. Some drivers, like cx88 has a sort of lock meant to protect such things. It is implemented on res_get/res_check/res_locked/res_free. Currently, the lock is only protecting simultaneous usage of the analog part of the driver. It works by not allowing to start two simultaneous video streams of the same memory access type, for example, while allowing multiple device opens, for example to call V4L controls, while another application is streaming. It also allows having a mmapped or overlay stream and a separate read() stream (used on applications like xawtv and xdtv to record a video on PCI devices that has enough bandwidth to provide two simultaneous streams). Maybe this kind of lock can be abstracted, and we can add a class of resource protectors inside the core, for the drivers to use it when needed. -- Cheers, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] Serialization flag example
On Fri, Apr 2, 2010 at 2:24 PM, Manu Abraham wrote: > Hi Devin, > >> Hello Manu, >> >> The argument I am trying to make is that there are numerous cases >> where you should not be able to use both a particular DVB and V4L >> device at the same time. The implementation of such locking should be >> handled by the v4l-dvb core, but the definition of the relationships >> dictating which devices cannot be used in parallel is still the >> responsibility of the driver. >> >> This way, a bridge driver can say "this DVB device cannot be used at >> the same time as this V4L device" but the actual enforcement of the >> locking is done in the core. For cases where the devices can be used >> in parallel, the bridge driver doesn't have to do anything. > > I follow what you mean. Why I emphasized that it shouldn't be at the > core, but basically in the bridge driver: > > Case 1 > - there is a 1:n relation, In this case there is only 1 path and 3 > users sharing that path > In such a case, You can use such a mentioned scheme, where things do > look okay, since it is only about locking a single path. > > Case 2 > - there is a n:n relation, in this case there are n paths and n users > In such a case, it is hard to make the core aware of all the details, > since there could be more than 1 resource of the same category; > Mapping each of them properly won't be easy, as for the same chip > driver Resource A on Card A, would mean different for Resource A on > Card B. > > Case 3 > - there is a m:n relation, in this case, there are m paths and n users > This case is even more painful than the previous ones. > > In cases 2 & 3, the option to handle such cases is using a > configuration scheme based on the card type. I guess handling such > would be quite daunting and hard to get right. I guess it would be > much more efficient and useful to have such a feature to be made > available in the bridge driver as it is a resource of the card > configuration, rather than a common bridge resource. Hi Manu, I don't have any problem with a bridge choosing to implement some much more complicated scheme to meet its own special requirements. However, it feels like the vast majority of bridges would fall into scenario #1, and having that functionality in the core would mean that all of those bridges would work properly (only needing a 2 line code change). Hence, making the core handle the common case and still allowing the bridge maintainer to override the logic if necessary would seem like an ideal solution. Nothing I have suggested precludes the bridge maintainer from *not* adding the code making the association in the core and instead adding his/her own locking infrastructure to the bridge driver. Right now, I can think of five or six bridges all of which fall into category #1. Should we really add effectively the exact same locking code to all those bridges, running the risk of of screwing up the cut/paste? Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] Serialization flag example
Hi Devin, > Hello Manu, > > The argument I am trying to make is that there are numerous cases > where you should not be able to use both a particular DVB and V4L > device at the same time. The implementation of such locking should be > handled by the v4l-dvb core, but the definition of the relationships > dictating which devices cannot be used in parallel is still the > responsibility of the driver. > > This way, a bridge driver can say "this DVB device cannot be used at > the same time as this V4L device" but the actual enforcement of the > locking is done in the core. For cases where the devices can be used > in parallel, the bridge driver doesn't have to do anything. I follow what you mean. Why I emphasized that it shouldn't be at the core, but basically in the bridge driver: Case 1 - there is a 1:n relation, In this case there is only 1 path and 3 users sharing that path In such a case, You can use such a mentioned scheme, where things do look okay, since it is only about locking a single path. Case 2 - there is a n:n relation, in this case there are n paths and n users In such a case, it is hard to make the core aware of all the details, since there could be more than 1 resource of the same category; Mapping each of them properly won't be easy, as for the same chip driver Resource A on Card A, would mean different for Resource A on Card B. Case 3 - there is a m:n relation, in this case, there are m paths and n users This case is even more painful than the previous ones. In cases 2 & 3, the option to handle such cases is using a configuration scheme based on the card type. I guess handling such would be quite daunting and hard to get right. I guess it would be much more efficient and useful to have such a feature to be made available in the bridge driver as it is a resource of the card configuration, rather than a common bridge resource. Manu -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] Serialization flag example
On Fri, Apr 2, 2010 at 1:43 PM, Manu Abraham wrote: > On Thu, Apr 1, 2010 at 10:24 PM, Mauro Carvalho Chehab > wrote: > >> You'll have issues also with -alsa and -dvb parts that are present on the >> drivers. >> >> Also, drivers like cx88 have another PCI device for mpeg-encoded streams. It >> behaves >> like two separate drivers (each with its own bind code at V4L core), but, as >> there's >> just one PCI bridge, and just one analog video decoder/analog audio decoder, >> the lock >> should cross between the different drivers. >> >> So, binding videobuf to v4l2_device won't solve the issue with most >> videobuf-aware drivers, >> nor the lock will work as expected, at least with most of the devices. >> >> Also, although this is not a real problem, your lock is too pedantic: it is >> blocking access to several ioctl's that don't need to be locked. For >> example, there are >> several ioctl's that just returns static: information: capabilities, >> supported video standards, >> etc. There are even some cases where dynamic ioctl's are welcome, like >> accepting QBUF/DQBUF >> without locking (or with a minimum lock), to allow different threads to >> handle the buffers. >> >> The big issue I see with this approach is that developers will become lazy >> on checking >> the locks inside the drivers: they'll just apply the flag and trust that all >> of their >> locking troubles were magically solved by the core. >> >> Maybe a better alternative would be to pass to the V4L2 core, optionally, >> some lock, >> used internally on the driver. This approach will still be pedantic, as all >> ioctls will >> keep being serialized, but at least the driver will need to explicitly >> handle the lock, >> and the same lock can be used on other parts of the driver. > > > IMO, A framework shouldn't lock. Why ? > > Different devices require different locking schemes, each and every > device require it in different ways. Some drivers might not even > require it, since they may be able to handle different systems > asynchronously. > > Locking and such device management, will be known to the driver alone > and not to any framework. While, this may benefit some few devices the > other set of devices will eventually be broken. Hello Manu, The argument I am trying to make is that there are numerous cases where you should not be able to use both a particular DVB and V4L device at the same time. The implementation of such locking should be handled by the v4l-dvb core, but the definition of the relationships dictating which devices cannot be used in parallel is still the responsibility of the driver. This way, a bridge driver can say "this DVB device cannot be used at the same time as this V4L device" but the actual enforcement of the locking is done in the core. For cases where the devices can be used in parallel, the bridge driver doesn't have to do anything. Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] Serialization flag example
On Thu, Apr 1, 2010 at 10:24 PM, Mauro Carvalho Chehab wrote: > You'll have issues also with -alsa and -dvb parts that are present on the > drivers. > > Also, drivers like cx88 have another PCI device for mpeg-encoded streams. It > behaves > like two separate drivers (each with its own bind code at V4L core), but, as > there's > just one PCI bridge, and just one analog video decoder/analog audio decoder, > the lock > should cross between the different drivers. > > So, binding videobuf to v4l2_device won't solve the issue with most > videobuf-aware drivers, > nor the lock will work as expected, at least with most of the devices. > > Also, although this is not a real problem, your lock is too pedantic: it is > blocking access to several ioctl's that don't need to be locked. For example, > there are > several ioctl's that just returns static: information: capabilities, > supported video standards, > etc. There are even some cases where dynamic ioctl's are welcome, like > accepting QBUF/DQBUF > without locking (or with a minimum lock), to allow different threads to > handle the buffers. > > The big issue I see with this approach is that developers will become lazy on > checking > the locks inside the drivers: they'll just apply the flag and trust that all > of their > locking troubles were magically solved by the core. > > Maybe a better alternative would be to pass to the V4L2 core, optionally, > some lock, > used internally on the driver. This approach will still be pedantic, as all > ioctls will > keep being serialized, but at least the driver will need to explicitly handle > the lock, > and the same lock can be used on other parts of the driver. IMO, A framework shouldn't lock. Why ? Different devices require different locking schemes, each and every device require it in different ways. Some drivers might not even require it, since they may be able to handle different systems asynchronously. Locking and such device management, will be known to the driver alone and not to any framework. While, this may benefit some few devices the other set of devices will eventually be broken. Manu -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] Serialization flag example
Hans Verkuil wrote: > On Thursday 01 April 2010 23:32:33 Mauro Carvalho Chehab wrote: >> Hans Verkuil wrote: Maybe a better alternative would be to pass to the V4L2 core, optionally, some lock, used internally on the driver. This approach will still be pedantic, as all ioctls will keep being serialized, but at least the driver will need to explicitly handle the lock, and the same lock can be used on other parts of the driver. >>> Well, I guess you could add a 'struct mutex *serialize;' field to >>> v4l2_device >>> that drivers can set. But I don't see much of a difference in practice. >> It makes easier to implement more complex approaches, where you may need to >> use more >> locks. Also, It makes no sense to make a DVB code or an alsa code dependent >> on a V4L >> header, just because it needs to see a mutex. > > What's in a name. v4l2_device is meant to be a top-level object in a driver. > There is nothing particularly v4l about it and it would be trivial to rename > it to media_device. This has nothing to do with a name. The point is that such mutex need to be used by other systems (for example, some drivers have alsa provided by snd-usb-audio). >> Also, a mutex at the driver need to be initialized inside the driver. It is >> not just one >> flag that someone writing a new driver will clone without really >> understanding what >> it is doing. > > Having a driver do mutex_init() really does not improve understanding. But > good documentation will. Creating a simple, easy to understand and well > documented locking scheme will go a long way to making our drivers better. Documentation is for sure needed. Having the mutex inside of the driver helps people to understand, as I doubt that most of the developers take a deep look inside V4L and Linux core implementations before writing a driver. I'm all for porting things that are common to the core, but things that depends on the driver internal logic, like the mutexes that protect the driver data structs, should be more visible (or be fully implemented) outside the core. > Now, having said all this, I do think upon reflection that using a pointer > to a mutex might be better. The main reason being that while I do think that > renaming v4l2_device to media_device is a good idea and that more code sharing > between v4l and dvb would benefit both (heck, perhaps there should be more > integration between v4l-dvb and alsa as well), the political reality is > different. With respect to v4l2_device: The reason should be technical, not political. A proper module/subsystem decoupling is interesting, to easy maintainership. As I said, back on LPC/2007, the core functions provided by v4l2_device makes sense and should also be used on DVB. That's my 2 cents. The reality is that migrating existing drivers to it will be very time consuming, and maybe not valuable enough, at least for those dvb drivers that are almost unmaintained nowadays, but I think that using it for new drivers and for the drivers where we have a constant pattern of maintainability would be a good thing. I think we should evolute to have a more integrated core for both V4L and DVB, that will provide the proper locking between the two subsystems. >>> Basically the CAP (capability) ioctls and all ENUM ioctls do not have to be >>> serialized. I am not sure whether the streaming ioctls should also be >>> included >>> here. I can't really grasp the consequences of whatever choice we make. If >>> we >>> want to be compatible with what happens today with ioctl and the BKL, then >>> we >>> need to lock and have videobuf unlock whenever it has to wait for an event. >> I suspect that the list of "must have" is driver-dependent. > > If needed one could allow drivers to override this function. But again, start > simple and only make it more complex if we really need to. Overengineering is > one of the worst mistakes one can make. I have seen too many projects fail > because of that. Ok. -- Cheers, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] Serialization flag example
On Thursday 01 April 2010 23:32:33 Mauro Carvalho Chehab wrote:
> Hans Verkuil wrote:
> >> Maybe a better alternative would be to pass to the V4L2 core, optionally,
> >> some lock,
> >> used internally on the driver. This approach will still be pedantic, as
> >> all ioctls will
> >> keep being serialized, but at least the driver will need to explicitly
> >> handle the lock,
> >> and the same lock can be used on other parts of the driver.
> >
> > Well, I guess you could add a 'struct mutex *serialize;' field to
> > v4l2_device
> > that drivers can set. But I don't see much of a difference in practice.
>
> It makes easier to implement more complex approaches, where you may need to
> use more
> locks. Also, It makes no sense to make a DVB code or an alsa code dependent
> on a V4L
> header, just because it needs to see a mutex.
What's in a name. v4l2_device is meant to be a top-level object in a driver.
There is nothing particularly v4l about it and it would be trivial to rename
it to media_device.
> Also, a mutex at the driver need to be initialized inside the driver. It is
> not just one
> flag that someone writing a new driver will clone without really
> understanding what
> it is doing.
Having a driver do mutex_init() really does not improve understanding. But
good documentation will. Creating a simple, easy to understand and well
documented locking scheme will go a long way to making our drivers better.
Now, having said all this, I do think upon reflection that using a pointer
to a mutex might be better. The main reason being that while I do think that
renaming v4l2_device to media_device is a good idea and that more code sharing
between v4l and dvb would benefit both (heck, perhaps there should be more
integration between v4l-dvb and alsa as well), the political reality is
different.
>
> > Regarding the 'pedantic approach': we can easily move the locking to
> > video_ioctl2:
> >
> > struct video_device *vdev = video_devdata(file);
> > int serialize = must_serialize_ioctl(vdev, cmd);
> >
> > if (serialize)
> > mutex_lock(&vdev->v4l2_dev->serialize_lock);
> > /* Handles IOCTL */
> > err = __video_do_ioctl(file, cmd, parg);
> > if (serialize)
> > mutex_unlock(&vdev->v4l2_dev->serialize_lock);
> >
> >
> > And must_serialize_ioctl() looks like this:
> >
> > static int must_serialize_ioctl(struct video_device *vdev, int cmd)
> > {
> > if (!vdev->v4l2_dev || !vdev->v4l2_dev->fl_serialize)
> > return 0;
> > switch (cmd) {
> > case VIDIOC_QUERYCAP:
> > case VIDIOC_ENUM_FMT:
> > ...
> > return 0;
> > }
> > return 1;
> > }
> >
> > Basically the CAP (capability) ioctls and all ENUM ioctls do not have to be
> > serialized. I am not sure whether the streaming ioctls should also be
> > included
> > here. I can't really grasp the consequences of whatever choice we make. If
> > we
> > want to be compatible with what happens today with ioctl and the BKL, then
> > we
> > need to lock and have videobuf unlock whenever it has to wait for an event.
>
> I suspect that the list of "must have" is driver-dependent.
If needed one could allow drivers to override this function. But again, start
simple and only make it more complex if we really need to. Overengineering is
one of the worst mistakes one can make. I have seen too many projects fail
because of that.
Regards,
Hans
--
Hans Verkuil - video4linux developer - sponsored by TANDBERG
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] Serialization flag example
Hans Verkuil wrote:
>> Maybe a better alternative would be to pass to the V4L2 core, optionally,
>> some lock,
>> used internally on the driver. This approach will still be pedantic, as all
>> ioctls will
>> keep being serialized, but at least the driver will need to explicitly
>> handle the lock,
>> and the same lock can be used on other parts of the driver.
>
> Well, I guess you could add a 'struct mutex *serialize;' field to v4l2_device
> that drivers can set. But I don't see much of a difference in practice.
It makes easier to implement more complex approaches, where you may need to use
more
locks. Also, It makes no sense to make a DVB code or an alsa code dependent on
a V4L
header, just because it needs to see a mutex.
Also, a mutex at the driver need to be initialized inside the driver. It is not
just one
flag that someone writing a new driver will clone without really understanding
what
it is doing.
> Regarding the 'pedantic approach': we can easily move the locking to
> video_ioctl2:
>
> struct video_device *vdev = video_devdata(file);
> int serialize = must_serialize_ioctl(vdev, cmd);
>
> if (serialize)
> mutex_lock(&vdev->v4l2_dev->serialize_lock);
> /* Handles IOCTL */
> err = __video_do_ioctl(file, cmd, parg);
> if (serialize)
> mutex_unlock(&vdev->v4l2_dev->serialize_lock);
>
>
> And must_serialize_ioctl() looks like this:
>
> static int must_serialize_ioctl(struct video_device *vdev, int cmd)
> {
> if (!vdev->v4l2_dev || !vdev->v4l2_dev->fl_serialize)
> return 0;
> switch (cmd) {
> case VIDIOC_QUERYCAP:
> case VIDIOC_ENUM_FMT:
> ...
> return 0;
> }
> return 1;
> }
>
> Basically the CAP (capability) ioctls and all ENUM ioctls do not have to be
> serialized. I am not sure whether the streaming ioctls should also be included
> here. I can't really grasp the consequences of whatever choice we make. If we
> want to be compatible with what happens today with ioctl and the BKL, then we
> need to lock and have videobuf unlock whenever it has to wait for an event.
I suspect that the list of "must have" is driver-dependent.
--
Cheers,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] Serialization flag example
On Thursday 01 April 2010 20:24:12 Mauro Carvalho Chehab wrote: > Hans, > > > Hans Verkuil wrote: > > I made a quick implementation which is available here: > > > > http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-serialize > > > > It's pretty easy to use and it also gives you a very simple way to block > > access to the video device nodes until all have been allocated by simply > > taking the serialization lock and holding it until we are done with the > > initialization. > > > > I converted radio-mr800.c and ivtv. > > > > That said, almost all drivers that register multiple device nodes probably > > suffer from a race condition when one of the device node registrations > > returns an error and all devices have to be unregistered and the driver > > needs to release all resources. > > > > Currently most if not all drivers just release resources and free the > > memory. > > But if an application managed to open one device before the driver removes > > it > > again, then we have almost certainly a crash. > > > > It is possible to do this correctly in the driver, but it really needs core > > support where a release callback can be installed in v4l2_device that is > > called when the last video_device is closed by the application. > > > > We already can cleanup correctly after the last close of a video_device, but > > there is no top-level release yet. > > > > > > Anyway, I tried to use the serialization flag in bttv as well, but I ran > > into > > problems with videobuf. Basically when you need to wait for some event you > > should release the serialization lock and grab it after the event arrives. > > > > Unfortunately videobuf has no access to v4l2_device at the moment. If we > > would > > have that, then videobuf can just release the serialization lock while > > waiting > > for something to happen. > > > While this code works like a charm with the radio devices, it probably > won't work fine with complex devices. Nor was it meant to. Although ivtv is a pretty complex device and it works fine there. But yes, when you also have alsa, dvb or other parts then you will have to think more carefully and possibly abandon the core serialization altogether. However, of the almost 80 drivers we have it is my conservative estimate that about 75% of those fall in the 'simple' device category, at least when it comes to locking. I would like to see a simple, but good, locking scheme for those 60 drivers. And the remaining 20 can take care of their own locking. > You'll have issues also with -alsa and -dvb parts that are present on the > drivers. > > Also, drivers like cx88 have another PCI device for mpeg-encoded streams. It > behaves > like two separate drivers (each with its own bind code at V4L core), but, as > there's > just one PCI bridge, and just one analog video decoder/analog audio decoder, > the lock > should cross between the different drivers. > > So, binding videobuf to v4l2_device won't solve the issue with most > videobuf-aware drivers, > nor the lock will work as expected, at least with most of the devices. As I said, you have to enable this serialization explicitly. And obviously you shouldn't enable it mindlessly. > Also, although this is not a real problem, your lock is too pedantic: it is > blocking access to several ioctl's that don't need to be locked. For example, > there are > several ioctl's that just returns static: information: capabilities, > supported video standards, > etc. There are even some cases where dynamic ioctl's are welcome, like > accepting QBUF/DQBUF > without locking (or with a minimum lock), to allow different threads to > handle the buffers. Which is why videobuf should be aware of such a global lock so that it can release it when it has to wait. > The big issue I see with this approach is that developers will become lazy on > checking > the locks inside the drivers: they'll just apply the flag and trust that all > of their > locking troubles were magically solved by the core. Well, for simple drivers (i.e. the vast majority) that is indeed the case. Locking is hard. If this can be moved into the core for most drivers, then that is good. I like it if developers can be lazy. Less chance of bugs. And mind that this is exactly the situation we have now with ioctl and the BKL! > Maybe a better alternative would be to pass to the V4L2 core, optionally, > some lock, > used internally on the driver. This approach will still be pedantic, as all > ioctls will > keep being serialized, but at least the driver will need to explicitly handle > the lock, > and the same lock can be used on other parts of the driver. Well, I guess you could add a 'struct mutex *serialize;' field to v4l2_device that drivers can set. But I don't see much of a difference in practice. Regarding the 'pedantic approach': we can easily move the locking to video_ioctl2: struct video_device *vdev = video_devdata(file); int serialize = must_serialize
Re: [RFC] Serialization flag example
Hans, Hans Verkuil wrote: > I made a quick implementation which is available here: > > http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-serialize > > It's pretty easy to use and it also gives you a very simple way to block > access to the video device nodes until all have been allocated by simply > taking the serialization lock and holding it until we are done with the > initialization. > > I converted radio-mr800.c and ivtv. > > That said, almost all drivers that register multiple device nodes probably > suffer from a race condition when one of the device node registrations > returns an error and all devices have to be unregistered and the driver > needs to release all resources. > > Currently most if not all drivers just release resources and free the memory. > But if an application managed to open one device before the driver removes it > again, then we have almost certainly a crash. > > It is possible to do this correctly in the driver, but it really needs core > support where a release callback can be installed in v4l2_device that is > called when the last video_device is closed by the application. > > We already can cleanup correctly after the last close of a video_device, but > there is no top-level release yet. > > > Anyway, I tried to use the serialization flag in bttv as well, but I ran into > problems with videobuf. Basically when you need to wait for some event you > should release the serialization lock and grab it after the event arrives. > > Unfortunately videobuf has no access to v4l2_device at the moment. If we would > have that, then videobuf can just release the serialization lock while waiting > for something to happen. While this code works like a charm with the radio devices, it probably won't work fine with complex devices. You'll have issues also with -alsa and -dvb parts that are present on the drivers. Also, drivers like cx88 have another PCI device for mpeg-encoded streams. It behaves like two separate drivers (each with its own bind code at V4L core), but, as there's just one PCI bridge, and just one analog video decoder/analog audio decoder, the lock should cross between the different drivers. So, binding videobuf to v4l2_device won't solve the issue with most videobuf-aware drivers, nor the lock will work as expected, at least with most of the devices. Also, although this is not a real problem, your lock is too pedantic: it is blocking access to several ioctl's that don't need to be locked. For example, there are several ioctl's that just returns static: information: capabilities, supported video standards, etc. There are even some cases where dynamic ioctl's are welcome, like accepting QBUF/DQBUF without locking (or with a minimum lock), to allow different threads to handle the buffers. The big issue I see with this approach is that developers will become lazy on checking the locks inside the drivers: they'll just apply the flag and trust that all of their locking troubles were magically solved by the core. Maybe a better alternative would be to pass to the V4L2 core, optionally, some lock, used internally on the driver. This approach will still be pedantic, as all ioctls will keep being serialized, but at least the driver will need to explicitly handle the lock, and the same lock can be used on other parts of the driver. -- Cheers, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC] Serialization flag example
I made a quick implementation which is available here: http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-serialize It's pretty easy to use and it also gives you a very simple way to block access to the video device nodes until all have been allocated by simply taking the serialization lock and holding it until we are done with the initialization. I converted radio-mr800.c and ivtv. That said, almost all drivers that register multiple device nodes probably suffer from a race condition when one of the device node registrations returns an error and all devices have to be unregistered and the driver needs to release all resources. Currently most if not all drivers just release resources and free the memory. But if an application managed to open one device before the driver removes it again, then we have almost certainly a crash. It is possible to do this correctly in the driver, but it really needs core support where a release callback can be installed in v4l2_device that is called when the last video_device is closed by the application. We already can cleanup correctly after the last close of a video_device, but there is no top-level release yet. Anyway, I tried to use the serialization flag in bttv as well, but I ran into problems with videobuf. Basically when you need to wait for some event you should release the serialization lock and grab it after the event arrives. Unfortunately videobuf has no access to v4l2_device at the moment. If we would have that, then videobuf can just release the serialization lock while waiting for something to happen. Regards, Hans -- Hans Verkuil - video4linux developer - sponsored by TANDBERG -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
