[alsa-devel] [Sound-open-firmware] [PATCH v4 05/14] ASoC: SOF: Add PCM operations support
Pierre-Louis Bossart
pierre-louis.bossart at linux.intel.com
Thu Feb 14 16:07:54 CET 2019
On 2/14/19 5:20 AM, Takashi Iwai wrote:
> 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?
Wow, nice catch!
The only explanation I have is that this is implemented with an optional
callback that's not set for baytrail, so in the initial development this
was not needed. It is definitively used for ApolloLake+, will fix this.
>
>
>> +
>> + 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);
Indeed the position of the mutex makes no sense. it should be taken
before the close or it's not needed. Will look into this.
>> + 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
> _______________________________________________
> Sound-open-firmware mailing list
> Sound-open-firmware at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
>
More information about the Alsa-devel
mailing list