Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 3 Sep 2020 09:40:23 +0200
From:   Maxime Ripard <maxime@cerno.tech>
To:     Samuel Holland <samuel@sholland.org>
Cc:     Jernej =?utf-8?Q?=C5=A0krabec?= <jernej.skrabec@siol.net>,
        peron.clem@gmail.com, Chen-Yu Tsai <wens@csie.org>,
        Mark Brown <broonie@kernel.org>,
        Liam Girdwood <lgirdwood@gmail.com>,
        Rob Herring <robh+dt@kernel.org>,
        Jaroslav Kysela <perex@perex.cz>,
        Takashi Iwai <tiwai@suse.com>,
        Marcus Cooper <codekipper@gmail.com>,
        alsa-devel@alsa-project.org, devicetree@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-sunxi@googlegroups.com
Subject: Re: [linux-sunxi] [PATCH 05/16] ASoc: sun4i-i2s: Add 20 and 24 bit
 support
Message-ID: <20200903074023.jccqp45br3er4h3g@gilmour.lan>
References: <20200704113902.336911-1-peron.clem@gmail.com>
 <20200704113902.336911-6-peron.clem@gmail.com>
 <1e320dfd-9388-54b2-dba9-7def0bf4bbad@sholland.org>
 <9148679.oVN3Z7rve9@kista>
 <fd714cb6-3650-1eb9-616d-33c00f1442eb@sholland.org>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="hf5an3s4qg2ib7h7"
Content-Disposition: inline
In-Reply-To: <fd714cb6-3650-1eb9-616d-33c00f1442eb@sholland.org>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


--hf5an3s4qg2ib7h7
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Wed, Sep 02, 2020 at 09:22:33PM -0500, Samuel Holland wrote:
> On 9/2/20 1:10 PM, Jernej =C5=A0krabec wrote:
> > Hi Samuel!
> >=20
> > Dne petek, 10. julij 2020 ob 07:44:51 CEST je Samuel Holland napisal(a):
> >> On 7/4/20 6:38 AM, Cl=C3=A9ment P=C3=A9ron wrote:
> >>> From: Marcus Cooper <codekipper@gmail.com>
> >>>
> >>> Extend the functionality of the driver to include support of 20 and
> >>> 24 bits per sample.
> >>>
> >>> Signed-off-by: Marcus Cooper <codekipper@gmail.com>
> >>> Signed-off-by: Cl=C3=A9ment P=C3=A9ron <peron.clem@gmail.com>
> >>> ---
> >>>
> >>>  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,>=20
> >>>  	case 16:
> >>>  		width =3D DMA_SLAVE_BUSWIDTH_2_BYTES;
> >>>  		break;
> >>>
> >>> +	case 32:
> >>> +		width =3D DMA_SLAVE_BUSWIDTH_4_BYTES;
> >>> +		break;
> >>
> >> This breaks the sun4i variants, because sun4i_i2s_get_wss returns 4 fo=
r a 32
> >> bit width, but it needs to return 3.
> >=20
> > I'm not sure what has WSS with physical width and DMA?
>=20
> This is the change where creating a S24_LE stream no longer fails with -E=
INVAL.
> So this is the change where userspace stops downsampling 24-bit audio sou=
rces.
> So this is the change where playback of 24-bit audio sources breaks, beca=
use WSS
> is programmed wrong.
>=20
> >> As a side note, I wonder why we use the physical width (the spacing be=
tween
> >> samples in RAM) to drive the slot width. S24_LE takes up 4 bytes per s=
ample
> >> 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 transmitte=
d the
> >> same as S24_3LE (which has no padding). It did not matter before, beca=
use
> >> the only supported format had no padding.
> >=20
> > Allwinner DMA engines support only 1, 2, 4 and sometimes 8 bytes for bu=
s=20
> > width, so if sample is 24 bits in size, we have no other way but to tra=
nsmit=20
> > padding too.
>=20
> I understand why we do 4 byte DMA from RAM <=3D> I2S FIFO; that was not my
> question. I'm referring to the actual wire format (FIFO <=3D> PCM_DIN/DOU=
T). The
> sample is already truncated from 32 bits to 24 bits in the FIFO -- that's=
 what
> TXIM and RXOM in FIFO_CTRL control.
>=20
> 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:
>=20
>     unsigned int word_size =3D params_width(params);
>     unsigned int slot_width =3D params_physical_width(params);
>=20
> I think slot_width should be the same as word_size, and I suggest changin=
g 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

--hf5an3s4qg2ib7h7
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYIAB0WIQRcEzekXsqa64kGDp7j7w1vZxhRxQUCX1Cd5wAKCRDj7w1vZxhR
xQAeAP9vjuO9Y3b+CKzmLOQ15SsDQDnKlVOEAQ0bmzds+Gn3dgEAvPzB50hkYbu/
m3XNc7Nyk17Y4tBWmLWyuoQ8IodG1QM=
=gvkH
-----END PGP SIGNATURE-----

--hf5an3s4qg2ib7h7--