Chain dma support

[jsarha]

Its probably worth squashing this up a bit, especially now that I had to mangle Ranjani's commit quite a bit when solving sof-dev rebase conflicts. But maybe this is easier to review in pieces.

Both 16- and 32-bit audio works with this PR, but getting 44.1kHz need still some more work.

[RanderWang]

Could you give a full picture of solution ? How to use chain dma ? How to support in topology ? How does driver do to cooperate with topology?

[ranj063]

@jsarha can you please squash the relevant patches and add descriptions? It is very hard to review it like this.

Also, IPC4 test fails spectacularly. So something is not right.

[jsarha]

Here is the squashed up version, but there is still something severely wrong with normal playback, and I am in the middle of debugging it.

[jsarha]

All the issues found in the review are not yet addressed, but pushed the branch again to trigger CI tests to see if the nocodec problems persist.

[jsarha]

I don't understand why details links lead to 404. Maybe another push will help. There is some more fixes this time too. I think this is the last unresolved issue: https://github.com/thesofproject/linux/pull/3953#discussion_r1005856837

[ranj063]

I don't understand why details links lead to 404. Maybe another push will help. There is some more fixes this time too. I think this is the last unresolved issue: #3953 (comment)

01.org is down. Can you check the internal link @jsarha

[jsarha]

Hmm, now that I tested this latest change a bit, it does not appear to work as it should. hw:0,0 does not work any more.

[jsarha]

The first patch was updated and now this appears to work Ok.

[jsarha]

Addressed the last comment, but there is conflicts now... rebasing.

[jsarha]

Yesterdays version now pushed again with @plbossart 's suggestions for descriptions and after a rather painful rebase. I am currently looking at how to add codec side format constraints to a chained dma pcm.

[jsarha]

Updated to work with this latest sof topology pr: https://github.com/thesofproject/sof/pull/6524

[jsarha]

Mostly editorial review. The only open I have is how the codec DAI limitation is handled, if at all?

Since this feature is enabled through topology, would it be enough to code the codec dai limitations there too? I think I could dig the hda dai limitations and apply them as constraints to sof pcm when chained dma is in use, but is that really necessary?

[plbossart]

Mostly editorial review. The only open I have is how the codec DAI limitation is handled, if at all?

Since this feature is enabled through topology, would it be enough to code the codec dai limitations there too? I think I could dig the hda dai limitations and apply them as constraints to sof pcm when chained dma is in use, but is that really necessary?

In general anything tied to hardware codec support cannot be part of the topology.

That said you bring-up an interesting point @jsarha. It turns out that the only codec DAI limitation is the 'sig_bits', but those are fixed in sound/soc/codecs/hdac_hda.c. I thought there were determined based on codec capabilities. @kv2019i do you think we could simplify all this, or get the values from the codec directly?

static struct snd_soc_dai_driver hdac_hda_dais[] = {
{
	.id = HDAC_ANALOG_DAI_ID,
	.name = "Analog Codec DAI",
	.ops = &hdac_hda_dai_ops,
	.playback = {
		.stream_name	= "Analog Codec Playback",
		.channels_min	= 1,
		.channels_max	= 16,
		.rates		= SNDRV_PCM_RATE_8000_192000,
		.formats	= STUB_FORMATS,
		.sig_bits	= 24,
	},
	.capture = {
		.stream_name    = "Analog Codec Capture",
		.channels_min   = 1,
		.channels_max   = 16,
		.rates = SNDRV_PCM_RATE_8000_192000,
		.formats = STUB_FORMATS,
		.sig_bits = 24,
	},
},

Adding @ranj063 and @ujfalusi since we have the same codec dai issue for the abstraction and dsp-less PRs.

Edit: in the AVS driver, the DAI information is created dynamically based on codec information, see https://github.com/thesofproject/linux/blob/551c11c310acbe5a6636a1eecedc5c762f407222/sound/soc/codecs/hda.c#L55

                stream->channels_min = pcm->stream[dir].channels_min;
		stream->channels_max = pcm->stream[dir].channels_max;
		stream->rates = pcm->stream[dir].rates;
		stream->formats = pcm->stream[dir].formats;
		stream->sig_bits = pcm->stream[dir].maxbps;

[jsarha]

On more round of applied review fixes, while waiting for https://github.com/thesofproject/linux/pull/3972 to get ready. The sig_bits are still not copied from codec side, but that should not make big functional difference. I'll see if I get it done before #3972 and this is merged , if not I'll do it as a separate PR.

[jsarha]

Latest changes: Make sure the tplg is using use_chain_dma correctly. I keep the loop in sof_ipc4_trigger_pipelines() and then add a check in sof_ipc4_chain_dma_trigger() that all elements in list have the attribute, since that is a requirement for the feature to work. Also improve comments in both places.

[jsarha]

Only the 0xffffffff => SOF_IPC4_INVALID_NODE_ID change.

[jsarha]

Addressed @plbossart 's comments and changed -ENOTSUPP to -ENODEV due to checkpatch error:

WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP #313: FILE: sound/soc/sof/ipc4-topology.c:528:

•           ret = -ENOTSUPP;
  

[plbossart]

Addressed @plbossart 's comments and changed -ENOTSUPP to -ENODEV due to checkpatch error:

WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP #313: FILE: sound/soc/sof/ipc4-topology.c:528:

•           ret = -ENOTSUPP;
  

oh, that one you can ignore @jsarha. There is no consensus in maintainer circles on this checkpatch warning so we keep using ENOTSUPP

if it is confusing it's because it is.

[jsarha]

oh, that one you can ignore @jsarha. There is no consensus in maintainer circles on this checkpatch warning so we keep using ENOTSUPP

if it is confusing it's because it is.

I do not think there is any pressing need to use ENOTSUPP either, should I still revert back to ENOTSUPP from ENODEV?

[jsarha]

I have also rebased this PR on top of #3972 if it is merged first.

[jsarha]

@plbossart here is the version that is rebased on top of the latest #3972 : https://github.com/jsarha/linux-sof/pull/new/chain_dma-on_top_of_3972

The only place that looks different to current version of this PR is hda-dai.c and hda-dai-ops.c. I also tested with the rebased PR that playback survives susped-resume cycle when playing and when paused.

[jsarha]

Now the PR is rebased on top of #3972 , so please ignore everything else but the three last commits.

[jsarha]

@plbossart 's comments addressed.

[jsarha]

Some typo and wording fixes applied, end removed one bad fix. Now it works again 😅.

[plbossart]

https://github.com/thesofproject/linux/pull/3972 is merged, please rebase @jsarha. Should be straightforward.

[jsarha]

Rebased on top of the latest sof-dev, fixed couple of typos more, and the checkpatch issue I added last night. Should be good now, please approve @ujfalusi , @ranj063 , @plbossart !

[jsarha]

Addressed @RanderWang 's comment and while at it moved the pipe_widget and pipeline variables down in scope.

[ranj063]

@plbossart @jsarha please don't merge this PR. HDA device with chained DMA has a lot of failures

[ranj063]

@jsarha the fixes for SOF firmware failures are in https://github.com/thesofproject/linux/pull/4030

[jsarha]

The problem was SOF FW being sensitive about fifo_size field being present in the message. I developed the whole time against cavs and forgot to test against SOF too. The problem should now be fixed. The changes lie in sof_ipc4_chain_dma_trigger() and in sof_ipc4_chain_dma_send_msg(). @ujfalusi , @ranj063 , @plbossart , and @RanderWang , please review again.