| Message ID | 20220331000449.41062-1-povik+lin@cutebit.org |
|---|---|
| Headers | show |
| Series | Apple Macs machine-level ASoC driver | expand |
On Thu, Mar 31, 2022 at 02:04:46AM +0200, Martin Povišer wrote: > Add a new ASoC card callback for filtering the kcontrols of the card's > constituent components. This lets the card take over some of the > controls, deciding their value instead of leaving it up to userspace. Define "filter". What is this trying to accomplish? As a matter of policy we don't put use case configuration in the kernel, the goal is to avoid having to update the kernel when people decide to do new things with their userspace. > Also, and here's the HACK: part, move dapm_new_widgets call in front > of the card's late_probe call. This way all kcontrols should have been > created (and are safe to use) by the time late_probe is called. This will break any card that adds new controls, you could add a second call earlier but deleting the existing call is going to break other users.
On Thu, Mar 31, 2022 at 02:04:49AM +0200, Martin Povišer wrote: > --- /dev/null > +++ b/sound/soc/apple/macaudio.c > @@ -0,0 +1,597 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * ASoC machine driver for Apple Silicon Macs > + * Please make the entire comment a C++ one so things look more intentional. > + /* CPU side is bit and frame clock master, I2S with both clocks inverted */ Please refer to clock providers here. > + ret = of_property_read_string(np, "link-name", &link->name); > + if (ret) { > + dev_err(card->dev, "Missing link name\n"); > + goto err_put_np; > + } This doesn't look like it's mandatory in the binding. > +static int macaudio_init(struct snd_soc_pcm_runtime *rtd) > +{ > + struct snd_soc_card *card = rtd->card; > + struct macaudio_snd_data *ma = snd_soc_card_get_drvdata(card); > + struct snd_soc_component *component; > + int ret, i; > + > + if (rtd->num_codecs > 1) { > + ret = macaudio_assign_tdm(rtd); > + if (ret < 0) > + return ret; > + } > + > + for_each_rtd_components(rtd, i, component) > + snd_soc_component_set_jack(component, &ma->jack, NULL); What is the jack configuration this is attempting to describe? It looks like you have some dedicated speaker driver devices which are going to get attached to jacks here for example. > +} macaudio_kctlfixes[] = { > + {"* ASI1 Sel", "Left"}, > + {"* ISENSE Switch", "Off"}, > + {"* VSENSE Switch", "Off"}, > + { } > +}; > + > +static bool macaudio_kctlfix_matches(const char *pattern, const char *name) > +{ > + if (pattern[0] == '*') { > + int namelen, patternlen; > + > + pattern++; > + if (pattern[0] == ' ') > + pattern++; > + > + namelen = strlen(name); > + patternlen = strlen(pattern); > + > + if (namelen > patternlen) > + name += (namelen - patternlen); > + } > + > + return !strcmp(name, pattern); > +} This looks worryingly like use case configuration. > +/* > + * Maybe this could be a general ASoC function? > + */ > +static void snd_soc_kcontrol_set_strval(struct snd_soc_card *card, > + struct snd_kcontrol *kcontrol, const char *strvalue) No, we should not be setting user visible control values from the kernel. This shouldn't be a machine driver function either. What are you trying to accomplish here?
> On 31. 3. 2022, at 13:59, Mark Brown <broonie@kernel.org> wrote: > > On Thu, Mar 31, 2022 at 02:04:49AM +0200, Martin Povišer wrote: > >> --- /dev/null >> +++ b/sound/soc/apple/macaudio.c >> @@ -0,0 +1,597 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * ASoC machine driver for Apple Silicon Macs >> + * > > Please make the entire comment a C++ one so things look more > intentional. > >> + /* CPU side is bit and frame clock master, I2S with both clocks inverted */ > > Please refer to clock providers here. > >> + ret = of_property_read_string(np, "link-name", &link->name); >> + if (ret) { >> + dev_err(card->dev, "Missing link name\n"); >> + goto err_put_np; >> + } > > This doesn't look like it's mandatory in the binding. Good catch! >> +static int macaudio_init(struct snd_soc_pcm_runtime *rtd) >> +{ >> + struct snd_soc_card *card = rtd->card; >> + struct macaudio_snd_data *ma = snd_soc_card_get_drvdata(card); >> + struct snd_soc_component *component; >> + int ret, i; >> + >> + if (rtd->num_codecs > 1) { >> + ret = macaudio_assign_tdm(rtd); >> + if (ret < 0) >> + return ret; >> + } >> + >> + for_each_rtd_components(rtd, i, component) >> + snd_soc_component_set_jack(component, &ma->jack, NULL); > > What is the jack configuration this is attempting to describe? It looks > like you have some dedicated speaker driver devices which are going to > get attached to jacks here for example. We know the speakers will ignore the set_jack call. There’s one jack and this way we know the jack codec will attach to it, for speakers it’s a no-op. (If you prefer I will special-case it to the jack codec.) >> +} macaudio_kctlfixes[] = { >> + {"* ASI1 Sel", "Left"}, >> + {"* ISENSE Switch", "Off"}, >> + {"* VSENSE Switch", "Off"}, >> + { } >> +}; >> + >> +static bool macaudio_kctlfix_matches(const char *pattern, const char *name) >> +{ >> + if (pattern[0] == '*') { >> + int namelen, patternlen; >> + >> + pattern++; >> + if (pattern[0] == ' ') >> + pattern++; >> + >> + namelen = strlen(name); >> + patternlen = strlen(pattern); >> + >> + if (namelen > patternlen) >> + name += (namelen - patternlen); >> + } >> + >> + return !strcmp(name, pattern); >> +} > > This looks worryingly like use case configuration. I go over this in the cover letter! This is fixing the TDM slot selection and disabling voltage/current sensing on the speaker amp codecs, which have no business being exposed to userspace as options. This is not use case, this not letting people blow their speakers from userspace. > >> +/* >> + * Maybe this could be a general ASoC function? >> + */ >> +static void snd_soc_kcontrol_set_strval(struct snd_soc_card *card, >> + struct snd_kcontrol *kcontrol, const char *strvalue) > > No, we should not be setting user visible control values from the > kernel. This shouldn't be a machine driver function either. What are > you trying to accomplish here? See above. Martin
> On 31. 3. 2022, at 14:08, Martin Povišer <povik@cutebit.org> wrote: > >> >> On 31. 3. 2022, at 13:59, Mark Brown <broonie@kernel.org> wrote: >> >> On Thu, Mar 31, 2022 at 02:04:49AM +0200, Martin Povišer wrote: >> >>> --- /dev/null >>> +++ b/sound/soc/apple/macaudio.c >>> @@ -0,0 +1,597 @@ >>> +// SPDX-License-Identifier: GPL-2.0-only >>> +/* >>> + * ASoC machine driver for Apple Silicon Macs >>> + * (snip) >>> +/* >>> + * Maybe this could be a general ASoC function? >>> + */ >>> +static void snd_soc_kcontrol_set_strval(struct snd_soc_card *card, >>> + struct snd_kcontrol *kcontrol, const char *strvalue) >> >> No, we should not be setting user visible control values from the >> kernel. This shouldn't be a machine driver function either. What are >> you trying to accomplish here? > > See above. > > Martin One thing I didn’t point out. The controls we are setting here are not visible from userspace. That’s the point of the ‘filter’ card method I am trying to establish in the other commit. With it, the card decides which controls are okay to be exported and which should be hidden. Here we are only setting hidden controls. Martin
On Thu, Mar 31, 2022 at 02:04:44AM +0200, Martin Povišer wrote: > I put together a machine-level ASoC driver for recent Apple Macs (the > ones with ARM64 SoCs) and want to gauge opinions. This would be a bit easier to review with a description of the hardware. > Commit 2 adds a new ASoC card method (filter_controls) to let the card > prevent some codec kcontrols from being visible to userspace. For example > the TAS2770 speaker amp driver would be happy to expose TDM slot selection > and ISENSE/VSENSE enables which is ridiculous. I am all ears on how to > make the patch acceptable to upstream. The broad issue here is that what you consider ridiculous someone else might have some bright ideas for configuring dynamically - if things are being exposed for dynamic configuration it's probably because someone wanted them, if the control is genuinely useless then it should just be removed. Rather than getting in the way of people's policy arguments about how to set things we expose them to userspace and let userspace worry about it, usually with the help of UCM files. The general userspace model is that people interact with their sound server more than the hardware card. This is also helpful for people developing use cases, it means they're not having to get the kernel rebuilt to tune things. The TDM swap thing you're mentioning looks like it's a left/right selection which people do use sometimes as a way of doing mono mixes and reorientation. The ISENSE/VSENSE is less obvious, though it's possible there's issues with not having enough slots on a heavily used TDM bus or sometimes disabling the speaker protection processing for whatever reason.
On Thu, Mar 31, 2022 at 02:08:51PM +0200, Martin Povišer wrote: > > On 31. 3. 2022, at 13:59, Mark Brown <broonie@kernel.org> wrote: > >> + for_each_rtd_components(rtd, i, component) > >> + snd_soc_component_set_jack(component, &ma->jack, NULL); > > What is the jack configuration this is attempting to describe? It looks > > like you have some dedicated speaker driver devices which are going to > > get attached to jacks here for example. > We know the speakers will ignore the set_jack call. There’s one jack and > this way we know the jack codec will attach to it, for speakers it’s a no-op. > (If you prefer I will special-case it to the jack codec.) It would be better to special case, this looks obviously wrong and will break if someone adds error handling. > >> + return !strcmp(name, pattern); > >> +} > > This looks worryingly like use case configuration. > I go over this in the cover letter! This is fixing the TDM slot selection > and disabling voltage/current sensing on the speaker amp codecs, which have > no business being exposed to userspace as options. This is not use case, > this not letting people blow their speakers from userspace. Your comments in the cover letter are all pretty vague too, that just says that these controls are "ridiculous" which isn't terribly specific about what the actual goal is. If it's just "I can't see why anyone would want to configure this" then that's a decision you're taking about what people might want to do which is broadly a use case configuration and the control should be left there in case someone comes up with an idea.
> On 31. 3. 2022, at 14:34, Mark Brown <broonie@kernel.org> wrote: > > On Thu, Mar 31, 2022 at 02:04:44AM +0200, Martin Povišer wrote: > >> I put together a machine-level ASoC driver for recent Apple Macs (the >> ones with ARM64 SoCs) and want to gauge opinions. > > This would be a bit easier to review with a description of the hardware. The typical hardware configuration the driver is supposed to be used with is this: * SoC with couple of I2S ports. * Array of speakers with individual speaker amp chips, hooked to a single I2S bus or split between two of the SoC’s I2S ports. Speakers can be one, two, four or six in total. The speaker amp chips resemble either TAS2770 or TAS2764. * Jack codec hooked to a separate I2S port, operating independently. (Codec driver supports set_jack.) The example in the binding patch describes an actual arrangement on one piece of hardware. >> Commit 2 adds a new ASoC card method (filter_controls) to let the card >> prevent some codec kcontrols from being visible to userspace. For example >> the TAS2770 speaker amp driver would be happy to expose TDM slot selection >> and ISENSE/VSENSE enables which is ridiculous. I am all ears on how to >> make the patch acceptable to upstream. > > The broad issue here is that what you consider ridiculous someone else > might have some bright ideas for configuring dynamically - if things are > being exposed for dynamic configuration it's probably because someone > wanted them, if the control is genuinely useless then it should just be > removed. Rather than getting in the way of people's policy arguments > about how to set things we expose them to userspace and let userspace > worry about it, usually with the help of UCM files. The general > userspace model is that people interact with their sound server more > than the hardware card. This is also helpful for people developing use > cases, it means they're not having to get the kernel rebuilt to tune > things. Well but these are codec drivers reused on different systems, it can both be 'not genuinely useless’ on some system and ridiculous to leave open on the systems I am trying to write drivers for. > The TDM swap thing you're mentioning looks like it's a left/right > selection which people do use sometimes as a way of doing mono mixes and > reorientation. The ISENSE/VSENSE is less obvious, though it's possible > there's issues with not having enough slots on a heavily used TDM bus or > sometimes disabling the speaker protection processing for whatever > reason. Not only that. On TAS2770 the default value for ‘ASI1 Sel’ is ‘I2C offset’ meaning the speaker amp driver ignores my set_tdm_slot calls. If you tell me it’s okay to change that behaviour and it won’t be considered backwards compatibility breaking, that would be part of the solution I am seeking here. But even then, what for example if the system has a single speaker (as it does on the Mac mini to be covered by this driver) and the I2S bus is left undriven for the duration of unused TDM slots? That may genuinely pose a risk of people blowing their speakers by switching something in alsamixer. Now I can actually make sure the I2S data lines are always zeroed out in the ASoC platform driver, but I would rather not even have to tie these loose ends for a control there’s no reason to expose in the first place (again, on this system). The ISENSE/VSENSE controls are also actually useless on these systems as we are not doing anything to pick up the measured values (which are sent back over the I2S lines). I don’t know if there can be driver conflict between two speaker amps trying to drive the I2S lines at the same time should the user happen to enable SENSE facilities on more than one of them. Now I can grudgingly study that and rule it out but I would rather hide the controls altogether. That’s the reasoning anyway. To reiterate, seems to me the controls are useless/confusing at best and dangerous at worst. Martin
On 31/03/2022 21.34, Mark Brown wrote: > On Thu, Mar 31, 2022 at 02:04:44AM +0200, Martin Povišer wrote: > >> I put together a machine-level ASoC driver for recent Apple Macs (the >> ones with ARM64 SoCs) and want to gauge opinions. > > This would be a bit easier to review with a description of the hardware. > >> Commit 2 adds a new ASoC card method (filter_controls) to let the card >> prevent some codec kcontrols from being visible to userspace. For example >> the TAS2770 speaker amp driver would be happy to expose TDM slot selection >> and ISENSE/VSENSE enables which is ridiculous. I am all ears on how to >> make the patch acceptable to upstream. > > The broad issue here is that what you consider ridiculous someone else > might have some bright ideas for configuring dynamically - if things are > being exposed for dynamic configuration it's probably because someone > wanted them, if the control is genuinely useless then it should just be > removed. Rather than getting in the way of people's policy arguments > about how to set things we expose them to userspace and let userspace > worry about it, usually with the help of UCM files. The general > userspace model is that people interact with their sound server more > than the hardware card. This is also helpful for people developing use > cases, it means they're not having to get the kernel rebuilt to tune > things. The problem with this model is that, in particular in the case of speaker amps, incorrect settings can cause your speakers to blow up. This has been a longstanding problem with ASoC platforms (I should know, I *melted* the speakers in a Chromebook by toggling the wrong alsamixer control once, it even warped the external case, all without making any audible noise). It's the kernel's job to ensure that broadly exposed user controls are safe and cannot be used to cause hardware damage; if that is possible, then that's a kernel security vulnerability worthy of a CVE, in my opinion. I think this idea of exposing what is effectively raw codec chip registers as ALSA controls that is so popular these days was a terrible idea from the start, and only makes some sense within the world of highly integrated vendor-controlled embedded platforms running kiosk-style software with no user control. It is completely unsuitable for a desktop Linux system, since it means users *will* destroy their hardware accidentally. So, some way or another, whatever is exposed has to be sanitized so that it can't go outside the envelope of what is safe for the hardware design. That cannot be known at the level of codec chips and speaker amp chips; it requires platform integration knowledge. That knowledge is what is (intended to be) encoded in the macaudio driver. It's supposed to know how to drive the underlying codec chips and disable access to things that don't make any sense on the platform, and expose controls to the user that are reasonable for what a user would want to do on that specific hardware platform, and no more.
On Thu, Mar 31, 2022 at 03:28:12PM +0200, Martin Povišer wrote: > > On 31. 3. 2022, at 14:34, Mark Brown <broonie@kernel.org> wrote: > > The broad issue here is that what you consider ridiculous someone else > > might have some bright ideas for configuring dynamically - if things are > > being exposed for dynamic configuration it's probably because someone > > wanted them, if the control is genuinely useless then it should just be > Well but these are codec drivers reused on different systems, it can both > be 'not genuinely useless’ on some system and ridiculous to leave open on > the systems I am trying to write drivers for. It wouldn't be the first time that we've had someone turn up with a new idea for how to configure an already existing bit of hardware, part of the reason for this approach is that people do get surprised by user creativity with their systems. > > The TDM swap thing you're mentioning looks like it's a left/right > > selection which people do use sometimes as a way of doing mono mixes and > > reorientation. The ISENSE/VSENSE is less obvious, though it's possible > > there's issues with not having enough slots on a heavily used TDM bus or > > sometimes disabling the speaker protection processing for whatever > > reason. > Not only that. On TAS2770 the default value for ‘ASI1 Sel’ is ‘I2C offset’ > meaning the speaker amp driver ignores my set_tdm_slot calls. If you tell > me it’s okay to change that behaviour and it won’t be considered backwards > compatibility breaking, that would be part of the solution I am seeking > here. Having the default state be muted or not routed is quite common, UCM files or equivalent are typically required for embedded style hardware like this. > But even then, what for example if the system has a single speaker (as it > does on the Mac mini to be covered by this driver) and the I2S bus is left > undriven for the duration of unused TDM slots? That may genuinely pose > a risk of people blowing their speakers by switching something in alsamixer. Right, so that's a more sensible and valid use case. We do have the platform_max feature available for precisely this reason - that's probably more appropriate here since if there's a danger of people blowing their speaker with a floating input they could also blow their speaker with just a very loud audio signal so limiting the volume people can set on the speaker driver seems sensible and would also cover them for misrouting. Whatever the device might pick up from noise on an undriven bus could also be played as audio down the bus. This does become a little fun with speaker protection as we'd want to raise the kernel limit so that userspace can dynamically manage the volume to contorl power (though that might be done with software control), but it's easy enoguh to raise limits later. On the other hand it seems like userspace might reasonably choose to do a mono mix for this output entirely in software, in which case telling the speaker amp to pick up one channel would make sense, or to just play out a stereo signal over I2S and have the amplifier do a mono mix and I'm not seeing why we'd force one or the other in the machine driver. > The ISENSE/VSENSE controls are also actually useless on these systems as we > are not doing anything to pick up the measured values (which are sent back > over the I2S lines). I don’t know if there can be driver conflict between Presumably someone might want to work on figuring that out though, and from a hardware safety point of view it would be better if they did. > two speaker amps trying to drive the I2S lines at the same time should > the user happen to enable SENSE facilities on more than one of them. > Now I can grudgingly study that and rule it out but I would rather hide > the controls altogether. Yes, having two devices driving the bus at the same time wouldn't be great. How is the TDM slot selection for the signals done in the hardware, I'm not seeing anything immediately obvious in the driver? I'd have thought that things would be implemented such that you could implement speaker protection on all speakers simultaneously but perhaps not. > That’s the reasoning anyway. To reiterate, seems to me the controls > are useless/confusing at best and dangerous at worst. I'm just not seeing an issue for the slot selection.
On Thu, Mar 31, 2022 at 10:28:56PM +0900, Hector Martin wrote: > The problem with this model is that, in particular in the case of > speaker amps, incorrect settings can cause your speakers to blow up. > This has been a longstanding problem with ASoC platforms (I should know, > I *melted* the speakers in a Chromebook by toggling the wrong alsamixer > control once, it even warped the external case, all without making any > audible noise). Yes, that's why we have platform_max - it was added for use with Chromebooks originally, someone else had the same idea you did. It's used less often than I'd like since most embedded systems and even things like Chromebooks have a software model where the actual sound card isn't accessible to normal users but that's not the case once you try to run a general purpose distro on there. > kiosk-style software with no user control. It is completely unsuitable > for a desktop Linux system, since it means users *will* destroy their > hardware accidentally. So, some way or another, whatever is exposed has > to be sanitized so that it can't go outside the envelope of what is safe > for the hardware design. That cannot be known at the level of codec > chips and speaker amp chips; it requires platform integration knowledge. Yes, we should be trying to exclude configurations that could be physically destructive but that's not what had been articulated and like I said in reply to his last mail it's really not clear to me that what's being proposed would actually accomplish the intended goal. Targeted restrictions that protect the system are fine and good, random "why would anyone want this?" or "this is how you accomplish use case X" ones are not since we do get users turning up with new ideas. This is one reason why it's important to articulate what the intended goal of changes is, what you've written above is perfectly fine and reasonable but there was nothing about this in the original changelogs, just statements about how silly it would be to configure these controls.
> On 31. 3. 2022, at 16:18, Mark Brown <broonie@kernel.org> wrote: > > On Thu, Mar 31, 2022 at 03:28:12PM +0200, Martin Povišer wrote: >>> On 31. 3. 2022, at 14:34, Mark Brown <broonie@kernel.org> wrote: > >>> The broad issue here is that what you consider ridiculous someone else >>> might have some bright ideas for configuring dynamically - if things are >>> being exposed for dynamic configuration it's probably because someone >>> wanted them, if the control is genuinely useless then it should just be > >> Well but these are codec drivers reused on different systems, it can both >> be 'not genuinely useless’ on some system and ridiculous to leave open on >> the systems I am trying to write drivers for. > > It wouldn't be the first time that we've had someone turn up with a new > idea for how to configure an already existing bit of hardware, part of > the reason for this approach is that people do get surprised by user > creativity with their systems. > >>> The TDM swap thing you're mentioning looks like it's a left/right >>> selection which people do use sometimes as a way of doing mono mixes and >>> reorientation. The ISENSE/VSENSE is less obvious, though it's possible >>> there's issues with not having enough slots on a heavily used TDM bus or >>> sometimes disabling the speaker protection processing for whatever >>> reason. > >> Not only that. On TAS2770 the default value for ‘ASI1 Sel’ is ‘I2C offset’ >> meaning the speaker amp driver ignores my set_tdm_slot calls. If you tell >> me it’s okay to change that behaviour and it won’t be considered backwards >> compatibility breaking, that would be part of the solution I am seeking >> here. > > Having the default state be muted or not routed is quite common, UCM > files or equivalent are typically required for embedded style hardware > like this. > >> But even then, what for example if the system has a single speaker (as it >> does on the Mac mini to be covered by this driver) and the I2S bus is left >> undriven for the duration of unused TDM slots? That may genuinely pose >> a risk of people blowing their speakers by switching something in alsamixer. > > Right, so that's a more sensible and valid use case. We do have the > platform_max feature available for precisely this reason - that's > probably more appropriate here since if there's a danger of people > blowing their speaker with a floating input they could also blow their > speaker with just a very loud audio signal so limiting the volume people > can set on the speaker driver seems sensible and would also cover them > for misrouting. Whatever the device might pick up from noise on an > undriven bus could also be played as audio down the bus. This does > become a little fun with speaker protection as we'd want to raise the > kernel limit so that userspace can dynamically manage the volume to > contorl power (though that might be done with software control), but > it's easy enoguh to raise limits later. > > On the other hand it seems like userspace might reasonably choose to do > a mono mix for this output entirely in software, in which case telling > the speaker amp to pick up one channel would make sense, or to just play > out a stereo signal over I2S and have the amplifier do a mono mix and > I'm not seeing why we'd force one or the other in the machine driver. Granted. If we make sure the volume caps are there to prevent damage under arbitrary input (which we should anyway) that covers slot misconfiguration too. >> The ISENSE/VSENSE controls are also actually useless on these systems as we >> are not doing anything to pick up the measured values (which are sent back >> over the I2S lines). I don’t know if there can be driver conflict between > > Presumably someone might want to work on figuring that out though, and > from a hardware safety point of view it would be better if they did. > >> two speaker amps trying to drive the I2S lines at the same time should >> the user happen to enable SENSE facilities on more than one of them. >> Now I can grudgingly study that and rule it out but I would rather hide >> the controls altogether. > > Yes, having two devices driving the bus at the same time wouldn't be > great. How is the TDM slot selection for the signals done in the > hardware, I'm not seeing anything immediately obvious in the driver? > I'd have thought that things would be implemented such that you could > implement speaker protection on all speakers simultaneously but perhaps > not. I don’t know. I would have to go study the details of this. Should I see if I can find a combination of ‘ASI1 Sel’ ‘VSENSE’ ‘ISENSE’ settings that would lead to driver conflict on one of the models, or is there a chance we could hide those controls just on the basis of ‘it doesn’t do anything usable and is possibly dangerous’? >> That’s the reasoning anyway. To reiterate, seems to me the controls >> are useless/confusing at best and dangerous at worst. > > I'm just not seeing an issue for the slot selection. Yeah, agreed there’s no (damage) issue as we should to proper volume caps anyway. Martin
On Thu, Mar 31, 2022 at 05:04:32PM +0200, Martin Povišer wrote: > > On 31. 3. 2022, at 16:18, Mark Brown <broonie@kernel.org> wrote: > > Yes, having two devices driving the bus at the same time wouldn't be > > great. How is the TDM slot selection for the signals done in the > > hardware, I'm not seeing anything immediately obvious in the driver? > > I'd have thought that things would be implemented such that you could > > implement speaker protection on all speakers simultaneously but perhaps > > not. > I don’t know. I would have to go study the details of this. Should I see > if I can find a combination of ‘ASI1 Sel’ ‘VSENSE’ ‘ISENSE’ settings > that would lead to driver conflict on one of the models, or is there > a chance we could hide those controls just on the basis of ‘it doesn’t > do anything usable and is possibly dangerous’? If ISENSE and VSENSE output are controlled by the same mux as routing then we should lock one of the controls out for at least stereo devices (it might be a good idea to check if the output is actually high Z when ISENSE and VSENSE are off rather than just driving zeros, if not it definitely has to be the routing control). My instinct is that it's better to preserve the ability to implement speaker protection in future since that is something that'd be broadly useful, especially if someone comes up with a generic speaker protection implementation in which case there should be an awful lot of systems out there which could benefit. > >> That’s the reasoning anyway. To reiterate, seems to me the controls > >> are useless/confusing at best and dangerous at worst. > > I'm just not seeing an issue for the slot selection. > Yeah, agreed there’s no (damage) issue as we should to proper volume > caps anyway. Though see above about how ISENSE/VSENSE output slot is controlled I guess :/
On Thu, Mar 31, 2022 at 02:04:47AM +0200, Martin Povišer wrote: > +#if 0 > dev_err(rtd->card->dev, > "N cpus to M codecs link is not supported yet\n"); > return -EINVAL; > +#endif > + cpu_dai = asoc_rtd_to_cpu(rtd, 0); We need to figure out an interface for describing which CODEC/CPU combinations are connected to each other. I'm not seeing a great way to do that right now, probably some side data table is going to be needed, or perhaps the CPU DAI drivers can be persuaded to only have one DAI actually register and claim to support more channels? I'm not sure how a configuraiton like this is going to work at userspace level if the multiple CPU DAIs end up being visible...
On Thu, 31 Mar 2022 02:04:44 +0200, Martin Povišer wrote: > I put together a machine-level ASoC driver for recent Apple Macs (the > ones with ARM64 SoCs) and want to gauge opinions. > > Commit 1 is the binding. It is some subset of simple-audio-card with > the extra distinction of allowing multiple CPU/CODEC DAIs per a DAI > link. I want to draw special attention to the issue of describing > speaker topologies. The way it now works is that the driver expects > the speakers to be declared in a fixed order in the sound-dai= list. > This populates a topology the driver expects on a particular machine > model. Mark (in CC) has made the suggestion of keeping the topology > descriptions with the codec nodes themselves in some generic manner, > akin to how sound-name-prefix= already helps identify codecs to the > user. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [4/5] ASoC: Introduce snd_soc_of_get_dai_link_cpus commit: 900dedd7e47cc3f8d93dfa0ae6ac6cf49eda0c97 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
> On 31. 3. 2022, at 17:36, Mark Brown <broonie@kernel.org> wrote: > > On Thu, Mar 31, 2022 at 05:04:32PM +0200, Martin Povišer wrote: >>> On 31. 3. 2022, at 16:18, Mark Brown <broonie@kernel.org> wrote: > >>> Yes, having two devices driving the bus at the same time wouldn't be >>> great. How is the TDM slot selection for the signals done in the >>> hardware, I'm not seeing anything immediately obvious in the driver? >>> I'd have thought that things would be implemented such that you could >>> implement speaker protection on all speakers simultaneously but perhaps >>> not. > >> I don’t know. I would have to go study the details of this. Should I see >> if I can find a combination of ‘ASI1 Sel’ ‘VSENSE’ ‘ISENSE’ settings >> that would lead to driver conflict on one of the models, or is there >> a chance we could hide those controls just on the basis of ‘it doesn’t >> do anything usable and is possibly dangerous’? > > If ISENSE and VSENSE output are controlled by the same mux as routing > then we should lock one of the controls out for at least stereo devices > (it might be a good idea to check if the output is actually high Z when > ISENSE and VSENSE are off rather than just driving zeros, if not it > definitely has to be the routing control). My instinct is that it's > better to preserve the ability to implement speaker protection in future > since that is something that'd be broadly useful, especially if someone > comes up with a generic speaker protection implementation in which case > there should be an awful lot of systems out there which could benefit. Sorry for having put this on hold for a while. I looked in the TAS2770 and TAS2764 drivers/datasheets, and to answer the questions we had: * VSENSE/ISENSE output slots are configured independently of audio samples routing. Kernel drivers configure the slots based on the 'ti,imon-slot-no' and 'ti,vmon-slot-no' properties of devicetree. * By default codecs transmit Hi-Z for duration of unused slots. So once we supply the devicetree props it should be electrically sound under any configuration of userspace knobs. One final thought on the playback routing controls: On systems with >2 speakers, the codecs need to be assigned slots through set_tdm_slot. The macaudio driver RFCed here assigns a single slot to each speaker, making the effect of each speaker's routing control this: 'I2C offset' -- uses a random slot 'Left' 'Right' 'LeftRight' -- uses the single slot we configured I suppose I better assign two slots to speakers in each left-right pair of the same kind (e.g. woofer 1, woofer 2, tweeter). This way the routing control will mimic its behavior from simple stereo systems but replicated within each left-right pair. (I would prefer to hide the controls altogether, but as I learned that hiding things unless proven dangerous is an ASoC non-goal, this way I can make the controls do something interesting.) Martin
On Fri, Apr 22, 2022 at 12:43:30PM +0200, Martin Povišer wrote: > I looked in the TAS2770 and TAS2764 drivers/datasheets, and to answer > the questions we had: > * VSENSE/ISENSE output slots are configured independently of audio samples > routing. Kernel drivers configure the slots based on the 'ti,imon-slot-no' > and 'ti,vmon-slot-no' properties of devicetree. > * By default codecs transmit Hi-Z for duration of unused slots. > So once we supply the devicetree props it should be electrically sound > under any configuration of userspace knobs. Great, that's a relief. > One final thought on the playback routing controls: On systems with >2 > speakers, the codecs need to be assigned slots through set_tdm_slot. > The macaudio driver RFCed here assigns a single slot to each speaker, > making the effect of each speaker's routing control this: > 'I2C offset' -- uses a random slot > 'Left' 'Right' 'LeftRight' -- uses the single slot we configured > I suppose I better assign two slots to speakers in each left-right pair > of the same kind (e.g. woofer 1, woofer 2, tweeter). This way the > routing control will mimic its behavior from simple stereo systems but > replicated within each left-right pair. (I would prefer to hide the > controls altogether, but as I learned that hiding things unless proven > dangerous is an ASoC non-goal, this way I can make the controls do > something interesting.) I don't quite grasp the difference between the arrangement you're proposing and assigning a single slot to each speaker? Possibly it's just a reordering of the slots?
> On 22. 4. 2022, at 13:19, Mark Brown <broonie@kernel.org> wrote: > > On Fri, Apr 22, 2022 at 12:43:30PM +0200, Martin Povišer wrote: > >> I looked in the TAS2770 and TAS2764 drivers/datasheets, and to answer >> the questions we had: > >> * VSENSE/ISENSE output slots are configured independently of audio samples >> routing. Kernel drivers configure the slots based on the 'ti,imon-slot-no' >> and 'ti,vmon-slot-no' properties of devicetree. > >> * By default codecs transmit Hi-Z for duration of unused slots. > >> So once we supply the devicetree props it should be electrically sound >> under any configuration of userspace knobs. > > Great, that's a relief. > >> One final thought on the playback routing controls: On systems with >2 >> speakers, the codecs need to be assigned slots through set_tdm_slot. >> The macaudio driver RFCed here assigns a single slot to each speaker, >> making the effect of each speaker's routing control this: > >> 'I2C offset' -- uses a random slot > >> 'Left' 'Right' 'LeftRight' -- uses the single slot we configured > >> I suppose I better assign two slots to speakers in each left-right pair >> of the same kind (e.g. woofer 1, woofer 2, tweeter). This way the >> routing control will mimic its behavior from simple stereo systems but >> replicated within each left-right pair. (I would prefer to hide the >> controls altogether, but as I learned that hiding things unless proven >> dangerous is an ASoC non-goal, this way I can make the controls do >> something interesting.) > > I don't quite grasp the difference between the arrangement you're > proposing and assigning a single slot to each speaker? Possibly it's > just a reordering of the slots? Ah, maybe what’s missing is the fact that the way the speaker amp drivers are written, if they are assigned two slots with a call to set_tdm_slot, the first slot is considered 'left' and the second is 'right'. So in the arrangement I am proposing the 'Left', 'Right' and 'LeftRight' values of the routing control have the nominal effect (within the left-right speaker pair), while in the other arrangement it is as I described above.
On Fri, Apr 22, 2022 at 01:28:20PM +0200, Martin Povišer wrote: > > On 22. 4. 2022, at 13:19, Mark Brown <broonie@kernel.org> wrote: > > On Fri, Apr 22, 2022 at 12:43:30PM +0200, Martin Povišer wrote: > >> One final thought on the playback routing controls: On systems with >2 > >> speakers, the codecs need to be assigned slots through set_tdm_slot. > >> The macaudio driver RFCed here assigns a single slot to each speaker, > >> making the effect of each speaker's routing control this: ... > > I don't quite grasp the difference between the arrangement you're > > proposing and assigning a single slot to each speaker? Possibly it's > > just a reordering of the slots? > Ah, maybe what’s missing is the fact that the way the speaker amp drivers > are written, if they are assigned two slots with a call to set_tdm_slot, > the first slot is considered 'left' and the second is 'right'. > So in the arrangement I am proposing the 'Left', 'Right' and 'LeftRight' > values of the routing control have the nominal effect (within the left-right > speaker pair), while in the other arrangement it is as I described above. So previously each speaker would get two slots but now it just gets one?
> On 22. 4. 2022, at 13:33, Mark Brown <broonie@kernel.org> wrote: > > On Fri, Apr 22, 2022 at 01:28:20PM +0200, Martin Povišer wrote: >>> On 22. 4. 2022, at 13:19, Mark Brown <broonie@kernel.org> wrote: >>> On Fri, Apr 22, 2022 at 12:43:30PM +0200, Martin Povišer wrote: > >>>> One final thought on the playback routing controls: On systems with >2 >>>> speakers, the codecs need to be assigned slots through set_tdm_slot. >>>> The macaudio driver RFCed here assigns a single slot to each speaker, >>>> making the effect of each speaker's routing control this: > > ... > >>> I don't quite grasp the difference between the arrangement you're >>> proposing and assigning a single slot to each speaker? Possibly it's >>> just a reordering of the slots? > >> Ah, maybe what’s missing is the fact that the way the speaker amp drivers >> are written, if they are assigned two slots with a call to set_tdm_slot, >> the first slot is considered 'left' and the second is 'right'. > >> So in the arrangement I am proposing the 'Left', 'Right' and 'LeftRight' >> values of the routing control have the nominal effect (within the left-right >> speaker pair), while in the other arrangement it is as I described above. > > So previously each speaker would get two slots but now it just gets one? No the other way around. Previously (with the driver as it is RFCed), each speaker gets a single slot, and 'Left', 'Right' and ‘LeftRight' values of the routing control don't do anything different from each other (well except maybe 'LeftRight' lessens the volume due to how the chip handles the edge case of mixing down two channels from the same slot). With the new arrangement I am proposing, the two speakers in a left-right pair get both the same two slots, meaning they get to choose one of the two slots based on the 'Left' 'Right' value of their routing control.
On Fri, Apr 22, 2022 at 01:44:06PM +0200, Martin Povišer wrote: > > So previously each speaker would get two slots but now it just gets one? > No the other way around. Previously (with the driver as it is RFCed), > each speaker gets a single slot, and 'Left', 'Right' and ‘LeftRight' > values of the routing control don't do anything different from each > other (well except maybe 'LeftRight' lessens the volume due to how > the chip handles the edge case of mixing down two channels from the > same slot). > With the new arrangement I am proposing, the two speakers in a left-right > pair get both the same two slots, meaning they get to choose one of the > two slots based on the 'Left' 'Right' value of their routing control. Ah, I think the confusion here is that I'm using slot and channel interchangably whereas you're saying that previously the driver would allocate two channels to each speaker with duplicate data?
> On 22. 4. 2022, at 14:22, Mark Brown <broonie@kernel.org> wrote: > > On Fri, Apr 22, 2022 at 01:44:06PM +0200, Martin Povišer wrote: > >>> So previously each speaker would get two slots but now it just gets one? > >> No the other way around. Previously (with the driver as it is RFCed), >> each speaker gets a single slot, and 'Left', 'Right' and ‘LeftRight' >> values of the routing control don't do anything different from each >> other (well except maybe 'LeftRight' lessens the volume due to how >> the chip handles the edge case of mixing down two channels from the >> same slot). > >> With the new arrangement I am proposing, the two speakers in a left-right >> pair get both the same two slots, meaning they get to choose one of the >> two slots based on the 'Left' 'Right' value of their routing control. > > Ah, I think the confusion here is that I'm using slot and channel > interchangably whereas you're saying that previously the driver would > allocate two channels to each speaker with duplicate data? I guess you could say that. Not that there’s duplicate data on the I2S bus, but the speaker amp would previously be configured to look for the left and right channel in the same TDM slot (see e.g. set_tdm_slot of tas2770 [0]). (Each speaker amp drives a single speaker, but it still has a notion of left and right channel.) [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/sound/soc/codecs/tas2770.c#n416
On Fri, Apr 22, 2022 at 02:36:03PM +0200, Martin Povišer wrote: > > Ah, I think the confusion here is that I'm using slot and channel > > interchangably whereas you're saying that previously the driver would > > allocate two channels to each speaker with duplicate data? > I guess you could say that. Not that there’s duplicate data on the I2S > bus, but the speaker amp would previously be configured to look for the > left and right channel in the same TDM slot (see e.g. set_tdm_slot of > tas2770 [0]). (Each speaker amp drives a single speaker, but it still > has a notion of left and right channel.) Oh, I see - the speaker actually allows configuration of the slots independently. Usually the left/right thing on mono devices only does something for I2S where the bus clocking enforces that there be both left and right channels. Either configuration is fine by me TBH, if you can do that then you could just keep them mapped to the same channel then mark the control as disabled since it should have no effect.
> On 22. 4. 2022, at 14:44, Mark Brown <broonie@kernel.org> wrote: > > On Fri, Apr 22, 2022 at 02:36:03PM +0200, Martin Povišer wrote: > >>> Ah, I think the confusion here is that I'm using slot and channel >>> interchangably whereas you're saying that previously the driver would >>> allocate two channels to each speaker with duplicate data? > >> I guess you could say that. Not that there’s duplicate data on the I2S >> bus, but the speaker amp would previously be configured to look for the >> left and right channel in the same TDM slot (see e.g. set_tdm_slot of >> tas2770 [0]). (Each speaker amp drives a single speaker, but it still >> has a notion of left and right channel.) > > Oh, I see - the speaker actually allows configuration of the slots > independently. Usually the left/right thing on mono devices only does > something for I2S where the bus clocking enforces that there be both > left and right channels. Either configuration is fine by me TBH, if you > can do that then you could just keep them mapped to the same channel > then mark the control as disabled since it should have no effect. Well but is there some established way to mark a control as disabled? Another issue here is that if I disable it I can’t leave the routing control in it’s default value, which is ‘I2C Offset’ and makes the speaker amp ignore the slot mapping.
On Fri, Apr 22, 2022 at 02:53:54PM +0200, Martin Povišer wrote: > > Oh, I see - the speaker actually allows configuration of the slots > > independently. Usually the left/right thing on mono devices only does > > something for I2S where the bus clocking enforces that there be both > > left and right channels. Either configuration is fine by me TBH, if you > > can do that then you could just keep them mapped to the same channel > > then mark the control as disabled since it should have no effect. > Well but is there some established way to mark a control as disabled? snd_ctl_activate_id(). > Another issue here is that if I disable it I can’t leave the routing > control in it’s default value, which is ‘I2C Offset’ and makes the speaker > amp ignore the slot mapping. Sure, that's fine - if a control genuinely has no effect it's fine to hide it from userspace. The issue is where it's just that you don't see the use, if the control demonstrably does nothing then that's fine.
> On 22. 4. 2022, at 15:06, Mark Brown <broonie@kernel.org> wrote: > > On Fri, Apr 22, 2022 at 02:53:54PM +0200, Martin Povišer wrote: > >>> Oh, I see - the speaker actually allows configuration of the slots >>> independently. Usually the left/right thing on mono devices only does >>> something for I2S where the bus clocking enforces that there be both >>> left and right channels. Either configuration is fine by me TBH, if you >>> can do that then you could just keep them mapped to the same channel >>> then mark the control as disabled since it should have no effect. > >> Well but is there some established way to mark a control as disabled? > > snd_ctl_activate_id(). Ha! Great. >> Another issue here is that if I disable it I can’t leave the routing >> control in it’s default value, which is ‘I2C Offset’ and makes the speaker >> amp ignore the slot mapping. > > Sure, that's fine - if a control genuinely has no effect it's fine to > hide it from userspace. The issue is where it's just that you don't see > the use, if the control demonstrably does nothing then that's fine. So I assume I can set the control from the machine driver then disable it. Anyway, good, this is what I meant earlier when I said the controls I want to hide are 'useless/confusing at best’. I must walk back that they are ‘dangerous at worst’, but I am glad we can hide them anyway. (Not all of them of course, ISENSE/VSENSE will not be hidden, neither the routing control on systems with single mono speaker.)
> On 4. 4. 2022, at 14:28, Mark Brown <broonie@kernel.org> wrote: > > On Thu, Mar 31, 2022 at 02:04:47AM +0200, Martin Povišer wrote: > >> +#if 0 >> dev_err(rtd->card->dev, >> "N cpus to M codecs link is not supported yet\n"); >> return -EINVAL; >> +#endif >> + cpu_dai = asoc_rtd_to_cpu(rtd, 0); > > We need to figure out an interface for describing which CODEC/CPU > combinations are connected to each other. I'm not seeing a great way to > do that right now, probably some side data table is going to be needed, > or perhaps the CPU DAI drivers can be persuaded to only have one DAI > actually register and claim to support more channels? I'm not sure how > a configuraiton like this is going to work at userspace level if the > multiple CPU DAIs end up being visible... To understand the issue better: How could the multiple CPU DAIs be visible from userspace? What about this interim solution: In case of N-to-M links we put in the most restrictive condition for checking capture/playback stream validity: we check all of the CPU DAIs. Whatever ends up being the proper solution later can only be less restrictive than this. As a reminder what happens on the Macs: the platform driver drives all the CPU-side I2S ports that belong to the link with the same data, so the particular CPU/CODEC wiring doesn’t matter.
On Fri, Apr 22, 2022 at 04:06:06PM +0200, Martin Povišer wrote: > > On 4. 4. 2022, at 14:28, Mark Brown <broonie@kernel.org> wrote: > > We need to figure out an interface for describing which CODEC/CPU > > combinations are connected to each other. I'm not seeing a great way to > > do that right now, probably some side data table is going to be needed, > > or perhaps the CPU DAI drivers can be persuaded to only have one DAI > > actually register and claim to support more channels? I'm not sure how > > a configuraiton like this is going to work at userspace level if the > > multiple CPU DAIs end up being visible... > To understand the issue better: How could the multiple CPU DAIs be > visible from userspace? If you register two separate DAIs (well, links) with the API without doing anything else the API will just expose them to userspace as two separate things with no indication that they're related. > What about this interim solution: In case of N-to-M links we put in > the most restrictive condition for checking capture/playback stream > validity: we check all of the CPU DAIs. Whatever ends up being the > proper solution later can only be less restrictive than this. That's not the issue here? > As a reminder what happens on the Macs: the platform driver drives > all the CPU-side I2S ports that belong to the link with the same data, > so the particular CPU/CODEC wiring doesn’t matter. Oh, that's not something I was aware of. In that case this is the wrong API - you should be using DPCM to map one front end onto multiple back ends (Kirkwood does something similar IIRC, there will be other examples but that's probably the simplest). The back ends probably don't really need to know that they're on the same physical bus (if indeed they are).
> On 25. 4. 2022, at 14:25, Mark Brown <broonie@kernel.org> wrote: > > On Fri, Apr 22, 2022 at 04:06:06PM +0200, Martin Povišer wrote: >>> On 4. 4. 2022, at 14:28, Mark Brown <broonie@kernel.org> wrote: > >>> We need to figure out an interface for describing which CODEC/CPU >>> combinations are connected to each other. I'm not seeing a great way to >>> do that right now, probably some side data table is going to be needed, >>> or perhaps the CPU DAI drivers can be persuaded to only have one DAI >>> actually register and claim to support more channels? I'm not sure how >>> a configuraiton like this is going to work at userspace level if the >>> multiple CPU DAIs end up being visible... > >> To understand the issue better: How could the multiple CPU DAIs be >> visible from userspace? > > If you register two separate DAIs (well, links) with the API without > doing anything else the API will just expose them to userspace as two > separate things with no indication that they're related. Sure, but what I am addressing here is a single DAI link with multiple CPU DAIs, invoked in DT like this: dai-link@0 { link-name = "Speakers"; mclk-fs = <256>; cpu { sound-dai = <&mca 0>, <&mca 1>; }; codec { sound-dai = <&speaker_left_woof1>, <&speaker_right_woof1>, <&speaker_left_tweet>, <&speaker_right_tweet>, <&speaker_left_woof2>, <&speaker_right_woof2>; }; }; >> What about this interim solution: In case of N-to-M links we put in >> the most restrictive condition for checking capture/playback stream >> validity: we check all of the CPU DAIs. Whatever ends up being the >> proper solution later can only be less restrictive than this. > > That's not the issue here? Well to me it looks like it is. Because if I invoke the DAI link like I quoted above, and the platform driver supports it, the playback/capture stream validity check is the only place it breaks down. Notwithstanding this may be the wrong API as you wrote. >> As a reminder what happens on the Macs: the platform driver drives >> all the CPU-side I2S ports that belong to the link with the same data, >> so the particular CPU/CODEC wiring doesn’t matter. > > Oh, that's not something I was aware of. In that case this is the wrong > API - you should be using DPCM to map one front end onto multiple back > ends (Kirkwood does something similar IIRC, there will be other examples > but that's probably the simplest). The back ends probably don't really > need to know that they're on the same physical bus (if indeed they are). I guess I need to look into that.
On Mon, Apr 25, 2022 at 02:34:33PM +0200, Martin Povišer wrote: > > On 25. 4. 2022, at 14:25, Mark Brown <broonie@kernel.org> wrote: > > If you register two separate DAIs (well, links) with the API without > > doing anything else the API will just expose them to userspace as two > > separate things with no indication that they're related. > Sure, but what I am addressing here is a single DAI link with multiple > CPU DAIs, invoked in DT like this: > dai-link@0 { > link-name = "Speakers"; > mclk-fs = <256>; > > cpu { > sound-dai = <&mca 0>, <&mca 1>; > }; > codec { > sound-dai = <&speaker_left_woof1>, > <&speaker_right_woof1>, > <&speaker_left_tweet>, > <&speaker_right_tweet>, > <&speaker_left_woof2>, > <&speaker_right_woof2>; > }; > }; You could parse this into two separate links for the benefit of the framewokr if you're using a custom machine driver (which I suspect you probably have to). > >> What about this interim solution: In case of N-to-M links we put in > >> the most restrictive condition for checking capture/playback stream > >> validity: we check all of the CPU DAIs. Whatever ends up being the > >> proper solution later can only be less restrictive than this. > > That's not the issue here? > Well to me it looks like it is. Because if I invoke the DAI link like > I quoted above, and the platform driver supports it, the playback/capture > stream validity check is the only place it breaks down. Notwithstanding > this may be the wrong API as you wrote. I am surprised that doesn't otherwise explode TBH - at the very least I'd expect it to show two PCMs to userspace which if I'm understanding your description correctly isn't really what's going on.
> On 25. 4. 2022, at 14:55, Mark Brown <broonie@kernel.org> wrote: > > On Mon, Apr 25, 2022 at 02:34:33PM +0200, Martin Povišer wrote: >>> On 25. 4. 2022, at 14:25, Mark Brown <broonie@kernel.org> wrote: > >>> If you register two separate DAIs (well, links) with the API without >>> doing anything else the API will just expose them to userspace as two >>> separate things with no indication that they're related. > >> Sure, but what I am addressing here is a single DAI link with multiple >> CPU DAIs, invoked in DT like this: > >> dai-link@0 { >> link-name = "Speakers"; >> mclk-fs = <256>; >> >> cpu { >> sound-dai = <&mca 0>, <&mca 1>; >> }; >> codec { >> sound-dai = <&speaker_left_woof1>, >> <&speaker_right_woof1>, >> <&speaker_left_tweet>, >> <&speaker_right_tweet>, >> <&speaker_left_woof2>, >> <&speaker_right_woof2>; >> }; >> }; > > You could parse this into two separate links for the benefit of the > framewokr if you're using a custom machine driver (which I suspect you > probably have to). Yeah, this is parsed by the ‘macaudio’ machine driver from the series. >>>> What about this interim solution: In case of N-to-M links we put in >>>> the most restrictive condition for checking capture/playback stream >>>> validity: we check all of the CPU DAIs. Whatever ends up being the >>>> proper solution later can only be less restrictive than this. > >>> That's not the issue here? > >> Well to me it looks like it is. Because if I invoke the DAI link like >> I quoted above, and the platform driver supports it, the playback/capture >> stream validity check is the only place it breaks down. Notwithstanding >> this may be the wrong API as you wrote. > > I am surprised that doesn't otherwise explode TBH - at the very least > I'd expect it to show two PCMs to userspace which if I'm understanding > your description correctly isn't really what's going on. I fill in a single snd_soc_dai_link, it exposes a single PCM and works like a charm. That is as long as I patch the playback/capture check in question. I read that to be the clear intention of ASoC code: a DAI link becomes one snd_soc_pcm_runtime.
On Mon, Apr 25, 2022 at 03:11:14PM +0200, Martin Povišer wrote: > > On 25. 4. 2022, at 14:55, Mark Brown <broonie@kernel.org> wrote: > > I am surprised that doesn't otherwise explode TBH - at the very least > > I'd expect it to show two PCMs to userspace which if I'm understanding > > your description correctly isn't really what's going on. > I fill in a single snd_soc_dai_link, it exposes a single PCM and works > like a charm. That is as long as I patch the playback/capture check in > question. > I read that to be the clear intention of ASoC code: a DAI link becomes > one snd_soc_pcm_runtime. Yes, so long as you boil it down to a single link it works fine but the bit on top of the binding where you tie the two CPU DAIs to what is actually exposed is all in code. The reason this stuff isn't filled in is that connecting the thing that applications see to the physical links isn't at all obvious and needs at least some driver sitting in the middle to make the links - I'd imagine there's a DSP sitting there which probably has quite a bit of flexability about how the various hardware components available are actually related. This makes figuring out what to do with the relationship between the multiple CPU DAIs hard.
> On 25. 4. 2022, at 15:46, Mark Brown <broonie@kernel.org> wrote: > > On Mon, Apr 25, 2022 at 03:11:14PM +0200, Martin Povišer wrote: >>> On 25. 4. 2022, at 14:55, Mark Brown <broonie@kernel.org> wrote: > >>> I am surprised that doesn't otherwise explode TBH - at the very least >>> I'd expect it to show two PCMs to userspace which if I'm understanding >>> your description correctly isn't really what's going on. > >> I fill in a single snd_soc_dai_link, it exposes a single PCM and works >> like a charm. That is as long as I patch the playback/capture check in >> question. > >> I read that to be the clear intention of ASoC code: a DAI link becomes >> one snd_soc_pcm_runtime. > > Yes, so long as you boil it down to a single link it works fine but the > bit on top of the binding where you tie the two CPU DAIs to what is > actually exposed is all in code. The reason this stuff isn't filled in > is that connecting the thing that applications see to the physical links > isn't at all obvious and needs at least some driver sitting in the > middle to make the links - I'd imagine there's a DSP sitting there which > probably has quite a bit of flexability about how the various hardware > components available are actually related. This makes figuring out what > to do with the relationship between the multiple CPU DAIs hard. I get the gist. Anyway unless you tell me otherwise I will assume I need to move to DPCM with the platform/machine driver.