Trace: dai unified identification and cleanup #2287
Conversation
src/drivers/intel/cavs/dmic.c
Outdated
There was a problem hiding this comment.
Why did you cut it to just d? dmic felt ok and d can be interpreted as debug.
There was a problem hiding this comment.
trace_ macros names are too long (esp. the version with ids) imo, and they are sometimes followed by very long messages which creates too long lines. I'd keep them short as long as they do not collide (preprocessor will let you know). Btw it could default to trace() as well if trace class is redef-ed appropriately locally.
There was a problem hiding this comment.
Would a scheme such as trace_t (for trace_event, t from trace), trace_e (for trace_error) and trace_v (for trace_verbose), and trace_it/trace_ie/trace_iv (for the counterparts with IDs) be good? This would also not include the component name where it isn't needed but breaks down if you ever need to use these traces outside the implementation C file.
There was a problem hiding this comment.
I think our end goal should be the API described by @paulstelian97, where we can also pass in our object handle i.e.
trace_err(object, "blah blah");
The object would contain all the necessary ID, control data so that we could turn trace ON/OFF from userspace.
There was a problem hiding this comment.
@lgirdwood @paulstelian97 Indeed the end goal would be to have something similar with dev_dbg/dev_err/dev_info etc in the Linux kernel.
Where the object passed as the first argument will keep the whole information needed.
There was a problem hiding this comment.
We can try do to what you suggest, but for this PR what we have here is enough. It's more of a cleanup that will also make refactoring that u plan easier
lgirdwood
left a comment
lgirdwood
left a comment
There was a problem hiding this comment.
I'm ok with the shortened trace_ calls atm, but our end goal should be the standardised API with object handles for userspace control.
src/drivers/intel/cavs/dmic.c
Outdated
There was a problem hiding this comment.
I think our end goal should be the API described by @paulstelian97, where we can also pass in our object handle i.e.
trace_err(object, "blah blah");
The object would contain all the necessary ID, control data so that we could turn trace ON/OFF from userspace.
|
Global trace renaming seems to be a longer task, so just reverted macros names (kept swapped trace_error_dmic, just in this unit, so a bit inconsistent but a bit more readable) to make a progress with this one and focus on proposed reuse of log entry ids for dai type.index (another one is useful dmic log reduction on the non-verbose level). |
paulstelian97
left a comment
paulstelian97
left a comment
There was a problem hiding this comment.
One nitpick: I'm not a fan of names like "trace_error_[comp]_id", it's mixing up the generic and specific parts. That said, this can be fixed in a future PR, the rest of the work here is good so I'll approve.
|
@mmaka1 can you rebase, I will then re-run CI, as I think issue was CI and not FW related. |
singalsu
left a comment
singalsu
left a comment
There was a problem hiding this comment.
Looks good. Thanks for cleaning up DMIC trace output!
|
Due to merge of PR#2303 this PR must be rebased or test results for KD tests with audio format 24b/32b ignored. |
|
SOFCI TEST |
Since this is the code of 'dai component', the trace macros are renamed to avoid collisions with names of 'dai' (ssp, dmic, ...) trace macros. Signed-off-by: Marcin Maka <marcin.maka@linux.intel.com>
Trace entry fields for IDs may be used by dai traces for type and index to track down each trace entry to a specific dai instance. Signed-off-by: Marcin Maka <marcin.maka@linux.intel.com>
Dmic instance should be identifiable in traces. Signed-off-by: Marcin Maka <marcin.maka@linux.intel.com>
Parameter verification should be done before any expensive resource allocations happen. Signed-off-by: Marcin Maka <marcin.maka@linux.intel.com>
Follows the notation used for id's by the sof logger tool. Type is followed by the index. Signed-off-by: Marcin Maka <marcin.maka@linux.intel.com>
8 participants
Ssp and other dai traces should follow the dmic example once there is no objection on re-using trace entry id's for dai type and index (instead of sending them sometimes as arguments).