-
Notifications
You must be signed in to change notification settings - Fork 140
[RFC] ASoC: SOF: set link dma channel in hda config #291
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
[RFC] ASoC: SOF: set link dma channel in hda config #291
Conversation
|
In FW, there are HDA playback dai and capture dai, because they use different DMA engine. |
0c01b56 to
85729c3
Compare
plbossart
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.
I am not sure the level of indirection and use of a device-level dai_config makes sense. This is really specific to HDaudio so could be exposed with a more private helper that is only invoked when SOF_HDA (and possibly HDAC_HDA) is used.
The other part that I don't get is why this is needed for capture only, and we don't need it for playback so far. Is this because of a lucky coincidence between driver and firmware implementation for playback? Or would this be needed for HDMI as well?
sound/soc/sof/intel/cnl.c
Outdated
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.
is this really specific to cnl?
How would you handle the case of a platform which can work in either I2S or HDaudio modes, with a run-time selection done?
and does this really belong in the ops or should it be a hook private to HDaudio cases?
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.
yes, APL and ICL should be included.
I will add a macro ifdef HDA to control it
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.
Humm, not sure. Even if you have HDA configured, you may have DMICs. So not all DAIs will support this callback. and since you invoke the callback from HDaudio code only, this might be better handled from a HDaudio-specific ops or helper.
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.
yes, I made a change
sound/soc/sof/intel/hda-stream.c
Outdated
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.
what happens if we have a platform with HDaudio and DMICs? They are already on the market for WHL windows platforms, need to think of this case upfront.
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.
It is removed
sound/soc/sof/intel/hda-dai.c
Outdated
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.
alignment is out of whack?
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.
yes, I will refine it later. I just finish this RFC before leaving office. Sorry, it is not perfect.
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.
Also better if the masks and slots can be embedded in a new structure, that way you are only passing a single ptr to a structure rather than 4 ptrs to ints.
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.
this is a ASoC interface and I just reuse it for hda.
sound/soc/sof/intel/hda-dai.c
Outdated
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.
init needed?
Also reverse xmas tree order.
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.
yes, I will refine it.
sound/soc/sof/ops.h
Outdated
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.
I am not sure this is right? We will have multiple dais, some of which will need a config and others will not. The level of indirection should be at the dai level and probably part of the hw_params?
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.
We made a agreement that it should be done as SSP at config time.
My last PR is done at hw_params. But it doesn't work for tone because there maybe
no hw_params for tone.
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.
@ranj063 can you comment on the tone? I am pretty sure you did a modification to pass the hw_params.
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.
@ranj063 yes, I am also very curious about this. I got some info from keyon only
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.
@RanderWang @plbossart sorry I havent had the time to review this PR yet. But seems like the code is outdated now and I'm lost on what the original question was about.
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 question was about tone support requiring hw_params to be set even in the absence of a PCM FE.
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.
@plbossart @RanderWang yes, the tone component didnt work for the longest time because I wasnt setting hw_params. So in order to set the hw_params, we introduced the concept of a virtual PCM FE (a pcm that is not registered with ALSA) which would be used to set the hw_params.
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.
@ranj063 do you have chance to talk about this(virtual PCM) in alsa-devel mailing list? That's a good idea to handle kinds of problems like this tone one, it would be good if we can align this solution from Takashi and Mark before implement SOF specific coding.
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.
@keyonjie not yet. I'll have to re-visit the tone patches soon.
c884c65 to
e4a6d1c
Compare
|
Update my PR. This patch is not only for capture but also playback. Now it is lucky to get playback work. |
Do you mean to say playback works because the link ID is set to zero which happens to be the one used by firmware? |
Your description is for capture because of DMA trace. For playback, host setting matches FW setting. |
@plbossart after my chat with Rander I understood the problem as follows: @RanderWang please correct me if I am wrong @ranj063 |
|
Sorry to chime in late, after discussion with Rander about the implementation difficulties with assigning Link DMA channels at topology parsing stage, I suggest we switch back to previous solution, that is, sending link_dma_id in hw_params(), and Ranjani's explanation about virtual PCM and hw_params() usage in tone case give me more confidence about doing it in that way: in driver side, in FW side, they are both minor changes with low risk to me, I vote for it. |
|
@keyonjie for my benefit : Is that something that happens only once? |
e4a6d1c to
01c3a3a
Compare
|
@ranj063 please refer to According to ASoC, it is called by fe_dai |
|
@ranj063 if one BE is shared by multiple FEs, BE hw_params would be called multiple times. Each FE would call BE hw_params. Just think that each FE works randomly not at the same time regularly, so each FE need to set BE. |
This is NOT a good design. You can have multiple FEs connected to a BE, and conversely we could send the same data on multiple BEs (think playing an alarm on both speakers and headset). We've circled on this before, why are we going back on this? This is a DAI property, not a FE property. |
|
@plbossart a few more questions for my understanding:
|
|
1. Is link dma used in the case of HDMI playback?
yes, same as for HDaudio
1. If so, we do we use the decoupled mode in this case?
use, there is no other choice. The only things that work are coupled
(for legacy w/o DSP) and decoupled (with DSP managing the link DMA).
…
1.
|
|
Thanks @plbossart. Then does it make sense to introduce the notion of coupled/decouple in the link dma config? Then we could choose to use the stream_tag or the link_dma_ch during dai_params depending on the mode. |
We can add an IPC type for this 'DAI params', e.g. SOF_IPC_DAI_PARAMS, we will assign link_dma_ch in BE dai(hda_dai) hw_params(), and send SOF_IPC_DAI_PARAMS to FW, the bonus of adding this is that it's possible to send dynamic hw_params for BE DAIs(including SSP DAI), comparing to that hard-coded in topology previously. |
|
test @RanderWang |
8fb0d36 to
aee508c
Compare
|
Update my RFC. |
aee508c to
f135705
Compare
sound/soc/sof/intel/hda-dai.c
Outdated
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.
Also better if the masks and slots can be embedded in a new structure, that way you are only passing a single ptr to a structure rather than 4 ptrs to ints.
sound/soc/sof/topology.c
Outdated
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.
alignment.
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.
it's way too big for an inline function?
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.
yes, I will refine it
plbossart
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.
Nice progress. This is starting to take shape the way I had envisioned, with an IPC similar to what happens for SSPs, and code that is mostly self-explanatory as I like it.
There are a couple of issues to fix, mostly awkward naming and snd_soc API usage, but we are not far from the solution. Thanks!
sound/soc/sof/topology.c
Outdated
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.
it's way too big for an inline function?
sound/soc/sof/topology.c
Outdated
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.
i like one variable per line, otherwise inits are hidden in plain sight...
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.
yes
|
@plbossart @RanderWang I am skeptical about a few things with the use of get_channel_map API:
Dont mean to be hindering progress but this will help me educate myself too. |
I search get_channel_map in Linux kernel, and get no reference of it. So I reuse it . I need a channel allocated by hda, not set a channel to hda, so I use get_channel_map. And I add a set_channel_map to release resource. |
f135705 to
7cbff19
Compare
sound/soc/sof/topology.c
Outdated
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.
You could just do
ret = snd_soc_dai_set_channel_map(dai, link->dpcm_playback, NULL,
link->dpcm_capture, NULL);
and eliminate rx_num and tx_num completely in this function
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.
link->dpcm_playback is defined as bool variable, so I don't do like this.
Ranjani is correct though that you are not really using get_channel_map in the intended way, since you ignore tx_num and rx_num. And along the same lines, you assign a number of resources in the _get but the _set is mainly about double-checking for errors. I am not sure this is consistent, you may want to allocated a stream_tag in the get if there is none, and do all the actions with side-effects in the _set. It may also be that you are doing an analogy between get/put (as done for runtime_pm) and get/set (as done for general controls and usual resources access), but they have vastly different semantics. A set is not supposed to release resources in general. |
7cbff19 to
762aec8
Compare
|
I refine my patch, Now release hda stream by hw_free interface in dai driver. |
|
@RanderWang looks good but for the fact that I think we still need to use set_channel_map instead of get_channel_map. BTW isthere a FW patch that goes with this PR? I'd like to give it a try on my board. |
|
update patch in FW |
plbossart
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.
In addition to cosmetic changes (error messages and fallthrough), I'd really like comments on what the hw_free part is doing? You have an else case that I just don't understand in the context of a link dma.
sound/soc/sof/topology.c
Outdated
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 error message seems wrong?
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.
yes, I will refine it
sound/soc/sof/topology.c
Outdated
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.
/* fall through */ comment needed
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.
yes, I will add it
sound/soc/sof/intel/hda-dai.c
Outdated
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.
in what cases is the 'else' selected? I really don't understand this code, sorry.
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.
else case: pcm is NULL, this the case for link dma resource free.
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.
let me re-ask the question: is hda_link_hw_free() called at different times with substream==NULL and substream != NULL conditions? if yes, can you explain the two cases?
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.
yes, two cases:
(1) it is called when audio stream is stopped, at this time you could get substream to do some work
(2) at dai_link_unload. I reuse this function with null substream.
It is very tricky to do like this. I also don't like it now. Now I want to do it in hda_dsp_free, but it is impossible do at there because there is no dai.
I am very struggled to find a interface to free logical stream. Actually, it is no need to free it and it would be free when sof is removed.
Now FW set wrong link dma channel which make capture failed. My algorithm is: allocate link dma channel in host and send hda config to FW to config link dma. Signed-off-by: Rander Wang <rander.wang@linux.intel.com>
762aec8 to
3b58d21
Compare
|
@plbossart @RanderWang Lack of setting link DMA channel in HD-A config in kernel side caused regression #322 Since the firmware has merged thesofproject/sof/pull/634 which requires this feature. |
What can I say? I've mentioned multiple times that we should not merge IPC changes on one side only. I am not going to merge code that is still being reviewed, sorry. |
@plbossart It is not caused by IPC change, as you know this IPC change doesn't make any non-hda case failed because it is not change the size of IPC package (union) and is not used now. Please refer to our CI test, all cases pass! Just HDA playback is failed. |
|
@plbossart I'm not pushing you to merge the PR since it seems we need more time to refine the solution kernel side. @lgirdwood It seems we need to revert the thesofproject/sof/pull/634 from FW master branch, to solve the toplogy loading failure #322. |
plbossart
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.
@RanderWang I will merge this PR since it's good enough and I will add follow-up comments myself stating that this will be reworked. We are clearly abusing the APIs and that'll likely be frowned upon.
Please continue to look for simpler ways to do this. Thank you.
Can you please add an enhancement ticket to track it? @plbossart |
|
@plbossart thanks for your guide, it is a very good lesson for me. I will go on moving forward. |
Booting 4.20 on SolidRun Clearfog issues this warning with DMA API debug enabled: WARNING: CPU: 0 PID: 555 at kernel/dma/debug.c:1230 check_sync+0x514/0x5bc mvneta f1070000.ethernet: DMA-API: device driver tries to sync DMA memory it has not allocated [device address=0x000000002dd7dc00] [size=240 bytes] Modules linked in: ahci mv88e6xxx dsa_core xhci_plat_hcd xhci_hcd devlink armada_thermal marvell_cesa des_generic ehci_orion phy_armada38x_comphy mcp3021 spi_orion evbug sfp mdio_i2c ip_tables x_tables CPU: 0 PID: 555 Comm: bridge-network- Not tainted 4.20.0+ #291 Hardware name: Marvell Armada 380/385 (Device Tree) [<c0019638>] (unwind_backtrace) from [<c0014888>] (show_stack+0x10/0x14) [<c0014888>] (show_stack) from [<c07f54e0>] (dump_stack+0x9c/0xd4) [<c07f54e0>] (dump_stack) from [<c00312bc>] (__warn+0xf8/0x124) [<c00312bc>] (__warn) from [<c00313b0>] (warn_slowpath_fmt+0x38/0x48) [<c00313b0>] (warn_slowpath_fmt) from [<c00b0370>] (check_sync+0x514/0x5bc) [<c00b0370>] (check_sync) from [<c00b04f8>] (debug_dma_sync_single_range_for_cpu+0x6c/0x74) [<c00b04f8>] (debug_dma_sync_single_range_for_cpu) from [<c051bd14>] (mvneta_poll+0x298/0xf58) [<c051bd14>] (mvneta_poll) from [<c0656194>] (net_rx_action+0x128/0x424) [<c0656194>] (net_rx_action) from [<c000a230>] (__do_softirq+0xf0/0x540) [<c000a230>] (__do_softirq) from [<c00386e0>] (irq_exit+0x124/0x144) [<c00386e0>] (irq_exit) from [<c009b5e0>] (__handle_domain_irq+0x58/0xb0) [<c009b5e0>] (__handle_domain_irq) from [<c03a63c4>] (gic_handle_irq+0x48/0x98) [<c03a63c4>] (gic_handle_irq) from [<c0009a10>] (__irq_svc+0x70/0x98) ... This appears to be caused by mvneta_rx_hwbm() calling dma_sync_single_range_for_cpu() with the wrong struct device pointer, as the buffer manager device pointer is used to map and unmap the buffer. Fix this. Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> Signed-off-by: David S. Miller <davem@davemloft.net>
Booting 4.20 on SolidRun Clearfog issues this warning with DMA API debug enabled: WARNING: CPU: 0 PID: 555 at kernel/dma/debug.c:1230 check_sync+0x514/0x5bc mvneta f1070000.ethernet: DMA-API: device driver tries to sync DMA memory it has not allocated [device address=0x000000002dd7dc00] [size=240 bytes] Modules linked in: ahci mv88e6xxx dsa_core xhci_plat_hcd xhci_hcd devlink armada_thermal marvell_cesa des_generic ehci_orion phy_armada38x_comphy mcp3021 spi_orion evbug sfp mdio_i2c ip_tables x_tables CPU: 0 PID: 555 Comm: bridge-network- Not tainted 4.20.0+ thesofproject#291 Hardware name: Marvell Armada 380/385 (Device Tree) [<c0019638>] (unwind_backtrace) from [<c0014888>] (show_stack+0x10/0x14) [<c0014888>] (show_stack) from [<c07f54e0>] (dump_stack+0x9c/0xd4) [<c07f54e0>] (dump_stack) from [<c00312bc>] (__warn+0xf8/0x124) [<c00312bc>] (__warn) from [<c00313b0>] (warn_slowpath_fmt+0x38/0x48) [<c00313b0>] (warn_slowpath_fmt) from [<c00b0370>] (check_sync+0x514/0x5bc) [<c00b0370>] (check_sync) from [<c00b04f8>] (debug_dma_sync_single_range_for_cpu+0x6c/0x74) [<c00b04f8>] (debug_dma_sync_single_range_for_cpu) from [<c051bd14>] (mvneta_poll+0x298/0xf58) [<c051bd14>] (mvneta_poll) from [<c0656194>] (net_rx_action+0x128/0x424) [<c0656194>] (net_rx_action) from [<c000a230>] (__do_softirq+0xf0/0x540) [<c000a230>] (__do_softirq) from [<c00386e0>] (irq_exit+0x124/0x144) [<c00386e0>] (irq_exit) from [<c009b5e0>] (__handle_domain_irq+0x58/0xb0) [<c009b5e0>] (__handle_domain_irq) from [<c03a63c4>] (gic_handle_irq+0x48/0x98) [<c03a63c4>] (gic_handle_irq) from [<c0009a10>] (__irq_svc+0x70/0x98) ... This appears to be caused by mvneta_rx_hwbm() calling dma_sync_single_range_for_cpu() with the wrong struct device pointer, as the buffer manager device pointer is used to map and unmap the buffer. Fix this. Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> Signed-off-by: David S. Miller <davem@davemloft.net>
[ Upstream commit a8fef9b ] Booting 4.20 on SolidRun Clearfog issues this warning with DMA API debug enabled: WARNING: CPU: 0 PID: 555 at kernel/dma/debug.c:1230 check_sync+0x514/0x5bc mvneta f1070000.ethernet: DMA-API: device driver tries to sync DMA memory it has not allocated [device address=0x000000002dd7dc00] [size=240 bytes] Modules linked in: ahci mv88e6xxx dsa_core xhci_plat_hcd xhci_hcd devlink armada_thermal marvell_cesa des_generic ehci_orion phy_armada38x_comphy mcp3021 spi_orion evbug sfp mdio_i2c ip_tables x_tables CPU: 0 PID: 555 Comm: bridge-network- Not tainted 4.20.0+ thesofproject#291 Hardware name: Marvell Armada 380/385 (Device Tree) [<c0019638>] (unwind_backtrace) from [<c0014888>] (show_stack+0x10/0x14) [<c0014888>] (show_stack) from [<c07f54e0>] (dump_stack+0x9c/0xd4) [<c07f54e0>] (dump_stack) from [<c00312bc>] (__warn+0xf8/0x124) [<c00312bc>] (__warn) from [<c00313b0>] (warn_slowpath_fmt+0x38/0x48) [<c00313b0>] (warn_slowpath_fmt) from [<c00b0370>] (check_sync+0x514/0x5bc) [<c00b0370>] (check_sync) from [<c00b04f8>] (debug_dma_sync_single_range_for_cpu+0x6c/0x74) [<c00b04f8>] (debug_dma_sync_single_range_for_cpu) from [<c051bd14>] (mvneta_poll+0x298/0xf58) [<c051bd14>] (mvneta_poll) from [<c0656194>] (net_rx_action+0x128/0x424) [<c0656194>] (net_rx_action) from [<c000a230>] (__do_softirq+0xf0/0x540) [<c000a230>] (__do_softirq) from [<c00386e0>] (irq_exit+0x124/0x144) [<c00386e0>] (irq_exit) from [<c009b5e0>] (__handle_domain_irq+0x58/0xb0) [<c009b5e0>] (__handle_domain_irq) from [<c03a63c4>] (gic_handle_irq+0x48/0x98) [<c03a63c4>] (gic_handle_irq) from [<c0009a10>] (__irq_svc+0x70/0x98) ... This appears to be caused by mvneta_rx_hwbm() calling dma_sync_single_range_for_cpu() with the wrong struct device pointer, as the buffer manager device pointer is used to map and unmap the buffer. Fix this. Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> Signed-off-by: David S. Miller <davem@davemloft.net> Signed-off-by: Sasha Levin <sashal@kernel.org>
Now FW set wrong link dma channel which make capture failed.
My algorithm is: allocate link dma channel in host and
send hda config to FW to config link dma.
This is a prototype, please ignore the format. Please give me some advices, thanks!
Signed-off-by: Rander Wang rander.wang@linux.intel.com