diff mbox series

[01/38] ASoC: soc-component: Add comment for the endianness flag

Message ID 20220504170905.332415-2-ckeepax@opensource.cirrus.com (mailing list archive)
State New
Headers show
Series Clean up usage of the endianness flag | expand

Commit Message

Charles Keepax May 4, 2022, 5:08 p.m. UTC
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(+)

Comments

Kirill Marinushkin May 8, 2022, 8:34 p.m. UTC | #1
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;
>
Charles Keepax May 9, 2022, 8:37 a.m. UTC | #2
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
Kirill Marinushkin May 9, 2022, 7:22 p.m. UTC | #3
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
Mark Brown May 9, 2022, 7:30 p.m. UTC | #4
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.
Kirill Marinushkin May 9, 2022, 8:11 p.m. UTC | #5
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.
Mark Brown May 9, 2022, 8:18 p.m. UTC | #6
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.
Charles Keepax May 10, 2022, 8:53 a.m. UTC | #7
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 mbox series

Patch

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;