Skip to content

Conversation

@keqiaozhang
Copy link
Contributor

No description provided.

@keqiaozhang keqiaozhang requested a review from a team as a code owner September 9, 2022 08:11
@keqiaozhang
Copy link
Contributor Author

Tested with different FW model:http://sof-ci.sh.intel.com/#/result/planresultdetail/15252. looks like everything is working fine.

kv2019i
kv2019i previously approved these changes Sep 13, 2022
Copy link
Contributor

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

Looks good! Couple of comments inline that can be fixed in a follow-up. Tested this locally and seems to work fine.

case-lib/lib.sh Outdated

is_ipc4_zephyr(){
local firmware_path firmware_name znum
fw_path=/sys/module/snd_sof_pci/parameters/fw_path
Copy link
Contributor

Choose a reason for hiding this comment

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

The above doesn't work if fw_path is not set. At least I had a local setup where I just use the standard path assumed by kernel for IPC4 FW.

Maybe if fw_path is NULL, check for /lib/firmware/intel/avs/"$platf"/dsp_basefw.bin . This is what is in SOF kernel driver by default unless fw_path is passed as a module parameter.

Above can be done in a seperate follow-up PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I can do that, but we need to solve one problem, the full fw_path should be /lib/firmware/intel/avs/"$platf"/"$key_type"/dsp_basefw.bin.

For example, the full fw path of TGLU_UP_HDA_IPC4ZPH is /lib/firmware/intel/avs/tgl/community/dsp_basefw.bin. The key_type is defined in community boards :https://github.com/thesofproject/linux/blob/topic/sof-dev/sound/soc/sof/sof-pci-dev.c#:~:text=/*%20all%20Up,%7D%3B.

I can use the DMI info to get the key_type, but better to let the kernel driver to expose the fw_path as Marc suggested below.

case-lib/lib.sh Outdated
dlogi 'SOF logs collection is globally disabled because DUT is running IPC4 mode'
if ipc4_used && ! is_ipc4_zephyr; then
# mtrace and sof-logger are not supported for closed source IPC4 Firmware
dlogi 'Currenly trace loggers are not supported for closed source IPC4 firmware.'
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's just put "IPC4 FW logging only support with SOF Zephyr build"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

Copy link
Contributor

@greg-intel greg-intel Sep 14, 2022

Choose a reason for hiding this comment

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

"Currenly" --> "Currently"?
Edit: Scratch that, I was looking at an outdated file.

case-lib/lib.sh Outdated
printf '%s' "$ldcFile"
}

func_logger_collect()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
func_logger_collect()
func_sof_logger_collect()

We have too many logging tools, important to label them well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

case-lib/lib.sh Outdated
ipc4_used || return 1

znum=$(strings "/lib/firmware/$firmware_path/$firmware_name" | grep -c -i zephyr)
test "$znum" -gt 10
Copy link
Collaborator

Choose a reason for hiding this comment

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

I recognize this code from the existing is_zephyr() function... which depends on the sof-logger .ldc file.

Please don't create a new, hidden and different is_zephyr() function here while keeping the old one. We need a single is_zephyr() function.

Either the old is_zephyr() function is still OK (because we still depend on sof-logger for DMA logs) and you call it here. Or it's not OK and you fix it; don't leave all the other is_zephyr() users broken.

Copy link
Collaborator

Choose a reason for hiding this comment

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

To find the firmware file you can try some combination of ./tools/sof-dump-status.py -P and ./tools/sof-dump-status.py -p. Again these are used in many other places, so if they don't work then them must be fixed.

@kv2019i , @plbossart, @ujfalusi sof-test needs something like /sys/module/snd_sof_pci/parameters/fw_path but reliable that it can count on, can the kernel expose that? Same thing for the topology: up to now sof-test/tools/sof-get-default-tplg.sh has been grepping journalctl; that's a horrible and unreliable hack.

PS: actually the Linux kernel request_firmware API should expose all the firmware files it has loaded from a filesystem, this sort of "manifest" seems like a bare minimum from a security perspective but that could take years.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please don't create a new, hidden and different is_zephyr() function here while keeping the old one. We need a single is_zephyr() function.

Yes, I also considered to reuse is_zephyr() function, but it depends on the sof-logger .ldc file. But IPC4 firmware has
no ldc file. if we want to reuse this function, we need to switch to using firmware binary instead of ldc file.

