Skip to content

test: ipc: Add FW testing IPC foundations. #1290

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
lgirdwood merged 1 commit into from
May 17, 2019
Merged

test: ipc: Add FW testing IPC foundations. #1290

lgirdwood merged 1 commit into from
May 17, 2019

Conversation

@lgirdwood
Copy link
Member

@lgirdwood lgirdwood commented Apr 21, 2019 •
edited
Loading

Add a new IPC global class for FW testing alongside the first "test" IPC
command for flooding the DSP with IPCs.

Needs to have CONFIG_DEBUG enabled.

Signed-off-by: Liam Girdwood liam.r.girdwood@linux.intel.com

@ranj063 I would expect the kernel to export a debugFS file where the user could

echo 1000 > /debugFS/sof/ipc_flood

to execute a test of a 1000 IPCs. Then

cat /debugFS/sof/ipc_flood

Would indicate number of tests until failure, where 1000 would be a pass.

@lrgirdwo
test: ipc: Add FW testing IPC foundations.
b56d644 
Add a new IPC global class for FW testing alongside the first "test" IPC
command for flooding the DSP with IPCs.

Needs to have CONFIG_DEBUG enabled.

Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
@ranj063
Copy link
Collaborator

ranj063 commented Apr 21, 2019

@lgirdwood ack. will implement the kernel side debugfs tonight.

xiulipan
xiulipan suggested changes Apr 21, 2019
View reviewed changes
Copy link
Contributor

@xiulipan xiulipan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add some inline comments to improve the debug feature.

xiulipan
xiulipan previously requested changes Apr 21, 2019
View reviewed changes
Copy link
Contributor

@xiulipan xiulipan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add some inline comments to improve the debug feature.

src/ipc/handler.c
uint32_t cmd = iCS(header);

switch (cmd) {
case SOF_IPC_TEST_IPC_FLOOD:
Copy link
Contributor

@xiulipan xiulipan Apr 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can add some trace log with the IPC index id to help identify which IPC is missed. We can only tell how many IPC is missed, but which one is missed will help the debug

Copy link
Collaborator

@ranj063 ranj063 Apr 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xiulipan that makes sense in the driver. the FW cant actually keep count because the IPC's may not coming in order.

Copy link
Contributor

@xiulipan xiulipan Apr 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ranj063
I would like to make test IPC counts the index id as its payload. To help us debug the order or sequence issues.

Copy link
Member Author

@lgirdwood lgirdwood Apr 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xiulipan inline comments ? care to elaborate.

Copy link
Collaborator

@ranj063 ranj063 Apr 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lgirdwood i think he meant he added some comments in the commit

@ranj063 ranj063 mentioned this pull request Apr 22, 2019
Closed
@xiulipan xiulipan added the ABI ABI change involved label Apr 22, 2019
@xiulipan xiulipan mentioned this pull request Apr 22, 2019
Merged
@xiulipan
Copy link
Contributor

xiulipan commented Apr 22, 2019

@lgirdwood @ranj063
As we will only have some count on linux side. I think we may miss some situations here:

  • FW handled the IPC, host got reply
    This should be we expected prefect result.
  • FW handled the IPC in wrong order, host got reply
    This may happen, as we are sending same IPC without payload to identify each other. But in real case, may cause problem(connect handled before comp new)
  • FW handled the IPC, host did not get reply
    Very simple case, we met a lot these days. Should be some DSP=>HOST IRQ issue.
  • FW handled the IPC in wrong order, host did not get reply
    The reply for the first did not reach HOST in time. Host got timeout without reply.
  • FW did not handle the IPC, host did not get reply
    Simple case when FW has some issue.

They will both end up in some count mismatch/match in Kernel side but need to be treated and debug independently. My suggestion is to add some index as payload here.

@ranj063
Copy link
Collaborator

ranj063 commented Apr 22, 2019

@xiulipan I agree that we should add some payload here too. Maybe that can be index. @lgirdwood do you want me to make the change for it?

@lgirdwood
Copy link
Member Author

lgirdwood commented Apr 22, 2019

@xiulipan sequence numbers will likely come in IPC2.0 if needed, the HW locking prevents out of IPC order.

@xiulipan
Copy link
Contributor

xiulipan commented Apr 22, 2019

@lgirdwood @ranj063
The order may not be wrong now, but how could we tell if the FW handled the IPC or not?
With an index it would be much more easier to find which IPC have delay or missed.

@lgirdwood
Copy link
Member Author

lgirdwood commented Apr 23, 2019

@xiulipan by examining the IPC state machine for both DSP and host side. See thesofproject/linux#846, I'm working on an equivalent for DSP side today. If you have time please take a look at thesofproject/linux#846 as it's mainly dev_err() statements than need to be added.

@lgirdwood lgirdwood requested a review from a team April 23, 2019 10:07
@lgirdwood
Copy link
Member Author

lgirdwood commented Apr 23, 2019

@zrombel I assume this is a server error on the CI ?

@zrombel
Copy link

zrombel commented Apr 23, 2019

@lgirdwood yes, some QB issue. PR is waiting for rebuild.

@lgirdwood
Copy link
Member Author

lgirdwood commented Apr 29, 2019

@ranj063 ready to merge this once kernel PR is ready. classified as MINOR 7

@lgirdwood lgirdwood merged commit b01ed57 into master May 17, 2019
@jajanusz
Copy link
Contributor

jajanusz commented May 17, 2019

@lgirdwood But do we have it on kernel side?

@jajanusz
Copy link
Contributor

jajanusz commented May 17, 2019

@lgirdwood Also, can we align API first and then extend it?

@lgirdwood lgirdwood deleted the lrg/topic/ipc-test branch September 12, 2019 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ranj063 ranj063 ranj063 left review comments

@bardliao bardliao Awaiting requested review from bardliao

@dbaluta dbaluta Awaiting requested review from dbaluta

@wwittbrx wwittbrx Awaiting requested review from wwittbrx

@zrombel zrombel Awaiting requested review from zrombel

+1 more reviewer

@xiulipan xiulipan xiulipan left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

ABI ABI change involved

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@lgirdwood @ranj063 @xiulipan @zrombel @jajanusz @lrgirdwo