Fix allocation of link dma channels #895

ranj063 · 2019-05-02T04:15:34Z

The recommended HDA HW programming sequence for setting the DMA format requires that the link DMA and host DMA channels be coupled before setting the format. This change means that host DMA or link DMA channels be reserved even if only one is used.

Statically assigned link DMA channels would mean that all the corresponding host DMA channels will need to be reserved, leaving only a few channels available at run-time. So, the suggestion here is to switch to dynamically assigning both host DMA channels and link DMA channels are run-time.

The host DMA channel is assigned when the pcm is opened as before. While choosing the link DMA channel, if the host DMA channel corresponding to the link DMA channel is already taken, the proposed method checks to make sure that the BE is connected to the FE that has been assigned this host DMA channel. For hostless streams, the host DMA channel is marked as opened to ensure it isn't assigned to another stream. After assigning the link DMA channel, a DAI_CONFIG IPC is sent to the FW to set the chosen link DMA channel.

This change should support N:M FE <->BE configurations as well.

The corresponding FW change for this PR is thesofproject/sof#1354

Tested on UP2 so far. Further testing in progress.

wenqingfu · 2019-05-02T16:34:24Z

link to issue #874
replaced PR #882

plbossart

Thanks @ranj063. This PR has the main benefit of yanking all the code I didn't like and the ASoC API abuse, but the parts on the locked/opened flags are a bit unclear, and the ABI change does not seem necessary.

sound/soc/sof/intel/hda-dai.c

plbossart · 2019-05-02T18:20:00Z

sound/soc/sof/intel/hda-dai.c

 	case SNDRV_PCM_TRIGGER_SUSPEND:
 	case SNDRV_PCM_TRIGGER_STOP:
+		if (stream.opened)
+			stream.opened = 0;


We directly handle internals of the hdac library but those assignments are only taken into account when there is an explicit call to hdac_ext_xyz(), so it's quite hard to follow the state machine here.

@plbossart understood. I probably need to add some more comments to why and how this is done here. Will address in v2

sound/soc/sof/intel/hda-stream.c

sound/soc/sof/intel/hda-trace.c

ranj063 · 2019-05-02T18:50:06Z

Thanks @ranj063. This PR has the main benefit of yanking all the code I didn't like and the ASoC API abuse, but the parts on the locked/opened flags are a bit unclear, and the ABI change does not seem necessary.

@plbossart I changed the ABI because if the kernel allocates link DMA channels dynamically, it is not compatible with the older version of the FW which require statically allocated channels and vice-versa.

But @lgirdwood commented that he'd like to address the compatibility by having the FW advertise which allocation scheme it uses and the driver can switch to that. This means that we wont be able to remove all the code which abuses the get_channel_map() API. What would be your take on it?

plbossart · 2019-05-02T20:27:06Z

Thanks @ranj063. This PR has the main benefit of yanking all the code I didn't like and the ASoC API abuse, but the parts on the locked/opened flags are a bit unclear, and the ABI change does not seem necessary.

@plbossart I changed the ABI because if the kernel allocates link DMA channels dynamically, it is not compatible with the older version of the FW which require statically allocated channels and vice-versa.

But @lgirdwood commented that he'd like to address the compatibility by having the FW advertise which allocation scheme it uses and the driver can switch to that. This means that we wont be able to remove all the code which abuses the get_channel_map() API. What would be your take on it?

Removing that code solves a long standing issue of abusing the ASoC APIs for HDaudio/iDISP specific stuff, keeping it is not really a goal..

I would err on the side of claiming HDaudio/iDISP never fully worked, and the recommended programming sequence was only communicated recently to the SOF team, and that this is not really a compatibility break but a redesign to deal with how the hardware actually works. We can require all users of Hdaudio/iDISP to update the firmware at this point, and in practice they will update based on new releases already planned. As long as 1.3 includes that release we don't really need to worry.

I hate to break compatibility but here we are enabling a feature.

As discussed I would put a pretend minor ABI change to report that the firmware needs to be upgraded to work around this known incompatibility when HDaudio is used.

ranj063 · 2019-05-02T23:01:24Z

@plbossart PR updated now.

lgirdwood

I've mentioned the ABI as this will be the second PR (after @bardliao timestamp) where we can align the FW/kernel after the recent mis-alignment. See https://github.com/orgs/thesofproject/projects/2 for details on MINOR features (and the delta)

lgirdwood · 2019-05-03T14:05:07Z

include/uapi/sound/sof/abi.h

 /* SOF ABI version major, minor and patch numbers */
 #define SOF_ABI_MAJOR 3
-#define SOF_ABI_MINOR 5
+#define SOF_ABI_MINOR 6


@lgirdwood fixed now

RanderWang

So in this commit, you need to send ipc message to FW ? I don't find it.

RanderWang · 2019-05-05T08:17:19Z

Correct two cases:

The sequence of PCM ops upon starting a PCM stream in SOF is as
follows (Showing only the hw_params and trigger ops):

Host hw_params -> Link hw_params -> Host START -> Link START

It should be Link hw_params -> Host hw_params -> Host START -> Link START

For HDA streams, the host DMA channel would be assigned during
host hw_params and the corresponding link DMA channel would be
assigned in link hw_params.

Here, host DMA channel would be assigned in host pcm open (hda_dsp_pcm_open).

RanderWang · 2019-05-05T08:25:21Z

I am confused: The flags: stream->opened and locked are set in start or stop cases, but the dma channel allocation is done at pcm_open for host and hw_param for BE. How does it work? The host dma and link dma have been allocated before trigger_start, right ?

RanderWang · 2019-05-05T08:54:28Z

include/sound/sof/dai-intel.h

 struct sof_ipc_dai_hda_params {
 	struct sof_ipc_hdr hdr;
 	uint32_t link_dma_ch;
+	uint32_t reserved[8];


It should be added in sof_ipc_pcm_params for you assign link dma channel in hw_param.
The sof_ipc_dai_hda_params is for dai_link loading

@RanderWang I suppose i didnt do that good of a job explaining things. Because of our requirement to couple before setting the HDA format, the host DMA stream tag and the link DMA stream tag will always need to be the same. So, I only send one stream tag to the DSP during host pcm hw_params. Because of the way, the link DMA tags are "reserved", the link dma channel assigned during link hw_params will always be the same as host dma stream tag.

Please give this PR a try and test all kinds of scenarios you think could be broken. That's really what I am doing too. @fredoh9 and myself have been testing a few kinds of concurrent use cases and havent been able to see any issues with our hda generic topology so far.

yes, I just said in FW patch. two cases:
(1) for two FEs share one BE, the param would be called two times and one of them the tag should be different.
(2) For some streams with only BE like tone generator, there is no host stream

@RanderWang when the hw_params will be the called the second time, we will ignore the channel as it will not be INVALID at that point.

yes, I get it.

RanderWang · 2019-05-06T04:57:34Z

And the host stream tag should be different in multi-thread cases.
For example:
(1) pcm1 is opened, and gets host dma channel 1.
(2) pcm2 is opened and gets host dma channel 2, the hw_params is called for BE of pcm2, so link dma channel 1 is allocated
(3) the hw_params is called for BE of pcm1, so link dma channel 2 is allocated.

For this sequence, the tag should be different. The key idea is: the open-hw_param-start is not a atomic operation

ranj063 · 2019-05-06T05:34:44Z

@RanderWang I'm going to have to challenge you to prove my theory wrong here. Take a look at

linux/sound/soc/soc-pcm.c

Line 933 in 1071743

mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);

pcm_hw_params() is locked before performing fe hw_params and the be hw_params. So what you're describing in the multi-threaded case isnt possible.

But like I said, this is my theory. Please do the needful to prove me wrong.

RanderWang · 2019-05-06T05:42:40Z

this lock only protects BE hw_param, not open function for FE, right ? FE allocates host dma in hda_dsp_pcm_open, which is not protected by this lock

RanderWang · 2019-05-06T05:44:36Z

Take it easy. What I asked is discussed with Pierre last year. I spent a least one month.

ranj063 · 2019-05-06T05:48:58Z

Take it easy. What I asked is discussed with Pierre last year. I spent a least one month.

@RanderWang we're talking about discussions that are stale. What you discussed with Pierre last year was before you implemented the programming sequence to couple/decouple while setting the DMA format. Lets not go down a rat hole. Lets stick with the new information and try to make progress.

This change is complicated and is likely not the final solution. But our current solution is severely compromised for concurrent use cases. So lets try to take one step at a time and try to fix the 1:1 FE <-> BE topologies.

ranj063 · 2019-05-06T05:50:16Z

this lock only protects BE hw_param, not open function for FE, right ? FE allocates host dma in hda_dsp_pcm_open, which is not protected by this lock

really? did you take a look at this?

linux/sound/soc/soc-pcm.c

Line 501 in 1071743

mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);

RanderWang · 2019-05-06T05:58:37Z

you don't got my idea. Each open and hw_param function is protected by lock, but the lock is released after open and acquired when hw_param is invoked ? So between open and hw_param, there is no lock.

RanderWang · 2019-05-06T06:03:42Z

And you could refine our driver, allocate host dma channel in hw_param for FE, not in open function.
And for 1:1 case, you don't need to set open or lock in trigger function. But you still need to do it for dma trace.

stevyan · 2019-05-06T09:12:51Z

@ranj063 This PR would cause an debugfs copying issue, when I tried to copy /sys/kernel/debug/sof/dsp to other directory, it couldn't be interrupted by any key, even though 'ctrl + c', and the system also hung. the same operation does't hung system based on daily build version. please help to check it.

Reproduce Steps
sudo su -c 'cp -rf /sys/kernel/debug/sof/dsp /tmp/'

ranj063 · 2019-05-07T04:11:56Z

@ranj063 This PR would cause an debugfs copying issue, when I tried to copy /sys/kernel/debug/sof/dsp to other directory, it couldn't be interrupted by any key, even though 'ctrl + c', and the system also hung. the same operation does't hung system based on daily build version. please help to check it.

Reproduce Steps
sudo su -c 'cp -rf /sys/kernel/debug/sof/dsp /tmp/'

@stevyan which platform did this happen on? CML or all of them?

ranj063 · 2019-05-07T04:15:31Z

And you could refine our driver, allocate host dma channel in hw_param for FE, not in open function.
And for 1:1 case, you don't need to set open or lock in trigger function. But you still need to do it for dma trace.

@RanderWang yes, thats what I am thinking too. Let me change the PR to do host dma stream tag assignment in hw_params.
But I dont understand why you say that we do not need to set open/locked in trigger. Imagine the case where we start I2S playback with Host DMA 0. Link DMA 0 will not be assigned. Then we start HDMI playback. This will assign host DMA 1 and link DMA 0 for the hDMI playback stream. If we want to avoid this, we have to lock link DMA 0 when the i2s stream was started, isnt it?

RanderWang · 2019-05-07T05:16:36Z

@RanderWang yes, thats what I am thinking too. Let me change the PR to do host dma stream tag assignment in hw_params.
But I dont understand why you say that we do not need to set open/locked in trigger. Imagine the case where we start I2S playback with Host DMA 0. Link DMA 0 will not be assigned. Then we start HDMI playback. This will assign host DMA 1 and link DMA 0 for the hDMI playback stream. If we want to avoid this, we have to lock link DMA 0 when the i2s stream was started, isnt it?

yes, I agree with you at this case. One concern is that: do you plan to only support N:1 (N >= 1) for this PR ? As I know, you could archive this without changing ABI or IPC, and your change is ready in FW.
To support 1:N, keyon has offered a solution which needs to at least change ABI, IPC and FW. I don't require a complete solution in one PR and we may do it in another PR to make it easy to review and test.

Another issue is that: stream should be protected by lock, just like hda_dsp_stream_get. I strongly recommend you improve hda_dsp_stream_get function, and move stream->locked setting to this function for:
(1) it is natural to do it in stream management function and dma trace could also be processed here.

