-
Notifications
You must be signed in to change notification settings - Fork 349
topology1: SoundWire/HDAudio: add headphone deep buffer support #4611
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
topology1: SoundWire/HDAudio: add headphone deep buffer support #4611
Conversation
defa9f5 to
7226163
Compare
7226163 to
7e5b1f7
Compare
|
SOFCI TEST |
|
@plbossart some failures on TGL, retesting to rule out CI. |
|
Oh man, it looks like this exposes a real bad kernel bug @bardliao @RanderWang FYI |
|
I can reproduce the issue on CML SKU 09C6 Also failure on |
|
It looks like the BE dailink is triggered twice SDW_STREAM_ENABLED = 3, This means the dailink/stream is already triggered/enabled and we try to trigger it again? |
|
with this diff diff --git a/sound/soc/intel/boards/sof_sdw.c b/sound/soc/intel/boards/sof_sdw.c
index dde94e256a5b..e7835047e1b8 100644
--- a/sound/soc/intel/boards/sof_sdw.c
+++ b/sound/soc/intel/boards/sof_sdw.c
@@ -312,12 +312,14 @@ int sdw_trigger(struct snd_pcm_substream *substream, int cmd)
case SNDRV_PCM_TRIGGER_START:
case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
case SNDRV_PCM_TRIGGER_RESUME:
+ dev_err(rtd->dev, "enabling stream for DAI %s", dai->name);
ret = sdw_enable_stream(sdw_stream);
break;
case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
case SNDRV_PCM_TRIGGER_SUSPEND:
case SNDRV_PCM_TRIGGER_STOP:
+ dev_err(rtd->dev, "disabling stream for DAI %s", dai->name);
ret = sdw_disable_stream(sdw_stream);
break;
default:the result is confirmed: something's not correct with a mixer topology. @bardliao I am running out of time for today, any idea why this might be happening? |
|
@plbossart @bardliao could there be a duplicate path in the topology that causes triggering twice ? or two different widgets that use the same DAI ? |
Yes, I am also thinking are there two different widgets that use the same DAI. I am trying to figure it out. |
|
@plbossart It looks like a timing issue. |
@plbossart See dpcm_be_dai_trigger |
Phew, this doesn't look good, are we missing a mutex somewhere then? |
The stream management currently flags an 'inconsistent state' error when a change is requested multiple times. This was added on purpose to identify programming mistakes. This is however imcompatible with the current ASoC-DPCM implementation. There is currently no mutual exclusion in dpcm_be_dai_trigger(), and if two FEs connected to the same BE are started concurrently the state management the BE may be triggered twice. The following code shows the state can be tested before being updated: case SNDRV_PCM_TRIGGER_START: if ((be->dpcm[stream].state != SND_SOC_DPCM_STATE_PREPARE) && (be->dpcm[stream].state != SND_SOC_DPCM_STATE_STOP) && (be->dpcm[stream].state != SND_SOC_DPCM_STATE_PAUSED)) continue; ret = soc_pcm_trigger(be_substream, cmd); if (ret) goto end; be->dpcm[stream].state = SND_SOC_DPCM_STATE_START; This patch suggests allowing the stream functions to be idempotent, i.e. they can be called multiple times. Note that the prepare case was already handling multiple calls, this was added in commit c32464c ("soundwire: stream: only prepare stream when it is configured.") FIXME: is this the right fix? Seem like DPCM should not trigger the same BE twice! BugLink: thesofproject/sof#4611 Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
@ranj063 for dynamic pipeline https://github.com/thesofproject/linux/blob/topic/sof-dev/sound/soc/sof/sof-audio.c#L180, do we need to protect this use_count ? this can result such issue. |
See thesofproject#4611 for more background. Signed-off-by: Marc Herbert <marc.herbert@intel.com>
STOP copying all files from dir: sof/tools/build_tools/topology/topology2/cavs/ to dir: sof/tools/build_tools/topology/ The original plan discussed in the reviews of commit c0bee42 ("topology: prepare for Topology2.0") and commit 308a24a ("topology2: Add build support") and more recently mentioned in thesofproject#4611 was to overwrite in place some v1 topologies with newer v2 topologies in order to "force" users to upgrade without them realizing it. This original plan is now being abandoned. v2 topologies will never overwrite v1 topologies and if ever then certainly not "en masse". Moreover, v2 topologies will be placed in a new /lib/firmware/ subdirectory. So, partial revert the aforementioned commits. More specifically. stop installing v2 topologies into the same directory as v1 topologies. Note there had never been any actual overwrite of any v1 topology yet because there had never been any v2 topology of the same name. At this point in time this gets rid of the following copies: tools/build_tools/topology: abi.conf tools/build_tools/topology: cavs-mixin-mixout-hda-2ch-cnl.conf tools/build_tools/topology: cavs-mixin-mixout-hda-2ch-cnl.tplg tools/build_tools/topology: cavs-mixin-mixout-hda-2ch-tgl.conf tools/build_tools/topology: cavs-mixin-mixout-hda-2ch-tgl.tplg tools/build_tools/topology: cavs-mixin-mixout-hda-4ch-cnl.conf tools/build_tools/topology: cavs-mixin-mixout-hda-4ch-cnl.tplg tools/build_tools/topology: cavs-mixin-mixout-hda-4ch-tgl.conf tools/build_tools/topology: cavs-mixin-mixout-hda-4ch-tgl.tplg tools/build_tools/topology: cavs-mixin-mixout-hda.conf tools/build_tools/topology: cavs-mixin-mixout-hda.tplg tools/build_tools/topology: cavs-passthrough-hdmi.conf tools/build_tools/topology: cavs-passthrough-hdmi.tplg tools/build_tools/topology: cavs-sdw.conf tools/build_tools/topology: cavs-sdw.tplg tools/build_tools/topology: cavs-tgl-nocodec.conf tools/build_tools/topology: cavs-tgl-nocodec.tplg tools/build_tools/topology: nhlt-ace-mtl-nocodec.bin tools/build_tools/topology: nhlt.bin tools/build_tools/topology: nhlt-cavs-tgl-nocodec.bin tools/build_tools/topology: sof-mtl-nocodec.conf tools/build_tools/topology: sof-mtl-nocodec.tplg From an "installer/" and sof-bin release perspective, this removes the following topology that was just added by commit ba26eef ("topology2: cavs-nocodec: Add support for MTL nocodec topology"). Installing v2 topologies into a new /lib/firmware/ subdirectory has not been implemented yet. No other change besides dropping these copies, everything else is the same. Signed-off-by: Marc Herbert <marc.herbert@intel.com>
b02ac91 to
bfc800d
Compare
|
@marc-hb I added your change and bumped the version required to 5.19, turns out we were missing patches that were only sent upstream in this cycle. @lgirdwood this should be good to go if Marc approves. |
marc-hb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CMake logic looks good to me now (but that's not the important bit)
|
The failures in https://sof-ci.01.org/sofpr/PR4611/build88/devicetest/ seem topology-related, so that's worrying. |
|
looks like a git rebase issue, this makes no sense. |
We don't support this topology in production and it's not clear if it's even maintained, move to development. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Use SoundWire convention with deep-buffer using device 31. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
We don't want to release these topologies to world+dog due to dependencies on kernel version, so let's make the deep-buffer optional. The next patch will enable these topologies in the development/ directory. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
…pologies This patch duplicates the configuration at the higher level and adds the "DEEP_BUFFER" definition. The topologies are generated in the kernel_dependent/v5.19 directory and shall only be used by distro/users when their kernel is based on 5.19-rc1 or later. If these topologies are used with an older kernel, the ASoC DPCM state machine issues will result in broken audio. We have no ability to detect a dependency on kernel to load a topology. This is not very elegant and will require changes in two places, but I don't see a better solution with M4. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
For some reason the topologies are in separate blocks, regroup them. No functionality change. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Before we enable the headset deep-buffer solution, we want to extract the NOJACK topologies where the headset deep-buffer would be an oxymoron. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Deep buffer is device 31 by convention Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
We don't want to release these topologies to world+dog due to dependencies on kernel version, so let's make the deep-buffer optional. The next patch will enable these topologies in the development/ directory. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
…t topologies This patch duplicates the configuration at the higher level and adds the "HEADSET_DEEP_BUFFER" definition. This is not very elegant and will require changes in two places, but I don't see a better solution with M4. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
bfc800d to
05109c7
Compare
Corrected that blatant mistake - collision between two PRs. Tested on UpExtreme w/ and w/o deep-buffer |
|
https://sof-ci.01.org/sofpr/PR4611/build90/devicetest/ has 2 instances of #5352, one rtcwake timeout, one BYT crash and one device not available. Routine. |
See #4611 for more background. Signed-off-by: Marc Herbert <marc.herbert@intel.com>
STOP copying all files from dir: sof/tools/build_tools/topology/topology2/cavs/ to dir: sof/tools/build_tools/topology/ The original plan discussed in the reviews of commit c0bee42 ("topology: prepare for Topology2.0") and commit 308a24a ("topology2: Add build support") and more recently mentioned in #4611 was to overwrite in place some v1 topologies with newer v2 topologies in order to "force" users to upgrade without them realizing it. This original plan is now being abandoned. v2 topologies will never overwrite v1 topologies and if ever then certainly not "en masse". Moreover, v2 topologies will be placed in a new /lib/firmware/ subdirectory. So, partial revert the aforementioned commits. More specifically. stop installing v2 topologies into the same directory as v1 topologies. Note there had never been any actual overwrite of any v1 topology yet because there had never been any v2 topology of the same name. At this point in time this gets rid of the following copies: tools/build_tools/topology: abi.conf tools/build_tools/topology: cavs-mixin-mixout-hda-2ch-cnl.conf tools/build_tools/topology: cavs-mixin-mixout-hda-2ch-cnl.tplg tools/build_tools/topology: cavs-mixin-mixout-hda-2ch-tgl.conf tools/build_tools/topology: cavs-mixin-mixout-hda-2ch-tgl.tplg tools/build_tools/topology: cavs-mixin-mixout-hda-4ch-cnl.conf tools/build_tools/topology: cavs-mixin-mixout-hda-4ch-cnl.tplg tools/build_tools/topology: cavs-mixin-mixout-hda-4ch-tgl.conf tools/build_tools/topology: cavs-mixin-mixout-hda-4ch-tgl.tplg tools/build_tools/topology: cavs-mixin-mixout-hda.conf tools/build_tools/topology: cavs-mixin-mixout-hda.tplg tools/build_tools/topology: cavs-passthrough-hdmi.conf tools/build_tools/topology: cavs-passthrough-hdmi.tplg tools/build_tools/topology: cavs-sdw.conf tools/build_tools/topology: cavs-sdw.tplg tools/build_tools/topology: cavs-tgl-nocodec.conf tools/build_tools/topology: cavs-tgl-nocodec.tplg tools/build_tools/topology: nhlt-ace-mtl-nocodec.bin tools/build_tools/topology: nhlt.bin tools/build_tools/topology: nhlt-cavs-tgl-nocodec.bin tools/build_tools/topology: sof-mtl-nocodec.conf tools/build_tools/topology: sof-mtl-nocodec.tplg From an "installer/" and sof-bin release perspective, this removes the following topology that was just added by commit ba26eef ("topology2: cavs-nocodec: Add support for MTL nocodec topology"). Installing v2 topologies into a new /lib/firmware/ subdirectory has not been implemented yet. No other change besides dropping these copies, everything else is the same. Signed-off-by: Marc Herbert <marc.herbert@intel.com>
This PR splits the headphone pipeline in two parts (host and dai-mixer) and adds a deep-buffer path.
Edit: this works fine now that I used pipeline indices larger for the FEs than the BE. Thanks @ranj063 for the hint. The topology now looks like this: