trace: convert trace time stamp to ticks by bardliao · Pull Request #1358 · thesofproject/sof

bardliao · 2019-05-03T14:21:35Z

The unit of trace header timestamp is ticks but it is nanosecond
in trace params. So we have to convert it to ticks.

Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com

lgirdwood

@bardliao Can you rename the ipc structure time delta variable to include a _ns suffix and comment for nanosecs, that way it's obvious. This wont be an ABI change.

xiulipan

The clock usage seems not like other user. Please take reference and made some modification.

xiulipan · 2019-05-04T22:14:46Z

@bardliao
Maybe this way?

Suggested change

return clk_pdata->clk[clock].ticks_per_msec / 1000 * ns / 1000;

return clk_pdata->clk[clock].ticks_per_msec * ns / 1000000;

@xiulipan It is a trade-off between overflow and accuracy. It may overflow when clk_pdata->clk[clock].ticks_per_msec * ns

Then maybe add a comment here to explain the overflow risk.

@bardliao would it be easier to use the clock here in this calculation since it will be in MHz (multiplied by 1000 it will be same timebase as host ns) ?

@lgirdwood Sorry I didn't get you. I think the clock here is an index, right? Did you mean something like double clock; in logger? I didn't find a similar variable here.

xiulipan · 2019-05-04T22:25:06Z

Why made this to a global variable?
You can take reference to other clock user like: sch->clock or just use PLATFORM_DEFAULT_CLOCK

xiulipan · 2019-05-04T22:26:13Z

This here is too complex, you can reference clock_ms_to_ticks user to simplify here.

@xiulipan Updated. But I don't know if using PLATFORM_DEFAULT_CLOCK is always right. for example, On baytrail, the .clk is PLATFORM_WORKQ_CLOCK, but the PLATFORM_DEFAULT_CLOCK is CLK_SSP

@bardliao
Then maybe you can take reference of sch->clock to pre define it for different platform.

@xiulipan updated.

Existing clock_ms_to_ticks() is used with the PLATFORM_DEFAULT_CLOCK in many places assuming this is correct define.
Wrt to BYT see this define:

/* platform WorkQ clock */ #define PLATFORM_WORKQ_CLOCK PLATFORM_DEFAULT_CLOCK

xiulipan · 2019-05-04T22:29:54Z

@bardliao
BTW, where is the linux part. Or no need for driver change?

bardliao · 2019-05-05T05:37:12Z

@bardliao
BTW, where is the linux part. Or no need for driver change?

@xiulipan The linux part is on thesofproject/linux#756

bardliao · 2019-05-05T05:52:09Z

@bardliao Can you rename the ipc structure time delta variable to include a _ns suffix and comment for nanosecs, that way it's obvious. This wont be an ABI change.

@lgirdwood Yes, done.

xiulipan · 2019-05-06T07:32:09Z

@bkokoszx
There are some timer related changes. Please have a look to see if the implementation is the right way.

RanderWang

LGTM

keyonjie · 2019-05-06T09:38:04Z

assign ticks to nanosecond?

I've also think that _ns is bit misleading here.

@bardliao external host IPC uses nanosecs, internal uses local time domain (which will be different for each platform)

@bardliao
I think the _ns is already with timestamp_ns. No need for delta.

Yes, thanks everyone. The _ns here is a mistake. I will fix it then

lgirdwood · 2019-05-06T13:40:57Z

@bardliao external host IPC uses nanosecs, internal uses local time domain (which will be different for each platform)

lgirdwood · 2019-05-06T13:54:33Z

@bardliao would it be easier to use the clock here in this calculation since it will be in MHz (multiplied by 1000 it will be same timebase as host ns) ?

lgirdwood · 2019-05-08T11:28:52Z

Why do we need this extern structure ?

@lgirdwood I need to know the clk id of platform_timer (eg. CLK_SSP) which is platform dependency.

@bardliao ok, can you rebase

@lgirdwood Yes, done.

The unit of trace header timestamp is ticks but it is nanosecond in trace params. So we have to convert it to ticks. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

bardliao · 2019-05-13T05:45:53Z

@mmaka1 Thanks for the comments. The PR has updated. thanks.

lgirdwood · 2019-05-14T11:59:35Z

@bardliao is the kernel part ready for merging yet ?

bardliao · 2019-05-15T05:18:03Z

@bardliao is the kernel part ready for merging yet ?

@lgirdwood I think basically @plbossart agree with that PR, but we need to deal with the missing ABI minor 6 issue. Please see thesofproject/linux#756 (comment)

lgirdwood · 2019-05-17T13:14:42Z

@zrombel @bardliao can you check the build failure.

tlauda · 2019-05-30T09:40:53Z

@bardliao I guess the kernel change is already merged? Is this one good to go?

xiulipan

Code LGTM
I will try to run some test later.

bardliao · 2019-05-30T09:54:54Z

@bardliao I guess the kernel change is already merged? Is this one good to go?

@tlauda Yes, the kernel change is already merged. thesofproject/linux#756

Now outdated, changes made to simplify further.

bardliao requested review from lgirdwood, plbossart and xiulipan May 3, 2019 14:21

bardliao requested review from lbetlej, libinyang and mmaka1 as code owners May 3, 2019 14:21

lgirdwood reviewed May 3, 2019

View reviewed changes

xiulipan suggested changes May 4, 2019

View reviewed changes

bardliao force-pushed the trace_timer branch from d4ae71c to 29e638e Compare May 5, 2019 05:35

bardliao requested a review from akloniex as a code owner May 5, 2019 05:35

bardliao force-pushed the trace_timer branch from 29e638e to a452737 Compare May 6, 2019 06:00

bardliao requested a review from RanderWang as a code owner May 6, 2019 06:00

xiulipan requested a review from bkokoszx May 6, 2019 07:30

RanderWang approved these changes May 6, 2019

View reviewed changes

keyonjie reviewed May 6, 2019

View reviewed changes

lgirdwood requested changes May 6, 2019

View reviewed changes

bardliao force-pushed the trace_timer branch from a452737 to c1f8b97 Compare May 7, 2019 01:01

lgirdwood requested changes May 8, 2019

View reviewed changes

bardliao force-pushed the trace_timer branch from c1f8b97 to 721804f Compare May 10, 2019 01:46

mmaka1 previously requested changes May 10, 2019

View reviewed changes

Comment thread src/ipc/handler.c Resolved

trace: convert trace time stamp to ticks

7bca336

The unit of trace header timestamp is ticks but it is nanosecond in trace params. So we have to convert it to ticks. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

bardliao force-pushed the trace_timer branch from 721804f to 7bca336 Compare May 13, 2019 05:39

tlauda requested review from lgirdwood, mmaka1 and xiulipan May 30, 2019 09:39

xiulipan approved these changes May 30, 2019

View reviewed changes

lgirdwood approved these changes May 31, 2019

View reviewed changes

lgirdwood merged commit 7bfbe43 into thesofproject:master May 31, 2019

bardliao mentioned this pull request Jun 9, 2019

Send time since boot to the firmware upon resume thesofproject/linux#41

Closed

	return clk_pdata->clk[clock].ticks_per_msec / 1000 * ns / 1000;
	return clk_pdata->clk[clock].ticks_per_msec * ns / 1000000;

Conversation

bardliao commented May 3, 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!

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

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!

xiulipan commented May 4, 2019

Uh oh!

bardliao commented May 5, 2019

Uh oh!

bardliao commented May 5, 2019

Uh oh!

xiulipan commented May 6, 2019

Uh oh!

RanderWang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkokoszx May 6, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bardliao commented May 13, 2019

Uh oh!

lgirdwood commented May 14, 2019

Uh oh!

bardliao commented May 15, 2019

Uh oh!

lgirdwood commented May 17, 2019

Uh oh!

tlauda commented May 30, 2019

Uh oh!

xiulipan left a comment

Choose a reason for hiding this comment

lgirdwood left a comment •

edited

Loading

bkokoszx May 6, 2019 •

edited

Loading