Re: [RFC PATCH 0/5] Apple Macs machine-level ASoC driver
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 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/soun...
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.)
participants (2)
-
Mark Brown -
Martin Povišer