[bkokoszx]

I've added counters for processed and received ipc tasks.
They are dropped in memory window after trace points.
They can help verify whether firmware handled ipc task correctly.

Signed-off-by: Bartosz Kokoszko bartoszx.kokoszko@linux.intel.com

[xiulipan]

@bkokoszx @lyakh
Nice patch for IPC debug. Maybe we can try to have some kernel driver to get the counter in Kernel.

[plbossart]

Forgive my ignorance of the firmware IPC, but if you want to debug jointly with the kernel we'd need to break down how many host/kernel-initiated IPCs were received/processed and how many firmware initiated- IPCs were sent/acked? The kernel has separate code flows for each.

[bkokoszx]

@xiulipan
Yes. It also will be useful if driver could dump this value after IPC timeout (excluding timeouts happened after CTM_SAVE, since this IPC turns off memory).

[bkokoszx]

@plbossart
The initial reason doing above PR is that we can observe whether firmware handled all received IPC's from host - then we can mark which side (kernel/firmware) causes timeouts in an easier way. I think driver can also count number ipc's send to FW and after all we can compare this values, but since we do not have any IPC queue I think I wouldn't be so useful.
I am not sure If I understand your question correctly and whether my answer is sufficient, please let me know. :)

[jajanusz]

@plbossart @bkokoszx I think these counters can be added independently on both sides.

[jajanusz]

@xiulipan
Yes. It also will be useful if driver could dump this value after IPC timeout (excluding timeouts happened after CTM_SAVE, since this IPC turns off memory).

Yea, we can get these values with slim driver, but when using full stack we need you to dump them for us. @xiulipan @plbossart Can someone from SW team add it? We need it asap for ipc timeouts debugging.

[jajanusz]

I've made something like this: https://github.com/jajanusz/linux/commits/ipc-debug
We can use it for testing because we don't want to merge debugging stuff to master for now.

[plbossart]

I've made something like this: https://github.com/jajanusz/linux/commits/ipc-debug
We can use it for testing because we don't want to merge debugging stuff to master for now.

@jajanusz I added comments directly. The one thing I'd like clarity on is whether the change of the FW_END definition is backwards compatible, no sure what it is used for in firmware.

[keyonjie]

How about those "dropping msg" happened in line 117? At the case handled_counter will be never equal to recceived_counter?

[bkokoszx]

@keyonjie
If I understand correctly in our current IPC flow, we do not have any IPC's queuing. Driver can send new IPC only when firmware accomplishes previous ipc task and triggers interrupt to host.
It happens in 177 line:

ipc->host_pending = 0;
/* write 1 to clear busy, and trigger interrupt to host*/
#if CAVS_VERSION == CAVS_VERSION_1_5
	ipc_write(IPC_DIPCT, ipc_read(IPC_DIPCT) | IPC_DIPCT_BUSY);

So in current flow we will never have situation that the variable _ipc->host_pending will be set to true in line 117.

[jajanusz]

IRT kernel side: According to the comments in thesofproject/linux#955 we can get these values from /sys/kernel/debug/sof/regs

I wonder if we need to have it permanently added to FW, it's something that you turn on when you have IPC issues. Other registers there have more general usage f.e. trace point is for debugging whole boot-up process, these values are something that you add temporary for debugging very specific case and then remove them.

[bkokoszx]

@jajanusz
Maybe we should add this to kconfig as disabled by default. What's your opinion?

[jajanusz]

@jajanusz
Maybe we should add this to kconfig as disabled by default. What's your opinion?

Well, It's better than having it all the time. Can you add it under DEBUG menu?

[bkokoszx]

I'll update.

[bkokoszx]

@jajanusz
Ok, I've updated. Please take a look.

[lgirdwood]

This should not be conditional, counters should always be at these offsets even if disabled.

[lgirdwood]

these will need to be added to kernel headers too.

[bkokoszx]

Updated. I've make PR on the kernel side:
thesofproject/linux#976

[jajanusz]

@lgirdwood What's the point of having them in headers, if these registers are conditional. They can be there, but they don't have to. 3 first registers are used always and logged to the dmesg. These 2 are of new kind that is for debugging very specific purpose (and they may not even be there - depends on kconfig) I'm not sure if kernel should care about them at all. If we want these registers to be added to the kernel code we should log them all the time like I wanted to do in thesofproject/linux#955 . Otherwise as @plbossart mentioned somewhere earlier we will end up with bunch of registers definitions that are not used and people will ask, why they are there at all.

So we should adapt the policy that we log all of them on some kind of related error or we make kernel not care about them and developer will just get what he wants from debugfs.

[plbossart]

If you make this conditional, then the kernel needs a mechanism to know if those counters are present/valid.
The problem with debugfs is that there is no serialization with other events, e.g. you really want to show those counters when there is an IPC timeout, which you can't do with debugfs.

[bkokoszx]

@lgirdwood
Can't we make those memory windows conditional?