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 <codekipper@xxxxxxxxx> > > >>> > > >>> Extend the functionality of the driver to include support of 20 and > > >>> 24 bits per sample. > > >>> > > >>> Signed-off-by: Marcus Cooper <codekipper@xxxxxxxxx> > > >>> Signed-off-by: Clément Péron <peron.clem@xxxxxxxxx> > > >>> --- > > >>> > > >>> 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
