Skip to content

Fix free topology #340

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ranj063 wants to merge 3 commits into from
Closed

Fix free topology #340

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b22277a
ASoC: SOF: free pcm dma pages in dai_unload
ranj063 Nov 30, 2018
3aeb3f6
ASoC: SOF: remove snd_soc_tplg_component_remove() call in free_topolo…
ranj063 Nov 30, 2018
53506ad
ASoC: SOF: remove unused variable
ranj063 Nov 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 4 additions & 26 deletions sound/soc/sof/pcm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -566,31 +566,6 @@ static int sof_pcm_new(struct snd_soc_pcm_runtime *rtd)
return ret;
}

static void sof_pcm_free(struct snd_pcm *pcm)
{
struct snd_sof_pcm *spcm;
struct snd_soc_pcm_runtime *rtd = pcm->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);

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;
}

if (spcm->pcm.playback)
snd_dma_free_pages(&spcm->stream[SNDRV_PCM_STREAM_PLAYBACK].page_table);

if (spcm->pcm.capture)
snd_dma_free_pages(&spcm->stream[SNDRV_PCM_STREAM_CAPTURE].page_table);

snd_sof_free_topology(sdev);
}

Copy link
Collaborator

@bardliao bardliao Nov 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we keep sof_pcm_free? Please see cbdc405. We still need to call snd_pcm_lib_preallocate_free_for_all in sof_pcm_free

Copy link
Member

@plbossart plbossart Nov 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's ugly to allocate dma pages in pcm_new and release them in dai_unload.

Copy link
Member

@plbossart plbossart Nov 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or let's allocate everything in dai_load, but not do a half-baked job in two places.

Copy link
Collaborator Author

@ranj063 ranj063 Nov 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we keep sof_pcm_free? Please see cbdc405. We still need to call snd_pcm_lib_preallocate_free_for_all in sof_pcm_free

@bardliao doesnt snd_pcm_free() already call this? Do we need to call it again in sof_pcm_free?

Copy link
Collaborator

@bardliao bardliao Dec 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ranj063 You are right. We don't need to call it in our driver. @plbossart I believe that we don't need snd_pcm_lib_preallocate_free_for_all() see https://patchwork.kernel.org/patch/6854731/
I vote for allocating everything in dai_load.

/* 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)
Expand Down Expand Up @@ -716,7 +691,11 @@ static int sof_pcm_probe(struct snd_soc_component *component)

static void sof_pcm_remove(struct snd_soc_component *component)
{
struct snd_sof_dev *sdev =
snd_soc_component_get_drvdata(component);

pm_runtime_disable(component->dev);
snd_sof_free_topology(sdev);
}

void snd_sof_new_platform_drv(struct snd_sof_dev *sdev)
Expand All @@ -736,7 +715,6 @@ void snd_sof_new_platform_drv(struct snd_sof_dev *sdev)
pd->ops = &sof_pcm_ops;
pd->compr_ops = &sof_compressed_ops;
pd->pcm_new = sof_pcm_new;
pd->pcm_free = sof_pcm_free;
pd->ignore_machine = drv_name;
pd->be_hw_params_fixup = sof_pcm_dai_link_fixup;
pd->be_pcm_base = SOF_BE_PCM_BASE;
Expand Down
14 changes: 6 additions & 8 deletions sound/soc/sof/topology.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1824,6 +1824,12 @@ static int sof_dai_unload(struct snd_soc_component *scomp,
{
struct snd_sof_pcm *spcm = dobj->private;

if (spcm->pcm.playback)
snd_dma_free_pages(&spcm->stream[SNDRV_PCM_STREAM_PLAYBACK].page_table);

if (spcm->pcm.capture)
snd_dma_free_pages(&spcm->stream[SNDRV_PCM_STREAM_CAPTURE].page_table);

list_del(&spcm->list);
kfree(spcm);

Expand Down Expand Up @@ -2629,8 +2635,6 @@ EXPORT_SYMBOL(snd_sof_load_topology);

void snd_sof_free_topology(struct snd_sof_dev *sdev)
{
struct snd_soc_dapm_context *dapm =
snd_soc_component_get_dapm(sdev->component);
struct snd_sof_route *sroute, *temp;
int ret;

Expand All @@ -2645,11 +2649,5 @@ void snd_sof_free_topology(struct snd_sof_dev *sdev)
kfree(sroute->private);
Copy link
Collaborator

@bardliao bardliao Nov 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am also thinking should we add list_del(&sroute->list); here. We don't need to keep the list after it is freed

kfree(sroute);
}

ret = snd_soc_tplg_component_remove(sdev->component,
SND_SOC_TPLG_INDEX_ALL);
if (ret < 0)
dev_err(sdev->dev,
"error: tplg component free failed %d\n", ret);
Copy link
Member

@plbossart plbossart Nov 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have mixed feelings about this.
Is the framework design intended to release all resources (similar to devm_ for memory allocation) or does it call tplg_component_remove for extra safety?

}
EXPORT_SYMBOL(snd_sof_free_topology);