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

yaochunhung · 2021-12-28T12:46:52Z

Hi,

I modify host position in every period not only larger than host period bytes. Host position will be updated more precisely. When linux side use pcm pointer callback to get host position data then the position will be updated based on pipeline period.
I also reset host position value as zero before replay ipc_stream_pcm_params to avoid unexpected value. I saw this invalid value when I run alsa conformance test and I implement PCM pointer function to get host position value but see invalid value at the beginning. After I provide this code, the invalid value will be replaced with zero. Please help to review this PR and any feedback are welcome. Thanks.

lgirdwood · 2021-12-28T20:42:28Z

src/audio/host.c

@yaochunhung this is a good fix, but it looks like we can tune it by setting the hd->report_pos to align with the 1ms data size. This is probably something the driver or topology should ask for (as it has impact on power) so could be fine tuned per use case.

@lgirdwood Thanks for your comment. I think this change will update data based on pipeline' period. and it will be set by different use cases(via topology). It will increase some MCPS due to updating host position information(maybe also impact power). Please advice if my opinion is incorrect. Thanks.

yes, I think if we can set the update period via teh driver or topology would be the best way forward. I dont think it will have too much impact on MCPS though, but it could impact power (on Intel and maybe others) as it can write to (and wake) IP blocks.

@lgirdwood Does it mean that we need to define another parameter for updated_period?
I have little bit confusion about power consumption because only pipeline_get_timestamp and mailbox_stream_write without sending ipc to AP side(send ipc is still under checking by host_period_bytes). Will it impact intel platforms? Thanks.

Sorry @yaochunhung I misread the reply. Yes we need a new flag coming from the driver i.e.

/* PCM params info - SOF_IPC_STREAM_PCM_PARAMS */ struct sof_ipc_pcm_params { struct sof_ipc_cmd_hdr hdr; uint32_t comp_id; uint32_t flags; /**< generic PCM flags - SOF_PCM_FLAG_ */ <<<<<<<<<< Can we ad a new flag here for continuous updates uint32_t reserved[2]; struct sof_ipc_stream_params params; } __attribute__((packed, aligned(4)));

@lgirdwood Thanks.
I check sof_ipc_pcm_params is filled in this function sof_pcm_hw_params at linux side and without platform specific callback. Not sure if what you want is to use sof_ipc_stream_params. The sof_ipc_stream_params could be add a flag naming as continuous_update_position(see code below maybe a better name for it) and it also can be decided in the pcm_hw_params callback function by different platforms.

struct sof_ipc_stream_params { struct sof_ipc_hdr hdr; struct sof_ipc_host_buffer buffer; uint32_t direction; /**< enum sof_ipc_stream_direction */ uint32_t frame_fmt; /**< enum sof_ipc_frame */ uint32_t buffer_fmt; /**< enum sof_ipc_buffer_format */ uint32_t rate; uint16_t stream_tag; uint16_t channels; uint16_t sample_valid_bytes; uint16_t sample_container_bytes; uint32_t host_period_bytes; uint16_t no_stream_position; /**< 1 means don't send stream position */ uint16_t continuous_update_position; /**< 1 means continue updating stream position */ <<<<< add a flag for it uint16_t reserved[2]; uint16_t chmap[SOF_IPC_MAX_CHANNELS]; /**< channel map - SOF_CHMAP_ */ } __packed;

and this flag is used for conditional checking

+ if (hd->continuous_update_position == 1) { pipeline_get_timestamp(dev->pipeline, dev, &hd->posn); mailbox_stream_write(dev->pipeline->posn_offset, &hd->posn, sizeof(hd->posn)); + }

@lgirdwood Thanks for suggestion. I add a PR to add this flag in kernel first. thesofproject/linux#3353 Please help to check if it is ok. Thanks!

@lgirdwood Do I need to modify abi.h because it looks like there is a checkpatch warning as below? If yes, how to modify it? Thanks.
https://sof-ci.01.org/sofpr/PR5137/build11487/checkpatch/

WARNING: Please update ABI in accordance with http://semver.org #72: FILE: src/include/ipc/stream.h:97: uint16_t no_stream_position; /**< 1 means don't send stream position */ + uint16_t cont_update_posn; /**< 1 means continue update stream position */

yes, we need to label the PR with the ABI flag - that way everyone knows there is ABI change. I've marked both PRs with ABI label now.

@lgirdwood I also update abi version to ABI-3.21. thanks

lyakh · 2021-12-29T06:53:22Z

src/ipc/ipc3/handler.c

Could you describe in which situations you see invalid position data being delivered to Linux?

@lyakh yes,I saw this invalid value when I run alsa conformance test and I implement PCM pointer function to get host position value but see invalid value at the beginning. After I provide this code, the invalid value will be replaced with zero. Not sure if this issue could be reproducible on Intels platforms because it looks like Intel uses different ways to get position at Linux side. thanks.

got it, thanks!

src/audio/host.c

dbaluta · 2022-01-04T15:36:05Z

@yaochunhung looks good to me. I think we also need to update the ABI minor version. @lgirdwood ?

ranj063 · 2022-01-04T16:42:13Z

src/include/ipc/stream.h

nit-pick: continuous instead of continue maybe?

@ranj063 I will fix it. Thanks

lgirdwood · 2022-01-05T14:55:35Z

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

yaochunhung · 2022-01-07T00:54:02Z

@lgirdwood I have updated version 3.21. please help to review it. Thanks.

ujfalusi · 2022-01-10T09:28:46Z

src/audio/host.c

+			pipeline_get_timestamp(dev->pipeline, dev, &hd->posn);
+			mailbox_stream_write(dev->pipeline->posn_offset,
+					     &hd->posn, sizeof(hd->posn));
+		}


The hd->report_pos is not used if hd->cont_update_posn == 1

if (!hd->no_stream_position) { hd->report_pos += bytes; if (hd->cont_update_posn) { /* * update position every period to support host query to get the * latest value if cont_update_posn == 1. */ pipeline_get_timestamp(dev->pipeline, dev, &hd->posn); mailbox_stream_write(dev->pipeline->posn_offset, &hd->posn, sizeof(hd->posn)); } else if (hd->host_period_bytes != 0 && hd->report_pos >= hd->host_period_bytes) { /* * host_period_bytes is set to zero to disable position update * by IPC for FW version before 3.11, so send IPC message to * driver according to this condition and report_pos. */ 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); } }

But I would be tempted to move the hd->report_pos += bytes; to the else case as with continuous update you do not even need to increment it.

What would be the side effect if this is done unconditionally, without a flag from the host?

@ujfalusi hd->report_pos is still needed when hd->cont_update_posn == 1 because when hd->cont_update_posn is set as '1', only update host position to mailbox buffer without sending ipc to notify the host.
About the side effect without a flag from the host,
Please reference the discussion as the link #5137 (comment). @lgirdwood has the suggestion about it. Thanks

