linux-kernel - Re: [PATCH v3 3/7] ASoC: sun4i-i2s: Add support for H6 I2S

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200917132128.e6z4gwoluipbzigm@gilmour.lan>
Date:   Thu, 17 Sep 2020 15:21:28 +0200
From:   Maxime Ripard <maxime@...no.tech>
To:     Samuel Holland <samuel@...lland.org>
Cc:     Clément Péron <peron.clem@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Rob Herring <robh+dt@...nel.org>, Chen-Yu Tsai <wens@...e.org>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>,
        Linux-ALSA <alsa-devel@...a-project.org>,
        devicetree <devicetree@...r.kernel.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Jernej Skrabec <jernej.skrabec@...l.net>,
        Marcus Cooper <codekipper@...il.com>
Subject: Re: [PATCH v3 3/7] ASoC: sun4i-i2s: Add support for H6 I2S

Hi,

On Sat, Sep 12, 2020 at 03:29:55PM -0500, Samuel Holland wrote:
> On 9/10/20 9:33 AM, Maxime Ripard wrote:
> > On Thu, Sep 03, 2020 at 09:54:39PM -0500, Samuel Holland wrote:
> >> On 9/3/20 3:58 PM, Maxime Ripard wrote:
> >>> On Thu, Sep 03, 2020 at 10:02:31PM +0200, Clément Péron wrote:
> >>>> Hi Maxime,
> >>>>
> >>>> On Wed, 29 Jul 2020 at 17:16, Mark Brown <broonie@...nel.org> wrote:
> >>>>>
> >>>>> On Wed, Jul 29, 2020 at 04:39:27PM +0200, Maxime Ripard wrote:
> >>>>>
> >>>>>> It really looks like the polarity of LRCK is fine though. The first word
> >>>>>> is sent with LRCK low, and then high, so we have channel 0 and then
> >>>>>> channel 1 which seems to be the proper ordering?
> >>
> >> Which image file is this in reference to?
> >>
> >>>>> Yes, that's normal.
> >>>>
> >>>> Thank you very much for this test.
> >>>>
> >>>> So I will revert the following commit:
> >>>>
> >>>> ASoC: sun4i-i2s: Fix the LRCK polarity
> >>>>
> >>>> https://github.com/clementperon/linux/commit/dd657eae8164f7e4bafe8b875031a7c6c50646a9
> >>>
> >>> Like I said, the current code is working as expected with regard to the
> >>> LRCK polarity. The issue is that the samples are delayed and start to be
> >>> transmitted on the wrong phase of the signal.
> >>
> >> Since an I2S LRCK frame is radially symmetric, "wrong phase" and "inverted
> >> polarity" look the same. The only way to definitively distinguish them is by
> >> looking at the sample data.
> >>
> >> In "i2s-h6.png", the samples are all zeroes, so you're assuming that the first
> >> sample transmitted (that is, when the bit clock starts transitioning) was a
> >> "left" sample.
> >>
> >> However, in "h6-i2s-start-data.png", there are pairs of samples we can look at.
> >> I'm still assuming that similar samples are a left/right pair, but that's
> >> probably a safe assumption. Here we see the first sample in each pair is
> >> transmitted with LRCK *high*, and the second sample in the pair is transmitted
> >> with LRCK *low*. This is the opposite of your claim above.
> >>
> >> An ideal test would put left/right markers and frame numbers in the data
> >> channel. The Python script below can generate such a file. Then you would know
> >> how much startup delay there is, which channel the "first sample" came from, and
> >> how each channel maps to the LRCK level.
> >>
> >> It would also be helpful to test DSP_A mode, where the LRCK signal is
> >> asymmetric and an inversion would be obvious.
> > 
> > I had no idea that there was a wave module in Python, that's a great
> > suggestion, thanks!
> > 
> > You'll find attached the screenshots for both the I2S and DSP_A formats.
> > I zoomed out a bit to be able to have the first valid samples, but it
> > should be readable.
> > 
> > The code I used is there:
> > https://github.com/mripard/linux/tree/sunxi/h6-i2s-test
> > 
> > It's basically the v3, plus the DT bits.
> > 
> > As you can see, in the i2s case, LRCK starts low and then goes up, with
> > the first channel (0x2*** samples) transmitted first, so everything
> > looks right here.
> > 
> > On the DSP_A screenshot, LRCK will be low with small bursts high, and
> > once again with the first channel being transmitted first, so it looks
> > right to me too.
> 
> Indeed, for H6 i2s0 with LRCK inversion in software, everything looks correct on
> the wire.
> 
> It's still concerning to me that the BSP has no evidence of this inversion,
> either for i2s0 or i2s1[1]. And the inversion seems not to be required for HDMI
> audio on mainline either (but there could be an inversion on the HDMI side or on
> the interconnect).

One can only guess here, but it's also quite easy to fix it at the card
level (or maybe there's a similar inversion in the codecs, or whatever).

> Even so, your research is sufficient justification for me that the code is
> correct as-is (with the inversion). Thank you very much for collecting the data!

You're welcome, thanks for that script :)

maxime

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)