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

ranj063 · 2019-05-02T04:16:19Z

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 kernel change for this PR is thesofproject/linux#895

xiulipan · 2019-05-02T06:10:19Z

@ranj063
manual trigger a test with thesofproject/linux#895
test logs on https://sof-ci.01.org/sofpr/PR1354/build1833/

ranj063 · 2019-05-02T06:11:54Z

Thanks @xiulipan

lgirdwood · 2019-05-02T07:31:31Z

src/include/uapi/abi.h

@@ -51,8 +51,8 @@
 #define __INCLUDE_UAPI_ABI_H__

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


I can't see where the ABI is changed ? Is there a patch missing from the PR ?

@lgirdwood there are no changes to the IPC structures in this PR but I changed the ABI because the way the link DMA channels are set is different. So kernel that sets the link DMA channel dynamically would expect the FW to do the same and if the FW is older where the link DMA channels are set statically, all HDA usecases would fail. So they are incompatible. I didnt know how else to call out this mismatch.

ok, but I also dont see any changes to the IPC programming flow (including in the kernel PR), this looks more like a dependency on register programming flow depending on how FW allocates HDA DMA channels ? If so, it's probably better to advertise this (using boot extended metadata) so the driver can then decide which programming version to use. This also means that 1:N could still be supported in FW/driver (and re-used when HW supports it).

@lgirdwood I could advertise this with boot extended metadata but I don't understand how this would help the 1:N case. This scenario would be broken even with the static allocation scheme that we have today.

@ranj063 ok, so you mean that "change will not work with 1:N FE <-> BE connections" is even true today without this change.

@lgirdwood yes thats correct

@lgirdwood the boot ext metadata would handle the backward compatibility between a new kernel and an older FW. But do you have any suggestions for newer FW and older kernel?
We send the link_dma_ch as part of the DAI config IPC. In the case of dynamic allocation, this would still be there but just that we ignore the link_dma_ch field which will always be 0 from the driver.

@lgirdwood answering my question myself. I think I've found a way to do this now. I will update the PR soon

ranj063 · 2019-05-02T22:49:18Z

@lgirdwood @plbossart I've updated both the kernel and FW PR's with minor ABI updates with the addition of reserved member in the hda params IPC structure. Let me know if it is too much and I'll trim it down.

@lgirdwood After a discussion with Pierre, we decided that the code in the driver to assign stream tags for link DMA statically is not suitable to be maintained and therefore it should be removed. This would mean that a new kernel version will be incompatible with an older firmware in terms of HDA functionality.
But we can justify this with the argument that this is broken nevertheless because of the recent programming sequence changes that were introduced in SOF.

lgirdwood

@ranj063 approved but you may be ABI MINOR 8, depends on @bardliao PR for trace timestamp.

ranj063 · 2019-05-03T16:41:33Z

@ranj063 approved but you may be ABI MINOR 8, depends on @bardliao PR for trace timestamp.

Thanks @lgirdwood . Fixed ABI minor to 8

RanderWang · 2019-05-05T08:36:15Z

src/audio/dai.c

+	/* set dma channel */
+	if (dd->chan == DMA_CHAN_INVALID)
+		/* get dma channel at first config only */
+		dd->chan = dma_channel_get(dd->dma, dev->params.stream_tag);


Actually dev->params.stream_tag is for host dma, not link dma. And host dma channel is not always equal to link dma channel.

No, this doesn't work, we should add a flag into params for link_stream_tag.

@RanderWang @keyonjie for us link DMA channel is always equal to host DMA channel, isnt it?
Thats the whole argument about why we're having all kinds of issues now. Care to explain your thoughts about a separate link_stream_tag? When would it be different from the host DMA tag? And if it is, how do you make sure that things dont break when you couple them foer setting the DMA format?

And before you call out "this doesnt work", I'd appreciate if you could try it out and show me the "not working" usecase. Thanks!

And before you call out "this doesnt work", I'd appreciate if you could try it out and show me the "not working" usecase. Thanks!

Sorry, didn't read the whole commit message, now I have more time to write a longer comment, I mean we need solution for M:N in a one shot, and it's good to leave link DMA channel indices allocation/decision from driver side(here your solution is assuming fixing them to host indices and no any flexibility for future).

Can you consider suggestion like below to solve M:N mapping issues:

++struct sof_hda_stream_tag { ++ uint32_t dai_index; /**< index of this type dai */ ++ uint32_t link_stream_tag; ++}; struct sof_ipc_stream_params { ... uint32_t rate; -- uint16_t stream_tag; ++ uint16_t host_stream_tag; uint16_t channels; ... ++ uint16_t hda_stream_cnt; ++ struct sof_hda_stream_tag link_stream_tag[0]; } __attribute__((packed));

w.r.t. channel indices allocation in driver side, we allocate them in each PCM hw_params, taking 1:N mapping as example, that is, allocate i for host_stream_tag, and i, j, k, ... for the N link_stream_tag array, the couple(to i) --> decouple for stream format setting here is safe here.

OTOH, for M:1 mapping(we have corresponding M FE PCMs in driver side), the 1st (hw_params() running) of M PCM will be allocated to i for host_stream_tag and link_stream_tag, the hw_params() for remaining PCMs only allocate j, k, ... for host_stream_tag in driver side, and in FW side, the HDA dai will only accept the 1st hw_params and ignore the remaining ones. So here it is also safe.

Better to do one more thing in driver side that I talked to Rander, make sure we will couple the channel only at the case that both host channel and link channel are in idle status, this can improve our robustness for concurrent PCMs cases, and that is actually recommended from hardware docs.

@RanderWang are you sure about this? Why would the tag be different? The link DMA ch is already in use and should not be changed when another FE is activated. In this case, the mixer would automatically come into play and it would mix the 2 streams. Why would you change the BE DAI channel?

And also for some stream with only BE like tone generator, there is no host stream, right ?

Agreed, but you would still need to hw_params in this case when you can pass in the stream_tag.

@keyonjie appreciate your thoughts but M:N is quite a complicated case and it cannot be fixed overnight. We do not have the ability in the FW to handle multiple BE's at this point. It needs a larger discussion. I cannot implement a half-baked solution with the hopes that someday it might be useful to fix something. With the current status of HDA platforms, we should resign to the fact that M:N is not supported and will be added later.

Hi @ranj063 sorry but have to request more than only 1:1 mapping works(we already have non 1:1 mixer case running), at least we should provide the extendable possibility for future, otherwise, we have to go back and forth about it, that consume our effort but no actually improvement from user experience point of view.

@keyonjie @RanderWang @ranj063 please make sure the programming flow and ABI is not restricted to 1:1 mappings as other HDA DMA HW may support 1:N or N:1.

@lgirdwood i now have some insight into how to make the PR a bit more generic for the M:N case. I will have the updates by tomorrow.

keyonjie

please make sure we allocate link stream tag in driver side at hw_params(), and send this stream tag to FW via IPC struct at runtime.

ranj063 · 2019-05-05T19:11:38Z

@keqiaozhang can I please request a round of validation tests (including some concurrent usecases) with this PR and corresponding linux PR thesofproject/linux#895 especially on the CML platform?

keqiaozhang · 2019-05-06T07:58:38Z

@keqiaozhang can I please request a round of validation tests (including some concurrent usecases) with this PR and corresponding linux PR thesofproject/linux#895 especially on the CML platform?

I verified above PRs on WHL-I2S and GLK, no obvious regressions, but I did run into the IPC times out issue once when doing the volume change stress test( unable to reproduce it again). Besides, these PRs can fix issue thesofproject/linux#854.

@stevyan, please help to check these PRs on other platforms.

Jiangxinx · 2019-05-07T08:20:55Z

@ranj063 Hi ranjani, I can't add your patch manually, could you help to check it?

$ git am 1354.patch
Applying: dai: assign link DMA channel at run-time
error: patch failed: src/include/uapi/abi.h:52
error: src/include/uapi/abi.h: patch does not apply
Patch failed at 0001 dai: assign link DMA channel at run-time
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

new changes to the PR

ranj063 · 2019-05-10T07:59:26Z

@plbossart @keyonjie @RanderWang updaed the PR now. I've updated the description with the new method.

The second commit is quite large but I couldnt separate any parts of it without breaking compilation. Appreciate your comments!

ranj063 · 2019-05-10T08:05:35Z

@lgirdwood updated PR now. Instead of setting the link DMA params during dai_params, the driver now sends multiple DAI_CONFIG ipc's to set the link DMA channel. This would scale better for the N:M usecase.

lyakh · 2019-05-21T08:59:09Z

src/include/uapi/ipc/dai-intel.h

@@ -112,6 +112,7 @@ struct sof_ipc_dai_ssp_params {
 struct sof_ipc_dai_hda_params {
 	struct sof_ipc_hdr hdr;
 	uint32_t link_dma_ch;
+	uint32_t reserved[4];


Not sure I understand - isn't this unrelated to the fix? Is this the reason to change the ABI minor number? If the answer to both questions is "yes," I think it's better to remove them from this patch

@lyakh this is a pretend ABI update to track this change. With this update to link DMA assignment, the kernel/FW are officially backwards incompatible

For incompatibility we have to increment the major version, don't we? Still you don't need to add those reserved fields, if they're unrelated.

@lyakh that was my original change but this is what has been agreed with maintainers. A major ABI change usually requires big changes to the IPC structs. I was told functional incompatibility cannot be classified a major ABI change. So, we advertise full HDA support only 3.8 onwards.

@ranj063 this and the ABI bump are not needed.

lgirdwood · 2019-05-22T20:16:30Z

@mmaka1 @keyonjie @lyakh any blockers or can this be merged ?

ranj063 · 2019-05-22T20:56:19Z

@lgirdwood could we please hold off merging this one until stress tests are complete and both the kernel/fw patches can go in together or else all CI tests will fail.

lgirdwood · 2019-05-24T11:02:09Z

@ranj063 ok, just ping me when we are good.

jajanusz · 2019-05-27T09:27:14Z

@mmaka1 @keyonjie please re-review

lgirdwood · 2019-05-27T18:46:32Z

@plbossart we are good to go here, with the exception of the ABI major bump. Since we are NOT changing ABI, and only changing HDA programming flow, we could use version number (i.e. v1.3 and above) to use new HDA programming flow and v1.2 and below for old flow ?

lgirdwood · 2019-05-27T19:03:31Z

@ranj063 could you revert the ABI update part if we agree on version number to change HDA programming flow.

xiulipan · 2019-05-28T03:57:34Z

Manually run with latest thesofproject/linux#895 binary

plbossart · 2019-05-28T18:43:10Z

@ranj063 could you revert the ABI update part if we agree on version number to change HDA programming flow.

I don't want to even maintain the old flow. It's just broken so we only need to claim HDaudio is supported starting with release 1.3

ranj063 · 2019-05-28T18:46:51Z

@ranj063 could you revert the ABI update part if we agree on version number to change HDA programming flow.

@plbossart @lgirdwood sure. I will update both the kernel/FW PR's to remove the ABI bump by EOD

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. This change means that the DAI_CONFIG IPC will be sent multiple times during link hw_params and link hw_free ioctl. The DAI config parameters will remain the same except for the link DMA channel that will be assigned at run-time. A value of DMA_CHAN_INVALID from the driver during hw_free indicates a request to free the current link DMA channel in use. The current channel in use is freed before assiging the new channel requested in the DAI_CONFIG IPC. Signed-off-by: Ranjani Sridharan <[email protected]>

ranj063 · 2019-05-28T21:51:51Z

@lgirdwood @plbossart updated to remove ABI change

plbossart · 2019-05-29T02:20:55Z

matching PR merged on kernel side.

jajanusz · 2019-05-29T09:32:00Z

@zrombel Please check CI

Stale

lgirdwood · 2019-05-29T10:03:34Z

@jajanusz thanks, we can merge now as soon as CI is done.

ranj063 requested review from lbetlej, lgirdwood, mmaka1, plbossart and a team as code owners May 2, 2019 04:16

ranj063 added the ABI ABI change involved label May 2, 2019

ranj063 requested review from RanderWang and keyonjie May 2, 2019 04:17

ranj063 mentioned this pull request May 2, 2019

Fix allocation of link dma channels thesofproject/linux#895

Merged

lgirdwood requested changes May 2, 2019

View reviewed changes

ranj063 force-pushed the fix/link-dma-allocation branch from b8bf039 to cceae5d Compare May 2, 2019 22:44

lgirdwood previously approved these changes May 3, 2019

View reviewed changes

ranj063 force-pushed the fix/link-dma-allocation branch from cceae5d to cc2c000 Compare May 3, 2019 16:39

RanderWang reviewed May 5, 2019

View reviewed changes

keyonjie requested a review from RanderWang May 5, 2019 08:46

keyonjie previously requested changes May 5, 2019

View reviewed changes

ranj063 force-pushed the fix/link-dma-allocation branch 2 times, most recently from 12051cb to f5309a0 Compare May 10, 2019 07:40

ranj063 requested a review from lgirdwood May 10, 2019 07:47

xiulipan mentioned this pull request May 18, 2019

ASoC: SOF: ipc: fix a race, leading to IPC timeouts thesofproject/linux#943

Merged

lyakh reviewed May 21, 2019

View reviewed changes

lgirdwood approved these changes May 22, 2019

View reviewed changes

jajanusz requested review from mmaka1 and keyonjie May 27, 2019 09:25

jajanusz approved these changes May 27, 2019

View reviewed changes

mmaka1 approved these changes May 27, 2019

View reviewed changes

lgirdwood added this to the 1.3 milestone May 27, 2019

wenqingfu mentioned this pull request May 28, 2019

[BUG] pipeline will interact with each other in HDA mode thesofproject/linux#854

Closed

ranj063 force-pushed the fix/link-dma-allocation branch from 4f239de to 28a6c96 Compare May 28, 2019 21:35

plbossart approved these changes May 28, 2019

View reviewed changes

keyonjie mentioned this pull request May 29, 2019

ASoC: SOF: topology: refine multi-core core status management thesofproject/linux#888

Closed

xiulipan mentioned this pull request May 29, 2019

Enable WAKEEN feature in sof hda driver thesofproject/linux#983

Merged

jajanusz merged commit 92fc706 into thesofproject:master May 29, 2019

xiulipan mentioned this pull request May 30, 2019

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

Merged

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

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

Conversation

ranj063 commented May 2, 2019 • edited Loading

xiulipan commented May 2, 2019

ranj063 commented May 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ranj063 commented May 2, 2019 • edited Loading

lgirdwood left a comment

Choose a reason for hiding this comment

ranj063 commented May 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keyonjie May 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keyonjie May 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keyonjie left a comment

Choose a reason for hiding this comment

ranj063 commented May 5, 2019

keqiaozhang commented May 6, 2019

Jiangxinx commented May 7, 2019

ranj063 commented May 10, 2019

ranj063 commented May 10, 2019

lyakh May 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgirdwood commented May 22, 2019

ranj063 commented May 22, 2019

lgirdwood commented May 24, 2019

jajanusz commented May 27, 2019

lgirdwood commented May 27, 2019

lgirdwood commented May 27, 2019

xiulipan commented May 28, 2019

plbossart commented May 28, 2019

ranj063 commented May 28, 2019

ranj063 commented May 28, 2019

plbossart commented May 29, 2019

jajanusz commented May 29, 2019

lgirdwood commented May 29, 2019

ranj063 commented May 2, 2019 •

edited

Loading

ranj063 commented May 2, 2019 •

edited

Loading

keyonjie May 6, 2019 •

edited

Loading

keyonjie May 6, 2019 •

edited

Loading

lyakh May 21, 2019 •

edited

Loading