ASoC: SOF: ipc: introduce cont_update_posn in sof_ipc_stream_params s… #3353

yaochunhung · 2022-01-04T02:19:14Z

…truct

The host stream position is updated when no_stream_position is set as 0.
However current implementation updates host stream position only when report
data is larger than or equal to host period size which is decided by the
period size of host side. It maybe cause host stream position update not
in time. Therefore this patch introduces another field "cont_update_posn",
a boolean value aimed to update host stream position continuously and based
on period size of pipeline. It can get better precise when need to update
host stream position from firmware.

Signed-off-by: YC Hung yc.hung@mediatek.com

yaochunhung · 2022-01-04T02:21:20Z

Please reference the discussion in thesofproject/sof#5137 (comment) Thank.

dbaluta · 2022-01-04T09:38:18Z

include/sound/sof/stream.h

@@ -85,8 +85,9 @@ struct sof_ipc_stream_params {

 	uint32_t host_period_bytes;
 	uint16_t no_stream_position; /**< 1 means don't send stream position */
+	uint16_t cont_update_posn; /**< 1 means continue update stream position */



Could we just take only 1 byte from reserved area. We are wasting so much space to keep a boolean information.

@dbaluta yes, I could modify it as one byte. Thanks

We will set the cont_update_posn to one in kernel unconditionally or what is the condition to set it?
Iow, will the use of this new flag going to be:

+ if (v->abi_version >= SOF_ABI_VER(3, 21, 0)) + msg->cont_update_posn = 1;

If it is then I argue that there is no point of having this flag in the first place and just do it unconditionally in firmware?

@ujfalusi Currently I propose to set it in the stream callback function pcm_hw_params e.g.

#define CONTINUOUS_UPDATE_POSITION 1 static int mt8195_dsp_pcm_hw_params(struct snd_sof_dev *sdev, struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params, struct sof_ipc_stream_params *ipc_params) { struct sof_ipc_fw_version *v = &sdev->fw_ready.version; if (v->abi_version >= SOF_ABI_VER(3, 21, 0)) ipc_params->cont_update_posn = CONTINUOUS_UPDATE_POSITION; return 0; }

I think this should be the default across all platforms, I don't really see a reason why this capability needs to be added 4 times when we all have to support the Chrome conformance suite. @cujomalainey any comments?

agreed, given we know the current default does not pass the test, lets push this into the core and replace the existing default

@yaochunhung can you please add a user for this new capability?

lgirdwood · 2022-01-05T13:25:45Z

Corresponding FW PR is thesofproject/sof#5137

lgirdwood · 2022-01-05T14:56:22Z

@yaochunhung ABI is 3.21, this will need updated in header file.

yaochunhung · 2022-01-05T16:04:50Z

@lgirdwood The original abi in kernel looks like different from firmware(kernel only 3.18, firmware 3.20). Should I need to make them consistent? Thanks
https://github.com/thesofproject/linux/blob/topic/sof-dev/include/uapi/sound/sof/abi.h#L29

yaochunhung · 2022-01-07T06:05:00Z

@lgirdwood I have updated abi version v3.21. please help to check it. thanks.

kv2019i · 2022-01-10T08:40:11Z

@yaochunhung wrote:

@lgirdwood The original abi in kernel looks like different from firmware(kernel only 3.18, firmware 3.20). Should I need to make them consistent? Thanks https://github.com/thesofproject/linux/blob/topic/sof-dev/include/uapi/sound/sof/abi.h#L29

The kernel side header is currently a bit under discussion, see #3298
and #3296 .

~~Let's try to merge PR3298 first and then you can bump to 3.21 in this PR.~~
@yaochunhung I have up on PR3296. Please just bump the version to 3.21 in your PR directly.

include/sound/sof/stream.h

kv2019i

Thanks @yaochunhung , this looks like a good addition to the IPC interface. I have some comments on how this is implemented, please see inline.

plbossart · 2022-01-10T22:39:24Z

@yaochunhung I am not sure I understand the ask, and this seems to be an IPC interface change without the matching firmware changes?

If the idea was to have the firmware send an IPC message, hence an interrupt, on a very frequent basis, then this idea will by construction get in the way of power savings. You might want to use the memory windows or some sort of shared SRAM to provide the precise position instead. It's my understanding that ChromeOS relies on the no-interrupt mode when available, so it's be rather odd to have an interrupt added just to add more precision to the elapsed time?

yaochunhung · 2022-01-11T08:26:11Z

@plbossart I only update IPC message more frequently and send interrupt to host based on original host period size. Please see firmware patch thesofproject/sof@af602b1
In host side, I implement pcm pointer callback as below(Sorry I didn't send this PR yet due to it is related with #3258 since I need to modify mt8195_ipc_msg_data)

snd_pcm_uframes_t mtk_adsp_pcm_pointer(struct snd_sof_dev *sdev,
				       struct snd_pcm_substream *substream)
{
	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
	struct snd_soc_component *scomp = sdev->component;
	struct snd_sof_pcm *spcm;
	struct sof_ipc_stream_posn posn;
	struct snd_sof_pcm_stream *stream;
	snd_pcm_uframes_t pos;
	int ret;

	spcm = snd_sof_find_spcm_dai(scomp, rtd);
	if (!spcm) {
		dev_warn_ratelimited(sdev->dev, "warn: can't find PCM with DAI ID %d\n",
				     rtd->dai_link->id);
		return 0;
	}

	stream = &spcm->stream[substream->stream];
	ret = snd_sof_ipc_msg_data(sdev, stream->substream, &posn, sizeof(posn));
	if (ret < 0) {
		dev_warn(sdev->dev, "failed to read stream position: %d\n", ret);
		return 0;
	}

	memcpy(&stream->posn, &posn, sizeof(posn));
	pos = spcm->stream[substream->stream].posn.host_posn;
	pos = bytes_to_frames(substream->runtime, pos);

	return pos;
}

plbossart · 2022-01-11T16:20:36Z

@plbossart I only update IPC message more frequently and send interrupt to host based on original host period size. Please see firmware patch thesofproject/sof@af602b1 In host side, I implement pcm pointer callback as below(Sorry I didn't send this PR yet due to it is related with #3258 since I need to modify mt8195_ipc_msg_data)

I don't follow at all, your reply mentions both interrupts generated by the firmware and IPC initiated by the kernel. which is it?
What does 'original host period size' mean? We don't modify period sizes in SOF, we take what the application specified. Are you talking about the firmware host period, which represents a buffer in DSP memory, or a kernel period, which represents a buffer in system DDR?

if you only send an IPC from the kernel to the firmware on a .pointer callback, then what does this patch actually do?

conversely, if the firmware can send notifications to the kernel on a position update, then why would you need to request the position on a .pointer as well?

Too many questions as you can see...

yaochunhung · 2022-01-20T09:17:17Z

@plbossart For mt8195, I set no_stream_position as zero and DSP will send stream position in the host_update_position function. where I said "original host period size" is the host_period_bytes which is set in host_params(the value is sent from host side, sof_pcm_hw_params: pcm.params.host_period_bytes = params_period_bytes(params);)

=== DSP firmware part ===
src/audio/host.c
		if (hd->host_period_bytes != 0 &&
		    hd->report_pos >= hd->host_period_bytes) {
			hd->report_pos = 0;

			/* send timestamped position to host
			 * (updates position first, by calling ops.position())
			 */
			pipeline_get_timestamp(dev->pipeline, dev, &hd->posn);
			mailbox_stream_write(dev->pipeline->posn_offset,
					     &hd->posn, sizeof(hd->posn));
			ipc_msg_send(hd->msg, &hd->posn, false);
		}

In the .pointer callback function, I use snd_sof_ipc_msg_data to read the host stream position.

===kernel part===
sound\soc\sof\mediatek\mt8195\mt8195.c)
static int mt8195_ipc_msg_data(struct snd_sof_dev *sdev,
				struct snd_pcm_substream *substream,
				void *p, size_t sz)
{
	if (!substream || !sdev->stream_box.size) {
		sof_mailbox_read(sdev, sdev->dsp_box.offset, p, sz);
	} else {
		struct sof_mtk_adsp_stream *mstream = substream->runtime->private_data;

		/* The stream might already be closed */
		if (!mstream)
			return -ESTRPIPE;

		sof_mailbox_read(sdev, mstream->stream.posn_offset, p, sz);
	}

	return 0;
}

For why need to provide this callback function. It is because when I test alsa_conformance_test, I found there are two points to update hw pointer. One is snd_pcm_period_elapsed, and it is called by IPC notification from DSP(SOF_IPC_STREAM_POSITION). Another is called from snd_pcm_common_ioctl(snd_pcm_hwsync) and it is not triggered by DSP IPC notification(Maybe somewhere in the host but I am not familiar with alsa_conformane_test codes and not sure how it can trigger snd_pcm_hwsync.) Therefore I implement this .pointer callback function to get more precisive value from DSP side. Thanks.

plbossart

Not getting the idea, see comments on the firmware side. You have not addressed my concerns on the number of interrupts generated by the firmware.

yaochunhung · 2022-04-12T13:17:59Z

@lgirdwood I have rebase to resolve the conflicts. Thanks.

lgirdwood · 2022-04-14T11:10:02Z

@plbossart @ranj063 @ujfalusi is there anything blocking this now from kernel side ?

plbossart · 2022-04-14T15:32:57Z

@plbossart @ranj063 @ujfalusi is there anything blocking this now from kernel side ?

there's an unexplained jump from ABI 19 to 21, and no user of that new flag...

dbaluta · 2022-04-14T15:41:42Z

there's an unexplained jump from ABI 19 to 21, and no user of that new flag...

FW ABI bump that we forgot to expand to Linux kernel ABI is this:

commit f6c48532717220a9390e664d6c73e7e6df944206
Author: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Date:   Wed Oct 6 10:12:48 2021 -0700

    IPC: DAI: Expand DAI_CONFIG IPC flags
    
    Some DAI components, such as HDaudio, need to be stopped in two steps
    a) stop the DAI component
    b) stop the DAI DMA
    
    This patch enables this two-step stop by expanding the DAI_CONFIG
    IPC flags and split them into 2 parts.
    
    The 4 LSB bits indicate when the DAI_CONFIG IPC is sent, ex: hw_params,
    hw_free or pause. The 4 MSB bits are used as the quirk flags to be used
    along with the command flags. The quirk flag called
    SOF_DAI_CONFIG_FLAGS_2_STEP_STOP shall be set along with the HW_PARAMS
    command flag, i.e. before the pipeline is started so that the stop/pause
    trigger op in the FW can take the appropriate action to either
    perform/skip the DMA stop. If set, the DMA stop will be executed when the
    DAI_CONFIG IPC is sent during hw_free. In the case of pause, DMA pause
    will be handled when the DAI_CONFIG IPC is sent with the PAUSE command
    flag.
    
    Along with this, modify the signature for the hda_ctrl_dai_widget_setup/
    hda_ctrl_dai_widget_free() functions to take additional flags as an
    argument and modify all users to pass the appropriate quirk flags. Only
    the HDA DAI's need to pass the SOF_DAI_CONFIG_FLAGS_2_STEP_STOP quirk
    flag during hw_params to indicate that it supports two-step stop and
    pause.
    
    Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

diff --git a/src/include/kernel/abi.h b/src/include/kernel/abi.h
index b9847c7de..c9db26448 100644
--- a/src/include/kernel/abi.h
+++ b/src/include/kernel/abi.h
@@ -29,7 +29,7 @@
 
 /** \brief SOF ABI version major, minor and patch numbers */
 #define SOF_ABI_MAJOR 3
-#define SOF_ABI_MINOR 19
+#define SOF_ABI_MINOR 20
 #define SOF_ABI_PATCH 0

lgirdwood · 2022-04-14T21:13:52Z

there's an unexplained jump from ABI 19 to 21, and no user of that new flag...

FW ABI bump that we forgot to expand to Linux kernel ABI is this:
commit f6c48532717220a9390e664d6c73e7e6df944206
Author: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Date:   Wed Oct 6 10:12:48 2021 -0700

@ranj063 IIRC this was all part of the DMA flow update for PM (which is now all working), could we be only missing the ABI vesion update here ?

dbaluta · 2022-04-20T16:14:28Z

@yaochunhung can you make a summary of this PR? Is it ready for merge? Are there any open issues?

lgirdwood · 2022-04-20T19:00:27Z

@yaochunhung can you make a summary of this PR? Is it ready for merge? Are there any open issues?

Ack - lest unblock this ASAP, all ABI is blocked on this.

plbossart · 2022-04-20T19:10:27Z

once #3603 is merged, this PR can be rebased and merged.

plbossart · 2022-04-21T13:24:31Z

@lgirdwood @kv2019i @ujfalusi can you approve #3603 so that this PR can proceed.

yaochunhung · 2022-04-22T02:22:21Z

@lgirdwood @plbossart @dbaluta, I rebase this PR, please help to check it. thanks.

miRoox · 2022-04-22T09:10:19Z

SOFCI TEST

plbossart · 2022-04-22T14:27:28Z

include/uapi/sound/sof/abi.h

-#define SOF_ABI_MINOR 20
-#define SOF_ABI_PATCH 1
+#define SOF_ABI_MINOR 21
+#define SOF_ABI_PATCH 0


sorry, I screwed up and should have reset this patch level earlier. I added #3610 to fix this, you'll have to rebase when it's merged. My bad.

@plbossart Never mind, I will rebase this PR when #3610 is merged. Thanks.

@plbossart I rebase this PR, thanks.

…truct The host stream position is updated when no_stream_position is set as 0. However current implementation updates host stream position only when report data is larger than or equal to host period size which is decided by the period size of host side. It maybe cause host stream position update not in time. Therefore this patch introduces another field "cont_update_posn", a boolean value aimed to update host stream position continuously and based on period size of pipeline. It can get better precise when need to update host stream position from firmware. Signed-off-by: YC Hung <yc.hung@mediatek.com>

yaochunhung · 2022-04-29T05:25:02Z

@lgirdwood @plbossart @dbaluta Please help to check if this PR is good to merge? Thanks.

