Re: ASoC: soc-pcm: Don't zero TDM masks in __soc_pcm_open() breaks SOF Audio in Lenovo laptops

Re: ASoC: soc-pcm: Don't zero TDM masks in __soc_pcm_open() breaks SOF Audio in Lenovo laptops

[Richard Fitzgerald]

On 9/12/22 14:42, Pierre-Louis Bossart wrote:
> 
> 
> On 12/9/22 01:37, Péter Ujfalusi wrote:
>>
>>
>> On 08/12/2022 18:37, Pierre-Louis Bossart wrote:
>>>
>>>
>>> On 12/8/22 10:17, Mark Brown wrote:
>>>> On Thu, Dec 08, 2022 at 02:02:02PM +0000, Joakim Tjernlund wrote:
>>>>> Several of our Lenovo laptops lost PC audio output in Teams in 5.15.81(also in 5.15.82)
>>>>> Revering above patch:https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?h=linux-5.15.y&id=b2ddd76237121155dcadfc4ae77ca1775dfc99f7
>>>>> fixes it.
>>>>>
>>>>> Any idea what the real fix is?
>>>>
>>>> Adding the Intel people.  I've no idea if there's issues with
>>>> dependencies, missing quirks for the hardare or anything in that stable
>>>> version.
>>>
>>> Humm, yes in the past we used the TDM masks to convey the 'stream_tag'
>>> for HDaudio.
>>>
>>> I can still see this in v5.15.85 in sound/soc/sof/intel/hda-dai.c
>>>
>>> 	/* set the stream tag in the codec dai dma params */
>>> 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>>> 		snd_soc_dai_set_tdm_slot(codec_dai, stream_tag, 0, 0, 0);
>>> 	else
>>> 		snd_soc_dai_set_tdm_slot(codec_dai, 0, stream_tag, 0, 0);
>>>
>>>
>>> That was changed in
>>> 636110411ca72 ASoC: Intel/SOF: use set_stream() instead of
>>> set_tdm_slots() for HDAudio
>>>
>>> snd_soc_dai_set_stream(codec_dai, hdac_stream(hext_stream),
>>> substream->stream);
>>>
>>> So my guess is that zeroing out TDM masks has a side effect on older
>>> stable kernels, and that effect is not seen on newer kernels.
>>>
>>> I don't really understand what the side effect might be though.
>>
>> The reason is that on the the tdm mask now became persistent and the HDA
>> code relied on the fact that it is volatile, it is reset for each stream
>> open.
>> The core would do some fixup if the tx/rx_mask is set:
>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/sound/soc/soc-pcm.c?h=linux-5.15.y&#n963
>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/sound/soc/soc-pcm.c?h=linux-5.15.y&#n968
>>
>> This did not happened when the tx/rx_mask was reset at open - and later
>> set bny the HDA driver, I guess after these checks, so the fixup did not
>> happened, but without resetting them we would use the previously set
>> masks to do fixups for the number of channels (!) based on a stream_tag
>> value, which has nothing to do with channels.
> 
> Agree with the analysis, so what would be the least bad recommendation?
> a) revert the "don't zero TDM masks" patch
> b) backport the change to use set_stream()?

Adding alsa-devel@alsa-project.org and patches@opensource.cirrus.com.
Please add relevant lists when sending emails so that other people who
might need to know, or have an opinion, are aware of the discussion.

Re: ASoC: soc-pcm: Don't zero TDM masks in __soc_pcm_open() breaks SOF Audio in Lenovo laptops

[Richard Fitzgerald]

On 9/12/22 16:18, Mark Brown wrote:
> On Fri, Dec 09, 2022 at 05:38:54PM +0200, Péter Ujfalusi wrote:
>> On 09/12/2022 16:42, Pierre-Louis Bossart wrote:
> 
>>> Agree with the analysis, so what would be the least bad recommendation?
>>> a) revert the "don't zero TDM masks" patch
>>> b) backport the change to use set_stream()?
> 
>> I would vote for b) unless other platforms report regressions.
> 
> I don't really care either way given the backport policy.

Neither do I.
If you want to minimize risk, revert the patch in older backports.

Lessons learned:
Don't hijack a data member to pass something different from what it
is intended to hold.
Don't depend on a bug.
Don't assume all code is using a data member for what is supposed to be
in that member.

Re: ASoC: soc-pcm: Don't zero TDM masks in __soc_pcm_open() breaks SOF Audio in Lenovo laptops

[Joakim Tjernlund]

On Fri, 2022-12-09 at 16:26 +0000, Richard Fitzgerald wrote:
> On 9/12/22 16:18, Mark Brown wrote:
> > On Fri, Dec 09, 2022 at 05:38:54PM +0200, Péter Ujfalusi wrote:
> > > On 09/12/2022 16:42, Pierre-Louis Bossart wrote:
> > 
> > > > Agree with the analysis, so what would be the least bad recommendation?
> > > > a) revert the "don't zero TDM masks" patch
> > > > b) backport the change to use set_stream()?
> > 
> > > I would vote for b) unless other platforms report regressions.
> > 
> > I don't really care either way given the backport policy.
> 
> Neither do I.
> If you want to minimize risk, revert the patch in older backports.
> 
> Lessons learned:
> Don't hijack a data member to pass something different from what it
> is intended to hold.
> Don't depend on a bug.
> Don't assume all code is using a data member for what is supposed to be
> in that member.

Did you reach a conclusion w.r.t what the should be?

 Jocke

Re: ASoC: soc-pcm: Don't zero TDM masks in __soc_pcm_open() breaks SOF Audio in Lenovo laptops

[Pierre-Louis Bossart]

On 12/13/22 10:57, Joakim Tjernlund wrote:
> On Fri, 2022-12-09 at 16:26 +0000, Richard Fitzgerald wrote:
>> On 9/12/22 16:18, Mark Brown wrote:
>>> On Fri, Dec 09, 2022 at 05:38:54PM +0200, Péter Ujfalusi wrote:
>>>> On 09/12/2022 16:42, Pierre-Louis Bossart wrote:
>>>
>>>>> Agree with the analysis, so what would be the least bad recommendation?
>>>>> a) revert the "don't zero TDM masks" patch
>>>>> b) backport the change to use set_stream()?
>>>
>>>> I would vote for b) unless other platforms report regressions.
>>>
>>> I don't really care either way given the backport policy.
>>
>> Neither do I.
>> If you want to minimize risk, revert the patch in older backports.
>>
>> Lessons learned:
>> Don't hijack a data member to pass something different from what it
>> is intended to hold.
>> Don't depend on a bug.
>> Don't assume all code is using a data member for what is supposed to be
>> in that member.
> 
> Did you reach a conclusion w.r.t what the should be?

I read the consensus is for a backport of the 'set_stream' stuff. You're
welcome to submit a patch.