[BUG] Broadwell: channel inversion on SSP0 output

Describe the bug
Left and right channels are inverted on Broadwell Samus

To Reproduce
play on speakers or headset

Reproduction Rate
100%

Expected behavior
left on left, right on right

Impact
user experience

Environment

Branch name and commit hash of the 2 repositories: sof (firmware/topology) and linux (kernel driver).
- Kernel: {SHA}3fb1efccd560e388ca2527dcdda282528bfa4cc4
- SOF: {SHA} 21f1344
Name of the topology file
- Topology: sof-bdw-rt5677.tplg
Name of the platform(s) on which the bug is observed.
- Platform: Broadwell Samus

Screenshots or console output

update: this does not look like a frame polarity issue, in DSP_A mode with a single bit pulse, the left channel is in the wrong location

This looks more like a bad DMA programming or a true data error

Edit: I double-checked my connections with the legacy driver + closed source firmware, and the left channel comes out in the expected location. This is really an SOF firmware issue

@plbossart

@plbossart I wonder if the DMA word size is 16 bits rather than 32 bit writes to the SSP FIFO.

@plbossart

@plbossart I wonder if the DMA word size is 16 bits rather than 32 bit writes to the SSP FIFO.

oh, so all samples would be off by 16 bits then, i.e. not a channel inversion but a delay pushing the right sample into the next frame?

DMA looks ok, but I do see lots of comments in ssp.c for frame polarity and "logic for inverted_frame" flag.

For DSP_A the frame polarity looks fine.

@lyakh

@lyakh FYI since you are scrubbing the DW-DMA code, might be something related to this issue.

The channel inversion appears to happen on pulseaudio --start. Without UCM files and running the UCM manually with amixer, everything is correct in ALSA without pulseaudio. I'm seeing

amixer -c 1 cset name='PDM1 L Mux' STO1 DAC MIX
... snip ...
amixer -c 1 cset name='Speaker Switch' on
speaker-test -Ddefault:rt5677 -c2 # correct
pulseaudio --start
speaker-test -Ddefault:rt5677 -c2 # inverted

Anyone else?

@caseorum

@caseorum Fwiw, pulseaudio start does "probe" each PCM by opening and trying many different params for each PCM (which will configure different SSP settings). I wonder here if the probing is setting some SSP register transitions that swap the channels ?
Could you try and "reset" the SSP settings (probably by writing all 0s or defaults to SSP regs) at the start of ssp_set_config().

@caseorum

@caseorum sorry but not buying the results. I have a Samus blue-wired to see the SSP signals and I could see the inversion at the lowest level, it's not related to amixer settings. I also see the inversion on Dell 9343 which uses a different codec.

@lgirdwood

@lgirdwood That's what I was thinking, whatever probing or other init pulseaudio is doing has the effect. I was hoping this might give someone who knows pulseaudio's probing better than I an idea. I'll take a look. It looks like ssp_set_config() already has some sensible resetting. Do you have something specific worth trying?

@plbossart I set amixer settings manually just to rule out any auto-UCM weirdness in pulseaudio, it's not related. This looks like firmware or kernel driver. Are you able to repro this? I don't have any plans to blue-wire mine. I boot a torvalds/master kernel with built-in FW/tplg through depthcharge directly into a minimal arch linux userspace with git master alsa and pulseaudio, and no initramfs, kernel modules, or non-default alsa/PA configs. It's almost the simplest possible testing environment.

@caseorum

@caseorum you can take pulseaudio out of the equation by using -Dhw:0,0 (or -Dhw:1,0 if you have HDMI first).

@lgirdwood

@lgirdwood Both the correct and inverted state are 100% reproducible and stable. They invert only on PA start and stay that way.

@lgirdwood

@lgirdwood @plbossart Here are the PA logs with SST and SOF, all else being equal. It looks like PA initially uses a period size of 1199 samples, then 1200 afterwards. That might be an off-by-one channel inversion. Thoughts?
palog-sof.txt
palog-sst.txt

that looks like a red-herring to me:

D: [pulseaudio] alsa-util.c: Set buffer size first (to 4797 samples), period size second (to 1199 samples).
I: [pulseaudio] alsa-util.c: Device hw:1 doesn't support 44100 Hz, changed to 48000 Hz.

there's no way SOF can work with odd period sizes.

@caseorum

Looking at ssp_config() it looks like rate and bit size are the only things PA could change (if permitted in teh topology). The only other thing that could impact here is whether PA was also trying to do some capture probing (and somehow capture probing cuases the swap by setting other SSP bits).
@caseorum may be worth a try disabling PA alsa-source module and try the test again. This would narrow down the possibilites further to rate and sample size.

@lgirdwood

@lgirdwood Good call. alsa-source is exactly the module causing the inversion.

Is there a trace or a dump you'd be interested in?

@caseorum

@caseorum can you disable PA again and try an arecord -Dhw:1,0 -f dat -d 5 > test.wav prior to speaker test. I would like to know if its capture or PA probing capture that causing the swap.
Also please try running the arecord and then run speaker test whilst arecord is still running.

speaker-test is correct if started before or during arecord capture, and stays that way as long as it runs, even after arecord has stopped. Channels are swapped if speaker-test is started after arecord exits.

@caseorum

@caseorum thanks, very useful info. One thing I do notice is that

