[mmaka1]

This is a proposal of the first step towards #2172.

New set of macros with compact names and well known level names mapped to the existing underlying trace implementation is proposed. Work to be continued for other components and dais once accepted. Driver level _cl_() macros defined with drv argument for completeness of the interface (drv/comp_p->drv to be used in future to retrieve dai/component info addr - see below).

All components' trace classes are reduced to a common TRACE_CLASS_COMPONENT (dais use a common TRACE_CLASS_DAI re-used from the dai component). Fine filtering, if required, still can be
done in a debug console using trace entry locations. Precise component identification will be restored in the logger tool once uuids are introduced for components and dais, as a much more scalable and conflict-less way to identify them. Adding a dedicated trace class for each individual component and dai and maintaining it as a flat central list does not scale and does not seem as a way to go (while trace classes are still good enough and cheap as a compile time mechanism for parts of the infrastructure).

The trace classes are not moved to the context (as proposed in #2172) which would be required by run-time filtering at the TRACE_CLASS level. Such a move also results in replacing compile time mechanism (zero run-time overhead at the dsp side) to run-time (+extra data to transfer with each entry). Perhaps TRACE_CLASS level filtering, done mostly by the infrastructure developers could be configured at compile time, so that new overhead is not necessary? While extensive component and dai log filtering could be easily implemented for run-time, using uuids, for dai driver and processing component developers?

Note: dai/component info (uuid+nice name+...) will also be stored at compile-time in a debug section copied to the ldc file and used by the logger. So that no uuids, no module names to print do not need to be h/c in the logger and do not need to be kept in run-time dsp memory. Filtering can be done by translating uuid back to debug addresses by the logger and sending them to fw as run-time ids, (+individual instance ids if required for super selective filtering). The pointer to dai/component info retrieved from drv/comp_p->drv and passed to the trace function probably does not have to be sent as a full new dword, just its index in info record array could be computed and packed inside existing entry structure.

[paulstelian97]

I like where this is going. @lgirdwood @jajanusz @dbaluta Any comments?

[paulstelian97]

For the individual component trace classes, wouldn't a redefinition of the component class also be needed? It's possible the component .c file doesn't actually include its own header last but could include other components as well (order of inclusion matters).

[singalsu]

Thanks, I like the more compact looking trace functions!

[mmaka1]

For the individual component trace classes, wouldn't a redefinition of the component class also be needed? It's possible the component .c file doesn't actually include its own header last but could include other components as well (order of inclusion matters).

That is true and that is why I wrote this was "weak" temporary solution to keep existing trace classes until something more scalable is implemented. I believe this is not a big issue if there is some class mismatch until reliable filtering is required/implemented. If the latter is going to happen first, source location based (so inside the makefile) define injected by the compiler could be more reliable.

[paulstelian97]

For the individual component trace classes, wouldn't a redefinition of the component class also be needed? It's possible the component .c file doesn't actually include its own header last but could include other components as well (order of inclusion matters).

That is true and that is why I wrote this was "weak" temporary solution to keep existing trace classes until something more scalable is implemented. I believe this is not a big issue if there is some class mismatch until reliable filtering is required/implemented. If the latter is going to happen first, source location based (so inside the makefile) define injected by the compiler could be more reliable.

I meant doing the #ifdef #undef #define thing in the .c files as well after all the includes, not just in the header files, for the current solution. We will see how this is changed in the future.

[dbaluta]

@mmaka1 Would be nice if we could retrieve component class from dev. Thus, this call would just transform to:

dev_info(dev, "set_fir_func()...");

We can do this in a later series, just writing my thoughts here.

[paulstelian97]

Good catch, I think adding a new field with the component class can be done in here, but my thoughts are about the componentless traces (comp_cl_info etc)

[dbaluta]

So as far as I understand the component from SOF is the equivalent of dev from Linux kernel. We can have two sets of tracing macros:

• for components

#define dev_emerg(dev, fmt, ...)
#define dev_crit(dev, fmt, ...)
#define dev_alert(dev, fmt, ...)
#define dev_err(dev, fmt, ...)
#define dev_warn(dev, fmt, ...)
#define dev_notice(dev, fmt, ...)
#define dev_info(dev, fmt, ...)
#define dev_dbg(dev, fmt, ...)

• for componentless trace we could use an equivalent of pr_* from the kernel

#define pr_emerg(dev, fmt, ...)
#define pr_crit(dev, fmt, ...)
#define pr_alert(dev, fmt, ...)
#define pr_err(dev, fmt, ...)
#define pr_warn(dev, fmt, ...)
#define pr_notice(dev, fmt, ...)
#define pr_info(dev, fmt, ...)
#define pr_debug(dev, fmt...)

We could use our own convention: e.g comp_info instead of dev_info and trace_info instead of pr_info.

We could also think about dynamic debugging with control from the AP.

OR to avoid having two sets of macros we can add a static NULL component and use it for componentless tracing.

[paulstelian97]

@dbaluta Yeah but still the equivalent of the pr_* traces still need to contain a trace class (there isn't a TRACE_CLASS_NULL as far as I know; just move them under the generic component trace class?). That would still be a behavior change and might even affect which traces are enabled vs which are disabled. That would conflict with getting the trace class from the actual component instances.

[jajanusz]

If we really want to have that loose dependency, it'd be better to not have #undef and let it fail on warn, cos it means that something is wrong if you have to redefine it. (But then we will have problem with comps that define traces in headers...)

Personally I don't like that loose dependency at all. Macro shouldn't depend on some symbol that comes from somewhere. But I guess you also don't want to redefine the same macros that differ just by class name over and over to mark that it has dependency on some symbol that contains chosen class name. We can use macro here like make_traces(some_prefix, TRACE_CLASS_*) that generates said set of macros, but metaprogramming in C is also meh.

I like idea of these traces at all, but it's not mergable with how TRACE_CLASS_COMP works atm, even as temporary solution.

[paulstelian97]

Of course the "make_traces" like idea isn't unheard of in this project (see the imx interrupt driver which has some reasonably similar).

So we need to figure out how these traces ought to work. I wonder if certain inline functions can be moved out of headers so the traces (except generic component traces) aren't even used in headers. Any performance risks in that @jajanusz ?

[mmaka1]

@jajanusz TRACE_CLASS_COMP is already defined at that place, so how the warning would help? I explained why I would not maintain any replacement that does not introduce another overhead better than that since we should reduce this to a common TRACE_CLASS_COMP on that level soon.

[paulstelian97]

Having specific classes means that you can enable and disable components at will. So the question is if we want to be able to tune this enabling/disabling per component or it's fine to do per verbosity.

[jajanusz]

@paulstelian97 Xtensa arch doesn't like calls + replacing small functions (like 1 op inline) with call will increase size few times. In ideal world we wouldn't have these inlines in headers, but cos of perf/size we cannot remove them on bigger scale.

[paulstelian97]

@mmaka1 Maybe it's better to just remove TRACE_CLASS. It requires too much maintenance (in fw/logger/testbench) and it's not useful at all (we have well structured components already and you can easily tell from where log is coming).

Removing trace classes outright will be interesting in terms of userspace compatibility as well as the ability to selectively enable and disable traces.

[mmaka1]

@paulstelian97 I believe we want to implement filtering per class. However using statically defined class id per each component will not last long because of reasons I tried to explain in the initial description. A better temporary (1st step) solution might be just to use TRACE_CLASS_COMP by all components and remove those weak redefines. A logger side filtering still could be done by grepping location name while future dsp side fine filtering for individual component implementations should be based on uuid retrieved from the dev object.

Edit: This applies symmetrically to dai tracing (ssp, dmic, ...). There is no common trace class defined but I may re-use TRACE_CLASS_DAI for dai_err/info/dbg() if it's OK.

[paulstelian97]

Hmmm, good point. Anyway this separation is reasonable to do for the component ones but what will we do with componentless traces that simply have to remain componentless (there is no reasonable way to attach a component)?

[mmaka1]

@paulstelian97 Imo, any code that is part of the component (dai) implementation is either executed in context of the initialized component instance or its driver instance. Any other static initializers will probably disappear once the system moves towards dynamic component/dai loading/discovery. And uuid is a property of the driver, so new ..._cl_....() would require the driver ptr and could retrieve the uuid.

[paulstelian97]

@paulstelian97 Imo, any code that is part of the component (dai) implementation is either executed in context of the initialized component instance or its driver instance. Any other static initializers will probably disappear once the system moves towards dynamic component/dai loading/discovery. And uuid is a property of the driver, so new ..._cl_....() would require the driver ptr and could retrieve the uuid.

Hm, alright, this sounds good to me. Driver instances, that sounds interesting (you'd group off statics into a structure or something, that would also have the classes?).

[plbossart]

what's the difference between e.g. pipe_info() and pipe_cl_info()?

is this the moral equivalent of pr_err("blah") vs. dev_err(dev, "blah")

[paulstelian97]

Pretty much, from what I understand, the *_cl_error one is the equivalent of pr_err.

I personally want to have as few of these cl* type prints as possible because without hacks for selecting the active component (like the thing @jajanusz is blocking for) I fear we will end up with some rather coarse selections possible for runtime enabling/disabling in the future.

[mmaka1]

@plbossart Yes, cl is device (/object/instance)-less version and pipe_cl_ was supposed to indicate that there was pipeline trace class included.

[mmaka1]

@paulstelian97 I hope that I already explained that cl version are going to get the driver object, identified by uuid (this one to be introduced soon, along with another proposal for new debug abi) while device version is going to provide that uuid by back ptr to its parent driver as well. So the fine identification will be back on the sof-logger site (no overhead on the dsp side, that would be another compile-time piece of data, nothing new sent over the trace channel).

[paulstelian97]

Ah fair, then guess I'll wait. Yes you did mention it, I just forgot (I need more coffee :) ).

[paulstelian97]

Still assuming RFC, now classes are gone and all traces have been moved to TRACE_CLASS_COMP? Is this even going to build, once you fix the merge conflict?

I want to know where everything is going with this one, to prevent getting a mess of unexpected issues down the road.

[paulstelian97]

I'm not sure this should even be an error in the first place (mentioning in passing). There was a discussion about this but I forgot what the conclusion was.

[mmaka1]

Yes, noticed that one too, could be a warning once there is one for that category. Please note I did not try to fix things (there are even some dangerous uses in some places I spotted while verifying the conversion) since should come as a separate patch I think.

[paulstelian97]

True, this should only actually focus on the trace macros themselves, a follow-up is pretty good to think about though.

[paulstelian97]

Maybe GH shows it wrong but that escape looks too far to the right for me.

[dbaluta]

@mmaka1 Maybe adding an example of usage in the commit message would be useful. Also, are these macros supposed to be used directly by SOF core/drivers or one always needs to create wrappers like comp_err, pipe_err etc.

[paulstelian97]

From what I understand I think these will only be used from wrappers (confirmation would be good though).

[mmaka1]

@dbaluta I re-painted parts that would be forked at the fastest pace imo, to stop maintaining unscalable solution and limit amount of code to re-work in future. Other parts could be re-named in a similar way to have unified naming across the code however it does not seem as critical as the part refactored by this patch at the moment and could be done later.

[paulstelian97]

Re-approving, for the most part this looks good. I still worry a little about the filtering abilities (they have to be done in userspace). Doing some filtering on DSP side will allow the etrace buffer to fill and wrap more slowly which in some cases can be important (when verbose traces are important and there are issues with DMA trace that the last traces don't flush), which will temporarily be a low priority issue.

Other than DSP-side filtering (useful for debug of crashes) everything else looks good. Do check the tests though.

[paulstelian97]

I have noticed you have done the first step towards getting proper trace classes for the comp_cl_* macros, now I think the class can in fact be stored in drv_p and you should pull it from there rather than using TRACE_CLASS_COMP. Sure, each component driver will have to specify its own class as a field in that driver but that's a small price to pay and the compiler can even optimize that access.

[mmaka1]

Not the class, but a ptr to static module info descriptor (or even better a much shorted index to the module info descriptor array, which can be packed into the existing trace data with no side increase if possible) put into the ldc file. I'd keep the class for base parts of the fw while use second level identification (uuid, not anything merged as a flat list) for components and dais. Will explain that in the final description once compilation issues are fixed soon.

[paulstelian97]

Yeah fair, but the class must still be there (not willing to accept breaking backward compatibility in multiple pull requests and this one is complex enough as-is so I want it preserved), at least for now. Changing the format of the trace message I recommend you do in a follow-up.

EDIT: Class can be inside that module. You pass the pointer and the trace macros extract the class.

[mmaka1]

I updated the description and title. Builds and tests are passing. I had to add protection to trace header and final solution would require more serious cleanup but this is another story for another patch (look at the order of using pre-prod directives and other includes). I tried to explain why I do not want to move trace classes to the context, while adding uuids identification to id later (two different use cases, doubt if the former is really worth the overhead). I also tried to explain next steps for uuids and in the meantime users may still easily filter their logs if they need) and I would not restore the old trace classes for components and dais temporarily unless somebody raises serious concern and real issue with that.

[paulstelian97]

Fair enough.

[paulstelian97]

Same comment for dai_cl_*

[paulstelian97]

Will reapprove once that is handled and build stops failing, this looks significantly better now.

[paulstelian97]

@jajanusz Your comment was addressed, any other comments?

@lgirdwood Can you take a look into this?

[paulstelian97]

I'm planning on updating my issue to mention this change, can we say that via this PR we deprecate the current trace classes system, besides the refactor itself? (and also losing out temporarily on the possibility to have a fine tuned filtering DSP side).