Re: [PATCH 5/6] ASoC: qcom: q6apm: add watermark event support

["Alexey Klimov" <alexey.klimov@linaro.org>]

On Tue May 19, 2026 at 2:15 PM BST, Srinivas Kandagatla wrote:
> Push-pull shared memory modules can report watermark events when the DSP
> read/write index reaches configured circular buffer levels.
>
> Add support for registering watermark levels with the shared memory module
> and route the resulting module event to q6apm clients using a new
> APM_CLIENT_EVENT_WATERMARK_EVENT event.
>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
> ---
>  sound/soc/qcom/qdsp6/audioreach.c | 36 ++++++++++++++++++++++
>  sound/soc/qcom/qdsp6/audioreach.h | 50 +++++++++++++++++++++++++++++++
>  sound/soc/qcom/qdsp6/q6apm.c      | 19 ++++++++++++
>  sound/soc/qcom/qdsp6/q6apm.h      |  2 ++
>  4 files changed, 107 insertions(+)
>
> diff --git a/sound/soc/qcom/qdsp6/audioreach.c b/sound/soc/qcom/qdsp6/audioreach.c
> index c984b12409dd..e6e9eb2e85aa 100644
> --- a/sound/soc/qcom/qdsp6/audioreach.c
> +++ b/sound/soc/qcom/qdsp6/audioreach.c
> @@ -1118,6 +1118,42 @@ static int audioreach_pcm_set_media_format(struct q6apm_graph *graph,
>  	return q6apm_send_cmd_sync(graph->apm, pkt, 0);
>  }
>  
> +int audioreach_shmem_register_event(struct q6apm_graph *graph, int bytes, int num_levels)
> +{
> +	struct apm_module_register_events *event;
> +	struct event_cfg_sh_mem_pull_push_mode_watermark_t *level;
> +	int i, payload_size;
> +	struct gpr_pkt *pkt __free(kfree) = NULL;
> +	void *p;
> +
> +	if (num_levels <= 0 || bytes <= 0)
> +		return -EINVAL;
> +
> +	payload_size = sizeof(*event) + sizeof(*level) + num_levels * sizeof(uint32_t);
> +
> +	pkt = audioreach_alloc_cmd_pkt(payload_size, APM_CMD_REGISTER_MODULE_EVENTS, 0,
> +				     graph->port->id, graph->shm_iid);
> +	if (IS_ERR(pkt))
> +		return PTR_ERR(pkt);
> +
> +	p = (void *)pkt + GPR_HDR_SIZE + APM_CMD_HDR_SIZE;
> +
> +	event = p;
> +	event->module_instance_id = graph->shm_iid;
> +	event->event_id = EVENT_ID_SH_MEM_PULL_PUSH_MODE_WATERMARK;
> +	event->is_register = 1;
> +	event->event_config_payload_size = sizeof(*level) + num_levels * sizeof(uint32_t);
> +	p += sizeof(*event);
> +	level = p;
> +	level->num_water_mark_levels = num_levels;
> +
> +	for (i = 0; i < num_levels; i++)
> +		level->level[i] = (i + 1) * bytes;

Let's hope that compilers optimize this to avoid multiplication, anyway
it shouln't be a hot path, although I don't really know.

- don't you need a check like num_levels<=ARRAY_SIZE(level->level) before
this loop? Looking at the code it seems that it relies on pkt which is
allocated using audioreach_alloc_cmd_pkt() taking payload_size into
account. Looks okay but I wanted to double check.

- level->level[] is of u32 types. Can level->level[i] = (i + 1) * bytes
overflow?

[..]
> +struct event_cfg_sh_mem_pull_push_mode_watermark_t {
> +	uint32_t num_water_mark_levels;
> +	uint32_t level[];
> +} __packed;
[..]

Best regards,
Alexey