Skip to content

Conversation

@singalsu
Copy link
Collaborator

No description provided.

@singalsu singalsu force-pushed the sound_dose branch 6 times, most recently from f038d75 to 710ed6d Compare June 16, 2025 16:52
Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just some opens.

@singalsu singalsu force-pushed the sound_dose branch 3 times, most recently from 6f6db17 to a608928 Compare June 24, 2025 09:33
Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, think this looks ready for review

@singalsu singalsu changed the title [very draft - don't review] Audio: Sound Dose: Add new component Audio: Sound Dose: Add new component Jun 27, 2025
Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you went with "notifications with timestamp" instead of a compressed stream of data. I guess this works as well, doesn't take DMA resources, and we can always add a DMA option later one. Added one comment about the timestamp accuracy if this is put in the common header.

};

struct sof_audio_feature {
int64_t stream_time_ms;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is a common feature header, I wonder if "ms" is accurate enough? Maybe even report just number of frames since start at let user-space/app convert. For sound does, this would be 1ms increments.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Number of frames depends on content sample rate, so I thought the time as relative from start is more generic. Not sure if sound dose user space part has connection to the part that decodes stream. But we could have both in the struct?

Looks microseconds would work too, wrap in 580000 years so it would be more generic if need for high accuracy.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done now

@singalsu singalsu force-pushed the sound_dose branch 2 times, most recently from c03beba to def60f6 Compare August 15, 2025 12:15
Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just minor comments, needs more documentation, I would add a small ReadMe.md describing the flow and linking to the Android docs.

<include/components/tdfb.conf>
<include/components/template_comp.conf>
<include/components/micsel.conf>
<include/components/sound_dose.conf>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the commit message describe how test topology a bit more, e.g. the pipeline used to test.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

ipc_msg_send(cd->msg, NULL, false);
}

/* IPC4 controls handler */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this is the main transport for data to the host, can you describe the flow in more detail.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added comments text

@singalsu singalsu force-pushed the sound_dose branch 5 times, most recently from 3aaf3a2 to 7f4e26d Compare September 17, 2025 13:25
@singalsu singalsu marked this pull request as ready for review September 17, 2025 13:38
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 46 out of 46 changed files in this pull request and generated 4 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +13 to +15
#include <stdint.h>

#define SOUND_DOSE_DEBUG 0
Copy link

Copilot AI Sep 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Consider using a Kconfig option instead of a hardcoded debug flag to allow runtime configuration of debug output without recompilation.

Suggested change
#include <stdint.h>
#define SOUND_DOSE_DEBUG 0
#include <stdint.h>
#include <sof/common.h>
#ifdef CONFIG_SND_SOC_SOF_SOUND_DOSE_DEBUG
#define SOUND_DOSE_DEBUG 1
#else
#define SOUND_DOSE_DEBUG 0
#endif

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kconfig is a recompile and I'd not like to expose it globally. I find it easier to set it from this header file than going through "ninja -C build-xxx menuconfig"

Comment on lines +89 to +91
/* TODO: 96 kHz and 192 kHz with integer decimation factor. The A-weight is not
* defined above 20 kHz, so high frequency energy is not needed. Also it will
* help keep the module load reasonable.
Copy link

Copilot AI Sep 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This TODO comment should include a ticket number or timeline for when this feature will be implemented, as it affects the component's supported sample rates.

Suggested change
/* TODO: 96 kHz and 192 kHz with integer decimation factor. The A-weight is not
* defined above 20 kHz, so high frequency energy is not needed. Also it will
* help keep the module load reasonable.
/* TODO (TICKET-1234): 96 kHz and 192 kHz with integer decimation factor. The A-weight is not
* defined above 20 kHz, so high frequency energy is not needed. Also it will
* help keep the module load reasonable.
* See TICKET-1234 for progress and timeline.

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to discuss this with my manager, it depends on overall priorities.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine, keep the TODO.

Comment on lines 128 to 129
sample = sat_int16(Q_MULTSR_32X32((int64_t)cd->gain, *x,
30, 15, 15));
Copy link

Copilot AI Sep 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The magic numbers for Q format (30, 15, 15) should be defined as named constants to improve code readability and maintainability.

Copilot uses AI. Check for mistakes.
Comment on lines 226 to 227
sample = sat_int32(Q_MULTSR_32X32((int64_t)cd->gain, *x,
30, 31, 31));
Copy link

Copilot AI Sep 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Similar to the S16 case, the Q format constants (30, 31, 31) should be defined as named constants for better maintainability.

Copilot uses AI. Check for mistakes.
Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blockers!

normal topology file. Or alternative use module options
tplg_filename and tplg_path for module snd_sof.

Get kcontrol_events from https://github.com/ujfalusi/kcontrol_events
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ujfalusi please upstream this test too in here in a follow up PR. Thanks !

@singalsu singalsu force-pushed the sound_dose branch 2 times, most recently from 85f5ff2 to f53a92d Compare September 19, 2025 14:10
@lgirdwood
Copy link
Member

@lrudyX ping - looks like CI got stuck ?

@singalsu singalsu force-pushed the sound_dose branch 2 times, most recently from 1452a41 to 81ec41b Compare September 23, 2025 13:19
@singalsu
Copy link
Collaborator Author

@lgirdwood No changes, just pushed a rebase to restart CI checks.

@singalsu
Copy link
Collaborator Author

@lrudyX The two required CI checks seem to have got stuck since 19 hours when I re-pushed. The previous failure seemed to be CI internal, not caused by these code changes.

@singalsu
Copy link
Collaborator Author

Now I added toml for wcl platform, not sure, hoping it was the issue with rimage in build.

@singalsu
Copy link
Collaborator Author

There's a fail in https://sof-ci.01.org/sof-pr-viewer/#/build/PR10058/build14749499 with MTL TestRtcAec. I wonder how this PR can impact it.

[    0.543563] <inf> ipc: ipc_cmd: rx  : 0x40001000|0x12010026
[    0.544001] <wrn> ipc: ipc4_get_drv: the provided UUID (b780a0a6-269f-466f-b477-23dfa05af758) can't be found!
[    0.545855] <wrn> llext: llext_link_plt: PLT: cannot find idx 14 name 
[    0.546508] <wrn> llext: llext_link_plt: PLT: cannot find idx 14 name 
[    0.557715] <err> llext: do_llext_load: Failed to link, ret -2

@kv2019i
Copy link
Collaborator

kv2019i commented Sep 26, 2025

@singalsu This had the QB fail still, so I now merged #10236 as it starts to block other PRs. IOW rebase is needed for this one,

The exported header file is missing the include for stdint.h
for the used uint32_t type. Without it some builds fail to
warning. Also the copyright text is updated.

This patch also adds static const to the array declaration.

Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
Need to drop the TLV header (used for Linux kernel bytes control)
from data to keep the existing format after the header was added
to sof-ctl generated data header. This header is not passed to
firmware, only the kernel that this usage of filter coefficients
passes, is using it.

Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
This patch adds script sof_sound_dose_time_domain_filters.m
that exports IIR and FIR coefficients to approximate A-weight
function. The current choice is IIR only for lower MCPS load.

The script sof_sound_dose_blobs.m creates a few control blobs
to test the sound_dose component.

A simple script sof_sound_dose_ref.m to compute dBFS and MEL
for a wav file is added to compare with firmware reported values.

Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
The added plot is useful especially with parametric IIR
equalizer tuning to see achieved response error vs. target.
A numerical mean(abs()) value of error is printed to help
see filter parameters change impact.

Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
The build topologies are
- sof-hda-benchmark-sound_dose32.tplg
- sof-mtl-sdw-benchmark-sound_dose32-sdw0.tplg
- sof-mtl-sdw-benchmark-sound_dose32-simplejack.tplg

The controls are for example initialized to
- sensitivity 100 dB, 0 dBFS equals 100 dBSPL (a worst case loudness)
- volume 0 dB, assumes codec gain for headphones is set to max
- gain 0 dB, user's music playback is not attenuated

Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
# ----------------------------------------

config COMP_SOUND_DOSE
default y
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the CI issue persists, @lgirdwood proposed to limit the feature to newer platforms. @singalsu You can remove this bit completely and leave default as not set. For all the newer Intel platforms (ace20 and newer), sound dose will get enabled as a module.

OTOH, I don't see how enabling the component on MTL could break the stub loadable modules...

This patch adds a new SOF component Sound Dose. The purpose is
to calculate for audio playback MEL values (momentary sound
exposure level) to provide to user space the data to compute the
sound dose CSD as defined in EN 50332-3.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
@lrudyX
Copy link

lrudyX commented Sep 30, 2025

Looks like the last change has significant impact on test result. Now we have PASS on internal CI.

@lgirdwood lgirdwood merged commit 951a27c into thesofproject:main Sep 30, 2025
27 of 47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants