ASoC: pxa: possible regressions

ASoC: pxa: possible regressions

[Petr Cvek]

Hello,

I was trying to update my system (PXA Magician) to 4.18-next and i've 
ran into few problems with sound. My setup has only a few changes from 
vanilla (change to using devm_snd_soc_register_card+devm_gpio_request). 
I've started digging around:

1) unimportant goto

Commit 7afd1b0b2ef9a812095 ("ASoC: pxa: move some functions to 
pxa2xx-lib") is this construct necessary?

	if (pcm->streams[SNDRV_PCM_STREAM_CAPTURE].substream) {
		ret = pxa2xx_pcm_preallocate_dma_buffer(pcm,
			SNDRV_PCM_STREAM_CAPTURE);
		if (ret)
			goto out;
	}
  out:
	return ret;

After calling pxa2xx_pcm_preallocate_dma_buffer() the code will jump to 
out anyway.

2) wrong units (sample rate vs osc rate)

Commit 05739375f1c0a1048 ("ASoC: pxa-ssp: remove .set_pll() and 
.set_clkdiv() callbacks"). The function pxa_ssp_hw_params() get's called 
with sample rate so:

	/* The values in the table are for 16 bits */
	if (width == 32)
		acds--;

	ret = pxa_ssp_set_pll(priv, bclk);
	if (ret < 0)
		return ret;

	ssacd = pxa_ssp_read_reg(ssp, SSACD);
	ssacd &= ~(SSACD_ACDS(7) | SSACD_SCDB_1X);

pxa_ssp_set_pll() will try to set something like 8000, 44100, ... and 
not the speed of an oscilator. The line should be probably:

	ret = pxa_ssp_set_pll(priv, m->pll);

and there is a previous call to pxa_ssp_set_pll():

	if (sscr0 & SSCR0_ACS) {
		ret = pxa_ssp_set_pll(priv, bclk);

This one is OK I suppose?

3) setup fails

Recent changes in ASoC PXA DMA are causing a fail of a condition in 
pxa_ssp_hw_params():

	if ((sscr0 & SSCR0_MOD) && !ttsa) {
		dev_err(&ssp->pdev->dev, "No TDM timeslot configured\n");
		return -EINVAL;
	}

It seems the SSCR0_MOD flag is set in the fall through switch statement 
in pxa_ssp_configure_dai_fmt():

	case SND_SOC_DAIFMT_DSP_A:
		sspsp |= SSPSP_FSRT;
		/* fall through */
	case SND_SOC_DAIFMT_DSP_B:
		sscr0 |= SSCR0_MOD | SSCR0_PSP;
		sscr1 |= SSCR1_TRAIL | SSCR1_RWOT;
		break;

the switch statement is an old code and it worked in the past versions.

4) names are not unique

The initialization of magician_dai structure wants to have (at least in 
the past versions) for the .name item:

	static struct snd_soc_dai_link magician_dai[] = {
	{
		.name = "uda1380",
		.stream_name = "UDA1380 Playback",
		.cpu_dai_name = "pxa-ssp-dai.0",
		...
	{
		.name = "uda1380",
		.stream_name = "UDA1380 Capture",
		.cpu_dai_name = "pxa2xx-i2s",

my personal branch has them named as "uda1380_playback" and 
"uda1380_capture".

5) Unbalanced references

When trying to play a sound (with quick-and-dirty workarounds for 
problems above):

   # cat /dev/urandom | aplay -

the system will crash when terminating aplay (stopping playback). The 
backtrace:

  ------------[ cut here ]------------
  WARNING: CPU: 0 PID: 352 at drivers/dma/dmaengine.c:788 
dma_release_channel+0x44/0xa0
  chan reference count 0 != 1
  Modules linked in: snd_soc_magician snd_soc_uda1380 snd_soc_pxa2xx_i2s 
spi_pxa2xx_platform snd_soc_pxa_ssp snd_soc_pxa2xx snd_pxa2xx_lib 
snd_pcm_dmaengine snd_soc_core snd_pcm videobuf2_common snd_timer snd 
ssp soundcore ... [last unloaded: udc_core]
  CPU: 0 PID: 352 Comm: aplay Tainted: G         C 
4.18.0-rc6-next-20180726-magician+ #15
  Hardware name: HTC Magician
  [<c0107d90>] (unwind_backtrace) from [<c010594c>] (show_stack+0x10/0x14)
  [<c010594c>] (show_stack) from [<c0111680>] (__warn+0xd4/0xec)
  [<c0111680>] (__warn) from [<c01112d0>] (warn_slowpath_fmt+0x44/0x6c)
  [<c01112d0>] (warn_slowpath_fmt) from [<c034908c>] 
(dma_release_channel+0x44/0xa0)
  [<c034908c>] (dma_release_channel) from [<bf3183d4>] 
(snd_dmaengine_pcm_close_release_chan+0x2c/0x3c [snd_pcm_dmaengine])
  [<bf3183d4>] (snd_dmaengine_pcm_close_release_chan 
[snd_pcm_dmaengine]) from [<bf1e4fbc>] 
(soc_pcm_components_close+0x64/0x70 [snd_soc_core])
  [<bf1e4fbc>] (soc_pcm_components_close [snd_soc_core]) from 
[<bf1e74a0>] (soc_pcm_close+0xac/0x1c0 [snd_soc_core])
  [<bf1e74a0>] (soc_pcm_close [snd_soc_core]) from [<bf1407c8>] 
(snd_pcm_release_substream+0x68/0xb8 [snd_pcm])
  [<bf1407c8>] (snd_pcm_release_substream [snd_pcm]) from [<bf14085c>] 
(snd_pcm_release+0x44/0x8c [snd_pcm])
  [<bf14085c>] (snd_pcm_release [snd_pcm]) from [<c01c39e4>] 
(__fput+0xdc/0x1e8)
  [<c01c39e4>] (__fput) from [<c0129e7c>] (task_work_run+0xc4/0xd4)
  [<c0129e7c>] (task_work_run) from [<c01053ac>] (do_work_pending+0xc0/0xc8)
  [<c01053ac>] (do_work_pending) from [<c0101068>] 
(slow_work_pending+0xc/0x20)
  Exception stack(0xc2ffbfb0 to 0xc2ffbff8)
  bfa0:                                     00000000 01f7a1a0 01f6fa3c 
00000000
  bfc0: 01f7a150 00000000 0009283c 00000006 000003f0 01f7a4c8 00000000 
01f7a4c8
  bfe0: b6ebbd70 be8ea960 b6e287b8 b6cd2018 60000010 00000004
  ---[ end trace b671f6531db6f6b3 ]---

I've only managed to discover there are two calls of 
pxa2xx_soc_pcm_new() (where DMA buffer is allocated) for both dai links:

	magician-audio magician-audio: uda1380-hifi-playback <-> pxa-ssp-dai.0 
mapping ok
	...
	magician-audio magician-audio: uda1380-hifi-capture <-> pxa2xx-i2s 
mapping ok

but I don't know if this information is relevant.

thanks for any help,
Petr Cvek

ASoC: pxa: possible regressions

[Daniel Mack]

Hi Petr,

On Wednesday, August 08, 2018 04:24 AM, Petr Cvek wrote:
> I was trying to update my system (PXA Magician) to 4.18-next and i've
> ran into few problems with sound.

Ah, so you have a Magician platform, that's very good!

> My setup has only a few changes from
> vanilla (change to using devm_snd_soc_register_card+devm_gpio_request).

Care to share these changes, or ideally rebase them on top of 4.18-next 
for mainline inclusion? Also, I'm curious what mainline version you used 
to be based on that worked for you.

> I've started digging around:

Thanks for that. Are you also willing to send patches to fix things once 
they work for you?

> 1) unimportant goto
> 
> Commit 7afd1b0b2ef9a812095 ("ASoC: pxa: move some functions to
> pxa2xx-lib") is this construct necessary?
> 
> 	if (pcm->streams[SNDRV_PCM_STREAM_CAPTURE].substream) {
> 		ret = pxa2xx_pcm_preallocate_dma_buffer(pcm,
> 			SNDRV_PCM_STREAM_CAPTURE);
> 		if (ret)
> 			goto out;
> 	}
>    out:
> 	return ret;
> 
> After calling pxa2xx_pcm_preallocate_dma_buffer() the code will jump to
> out anyway.

True. That's a cosmetic cleanup which can go in for 4.19. Would you want 
to send a patch after the merge window?

> 2) wrong units (sample rate vs osc rate)
> 
> Commit 05739375f1c0a1048 ("ASoC: pxa-ssp: remove .set_pll() and
> .set_clkdiv() callbacks"). The function pxa_ssp_hw_params() get's called
> with sample rate so:
> 
> 	/* The values in the table are for 16 bits */
> 	if (width == 32)
> 		acds--;
> 
> 	ret = pxa_ssp_set_pll(priv, bclk);
> 	if (ret < 0)
> 		return ret;
> 
> 	ssacd = pxa_ssp_read_reg(ssp, SSACD);
> 	ssacd &= ~(SSACD_ACDS(7) | SSACD_SCDB_1X);
> 
> pxa_ssp_set_pll() will try to set something like 8000, 44100, ... and
> not the speed of an oscilator. The line should be probably:
> 
> 	ret = pxa_ssp_set_pll(priv, m->pll);
> 
> and there is a previous call to pxa_ssp_set_pll():

Ah, yes. Does changing that fix it for you?

The changes in that machine driver where done blindly because I don't 
have Magician hardware, and I really don't like doing things that way.

> 3) setup fails
> 
> Recent changes in ASoC PXA DMA are causing a fail of a condition in
> pxa_ssp_hw_params():
> 
> 	if ((sscr0 & SSCR0_MOD) && !ttsa) {

What are the values of sscr0 and ttsa in your case? Can you cross-check 
that with a kernel that doesn't have my recent changes?

> 		dev_err(&ssp->pdev->dev, "No TDM timeslot configured\n");
> 		return -EINVAL;
> 	}
> 
> It seems the SSCR0_MOD flag is set in the fall through switch statement
> in pxa_ssp_configure_dai_fmt():
> 
> 	case SND_SOC_DAIFMT_DSP_A:
> 		sspsp |= SSPSP_FSRT;
> 		/* fall through */
> 	case SND_SOC_DAIFMT_DSP_B:
> 		sscr0 |= SSCR0_MOD | SSCR0_PSP;
> 		sscr1 |= SSCR1_TRAIL | SSCR1_RWOT;
> 		break;
> 
> the switch statement is an old code and it worked in the past versions.

Yes, it was merely moved to some location that is called earlier. This 
was fixing a regression caused by a8bd0ee558714 ("ASoC: raumfeld: Use 
static DAI format setup"), so supposedly, all PXA sound was broken since 
v4.0.

> 4) names are not unique
> 
> The initialization of magician_dai structure wants to have (at least in
> the past versions) for the .name item:
> 
> 	static struct snd_soc_dai_link magician_dai[] = {
> 	{
> 		.name = "uda1380",
> 		.stream_name = "UDA1380 Playback",
> 		.cpu_dai_name = "pxa-ssp-dai.0",
> 		...
> 	{
> 		.name = "uda1380",
> 		.stream_name = "UDA1380 Capture",
> 		.cpu_dai_name = "pxa2xx-i2s",
> 
> my personal branch has them named as "uda1380_playback" and
> "uda1380_capture".

Care to send a patch for that as well, please?

> 5) Unbalanced references
> 
> When trying to play a sound (with quick-and-dirty workarounds for
> problems above):
> 
>     # cat /dev/urandom | aplay -
> 
> the system will crash when terminating aplay (stopping playback). The
> backtrace:

So you did hear a sound?

[...]

> I've only managed to discover there are two calls of
> pxa2xx_soc_pcm_new() (where DMA buffer is allocated) for both dai links:
> 
> 	magician-audio magician-audio: uda1380-hifi-playback <-> pxa-ssp-dai.0
> mapping ok
> 	...
> 	magician-audio magician-audio: uda1380-hifi-capture <-> pxa2xx-i2s
> mapping ok
> 
> but I don't know if this information is relevant.

That seems fine. The question is why there are are unbalanced calls to 
snd_dmaengine_pcm_close_release_chan(). Could you dig a bit further 
maybe and add some printk()?

Thanks for helping fix these regressions!


Daniel

ASoC: pxa: possible regressions

[Petr Cvek]

Dne 8.8.2018 v 07:38 Daniel Mack napsal(a):
> 
>> My setup has only a few changes from
>> vanilla (change to using devm_snd_soc_register_card+devm_gpio_request).
> 
> Care to share these changes, or ideally rebase them on top of 4.18-next 
> for mainline inclusion? Also, I'm curious what mainline version you used 
> to be based on that worked for you.

I was planning to send patches before I've found regressions in the 
current -next.

I think I've had the first working version somewhere around 3.13, but I 
was updating versions only sporadically and fixing audio had low 
priority (higher priority got iwmmxt compiler, platform init source 
code, charger, USB, MMC and IrDA). Some versions until now worked, some 
didn't.

The last version I've used and where the sound worked was 
4.14.0-rc7-next from somewhere around 3rd November 2017. The patch for 
sound subsystem should be the same as the current one.

The current patch is based on vanilla 4.18.0-rc6-next-20180726.

> 
>> 2) wrong units (sample rate vs osc rate)
>>
>> Commit 05739375f1c0a1048 ("ASoC: pxa-ssp: remove .set_pll() and
>> .set_clkdiv() callbacks"). The function pxa_ssp_hw_params() get's called
>> with sample rate so:
>>
>> ????/* The values in the table are for 16 bits */
>> ????if (width == 32)
>> ??????? acds--;
>>
>> ????ret = pxa_ssp_set_pll(priv, bclk);
>> ????if (ret < 0)
>> ??????? return ret;
>>
>> ????ssacd = pxa_ssp_read_reg(ssp, SSACD);
>> ????ssacd &= ~(SSACD_ACDS(7) | SSACD_SCDB_1X);
>>
>> pxa_ssp_set_pll() will try to set something like 8000, 44100, ... and
>> not the speed of an oscilator. The line should be probably:
>>
>> ????ret = pxa_ssp_set_pll(priv, m->pll);
>>
>> and there is a previous call to pxa_ssp_set_pll():
> 
> Ah, yes. Does changing that fix it for you?
> 

Nope it just allows the code to proceed behind.

The fixing 3) (SSCR0_MOD check) has the same effect. It just postpones 
the closing of the device until a valid termination of aplay.

> 
>> 3) setup fails
>>
>> Recent changes in ASoC PXA DMA are causing a fail of a condition in
>> pxa_ssp_hw_params():
>>
>> ????if ((sscr0 & SSCR0_MOD) && !ttsa) {
> 
> What are the values of sscr0 and ttsa in your case? Can you cross-check 
> that with a kernel that doesn't have my recent changes?
> 

-> full values in the attached log (the flag is set in the switch case 
and ttsa is zeroed)

On 4.14 (not sure, if it is the correct SSP controller)

Idle:

SSCR0:
	# devmem 0x41000000 32
	0x4000003F <- 31 bit is SSCR0_MOD
SSCR1:
	# devmem 0x41000004 32
	0x00C01D80
SSTSA:
	# devmem 0x41000030 32
	0x00000000 <- is "ttsa" variable
SSRSA:
	# devmem 0x41000034 32
	0x00000000
SSTSS:
	# devmem 0x41000038 32
	0x00000000

Playing noise:

SSCR0:
	# devmem 0x41000000 32
	0x400000BF <- 31 bit is SSCR0_MOD
SSCR1:
	# devmem 0x41000004 32
	0x00E01D80
SSTSA:
	# devmem 0x41000030 32
	0x00000000 <- is "ttsa" variable
SSRSA:
	# devmem 0x41000034 32
	0x00000000
SSTSS:
	# devmem 0x41000038 32
	0x00000000

.. so the time slots are correctly disabled.

>> 4) names are not unique
>>
>> The initialization of magician_dai structure wants to have (at least in
>> the past versions) for the .name item:
>>
>> ????static struct snd_soc_dai_link magician_dai[] = {
>> ????{
>> ??????? .name = "uda1380",
>> ??????? .stream_name = "UDA1380 Playback",
>> ??????? .cpu_dai_name = "pxa-ssp-dai.0",
>> ??????? ...
>> ????{
>> ??????? .name = "uda1380",
>> ??????? .stream_name = "UDA1380 Capture",
>> ??????? .cpu_dai_name = "pxa2xx-i2s",
>>
>> my personal branch has them named as "uda1380_playback" and
>> "uda1380_capture".
> 
> Care to send a patch for that as well, please?

If this is a correct solution then yes.

> 
>> 5) Unbalanced references
>>
>> When trying to play a sound (with quick-and-dirty workarounds for
>> problems above):
>>
>> ??? # cat /dev/urandom | aplay -
>>
>> the system will crash when terminating aplay (stopping playback). The
>> backtrace:
> 
> So you did hear a sound?

No, nothing went through. Only clicking was probably caused by muting 
the speaker output.

> 
> That seems fine. The question is why there are are unbalanced calls to 
> snd_dmaengine_pcm_close_release_chan(). Could you dig a bit further 
> maybe and add some printk()?

Yes I did, patch with asserts and matching log attached. It gets really 
ugly but though. It is full dump of changes, so there is a conversion of 
magician-audio driver for module_platform_driver (wasn't able to load it 
from vanilla from around 3.13) and some bugfixes in uda codec 
(uda1380-hifi removed because it didn't work with split magician I2S/SSP 
channels and I didn't test it lately, and a bugfix for name/register 
settings).

Petr
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-ASoC-pxa-magician-testing.patch
Type: text/x-patch
Size: 53389 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180808/e511863f/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: magician_sound.log
Type: text/x-log
Size: 16299 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180808/e511863f/attachment-0003.bin>

ASoC: pxa: possible regressions

[Daniel Mack]

Hi Petr,

On Wednesday, August 08, 2018 11:07 AM, Petr Cvek wrote:
> Dne 8.8.2018 v 07:38 Daniel Mack napsal(a):

> I was planning to send patches before I've found regressions in the
> current -next.
> 
> I think I've had the first working version somewhere around 3.13, but I
> was updating versions only sporadically and fixing audio had low
> priority (higher priority got iwmmxt compiler, platform init source
> code, charger, USB, MMC and IrDA). Some versions until now worked, some
> didn't.
> 
> The last version I've used and where the sound worked was
> 4.14.0-rc7-next from somewhere around 3rd November 2017. The patch for
> sound subsystem should be the same as the current one.

That's strange, because as I said, the SSP rework went in in 4.0, so 
without my regression fix (737e370a57e4e8 "ASoC: pxa-ssp: allow more 
flexible setup order"), I doubt SSP had worked for you.

>>> 3) setup fails
>>>
>>> Recent changes in ASoC PXA DMA are causing a fail of a condition in
>>> pxa_ssp_hw_params():
>>>
>>>  ????if ((sscr0 & SSCR0_MOD) && !ttsa) {
>>
>> What are the values of sscr0 and ttsa in your case? Can you cross-check
>> that with a kernel that doesn't have my recent changes?
>>
> 
> -> full values in the attached log (the flag is set in the switch case
> and ttsa is zeroed)
> 
> On 4.14 (not sure, if it is the correct SSP controller)

Hmm, why shouldn't it be the correct one? And do you hear sound with 
that 4.14 setup?

> 
> Idle:
> 
> SSCR0:
> 	# devmem 0x41000000 32
> 	0x4000003F <- 31 bit is SSCR0_MOD
> SSCR1:
> 	# devmem 0x41000004 32
> 	0x00C01D80
> SSTSA:
> 	# devmem 0x41000030 32
> 	0x00000000 <- is "ttsa" variable
> SSRSA:
> 	# devmem 0x41000034 32
> 	0x00000000
> SSTSS:
> 	# devmem 0x41000038 32
> 	0x00000000
> 
> Playing noise:
> 
> SSCR0:
> 	# devmem 0x41000000 32
> 	0x400000BF <- 31 bit is SSCR0_MOD
> SSCR1:
> 	# devmem 0x41000004 32
> 	0x00E01D80
> SSTSA:
> 	# devmem 0x41000030 32
> 	0x00000000 <- is "ttsa" variable
> SSRSA:
> 	# devmem 0x41000034 32
> 	0x00000000
> SSTSS:
> 	# devmem 0x41000038 32
> 	0x00000000
> 
> .. so the time slots are correctly disabled.

The idea was to compare a working against a non-working setup to spot 
the differences in the register settings.

If recent mainline versions have worked for you (modulo some minor 
modifications), I think one good way forward could be to bisect the 
problems and make the platform work for 4.18 again. Then, see if any of 
the recent ASoC/DMA related PXA patches break it again.

Sorry for the trouble, but without hardware, these things are hard to 
reproduce on my side.


Let me know if I can help further, but not that my resources are a bit 
limited in the next weeks.


Thanks,
Daniel

ASoC: pxa: possible regressions

[Petr Cvek]

Dne 13.8.2018 v 07:54 Daniel Mack napsal(a):
> Hi Petr,
> 
> On Wednesday, August 08, 2018 11:07 AM, Petr Cvek wrote:
>> Dne 8.8.2018 v 07:38 Daniel Mack napsal(a):
> 
>> I was planning to send patches before I've found regressions in the
>> current -next.
>>
>> I think I've had the first working version somewhere around 3.13, but I
>> was updating versions only sporadically and fixing audio had low
>> priority (higher priority got iwmmxt compiler, platform init source
>> code, charger, USB, MMC and IrDA). Some versions until now worked, some
>> didn't.
>>
>> The last version I've used and where the sound worked was
>> 4.14.0-rc7-next from somewhere around 3rd November 2017. The patch for
>> sound subsystem should be the same as the current one.
> 
> That's strange, because as I said, the SSP rework went in in 4.0, so
> without my regression fix (737e370a57e4e8 "ASoC: pxa-ssp: allow more
> flexible setup order"), I doubt SSP had worked for you.
> 

OK I found the problem I've had compiled and load (on both v4.14 and
v4.18) snd-soc-pxa2xx driver (from linux/sound/arm/pxa2xx-pcm.c). This
component calls pxa2xx_soc_pcm_new() which is already called by
snd_soc_pxa_ssp (pxa-ssp-dai.0) or snd_soc_pxa2xx_i2s (pxa2xx-i2s). In
the function the dma gets registered twice, which is a wrong behavior
(kernel oops on DMA stopping). Without snd-soc-pxa2xx driver loaded the
playback works (after the rest of my patches) so it seems in the old
version DMA wasn't registered twice.

So the easy solution for snd_soc_pxa_ssp OR snd_soc_pxa2xx_i2s would be
a mutual exclusivity with snd_soc_pxa_ssp. I don't know about other
users, but I can add a condition to magician Kconfig entry.

Best regards,
Petr