Re: [PATCH v8 14/22] ASoC: qdsp6: audioreach: add basic pkt alloc support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Thanks Amadeusz for reviewing,

On 28/09/2021 09:00, Amadeusz Sławiński wrote:
On 9/27/2021 3:55 PM, Srinivas Kandagatla wrote:
Add basic helper functions for AudioReach.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>
---
  sound/soc/qcom/qdsp6/audioreach.c | 265 ++++++++++++
  sound/soc/qcom/qdsp6/audioreach.h | 668 ++++++++++++++++++++++++++++++
  2 files changed, 933 insertions(+)
  create mode 100644 sound/soc/qcom/qdsp6/audioreach.c
  create mode 100644 sound/soc/qcom/qdsp6/audioreach.h

diff --git a/sound/soc/qcom/qdsp6/audioreach.c b/sound/soc/qcom/qdsp6/audioreach.c
new file mode 100644
index 000000000000..34ec4c0d0175
--- /dev/null
+++ b/sound/soc/qcom/qdsp6/audioreach.c
@@ -0,0 +1,265 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2020, Linaro Limited
+
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/soc/qcom/apr.h>
+#include <dt-bindings/soc/qcom,gpr.h>
+#include "audioreach.h"
+
+/* SubGraph Config */
+struct apm_sub_graph_data {
+    struct apm_sub_graph_cfg sub_graph_cfg;
+    struct apm_prop_data perf_data;
+    struct apm_sg_prop_id_perf_mode perf;
+    struct apm_prop_data dir_data;
+    struct apm_sg_prop_id_direction dir;
+    struct apm_prop_data sid_data;
+    struct apm_sg_prop_id_scenario_id sid;
+
+} __packed;
+
+#define APM_SUB_GRAPH_CFG_NPROP    3
+
+struct apm_sub_graph_params  {
+    struct apm_module_param_data param_data;
+    uint32_t num_sub_graphs;
+    struct apm_sub_graph_data sg_cfg[];
+} __packed;
+
+#define APM_SUB_GRAPH_PSIZE(n) ALIGN(sizeof(struct apm_sub_graph_params) + \
+                n * sizeof(struct apm_sub_graph_data), 8)

This looks, like you could use struct_size helper, something like:
#define APM_SUB_GRAPH_PSIZE(n) ALIGN(struct_size(apm_sub_graph_params, apm_sub_graph_data, n), 8)


struct_size() macro first argument is structure pointer, so above will not work as it is.

I agree that we could use struct_size() in some cases, but in this particular case we will endup with more local pointer variables that are just declared to keep struct_size() macro happy.

IMO, Which will not make code any better than what it is now.

I will revist these and see if it makes sense to use it in other places.

+/* container config */
+struct apm_container_obj  {
+    struct apm_container_cfg container_cfg;
+    /* Capability ID list */
+    struct apm_prop_data cap_data;
+    uint32_t num_capability_id;
+    uint32_t capability_id;
+
+    /* Container graph Position */
+    struct apm_prop_data pos_data;
+    struct apm_cont_prop_id_graph_pos pos;
+
+    /* Container Stack size */
+    struct apm_prop_data stack_data;
+    struct apm_cont_prop_id_stack_size stack;
+
+    /* Container proc domain id */
+    struct apm_prop_data domain_data;
+    struct apm_cont_prop_id_domain domain;
+} __packed;
+
+struct apm_container_params  {
+    struct apm_module_param_data param_data;
+    uint32_t num_containers;
+    struct apm_container_obj cont_obj[];
+} __packed;
+
+#define APM_CONTAINER_PSIZE(n) ALIGN(sizeof(struct apm_container_params) + \
+                n * sizeof(struct apm_container_obj), 8)
+

Same here, use struct_size, and same applies for other defines in this file, I won't add comment to all of them ;)

+/* Module List config */
+struct apm_mod_list_obj {
+    /* Modules list cfg */
+    uint32_t sub_graph_id;
+    uint32_t container_id;
+    uint32_t num_modules;
+    struct apm_module_obj mod_cfg[];
+} __packed;
+
+struct apm_module_list_params {
+    struct apm_module_param_data param_data;
+    uint32_t num_modules_list;
+    /* Module list config array */
+    struct apm_mod_list_obj mod_list_obj[];
+} __packed;
+

(...)

diff --git a/sound/soc/qcom/qdsp6/audioreach.h b/sound/soc/qcom/qdsp6/audioreach.h
new file mode 100644
index 000000000000..556443155416
--- /dev/null
+++ b/sound/soc/qcom/qdsp6/audioreach.h
@@ -0,0 +1,668 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef __AUDIOREACH_H__
+#define __AUDIOREACH_H__
+#include <linux/types.h>
+#include <linux/soc/qcom/apr.h>
+#include <sound/soc.h>
+
+/* Module IDs */
+#define MODULE_ID_WR_SHARED_MEM_EP    0x07001000
+#define MODULE_ID_RD_SHARED_MEM_EP    0x07001001
+#define MODULE_ID_GAIN            0x07001002
+#define MODULE_ID_PCM_CNV        0x07001003
+#define MODULE_ID_PCM_ENC        0x07001004
+#define MODULE_ID_PCM_DEC        0x07001005
+#define MODULE_ID_CODEC_DMA_SINK    0x07001023
+#define MODULE_ID_CODEC_DMA_SOURCE    0x07001024
+#define MODULE_ID_I2S_SINK        0x0700100A
+#define MODULE_ID_I2S_SOURCE        0x0700100b

Small 'b' in hex value, use 'B' for consistency

I agree, will try find such instances and fix them before I send out next version.


