linux-kernel - Re: [PATCH 2/2] ASoC: fsl: fsl_qmc_audio: Only request completion on last channel

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] [day] [month] [year] [list]

Message-ID: <20250509121242.1f660e7f@bootlin.com>
Date: Fri, 9 May 2025 12:12:42 +0200
From: Herve Codina <herve.codina@...tlin.com>
To: Christophe Leroy <christophe.leroy@...roup.eu>
Cc: Qiang Zhao <qiang.zhao@....com>, Shengjiu Wang
 <shengjiu.wang@...il.com>, Xiubo Li <Xiubo.Lee@...il.com>, Fabio Estevam
 <festevam@...il.com>, Nicolin Chen <nicoleotsuka@...il.com>, Liam Girdwood
 <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>, Jaroslav Kysela
 <perex@...ex.cz>, Takashi Iwai <tiwai@...e.com>,
 linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org,
 linux-arm-kernel@...ts.infradead.org, linux-sound@...r.kernel.org
Subject: Re: [PATCH 2/2] ASoC: fsl: fsl_qmc_audio: Only request completion
 on last channel

Hi Christophe,

On Fri, 9 May 2025 11:13:12 +0200
Christophe Leroy <christophe.leroy@...roup.eu> wrote:

> Hi Hervé,
> 
> Le 09/05/2025 à 10:45, Herve Codina a écrit :
> > On Fri,  9 May 2025 09:48:45 +0200
> > Christophe Leroy <christophe.leroy@...roup.eu> wrote:
> >   
> >> In non-interleaved mode, several QMC channels are used in sync.
> >> More details can be found in commit 188d9cae5438 ("ASoC: fsl:
> >> fsl_qmc_audio: Add support for non-interleaved mode.")
> >> At the time being, an interrupt is requested on each channel to
> >> perform capture/playback completion, allthough the completion is
> >> really performed only once all channels have completed their work.
> >>
> >> This leads to a lot more interrupts than really needed. Looking at
> >> /proc/interrupts shows ~3800 interrupts per second when using
> >> 4 capture and 4 playback devices with 5ms periods while
> >> only 1600 (200 x 4 + 200 x 4) periods are processed during one second.
> >>
> >> The QMC channels work in sync, the one started first is the one
> >> finishing first and the one started last is the one finishing last,  
> > 
> > How can we be sure about that?
> > 
> > The mapping on the TDM bus has to be taken into account.
> > 
> > chan 0 -> TDM bits 0..8
> > chan 1 -> TDM bits 16..23
> > chan 2 -> TDM bits 9..15  
> 
> In interleaved mode, the QMC will not allow that. You can have 
> TS0-TS1-TS2 or TS1-TS2-TS0 but you can't have TS0-TS2-TS1.
> 
> In non-interleaved mode we mimic the interleaved mode so I don't expect 
> it either.

I am not so sure that the case shouldn't be handled by QMC.
Even if it is not possible at QMC level, you can have it at qmc_audio
level.

The qmc_audio driver depends on the DT binding:
	dai@18 {
            reg = <18>;
            /* Non-interleaved mode */
            fsl,qmc-chan = <&qmc 18>, <&qmc 19>;
        };

but you can have
	dai@18 {
            reg = <18>;
            /* Non-interleaved mode */
            fsl,qmc-chan = <&qmc 19>, <&qmc 18>;
        };

> 
> > 
> > In that case chan 1 can finish after chan 2.
> > 
> > qmc_chan_get_ts_info() could be used to get struct qmc_chan_ts_info
> > and [rx,tx]_ts_mask field in the struct give the mapping information.
> > 
> > The channel that ends last is the one with the highest bit set in the
> > mask (rx_tx_mask for capture and tx_ts_mask for playback).  
> 
> That would be right if the channels were starting all at exactely the 
> same time. But qmc_audio_pcm_write_submit() and 
> qmc_audio_pcm_read_submit() are calling resp. qmc_chan_write_submit() 
> and qmc_chan_read_submit() one by one.
> 
> Even if that happens it shouldn't be a problem on its own as there are 
> only a few microseconds between each Timeslot (a full cycle is 125 µs). 
> And also because calling snd_pcm_period_elapsed() doesn't have any 
> destructive effect on ongoing processing.
> 
> So I wouldn't make it too complicated. Here the benefit is real and 
> worth it.

I fully understand the benefit and I am not against the feature.
Also, I fully agree that it has to be kept as simple as possible.

My point is to avoid some possible regressions.

Maybe during probe, when the channels are parsed [0], the code should take
of the channel location on TDM to have them in the better order in its
table.

We can imagine that after filling the array, the driver sorts the array
using sort() or sort_r() [1] to ensure that the last item in the array
is the last on the TDM bus.

[0] https://elixir.bootlin.com/linux/v6.15-rc5/source/sound/soc/fsl/fsl_qmc_audio.c#L833
[1] https://elixir.bootlin.com/linux/v6.15-rc5/source/include/linux/sort.h

-- 
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com