Re: [alsa-devel] [PATCH v4 05/14] ASoC: SOF: Add PCM operations support

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