[RanderWang]

This issue is found on WHL with pulse audio enabled.
Pulse audio would try to capture stream at booting stage,
then close the capture stream. This makes HDA controller
hang. The root cause is: DMA trace would use host dma
channel one, and capture stream would use link dma channel
one. DMA trace would set decouple mode for host dma channel
one and link dma channel one. Capture would also do as DMA
trace in hw_params function, but set couple mode for these
two dma when pcm is closed. However, DMA trace needs decouple
mode, so HDA controller hangs.

And another potential risk is when applying coupled and decopuled
mode for stream setup, this would break some working capture streams.
Now allocate stream for DMA trace with max valid stream tag so
that the normal catpure stream tag allocated by host and link dma
allocator would be the same and the setting of coupled & decoupled
mode would not affect other streams. And also refine the host stream
allocation function to make sure the allocated host stream would not
affect other link streams.

fix #767

[keyonjie]

This issue is found on WHL with pulse audio enabled.
Pulse audio would try to capture stream at booting stage,
then close the capture stream. This makes HDA controller
hang. The root cause is: DMA trace would use host dma
channel one, and capture stream would use link dma channel
one. DMA trace would set decouple mode for host dma channel
one and link dma channel one. Capture would also do as DMA
trace in hw_params function, but set couple mode for these

Why channel one possible to be allocated to capture stream since it(host dma channel 1) is already used by DMA trace? Is couple mode can couple the same channel index between host dma and link dma only?

If so, is it possible that the commit dca70eb we did a month back( set couple -> program stream format -> set decouple) in hda_dsp_stream_hw_params() also introduce similar risk?

two dma when pcm is closed. However, DMA trace needs decouple
mode, so HDA controller hangs.

Actually, SOF always requires decouple mode in hda dsp mode, so there is no
need to set couple mode

fix #767

[RanderWang]

Why channel one possible to be allocated to capture stream since it(host dma channel 1) is already used by DMA trace? Is couple mode can couple the same channel index between host dma and link dma only?

Because the allocation of link dma is independent of host dma. To copule different channel ? I don't find any register setting in spec.

If so, is it possible that the commit dca70eb we did a month back( set couple -> program stream format -> set decouple) in hda_dsp_stream_hw_params() also introduce similar risk?

No, first I removed this patch but still got error

[RanderWang]

I also need to remove setting couple mode in host dma and I will update it tomorrow.

Another solution is to check the usage of both host dma and link dma to set mode. But is there any benefit to set dma in couple mode when it is free ?

[plbossart]

is there any benefit to set dma in couple mode when it is free ?

It's the recommended hardware sequence, so let's just follow it blindly rather than make conjectures on how hardware might work. Thanks!

[plbossart]

@lbetlej can you comment on this one? I think there are expectations that the DMA channels remain in coupled mode unless they are explicitly in use by the DSP.

[ranj063]

@RanderWang how about calling snd_hdac_ext_stream_decouple(bus, stream, true) in hda_dsp_trace_init() instead?

[RanderWang]

@RanderWang how about calling snd_hdac_ext_stream_decouple(bus, stream, true) in hda_dsp_trace_init() instead?

There is another issue that: trace log is not updated after capture close. So your suggestion is not fit for this case.

[RanderWang]

add detail in spec about decoupled and coupled bit:

Note:	The bits should only be modified when the corresponding host DMA and link DMA are idle, i.e.
RUN bits are cleared, and DMA contexts have been destroyed through SRST bits if it was previously
activated.

Note:	The bit does not really enable or disable the Audio DSP operation, but mainly to workaround
some legacy Intel HD Audio driver software such that if bit = 0, ADSPxBA (BAR2) is mapped to the
Intel HD Audio memory mapped configuration registers, for compliancy with some legacy SW
implementation.  If bit = 1, only then ADSPxBA (BAR2) is mapped to the actual Audio DSP memory
mapped configuration registers.

[ranj063]

@RanderWang how about calling snd_hdac_ext_stream_decouple(bus, stream, true) in hda_dsp_trace_init() instead?

There is another issue that: trace log is not updated after capture close. So your suggestion is not fit for this case.

@RanderWang yes of course. thanks.

[ranj063]

@plbossart @RanderWang this seems like a nagging problem we've been dealing with because of trace DMA. Just wondering if it makes sense to maybe reserve channel 1 exclusively for trace?

[RanderWang]

@plbossart @RanderWang this seems like a nagging problem we've been dealing with because of trace DMA. Just wondering if it makes sense to maybe reserve channel 1 exclusively for trace?

Yes, I just discussed with keyon and Mengdong, we should reserve max - 1 for trace and I will explain later. There is another bug my PR can't fix it.

[RanderWang]

Even we never set coupled mode in stream close function, we would still have a bug about coupled mode setting. @keyonjie mentioned: the commit dca70eb we did a month back( set couple -> program stream format -> set decouple) in hda_dsp_stream_hw_params() would affect capture stream. The reason is:
DMA trace only occupies the host dma channel one and doesn't use link dma. this affects other capture stream. For example

(1) capture stream 2 defined in topology would get HDC3 (host dma channel 3) and LDC2
(link dma channel 2) from allocator when topology loaded, then It begins to run.

(2) capture stream 1 would use HDC2 and LDC1, apply the sequence above: couple HDC2 and LDC2 (not LDC1 because HDC2 could only couple & decouple with LDC2). At this time capture stream 2 is broken.

The issue is caused by DMA trace which makes other capture get mismatched channel number. We could
reserve max capture stream number - 1 to DMA trace so that each capture stream would get the same channel number of HDC & LDC. So each coupled & decouple setting would not affect other streams.

To modify allocator for HDC and LDC is very tricky because HDC is allocated independent of LDC and their allocators are different.

There is no such issue with skylake driver because there is no DMA trace to occupy the first channel. And last year we spent 3 months to debug and fix capture issue on HDA is also for this reason.

[ranj063]

@RanderWang I also think we need a way to disable trace DMA to quickly debug such problems. There's no simple way to do this today.

[RanderWang]

@RanderWang I also think we need a way to disable trace DMA to quickly debug such problems. There's no simple way to do this today.

I fully agree with you. Actually we discussed with @lgirdwood @xiulipan , but got no result.

[RanderWang]

Update my PR. Now stream allocator would allocate last stream in list and DMA trace will use the stream with max valid stream tag to avoid occupying the stream tag one. I change the stream_get function because
there will be another reserved capture stream for buffer probing (@keyonjie )which could also work with this change.

[plbossart]

see big open on fundamental architecture assumptions below.

[plbossart]

Sorry, I don't get why a stream would need to have the same tag on both host and link DMA. This only makes sense if you have a passthrough flow, but the moment you start mixing two streams this assumption is no longer valid. Likewise if we have a stream that originates in the firmware, like tone generator, there will be no host DMA.
What am I missing?

[RanderWang]

Good point! I just draw a picture to describe all the cases. To fix this is very complex for (1) host dma and link dma allocator are independent (2) link dma channels are allocated at topology loading time but host dma channels are allocated at pcm_hw_params. I don't want to introduce many new bugs and I would first check the sequence of coupled-set_format-decoupled

[plbossart]

@RanderWang your picture is what I have in mind so at least on this we are in agreement.
I however have a real problem merging code that is known to be problematic. we've been circling about the HDaudio link DMA for 4+ months and I'd like to move on with a solution that scales, so please revisit this and suggest a more complete solution, not a work-around.

[RanderWang]

@plbossart @keyonjie

Thanks, I also hate submitting immature patch. I think we are in agreement about the solution about link dma usage. And now we find a coupled-decoupled issue, I discussed with keyon and got my solution. I think it is a complete one and need your advise.

Now there are three kinds of hda|link dma allocation, the time sequence is

init SOF  ---> DMA trace init (host dma allocation) ---> topology load ---> link dma allocation ---> pcm_open(host dma allocation)

DMA trace needs to be initiated before topology loading for we could use DMA trace to debug topology loading. And link dma allocation would only happens at topology loading time. When one pcm is opened,
host dma channel would be allocated dynamically.

Now link dma allocation is complete for above picture. There is no need to change it. And for link dma allocation is not dynamically and it happens before all the pcm_open case, we could change host dma allocation to give a solution.

In host dma allocation function for pcm_open which is for FE.

(1) find a free host dma channel and check whether the matched (the same channel number) link dma channel is free. If both are free, return the host dma channel. It is easy to do this in driver.

(2) if the matched link dma channel is used, check whether this is used by BE of this FE pcm. If yes, return
the host dma channel.

(3) if not and there are some free channels, goto (1) to check next free host dma channel.

(4) if there is no free channel, return error.

Here one exception, host dma allocation for DMA trace, which happens before link dma allocation. The solution is allocating max channel done as this PR now. To use above algorithm in link dma allocator ？
It is hard because link dma allocator is implemented by legacy hda and we share it with other drivers.

I implemented a prototype and tested it.

Thanks for your idea!

[plbossart]

@RanderWang I appreciate the well-worded description, thank you for working on this. I have a conceptual issue on this point:

(2) if the matched link dma channel is used, check whether this is used by BE of this FE pcm. If yes, return
the host dma channel.

This is not clear to me. In the picture above, you can have a 1:N relationship between 1 FE and N BEs. So how would this work?

Also I don't get what you are trying to do by trying to match host and link DMA numbers? Why is this needed in the first place? Is there any issue preventing us from using an independent allocation on the link and host sides, possibly with the trace pre-allocated?

[RanderWang]

@RanderWang I appreciate the well-worded description, thank you for working on this. I have a conceptual issue on this point:

(2) if the matched link dma channel is used, check whether this is used by BE of this FE pcm. If yes, return
the host dma channel.

This is not clear to me. In the picture above, you can have a 1:N relationship between 1 FE and N BEs. So how would this work?

Yes, this works for this case. Please check the following explanation first. To set format, FE would couple host dma and link dma first and should not break other BE streams, only its own BE stream. So if one of the BE use the matched channel, The FE would only affect its own BE. For example, one FE has two BE which use link dma channel 1 and 2. FE could use host dma channel 1 or 2 because it will couple host dma channel 1 with link dma channel 1, or 2 with 2. This case would not break other BE steam with channel 3 or others.

Why is this needed in the first place?

This is for dynamic host dma only stream. Now only DMA trace is a example of this type.

Also I don't get what you are trying to do by trying to match host and link DMA numbers?
Is there any issue preventing us from using an independent allocation on the link and host sides, possibly with the trace pre-allocated?

It is caused by the patch for HW requirement. dca70eb. When we using an independent allocation, pcm stream would get different channel for host dma and link dma, and this would make playback or capture failed.

For example
(1) capture stream 2 defined in topology would get HDC3 (host dma channel 3) and LDC2
(link dma channel 2) from allocator when topology loaded, then It begins to run.

(2) capture stream 1 would use HDC2 and LDC1, apply the sequence above to fix format issue: couple HDC2 and LDC2 (not LDC1 because HDC2 could only couple & decouple with LDC2). At this time capture stream 2 is broken.

[plbossart]

@RanderWang Are we in agreement that the connection between FE and BE is dynamic, and possibly controlled by mixer settings? If yes, please elaborate on your solution. I can see a corner case where a FE is initially connected to 2 BE using e.g ch 1 and 2, so the FE get host ch 1, but later the BE with ch1 is disabled so now we have the FE using host ch 1 and the remaining BE link ch 2.

If your solution assumes a static connection between FE and BE, then you missed the 'dynamic' prefix of DPCM...

[RanderWang]

I can see a corner case where a FE is initially connected to 2 BE using e.g ch 1 and 2, so the FE get host ch 1, but later the BE with ch1 is disabled so now we have the FE using host ch 1 and the remaining BE link ch 2.

If BE with ch1 is disabled, in step(2) the BE would not be found by this FE and this channel is used, so this ch1 can't be allocated by FE, and FE would check next ch2, which is fit for it. This BE finding algorithm is implemented in ASoC code, we can use it.

[ranj063]

@RanderWang an alternate suggestion, trace dma channel will always be allocated even before tplg is loaded. So there is no possibility of a stream being open at this point. Why bother iterating then? Cant we just allocate list_last_entry?

[RanderWang]

Yes, I also considered this solution. But there will be another reserved stream for buffer probing which is used for checking buffer content between each processing component in FW, we have to allocate list_second_last_entry.

[ranj063]

@RanderWang thanks. can you please educate me as to where this buffer probing is done?

[RanderWang]

@keyonjie can you help to answer this question ? Thanks!

[keyonjie]

@ranj063 the FW buffer probing feature is in our plan, we discussed that when in Gdansk @plbossart also there, it is used to probe buffers between components, dump contents to host side for debugging purpose, it will work similar to dma trace.

So here, we need to make sure our solution is extendable, there might be > 1 dailess capture streams in the future.

20190416:
When consider it more, those buffer probing pipelines are similar to normal I2S capture ones as their host dma indies are allocated in hw_params stage(after BE ones already allocated), so it looks doable to me just reserving the last index for DMA trace only.
@RanderWang please checking the existence of DMA trace when reserving this, we should make sure all those DMA trace related stuff can be excluded out via one kconfig item.

[RanderWang]

Update my prototype, thanks for review.

[plbossart]

I can see a corner case where a FE is initially connected to 2 BE using e.g ch 1 and 2, so the FE get host ch 1, but later the BE with ch1 is disabled so now we have the FE using host ch 1 and the remaining BE link ch 2.

If BE with ch1 is disabled, in step(2) the BE would not be found by this FE and this channel is used, so this ch1 can't be allocated by FE, and FE would check next ch2, which is fit for it. This BE finding algorithm is implemented in ASoC code, we can use it.

You didn't get the point. If the BE with ch1 is disabled after the stream started, then you would have a case where the FE uses ch1 and the remaining BE use ch2. If that can work, then your algorithm is not necessary, you would use any mapping. If this does not work then we have a very large problem with link DMA support.

[RanderWang]

If BE with ch1 is disabled, in step(2) the BE would not be found by this FE and this channel is used, so this ch1 can't be allocated by FE, and FE would check next ch2, which is fit for it. This BE finding algorithm is implemented in ASoC code, we can use it.

You didn't get the point. If the BE with ch1 is disabled after the stream started, then you would have a case where the FE uses ch1 and the remaining BE use ch2. If that can work, then your algorithm is not necessary, you would use any mapping.

yes, the algorithm is work for this case. when FE and BE are working, they are decoupled, so they don't affect each other. FW would copy data from host dma channel 1 to link dma channel 2. But for this case, if without the algorithm, there is another stream will allocated host channel 2, and couple with BE ch2, this breaks the FE in above case.

If this does not work then we have a very large problem with link DMA support.

@plbossart First I quote the algorithm in dpcm. I think if BE with ch1 is disabled by dpcm, there will be a sequence of stop-and-reopen.

The audio driver processes this as follows :-

    (1) Machine driver receives Jack removal event.
    (2) Machine driver OR audio HAL disables the Headset path.
    (3) DPCM runs the PCM trigger(stop), hw_free(), shutdown() operations on DAI0 for headset since the  path is now disabled.
    (4) Machine driver or audio HAL enables the speaker path.
    (5) DPCM runs the PCM ops for startup(), hw_params(), prepare() and trigger(start) for DAI1 Speakers since the path is enabled.

[RanderWang]

The above dpcm algorithm is for one case only, mixer and mux event would also dynamically change FE & BE also. But I am sure my algorithm could work with them.

[plbossart]

You didn't explain how, so that's not helping.

[RanderWang]

So, I understand the the static link you comment. If FE is running and not closed, the host dma usage is static, and not change from ch1 to ch2 in above your case. I don't think there is a big problem. First host dma channel 2 can't be used by other FE stream, so it is not a waste of channel. and second host dma and link dma now are in decoupled mode, and FW would copy data correctly to make them work.

[RanderWang]

You didn't explain how, so that's not helping.

For mixer case, just assume two FE and one BE.
(1) BE use ch1
(2) first FE would get ch1 in step (2) in my algorithm. The FE set coupled-decouple mode to set format, this would only affect BE with ch1 and not affect other stream.
(3) the second FE would get ch2 in step(1) in my algorithm for link dma ch2 is not used, so the setting of coupled-decouple mode is safe.

if there are three FEs and two BEs, FE1 <----> BE1(running), FE2 & FE3 <------> BE2 to be setup.
BE1 uses ch1, BE2 use ch2, FE2 would get host dma ch2, and FE3 would get host dma ch3. FE2 & FE3 will not affect FE1 and BE1

For mux case, just assume two FE and one BE. FE1 <--------> BE1, FE2 is not connected. The allocation flow is like above. If FE2 is changed to connect BE1, and FE1 is not connect, we don't need to change our host dma allocation.

[RanderWang]

One word to summary this PR:
The connection between host and link dma is maintained by FW, and when stream is working, host and link
dma are in decoupled mode, and one host dma channel can work with any link dma channel.

This PR is to fix two cases:
(1) when a stream is started, a hardware workaround in stream_hw_params (coupled-set-format-decouple) would not affect other running stream.
(2) when a stream is closed, it will be restored to coupled mode. This PR make sure this case don't affect other running stream.

[RanderWang]

How to fix? just avoid allocating the same host & link dma channel to different pcm stream.

[plbossart]

For mixer case, just assume two FE and one BE. (1) BE use ch1 (2) first FE would get ch1 in step (2) in my algorithm. The FE set coupled-decouple mode to set format, this would only affect BE with ch1 and not affect other stream. (3) the second FE would get ch2 in step(1) in my algorithm for link dma ch2 is not used, so the setting of coupled-decouple mode is safe. if there are three FEs and two BEs, FE1 <----> BE1(running), FE2 & FE3 <------> BE2 to be setup. BE1 uses ch1, BE2 use ch2, FE2 would get host dma ch2, and FE3 would get host dma ch3. FE2 & FE3 will not affect FE1 and BE1 For mux case, just assume two FE and one BE. the flow is just like above.

I was talking about 1 FE and 2 BEs, with BEs selected by a kcontrol. Please describe how this might work.

[RanderWang]

I was talking about 1 FE and 2 BEs, with BEs selected by a kcontrol. Please describe how this might work.

I understood your idea, but when I checked the code in ASoC and I only found mixer and mux would change the dpcm patch. The update function is:

/* Called by DAPM mixer/mux changes to update audio routing between PCMs and
 * any DAI links.
 */
int soc_dpcm_runtime_update(struct snd_soc_card *card)

As I know, mixer and mux don't support 1:N(one FE works with N BEs), so I just explained the current status.

@lgirdwood @keyonjie @bardliao Do you know whether dpcm supports 1 FE work with N BEs and how ?
Thanks!

[keyonjie]

hi @RanderWang can we do some tests to check if the micro-seconds couple-decouple toggling(in the fixes for format setting) really introduce any issues(noise? hang?), if it looks fine in our real test, maybe it is wise to not spend big effort on discussion and try to design complicate channel index allocation algorithm which is actually hard to handle all possible nowaday's and future's mappings b/w host DMA and link DMA channel indices.

[RanderWang]

@keyonjie , Yes I will.

[RanderWang]

@plbossart For 1 FE and 2 BEs, with BEs selected by a kcontrol. I first talk about dpcm flow.
(1) a mixer or mux event will be triggered when they are set by users
(2) dpcm call soc_dpcm_runtime_update to update path. First remove the disable BE and invoke hw_free to disable BE, second add BE in the path and invoke hw_param to enable new BE.

And our two risks：seting couple mode for workaround at FE pcm_open and retoring couple mode at pcm_close.

For 1 FE and 2 BEs case: there is no FE stream open, so there is no risk for workaround. But there is one
risk if one BE is disabled and close. The disabled BE will be coupled with FE and make a error. So I need to remove the coupled mode setting when pcm is closed. And we discussed the coupled mode is no need for SOF, neither I2S or HDA.

I can't image other risks. And if you have one , please point out. Thanks!

[RanderWang]

@plbossart @keyonjie
I tested the impact of the workaround: couple-setformat-decouple.
In hda_dsp_pcm_pointer, which would be triggered every a few ms, I added these code to test the workaround.

        /* couple host and link DMA, disable DSP features */
	snd_sof_dsp_update_bits(sdev, HDA_DSP_PP_BAR, SOF_HDA_REG_PP_PPCTL,
				0x80, 0);

	if (hda && !hda->no_ipc_position) {
		/* read position from IPC position */
		pos = spcm->stream[substream->stream].posn.host_posn;
		goto found;
	}

	/* decouple host and link DMA, enable DSP features */
	snd_sof_dsp_update_bits(sdev, HDA_DSP_PP_BAR, SOF_HDA_REG_PP_PPCTL,
				0x80, 0x80);

The result is: I got big noise and XRUN happened, then aplay was broken. So the impact is great and we need to avoid this bug.

[RanderWang]

Update my PR, break the solution to three parts
(1) refine host stream allocation
(2) reserve last stream for dma trace
(3) delete coupled mode setting in hw_free

Tested on WHL.
I discussed with @keyonjie and got a conclusion that this PR can work with dpcm.

[plbossart]

I have been asking for clarifications on why you need to have a match between link and host DMA channels. You keep providing the same patches without explanations.
I don't see any merit in all this. Link and host DMA channels are not described as needing to be tied in the hardware specification so NAK on this PR.

[RanderWang]

@plbossart , I explained many times, for example