[alsa-devel] Question about struct snd_soc_dai() :: cpu_dai->codec
Hi ALSA SoC
My current headache is ALSA SoC's each modules (= Card/Codec/CPU/Platform) doesn't care about "unbind/rmmod". For example, if someone unbinded/rmmoded "Codec", Card or other modules doesn't know about it. Thus, user can continue to use this sound card, and kernel will be Oops.
So, I would like to solve this issue, and for this purpose, now I'm investigating about ALSA SoC structures. It is now super complex, and I noticed that it is having duplicated struct members.
If possible, I would like to cleanup this issue.
During this investigating, I noticed 1 strange operation
${LINUX}/sound/soc/soc-core.c :: snd_soc_runtime_set_dai_fmt
int snd_soc_runtime_set_dai_fmt(xxx) { ... /* Flip the polarity for the "CPU" end of a CODEC<->CODEC link */ if (cpu_dai->codec) { ... } ... }
struct snd_soc_dai { ... /* parent platform/codec */ struct snd_soc_codec *codec; struct snd_soc_component *component; ... }
According to snd_soc_dai, *codec is its "parent". I wonder does "cpu_dai" really can have "codec" parent ?? I think this is not correct, and we can remove this operation ?
On Tue, Jul 26, 2016 at 05:41:56AM +0000, Kuninori Morimoto wrote:
Hi ALSA SoC
My current headache is ALSA SoC's each modules (= Card/Codec/CPU/Platform) doesn't care about "unbind/rmmod". For example, if someone unbinded/rmmoded "Codec", Card or other modules doesn't know about it. Thus, user can continue to use this sound card, and kernel will be Oops.
Are you sure about this? Have you tried removing a module?
During card probe, asoc will hold a reference to the component. See the calls to try_module_get(). This will prevent from unloading under normal cases.
Hi Vinod
My current headache is ALSA SoC's each modules (= Card/Codec/CPU/Platform) doesn't care about "unbind/rmmod". For example, if someone unbinded/rmmoded "Codec", Card or other modules doesn't know about it. Thus, user can continue to use this sound card, and kernel will be Oops.
Are you sure about this? Have you tried removing a module?
During card probe, asoc will hold a reference to the component. See the calls to try_module_get(). This will prevent from unloading under normal cases.
Very easy pseudo code is like this
rtd->card = card; rtd->cpu = cpu; rtd->codec = codec; rtd->platform = platform
Here, card/cpu/codec/platform are SoC device/driver, and "ALSA sound card" is using this rtd. Now, I can unbind/rmmod each of them (for example codec). But rtd (= ALSA sound card) doesn't know about it. Thus, rtd will access to invalid pointer after unbind/rmmod.
On Wed, 27 Jul 2016 05:21:11 +0200, Vinod Koul wrote:
On Tue, Jul 26, 2016 at 05:41:56AM +0000, Kuninori Morimoto wrote:
Hi ALSA SoC
My current headache is ALSA SoC's each modules (= Card/Codec/CPU/Platform) doesn't care about "unbind/rmmod". For example, if someone unbinded/rmmoded "Codec", Card or other modules doesn't know about it. Thus, user can continue to use this sound card, and kernel will be Oops.
Are you sure about this? Have you tried removing a module?
During card probe, asoc will hold a reference to the component. See the calls to try_module_get(). This will prevent from unloading under normal cases.
For unloading the module, yes, it should have been prevented by managing the module refcount. However, unbinding can't be stopped by that. It's a known problem.
Morimoto-san, do you see the issue really via module unloading, or is it only via unbinding?
Takashi
Hi Takashi-san
Thank you for your help
My current headache is ALSA SoC's each modules (= Card/Codec/CPU/Platform) doesn't care about "unbind/rmmod". For example, if someone unbinded/rmmoded "Codec", Card or other modules doesn't know about it. Thus, user can continue to use this sound card, and kernel will be Oops.
Are you sure about this? Have you tried removing a module?
During card probe, asoc will hold a reference to the component. See the calls to try_module_get(). This will prevent from unloading under normal cases.
For unloading the module, yes, it should have been prevented by managing the module refcount. However, unbinding can't be stopped by that. It's a known problem.
Morimoto-san, do you see the issue really via module unloading, or is it only via unbinding?
I tried, I thought this issue was for both unload/unbind, but it was only for unbinding. is this correct ?
# insmod cpu driver
> insmod home/snd-soc-rcar.ko [ 94.853449] asoc-simple-graph-dpcm-card asoc-simple-graph-dpcm-card.0.auto: snd-soc-dummy-dai <-> rsnd-dai.0 mapping ok [ 94.864466] asoc-simple-graph-dpcm-card asoc-simple-graph-dpcm-card.0.auto: snd-soc-dummy-dai <-> rsnd-dai.1 mapping ok [ 94.876047] asoc-simple-graph-dpcm-card asoc-simple-graph-dpcm-card.0.auto: ak4642-hifi <-> snd-soc-dummy-dai mapping ok [ 94.888157] rcar_sound ec500000.sound: probed
# check it
> lsmod Module Size Used by Not tainted snd_soc_rcar 47695 2 ...
# try rmmod
> rmmod snd_soc_rcar rmmod: can't unload 'snd_soc_rcar': Resource temporarily unavailable
# try playback, it still working
> aplay xxxx.wav
# try unbind
> echo ec500000.sound > /sys/devices/platform/ec500000.sound/driver/unbind
# sound card will die
> aplay xxxx.wav [ 230.236466] Unable to handle kernel NULL pointer dereference at virtual address 00000014 [ 230.244580] pgd = eea60000 [ 230.247320] [00000014] *pgd=6e9a3835, *pte=00000000, *ppte=00000000 [ 230.253628] Internal error: Oops: 17 [#1] SMP ARM [ 230.258335] Modules linked in: snd_soc_rcar regmap_mmio ... [ 230.607462] 1fe0: 00000005 bed645b0 b6e2745d b6e28b86 60000030 bed64600 0645484d 82551041 [ 230.615643] Backtrace: [ 230.618105] [<c02eab58>] (pinctrl_select_state) from [<c02ead14>] (pinctrl_pm_select_state+0x28/0x58) [ 230.627330] r9:ee814f00 r8:ee8b8b98 r7:00000000 r6:ee927900 r5:ef2ba410 r4:ee8c9000 [ 230.635131] [<c02eacec>] (pinctrl_pm_select_state) from [<c02ead64>] (pinctrl_pm_select_default_state+0x20/0x2c) [ 230.645311] r7:00000000 r6:ee88b200 r5:ef378800 r4:ee8c9000 [ 230.651016] [<c02ead44>] (pinctrl_pm_select_default_state) from [<c040ab90>] (soc_pcm_open+0x34/0x798) [ 230.660331] [<c040ab5c>] (soc_pcm_open) from [<c040c0e4>] (dpcm_fe_dai_open+0xbc/0x524) [ 230.668340] r10:ee8c9000 r9:ee8b8bb0 r8:ee8b8b98 r7:00000280 r6:ee8c9000 r5:00000001 [ 230.676222] r4:ef378800 ...
On Wed, Jul 27, 2016 at 07:57:05AM +0200, Takashi Iwai wrote:
On Wed, 27 Jul 2016 05:21:11 +0200, Vinod Koul wrote:
On Tue, Jul 26, 2016 at 05:41:56AM +0000, Kuninori Morimoto wrote:
Hi ALSA SoC
My current headache is ALSA SoC's each modules (= Card/Codec/CPU/Platform) doesn't care about "unbind/rmmod". For example, if someone unbinded/rmmoded "Codec", Card or other modules doesn't know about it. Thus, user can continue to use this sound card, and kernel will be Oops.
Are you sure about this? Have you tried removing a module?
During card probe, asoc will hold a reference to the component. See the calls to try_module_get(). This will prevent from unloading under normal cases.
For unloading the module, yes, it should have been prevented by managing the module refcount. However, unbinding can't be stopped by that. It's a known problem.
Oh yes, unload is an issue. Are these any solutions to prevent this?
In core, should we de-register the card if one of the components exits. The .remove should be called for the driver, thus triggering unregister?
On Wed, Jul 27, 2016 at 10:51:26PM +0530, Vinod Koul wrote:
On Wed, Jul 27, 2016 at 07:57:05AM +0200, Takashi Iwai wrote:
For unloading the module, yes, it should have been prevented by managing the module refcount. However, unbinding can't be stopped by that. It's a known problem.
Oh yes, unload is an issue. Are these any solutions to prevent this?
In core, should we de-register the card if one of the components exits. The .remove should be called for the driver, thus triggering unregister?
That's the theory but it's full of holes at the minute. Someone needs to sit down and fix all the holes in there.
On Wed, 27 Jul 2016 20:04:56 +0200, Mark Brown wrote:
On Wed, Jul 27, 2016 at 10:51:26PM +0530, Vinod Koul wrote:
On Wed, Jul 27, 2016 at 07:57:05AM +0200, Takashi Iwai wrote:
For unloading the module, yes, it should have been prevented by managing the module refcount. However, unbinding can't be stopped by that. It's a known problem.
Oh yes, unload is an issue. Are these any solutions to prevent this?
In core, should we de-register the card if one of the components exits. The .remove should be called for the driver, thus triggering unregister?
That's the theory but it's full of holes at the minute. Someone needs to sit down and fix all the holes in there.
Yeah, and it's not so trivial. The unbind can be called at any time, even during a stream is running. This is a kind of hot unplug. The whole nightmare we've faced with usb-audio and co will strike again.
I'm wondering whether it's a better option to block the unbind behavior, either in driver base (allowing to return an error) or in the sound side (waiting in remove() until the sane point).
Takashi
On Wed, Jul 27, 2016 at 08:11:49PM +0200, Takashi Iwai wrote:
I'm wondering whether it's a better option to block the unbind behavior, either in driver base (allowing to return an error) or in the sound side (waiting in remove() until the sane point).
That's certainly going to be a lot easier and part of the reason it's never been looked at much is that (unlike USB) there's very little reason why an ASoC sound card would ever be hotplugged - even in development these days the normal development flow involves rebooting.
On Wed, 27 Jul 2016 20:22:33 +0200, Mark Brown wrote:
On Wed, Jul 27, 2016 at 08:11:49PM +0200, Takashi Iwai wrote:
I'm wondering whether it's a better option to block the unbind behavior, either in driver base (allowing to return an error) or in the sound side (waiting in remove() until the sane point).
That's certainly going to be a lot easier and part of the reason it's never been looked at much is that (unlike USB) there's very little reason why an ASoC sound card would ever be hotplugged - even in development these days the normal development flow involves rebooting.
Actually there is already the suppress_bind_attr flag in struct device_driver. For a simple platform driver like snd-soc-rcar, it's easy like:
diff --git a/sound/soc/sh/rcar/core.c b/sound/soc/sh/rcar/core.c index 3351a701c60e..d019824927de 100644 --- a/sound/soc/sh/rcar/core.c +++ b/sound/soc/sh/rcar/core.c @@ -1251,6 +1251,7 @@ static struct platform_driver rsnd_driver = { .driver = { .name = "rcar_sound", .of_match_table = rsnd_of_match, + .suppress_bind_attrs = true, }, .probe = rsnd_probe, .remove = rsnd_remove,
Then there will be no sysfs bind/unbind for this driver. (Note: totally untested: let me know if it really works.)
The same technique is likely available for i2c and spi codec drivers. But it's another open question whether we should suppress the sysfs bind/unbind of these devices at all. My gut feeling is that sysfs bind/unbind are mostly useless for drivers like ASoC codecs. At least, it would be much safer to disable for now.
Takashi
On Wed, Jul 27, 2016 at 10:22:41PM +0200, Takashi Iwai wrote:
On Wed, 27 Jul 2016 20:22:33 +0200, Mark Brown wrote:
On Wed, Jul 27, 2016 at 08:11:49PM +0200, Takashi Iwai wrote:
I'm wondering whether it's a better option to block the unbind behavior, either in driver base (allowing to return an error) or in the sound side (waiting in remove() until the sane point).
That's certainly going to be a lot easier and part of the reason it's never been looked at much is that (unlike USB) there's very little reason why an ASoC sound card would ever be hotplugged - even in development these days the normal development flow involves rebooting.
I agree, it makese no sense for devices to be hotplugged. And for developement flows people can do rmmond and insmod. That works fine!
Actually there is already the suppress_bind_attr flag in struct device_driver. For a simple platform driver like snd-soc-rcar, it's easy like:
I like this idea, should we do this per driver or in core? I think we should let drivers decide..
diff --git a/sound/soc/sh/rcar/core.c b/sound/soc/sh/rcar/core.c index 3351a701c60e..d019824927de 100644 --- a/sound/soc/sh/rcar/core.c +++ b/sound/soc/sh/rcar/core.c @@ -1251,6 +1251,7 @@ static struct platform_driver rsnd_driver = { .driver = { .name = "rcar_sound", .of_match_table = rsnd_of_match,
.suppress_bind_attrs = true,Then there will be no sysfs bind/unbind for this driver. (Note: totally untested: let me know if it really works.)
The same technique is likely available for i2c and spi codec drivers. But it's another open question whether we should suppress the sysfs bind/unbind of these devices at all. My gut feeling is that sysfs bind/unbind are mostly useless for drivers like ASoC codecs. At least, it would be much safer to disable for now.
On 07/28/2016 05:46 AM, Vinod Koul wrote:
On Wed, Jul 27, 2016 at 10:22:41PM +0200, Takashi Iwai wrote:
On Wed, 27 Jul 2016 20:22:33 +0200, Mark Brown wrote:
On Wed, Jul 27, 2016 at 08:11:49PM +0200, Takashi Iwai wrote:
I'm wondering whether it's a better option to block the unbind behavior, either in driver base (allowing to return an error) or in the sound side (waiting in remove() until the sane point).
That's certainly going to be a lot easier and part of the reason it's never been looked at much is that (unlike USB) there's very little reason why an ASoC sound card would ever be hotplugged - even in development these days the normal development flow involves rebooting.
I agree, it makese no sense for devices to be hotplugged. And for developement flows people can do rmmond and insmod. That works fine!
I don't agree. In my opinion hot-plug is an essential feature of a modern device driver framework and if ASoC wants to claim to fall in this category we ought to support it. Hotplug is something that always pops up sooner or later. E.g. if someone puts a ASoC supported CODEC on a hot-pluggable device (maybe USB) we don't want to duplicate the code, but be able to reuse.
One area that e.g. requires hot-plug/-unplug and were ASoC supported devices are used is embedded development boards that support shields and devicetree overlays. Like e.g. RPI and similar.
The reason why we don't support hot-unplug in ASoC at the moment is because it is not trivial to implement and nobody has cared enough yet.
But if somebody wants to and has the resources to implement this we should not discourage this.
I'd very much prefer to have proper hot-plug support instead of prohibiting unbinding even on systems that do not require hot-plug support normally. It's a much cleaner solution.
- Lars
On Thu, 28 Jul 2016 22:33:31 +0200, Lars-Peter Clausen wrote:
On 07/28/2016 05:46 AM, Vinod Koul wrote:
On Wed, Jul 27, 2016 at 10:22:41PM +0200, Takashi Iwai wrote:
On Wed, 27 Jul 2016 20:22:33 +0200, Mark Brown wrote:
On Wed, Jul 27, 2016 at 08:11:49PM +0200, Takashi Iwai wrote:
I'm wondering whether it's a better option to block the unbind behavior, either in driver base (allowing to return an error) or in the sound side (waiting in remove() until the sane point).
That's certainly going to be a lot easier and part of the reason it's never been looked at much is that (unlike USB) there's very little reason why an ASoC sound card would ever be hotplugged - even in development these days the normal development flow involves rebooting.
I agree, it makese no sense for devices to be hotplugged. And for developement flows people can do rmmond and insmod. That works fine!
I don't agree. In my opinion hot-plug is an essential feature of a modern device driver framework and if ASoC wants to claim to fall in this category we ought to support it. Hotplug is something that always pops up sooner or later. E.g. if someone puts a ASoC supported CODEC on a hot-pluggable device (maybe USB) we don't want to duplicate the code, but be able to reuse.
One area that e.g. requires hot-plug/-unplug and were ASoC supported devices are used is embedded development boards that support shields and devicetree overlays. Like e.g. RPI and similar.
The reason why we don't support hot-unplug in ASoC at the moment is because it is not trivial to implement and nobody has cared enough yet.
But if somebody wants to and has the resources to implement this we should not discourage this.
I'd very much prefer to have proper hot-plug support instead of prohibiting unbinding even on systems that do not require hot-plug support normally. It's a much cleaner solution.
Well, but hot-unplugging only a component like codec would be needed in a real scenario? It's a different story from the hotplug in the card level.
Takashi
On 07/28/2016 10:42 PM, Takashi Iwai wrote:
On Thu, 28 Jul 2016 22:33:31 +0200, Lars-Peter Clausen wrote:
On 07/28/2016 05:46 AM, Vinod Koul wrote:
On Wed, Jul 27, 2016 at 10:22:41PM +0200, Takashi Iwai wrote:
On Wed, 27 Jul 2016 20:22:33 +0200, Mark Brown wrote:
On Wed, Jul 27, 2016 at 08:11:49PM +0200, Takashi Iwai wrote:
I'm wondering whether it's a better option to block the unbind behavior, either in driver base (allowing to return an error) or in the sound side (waiting in remove() until the sane point).
That's certainly going to be a lot easier and part of the reason it's never been looked at much is that (unlike USB) there's very little reason why an ASoC sound card would ever be hotplugged - even in development these days the normal development flow involves rebooting.
I agree, it makese no sense for devices to be hotplugged. And for developement flows people can do rmmond and insmod. That works fine!
I don't agree. In my opinion hot-plug is an essential feature of a modern device driver framework and if ASoC wants to claim to fall in this category we ought to support it. Hotplug is something that always pops up sooner or later. E.g. if someone puts a ASoC supported CODEC on a hot-pluggable device (maybe USB) we don't want to duplicate the code, but be able to reuse.
One area that e.g. requires hot-plug/-unplug and were ASoC supported devices are used is embedded development boards that support shields and devicetree overlays. Like e.g. RPI and similar.
The reason why we don't support hot-unplug in ASoC at the moment is because it is not trivial to implement and nobody has cared enough yet.
But if somebody wants to and has the resources to implement this we should not discourage this.
I'd very much prefer to have proper hot-plug support instead of prohibiting unbinding even on systems that do not require hot-plug support normally. It's a much cleaner solution.
Well, but hot-unplugging only a component like codec would be needed in a real scenario? It's a different story from the hotplug in the card level.
With overlays I'm not sure if we can control the remove order. So the component might be removed before the card complex.
On Thu, Jul 28, 2016 at 10:33:31PM +0200, Lars-Peter Clausen wrote:
On 07/28/2016 05:46 AM, Vinod Koul wrote:
I agree, it makese no sense for devices to be hotplugged. And for developement flows people can do rmmond and insmod. That works fine!
I don't agree. In my opinion hot-plug is an essential feature of a modern device driver framework and if ASoC wants to claim to fall in this category we ought to support it. Hotplug is something that always pops up sooner or later. E.g. if someone puts a ASoC supported CODEC on a hot-pluggable device (maybe USB) we don't want to duplicate the code, but be able to reuse.
Right, so there's two bits to hotplug - there's hotplugging individual components separately to the card and there's hotplugging cards en masse including some of their components. The latter case definitely does make sense and should have a reasonable chance of working already. Hotplugging individual components is much more of a nice to have, though as you say if someone wants to implement it that's obviously not a problem.
Lars,
On Jul 29 2016 05:33, Lars-Peter Clausen wrote:
Hotplug is something that always pops up sooner or later. E.g. if someone puts a ASoC supported CODEC on a hot-pluggable device (maybe USB) we don't want to duplicate the code, but be able to reuse.
(A bit to sidetrack)
To me, it's unclear for devices on USB. When ALSA SoC part supports these devices, what is the scenario you assumed? In short, assuming we put codes to ALSA SoC part, what is the shape of the corresponding devices and links of pairs of endpoints? Additionally, in this case, what codes are able to be reused?
Regards
Takashi Sakamoto
On 07/29/2016 02:30 AM, Takashi Sakamoto wrote:
Lars,
On Jul 29 2016 05:33, Lars-Peter Clausen wrote:
Hotplug is something that always pops up sooner or later. E.g. if someone puts a ASoC supported CODEC on a hot-pluggable device (maybe USB) we don't want to duplicate the code, but be able to reuse.
(A bit to sidetrack)
To me, it's unclear for devices on USB. When ALSA SoC part supports these devices, what is the scenario you assumed? In short, assuming we put codes to ALSA SoC part, what is the shape of the corresponding devices and links of pairs of endpoints? Additionally, in this case, what codes are able to be reused?
Lets say you have USB stick with a small micro controller or FPGA which has a USB interface on one side and a I2S and I2C interface on the other side. The I2S and I2C are connected to a CODEC. I2S for data, I2C for control. If the interface is implemented in a way so that it is just a simple USB to I2C bridge, this means the raw I2C commands are send over the USB interface you can implement a I2C adapter driver for this bridge. If you have that you can instantiate the existing ASoC CODEC driver, which is a I2C device driver, on the bus registered by the adapter.
- Lars
On Fri, Jul 29, 2016 at 11:07:49AM +0200, Lars-Peter Clausen wrote:
On 07/29/2016 02:30 AM, Takashi Sakamoto wrote:
Lars,
On Jul 29 2016 05:33, Lars-Peter Clausen wrote:
Hotplug is something that always pops up sooner or later. E.g. if someone puts a ASoC supported CODEC on a hot-pluggable device (maybe USB) we don't want to duplicate the code, but be able to reuse.
(A bit to sidetrack)
To me, it's unclear for devices on USB. When ALSA SoC part supports these devices, what is the scenario you assumed? In short, assuming we put codes to ALSA SoC part, what is the shape of the corresponding devices and links of pairs of endpoints? Additionally, in this case, what codes are able to be reused?
Lets say you have USB stick with a small micro controller or FPGA which has a USB interface on one side and a I2S and I2C interface on the other side. The I2S and I2C are connected to a CODEC. I2S for data, I2C for control. If the interface is implemented in a way so that it is just a simple USB to I2C bridge, this means the raw I2C commands are send over the USB interface you can implement a I2C adapter driver for this bridge. If you have that you can instantiate the existing ASoC CODEC driver, which is a I2C device driver, on the bus registered by the adapter.
That still seems a bit fancy hardware :)
If we can reasonably support this, I am for it. But not making stuff overtly complex...
On Fri, 29 Jul 2016 18:07:02 +0200, Vinod Koul wrote:
On Fri, Jul 29, 2016 at 11:07:49AM +0200, Lars-Peter Clausen wrote:
On 07/29/2016 02:30 AM, Takashi Sakamoto wrote:
Lars,
On Jul 29 2016 05:33, Lars-Peter Clausen wrote:
Hotplug is something that always pops up sooner or later. E.g. if someone puts a ASoC supported CODEC on a hot-pluggable device (maybe USB) we don't want to duplicate the code, but be able to reuse.
(A bit to sidetrack)
To me, it's unclear for devices on USB. When ALSA SoC part supports these devices, what is the scenario you assumed? In short, assuming we put codes to ALSA SoC part, what is the shape of the corresponding devices and links of pairs of endpoints? Additionally, in this case, what codes are able to be reused?
Lets say you have USB stick with a small micro controller or FPGA which has a USB interface on one side and a I2S and I2C interface on the other side. The I2S and I2C are connected to a CODEC. I2S for data, I2C for control. If the interface is implemented in a way so that it is just a simple USB to I2C bridge, this means the raw I2C commands are send over the USB interface you can implement a I2C adapter driver for this bridge. If you have that you can instantiate the existing ASoC CODEC driver, which is a I2C device driver, on the bus registered by the adapter.
That still seems a bit fancy hardware :)
If we can reasonably support this, I am for it. But not making stuff overtly complex...
Yes, we don't have to overreact to a dream immediately now. We should consider whether it can be shifted to the card-level instead before worrying too much about hot-unplug of an ASoC component. Then the whole story becomes far easier.
And, the suppress_bind_attrs flag I suggested is only to suppress the sysfs bind/unbind, and doesn't mean to prohibit hotplug itself via the bus driver.
Takashi
On Jul 30 2016 05:41, Takashi Iwai wrote:
That still seems a bit fancy hardware :)
If we can reasonably support this, I am for it. But not making stuff overtly complex...
Yes, we don't have to overreact to a dream immediately now.
As I wrote at first, my question is a sidetrack. So you can still continue the discussion for loadable ALSA SoC part.
In my understanding, one of the aim of ALSA SoC part is to reuse drivers of audio IPs, typically hardware ADC/DAC (so-called 'codec'). To achieve this, the ideas of 'DAI' and 'link' are introduced for better abstraction, additionally to historical 'card' and 'components'.
In a point of 'reuse of codes', I cannot imagine what Lars said for USB devices, then post the questions.
We should consider whether it can be shifted to the card-level instead before worrying too much about hot-unplug of an ASoC component. Then the whole story becomes far easier.
And, the suppress_bind_attrs flag I suggested is only to suppress the sysfs bind/unbind, and doesn't mean to prohibit hotplug itself via the bus driver.
In my opinion, the relationship between DAI driver and codec or component driver is not described as relationship between kernel modules. The issue cannot be resolved by dependency of loadable modules and we need to manage the relationship of loaded modules somewhere. In this meaning, I can understand the idea which Mr.Morimoto described.
But I think it's logically difficult to manage state of sound card; e.g. disconnect. When one sound card instance consists of instances of several 'DAI', 'Codecs' and 'Components' (this 'component' is not in ALSA core contexts[1]) and we try to unload one of them, then which state the card should be assigned to? Or no 'Codecs' drivers are loaded, then which state should be assigned to the card?
Additionally, when old Codec driver is unloaded and new Codec driver is loaded, then what should we do for corresponding PCM character devices are? Currently, once snd_card_regsiter() is called, we cannot insert/delete ALSA components such like PCM.
[1] For the context, please refer to 'Writing an ALSA driver'. http://www.alsa-project.org/~tiwai/writing-an-alsa-driver/ch02s03.html#basic...
Regards
Takashi Sakamoto
On Sat, Jul 30, 2016 at 06:45:04AM +0900, Takashi Sakamoto wrote:
In a point of 'reuse of codes', I cannot imagine what Lars said for USB devices, then post the questions.
Someone might make a fancy device connected via USB which doesn't conform to the USB specs.
But I think it's logically difficult to manage state of sound card; e.g. disconnect. When one sound card instance consists of instances of several 'DAI', 'Codecs' and 'Components' (this 'component' is not in ALSA core contexts[1]) and we try to unload one of them, then which state the card should be assigned to? Or no 'Codecs' drivers are loaded, then which state should be assigned to the card?
The card only instantiates when all the components of the card are present, until then it defers probe.
Additionally, when old Codec driver is unloaded and new Codec driver is loaded, then what should we do for corresponding PCM character devices are? Currently, once snd_card_regsiter() is called, we cannot insert/delete ALSA components such like PCM.
The card should be deinstantiated and reinstantiated whenever a component driver unbinds and rebinds (respectively). You'd need to completely deregister the card to change the list of things it's expecting currently.
On Jul 30 2016 07:08, Mark Brown wrote:
But I think it's logically difficult to manage state of sound card; e.g. disconnect. When one sound card instance consists of instances of several 'DAI', 'Codecs' and 'Components' (this 'component' is not in ALSA core contexts[1]) and we try to unload one of them, then which state the card should be assigned to? Or no 'Codecs' drivers are loaded, then which state should be assigned to the card?
The card only instantiates when all the components of the card are present, until then it defers probe.
Oops. I forgot ALSA soc part utilizes EPROBE_DEFER. Thanks for your correction.
Additionally, when old Codec driver is unloaded and new Codec driver is loaded, then what should we do for corresponding PCM character devices are? Currently, once snd_card_regsiter() is called, we cannot insert/delete ALSA components such like PCM.
The card should be deinstantiated and reinstantiated whenever a component driver unbinds and rebinds (respectively). You'd need to completely deregister the card to change the list of things it's expecting currently.
In a point of application interfaces, I guess that current implementation of ALSA soc part includes a bug that it's possible to unload codec or component modules when any ALSA character devices are opened. The framework has no codes to manage reference counting of character devices or loaded codecs, components.
Here, any of suggestions comes from my code reading. I apologize if they're wrong.
Regards
Takashi Sakamoto
On Thu, Aug 04, 2016 at 12:17:57PM +0900, Takashi Sakamoto wrote:
On Jul 30 2016 07:08, Mark Brown wrote:
The card should be deinstantiated and reinstantiated whenever a component driver unbinds and rebinds (respectively). You'd need to completely deregister the card to change the list of things it's expecting currently.
In a point of application interfaces, I guess that current implementation of ALSA soc part includes a bug that it's possible to unload codec or component modules when any ALSA character devices are opened. The framework has no codes to manage reference counting of character devices or loaded codecs, components.
Yes, exactly - we don't cope very well with that situation and we really ought to but since it's hard to trigger without trying in practice it's never been a priority.
On Aug 4 2016 19:28, Mark Brown wrote:
On Thu, Aug 04, 2016 at 12:17:57PM +0900, Takashi Sakamoto wrote:
On Jul 30 2016 07:08, Mark Brown wrote:
The card should be deinstantiated and reinstantiated whenever a component driver unbinds and rebinds (respectively). You'd need to completely deregister the card to change the list of things it's expecting currently.
In a point of application interfaces, I guess that current implementation of ALSA soc part includes a bug that it's possible to unload codec or component modules when any ALSA character devices are opened. The framework has no codes to manage reference counting of character devices or loaded codecs, components.
Yes, exactly - we don't cope very well with that situation and we really ought to but since it's hard to trigger without trying in practice it's never been a priority.
Ugly... completely ugly idea for user space applications and operating system... It's better for developers for ALSA soc part to pay enough attention not only to their hardwares but also to application interfaces.
Please assume that a loaded module for SoC's sound interface which supports Jack detection, and pulseaudio runs on the system. Then, typically, the process listen to ALSA ctrl character device for Jack detection.
In this case, when modules for codec or component are unloaded, what happends? In worst case, pulseaudio process can kill the system in a system call path or something else, because the modules for SoC's sound interface is still loaded and userspace applications can execute system calls via ALSA character devices.
It should be forbidden to build ALSA soc part as loadable kernel modules. It's really danger...
Regards
Takashi Sakamoto
On Thu, 04 Aug 2016 14:12:09 +0200, Takashi Sakamoto wrote:
On Aug 4 2016 19:28, Mark Brown wrote:
On Thu, Aug 04, 2016 at 12:17:57PM +0900, Takashi Sakamoto wrote:
On Jul 30 2016 07:08, Mark Brown wrote:
The card should be deinstantiated and reinstantiated whenever a component driver unbinds and rebinds (respectively). You'd need to completely deregister the card to change the list of things it's expecting currently.
In a point of application interfaces, I guess that current implementation of ALSA soc part includes a bug that it's possible to unload codec or component modules when any ALSA character devices are opened. The framework has no codes to manage reference counting of character devices or loaded codecs, components.
Yes, exactly - we don't cope very well with that situation and we really ought to but since it's hard to trigger without trying in practice it's never been a priority.
Ugly... completely ugly idea for user space applications and operating system... It's better for developers for ALSA soc part to pay enough attention not only to their hardwares but also to application interfaces.
Please assume that a loaded module for SoC's sound interface which supports Jack detection, and pulseaudio runs on the system. Then, typically, the process listen to ALSA ctrl character device for Jack detection.
In this case, when modules for codec or component are unloaded, what happends?
You can't unload. The module unload is already protected by the proper module refcounting.
In worst case, pulseaudio process can kill the system in a system call path or something else, because the modules for SoC's sound interface is still loaded and userspace applications can execute system calls via ALSA character devices.
It should be forbidden to build ALSA soc part as loadable kernel modules. It's really danger...
The only danger is about the dynamic unbinding, and it has nothing to do with the module. And, protecting the sysfs unbind itself is trivial, too: as I already suggested, we can put the flags in appropriate places.
Takashi
On Aug 4 2016 21:27, Takashi Iwai wrote:
On Thu, 04 Aug 2016 14:12:09 +0200, Takashi Sakamoto wrote:
On Aug 4 2016 19:28, Mark Brown wrote:
On Thu, Aug 04, 2016 at 12:17:57PM +0900, Takashi Sakamoto wrote:
On Jul 30 2016 07:08, Mark Brown wrote:
The card should be deinstantiated and reinstantiated whenever a component driver unbinds and rebinds (respectively). You'd need to completely deregister the card to change the list of things it's expecting currently.
In a point of application interfaces, I guess that current implementation of ALSA soc part includes a bug that it's possible to unload codec or component modules when any ALSA character devices are opened. The framework has no codes to manage reference counting of character devices or loaded codecs, components.
Yes, exactly - we don't cope very well with that situation and we really ought to but since it's hard to trigger without trying in practice it's never been a priority.
Ugly... completely ugly idea for user space applications and operating system... It's better for developers for ALSA soc part to pay enough attention not only to their hardwares but also to application interfaces.
Please assume that a loaded module for SoC's sound interface which supports Jack detection, and pulseaudio runs on the system. Then, typically, the process listen to ALSA ctrl character device for Jack detection.
In this case, when modules for codec or component are unloaded, what happends?
You can't unload. The module unload is already protected by the proper module refcounting.
Hm. For my information, could you please show call graph to increment the reference counter of codec/component modules when modules for SoC's sound interfaces refer to them?
Regards
Takashi Sakamoto
On Thu, 04 Aug 2016 15:39:40 +0200, Takashi Sakamoto wrote:
On Aug 4 2016 21:27, Takashi Iwai wrote:
On Thu, 04 Aug 2016 14:12:09 +0200, Takashi Sakamoto wrote:
On Aug 4 2016 19:28, Mark Brown wrote:
On Thu, Aug 04, 2016 at 12:17:57PM +0900, Takashi Sakamoto wrote:
On Jul 30 2016 07:08, Mark Brown wrote:
The card should be deinstantiated and reinstantiated whenever a component driver unbinds and rebinds (respectively). You'd need to completely deregister the card to change the list of things it's expecting currently.
In a point of application interfaces, I guess that current implementation of ALSA soc part includes a bug that it's possible to unload codec or component modules when any ALSA character devices are opened. The framework has no codes to manage reference counting of character devices or loaded codecs, components.
Yes, exactly - we don't cope very well with that situation and we really ought to but since it's hard to trigger without trying in practice it's never been a priority.
Ugly... completely ugly idea for user space applications and operating system... It's better for developers for ALSA soc part to pay enough attention not only to their hardwares but also to application interfaces.
Please assume that a loaded module for SoC's sound interface which supports Jack detection, and pulseaudio runs on the system. Then, typically, the process listen to ALSA ctrl character device for Jack detection.
In this case, when modules for codec or component are unloaded, what happends?
You can't unload. The module unload is already protected by the proper module refcounting.
Hm. For my information, could you please show call graph to increment the reference counter of codec/component modules when modules for SoC's sound interfaces refer to them?
Just grep try_module_get() and module_put() calls.
Takashi
On 07/26/2016 07:41 AM, Kuninori Morimoto wrote:
Hi ALSA SoC
My current headache is ALSA SoC's each modules (= Card/Codec/CPU/Platform) doesn't care about "unbind/rmmod". For example, if someone unbinded/rmmoded "Codec", Card or other modules doesn't know about it. Thus, user can continue to use this sound card, and kernel will be Oops.
So, I would like to solve this issue, and for this purpose, now I'm investigating about ALSA SoC structures. It is now super complex, and I noticed that it is having duplicated struct members.
If possible, I would like to cleanup this issue.
During this investigating, I noticed 1 strange operation
${LINUX}/sound/soc/soc-core.c :: snd_soc_runtime_set_dai_fmt
int snd_soc_runtime_set_dai_fmt(xxx) { ... /* Flip the polarity for the "CPU" end of a CODEC<->CODEC link */ if (cpu_dai->codec) { ... } ... }
struct snd_soc_dai { ... /* parent platform/codec */ struct snd_soc_codec *codec; struct snd_soc_component *component; ... }
According to snd_soc_dai, *codec is its "parent". I wonder does "cpu_dai" really can have "codec" parent ?? I think this is not correct, and we can remove this operation ?
This is for CODEC<->CODEC links where no CPU DAI is involved and the "CPU" side of the DAI link is actually a CODEC.
Ideally we'd make the DAI links agnostic to what is connected, e.g. it shouldn't matter what type is connected to what side. But that does not work since things are anti-symmetric between CPU and CODEC DAIs. On CODEC DAIs the capture stream is output, on CPU DAIs the capture stream is input, similar thing for playback. So we need to know whether the DAI is a CPU DAI or a CODEC DAI.
Fixing this at this point is near to impossible since it requires invasive changes to all existing drivers. So we need this code and can't drop it.
The best we can do is try to come up with a generic interface that is DAI type independent and recommend this interface for new drivers.
Hi Lars
Thank you for your feedback
int snd_soc_runtime_set_dai_fmt(xxx) { ... /* Flip the polarity for the "CPU" end of a CODEC<->CODEC link */ if (cpu_dai->codec) { ... } ... }
(snip)
This is for CODEC<->CODEC links where no CPU DAI is involved and the "CPU" side of the DAI link is actually a CODEC.
Wow !!
Ideally we'd make the DAI links agnostic to what is connected, e.g. it shouldn't matter what type is connected to what side. But that does not work since things are anti-symmetric between CPU and CODEC DAIs. On CODEC DAIs the capture stream is output, on CPU DAIs the capture stream is input, similar thing for playback. So we need to know whether the DAI is a CPU DAI or a CODEC DAI.
Fixing this at this point is near to impossible since it requires invasive changes to all existing drivers. So we need this code and can't drop it.
The best we can do is try to come up with a generic interface that is DAI type independent and recommend this interface for new drivers.
Hmm... OK, now, I'm investigating ALSA SoC struct connection. And yes, existing code has deep relationship each other. Thus, fixing/droping seems very dificult... I think each struct has random pointer connection which doesn't care about ALSA SoC's hierarchy.
I would like to indicate my opinion here. My strange point are...
1. snd_soc_pcm_runtime has pointer to Card/Codec/Platform, but no CPU 2. snd_soc_dai has pointer to [component] and [Codec] [component] is its parent, [Codec] is maybe Hack 3. [component] has pointer to [card] and [Codec] [card] is understandable, [Codec] is maybe Hack
It seems many struct would like to access to [Codec], thus, each struct has codec pointer (as hack ?), but it is ignoring ALSA SoC hierarchy.
It will be more clear if...
IDEA 1
Each component (= CPU/Codec/Platform) has pointer to Card now. How about Card has pointer to each component ? if so, we can access to every struct. Card -> CPU/Codec/Platform, CPU/Codec/Platform -> Card Then, we want to have component related macro/flag
#define component_is_cpu(component) (component->flag & COMPONENT_CPU) #define component_is_codec(component) (component->flag & COMPONENT_CODEC) #define component_is_platform(component) (component->flag & COMPONENT_PLATFORM)
#define component_to_cpu() container_of(xxx) #define component_to_codec() container_of(xxx) #define component_to_platorm() container_of(xxx)
IDEA 2
if IDEA1 was OK, snd_soc_pcm_runtime already has card pointer. This means snd_soc_pcm_runtime can access to every component struct.
snd_soc_dai has its parent component. This means snd_soc_dai can access to Card, and Card can access to every component. And then, cpu_dai->codec has no issue if we can use component_is_xxx() macro ?
- if (cpu_dai->codec) + if (component_is_codec(cpu_dai->component))
If IDEA1 / IDEA2 are OK, we can cleanup each struct, especially hack-able random pointer ?
Best regards --- Kuninori Morimoto
On 07/29/2016 04:24 AM, Kuninori Morimoto wrote:
Hi Lars
Thank you for your feedback
int snd_soc_runtime_set_dai_fmt(xxx) { ... /* Flip the polarity for the "CPU" end of a CODEC<->CODEC link */ if (cpu_dai->codec) { ... } ... }
(snip)
This is for CODEC<->CODEC links where no CPU DAI is involved and the "CPU" side of the DAI link is actually a CODEC.
Wow !!
Ideally we'd make the DAI links agnostic to what is connected, e.g. it shouldn't matter what type is connected to what side. But that does not work since things are anti-symmetric between CPU and CODEC DAIs. On CODEC DAIs the capture stream is output, on CPU DAIs the capture stream is input, similar thing for playback. So we need to know whether the DAI is a CPU DAI or a CODEC DAI.
Fixing this at this point is near to impossible since it requires invasive changes to all existing drivers. So we need this code and can't drop it.
The best we can do is try to come up with a generic interface that is DAI type independent and recommend this interface for new drivers.
Hmm... OK, now, I'm investigating ALSA SoC struct connection. And yes, existing code has deep relationship each other. Thus, fixing/droping seems very dificult... I think each struct has random pointer connection which doesn't care about ALSA SoC's hierarchy.
I would like to indicate my opinion here. My strange point are...
- snd_soc_pcm_runtime has pointer to Card/Codec/Platform, but no CPU
- snd_soc_dai has pointer to [component] and [Codec] [component] is its parent, [Codec] is maybe Hack
dai->codec is something we could get rid of since it is the same as dai->component->codec. And for drivers were we know that it is a CODEC driver you can use snd_soc_component_to_codec(dai->component). For those drivers we know that this returns a valid pointer.
But there are a lot of users which all need to be updated. If you want to do it go ahead. If you do the conversion this should be done by the use of helper functions, so we can change the implementation if necessary.
- [component] has pointer to [card] and [Codec] [card] is understandable, [Codec] is maybe Hack
It seems many struct would like to access to [Codec], thus, each struct has codec pointer (as hack ?), but it is ignoring ALSA SoC hierarchy.
This is a leftover of the old hierarchy where snd_soc_codec was not a sub-struct of snd_soc_component.
It will be more clear if...
IDEA 1
Each component (= CPU/Codec/Platform) has pointer to Card now. How about Card has pointer to each component ? if so, we can access to every struct. Card -> CPU/Codec/Platform, CPU/Codec/Platform -> Card Then, we want to have component related macro/flag
#define component_is_cpu(component) (component->flag & COMPONENT_CPU) #define component_is_codec(component) (component->flag & COMPONENT_CODEC) #define component_is_platform(component) (component->flag & COMPONENT_PLATFORM)
#define component_to_cpu() container_of(xxx) #define component_to_codec() container_of(xxx) #define component_to_platorm() container_of(xxx)
I though about this when doing the refactoring of the ASoC core. But in the end it does not make too much of a difference whether we have a flag field or a CODEC pointer field. The former would have just caused more code churn so I went with the later.
You wouldn't be able to simplify the code by this. You need to replace all component->codec checks by component_is_codec(component). So the overall code complexity stays the same.
The helper macros might still make sense to hide the actual implementation.
IDEA 2
if IDEA1 was OK, snd_soc_pcm_runtime already has card pointer. This means snd_soc_pcm_runtime can access to every component struct.
snd_soc_dai has its parent component. This means snd_soc_dai can access to Card, and Card can access to every component. And then, cpu_dai->codec has no issue if we can use component_is_xxx() macro ?
- if (cpu_dai->codec) + if (component_is_codec(cpu_dai->component))If IDEA1 / IDEA2 are OK, we can cleanup each struct, especially hack-able random pointer ?
In my opinion the flags are just as much a hack as the pointer. In an ideal setup the component does not need to know what type it is. The reason why we need this in ASoC is because the framework has grown over time and we need to support legacy code.
On Fri, Jul 29, 2016 at 11:01:27AM +0200, Lars-Peter Clausen wrote:
In my opinion the flags are just as much a hack as the pointer. In an ideal setup the component does not need to know what type it is. The reason why we need this in ASoC is because the framework has grown over time and we need to support legacy code.
Yes, the pointer is essentially already a flag - having a separate flag really doesn't add a huge amount here. What we need to do is get rid of the need for the flag entirely.
Hi Lars, Mark
In my opinion the flags are just as much a hack as the pointer. In an ideal setup the component does not need to know what type it is. The reason why we need this in ASoC is because the framework has grown over time and we need to support legacy code.
Yes, the pointer is essentially already a flag - having a separate flag really doesn't add a huge amount here. What we need to do is get rid of the need for the flag entirely.
I can agree to remove flag entirely, and actually it is my final target. But we need many steps for it IMO.
I would like to cleanup (= remove random pointer) as 1st step. My 1st concern is that each struct which want to have codec pointer has codec pointer randomly. And it makes component cleanup difficult IMO.
2nd concern is rtd has *codec, *platform, and *dai (for cpu/codec). These are different type of pointer. If platform can have dummy *dai, we can do like below ?
struct snd_soc_pcm_runtime { ... struct snd_soc_dai *cpu; struct snd_soc_dai *codec; struct snd_soc_dai *platform; ... }
Here, we can get original *codec or *platform by using component_to_xxx, because dai already have *component; It can be more cleanup ?
Hi Lars, Mark
My previous mail was missing point...
In my opinion the flags are just as much a hack as the pointer. In an ideal setup the component does not need to know what type it is. The reason why we need this in ASoC is because the framework has grown over time and we need to support legacy code.
Yes, the pointer is essentially already a flag - having a separate flag really doesn't add a huge amount here. What we need to do is get rid of the need for the flag entirely.
I can agree to remove flag entirely, and actually it is my final target. But we need many steps for it IMO.
I would like to cleanup (= remove random pointer) as 1st step. My 1st concern is that each struct which want to have codec pointer has codec pointer randomly. And it makes component cleanup difficult IMO.
2nd concern is rtd has *codec, *platform, and *dai (for cpu/codec). These are different type of pointer. If platform can have dummy *dai, we can do like below ?
struct snd_soc_pcm_runtime { ... struct snd_soc_dai *cpu; struct snd_soc_dai *codec; struct snd_soc_dai *platform; ... }
Here, we can get original *codec or *platform by using component_to_xxx, because dai already have *component; It can be more cleanup ?
I mean current ALSA SoC has duplicate pointer, and using different type of pointer for CPU/Codec/Platform/Card. I agree that we would like to have flag-less style.
If my understanding was correct, we can more cleanup struct relationship if we can exchange pointer style. I think this is related to flag-less style.
For example, current rtd <-> CPU/Codec/Platform relationship is using different type of pointer. It makes flag-less style difficult. But we can use same type of pointer if we can have dummy dai on platform, like above idea. Similar idea is for component.
I think this kind of cleanup is needed, and it makes hotplug support easier. Otherwise, hotplug support will add more random pointer on each struct, and it makes cleanup more difficult.
# Here, I said "flag-less" style, but maybe we need flag somehow to keep # current style ? For example I don't think we can have flag-less cristal-clear # style for CODEC-CODEC support or DPCM etc ...
On 08/02/2016 08:47 AM, Kuninori Morimoto wrote:
Hi Lars, Mark
My previous mail was missing point...
In my opinion the flags are just as much a hack as the pointer. In an ideal setup the component does not need to know what type it is. The reason why we need this in ASoC is because the framework has grown over time and we need to support legacy code.
Yes, the pointer is essentially already a flag - having a separate flag really doesn't add a huge amount here. What we need to do is get rid of the need for the flag entirely.
I can agree to remove flag entirely, and actually it is my final target. But we need many steps for it IMO.
I would like to cleanup (= remove random pointer) as 1st step. My 1st concern is that each struct which want to have codec pointer has codec pointer randomly. And it makes component cleanup difficult IMO.
2nd concern is rtd has *codec, *platform, and *dai (for cpu/codec). These are different type of pointer. If platform can have dummy *dai, we can do like below ?
struct snd_soc_pcm_runtime { ... struct snd_soc_dai *cpu; struct snd_soc_dai *codec; struct snd_soc_dai *platform; ... }
Here, we can get original *codec or *platform by using component_to_xxx, because dai already have *component; It can be more cleanup ?
I mean current ALSA SoC has duplicate pointer, and using different type of pointer for CPU/Codec/Platform/Card. I agree that we would like to have flag-less style.
If my understanding was correct, we can more cleanup struct relationship if we can exchange pointer style. I think this is related to flag-less style.
For example, current rtd <-> CPU/Codec/Platform relationship is using different type of pointer. It makes flag-less style difficult. But we can use same type of pointer if we can have dummy dai on platform, like above idea. Similar idea is for component.
I think this kind of cleanup is needed, and it makes hotplug support easier. Otherwise, hotplug support will add more random pointer on each struct, and it makes cleanup more difficult.
# Here, I said "flag-less" style, but maybe we need flag somehow to keep # current style ? For example I don't think we can have flag-less cristal-clear # style for CODEC-CODEC support or DPCM etc ...
I think moving forward we should get rid of the whole CPU/CODEC/platform concept. This is an outdated view of how the hardware looks like. When ASoC was initially introduce all hardware basically had a CPU side DAI, a CODEC side DAI and a DMA. The DMA was connected to the CPU DAI and the CPU DAI was connected to the CODEC DAI and that was it. The CPU side was also really simple, no signal processing, no signal routing, just the raw audio data directly transferred via DMA (or PIO sometimes) to the CPU DAI controller. And all digital audio was assumed to be in a single digital domain running at them same clock rate.
This no longer reflects todays typical systems. Sometimes you have more than those three components (e.g. additional amplifier IC, BT chip ...), sometimes you less (just a DAC or ADC directly connected to a DMA in the SoC). You often have complex routing and processing capabilities on the host side. Also you have often multiple digital domains with sample-rate converters between them.
Yet still at the core ASoC keeps the CPU/CODEC/platform concept. DPCM e.g. tries to work with these concepts by introducing frontend and backend DAIs and use dummy components to compensate for when the software model does not match the hardware model. This makes it code more complicated than it has to be and less efficient than it can be.
Hi Lars
I think moving forward we should get rid of the whole CPU/CODEC/platform concept. This is an outdated view of how the hardware looks like. When ASoC was initially introduce all hardware basically had a CPU side DAI, a CODEC side DAI and a DMA. The DMA was connected to the CPU DAI and the CPU DAI was connected to the CODEC DAI and that was it. The CPU side was also really simple, no signal processing, no signal routing, just the raw audio data directly transferred via DMA (or PIO sometimes) to the CPU DAI controller. And all digital audio was assumed to be in a single digital domain running at them same clock rate.
This no longer reflects todays typical systems. Sometimes you have more than those three components (e.g. additional amplifier IC, BT chip ...), sometimes you less (just a DAC or ADC directly connected to a DMA in the SoC). You often have complex routing and processing capabilities on the host side. Also you have often multiple digital domains with sample-rate converters between them.
Yet still at the core ASoC keeps the CPU/CODEC/platform concept. DPCM e.g. tries to work with these concepts by introducing frontend and backend DAIs and use dummy components to compensate for when the software model does not match the hardware model. This makes it code more complicated than it has to be and less efficient than it can be.
I agree to your opinion. OTOH, we would like to use/keep existing current all drivers. Thus, I think we need super many small and slow steps. Or, we need new ASoC2 ?
I can agree that we should get rid of current CPU/Codec/Platform, but, removing all of them is a little bit over-kill for me. I think ALSA SoC "framework" should care "component" only, and it doesn't care what it was. OTOH "driver" side can use existing CPU/Codec/Platform and/or AUX/compr etc as helper ? (I'm not sure detail of AUX/compr actually...) And each "component" has "dai".
My image is like this. This allows many / few components, and many / few DAIs. Current DPCM style is automatically supported, and it is easy to add new style device. What do you think ?
/* * DAI has parent (= component) pointer * and rtd interface list_head */ struct snd_soc_dai { ... struct snd_soc_component *component; /* parent */ struct list_head *dai_node; /* struct snd_soc_pcm_runtime */ };
/* * Each component has at least 1 DAI * * CPU/Codec will have many DAIs * Card/Platform and other will have dummy DAI as interface */ struct snd_soc_component { ... struct snd_soc_dai *dais; };
/* * We can use Card/CPU/Codec/Platform/AUX/COMPR helper, * all includes "component". * Of course each driver can create original one, * or don't use helper is no problem */ struct snd_soc_card { ... struct snd_soc_component component; };
struct snd_soc_cpu { ... struct snd_soc_component component; };
struct snd_soc_codec { ... struct snd_soc_component component; };
struct snd_soc_platform { ... struct snd_soc_component component; };
struct snd_soc_aux { ... struct snd_soc_component component; }; struct snd_soc_compr { ... struct snd_soc_component component; };
...
/* * rtd has each DAI interface list. * But it doesn't care what it was */ struct snd_soc_pcm_runtime { ... struct list_head *dai_node_head; /* struct snd_soc_dai */ }
/* * each function just call dai or component function * it doesn't care what it was */ static int soc_pcm_trigger(struct snd_pcm_substream *substream, int cmd) { struct snd_soc_pcm_runtime *rtd = substream->private_data; struct snd_soc_component *component; struct snd_soc_dai *dai; int ret = 0;
/* * call each component's ops */ list_for_each_entry(dai, rtd->xxx) { component = snd_soc_dai_to_component(dai); ret = component->ops->trigger(dai, xxx); if (ret < 0) goto trigger_end; }
trigger_end: return ret; }
On 08/04/2016 04:38 AM, Kuninori Morimoto wrote:
Hi Lars
I think moving forward we should get rid of the whole CPU/CODEC/platform concept. This is an outdated view of how the hardware looks like. When ASoC was initially introduce all hardware basically had a CPU side DAI, a CODEC side DAI and a DMA. The DMA was connected to the CPU DAI and the CPU DAI was connected to the CODEC DAI and that was it. The CPU side was also really simple, no signal processing, no signal routing, just the raw audio data directly transferred via DMA (or PIO sometimes) to the CPU DAI controller. And all digital audio was assumed to be in a single digital domain running at them same clock rate.
This no longer reflects todays typical systems. Sometimes you have more than those three components (e.g. additional amplifier IC, BT chip ...), sometimes you less (just a DAC or ADC directly connected to a DMA in the SoC). You often have complex routing and processing capabilities on the host side. Also you have often multiple digital domains with sample-rate converters between them.
Yet still at the core ASoC keeps the CPU/CODEC/platform concept. DPCM e.g. tries to work with these concepts by introducing frontend and backend DAIs and use dummy components to compensate for when the software model does not match the hardware model. This makes it code more complicated than it has to be and less efficient than it can be.
I agree to your opinion. OTOH, we would like to use/keep existing current all drivers. Thus, I think we need super many small and slow steps. Or, we need new ASoC2 ?
I can agree that we should get rid of current CPU/Codec/Platform, but, removing all of them is a little bit over-kill for me. I think ALSA SoC "framework" should care "component" only, and it doesn't care what it was. OTOH "driver" side can use existing CPU/Codec/Platform and/or AUX/compr etc as helper ? (I'm not sure detail of AUX/compr actually...) And each "component" has "dai".
My image is like this. This allows many / few components, and many / few DAIs. Current DPCM style is automatically supported, and it is easy to add new style device. What do you think ?
To be honest I'd also get rid of DAIs has a top level concept. This image (http://metafoo.de/the_new_asoc.svg) is something I've put together awhile ago how I think we should lay things out if we do a major refactoring of the ASoC core.
The big green and blue boxes are components. Green are On-SoC components, blue are external components.
The light grey boxes are domains. Domains have certain properties, like e.g. samplerates. There are different types of domains and different types have different kinds of properties. E.g. in the PCM domain we care about the memory layout of the samples in addition to the samplerate and the number of channels, while in the digital domain however we do no longer care about the memory layout since here data is processed one sample at a time. In the I2S link domain we care about which I2S mode is used (I2S, LJ, ...), while in a SPDIF link domain we don't care about such properties.
The dark grey boxes are bridges between domains and they translate properties from one domain to another. This can either be straight forward propagation like a PCM device where the samplerate in the source and target domain are the same, or it can modify the properties, e.g. a samplerate converter between two digital domains will change the samplerate according to the interpolation/decimation factor of the SRC.
In addition to just translating the properties a bridge can also translate the constraints on properties from one domain to another or even add its own constraints. E.g. if a CODEC has a constraint that it can run either with 48kHz or 96kHz this constraint is propagated through the bridges to the PCM devices so that the userspace application using the PCM is aware of this. Bridges can also add their own constraints e.g. a SRC might have multiple interpolation/decimation rates available, so it might say the samplerate in the target domain must be 1x or 2x that of the source domain. Going back to the example that means for a CODEC that supports either 48kHz or 96kHz and there is a SRC with interpolation factors of 1 or 2 then the PCM device will present the list of either 24kHz, 48kHz or 96kHz to the userspace application.
A bridge is also not limited to one source and one sink. It can have multiple sources and multiple sinks e.g. for a crossbar with a complex routing matrix.
Obviously we can't convert everything at the same time and it will take a lot of time and effort to update all drivers to this new model. This is where the legacy bridge kicks in which still keeps the concept of 1 CPU, 1 CODEC, 1 platform. If you want to use the advanced features of the new framework you have to update your driver, if you are OK with the current set of features just keep the drivers the way they are and use the legacy bridge that is automatically managed by the ASoC core.
On Thu, Aug 04, 2016 at 10:21:10AM +0200, Lars-Peter Clausen wrote:
To be honest I'd also get rid of DAIs has a top level concept. This image (http://metafoo.de/the_new_asoc.svg) is something I've put together awhile ago how I think we should lay things out if we do a major refactoring of the ASoC core.
I do think they have a useful purpose in representing the edges of chips in a convenient bundle and inject configuration. I too would like to see them less prominent in the code internals but they are useful as an interface.
Obviously we can't convert everything at the same time and it will take a lot of time and effort to update all drivers to this new model. This is where the legacy bridge kicks in which still keeps the concept of 1 CPU, 1 CODEC, 1 platform. If you want to use the advanced features of the new framework you have to update your driver, if you are OK with the current set of features just keep the drivers the way they are and use the legacy bridge that is automatically managed by the ASoC core.
I actually keep thinking that it might be easier to refactor the simple CPU plus platform SoCs into components than anything else - there's going to be gotchas in there but it seems like a useful string to pull at. Though realistically if we're going to get companies working on this it's probably easier to get DPCM people started off.
Hi Lars
Thank you for your idea
To be honest I'd also get rid of DAIs has a top level concept. This image (http://metafoo.de/the_new_asoc.svg) is something I've put together awhile ago how I think we should lay things out if we do a major refactoring of the ASoC core.
I think most important ideas on this image is ALSA SoC new style has - "component" can have many "domain" - "domain" can connect to other "domain" by using "bridge" - "bridge" can connect to many "domain"s - "domain" has certain properties, and "bridge" translate it to other "domain"
Here, my opinions are... - Actually, I want to have more and more simple style. If "domain" can have its format / frequency etc, "domain" list/array without "bridge" is very enough ? or no distinction in "domain" and "bridge" ? because we can have many "domain" on new style ? something like struct snd_soc_node; struct snd_soc_domain { struct snd_soc_node; }; struct snd_soc_bridge { struct snd_soc_node; }; This case ALSA SoC cares snd_soc_node only; - If we have "bridge", I think "component property" can be "component needs to setup connected bridge's parameter in init time", and "component should follow bridge's parameter when playback time" is nicer ? It is easy to care about "crossbar" case ?
On your image, DMA / Audio controller / CODEC are component inheritance struct, Playback / Capture / Digital / Link / Analog are domain inheritance struct, PCM / DAI / DAC / ADC / crossbar are bridge inheritance struct. ALSA SoC framework cares "component", "domain", "bridge" only. Each driver can create original structure if it was based on "component" or "domain" or "bridge", or it can use soc-core's basic helper (= CPU / Codec / Plaform etc). Then, it is easy to add new device / connection in the future ? And it is easy to use OF graph style on DT ?
struct snd_soc_component {
/* we want to have rtd list here for hotplug purpose ? */
struct list_head component_domain_head; };
struct snd_soc_domain {
struct snd_soc_component *component; /* parent */
struct list_head component_domain; };
struct snd_soc_bridge {
/* connected domain will sets available parameter */ unsigned int fmt, ...;
struct list_head runtime_bridge; struct snd_soc_domain *domains[0] /* domain array */ };
struct snd_soc_pcm_runtime {
struct list_head runtime_bridge_head; };
static int soc_pcm_trigger(struct snd_pcm_substream *substream, int cmd) { struct snd_soc_pcm_runtime *rtd = substream->private_data; struct snd_soc_bridge *bridge; struct snd_soc_domain *domain;
for_each_bridge(bridge, rtd) { for_each_domain(domain, bridge) { domain->trigger(domain, bridge, rtd); } } ... }
The dark grey boxes are bridges between domains and they translate properties from one domain to another. This can either be straight forward propagation like a PCM device where the samplerate in the source and target domain are the same, or it can modify the properties, e.g. a samplerate converter between two digital domains will change the samplerate according to the interpolation/decimation factor of the SRC.
In addition to just translating the properties a bridge can also translate the constraints on properties from one domain to another or even add its own constraints. E.g. if a CODEC has a constraint that it can run either with 48kHz or 96kHz this constraint is propagated through the bridges to the PCM devices so that the userspace application using the PCM is aware of this. Bridges can also add their own constraints e.g. a SRC might have multiple interpolation/decimation rates available, so it might say the samplerate in the target domain must be 1x or 2x that of the source domain. Going back to the example that means for a CODEC that supports either 48kHz or 96kHz and there is a SRC with interpolation factors of 1 or 2 then the PCM device will present the list of either 24kHz, 48kHz or 96kHz to the userspace application.
I think this is current DPCM's dai_link->be_hw_params_fixup portion ? I think "bridge" can/should handle it ?
A bridge is also not limited to one source and one sink. It can have multiple sources and multiple sinks e.g. for a crossbar with a complex routing matrix.
Do you mean runtime-path-change by this "crossbar" ?
--A1-+-B1-- | --A2-+-B2--
For example, like this ??
> amixer set "A1-B1" ... // connect > aplay xxx > amixer set "A1 B1" ... // disconnect > amixer set "A1-B2" ... // connect > aplay xxx
Or having fixed path card ?
> aplay -D plughw:0,0 xxx // A1-B1 path > aplay -D plughw:0,1 xxx // A1-B2 path > aplay -D plughw:0,2 xxx // B1-A1 path > aplay -D plughw:0,3 xxx // B1-B2 path ...
Obviously we can't convert everything at the same time and it will take a lot of time and effort to update all drivers to this new model. This is where the legacy bridge kicks in which still keeps the concept of 1 CPU, 1 CODEC, 1 platform. If you want to use the advanced features of the new framework you have to update your driver, if you are OK with the current set of features just keep the drivers the way they are and use the legacy bridge that is automatically managed by the ASoC core.
I think it will be super x 10 long term project Because of it, we need to consider about - "Big picture" and "details" of final style - consider how to goto there from current style
Best regards --- Kuninori Morimoto
On Thu, Aug 04, 2016 at 02:38:33AM +0000, Kuninori Morimoto wrote:
I agree to your opinion. OTOH, we would like to use/keep existing current all drivers. Thus, I think we need super many small and slow steps. Or, we need new ASoC2 ?
Small steps is how we do things in the kernel.
OTOH "driver" side can use existing CPU/Codec/Platform and/or AUX/compr etc as helper ? (I'm not sure detail of AUX/compr actually...) And each "component" has "dai".
Auxiliary devices are just CODECs that don't have DAIs hooked up. Compressed DAIs are just DAIs for compressed audio.
participants (6)
-
Kuninori Morimoto -
Lars-Peter Clausen -
Mark Brown -
Takashi Iwai -
Takashi Sakamoto -
Vinod Koul