From: Marcus Cooper <[email protected]>
Extend the functionality of the driver to include support of 20 and
24 bits per sample.
Signed-off-by: Marcus Cooper <[email protected]>
Signed-off-by: Clément Péron <[email protected]>
---
sound/soc/sunxi/sun4i-i2s.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
index f78167e152ce..bc7f9343bc7a 100644
--- a/sound/soc/sunxi/sun4i-i2s.c
+++ b/sound/soc/sunxi/sun4i-i2s.c
@@ -577,6 +577,9 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream,
case 16:
width = DMA_SLAVE_BUSWIDTH_2_BYTES;
break;
+ case 32:
+ width = DMA_SLAVE_BUSWIDTH_4_BYTES;
+ break;
default:
dev_err(dai->dev, "Unsupported physical sample width: %d\n",
params_physical_width(params));
@@ -1063,6 +1066,10 @@ static int sun4i_i2s_dai_probe(struct snd_soc_dai *dai)
return 0;
}
+#define SUN4I_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | \
+ SNDRV_PCM_FMTBIT_S20_LE | \
+ SNDRV_PCM_FMTBIT_S24_LE)
+
static struct snd_soc_dai_driver sun4i_i2s_dai = {
.probe = sun4i_i2s_dai_probe,
.capture = {
@@ -1070,14 +1077,14 @@ static struct snd_soc_dai_driver sun4i_i2s_dai = {
.channels_min = 1,
.channels_max = 8,
.rates = SNDRV_PCM_RATE_8000_192000,
- .formats = SNDRV_PCM_FMTBIT_S16_LE,
+ .formats = SUN4I_FORMATS,
},
.playback = {
.stream_name = "Playback",
.channels_min = 1,
.channels_max = 8,
.rates = SNDRV_PCM_RATE_8000_192000,
- .formats = SNDRV_PCM_FMTBIT_S16_LE,
+ .formats = SUN4I_FORMATS,
},
.ops = &sun4i_i2s_dai_ops,
.symmetric_rates = 1,
--
2.25.1
On Sat, Jul 04, 2020 at 01:38:51PM +0200, Cl?ment P?ron wrote:
> From: Marcus Cooper <[email protected]>
>
> Extend the functionality of the driver to include support of 20 and
> 24 bits per sample.
>
> Signed-off-by: Marcus Cooper <[email protected]>
> Signed-off-by: Cl?ment P?ron <[email protected]>
Acked-by: Maxime Ripard <[email protected]>
Thanks!
Maxime
On 7/4/20 6:38 AM, Clément Péron wrote:
> From: Marcus Cooper <[email protected]>
>
> Extend the functionality of the driver to include support of 20 and
> 24 bits per sample.
>
> Signed-off-by: Marcus Cooper <[email protected]>
> Signed-off-by: Clément Péron <[email protected]>
> ---
> sound/soc/sunxi/sun4i-i2s.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> index f78167e152ce..bc7f9343bc7a 100644
> --- a/sound/soc/sunxi/sun4i-i2s.c
> +++ b/sound/soc/sunxi/sun4i-i2s.c
> @@ -577,6 +577,9 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream,
> case 16:
> width = DMA_SLAVE_BUSWIDTH_2_BYTES;
> break;
> + case 32:
> + width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> + break;
This breaks the sun4i variants, because sun4i_i2s_get_wss returns 4 for a 32 bit
width, but it needs to return 3.
As a side note, I wonder why we use the physical width (the spacing between
samples in RAM) to drive the slot width. S24_LE takes up 4 bytes per sample in
RAM, which we need for DMA. But I don't see why we would want to transmit the
padding over the wire. I would expect it to be transmitted the same as S24_3LE
(which has no padding). It did not matter before, because the only supported
format had no padding.
Regards,
Samuel
> default:
> dev_err(dai->dev, "Unsupported physical sample width: %d\n",
> params_physical_width(params));
> @@ -1063,6 +1066,10 @@ static int sun4i_i2s_dai_probe(struct snd_soc_dai *dai)
> return 0;
> }
>
> +#define SUN4I_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | \
> + SNDRV_PCM_FMTBIT_S20_LE | \
> + SNDRV_PCM_FMTBIT_S24_LE)
> +
> static struct snd_soc_dai_driver sun4i_i2s_dai = {
> .probe = sun4i_i2s_dai_probe,
> .capture = {
> @@ -1070,14 +1077,14 @@ static struct snd_soc_dai_driver sun4i_i2s_dai = {
> .channels_min = 1,
> .channels_max = 8,
> .rates = SNDRV_PCM_RATE_8000_192000,
> - .formats = SNDRV_PCM_FMTBIT_S16_LE,
> + .formats = SUN4I_FORMATS,
> },
> .playback = {
> .stream_name = "Playback",
> .channels_min = 1,
> .channels_max = 8,
> .rates = SNDRV_PCM_RATE_8000_192000,
> - .formats = SNDRV_PCM_FMTBIT_S16_LE,
> + .formats = SUN4I_FORMATS,
> },
> .ops = &sun4i_i2s_dai_ops,
> .symmetric_rates = 1,
>
Hi Samuel!
Dne petek, 10. julij 2020 ob 07:44:51 CEST je Samuel Holland napisal(a):
> On 7/4/20 6:38 AM, Cl?ment P?ron wrote:
> > From: Marcus Cooper <[email protected]>
> >
> > Extend the functionality of the driver to include support of 20 and
> > 24 bits per sample.
> >
> > Signed-off-by: Marcus Cooper <[email protected]>
> > Signed-off-by: Cl?ment P?ron <[email protected]>
> > ---
> >
> > sound/soc/sunxi/sun4i-i2s.c | 11 +++++++++--
> > 1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> > index f78167e152ce..bc7f9343bc7a 100644
> > --- a/sound/soc/sunxi/sun4i-i2s.c
> > +++ b/sound/soc/sunxi/sun4i-i2s.c
> > @@ -577,6 +577,9 @@ static int sun4i_i2s_hw_params(struct
> > snd_pcm_substream *substream,>
> > case 16:
> > width = DMA_SLAVE_BUSWIDTH_2_BYTES;
> > break;
> >
> > + case 32:
> > + width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> > + break;
>
> This breaks the sun4i variants, because sun4i_i2s_get_wss returns 4 for a 32
> bit width, but it needs to return 3.
I'm not sure what has WSS with physical width and DMA?
>
> As a side note, I wonder why we use the physical width (the spacing between
> samples in RAM) to drive the slot width. S24_LE takes up 4 bytes per sample
> in RAM, which we need for DMA. But I don't see why we would want to
> transmit the padding over the wire. I would expect it to be transmitted the
> same as S24_3LE (which has no padding). It did not matter before, because
> the only supported format had no padding.
Allwinner DMA engines support only 1, 2, 4 and sometimes 8 bytes for bus
width, so if sample is 24 bits in size, we have no other way but to transmit
padding too.
Best regards,
Jernej
>
> Regards,
> Samuel
>
> > default:
> > dev_err(dai->dev, "Unsupported physical sample width:
%d\n",
> >
> > params_physical_width(params));
> >
> > @@ -1063,6 +1066,10 @@ static int sun4i_i2s_dai_probe(struct snd_soc_dai
> > *dai)>
> > return 0;
> >
> > }
> >
> > +#define SUN4I_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | \
> > + SNDRV_PCM_FMTBIT_S20_LE | \
> > + SNDRV_PCM_FMTBIT_S24_LE)
> > +
> >
> > static struct snd_soc_dai_driver sun4i_i2s_dai = {
> >
> > .probe = sun4i_i2s_dai_probe,
> > .capture = {
> >
> > @@ -1070,14 +1077,14 @@ static struct snd_soc_dai_driver sun4i_i2s_dai = {
> >
> > .channels_min = 1,
> > .channels_max = 8,
> > .rates = SNDRV_PCM_RATE_8000_192000,
> >
> > - .formats = SNDRV_PCM_FMTBIT_S16_LE,
> > + .formats = SUN4I_FORMATS,
> >
> > },
> > .playback = {
> >
> > .stream_name = "Playback",
> > .channels_min = 1,
> > .channels_max = 8,
> > .rates = SNDRV_PCM_RATE_8000_192000,
> >
> > - .formats = SNDRV_PCM_FMTBIT_S16_LE,
> > + .formats = SUN4I_FORMATS,
> >
> > },
> > .ops = &sun4i_i2s_dai_ops,
> > .symmetric_rates = 1,
On 9/2/20 1:10 PM, Jernej Škrabec wrote:
> Hi Samuel!
>
> Dne petek, 10. julij 2020 ob 07:44:51 CEST je Samuel Holland napisal(a):
>> On 7/4/20 6:38 AM, Clément Péron wrote:
>>> From: Marcus Cooper <[email protected]>
>>>
>>> Extend the functionality of the driver to include support of 20 and
>>> 24 bits per sample.
>>>
>>> Signed-off-by: Marcus Cooper <[email protected]>
>>> Signed-off-by: Clément Péron <[email protected]>
>>> ---
>>>
>>> sound/soc/sunxi/sun4i-i2s.c | 11 +++++++++--
>>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
>>> index f78167e152ce..bc7f9343bc7a 100644
>>> --- a/sound/soc/sunxi/sun4i-i2s.c
>>> +++ b/sound/soc/sunxi/sun4i-i2s.c
>>> @@ -577,6 +577,9 @@ static int sun4i_i2s_hw_params(struct
>>> snd_pcm_substream *substream,>
>>> case 16:
>>> width = DMA_SLAVE_BUSWIDTH_2_BYTES;
>>> break;
>>>
>>> + case 32:
>>> + width = DMA_SLAVE_BUSWIDTH_4_BYTES;
>>> + break;
>>
>> This breaks the sun4i variants, because sun4i_i2s_get_wss returns 4 for a 32
>> bit width, but it needs to return 3.
>
> I'm not sure what has WSS with physical width and DMA?
This is the change where creating a S24_LE stream no longer fails with -EINVAL.
So this is the change where userspace stops downsampling 24-bit audio sources.
So this is the change where playback of 24-bit audio sources breaks, because WSS
is programmed wrong.
>> As a side note, I wonder why we use the physical width (the spacing between
>> samples in RAM) to drive the slot width. S24_LE takes up 4 bytes per sample
>> in RAM, which we need for DMA. But I don't see why we would want to
>> transmit the padding over the wire. I would expect it to be transmitted the
>> same as S24_3LE (which has no padding). It did not matter before, because
>> the only supported format had no padding.
>
> Allwinner DMA engines support only 1, 2, 4 and sometimes 8 bytes for bus
> width, so if sample is 24 bits in size, we have no other way but to transmit
> padding too.
I understand why we do 4 byte DMA from RAM <=> I2S FIFO; that was not my
question. I'm referring to the actual wire format (FIFO <=> PCM_DIN/DOUT). The
sample is already truncated from 32 bits to 24 bits in the FIFO -- that's what
TXIM and RXOM in FIFO_CTRL control.
If a sample is 24 bits wide, why would we send 32 BCLKs for every LRCK? I would
expect the slot width to match the sample resolution by default. But yet we have
this code in the driver:
unsigned int word_size = params_width(params);
unsigned int slot_width = params_physical_width(params);
I think slot_width should be the same as word_size, and I suggest changing it
before adding 20/24-bit support.
> Best regards,
> Jernej
Regards,
Samuel
>>> default:
>>> dev_err(dai->dev, "Unsupported physical sample width:
> %d\n",
>>>
>>> params_physical_width(params));
>>>
>>> @@ -1063,6 +1066,10 @@ static int sun4i_i2s_dai_probe(struct snd_soc_dai
>>> *dai)>
>>> return 0;
>>>
>>> }
>>>
>>> +#define SUN4I_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | \
>>> + SNDRV_PCM_FMTBIT_S20_LE | \
>>> + SNDRV_PCM_FMTBIT_S24_LE)
>>> +
>>>
>>> static struct snd_soc_dai_driver sun4i_i2s_dai = {
>>>
>>> .probe = sun4i_i2s_dai_probe,
>>> .capture = {
>>>
>>> @@ -1070,14 +1077,14 @@ static struct snd_soc_dai_driver sun4i_i2s_dai = {
>>>
>>> .channels_min = 1,
>>> .channels_max = 8,
>>> .rates = SNDRV_PCM_RATE_8000_192000,
>>>
>>> - .formats = SNDRV_PCM_FMTBIT_S16_LE,
>>> + .formats = SUN4I_FORMATS,
>>>
>>> },
>>> .playback = {
>>>
>>> .stream_name = "Playback",
>>> .channels_min = 1,
>>> .channels_max = 8,
>>> .rates = SNDRV_PCM_RATE_8000_192000,
>>>
>>> - .formats = SNDRV_PCM_FMTBIT_S16_LE,
>>> + .formats = SUN4I_FORMATS,
>>>
>>> },
>>> .ops = &sun4i_i2s_dai_ops,
>>> .symmetric_rates = 1,
>
>
>
>
On Wed, Sep 02, 2020 at 09:22:33PM -0500, Samuel Holland wrote:
> On 9/2/20 1:10 PM, Jernej Škrabec wrote:
> > Hi Samuel!
> >
> > Dne petek, 10. julij 2020 ob 07:44:51 CEST je Samuel Holland napisal(a):
> >> On 7/4/20 6:38 AM, Clément Péron wrote:
> >>> From: Marcus Cooper <[email protected]>
> >>>
> >>> Extend the functionality of the driver to include support of 20 and
> >>> 24 bits per sample.
> >>>
> >>> Signed-off-by: Marcus Cooper <[email protected]>
> >>> Signed-off-by: Clément Péron <[email protected]>
> >>> ---
> >>>
> >>> sound/soc/sunxi/sun4i-i2s.c | 11 +++++++++--
> >>> 1 file changed, 9 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> >>> index f78167e152ce..bc7f9343bc7a 100644
> >>> --- a/sound/soc/sunxi/sun4i-i2s.c
> >>> +++ b/sound/soc/sunxi/sun4i-i2s.c
> >>> @@ -577,6 +577,9 @@ static int sun4i_i2s_hw_params(struct
> >>> snd_pcm_substream *substream,>
> >>> case 16:
> >>> width = DMA_SLAVE_BUSWIDTH_2_BYTES;
> >>> break;
> >>>
> >>> + case 32:
> >>> + width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> >>> + break;
> >>
> >> This breaks the sun4i variants, because sun4i_i2s_get_wss returns 4 for a 32
> >> bit width, but it needs to return 3.
> >
> > I'm not sure what has WSS with physical width and DMA?
>
> This is the change where creating a S24_LE stream no longer fails with -EINVAL.
> So this is the change where userspace stops downsampling 24-bit audio sources.
> So this is the change where playback of 24-bit audio sources breaks, because WSS
> is programmed wrong.
>
> >> As a side note, I wonder why we use the physical width (the spacing between
> >> samples in RAM) to drive the slot width. S24_LE takes up 4 bytes per sample
> >> in RAM, which we need for DMA. But I don't see why we would want to
> >> transmit the padding over the wire. I would expect it to be transmitted the
> >> same as S24_3LE (which has no padding). It did not matter before, because
> >> the only supported format had no padding.
> >
> > Allwinner DMA engines support only 1, 2, 4 and sometimes 8 bytes for bus
> > width, so if sample is 24 bits in size, we have no other way but to transmit
> > padding too.
>
> I understand why we do 4 byte DMA from RAM <=> I2S FIFO; that was not my
> question. I'm referring to the actual wire format (FIFO <=> PCM_DIN/DOUT). The
> sample is already truncated from 32 bits to 24 bits in the FIFO -- that's what
> TXIM and RXOM in FIFO_CTRL control.
>
> If a sample is 24 bits wide, why would we send 32 BCLKs for every LRCK? I would
> expect the slot width to match the sample resolution by default. But yet we have
> this code in the driver:
>
> unsigned int word_size = params_width(params);
> unsigned int slot_width = params_physical_width(params);
>
> I think slot_width should be the same as word_size, and I suggest changing it
> before adding 20/24-bit support.
Generally speaking, the slot width doesn't necessarily match the
physical width. With TDM for example you may very well have slots
larger than their samples.
That being said, S24 is explicitly a format where you send a sample of
24 bits in a 32-bit word (in the lowest three bytes, little endian)
See:
https://elixir.bootlin.com/linux/v5.9-rc3/source/sound/core/pcm_misc.c#L75
https://mailman.alsa-project.org/pipermail/alsa-devel/2013-April/061073.html
24 bits of data over three bytes like you suggest is S24_3LE
Maxime
On Thu, Sep 03, 2020 at 09:40:23AM +0200, Maxime Ripard wrote:
> On Wed, Sep 02, 2020 at 09:22:33PM -0500, Samuel Holland wrote:
> > On 9/2/20 1:10 PM, Jernej Škrabec wrote:
> > > Hi Samuel!
> > >
> > > Dne petek, 10. julij 2020 ob 07:44:51 CEST je Samuel Holland napisal(a):
> > >> On 7/4/20 6:38 AM, Clément Péron wrote:
> > >>> From: Marcus Cooper <[email protected]>
> > >>>
> > >>> Extend the functionality of the driver to include support of 20 and
> > >>> 24 bits per sample.
> > >>>
> > >>> Signed-off-by: Marcus Cooper <[email protected]>
> > >>> Signed-off-by: Clément Péron <[email protected]>
> > >>> ---
> > >>>
> > >>> sound/soc/sunxi/sun4i-i2s.c | 11 +++++++++--
> > >>> 1 file changed, 9 insertions(+), 2 deletions(-)
> > >>>
> > >>> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> > >>> index f78167e152ce..bc7f9343bc7a 100644
> > >>> --- a/sound/soc/sunxi/sun4i-i2s.c
> > >>> +++ b/sound/soc/sunxi/sun4i-i2s.c
> > >>> @@ -577,6 +577,9 @@ static int sun4i_i2s_hw_params(struct
> > >>> snd_pcm_substream *substream,>
> > >>> case 16:
> > >>> width = DMA_SLAVE_BUSWIDTH_2_BYTES;
> > >>> break;
> > >>>
> > >>> + case 32:
> > >>> + width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> > >>> + break;
> > >>
> > >> This breaks the sun4i variants, because sun4i_i2s_get_wss returns 4 for a 32
> > >> bit width, but it needs to return 3.
> > >
> > > I'm not sure what has WSS with physical width and DMA?
> >
> > This is the change where creating a S24_LE stream no longer fails with -EINVAL.
> > So this is the change where userspace stops downsampling 24-bit audio sources.
> > So this is the change where playback of 24-bit audio sources breaks, because WSS
> > is programmed wrong.
> >
> > >> As a side note, I wonder why we use the physical width (the spacing between
> > >> samples in RAM) to drive the slot width. S24_LE takes up 4 bytes per sample
> > >> in RAM, which we need for DMA. But I don't see why we would want to
> > >> transmit the padding over the wire. I would expect it to be transmitted the
> > >> same as S24_3LE (which has no padding). It did not matter before, because
> > >> the only supported format had no padding.
> > >
> > > Allwinner DMA engines support only 1, 2, 4 and sometimes 8 bytes for bus
> > > width, so if sample is 24 bits in size, we have no other way but to transmit
> > > padding too.
> >
> > I understand why we do 4 byte DMA from RAM <=> I2S FIFO; that was not my
> > question. I'm referring to the actual wire format (FIFO <=> PCM_DIN/DOUT). The
> > sample is already truncated from 32 bits to 24 bits in the FIFO -- that's what
> > TXIM and RXOM in FIFO_CTRL control.
> >
> > If a sample is 24 bits wide, why would we send 32 BCLKs for every LRCK? I would
> > expect the slot width to match the sample resolution by default. But yet we have
> > this code in the driver:
> >
> > unsigned int word_size = params_width(params);
> > unsigned int slot_width = params_physical_width(params);
> >
> > I think slot_width should be the same as word_size, and I suggest changing it
> > before adding 20/24-bit support.
>
> Generally speaking, the slot width doesn't necessarily match the
> physical width. With TDM for example you may very well have slots
> larger than their samples.
>
> That being said, S24 is explicitly a format where you send a sample of
> 24 bits in a 32-bit word (in the lowest three bytes, little endian)
>
> See:
> https://elixir.bootlin.com/linux/v5.9-rc3/source/sound/core/pcm_misc.c#L75
> https://mailman.alsa-project.org/pipermail/alsa-devel/2013-April/061073.html
>
> 24 bits of data over three bytes like you suggest is S24_3LE
>
My understanding is physical_width refers to the in memory
representation, but shouldn't be used to control the slot width
on the bus. If not specified otherwise (say through the set_tdm
callback), and if the appropriate BCLK is supported, then the slot
should be just large enough to hold the data.
Thanks,
Charles
On Fri, Sep 04, 2020 at 04:16:49PM +0000, Charles Keepax wrote:
> My understanding is physical_width refers to the in memory
> representation, but shouldn't be used to control the slot width
> on the bus. If not specified otherwise (say through the set_tdm
> callback), and if the appropriate BCLK is supported, then the slot
> should be just large enough to hold the data.
Indeed. The framework isn't great here in tying the memory and wire
formats together, ideally there would be more support for them being
unrelated without DPCM.