Thanks for the comments. On 03/01/18 00:03, Bjorn Andersson wrote:
On Thu 14 Dec 09:33 PST 2017, srinivas.kandagatla@xxxxxxxxxx wrote: [..]+ +enum stream_state { + IDLE = 0, + STOPPED, + RUNNING,These are too generic.
Yep, I will prefix them with Q6ASM.
+}; + +struct q6asm_dai_rtd { + struct snd_pcm_substream *substream; + dma_addr_t phys; + unsigned int pcm_size; + unsigned int pcm_count; + unsigned int pcm_irq_pos; /* IRQ position */ + unsigned int periods; + uint16_t bits_per_sample; + uint16_t source; /* Encoding source bit mask */ + + struct audio_client *audio_client; + uint16_t session_id; + + enum stream_state state; + bool set_channel_map; + char channel_map[8];There's a define for this 8
Yes, this is max channels.
+}; + +struct q6asm_dai_data { + u64 sid; +}; + +static struct snd_pcm_hardware q6asm_dai_hardware_playback = { + .info = (SNDRV_PCM_INFO_MMAP | + SNDRV_PCM_INFO_BLOCK_TRANSFER | + SNDRV_PCM_INFO_MMAP_VALID | + SNDRV_PCM_INFO_INTERLEAVED | + SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME), + .formats = (SNDRV_PCM_FMTBIT_S16_LE | + SNDRV_PCM_FMTBIT_S24_LE), + .rates = SNDRV_PCM_RATE_8000_192000, + .rate_min = 8000, + .rate_max = 192000, + .channels_min = 1, + .channels_max = 8, + .buffer_bytes_max = (PLAYBACK_MAX_NUM_PERIODS * + PLAYBACK_MAX_PERIOD_SIZE), + .period_bytes_min = PLAYBACK_MIN_PERIOD_SIZE, + .period_bytes_max = PLAYBACK_MAX_PERIOD_SIZE, + .periods_min = PLAYBACK_MIN_NUM_PERIODS, + .periods_max = PLAYBACK_MAX_NUM_PERIODS,If you just put the numbers here, instead of using the PLAYBACK_ defines, it's possible to grok the values of this struct without having to jump to the defines for each one.
This is usually done this way in may other drivers!,
+ .fifo_size = 0, +}; + +/* Conventional and unconventional sample rate supported */ +static unsigned int supported_sample_rates[] = { + 8000, 11025, 12000, 16000, 22050, 24000, 32000, 44100, 48000, + 88200, 96000, 176400, 192000 +}; + +static struct snd_pcm_hw_constraint_list constraints_sample_rates = {
It is used in q6asm_dai_open().
+ .count = ARRAY_SIZE(supported_sample_rates), + .list = supported_sample_rates, + .mask = 0, +}; ++ +static int q6asm_dai_prepare(struct snd_pcm_substream *substream) +{ + struct snd_pcm_runtime *runtime = substream->runtime; + struct snd_soc_pcm_runtime *soc_prtd = substream->private_data; + struct q6asm_dai_rtd *prtd = runtime->private_data; + struct q6asm_dai_data *pdata; + int ret; + + pdata = dev_get_drvdata(soc_prtd->platform->dev); + if (!pdata) + return -EINVAL; + + if (!prtd || !prtd->audio_client) { + pr_err("%s: private data null or audio client freed\n", + __func__); + return -EINVAL; + } + + prtd->pcm_count = snd_pcm_lib_period_bytes(substream); + prtd->pcm_irq_pos = 0; + /* rate and channels are sent to audio driver */ + if (prtd->state) { + /* clear the previous setup if any */ + q6asm_cmd(prtd->audio_client, CMD_CLOSE); + q6asm_unmap_memory_regions(substream->stream, + prtd->audio_client); + q6routing_dereg_phy_stream(soc_prtd->dai_link->id, + SNDRV_PCM_STREAM_PLAYBACK); + } + + ret = q6asm_map_memory_regions(substream->stream, prtd->audio_client, + prtd->phys, + (prtd->pcm_size / prtd->periods), + prtd->periods); + + if (ret < 0) { + pr_err("Audio Start: Buffer Allocation failed rc = %d\n", + ret); + return -ENOMEM; + } + + ret = q6asm_open_write(prtd->audio_client, FORMAT_LINEAR_PCM, + prtd->bits_per_sample); + if (ret < 0) { + pr_err("%s: q6asm_open_write failed\n", __func__); + q6asm_audio_client_free(prtd->audio_client); + prtd->audio_client = NULL;Do you need to roll back the q6asm_map_memory_regions?
yes you are correct, we should roll back the map.
+ return -ENOMEM; + } + + prtd->session_id = q6asm_get_session_id(prtd->audio_client); + ret = q6routing_reg_phy_stream(soc_prtd->dai_link->id, LEGACY_PCM_MODE, + prtd->session_id, substream->stream); + if (ret) { + pr_err("%s: stream reg failed ret:%d\n", __func__, ret); + return ret; + } + + ret = q6asm_media_format_block_multi_ch_pcm( + prtd->audio_client, runtime->rate, + runtime->channels, !prtd->set_channel_map, + prtd->channel_map, prtd->bits_per_sample);set_channel_map and channel_map aren't referenced elsewhere. If this isn't used consider removing it for now.
Will take a closer look before sending next version.
Any suggestions are welcome, I did not find a better way to append sid to iova address from iommu.+ if (ret < 0) + pr_info("%s: CMD Format block failed\n", __func__); + + prtd->state = RUNNING; + + return 0; +} +[..]+static int q6asm_dai_pcm_new(struct snd_soc_pcm_runtime *rtd) +{ + struct snd_pcm *pcm = rtd->pcm; + struct snd_pcm_substream *substream; + struct snd_card *card = rtd->card->snd_card; + struct device *dev = card->dev; + struct device_node *node = dev->of_node; + struct q6asm_dai_data *pdata = dev_get_drvdata(rtd->platform->dev); + struct of_phandle_args args; + + int size, ret = 0; + + ret = of_parse_phandle_with_fixed_args(node, "iommus", 1, 0, &args); + if (ret < 0) + pdata->sid = -1; + else + pdata->sid = args.args[0]; +Is this really how you're supposed to deal with the iommu?
Currently downstream abstracts this in ion apis.
+ + + substream = pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].substream; + size = q6asm_dai_hardware_playback.buffer_bytes_max; + ret = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, dev, size, + &substream->dma_buffer); + if (ret) { + dev_err(dev, "Cannot allocate buffer(s)\n"); + return ret;Just fall through.
yep
+ } + + return ret; +} +[..]+static struct snd_soc_dai_driver q6asm_fe_dais[] = { + { + .playback = { + .stream_name = "MultiMedia1 Playback", + .rates = (SNDRV_PCM_RATE_8000_192000| + SNDRV_PCM_RATE_KNOT), + .formats = (SNDRV_PCM_FMTBIT_S16_LE | + SNDRV_PCM_FMTBIT_S24_LE), + .channels_min = 1, + .channels_max = 8, + .rate_min = 8000, + .rate_max = 192000, + }, + .name = "MM_DL1", + .probe = fe_dai_probe, + .id = MSM_FRONTEND_DAI_MULTIMEDIA1, + }, + { + .playback = { + .stream_name = "MultiMedia2 Playback", + .rates = (SNDRV_PCM_RATE_8000_192000| + SNDRV_PCM_RATE_KNOT), + .formats = (SNDRV_PCM_FMTBIT_S16_LE | + SNDRV_PCM_FMTBIT_S24_LE), + .channels_min = 1, + .channels_max = 8, + .rate_min = 8000, + .rate_max = 192000,I presume the listed frontend DAIs needs to match the firmware of the DSP (and features of hardware)? Can we get away with a single list for all versions of the adsp?
Yes, DSP supports 8 concurrent streams both playback and record streams.For now I have only added two entires to keep the patch simple but this should be ideally 8 entries.
In msm-4.4 the max rate for these where changed to 384000, see:
9c46f74b2724 ("ASoC: msm: add 384KHz playback support")
sure i will include that in next version.
+ }, + .name = "MM_DL2", + .probe = fe_dai_probe, + .id = MSM_FRONTEND_DAI_MULTIMEDIA2, + }, +}; +Regards, Bjorn
-- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
