thanks Pierre for taking time to review the patches.
On 03/09/2021 16:13, Pierre-Louis Bossart wrote:
On 9/3/21 6:20 AM, Srinivas Kandagatla wrote:
Audioreach module configuration helpers, which will be used by the q6apm-dai driver.
Also add Kconfig and Makefile changes as now all the code for audioreach and q6apm are in at this point.
separate patches?
Sure I will do that in next spin.
+int audioreach_graph_send_cmd_sync(struct q6apm_graph *graph,
struct gpr_pkt *pkt, uint32_t rsp_opcode)+{
- struct device *dev = graph->dev;
- struct gpr_hdr *hdr = &pkt->hdr;
- int rc;
- mutex_lock(&graph->cmd_lock);
- graph->result.opcode = 0;
- graph->result.status = 0;
- rc = gpr_send_port_pkt(graph->port, pkt);
- if (rc < 0)
goto err;- if (rsp_opcode)
rc = wait_event_timeout(graph->cmd_wait,(graph->result.opcode == hdr->opcode) ||(graph->result.opcode == rsp_opcode),5 * HZ);- else
rc = wait_event_timeout(graph->cmd_wait,(graph->result.opcode == hdr->opcode),5 * HZ);- if (!rc) {
dev_err(dev, "CMD timeout for [%x] opcode\n", hdr->opcode);rc = -ETIMEDOUT;- } else if (graph->result.status > 0) {
dev_err(dev, "DSP returned error[%x] %x\n", hdr->opcode,graph->result.status);rc = -EINVAL;- } else {
dev_err(dev, "DSP returned [%x]\n", graph->result.status);rc = 0;- }
+err:
- mutex_unlock(&graph->cmd_lock);
- return rc;
+} +EXPORT_SYMBOL_GPL(audioreach_graph_send_cmd_sync);
I'm pretty sure I've seen this code before...
This looks almost identical to
int q6apm_send_cmd_sync(struct q6apm *apm, struct gpr_pkt *pkt, uint32_t rsp_opcode)
Can this be refactored?
I did this in next version.
If not, at the very least make sure the errors messages are unique...
+static int audioreach_codec_dma_set_media_format(struct q6apm_graph *graph,
struct audioreach_module *module,struct audioreach_module_config *cfg)maybe explain what a 'codec dma' is?
Qualcomm LPASS (Low Power Audio Subsystem) has digital part of codec integrated into it, and connected to Analog part of codec using SoundWire.
+{
- struct apm_module_param_data *param_data;
- struct apm_codec_dma_module_intf_cfg *intf_cfg;
- struct apm_module_hw_ep_mf_cfg *hw_cfg;
- struct apm_module_frame_size_factor_cfg *fs_cfg;
- struct apm_module_hw_ep_power_mode_cfg *pm_cfg;
- int ic_sz, ep_sz, fs_sz, pm_sz, dl_sz;
- int rc, payload_size;
- struct gpr_pkt *pkt;
- void *p;
...
+static int audioreach_i2s_set_media_format(struct q6apm_graph *graph,
struct audioreach_module *module,struct audioreach_module_config *cfg)+{
- struct apm_module_frame_size_factor_cfg *fs_cfg;
- struct apm_module_param_data *param_data;
- struct apm_i2s_module_intf_cfg *intf_cfg;
- struct apm_module_hw_ep_mf_cfg *hw_cfg;
- int ic_sz, ep_sz, fs_sz;
- int rc, payload_size;
- struct gpr_pkt *pkt;
- void *p;
- ic_sz = APM_I2S_INTF_CFG_PSIZE;
- ep_sz = APM_HW_EP_CFG_PSIZE;
- fs_sz = APM_FS_CFG_PSIZE;
- payload_size = ic_sz + ep_sz + fs_sz;
- p = audioreach_alloc_apm_cmd_pkt(payload_size, APM_CMD_SET_CFG, 0);
- if (IS_ERR(p))
return -ENOMEM;- pkt = p;
- p = p + GPR_HDR_SIZE + APM_CMD_HDR_SIZE;
- intf_cfg = p;
- param_data = &intf_cfg->param_data;
- param_data->module_instance_id = module->instance_id;
- param_data->error_code = 0;
- param_data->param_id = PARAM_ID_I2S_INTF_CFG;
- param_data->param_size = ic_sz - APM_MODULE_PARAM_DATA_SIZE;
- intf_cfg->cfg.intf_idx = module->hw_interface_idx;
- intf_cfg->cfg.sd_line_idx = module->sd_line_idx;
- switch (cfg->fmt & SND_SOC_DAIFMT_MASTER_MASK) {
- case SND_SOC_DAIFMT_CBS_CFS:
CBC_CFC
intf_cfg->cfg.ws_src = CONFIG_I2S_WS_SRC_INTERNAL;break;- case SND_SOC_DAIFMT_CBM_CFM:
CBP_CFP
Sure I was not aware of these new formats, will use them.
/* CPU is slave */intf_cfg->cfg.ws_src = CONFIG_I2S_WS_SRC_EXTERNAL;break;- default:
break;- }
+int q6apm_map_memory_regions(struct q6apm_graph *graph,
unsigned int dir, phys_addr_t phys,size_t period_sz, unsigned int periods)+{
- struct audioreach_graph_data *data;
- struct audio_buffer *buf;
- unsigned long flags;
- int cnt;
- int rc;
- if (dir == SNDRV_PCM_STREAM_PLAYBACK)
data = &graph->rx_data;- else
data = &graph->tx_data;- spin_lock_irqsave(&graph->lock, flags);
- if (data->buf) {
dev_err(graph->dev, "Buffer already allocated\n");spin_unlock_irqrestore(&graph->lock, flags);return 0;- }
- buf = kzalloc(((sizeof(struct audio_buffer)) * periods), GFP_ATOMIC);
Why GFP_ATOMIC?
Its under spinlock.
- if (!buf) {
spin_unlock_irqrestore(&graph->lock, flags);return -ENOMEM;- }
- if (dir == SNDRV_PCM_STREAM_PLAYBACK)
data = &graph->rx_data;- else
data = &graph->tx_data;- data->buf = buf;
- buf[0].phys = phys;
- buf[0].size = period_sz;
- for (cnt = 1; cnt < periods; cnt++) {
if (period_sz > 0) {buf[cnt].phys = buf[0].phys + (cnt * period_sz);buf[cnt].size = period_sz;}- }
- data->num_periods = periods;
- spin_unlock_irqrestore(&graph->lock, flags);
- rc = audioreach_map_memory_regions(graph, dir, period_sz,
periods, 1);- if (rc < 0) {
dev_err(graph->dev, "Memory_map_regions failed\n");audioreach_graph_free_buf(graph);- }
- return rc;
+} +EXPORT_SYMBOL_GPL(q6apm_map_memory_regions);
+int q6apm_write_async(struct q6apm_graph *graph, uint32_t len, uint32_t msw_ts,
uint32_t lsw_ts, uint32_t wflags)+{
- struct gpr_pkt *pkt;
- void *p;
- int rc, payload_size, iid;
- struct apm_data_cmd_wr_sh_mem_ep_data_buffer_v2 *write;
- struct audio_buffer *ab;
- unsigned long flags;
- payload_size = sizeof(*write);
- iid = q6apm_graph_get_rx_shmem_module_iid(graph);
- p = audioreach_alloc_pkt(payload_size,
DATA_CMD_WR_SH_MEM_EP_DATA_BUFFER_V2,graph->rx_data.dsp_buf | (len << APM_WRITE_TOKEN_LEN_SHIFT),graph->port->id, iid);- if (IS_ERR(p))
return -ENOMEM;- pkt = p;
- p = p + GPR_HDR_SIZE;
- write = p;
- spin_lock_irqsave(&graph->lock, flags);
- ab = &graph->rx_data.buf[graph->rx_data.dsp_buf];
- write->buf_addr_lsw = lower_32_bits(ab->phys);
- write->buf_addr_msw = upper_32_bits(ab->phys);
- write->buf_size = len;
- write->timestamp_lsw = lsw_ts;
- write->timestamp_msw = msw_ts;
- write->mem_map_handle = graph->rx_data.mem_map_handle;
- if (wflags == NO_TIMESTAMP)
write->flags = 0;- else
write->flags = 0x80000000;BIT(31) ?
This should be #define WR_SH_MEM_EP_BIT_MASK_TIMESTAMP_VALID_FLAG 0x80000000
it's also odd to define NO_TIMESTAMP, it's usually when you want a timestamp that you ask for it...
Thats right, I can try to cleanup some logic around this, It might be that I have inherited some of this logic from old dsp drivers :-)
- graph->rx_data.dsp_buf++;
- if (graph->rx_data.dsp_buf >= graph->rx_data.num_periods)
graph->rx_data.dsp_buf = 0;- spin_unlock_irqrestore(&graph->lock, flags);
- rc = gpr_send_port_pkt(graph->port, pkt);
- kfree(pkt);
- return rc;
+} +EXPORT_SYMBOL_GPL(q6apm_write_async);