topology: update all HDA pipelines to use timer by tlauda · Pull Request #1131 · thesofproject/sof

tlauda · 2019-03-14T16:53:14Z

Updates all HDA pipelines in topologies to use timer as a scheduling source.

ranj063 · 2019-03-14T17:40:27Z

@tlauda the changes look good but can we please have a round of regression testing on all affected topologies before merging this

tlauda · 2019-03-14T21:53:55Z

@ranj063 @lgirdwood Also it needs to be mentioned that for the full functionality to work we need #1130, #1125 and #1089.

lgirdwood · 2019-03-15T11:43:34Z

This would be a lot better if this was SCHEDULE_SOURCE where various SCHEDULE_SOURCE_s can be defined in schedule.m4. i.e.

# list of generic scheduling sources define (SCHEDULE_SOURCE_DMA0, 0) ... define (SCHEDULE_SOURCE_TIMER0, 100) define (SCHEDULE_SOURCE_TIMER1, 101)

This would be then be made platform specific in apl.m4

define (SCHEDULE_SOURCE_HPGPDMA0, SCHEDULE_SOURCE_DMA0)

This then allows platform abstraction and the scheduler source value can be passed in as a tuple. This also means the platform scheduler source mappings must align between topology and FW.

This would be a lot better if this was SCHEDULE_SOURCE where various SCHEDULE_SOURCE_s can be defined in schedule.m4. i.e.

# list of generic scheduling sources define (SCHEDULE_SOURCE_DMA0, 0) ... define (SCHEDULE_SOURCE_TIMER0, 100) define (SCHEDULE_SOURCE_TIMER1, 101)

This would be then be made platform specific in apl.m4

define (SCHEDULE_SOURCE_HPGPDMA0, SCHEDULE_SOURCE_DMA0)

This then allows platform abstraction and the scheduler source value can be passed in as a tuple. This also means the platform scheduler source mappings must align between topology and FW.

um, I don't think it's a good idea as schedule source actually can be unlimited, it can be GPDMA or HDA-DMA(host/link), it could also be any possible components(e.g. volume/src/kpb/eq) if we extend our pipeline concept in the future(it can be host-less, dai-less, pipeline is only a special component(named bin) and can be connected to any other components/pipelines).

@lgirdwood Agreed, but that would require changes in pipeline ipc, and adding something like schedule_type/mode instead of current timer_delay. I wouldn't like to change the API now, because HDA pipelines are waiting to be fixed. Can we postpone this couple of weeks, until we agree on the best new interface approach including future needs.

@keyonjie this is the whole point, the scheduling for each pipeline can have multiple configurations, but your change only allows one. We always have to think of the future when creating new APIs/ABIs.
@tlauda I can merge as long as this is converted to an ID rather than a flag in topology (that way we are not changing this API twice in weeks). We can then add the flexibility in a subsequent update with the ABI update.

@keyonjie this is the whole point, the scheduling for each pipeline can have multiple configurations, but your change only allows one. We always have to think of the future when creating new APIs/ABIs.
@tlauda I can merge as long as this is converted to an ID rather than a flag in topology (that way we are not changing this API twice in weeks). We can then add the flexibility in a subsequent update with the ABI update.

@lgirdwood let's align on the intentions here as we had misunderstanding about what we wanted here:

sounds you are asking for something like "SCHEDULE_METHOD" configuration here, e.g.:

SCHEDULE_METHOD_INTERRUPT SCHEDULE_METHOD_XTENSA_TIMER_0 SCHEDULE_METHOD_XTENSA_TIMER_1 SCHEDULE_METHOD_EXTERNAL_TIMER_0

Is that your point?

What we implemented here was actually adding something like "SCHEDULE_INTERVAL", such as:

pipeline 1(e.g. ssp0 playback): scheduled with DMA interrupt, SCHEDULE_INTERVAL for ppl1 set to be 0. pipeline 2(e.g. ssp1 playback): scheduled with time interval(e.g. in every 1ms, not intend to specify and TIMER it will be used), SCHEDULE_INTERVAL for ppl2 set to be 1000. pipeline 3(e.g. ssp2 playback): scheduled with time interval(e.g. in every 20ms, not intend to specify and TIMER it will be used), SCHEDULE_INTERVAL for ppl3 set to be 20000. pipeline 4(e.g. HDMI1 playback): scheduled with time interval(e.g. in every 5ms, not intend to specify and TIMER it will be used), SCHEDULE_INTERVAL for ppl1 set to be 5000.

My point is that what kind of configurations the FW really needed from topologies, only SCHEDULE_INTERVAL, or SCHEDULE_METHOD + SCHEDULE_INTERVAL?

sounds you are asking for something like "SCHEDULE_METHOD" configuration here, e.g.:

SCHEDULE_METHOD_INTERRUPT SCHEDULE_METHOD_XTENSA_TIMER_0 SCHEDULE_METHOD_XTENSA_TIMER_1 SCHEDULE_METHOD_EXTERNAL_TIMER_0

Is that your point?

Yes. But it's better presented in English as SCHEDULE_INITIATOR (better than method or source)

What we implemented here was actually adding something like "SCHEDULE_INTERVAL", such as:

pipeline 1(e.g. ssp0 playback): scheduled with DMA interrupt, SCHEDULE_INTERVAL for ppl1 set to be 0. pipeline 2(e.g. ssp1 playback): scheduled with time interval(e.g. in every 1ms, not intend to specify and TIMER it will be used), SCHEDULE_INTERVAL for ppl2 set to be 1000. pipeline 3(e.g. ssp2 playback): scheduled with time interval(e.g. in every 20ms, not intend to specify and TIMER it will be used), SCHEDULE_INTERVAL for ppl3 set to be 20000. pipeline 4(e.g. HDMI1 playback): scheduled with time interval(e.g. in every 5ms, not intend to specify and TIMER it will be used), SCHEDULE_INTERVAL for ppl1 set to be 5000.

SCHEDULE_INTERVAL defines the time frequency/interval/period.
SCHEDULE_INITIATOR defines the time domain.

There can be many different time/clock domains within the audio architecture e.g. when SSP is slave then it uses a different time domain to another SSP which is master (even if MCLK, BCLK and FRAME are all the same frequency). We need to be able to configure this.

In fact I would rename SCHEDULE_INTERVAL to SCHEDULE_PERIOD or SCHEDULE_FREQ as these make more sense than interval.

OK, let me rename this SCHEDULE_TIMER to SCHEDULE_PERIOD, and don't touch the SCHEDULE_INITIATOR at this moment, does that make sense?

@keyonjie not following, wont you need to specify the initiator to make HDA use timer ?

@keyonjie not following, wont you need to specify the initiator to make HDA use timer ?

no, we didn't intend to specify the initiator, we only wanted to specify the SCHEDULE_PERIOD actually.

lgirdwood · 2019-03-15T11:45:37Z

@keyonjie can you make this update as it's currently very limited to a single use case.
@tlauda I guess all the dependency PRs are being tested on #integration ?

tlauda · 2019-03-15T12:12:36Z

@lgirdwood They are tested with my private branch on WHL.

ranj063 · 2019-03-20T20:07:21Z

@tlauda I verified this change on the UP2 with cml-003 branch for the fw and tplg and HDMI playback works.

But just for my understanding, why do we use timer driven scheduling for HDMI and dai driven for SSP?

plbossart · 2019-03-20T20:23:45Z

But just for my understanding, why do we use timer driven scheduling for HDMI and dai driven for SSP?

My understanding is that the interrupts don't quite work for HDaudio host or link DMAs, but they work fine for GPDMA.

tlauda · 2019-03-20T20:55:55Z

@ranj063 Exactly what Pierre said. Until recently the internal implementation of hda-dma used timer scheduling, but it wasn't working quite well. Now we have flexible support for different types of scheduling and quite well written schedule api. On cAVS platforms we would like to switch to timer scheduling also for GPDMA based pipelines to reduce number of interrupts and preemptions.

ranj063 · 2019-03-20T20:58:09Z

@ranj063 Exactly what Pierre said. Until recently the internal implementation of hda-dma used timer scheduling, but it wasn't working quite well. Now we have flexible support for different types of scheduling and quite well written schedule api. On cAVS platforms we would like to switch to timer scheduling also for GPDMA based pipelines to reduce number of interrupts and preemptions.

Thanks @plbossart and @tlauda for the explanation!

plbossart · 2019-03-20T21:27:25Z

@tlauda I hope you are aware that some of our Hifiberry boards are clock master, meaning the SOC is slave. It'd be really good if we avoid mandating the use of the timer-based scheduling in all cases, since it'd have the ripple effect of mandating the introduction of an ASRC. Since those boards are typically for S/PDIF you want to transmit the audio as is, with not even a volume control that could introduce a precision loss.

tlauda · 2019-03-20T21:56:42Z

@plbossart We can set scheduling source per pipeline, so there shouldn't be any problem with such cases.

plbossart · 2019-03-20T23:36:46Z

@plbossart We can set scheduling source per pipeline, so there shouldn't be any problem with such cases.

Thanks for the precisions, I was about to enter my usual paranoia mode ;-)

lgirdwood · 2019-03-21T15:34:32Z

