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@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware