[alsa-devel] SoC scenarii API
Hi Liam and Mark,
As mioa701 submission was stopped due to the need of a generic scenarii API, this a first attempt to design such an API in order to unblock mioa701 submission.
This is oversimplified, no convenience macros are provided, just external data structures and namespace polution.
I'm expecting a lot of comments from both of you.
Cheers.
-- Robert
enum pin_change { SCEN_PIN_OFF = 0, /* Disable the pin */ SCEN_PIN_ON, /* Enable the pin */ SCEN_PIN_LEAVE /* Leave the pin in previous state */ };
struct setup_mixmux { char *mixname; /* Codec Mixer or Muxer name */ int val; /* Codec Mixer or Muxer value to enforce */ };
/** * struct soc_scenario - describes one sound usecase * @name: Scenario name, value as will be seen in alsa "SoC Mode" alsa control * @pin_setup: Pin configuration to perform on scenario activation * Table of all pins, as they should be configured. Each elements is a * pin_change value, describing how to handle a specific pin. * Table size must be the same as in snd_soc_scenario_init(). * @mixer_cleanup: Mixers/muxes to set up in phase (b) * Table of all mixers/muxes to modify, NULL terminated. * @mixer_setup: Mixers/muxers to set up in phase (c) * Table of all mixers/muxes to modify, NULL terminated * @lvol_master: Left volume aliased to "SoC Volume" * @rvol_master: Right volume aliased to "SoC Volume" * @lvol_mute: Left volume mute aliased to "SoC Volume" mute control * @rvol_mute: Right volume mute aliased to "SoC Volume" mute control * * This structure describes what a scenario change implies. The behaviour is to : * a) enable several pins, disable others, leave others in their state * (understand here snd_soc_dapm_enable_pin) * b) set up some mixers and muxers to prepare the change, and clean up. * This should be normally used to restore a "default state" if need be, before * activating the mixers/muxers into their final state. * Note: table in NULL, or always terminated by NULL pointer * Note: this cleanup may/should be shared amongst all scenarii * c) set up some mixers and muxers to the final state * This should be normally used to restore a "default state" if need be, before * activating the mixers/muxers into their final state. * Note: table in NULL, or always terminated by NULL pointer */ struct soc_scenario { const char *name; /* scenario name */ const unsigned char pin_setup[]; /* pin_change for pins */ const struct setup_mixmux mixes_cleanup[]; /* mix cleanup */ const struct setup_mixmux mixes_setup[]; /* mix setup */ const char *lvol_master; /* left volume master */ const char *rvol_master; /* right volume master */ const char *lvol_mute; /* left volume mute */ const char *rvol_mute; /* right volume mute */ };
/** * snd_soc_scenario_init - initialize soc scenarii * @codec: codec associated to the pins/mixers/muxes/volumes/mutes * @scenario: table of scenarii * @nb_scenarii: number of scenarii * @pin_names: table of pin names * @nb_pins: number of pins handled by scenario * * Initialise the soc scenarii engine. The first scenario (0) will be used. * By default, the user could leave this scenario as non intrusive (not a single * pin changed, no mixers/muxes changes, and volume master inactive). */ int snd_soc_scenario_init(struct snd_soc_codec *codec, struct soc_scenario *scenario, int nb_scenarii, char *pin_names[], int nb_pins);
/** * snd_soc_scenario_init - initialize soc scenarii * @codec: codec used for the init */ void snd_soc_scenario_release(struct snd_soc_codec *code);
On Tue, Jan 13, 2009 at 12:36:13PM +0100, Robert Jarzmik wrote:
As mioa701 submission was stopped due to the need of a generic scenarii API, this a first attempt to design such an API in order to unblock mioa701 submission.
You can still carry on submitting the core machine support and then patch it to add the scenario stuff later. That'd probably make review slightly easier too since it'd identify the new stuff explicitly.
In general I'd like to see more exploration of the use cases that this is intended to satisfy, including in terms of the mioa701 itself. The documentation should make it clear that this is not intended to be a scalable solution and is only intended to be useful for hardware that is very limited.
What are you using for user space - is it one of the standard stacks like FSO? Looking at the features you've got I'm a bit concerned that the scenarios may get limiting in future: with both bluetooth and GSM things are already pretty complex. The only thing it's missing compared to OpenMoko is WiFi. For example, with your current scenarios I'm not sure if it'd be possible to record a call or do simultaneous record and playback? For dealing with overheating I *expect* that you only need to have the machine driver prevent particular combinations of outputs being simultaneously enabled.
That said, this looks mostly reasonable as a scenario API. I assume that it creates new controls for the master volume and for selecting the scenario? The main issues I can see are with how state transitions are managed and with how machine drivers interact with this if they need to update the configuration at run time.
struct setup_mixmux { char *mixname; /* Codec Mixer or Muxer name */ int val; /* Codec Mixer or Muxer value to enforce */ };
This structure needs namespacing. Also, "Mux" not "Muxer".
/**
- struct soc_scenario - describes one sound usecase
snd_soc_scenario.
- @name: Scenario name, value as will be seen in alsa "SoC Mode" alsa control
- @pin_setup: Pin configuration to perform on scenario activation
- Table of all pins, as they should be configured. Each elements is a
- pin_change value, describing how to handle a specific pin.
Table size must be the same as in snd_soc_scenario_init().
The table size ought to be specified in only one place.
- @mixer_cleanup: Mixers/muxes to set up in phase (b)
- Table of all mixers/muxes to modify, NULL terminated.
- @mixer_setup: Mixers/muxers to set up in phase (c)
- Table of all mixers/muxes to modify, NULL terminated
Hrm. This all feels either too flexible or too inflexible WRT state transitions. If you do need to impose ordering beyond what DAPM can figure out for itself then I'd expect to see any number of steps being allowed. If that isn't required then the intermediate step could just be dropped. If there were going to be some sort of "idle" state to transition through I'd expect to just see that identified and then the switchover to do a state->idle->state transition.
This might also be better with snd_ctl_elem_values so that it can cope with any control - there's definitely a need for configuring PGAs per-scenario, for example. Please also use a number of elements parameter for consistency with the rest of the API (both here and ASoC wide).
struct soc_scenario { const char *name; /* scenario name */ const unsigned char pin_setup[]; /* pin_change for pins */ const struct setup_mixmux mixes_cleanup[]; /* mix cleanup */ const struct setup_mixmux mixes_setup[]; /* mix setup */
More natural would be pointers to arrays...
int snd_soc_scenario_init(struct snd_soc_codec *codec, struct soc_scenario *scenario, int nb_scenarii,
Please make this take a snd_soc_card rather than a snd_soc_codec. Most of the card-wide APIs currently take a codec but this is in the process of being fixed. Also, scenarios is the more usual plural in English. For consistency with the rest of the API it'd be nice to use num rather than nb.
Some of the documentation about the situations where this API should be used should go with this function.
char *pin_names[], int nb_pins);
I'm not sure why the pin names are specified separately here? Would it not be better to just use the pin lists in the scenarios.
/**
- snd_soc_scenario_init - initialize soc scenarii
- @codec: codec used for the init
*/ void snd_soc_scenario_release(struct snd_soc_codec *code);
I sense bitrot :)
Mark Brown broonie@sirena.org.uk writes:
On Tue, Jan 13, 2009 at 12:36:13PM +0100, Robert Jarzmik wrote:
As mioa701 submission was stopped due to the need of a generic scenarii API, this a first attempt to design such an API in order to unblock mioa701 submission.
You can still carry on submitting the core machine support and then patch it to add the scenario stuff later. That'd probably make review slightly easier too since it'd identify the new stuff explicitly.
Mmm ... at the risk of having another hardware incident ... why not ? Let's take chances and see if another mio overheats.
In general I'd like to see more exploration of the use cases that this is intended to satisfy, including in terms of the mioa701 itself. The documentation should make it clear that this is not intended to be a scalable solution and is only intended to be useful for hardware that is very limited.
More comments then ? You know how poor my english is, you'll have to cope with it, sorry ... I'll add some information at the beginning.
What are you using for user space - is it one of the standard stacks like FSO? Looking at the features you've got I'm a bit concerned that
No. Userspace is Trolltech's Qtopia over alsa-lib.
the scenarios may get limiting in future: with both bluetooth and GSM things are already pretty complex. The only thing it's missing compared to OpenMoko is WiFi. For example, with your current scenarios I'm not sure if it'd be possible to record a call or do simultaneous record and playback?
You're right. That functionnality is not available. That's the price to pay, somehow.
For dealing with overheating I *expect* that you only need to have the machine driver prevent particular combinations of outputs being simultaneously enabled.
Ah, I feel you're right. The problem is, we don't have the specification, so we cannot be sure what creates the overheating.
That said, this looks mostly reasonable as a scenario API. I assume that it creates new controls for the master volume and for selecting the scenario?
Yes, namely "SoC Volume" and "SoC Mode".
The main issues I can see are with how state transitions are managed and with how machine drivers interact with this if they need to update the configuration at run time.
Ah, the missing pre_scenario() and post_scenario() handlers in snd_soc_scenario I guess. I thought about these and forgot them. Will these deal with your concern ?
struct setup_mixmux { char *mixname; /* Codec Mixer or Muxer name */ int val; /* Codec Mixer or Muxer value to enforce */ };
This structure needs namespacing. Also, "Mux" not "Muxer".
Right, I'll amend that.
/**
- struct soc_scenario - describes one sound usecase
snd_soc_scenario.
- @name: Scenario name, value as will be seen in alsa "SoC Mode" alsa control
- @pin_setup: Pin configuration to perform on scenario activation
- Table of all pins, as they should be configured. Each elements is a
- pin_change value, describing how to handle a specific pin.
Table size must be the same as in snd_soc_scenario_init().The table size ought to be specified in only one place.
- @mixer_cleanup: Mixers/muxes to set up in phase (b)
- Table of all mixers/muxes to modify, NULL terminated.
- @mixer_setup: Mixers/muxers to set up in phase (c)
- Table of all mixers/muxes to modify, NULL terminated
Hrm. This all feels either too flexible or too inflexible WRT state transitions. If you do need to impose ordering beyond what DAPM can figure out for itself then I'd expect to see any number of steps being allowed. If that isn't required then the intermediate step could just be dropped. If there were going to be some sort of "idle" state to transition through I'd expect to just see that identified and then the switchover to do a state->idle->state transition.
As you wish. mixer_cleanup can be removed, as a state->idle->state will do the same thing.
This might also be better with snd_ctl_elem_values so that it can cope with any control - there's definitely a need for configuring PGAs per-scenario, for example.
Right, I'll amend that.
Please also use a number of elements parameter for consistency with the rest of the API (both here and ASoC wide).
OK.
struct soc_scenario { const char *name; /* scenario name */ const unsigned char pin_setup[]; /* pin_change for pins */ const struct setup_mixmux mixes_cleanup[]; /* mix cleanup */ const struct setup_mixmux mixes_setup[]; /* mix setup */
More natural would be pointers to arrays...
Maybe. I was thinking of some macro magic to define each soc_scenario (thus the const). Let me activate thing a bit about it.
int snd_soc_scenario_init(struct snd_soc_codec *codec, struct soc_scenario *scenario, int nb_scenarii,
Please make this take a snd_soc_card rather than a snd_soc_codec. Most of the card-wide APIs currently take a codec but this is in the process of being fixed. Also, scenarios is the more usual plural in English. For consistency with the rest of the API it'd be nice to use num rather than nb.
OK.
Some of the documentation about the situations where this API should be used should go with this function.
char *pin_names[], int nb_pins);I'm not sure why the pin names are specified separately here? Would it not be better to just use the pin lists in the scenarios.
There are _no_ pin lists in scenarios. The scenarios only define a transition for each pin index. The actual pins are initialized once in the init function (ie. pin_names[0] = "Rear Speaker" for example). Then, in each scenario, pin_setup[0] will tell what to do to "Rear Speaker" : leave it, activate it or deactivate it.
/**
- snd_soc_scenario_init - initialize soc scenarii
- @codec: codec used for the init
*/ void snd_soc_scenario_release(struct snd_soc_codec *code);
I sense bitrot :)
Yes ...
I'll send an update soon.
-- Robert
On Tue, Jan 13, 2009 at 11:31:27PM +0100, Robert Jarzmik wrote:
Mark Brown broonie@sirena.org.uk writes:
You can still carry on submitting the core machine support and then patch it to add the scenario stuff later. That'd probably make review slightly easier too since it'd identify the new stuff explicitly.
Mmm ... at the risk of having another hardware incident ... why not ? Let's take chances and see if another mio overheats.
As said elsewhere in the thread you should be able to prevent this by preventing the affected outputs being simultaneously enabled (or whatever the configuration was that caused the trouble - output amplifiers are the things most likely to produce heat).
In general I'd like to see more exploration of the use cases that this is intended to satisfy, including in terms of the mioa701 itself. The documentation should make it clear that this is not intended to be a scalable solution and is only intended to be useful for hardware that is very limited.
More comments then ? You know how poor my english is, you'll have to cope with
Your standards for what good English is are very high :) .
What I mean is that we need to make sure that the case for in-kernel support has been made before it goes in and we also need to make sure that if it does go in it's made clear that this shouldn't be the standard way of doing scenarios.
What are you using for user space - is it one of the standard stacks like FSO? Looking at the features you've got I'm a bit concerned that
No. Userspace is Trolltech's Qtopia over alsa-lib.
Hrm. That's what OpenMoko used for their initial GTA02 release - they were using user space scenarios for that.
to OpenMoko is WiFi. For example, with your current scenarios I'm not sure if it'd be possible to record a call or do simultaneous record and playback?
You're right. That functionnality is not available. That's the price to pay, somehow.
This was the major part of the push to keep this out of kernel: it's difficult to impossible to figure out all the scenarios that users might want to run in and express them in a clean fashion. How the audio is routed ends up being a runtime policy decision since so much of it is about the needs of the user space applications at any given moment rather than on the properties of the hardware.
The other part was that it makes scenario development much easier (since you can use standard UIs and don't need to rebuild the kernel).
For dealing with overheating I *expect* that you only need to have the machine driver prevent particular combinations of outputs being simultaneously enabled.
Ah, I feel you're right. The problem is, we don't have the specification, so we cannot be sure what creates the overheating.
Oh, I see. If you're not sure restricting it to one output from the device (ie, only those that make the device itself make noise) at once would *probably* cover it. There may be some interaction with the heat output from the RF stuff, though that'd be an issue anyway since the scenario stuff has no knowledge of that.
The main issues I can see are with how state transitions are managed and with how machine drivers interact with this if they need to update the configuration at run time.
Ah, the missing pre_scenario() and post_scenario() handlers in snd_soc_scenario I guess. I thought about these and forgot them. Will these deal with your concern ?
That doesn't give the machine drivers any way to do stuff other than on a user-initiated change.
char *pin_names[], int nb_pins);
I'm not sure why the pin names are specified separately here? Would it not be better to just use the pin lists in the scenarios.
There are _no_ pin lists in scenarios. The scenarios only define a transition for each pin index. The actual pins are initialized once in the init function (ie. pin_names[0] = "Rear Speaker" for example). Then, in each scenario, pin_setup[0] will tell what to do to "Rear Speaker" : leave it, activate it or deactivate it.
Ah, ouch. That does become hard to read...
Hi Mark,
It's been a while since my last post. This is a new proposition for the API. I hope I have not forgotten what has been said previously.
As a side note, I'm a bit bothered by your comment about using "snd_ctl_elem_value" instead of integers in structure soc_scen_setup_mix. My concern is about size : snd_ctl_elem_value is huge, and as there will be many mixes/muxes, the memory print will be huge. Can't we deal with only integers for mixers and muxes ?
Cheers.
-- Robert
On Fri, Feb 20, 2009 at 08:04:24PM +0100, Robert Jarzmik wrote:
As a side note, I'm a bit bothered by your comment about using "snd_ctl_elem_value" instead of integers in structure soc_scen_setup_mix. My concern is about size : snd_ctl_elem_value is huge, and as there will be many mixes/muxes, the memory print will be huge. Can't we deal with only integers for mixers and muxes ?
I don't think it'd be acceptable to limit this to only working with a subset of controls, that'd mean that any controls that don't happen to fit in an integer. Allowing for a pointer to an out of line value would deal with the size issue, I'd expect.
participants (2)
-
Mark Brown -
Robert Jarzmik