Re: [PATCH] media: zoran: move to dma-mapping interface
On Wed, 2018-04-25 at 14:22 -0300, Mauro Carvalho Chehab wrote: > Em Wed, 25 Apr 2018 17:58:25 +0200 > Arnd Bergmann <a...@arndb.de> escreveu: > > > On Wed, Apr 25, 2018 at 5:26 PM, Christoph Hellwig <hch@infradead.o > > rg> wrote: > > > On Wed, Apr 25, 2018 at 01:15:18PM +0200, Arnd Bergmann wrote: > > > > That thought had occurred to me as well. I removed the oldest ISDN > > > > drivers already some years ago, and the OSS sound drivers > > > > got removed as well, and comedi got converted to the dma-mapping > > > > interfaces, so there isn't much left at all now. This is what we > > > > have as of v4.17-rc1: > > > > > > Yes, I've been looking at various grotty old bits to purge. Usually > > > I've been looking for some non-tree wide patches and CCed the last > > > active people to see if they care. In a few cases people do, but > > > most often no one does. > > > > Let's start with this one (zoran) then, as Mauro is keen on having > > all media drivers compile-testable on x86-64 and arm. > > > > Trent Piepho and Hans Verkuil both worked on this driver in the > > 2008/2009 timeframe and those were the last commits from anyone > > who appears to have tested their patches on actual hardware. > > Zoran is a driver for old hardware. I don't doubt that are people > out there still using it, but who knows? > > I have a few those boards packed somewhere. I haven't work with PCI > hardware for a while. If needed, I can try to seek for them and > do some tests. I need first to unpack a machine with PCI slots... > the NUCs I generally use for development don't have any :-) > > Anyway, except for virt_to_bus() and related stuff, I think that this > driver is in good shape, as Hans did a lot of work in the past to > make it to use the current media framework. I still have a zoran board. And my recently purchased ryzen system has PCI slots. To my surprise they are not uncommon on new socket AM4 boards. However, I think the zoran board I have is 5V PCI and that is rather uncommon. Also becoming uncommon is analog NTSC/PAL video that this chip is designed for! If anyone is using these still, they would be in legacy systems for these legacy video formats.
Re: [ANNOUNCE] git tree repositories
On Thu, 21 Jan 2010, Mauro Carvalho Chehab wrote: OTOH, since with git it is common to have multiple branches within one repository, I'm not sure how it works. It would be cool if git would support per-branch descriptions, and git web could display them. I don't think git supports it. In kernel.org, people prefer to use more than one repository when they have more than one need. stgit lets me set descriptions for each branch. The descriptions are there under the branch in the config file. I don't think git-branch shows any kind of description for the branch. -- 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: How to use saa7134 gpio via gpio-sysfs?
On Sat, 16 Jan 2010, hermann pitton wrote: Am Freitag, den 15.01.2010, 17:27 -0800 schrieb Trent Piepho: On Sat, 16 Jan 2010, hermann pitton wrote: Am Dienstag, den 12.01.2010, 04:13 +0100 schrieb hermann pitton: gpio-sysfs creates /sys/class/gpio/export /sys/class/gpio/import but no gpion entries so far. The saa713x driver predates the generic gpio layer by years and years, so it doesn't use it. It also doesn't need to use it. Since the gpios are managed by the saa713x driver, and they also used by the saa713x driver, there is no need to interface two different drivers together. There are tons of drivers for devices that have gpios like this, but they don't use the gpio layer. But with gpio access via sysfs for generic gpios, there is something useful about having the saa713x driver support generic gpios. IIRC, somehow wrote a gpio only bt848 driver that didn't do anything but export gpios. In order to do this, you'll have to write code for the saa7134 driver to have it register with the gpio layer. I think you could still have the saa7134 driver itself use its gpio directly. That would avoid a performance penalty in the driver. Thanks for more details, but I'm still wondering what pins ever could be interesting in userland, given that they are all treated such different per device, and we count up to 200 different boards these days. There are some cards for intended for survilence or embedded applications that have headers on them to connect things to the GPIOs. Like alarms or camera controllers and stuff like that. The GPIO only bttv driver was created by someone who just soldered a bunch of wires on a cheap bt848 card, you can get them for just a few dollars, as it was a cheap and easy way to get a bunch of gpios in a pc. See his page here http://www.bu3sch.de/joomla/index.php/bt8xx-based-gpio-card There are cards you can get that just have GPIOs, but they end up being rather expensive. Here's one: http://www.acromag.com/parts.cfm?Model_ID=317Product_Function_ID=4Category_ID=18Group_ID=1 Way fancier than a tv card, but it's $600. I think if I was doing the coding, I'd add a field in the card description for what GPIOs should be exported. I.e., which ones have an external header. Maybe in addition to, or instead of, I'd have a module option that would cause GPIOs to be exported. A bitmask of which to export would be enough. -- 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: How to use saa7134 gpio via gpio-sysfs?
On Sat, 16 Jan 2010, hermann pitton wrote: Am Dienstag, den 12.01.2010, 04:13 +0100 schrieb hermann pitton: gpio-sysfs creates /sys/class/gpio/export /sys/class/gpio/import but no gpion entries so far. You have to explictly export the GPIO lines to get them to appear. Either in the kernel with gpio_export() or from sysfs. You also can't export GPIOs that something in the kernel is using. My original design didn't have this restriction. I felt there were valid debug and development reasons for doing so, having used them myself for debug and development of embedded systems, but David Brownell felt otherwise. I didn't even have the concept of export originally, all the gpios would show up. After all, all your PCI and USB devices, I2C chips or busses, etc. show up in sysfs. Nothing else does this must be exported to show up business. You can memory map the saa7133 PCI address space and modify the gpios, and anything else, with direct register access from userspace, and that's just fine. But being able to observe and modify the gpios with a gpio-only interface is just way too dangerous. Doesn't make sense. But I'm digressing. This sysfs interface only works with the gpio using the generic gpio layer. This was designed for generic gpios on SoCs that would be providing by some kind of platform driver or I2C gpio extenders. The gpios get and used by various other drivers. Like say write protects on memory cards, or system LEDs. I wrote a JTAG driver that used it. The point of the gpio layer is to interface drivers the provide gpios with other, different, drivers that use the gpio. It was introduced in the mid 2.6.20s IIRC. The saa713x driver predates the generic gpio layer by years and years, so it doesn't use it. It also doesn't need to use it. Since the gpios are managed by the saa713x driver, and they also used by the saa713x driver, there is no need to interface two different drivers together. There are tons of drivers for devices that have gpios like this, but they don't use the gpio layer. But with gpio access via sysfs for generic gpios, there is something useful about having the saa713x driver support generic gpios. IIRC, somehow wrote a gpio only bt848 driver that didn't do anything but export gpios. In order to do this, you'll have to write code for the saa7134 driver to have it register with the gpio layer. I think you could still have the saa7134 driver itself use its gpio directly. That would avoid a performance penalty in the driver. -- 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 v2] Another approach to IR
On Wed, 2 Dec 2009, Jarod Wilson wrote: My main point is that each of these devices has device ID that can be determined without having to first do some protocol analysis and table lookups to figure out which device some random IR input is actually coming from. Heh, right back at ya ;) The fact that you need to do some more work to separate 2 physical devices does not mean it should not be done. No, but it means added complexity inside the kernel. I'm questioning whether the added complexity is worth it, when I doubt the vast majority of users would take advantage of it, and it can already be done in userspace. Although... Damn. The userspace approach would only work if the device were passing raw IR to userspace, so in the in-kernel decoding case, yeah, I guess you'd need separate input devices for each remote to use them independently. Meh. Doubt I'd ever use it, but I guess I'll concede that it makes some sense to do the extra work. You just need to send a tuple that contrains the keycode plus some kind of id for the remote it came from. That's what I did for lirc, it decodes the sparc/mark into a remote id and key code tuple. It's certainly a common thing to want. Anyone who has existing remotes and components that use them would want it. You don't want your computer turning off when you push the power button on the DVD player's remote, do you? Each host connected to a network interface doesn't get its own device. -- 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 v2] Another approach to IR
On Wed, 2 Dec 2009, Jon Smirl wrote: A bluetooth remote has a specific device ID that the receiver has to pair with. Your usb mouse and keyboard each have specific device IDs. A usb IR *receiver* has a specific device ID, the remotes do not. So there's the major difference from your examples. Actually remotes do have an ID. They all transmit vendor/device pairs which is exactly how USB works. *All* remotes? That's a bold statement. I'm sure there are some that only transmit 8 bits of so of scancode. I think remotes are more like hosts on a network than usb or bluetooth devices. Remotes do not join or leave a receiver, while things like usb devices do explictly join and leave the hub. Now I understand that if 2 remotes send completely identical signals we won't be able to separate them, but in cases when we can I think we should. I don't have a problem with that, if its a truly desired feature. But for the most part, I don't see the point. Generally, you go from having multiple remotes, one per device (where device is your TV, amplifier, set top box, htpc, etc), to having a single universal remote that controls all of those devices. But for each device (IR receiver), *one* IR command set. The desire to use multiple distinct remotes with a single IR receiver doesn't make sense to me. Perhaps I'm just not creative enough in my use of IR. :) Most universal remotes I'm familiar with emulate multiple remotes. I.e. my tv remote generates one set of scancodes for the numeric keys. The DVD remote generates a different set. The amplifier remote in tv mode generates the same codes as the tv remote, and in dvd mode the same codes as the dvd remote. From the perspective of the IR receiver there is no difference between having both the DVD and TV remotes, or using the aplifier remote to control both devices. Now, my aplifier remote has a number of modes. Some control devices I have, like vcr mode, and there is nothing I can do about that. Some, like md mode don't control devices I have. That means they are free to do things on the computer. Someone else with the same remote (or any number of remotes that use the same protocol and scancodes) might have different devices. So I want my computer to do stuff when I push JVC MD #xx keys, but ignore JVC VCR #yyy yets. Someone with an MD player and not a VCR would want to opposite. Rather than force everyone to create custom keymaps, it's much easier if we can use the standard keymaps from a database of common remotes and simply tell mythtv to only use remote #xxx or not to use remote #yyy. It sounds like you're thinking of a receiver that came bundled with a remote and that's it. Not someone with a number of remotes that came with different pieces of AV gear that they want to use with their computer. -- 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: Replace Mercurial with GIT as SCM
On Tue, 1 Dec 2009, Patrick Boettcher wrote: To start right away: I'm in favour of using GIT because of difficulties I have with my daily work with v4l-dvb. It is in my nature do to mistakes, so I need a tool which assists me in fixing those, I have not found a simple way to do my stuff with HG. Try the mq extension. It's included by default with mercurial, you just need to add: [extensions] hgext.mq= to your .hgrc file. It lets you maintain a stack of patches that you can freely push and pop. You can make changes and then commit them to one of the existing patches. Like git commit -amend, except you can amend any patch not just the last one. IMHO, it's better than stock git when you're trying to make a good patch series. There is something called stgit which is very much like mq and a little better I think. -- 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] Should we create a raw input interface for IR's ? - Was: Re: [PATCH 1/3 v2] lirc core device driver infrastructure
On Thu, 26 Nov 2009, Mauro Carvalho Chehab wrote: See above. Also, several protocols have a way to check if a keystroke were properly received. When handling just one protocol, we can use this to double check the key. However, on a multiprotocol mode, we'll need to disable this feature. I don't think so. We can pass the space/mark data to all (configured, i.e. with active mapping) protocol handlers at once. Should a check fail, we ignore the data. Perhaps another protocol will make some sense out of it. What happens if it succeeds on two protocol handlers? Then you use the protocol that fits best. For instance decoding with one protocol might produce a scancode that isn't assigned to any key, while another protocol produces an assigned scancode. Clearly then the latter is most likely to be correct. It also possible to have a space/mark length that is within the allowable tolerances for one remote, but is even closer another remote. You don't want to just find *a* match, you want to find the *best* match. The in kernel code in v4l is very simple in that it is only designed to work with one procotol and one remote. Once you have multiple remotes of any type things become much more complicted. Keep in mind that remotes that aren't even intended to be used with the computer but are used in the same room will still be received by the receiver. It's not enough to decode the signals you expect to receive, you must also not get confused by random signals destined for somewhere else. -- 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] Should we create a raw input interface for IR's ? - Was: Re: [PATCH 1/3 v2] lirc core device driver infrastructure
On Thu, 26 Nov 2009, Mauro Carvalho Chehab wrote: lircd supports input layer interface. Yet, patch 3/3 exports both devices that support only pulse/space raw mode and devices that generate scan codes via the raw mode interface. It does it by generating artificial pulse codes. Nonsense! There's no generation of artificial pulse codes in the drivers. The LIRC interface includes ways to pass decoded IR codes of arbitrary length to userspace. I might have got wrong then a comment in the middle of the imon_incoming_packet() of the SoundGraph iMON IR patch: + /* + * Translate received data to pulse and space lengths. + * Received data is active low, i.e. pulses are 0 and + * spaces are 1. I'm not sure about this specific code, but what is likely going on here is the waveform is being RLE encoding. For example, a cx88 receiver has two ways of being connected (without using an external decoder chip). One generates an IRQ on each edge of the signal. The time between IRQs gives mark/space lengths which is what lirc expects. This is how a simple serial port receiver works too. Another connections effectivly samples the waveform one bit deep at IIRC 4kHz. I think that's what the code you are looking at gets. The code extracts the edges from the waveform and returns the time between them. In effect one is run length encoding a sequence of bits. -- 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: IR raw input is not sutable for input system
On Wed, 25 Nov 2009, Maxim Levitsky wrote: Its not the case. There are many protocols, I know that by experimenting with my universal remote. There are many receivers, and all have different accuracy. Most remotes aren't designed to be used with PC, thus user has to invent mapping between buttons and actions. Its is not possible to identify remotes accurately, many remotes send just a 8 bit integer that specifies the 'model' thus many remotes can share it. The signal recevied by the ir receiver contains glitches. Depending on the receiver there can be quite a few. It is also not trivial to turn the raw signal sent by the remote into a digital value, even if you know what to expect. It takes digital signal processing techniques to turn the messy sequence of inaccurate mark and space lengths into a best guess at what digital code the remote sent. It's like turning raw VBI data into decoded ASCII teletext from a simulated keyboard device, all in the kernel. Kernel job is to take the information from device and present it to userspace using uniform format, that is kernel does 1:1 translating, but doesn't parse the data. One thing that could be done, unless it has changed much since I wrote it 10+ years ago, is to take the mark/space protocol the ir device uses and sent that data to lircd via the input layer. It would be less efficient, but would avoid another kernel interface. Of course the input layer to lircd interface would be somewhat different than other input devices, so it's not entirely correct to say another interface is avoided. -- 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: [PATCH] tuner-simple: possible read buffer overflow?
On Sat, 3 Oct 2009, Roel Kluin wrote:
Prevent read from t_params-ranges[-1].
Signed-off-by: Roel Kluin roel.kl...@gmail.com
---
This is only required when t_params-count can be 0, can it?
Shouldn't be possible, or the tuner would be useless.
- if (i == t_params-count) {
+ if (i == t_params-count i) {
tuner_dbg(frequency out of range (%d %d)\n,
*frequency, t_params-ranges[i - 1].limit);
*frequency = t_params-ranges[--i].limit;
--
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: V4L2 drivers: potentially dangerous and inefficient msecs_to_jiffies() calculation
On Mon, 14 Sep 2009, Andreas Mohr wrote: cam-module_param.frame_timeout * 1000 * msecs_to_jiffies(1) ); multiple times each. What they should do instead is frame_timeout * msecs_to_jiffies(1000), I'd think. msecs_to_jiffies(1) is quite a bit too boldly assuming that all of the msecs_to_jiffies(x) implementation branches always round up. It might also wait far longer than desired. On a 100 Hz kernel a jiffie is 10 ms. 1000 * msecs_to_jiffies(1) will wait 10 seconds instead of 1. But, I believe there is an issue with msecs_to_jiffies(x) overflowing for very high values of x. I'm not sure where that point is though. -- 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] Infrared Keycode standardization
On Thu, 27 Aug 2009, Devin Heitmueller wrote: The biggest challenge with that approach is that lirc is still maintained out-of-kernel, and the inputdev solution does not require lirc at all (which is good for inexperienced end users who want their product to just work). If distros started packing lirc as a basic system daemon things would generally just work too. After all, there is plenty of other user space software one needs to do anything. The other big issue is that right now remotes get associated automaticallywith products as part of the device profile. While this has the disadvantage that there is not a uniform mechanism to specify a different remote than the one that ships with the product, it does have the advantage of the product working out-of-the-box with whatever remote it came with. It's a usability issue, but what I would consider a pretty important one. lirc isn't limited to one remote at a time. You can have many different remotes supported at once. So it's not always necessary to know which remote you have before the remote will work. -- 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: [linux-dvb] Can ir polling be turned off in cx88 module for Leadtek 1000DTV card?
On Wed, 26 Aug 2009, Andy Walls wrote: On Wed, 2009-08-26 at 07:33 -0700, Dalton Harvie wrote: If there isn't, would it be a good idea? Maybe. Thanks for any help. Try this. It adds a module option noir that accepts an array of int's. For a 0, that card's IR is set up as normal; for a 1, that card's IR is not initialized. # modprobe cx88 noir=1,1 I think this is a good idea. I was going to do someting similar to stop the excessive irqs from my cx88 cards, which don't even have remote receivers. I haven't tried, but maybe it is possible to only turn on polling when the event device is opened. -- 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: [PULL] http://www.linuxtv.org/hg/~hverkuil/v4l-dvb
On Thu, 6 Aug 2009, Hans Verkuil wrote: On Thursday 06 August 2009 20:10:38 Trent Piepho wrote: On Thu, 6 Aug 2009, Hans Verkuil wrote: Why do you need two routines that will always return zero? Why to create a code that will never be used? v4l2-compat-ioctl32.c is already complex enough without adding any bogus code. When the RDS encoder driver from Eduardo is added, then he will add the string controls to ctrl_is_pointer() since his driver is the first to actually implement string controls. Could one of the upper bits of the control id be used as a flag? It would make it a lot easier to check the control's type than by using some inline function with a giant case statement that needs to be updated each time a new control is added. No, you can't. That would require users to OR that flag each time you want to use such a control. My original plan was to just check for a non-zero There is no reason we need to allocated control IDs sequentially, is there? So just give string controls IDs in the range 0x8000 to 0x and then one can tell if a control is a string control be ANDing the id with (131). -- 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: double unlock in bttv_poll() and in saa7134-video.c
On Sat, 25 Jul 2009, Dan Carpenter wrote:
My source code checker, smatch (http://repo.or.cz/w/smatch.git),
complains about a double unlock in bttv_poll() from
drivers/media/video/bt8xx/bttv-driver.c. It unlocks on line 3190 and
again on 3201.
How about this:
http://linuxtv.org/hg/~tap/bttv?cmd=changeset;node=35ddb77b68f8
diff -r fd96af63f79b -r 35ddb77b68f8
linux/drivers/media/video/bt8xx/bttv-driver.c
--- a/linux/drivers/media/video/bt8xx/bttv-driver.c Fri Jun 19 19:56:56
2009 +
+++ b/linux/drivers/media/video/bt8xx/bttv-driver.c Wed Jul 29 16:33:45
2009 -0700
@@ -3191,15 +3191,14 @@ static unsigned int bttv_poll(struct fil
return videobuf_poll_stream(file, fh-vbi, wait);
}
+ mutex_lock(fh-cap.vb_lock);
if (check_btres(fh,RESOURCE_VIDEO_STREAM)) {
- mutex_lock(fh-cap.vb_lock);
/* streaming capture */
if (list_empty(fh-cap.stream))
goto err;
buf = list_entry(fh-cap.stream.next,struct
bttv_buffer,vb.stream);
} else {
/* read() capture */
- mutex_lock(fh-cap.vb_lock);
if (NULL == fh-cap.read_buf) {
/* need to capture a new frame */
if (locked_btres(fh-btv,RESOURCE_VIDEO_STREAM))
@@ -3217,7 +3216,6 @@ static unsigned int bttv_poll(struct fil
fh-cap.ops-buf_queue(fh-cap,fh-cap.read_buf);
fh-cap.read_off = 0;
}
- mutex_unlock(fh-cap.vb_lock);
buf = (struct bttv_buffer*)fh-cap.read_buf;
}
--
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: lsmod path hardcoded in v4l/Makefile
On Mon, 27 Jul 2009, Mauro Carvalho Chehab wrote: Em Tue, 21 Jul 2009 09:14:36 +0200 Matthias Schwarzott z...@gentoo.org escreveu: It is not a good idea to run as root. Most people compile everything with a normal user and then use sudo command to install/remove/insert modules. Unfortunately, depending on the distribution, sudo inherits PATH from the normal user, instead of root. Due to that, if you replace it for just lsmod, it will fail for people that don't use gentoo. Maybe good solution is to test if lsmod (and other similar tools) are at /sbin or /usr/sbin. Alternatively, we can try to replace lsmod by something like (untested): v4l_modules := $(shell PATH=$PATH:/usr/local/sbin:/usr/sbin:/sbin lsmod|cut -d' ' -f1 ) $(patsubst %.ko,%,$(inst-m)) Check my patch again, we can just delete the v4l_modules line as nothing uses it. -- 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: [Bugme-new] [Bug 13709] New: b2c2-flexcop: no frontend driver found for this B2C2/FlexCop adapter w/ kernel-2.6.31-rc2
On Mon, 20 Jul 2009, Andrew Morton wrote: On Mon, 20 Jul 2009 13:21:33 -0700 (PDT) Trent Piepho xy...@speakeasy.org wrote: On Mon, 20 Jul 2009, Andrew Morton wrote: I produced a patch that fixed this problem over a month ago, http://www.linuxtv.org/hg/~tap/v4l-dvb/rev/748c762fcf3e Where is that patch now? It isn't present in linux-next. Mauro has how pulled it from me and so it will probably show up in his tree soon. Also, is there any way of avoiding this? +#define FE_SUPPORTED(fe) (defined(CONFIG_DVB_##fe) || \ + (defined(CONFIG_DVB_##fe##_MODULE) defined(MODULE))) That's just way too tricky. It expects all versions of the preprocessor to be correctly implemented (unlikely) and there are other tools like unifdef which want to parse kernel #defines. What's so tricky about it? A quick grep shows hundreds of uses of ## for concatenation. otoh the trick does produce a nice result and doing it any other way (which I can think of) would make a mess. This particular kind of test is something that is used many times in the dvb code. This isn't the first time someone has gotten it wrong by a long shot. Mauro suggested, and I agree, that for the next kernel we should put FE_SUPPORTED into a dvb header file and convert the many existing tests to use it. Maybe this will reduce the number of mistakes. Or just revert whichever patch broke things. Your changelog describes this as simply A recent patch (bad changelog!) so I am unable to judge this. It was commit d66b94b4aa2f40e134f8c07c58ae74ef3d523ee0, but it was not committed until five days after I made my patch, so there was no hash to reference yet. I suppose I should have used the patch title. -- 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: [Bugme-new] [Bug 13709] New: b2c2-flexcop: no frontend driver found for this B2C2/FlexCop adapter w/ kernel-2.6.31-rc2
On Mon, 20 Jul 2009, Andrew Morton wrote: (switched to email. Please respond via emailed reply-to-all, not via the bugzilla web interface). Guys, this is reportedly a post-2.6.30 regression - I'll ask Rafael to add it to the regression tracking list. btw, does the flexcop driver have a regular maintainer? Or someone who wants to volunteer? MAINTAINERS is silent about it.. I produced a patch that fixed this problem over a month ago, http://www.linuxtv.org/hg/~tap/v4l-dvb/rev/748c762fcf3e Maybe it should go into 2.6.31? Thanks. On Sun, 5 Jul 2009 01:36:31 GMT bugzilla-dae...@bugzilla.kernel.org wrote: http://bugzilla.kernel.org/show_bug.cgi?id=13709 Summary: b2c2-flexcop: no frontend driver found for this B2C2/FlexCop adapter w/ kernel-2.6.31-rc2 Product: v4l-dvb Version: unspecified Kernel Version: 2.6.31-rc1 Platform: All OS/Version: Linux Tree: Mainline Status: NEW Severity: normal Priority: P1 Component: dvb-frontend AssignedTo: v4l-dvb_dvb-front...@kernel-bugs.osdl.org ReportedBy: bugzilla.kernel@boris64.net Regression: Yes Hi kernel people! Since kernel-2.6.31-rc1 my Technisat SkyStar2 DVB card isn't working anymore, because no frontend driver is found. The frontend 'ST STV0299 DVB-S' is compiled into the kernel and _did_ work fine in pre-2.6.31 kernels. [lspci] ... 05:02.0 Network controller: Techsan Electronics Co Ltd B2C2 FlexCopII DVB chip / Technisat SkyStar2 DVB card (rev 02) [/lspci] [dmesg] Working kernel-2.6.30.1: ... b2c2-flexcop: B2C2 FlexcopII/II(b)/III digital TV receiver chip loaded successfully b2c2_flexcop_pci :05:02.0: PCI INT A - GSI 18 (level, low) - IRQ 18 b2c2-flexcop: MAC address = 00:d0:d7:0f:30:58 b2c2-flexcop: found 'ST STV0299 DVB-S' . b2c2-flexcop: initialization of 'Air2PC/AirStar 2 ATSC 3rd generation (HD5000)' at the 'PCI' bus controlled by a 'FlexCopIIb' complete ... Non-working kernel-2.6.31-rc: ... b2c2-flexcop: B2C2 FlexcopII/II(b)/III digital TV receiver chip loaded successfully b2c2_flexcop_pci :05:02.0: PCI INT A - GSI 18 (level, low) - IRQ 18 b2c2-flexcop: MAC address = 00:d0:d7:0f:30:58 b2c2-flexcop: no frontend driver found for this B2C2/FlexCop adapter b2c2_flexcop_pci :05:02.0: PCI INT A disabled ... [/dmesg] I'll attach full dmesg+lspci. Please feel free to contact me if you need more infos. Thank you in advance ;) -- Configure bugmail: http://bugzilla.kernel.org/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are on the CC list for the bug. -- 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 -- 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: [PATCH 1/2] Compatibility layer for hrtimer API
On Fri, 3 Jul 2009, Jean Delvare wrote:
Kernels 2.6.22 to 2.6.24 (inclusive) need some compatibility quirks
for the hrtimer API. For older kernels, some required functions were
not exported so there's nothing we can do. This means that drivers
using the hrtimer infrastructure will no longer work for kernels older
than 2.6.22.
Signed-off-by: Jean Delvare kh...@linux-fr.org
---
v4l/compat.h | 18 ++
1 file changed, 18 insertions(+)
--- a/v4l/compat.h
+++ b/v4l/compat.h
@@ -480,4 +480,22 @@ static inline unsigned long v4l_compat_f
}
#endif
+/*
+ * Compatibility code for hrtimer API
+ * This will make hrtimer usable for kernels 2.6.22 and later.
+ * For earlier kernels, not all required functions are exported
+ * so there's nothing we can do.
+ */
+
+#if LINUX_VERSION_CODE KERNEL_VERSION(2, 6, 25) \
+ LINUX_VERSION_CODE = KERNEL_VERSION(2, 6, 22)
+#include linux/hrtimer.h
Instead of including hrtimer.h from compat.h it's better if you check if it
has already been included and only enable the compat code in that case.
That way hrtimer doesn't get included for files that don't need it and
might define something that conflicts with something from hrtimer. And it
prevents someone from forgetting to include hrtimer when they needed it,
but having the error masked because compat.h is doing it for them.
+/* Forward a hrtimer so it expires after the hrtimer's current now */
+static inline unsigned long hrtimer_forward_now(struct hrtimer *timer,
+ ktime_t interval)
+{
+ return hrtimer_forward(timer, timer-base-get_time(), interval);
+}
+#endif
+
#endif /* _COMPAT_H */
--
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: lsmod path hardcoded in v4l/Makefile
On Tue, 23 Jun 2009, Matthias Schwarzott wrote: On Mon, 2009-06-22 at 16:36 +0200, Matthias Schwarzott wrote: It seems the path to lsmod tool is hardcoded in the Makefile for out-of-tree building of v4l-dvb. Shouldn't $PATH of root be considered safe? Else the distro or the system I believe make will set the variable whenever the makefile is used, even when building as non-root. It turns out that it was just lsmod with no path originally, but Michael Krufky changed it back in 2005 (commit b0e7b40744ef) to have a hardcoded path. Then later in commit c91e7f84a1d6 the only use of 'v4l_modules' was deleted, so we can just delete this line and not worry about sbin and paths. Mauro, Please pull from http://linuxtv.org/hg/~tap/fix for the following changeset: build: Remove module list cruft http://linuxtv.org/hg/~tap/fix?cmd=changeset;node=fb228bb1ad9f -- 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: lsmod path hardcoded in v4l/Makefile
On Mon, 22 Jun 2009, Andy Walls wrote: On Mon, 2009-06-22 at 16:36 +0200, Matthias Schwarzott wrote: Hi list! It seems the path to lsmod tool is hardcoded in the Makefile for out-of-tree building of v4l-dvb. Now at least gentoo has moved lsmod from /sbin to /bin. Won't your patch cause breakage for everyone who hasn't moved lsmod from /sbin and doesn't have sbin in the path? Which was, and perhaps still is, the most common situation? It would be better to do something that does not break things that used to work. Additionally it is bad style (or at least I am told so), to not rely on $PATH but hardcode pathes for tools that should be in $PATH. It's a potential security hole to rely on $PATH instead of absolute paths when running a command as root. Since many of the commnads in the Makefile rely on $PATH, including executions of 'install' which usually would be run as root, I suppose secuirty concerns don't matter. -Andy So the attached patch removes the hardcoded /sbin from the lsmod call. Signed-off-by: Matthias Schwarzott z...@gentoo.org Regards Matthias -- 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 -- 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: [PULL] http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-misc
On Sat, 20 Jun 2009, Hans Verkuil wrote: - compat: fix __fls check for the arm architecture. This one isn't quite right. The __fls defined for arm in 2.6.27 (between v2.6.26-7260-g0c65f45 and v2.6.28-rc6-187-g94fc733) isn't the same as the __fls() used everwhere else in the kernel. This alternate fix should work. 01/01: compat: Fix __fls for certain ARM kernels http://linuxtv.org/hg/~tap/fix?cmd=changeset;node=518b7754cd3d -- 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: [PATCH] af9015: avoid magically sized temporal buffer in eeprom_dump
On Thu, 18 Jun 2009, Jan Nikitenko wrote: Replace printing to magically sized temporal buffer with use of KERN_CONT temporary not temporal. - sprintf(buf2, %02x , val); + deb_info(KERN_CONT, %02x, val); No comma after KERN_CONT else - strcpy(buf2, -- ); - strcat(buf, buf2); + deb_info(KERN_CONT, --); No comma after KERN_CONT Just use printk() instead of deb_info() for the ones that use KERN_CONT. if (reg == 0xff) break; } - deb_info(%s\n, buf); + deb_info(KERN_CONT \n); return 0; } -- 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 -- 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: [PATCH] adding support for setting bus parameters in sub device
On Wed, 17 Jun 2009, Guennadi Liakhovetski wrote: On Wed, 17 Jun 2009, Hans Verkuil wrote: It is my strong opinion that while autonegotiation is easy to use, it is not a wise choice to make. Filling in a single struct with the bus settings to use for each board-subdev combination (usually there is only one) is simple, straight-forward and unambiguous. And I really don't see why that should take much time at all. And I consider it a very good point that the programmer is forced to think about this for a bit. Ok, my opinion is, that we should keep autonegotiation, but if you like, we can print a BIG-FAT-WARNING if both polarities are supported and no platform preference is set. I think, we've heard all opinions, unless someone would like to add something? Would it be fair to ask Mauro to make a decision? Or we can just count votes (which I would obviously prefer), but I'll accept Mauro's decision too. There is a similar situation in the networking code, where there is a driver for a PHY and another driver for a MAC, much like a sensor and bridge. phylib will find the common subset of the supported modes between the MAC and the detected PHY and use that to configure aneg advertisement settings. -- 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: [PULL] generic image bounds setting and alignment function
On Wed, 17 Jun 2009, Guennadi Liakhovetski wrote:
On Tue, 16 Jun 2009, Trent Piepho wrote:
/* up the smaller alignment until we have enough */
do {
- if (walign = halign walign wmaxa) {
+ if (halign = hmaxa ||
+ (walign = halign walign wmaxa)) {
Do I understand it right now, that as soon as your halign now reaches
hmaxa, you'll stop incrementing halign and then keep incrementing walign
even beyond wmaxa?...
Yes.
It's clearly documented that the function isn't designed to handle
impossible constraints. If both walign and halign are at max and there
still needs to be more alignment then there is no possible resolution, no
matter what width height started at.
This is just one of many ways the constraints could be impossible. Max
width being less than min width is another obvious example. I decided it
wasn't worth complicating the code to check for all of these things. And
if the function did return failure the callers would need to check that.
None of the code v4l_bound_align_image() replaces has a failure path for
impossible image size constraints, because the constraints aren't
impossible.
--
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: bttv problem loading takes about several minutes
On Wed, 17 Jun 2009, Halim Sahin wrote: Hi, In the past I could use this card by typing modprobe bttv card=34 tuner=24 gbuffers=16 What card do you actually have? What is the PCI subsystem vendor/device and what tuner does it actually have? Hans, the problem might be with bttv audio probing. This card has needs_tvaudio set to 0, which used to mean that tvaudio would not be probed or loaded. But with your changes to bttv audio probing this behavior has changed. Now tvaudio is always loaded and probed if another audio chip hasn't been detected. The needs_tvaudio field is totally ignored. Giving this command with current drivers has some problems: 1. it takes several minutes to load bttv module. 2. capturing doesn't work any more (dropped frames etc). Tested with current v4l-dvb from hg, ubuntu 9.04, debian lenny. I have a bt878 based card from leadtek. Here is my output after loading the driver: [ 3013.735459] bttv: driver version 0.9.17 loaded [ 3013.735470] bttv: using 32 buffers with 16k (4 pages) each for capture [ 3013.735542] bttv: Bt8xx card found (0). [ 3013.735562] bttv0: Bt878 (rev 17) at :00:0b.0, irq: 19, latency: 32, mmio : 0xf780 [ 3013.737762] bttv0: using: Leadtek WinFast 2000/ WinFast 2000 XP [card=34,insm od option] [ 3013.737825] bttv0: gpio: en=, out= in=003ff502 [init] [ 3148.136017] bttv0: tuner type=24 [ 3148.136029] bttv0: i2c: checking for MSP34xx @ 0x80... not found [ 3154.536019] bttv0: i2c: checking for TDA9875 @ 0xb0... not found [ 3160.936018] bttv0: i2c: checking for TDA7432 @ 0x8a... not found [ 3167.351398] bttv0: registered device video0 [ 3167.351434] bttv0: registered device vbi0 [ 3167.351463] bttv0: registered device radio0 [ 3167.351485] bttv0: PLL: 28636363 = 35468950 . ok [ 3167.364182] input: bttv IR (card=34) as /class/input/input6 Please help! Regards Halim -- Halim Sahin E-Mail: halim.sahin (at) t-online.de -- 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 -- 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: [PULL] generic image bounds setting and alignment function
On Wed, 17 Jun 2009, Guennadi Liakhovetski wrote: On Tue, 16 Jun 2009, Trent Piepho wrote: On Tue, 16 Jun 2009, Guennadi Liakhovetski wrote: 01/14: compat: handle __fls http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=c4b55ce6c273 02/14: v4l2: Create helper function for bounding and aligning images http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=b4d3ec8d363d I am sorry, I will not bother with saving, reformatting, pasting... Just wanted to ask about this place: I guess you do not use Mercurial like all other v4l-dvb developers? I do use hg, though not for development, but for interacting with all other v4l-dvb developers Because you are making a big deal about a simple operation than can be done with a few keystrokes. If I wanted this patch quoted in my editor, I can simply type: !!cd ~/v4l-dvb ; hg export b4d3ec8d363d | sed 's/^/ /' No, typing this is not a big deal, as you say. But I do not understand _wny_ everyone, wishing to review / comment on your patches has to do that. And another problem of your approach you confirm yourself in this post: Using pull requests is something all v4l developers, besides you, do as well. No one, besides you, seems to find it a problem. It's been this way for years. I'm not the one who came up with this system. Sometimes one needs to go the mountain instead of expecting the mountain to come to oneself. See? Now hg will have two patches, which Mauro will then have to merge into one when exporting to git, and which then will be very difficult to Oh well. It's happened plenty of times before. I try not to make a habit of it. One can find many many patches in the linux git tree that have bugs in them. Often there are patches that fix these bugs included in the same upstream merge. IOW, the bug was found and fixed before the patch was merged upstream but the fix wasn't folded into the original patch, because the original patch was already in git and someone didn't want to do a git-rebase. One advantage of the hg tree is we get an extra opportunity to fix these things before sending them to git. everyone gets to see and review your patches only when they are already in your external repository ready to be pulled by the maintainer. s/in your/posted in the/; s/repository/mailing list/; s/pulled/applied/; everyone gets to see and review your patches only when they are already posted in the external mailing list ready to be applied by the maintainer. Is that really any different? -- 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: [PATCHv7 2/9] v4l2: video device: Add V4L2_CTRL_CLASS_FM_TX controls
On Tue, 16 Jun 2009, Eduardo Valentin wrote: On Sun, Jun 14, 2009 at 06:59:13PM +0200, ext Hans Verkuil wrote: On Sunday 14 June 2009 18:23:41 Trent Piepho wrote: similar V4L2_CID_MPEG_EMPHASIS control and others might well appear in the future, so I think this name should be more specific to the FM_TX API. The cx88 driver could get support for setting the fm preemphasis via a control. I added support via a module option, but a control would be better. You're saying it shouldn't use this fm preemphasis control? Correct. This set the pre-emphasis when transmitting. For receiving you want a separate control. Although the enum should be made generic. So FM_TX can be removed from the enum. Why should we have one rx and one tx control for this? Because you can have both receivers and transmitters in one device and you want independent control of the two. Yes, agreed here. There is the possibility to have receiver and transmitter both in the same device. So, I think it is better to have separated controls. Is both a receiver and transmitter in the same device different than having two receivers or two transmitters? In which case, since controls are not assigned to a specific input, how does one handle that? -- 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: [PULL] http://kernellabs.com/hg/~mkrufky/k2c2
On Tue, 16 Jun 2009, Mauro Carvalho Chehab wrote:
Em Tue, 16 Jun 2009 11:19:29 -0400
Michael Krufky mkru...@linuxtv.org escreveu:
+static int cx23885_dvb_set_frontend(struct dvb_frontend *fe,
+ struct dvb_frontend_parameters *param)
You could make this an HVR1275 specific function and then do away with the
first case statement. With a name like cx23885_dvb_set_frontend() it
appears that all boards will use it, when it is only ever called with an
HVR1275.
+ switch (param-u.vsb.modulation) {
+ case VSB_8:
+ cx23885_gpio_clear(dev, GPIO_5);
+ break;
+ case QAM_64:
+ case QAM_256:
+ default:
+ cx23885_gpio_set(dev, GPIO_5);
+ break;
Using the modulation to switch inputs is of course a hack, needed because
the dvb api lacks a concept of multiple inputs. I do not know if any still
exist, but there were cable systems which broadcast with both QAM and VS.
Argh! this looks like a hack! Don't you have a better approach for it?
--
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: [PULL] generic image bounds setting and alignment function
On Tue, 16 Jun 2009, Mauro Carvalho Chehab wrote:
Em Tue, 16 Jun 2009 17:57:20 +0200 (CEST)
Guennadi Liakhovetski g.liakhovet...@gmx.de escreveu:
On Sat, 30 May 2009, Trent Piepho wrote:
+ if (walign + halign salign) {
+ /* Max walign where there is still a valid width */
+ unsigned int wmaxa = __fls(wmax ^ (wmin - 1));
I cannot follow correctness of the above, sorry. Take a simple example:
wmax=0x7f, wmin=7, wmaxa = __fls(0x7f ^ 6) = __fls(0x79) = 0. And the
comment says it's the maximum walign where there is still a valid width.
What am I missing?
+
+ /* up the smaller alignment until we have enough */
+ do {
+ if (walign = halign walign wmaxa) {
As I'm still cooking the patches, I prefer to postpone the align ones until we
are comfortable with them.
Trent,
Could you please take a look on the above comments
There is no bug with the wmaxa code, but there is a different bug
elsewhere.
Please pull from http://linuxtv.org/hg/~tap/fix
for the following changeset:
01/01: v4l2: Fix flaw in alignment code
http://linuxtv.org/hg/~tap/fix?cmd=changeset;node=4ef7fb102b6c
--
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: [PATCH] High resolution timer for cx88 remotes
On Tue, 16 Jun 2009, Mauro Carvalho Chehab wrote: Em Sat, 23 May 2009 14:06:01 +0200 AH andrzej.ha...@wp.pl escreveu: Patched driver seems to work on my system, with kernel 2.6.28. I have removed kernel checks for versions below 2.6.20 - they were because of API changes in scheduler. If you are going to break compatibility below 2.6.20 then you should add the driver to versions.txt -- 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: [PATCHv7 2/9] v4l2: video device: Add V4L2_CTRL_CLASS_FM_TX controls
On Sun, 14 Jun 2009, Eduardo Valentin wrote:
+/* FM Modulator class control IDs */
+#define V4L2_CID_FM_TX_CLASS_BASE (V4L2_CTRL_CLASS_FM_TX | 0x900)
+#define V4L2_CID_FM_TX_CLASS ? ? ? ? ? ? ? ? (V4L2_CTRL_CLASS_FM_TX | 1)
+
+#define V4L2_CID_RDS_ENABLED ? ? ? ? ? ? ? ? (V4L2_CID_FM_TX_CLASS_BASE +
1)
+#define V4L2_CID_RDS_PI ? ? ? ? ? ? ? ? ? ? ? ? ? ?
?(V4L2_CID_FM_TX_CLASS_BASE + 2)
+#define V4L2_CID_RDS_PTY ? ? ? ? ? ? ? ? ? ? (V4L2_CID_FM_TX_CLASS_BASE +
3)
+#define V4L2_CID_RDS_PS_NAME ? ? ? ? ? ? ? ? (V4L2_CID_FM_TX_CLASS_BASE +
4)
+#define V4L2_CID_RDS_RADIO_TEXT ? ? ? ? ? ? ? ? ? ?
?(V4L2_CID_FM_TX_CLASS_BASE + 5)
I think these RDS controls should be renamed to V4L2_CID_RDS_TX_. This makes
it clear that these controls relate to the RDS transmitter instead of a
receiver. I would not be surprised to see similar controls appear for an RDS
receiver in the future.
So there should there be different controls to set the same thing, one set
for tx and another for rx?
+#define V4L2_CID_PREEMPHASIS ? ? ? ? ? ? ? ? (V4L2_CID_FM_TX_CLASS_BASE +
17)
+enum v4l2_fm_tx_preemphasis {
+ ? ? V4L2_FM_TX_PREEMPHASIS_DISABLED ? ? ? ? = 0,
+ ? ? V4L2_FM_TX_PREEMPHASIS_50_uS ? ? ? ? ? ?= 1,
+ ? ? V4L2_FM_TX_PREEMPHASIS_75_uS ? ? ? ? ? ?= 2,
+};
I suggest renaming this to V4L2_CID_FM_TX_PREEMPHASIS. There is already a
similar V4L2_CID_MPEG_EMPHASIS control and others might well appear in the
future, so I think this name should be more specific to the FM_TX API.
The cx88 driver could get support for setting the fm preemphasis via a
control. I added support via a module option, but a control would be
better. You're saying it shouldn't use this fm preemphasis control?
--
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: [PATCH -next] v4l: expose function outside of ifdef/endif block
On Fri, 12 Jun 2009, Randy Dunlap wrote: From: Randy Dunlap randy.dun...@oracle.com Move v4l_bound_align_image() outside of an #ifdef CONFIG_I2C block so that it is always built. Fixes a build error: clamp_align() should be moved as well, since it's only used by v4l_bound_align_image(). I'm attaching an alternate version that fixes this. Labeled the endif too. drivers/media/video/v4l2-common.c |3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) --- linux-next-20090612.orig/drivers/media/video/v4l2-common.c +++ linux-next-20090612/drivers/media/video/v4l2-common.c @@ -915,6 +915,7 @@ const unsigned short *v4l2_i2c_tuner_add return NULL; } EXPORT_SYMBOL_GPL(v4l2_i2c_tuner_addrs); +#endif /* Clamp x to be between min and max, aligned to a multiple of 2^align. min * and max don't have to be aligned, but there must be at least one valid @@ -986,5 +987,3 @@ void v4l_bound_align_image(u32 *w, unsig } } EXPORT_SYMBOL_GPL(v4l_bound_align_image); - -#endif# HG changeset patch # User Trent Piepho xy...@speakeasy.org # Date 1244834958 25200 # Node ID 23bd6516eafcc06ffb590073e744c7e17382aef9 # Parent 01fd4e3bd1c0fb52cb15acbd22ca7f4857170e6e v4l2: Move bounding code outside I2C ifdef block From: Trent Piepho xy...@speakeasy.org Move v4l_bound_align_image() and clamp_align() outside of an ifdef block for I2C related code. Can cause an undefined reference to `v4l_bound_align_image' if I2C isn't enabled. Priority: high Signed-off-by: Trent Piepho xy...@speakeasy.org Reported-by: Randy Dunlap randy.dun...@oracle.com diff -r 01fd4e3bd1c0 -r 23bd6516eafc linux/drivers/media/video/v4l2-common.c --- a/linux/drivers/media/video/v4l2-common.c Thu Jun 11 15:31:22 2009 -0700 +++ b/linux/drivers/media/video/v4l2-common.c Fri Jun 12 12:29:18 2009 -0700 @@ -997,6 +997,8 @@ const unsigned short *v4l2_i2c_tuner_add } EXPORT_SYMBOL_GPL(v4l2_i2c_tuner_addrs); +#endif /* defined(CONFIG_I2C) */ + /* Clamp x to be between min and max, aligned to a multiple of 2^align. min * and max don't have to be aligned, but there must be at least one valid * value. E.g., min=17,max=31,align=4 is not allowed as there are no multiples @@ -1067,5 +1069,3 @@ void v4l_bound_align_image(u32 *w, unsig } } EXPORT_SYMBOL_GPL(v4l_bound_align_image); - -#endif
Re: PULL request - http://linuxtv.org/hg/~pb/v4l-dvb/
On Thu, 11 Jun 2009, Patrick Boettcher wrote: On Wed, 27 May 2009, Trent Piepho wrote: On Tue, 26 May 2009, Patrick Boettcher wrote: Does this patch to fix these problems look ok? In fact, everything looks correct in my eyes. I'll ask Mauro to pull any minute from now. I even have an explaination why the failing attach of the itd1000 was not errored out: when I did the development I was using a userspace proprietary driver to validate, for that I needed the demod to be attached... Thanks for cleaning up this mess. Now that that patch is done, here is the rest of the series with dvb-pll conversions. There is an additional patch to the flexcop card detecting code. The #if defined(CONFIG_...) stuff didn't take into account code being compiled into the kernel. Here's the series: 01/08: dvb-pll: Add Samsung TDTC9251DH0 DVB-T NIM http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=3d148fee550b 02/08: dvb-pll: Add support for Samsung TBDU18132 DVB-S NIM http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=5a8e32e94aa7 03/08: dvb-pll: Add support for Samsung TBMU24112 DVB-S NIM http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=c28394056a5a 04/08: dvb-pll: Add support for Alps TDEE4 DVB-C NIM http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=90f54e498f90 05/08: b2c2: fix frontends compiled into kernel http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=748c762fcf3e 06/08: b2c2: Use dvb-pll for AirStar DVB-T's tuner http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=fcacc65753f1 07/08: b2c2: Use dvb-pll for Skystar2 rev 2.3 and rev 2.6 http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=8ef37f1432e6 08/08: b2c2: Use dvb-pll for Cablestar2 http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=01fd4e3bd1c0 b2c2/flexcop-fe-tuner.c | 283 +++- frontends/dvb-pll.c | 75 frontends/dvb-pll.h |4 3 files changed, 172 insertions(+), 190 deletions(-) -- 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: S_FMT vs. S_CROP
On Wed, 10 Jun 2009, Hans Verkuil wrote: On Wednesday 10 June 2009 18:02:39 Guennadi Liakhovetski wrote: This question - how S_FMT and S_CROP affest image geometry - has been discussed at least twice before - that's only with my participation, don't know if and how often it has come up before. But the fact, that in two discussions we came up with different results seems to suggest, that this is not something trivially known by all except me. First time I asked this question in this thread http://www.mail-archive.com/linux-media@vger.kernel.org/msg00052.html and Mauro replied (see above thread for a complete reply): On Thu, 8 Jan 2009, Mauro Carvalho Chehab wrote: On Wed, 7 Jan 2009 10:14:31 +0100 (CET) Guennadi Liakhovetski g.liakhovet...@gmx.de wrote: [snip] For example on mt9t031 binning and skipping are used for that. Whereas CROP uses the current scaling configuration and selects a sub-window, so, once you've done S_FMT to 320x240, a crop request for 640x480 might well fail. I also understand this way. You cannot crop with a resolution bigger than what you've selected. (Let's call this statement M1:-)) If I read the spec correctly, in particular section 1.11.1, then cropping comes before scaling, so you can crop to 640x480 (S_CROP) and scale that to 320x240 (S_FMT). S_FMT scales the cropped rectangle. This is my understanding of how it's supposed to work as well. -- 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: [PULL] http://linuxtv.org/hg/~pinchartl/uvcvideo/
On Tue, 9 Jun 2009, Mauro Carvalho Chehab wrote: Em Tue, 9 Jun 2009 18:08:38 +0200 Hans Verkuil hverk...@xs4all.nl escreveu: Should I submit a patch that implement VIDIOC_S_JPEGCOMP support in the UVC driver and implement a JPEG compression quality control later, or would you prefer the driver not to implement VIDIOC_S_JPEGCOMP at all ? As there are existing applications using that ioctl a few users are pushing for VIDIOC_S_JPEGCOMP support. I prefer the later. Adding a new ioctl support just to deprecate it on the next kernel doesn't seem nice. Let's add directly the newer controls and add a patch marking this as deprecated. I'm not sure whether we can deprecate JPEGCOMP. It is used in too many places. Perhaps we should create a set of JPEG controls that match what is in v4l2_jpegcompression and convert all the drivers that use JPEGCOMP to these new controls. Then the v4l core can map S/G_JPEGCOMP ioctls to a set of control read/writes. I'm working on string control support, so that will allow us to handle the APP_data and COM_data fields. This seems to be the correct approach. Implement it as controls, and let video_ioctl2 to handle the calls to the legacy ioctls, while marking it as deprecate. One problem is that COM and APP segment actually have a string of bytes associated with them. Right now we only have boolean and interger controls. There is no way to set a control to the 32-bytes you want put into a APP segment. Now, if we added NUL terminated string and fixed length byte arrays as control types this could be done. I know that's something I've mentioned before. It would also be a step to allowing drivers to export metadata about captured images (exposured info, focus tracking, etc.) via a control-like interface. -- 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: [PATCH] bt8xx: Add support for the Conexant Fusion 878a / Twinhan VP 1025 DVB-S
On Sun, 7 Jun 2009, Michael Stapelberg wrote: Add fefe:0001 to the list of identifiers for the bt8xx driver. The chip is named Conexant Fusion 878a, the card is a Twinhan VP 1025 DVB-S PCI. Please commit the attached patch. You can remove Conexant Fusion from the board name. All the boards for that driver use that same chip. Just use Twinhan VP 1025 DVB-S. Don't you need to add it to the list in bttv-cards.c in order to use the card? -- 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: [PATCHv5 1 of 8] v4l2_subdev i2c: Add v4l2_i2c_new_subdev_board i2c helper function
On Sat, 6 Jun 2009, Hans Verkuil wrote:
On Saturday 06 June 2009 13:59:19 Hans Verkuil wrote:
I propose to change the API as follows:
#define V4L2_I2C_ADDRS(addr, addrs...) \
((const unsigned short []){ addr, ## addrs, I2C_CLIENT_END })
struct v4l2_subdev *v4l2_i2c_new_subdev(struct v4l2_device *v4l2_dev,
struct i2c_adapter *adapter,
const char *module_name, const char *client_type,
u8 addr, const unsigned short *addrs);
struct v4l2_subdev *v4l2_i2c_new_subdev_cfg(struct v4l2_device *v4l2_dev,
struct i2c_adapter *adapter,
const char *module_name, const char *client_type,
int irq, void *platform_data,
u8 addr, const unsigned short *addrs);
/* Only available for kernels = 2.6.26 */
struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device
*v4l2_dev, struct i2c_adapter *adapter, const char *module_name, struct
i2c_board_info *info, const unsigned short *addrs);
Maybe addrs could be changed to something like probed_addrs so it's
easier to tell that the two address fields are used differently?
Is v4l2_i2c_new_subdev() in effect just a wrapper around
v4l2_i2c_new_subdev_cfg() that calls it with NO_IRQ and NULL for irq and
platform_data?
And could v4l2_i2c_new_subdev_cfg() be done like this?
struct v4l2_subdev *v4l2_i2c_new_subdev_cfg(struct v4l2_device *v4l2_dev,
struct i2c_adapter *adapter, const char *module_name,
const char *client_type, int irq, void *platform_data,
u8 addr, const unsigned short *addrs)
{
struct i2c_board_info info = {
.addr = addr, .platform_data = platform_data, .irq = irq, };
strlcpy(info.type, client_type, sizeof(info.type));
return v4l2_i2c_new_subdev_board(v4l2_dev, adapter, module_name,
info, addrs);
}
--
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: proposal for new i2c.h macro to initialize i2c address lists on the fly
On Sat, 6 Jun 2009, Hans Verkuil wrote:
For video4linux we sometimes need to probe for a single i2c address.
Normally you would do it like this:
static const unsigned short addrs[] = {
addr, I2C_CLIENT_END
};
client = i2c_new_probed_device(adapter, info, addrs);
This is a bit awkward and I came up with this macro:
#define V4L2_I2C_ADDRS(addr, addrs...) \
((const unsigned short []){ addr, ## addrs, I2C_CLIENT_END })
This can construct a list of one or more i2c addresses on the fly. But this
is something that really belongs in i2c.h, renamed to I2C_ADDRS.
With this macro we can just do:
client = i2c_new_probed_device(adapter, info, I2C_ADDRS(addr));
Comments?
I agree it's better placed with i2c.h. It's also possible to come up with
a more syntax more variadic function like, which can be called like this:
i2c_new_probed_device(adapter, info, addr1);
i2c_new_probed_device(adapter, info, addr1, addr2);
Using a macro like this...
#define i2c_new_probed_device(adap,info,addr,addrs...) ({ \
const unsigned short _addrs[] = {addr, ## addrs, I2C_CLIENT_END }; \
_i2c_new_probed_device(adap, info, _addrs); })
--
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: [PULL] generic image bounds setting and alignment function
On Mon, 1 Jun 2009, Robert Jarzmik wrote:
Trent Piepho xy...@speakeasy.org writes:
Please pull from http://linuxtv.org/hg/~tap/v4l-dvb
This series adds a function for bounding and alignment image sizes and
modifies a number of drivers to use it. It came up when the pxa patches to
deal with the alignment issues for that driver were posted. I haven't
tested these patches for pxa.
Hi Trent,
I didn't see the review of that serie, I'm curious what others said.
As for my comments, I'll inline your code, sorry about that.
02/14: v4l2: Create helper function for bounding and aligning images
http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=b4d3ec8d363d
+static unsigned int clamp_align(unsigned int x, unsigned int min,
+ unsigned int max, unsigned int align)
+{
+ /* Bits that must be zero to be aligned */
+ unsigned int mask = ~((1 align) - 1);
+
+ /* Round to nearest aligned value */
+ if (align)
+ x = (x + (1 (align - 1))) mask;
If I'm not mistaken, these lines are an equivalent of :
balign = 1 align;
if (align)
x = ALIGN(x + 1 - balign/2, balign);
Isn't that simpler to read ?
I don't think so, it's not obvious that it will round to the nearest value.
You have to look up ALIGN and then __ALIGN_MASK and see that it will in
effect add balign - 1 and then reduce x + 1 - balign/2 + balign - 1 into x
+ balign/2.
It also generates worse code. You'd think the compiled would be able to
optimize it into the same code, but it doesn't. Unless there is some issue
with how it will work with negative values that causes a subtle difference.
--
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: webcam drivers and V4L2_MEMORY_USERPTR support
On Mon, 1 Jun 2009, Stefan Kost wrote: I have implemented support for V4L2_MEMORY_USERPTR buffers in gstreamers v4l2src [1]. This allows to request shared memory buffers from xvideo, capture into those and therefore save a memcpy. This works great with the v4l2 driver on our embedded device. When I was testing this on my desktop, I noticed that almost no driver seems to support it. I tested zc0301 and uvcvideo, but also grepped the kernel driver sources. It seems that gspca might support it, but I ave not confirmed it. Is there a technical reason for it, or is it simply not implemented? userptr support is relatively new and so it has less support, especially with driver that pre-date it. Maybe USB cams use a compressed format and so userptr with xvideo would not work anyway since xv won't support the camera's native format. It certainly could be done for bt8xx, cx88, saa7134, etc. -- 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: [PATCH] Leadtek WinFast DTV-1800H support
On Sun, 31 May 2009, Miroslav [UTF-8] ??ustek wrote: Trent Piepho xyzzy at speakeasy.org writes: Instead of raising the reset line here, why not change the gpio settings in the card definition to have it high? Change gpio1 for television to 0x7050 and radio to 0x7010. Personally, I don't know when these .gpioX members are used (before firmware loads or after...). But I assume that adding the high on reset pin shouldn't break anything, so we can do this. They are used whenever the video mux is set. That will happen when changing inputs and when the cx8800 driver first initializes the card. Opening the radio device does an implicit change to the radio input. It looks like firmware is loaded when needed as part of setting the tuner frequency. I think that makes it safe to assume that the gpios will be set before firmware is loaded. Though it might be possible if the cx8802 driver is loaded before the cx8800 driver. Since you have the hardware, it would be easy to check. Add printk's in the reset callback code you wrote so you'll know when it's called. Then set video_debug to 1 and look for video_mux: lines, which indicate the card gpio values are being set. It seems clear that the xc2028 has an active low reset line. To reset, the line must be lowered for some time period (probably very short) and then raised to enable the chip, at which point there should be a delay (probably longer) while waiting for the chip to come out of reset. If you think about it, it does not matter what state the reset line is in before we lower it. It can be high, it can be low, the chip be reset either way. And shouldn't we put tuner reset pin to 0 when in composite and s-video mode? These inputs don't use tuner or do they? It should be unnecessary to do that, but might help with power consumption. To do it, change the composite and s-video gpio1 values to 0x7060. If I look in dmesg I can see that firmware is loaded into tuner even when these modes are used (I'm using MPlayer to select the input). And due to callbacks issued duting firmware loading, tuner is turned on (reset pin = 1) no matter if it was turned off by .gpioX setting. It seems like firmware loaded should only happen on frequency change. And shouldn't we use the mask bits [24:16] of MO_GPX_IO in .gpioX members too? I know only few GPIO pins and the other I don't know either what direction they should be. That means GPIO pins which I don't know are set as Hi-Z = inputs... Now, when I think of that, if it works it's safer when the other pins are in Hi-Z mode. Never mind. Normally all the unused gpio lines are just set as inputs. Then the reset can be done with: case XC2028_TUNER_RESET: /* GPIO 12 (xc3028 tuner reset) */ cx_write(MO_GP1_IO, 0x101000); mdelay(50); cx_write(MO_GP1_IO, 0x101010); mdelay(50); return 0; Earlier I was told to use 'cx_set' and 'cx_clear' because using 'cx_write' is risky. see here: http://www.spinics.net/lists/linux-dvb/msg29777.html Steven is wrong there and you are right. The cx88 gpio lines have the mask bits in the 3rd byte, which allow changing a gpio without modifying the others with a single write. It is better to use this than to do a read-modify-write. That is actually less safe, since it's not an atomic operation. And when you are using 'cx_set' and 'cx_clear' you need 3 calls. The first to set the direction bit, the second to set 0 on reset pin and the third to set 1 on reset pin. You could use cx_andor() to set the direction bit and lower the reset pin in one call. cx_set/cx_clear are just calls to cx_andor(). But using the mask bit and cx_write is best. Though I have to wonder why each card needs its own xc2028 reset function. Shouldn't they all be the same other than what gpio they change? My English goes lame here. Do you mean that reset function shouldn't use GPIO at all? There is other code for xc2028 reset for different cards, e.g. cx88_dvico_xc2028_callback, cx88_xc3028_geniatech_tuner_callback, cx88_xc2028_tuner_callback, cx88_pv_8000gt_callback, and even cx88_xc5000_tuner_callback. Shouldn't these functions all do the same thing other than what gpio line they change? @@ -2882,6 +2946,16 @@ cx_set(MO_GP0_IO, 0x0080); /* 702 out of reset */ udelay(1000); break; + + case CX88_BOARD_WINFAST_DTV1800H: + /* GPIO 12 (xc3028 tuner reset) */ + cx_set(MO_GP1_IO, 0x1010); + mdelay(50); + cx_clear(MO_GP1_IO, 0x10); + mdelay(50); + cx_set(MO_GP1_IO, 0x10); + mdelay(50); + break; } } Couldn't you replace this with: case CX88_BOARD_WINFAST_DTV1800H: cx88_xc3028_winfast1800h_callback(code, XC2028_TUNER_RESET, 0); break; Yes
Re: [PATCH] bttv-driver.c :poll method lose race condition for capture video
On Sun, 31 May 2009, Figo.zhang wrote:
bttv-driver.c,cx23885-video.c,cx88-video.c: poll method lose race condition
for capture video.
Please use patch titles that are not so long. It would be nice if you
could describe this race condition.
Signed-off-by: Figo.zhang figo1...@gmail.com
---
drivers/media/video/bt8xx/bttv-driver.c |7 +--
drivers/media/video/cx23885/cx23885-video.c | 15 +++
drivers/media/video/cx88/cx88-video.c | 13 ++---
3 files changed, 26 insertions(+), 9 deletions(-)
diff --git a/drivers/media/video/bt8xx/bttv-driver.c
b/drivers/media/video/bt8xx/bttv-driver.c
index 23b7499..9cadfcc 100644
--- a/drivers/media/video/bt8xx/bttv-driver.c
+++ b/drivers/media/video/bt8xx/bttv-driver.c
@@ -3152,6 +3152,7 @@ static unsigned int bttv_poll(struct file *file,
poll_table *wait)
struct bttv_fh *fh = file-private_data;
struct bttv_buffer *buf;
enum v4l2_field field;
+ unsigned int rc = 0;
if (V4L2_BUF_TYPE_VBI_CAPTURE == fh-type) {
if (!check_alloc_btres(fh-btv,fh,RESOURCE_VBI))
@@ -3160,6 +3161,7 @@ static unsigned int bttv_poll(struct file *file,
poll_table *wait)
}
if (check_btres(fh,RESOURCE_VIDEO_STREAM)) {
+ mutex_lock(fh-cap.vb_lock);
/* streaming capture */
if (list_empty(fh-cap.stream))
return POLLERR;
Won't you return with the mutex held here?
@@ -3191,8 +3193,9 @@ static unsigned int bttv_poll(struct file *file,
poll_table *wait)
poll_wait(file, buf-vb.done, wait);
if (buf-vb.state == VIDEOBUF_DONE ||
buf-vb.state == VIDEOBUF_ERROR)
- return POLLIN|POLLRDNORM;
- return 0;
+ rc = POLLIN|POLLRDNORM;
+ mutex_unlock(fh-cap.vb_lock);
+ return rc;
err:
mutex_unlock(fh-cap.vb_lock);
return POLLERR;
I think the logic for these changes will be cleaner to do something like this:
{
unsigned int rc = POLLERR;
if (some error);
goto done;
if (other error)
goto done;
if (buf-vb.state == VIDEOBUF_DONE ||
buf-vb.state == VIDEOBUF_ERROR)
rc = POLLIN|POLLRDNORM;
else
rc = 0;
done:
mutex_unlock(fh-vidq.vb_lock);
return rc;
}
--
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
[PULL] generic image bounds setting and alignment function
Mauro, Please pull from http://linuxtv.org/hg/~tap/v4l-dvb This series adds a function for bounding and alignment image sizes and modifies a number of drivers to use it. It came up when the pxa patches to deal with the alignment issues for that driver were posted. I haven't tested these patches for pxa. for the following 14 changesets: 01/14: compat: handle __fls http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=c4b55ce6c273 02/14: v4l2: Create helper function for bounding and aligning images http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=b4d3ec8d363d 03/14: pxa-camera: Use v4l bounding/alignment function http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=cb48209c1841 04/14: sh_mobile_ceu_camera: Use v4l bounding/alignment function http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=08dc3db16e9a 05/14: zoran: Use v4l bounding/alignment functiob http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=d65d274ca9da 06/14: vivi: Use v4l bounding/alignment function http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=67a3342606c2 07/14: saa7134: Use v4l bounding/alignment function http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=8e79122888da 08/14: cx88: Use v4l bounding/alignment function http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=7dc2db9c0b34 09/14: w8968cf: Use v4l bounding/alignment function http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=1733bbc12c3a 10/14: cx23885: Use v4l bounding/alignment function http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=605ec680bd75 11/14: mt9: Use v4l bounding/alignment function http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=d92d47e0d76f 12/14: cx231xx: Use v4l bounding/alignment function http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=3da0c824a487 13/14: em28xx: Use v4l bounding/alignment function http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=42b042c376ec 14/14: cx231xx: TRY_FMT should not actually set anything http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=81ca6b823ec4 linux/drivers/media/video/cx231xx/cx231xx-avcore.c | 17 +--- linux/drivers/media/video/cx231xx/cx231xx-video.c | 26 +-- linux/drivers/media/video/cx231xx/cx231xx.h|3 linux/drivers/media/video/cx23885/cx23885-video.c | 11 --- linux/drivers/media/video/cx88/cx88-video.c| 11 --- linux/drivers/media/video/em28xx/em28xx-video.c| 38 +++--- linux/drivers/media/video/mt9m001.c| 12 --- linux/drivers/media/video/mt9t031.c| 14 linux/drivers/media/video/mt9v022.c| 12 --- linux/drivers/media/video/pxa_camera.c | 36 ++ linux/drivers/media/video/saa7134/saa7134-video.c | 11 --- linux/drivers/media/video/sh_mobile_ceu_camera.c | 12 --- linux/drivers/media/video/v4l2-common.c| 73 - linux/drivers/media/video/vivi.c | 11 --- linux/drivers/media/video/w9968cf.c| 39 --- linux/drivers/media/video/zoran/zoran_driver.c | 14 +--- linux/include/media/v4l2-common.h | 10 ++ v4l/compat.h | 15 18 files changed, 165 insertions(+), 200 deletions(-) Thanks, Trent -- 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: PULL request - http://linuxtv.org/hg/~pb/v4l-dvb/
On Mon, 25 May 2009, Mauro Carvalho Chehab wrote: + if (params-frequency = 4800 params-frequency = 15400) \ + bs = 0x09; + if (params-frequency = 16100 params-frequency = 43900) \ + bs = 0x0a; + if (params-frequency = 44700 params-frequency = 86300) \ + bs = 0x08; Just remove the backslash. You don't need they. The original code has this, but bs will be zero for a frequency between 154 MHz to 161 Mhz as welll as 439-447 MHz. Probably wrong. My guess is the data sheet says, low band 48 to 154 MHz, mid band 161 MHz to 439 MHz, etc., where 154 is the frequency of the last channel in the low band and 161 is the first channel in the mid band. Then someone translated this to code without really understanding what's going on. It should probably be: if (params-frequency 44300) bs = 0x08; else if (params-frequency 15750) bs = 0x0a; elsebs = 0x09; The tuner's limits should already be enforced elsewhere. Or just convert this to use dvb_pll. -- 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,PATCH] VIDIOC_G_EXT_CTRLS does not handle NULL pointer correctly
On Mon, 25 May 2009, Laurent Pinchart wrote:
diff -r e0d881b21bc9 linux/drivers/media/video/v4l2-ioctl.c
--- a/linux/drivers/media/video/v4l2-ioctl.c Tue May 19 15:12:17 2009 +0200
+++ b/linux/drivers/media/video/v4l2-ioctl.c Sun May 24 18:26:29 2009 +0200
@@ -402,6 +402,10 @@
a specific control that caused it. */
p-error_idx = p-count;
user_ptr = (void __user *)p-controls;
+ if (p-count KMALLOC_MAX_SIZE / sizeof(p-controls[0])) {
+ err = -ENOMEM;
+ goto out_ext_ctrl;
+ }
if (p-count) {
ctrls_size = sizeof(struct v4l2_ext_control) * p-count;
/* Note: v4l2_ext_controls fits in sbuf[] so mbuf is
still NULL. */
@@ -1859,6 +1863,10 @@
a specific control that caused it. */
p-error_idx = p-count;
user_ptr = (void __user *)p-controls;
+ if (p-count KMALLOC_MAX_SIZE / sizeof(p-controls[0])) {
+ err = -ENOMEM;
+ goto out_ext_ctrl;
+ }
if (p-count) {
ctrls_size = sizeof(struct v4l2_ext_control) * p-count;
/* Note: v4l2_ext_controls fits in sbuf[] so mbuf is
still NULL. */
Restricting v4l2_ext_controls::count to values smaller than KMALLOC_MAX_SIZE /
sizeof(struct v4l2_ext_control) should be enough, but we might want to
restrict the value even further. I'd like opinions on this.
One thing that could be done is to call access_ok() on the range before
kmalloc'ing a buffer. If p-count is too high, then it's possible that the
copy_from_user will fail because the process does not have the address
space to copy.
--
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: [PATCH 1/5 - part 2] V4L2 patches for Intel Moorestown Camera Imaging Drivers
On Sat, 23 May 2009, Mauro Carvalho Chehab wrote:
+ + if (intel-open) { + ++intel-open; + DBG_DD((device has opened
already - %d\n, intel-open)); + return 0; + } + + file-private_data
= dev; + /* increment our usage count for the driver */ +
++intel-open; + DBG_DD((intel_open is %d\n, intel-open)); +
Open is not the proper place to clean the configuration, since a V4L2
device should support more than one open. You can use a different
userspace app to control your device, while other application is
streaming it.
It looks like only the first open will set the configuration, subsequent
ones don't do anything. So maybe this driver is ok for multiple opens?
Doesn't the kernel make open and close atomic, so some kind of barrier or
atomic variable isn't necessary?
+static int intel_g_fmt_cap(struct file *file, void *priv,
+ struct v4l2_format *f)
+{
+ struct video_device *dev = video_devdata(file);
+ struct intel_isp_device *intel = video_get_drvdata(dev);
+ int ret;
+
+ DBG_DD((intel_g_fmt_cap\n));
+ if (f-type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
Once switched to video-ioctl2, it don't be necessary to check the type of
buffer. vidioc_g_fmt_cap will only be called with video capture buffers.
It's the same with all the other vidioc_*_cap methods.
+static int intel_s_input(struct file *file, void *priv, unsigned int i)
+{
+ return 0;
+}
Wouldn't it be better to return an error if the input is something other
than zero?
+ snrcfg = intel-sys_conf.isi_config;
+ index = f-index;
+
+ if (f-type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
+ ret = -EINVAL;
+ else {
+ if (snrcfg-type == SENSOR_TYPE_SOC)
+ if (index = 8)
+ return -EINVAL;
+ if (index = sizeof(fmts) / sizeof(*fmts))
+ return -EINVAL;
+ memset(f, 0, sizeof(*f));
+ f-index = index;
Saving index, clearing f, and restoring index isn't necessary.
video-ioctl2 will take care of clearing everything that needs to be
cleared.
+ f-type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
Not necessary either, you already know type is set correctly.
+ if (buf-memory != V4L2_MEMORY_MMAP ||
+ buf-type != V4L2_BUF_TYPE_VIDEO_CAPTURE ||
+ buf-index = intel-num_frames || buf-index 0)
+ return -EINVAL;
You don't need to check buf-type, it will be a type your driver supports.
buf-index is unsigned, obviously it can't be less than zero. The same
applies to all the other buffer functions. Looks like you copied from old
code. Some drivers had these unnecessary checks but they should have all
been cleaned up by now.
+ pci_read_config_word(dev, PCI_VENDOR_ID, intel-vendorID);
+ pci_read_config_word(dev, PCI_DEVICE_ID, intel-deviceID);
Do you ever use these after saving them?
--
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: Sound capture with Osprey 230
On Mon, 11 May 2009, Sverker Abrahamsson wrote: Hi all, I've been using Osprey 230 cards for AV capture for several years, earlier with a modified version of Viewcast's driver but it was never very stable. When doing a new setup I therefore wanted to get the Alsa driver to work. I found that there were two trees in the repository in regards to these cards, http://linuxtv.org/hg/~mchehab/osprey and http://linuxtv.org/hg/~tap/osprey. It seems that mchehab tree is the patches that Viewcast submitted which does not address the necessary changes for ALSA driver while tap tree does but for Osprey 440 and older kernels. Mauro's tree with viewcast's patches is even older than mine, wrt kernel support. I've therefore ported the changes from tap to the main tree and added support for detecting Osprey 210/220/230 plus a minor fix to support specifying digital_rate as module parameter. It might also work for Osprey 240 (which is PCI-e variant of 230) but I don't have any such card so I haven't been able to test. Instead of modifying my patch, it would be better if you could provide a patch on top of it that adds support for your new card. The only question mark I have is that the current implementation use the depreciated interfaces from bttv-if.c to find which bttv driver corresponds to this audio driver and adds a function to get the bttv core. It is suggested to use the routines in bttv-gpio.c instead but I don't find an obvious replacement for bttv_get_pcidev nor how to get bttv_core. The interface in bttv-if.c has been deprecated for years now, yet no one has come up with something to replace it with. I think Gerd was getting a bit ahead of himself when he declared it obsolete. I see two alternatives: 1. Implement snd-87x module as a subdevice to bttv. Is this correct as the video and audio devices are two separate pci devices? The audio and video devices aren't just separate pci devices, they are also two unrelated devices to the linux device model. The driver model doesn't have any means to call one a subdevice of the other. Somehow, there needs to be a means for the audio driver to find the video driver so that it can get access to the gpio lines and the i2c bus. But, this is only necessary for the osprey cards. The audio driver for other cards doesn't need gpios or i2c. So, it would be nice to allow just the audio driver with no video to be loaded. The problem with my implementation is that after the audio bttv driver gets a pointer to the video driver's core, the video driver could go away and leave the audio driver with a dangling pointer. That's one of the reasons I haven't merged my osprey code. The other is that I have cards with bttv audio to test with. -- 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: [PULL] http://endr...@linuxtv.org/hg/~endriss/v4l-dvb
On Wed, 13 May 2009, Oliver Endriss wrote: 02/05: dvb-ttpci: Check transport error indicator flag http://endr...@linuxtv.org/hg/~endriss/v4l-dvb?cmd=changeset;node=8a742338523d Are you sure this is a good idea? The cx88 driver doesn't do this. -- 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: [patch 4/4] zoran: fix /|| error
On Tue, 12 May 2009, Mauro Carvalho Chehab wrote: Em Tue, 12 May 2009 17:18:20 -0400 Devin Heitmueller dheitmuel...@kernellabs.com escreveu: On Tue, May 12, 2009 at 4:39 PM, a...@linux-foundation.org wrote: From: Roel Kluin roel.kl...@gmail.com Fix /|| typo. `default_norm' can be 0 (PAL), 1 (NTSC) or 2 (SECAM), the condition tested was impossible. Signed-off-by: Roel Kluin roel.kl...@gmail.com Cc: Mauro Carvalho Chehab mche...@redhat.com Cc: Hans Verkuil hverk...@xs4all.nl Signed-off-by: Andrew Morton a...@linux-foundation.org --- Hello, Was the patch actually tested against the hardware in question? While I agree that it looks ok, it can result in the default logic being inverted in some cases, which could expose other bugs and result in a regression. I just want to be confident that this patch was tested by somebody with the hardware and it isn't going into the codebase because it obviously cannot be right. Hans and Jean worked on it. Both are at PAL area, so they won't notice such error without a standards generator, since the default is to assume that the signal is PAL. With this patch, PAL should keep working, but I can't see how NTSC or SECAM would work without it. NTSC works fine without it. The code with the bug was supposed to check for an out of range module parameter and fix it, but it was broken and did nothing. There is no problem if default_norm was set to an ok value, but if someone specified default_norm=42 then the driver wouldn't fix it and something bad might happen. Maybe it would read off the end of the norms array and crash? -- 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: [PATCH] v4l2: fill the reserved fields of VIDIOC_REQBUFS ioctl
On Wed, 29 Apr 2009, [UTF-8] N??meth M??rton wrote: The parameter of VIDIOC_REQBUFS is a pointer to struct v4l2_requestbuffers. This structure has reserved fields which has to be filled with zeros according to the V4L2 API specification, revision 0.24 [1]. As I read the spec, the reserved fields can be used for input from user space if the buffer is of type V4L2_BUF_TYPE_PRIVATE or higher. -- 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: [PATCH] v4l2: fill the reserved fields of VIDIOC_REQBUFS ioctl
On Thu, 30 Apr 2009, Trent Piepho wrote:
On Wed, 29 Apr 2009, [UTF-8] N??meth M??rton wrote:
The parameter of VIDIOC_REQBUFS is a pointer to struct v4l2_requestbuffers.
This structure has reserved fields which has to be filled with zeros
according to the V4L2 API specification, revision 0.24 [1].
As I read the spec, the reserved fields can be used for input from user
space if the buffer is of type V4L2_BUF_TYPE_PRIVATE or higher.
Here is a patch that fixes this problem, along with the S_FMT problem you
patched earlier. TRY_FMT had the same problem as S_FMT, so I fixed that
one as well too.
v4l2-ioctl: Clear buffer type specific trailing fields/padding
From: Trent Piepho xy...@speakeasy.org
Some ioctls have structs that are a different size depending on what type
of buffer is being used. If the buffer type leaves a field unused or has
padding space at the end, this space should be zeroed out.
The problems with S_FMT and REQBUFS were original identified and patched by
M??rton N??meth nm...@freemail.hu.
Priority: normal
Signed-off-by: Trent Piepho xy...@speakeasy.org
diff -r 7b786cb576e5 -r 82ef5d6e29e3 linux/drivers/media/video/v4l2-ioctl.c
--- a/linux/drivers/media/video/v4l2-ioctl.cThu Apr 30 09:14:13 2009 -0700
+++ b/linux/drivers/media/video/v4l2-ioctl.cThu Apr 30 09:15:56 2009 -0700
@@ -42,6 +42,12 @@
if (vfd-debug V4L2_DEBUG_IOCTL_ARG) \
printk(KERN_DEBUG %s: fmt, vfd-name, ## arg);\
} while (0)
+
+/* Zero out the end of the struct pointed to by p. Everthing after, but
+ * not including, the specified field is cleared. */
+#define CLEAR_AFTER_FIELD(p, field) \
+ memset((u8 *)(p) + offsetof(typeof(*(p)), field) + sizeof((p)-field), \
+ 0, sizeof(*(p)) - offsetof(typeof(*(p)), field) - sizeof((p)-field))
struct std_descr {
v4l2_std_id std;
@@ -783,44 +789,53 @@ static long __video_do_ioctl(struct file
switch (f-type) {
case V4L2_BUF_TYPE_VIDEO_CAPTURE:
+ CLEAR_AFTER_FIELD(f, fmt.pix);
v4l_print_pix_fmt(vfd, f-fmt.pix);
if (ops-vidioc_s_fmt_vid_cap)
ret = ops-vidioc_s_fmt_vid_cap(file, fh, f);
break;
case V4L2_BUF_TYPE_VIDEO_OVERLAY:
+ CLEAR_AFTER_FIELD(f, fmt.win);
if (ops-vidioc_s_fmt_vid_overlay)
ret = ops-vidioc_s_fmt_vid_overlay(file,
fh, f);
break;
case V4L2_BUF_TYPE_VIDEO_OUTPUT:
+ CLEAR_AFTER_FIELD(f, fmt.pix);
v4l_print_pix_fmt(vfd, f-fmt.pix);
if (ops-vidioc_s_fmt_vid_out)
ret = ops-vidioc_s_fmt_vid_out(file, fh, f);
break;
case V4L2_BUF_TYPE_VIDEO_OUTPUT_OVERLAY:
+ CLEAR_AFTER_FIELD(f, fmt.win);
if (ops-vidioc_s_fmt_vid_out_overlay)
ret = ops-vidioc_s_fmt_vid_out_overlay(file,
fh, f);
break;
case V4L2_BUF_TYPE_VBI_CAPTURE:
+ CLEAR_AFTER_FIELD(f, fmt.vbi);
if (ops-vidioc_s_fmt_vbi_cap)
ret = ops-vidioc_s_fmt_vbi_cap(file, fh, f);
break;
case V4L2_BUF_TYPE_VBI_OUTPUT:
+ CLEAR_AFTER_FIELD(f, fmt.vbi);
if (ops-vidioc_s_fmt_vbi_out)
ret = ops-vidioc_s_fmt_vbi_out(file, fh, f);
break;
case V4L2_BUF_TYPE_SLICED_VBI_CAPTURE:
+ CLEAR_AFTER_FIELD(f, fmt.sliced);
if (ops-vidioc_s_fmt_sliced_vbi_cap)
ret = ops-vidioc_s_fmt_sliced_vbi_cap(file,
fh, f);
break;
case V4L2_BUF_TYPE_SLICED_VBI_OUTPUT:
+ CLEAR_AFTER_FIELD(f, fmt.sliced);
if (ops-vidioc_s_fmt_sliced_vbi_out)
ret = ops-vidioc_s_fmt_sliced_vbi_out(file,
fh, f);
break;
case V4L2_BUF_TYPE_PRIVATE:
+ /* CLEAR_AFTER_FIELD(f, fmt.raw_data); - does nothing
*/
if (ops-vidioc_s_fmt_type_private)
ret = ops-vidioc_s_fmt_type_private(file,
fh, f);
@@ -837,46 +852,55 @@ static long __video_do_ioctl(struct file
[pull] http://linuxtv.org/hg/~tap/v4l-dvb
Mauro, Please pull from http://linuxtv.org/hg/~tap/v4l-dvb for the following 4 changesets: 01/04: compat: Add DMA_BIT_MASK() macro http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=486add0e3f1f 02/04: zoran: fix bug when enumerating format -1 http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=f144948f22c8 03/04: v4l2-ioctl: Check buffer types using g_fmt instead of try_fmt http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=b54857195e2c 04/04: v4l2-ioctl: Clear buffer type specific trailing fields/padding http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=7cbf08aeb840 linux/drivers/media/video/v4l2-ioctl.c | 45 - linux/drivers/media/video/zoran/zoran_driver.c | 30 +++- v4l/compat.h |7 ++- 3 files changed, 55 insertions(+), 27 deletions(-) Thanks, Trent -- 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: [PATCH] [0904_14] Siano: assemble all components to one kernel module
On Mon, 27 Apr 2009, Uri Shkolnik wrote: --- On Tue, 4/21/09, Mauro Carvalho Chehab mche...@infradead.org wrote: If the system includes SDIO and OMAP SPI/SPP, the module build will discard the USB interface driver, but the SDIO and the OMAP SPI will be built. The patch you've provided just merge everything. If you're proposing a newer model, you should send a patchset with the complete Kbuild refactor. For now, it is better to postpone this patch until we merge non-kbuild changes. Can you name another driver that works this way?? It is considered better to build a new module for a different interface.? That way one can decide at run time if the interface is needed or not and only load the module if needed.? If everything is built into one module then one must decide at compile time what interfaces to support.? But it is often the case that kernels are compiled on different systems than run them. This model also sounds different to me from what I've seen so far. 1) PLUTO Kconfig config DVB_PLUTO2 tristate Pluto2 cards depends on DVB_CORE PCI I2C As we can see, it depends on PCI and I2C (and DVB_CORE, but that is outside the current discussion), which means, that if the target system lacks either PCI or I2C (and many Linux target do not have those) the entire Pluto module will not be built, even if selected. I do not think you understand. This driver is for a device that is access via a PCI bus. It creates an i2c adapter that is used to control the tuner chip on the device. The device is not accessed via i2c. What's more, and this is a key point you are missing, it can not be accessed by PCI *or* I2C. This is the same with all the other drivers you may have found that use both PCI and I2C. It's a totally different situation. Look at the how the B2C2 flexcop driver works. This hardware is availble in both PCI and USB versions. config DVB_TTUSB_BUDGET tristate Technotrend/Hauppauge Nova-USB devices depends on DVB_CORE USB I2C PCI Support for external USB adapters designed by Technotrend and This module while not be build (even if chosen) on target system that lacks USB stack (additional to PCI and I2C). This looks like a mistake. The driver is for a USB device and should not depend on PCI, but the driver uses the pci dma api. First, SMS (Siano based device) requires at least one available bus (either USB, SDIO, SPI/SPP, HIF/Parallel, TS_I2C, ), not a combination like the TTUSB. (Note, if multiple buses available, the module can use them simultaneously, for example - two different instances (SDIO and USB) dongles may co-exist on the same system). There is no reason to prevent some systems which have only SDIO interface to work if there is no USB and vice versa. Only if there are no supported (by SMS) buses at all, the drive will not be built. Second, in order to reduce module size, there is no need to compile and link to the binary module a bus interface component that will never be used, more than that, if there are no suitable (external) headers, the build process will fail. So, the Kconfig and Makefile need to ensure that the module will be built and function OK on target systems which have full set AND subset of the total supported buses (by SMS). The point you are missing is that by compiling all support into one module you are forcing the supported busses to be chosen at compile time. If they were separate modules then the supported busses could be loaded at run time. -- 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: [Mjpeg-users] [PATCH] zoran: invalid test on unsigned
On Mon, 27 Apr 2009, Alan Cox wrote:
On Sun, 26 Apr 2009 17:45:07 +0200
Roel Kluin roel.kl...@gmail.com wrote:
fmt-index is unsigned. test doesn't work
Signed-off-by: Roel Kluin roel.kl...@gmail.com
---
Is there another test required?
+++ b/drivers/media/video/zoran/zoran_driver.c
@@ -1871,7 +1871,7 @@ static int zoran_enum_fmt(struct zoran *zr, struct
v4l2_fmtdesc *fmt, int flag)
if (num == fmt-index)
break;
}
- if (fmt-index 0 /* late, but not too late */ || i == NUM_FORMATS)
+ if (i == NUM_FORMATS)
return -EINVAL;
If it's unsigned then don't we need i = NUM_FORMATS ?
That part of the patch is fine. It turns out there is another problem that
already existed in this function. It's clearer with a few more lines of
context.
int num = -1, i;
for (i = 0; i NUM_FORMATS; i++) {
if (zoran_formats[i].flags flag)
num++;
if (num == fmt-index)
break;
}
if (i == NUM_FORMATS)
return -EINVAL;
/* stuff to return format i */
The v4l2 api enumerates formats separately for each buffer type, e.g. there
is one list of formats for video capture and a different list for video
output. The driver just has one list of formats and each format is flagged
with the type(s) it can be used with. So when someone requests the capture
format at index 2 we search the list and return the 3rd capture format we
find. Since we don't know how many capture formats there are (NUM_FORMATS
is the number of formats in the driver's single list, i.e. the union of all
format types) we can't reject an index that is too large until after
searching the whole list.
The problem with this code is if someone requests a format at fmt-index ==
(unsigned)-1. If the first format in the array doesn't have the requested
type then num will still be -1 when it's compared to fmt-index and there
will appear to be a match.
Here's a patch to redo the function that should fix everything:
zoran: fix bug when enumerating format -1
If someone requests a format at fmt-index == (unsigned)-1 and the first
format in the array doesn't have the requested type then num will still be
-1 when it's compared to fmt-index and there will appear to be a match.
Restructure the loop so this can't happen. It's simpler this way too. The
unnecessary check for (unsigned)fmt-index 0 found by Roel Kluin
roel.kl...@gmail.com is removed this way too.
Signed-off-by: Trent Piepho xy...@speakeasy.org
diff -r 63eba6df4b8a -r c247021eb11c
linux/drivers/media/video/zoran/zoran_driver.c
--- a/linux/drivers/media/video/zoran/zoran_driver.cMon Apr 27 12:13:04
2009 -0700
+++ b/linux/drivers/media/video/zoran/zoran_driver.cMon Apr 27 12:25:51
2009 -0700
@@ -1871,22 +1871,20 @@ static int zoran_querycap(struct file *f
static int zoran_enum_fmt(struct zoran *zr, struct v4l2_fmtdesc *fmt, int flag)
{
- int num = -1, i;
-
- for (i = 0; i NUM_FORMATS; i++) {
- if (zoran_formats[i].flags flag)
- num++;
- if (num == fmt-index)
- break;
- }
- if (fmt-index 0 /* late, but not too late */ || i == NUM_FORMATS)
- return -EINVAL;
-
- strncpy(fmt-description, zoran_formats[i].name,
sizeof(fmt-description)-1);
- fmt-pixelformat = zoran_formats[i].fourcc;
- if (zoran_formats[i].flags ZORAN_FORMAT_COMPRESSED)
- fmt-flags |= V4L2_FMT_FLAG_COMPRESSED;
- return 0;
+ unsigned int num, i;
+
+ for (num = i = 0; i NUM_FORMATS; i++) {
+ if (zoran_formats[i].flags flag num++ == fmt-index) {
+ strncpy(fmt-description, zoran_formats[i].name,
+ sizeof(fmt-description) - 1);
+ /* fmt struct pre-zeroed, so adding '\0' not neeed */
+ fmt-pixelformat = zoran_formats[i].fourcc;
+ if (zoran_formats[i].flags ZORAN_FORMAT_COMPRESSED)
+ fmt-flags |= V4L2_FMT_FLAG_COMPRESSED;
+ return 0;
+ }
+ }
+ return -EINVAL;
}
static int zoran_enum_fmt_vid_cap(struct file *file, void *__fh,
--
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: [PATCH] [0904_14] Siano: assemble all components to one kernel module
On Mon, 20 Apr 2009, Uri Shkolnik wrote: better to have the BUS configurable, e. g. just because you have USB interface, it doesn't mean that you want siano for USB, instead of using SDIO. Since the module is using dynamic registration, I don't find it a problem. When the system has both USB and SDIO buses, both USB and SDIO interface driver will be compiled and linked to the module. When a Siano based device (or multiple Siano devices) will be connected, they will be register internally in the core and activated. Any combination is allow (multiple SDIO, multiple USB and any mix). This is not the way linux drivers normally work. Usually there are multiple modules so that only the ones that need to be loaded are loaded. It sounds like you are designing this to be custom compiled for each system, but that's not usually they way things work. -- 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: patch: s2255drv high quality mode and video status querying
On Tue, 7 Apr 2009, Dean A. wrote:
+static int vidioc_g_parm(struct file *file, void *priv,
+ struct v4l2_streamparm *sp)
+{
+ struct s2255_fh *fh = priv;
+ struct s2255_dev *dev = fh-dev;
+ if (sp-type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
+ return -EINVAL;
You do not need to check the buffer type, video_ioctl2 does it already.
--
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: [PATCH] [0904_16] Siano: smsdvb - additional case of endian handling.
On Sun, 5 Apr 2009, Uri Shkolnik wrote: PidMsg.xMsgHeader.msgLength = sizeof(PidMsg); PidMsg.msgData[0] = feed-pid; - /* smsendian_handle_tx_message((struct SmsMsgHdr_ST *)PidMsg); */ + smsendian_handle_tx_message((struct SmsMsgHdr_ST *)PidMsg); return smsclient_sendrequest(client-smsclient, PidMsg, sizeof(PidMsg)); I don't get it, you wrote the code commented out in one patch, and now you're going to sumbit patches to uncomment it one line at a time? -- 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: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model
On Sun, 5 Apr 2009, Andy Walls wrote: Here is where LIRC may be its own worst enemy. LIRC has filled some shortcomings in the kernel for support of IR device functions for so long (LWN says LIRC is 10 years old), that large numbers of users have come to depend on its operation, while at the same time apparently removing impetus for doing much to update the in kernel IR device support. More than that. In 1997 I bought a serial port remote off ebay and tried to get it to work with linux. I found an abandoned project from the Metzler brothers called LIRC, though it didn't work. I wrote a new protocol for the serial port driver, which was the only one at the time, rewrote the remote pulse decoding code and came up a new socket based the client/server protocol and wrote the x-event client. At that point remotes were defined in header files so make was still needed to add a new one. -- 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: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model
On Sun, 5 Apr 2009, Mike Isely wrote: 1. The switch statement in ir-kbd-i2c.c:ir_attach() is apparently implicitly trying to assume a particular type of remote based on the I2C address of the IR receiver it's talking to. Yuck. That's really not right at all. The IR receiver used does not automatically mean which remote is used. What if the vendor switches remotes? That's happened with the PVR-USB2 hardware in the past (based on photos I've seen). Who's to say the next remote to be supplied is compatible? IMHO, the way the remote supported is compiled into the kernel is absurd. The system I setup 12 years ago was better than this. At least the remote was compiled into a userspace daemon and multiple remotes were supported at the same time. The keycode system I used had a remote id/key id split, so you could have volume up key on any remote control the mixer but make the power buttons on different remotes turn on different apps. 3. A given IR remote may be described by much more than what 'scan codes' it produces. I don't know a lot about IR, but looking at the typical lirc definition for a remote, there's obvious timing and protocol parameters as well. Just being able to swap scan codes around is not always going to be enough. A remote typically sends a header sequence of a long pulse and space before the code. The length of the pulse on the space varies greatly by remote and makes a good way to identify the remote when multiple ones are supported. Then a pulse coded remote sends a sequence bits, usually 8 to 32. The length of the pulse identifies 1s or 0s. Different remotes have different pulse lengths and different spaces between them. RC5 remotes use Manchester encoding for this part. When you hold a key down some remotes just repeat the same sequence over and over again. Some repeat the scan code but omit the header part. Some send out a special pulse sequence to indicate the last key is being held down. With the latter two methods you can tell the difference between a key being held down and a key being pressed repeatedly. With the first you have guess based on how fast the repeats are coming in. This is very different than a keyboard, which sends a code when you press a key and another when you release it. The rate at which remotes repeat varies greatly. You might find that one remote makes your volume change annoying slowly while another is much too fast to be usable. Remote keys usually start repeating without delay, so if you let a toggle like 'mute' repeat it becomes almost impossible to hit it just once. Entering numbers becomes impossible as well. -- 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: When is a wake_up() not a wake up?
On Fri, 3 Apr 2009, Andy Walls wrote:
1. A work queue thread or read() call needs to send a command to the
CX23418 using the cx18_api_call() function
2. It fills out a mailbox with a command for the CX23418
3. It prepares to wait, just in case a wait is needed
4. A SW1 interrupt is sent to the CX23418 telling it a mailbox is ready
5. The ack filed in the mailbox, a PCI MMIO location, is checked to see
if the mailbox was ack'ed already
6. If not, schedule_timeout() for up to 10 msecs (a near eternity...)
7. Clean up the wait and move on
10ms isn't that long. With a 100 HZ kernel it's only one jiffie. The
shortest time msleep() can sleep on a 100 HZ kernel is 20 ms.
Is it possible that it's simply taking more than 10 ms for your process to
get run?
Mar 31 23:36:56 palomino kernel: cx18-0: irq: sending interrupt SW1: 8 to
send CX18_CPU_DE_SET_MDL
Mar 31 23:36:56 palomino kernel: cx18-0: api: scheduling to wait for ack of
CX18_CPU_DE_SET_MDL: req 51267 ack 51266, pid 21092, state 2
Mar 31 23:36:56 palomino kernel: cx18-0: irq: received interrupts SW1: 0
SW2: 8 HW2: 0
Mar 31 23:36:56 palomino kernel: cx18-0: irq: Wake up initiated on pid 21092
in state 2
Mar 31 23:36:56 palomino kernel: cx18-0: irq: Wake up succeeded on pid
21092, state 2 - 0
Mar 31 23:36:56 palomino kernel: cx18-0: api: done wait for ack of
CX18_CPU_DE_SET_MDL: req 51267 ack 51267, current pid 21092, current state 0,
state 0
Mar 31 23:36:56 palomino kernel: cx18-0: warning: failed to be awakened upon
RPU acknowledgment sending CX18_CPU_DE_SET_MDL; timed out waiting 10 msecs
Try some higher resolution time stamps, you can't really tell much about
when things are happening.
Here's some code I wrote to do it on x86.
#include linux/calc64.h
#define MHZ 1666.787
#define INT (unsigned int)(MHZ * (112) + 0.5)
#define FRAC(unsigned int)(MHZ * (121) / 1000.0 + 0.5)
#define timestamp(str, args...) { u64 _time; u32 _rem; rdtscll(_time); \
_time = (_time - starttime)12; \
_rem = do_div(_time, INT); \
printk(%s - %u.%03u us str \n, __func__, \
(unsigned int)_time, (_rem 9) / FRAC , ## args); }
static u64 starttime;
Change MHZ to what /proc/cpuinfo tells you. Call 'rdtscll(starttime)' some
time when your driver inits or a device gets opened or something like that.
Makes the time stamps easier to read when they start near zero.
--
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: [linuxtv-commits] [hg:v4l-dvb] v4l2-ioctl: Check format for S_PARM and G_PARM
On Sun, 29 Mar 2009, Hans Verkuil wrote: On Sunday 29 March 2009 13:03:13 Trent Piepho wrote: How does overlay depend on video capture in any way? It's perfectly reasonable for a driver to support _only_ overlay and not video capture. The zr36067 chip is only designed to support uncompressed data for video overlay for example. Allowing uncompressed video capture is a hack that the driver didn't have at one point. Ah. Live and learn. In that case the spec is out of date and needs to be updated. Do you mean there is something in the spec that makes overlay depend on video capture? Or that s/g_parm don't mention buffer types other than video capture and video output? No, I agree with the spec in that I see no use case for it. Should there be one, then I'd like to see that in an actual driver implementation and in that case the spec should be adjusted as well. How about getting the frame rate of video overlay? Works with bttv. Hmm, I grepped only on s_parm, not on g_parm. It would be nice to use s_parm with drivers like bttv to reduce the frame rate. With multi-chip capture cards running out of bus bandwidth is a big problem. Reducing the frame rate is one way to deal with it. The bt8x8 and cx2388x chips do have a temporal decimation feature and I've tried to add support for it but I never got it to work correctly. In addition, g/s_parm is only used in combination with webcams/sensors for which overlays and vbi are irrelevant. There are several drivers for non-webcams, like bttv, saa7134, and saa7146, that support g_parm. Why is returning the frame rate for video capture not valid? Why does the number of buffers used for read() mode only make sense for webcams? OK, I'd forgot to check for the usage of g_parm. My bad. There is also a default g_parm handler in video_ioctl2 that might be used if the driver doesn't provide one. I don't know what drivers actually use it. Thinking about it now, I think what makes the most sense is to use capture for VIDEO_OVERLAY, VBI_CAPTURE, and SLICED_VBI_CAPTURE. And use output for VBI_OUTPUT, SLICED_VBI_OUTPUT and VIDEO_OUTPUT_OVERLAY. You're absolutely correct. I was too hasty. Can you update the spec to reflect this? I'll try, but I hear the doc build system is quite a challenge. I also wonder whether check_fmt shouldn't check for the presence of the s_fmt callbacks instead of try_fmt since try_fmt is an optional ioctl. I noticed that too. saa7146 doesn't have a try_fmt call for vbi_capture but is apparently supposed to support it. I sent a message about that earlier. I saw that. So why not check for s_fmt instead of try_fmt? That would solve this potential problem. Because that's clearly a change that belongs in another patch. OK, great. My patch just called check_fmt() for s/g_parm. I haven't touched check_fmt(). But you're right that try_fmt is optional so it makes a bad test. Maybe it should use g_fmt? saa7146 doesn't provide a s_fmt call either for vbi capture, only g_fmt. Though maybe this is a bug in saa7146? It seems like any driver that provides g_fmt can always just use that same method for s_fmt as well and be correct. -- 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: Problems with Hauppauge HVR 1600 and cx18 driver
On Sun, 29 Mar 2009, Andy Walls wrote:
On Sun, 2009-03-29 at 01:24 -0700, Trent Piepho wrote:
wait_event() should take care of this. wait_event(q, test) basically does:
for(;;) {
// point A
add_me_to_waitqueue(q);
set_current_state(TASK_UNINTERRUPTIBLE);
if (test)
break;
// point B
schedule();
}
clean_up_wait_stuff();
As you know, the condition is checked even before this loop is entered,
to avoid even being even added to a waitqueue. (Thank God for ctags...)
I think the initial check of the condition is just an optimization and
everything will still work without it. Seeing as all this is inlined, I
wonder if it's a good optimization...
As you may have noticed, the original code was using
wait_event_timeout() before like this:
CX18_DEBUG_HI_IRQ(sending interrupt SW1: %x to send %s\n,
irq, info-name);
cx18_write_reg_expect(cx, irq, SW1_INT_SET, irq, irq);
ret = wait_event_timeout(
*waitq,
cx18_readl(cx, mb-ack) == cx18_readl(cx,
mb-request),
timeout);
Because waiting for the ack back is the right thing to do, but certainly
waiting too long is not warranted.
This gave me the occasional log message like this:
1: cx18-0: irq: sending interrupt SW1: 8 to send CX18_CPU_DE_SET_MDL
2: cx18-0: irq: received interrupts SW1: 0 SW2: 8 HW2: 0
3: cx18-0: irq: received interrupts SW1: 1 SW2: 0 HW2: 0
4: cx18-0: warning: sending CX18_CPU_DE_SET_MDL timed out waiting 10 msecs
for RPU acknowledgement
Where line 1 is the driver notifiying the firmware with a SW1 interrupt.
Line 2 is the firmware responding back to the cx18_irq_handler() with
the Ack interrupt in SW2 (the flags match, 8 8, by design).
Line 3 is an unrelated incoming video buffer notification for the cx18
driver.
Line 4 is the wait_event_timeout() timing out.
Could it be that the wait_event doesn't actually run and check its
condition until _after_ line 3? In that case SW2 != 8 and so it goes back
to sleep? Calling wake_up() just makes the processes on the waitq
runnable, they don't actually run until later, possibly much later.
If your event occurs and wake_up() is called at point A, then the test
should be true when it's checked and schedule() is never called. If the
event happens at point B, then the process' state will have been changed to
TASK_RUNNING by wake_up(), remember it's already on the waitqueue at this
point, and schedule() won't sleep.
OK, for some reason, I thought schedule() and schedule_timeout() would
go to sleep anyway.
AFAIK, they'll still cause the kernel schedule a process. Maybe a
different process. But the original process is still in TASK_RUNNING state
and so still in the run queue and will get run again. If it was in
TASK_(UN)INTERRUPTIBLE state then it wouldn't be in the run queue and
wouldn't run again until something woke it up.
I think what's probably happening is the test, cx18_readl(cx, mb-ack) ==
cx18_readl(cx, mb-request), is somehow not true even though the ack has
been received.
A PCI bus read error could be the culprit here. That's the only thing I
can think of. We only get one notification via IRQ from the firmware.
Maybe a new request was added?
No, I lock the respective epu2apu or epu2cpu mailboxes respectively with
a mutex.
But in your log:
1: cx18-0: irq: sending interrupt SW1: 8 to send CX18_CPU_DE_SET_MDL
2: cx18-0: irq: received interrupts SW1: 0 SW2: 8 HW2: 0
3: cx18-0: irq: received interrupts SW1: 1 SW2: 0 HW2: 0
4: cx18-0: warning: sending CX18_CPU_DE_SET_MDL timed out waiting 10 msecs
for RPU acknowledgement
Isn't the wait_event_timeout() waiting until line 4? And doesn't line 3
mean something has changed the registers? Changed them before the
wait_event finished?
I think calling wait_event()'s with something that tests a hardware
register is a little iffy. It's better if the irq handler sets some driver
state flag (atomically!) that indicates the event you were waiting for has
happened and then you check that flag.
I was toying with setting an atomic while in the IRQ handler. But then
I realized when we get the ack interrupt, the firmware should actually
be done. So really the wakeup() is the only indicator I really need.
Checking for ack == req is just a formality I guess.
If you use an interruptible timeout, then you could get interrupted with a
signal before the irq handler has woken you.
There wasn't a wait_timeout(), so I had tried something like this in my
first iteration:
It's called sleep_on_timeout(q, timeout).
--
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: [PATCH 3/4] ARM: DaVinci: DM646x Video: Add VPIF display driver
On Mon, 30 Mar 2009, Hans Verkuil wrote:
On Thursday 26 March 2009 14:22:32 Chaithrika U S wrote:
+ /* one field is displayed configure the next
+ frame if it is available else hold on current
+ frame */
Comment isn't in standard format with *'s on the side. If you split this
into multiple functions like Hans suggested it wouldn't be indented so
much.
+static int vpif_enum_fmt_vid_out(struct file *file, void *priv,
+ struct v4l2_fmtdesc *fmt)
+{
+ unsigned int index = 0;
+
+ if (fmt-index != 0) {
+ v4l2_err(vpif_obj.v4l2_dev, Invalid format index\n);
+ return -EINVAL;
+ }
+
+ /* Fill in the information about format */
+ index = fmt-index;
+ memset(fmt, 0, sizeof(*fmt));
For most if not all of these functions v4l2_ioctl2 will take care of zeroing
the structs.
It is supposed to be all of them. If there are any I missed let me know so
I can fix it.
+static int vpif_g_fmt_vid_out(struct file *file, void *priv,
+ struct v4l2_format *fmt)
+{
[...]
+ /* Check the validity of the buffer type */
+ if (common-fmt.type != fmt-type)
+ return -EINVAL;
+
+ if (V4L2_BUF_TYPE_VIDEO_OUTPUT != fmt-type) {
+ if (vid_ch-std_info.vbi_supported == 0)
+ return -EINVAL;
+ }
All XXX_fmt_vid_out methods will only be called with fmt-type equal to
VIDEO_OUTPUT. You don't need to check.
+static int vpif_s_fmt_vid_out(struct file *file, void *priv,
+ struct v4l2_format *fmt)
+{
+ if (V4L2_BUF_TYPE_VIDEO_OUTPUT == fmt-type) {
Don't need this check.
+ struct v4l2_pix_format *pixfmt = fmt-fmt.pix;
+ /* Check for valid field format */
+ ret = vpif_check_format(channel, pixfmt);
+ if (ret)
+ return ret;
Rather than fail if the format requested isn't acceptable to the driver,
you should modify the requested format to something that is. For example,
if you need an even number of lines and they ask for an odd number, reduce
the number of lines by 1 instead of failing.
+static int vpif_enum_output(struct file *file, void *fh,
+ struct v4l2_output *output)
+{
+
+ struct vpif_config *config = vpif_dev-platform_data;
+ int index = output-index;
+
+ memset(output, 0, sizeof(*output));
+ if (index config-output_count) {
+ v4l2_dbg(1, debug, vpif_obj.v4l2_dev,
+ Invalid output index\n);
+ return -EINVAL;
+ }
+
+ output-index = index;
Another save index, memset, restore index sequence that isn't needed.
+#define ISALIGNED(a)(0 == (a%8))
Here you need parenthesis: (0 == ((a) % 8))
If 'a' isn't unsigned, then (0 == ((a) 7)) will be more efficient. For
unsigned values the compiler should be able to make the transformation to
using instead of %, but not for signed.
+struct vpif_config_params {
+ u8 min_numbuffers;
+ u8 numbuffers[VPIF_DISPLAY_NUM_CHANNELS];
+ u32 min_bufsize[VPIF_DISPLAY_NUM_CHANNELS];
+ u32 channel_bufsize[VPIF_DISPLAY_NUM_CHANNELS];
+};
If you put the larger fields first the structure will be padded less.
--
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: [linuxtv-commits] [hg:v4l-dvb] v4l2-ioctl: Check format for S_PARM and G_PARM
On Sun, 29 Mar 2009, Hans Verkuil wrote: On Sunday 29 March 2009 12:21:58 Trent Piepho wrote: On Sun, 29 Mar 2009, Hans Verkuil wrote: On Sunday 29 March 2009 10:50:02 Patch from Trent Piepho wrote: From: Trent Piepho xy...@speakeasy.org v4l2-ioctl: Check format for S_PARM and G_PARM Return EINVAL if VIDIOC_S/G_PARM is called for a buffer type that the driver doesn't define a -vidioc_try_fmt_XXX() method for. Several other ioctls, like QUERYBUF, QBUF, and DQBUF, etc. do this too. It saves each driver from having to check if the buffer type is one that it supports. Hi Trent, I wonder whether this change is correct. Looking at the spec I see that g/s_parm only supports VIDEO_CAPTURE, VIDEO_OUTPUT and PRIVATE or up. So what should happen if the type is VIDEO_OVERLAY? I think the g/s_parm implementation in v4l2-ioctl.c should first exclude the unsupported types before calling check_fmt. This change doesn't actually enable g/s_parm for VIDEO_OVERLAY (or VBI_CAPTURE). It's the later bttv and saa7146 changes that do that. I considered this when I made those changes, as mentioned in those patch descriptions, but decided it was better to allow it. In those drivers g_parm only returns the frame rate, which seems just as valid for VIDEO_OVERLAY as it does for VIDEO_CAPTURE. Why should the driver not be allowed to return the frame rate for video overlay? Why should setting the overlay frame rate not be allowed? Those seems like perfectly reasonable operations to me. Not to me. VIDEO_OVERLAY just defines where the overlay is. But the actual framerate is entirely dependent on the VIDEO_CAPTURE framerate. Just keep to the spec for now. If a new driver appears that needs it then we can always change it. How does overlay depend on video capture in any way? It's perfectly reasonable for a driver to support _only_ overlay and not video capture. The zr36067 chip is only designed to support uncompressed data for video overlay for example. Allowing uncompressed video capture is a hack that the driver didn't have at one point. The spec doesn't explicitly say that only VIDEO_CAPTURE, VIDEO_OUTPUT and PRIVATE are supported. It says the capture field of the parm union is used when type is VIDEO_CAPTURE, the output field is used for VIDEO_OUTPUT, and raw_data is used for PRIVATE or higher. You're right in that it doesn't say what you're supposed to use for VIDEO_OVERLAY, VBI_CAPTURE or any other buffer types. But it doesn't say they're not allowed either. IMHO, it's likely the spec authors' intent wasn't to not allow g_parm with VIDEO_OVERLAY, but rather that they just didn't think of that case. No, I agree with the spec in that I see no use case for it. Should there be one, then I'd like to see that in an actual driver implementation and in that case the spec should be adjusted as well. How about getting the frame rate of video overlay? Works with bttv. In addition, g/s_parm is only used in combination with webcams/sensors for which overlays and vbi are irrelevant. There are several drivers for non-webcams, like bttv, saa7134, and saa7146, that support g_parm. Why is returning the frame rate for video capture not valid? Why does the number of buffers used for read() mode only make sense for webcams? Thinking about it now, I think what makes the most sense is to use capture for VIDEO_OVERLAY, VBI_CAPTURE, and SLICED_VBI_CAPTURE. And use output for VBI_OUTPUT, SLICED_VBI_OUTPUT and VIDEO_OUTPUT_OVERLAY. I also wonder whether check_fmt shouldn't check for the presence of the s_fmt callbacks instead of try_fmt since try_fmt is an optional ioctl. I noticed that too. saa7146 doesn't have a try_fmt call for vbi_capture but is apparently supposed to support it. I sent a message about that earlier. I saw that. So why not check for s_fmt instead of try_fmt? That would solve this potential problem. Because that's clearly a change that belongs in another patch. -- 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
[pull] http://linuxtv.org/hg/~tap/v4l-dvb
Mauro, Please pull from http://linuxtv.org/hg/~tap/v4l-dvb for the following 14 changesets: 01/14: build: Fix kernel output directory support http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=346bab8698ea 02/14: v4l2-ioctl: Check format for S_PARM and G_PARM http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=44632c5fe5b2 03/14: saa7146: Remove buffer type check from vidioc_g_parm http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=c879e8f8c32b 04/14: bttv: Remove buffer type check from vidioc_g_parm http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=935d095cbc31 05/14: gspca: Stop setting buffer type, and avoid memset in querycap http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=089aaa41d473 06/14: omap24xxcam: Remove buffer type check from vidioc_s/g_parm http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=c545d5d1afcc 07/14: stkwebcam: Remove buffer type check from g_parm and q/dq/reqbufs http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=fb30bf07ca40 08/14: vino: Remove code for things already done by video_ioctl2 http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=900a6e99e7b0 09/14: cafe_ccic: Remove buffer type check from XXXbuf http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=e698f5e2e1db 10/14: cx23885-417: Don't need to zero ioctl parameter fields http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=76e7154479e4 11/14: cx88-blackbird: Stop setting buffer type in XXX_fmt_vid_cap http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=ea402dd306a6 12/14: meye: Remove buffer type checks from XXX_fmt_vid_cap, XXXbuf http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=1d2ae100255d 13/14: usbvision: Remove buffer type checks from enum_fmt_vid_cap, XXXbuf http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=5dd6945acfc3 14/14: zr364xx: Remove code for things already done by video_ioctl2 http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=406c54139ea6 linux/drivers/media/common/saa7146_video.c|4 linux/drivers/media/video/bt8xx/bttv-driver.c |3 linux/drivers/media/video/cafe_ccic.c | 14 linux/drivers/media/video/cx23885/cx23885-417.c | 29 - linux/drivers/media/video/cx88/cx88-blackbird.c |5 linux/drivers/media/video/gspca/gspca.c |5 linux/drivers/media/video/meye.c | 32 - linux/drivers/media/video/omap24xxcam.c |7 linux/drivers/media/video/stk-webcam.c| 11 linux/drivers/media/video/usbvision/usbvision-video.c | 10 linux/drivers/media/video/v4l2-ioctl.c|7 linux/drivers/media/video/vino.c | 335 +++--- linux/drivers/media/video/zr364xx.c | 16 v4l/Makefile |3 14 files changed, 158 insertions(+), 323 deletions(-) Thanks, Trent -- 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: [PATCH] v4l2: fill reserved fields of VIDIOC_ENUMAUDIO also
On Fri, 27 Mar 2009, Mauro Carvalho Chehab wrote: On Wed, 25 Mar 2009 17:51:39 +0100 N?meth M?rton nm...@freemail.hu wrote: From: M?rton N?meth nm...@freemail.hu When enumerating audio inputs with VIDIOC_ENUMAUDIO the gspca_sunplus driver does not fill the reserved fields of the struct v4l2_audio with zeros as required by V4L2 API revision 0.24 [1]. Add the missing initializations to the V4L2 framework. The patch was tested with v4l-test 0.10 [2] with gspca_sunplus driver and with Trust 610 LCD pow...@m ZOOM webcam. It didn't apply against the development tree. Anyway, a recent patch removed the need of memset there. the memory fill with zero now happens at the same code we copy the structure values. That code is in video_ioctl2, which gspca doesn't use. -- 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: [PATCH] v4l2: fill reserved fields of VIDIOC_ENUMAUDIO also
On Fri, 27 Mar 2009, Hans Verkuil wrote: On Friday 27 March 2009 20:45:40 Trent Piepho wrote: On Fri, 27 Mar 2009, Mauro Carvalho Chehab wrote: On Wed, 25 Mar 2009 17:51:39 +0100 N?meth M?rton nm...@freemail.hu wrote: From: M?rton N?meth nm...@freemail.hu When enumerating audio inputs with VIDIOC_ENUMAUDIO the gspca_sunplus driver does not fill the reserved fields of the struct v4l2_audio with zeros as required by V4L2 API revision 0.24 [1]. Add the missing initializations to the V4L2 framework. The patch was tested with v4l-test 0.10 [2] with gspca_sunplus driver and with Trust 610 LCD pow...@m ZOOM webcam. It didn't apply against the development tree. Anyway, a recent patch removed the need of memset there. the memory fill with zero now happens at the same code we copy the structure values. That code is in video_ioctl2, which gspca doesn't use. Yes, gspca does use video_ioctl2. You're probably confused with uvcvideo, which doesn't use it. You're right, I was thinking about N?meth's earlier patches for the same things in uvcvideo. This patch wasn't for gspca anyway, it was for the v4l2 core, and Mauro's right it's not necessary as my patch series fixed all these problems. -- 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: Fw: [PATCH] cx88: Missing failure checks
On Thu, 26 Mar 2009, Mauro Carvalho Chehab wrote: From: Alan Cox a...@lxorguk.ukuu.org.uk To: linux-ker...@vger.kernel.org, mche...@infradead.org Subject: [PATCH] cx88: Missing failure checks The ioremap one was reported in October 2007 (Bug 9146), the kmalloc one was blindingly obvious while looking at the ioremap one The bug suggests some other configuration for lots of I/O memory (32MB per device is ioremapped) but I'll leave that to the real maintainers Each function has a 16 MB window and Linux can use three of the functions. IIRC, all the function's register windows are the same so only one needs to be mapped. -- 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: The right way to interpret the content of SNR, signal strength and BER from HVR 4000 Lite
On Fri, 20 Mar 2009, Devin Heitmueller wrote: On Thu, Mar 19, 2009 at 7:06 PM, Trent Piepho xy...@speakeasy.org wrote: The argument being put forth is based on the relative efficiency of the multiply versus divide opcodes on modern CPU architectures?? ?And Maybe I just like writing efficient code to do interesting things? Wow, um, ok. You realize that getting the SNR on most devices is probably going to require an i2c call that is going to take a couple hundred CPU instructions, not to mention I/O, right? And that you're doing this in an expensive ioctl call? So perhaps a micro-optimization with no visible gain, at the cost of readability and complexity shouldn't be a overriding consideration? Did I submit a patch to modify dvb-apps? I just wanted to show how to convert fixed point numbers to IEEE 754 floating using only integer math. I think such things are interesting. that you're going to be able to get an SNR with a higher level of precision than 0.1 dB?? (if the hardware suggests that it can then it's LYING to you) Not really. ?Absolute accuracy is not going to be that good of course. ?But the error measurements from which SNR is calculated do support precision of better than 0.1 dB. ?That precision does give you more power when fine tuning antenna position. Put another way, what advantage is there of less precision? Well, here is one disadvantage: The driver decides the SNR is 23.1. So I convert that to your format: 0x1719. Then userland gets it back You've assumed the driver will find SNR in decimal fixed point. The lgdt demods and the oren demods all *start* with binary fixed point. So you have the opposite problem were converting to decimal fixed point changes the result. and goes to display it. I'm going to show the user an SNR of 23.09765625, and I have no way to know what the expected precision (and thus I don't know where to round). So the end result is the user sees a really stupid number in the GUI (and might actually think it is more accurate than it really is). Or when I push patches to applications I just round to 0.1dB anyway. It also means apps like femon and zap are going to have to change to support a non-fixed width result with no appreciable gain in value. Since 8 binary digits is 2.4 decimal digits, it's perfectly ok to display SNR with 3 fixed decimal digits. You could just as well round to 1 fixed decimal digit too, in which case you've got exactly what you'd get if you started with the decimal fixed point, except the extra precision is there for a situation where it is useful. By saying explicitly there is one digit of precision - it allows for applications to know how to round, and I continue to disagree with Binary fixed point says explicitly that there are 8 binary digits of precision. Decimal fixed point says there are 3.3219281 binary digits of precision. We stopped using BCD 30 years ago, computers do everything in binary now. -- 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: Results of the 'dropping support for kernels 2.6.22' poll
On Fri, 20 Mar 2009, Devin Heitmueller wrote: On Fri, Mar 20, 2009 at 6:20 PM, Mauro Carvalho Chehab mche...@infradead.org wrote: My suggestion is to keep a backporting system, but more targeted at the end-users. The reasons are the ones explained above. Basically: Ok, so just so we're all on the same page - we're telling all the developers not willing to run a bleeding edge rc kernel to screw off? Got an Nvidia video card? Go away! The wireless broken in this week's -rc candidate? Go away! Your distro doesn't yet support the bleeding edge kernel? Go away! Want to have a stable base on which to work so you can focus on v4l-dvb development? Go away! Maybe you're supposed to have ten computers running different kernels? -- 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: [PATCH] gspca: add missing .type field check in VIDIOC_G_PARM
On Thu, 19 Mar 2009, [ISO-8859-1] N?meth M?rton wrote:
David Ellingsworth wrote:
2009/3/18 N?meth M?rton nm...@freemail.hu:
From: M?rton N?meth nm...@freemail.hu
The gspca webcam driver does not check the .type field of struct
v4l2_streamparm.
This field is an input parameter for the driver according to V4L2 API
specification,
revision 0.24 [1]. Add the missing check.
The missing check was recognised by v4l-test 0.10 [2] together with
gspca_sunplus driver
and with Trust 610 LCD pow...@m ZOOM webcam. This patch was verified
also with
v4l-test 0.10.
References:
[1] V4L2 API specification, revision 0.24
http://v4l2spec.bytesex.org/spec/r11680.htm
[2] v4l-test: Test environment for Video For Linux Two API
http://v4l-test.sourceforge.net/
Signed-off-by: M?rton N?meth nm...@freemail.hu
---
--- linux-2.6.29-rc8/drivers/media/video/gspca/gspca.c.orig 2009-03-14
12:29:38.0 +0100
+++ linux-2.6.29-rc8/drivers/media/video/gspca/gspca.c 2009-03-18
16:51:03.0 +0100
@@ -1320,6 +1320,9 @@ static int vidioc_g_parm(struct file *fi
{
struct gspca_dev *gspca_dev = priv;
+ if (parm-type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
+ return -EINVAL;
+
memset(parm, 0, sizeof *parm);
parm-type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
^^^
This line should be deleted as it's no longer needed.
Because memset() clears the whole parm structure this line is necessary. In
other
drivers the following code is there:
tmp = parm-type;
memset(parm, 0, sizeof(*parm));
parm-type = parm;
The memset isn't needed anymore either, I put it into v4l2_ioctl. I
removed most of the code like that but I may have missed some drivers.
--
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: Results of the 'dropping support for kernels 2.6.22' poll
On Sun, 15 Mar 2009, Hans Verkuil wrote: On Sunday 15 March 2009 17:39:11 Trent Piepho wrote: Because there are patches that touch both the media tree and outside it? I don't buy it. Even for sub-systems that only use full git trees, you almost never see a patch that touches multiple areas of maintainership. It's just too hard to deal with getting acks from everyone involved, dealing with the out-of-sync development git trees of the multiple areas you want to touch, figuring out who will take your patch, etc. Think embedded devices like omap, davinci and other SoC devices. These all require changes in both v4l-dvb and arch at the same time. Easy to do if you have a full git tree, much harder to do in the current situation. These devices will become much more important in the coming months and years, so having a proper git tree will definitely help. This is a relatively new development and before that it was indeed rare to see patches touching on areas outside the media tree. Not anymore, though. ALSA has a full git tree now, so there should be all these patches that touch sound/ and something out side of sound too? I'm not seeing them. The only patches that touch inside and outside of ALSA are the ones that fix some misspelled word in 100 random files or rename a linux header file. -- 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: [PATCH] gspca: add missing .type field check in VIDIOC_G_PARM
On Thu, 19 Mar 2009, [UTF-8] N??meth M??rton wrote: The gspca webcam driver does not check the .type field of struct v4l2_streamparm. This field is an input parameter for the driver according to V4L2 API specification, revision 0.24 [1]. Add the missing check. I think this check could go in the v4l2 core too. It already does a similar check for QUERYBUF, QBUF, DQBUF, etc. If the driver doesn't provide a method for -vidioc_try_fmt_foo() then the v4l2 core will reject a call with .type == V4L2_BUF_TYPE_foo. It seems like it should be ok to do this for S_PARM and G_PARM too. -- 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: Improve DKMS build of v4l-dvb?
On Tue, 17 Mar 2009, Alain Kalker wrote: Op vrijdag 13-03-2009 om 02:12 uur [tijdzone -0700], schreef Trent Piepho: On Mon, 9 Mar 2009, Alain Kalker wrote: Firstly: generating a .config with just one config variable for the requested driver set to 'm' merged with the config for the kernel being built for, and then doing a make silentoldconfig. Big disatvantage is that full kernel source is required for the 'silentoldconfig' target to be available. Does that actually work? Figuring out that needs to be turned on to enable some config options is a hard problem. It's not just simple dependencies between modules, but complex expressions that need to be satisfied. E.g., something depends on A || B, which do you turn on, A or B? There are multiple solutions so how does the code decide which is best? Well, make_kconfig.pl does quite a nice job trying to select as many drivers without causing conflicts. What I did in make_kconfig.pl was just turn everything on, then recursively disable anything that has a failed dependency. There isn't any intelligence when it comes to choices where you can have driver set A or driver set B, but not both. Options that we want disabled, like some drivers' advanced debug controls, must be explicitly disabled in make_kconfig. Still, it ends up doing what we want in the end, which is to compile all the drivers that we can compile. Anyway, you're quite right about this being a hard problem, and the fact that the Kconfig system wasn't designed to be very helpful in auto-selecting dependencies and resolving conflicts the same way modern package managers are, doesn't make it any easier. From what I can tell, solving the dependency problem is easily shown to be the same as the classical satisfiability problem, which is proven to be NP complete. Now, there are heuristics that can usually solve SAT problems quicker but finding the best solution quickly is quite a bit harder. For the moment, I would suggest either to choose a default which works for most people, or ask the user (using any Kconfig menu tool, if only they didn't need write access to the kernel source, grrr!) to choose among alternatives if no combination of options can be selected automatically. You don't need write access to the kernel source. The kernel's config programs have to be built, but that can be done ahead of time. Once they are, then you can use that menu tool from v4l-dvb without write access to the kernel source. There is support for an alternate output directory for the kernel that can work too. In the kernel dir, run make O=~/kernel-output-dir menuconfig. That should not require write access to the kernel source dir and will put the necessary config programs in ~/kernel-output-dir. Then point v4l-dvb at the kernel output dir, with make release DIR=~/kernel-output-dir. See the explanation from my changeset that added this, http://linuxtv.org/hg/v4l-dvb/log/6331 Good thing I wrote this 17 months ago when I did the work, instead of just using some two word patch description, since I sure wouldn't remember how all that works today. -- 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: bttv, tvaudio and ir-kbd-i2c probing conflict
On Tue, 17 Mar 2009, Jean Delvare wrote: On Mon, 16 Mar 2009 15:47:17 -0700 (PDT), Trent Piepho wrote: On Mon, 16 Mar 2009, Jean Delvare wrote: You are unfair. The pull request came with a short log of all the changes. short log. His entire series was decribed with fewer words than I would use on a single patch that changes ten lines. In general I tend to like detailed patch logs as much as you do. But in this case Hans is doing almost all the work by himself and it is very needed, and the faster completed, the better. So I am really to trade log details for a faster conversion. I guess that I don't consider documentation to be optional. (...) I am not familiar enough with this part of the code to say. But I guess it doesn't really matter, as it wasn't my point anyway. It seems like your point was that conversions to v4l2_subdev allow drivers to be more efficient remove lots of code. The numbers I see just don't support that claim. No, sorry if I didn't make it clear, but that wasn't my point. My point was only about the change in i2c binding model. This change clearly results in a net shrink as far as lines of code are concerned. Does it? When we can use the model as it's designed, then I think it's clearly much better. But when one is emulating the detection behaviour, like it appears the bttv patches do, I don't see what's better. -- 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: [PULL] http://linuxtv.org/hg/~dheitmueller/hvr950q-analog2/
On Sun, 15 Mar 2009, Devin Heitmueller wrote: au0828: remove memset calls in v4l2 routines. The userland callers are responsible for clearing the output buffers, so remove the unneeded memset calls. A driver should not assume that _userspace_ has cleared the buffers. In some cases userspace is supposed to clear certain fields, but you shouldn't assume it. AFAIK, for read-only ioctls there is no expectation at all that userspace will clear the buffer. Your patch is still right though, as now the videodev core will take care of this so individual drivers don't have to. -- 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: REVIEW: bttv conversion to v4l2_subdev
On Mon, 16 Mar 2009, Hans Verkuil wrote:
+++ b/linux/drivers/media/video/bt8xx/bttvp.h Sun Mar 15 13:07:15 2009
+0100
@@ -331,6 +331,7 @@ struct bttv {
unsigned int tuner_type; /* tuner chip type */
unsigned int tda9887_conf;
unsigned int svhs, dig;
+ int has_saa6588;
Does it need to be a 32 or 64 bit integer?
I'll replace it with a u8.
struct bttv_pll_info pll;
int triton1;
int gpioirq;
Due to field alignment in the structure it won't make any difference.
--
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: [PATCH v2 3/4] pxa_camera: Redesign DMA handling
On Mon, 16 Mar 2009, Robert Jarzmik wrote: Guennadi Liakhovetski g.liakhovet...@gmx.de writes: What is QIF? Do you mean Quick Capture Interface - QCI? I also see CIF used in the datasheet, probably, for Capture InterFace, but I don't see QIF anywhere. Also, please explain the first time you use the abbreviation. Also fix it in the commit message to patch 1/4. OK, will replace with QCI, my bad. In video capture, QIF and CIF usually refer to image size. CIF would be 320x240 for NTSC and QIF or QCIF would be 160x120. -- 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: bttv, tvaudio and ir-kbd-i2c probing conflict
On Mon, 16 Mar 2009, Jean Delvare wrote: On Mon, 16 Mar 2009 12:43:33 -0700 (PDT), Trent Piepho wrote: On Mon, 16 Mar 2009, Jean Delvare wrote: Come on, just look at ir-kbd-i2c and tvaudio again, see how great are these drivers which have been designed on top of the legacy i2c binding model. Look at the bttv mess. Look at the zoran driver conversion done by Hans a few weeks ago, which killed what, 3000 lines of code? The old binding model was so bad that DVB doesn't even use it. IIRC, the zoran patch removed more like 1000 lines. You don't remember correctly: 37 files changed, 4570 insertions(+), 7404 deletions(-) That's 2834 lines. 1570 of which is for the removal of the saa7111 and saa7114 and I agree this shouldn't count, so we're down to 1264 lines removed. The removal of some features also shouldn't count but I doubt it amounts to more than a few hundred lines. 1264 lines is more like 1000 lines than it is like 3000 lines! In fact, to get a more accurate figure you can just look at the result of my own conversion. There were two patches: 13 files changed, 510 insertions(+), 625 deletions(-) 11 files changed, 549 deletions(-) This is -664 lines total. The exact number doesn't really matter. The bottom line is that the new binding model requires significantly less glue code than the legacy model. If you think some more about it, it means a lot. Then how come the bttv patch added lines of code and a couple kB to the compiled driver size? But it also deleted v4l1 support, highmem support, and bigphys_area support. Maybe other things, Hans doesn't decribe his patches, so there's really no way to know what the zoran patch really did other than to weed through 10,000+ lines of diff which is mostly but not entirely moving blocks of code from one space to another and reindenting them. You are unfair. The pull request came with a short log of all the changes. short log. His entire series was decribed with fewer words than I would use on a single patch that changes ten lines. If one includes the v4l1-compat module that is now providing v4l1 support (though it doesn't work correctly for zoran), the driver and the compat module are larger than the old driver was. Of course one can now turn off v4l1 support and get a smaller driver than before. And the v4l1 compat already existed and can be shared. But I think it's more correct to say the size reduction of the zoran driver was from removing features and not from v4l2_subdev. It seems like more of the the other subdev conversions have overall added more code than they removed. I am not familiar enough with this part of the code to say. But I guess it doesn't really matter, as it wasn't my point anyway. It seems like your point was that conversions to v4l2_subdev allow drivers to be more efficient remove lots of code. The numbers I see just don't support that claim. -- 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: [PATCH] LED control
On Sun, 15 Mar 2009, Jean-Francois Moine wrote: On Sat, 14 Mar 2009 13:16:11 -0700 (PDT) Trent Piepho xy...@speakeasy.org wrote: There is already a sysfs led interface, you could just have the driver export the leds to the led subsystem and use that. Yes, but: - this asks to have a kernel generated with CONFIG_NEW_LEDS, So? - the user must use some new program to access /sys/class/leds/device, echo, cat? - he must know how the LEDs of his webcam are named in the /sys tree. Just give them a name like video0:power and it will be easy enough to associate them with the device. I think links in sysfs would do it to, /sys/class/video4linux/video0/device/ledname or something like that. The advantage of using the led class is that you get support for triggers and automatic blink functions, etc. -- 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: REVIEW: bttv conversion to v4l2_subdev
On Sun, 15 Mar 2009, Hans Verkuil wrote: Hi Mauro, Can you review my ~hverkuil/v4l-dvb-bttv2 tree? It would be a lot easier if you would provide patch descriptions. -- 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: REVIEW: bttv conversion to v4l2_subdev
On Sun, 15 Mar 2009, Hans Verkuil wrote: On Sunday 15 March 2009 17:04:43 Trent Piepho wrote: On Sun, 15 Mar 2009, Hans Verkuil wrote: Hi Mauro, Can you review my ~hverkuil/v4l-dvb-bttv2 tree? It would be a lot easier if you would provide patch descriptions. Here it is: - bttv: convert to v4l2_subdev. You aren't even trying. I could easily write two pages on this patch. What new module parameters did you add? Why? What module parameters did you delete? Why? How does one translate a existing modprobe.conf file? Why are the i2c addresses from various i2c chips moved into the bttv driver? Doesn't it make more sense that the addresses for chip X should be in the driver for chip X? How has module loading changed? Can one no longer *not* autoload modules if you are trying to test drivers that are not installed in /lib/modules? What fields did you add to the card database? Why? How much did the size increase? What is the never set has_saa6588 field in tvcards needed for? What are the parameters to bttv_call_all? How did you change the probing sequence? What was it before? What is it now? Where do the subdevs you created get deleted? -- 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: bttv, tvaudio and ir-kbd-i2c probing conflict
On Sun, 15 Mar 2009, Jean Delvare wrote: On Sun, 15 Mar 2009 13:44:01 +0100, Hans Verkuil wrote: This is the typical multifunction device problem. It isn't specifically related to I2C, the exact same problem happens for other devices, for example a PCI south bridge including hardware monitoring and SMBus, or a Super-I/O chip including hardware monitoring, parallel port, infrared, watchdog, etc. Linux currently only allows one driver to bind to a given device, so it becomes very difficult to make per-function drivers for such devices. For very specific devices, it isn't necessarily a big problem. You can simply make an all-in-one driver for that specific device. The real problem is when the device in question is fully compatible with other devices which only implement functionality A _and_ fully compatible with other devices which only implement functionality B. You don't really want to support functions A and B in the same driver if most devices out there have either function but not both. You can also split the device into multiple devices. Most SoCs have one register block where all kinds of devices, from i2c controllers to network adapters, exist. This is shown to linux as many devices, rather than one massive multifunction device. -- 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: REVIEW: bttv conversion to v4l2_subdev
On Sun, 15 Mar 2009, Andy Walls wrote: On Sun, 2009-03-15 at 10:28 -0700, Trent Piepho wrote: Why are the i2c addresses from various i2c chips moved into the bttv driver? Doesn't it make more sense that the addresses for chip X should be in the driver for chip X? One reason that this may be undesirable is that the devices can be set to slightly different addresses via external straps (probably a corner case, I know). The bridge driver has the best chance of knowing what chips are where with certainty. If one knows that some card has a certain chip at a certain address, then it makes the most sense to store that with the rest of the card's data, i.e. in the bridge driver. But the bttv code looks like (I don't know what Hans intended, since he didn't feel anyone else needed to know what, why, or how his code does whatever it is that it does) it is probing all known address that a given chip could be at to see if the chip is there. In that case what's really being used is an address list from the I2C chip's datasheet and any bridge driver that wants to probe like that will use the same list, which IMHO, makes more sense to put in the driver of the I2C chip rather than every driver that uses the I2C chip. -- 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: bttv, tvaudio and ir-kbd-i2c probing conflict
On Sun, 15 Mar 2009, Andy Walls wrote: On Sun, 2009-03-15 at 18:12 +0100, Jean Delvare wrote: This is the typical multifunction device problem. It isn't specifically related to I2C, But the specific problem that Hans' brings up is precisely a Linux kernel I2C subsystem *software* prohibition on two i2c_clients binding to the same address on the same adapter. For a lot of i2c devices, it would be difficult for two drivers to access the device at the same time without some kind of locking. If you take the reads and writes of one driver and then intersperse the reads and writes of another driver, the resulting sequence from the i2c device's point of view is completely broken. But, I suppose there are some devices where if the drivers all use i2c_smbus_read/write_byte/word_data or equivalent atomic transactions with i2c_transfer(), then you could get away with two drivers talking to the same chip. -- 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: [PATCH] LED control
On Sat, 14 Mar 2009, Mauro Carvalho Chehab wrote: On Sat, 14 Mar 2009 12:59:23 +0100 Jean-Francois Moine moin...@free.fr wrote: + entryconstantV4L2_CID_LEDS/constant/entry + entryinteger/entry + entrySwitch on or off the LEDs or illuminators of the device. +In the control value, each LED may be coded in one bit (0: off, 1: on) or in +many bits (light intensity)./entry + /row + row The idea of having some sort of control over the LEDs is interesting, but we should have a better way of controlling it. If the LED may have more than one bit, maybe the better would be to create more than one CID entry. Something like: V4L2_CID_LED_POWER- for showing that the camera is being used V4L2_CID_LED_LIGHT- for normal white light V4L2_CID_LED_INFRARED - for dark light, using infrared ... This way a driver can enumberate what kind of leds are available, and get the power intensity range for each individual one. There is already a sysfs led interface, you could just have the driver export the leds to the led subsystem and use that. -- 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: identifying camera sensor
On Thu, 5 Mar 2009, Hans Verkuil wrote: Reserved bits are there for a reason. If a particular bit of information it a perfect match with for that API, then it seems utterly pointless to me to decide not to use them 'just because we might run out in the future'. It would be one thing if there was just a possibility of them running out in the distant future. Rather, they'll run out before you even get started on adding what's already available for digital cameras. Focus distance, exposure compensation, sensor manufacture and model, gravity sensors, detailed gamma compensation curves, and so on. Though if one had considered allowing the control api to be used to provide sensor properties, then the solution to this problem would now be quite simple and obvious. In this case you want to have device names. While not impossible, it is very hard to pass strings over the control api. Lots of issues with 32-64 bit compatibility and copying to/from user space. Also, in this case the That's true. While I think the control api is the best one for providing ancillary data for images and sensor attributes, it's not perfect. I'd add a means to define the data type of a control/attribute and allow things besides an s32. Probably ASCIIZ strings and fix length byte arrays. Also flags for: constant vs volatile Constant attributes don't change, e.g. sensor manufacturer name. Volatile ones can change, e.g. focus distance. global vs per-input Global attributes are the same for the entire device, while per-input attributes are different for each input. An API for querying the attribute for an input different the current might be nice, but I think it might be one of things that seem more important than they really are. Frame syncable Frame syncable attributes (which only make sense for volatile attributes) can be synced to the exact frame they were measured at. For instance, the the camera provides a focus distance value for software face tracking, it's important that the right focus distance be associated with the correct frame. There should be an API by which one can request that the attribute be provided with each frame, maybe tacked onto the end of the v4l2_buffer structure. control API is NOT a good match, since this isn't a single piece of data, instead you can have multiple sensor devices or other video enhancement devices that an application might need to know about. Which is why my last brainstorm suggestion was an ENUM_CHIPS ioctl. How would enum chips be different than enum controls? -- 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: [PATCH] cx88: Add IR support to pcHDTV HD3000 HD5500
On Thu, 5 Mar 2009, Erik S. Beiser wrote: Thanks for your comments, Trent. My responses below: Trent Piepho wrote: On Sat, 28 Feb 2009, Erik S. Beiser wrote: cx88: Add IR support to pcHDTV HD3000 HD5500 Signed-off-by: Erik S. Beiser er...@bu.edu --- Idea originally from http://www.pchdtv.com/forum/viewtopic.php?t=1529 I made it into this small patch and added the HD3000 support also, which I have I've actually been using this for over a year, updating for new kernel versions. I'm tired of doing so, and would like to try and have it merged upstream -- I hope I got all the patch-mechanics correct. I just updated and tested it today on 2.6.28.7 vanilla. Thanks. You forgot a patch description. Ok, I had looked around and saw many patches that had the one-liner descriptions, which I thought would be sufficient for a four line patch. At your request, I can add a couple lines description when I fix it (see below). You won't see such patch descriptions from me. Clearly your patch made some very significant changes and assumptions that really should have in the description. Since neither the HD-3000 or HD-5500 came with a remote, and at least my HD-3000 didn't even come with an IR receiver. So I have to ask why configuring the driver to work a remote you happened to have is any more correct than configuring it to work a remote someone else happens to have? True, the vendor doesn't seem to sell a remote or IR receiver with these cards. I was actually surprised when I got mine to see the jack for an IR receiver, which can be made to work if one has those parts from another source. I don't think that because these cards were sold without a remote and receiver should mean that we don't expose the receiver functionality. I didn't see that happening elsewhere, so I adopted this 'worksforme' solution. You have a valid point about the key mapping, which I did not fully consider. I don't have a good justification. OTOH how does someone with one of those cards use a remote different from what came with their card? Is that possible? The problem with exposing the receiver this way is that it's unlikely to be useful to anyone but yourself. Given the significant performance cost of enabling ir sampling, and the very limited usefulness, I don't think this belongs in the kernel. All I'm really trying to accomplish is to somehow get inputs from a remote exposed through some device, with which I could parse stuff with lirc. This method exposed it via a /dev/input/eventN node. I admit I hadn't paid too much attention to the keymapping before. (Just trying to get the thing to work.) But you prompted me to dig deeper, and I see that in drivers/media/common/ir-keymaps.c there is a ir_codes_empty mapping. I tried it tonight with that mapping instead, and a /dev/input/eventN device was created, but I don't seem to get anything from it. Which I guess isn't too surprising, but if so, then how can I go about generically exposing signals from the IR port to userspace? You might look at the patches from Jon Simrl that try to add IR support to the input system. They use configfs to define remote keycodes. You could also create a device the exports the raw timings to lircd to decode. I think lircd might still use the mode2 interface I created for the ir serial driver over a decade ago. That would be easy to copy. This patch also causes these cards to generate 101 interrupts per second per card, even when not in use. That seems pretty costly for a card that doesn't even come with an ir sensor. Whoa, I didn't realize that. Can you point me to how I can verify that? Is that simply an effect of the ir-sampling type? Might a different type be preferred? I could do some experimenting. Just look at /proc/interrupts. The ir sampling mode isn't documented in the cx23880 datasheet, but it looks like it samples gpio16 at about 250 us intervals and generate an interrupt each time 32 samples are ready. Hugely inefficient. A serial port ir receiver would be much better. The cx23880 can operate that way with gpio22 and gpio23, but I think the mpeg ts interface uses those gpios. Maybe there is some way to turn on the remote sampling when something is listening for remote events? -- 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: Kconfig changes in /hg/v4l-dvb caused dvb_usb_cxusb to stop building (fwd)
On Mon, 9 Mar 2009, Mauro Carvalho Chehab wrote: Btw, if you look at DVB_FE_CUSTOMISE help, it is recommended tho unselect it, if you're not sure what to do. Anyways, here's what I get: $ grep ^CONFIG .config [everything is 'm'] CONFIG_DVB_VES1820=m CONFIG_DVB_STV0297=m CONFIG_DVB_LNBP21=m Seems perfect to my eyes. I think it might be nicer if the default value for a frontend when customize was turned on was whatever it was selected to by the drivers that use it. When you don't use customize, all the frontends default to 'n'. If you set some driver to 'm', it will set all the frontends it uses to 'm'. Set the driver to 'y' and then those frontends get set to 'y'. If you turn on customize, the default for the frontends should be same as what they are when customize is off. The difference is now you can see them and change their value. -- 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: Improve DKMS build of v4l-dvb?
On Mon, 9 Mar 2009, Alain Kalker wrote: Martin has an older version of the drivers packaged for building with DKMS on Ubuntu in his PPA[5], but it currently has some disadvantages: A. It builds all available drivers, no matter which hardware is actually installed in the system. This takes a lot of time, and may not be practical at all on systems with limited resources (e.g. embedded, MIDs, netbooks) B. It currently has no support for Jockey to detect installed hardware, so individual drivers can be selected. To address these issues, I would like to propose the following: A. Building individual drivers (i.e. sets of modules which constitute a fully-functional driver), without having to manually configure them using make menuconfig I see two possibilities for realizing this: Firstly: generating a .config with just one config variable for the requested driver set to 'm' merged with the config for the kernel being built for, and then doing a make silentoldconfig. Big disatvantage is that full kernel source is required for the 'silentoldconfig' target to be available. Does that actually work? Figuring out that needs to be turned on to enable some config options is a hard problem. It's not just simple dependencies between modules, but complex expressions that need to be satisfied. E.g., something depends on A || B, which do you turn on, A or B? There are multiple solutions so how does the code decide which is best? Secondly, the script v4l/scripts/analyze_build.pl generates a list of modules that will get built for each Kconfig variable selected, but it currently has no way of determing all the module dependencies that make up a fully functional driver. I just wrote analyze_build.pl to make it easier for developers to figure out that source files make up a module and how to enable it. It's not actually used by the build system. It's also not perfect when it comes to parsing makefiles, i.e. it no where near a re-implementation of make's parser in perl. It understands the typical syntax used by the kernel makefiles but sometimes there is some unusual bit of make code that it won't parse. The script v4l/scripts/check_deps.pl tries to discover dependencies between Kconfig variables, but it currently is somewhat slow, and hase a few other problems. That it is! It's not totally perfect either. Sometimes a driver will only depend on another if something is turned on. But the way check_deps.pl works won't know that. There are also lots of Kconfig variables that don't turn on a module but instead modify what a module does or are used for menus. I think a better system would be use the dependencies in the Kconfig files instead of trying to figure them out from the source. B. To enable hardware autodetection before installing drivers, we need to have a list of modaliases of all supported hardware. This may be the hardest part, because many VendorIDs and ProductIDs are scattered throughout the code. Also coldbooting/warmbooting hardware is a problem. Extract that from the compiled modules should be easy. -- 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
[pull] http://linuxtv.org/hg/~tap/v4l-dvb
Mauro, Please pull from http://linuxtv.org/hg/~tap/v4l-dvb for the following changeset: 01/01: build: have make_kconfig.pl ignore comments http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=9debb0a2ec70 make_kconfig.pl | 13 + 1 files changed, 9 insertions(+), 4 deletions(-) Thanks, Trent -- 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: The right way to interpret the content of SNR, signal strength and BER from HVR 4000 Lite
On Fri, 13 Mar 2009, Devin Heitmueller wrote: On Fri, Mar 13, 2009 at 5:11 PM, Trent Piepho xy...@speakeasy.org wrote: I like 8.8 fixed point a lot better. ?It gives more precision. ?The range is more in line with that the range of real SNRs are. ?Computers are binary, so the math can end up faster. ?It's easier to read when printed out in hex, since you can get the integer part of SNR just by looking at the first byte. ?E.g., 25.3 would be 0x194C, 0x19 = 25 db, 0x4c = a little more than quarter. ?Several drivers already use it. Wow, I know you said you like that idea alot better, but I read it and it made me feel sick to my stomach. Once we have a uniform format, we won't need to show it in hex at all. Tools like femon and scan can But if you do see it in hex, it's easier to understand. If it's not shown in hex, you still have better precision and better math. What advantage is there to using something that's 4.1 decimal fixed point on a binary computer? On a separate note, do you know specifically which drivers use that format? I was putting together a table of all the various or51211, or51132, and lgdt330x at least. Don't know about the other lg demods. The dvb match code they all use makes it's easy to get the snr in 8.8 fixed point with the typical log caclulations required. -- 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: The right way to interpret the content of SNR, signal strength and BER from HVR 4000 Lite
On Fri, 13 Mar 2009, Andy Walls wrote: On Fri, 2009-03-13 at 10:27 -0400, Devin Heitmueller wrote: On Fri, Mar 13, 2009 at 12:19 AM, Ang Way Chuang wc...@nav6.org wrote: Yes, please :) Yeah, Michael Krufky and I were discussing it in more detail yesterday on the #linuxtv ML. Essentially there are a few issues: 1. Getting everyone to agree on the definition of SNR, and what units to represent it in. It seems like everybody I have talked to has no issue with doing in 0.1 dB increments, so for example an SNR of 25.3 would be presented as 0x00FD. +/- 0.1 dB indicates increases or decreases of about 2.3% which should be just fine as a step size. I've found that the extra precision helps when trying to align an antenna. I turn the antenna a few degrees and then measure snr for a while. Then make a plot of snr vs antenna rotation. With the extra precision you can see the average snr change as you fine tune to rotation. Rounding off the extra digit makes it harder to see. What is the advantage to using base 10 fixed point on binary computer? -- 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: [PATCH] Add support for ProVideo PV-183 to bttv
On Fri, 13 Mar 2009, Alan McIvor wrote:
+
+{ 0x15401830, BTTV_BOARD_PV183, Provideo PV183-1 },
+{ 0x15401831, BTTV_BOARD_PV183, Provideo PV183-2 },
+{ 0x15401832, BTTV_BOARD_PV183, Provideo PV183-3 },
+{ 0x15401833, BTTV_BOARD_PV183, Provideo PV183-4 },
+{ 0x15401834, BTTV_BOARD_PV183, Provideo PV183-5 },
+{ 0x15401835, BTTV_BOARD_PV183, Provideo PV183-6 },
+{ 0x15401836, BTTV_BOARD_PV183, Provideo PV183-7 },
+{ 0x15401837, BTTV_BOARD_PV183, Provideo PV183-8 },
+
{ 0, -1, NULL }
};
Looks like you used spaces here instead of tabs. If you run make commit
from the v4l-dvb tree it will fix these things.
--
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: [PATCH 1/4] pxa_camera: Remove YUV planar formats hole
On Mon, 9 Mar 2009, Robert Jarzmik wrote: Ok, this one will change I presume - new alignment calculations and line-breaking. In fact, if you adjust width and height earlier in set_fmt, maybe you'll just remove any rounding here completely. Helas, not fully. The problem is with passthrough and rgb formats, where I don't enforce width/height. In the newest form of the patch I have this : if (pcdev-channels == 3) *size = icd-width * icd-height * 2; else *size = roundup(icd-width * icd-height * ((icd-current_fmt-depth + 7) 3), 8); If icd-current_fmt-depth could be set to 16 for planar formats, then you could get rid of the special case here. -- 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: V4L2 spec
On Mon, 9 Mar 2009, Hans Verkuil wrote: On Monday 09 March 2009 12:08:39 Mauro Carvalho Chehab wrote: On Fri, 6 Mar 2009, wk wrote: Hans Verkuil wrote: Hi Mauro, I noticed that there is an ancient V4L2 spec in our tree in the v4l/API directory. Is that spec used in any way? I don't think so, so I suggest that it is removed. OK. The V4L1 spec that is there should probably be moved to the v4l2-spec directory as that is where people would look for it. We can just keep it there for reference. Nah. Let's just strip and point to some place where V4L1 doc is available, adding some warning that the API is outdated and will be removed from kernel soon. I don't think we should remove the doc from the repo until all drivers are converted to v4l2. I think it would be useful to keep around. Consider someone trying to convert some old app or driver from v4l1 to v4l2. It would be useful for them to have the spec for v4l1 to know what the software was trying to do. You could just point somewhere else, but things move, and that's another link that will need to be kept up to date. And there could be updates to it, like common ways that v4l1 apps violate the spec that you need to deal with when converting to v4l2. -- 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: [PULL] http://www.linuxtv.org/hg/~hverkuil/v4l-dvb-notify
On Mon, 9 Mar 2009, Mauro Carvalho Chehab wrote: On Mon, 9 Mar 2009 08:45:42 +0100 Hans Verkuil hverk...@xs4all.nl wrote: On Monday 09 March 2009 02:20:19 Trent Piepho wrote: On Sun, 8 Mar 2009, Hans Verkuil wrote: - zoran/bt819: use new notify functionality. You put compat.h in the wrong spot in this patch. It goes before any header file that are in v4l-dvb, but you've moved it to after v4l2-common. That's not what README.patches says: There are several compatibility procedures already defined in compat.h file. If needed, it is better to include it after all other kernel standard headers, and before any specific header for that file. Something like: The docs are wrong then. I'm the one who came up with the current system for placing compat.h, I think I know how I designed it. It should come after files that come from the kernel source (i.e., header files that are from the old kernel) and before any headers that are in v4l-dvb hg (i.e., headers that are the same no matter what kernel). This way we can take a type like struct delayed_work and use a #define to change that into struct work_struct when compiling on a kernel where the structure still had that name. Since all v4l-dvb code, even headers like media/v4l-common.h, will use the current struct delayed_work, compat.h must come before them for the #define to fix the struct name. We also use this for the common problem of a function gaining or losing an argument. If the macro comes before the funtion is defined in the header from the kernel source then it would mess the function prototype up. should be included at the files under v4l-dvb tree. This header also includes linux/version.h. I changed the build system long ago to include version.h via a gcc command line option. This way you can stick a kernel version compat ifdef anywhere in the file, for instance not including mutex.h was a common thing at the beginning of files when we had compat for pre-mutex kernels, and not have to worry about putting version.h or compat.h before the ifdef. -- 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: [PULL] http://www.linuxtv.org/hg/~hverkuil/v4l-dvb
On Tue, 10 Mar 2009, Hans Verkuil wrote: On Tuesday 10 March 2009 00:50:41 Mauro Carvalho Chehab wrote: On Mon, 9 Mar 2009 08:16:53 +0100 Hans Verkuil hverk...@xs4all.nl wrote: On Monday 09 March 2009 02:07:33 Trent Piepho wrote: On Sun, 8 Mar 2009, Hans Verkuil wrote: The last one fixes an ivtv regression caused by this change: changeset: 10811:0a0eba8e64d5 user:Trent Piepho xy...@speakeasy.org date:Tue Mar 03 20:21:02 2009 -0800 summary: videodev: only copy needed part of RW ioctl's parameter The fix is simple: switch on the full ioctl command instead of just the NR field. Thanks to Martin Dauskardt for doing the bisect and tracing the breakage to this change. Switching on the whole ioctl makes the switch statement a lot less efficient. I'd rather just put a if (_IOC_TYPE(cmd) != 'V') return 0; in there. That should fix the non-v4l2 ioctls, right? That was my first thought as well, but I realized that this is one area where you really do not want to take any risk. IMO, it is better to use Trent's way, and reduce the number of places that do memset(0). Personally I think this code is overoptimization. In my view the performance advantage is minimal while reducing the readability of the code. In general, it is a good idea to avoid copying a data that won't be used from userspace. I liked the optimizations done. Let's just fix what's broken and test a lot to avoid causing a kernel regression. I strongly suspect that the extra function call and tests involved takes longer than the initial copy_from_user and memset. Perhaps with the exception of G_FMT, which is really the only one that has a non-trivial amount of data to copy. None of the commands involved are high-performance commands (well, we haven't any of those anyway), nor is a tight inner-loop involved. I'll see if I can run some benchmarks, but I remain convinced that this change has no benefit. If you bothered to read my commit messages and look at the diff stat, you'd see that this really wasn't about saving a few cycles. That's just a nice extra bonus. Code that was scattered thoughout the tree to clear struct fields was replaced with *less* code that is all in one place. It's not a more complicated solution, it's a simpler one. What's more, duplicating struct clearing over and over leads to bugs. For instance, there were two ioctls that were broken because important fields passed from userspace were mistakenly cleared before the driver got them. I mentioned that in my commit message. There are also many many places where drivers did not clear fields that they should have, leaving whatever garbage was in there. Now all those problems are fixed and driver authors don't have to worry about clearing fields and which fields they should clear. -- 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