+#define MODULE_ID_DATA_LOGGING        0x0700101A
+
+#define APM_CMD_GET_SPF_STATE        0x01001021
+#define APM_CMD_RSP_GET_SPF_STATE    0x02001007
+
+#define APM_MODULE_INSTANCE_ID        0x00000001
+#define PRM_MODULE_INSTANCE_ID        0x00000002
+#define AMDB_MODULE_INSTANCE_ID        0x00000003
+#define VCPM_MODULE_INSTANCE_ID        0x00000004
+#define AR_MODULE_INSTANCE_ID_START    0x00006000
+#define AR_MODULE_INSTANCE_ID_END    0x00007000
+#define AR_MODULE_DYNAMIC_INSTANCE_ID_START    0x00007000
+#define AR_MODULE_DYNAMIC_INSTANCE_ID_END    0x00008000
+#define AR_CONT_INSTANCE_ID_START    0x00005000
+#define AR_CONT_INSTANCE_ID_END        0x00006000
+#define AR_SG_INSTANCE_ID_START        0x00004000
+
+#define APM_CMD_GRAPH_OPEN            0x01001000
+#define APM_CMD_GRAPH_PREPARE            0x01001001
+#define APM_CMD_GRAPH_START            0x01001002
+#define APM_CMD_GRAPH_STOP            0x01001003
+#define APM_CMD_GRAPH_CLOSE            0x01001004
+#define APM_CMD_GRAPH_FLUSH            0x01001005
+#define APM_CMD_SET_CFG                0x01001006
+#define APM_CMD_GET_CFG                0x01001007
+#define APM_CMD_SHARED_MEM_MAP_REGIONS        0x0100100c

Small 'c' here, use 'C'

+#define APM_CMD_SHARED_MEM_UNMAP_REGIONS    0x0100100d

and 'D' insted of 'd' here.

+#define APM_CMD_RSP_SHARED_MEM_MAP_REGIONS    0x02001001
+#define APM_CMD_RSP_GET_CFG            0x02001000
+#define APM_CMD_CLOSE_ALL            0x01001013
+#define APM_CMD_REGISTER_SHARED_CFG        0x0100100A
+
+#define APM_MEMORY_MAP_SHMEM8_4K_POOL        3
+
+struct apm_cmd_shared_mem_map_regions {
+    uint16_t mem_pool_id;
+    uint16_t num_regions;
+    uint32_t property_flag;
+} __packed;
+
+struct apm_shared_map_region_payload {
+    uint32_t shm_addr_lsw;
+    uint32_t shm_addr_msw;
+    uint32_t mem_size_bytes;
+} __packed;
+
+struct apm_cmd_shared_mem_unmap_regions {
+    uint32_t mem_map_handle;
+} __packed;
+
+struct apm_cmd_rsp_shared_mem_map_regions {
+    uint32_t mem_map_handle;
+} __packed;
+

(...)


+#define DATA_CMD_RD_SH_MEM_EP_DATA_BUFFER        0x04001003
+
+struct data_cmd_rd_sh_mem_ep_data_buffer {
+    uint32_t buf_addr_lsw;
+    uint32_t buf_addr_msw;
+    uint32_t mem_map_handle;
+    uint32_t buf_size;
+};
+
+#define DATA_CMD_RSP_RD_SH_MEM_EP_DATA_BUFFER        0x05001002
+
+struct data_cmd_rsp_rd_sh_mem_ep_data_buffer_done {
+    uint32_t status;
+    uint32_t buf_addr_lsw;
+    uint32_t buf_addr_msw;
+    uint32_t mem_map_handle;
+    uint32_t data_size;
+    uint32_t offset;
+    uint32_t timestamp_lsw;
+    uint32_t timestamp_msw;
+    uint32_t flags;
+    uint32_t num_frames;
+};

Rest of structs is marked with __packed and it seems like it describes communication protocol, so it seems like you missed it here.

These are mostly u32 it did work without packed, but I agree with you on missing __packed. Will fix such instances.

--srini

+
+#define DATA_CMD_RD_SH_MEM_EP_DATA_BUFFER_V2        0x0400100B
+
+struct data_cmd_rd_sh_mem_ep_data_buffer_v2 {
+    uint32_t buf_addr_lsw;
+    uint32_t buf_addr_msw;
+    uint32_t mem_map_handle;
+    uint32_t buf_size;
+    uint32_t md_buf_addr_lsw;
+    uint32_t md_buf_addr_msw;
+    uint32_t md_mem_map_handle;
+    uint32_t md_buf_size;
+};

Same comment as above.

+
+#define DATA_CMD_RSP_RD_SH_MEM_EP_DATA_BUFFER_V2    0x05001005
+
+struct data_cmd_rsp_rd_sh_mem_ep_data_buffer_done_v2 {
+    uint32_t status;
+    uint32_t buf_addr_lsw;
+    uint32_t buf_addr_msw;
+    uint32_t mem_map_handle;
+    uint32_t data_size;
+    uint32_t offset;
+    uint32_t timestamp_lsw;
+    uint32_t timestamp_msw;
+    uint32_t flags;
+    uint32_t num_frames;
+    uint32_t md_status;
+    uint32_t md_buf_addr_lsw;
+    uint32_t md_buf_addr_msw;
+    uint32_t md_mem_map_handle;
+    uint32_t md_size;
+} __packed;
+
+#define PARAM_ID_RD_SH_MEM_CFG                0x08001007
+
+struct param_id_rd_sh_mem_cfg {
+    uint32_t num_frames_per_buffer;
+    uint32_t metadata_control_flags;
+
+} __packed;
+

(...)



[Index of Archives]     [ALSA User]     [Linux Audio Users]     [Pulse Audio]     [Kernel Archive]     [Asterisk PBX]     [Photo Sharing]     [Linux Sound]     [Video 4 Linux]     [Gimp]     [Yosemite News]

  Powered by Linux