Thanks Pierre, On 15/09/2021 17:11, Pierre-Louis Bossart wrote:
+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; + int cnt; + int rc; + + if (dir == SNDRV_PCM_STREAM_PLAYBACK) + data = &graph->rx_data; + else + data = &graph->tx_data; + + mutex_lock(&graph->lock); + + if (data->buf) { + dev_err(graph->dev, "Buffer already allocated\n"); + mutex_unlock(&graph->lock); + return 0;is this an error worth of dev_err() if you return 0?
No, its removed now.
+ } + + buf = kzalloc(((sizeof(struct audio_buffer)) * periods), GFP_KERNEL); + if (!buf) { + mutex_unlock(&graph->lock); + 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; + + mutex_unlock(&graph->lock); + + 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 apm_data_cmd_wr_sh_mem_ep_data_buffer_v2 *write; + int rc, payload_size, iid; + struct audio_buffer *ab; + struct gpr_pkt *pkt; + + payload_size = sizeof(*write);nit-pick on variable-naming: I get confused between actions and objects.
this is payload_size for the command packet.
+ + iid = q6apm_graph_get_rx_shmem_module_iid(graph); + pkt = 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(pkt)) + return -ENOMEM; + + write = (void *)pkt + GPR_HDR_SIZE; + + mutex_lock(&graph->lock); + 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; + write->flags = wflags; + + graph->rx_data.dsp_buf++; + + if (graph->rx_data.dsp_buf >= graph->rx_data.num_periods) + graph->rx_data.dsp_buf = 0; + + mutex_unlock(&graph->lock); + + rc = gpr_send_port_pkt(graph->port, pkt); + + kfree(pkt); + + return rc; +} +EXPORT_SYMBOL_GPL(q6apm_write_async); + +int q6apm_read(struct q6apm_graph *graph) +{ + struct data_cmd_rd_sh_mem_ep_data_buffer_v2 *read; + struct audioreach_graph_data *port; + struct audio_buffer *ab; + struct gpr_pkt *pkt; + int rc, iid; + + iid = q6apm_graph_get_tx_shmem_module_iid(graph); + pkt = audioreach_alloc_pkt(sizeof(*read), DATA_CMD_RD_SH_MEM_EP_DATA_BUFFER_V2, + graph->tx_data.dsp_buf, graph->port->id, iid); + if (IS_ERR(pkt)) + return -ENOMEM; + + read = (void *)pkt + GPR_HDR_SIZE;same nit-pick on variable naming, with the additional present/past grammar issue that you don't know if it's a read buffer or a pointer to data read in the past.
do you think adding "_cmd" suffix like read_cmd would make more sense? --srini
+ + mutex_lock(&graph->lock); + port = &graph->tx_data; + ab = &port->buf[port->dsp_buf]; + + read->buf_addr_lsw = lower_32_bits(ab->phys); + read->buf_addr_msw = upper_32_bits(ab->phys); + read->mem_map_handle = port->mem_map_handle; + read->buf_size = ab->size; + + port->dsp_buf++; + + if (port->dsp_buf >= port->num_periods) + port->dsp_buf = 0; + + mutex_unlock(&graph->lock); + + rc = gpr_send_port_pkt(graph->port, pkt); + kfree(pkt); + + return rc; +} +EXPORT_SYMBOL_GPL(q6apm_read); + static int graph_callback(struct gpr_resp_pkt *data, void *priv, int op) { struct data_cmd_rsp_rd_sh_mem_ep_data_buffer_done_v2 *rd_done;
