trace: avoid passing va_list #4574

ranj063 · 2021-07-29T19:47:48Z

Passing va_list seems to result in issues with stopping
the DMA. Call log_func() and mtrace_dict_entry() in the
_log_message() macro instead to prevent this.

This is an alternative for #4573 but needs to be tested with zephyr. Fixes the suspend/resume with capture issue on helios in my tests

Passing va_list seems to result in issues with stopping the DMA. Call log_func() and mtrace_dict_entry() in the _log_message() macro instead to prevent this. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

ranj063 · 2021-07-29T23:05:25Z

SOFCI TEST

to allow the build to succeed. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

lyakh

I'll test this with Zephyr a bit later

lyakh · 2021-07-30T06:04:10Z

src/include/sof/trace/trace.h

+	(lvl <= MTRACE_DUPLICATION_LEVEL ? \
+		mtrace_dict_entry(true, (uint32_t)&log_entry, \
+				  META_COUNT_VARAGS_BEFORE_COMPILE(__VA_ARGS__), \
+				  ##__VA_ARGS__) : 0); \


The ?: operator is (usually) used for returning a value. Do I understand it correctly that you just meant an if here? Also it would be nice to keep indentation for end-of-line backslashes

lgirdwood · 2021-07-30T10:21:48Z

Lets try for this alternative before the revert as this still keeps the Zephyr trace working. @lyakh please report any Zephyr issues.
Also looks like the UTs are failing to build

cd /quickbuild/workspace1/24733/SOF_FW/build_ut/test/cmocka/src/audio/mixer && /usr/local/bin/cmake -E cmake_link_script CMakeFiles/mixer.dir/link.txt --verbose=1
/localdisk/tools/xtensa/RG-2017.8-linux/XtensaTools/bin/xt-xcc    CMakeFiles/mixer.dir/mixer_test.c.o CMakeFiles/mixer.dir/comp_mock.c.o CMakeFiles/mixer.dir/__/__/notifier_mocks.c.o CMakeFiles/mixer.dir/__/__/__/__/__/src/audio/buffer.c.o CMakeFiles/mixer.dir/__/__/__/__/__/src/audio/mixer.c.o CMakeFiles/mixer.dir/__/__/__/__/__/src/ipc/ipc3/helper.c.o CMakeFiles/mixer.dir/__/__/__/__/__/src/ipc/ipc-common.c.o CMakeFiles/mixer.dir/__/__/__/__/__/src/audio/pipeline/pipeline-graph.c.o CMakeFiles/mixer.dir/__/__/__/__/__/src/audio/pipeline/pipeline-params.c.o CMakeFiles/mixer.dir/__/__/__/__/__/src/audio/pipeline/pipeline-schedule.c.o CMakeFiles/mixer.dir/__/__/__/__/__/src/audio/pipeline/pipeline-stream.c.o CMakeFiles/mixer.dir/__/__/__/__/__/src/audio/pipeline/pipeline-xrun.c.o  -o mixer ../../../libcommon_mock.a -T/quickbuild/workspace1/24733/SOF_FW/build_ut/test/cmocka/memory_mock.x /localdisk/tools/cmocka/build_cavs2x_LX6HiFi3_2017_8/install/lib/libcmocka-static.a ../../../libcommon_mock.a -lm -lgcc -Wl,--no-check-sections -ucall_user_start -Wl,-static -lm 
CMakeFiles/mixer.dir/__/__/__/__/__/src/audio/buffer.c.o: In function `buffer_unlock':
/quickbuild/workspace1/24733/SOF_FW/src/arch/xtensa/include/arch/lib/cache.h:123: undefined reference to `mtrace_dict_entry'
/quickbuild/workspace1/24733/SOF_FW/src/arch/xtensa/include/arch/lib/cache.h:123: undefined reference to `mtrace_dict_entry'
CMakeFiles/mixer.dir/__/__/__/__/__/src/audio/buffer.c.o: In function `buffer_alloc':
/quickbuild/workspace1/24733/SOF_FW/src/audio/buffer.c:44: undefined reference to `mtrace_dict_entry'
/quickbuild/workspace1/24733/SOF_FW/src/audio/buffer.c:48: undefined reference to `mtrace_dict_entry'
/quickbuild/workspace1/24733/SOF_FW/src/audio/buffer.c:35: undefined reference to `mtrace_dict_entry'
CMakeFiles/mixer.dir/__/__/__/__/__/src/audio/buffer.c.o:/quickbuild/workspace1/24733/SOF_FW/src/audio/buffer.c:81: more undefined references to `mtrace_dict_entry' follow
test/cmocka/src/audio/mixer/CMakeFiles/mixer.dir/build.make:263: recipe for target 'test/cmocka/src/audio/mixer/mixer' failed
make[2]: *** [test/cmocka/src/audio/mixer/mixer] Error 2
make[2]: Leaving directory '/quickbuild/workspace1/24733/SOF_FW/build_ut'
CMakeFiles/Makefile2:1645: recipe for target 'test/cmocka/src/audio/mixer/CMakeFiles/mixer.dir/all' failed

plbossart

I have zero understanding of the relationships between the valist and APL build issues, and likewise why the use of the valist results in a DMA fail on CML only.
I would prefer a clean revert to this experiment-based approach where we really have no idea what we are doing. Nothing personal, I just feel like there is a different problem we haven't even root-cause behind all this.

plbossart · 2021-07-30T10:50:39Z

src/arch/xtensa/configs/apollolake_defconfig

+CONFIG_COMP_KPB=n
+CONFIG_COMP_SEL=n
+CONFIG_COMP_CROSSOVER=n
+CONFIG_COMP_DRC=n


A change to be va_list leads to a build failure?
Something's not right here...

@plbossart i've made the _log_message() macro heavier with this change and so it breaks the bss section for APL

lyakh · 2021-07-30T13:41:02Z

src/arch/xtensa/configs/apollolake_defconfig

 CONFIG_COMP_SRC_SMALL=y
 CONFIG_COMP_TDFB=n
 CONFIG_COMP_TONE=n
+CONFIG_COMP_KPB=n


btw, we have a weird sof-apl-keyword-detect.m4 which presumably uses KPB on APL, although FWIW my brief test of that topology on UP2 wasn't very successful, so maybe it's broken anyway

lyakh · 2021-07-30T14:38:42Z

I'll test this with Zephyr a bit later

@ranj063 Tested-by: @lyakh

ranj063 · 2021-07-30T16:13:46Z

I would prefer a clean revert to this experiment-based approach where we really have no idea what we are doing. Nothing personal, I just feel like there is a different problem we haven't even root-cause behind all this.

I dont mind this @plbossart but your PR seems to show failures on 2 other devices that I dont see with mine. Can you please look into it?

ranj063 · 2021-07-30T16:32:29Z

Lets try for this alternative before the revert as this still keeps the Zephyr trace working. @lyakh please report any Zephyr issues.

@lgirdwood I'm not able to decipher this failure. the function is defined in trace.c just as before except the signature is changed. I'm not sure why we're seeing this failure. Do you have any hints for me?

plbossart · 2021-07-30T17:15:43Z

I would prefer a clean revert to this experiment-based approach where we really have no idea what we are doing. Nothing personal, I just feel like there is a different problem we haven't even root-cause behind all this.

I dont mind this @plbossart but your PR seems to show failures on 2 other devices that I dont see with mine. Can you please look into it?

The errors you are referring to are a completely unrelated failure on CML_SDW on trig-start. ADL has a similar issue.

I've spent enough time on these firmware issues, my position is that we have to revert all of the CML issues.

lgirdwood · 2021-07-30T20:31:32Z

This actually looks like the correct fix (and aligns with Zephyr variadic tracing APIs) as I'm 99% sure now this is a compiler bug in the libgcc handling of the xtensa calling convention wrt to register and stack window spilling/shifting. e.g. the GCC va_list is

typedef struct {
   unsigned int gp_offset;
   unsigned int fp_offset;
   void *overflow_arg_area;
   void *reg_save_area;  <<< this will probably break as the registers may be shifted by 4, 8 or 12 in HW and the mapping could lose alignment
} va_list[1];

XCC handles this as it has a better understanding of the window shifting/spilling and a different calling conventions ABI compared to GCC. The va_list definition in XCC may also have extra members to deal with windows spill/shift.

lgirdwood · 2021-07-30T21:45:37Z

@ranj063 are you able to rework this next week ?

ranj063 · 2021-07-30T21:52:12Z

@ranj063 are you able to rework this next week ?

@lgirdwood yes, I can squash this with Marc's original commits and resubmit next week.

ranj063 · 2021-07-30T21:52:47Z

closing for now. will resubmit next week

ranj063 requested a review from akloniex as a code owner July 29, 2021 19:47

ranj063 force-pushed the fix/trace_cml branch from 00db028 to 6b2f74c Compare July 29, 2021 19:49

ranj063 requested review from dbaluta, lbetlej, lgirdwood, mmaka1 and plbossart as code owners July 29, 2021 19:49

ranj063 force-pushed the fix/trace_cml branch from 6b2f74c to 1104149 Compare July 29, 2021 19:53

trace: avoid passing va_list

c4744ab

Passing va_list seems to result in issues with stopping the DMA. Call log_func() and mtrace_dict_entry() in the _log_message() macro instead to prevent this. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

ranj063 force-pushed the fix/trace_cml branch from 1104149 to c4744ab Compare July 29, 2021 20:02

APL defconfig: remove some unnecessary components

757755a

to allow the build to succeed. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

ranj063 force-pushed the fix/trace_cml branch from c408061 to 757755a Compare July 30, 2021 01:52

lyakh requested changes Jul 30, 2021

View reviewed changes

plbossart reviewed Jul 30, 2021

View reviewed changes

lyakh reviewed Jul 30, 2021

View reviewed changes

ranj063 closed this Jul 30, 2021

trace: avoid passing va_list #4574

trace: avoid passing va_list #4574

Uh oh!

Conversation

ranj063 commented Jul 29, 2021

Uh oh!

ranj063 commented Jul 29, 2021

Uh oh!

lyakh left a comment

Choose a reason for hiding this comment

Uh oh!

lyakh Jul 30, 2021

Choose a reason for hiding this comment

Uh oh!

lgirdwood commented Jul 30, 2021

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

plbossart Jul 30, 2021

Choose a reason for hiding this comment

Uh oh!

ranj063 Jul 30, 2021

Choose a reason for hiding this comment

Uh oh!

lyakh Jul 30, 2021

Choose a reason for hiding this comment

Uh oh!

lyakh commented Jul 30, 2021

Uh oh!

ranj063 commented Jul 30, 2021

Uh oh!

ranj063 commented Jul 30, 2021

Uh oh!

plbossart commented Jul 30, 2021

Uh oh!

lgirdwood commented Jul 30, 2021

Uh oh!

lgirdwood commented Jul 30, 2021

Uh oh!

ranj063 commented Jul 30, 2021

Uh oh!

ranj063 commented Jul 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants