linux-kernel - RE: [PATCH] ASoC: Intel: Atom: use hardware counter to update hw

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: <DM6PR11MB3642AF0905E0CF57B094906297730@DM6PR11MB3642.namprd11.prod.outlook.com>
Date:   Tue, 28 Jul 2020 17:02:23 +0000
From:   "Lu, Brent" <brent.lu@...el.com>
To:     Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
        "alsa-devel@...a-project.org" <alsa-devel@...a-project.org>
CC:     "Rojewski, Cezary" <cezary.rojewski@...el.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Jie Yang <yang.jie@...ux.intel.com>,
        Takashi Iwai <tiwai@...e.com>,
        Liam Girdwood <liam.r.girdwood@...ux.intel.com>,
        Mark Brown <broonie@...nel.org>
Subject: RE: [PATCH] ASoC: Intel: Atom: use hardware counter to update hw_ptr

> 
> So if there are already quirks in atom machine drivers to change the period
> size, why is this patch necessary?
> 

The story is: google implemented the constraint but doesn't know why it works
so asked us to explain. After checking the two counters I realized the increase of
ring buffer pointer follows the period size setting in hw_param (256) but the
period of interrupt is always 5ms instead of 5.33 so it's running little bit too fast.
It seems the LPE keeps tracking the difference of two counters. When the
difference exceeds 2160 samples, the next interrupt will be canceled so the
hardware counter could catch up a little.

[   43.208299] intel_sst_acpi 808622A8:00: mrfld ring_buffer_counter 107520 hardware_counter 98880 pcm delay 8640 (in bytes)
[   43.208306] intel_sst_acpi 808622A8:00: buffer ptr 26880 pcm_delay rep: 2160
[   43.208321] sound pcmC1D0p: [Q] pos 26880 hw_ptr 26880 appl_ptr 40000 avail 191680
=> one interrupt is skipped.
[   43.218299] intel_sst_acpi 808622A8:00: mrfld ring_buffer_counter 108544 hardware_counter 100800 pcm delay 7744 (in bytes)
[   43.218307] intel_sst_acpi 808622A8:00: buffer ptr 27136 pcm_delay rep: 1936
[   43.218336] sound pcmC1D0p: [Q] pos 27136 hw_ptr 27136 appl_ptr 40000 avail 191936

So I think why not using the hardware counter? It increases 240 samples every 5ms
perfectly match the 48000 sample rate. The test result is good but I know there must
be a reason for the original designer to use ring buffer counter instead of hardware
counter. I uploaded this patch to see if anyone still remember the reason and share
some insight with me.

I totally agree that we shouldn't touch this part of design. Do you think it make sense
to add a constraint to enforce the period size in machine driver? If yes then I would
upload patches for Chrome atom machines for google.

Regards,
Brent

> > I'm curious why not just using hardware counter to update hw_ptr and
> > get rid of the period setting in hw_param? It seems to me the ring
> > buffer counter does not reflect the real status.
> 
> I don't recall precisely what this hardware counter does. I vaguely recall it's
> tied to the 19.2MHz external timer which is also used to schedule the 1ms
> SBA mixer and the SSP IOs. And by comparing with the ring buffer pointer
> you can infer the delay inside the DSP. I think you are also making an
> assumption that all streams are tied to the output rate, but that's most likely
> a bad assumption. The hard-coded topology supported media, speech and
> compressed data and the consumption rate on the DMA side could be faster
> with some buffering happening in the DSP.
> It's not a passthrough DMA in all cases.
> 
> This is really legacy code that no one really fully understands nor plans on
> improving, it'd be a bad idea to change the pcm pointer reports now, 6 years
> after the initial code release and after all initial contributors moved on. It's
> what it is.
>