/* start the SSP for either playback or capture */
static void ssp_start(struct dai *dai, int direction)
{
	struct ssp_pdata *ssp = dai_get_drvdata(dai);

	spin_lock(&dai->lock);

	dai_info(dai, "ssp_start()");

	/* enable DMA */
	if (direction == DAI_DIR_PLAYBACK) {
		ssp_update_bits(dai, SSCR1, SSCR1_TSRE | SSCR1_EBCEI,
				SSCR1_TSRE | SSCR1_EBCEI);
		ssp_update_bits(dai, SSCR0, SSCR0_SSE, SSCR0_SSE);
		ssp_update_bits(dai, SSCR0, SSCR0_TIM, 0);
		ssp_update_bits(dai, SSTSA, SSTSA_TSEN, SSTSA_TSEN);
	} else {
		ssp_update_bits(dai, SSCR1, SSCR1_RSRE | SSCR1_EBCEI,
				SSCR1_RSRE | SSCR1_EBCEI);
		ssp_update_bits(dai, SSCR0, SSCR0_SSE, SSCR0_SSE);
		ssp_update_bits(dai, SSCR0, SSCR0_RIM, 0);
		ssp_update_bits(dai, SSRSA, SSRSA_RSEN, SSRSA_RSEN);
	}

	/* enable port */
	ssp->state[direction] = COMP_STATE_ACTIVE;

	spin_unlock(&dai->lock);
}

/* stop the SSP for either playback or capture */
static void ssp_stop(struct dai *dai, int direction)
{
	struct ssp_pdata *ssp = dai_get_drvdata(dai);

	spin_lock(&dai->lock);

	/* stop Rx if neeed */
	if (direction == DAI_DIR_CAPTURE &&
	    ssp->state[SOF_IPC_STREAM_CAPTURE] != COMP_STATE_PREPARE) {
		ssp_update_bits(dai, SSCR1, SSCR1_RSRE, 0);
		ssp_update_bits(dai, SSCR0, SSCR0_RIM, SSCR0_RIM);
		ssp->state[SOF_IPC_STREAM_CAPTURE] = COMP_STATE_PREPARE;
		dai_info(dai, "ssp_stop(), RX stop");
	}

	/* stop Tx if needed */
	if (direction == DAI_DIR_PLAYBACK &&
	    ssp->state[SOF_IPC_STREAM_PLAYBACK] != COMP_STATE_PREPARE) {
		ssp_update_bits(dai, SSCR1, SSCR1_TSRE, 0);
		ssp_update_bits(dai, SSCR0, SSCR0_TIM, SSCR0_TIM);
		ssp->state[SOF_IPC_STREAM_PLAYBACK] = COMP_STATE_PREPARE;
		dai_info(dai, "ssp_stop(), TX stop");
	}

	/* disable SSP port if no users */
	if (ssp->state[SOF_IPC_STREAM_CAPTURE] == COMP_STATE_PREPARE &&
	    ssp->state[SOF_IPC_STREAM_PLAYBACK] == COMP_STATE_PREPARE) {
		ssp_update_bits(dai, SSCR0, SSCR0_SSE, 0);
		dai_info(dai, "ssp_stop(), SSP port disabled");
	}

	spin_unlock(&dai->lock);
}

that we never clear SSRSA_RSEN or SSRSA_TSEN in ssp_stop() after we set them in ssp_start(). @plbossart ?

@caseorum

This is awesome, thanks @caseorum for finding this. I will look into this later this week.

@lgirdwood

@lgirdwood I think you found the bug. I've tested the change you're suggesting (9702bfd) and it works on my samus. Want a PR for this?

@lgirdwood

@lgirdwood I think you found the bug. I've tested the change you're suggesting (9702bfd) and it works on my samus. Want a PR for this?

Yes, please send a PR for that, thanks.

@caseorum

@caseorum yes please 👍 We can hopefully get this in v1.6-rc1 today and definately v1.6

@lgirdwood

Just want to say thanks @lgirdwood, @plbossart, and everyone. This project is awesome work.

Navigation Menu

Repository navigation

[BUG] Broadwell: channel inversion on SSP0 output #2459

4 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

[BUG] Broadwell: channel inversion on SSP0 output #2459

Description

Activity

plbossart commented on Feb 27, 2020

lgirdwood commented on Feb 27, 2020

plbossart commented on Feb 27, 2020

lgirdwood commented on Feb 28, 2020

plbossart commented on Feb 28, 2020

plbossart commented on Aug 11, 2020

caseorum commented on Sep 19, 2020

lgirdwood commented on Sep 21, 2020

plbossart commented on Sep 21, 2020

caseorum commented on Sep 21, 2020

plbossart commented on Sep 21, 2020

4 remaining items

caseorum commented on Sep 22, 2020

caseorum commented on Sep 22, 2020

plbossart commented on Sep 22, 2020

lgirdwood commented on Sep 22, 2020

caseorum commented on Sep 23, 2020

caseorum commented on Sep 23, 2020

lgirdwood commented on Sep 23, 2020

caseorum commented on Sep 23, 2020

lgirdwood commented on Sep 23, 2020

plbossart commented on Sep 23, 2020

caseorum commented on Sep 23, 2020

keyonjie commented on Sep 24, 2020

lgirdwood commented on Sep 24, 2020

caseorum commented on Oct 2, 2020

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions