ASoC: Intel: update pcm_params to provide proper period size to FW by mrajwa · Pull Request #1037 · thesofproject/linux

mrajwa · 2019-06-14T05:16:00Z

This patch enables to pass proper period_size value to the firmware.
It is needed to synchronize non-standard copy like i.e draining tasks

Signed-off-by: Marcin Rajwa marcin.rajwa@linux.intel.com

keyonjie

fix your subject for each commit, please refer to what others do.
the change to host_period_bytes is wrong.
better to rename irq_mode to period_irq_mode or similar one.
only 2 commits needed: 1, add period_irq_mode flag to struct(and maybe minor abi version bump needed?). 2. change to use the new struct in one shot(single commit).

plbossart

This is an ABI change...
Please fix all your commits to start with 'ASoC: SOF: Intel" or "ASoC: SOF: ipc" if it's not Intel-specific
the part on no_irq mode is very controversial and I don't know why you need a period if you use the no-irq mode. Something is not quite right there.

plbossart · 2019-06-14T10:37:25Z


-	/* for notifying host period has completed - 0 means no period IRQ */
 	uint32_t host_period_bytes;
+	uint32_t irq_mode; /* 1 means IRQ mode ON */


you've wasting an entire 32-bit field for a boolean. Maybe use 16 bits and add another 16-bit as reserved?

@plbossart, yes good suggestion. I have just updated.

plbossart · 2019-06-14T10:39:26Z

 	/* set host_period_bytes to 0 if no IPC position */
 	if (hda && hda->no_ipc_position)
-		ipc_params->host_period_bytes = 0;
+		ipc_params->irq_mode = 0;


Arrh, here we go again. The no_ipc_position has nothing to do with the no_irq mode. we've punted on this and it comes back.

@plbossart, the naming wasn't clear to me. Do you think period_irq would be OK?

keyonjie · 2019-06-19T01:39:05Z

@mrajwa please provide the corresponding FW change at the same time, changing host_period_bytes to non-0 in driver only will lead the FW keep sending position IPCs to FW on SKL+, this may confuse driver.

plbossart

@mrajwa this PR as is does not change the behavior, you introduce a new field but don't set it? Also the commit order is inverted.

plbossart · 2019-06-19T15:52:34Z

 	/* set host_period_bytes to 0 if no IPC position */
 	if (hda && hda->no_ipc_position)
-		ipc_params->host_period_bytes = 0;
+		ipc_params->period_irq = 0;


In this series, period_irq is never set to anything different from zero, so why do we need this parameter. Something's missing I guess

Its not used here but FW uses it:

/* NO_IRQ mode if period_irq == 0 */
if (dev->params.period_irq != 0) { ... }
ALthough you won't find it in current master, I will submit a PR to master soon.

Also, please note the main focus of this PR is host_period_bytes. Previously it was set to 0 at kernel side so each time FW wanted to use it read 0 instead of true value. It was set to 0 by kernel because somebody decided to use host_period_bytes as an indicator of some sort of position - now, after this change we will use period_irq for this purpose.

plbossart · 2019-06-19T15:53:20Z

 	/* set host_period_bytes to 0 if no IPC position */
 	if (hda && hda->no_ipc_position)
-		ipc_params->host_period_bytes = 0;
+		ipc_params->period_irq = 0;


Also the field is used before it's added to the header file in the next commit, this would break git bisect. Don't do it.

Yes, good catch thanks.

keyonjie · 2019-06-20T05:27:07Z

In my opinion, @plbossart @mrajwa please consider replacing the change here with something in this commit: 17558e9, that is:

From 17558e9cf7288c1fe50d1f88a191fc6d0fa2666c Mon Sep 17 00:00:00 2001
From: Keyon Jie <yang.jie@linux.intel.com>
Date: Thu, 23 May 2019 13:51:41 +0800
Subject: [PATCH] ASoC: SOF: add flag for position update ipc