dbaluta · 2022-05-02T07:46:33Z

@lgirdwood there is a boot failure for ADLP not sure it this PR has any thing to do with it.

dbaluta · 2022-05-03T08:28:36Z

@kv2019i ok with you?

yaochunhung requested review from juimonen, keyonjie, lyakh and ranj063 as code owners January 4, 2022 02:19

yaochunhung requested a review from lgirdwood January 4, 2022 02:19

yaochunhung force-pushed the sof-continuous-update-flag branch from f735554 to 3e154f1 Compare January 4, 2022 02:27

yaochunhung mentioned this pull request Jan 4, 2022

Update host position value in every period to improve precision thesofproject/sof#5137

Merged

dbaluta reviewed Jan 4, 2022

View reviewed changes

yaochunhung force-pushed the sof-continuous-update-flag branch 2 times, most recently from 93e9868 to 0e4fe79 Compare January 5, 2022 00:59

lgirdwood added the ABI involves ABI changes, need extra attention label Jan 5, 2022

lgirdwood added this to ABI MINOR in ABI3 Change Classifier - MAKE SURE KERNEL AND FW PRs ALIGNED PRIOR TO MERGE Jan 5, 2022

lgirdwood added this to the ABI-3.21 milestone Jan 5, 2022

yaochunhung force-pushed the sof-continuous-update-flag branch from 0e4fe79 to 0a9ab54 Compare January 6, 2022 02:28

kv2019i requested review from ujfalusi and jsarha January 7, 2022 16:52

kv2019i reviewed Jan 10, 2022

View reviewed changes

include/sound/sof/stream.h Show resolved Hide resolved

kv2019i requested changes Jan 10, 2022

View reviewed changes

yaochunhung force-pushed the sof-continuous-update-flag branch from 0a9ab54 to f837de3 Compare January 20, 2022 09:25

plbossart requested changes Jan 21, 2022

View reviewed changes

ranj063 previously approved these changes Apr 14, 2022

View reviewed changes

lgirdwood added the P1 Blocker bugs or important features label Apr 20, 2022

yaochunhung dismissed ranj063’s stale review via e2a32dd April 22, 2022 00:31

yaochunhung force-pushed the sof-continuous-update-flag branch from 3fcd766 to e2a32dd Compare April 22, 2022 00:31

plbossart requested changes Apr 22, 2022

View reviewed changes

yaochunhung force-pushed the sof-continuous-update-flag branch from e2a32dd to 9b9d863 Compare April 26, 2022 05:47

dbaluta approved these changes Apr 29, 2022

View reviewed changes

plbossart approved these changes Apr 29, 2022

View reviewed changes

plbossart requested review from cujomalainey, kv2019i, ranj063 and lgirdwood April 29, 2022 14:37

ranj063 approved these changes Apr 29, 2022

View reviewed changes

kv2019i approved these changes May 3, 2022

View reviewed changes

plbossart merged commit 3ea17bd into thesofproject:topic/sof-dev May 3, 2022

ASoC: SOF: ipc: introduce cont_update_posn in sof_ipc_stream_params s… #3353

ASoC: SOF: ipc: introduce cont_update_posn in sof_ipc_stream_params s… #3353

Conversation

yaochunhung commented Jan 4, 2022 • edited

yaochunhung commented Jan 4, 2022

Choose a reason for hiding this comment

yaochunhung Jan 4, 2022 • edited

Choose a reason for hiding this comment

ujfalusi Jan 10, 2022 • edited

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 Jan 5, 2022

lgirdwood commented Jan 5, 2022

yaochunhung commented Jan 5, 2022 • edited

yaochunhung commented Jan 7, 2022

kv2019i commented Jan 10, 2022 • edited

kv2019i left a comment

Choose a reason for hiding this comment

plbossart commented Jan 10, 2022

yaochunhung commented Jan 11, 2022

plbossart commented Jan 11, 2022

yaochunhung commented Jan 20, 2022

plbossart left a comment

Choose a reason for hiding this comment

yaochunhung commented Apr 12, 2022

lgirdwood commented Apr 14, 2022

plbossart commented Apr 14, 2022

dbaluta commented Apr 14, 2022

lgirdwood commented Apr 14, 2022

dbaluta commented Apr 20, 2022

lgirdwood commented Apr 20, 2022

plbossart commented Apr 20, 2022

plbossart commented Apr 21, 2022

yaochunhung commented Apr 22, 2022

miRoox commented Apr 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaochunhung commented Apr 29, 2022

dbaluta commented May 2, 2022

dbaluta commented May 3, 2022

yaochunhung commented Jan 4, 2022 •

edited

yaochunhung Jan 4, 2022 •

edited

ujfalusi Jan 10, 2022 •

edited

yaochunhung commented Jan 5, 2022 •

edited

kv2019i commented Jan 10, 2022 •

edited