Re: [PATCH 3/4] ir-core: move decoding state to ir_raw_event_ctrl
On Mon, Jun 21, 2010 at 7:04 AM, Andy Walls awa...@md.metrocast.net wrote: On Sun, 2010-06-20 at 23:51 -0400, Jarod Wilson wrote: On Sun, Jun 20, 2010 at 8:47 PM, Andy Walls awa...@md.metrocast.net wrote: As for the building your own kernel thing... I've been doing my work mainly on a pair of x86_64 systems, one a ThinkPad T61 running Fedora 13, and the other an HP xw4400 workstation running RHEL6. In both cases, I copied a distro kernel's, config file out of /boot/, and then ran make oldconfig over it and build straight from what's in my tree, which works well enough on both setups. I pulled the config out of the kernel*.src.rpm after 'rpmbuild -bp' (I only saw the configs in /boot after doing that :P ), and then ran 'make oldconfig'. The first time around I forgot to do a modules_install and the ext[234] modules weren't in the initramfs. That made it hard for the kernel to read the /boot and / filesystems. ;) After fixing that idiocy, it now hangs in early boot - just a blinking cursor. I'm speculating it is a problem with support for my old-ish ATI Radeon Xpress 200 video chipset with a vanilla kernel. I'll work it out eventually. Seems you've already gotten around this, but it did remind me of someone on one of the fedora mailing lists who said w/their older radeon hardware, they could only boot recent kernels if the passed in nomodeset=1 or something like that. I'll have time on Thursday night to try again. No rush yet, we've got a while before the merge window still. Christoph (Bartelmus) helped me out with a bunch of ioctl documentation this weekend, so I've got that to add to the tree, then I think I'll be prepared to resubmit the lirc bits. I'll shoot for doing that next weekend, and hopefully, that'll give you a chance to try 'em out before then and provide any necessary feedback/fixes/etc. (Not that we can't also just fix things up as needed post-merge). I'm still up in the air as to what I should work on next... So many lirc drivers left to port still... Maybe zilog, maybe streamzap... Maybe the MCE IR keyboard... I've got a PVR-150 and HVR-1600 both with the Zilog Z8's on them. If I ever get my act together, I'll at least be able to test that and integrate any changes into the ivtv cx18 drivers. I've recently seen users having trouble on IRC, BTW. Yeah, me too. Hoping to dig into porting (and fixing) lirc_zilog RSN... -- Jarod Wilson ja...@wilsonet.com -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] ir-core: move decoding state to ir_raw_event_ctrl
On Sun, 2010-06-20 at 23:51 -0400, Jarod Wilson wrote: On Sun, Jun 20, 2010 at 8:47 PM, Andy Walls awa...@md.metrocast.net wrote: As for the building your own kernel thing... I've been doing my work mainly on a pair of x86_64 systems, one a ThinkPad T61 running Fedora 13, and the other an HP xw4400 workstation running RHEL6. In both cases, I copied a distro kernel's, config file out of /boot/, and then ran make oldconfig over it and build straight from what's in my tree, which works well enough on both setups. I pulled the config out of the kernel*.src.rpm after 'rpmbuild -bp' (I only saw the configs in /boot after doing that :P ), and then ran 'make oldconfig'. The first time around I forgot to do a modules_install and the ext[234] modules weren't in the initramfs. That made it hard for the kernel to read the /boot and / filesystems. ;) After fixing that idiocy, it now hangs in early boot - just a blinking cursor. I'm speculating it is a problem with support for my old-ish ATI Radeon Xpress 200 video chipset with a vanilla kernel. I'll work it out eventually. I'll have time on Thursday night to try again. No rush yet, we've got a while before the merge window still. Christoph (Bartelmus) helped me out with a bunch of ioctl documentation this weekend, so I've got that to add to the tree, then I think I'll be prepared to resubmit the lirc bits. I'll shoot for doing that next weekend, and hopefully, that'll give you a chance to try 'em out before then and provide any necessary feedback/fixes/etc. (Not that we can't also just fix things up as needed post-merge). I'm still up in the air as to what I should work on next... So many lirc drivers left to port still... Maybe zilog, maybe streamzap... Maybe the MCE IR keyboard... I've got a PVR-150 and HVR-1600 both with the Zilog Z8's on them. If I ever get my act together, I'll at least be able to test that and integrate any changes into the ivtv cx18 drivers. I've recently seen users having trouble on IRC, BTW. Regards, Andy -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] ir-core: move decoding state to ir_raw_event_ctrl
On Sun, Jun 20, 2010 at 8:47 PM, Andy Walls awa...@md.metrocast.net wrote: On Thu, 2010-06-17 at 11:11 -0400, Jarod Wilson wrote: On Thu, Jun 17, 2010 at 8:14 AM, Andy Walls awa...@md.metrocast.net wrote: A fully functional tree carrying both of David's patches and the entire stack of other patches I've submitted today, based on top of the linuxtv staging/rc branch, can be found here: http://git.wilsonet.com/linux-2.6-ir-wip.git/?a=shortlog;h=refs/heads/patches Also includes the lirc patches that I believe are ready to be submitted for actual consideration (note that they're dependent on David's two patches). I'll try and play with this this weekend along with some cx23885 cleanup. Excellent. A few things to note... Jarrod, I was unable to get this task completed in the time I had available this weekend. A power supply failure, unexpected hard drive replacement, and my inability to build/install a kernel from a git tree that would actually boot my Fedora 12 installation didn't help. (My productivity has tanked since v4l-dvb went to GIT for CM, and the last time I built a real kernel without rpmbuild was for RedHat 9. I'm still working out processes for doing basic things, sorry.) Heh, yeah, hardware failure is always fun when you're trying really hard to get something done. :) As for the building your own kernel thing... I've been doing my work mainly on a pair of x86_64 systems, one a ThinkPad T61 running Fedora 13, and the other an HP xw4400 workstation running RHEL6. In both cases, I copied a distro kernel's, config file out of /boot/, and then ran make oldconfig over it and build straight from what's in my tree, which works well enough on both setups. I'll have time on Thursday night to try again. No rush yet, we've got a while before the merge window still. Christoph (Bartelmus) helped me out with a bunch of ioctl documentation this weekend, so I've got that to add to the tree, then I think I'll be prepared to resubmit the lirc bits. I'll shoot for doing that next weekend, and hopefully, that'll give you a chance to try 'em out before then and provide any necessary feedback/fixes/etc. (Not that we can't also just fix things up as needed post-merge). I'm still up in the air as to what I should work on next... So many lirc drivers left to port still... Maybe zilog, maybe streamzap... Maybe the MCE IR keyboard... -- Jarod Wilson ja...@wilsonet.com -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] ir-core: move decoding state to ir_raw_event_ctrl
On Wed, 2010-06-16 at 16:41 -0400, Jarod Wilson wrote: On Wed, Jun 16, 2010 at 4:04 PM, Jarod Wilson ja...@wilsonet.com wrote: ... I have another suggestion, let's keep the client register/unregister callbacks for decoders (but add a comment that they're only used for lirc). Then teach drivers/media/IR/ir-raw-event.c to keep track of the raw clients so that it can pass all pre-existing clients to newly added decoders. I'll post two patches (compile tested only) in a few seconds to show what I mean. Consider them now runtime tested as well. They appear to do the trick, the lirc bridge comes up just fine, even when ir-lirc-codec isn't loaded until after mceusb. *Much* better implementation than my ugly trick. I'll ack your patches and submit a series on top of them for lirc support, hopefully this evening (in addition to a few other fixes that aren't dependent on any of them). A fully functional tree carrying both of David's patches and the entire stack of other patches I've submitted today, based on top of the linuxtv staging/rc branch, can be found here: http://git.wilsonet.com/linux-2.6-ir-wip.git/?a=shortlog;h=refs/heads/patches Also includes the lirc patches that I believe are ready to be submitted for actual consideration (note that they're dependent on David's two patches). I'll try and play with this this weekend along with some cx23885 cleanup. Regards, Andy -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] ir-core: move decoding state to ir_raw_event_ctrl
On Thu, Jun 17, 2010 at 8:14 AM, Andy Walls awa...@md.metrocast.net wrote: On Wed, 2010-06-16 at 16:41 -0400, Jarod Wilson wrote: On Wed, Jun 16, 2010 at 4:04 PM, Jarod Wilson ja...@wilsonet.com wrote: ... I have another suggestion, let's keep the client register/unregister callbacks for decoders (but add a comment that they're only used for lirc). Then teach drivers/media/IR/ir-raw-event.c to keep track of the raw clients so that it can pass all pre-existing clients to newly added decoders. I'll post two patches (compile tested only) in a few seconds to show what I mean. Consider them now runtime tested as well. They appear to do the trick, the lirc bridge comes up just fine, even when ir-lirc-codec isn't loaded until after mceusb. *Much* better implementation than my ugly trick. I'll ack your patches and submit a series on top of them for lirc support, hopefully this evening (in addition to a few other fixes that aren't dependent on any of them). A fully functional tree carrying both of David's patches and the entire stack of other patches I've submitted today, based on top of the linuxtv staging/rc branch, can be found here: http://git.wilsonet.com/linux-2.6-ir-wip.git/?a=shortlog;h=refs/heads/patches Also includes the lirc patches that I believe are ready to be submitted for actual consideration (note that they're dependent on David's two patches). I'll try and play with this this weekend along with some cx23885 cleanup. Excellent. A few things to note... Many of the lirc_dev ioctls are currently commented out, and haven't in any way been wired up to tx callbacks, I've only enabled the minimum necessary for mceusb. The ioctls are all using __u32 params, which, if you're on x86_64, will require a patched lirc userspace build to make the ioctl types match. I'm using this patch atm: http://wilsonet.com/jarod/lirc_misc/lirc-0.8.6-make-ioctls-u32.patch (In the future, lirc userspace should obviously just build against media/lirc.h). -- Jarod Wilson ja...@wilsonet.com -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] ir-core: move decoding state to ir_raw_event_ctrl
On Sun, Jun 13, 2010 at 4:29 PM, David Härdeman da...@hardeman.nu wrote: On Wed, Jun 09, 2010 at 09:25:44PM -0400, Jarod Wilson wrote: On Wed, Jun 9, 2010 at 2:15 PM, Jarod Wilson ja...@redhat.com wrote: On Wed, Jun 09, 2010 at 07:56:21PM +0200, David Härdeman wrote: On Wed, Jun 09, 2010 at 09:29:08AM -0400, Jarod Wilson wrote: ... So this definitely negatively impacts my ir-core-to-lirc_dev (ir-lirc-codec.c) bridge driver, as it was doing the lirc_dev device registration work in its register function. However, if (after your patchset) we add a new pair of callbacks replacing raw_register and raw_unregister, which are optional, that work could be done there instead, so I don't think this is an insurmountable obstacle for the lirc bits. While I'm not sure exactly what callbacks you're suggesting, Essentially: .setup_other_crap .tear_down_other_crap ...which in the ir-lirc-codec case, register ir-lirc-codec for a specific hardware receiver as an lirc_dev client, and conversely, tear it down. it still sounds like the callbacks would have the exact same problems that the current code has (i.e. the decoder will be blissfully unaware of hardware which exists before the decoder is loaded). Right? In my head, this was going to work out, but you're correct, I still have the exact same problem -- its not in ir_raw_handler_list yet when ir_raw_event_register runs, and thus the callback never fires, so lirc_dev never actually gets wired up to ir-lirc-codec. It now knows about the lirc decoder, but its completely useless. Narf. And now I have it working atop your patches. Its a bit of a nasty-ish hack, at least for the lirc case, but its working, even in the case where the decoder drivers aren't actually loaded until after the device driver. I've added one extra param to each protocol-specific struct in ir-core-priv.h (bool initialized) and hooked into the protocol-specific decode functions to both determine whether a protocol should be enabled or disabled by default, and to run any additionally required initialization (such as in the ir-lirc-codec case). So initially, mceusb comes up with all decoders enabled. Then when ir comes in, every protocol-specific decoder fires. Each of them check for whether or not they've been fully initialized, and if not, we check the loaded keymap, and if it doesn't match, we disable that decoder (bringing back the disable protocol handlers we don't need functionality that disappeared w/this patchset). In the lirc case, we actually do all the work needed to wire up the connection over to lirc_dev. This works perfectly fine for all the in-kernel decoders, but has one minor shortcoming for ir-lirc-codec, in that /dev/lirc0 won't actually exist until the first incoming ir signal is seen. lircd can handle this case just fine, it'll wait for /dev/lirc0 to show up, but it doesn't come up fast enough to catch and decode the very first incoming ir signal. Subsequent ones work perfectly fine though. This need to initialize the link via incoming ir is a bit problematic if you're using a device for transmit-only (e.g., and mceusb device hooked to a mythtv backend in the closet or something), as there would be a strong possibility of /dev/lirc0 never getting hooked up. I can think of a few workarounds, but none are particularly clean and/or automagic. Not sure how palatable it is, but here it is: I think it sounds pretty awful :) So my suspicion wrt palatability was correct. :) I have another suggestion, let's keep the client register/unregister callbacks for decoders (but add a comment that they're only used for lirc). Then teach drivers/media/IR/ir-raw-event.c to keep track of the raw clients so that it can pass all pre-existing clients to newly added decoders. I'll post two patches (compile tested only) in a few seconds to show what I mean. Consider them now runtime tested as well. They appear to do the trick, the lirc bridge comes up just fine, even when ir-lirc-codec isn't loaded until after mceusb. *Much* better implementation than my ugly trick. I'll ack your patches and submit a series on top of them for lirc support, hopefully this evening (in addition to a few other fixes that aren't dependent on any of them). -- Jarod Wilson ja...@wilsonet.com -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] ir-core: move decoding state to ir_raw_event_ctrl
On Wed, Jun 16, 2010 at 4:04 PM, Jarod Wilson ja...@wilsonet.com wrote: ... I have another suggestion, let's keep the client register/unregister callbacks for decoders (but add a comment that they're only used for lirc). Then teach drivers/media/IR/ir-raw-event.c to keep track of the raw clients so that it can pass all pre-existing clients to newly added decoders. I'll post two patches (compile tested only) in a few seconds to show what I mean. Consider them now runtime tested as well. They appear to do the trick, the lirc bridge comes up just fine, even when ir-lirc-codec isn't loaded until after mceusb. *Much* better implementation than my ugly trick. I'll ack your patches and submit a series on top of them for lirc support, hopefully this evening (in addition to a few other fixes that aren't dependent on any of them). A fully functional tree carrying both of David's patches and the entire stack of other patches I've submitted today, based on top of the linuxtv staging/rc branch, can be found here: http://git.wilsonet.com/linux-2.6-ir-wip.git/?a=shortlog;h=refs/heads/patches Also includes the lirc patches that I believe are ready to be submitted for actual consideration (note that they're dependent on David's two patches). -- Jarod Wilson ja...@wilsonet.com -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] ir-core: move decoding state to ir_raw_event_ctrl
On Wed, Jun 09, 2010 at 09:25:44PM -0400, Jarod Wilson wrote: On Wed, Jun 9, 2010 at 2:15 PM, Jarod Wilson ja...@redhat.com wrote: On Wed, Jun 09, 2010 at 07:56:21PM +0200, David Härdeman wrote: On Wed, Jun 09, 2010 at 09:29:08AM -0400, Jarod Wilson wrote: ... So this definitely negatively impacts my ir-core-to-lirc_dev (ir-lirc-codec.c) bridge driver, as it was doing the lirc_dev device registration work in its register function. However, if (after your patchset) we add a new pair of callbacks replacing raw_register and raw_unregister, which are optional, that work could be done there instead, so I don't think this is an insurmountable obstacle for the lirc bits. While I'm not sure exactly what callbacks you're suggesting, Essentially: .setup_other_crap .tear_down_other_crap ...which in the ir-lirc-codec case, register ir-lirc-codec for a specific hardware receiver as an lirc_dev client, and conversely, tear it down. it still sounds like the callbacks would have the exact same problems that the current code has (i.e. the decoder will be blissfully unaware of hardware which exists before the decoder is loaded). Right? In my head, this was going to work out, but you're correct, I still have the exact same problem -- its not in ir_raw_handler_list yet when ir_raw_event_register runs, and thus the callback never fires, so lirc_dev never actually gets wired up to ir-lirc-codec. It now knows about the lirc decoder, but its completely useless. Narf. And now I have it working atop your patches. Its a bit of a nasty-ish hack, at least for the lirc case, but its working, even in the case where the decoder drivers aren't actually loaded until after the device driver. I've added one extra param to each protocol-specific struct in ir-core-priv.h (bool initialized) and hooked into the protocol-specific decode functions to both determine whether a protocol should be enabled or disabled by default, and to run any additionally required initialization (such as in the ir-lirc-codec case). So initially, mceusb comes up with all decoders enabled. Then when ir comes in, every protocol-specific decoder fires. Each of them check for whether or not they've been fully initialized, and if not, we check the loaded keymap, and if it doesn't match, we disable that decoder (bringing back the disable protocol handlers we don't need functionality that disappeared w/this patchset). In the lirc case, we actually do all the work needed to wire up the connection over to lirc_dev. This works perfectly fine for all the in-kernel decoders, but has one minor shortcoming for ir-lirc-codec, in that /dev/lirc0 won't actually exist until the first incoming ir signal is seen. lircd can handle this case just fine, it'll wait for /dev/lirc0 to show up, but it doesn't come up fast enough to catch and decode the very first incoming ir signal. Subsequent ones work perfectly fine though. This need to initialize the link via incoming ir is a bit problematic if you're using a device for transmit-only (e.g., and mceusb device hooked to a mythtv backend in the closet or something), as there would be a strong possibility of /dev/lirc0 never getting hooked up. I can think of a few workarounds, but none are particularly clean and/or automagic. Not sure how palatable it is, but here it is: I think it sounds pretty awful :) I have another suggestion, let's keep the client register/unregister callbacks for decoders (but add a comment that they're only used for lirc). Then teach drivers/media/IR/ir-raw-event.c to keep track of the raw clients so that it can pass all pre-existing clients to newly added decoders. I'll post two patches (compile tested only) in a few seconds to show what I mean. -- David Härdeman -- 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] ir-core: move decoding state to ir_raw_event_ctrl
On Wed, Jun 09, 2010 at 09:29:08AM -0400, Jarod Wilson wrote: On Tue, Jun 08, 2010 at 11:46:36PM -0400, Jarod Wilson wrote: On Tue, Jun 8, 2010 at 1:50 PM, David Härdeman da...@hardeman.nu wrote: b) Mauro mentioned in 4bdf28c0.4060...@redhat.com that: I liked the idea of your redesign, but I didn't like the removal of a per-decoder sysfs entry. As already discussed, there are cases where we'll need a per-decoder sysfs entry (lirc_dev is probably one of those cases - also Jarod's imon driver is currently implementing a modprobe parameter that needs to be moved to the driver). could you please confirm if your lirc and/or imon drivers would be negatively affected by the proposed patches? Will do so once I get them wedged in on top. Got it all merged and compiling, but not yet runtime tested. Compiling alone sheds some light on things though... So this definitely negatively impacts my ir-core-to-lirc_dev (ir-lirc-codec.c) bridge driver, as it was doing the lirc_dev device registration work in its register function. However, if (after your patchset) we add a new pair of callbacks replacing raw_register and raw_unregister, which are optional, that work could be done there instead, so I don't think this is an insurmountable obstacle for the lirc bits. While I'm not sure exactly what callbacks you're suggesting, it still sounds like the callbacks would have the exact same problems that the current code has (i.e. the decoder will be blissfully unaware of hardware which exists before the decoder is loaded). Right? As for the imon driver, the modprobe parameter in question (iirc) was already removed from the driver, as its functionality is replaced by implementing a change_protocol callback. However, there's a request to add it (or something like it) back to the driver to allow disabling the IR part altogether, and there are a few other modparams that might be better suited as sysfs entries. However, its actually not relevant to the case of registering raw protocol handlers, as the imon devices do their decoding in hardware. I can see the possibility for protocol-specific knobs in sysfs though. But I think the same optional callbacks I'd use to keep the lirc bits working could also be used for this. Can't think of a good name for these yet, probably need more coffee first... ;) But those sysfs entries wouldn't be per-decoder-per-hardware-devicethey'd just be per-hardware-device...right? -- David Härdeman -- 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] ir-core: move decoding state to ir_raw_event_ctrl
On Wed, Jun 09, 2010 at 07:56:21PM +0200, David Härdeman wrote: On Wed, Jun 09, 2010 at 09:29:08AM -0400, Jarod Wilson wrote: On Tue, Jun 08, 2010 at 11:46:36PM -0400, Jarod Wilson wrote: On Tue, Jun 8, 2010 at 1:50 PM, David Härdeman da...@hardeman.nu wrote: b) Mauro mentioned in 4bdf28c0.4060...@redhat.com that: I liked the idea of your redesign, but I didn't like the removal of a per-decoder sysfs entry. As already discussed, there are cases where we'll need a per-decoder sysfs entry (lirc_dev is probably one of those cases - also Jarod's imon driver is currently implementing a modprobe parameter that needs to be moved to the driver). could you please confirm if your lirc and/or imon drivers would be negatively affected by the proposed patches? Will do so once I get them wedged in on top. Got it all merged and compiling, but not yet runtime tested. Compiling alone sheds some light on things though... So this definitely negatively impacts my ir-core-to-lirc_dev (ir-lirc-codec.c) bridge driver, as it was doing the lirc_dev device registration work in its register function. However, if (after your patchset) we add a new pair of callbacks replacing raw_register and raw_unregister, which are optional, that work could be done there instead, so I don't think this is an insurmountable obstacle for the lirc bits. While I'm not sure exactly what callbacks you're suggesting, Essentially: .setup_other_crap .tear_down_other_crap ...which in the ir-lirc-codec case, register ir-lirc-codec for a specific hardware receiver as an lirc_dev client, and conversely, tear it down. it still sounds like the callbacks would have the exact same problems that the current code has (i.e. the decoder will be blissfully unaware of hardware which exists before the decoder is loaded). Right? In my head, this was going to work out, but you're correct, I still have the exact same problem -- its not in ir_raw_handler_list yet when ir_raw_event_register runs, and thus the callback never fires, so lirc_dev never actually gets wired up to ir-lirc-codec. It now knows about the lirc decoder, but its completely useless. Narf. As for the imon driver, the modprobe parameter in question (iirc) was already removed from the driver, as its functionality is replaced by implementing a change_protocol callback. However, there's a request to add it (or something like it) back to the driver to allow disabling the IR part altogether, and there are a few other modparams that might be better suited as sysfs entries. However, its actually not relevant to the case of registering raw protocol handlers, as the imon devices do their decoding in hardware. I can see the possibility for protocol-specific knobs in sysfs though. But I think the same optional callbacks I'd use to keep the lirc bits working could also be used for this. Can't think of a good name for these yet, probably need more coffee first... ;) But those sysfs entries wouldn't be per-decoder-per-hardware-devicethey'd just be per-hardware-device...right? Most likely. But I think its possible someone would want to want to tweak some parameter that is both protocol and hardware device specific. Just sheer speculation at the moment though, I don't have a concrete example. -- Jarod Wilson ja...@redhat.com -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] ir-core: move decoding state to ir_raw_event_ctrl
On Wed, Jun 9, 2010 at 2:15 PM, Jarod Wilson ja...@redhat.com wrote: On Wed, Jun 09, 2010 at 07:56:21PM +0200, David Härdeman wrote: On Wed, Jun 09, 2010 at 09:29:08AM -0400, Jarod Wilson wrote: ... So this definitely negatively impacts my ir-core-to-lirc_dev (ir-lirc-codec.c) bridge driver, as it was doing the lirc_dev device registration work in its register function. However, if (after your patchset) we add a new pair of callbacks replacing raw_register and raw_unregister, which are optional, that work could be done there instead, so I don't think this is an insurmountable obstacle for the lirc bits. While I'm not sure exactly what callbacks you're suggesting, Essentially: .setup_other_crap .tear_down_other_crap ...which in the ir-lirc-codec case, register ir-lirc-codec for a specific hardware receiver as an lirc_dev client, and conversely, tear it down. it still sounds like the callbacks would have the exact same problems that the current code has (i.e. the decoder will be blissfully unaware of hardware which exists before the decoder is loaded). Right? In my head, this was going to work out, but you're correct, I still have the exact same problem -- its not in ir_raw_handler_list yet when ir_raw_event_register runs, and thus the callback never fires, so lirc_dev never actually gets wired up to ir-lirc-codec. It now knows about the lirc decoder, but its completely useless. Narf. And now I have it working atop your patches. Its a bit of a nasty-ish hack, at least for the lirc case, but its working, even in the case where the decoder drivers aren't actually loaded until after the device driver. I've added one extra param to each protocol-specific struct in ir-core-priv.h (bool initialized) and hooked into the protocol-specific decode functions to both determine whether a protocol should be enabled or disabled by default, and to run any additionally required initialization (such as in the ir-lirc-codec case). So initially, mceusb comes up with all decoders enabled. Then when ir comes in, every protocol-specific decoder fires. Each of them check for whether or not they've been fully initialized, and if not, we check the loaded keymap, and if it doesn't match, we disable that decoder (bringing back the disable protocol handlers we don't need functionality that disappeared w/this patchset). In the lirc case, we actually do all the work needed to wire up the connection over to lirc_dev. This works perfectly fine for all the in-kernel decoders, but has one minor shortcoming for ir-lirc-codec, in that /dev/lirc0 won't actually exist until the first incoming ir signal is seen. lircd can handle this case just fine, it'll wait for /dev/lirc0 to show up, but it doesn't come up fast enough to catch and decode the very first incoming ir signal. Subsequent ones work perfectly fine though. This need to initialize the link via incoming ir is a bit problematic if you're using a device for transmit-only (e.g., and mceusb device hooked to a mythtv backend in the closet or something), as there would be a strong possibility of /dev/lirc0 never getting hooked up. I can think of a few workarounds, but none are particularly clean and/or automagic. Not sure how palatable it is, but here it is: http://git.wilsonet.com/linux-2.6-ir-wip.git/?a=commitdiff;h=8f2be576ecb82448daa0c0d789bf0c978c66b103 -- Jarod Wilson ja...@wilsonet.com -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] ir-core: move decoding state to ir_raw_event_ctrl
On Mon, Jun 07, 2010 at 04:15:30PM -0400, Jarod Wilson wrote:
On Mon, Jun 07, 2010 at 09:00:03PM +0200, David Härdeman wrote:
On Mon, May 03, 2010 at 05:00:05PM -0300, Mauro Carvalho Chehab wrote:
David Härdeman wrote:
This patch moves the state from each raw decoder into the
ir_raw_event_ctrl struct.
This allows the removal of code like this:
spin_lock(decoder_lock);
list_for_each_entry(data, decoder_list, list) {
if (data-ir_dev == ir_dev)
break;
}
spin_unlock(decoder_lock);
return data;
which is currently run for each decoder on each event in order
to get the client-specific decoding state data.
In addition, ir decoding modules and ir driver module load
order is now independent. Centralizing the data also allows
for a nice code reduction of about 30% per raw decoder as
client lists and client registration callbacks are no longer
necessary.
The registration callbacks will likely still be needed by lirc,
as you need to create/delete lirc_dev interfaces, when the module
is registered, but I might be wrong. It would be interesting to
add lirc_dev first, in order to be sure about the better interfaces
for it.
Or the lirc_dev patch can add whatever interfaces it needs. Anyway, the
current interfaces are not good enough since it'll break if lirc_dev is
loaded after the hardware modules.
This is something I've been meaning to mention myself. On system boot, if
an mceusb device is connected, it pretty regularly only has the NEC
decoder available to use. I have to reload mceusb, or make sure ir-core is
explicitly loaded, wait a bit, then load mceusb, if I want to have all of
the protocol handlers available -- which includes the needed-by-default
rc6 one. I've only briefly tinkered with trying to fix it, sounds like you
may already have fixage within this patchset.
The problem is that without the patchset, each decoder is expected to
carry it's own list of datastructures for each hardware receiver.
Hardware receiver addition/removal is signalled through a callback to
the decoder, but the callback will (naturally) not be invoked if the
hardware driver is already loaded when the decoder is. So loading a
decoder late or reloading a decoder will mean that it doesn't know
about pre-existing hardware.
In addition, random module load order is currently broken (try loading
decoders first and hardware later and you'll see). With this patch, it
works again.
Want.
Then please help me with two things:
a) Test the patches I just sent (especially 6/8 and 7/8, they should
be independent from the rest)
b) Mauro mentioned in 4bdf28c0.4060...@redhat.com that:
I liked the idea of your redesign, but I didn't like the removal
of a per-decoder sysfs entry. As already discussed, there are
cases where we'll need a per-decoder sysfs entry (lirc_dev is
probably one of those cases - also Jarod's imon driver is
currently implementing a modprobe parameter that needs to be
moved to the driver).
could you please confirm if your lirc and/or imon drivers would be
negatively affected by the proposed patches?
--
David Härdeman
--
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] ir-core: move decoding state to ir_raw_event_ctrl
On Tue, Jun 8, 2010 at 1:50 PM, David Härdeman da...@hardeman.nu wrote:
On Mon, Jun 07, 2010 at 04:15:30PM -0400, Jarod Wilson wrote:
On Mon, Jun 07, 2010 at 09:00:03PM +0200, David Härdeman wrote:
On Mon, May 03, 2010 at 05:00:05PM -0300, Mauro Carvalho Chehab wrote:
David Härdeman wrote:
This patch moves the state from each raw decoder into the
ir_raw_event_ctrl struct.
This allows the removal of code like this:
spin_lock(decoder_lock);
list_for_each_entry(data, decoder_list, list) {
if (data-ir_dev == ir_dev)
break;
}
spin_unlock(decoder_lock);
return data;
which is currently run for each decoder on each event in order
to get the client-specific decoding state data.
In addition, ir decoding modules and ir driver module load
order is now independent. Centralizing the data also allows
for a nice code reduction of about 30% per raw decoder as
client lists and client registration callbacks are no longer
necessary.
The registration callbacks will likely still be needed by lirc,
as you need to create/delete lirc_dev interfaces, when the module
is registered, but I might be wrong. It would be interesting to
add lirc_dev first, in order to be sure about the better interfaces
for it.
Or the lirc_dev patch can add whatever interfaces it needs. Anyway, the
current interfaces are not good enough since it'll break if lirc_dev is
loaded after the hardware modules.
This is something I've been meaning to mention myself. On system boot, if
an mceusb device is connected, it pretty regularly only has the NEC
decoder available to use. I have to reload mceusb, or make sure ir-core is
explicitly loaded, wait a bit, then load mceusb, if I want to have all of
the protocol handlers available -- which includes the needed-by-default
rc6 one. I've only briefly tinkered with trying to fix it, sounds like you
may already have fixage within this patchset.
The problem is that without the patchset, each decoder is expected to
carry it's own list of datastructures for each hardware receiver.
Hardware receiver addition/removal is signalled through a callback to
the decoder, but the callback will (naturally) not be invoked if the
hardware driver is already loaded when the decoder is. So loading a
decoder late or reloading a decoder will mean that it doesn't know
about pre-existing hardware.
In addition, random module load order is currently broken (try loading
decoders first and hardware later and you'll see). With this patch, it
works again.
Want.
Then please help me with two things:
a) Test the patches I just sent (especially 6/8 and 7/8, they should
be independent from the rest)
Working on merging them into a tree here locally. There's been a bit
of churn, so the last few didn't apply cleanly, but I'm almost there.
b) Mauro mentioned in 4bdf28c0.4060...@redhat.com that:
I liked the idea of your redesign, but I didn't like the removal
of a per-decoder sysfs entry. As already discussed, there are
cases where we'll need a per-decoder sysfs entry (lirc_dev is
probably one of those cases - also Jarod's imon driver is
currently implementing a modprobe parameter that needs to be
moved to the driver).
could you please confirm if your lirc and/or imon drivers would be
negatively affected by the proposed patches?
Will do so once I get them wedged in on top.
--
Jarod Wilson
ja...@wilsonet.com
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] ir-core: move decoding state to ir_raw_event_ctrl
On Mon, May 03, 2010 at 05:00:05PM -0300, Mauro Carvalho Chehab wrote:
David Härdeman wrote:
This patch moves the state from each raw decoder into the
ir_raw_event_ctrl struct.
This allows the removal of code like this:
spin_lock(decoder_lock);
list_for_each_entry(data, decoder_list, list) {
if (data-ir_dev == ir_dev)
break;
}
spin_unlock(decoder_lock);
return data;
which is currently run for each decoder on each event in order
to get the client-specific decoding state data.
In addition, ir decoding modules and ir driver module load
order is now independent. Centralizing the data also allows
for a nice code reduction of about 30% per raw decoder as
client lists and client registration callbacks are no longer
necessary.
The registration callbacks will likely still be needed by lirc,
as you need to create/delete lirc_dev interfaces, when the module
is registered, but I might be wrong. It would be interesting to
add lirc_dev first, in order to be sure about the better interfaces
for it.
Or the lirc_dev patch can add whatever interfaces it needs. Anyway, the
current interfaces are not good enough since it'll break if lirc_dev is
loaded after the hardware modules.
Also, from one side, you reduced the code size, but, on the other hand,
you've increased the memory usage, as now the protocol data will be
stored even for protocols that weren't compiled/loaded.
In 4bbf3309.6020...@infradead.org, Mauro Carvalho Chehab wrote:
Andy Walls wrote:
Encoding pulse vs space with a negative sign, even if now hidden
with macros, is still just using a sign instead of a boolean.
Memory in modern computers (and now microcontrollers) is cheap and
only getting cheaper. Don't give up readability, flexibility, or
mainatainability, for the sake of saving memory.
That was my point since the beginning: the amount of saved memory
doesn't justify the lack of readability.
Are you worried about memory usage now?
Probably, the code size savings are big enough to justify the runtime
memory footprint, at least with the current decoders. Not sure how big
this will become when we add lirc_dev and other decoders that might be
missing.
Right now, the reasonable default is a user machine with one hardware
decoder and with all of the rc-core decoders loaded (cause that's how
his/her distro will set it up). For that machine, the patch will save a
lot of memory, not waste it (~380 lines less code...I'll assure you it's
a net gain).
In addition, random module load order is currently broken (try loading
decoders first and hardware later and you'll see). With this patch, it
works again.
Anyway, I'll post a new patch series this evening and then we can go
back to our regular arguing :)
--
David Härdeman
--
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] ir-core: move decoding state to ir_raw_event_ctrl
On Mon, Jun 07, 2010 at 09:00:03PM +0200, David Härdeman wrote:
On Mon, May 03, 2010 at 05:00:05PM -0300, Mauro Carvalho Chehab wrote:
David Härdeman wrote:
This patch moves the state from each raw decoder into the
ir_raw_event_ctrl struct.
This allows the removal of code like this:
spin_lock(decoder_lock);
list_for_each_entry(data, decoder_list, list) {
if (data-ir_dev == ir_dev)
break;
}
spin_unlock(decoder_lock);
return data;
which is currently run for each decoder on each event in order
to get the client-specific decoding state data.
In addition, ir decoding modules and ir driver module load
order is now independent. Centralizing the data also allows
for a nice code reduction of about 30% per raw decoder as
client lists and client registration callbacks are no longer
necessary.
The registration callbacks will likely still be needed by lirc,
as you need to create/delete lirc_dev interfaces, when the module
is registered, but I might be wrong. It would be interesting to
add lirc_dev first, in order to be sure about the better interfaces
for it.
Or the lirc_dev patch can add whatever interfaces it needs. Anyway, the
current interfaces are not good enough since it'll break if lirc_dev is
loaded after the hardware modules.
This is something I've been meaning to mention myself. On system boot, if
an mceusb device is connected, it pretty regularly only has the NEC
decoder available to use. I have to reload mceusb, or make sure ir-core is
explicitly loaded, wait a bit, then load mceusb, if I want to have all of
the protocol handlers available -- which includes the needed-by-default
rc6 one. I've only briefly tinkered with trying to fix it, sounds like you
may already have fixage within this patchset.
...
In addition, random module load order is currently broken (try loading
decoders first and hardware later and you'll see). With this patch, it
works again.
Want.
Anyway, I'll post a new patch series this evening and then we can go
back to our regular arguing :)
Hey, at least we're making progress too! :)
--
Jarod Wilson
ja...@redhat.com
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] ir-core: move decoding state to ir_raw_event_ctrl
David Härdeman wrote:
This patch moves the state from each raw decoder into the
ir_raw_event_ctrl struct.
This allows the removal of code like this:
spin_lock(decoder_lock);
list_for_each_entry(data, decoder_list, list) {
if (data-ir_dev == ir_dev)
break;
}
spin_unlock(decoder_lock);
return data;
which is currently run for each decoder on each event in order
to get the client-specific decoding state data.
In addition, ir decoding modules and ir driver module load
order is now independent. Centralizing the data also allows
for a nice code reduction of about 30% per raw decoder as
client lists and client registration callbacks are no longer
necessary.
The registration callbacks will likely still be needed by lirc,
as you need to create/delete lirc_dev interfaces, when the module
is registered, but I might be wrong. It would be interesting to
add lirc_dev first, in order to be sure about the better interfaces
for it.
Also, from one side, you reduced the code size, but, on the other hand,
you've increased the memory usage, as now the protocol data will be
stored even for protocols that weren't compiled/loaded. Probably, the
code size savings are big enough to justify the runtime memory footprint,
at least with the current decoders. Not sure how big this will become
when we add lirc_dev and other decoders that might be missing.
Out-of-tree modules can still use a similar trick to what
the raw decoders did before this patch until they are merged.
Signed-off-by: David Härdeman da...@hardeman.nu
---
drivers/media/IR/ir-core-priv.h| 37 -
drivers/media/IR/ir-jvc-decoder.c | 90 ++-
drivers/media/IR/ir-nec-decoder.c | 89 +++
drivers/media/IR/ir-raw-event.c| 48 +++--
drivers/media/IR/ir-rc5-decoder.c | 103
+---
drivers/media/IR/ir-rc6-decoder.c | 92 ++--
drivers/media/IR/ir-sony-decoder.c | 93 +++--
7 files changed, 87 insertions(+), 465 deletions(-)
diff --git a/drivers/media/IR/ir-core-priv.h b/drivers/media/IR/ir-core-priv.h
index 821d012..1e9464a 100644
--- a/drivers/media/IR/ir-core-priv.h
+++ b/drivers/media/IR/ir-core-priv.h
@@ -23,8 +23,6 @@ struct ir_raw_handler {
u64 protocols; /* which are handled by this handler */
int (*decode)(struct input_dev *input_dev, struct ir_raw_event event);
- int (*raw_register)(struct input_dev *input_dev);
- int (*raw_unregister)(struct input_dev *input_dev);
};
struct ir_raw_event_ctrl {
@@ -34,6 +32,41 @@ struct ir_raw_event_ctrl {
enum raw_event_type last_type; /* last event type */
struct input_dev*input_dev; /* pointer to the
parent input_dev */
u64 enabled_protocols; /* enabled raw
protocol decoders */
+
+ /* raw decoder state follows */
+ struct ir_raw_event prev_ev;
+ struct nec_dec {
+ int state;
+ unsigned count;
+ u32 bits;
+ } nec;
+ struct rc5_dec {
+ int state;
+ u32 bits;
+ unsigned count;
+ unsigned wanted_bits;
+ } rc5;
+ struct rc6_dec {
+ int state;
+ u8 header;
+ u32 body;
+ bool toggle;
+ unsigned count;
+ unsigned wanted_bits;
+ } rc6;
+ struct sony_dec {
+ int state;
+ u32 bits;
+ unsigned count;
+ } sony;
+ struct jvc_dec {
+ int state;
+ u16 bits;
+ u16 old_bits;
+ unsigned count;
+ bool first;
+ bool toggle;
+ } jvc;
};
/* macros for IR decoders */
diff --git a/drivers/media/IR/ir-jvc-decoder.c
b/drivers/media/IR/ir-jvc-decoder.c
index 1055de4..8894d8b 100644
--- a/drivers/media/IR/ir-jvc-decoder.c
+++ b/drivers/media/IR/ir-jvc-decoder.c
@@ -25,10 +25,6 @@
#define JVC_TRAILER_PULSE(1 * JVC_UNIT)
#define JVC_TRAILER_SPACE (35 * JVC_UNIT)
-/* Used to register jvc_decoder clients */
-static LIST_HEAD(decoder_list);
-DEFINE_SPINLOCK(decoder_lock);
-
enum jvc_state {
STATE_INACTIVE,
STATE_HEADER_SPACE,
@@ -38,39 +34,6 @@ enum jvc_state {
STATE_TRAILER_SPACE,
};
-struct decoder_data {
- struct list_headlist;
- struct ir_input_dev *ir_dev;
-
- /* State machine control */
- enum jvc_state state;
- u16 jvc_bits;
- u16 jvc_old_bits;
- unsignedcount;
- boolfirst;
- booltoggle;
-};
-
-
-/**
- * get_decoder_data()- gets decoder