@yaochunhung, I see. so this will update the position data in between IPCs, no_stream_position == 0 -> the position IPC is enabled.
How is the position updated in case of `no_stream_position == 1'?

Please see my review comment, I think we should clean up the semantics, the old fields are quite hard to follow (and worse if we add more fields on top). I think host should still have option to prevent both mailbox and IPC updates (traffic to mailbox is non-zero cost on many platforms so if host can get position info via other means, it probably doesn't need the mailbox traffic -> the case on Intel platforms)

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.

src/audio/host.c

lgirdwood · 2022-01-18T12:20:55Z

@yaochunhung any update, are we aligned kernel ?

yaochunhung · 2022-01-19T00:29:09Z

@lgirdwood There are still some discussion in the kernel PR thesofproject/linux#3353 . I will check and give feedback on it. thanks.

plbossart · 2022-01-21T00:09:20Z

src/audio/host.c

+			pipeline_get_timestamp(dev->pipeline, dev, &hd->posn);
+			mailbox_stream_write(dev->pipeline->posn_offset,
+					     &hd->posn, sizeof(hd->posn));
+		}


I have tried multiple times to understand this PR, and I really don't get the idea.

If the idea is to send an IPC back to the host to update the timestamps every time a host DMA block is handled by the firmware, this is a VERY BAD IDEA.

if e.g. the host component retrieves blocks of 1ms, then you will have 1000 interrupts per second to update a timestamp, but the .pointer on the kernel side only needs this information on a period_elapsed event, or on a snd_pcm_status request.

It would be a much better idea to cache the timestamp in a memory window shared with the host and have the driver read from it.

@plbossart I don't change IPC(interrupt) in this PR. I still keep the conditional check in the if condition below, which still depends on host_period_bytes(period size from HOST side,not host component period in DSP firmware side), I only want to update host position data in host component period(1ms) but doensn't send IPC to kernel. Thanks.

if (!hd->no_stream_position) { hd->report_pos += bytes; + if (hd->cont_update_posn) { + /* + ***** update position every period to support host query to get the + * latest value if cont_update_posn == 1.**** + */ + pipeline_get_timestamp(dev->pipeline, dev, &hd->posn); + mailbox_stream_write(dev->pipeline->posn_offset, + &hd->posn, sizeof(hd->posn)); + } if (hd->host_period_bytes != 0 && hd->report_pos >= hd->host_period_bytes) { /* * host_period_bytes is set to zero to disable position update * by IPC for FW version before 3.11, so send IPC message to * driver according to this condition and report_pos. */ 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); <==Send IPC here, it depend on host_period_bytes } }

ok, if you update the position in a mailbox that's fine, but then why do you initiate an IPC in your .pointer function in the kernel? You are not using the mailbox information, are you? I really don't see how the two sides work together.

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 yes, I write host position into mailbox information on DSP firmware, so I get the information in .pointer callback function mtk_adsp_pcm_pointer. Thanks.

host.c mailbox_stream_write(dev->pipeline->posn_offset, &hd->posn, sizeof(hd->posn));

Ack @plbossart , the "ipc_msg_data" will read from the mailbox (not send an IPC) on all platforms. Most use the common implementation which is a call to snd_sof_dsp_mailbox_read().

So basicly this adds a new variable to the interface to configuration position updates:

should position in mailbox be updated at a) DSP tick rate, b) host period rate or c) not at all

should IPC be sent at a) DSP tick rate, b) host period rate, c) not at all

Sending IPC without mailbox update makes no sense, so 2 depends on 1.

On Intel platforms, we use 1c/2c for production, but allow 1b+2b for debug.

@yaochunhung is seeing issues for production and would like the ability to configure 1a+2b (or 1a+2c ?) for mtk platforms.

Updating mailbox more often has a performance impact, but I think the SOF interface should allow this.

I am still confused, 'host_period_rate' means what?
a) the ALSA period in the DDR ring buffer
b) the size of the host buffer component in the firmware?

if this is b), then I maintain that 2b) is crazy.
if this is a), then I don't see how the precision is increased.

@plbossart it is a), and we update mailbox data in b) when add the codes below,

host.c in DSP firmware + if (hd->cont_update_posn) { + /* + ***** update position every period to support host query to get the + * latest value if cont_update_posn == 1.**** + */ + pipeline_get_timestamp(dev->pipeline, dev, &hd->posn); + mailbox_stream_write(dev->pipeline->posn_offset, + &hd->posn, sizeof(hd->posn)); + }

then it will be gotten in .pointer callback when call snd_sof_ipc_msg_data function as @kv2019i mentioned. thanks.

@plbossart So it's the ALSA period_size. Whenever application calls snd_pcm_avail() (or related calls), this will do a hwsync, , and end up calling pointer() SOF DSP ops. For MTK, this would not use a MMIO or info from most recently sent position IPC, but rather read the position from mailbox directly (i.e. mapped DSP memory). Main drawback I see is the high amount of SRAM write traffic on DSP side, but other than that, this should get better accuracy.

ok that makes sense, but now I am confused on what we do today before this PR? Is this saying that when a period_elapsed event is signaled, the firmware does not have the latest information?

In other words, I wonder if this 'new' behavior should have been the default behavior from the start, and if we need the new flag in the IPC structure or just an unconditional change in the firmware?

@plbossart As @kv2019i mentioned, there are two ways to update position information. one is (A). period_elapsed event and another is (B).application call snd_pcm_avail(or related calls). For case (A), there is no change with/without this PR. For case (B), firmware will have the latest information with this PR. alsa_conformance_test will use both case (A) and (B). I set a new flag "cont_update_posn" in IPC structure "sof_ipc_stream_params " so that user can decide to use this new behavior on their platforms or not. Thanks.

yaochunhung · 2022-01-25T15:54:42Z

@plbossart @kv2019i @ujfalusi Thanks for review and comments. Please let me know if this PR is ok or I need any improvement on it. Thanks.

plbossart

please rebase and revisit the commit message for clarify, thank you @yaochunhung

src/audio/host.c

RanderWang

it is developed as ipc4 now

plbossart · 2022-01-26T15:56:33Z

src/audio/host.c

I am a bit confused now on the relationship between 'no_stream_position' and 'cont_update_posn'.

Is there really a hierarchy between the two, or could 'cont_update_posn' be enabled when we don't send any IPC?

@plbossart good question. from the description of no_stream_position is "1 means don't send stream position". In original design, IPC should be sent when no_stream_position is zero and satisfy the condition below,

"if (hd->host_period_bytes != 0 && hd->report_pos >= hd->host_period_bytes)"

if no_stream_position is set as 1, that means host won't get stream position from DSP firmware. Maybe cont_update_posn will be no used since host won't get stream from DSP firmware? Thanks.

I wonder if there is any case that needs to update stream position from DSP firmware without IPC requirement because DSP send IPC : SOF_IPC_STREAM_POSITION to notify kernel then call ipc_period_elapsed to update stream position and call snd_sof_pcm_period_elapsed for period wakeup mode.

The position can be available to host in 4 ways today.

Host sends IPC (latency adds some variability here - not very accurate)

FW sends notifications to host (more accurate than 1 since its a one way message, but still subject to host latencies when receiving the notification).

FW writes to shared SRAM for every copy(). Accurate to 1ms (LL scheduling tick)

Host reads from HW position registers (Intel HW can do this today).

@lgirdwood Thanks for detail explanation. I have some questions below,
Q1. Does any platform use method 1 since it is not accurate?
Q2. Does Method 3 exist in current design or it is the same as this PR do?
Q3. Can Method 3 be used alone? I think we need one way to notify Host to get position(e.g method 2)?
Thanks a lot.

@plbossart Thanks for providing suggestion to ensure timestamp query are unified. But I am not sure if it is fine to move the code below to the no_stream_position case(outside of the if(!hd->no_stream_position))

+ hd->report_pos += bytes; + if (hd->cont_update_posn) + update_mailbox = true;

because hd->report_pos is circular and will be reset as zero under the condition. thanks.

if (hd->host_period_bytes != 0 && hd->report_pos >= hd->host_period_bytes) { hd->report_pos = 0;

I can't figure out this code, it's just convoluted for no good reason. @lgirdwood can you take a look?

@lgirdwood Sorry to bother. Could you please help to check it and give suggestion if any? Thanks.

@yaochunhung apologies, I did not see the question.
It looks like to me that we only need the

+ if (update_mailbox) { + pipeline_get_timestamp(dev->pipeline, dev, &hd->posn); + mailbox_stream_write(dev->pipeline->posn_offset, + &hd->posn, sizeof(hd->posn));

We can write to the mailbox on every pipeline time tick. However, this should be a topology option "update mailbox position" flag (just like the no position update flag). Would this work for you to read from mailbox ? This would mean no IPC traffic and host could read when it pleased ?

@lgirdwood @plbossart Thanks for comment. I provide this patch and it looks like fine for me. I will provide a update for review. Thanks.

The position is currently updated by the host component when a period was consumed. The granularity of this position is too large, applications relying on timer-based scheduling may query the position at any time and conformance test suites fail. This patch introduce another field "cont_update_posn" in sof_ipc_stream_params which suggests a more precise position reporting, by storing the latest position in a memory window when the host component completes a transfer. This will increase the traffic to memory windows, but allow for a much smaller granularity in position updates. In hindsight, this solution should have been the default behavior but to avoid backwards compatibility issues is added as a new capability controlled by an ABI check. Only with a recent-enough Linux kernel will this behavior be enabled. Signed-off-by: YC Hung <yc.hung@mediatek.com>

Reset host position in ipc_stream_pcm_params before reply. It could avoid that linux side get invalid value before host position updating. This invalid value happens when running alsa conformance test and use pcm pointer function to get host position value but see invalid value at the beginning. After providing this patch, the invalid value will be replaced with zero. Signed-off-by: YC Hung <yc.hung@mediatek.com>

lgirdwood

@plbossart good for kernel side ?
@yaochunhung this will make v2.1 since it's default state is OFF and only enabled by topology.

dbaluta · 2022-03-02T15:43:47Z

src/include/kernel/abi.h

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



@yaochunhung @lgirdwood what is SOF_ABI_PATCH and why is decremented?

@dbaluta see https://semver.org/

plbossart

Sorry I missed the update, thanks @lgirdwood for the ping.

yaochunhung requested review from dbaluta, lbetlej, lgirdwood, mmaka1 and plbossart as code owners December 28, 2021 12:46

yaochunhung requested review from afq984 and kuanhsuncheng December 28, 2021 12:47

yaochunhung force-pushed the host-position-refine branch from c2c7907 to cd72eaf Compare December 28, 2021 13:01

lgirdwood reviewed Dec 28, 2021

View reviewed changes

lyakh reviewed Dec 29, 2021

View reviewed changes

yaochunhung mentioned this pull request Jan 4, 2022

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

Merged

yaochunhung force-pushed the host-position-refine branch 3 times, most recently from aba8c3d to 1e1d8d9 Compare January 4, 2022 11:25

dbaluta reviewed Jan 4, 2022

View reviewed changes

src/audio/host.c Outdated Show resolved Hide resolved

dbaluta reviewed Jan 4, 2022

View reviewed changes

src/audio/host.c Outdated Show resolved Hide resolved

yaochunhung force-pushed the host-position-refine branch 2 times, most recently from 44ba555 to b0e38c5 Compare January 4, 2022 14:32

ranj063 reviewed Jan 4, 2022

View reviewed changes

yaochunhung force-pushed the host-position-refine branch from b0e38c5 to bb43bec Compare January 5, 2022 00:56

lgirdwood added the ABI ABI change involved label Jan 5, 2022

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

yaochunhung force-pushed the host-position-refine branch from bb43bec to af602b1 Compare January 5, 2022 15:49

ujfalusi requested changes Jan 10, 2022

View reviewed changes

kv2019i requested changes Jan 10, 2022

View reviewed changes

src/audio/host.c Show resolved Hide resolved

plbossart requested changes Jan 21, 2022

View reviewed changes

plbossart requested changes Jan 25, 2022

View reviewed changes

src/audio/host.c Show resolved Hide resolved

yaochunhung force-pushed the host-position-refine branch from 2a0c3b4 to a7e7be7 Compare January 26, 2022 04:05

yaochunhung requested review from RanderWang and marcinszkudlinski as code owners January 26, 2022 04:05

RanderWang approved these changes Jan 26, 2022

View reviewed changes

plbossart reviewed Jan 26, 2022

View reviewed changes

yaochunhung added 2 commits February 21, 2022 21:08

yaochunhung force-pushed the host-position-refine branch from a7e7be7 to 6589c49 Compare February 21, 2022 13:09

lgirdwood approved these changes Mar 2, 2022

View reviewed changes

dbaluta reviewed Mar 2, 2022

View reviewed changes

plbossart approved these changes Mar 2, 2022

View reviewed changes

lgirdwood merged commit ba29bdc into thesofproject:main Mar 3, 2022

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

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

Uh oh!

Conversation

yaochunhung commented Dec 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

yaochunhung Jan 3, 2022 • 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!

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!

yaochunhung Dec 29, 2021 • 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!

Uh oh!

Uh oh!

dbaluta commented Jan 4, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgirdwood commented Jan 5, 2022

Uh oh!

yaochunhung commented Jan 7, 2022

Uh oh!

ujfalusi Jan 10, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lgirdwood commented Jan 18, 2022

Uh oh!

yaochunhung commented Jan 19, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yaochunhung Jan 21, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

yaochunhung commented Dec 28, 2021 •

edited

Loading

yaochunhung Jan 3, 2022 •

edited

Loading

yaochunhung Dec 29, 2021 •

edited

Loading

ujfalusi Jan 10, 2022 •

edited

Loading

yaochunhung Jan 21, 2022 •

edited

Loading

yaochunhung Jan 26, 2022 •

edited

Loading

yaochunhung Jan 27, 2022 •

edited

Loading