Message-ID: <de23cc38-78c1-e45c-9367-847854f31c60@linaro.org>
Date:   Wed, 3 Jan 2018 16:27:16 +0000
From:   Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
To:     Bjorn Andersson <bjorn.andersson@...aro.org>
Cc:     Andy Gross <andy.gross@...aro.org>,
        Mark Brown <broonie@...nel.org>, linux-arm-msm@...r.kernel.org,
        alsa-devel@...a-project.org, David Brown <david.brown@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Liam Girdwood <lgirdwood@...il.com>,
        Patrick Lai <plai@...eaurora.org>,
        Banajit Goswami <bgoswami@...eaurora.org>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>, linux-soc@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, sboyd@...eaurora.org
Subject: Re: [RESEND PATCH v2 12/15] ASoC: qcom: qdsp6: Add support to q6asm
 dai driver

Thanks for the comments.

On 03/01/18 00:03, Bjorn Andersson wrote:
> On Thu 14 Dec 09:33 PST 2017, srinivas.kandagatla@...aro.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
> 

Powered by blists - more mailing lists