(2) you don't need to protect stream with lock, this function does it.

(3) you could allocate host dma & link dma and reserve corresponding dma channel in hw_params, which is protected by ASoC lock, so there is no multi-thread issue. If the allocation is done in hw_params but reservation of dma is done in trigger function, the reserved channel maybe allocated by other pcm in other thread before this thread invokes trigger function. (correct me if I misunderstand)

(4) it is easy to support 1:N case with this change, because all the change should be in hda_dsp_stream_get function.

one basic idea: BE hw_params would be called before FE hw_params by ASoC.

plbossart

Thanks @ranj063 minor newlines issues but otherwise we are good to go. Can you update and I'll merge tomorrow after one CI round?

sound/soc/sof/intel/hda-dai.c

plbossart · 2019-05-28T22:27:32Z

sound/soc/sof/topology.c

 #include "ops.h"

 #define COMP_ID_UNASSIGNED		0xffffffff
+


different patch

@plbossat did you mean to separate out the topology parts into a separate patch?

no, move the newline somewhere else

@plbossart ahh ok. I've removed it. I dont know how it sneaked in.

sound/soc/sof/intel/hda-dai.c

The recommended HDA HW programming sequence for setting the DMA format requires that the link DMA and host DMA channels be coupled before setting the format. This change means that host DMA or link DMA channels be reserved even if only one is used. Statically assigned link DMA channels would mean that all the corresponding host DMA channels will need to be reserved, leaving only a few channels available at run-time. So, the suggestion here is to switch to dynamically assigning both host DMA channels and link DMA channels are run-time. The host DMA channel is assigned when the pcm is opened as before. While choosing the link DMA channel, if the host DMA channel corresponding to the link DMA channel is already taken, the proposed method checks to make sure that the BE is connected to the FE that has been assigned this host DMA channel. Once the link DMA channel is assigned, an IPC is sent to the DSP to set the link DMA channel. The link DMA channel is freed during hw_free() and also in the SUSPEND trigger callback. It will be re-assigned when hw_params are set upon resume. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Due to the HW programming sequence requirement that the host and link DMA channels need to be coupled/decoupled during pcm hw_params, the host DMA channel corresponding to the link DMA channel in use for hostless streams needs to be reserved. This is achieved by adding a host_reserved flag in the sof_intel_hda_stream structure which is checked when assigning a host DMA channel. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

…pend Paused streams do not get suspended when the system enters S3. So, clear and release link DMA channel for such streams in the hda_dsp_set_hw_params_upon_resume() callback. Also, invalidate the link DMA channel in the DAI config before restoring the dai config upon resume. Also, modify the signature for the set_hw_params_upon_resume() op to return an int. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Host and link DMA are decoupled during FE hw_params. So, they must be coupled in hw_free if the link DMA channel is idle. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

plbossart

Thanks @ranj063 looks good to me. Need a second approver to merge!

keyonjie

Good to go for me, it is fundamental change for link dma support, let's go with incremental fixes if issues exposed on it.

plbossart · 2019-05-29T02:09:26Z

Alright let's merge and test. Thanks @ranj063

plbossart · 2019-05-29T02:15:24Z

@xiulipan @wenqingfu the CI is completely in the weeds. The build fail is in the make bindebpkg phase.

plbossart · 2019-05-29T02:19:48Z

The compilation works fine on my machine so I will go ahead and merge.

xiulipan · 2019-05-29T09:06:54Z

@plbossart @ranj063
The build fail is because there is a warnging and we treat warning as error in Travis.

sound/soc/sof/intel/hda-dsp.c: In function ‘hda_dsp_set_hw_params_upon_resume’:
sound/soc/sof/intel/hda-dsp.c:469:6: error: variable ‘stream_tag’ set but not used [-Werror=unused-but-set-variable]
  int stream_tag;
      ^~~~~~~~~~
cc1: all warnings being treated as errors

plbossart · 2019-05-29T21:25:53Z

On 5/29/19 4:06 AM, xiulipan wrote: @plbossart <https://github.com/plbossart> @ranj063 <https://github.com/ranj063> The build fail is because there is a warnging and we treat warning as error in Travis. |sound/soc/sof/intel/hda-dsp.c: In function ‘hda_dsp_set_hw_params_upon_resume’: sound/soc/sof/intel/hda-dsp.c:469:6: error: variable ‘stream_tag’ set but not used [-Werror=unused-but-set-variable] int stream_tag; ^~~~~~~~~~ cc1: all warnings being treated as errors |

Ah yes. With CI completely gone yesterday we didn't see this. Will send a PR to quickly fix this.

ranj063 added the ABI involves ABI changes, need extra attention label May 2, 2019

ranj063 requested review from a team, RanderWang and keyonjie May 2, 2019 04:16

ranj063 mentioned this pull request May 2, 2019

dai: assign link DMA channel at run-time thesofproject/sof#1354

Merged

plbossart reviewed May 2, 2019

View reviewed changes

ranj063 force-pushed the fix/link-dma-allocation branch from 7fad818 to 4abf19c Compare May 2, 2019 22:45

lgirdwood requested changes May 3, 2019

View reviewed changes

ranj063 force-pushed the fix/link-dma-allocation branch from 4abf19c to 56d6814 Compare May 3, 2019 16:40

RanderWang reviewed May 5, 2019

View reviewed changes

wenqingfu mentioned this pull request May 6, 2019

[BUG][CML] HDA Analog and HDA Digital pipeline will interact with each other #874

Closed

plbossart requested changes May 28, 2019

View reviewed changes

ranj063 added 4 commits May 28, 2019 15:38

ASoC: SOF: hda: couple host and link DMA during FE hw_free

c1574ab

Host and link DMA are decoupled during FE hw_params. So, they must be coupled in hw_free if the link DMA channel is idle. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

ranj063 force-pushed the fix/link-dma-allocation branch from b249c76 to c1574ab Compare May 28, 2019 22:45

plbossart approved these changes May 29, 2019

View reviewed changes

plbossart requested review from RanderWang, keyonjie, lgirdwood and lyakh and removed request for keyonjie, lgirdwood and lyakh May 29, 2019 00:57

keyonjie approved these changes May 29, 2019

View reviewed changes

plbossart merged commit c6cc9ca into thesofproject:topic/sof-dev May 29, 2019

ranj063 deleted the fix/link-dma-allocation branch May 29, 2019 05:48

kv2019i mentioned this pull request May 29, 2019

[Bug][CML] Pipeline will interact with each other with 3 aplay thesofproject/sof#1413

Closed

xiulipan mentioned this pull request May 29, 2019

Enable WAKEEN feature in sof hda driver #983

Merged

xiulipan mentioned this pull request May 30, 2019

ASoC: SOF: Intel: hda-dsp: fix compilation warning #989

Merged

Fix allocation of link dma channels #895

Fix allocation of link dma channels #895

Uh oh!

Conversation

ranj063 commented May 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wenqingfu commented May 2, 2019

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ranj063 commented May 2, 2019

Uh oh!

plbossart commented May 2, 2019

Uh oh!

ranj063 commented May 2, 2019

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RanderWang left a comment

Choose a reason for hiding this comment

Uh oh!

RanderWang commented May 5, 2019

Uh oh!

RanderWang commented May 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RanderWang commented May 6, 2019

Uh oh!

ranj063 commented May 6, 2019

Uh oh!

RanderWang commented May 6, 2019

Uh oh!

RanderWang commented May 6, 2019

Uh oh!

ranj063 commented May 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ranj063 commented May 6, 2019

Uh oh!

RanderWang commented May 6, 2019

Uh oh!

RanderWang commented May 6, 2019

Uh oh!

stevyan commented May 6, 2019

Uh oh!

ranj063 commented May 7, 2019

Uh oh!

ranj063 commented May 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RanderWang commented May 7, 2019

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

ranj063 commented May 2, 2019 •

edited

Loading

ranj063 commented May 6, 2019 •

edited

Loading

ranj063 commented May 7, 2019 •

edited

Loading