Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751778AbeACQ2q (ORCPT + 1 other); Wed, 3 Jan 2018 11:28:46 -0500 Received: from mail-wm0-f66.google.com ([74.125.82.66]:40692 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751596AbeACQ1T (ORCPT ); Wed, 3 Jan 2018 11:27:19 -0500 X-Google-Smtp-Source: ACJfBou895mJ4YmmNlsuuyuePv9lgn1Gh6gn7AG+X6+k7dVO/ZFayBYCmft+cB41Ao+OS9aveAzKcg== Subject: Re: [RESEND PATCH v2 12/15] ASoC: qcom: qdsp6: Add support to q6asm dai driver To: Bjorn Andersson Cc: Andy Gross , Mark Brown , linux-arm-msm@vger.kernel.org, alsa-devel@alsa-project.org, David Brown , Rob Herring , Mark Rutland , Liam Girdwood , Patrick Lai , Banajit Goswami , Jaroslav Kysela , Takashi Iwai , linux-soc@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, sboyd@codeaurora.org References: <20171214173402.19074-1-srinivas.kandagatla@linaro.org> <20171214173402.19074-13-srinivas.kandagatla@linaro.org> <20180103000306.GT478@tuxbook> From: Srinivas Kandagatla Message-ID: Date: Wed, 3 Jan 2018 16:27:16 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <20180103000306.GT478@tuxbook> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: Thanks for the comments. On 03/01/18 00:03, Bjorn Andersson wrote: > On Thu 14 Dec 09:33 PST 2017, srinivas.kandagatla@linaro.org 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. >> + 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? > Any suggestions are welcome, I did not find a better way to append sid to iova address from 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 >