In some cases, FW might need use the host_period_bytes even no position
update ipc reqiured from driver, here add another flag for position update,
and preserve host_period_bytes for FW to use.

This might require corresponding FW change and ABI alignment.

Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
---
 include/sound/sof/stream.h    | 5 +++--
 sound/soc/sof/intel/hda-pcm.c | 4 ++--
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/include/sound/sof/stream.h b/include/sound/sof/stream.h
index 643f175cb479a..1637c488184c8 100644
--- a/include/sound/sof/stream.h
+++ b/include/sound/sof/stream.h
@@ -83,10 +83,11 @@ struct sof_ipc_stream_params {
 	uint16_t sample_valid_bytes;
 	uint16_t sample_container_bytes;
 
-	/* for notifying host period has completed - 0 means no period IRQ */
+	/* for notifying host period has completed */
 	uint32_t host_period_bytes;
+	uint32_t no_ipc_position; /* 1 means no position ipc needed */
 
-	uint32_t reserved[2];
+	uint32_t reserved[1];
 	uint16_t chmap[SOF_IPC_MAX_CHANNELS];	/**< channel map - SOF_CHMAP_ */
 } __packed;
 
diff --git a/sound/soc/sof/intel/hda-pcm.c b/sound/soc/sof/intel/hda-pcm.c
index 9b730f1835291..e164dbddc59d5 100644
--- a/sound/soc/sof/intel/hda-pcm.c
+++ b/sound/soc/sof/intel/hda-pcm.c
@@ -116,9 +116,9 @@ int hda_dsp_pcm_hw_params(struct snd_sof_dev *sdev,
 	/* disable SPIB, to enable buffer wrap for stream */
 	hda_dsp_stream_spib_config(sdev, stream, HDA_DSP_SPIB_DISABLE, 0);
 
-	/* set host_period_bytes to 0 if no IPC position */
+	/* update no_ipc_position flag for ipc param */
 	if (hda && hda->no_ipc_position)
-		ipc_params->host_period_bytes = 0;
+		ipc_params->no_ipc_position = 1;
 
 	ipc_params->stream_tag = hstream->stream_tag;

And sure, we need corresponding FW change at the same time.

mrajwa · 2019-06-26T23:37:22Z

I have created corresponding FW PR thesofproject/sof#1592

This functionality has been proven to work during KWD tests.

ranj063 · 2019-07-02T05:55:11Z

@mrajwa can you please rebase and resubmit?

keyonjie · 2019-07-02T11:43:54Z

@ranj063 @mrajwa let's submit it to alsa mailing list. @mrajwa do you need my help on that?

plbossart · 2019-08-13T21:43:18Z

@keyonjie @mrajwa this PR has gone nowhere and the explanations on the alsa-devel mailing list did not exactly shine by their clarity.
If you want this in the kernel you will have to rework this patch and clarify the intended behavior.
Thank you

keyonjie · 2019-09-10T03:07:37Z

@plbossart the new kernel (with this PR merged) will not work correctly with old FW(without Marcin's FW PR merged), even for the normal (not KWD) streams, that means, there will be many position update IPCs sent from FW with normal headset/speaker playback there, which might influence the operation in Linux SOF driver.

We can say we don't have released platform with KWD stuff, but we do have released platforms with other normal streams already, where the kernel won't be backwards compatible with this Kernel PR merged.

So, looks like we need a major version bump, or to use strict ABI match while only bumping minor version?

plbossart · 2019-09-10T13:30:29Z

we need a minor ABI bump and the driver needs to conditionally execute the old code that sets host_period_bytes to zero when the ABI level is lower that e.g. 11.

wenqingfu · 2019-09-19T16:01:47Z

@mrajwa can you please follow up with @plbossart 's comment?

keyonjie · 2019-09-19T23:54:52Z

Hi, @mrajwa you can use something like this:

...
	struct sof_ipc_fw_version *v = &sdev->fw_ready->version;
...
if (hda && hda->no_ipc_position) {
                /* use old host_period_bytes==0 for no position update IPC reqirement */
                if (v->abi_version < SOF_ABI_VER(3, 10, 0))
		        ipc_params->host_period_bytes = 0;
                else /* use new no_stream_position flag */
		        ipc_params->no_stream_position  = 1;

mrajwa · 2019-09-20T10:22:41Z

@wenqingfu, @plbossart @keyonjie I have just updated this patch. Now kernel will be able to handle ABI mismatch if somebody use old FW with newer kernel.

plbossart

not there yet...

plbossart · 2019-09-20T15:29:55Z

+		else
+			ipc_params->no_stream_position = 1;
+	}
+		ipc_params->no_stream_position  = 1;


That looks wrong?

The host period bytes value needs to be passed to firmware. However current implementation uses this field for different purpose - to indicate whether FW should send stream position to the host. Therefore this patch introduces another field "no_stream_position", a boolean value aimed to store information about position tracking. This way host_period_bytes preserves its original value. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

mrajwa · 2019-09-20T16:42:13Z

@plbossart, I have addressed your findings. I dunno how the commit message was affected as I only amended the old one..

plbossart

Thanks @mrajwa
Need 2nd approver to merge later today

keyonjie

looks good, @mrajwa can you bump the FW ABI version to 3.11.0 in your FW PR#1592 also please?

mrajwa · 2019-09-20T17:17:51Z

Looks like the ABI check should be changed here from 3.10.0 to 3.11.0 as current version is 3.10.0 do you agree?

This patch prevents the reset of host period bytes and uses no_stream_position to record requests for stream position. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

keyonjie · 2019-09-20T17:36:27Z

Looks like the ABI check should be changed here from 3.10.0 to 3.11.0 as current version is 3.10.0 do you agree?

Yes, thanks for spotting that.

….c:367 Use irq*_rcu() functions to fix this kernel warning: WARNING: CPU: 0 PID: 0 at kernel/context_tracking.c:367 ct_irq_enter+0xa0/0xd0 Modules linked in: CPU: 0 PID: 0 Comm: swapper/0 Not tainted 6.7.0-rc3-64bit+ thesofproject#1037 Hardware name: 9000/785/C3700 IASQ: 0000000000000000 0000000000000000 IAOQ: 00000000412cd758 00000000412cd75c IIR: 03ffe01f ISR: 0000000000000000 IOR: 0000000043c20c20 CPU: 0 CR30: 0000000041caa000 CR31: 0000000000000000 ORIG_R28: 0000000000000005 IAOQ[0]: ct_irq_enter+0xa0/0xd0 IAOQ[1]: ct_irq_enter+0xa4/0xd0 RP(r2): irq_enter+0x34/0x68 Backtrace: [<000000004034a3ec>] irq_enter+0x34/0x68 [<000000004030dc48>] do_cpu_irq_mask+0xc0/0x450 [<0000000040303070>] intr_return+0x0/0xc Signed-off-by: Helge Deller <deller@gmx.de>

mrajwa requested review from juimonen, keyonjie, lgirdwood, lyakh, plbossart and ranj063 as code owners June 14, 2019 05:16

keyonjie reviewed Jun 14, 2019

View reviewed changes

Comment thread sound/soc/sof/pcm.c Resolved

mrajwa changed the title ~~pcm_params: provide proper period size to FW~~ ASoC: Intel: update pcm_params to provide proper period size to FW Jun 14, 2019

keyonjie suggested changes Jun 14, 2019

View reviewed changes

Comment thread include/sound/sof/stream.h Resolved

mrajwa mentioned this pull request Jun 14, 2019

KWD: Sometimes wait for buffer empty timeout issue happens during draining thesofproject/sof#1406

Closed

plbossart requested changes Jun 14, 2019

View reviewed changes

mrajwa force-pushed the kwd_release branch from 3117541 to e1e324a Compare June 18, 2019 23:24

plbossart requested changes Jun 19, 2019

View reviewed changes

mrajwa force-pushed the kwd_release branch from e1e324a to b65456d Compare June 26, 2019 09:16

mrajwa mentioned this pull request Jun 26, 2019

ipc: align stream.h with kernel thesofproject/sof#1592

Merged

keyonjie reviewed Jun 27, 2019

View reviewed changes

Comment thread include/sound/sof/stream.h Resolved

mrajwa force-pushed the kwd_release branch 3 times, most recently from a110fe8 to 3b35b7e Compare June 28, 2019 07:04

keyonjie reviewed Jun 28, 2019

View reviewed changes

Comment thread include/sound/sof/stream.h Resolved

mrajwa force-pushed the kwd_release branch from 3b35b7e to fd5bcd9 Compare June 28, 2019 07:41

keyonjie previously approved these changes Jul 2, 2019

View reviewed changes

plbossart added the upstream Patch has been sent to upstream (e.g. alsa-devel) label Jul 17, 2019

jajanusz mentioned this pull request Jul 25, 2019

Revert " ipc: align stream.h with kernel" thesofproject/sof#1669

Merged

wenqingfu added the ABI involves ABI changes, need extra attention label Sep 19, 2019

YvonneYang2 mentioned this pull request Sep 19, 2019

[BUG] WoV cannot trigger with topic/sof-dev(kernel) and master(sof) branch thesofproject/sof#1775

Closed

mrajwa dismissed stale reviews from ranj063 and plbossart via dd16988 September 20, 2019 10:35

mrajwa force-pushed the kwd_release branch from f349d13 to dd16988 Compare September 20, 2019 10:35

plbossart requested changes Sep 20, 2019

View reviewed changes

mrajwa force-pushed the kwd_release branch from dd16988 to cf7cd86 Compare September 20, 2019 16:39

plbossart previously approved these changes Sep 20, 2019

View reviewed changes

keyonjie self-requested a review September 20, 2019 17:08

keyonjie previously approved these changes Sep 20, 2019

View reviewed changes

ASoC: SOF: Intel: fix reset of host_period_bytes

55f824d

This patch prevents the reset of host period bytes and uses no_stream_position to record requests for stream position. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

mrajwa dismissed stale reviews from keyonjie and plbossart via 55f824d September 20, 2019 17:35

mrajwa force-pushed the kwd_release branch from cf7cd86 to 55f824d Compare September 20, 2019 17:35

keyonjie requested review from keyonjie, plbossart and ranj063 September 20, 2019 17:37

keyonjie approved these changes Sep 20, 2019

View reviewed changes

plbossart approved these changes Sep 20, 2019

View reviewed changes

plbossart merged commit 29561c6 into topic/sof-dev Sep 20, 2019

Conversation

mrajwa commented Jun 14, 2019

Uh oh!

Uh oh!

keyonjie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

plbossart 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

keyonjie commented Jun 19, 2019

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrajwa Jun 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

keyonjie commented Jun 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrajwa commented Jun 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ranj063 commented Jul 2, 2019

Uh oh!

keyonjie commented Jul 2, 2019

Uh oh!

plbossart commented Aug 13, 2019

Uh oh!

keyonjie commented Sep 10, 2019

Uh oh!

plbossart commented Sep 10, 2019

Uh oh!

wenqingfu commented Sep 19, 2019

Uh oh!

keyonjie commented Sep 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrajwa commented Sep 20, 2019

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mrajwa commented Sep 20, 2019

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

keyonjie left a comment

Choose a reason for hiding this comment

Uh oh!

mrajwa commented Sep 20, 2019

Uh oh!

keyonjie commented Sep 20, 2019

Uh oh!

Reviewers

Uh oh!

mrajwa Jun 25, 2019 •

edited

Loading

keyonjie commented Jun 20, 2019 •

edited

Loading

mrajwa commented Jun 26, 2019 •

edited

Loading

keyonjie commented Sep 19, 2019 •

edited

Loading