Re: [alsa-devel] ASoC: pxa: possible regressions
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?
- 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?
- 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.
- 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.
- 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?
- 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
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.
- 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.
- 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.
- 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.
- 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
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.
- 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
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
participants (2)
-
Daniel Mack -
Petr Cvek