Serialization flag example
----- Original Nachricht ----
Von: Andy Walls <[email protected]> An: Mauro Carvalho Chehab <[email protected]> Datum: 03.04.2010 02:47 Betreff: Re: [RFC] Serialization flag example
Quoted 5 lines · Mauro Carvalho Chehab
Quoted 3 lines
Devin Heitmueller wrote:
In the case of a V4L x DVB type of lock, this is not to protect some
memory, but,
Quoted 8 lines
Quoted 1 line
instead, to limit the usage of a hardware that is not capable of
simultaneously
Quoted 1 line
provide V4L and DVB streams. This is a common case on almost all devices,
but, as
Quoted 1 line
Hermann pointed, there are a few devices that are capable of doing both
analog
Quoted 1 line
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.
Quoted 1 line
I know a driver that does:
Me too ;) and Trent tested on cx88xx, IIRC.
Quoted 1 line
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
Quoted 2 lines
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.
Quoted 6 lines
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
Quoted 3 lines
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
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
Quoted 6 lines · 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.
Signature · 3 lines
Regards, Laurent Pinchart
Quoted 11 lines · On Tuesday 06 April 2010 00:46:11 Laurent Pinchart wrote:
Quoted 6 lines · 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
Signature · 1 line
Hans Verkuil - video4linux developer - sponsored by TANDBERG
Quoted 15 lines · On Tuesday 06 April 2010 00:58:54 Hans Verkuil wrote:
Quoted 11 lines · On Tuesday 06 April 2010 00:46:11 Laurent Pinchart wrote:
Quoted 6 lines · 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:
Quoted 7 lines
http://linuxtv.org/hg/~hverkuil/v4l-dvb-ser2/
It adds two callbacks to
ioctl_ops:pre_hookandpost_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
Quoted 7 lines
Regards,
Hans
Signature · 1 line
Hans Verkuil - video4linux developer - sponsored by TANDBERG
Quoted 34 lines · Hans Verkuil
Quoted 13 lines · On Tuesday 06 April 2010 00:58:54 Hans Verkuil wrote:
Quoted 10 lines · On Tuesday 06 April 2010 00:46:11 Laurent Pinchart wrote:
Quoted 6 lines · 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:
Quoted 7 lines
http://linuxtv.org/hg/~hverkuil/v4l-dvb-ser2/
It adds two callbacks to
ioctl_ops:pre_hookandpost_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_devicestruct in the control handler and from there you can accessioctl_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.
Quoted 8 lines
Something like this should work:
int
pre_hook(struct video_device *vdev, enum v4l2_priority prio, int cmd); voidpost_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_fhstruct 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.
Signature · 2 lines
Cheers, Mauro
Quoted 68 lines
Quoted 50 lines · Hans Verkuil
Quoted 17 lines · On Tuesday 06 April 2010 00:58:54 Hans Verkuil wrote:
Quoted 7 lines · On Tuesday 06 April 2010 00:46:11 Laurent Pinchart wrote:
Quoted 6 lines · 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.
Quoted 5 lines
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:
Quoted 8 lines
It adds two callbacks to
ioctl_ops:pre_hookandpost_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_devicestruct in the control handler and
from there you can accessioctl_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);
voidpost_hook(struct video_device *vdev, int cmd);Quoted 5 lines
The prio is there to make priority checking possible. It will be initially
unused, but as soon as the events API with the newv4l2_fhstruct 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.
Quoted 4 lines
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?
Quoted 6 lines
- 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.
Quoted 4 lines
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
Quoted 10 lines
--
Cheers,
Mauro
--
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
Signature · 1 line
Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom
Quoted 7 lines · Hans Verkuil
Quoted 4 lines
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.
Quoted 9 lines
Quoted 6 lines
- 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
-EBUSYfor 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).
Quoted 11 lines
Quoted 4 lines
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.
Quoted 6 lines
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.
Signature · 2 lines
Cheers, Mauro