Copy link
Contributor Author

@keqiaozhang keqiaozhang Sep 14, 2022

Choose a reason for hiding this comment

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

To find the firmware file you can try some combination of ./tools/sof-dump-status.py -P and ./tools/sof-dump-status.py -p

./tools/sof-dump-status.py -P only works for IPC3 not IPC4. Part of path is fixed as /lib/firmware/intel/sof/, it only check if the DUT is community board, if yes, the fw_path will be combined to /lib/firmware/intel/sof/community

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can the kernel expose that? Same thing for the topology: up to now sof-test/tools/sof-get-default-tplg.sh has been grepping journalctl; that's a horrible and unreliable hack.

I totally agree with this proposal, then we can get the reliable fw_path from the journalctl.

Copy link
Collaborator

@marc-hb marc-hb Sep 16, 2022

Choose a reason for hiding this comment

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

I meant: "can you please add mtrace support to check-sof-logger first"?

I‘m trying to add mtrace in check-sof-logger.sh, but I realized that mtrace-reader.py should run while tests are running. If no tests are running, the log outputs will be empty.

In check-sof-logger.sh, we reload sof driver first, then collect and analyze the logs, which is not suitable for mtrace.
Do we need to start mtrace while reloading the driver?

I'm not sure if it's a mtrace limitation.
@kv2019i , please help to explain it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I fully understand your point.
IPC3 and IPC4 are using a completely different path:
IPC3: /lib/firmware/intel/sof or /lib/firmware/intel/sof/community/
IPC4: /lib/firmware/intel/sof-ipc4/$platform/community or /lib/firmware/intel/sof-ipc4/$platform/debug/

So if we add a function like is_fw_file_zephyr($filename) -> return true/false, we first need to check ipc4_used or not.
If we don't know the IPC type, then it's hard for us to know the specific fw_path,
So the process should be:

  1. check ipc type ipc4_used
  2. get the fw_path
  3. check if the fw is zephyr

Almost the same steps as I did in function is_ipc4_zephyr.

Copy link
Collaborator

@marc-hb marc-hb Sep 19, 2022

Choose a reason for hiding this comment

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

So if we add a function like is_fw_file_zephyr($filename) -> return true/false, we first need to check ipc4_used or not.
...
Almost the same steps as I did in function is_ipc4_zephyr

Yes but is_ipc4_zephyr is monolithic hence unscalable and unmaintainable. Break it down in smaller, modular, re-usable and maintainable functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I didn't figure out a better solution to break is_ipc4_zephyr down in smaller.

I'm thinking to use firmware file to check zephyr in is_zephyr instead of ldc file.
We can refine this part after thesofproject/linux#3867 is supported.

Then combine these two functions ipc4_used + is_zephyr to replace is_ipc4_zephyr.

any other suggestions?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm thinking to use firmware file to check zephyr in is_zephyr instead of ldc file.

Yes, this is what I suggested above: is_fw_file_zephyr($filename). We unfortunately need two different is_zephyr_XXX() functions for now. Better make that obvious rather than hide it buried deep in a big, monolithic is_ipc4_zephyr() that looks like it's calling the usual is_zephyr() while doing something different behind the scenes.

Copy link
Collaborator

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

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

As with cavstool before, can you please fix check-sof-logger.sh first? This will reduce the impact when something goes wrong (as it happened with cavstool) and it should be done anyway.

Note the earlier cavstool attempt in sof-test #897 (to be abandoned?). It has relevant context about func_lib_start_log_collect() and others.

case-lib/lib.sh Outdated
ipc4_used || return 1

znum=$(strings "/lib/firmware/$firmware_path/$firmware_name" | grep -c -i zephyr)
test "$znum" -gt 10
Copy link
Collaborator

Choose a reason for hiding this comment

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

To find the firmware file you can try some combination of ./tools/sof-dump-status.py -P and ./tools/sof-dump-status.py -p. Again these are used in many other places, so if they don't work then them must be fixed.

@kv2019i , @plbossart, @ujfalusi sof-test needs something like /sys/module/snd_sof_pci/parameters/fw_path but reliable that it can count on, can the kernel expose that? Same thing for the topology: up to now sof-test/tools/sof-get-default-tplg.sh has been grepping journalctl; that's a horrible and unreliable hack.

