[alsa-devel] Splitting up the HDA-INTEL modules
Hi all,
when I looked today through the files of the snd-hda-intel module, I was a bit shocked by this complex amount of code (patch_realtek.c for example). I would think about splitting up the code in a way each chipset gets its own .c file - which will make debugging, patching or editing a lot easier IMO, and for second people can then comment on just their specific chipset in config.
What do you think about this?
Marco
PS: If there are no objections, I'll start working on realtek.c on weekend.
I don't think that it would be very easy, as there are several instances where code relies on other codec definitions.
In all actuality, what is needed is a more generic driver that is driven by userspace tools. Takashi and I worked on this a little last year, but we have become too busy with new system influx (and I have other projects outside Alsa consuming my time). Maybe we can get a core group together and actually define and implement an API for this.
My idea is a driver that takes settings from a config file that are either 32bit register values (aka Sigmatel), or individual Node parameters (Realtek, Conexant, etc). This way we can define model config files, and associate their PCI Quirk in a central config file that can be edited in userspace. This would be much easier to support, for users, distro vendors, hardware vendors, and us.
I'll try to write up a proposal in the next few weeks, if we can get others on board to work on this.
Tobin
On Tue, 2008-01-15 at 19:36 +0100, Marco Schuster wrote:
Hi all,
when I looked today through the files of the snd-hda-intel module, I was a bit shocked by this complex amount of code (patch_realtek.c for example). I would think about splitting up the code in a way each chipset gets its own .c file - which will make debugging, patching or editing a lot easier IMO, and for second people can then comment on just their specific chipset in config.
What do you think about this?
Marco
PS: If there are no objections, I'll start working on realtek.c on weekend.
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
At Tue, 15 Jan 2008 11:15:01 -0800, Tobin Davis wrote:
I don't think that it would be very easy, as there are several instances where code relies on other codec definitions.
In all actuality, what is needed is a more generic driver that is driven by userspace tools. Takashi and I worked on this a little last year, but we have become too busy with new system influx (and I have other projects outside Alsa consuming my time).
FYI, the old pieces of codes are found at
ftp://ftp.suse.com/pub/people/tiwai/hda-tools/
Takashi
On Jan 15, 2008 4:36 PM, Marco Schuster marco@harddisk.is-a-geek.org wrote:
when I looked today through the files of the snd-hda-intel module, I was a bit shocked by this complex amount of code (patch_realtek.c for example). I would think about splitting up the code in a way each chipset gets its own .c file - which will make debugging, patching or editing a lot easier IMO, and for second people can then comment on just their specific chipset in config.
What do you think about this?
One interesting development for hda drivers would be to keep the basic infrastructure for each codec in the driver, and configure them from userspace. That would remove all quirks and special cases from the kernel, and allow easier update for users without the need of deploying a new kernel (or new ALSA), just updating the userspace tool.
I am for this idea as well. The whole idea of the hard-coded 'model' definitions is kind of counter-intuitive considering the chipsets allow for dynamic reconfiguration. The kernel driver right now is lacking support to really take advantage of this flexibility.
If this was done, the burden for adding new models could then be shared among non-kernel developers because most people should be capable of playing around with a configuration file/tool until a working configuration is found and then sharing their results. When this requires modifying the driver code, it unfortunately makes all the work for adding new models fall on kernel developers who might be busy with other things.
Also, it should allow for reconfiguration of the ports without re-inserting the kernel module. This would help out in configurations where dynamic modules are not used and a reboot would otherwise be needed.
Could this all be done through a /proc interface? For example, a /proc/asound/hda directory which contained files representing the different connections in the chipset?
Thanks, -Andrew
On Jan 15, 2008 2:20 PM, Claudio Matsuoka cmatsuoka@gmail.com wrote:
On Jan 15, 2008 4:36 PM, Marco Schuster marco@harddisk.is-a-geek.org wrote:
when I looked today through the files of the snd-hda-intel module, I was a bit shocked by this complex amount of code (patch_realtek.c for example). I would think about splitting up the code in a way each chipset gets its own .c file - which will make debugging, patching or editing a lot easier IMO, and for second people can then comment on just their specific chipset in config.
What do you think about this?
One interesting development for hda drivers would be to keep the basic infrastructure for each codec in the driver, and configure them from userspace. That would remove all quirks and special cases from the kernel, and allow easier update for users without the need of deploying a new kernel (or new ALSA), just updating the userspace tool.
Andrew Paprocki schrieb:
Could this all be done through a /proc interface? For example, a /proc/asound/hda directory which contained files representing the different connections in the chipset?
/proc isn't constant, right? Marco
PS: Is it usual here to CC all people on the thread?
Actually, if this were done correctly, it would use the /sys file system for configuration changes. /proc should be read only space.
As it sits now, you should be able to change the model configuration by typing "echo <model> /sys/module/snd_hda_intel/parameters/model", but I don't think it's setup to dynamic changing yet. The /proc/asound/card0/codec# file contains all of the connection configurations for each codec NID. With that and the model diagram from most vendors, it would be easy to reconfigure on the fly.
On Tue, 2008-01-15 at 21:08 +0100, Marco Schuster wrote:
Andrew Paprocki schrieb:
Could this all be done through a /proc interface? For example, a /proc/asound/hda directory which contained files representing the different connections in the chipset?
/proc isn't constant, right? Marco
PS: Is it usual here to CC all people on the thread?
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
At Tue, 15 Jan 2008 13:23:52 -0800, Tobin Davis wrote:
Actually, if this were done correctly, it would use the /sys file system for configuration changes. /proc should be read only space.
As it sits now, you should be able to change the model configuration by typing "echo <model> /sys/module/snd_hda_intel/parameters/model", but I don't think it's setup to dynamic changing yet. The /proc/asound/card0/codec# file contains all of the connection configurations for each codec NID. With that and the model diagram from most vendors, it would be easy to reconfigure on the fly.
The dynamic model change isn't too difficult to implement. But, this alone wouldn't be helpful. It's nothing but reloading the module with a different module option.
The problem is how to pass the proper data *before* setting up the device. So far, the device probe routine is called when the driver is loaded.
Takashi
On Tue, 2008-01-15 at 21:08 +0100, Marco Schuster wrote:
Andrew Paprocki schrieb:
Could this all be done through a /proc interface? For example, a /proc/asound/hda directory which contained files representing the different connections in the chipset?
/proc isn't constant, right? Marco
PS: Is it usual here to CC all people on the thread?
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
-- Tobin Davis
Memory should be the starting point of the present. _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
At Tue, 15 Jan 2008 21:08:49 +0100, Marco Schuster wrote:
Andrew Paprocki schrieb:
Could this all be done through a /proc interface? For example, a /proc/asound/hda directory which contained files representing the different connections in the chipset?
/proc isn't constant, right?
/sys isn't too :)
Marco
PS: Is it usual here to CC all people on the thread?
Yes.
Takashi
At Tue, 15 Jan 2008 17:20:00 -0200, Claudio Matsuoka wrote:
On Jan 15, 2008 4:36 PM, Marco Schuster marco@harddisk.is-a-geek.org wrote:
when I looked today through the files of the snd-hda-intel module, I was a bit shocked by this complex amount of code (patch_realtek.c for example). I would think about splitting up the code in a way each chipset gets its own .c file - which will make debugging, patching or editing a lot easier IMO, and for second people can then comment on just their specific chipset in config.
What do you think about this?
One interesting development for hda drivers would be to keep the basic infrastructure for each codec in the driver, and configure them from userspace. That would remove all quirks and special cases from the kernel, and allow easier update for users without the need of deploying a new kernel (or new ALSA), just updating the userspace tool.
Yes, that's my favarite option.
It's a good question whether the setup should be the codec (driver) dependent or generic. I tend to believe that we'll need some codec-specific workarounds, at least, to keep the same level of the driver support as now.
In my earler development, I implemented some byte-code implementer in the driver so that it can be handled in a generic way. The current codec specific implementation is interpreted to a byte-code in the user-space tool, and it runs on the driver. Of course, this method adds an interpreter to the driver, and I'm also not sure whether this method is optimal.
In theory, we can create a good generic parser with minimal hints. The problem of a generic parser is that it's often fragile to the code changes and the breakage isn't visible in a straight way. The hw-specific implementation is, OTOH, awfully ad hoc, difficult to extend to other hardwares, but it's simple and solid.
Takashi
On Jan 16, 2008 9:54 AM, Takashi Iwai tiwai@suse.de wrote:
It's a good question whether the setup should be the codec (driver) dependent or generic. I tend to believe that we'll need some codec-specific workarounds, at least, to keep the same level of the driver support as now.
In my earler development, I implemented some byte-code implementer in the driver so that it can be handled in a generic way. The current codec specific implementation is interpreted to a byte-code in the user-space tool, and it runs on the driver. Of course, this method adds an interpreter to the driver, and I'm also not sure whether this method is optimal.
I'll have a look at the current hda-tools and see how it works before emmiting any opinion, I am aware that the codec configuration is more complicated than it looks.
At Wed, 16 Jan 2008 10:51:45 -0200, Claudio Matsuoka wrote:
On Jan 16, 2008 9:54 AM, Takashi Iwai tiwai@suse.de wrote:
It's a good question whether the setup should be the codec (driver) dependent or generic. I tend to believe that we'll need some codec-specific workarounds, at least, to keep the same level of the driver support as now.
In my earler development, I implemented some byte-code implementer in the driver so that it can be handled in a generic way. The current codec specific implementation is interpreted to a byte-code in the user-space tool, and it runs on the driver. Of course, this method adds an interpreter to the driver, and I'm also not sure whether this method is optimal.
I'll have a look at the current hda-tools and see how it works before emmiting any opinion, I am aware that the codec configuration is more complicated than it looks.
I put the versoin 0.0.10b now. It includes the patches to the latest ALSA tree but no other changes. ftp://ftp.suse.com/pub/people/tiwai/hda-tools/hda-tools-0.0.10b.tar.bz2
(it might take some time to be exported to the outside.)
Takashi
On Jan 16, 2008 12:54 PM, Takashi Iwai tiwai@suse.de wrote:
I put the versoin 0.0.10b now. It includes the patches to the latest ALSA tree but no other changes. ftp://ftp.suse.com/pub/people/tiwai/hda-tools/hda-tools-0.0.10b.tar.bz2
(it might take some time to be exported to the outside.)
I see that, while in userspace, this is very similar in overall structure to the current kernelspace driver (the file layout being much more organized, however). Have you considered the option to have codec_config_preset for each model specified in a description file that is parsed and sent to the actual driver at runtime, instead of hard-wired inside the library? The parser itself doesn't seem hard to implement, especially if we use the C preprocessor to handle includes and macros. Handling of unsol events would be less flexible, but I don't know if there are many uses for it except muting/unmuting other channels when using external microphones and headphones (and these cases could be well covered by specifying what to mute/unmute in the description file).
The description file could also be very similar to the format currently used in the C source, just making it easier to parse and having one model per file (with common definitions included via preprocessor). I'm not sure how well this would fit for Sigmatel codecs, but seems to be apropriate for Realtek.
At Sat, 19 Jan 2008 17:19:25 -0200, Claudio Matsuoka wrote:
On Jan 16, 2008 12:54 PM, Takashi Iwai tiwai@suse.de wrote:
I put the versoin 0.0.10b now. It includes the patches to the latest ALSA tree but no other changes. ftp://ftp.suse.com/pub/people/tiwai/hda-tools/hda-tools-0.0.10b.tar.bz2
(it might take some time to be exported to the outside.)
I see that, while in userspace, this is very similar in overall structure to the current kernelspace driver (the file layout being much more organized, however). Have you considered the option to have codec_config_preset for each model specified in a description file that is parsed and sent to the actual driver at runtime, instead of hard-wired inside the library? The parser itself doesn't seem hard to implement, especially if we use the C preprocessor to handle includes and macros. Handling of unsol events would be less flexible, but I don't know if there are many uses for it except muting/unmuting other channels when using external microphones and headphones (and these cases could be well covered by specifying what to mute/unmute in the description file).
The description file could also be very similar to the format currently used in the C source, just making it easier to parse and having one model per file (with common definitions included via preprocessor). I'm not sure how well this would fit for Sigmatel codecs, but seems to be apropriate for Realtek.
The current implementation is in such a form intentionally to make a transition smoothly. In this way, you can port the existing kernel code quite easily to the new framework without losing functionality. The worst thing is that the machine doesn't work any longer by this transition. That's why I don't want to move to a generic parser or a dynamic parsing at the very first step.
Once after we get a certain working version, we can move to the further steps. The primary goal is to create a more generic and powerful parser that supports all codecs without extra codec-specific hacks. It'd be likely necessary to provide some extra hints/info, but it should be as small as possible. For this, we need to develop a good algorithm to parse the widget topology and build PCM streams and mixer elements appropriately. This will be a fun and interesting.
Another thing in my mind is to develop a codec simulator. This reads the codec proc file and responses to the codec commands like the given codec. It can log the command request/response sequences, so that you can catch regressions by the changes of codes. This would be especially helpful the transition from the codec-specific code to the generic parser. So, a collection of proc files will be very benifitable in future, too.
Takashi
On Jan 21, 2008 11:32 AM, Takashi Iwai tiwai@suse.de wrote:
The current implementation is in such a form intentionally to make a transition smoothly. In this way, you can port the existing kernel code quite easily to the new framework without losing functionality. The worst thing is that the machine doesn't work any longer by this transition. That's why I don't want to move to a generic parser or a dynamic parsing at the very first step.
Right, it's a good strategy.. I've built 0.0.10b against hg with only minor changes (notably 3 instances of -Werror-implicit-function-declaration had to be removed from configure.in). However, hda-config still complains about ioctl PVERSION error (ioctl() with HDA_IOCTL_PVERSION returns -1). Known problem or should I investigate further?
On Jan 23, 2008 5:46 PM, Claudio Matsuoka cmatsuoka@gmail.com wrote:
However, hda-config still complains about ioctl PVERSION error (ioctl() with HDA_IOCTL_PVERSION returns -1).
Uhm, what device maj/min is hda-config supposed to use?
At Thu, 24 Jan 2008 14:12:35 -0200, Claudio Matsuoka wrote:
On Jan 23, 2008 5:46 PM, Claudio Matsuoka cmatsuoka@gmail.com wrote:
However, hda-config still complains about ioctl PVERSION error (ioctl() with HDA_IOCTL_PVERSION returns -1).
Uhm, what device maj/min is hda-config supposed to use?
It depends on kernel config (CONFIG_SND_DYNAMIC_MINORS). It's created as /dev/snd/hwC*D0 by udev, usually.
Takashi
On Jan 24, 2008 2:24 PM, Takashi Iwai tiwai@suse.de wrote:
It depends on kernel config (CONFIG_SND_DYNAMIC_MINORS). It's created as /dev/snd/hwC*D0 by udev, usually.
Right, I had a problem with my dev nodes, it now works past the device opening and the ioctl is correctly handled by hda_hwdep_ioctl(). Some problems with GET_CODEC_INFO now, I'll investigate.
At Wed, 23 Jan 2008 17:46:25 -0200, Claudio Matsuoka wrote:
On Jan 21, 2008 11:32 AM, Takashi Iwai tiwai@suse.de wrote:
The current implementation is in such a form intentionally to make a transition smoothly. In this way, you can port the existing kernel code quite easily to the new framework without losing functionality. The worst thing is that the machine doesn't work any longer by this transition. That's why I don't want to move to a generic parser or a dynamic parsing at the very first step.
Right, it's a good strategy.. I've built 0.0.10b against hg with only minor changes (notably 3 instances of -Werror-implicit-function-declaration had to be removed from configure.in). However, hda-config still complains about ioctl PVERSION error (ioctl() with HDA_IOCTL_PVERSION returns -1). Known problem or should I investigate further?
Well, sorry, I've not tested the latest version with the latest driver at all. I just made it up to be applicable without rejects.
I'll track the problem tomorrow (or today if I have time)...
Takashi
At Tue, 15 Jan 2008 19:36:34 +0100, Marco Schuster wrote:
Hi all,
when I looked today through the files of the snd-hda-intel module, I was a bit shocked by this complex amount of code (patch_realtek.c for example). I would think about splitting up the code in a way each chipset gets its own .c file - which will make debugging, patching or editing a lot easier IMO, and for second people can then comment on just their specific chipset in config.
What do you think about this?
Marco
PS: If there are no objections, I'll start working on realtek.c on weekend.
Cons:
- Too many kernel configs are also no preferred option. I already splitted up to several configs, and I got many stupid bug reports.
- Moving the code makes hard to track the changes, especially the regression between versions.
I'm strongly for the solution with the user-space setup tool. The project was stopped simply because of my lack of time.
Takashi
participants (5)
-
Andrew Paprocki -
Claudio Matsuoka -
Marco Schuster -
Takashi Iwai -
Tobin Davis