On 9/3/21 6:20 AM, Srinivas Kandagatla wrote:
commit message?
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>
> ---
> sound/soc/qcom/Kconfig | 5 +
> sound/soc/qcom/qdsp6/Makefile | 1 +
> sound/soc/qcom/qdsp6/q6apm-dai.c | 504 +++++++++++++++++++++++++++++++
> 3 files changed, 510 insertions(+)
> create mode 100644 sound/soc/qcom/qdsp6/q6apm-dai.c
>
> diff --git a/sound/soc/qcom/Kconfig b/sound/soc/qcom/Kconfig
> index 66d8436ab0a8..fb1921889dc4 100644
> --- a/sound/soc/qcom/Kconfig
> +++ b/sound/soc/qcom/Kconfig
> @@ -84,7 +84,12 @@ config SND_SOC_QDSP6_ASM_DAI
> select SND_SOC_COMPRESS
> tristate
>
> +config SND_SOC_QDSP6_APM_DAI
> + select SND_SOC_COMPRESS
> + tristate
> +
> config SND_SOC_QDSP6_APM
> + select SND_SOC_QDSP6_APM_DAI
> tristate
usually it's tristate then select?
> +static int q6apm_dai_prepare(struct snd_soc_component *component,
> + struct snd_pcm_substream *substream)
> +{
> + struct snd_pcm_runtime *runtime = substream->runtime;
> + struct q6apm_dai_rtd *prtd = runtime->private_data;
> + struct audioreach_module_config cfg;
> + struct q6apm_dai_data *pdata;
> + int ret;
> +
> + pdata = snd_soc_component_get_drvdata(component);
> + if (!pdata)
> + return -EINVAL;
> +
> + if (!prtd || !prtd->graph) {
> + dev_err(component->dev, "%s: private data null or audio client freed\n",
> + __func__);
> + return -EINVAL;
> + }
> +
> + cfg.direction = substream->stream;
> + cfg.sample_rate = runtime->rate;
> + cfg.num_channels = runtime->channels;
> + cfg.bit_width = prtd->bits_per_sample;
> +
> + prtd->pcm_count = snd_pcm_lib_period_bytes(substream);
> + prtd->pcm_irq_pos = 0;
> + /* rate and channels are sent to audio driver */
> + ret = q6apm_graph_media_format_shmem(prtd->graph, &cfg);
> + if (ret < 0) {
> + dev_err(component->dev, "%s: q6apm_open_write failed\n", __func__);
> + return ret;
> + }
> +
> + ret = q6apm_graph_media_format_pcm(prtd->graph, &cfg);
> + if (ret < 0)
> + pr_info("%s: CMD Format block failed\n", __func__);
> +
> + ret = q6apm_map_memory_regions(prtd->graph,
> + substream->stream,
> + prtd->phys,
> + (prtd->pcm_size / prtd->periods),
> + prtd->periods);
> +
> + if (ret < 0) {
> + dev_err(component->dev, "Audio Start: Buffer Allocation failed rc = %d\n",
> + ret);
> + return -ENOMEM;
> + }
> +
> + ret = q6apm_graph_prepare(prtd->graph);
> + if (ret) {
> + dev_err(component->dev, "Failed to prepare Graph %d\n", ret);
> + return ret;
> + }
> +
> + ret = q6apm_graph_start(prtd->graph);
> + if (ret) {
> + dev_err(component->dev, "Failed to Start Graph %d\n", ret);
> + return ret;
> + }
> +
> + if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) {
> + int i;
> + /* Queue the buffers */
> + for (i = 0; i < runtime->periods; i++)
> + q6apm_read(prtd->graph);
> +
> + }
shouldn't the buffers be queued *before* starting? maybe add a comment
on why this is done in this order.
> + prtd->state = Q6APM_STREAM_RUNNING;
> +
> + return 0;
> +}
> +
> +static int q6apm_dai_trigger(struct snd_soc_component *component,
> + struct snd_pcm_substream *substream, int cmd)
> +{
> + struct snd_pcm_runtime *runtime = substream->runtime;
> + struct q6apm_dai_rtd *prtd = runtime->private_data;
> + int ret = 0;
> +
> + switch (cmd) {
> + case SNDRV_PCM_TRIGGER_START:
> + case SNDRV_PCM_TRIGGER_RESUME:
> + case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> + ret = q6apm_write_async(prtd->graph, prtd->pcm_count, 0, 0, NO_TIMESTAMP);
> + break;
surprising, why do this only for playback?
> + case SNDRV_PCM_TRIGGER_STOP:
> + prtd->state = Q6APM_STREAM_STOPPED;
equally surprising, you just store a state but don't take any action?
> + break;
> + case SNDRV_PCM_TRIGGER_SUSPEND:
> + case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
and for those two you do nothing?
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> +
> + return ret;
> +}
> +
> +static int q6apm_dai_open(struct snd_soc_component *component,
> + struct snd_pcm_substream *substream)
> +{
> + struct snd_pcm_runtime *runtime = substream->runtime;
> + struct snd_soc_pcm_runtime *soc_prtd = substream->private_data;
> + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(soc_prtd, 0);
> + struct q6apm_dai_rtd *prtd;
> + struct q6apm_dai_data *pdata;
> + struct device *dev = component->dev;
> + int ret;
> + int graph_id;
> +
> + graph_id = cpu_dai->driver->id;
> +
> + pdata = snd_soc_component_get_drvdata(component);
> + if (!pdata) {
> + dev_err(component->dev, "Drv data not found ..\n");
dev_err(dev, for consistency?
> + return -EINVAL;
> + }
> +
> + prtd = kzalloc(sizeof(*prtd), GFP_KERNEL);
> + if (prtd == NULL)
if (!prtd)
> + return -ENOMEM;
> +
> + prtd->substream = substream;
> +
> + prtd->graph = q6apm_graph_open(dev, (q6apm_cb)event_handler,
> + prtd, graph_id);
> + if (IS_ERR(prtd->graph)) {
> + pr_info("%s: Could not allocate memory\n", __func__);
dev_info(dev,
> + ret = PTR_ERR(prtd->graph);
> + kfree(prtd);
> + return ret;
> + }
> +
> + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> + runtime->hw = q6apm_dai_hardware_playback;
> + else if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
> + runtime->hw = q6apm_dai_hardware_capture;
> +
> + /* Ensure that buffer size is a multiple of period size */
> + ret = snd_pcm_hw_constraint_integer(runtime,
> + SNDRV_PCM_HW_PARAM_PERIODS);
> + if (ret < 0) {
> + dev_err(dev, "snd_pcm_hw_constraint_integer failed\n");
> + return ret;
kfree(prtd)?
The error handling is broken in the rest of this function as well.
please revisit.
> + }
> +
> + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> + ret = snd_pcm_hw_constraint_minmax(runtime,
> + SNDRV_PCM_HW_PARAM_BUFFER_BYTES,
> + BUFFER_BYTES_MIN, BUFFER_BYTES_MAX);
> + if (ret < 0) {
> + dev_err(dev, "constraint for buffer bytes min max ret = %d\n",
> + ret);
> + return ret;
> + }
> + }
> +
> + ret = snd_pcm_hw_constraint_step(runtime, 0,
> + SNDRV_PCM_HW_PARAM_PERIOD_BYTES, 32);
> + if (ret < 0) {
> + dev_err(dev, "constraint for period bytes step ret = %d\n",
> + ret);
> + return ret;
> + }
> + ret = snd_pcm_hw_constraint_step(runtime, 0,
> + SNDRV_PCM_HW_PARAM_BUFFER_BYTES, 32);
> + if (ret < 0) {
> + dev_err(dev, "constraint for buffer bytes step ret = %d\n",
> + ret);
> + return ret;
> + }
> +
> + runtime->private_data = prtd;
> + runtime->dma_bytes = BUFFER_BYTES_MAX;
> + if (pdata->sid < 0)
> + prtd->phys = substream->dma_buffer.addr;
> + else
> + prtd->phys = substream->dma_buffer.addr | (pdata->sid << 32);
> +
> + snd_pcm_set_runtime_buffer(substream, &substream->dma_buffer);
> +
> + return 0;
> +}
> +
> +static int q6apm_dai_close(struct snd_soc_component *component,
> + struct snd_pcm_substream *substream)
> +{
> + struct snd_pcm_runtime *runtime = substream->runtime;
> + struct q6apm_dai_rtd *prtd = runtime->private_data;
> +
> + if (prtd && prtd->graph) {
This is always true if the open succeeds...
> + q6apm_graph_stop(prtd->graph);
> +
> + q6apm_unmap_memory_regions(prtd->graph,
> + substream->stream);
> + q6apm_graph_close(prtd->graph);
> + prtd->graph = NULL;
> + kfree(prtd);
> + runtime->private_data = NULL;
> + }
> +
> + return 0;
> +}
> +
> +static snd_pcm_uframes_t q6apm_dai_pointer(struct snd_soc_component *component,
> + struct snd_pcm_substream *substream)
> +{
> +
> + struct snd_pcm_runtime *runtime = substream->runtime;
> + struct q6apm_dai_rtd *prtd = runtime->private_data;
> +
> + if (prtd->pcm_irq_pos >= prtd->pcm_size)
> + prtd->pcm_irq_pos = 0;
that's surprising, no wrap-around?
> +
> + return bytes_to_frames(runtime, (prtd->pcm_irq_pos));
> +}
> +
> +static int q6apm_dai_mmap(struct snd_soc_component *component,
> + struct snd_pcm_substream *substream,
> + struct vm_area_struct *vma)
> +{
> + struct snd_pcm_runtime *runtime = substream->runtime;
> + struct device *dev = component->dev;
> +
> + return dma_mmap_coherent(dev, vma,
> + runtime->dma_area, runtime->dma_addr,
> + runtime->dma_bytes);
> +}
> +
> +static int q6apm_dai_hw_params(struct snd_soc_component *component,
> + struct snd_pcm_substream *substream,
> + struct snd_pcm_hw_params *params)
> +{
> + struct snd_pcm_runtime *runtime = substream->runtime;
> + struct q6apm_dai_rtd *prtd = runtime->private_data;
> +
> + prtd->pcm_size = params_buffer_bytes(params);
> + prtd->periods = params_periods(params);
> +
> + switch (params_format(params)) {
> + case SNDRV_PCM_FORMAT_S16_LE:
> + prtd->bits_per_sample = 16;
> + break;
> + case SNDRV_PCM_FORMAT_S24_LE:
> + prtd->bits_per_sample = 24;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int q6apm_dai_pcm_new(struct snd_soc_component *component,
> + struct snd_soc_pcm_runtime *rtd)
> +{
> + struct snd_pcm_substream *psubstream, *csubstream;
> + struct snd_pcm *pcm = rtd->pcm;
> + struct device *dev;
> + int size, ret;
> +
> + dev = component->dev;
> + size = BUFFER_BYTES_MAX;
> + psubstream = pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].substream;
> + if (psubstream) {
> + ret = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, dev, size,
> + &psubstream->dma_buffer);
> + if (ret) {
> + dev_err(dev, "Cannot allocate buffer(s)\n");
for playback. Using the same error messages in different cases isn't
very helpful to debug bad sequences...
> + return ret;
> + }
> + }
> +
> + csubstream = pcm->streams[SNDRV_PCM_STREAM_CAPTURE].substream;
> + if (csubstream) {
> + ret = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, dev, size,
> + &csubstream->dma_buffer);
> + if (ret) {
> + dev_err(dev, "Cannot allocate buffer(s)\n");
for capture
> + if (psubstream)
> + snd_dma_free_pages(&psubstream->dma_buffer);
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +MODULE_LICENSE("GPL v2");
"GPL" is enough, the SPDX line deals with the license version.