@keyonjie any ETA for update ?

RanderWang · 2019-03-22T06:36:55Z

I validated this PR on WHL with HDA codec. It works.
And now on master, HDA codec still can't work. It is hard for us to develop and validate HDA now.

Thanks!

lgirdwood · 2019-03-22T09:16:51Z

I validated this PR on WHL with HDA codec. It works.

Great, but it's missing rework for review comments.

And now on master, HDA codec still can't work. It is hard for us to develop and validate HDA now.

It's hard for anyone to help you if you don't provide any context or issue number.

RanderWang · 2019-03-22T09:57:36Z

I validated this PR on WHL with HDA codec. It works.

Great, but it's missing rework for review comments.

And now on master, HDA codec still can't work. It is hard for us to develop and validate HDA now.

It's hard for anyone to help you if you don't provide any context or issue number.

This patch is to fix the bug: #1114. Actually, HDA codec is also failed. This bug is referenced already.

keyonjie · 2019-03-22T10:00:06Z

@tlauda my update here: https://github.com/keyonjie/sof/tree/timer_scheduler
please help integrate and update.

tlauda · 2019-03-22T10:08:33Z

@keyonjie Done.

lgirdwood

Lets rename deadline to period and add initiator/source as the last arg. This only requires some renaming in the topology IPC for pipe new.

struct sof_ipc_pipe_new {
	struct sof_ipc_cmd_hdr hdr;
	uint32_t comp_id;	/**< component id for pipeline */
	uint32_t pipeline_id;	/**< pipeline id */
	uint32_t sched_id;	/**< Scheduling component id */
	uint32_t core;		/**< core we run on */
	uint32_t deadline;	/**< execution completion deadline in us*/  <<<<<< rename to period.
	uint32_t priority;	/**< priority level 0 (low) to 10 (max) */
	uint32_t period_mips;	/**< worst case instruction count per period */
	uint32_t frames_per_sched;/**< output frames of pipeline, 0 is variable */
	uint32_t xrun_limit_usecs; /**< report xruns greater than limit */

	/* non zero if timer scheduled, otherwise DAI DMA irq scheduled */
	uint32_t timer_delay;  <<< rename to initiator or time_domain.
} __attribute__((packed));

lgirdwood · 2019-03-22T11:44:03Z

Ok, this overlaps with "deadline" - and deadline could probably be changed to "period". Your current "schedule_period" could become "time_domain"

tlauda · 2019-03-22T13:11:59Z

@lgirdwood What about kernel changes? Should we also increment ABI?

lgirdwood · 2019-03-22T15:14:49Z

@lgirdwood What about kernel changes? Should we also increment ABI?

Yes and also similar PR (with ABI changes) for kernel too.

keyonjie · 2019-03-22T17:58:48Z

@tlauda my update here: https://github.com/keyonjie/sof/tree/timer_scheduler
please help integrate and update.

@lgirdwood this make sense?

lgirdwood · 2019-03-24T13:20:15Z

@keyonjie yep better, I would just do a small change for "initiator" -> "time_domain" as this makes more sense after all our discussions. This should future proof thing and help enable components like ASRC, AEC, etc

tlauda · 2019-03-26T14:26:29Z

@lgirdwood Applied your suggestions. Please let me know if it's OK, so I can prepare kernel changes.

lgirdwood

Looks good - technically we are renaming only the ABI parts so it may not need the bump (but there is no harm with increment)

Rename 'deadline' to 'period' to indicate pipeline scheduling period, and add time domain to indicate the pipeline scheduling domain (e.g. DMA interrupt, timer interrupt). Please be noticed that this might introduce an abi change so consequent abi changes in both FW and driver side are needed. Haven't set the flag in topology/sof/sof-xxx.m4 yet, that means it will use io/interrupt scheduling for those topologies/pipelines at the moment, if wanna to change them, please change them specific there. Signed-off-by: Keyon Jie <yang.jie@linux.intel.com> Signed-off-by: Tomasz Lauda <tomasz.lauda@linux.intel.com>

Updates all HDA pipelines in topologies to use timer as a scheduling source. Signed-off-by: Tomasz Lauda <tomasz.lauda@linux.intel.com>

tlauda · 2019-03-27T10:18:02Z

@lgirdwood Kernel patch: thesofproject/linux#752.

lgirdwood · 2019-03-27T14:39:38Z

@tlauda lets remove the ABI bump and we can merge this today.

Updates sof_ipc_pipe_new struct to better handle different time domains: - Changes "deadline" to "period". - Changes "timer_delay" to "time_domain". Signed-off-by: Tomasz Lauda <tomasz.lauda@linux.intel.com>

tlauda · 2019-03-27T14:43:46Z

@lgirdwood Done.

tlauda · 2019-03-27T18:07:11Z

@lgirdwood It's good to merge. Travis is just stuck.

lgirdwood · 2019-03-27T21:43:54Z

@tlauda yeah, it's stuck on others too tonight, must be an outage. I have confidence in this as it's small PR and other CIs passing

tlauda requested review from lbetlej, lgirdwood, mmaka1, plbossart, ranj063 and xiulipan as code owners March 14, 2019 16:53

tlauda mentioned this pull request Mar 14, 2019

volume pipeline copy failure with HDMI playback on APL-UP2-pcm512x. #1114

Closed

lgirdwood requested changes Mar 15, 2019

View reviewed changes

tlauda force-pushed the topic/hda-timer-topology branch from 6eef7b7 to bfd27d5 Compare March 15, 2019 12:16

tlauda force-pushed the topic/hda-timer-topology branch 2 times, most recently from 9e10a1f to 66c1ec1 Compare March 22, 2019 10:07

lgirdwood requested changes Mar 22, 2019

View reviewed changes

tlauda force-pushed the topic/hda-timer-topology branch from 66c1ec1 to 7e0f8d1 Compare March 26, 2019 14:24

tlauda requested review from bardliao, dbaluta, wwittbrx and zrombel as code owners March 26, 2019 14:24

tlauda force-pushed the topic/hda-timer-topology branch from 7e0f8d1 to fdccf7d Compare March 26, 2019 14:33

lgirdwood approved these changes Mar 26, 2019

View reviewed changes

keyonjie and others added 2 commits March 27, 2019 08:38

topology: update all HDA pipelines to use timer

0b0b94a

Updates all HDA pipelines in topologies to use timer as a scheduling source. Signed-off-by: Tomasz Lauda <tomasz.lauda@linux.intel.com>

tlauda force-pushed the topic/hda-timer-topology branch 2 times, most recently from 05eb74d to 344ff1b Compare March 27, 2019 08:22

tlauda force-pushed the topic/hda-timer-topology branch from 344ff1b to ab7eb5b Compare March 27, 2019 11:57

tlauda mentioned this pull request Mar 27, 2019

ASoC: SOF: update sof_ipc_pipe_new thesofproject/linux#752

Merged

pipeline: update sof_ipc_pipe_new struct

4b5d983

Updates sof_ipc_pipe_new struct to better handle different time domains: - Changes "deadline" to "period". - Changes "timer_delay" to "time_domain". Signed-off-by: Tomasz Lauda <tomasz.lauda@linux.intel.com>

tlauda force-pushed the topic/hda-timer-topology branch from ab7eb5b to 4b5d983 Compare March 27, 2019 14:42

lgirdwood merged commit 742f83b into thesofproject:master Mar 27, 2019

tlauda deleted the topic/hda-timer-topology branch November 22, 2019 11:25

Conversation

tlauda commented Mar 14, 2019

Uh oh!

ranj063 commented Mar 14, 2019

Uh oh!

tlauda commented Mar 14, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlauda Mar 15, 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!

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!

lgirdwood commented Mar 15, 2019

Uh oh!

tlauda commented Mar 15, 2019

Uh oh!

ranj063 commented Mar 20, 2019

Uh oh!

plbossart commented Mar 20, 2019 via email

Uh oh!

tlauda commented Mar 20, 2019

Uh oh!

ranj063 commented Mar 20, 2019

Uh oh!

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

Uh oh!

Uh oh!

tlauda commented Mar 20, 2019

Uh oh!

plbossart commented Mar 20, 2019

Uh oh!

lgirdwood commented Mar 21, 2019

Uh oh!

RanderWang commented Mar 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgirdwood commented Mar 22, 2019

Uh oh!

RanderWang commented Mar 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keyonjie commented Mar 22, 2019

Uh oh!

tlauda commented Mar 22, 2019

Uh oh!

lgirdwood left a comment • 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!

lgirdwood Mar 22, 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!

tlauda commented Mar 22, 2019

Uh oh!

lgirdwood commented Mar 22, 2019

Uh oh!

keyonjie commented Mar 22, 2019

Uh oh!

lgirdwood commented Mar 24, 2019

Uh oh!

tlauda commented Mar 26, 2019

Uh oh!

tlauda Mar 15, 2019 •

edited

Loading

plbossart commented Mar 20, 2019 •

edited

Loading

RanderWang commented Mar 22, 2019 •

edited

Loading

RanderWang commented Mar 22, 2019 •

edited

Loading

lgirdwood left a comment •

edited

Loading

lgirdwood Mar 22, 2019 •

edited

Loading