| Message ID | 20220504170905.332415-2-ckeepax@opensource.cirrus.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Clean up usage of the endianness flag | expand |
Hello Charles, In the [PATCH 00/38] of this patch set, you write: > 2) Devices behind non-audio buses, SPI just moves bits and doesn't > really define an endian for audio data on the bus. Thus it seems the > CODEC probably can care about the endian. The only devices that fall > into this group (mostly for AoV) are: rt5514-spi.c, rt5677-spi.c, > cros_ec_codec.c (only the AoV). From my experience with some PCM codecs by TI, they can not care about the endianness. For example, in driver [1], if I allow BE format as well, and configure the sound card to use BE, it will not work. I have no sound with BE. In these cases, the codec HW supports *only* LE. That's why their `snd_soc_dai_driver` structures provide only LE in the `format` field. If such drivers specify `endianness = 1`, then `soc-core` will extend their supported formats with BE counter-parts, see [2]. AFAIU, it will have the same effect, as if the drivers themselves provided their formats in both LE | BE. I don't think adding `endianness = 1` to such codecs will work as expected. Unfortunately, I don't have an easy access to HW today, to confirm or disprove this understanding. Best regards, Kirill --- [1] https://elixir.bootlin.com/linux/latest/source/sound/soc/codecs/pcm3060.c#L189 [2] https://elixir.bootlin.com/linux/latest/source/sound/soc/soc-core.c#L2540 On 5/4/22 7:08 PM, Charles Keepax wrote: > Add a comment to make the purpose of the endianness flag on the > snd_soc_component structure more clear. > > Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com> > --- > include/sound/soc-component.h | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/include/sound/soc-component.h b/include/sound/soc-component.h > index 766dc6f009c0b..5a764c3099d3e 100644 > --- a/include/sound/soc-component.h > +++ b/include/sound/soc-component.h > @@ -169,6 +169,15 @@ struct snd_soc_component_driver { > unsigned int idle_bias_on:1; > unsigned int suspend_bias_off:1; > unsigned int use_pmdown_time:1; /* care pmdown_time at stop */ > + /* > + * Indicates that the component does not care about the endianness of > + * PCM audio data and the core will ensure that both LE and BE variants > + * of each used format are present. Typically this is because the > + * component sits behind a bus that abstracts away the endian of the > + * original data, ie. one for which the transmission endian is defined > + * (I2S/SLIMbus/SoundWire), or the concept of endian doesn't exist (PDM, > + * analogue). > + */ > unsigned int endianness:1; > unsigned int non_legacy_dai_naming:1; >
On Sun, May 08, 2022 at 10:34:12PM +0200, Kirill Marinushkin wrote: > In the [PATCH 00/38] of this patch set, you write: > > 2) Devices behind non-audio buses, SPI just moves bits and doesn't > > really define an endian for audio data on the bus. Thus it seems the > > CODEC probably can care about the endian. The only devices that fall > > into this group (mostly for AoV) are: rt5514-spi.c, rt5677-spi.c, > > cros_ec_codec.c (only the AoV). > > From my experience with some PCM codecs by TI, they can not care about the > endianness. For example, in driver [1], if I allow BE format as > well, and configure > the sound card to use BE, it will not work. I have no sound with BE. > In these cases, the codec HW supports *only* LE. That's why their > `snd_soc_dai_driver` > structures provide only LE in the `format` field. > If such drivers specify `endianness = 1`, then `soc-core` will > extend their supported > formats with BE counter-parts, see [2]. AFAIU, it will have the same > effect, as if the > drivers themselves provided their formats in both LE | BE. > > I don't think adding `endianness = 1` to such codecs will work as expected. > Unfortunately, I don't have an easy access to HW today, to confirm > or disprove > this understanding. > This sounds like an error on the CPU side of the DAI link rather than the CODEC side of the DAI link. The formats that will be supported on the link are the union of the CPU and CODEC supported formats, ie. a format must be supported on both for the DAI to support it. The CPU I2S hardware should be sending out the bits in the same order regardless of if the data you feed it is BE or LE, as I2S specifies an ordering for the bits. If this is not the case then the host I2S controller is claiming to support an endian it does not, and the problem should be fixed on that side by removing the supported endian. Thanks, Charles
Hello Charles, On 5/9/22 10:37 AM, Charles Keepax wrote: > On Sun, May 08, 2022 at 10:34:12PM +0200, Kirill Marinushkin wrote: >> In the [PATCH 00/38] of this patch set, you write: >>> 2) Devices behind non-audio buses, SPI just moves bits and doesn't >>> really define an endian for audio data on the bus. Thus it seems the >>> CODEC probably can care about the endian. The only devices that fall >>> into this group (mostly for AoV) are: rt5514-spi.c, rt5677-spi.c, >>> cros_ec_codec.c (only the AoV). >> From my experience with some PCM codecs by TI, they can not care about the >> endianness. For example, in driver [1], if I allow BE format as >> well, and configure >> the sound card to use BE, it will not work. I have no sound with BE. >> In these cases, the codec HW supports *only* LE. That's why their >> `snd_soc_dai_driver` >> structures provide only LE in the `format` field. >> If such drivers specify `endianness = 1`, then `soc-core` will >> extend their supported >> formats with BE counter-parts, see [2]. AFAIU, it will have the same >> effect, as if the >> drivers themselves provided their formats in both LE | BE. >> >> I don't think adding `endianness = 1` to such codecs will work as expected. >> Unfortunately, I don't have an easy access to HW today, to confirm >> or disprove >> this understanding. >> > This sounds like an error on the CPU side of the DAI link rather > than the CODEC side of the DAI link. The formats that will be > supported on the link are the union of the CPU and CODEC supported > formats, ie. a format must be supported on both for the DAI to > support it. Yes, agree, both sides of the DAI link should provide only endianness they support. This works currently, but, from my understending, it will break, after we set `endianness = 1`. As soon as we start setting `endianness = 1`, the function `convert_endianness_formats()` will extend LE to (LE | BE), right? If so, setting `endianness = 1` is the source of an error, right? I could be missing something. Let's see what other reviewers think. > The CPU I2S hardware should be sending out the bits in the same > order regardless of if the data you feed it is BE or LE, as I2S > specifies an ordering for the bits. What does the endianness in the driver configure, then? > If this is not the case then > the host I2S controller is claiming to support an endian it does > not, and the problem should be fixed on that side by removing the > supported endian. I think we have a misundersanding of my example. In my example, i don't mean, that my CPU part of the DAI link is broken. What i tried to demonstrate - is that if i set the unsupported endianness, i wouldn't expect that "the CODEC probably can care about the endian", as the message in [PATCH 00/38] suggests. I would expect, that i will have no sound. Best regards, Kirill > > Thanks, > Charles
On Mon, May 09, 2022 at 09:22:42PM +0200, Kirill Marinushkin wrote: > On 5/9/22 10:37 AM, Charles Keepax wrote: > > This sounds like an error on the CPU side of the DAI link rather > > than the CODEC side of the DAI link. The formats that will be > > supported on the link are the union of the CPU and CODEC supported > > formats, ie. a format must be supported on both for the DAI to > > support it. > Yes, agree, both sides of the DAI link should provide only endianness they > support. > This works currently, but, from my understending, it will break, after we > set `endianness = 1`. > As soon as we start setting `endianness = 1`, the function > `convert_endianness_formats()` will extend LE to (LE | BE), right? > If so, setting `endianness = 1` is the source of an error, right? If doing this for the CODEC side of the link causes an issue it's just exposing an existing bug on the CPU side of the link which may already be affecting other systems - like Charles says the CODEC is expecting a fixed bit order regardless of the memory layout of the data. > > The CPU I2S hardware should be sending out the bits in the same > > order regardless of if the data you feed it is BE or LE, as I2S > > specifies an ordering for the bits. > What does the endianness in the driver configure, then? On the CODEC driver side it is meaningless. On the CPU side it controls the in memory layout of the data. > > If this is not the case then > > the host I2S controller is claiming to support an endian it does > > not, and the problem should be fixed on that side by removing the > > supported endian. > I think we have a misundersanding of my example. > In my example, i don't mean, that my CPU part of the DAI link is broken. > What i tried to demonstrate - is that if i set the unsupported endianness, i > wouldn't expect that "the CODEC probably can care about the endian", as the > message in [PATCH 00/38] suggests. I would expect, that i will have no > sound. If the CPU side of the link is fine then there should be no problem, we simply start supporting both endian settings all the way through the chain, if userspace chooses something that wasn't supported before then the CPU side driver will look at what's being configured and set up the hardware appropriately.
Hello Charles, Mark, Thank you for the clarification! Without such a deep understanding of ASoC, as you have, I see a risk in a bulk enable of `endianness = 1`, the way we do in this patch set. Here, we enable an extra feature. Worst case, if some codec doesn't support the feature, we will have a system, which thinks that it's supported, but in reality, it doesn't work. And we will not even have a error message, because the driver advertises the feature as supported. Maybe my carefulness is not applicable here. I see that i don't have enough expertise in `endianness = 1`, to participate in making the decision here. But at least i want to ensure, that we all understand the risk. Best Regards, Kirill On 5/9/22 9:30 PM, Mark Brown wrote: > On Mon, May 09, 2022 at 09:22:42PM +0200, Kirill Marinushkin wrote: >> On 5/9/22 10:37 AM, Charles Keepax wrote: >>> This sounds like an error on the CPU side of the DAI link rather >>> than the CODEC side of the DAI link. The formats that will be >>> supported on the link are the union of the CPU and CODEC supported >>> formats, ie. a format must be supported on both for the DAI to >>> support it. >> Yes, agree, both sides of the DAI link should provide only endianness they >> support. >> This works currently, but, from my understending, it will break, after we >> set `endianness = 1`. >> As soon as we start setting `endianness = 1`, the function >> `convert_endianness_formats()` will extend LE to (LE | BE), right? >> If so, setting `endianness = 1` is the source of an error, right? > If doing this for the CODEC side of the link causes an issue it's just > exposing an existing bug on the CPU side of the link which may already > be affecting other systems - like Charles says the CODEC is expecting a > fixed bit order regardless of the memory layout of the data. > >>> The CPU I2S hardware should be sending out the bits in the same >>> order regardless of if the data you feed it is BE or LE, as I2S >>> specifies an ordering for the bits. >> What does the endianness in the driver configure, then? > On the CODEC driver side it is meaningless. On the CPU side it controls > the in memory layout of the data. > >>> If this is not the case then >>> the host I2S controller is claiming to support an endian it does >>> not, and the problem should be fixed on that side by removing the >>> supported endian. >> I think we have a misundersanding of my example. >> In my example, i don't mean, that my CPU part of the DAI link is broken. >> What i tried to demonstrate - is that if i set the unsupported endianness, i >> wouldn't expect that "the CODEC probably can care about the endian", as the >> message in [PATCH 00/38] suggests. I would expect, that i will have no >> sound. > If the CPU side of the link is fine then there should be no problem, we > simply start supporting both endian settings all the way through the > chain, if userspace chooses something that wasn't supported before then > the CPU side driver will look at what's being configured and set up the > hardware appropriately.
On Mon, May 09, 2022 at 10:11:03PM +0200, Kirill Marinushkin wrote: Please don't top post, reply in line with needed context. This allows readers to readily follow the flow of conversation and understand what you are talking about and also helps ensure that everything in the discussion is being addressed. > Without such a deep understanding of ASoC, as you have, I see a risk in a > bulk enable of `endianness = 1`, the way we do in this patch set. > Here, we enable an extra feature. Worst case, if some codec doesn't support > the feature, we will have a system, which thinks that it's supported, but in > reality, it doesn't work. And we will not even have a error message, because > the driver advertises the feature as supported. > Maybe my carefulness is not applicable here. I see that i don't have enough > expertise in `endianness = 1`, to participate in making the decision here. > But at least i want to ensure, that we all understand the risk. The risk here is that we expose a preexisting bug in some CPU side driver to a wider set of users, but those drivers would still be buggy no matter what and may already be causing problems on some systems even if that's not been reported so if there is a problem it's always possible that we end up helping some users who currently have issues by helping someone realise what the problem is. However most likely nobody will notice anything either way and most systems will continue using exactly the same formats they already were.
On Mon, May 09, 2022 at 09:22:42PM +0200, Kirill Marinushkin wrote: > On 5/9/22 10:37 AM, Charles Keepax wrote: > >On Sun, May 08, 2022 at 10:34:12PM +0200, Kirill Marinushkin wrote: > >This sounds like an error on the CPU side of the DAI link rather > >than the CODEC side of the DAI link. The formats that will be > >supported on the link are the union of the CPU and CODEC supported > >formats, ie. a format must be supported on both for the DAI to > >support it. > > Yes, agree, both sides of the DAI link should provide only > endianness they support. > > This works currently, but, from my understending, it will break, > after we set `endianness = 1`. It will break if the CPU side claims it supports endians which it doesn't which I believe is what you have in your system. > As soon as we start setting `endianness = 1`, the function > `convert_endianness_formats()` will extend LE to (LE | BE), right? Correct. > If so, setting `endianness = 1` is the source of an error, right? No, potentially it exposes an error but it is not the source of the error. Endian should have no meaning once you cross an I2S bus. > >If this is not the case then > >the host I2S controller is claiming to support an endian it does > >not, and the problem should be fixed on that side by removing the > >supported endian. > > I think we have a misundersanding of my example. > > In my example, i don't mean, that my CPU part of the DAI link is broken. Why are you sure the CPU part of the DAI link isn't broken? I2S defines the order bits should be sent out on the bus, this means if the CPU side recieves big endian or little endian data it doesn't matter, the actual bus signals should be identical in both cases. If only one endian works the actual signals on the bus must have been different depending on which endian you played in which case the host side doesn't match the I2S specification. My point is that what you are seeing is exactly what you get if your CPU DAI claimed to support both big and little endian but just puts that data directly out on the bus, rather than adapting to the endian of data it receives. And to be fair this is exactly what I am trying to tackle with this series. The fact we have so many CODECs that are incorrectly not specifying the endianness flag means that it is very easy for the host side guys to do testing and not realise that the CODEC is the one limiting the supported endians on the DAI link and thus miss they have claimed to support endians they don't. I believe you are right that there is a reasonable chance will see some breakage after these patches, but that breakage should be exposing host side bugs which we need to fix anyway. > What i tried to demonstrate - is that if i set the unsupported > endianness, i wouldn't expect that "the CODEC probably can care > about the endian", as the message in [PATCH 00/38] suggests. That comment in my original post was in respect to CODECs that recieve/transmit audio directly over a SPI bus rather than a normal audio bus. The is no standard governing how audio data is transmitted over the SPI bus, thus endian could be important, although I need to do some more work to analyse how people are using these links. Often they are implemented effectively as a CPU side DAI anyway. Thanks, Charles
diff --git a/include/sound/soc-component.h b/include/sound/soc-component.h index 766dc6f009c0b..5a764c3099d3e 100644 --- a/include/sound/soc-component.h +++ b/include/sound/soc-component.h @@ -169,6 +169,15 @@ struct snd_soc_component_driver { unsigned int idle_bias_on:1; unsigned int suspend_bias_off:1; unsigned int use_pmdown_time:1; /* care pmdown_time at stop */ + /* + * Indicates that the component does not care about the endianness of + * PCM audio data and the core will ensure that both LE and BE variants + * of each used format are present. Typically this is because the + * component sits behind a bus that abstracts away the endian of the + * original data, ie. one for which the transmission endian is defined + * (I2S/SLIMbus/SoundWire), or the concept of endian doesn't exist (PDM, + * analogue). + */ unsigned int endianness:1; unsigned int non_legacy_dai_naming:1;
Add a comment to make the purpose of the endianness flag on the snd_soc_component structure more clear. Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com> --- include/sound/soc-component.h | 9 +++++++++ 1 file changed, 9 insertions(+)