audio: cadence: add optional API error reporting#10604
audio: cadence: add optional API error reporting#10604lgirdwood merged 1 commit intothesofproject:mainfrom
Conversation
ujfalusi
requested review from
dbaluta,
kv2019i,
lbetlej,
lgirdwood,
mmaka1 and
plbossart
as code owners
There was a problem hiding this comment.
Pull request overview
This PR extends the Cadence codec processing path to optionally report the underlying Cadence API error code to callers, while keeping the existing behavior where non-fatal API errors do not fail processing.
Changes:
- Extend
cadence_codec_process_data()with an optionalapi_error_codeoutput parameter. - Suppress non-fatal warning logs for selected Cadence API calls and add debug tracing when a stored API error code is overwritten.
- Update IPC3 and IPC4 Cadence module call sites to pass
NULLfor the new optional parameter.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/include/sof/audio/module_adapter/module/cadence.h | Updates the public function prototype to accept an optional API error output parameter. |
| src/audio/module_adapter/module/cadence.c | Implements optional API error code storage and overwrite tracing; suppresses non-fatal warning prints on the updated path. |
| src/audio/module_adapter/module/cadence_ipc3.c | Adapts the IPC3 processing call site to the new function signature by passing NULL. |
| src/audio/module_adapter/module/cadence_ipc4.c | Adapts the IPC4 processing call site to the new function signature by passing NULL. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if (*api_error_code && *api_error_code != ret) | ||
| comp_dbg(dev, "overwriting api error code at %s: old %#x new %#x", | ||
| cmd_name, *api_error_code, ret); |
There was a problem hiding this comment.
comp_dbg() uses %#x to print int values (ret and *api_error_code). In C this format expects an unsigned int, so this can trigger format warnings/UB for negative (high-bit-set) error codes. Consider casting to unsigned (or switching the stored type to an unsigned/UWORD32) when printing.
| cmd_name, *api_error_code, ret); | |
| cmd_name, (unsigned int)*api_error_code, (unsigned int)ret); |
There was a problem hiding this comment.
well, the cadence codebase and defines are sloppy, this is what it is.
The error is described as bits (bit 31 - fatal, bit 11-14 class, etc), but defined as int.
| uint32_t direction); | ||
| int cadence_codec_apply_params(struct processing_module *mod, int size, void *data); | ||
| int cadence_codec_process_data(struct processing_module *mod); | ||
| int cadence_codec_process_data(struct processing_module *mod, int *api_error_code); |
There was a problem hiding this comment.
The new output parameter uses int *api_error_code, but Cadence defines XA_ERRORCODE (see xa_type_def.h). Using XA_ERRORCODE * here would make the API intent explicit and avoid accidental type mismatches if Cadence ever changes the underlying type.
| int cadence_codec_process_data(struct processing_module *mod, int *api_error_code); | |
| int cadence_codec_process_data(struct processing_module *mod, XA_ERRORCODE *api_error_code); |
There was a problem hiding this comment.
right, can be done, but the same 'int' is returned in case of error, again, it is not a purpose of the patch to correct the cadence module sloppy code.
Extend cadence_codec_process_data() with an optional output parameter for returning Cadence API error codes while preserving existing return behavior: non-fatal API errors still return 0 and fatal errors are returned. Suppress non-fatal warning prints in this path and add lightweight overwrite tracing for repeated API error updates. Update cadence IPC3 and IPC4 call sites to pass NULL for now. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
ujfalusi
force-pushed
the
peter/pr/cadence_silence_nonfatal
branch
from
c44f608 to
d088eb0
Compare
|
Changes since v1:
|
| API_CALL(cd, XA_API_CMD_INPUT_OVER, 0, NULL, ret); | ||
| if (ret != LIB_NO_ERROR) { | ||
| if (api_error_code) | ||
| *api_error_code = ret; |
There was a problem hiding this comment.
not cadence_store_api_error_code()?
There was a problem hiding this comment.
No, as this is the first call which can generate return code, it will not override previous code.
|
@ujfalusi can you check CI thanks ! |
|
zmain fails are not related, since the compr is not tested in CI, this change cannot affect tests, I don't think internal CI is testing compr either, but will try to find the fail log to have an idea. |
|
@lrudyX good to merge, dont think this is tested in internal CI. |
|
we can re-trigger CI on this to be certain? |
| comp_err(dev, "processing failed with error: %x", ret); | ||
| return ret; | ||
| } | ||
| comp_warn(dev, "processing failed with nonfatal error: %x", ret); |
There was a problem hiding this comment.
what if we instead just simply change the level to DBG for the non fatal codes?
There was a problem hiding this comment.
these codes are on the other hand are codec specific, the same reason code can mean completely different thing for mp3, ogg, flac, etc. Decoding is really not that simple.
Or, keep the prints as comp_dbg() and still have the ability to return the reason code if the caller wants to handle non fatal?
There was a problem hiding this comment.
Lets see how this works in practice, if we need more info we can add it later.
Extend cadence_codec_process_data() with an optional output parameter for returning Cadence API error codes while preserving existing return behavior: non-fatal API errors still return 0 and fatal errors are returned.
Suppress non-fatal warning prints in this path and add lightweight overwrite tracing for repeated API error updates.
Update cadence IPC3 and IPC4 call sites to pass NULL for now.