Skip to content

Conversation

@plbossart
Copy link
Member

The DMA handling is not correct on capture when we have 4 channels aggregated from 2 DAIs on 2 different links. We currently setup the DMA for 2 channels, which causes the data to be recorded at half-speed.

Also this enables chain-DMA support for all interfaces, only tested on ALH playback for now.

Still draft since the capture w/ chain DMA does not work - xrun on capture.

@plbossart
Copy link
Member Author

Moving to ready. This was tested in hardware, and the only problematic case is the capture which is also broken for MTL, so it's a firmware issue clearly.

@plbossart
Copy link
Member Author

actually, need to fix checkpatch issues.

@plbossart plbossart force-pushed the fix/lnl-alh-chain-dma branch from f5a0eb6 to aee60c1 Compare August 29, 2023 16:46
@plbossart
Copy link
Member Author

wow, clang found an uninitialized variable....Amazing that other compilers don't detect this!

sound/soc/sof/intel/hda-dai.c:473:4: error: variable 'ch_mask' is uninitialized when used here [-Werror,-Wuninitialized]
                        ch_mask |= rtd->dai_link->codec_ch_maps[j].ch_mask;
                        ^~~~~~~
sound/soc/sof/intel/hda-dai.c:443:13: note: initialize the variable 'ch_mask' to silence this warning
        int ch_mask;
                   ^
                    = 0

@plbossart plbossart force-pushed the fix/lnl-alh-chain-dma branch from aee60c1 to d690df9 Compare August 29, 2023 18:16
bardliao
bardliao previously approved these changes Aug 30, 2023
Copy link
Collaborator

@bardliao bardliao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Collaborator

@ujfalusi ujfalusi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@plbossart, I'm not sure about the tying of the ChainDMA to DSPless mode, is it future proof?

RanderWang
RanderWang previously approved these changes Aug 31, 2023
Copy link

@RanderWang RanderWang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@plbossart plbossart dismissed stale reviews from RanderWang and bardliao via 97a0775 September 12, 2023 21:12
@plbossart plbossart force-pushed the fix/lnl-alh-chain-dma branch from d690df9 to 97a0775 Compare September 12, 2023 21:12
@plbossart
Copy link
Member Author

@RanderWang @ujfalusi v2 updated, see diff
v2.diff.txt

Copy link
Collaborator

@ujfalusi ujfalusi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about the definition of the snd_sof_is_dai_supported and especially the SOF_DAI_FEATURE_ definition.

Also, I think there is an issue with the dai validity check for Intel platforms.

…ed dailink

The existing code derives the channel map used to program the HDaudio
link DMA from the hw_params, but that is not quite right in the case
of aggregation. The code in soc-pcm.c splits the hw_params depending
on the codec_ch_map, and we need to reconstruct the channel-map to
insert the data in the right places.

This issue is seen only on amplifier feedback capture where the data
from the second amplifier was replaced by that of the first amplifier.

Note that the loop iterator of the macro for_each_rtd_cpu_dais() is
reused in a following loop. This is different to all existing usages
of that macro, hence the use of a boolean flag to avoid an access to
an uninitialized variable.

Fixes: 2960ee5 ("ASoC: SOF: Intel: hda-dai: add helpers for SoundWire callbacks")
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
The generic format calculation is shared between SSP, DMIC and
SoundWire.

In the latter case, the dailink may handle more channels than what
each DAI can handle. This is specifically the case for aggregated
configurations, specifically for capture where we may want to handle a
4ch stream using two DAIs.

The issue is that soc-pcm.c already splits the channels on a per-dai
basis, so we need to count the total number of channels handled by the
dailink. This isn't very elegant but the information on the dailink
configuration is not available at the DAI level.

The net effect of this confusion between DAI and dailink channels was
that the data was recorded 2x slower than normal, and when played at
the normal rate a 2x frequency shift can be heard.

Fixes: 1254773 ("ASoC: SOF: Intel: hda-dai-ops: add/select DMA ops for SSP")
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
The chain_dma mode is currently only handled for HDaudio, but can be
used for orther DAIs starting with LunarLake. Move the chain_dma
handling earlier.

Error detection for the chain_dma case for older platforms is handled
at a different level.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
IPC4 introduced a 'chain-dma' mode when host and link DMA are
connected by firmware without using a regular pipeline or the ability
to add intermediate connections. This mode is not available on all
platforms and all links, so add a platform-specific callback to help
the SOF ipc4-topology core handle different hardware+firmware
configurations.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reuse existing function to get the interface mask and expose it to the
SOF core with a callback - the main user is the IPC4 topology so only
HDaudio platforms provide this callback.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
@plbossart
Copy link
Member Author

new version with simpler 'is_chain_dma_supported' callback and all comments addressed. Feedback welcome.

bardliao
bardliao previously approved these changes Sep 26, 2023
RanderWang
RanderWang previously approved these changes Sep 26, 2023
Copy link

@RanderWang RanderWang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Use the existing callbacks and mix/match of HDaudio and SoundWire
support.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Now that we have a 'is_chain_dma_supported' callback we can use it to
double-check possible disconnects between a topology file enabling
chain-dma for a DAI and the hardware/firmware capabilities.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
@plbossart plbossart dismissed stale reviews from RanderWang and bardliao via ad29504 October 3, 2023 16:08
@plbossart plbossart force-pushed the fix/lnl-alh-chain-dma branch from da910d3 to ad29504 Compare October 3, 2023 16:08
Copy link

@RanderWang RanderWang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants