From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: [PATCH v4 05/14] ASoC: SOF: Add PCM operations support Date: Thu, 14 Feb 2019 12:20:26 +0100 Message-ID: References: <20190213220734.10471-1-pierre-louis.bossart@linux.intel.com> <20190213220734.10471-6-pierre-louis.bossart@linux.intel.com> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20190213220734.10471-6-pierre-louis.bossart@linux.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: Pierre-Louis Bossart Cc: alsa-devel@alsa-project.org, andriy.shevchenko@intel.com, Daniel Baluta , liam.r.girdwood@linux.intel.com, vkoul@kernel.org, broonie@kernel.org, Alan Cox , sound-open-firmware@alsa-project.org List-Id: alsa-devel@alsa-project.org On Wed, 13 Feb 2019 23:07:25 +0100, Pierre-Louis Bossart wrote: > +static int sof_pcm_open(struct snd_pcm_substream *substream) > +{ .... > + snd_sof_pcm_platform_open(sdev, substream); No error check? > + > + mutex_unlock(&spcm->mutex); > + return 0; > +} > + > +static int sof_pcm_close(struct snd_pcm_substream *substream) > +{ > + struct snd_soc_pcm_runtime *rtd = substream->private_data; > + struct snd_soc_component *component = > + snd_soc_rtdcom_lookup(rtd, DRV_NAME); > + struct snd_sof_dev *sdev = > + snd_soc_component_get_drvdata(component); > + struct snd_sof_pcm *spcm = rtd->private; > + int err; > + > + /* nothing todo for BE */ > + if (rtd->dai_link->no_pcm) > + return 0; > + > + dev_dbg(sdev->dev, "pcm: close stream %d dir %d\n", spcm->pcm.pcm_id, > + substream->stream); > + > + snd_sof_pcm_platform_close(sdev, substream); Doesn't it need spcm->mutex lock while open is called inside mutex? Or, better to reconsider: what does spcm->mutex protect? > + > + mutex_lock(&spcm->mutex); > + pm_runtime_mark_last_busy(sdev->dev); > + > + err = pm_runtime_put_autosuspend(sdev->dev); > + if (err < 0) > + dev_err(sdev->dev, "error: pcm close failed to idle %d\n", > + err); > + > + mutex_unlock(&spcm->mutex); > + return 0; > +} > + > +static struct snd_pcm_ops sof_pcm_ops = { > + .open = sof_pcm_open, > + .close = sof_pcm_close, > + .ioctl = snd_pcm_lib_ioctl, > + .hw_params = sof_pcm_hw_params, > + .hw_free = sof_pcm_hw_free, > + .trigger = sof_pcm_trigger, > + .pointer = sof_pcm_pointer, > + .page = snd_pcm_sgbuf_ops_page, > +}; > + > +/* > + * Pre-allocate playback/capture audio buffer pages. > + * no need to explicitly release memory preallocated by sof_pcm_new in pcm_free > + * snd_pcm_lib_preallocate_free_for_all() is called by the core. > + */ > +static int sof_pcm_new(struct snd_soc_pcm_runtime *rtd) > +{ > + struct snd_soc_component *component = > + snd_soc_rtdcom_lookup(rtd, DRV_NAME); > + struct snd_sof_dev *sdev = > + snd_soc_component_get_drvdata(component); > + struct snd_sof_pcm *spcm; > + struct snd_pcm *pcm = rtd->pcm; > + struct snd_soc_tplg_stream_caps *caps; > + int ret = 0, stream = SNDRV_PCM_STREAM_PLAYBACK; > + > + /* find SOF PCM for this RTD */ > + spcm = snd_sof_find_spcm_dai(sdev, rtd); > + if (!spcm) { > + dev_warn(sdev->dev, "warn: can't find PCM with DAI ID %d\n", > + rtd->dai_link->id); > + return 0; > + } > + rtd->private = spcm; > + > + dev_dbg(sdev->dev, "creating new PCM %s\n", spcm->pcm.pcm_name); > + > + /* do we need to pre-allocate playback audio buffer pages */ > + if (!spcm->pcm.playback) > + goto capture; > + > + caps = &spcm->pcm.caps[stream]; > + > + /* pre-allocate playback audio buffer pages */ > + dev_dbg(sdev->dev, "spcm: allocate %s playback DMA buffer size 0x%x max 0x%x\n", > + caps->name, caps->buffer_size_min, caps->buffer_size_max); > + > + ret = snd_pcm_lib_preallocate_pages(pcm->streams[stream].substream, > + SNDRV_DMA_TYPE_DEV_SG, sdev->dev, > + le32_to_cpu(caps->buffer_size_min), > + le32_to_cpu(caps->buffer_size_max)); > + if (ret) { > + dev_err(sdev->dev, "error: can't alloc DMA buffer size 0x%x/0x%x for %s %d\n", > + caps->buffer_size_min, caps->buffer_size_max, > + caps->name, ret); > + return ret; > + } The error check here is redundant, please drop. snd_pcm_lib_preallocate_pages() was changed to be void function recently, so it'll be a build error. > + > +capture: > + stream = SNDRV_PCM_STREAM_CAPTURE; > + > + /* do we need to pre-allocate capture audio buffer pages */ > + if (!spcm->pcm.capture) > + return ret; > + > + caps = &spcm->pcm.caps[stream]; > + > + /* pre-allocate capture audio buffer pages */ > + dev_dbg(sdev->dev, "spcm: allocate %s capture DMA buffer size 0x%x max 0x%x\n", > + caps->name, caps->buffer_size_min, caps->buffer_size_max); > + > + ret = snd_pcm_lib_preallocate_pages(pcm->streams[stream].substream, > + SNDRV_DMA_TYPE_DEV_SG, sdev->dev, > + le32_to_cpu(caps->buffer_size_min), > + le32_to_cpu(caps->buffer_size_max)); > + if (ret) > + dev_err(sdev->dev, "error: can't alloc DMA buffer size 0x%x/0x%x for %s %d\n", > + caps->buffer_size_min, caps->buffer_size_max, > + caps->name, ret); Ditto. > + > + return ret; > +} > + > +/* fixup the BE DAI link to match any values from topology */ > +static int sof_pcm_dai_link_fixup(struct snd_soc_pcm_runtime *rtd, > + struct snd_pcm_hw_params *params) > +{ > + struct snd_interval *rate = hw_param_interval(params, > + SNDRV_PCM_HW_PARAM_RATE); > + struct snd_interval *channels = hw_param_interval(params, > + SNDRV_PCM_HW_PARAM_CHANNELS); > + struct snd_mask *fmt = hw_param_mask(params, SNDRV_PCM_HW_PARAM_FORMAT); > + struct snd_soc_component *component = > + snd_soc_rtdcom_lookup(rtd, DRV_NAME); > + struct snd_sof_dev *sdev = > + snd_soc_component_get_drvdata(component); > + struct snd_sof_dai *dai = > + snd_sof_find_dai(sdev, (char *)rtd->dai_link->name); > + > + /* no topology exists for this BE, try a common configuration */ > + if (!dai) { > + dev_warn(sdev->dev, "warning: no topology found for BE DAI %s config\n", > + rtd->dai_link->name); > + > + /* set 48k, stereo, 16bits by default */ > + rate->min = 48000; > + rate->max = 48000; > + > + channels->min = 2; > + channels->max = 2; > + > + snd_mask_none(fmt); > + snd_mask_set(fmt, (__force int)SNDRV_PCM_FORMAT_S16_LE); Use snd_mask_set_format() macro. That avoids the ugly cast. > + > + return 0; > + } > + > + /* read format from topology */ > + snd_mask_none(fmt); > + > + switch (dai->comp_dai.config.frame_fmt) { > + case SOF_IPC_FRAME_S16_LE: > + snd_mask_set(fmt, (__force int)SNDRV_PCM_FORMAT_S16_LE); > + break; > + case SOF_IPC_FRAME_S24_4LE: > + snd_mask_set(fmt, (__force int)SNDRV_PCM_FORMAT_S24_LE); > + break; > + case SOF_IPC_FRAME_S32_LE: > + snd_mask_set(fmt, (__force int)SNDRV_PCM_FORMAT_S32_LE); > + break; Ditto for these three, too. thanks, Takashi