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

[Srinivas Kandagatla]

On 5/20/26 10:11 PM, Alexey Klimov wrote:
> 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@xxxxxxxxxxxxxxxx>
>> ---
>>  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.
> 
thanks for the review Alexey,
Yes, its okay, its flexible array in that struct , as long we allocate
enough space at the end equal to number of levels its fine. This is
exactly what we do in
payload_size = sizeof(*event) + sizeof(*level) + num_levels *
sizeof(uint32_t);

> - level->level[] is of u32 types. Can level->level[i] = (i + 1) * bytes
> overflow?you mean overflow u32 bit value? Technically yes, but the level is
period bytes are limited to max of period size and num of periods which
is BUFFER_MAX SIZE that is 64k*8,


--srini

> 
> [..]
>> +struct event_cfg_sh_mem_pull_push_mode_watermark_t {
>> +	uint32_t num_water_mark_levels;
>> +	uint32_t level[];
>> +} __packed;
> [..]
> 
> Best regards,
> Alexey