Audio: Aria: Fix handling of S24_LE format, update blob ABI headers in topologies #9614
Conversation
| @@ -57,17 +57,19 @@ inline void aria_algo_calc_gain(struct aria_data *cd, size_t gain_idx, | |||
| inu = AE_LA128_PP(in); | |||
| for (i = 0; i < n; i += 4) { | |||
| AE_LA32X2X2_IP(in_sample, in_sample1, inu, in); | |||
| in_sample = AE_MAXABS32S(in_sample, in_sample1); | |||
| max_data = AE_MAXABS32S(max_data, in_sample); | |||
| max_data = AE_MAXABS32S(max_data, AE_SLAI32(in_sample1, 8)); | |||
There was a problem hiding this comment.
is this intentional? So in_sample below is used as set in line 59 above?
There was a problem hiding this comment.
Yes, it's intentional. I think this is more clear, and no need to have left shift for both first AE_MAXABS32S input arguments.
There was a problem hiding this comment.
Also, now all (generic, hifi3/4, hifi5) versions of Aria are bit exact with the input that I use.
src/audio/aria/aria.c
Outdated
| return -EINVAL; | ||
| } | ||
| memcpy_s(&cd->att, sizeof(uint32_t), fragment, sizeof(uint32_t)); | ||
| cd->att = (size_t)(*(uint32_t *)fragment); |
There was a problem hiding this comment.
so presumably fragment is always aligned?
There was a problem hiding this comment.
I don't know it's ensured... should I then memcpy() it to a local uint32_t and then type cast it to Aria's component data. I think it was a mistake to use size_t for most of Aria's internal state. But I didn't want to change everything with this fix.
There was a problem hiding this comment.
It doesn't look like the align is ensured to be aligned. Even in handler.c ipc4_set_vendor_config_module_instance drv->ops.set_large_config() the type is char. Maybe it's best that I change the Aria component data type to uint32_t and keep the memcpy().
There was a problem hiding this comment.
You could just fix the types instead ? Sounds like we need to do it anyway ?
There was a problem hiding this comment.
Yes, optimizing the code needs fixing the types.
There was a problem hiding this comment.
Fixed now, it needed only one aria.h edit to change the memcpy destination type.
There was a problem hiding this comment.
It doesn't look like the align is ensured to be aligned. Even in handler.c ipc4_set_vendor_config_module_instance drv->ops.set_large_config() the type is char. Maybe it's best that I change the Aria component data type to uint32_t and keep the memcpy().
@singalsu there isn't a huge difference (supposedly on all 32-bit platforms, at least on Xtensa) between size_t and uint32_t, both are unsigned 32-bit integers. But if this should run on 64-bit ARM cores too, then there could be a difference
There was a problem hiding this comment.
It failed in testbench 64bit x86 build.
The bits 31:24 are defined as don't care for S24_LE format while the code in functions aria_algo_calc_gain() and aria_algo_get_data() can work only if the bits contain the sign extension. If such data is received, the processed audio output is be corrupt. The generic C version is fixed with use of helper function sign_extend_s24(). Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
These changes implement the missing sign extension handling for input data. The highest 8 bits in S24_LE format are don't care. The presence of sign extension can't be assumed. The max of max_data can be moved out from while loop as a small improvement. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
These changes implement the missing sign extension handling for input data. The highest 8 bits in S24_LE format are don't care. The presence of sign extension can't be assumed. The max of the two max values can be moved out from while loop as a small improvement. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
The impact of parameter attenuation applied at aria_init() and in bytes control blob is explained in more clear way. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
It depends on architecture but size_t and uint32_t are not necessarily bits compatible (on 64bit arch like testbench on x86). Therefore the type for att in component data of Aria is changed to uint32_t. A similar check as in aria_init() is added to avoid an illegal value in the blob to break the processing algorithm. An info level trace is added to see in trace if the blob is applied by the Linux topology or in run-time. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
singalsu
force-pushed
the
aria_s24_format_fixes
branch
from
dbef83a
to
ea6548d
Compare
| 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, | ||
| 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, | ||
| 0x01, 0x00, 0x00, 0x00" | ||
| } |
There was a problem hiding this comment.
When looking at the quite similar hex values of the blob it might be that this would have worked if I had first done the size_t vs. uint32_t fix to code that failed on 64 bit build. I think I should rewrite the commit message to say it's a blob ABI update and blobs added for all allowed configurations of Aria.
The blob for Aria is updated for current ABI version and with options for all possible configurations. The new blobs are passthrough.conf, and blobs for parameter values 1 - 3. The benchmark topologies are configured to to use parameter value 2 (+12 dB level boost target) and parameter value 1 (+6 dB boost target). Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
singalsu
force-pushed
the
aria_s24_format_fixes
branch
from
ea6548d
to
6392107
Compare
singalsu
changed the title
|
@lgirdwood all green |
plus other smaller