drivers: dma: intel-adsp-hda: Correct DGCS:SCS bit for 32bit sample size #63784

ujfalusi · 2023-10-11T05:23:43Z

If the channel was used for 16bit in the once, subsequent 32bit sample size audio will be broken since the SCS bit remains set.

Example sequence with SOF:
normal audio playback with 16bit
ChainDMA audio playback with 16bit
normal audio playback with 16bit

The last playback results garbled audio.

Introduce intel_adsp_hda_set_sample_container_size() helper function to handle the SCS bit and use it in the driver.

kv2019i

Excellent find @ujfalusi !

This fixes SOF bug tracked as thesofproject/sof#8236

jsarha

Makes perfect sense.

mengdonglin · 2023-10-11T08:28:19Z

If the channel was used for 16bit in the once, subsequent 32bit sample size audio will be broken since the SCS bit remains set.

Example sequence with SOF:
normal audio playback with 16bit
ChainDMA audio playback with 16bit
normal audio playback with 16bit
The last playback results garbled audio.

@ujfalusi I have silly question. The last normal playback is still 16bit, why it results garbled audio? Or does chain DMA uses 16bit container, and normal playback use 32bit container with 16bit valid?

ujfalusi · 2023-10-11T10:13:17Z

If the channel was used for 16bit in the once, subsequent 32bit sample size audio will be broken since the SCS bit remains set.
Example sequence with SOF:
normal audio playback with 16bit
ChainDMA audio playback with 16bit
normal audio playback with 16bit
The last playback results garbled audio.

@ujfalusi I have silly question. The last normal playback is still 16bit, why it results garbled audio? Or does chain DMA uses 16bit container, and normal playback use 32bit container with 16bit valid?

Yes, that is correct:
With ChainDMA the linkDMA is configured to the same format as the hostDMA - the firmware merely forwards the data stream.
When ChainDMA is not used the linkDMA is most of the time have fixed configuration of 32bit sample size.
The firmware converts the 16bit from host to 32bit internally and it is (tries to) sending 32bit sample size to the codec, but the linkDMA retained the SCS=1 from the ChainDMA use, the codec is also configured to expect 32bit data, but the configuration of HDA and DMA is off and that causes garbled audio.

drivers/dma/dma_intel_adsp_hda.c

ujfalusi · 2023-10-11T10:36:21Z

Changes since v1:

Braces added around the if () else statements

plbossart

The host side is fine, the link has a change that's not very clear @ujfalusi

plbossart · 2023-10-11T13:11:03Z

drivers/dma/dma_intel_adsp_hda.c

 		*DGCS(cfg->base, cfg->regblock_size, channel) |= DGCS_SCS;
+	} else {
+		*DGCS(cfg->base, cfg->regblock_size, channel) &= ~DGCS_SCS;


this one changes the behavior if res != 0, is this intentional @ujfalusi?

Oh, I have missed that, sorry I will update as soon as I can.

plbossart · 2023-10-11T13:11:24Z

drivers/dma/dma_intel_adsp_hda.c

 		*DGCS(cfg->base, cfg->regblock_size, channel) |= DGCS_SCS;
+	} else {
+		*DGCS(cfg->base, cfg->regblock_size, channel) &= ~DGCS_SCS;


same here, change with res!=0?

teburd

Missed the ret != 0 drop that @plbossart found while looking at this on my phone screen. This does need fixing before this should be merged, so marked -1

ujfalusi · 2023-10-11T15:42:23Z

Changes since v2:

do not ignore the res from intel_adsp_hda_set_buffer() in the linkDMA configurations

ujfalusi · 2023-10-11T15:50:14Z

One more try, sorry, I missed something else

ujfalusi · 2023-10-11T15:54:15Z

Changes since v3:

corrected the copy-paste issue of replacing dest_data_size with source_data_size.

If the channel was used for 16bit in the once, subsequent 32bit sample size audio will be broken since the SCS bit remains set. Example sequence with SOF: normal audio playback with 16bit ChainDMA audio playback with 16bit normal audio playback with 16bit The last playback results garbled audio. Introduce intel_adsp_hda_set_sample_container_size() helper function to handle the SCS bit and use it in the driver. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

ujfalusi · 2023-10-12T06:17:38Z

Changes since v4:

a bit more elegant way is to introduce intel_adsp_hda_set_sample_container_size() and use that in the driver
the resulting code is easier to read and shorter.

ranj063 · 2023-10-12T13:46:19Z

soc/xtensa/intel_adsp/common/include/intel_adsp_hda.h

+		*DGCS(base, regblock_size, sid) |= DGCS_SCS;
+	} else {
+		*DGCS(base, regblock_size, sid) &= ~DGCS_SCS;
+	}


do we need braces for the if/else?

@ranj063 yes, this is actually required in Zephyr

ujfalusi requested review from teburd, abonislawski, kv2019i and jxstelter October 11, 2023 05:23

zephyrbot added the area: DMA Direct Memory Access label Oct 11, 2023

zephyrbot assigned teburd Oct 11, 2023

ujfalusi mentioned this pull request Oct 11, 2023

[BUG][MTL][HDA] Sporadically Audio glitches on on-board speakers upon unplug of HDMI/DP external display cable thesofproject/sof#8236

Closed

kv2019i previously approved these changes Oct 11, 2023

View reviewed changes

kv2019i requested review from tmleman and serhiy-katsyuba-intel October 11, 2023 06:35

abonislawski previously approved these changes Oct 11, 2023

View reviewed changes

tmleman previously approved these changes Oct 11, 2023

View reviewed changes

jsarha approved these changes Oct 11, 2023

View reviewed changes

serhiy-katsyuba-intel previously approved these changes Oct 11, 2023

View reviewed changes

jxstelter previously approved these changes Oct 11, 2023

View reviewed changes

fabiobaltieri requested changes Oct 11, 2023

View reviewed changes

drivers/dma/dma_intel_adsp_hda.c Outdated Show resolved Hide resolved

ujfalusi dismissed stale reviews from jxstelter, serhiy-katsyuba-intel, tmleman, abonislawski, and kv2019i via 50b473e October 11, 2023 10:36

ujfalusi force-pushed the peter/pr/hda_dma_scs_bit_clear_01 branch from a3cb662 to 50b473e Compare October 11, 2023 10:36

fabiobaltieri previously approved these changes Oct 11, 2023

View reviewed changes

fabiobaltieri added this to the v3.5.0 milestone Oct 11, 2023

teburd approved these changes Oct 11, 2023

View reviewed changes

abonislawski previously approved these changes Oct 11, 2023

View reviewed changes

plbossart reviewed Oct 11, 2023

View reviewed changes

teburd requested changes Oct 11, 2023

View reviewed changes

teburd added the bug The issue is a bug, or the PR is fixing a bug label Oct 11, 2023

ujfalusi dismissed stale reviews from abonislawski and fabiobaltieri via 59600b2 October 11, 2023 15:41

ujfalusi force-pushed the peter/pr/hda_dma_scs_bit_clear_01 branch from 50b473e to 59600b2 Compare October 11, 2023 15:41

ujfalusi force-pushed the peter/pr/hda_dma_scs_bit_clear_01 branch from 59600b2 to b9a3498 Compare October 11, 2023 15:53

ujfalusi force-pushed the peter/pr/hda_dma_scs_bit_clear_01 branch from b9a3498 to d37c057 Compare October 12, 2023 06:16

ujfalusi requested review from dcpleung, andyross and nashif as code owners October 12, 2023 06:16

zephyrbot added the platform: Intel ADSP Intel Audio platforms label Oct 12, 2023

zephyrbot requested review from lgirdwood and lyakh October 12, 2023 06:16

plbossart approved these changes Oct 12, 2023

View reviewed changes

ranj063 reviewed Oct 12, 2023

View reviewed changes

kv2019i approved these changes Oct 12, 2023

View reviewed changes

teburd approved these changes Oct 12, 2023

View reviewed changes

fabiobaltieri approved these changes Oct 12, 2023

View reviewed changes

fabiobaltieri requested a review from jhedberg October 12, 2023 14:14

jhedberg approved these changes Oct 12, 2023

View reviewed changes

jhedberg merged commit 8dfa116 into zephyrproject-rtos:main Oct 12, 2023
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: dma: intel-adsp-hda: Correct DGCS:SCS bit for 32bit sample size #63784

drivers: dma: intel-adsp-hda: Correct DGCS:SCS bit for 32bit sample size #63784

ujfalusi commented Oct 11, 2023 •

edited

kv2019i left a comment

jsarha left a comment

mengdonglin commented Oct 11, 2023

ujfalusi commented Oct 11, 2023

ujfalusi commented Oct 11, 2023

plbossart left a comment

plbossart Oct 11, 2023

ujfalusi Oct 11, 2023

plbossart Oct 11, 2023

teburd left a comment •

edited

ujfalusi commented Oct 11, 2023

ujfalusi commented Oct 11, 2023

ujfalusi commented Oct 11, 2023

ujfalusi commented Oct 12, 2023

ranj063 Oct 12, 2023

kv2019i Oct 12, 2023

drivers: dma: intel-adsp-hda: Correct DGCS:SCS bit for 32bit sample size #63784

drivers: dma: intel-adsp-hda: Correct DGCS:SCS bit for 32bit sample size #63784

Conversation

ujfalusi commented Oct 11, 2023 • edited

kv2019i left a comment

Choose a reason for hiding this comment

jsarha left a comment

Choose a reason for hiding this comment

mengdonglin commented Oct 11, 2023

ujfalusi commented Oct 11, 2023

ujfalusi commented Oct 11, 2023

plbossart left a comment

Choose a reason for hiding this comment

plbossart Oct 11, 2023

Choose a reason for hiding this comment

ujfalusi Oct 11, 2023

Choose a reason for hiding this comment

plbossart Oct 11, 2023

Choose a reason for hiding this comment

teburd left a comment • edited

Choose a reason for hiding this comment

ujfalusi commented Oct 11, 2023

ujfalusi commented Oct 11, 2023

ujfalusi commented Oct 11, 2023

ujfalusi commented Oct 12, 2023

ranj063 Oct 12, 2023

Choose a reason for hiding this comment

kv2019i Oct 12, 2023

Choose a reason for hiding this comment

ujfalusi commented Oct 11, 2023 •

edited

teburd left a comment •

edited