Splitting up the HDA-INTEL modules

Splitting up the HDA-INTEL modules

[Marco Schuster]

[-- Attachment #1.1: Type: text/plain, Size: 523 bytes --]

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.


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: Splitting up the HDA-INTEL modules

[Tobin Davis]

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

-- 
Tobin Davis  


forget this, i've misread my own code. how could i think i could be wrong? ;)

@cooker ML from Pixel

Re: Splitting up the HDA-INTEL modules

[Claudio Matsuoka]

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.

Re: Splitting up the HDA-INTEL modules

[Andrew Paprocki]

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.

Re: Splitting up the HDA-INTEL modules

[Marco Schuster]

[-- Attachment #1.1: Type: text/plain, Size: 299 bytes --]

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?


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: Splitting up the HDA-INTEL modules

[Tobin Davis]

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

-- 
Tobin Davis  


Memory should be the starting point of the present.

Re: Splitting up the HDA-INTEL modules

[Jonathan Woithe]

Marco Schuster 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.

The problem with such a split is, as Tobin went on to say, that there are a
lot of interdependencies and shared code within each chipset's family:

> I don't think that it would be very easy, as there are several instances
> where code relies on other codec definitions.

One could argue that therefore we have toplevel realtek/sigmatek/etc
"librarys" which are used by each chipset-specific driver but IMHO that is
hardly going to count as a simplification.

Tobin then said:
> In all actuality, what is needed is a more generic driver that is driven
> by userspace tools. ... Maybe we can get a core group together and
> actually define and implement an API for this.

I think this idea has merit and may be worth pursuing.  I would be
interested in assisting and providing input to the process although I
haven't time at present to take a lead role.

> 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.

Agreed, at least in theory.  There may be complications due to the varying
topography of the chips; the userspace API may have to be relatively
complex to support things like record source switch options (which are
vastly different for diffenent chips).  Even so, if we can get this to work
the ongoing support effort for HDA will be much reduced so it's probably
worth the effort.

> I'll try to write up a proposal in the next few weeks, if we can get
> others on board to work on this.

I think this would be worth looking at.

Regards
  jonathan

Re: Splitting up the HDA-INTEL modules

[Takashi Iwai]

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

Re: Splitting up the HDA-INTEL modules

[Takashi Iwai]

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

Re: Splitting up the HDA-INTEL modules

[Takashi Iwai]

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

Re: Splitting up the HDA-INTEL modules

[Takashi Iwai]

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

Re: Splitting up the HDA-INTEL modules

[Takashi Iwai]

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
> 

Re: Splitting up the HDA-INTEL modules

[Claudio Matsuoka]

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.

Re: Splitting up the HDA-INTEL modules

[Takashi Iwai]

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

Re: Splitting up the HDA-INTEL modules

[Claudio Matsuoka]

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.

Re: Splitting up the HDA-INTEL modules

[Takashi Iwai]

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

Re: Splitting up the HDA-INTEL modules

[Claudio Matsuoka]

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?

Re: Splitting up the HDA-INTEL modules

[Claudio Matsuoka]

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?

Re: Splitting up the HDA-INTEL modules

[Takashi Iwai]

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

Re: Splitting up the HDA-INTEL modules

[Takashi Iwai]

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

Re: Splitting up the HDA-INTEL modules

[Claudio Matsuoka]

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.