PS: actually the Linux kernel request_firmware API should expose all the firmware files it has loaded from a filesystem, this sort of "manifest" seems like a bare minimum from a security perspective but that could take years.

case-lib/lib.sh Outdated
log_file=$LOG_ROOT/slogger.txt
log_opt="-t"
func_logger_collect "$log_file" "$log_opt"
if is_ipc4_zephyr; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if is_ipc4_zephyr; then
if is_zephyr && is_ipc4; then


fi

if is_ipc4_zephyr; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if is_ipc4_zephyr; then
if is_zephyr && is_ipc4; then

kv2019i
kv2019i previously approved these changes Sep 14, 2022
@marc-hb
Copy link
Collaborator

marc-hb commented Sep 14, 2022

Another important problem: there are only 3 platforms tested in https://sof-ci.01.org/softestpr/PR956/build135/devicetest/ and they're ALL part of the same audio hardware generation and they are all Zephyr. This is especially problematic for a PR like this one that has messy is_zephyr copy/paste/diverge.

Every sof-test PR needs this : https://sof-ci.01.org/linuxpr/PR3860/build714/devicetest/ (part of thesofproject/linux#3860), we can't wait nightly tests to discover regressions.

@keqiaozhang
Copy link
Contributor Author

Another important problem: there are only 3 platforms tested

I created a new test plan and covered all our test configs: xtos-ipc3, zephyr-ipc3. zephyr-ipc4 and cavs-ipc4:
http://sof-ci.sh.intel.com/#/result/planresultdetail/15402

The results look good. All failures are identified as known issues(sof-logger and IPC4 related issues). The new logging tool mtrace works as expected and sof-logger is not effected.

kv2019i
kv2019i previously approved these changes Sep 15, 2022
Copy link
Contributor

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

Seems to work great! And we really, really, really need this as currently we are running blind w.r.t. FW logs in IPC4 test runs.

@marc-hb
Copy link
Collaborator

marc-hb commented Sep 15, 2022

I created a new test plan and covered all our test configs: xtos-ipc3, zephyr-ipc3. zephyr-ipc4 and cavs-ipc4:
http://sof-ci.sh.intel.com/#/result/planresultdetail/15402

This is good but it's a once-off. The (messy) logging code in sof-test is going to evolve more and again very soon and not just to fix the regressions introduced by this PR. Also, this URL is not public.

Every sof-test PR needs this : https://sof-ci.01.org/linuxpr/PR3860/build714/devicetest/ (part of thesofproject/linux#3860), we can't wait nightly tests to discover regressions.

@keqiaozhang
Copy link
Contributor Author

can you please fix check-sof-logger.sh first?

  1. sof-logger failure has nothing to do with mtrace, it's only for IPC3 test. they are old known issues and I don't think we can easily fix them all([BUG] IPC timeout in sof-logger-check sof#6042).
  2. check-sof-logger.sh is not suitable for IPC4 test, so we didn't add check-sof-logger.sh to the formal test plan of IPC4, maybe we can add a new case like check-mtrace.sh in future.

@kv2019i
Copy link
Contributor

kv2019i commented Sep 19, 2022

I do agree we need to add a separate test case for mtrace that just checks availability of mtrace. But @marc-hb is that a blocker for this PR?

Btw, I filed a separate enhancement item for kernel to expose the chosen tplg+fw -> thesofproject/linux#3867

@kv2019i
Copy link
Contributor

kv2019i commented Sep 19, 2022

And @marc-hb and @keqiaozhang , please note mtrace (SRAM logging) is not a replacement for sof-logger (DMA based logging). We will still have a new DMA-based logging system for IPC4, which is based on probes. For now, mtrace is just a new way to get lots out with no direct comparison in IPC3 SOF builds.

@marc-hb
Copy link
Collaborator

marc-hb commented Sep 19, 2022

can you please fix check-sof-logger.sh first?

sof-logger failure has nothing to do with mtrace

Sorry for the confusion, I meant: "can you please add mtrace support to check-sof-logger first"?

check-sof-logger.sh is not suitable for IPC4 test,

check-sof-logger.sh is generic enough to test any logs from anywhere.

maybe we can add a new case like check-mtrace.sh in future.

Please no copy/paste/diverge.

@marc-hb
Copy link
Collaborator

marc-hb commented Sep 19, 2022

I do agree we need to add a separate test case for mtrace that just checks availability of mtrace.

That's exactly what check-sof-logger.sh does for two earlier types of SRAM logging already.

But @marc-hb is that a blocker for this PR?

No but it would be safer. When we added cavstool we started with check-sof-logger which avoided causing a lot of intermittent failures (#897)

@kv2019i
Copy link
Contributor

kv2019i commented Sep 20, 2022

@marc-hb wrote:

I do agree we need to add a separate test case for mtrace that just checks availability of mtrace.
That's exactly what check-sof-logger.sh does for two earlier types of SRAM logging already.

I didn't realize that. So ack, then it's appropriate to also test mtrace.

But @marc-hb is that a blocker for this PR?
No but it would be safer. When we added cavstool we started with check-sof-logger which avoided causing a lot of intermittent failures (#897)

Fair enough. The situation is a bit more urgent now as we have unusual number of P1 bugs open where issues are not happening on all hardware, and we have no visibility to FW logs in current CI results.

@marc-hb
Copy link
Collaborator

marc-hb commented Sep 20, 2022

To be clear I'm NOT blocking this PR on a check-sof-logger update. My main problems are with the monolithic and unmaintainable is_ipc4_zephyr() and the recent and major loss of sof-test PR testing in general which is especially relevant here considering the significant refactoring and conditionals. https://sof-ci.01.org/softestpr/PR956/build137/devicetest/ is basically testing a single configuration! I know one version of this PR has been manually tested once but sorry that's way too limited and unsafe. There will be further changes.

@keqiaozhang
Copy link
Contributor Author

@marc-hb I have removed is_ipc4_zephyr function and added a new one is_firmware_file_zephyr.
So we can use is_ipc4 && is_firmware_file_zephyr for IPC4 Zephyr firmware check.

The is_firmware_file_zephyr is not the final version, now we still need to check the ipc type first and use different ways to get the firmware name/path for IPC3 and IPC4. This part will be optimized after driver can expose the FW path to userspace via debugfs, then we can remove function is_zephyr and use is_firmware_file_zephyr instead.

PR testing is passed, will do more test next.


# when sof logger collect is open
if [ "X$SOF_LOG_COLLECT" == "X1" ]; then
if [ "X$SOF_LOG_COLLECT" == "X1" ] && ! is_ipc4; then
Copy link
Contributor

Choose a reason for hiding this comment

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

For the sake of consistency:

if [ ! is_ipc4 ] && [ "X$SOF_LOG_COLLECT" == "X1" ]; then

And by reversing the checks it is easier to see that this branch is only for 'not IPC4'.


fi

if is_ipc4 && is_firmware_file_zephyr; then
Copy link
Contributor

Choose a reason for hiding this comment

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

For the sake of consistency:

if [ is_ipc4 ] && [ is_firmware_file_zephyr ]; then

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ujfalusi this syntax is wrong. if is_ipc4 is invoking the is_ipc4() function and checking its return value (as intended). if [ is_ipc4 ] is doing something totally different: it's merely checking whether the "is_ipc4" string is not empty (using a confusing syntax) and it's not invoking any function.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Duplicate of a resolved discussion below. Please resolve all comments like this one or none :-)

{
local exit_status=${1:-0}
local loggerBin wcLog; loggerBin=sof-logger
local mtraceBin; mtraceBin=mtrace-reader.py
Copy link
Contributor

Choose a reason for hiding this comment

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

These can be within their respective if branch?

case-lib/lib.sh Outdated
# Cleaned up by func_exit_handler() in hijack.sh
# shellcheck disable=SC2024
sudo "${loggerCmd[@]}" > "$logfile" &
if [ "X$is_etrace" == "X0" ];then
Copy link
Contributor

Choose a reason for hiding this comment

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

space between ; and then

}
fi

local mtraceCmd="$MTRACE"
Copy link
Contributor

Choose a reason for hiding this comment

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

In the last patch you use:

local mtraceBin; mtraceBin=mtrace-reader.py

Which one is the preferred one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because in func_mtrace_collect(), the local mtraceCmd="$MTRACE" can be set when testing with IPC4-Zephyr platforms.

So in hijack.sh, if we test on non-IPC4 Zephyr platforms. the mtraceCmd will be empty, we cannot reuse mtraceCmd in hijack.sh. So I set mtrace separately in these 2 scripts.

# 0: DUT runs IPC3 mode, ipc_used return 1(false)
# 1: DUT runs IPC4 mode, ipc4_used return 0(true)
# 0: DUT runs IPC3 mode, is_ipc4 return 1(false)
# 1: DUT runs IPC4 mode, is_ipc4 return 0(true)
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment does not make any sense
0: DUT runs IPC3 mode, is_ipc4 return 1(false)
1: DUT runs IPC4 mode, is_ipc4 return 0(true)
if ipc_type is 1, this function returns 1 (is_ipc4 is true)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just checked, for IPC3 mode, the ipc_type is -1(updated).
I don't see any problems here. if ipc_type=1, is_ipc4 function returns 0. which means is_ipc4 is true.
We use || here.

 [ "$ipc_type" -eq 1 ] || {
        return 1
    }
    return 0

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, I see. Thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

Would not be easier on the reader to

if [ "$ipc_type" -eq 1 ]; then return 0; fi
return 1

Copy link
Collaborator

Choose a reason for hiding this comment

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

|| is useful for one-liner like sanity_check || die or to avoid ! negations (that soon become double negations and that can't be copied to the command line). For basic if/then/else then I agree with @ujfalusi that it is simpler and just more readable to use... a plain if/then/else. One size does not fit all.

case-lib/lib.sh Outdated
log_file=$LOG_ROOT/etrace.txt
log_opt=""
func_sof_logger_collect "$log_file" "$log_opt"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not following this syntact

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It means only non-ipc4 platforms have etrace support.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's relying on short-circuit evaluation and equivalent to

if ! is_ipc4; then
  ...

with the difference that ! cannot be copied to the interactive command line whereas this can be.

It reads like in plain English: "either IPC4 is true, OR we do the rest".

It's also more useful for one-liners, here it does not save any line.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand why @ujfalusi was confused, I was too. Please change to something like this:

else # once-off etrace collection at end of test
  if is_ipc4; then
     dlogi "No end of test etrace collection for IPC4"
  else
     log_file=$LOG_ROOT/etrace.txt
     ...
  fi
fi

kv2019i
kv2019i previously approved these changes Sep 26, 2022
Copy link
Contributor

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

Still ok to me. There are a few obviously wrong comments that are probably better to address immediately, but I did not see any showstoppers in functionality side.

Signed-off-by: Keqiao Zhang <keqiao.zhang@intel.com>
mtrace is a new logging tool. Tool to stream data from Linux
SOF driver mtrace debugfs interface to standard output.

Signed-off-by: Keqiao Zhang <keqiao.zhang@intel.com>
kv2019i
kv2019i previously approved these changes Sep 26, 2022
@keqiaozhang
Copy link
Contributor Author

keqiaozhang commented Sep 26, 2022

Tested this PR with different models, mtrace works as expected on IPC4ZPH platforms(except for the check-sof-logger.sh) and sof-logger is not effected.
http://sof-ci.sh.intel.com/#/result/planresultdetail/15672
http://sof-ci.sh.intel.com/#/result/planresultdetail/15686

2 on device PR tests also passed.

@lgirdwood
Copy link
Member

All - need some working mtrace sooner rather than later - we can polish it incrementally but we need to start getting the data out.

Copy link
Collaborator

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

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

A couple small changes and we're good to go.

All - need some working mtrace sooner rather than later - we can polish it incrementally but we need to start getting the data out.

This code is incredibly awkward, badly designed and brittle for what it does. @keqiaozhang is doing a delicate and amazing refactoring job but we have 1.9 persons total understanding it and this code is quite critical (as demonstrated by how much this PR is expected). One of the 1.9 persons is me and I have to spend literally 1 hour to make sense of it EVERY TIME I LOOK AT IT so I can't afford to make it worse than it already is.

Thanks everyone for your patience.

# 0: DUT runs IPC3 mode, ipc_used return 1(false)
# 1: DUT runs IPC4 mode, ipc4_used return 0(true)
# 0: DUT runs IPC3 mode, is_ipc4 return 1(false)
# 1: DUT runs IPC4 mode, is_ipc4 return 0(true)
Copy link
Collaborator

Choose a reason for hiding this comment

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

|| is useful for one-liner like sanity_check || die or to avoid ! negations (that soon become double negations and that can't be copied to the command line). For basic if/then/else then I agree with @ujfalusi that it is simpler and just more readable to use... a plain if/then/else. One size does not fit all.

case-lib/lib.sh Outdated
log_file=$LOG_ROOT/etrace.txt
log_opt=""
func_sof_logger_collect "$log_file" "$log_opt"
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand why @ujfalusi was confused, I was too. Please change to something like this:

else # once-off etrace collection at end of test
  if is_ipc4; then
     dlogi "No end of test etrace collection for IPC4"
  else
     log_file=$LOG_ROOT/etrace.txt
     ...
  fi
fi

# when sof logger collect is open
if [ "X$SOF_LOG_COLLECT" == "X1" ]; then
if ! is_ipc4 && [ "X$SOF_LOG_COLLECT" == "X1" ]; then
local loggerBin wcLog; loggerBin=sof-logger
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please don't change or move this line. This refactoring is complex and risky enough, stick to the bare minimum required.

Add a simple one-line comment like # IPC4 is very different and handled below

Copy link
Collaborator

Choose a reason for hiding this comment

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

ping?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please don't change or move this line. This refactoring is complex and risky enough, stick to the bare minimum required.

Hope I didn't misunderstand your point. PR is updated for

local loggerBin wcLog; loggerBin=sof-logger

Align the name with other similar functions and
refine the is_ipc4 function.

Signed-off-by: Keqiao Zhang <keqiao.zhang@intel.com>
…phyr

This function can check if the running firmware file is built with Zephyr.

Signed-off-by: Keqiao Zhang <keqiao.zhang@intel.com>
@keqiaozhang
Copy link
Contributor Author

keqiaozhang commented Sep 27, 2022

I have updated this PR based on the last comments. Thanks you all for your review and patience. Tests are running.

[Edit]: in internal test run 15728 mtrace works as expected.

case-lib/lib.sh Outdated
else
# TODO: let the kernel driver expose the FW path
# and get the FW path by grepping journalctl.
return 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should die here because we don't know the answer. return 1 means "not Zephyr"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated with die.

marc-hb
marc-hb previously approved these changes Sep 27, 2022
Copy link
Collaborator

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

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

  • You forgot to squash one fixup commit
  • I don't see any mtrace output in internal test run 15728

# when sof logger collect is open
if [ "X$SOF_LOG_COLLECT" == "X1" ]; then
if ! is_ipc4 && [ "X$SOF_LOG_COLLECT" == "X1" ]; then
local loggerBin wcLog; loggerBin=sof-logger
Copy link
Collaborator

Choose a reason for hiding this comment

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

ping?

kv2019i
kv2019i previously approved these changes Sep 27, 2022
Copy link
Contributor

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

Tested on local setup and seems to work fine.

…phyr

This function can check if the running firmware file is built with Zephyr.

Signed-off-by: Keqiao Zhang <keqiao.zhang@intel.com>
The mtrace is only support IPC4 platforms and mtrace should run
while tests are running ATM. If mtrace is not running, DSP will
stop outputting logs when buffer is full. So we need to keep the
mtrace running during the test, or you won't see the error log.

Signed-off-by: Keqiao Zhang <keqiao.zhang@intel.com>
need to kill the mtrace-reader.py process after the test
to prevent the conflicts between each case.

Signed-off-by: Keqiao Zhang <keqiao.zhang@intel.com>
@keqiaozhang
Copy link
Contributor Author

Sorry, I didn't get your point.

  • I don't see any mtrace output in internal test run 15728

Please check the cases under "TGLU_UP_HDA_IPC4ZPH" and "TGLU_RVP_NOCODEC_IPC4ZPH", I added 2 IPC4 platforms to check the mtrace. Others are XTOS-IPC3, Zephyr-IPC3, CAVS-IPC4.

@marc-hb
Copy link
Collaborator

marc-hb commented Sep 28, 2022

Please check the cases under ...

I see it now, sorry for the noise.

@keqiaozhang keqiaozhang merged commit c0729e8 into thesofproject:main Sep 29, 2022
@marc-hb marc-hb added the area:logs Log and results collection, storage, etc. label Jun 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:logs Log and results collection, storage, etc.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants