Skip to content

Fix allocation of link dma channels #895

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

Merged
plbossart merged 7 commits into from
May 29, 2019
Merged

Fix allocation of link dma channels #895

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4de1c79
ASoC: SOF: hda: save handle to sdev in sof_intel_hda_stream
ranj063 May 11, 2019
b8e2520
ASoC: SOF: topology: add cpu_dai_name for DAIs
ranj063 May 11, 2019
a90ad77
ASoC: SOF: Intel: hda: add new macro hstream_to_sof_hda_stream()
ranj063 May 28, 2019
4801d3e
ASoC: SOF: assign link DMA channel at run-time
ranj063 May 11, 2019
fb05cd2
ASoC: SOF: hda: reserve host DMA channel for hostless streams
ranj063 May 11, 2019
6a64045
ASoC: SOF: Intel: hda: release link DMA for paused streams during sus…
ranj063 May 20, 2019
c1574ab
ASoC: SOF: hda: couple host and link DMA during FE hw_free
ranj063 May 21, 2019
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
1 change: 1 addition & 0 deletions sound/soc/sof/intel/apl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ const struct snd_sof_dsp_ops sof_apl_ops = {
.pcm_open = hda_dsp_pcm_open,
.pcm_close = hda_dsp_pcm_close,
.pcm_hw_params = hda_dsp_pcm_hw_params,
.pcm_hw_free = hda_dsp_stream_hw_free,
.pcm_trigger = hda_dsp_pcm_trigger,
.pcm_pointer = hda_dsp_pcm_pointer,

Expand Down
1 change: 1 addition & 0 deletions sound/soc/sof/intel/cnl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,7 @@ const struct snd_sof_dsp_ops sof_cnl_ops = {
.pcm_open = hda_dsp_pcm_open,
.pcm_close = hda_dsp_pcm_close,
.pcm_hw_params = hda_dsp_pcm_hw_params,
.pcm_hw_free = hda_dsp_stream_hw_free,
.pcm_trigger = hda_dsp_pcm_trigger,
.pcm_pointer = hda_dsp_pcm_pointer,

Expand Down
293 changes: 192 additions & 101 deletions sound/soc/sof/intel/hda-dai.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,62 +30,90 @@ struct hda_pipe_params {
};

/*
* Unlike GP dma, there is a set of stream registers in hda controller
* to control the link dma channels. Each register controls one link
* dma channel and the relation is fixed. To make sure FW uses correct
* link dma channels, host allocates stream registers and sends the
* corresponding link dma channels to FW to allocate link dma channel
*
* FIXME: this API is abused in the sense that tx_num and rx_num are
* passed as arguments, not returned. We need to find a better way to
* retrieve the stream tag allocated for the link DMA
* This function checks if the host dma channel corresponding
* to the link DMA stream_tag argument is assigned to one
* of the FEs connected to the BE DAI.
*/
static int hda_link_dma_get_channels(struct snd_soc_dai *dai,
unsigned int *tx_num,
unsigned int *tx_slot,
unsigned int *rx_num,
unsigned int *rx_slot)
static bool hda_check_fes(struct snd_soc_pcm_runtime *rtd,
int dir, int stream_tag)
{
struct hdac_bus *bus;
struct hdac_ext_stream *stream;
struct snd_pcm_substream substream;
struct snd_sof_dev *sdev =
snd_soc_component_get_drvdata(dai->component);

bus = sof_to_bus(sdev);

memset(&substream, 0, sizeof(substream));
if (*tx_num == 1) {
substream.stream = SNDRV_PCM_STREAM_PLAYBACK;
stream = snd_hdac_ext_stream_assign(bus, &substream,
HDAC_EXT_STREAM_TYPE_LINK);
if (!stream) {
dev_err(bus->dev, "error: failed to find a free hda ext stream for playback");
return -EBUSY;
}
struct snd_pcm_substream *fe_substream;
struct hdac_stream *fe_hstream;
struct snd_soc_dpcm *dpcm;

for_each_dpcm_fe(rtd, dir, dpcm) {
fe_substream = snd_soc_dpcm_get_substream(dpcm->fe, dir);
fe_hstream = fe_substream->runtime->private_data;
if (fe_hstream->stream_tag == stream_tag)
return true;
}

snd_soc_dai_set_dma_data(dai, &substream, stream);
*tx_slot = hdac_stream(stream)->stream_tag - 1;
return false;
}

static struct hdac_ext_stream *
hda_link_stream_assign(struct hdac_bus *bus,
struct snd_pcm_substream *substream)
{
struct snd_soc_pcm_runtime *rtd = substream->private_data;
struct sof_intel_hda_stream *hda_stream;
struct hdac_ext_stream *res = NULL;
struct hdac_stream *stream = NULL;

dev_dbg(bus->dev, "link dma channel %d for playback", *tx_slot);
int stream_dir = substream->stream;

if (!bus->ppcap) {
dev_err(bus->dev, "stream type not supported\n");
return NULL;
}

if (*rx_num == 1) {
substream.stream = SNDRV_PCM_STREAM_CAPTURE;
stream = snd_hdac_ext_stream_assign(bus, &substream,
HDAC_EXT_STREAM_TYPE_LINK);
if (!stream) {
dev_err(bus->dev, "error: failed to find a free hda ext stream for capture");
return -EBUSY;
list_for_each_entry(stream, &bus->stream_list, list) {
struct hdac_ext_stream *hstream =
stream_to_hdac_ext_stream(stream);
if (stream->direction != substream->stream)
continue;

hda_stream = hstream_to_sof_hda_stream(hstream);

/* check if link is available */
if (!hstream->link_locked) {
if (stream->opened) {
/*
* check if the stream tag matches the stream
* tag of one of the connected FEs
*/
if (hda_check_fes(rtd, stream_dir,
stream->stream_tag)) {
res = hstream;
break;
}
} else {
res = hstream;

/*
* This must be a hostless stream.
* So reserve the host DMA channel.
*/
hda_stream->host_reserved = 1;
break;
}
}
}

snd_soc_dai_set_dma_data(dai, &substream, stream);
*rx_slot = hdac_stream(stream)->stream_tag - 1;

dev_dbg(bus->dev, "link dma channel %d for capture", *rx_slot);
if (res) {
/*
* Decouple host and link DMA. The decoupled flag
* is updated in snd_hdac_ext_stream_decouple().
*/
if (!res->decoupled)
snd_hdac_ext_stream_decouple(bus, res, true);
spin_lock_irq(&bus->reg_lock);
res->link_locked = 1;
res->link_substream = substream;
spin_unlock_irq(&bus->reg_lock);
}

return 0;
return res;
}

static int hda_link_dma_params(struct hdac_ext_stream *stream,
Expand Down Expand Up @@ -122,6 +150,51 @@ static int hda_link_dma_params(struct hdac_ext_stream *stream,
return 0;
}

/* Send DAI_CONFIG IPC to the DAI that matches the dai_name and direction */
static int hda_link_config_ipc(struct sof_intel_hda_stream *hda_stream,
const char *dai_name, int channel, int dir)
{
struct sof_ipc_dai_config *config;
struct snd_sof_dai *sof_dai;
struct sof_ipc_reply reply;
int ret = 0;

list_for_each_entry(sof_dai, &hda_stream->sdev->dai_list, list) {
if (!sof_dai->cpu_dai_name)
continue;

if (!strcmp(dai_name, sof_dai->cpu_dai_name) &&
dir == sof_dai->comp_dai.direction) {
config = sof_dai->dai_config;

if (!config) {
dev_err(hda_stream->sdev->dev,
"error: no config for DAI %s\n",
sof_dai->name);
return -EINVAL;
}

/* update config with stream tag */
config->hda.link_dma_ch = channel;

/* send IPC */
ret = sof_ipc_tx_message(hda_stream->sdev->ipc,
config->hdr.cmd,
config,
config->hdr.size,
&reply, sizeof(reply));

if (ret < 0)
dev_err(hda_stream->sdev->dev,
"error: failed to set dai config for %s\n",
sof_dai->name);
return ret;
}
}

return -EINVAL;
}

static int hda_link_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params,
struct snd_soc_dai *dai)
Expand All @@ -135,20 +208,31 @@ static int hda_link_hw_params(struct snd_pcm_substream *substream,
struct hda_pipe_params p_params = {0};
struct hdac_ext_link *link;
int stream_tag;
int ret;

link_dev = snd_soc_dai_get_dma_data(dai, substream);
link_dev = hda_link_stream_assign(bus, substream);
if (!link_dev)
return -EBUSY;

stream_tag = hdac_stream(link_dev)->stream_tag;

hda_stream = hstream_to_sof_hda_stream(link_dev);

/* update the DSP with the new tag */
ret = hda_link_config_ipc(hda_stream, dai->name, stream_tag - 1,
substream->stream);
if (ret < 0)
return ret;

snd_soc_dai_set_dma_data(dai, substream, (void *)link_dev);

hda_stream = container_of(link_dev, struct sof_intel_hda_stream,
hda_stream);
hda_stream->hw_params_upon_resume = 0;

link = snd_hdac_ext_bus_get_link(bus, codec_dai->component->name);
if (!link)
return -EINVAL;

stream_tag = hdac_stream(link_dev)->stream_tag;

/* set the stream tag in the codec dai dma params */
/* set the stream tag in the codec dai dma params */
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
snd_soc_dai_set_tdm_slot(codec_dai, stream_tag, 0, 0, 0);
else
Expand Down Expand Up @@ -181,8 +265,7 @@ static int hda_link_pcm_prepare(struct snd_pcm_substream *substream,
struct snd_soc_pcm_runtime *rtd = snd_pcm_substream_chip(substream);
int stream = substream->stream;

hda_stream = container_of(link_dev, struct sof_intel_hda_stream,
hda_stream);
hda_stream = hstream_to_sof_hda_stream(link_dev);

/* setup hw_params again only if resuming from system suspend */
if (!hda_stream->hw_params_upon_resume)
Expand All @@ -199,8 +282,24 @@ static int hda_link_pcm_trigger(struct snd_pcm_substream *substream,
{
struct hdac_ext_stream *link_dev =
snd_soc_dai_get_dma_data(dai, substream);
struct sof_intel_hda_stream *hda_stream;
struct snd_soc_pcm_runtime *rtd;
struct hdac_ext_link *link;
struct hdac_stream *hstream;
struct hdac_bus *bus;
int stream_tag;
int ret;

hstream = substream->runtime->private_data;
bus = hstream->bus;
rtd = snd_pcm_substream_chip(substream);

link = snd_hdac_ext_bus_get_link(bus, rtd->codec_dai->component->name);
if (!link)
return -EINVAL;

hda_stream = hstream_to_sof_hda_stream(link_dev);

dev_dbg(dai->dev, "In %s cmd=%d\n", __func__, cmd);
switch (cmd) {
case SNDRV_PCM_TRIGGER_RESUME:
Expand All @@ -217,8 +316,22 @@ static int hda_link_pcm_trigger(struct snd_pcm_substream *substream,
case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
snd_hdac_ext_link_stream_start(link_dev);
break;
case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
case SNDRV_PCM_TRIGGER_SUSPEND:
/*
* clear and release link DMA channel. It will be assigned when
* hw_params is set up again after resume.
*/
ret = hda_link_config_ipc(hda_stream, dai->name,
DMA_CHAN_INVALID, substream->stream);
if (ret < 0)
return ret;
stream_tag = hdac_stream(link_dev)->stream_tag;
snd_hdac_ext_link_clear_stream_id(link, stream_tag);
snd_hdac_ext_stream_release(link_dev,
HDAC_EXT_STREAM_TYPE_LINK);

/* fallthrough */
case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
case SNDRV_PCM_TRIGGER_STOP:
snd_hdac_ext_link_stream_clear(link_dev);
break;
Expand All @@ -228,62 +341,41 @@ static int hda_link_pcm_trigger(struct snd_pcm_substream *substream,
return 0;
}

/*
* FIXME: This API is also abused since it's used for two purposes.
* when the substream argument is NULL this function is used for cleanups
* that aren't necessarily required, and called explicitly by handling
* ASoC core structures, which is not recommended.
* This part will be reworked in follow-up patches.
*/
static int hda_link_hw_free(struct snd_pcm_substream *substream,
struct snd_soc_dai *dai)
{
const char *name;
unsigned int stream_tag;
struct sof_intel_hda_stream *hda_stream;
struct hdac_bus *bus;
struct hdac_ext_link *link;
struct hdac_stream *hstream;
struct hdac_ext_stream *stream;
struct snd_soc_pcm_runtime *rtd;
struct hdac_ext_stream *link_dev;
struct snd_pcm_substream pcm_substream;

memset(&pcm_substream, 0, sizeof(pcm_substream));
if (substream) {
hstream = substream->runtime->private_data;
bus = hstream->bus;
rtd = snd_pcm_substream_chip(substream);
link_dev = snd_soc_dai_get_dma_data(dai, substream);
snd_hdac_ext_stream_decouple(bus, link_dev, false);
name = rtd->codec_dai->component->name;
link = snd_hdac_ext_bus_get_link(bus, name);
if (!link)
return -EINVAL;

if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
stream_tag = hdac_stream(link_dev)->stream_tag;
snd_hdac_ext_link_clear_stream_id(link, stream_tag);
}
int ret;

link_dev->link_prepared = 0;
} else {
/* release all hda streams when dai link is unloaded */
pcm_substream.stream = SNDRV_PCM_STREAM_PLAYBACK;
stream = snd_soc_dai_get_dma_data(dai, &pcm_substream);
if (stream) {
snd_soc_dai_set_dma_data(dai, &pcm_substream, NULL);
snd_hdac_ext_stream_release(stream,
HDAC_EXT_STREAM_TYPE_LINK);
}
hstream = substream->runtime->private_data;
bus = hstream->bus;
rtd = snd_pcm_substream_chip(substream);
link_dev = snd_soc_dai_get_dma_data(dai, substream);
hda_stream = hstream_to_sof_hda_stream(link_dev);

pcm_substream.stream = SNDRV_PCM_STREAM_CAPTURE;
stream = snd_soc_dai_get_dma_data(dai, &pcm_substream);
if (stream) {
snd_soc_dai_set_dma_data(dai, &pcm_substream, NULL);
snd_hdac_ext_stream_release(stream,
HDAC_EXT_STREAM_TYPE_LINK);
}
}
/* free the link DMA channel in the FW */
ret = hda_link_config_ipc(hda_stream, dai->name, DMA_CHAN_INVALID,
substream->stream);
Copy link

@keyonjie keyonjie May 13, 2019

Choose a reason for hiding this comment

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

This is fine to me for short-term solution.
Though here you introduce a new IPC sending, it looks inevitable in current implementation,

dai_new()
    dai_config() // get dma channel
        dai_params()
    dai_config(DMA_CHAN_INVALID) // optional, used for HDA dai here to free DMA channel.
dai_free() //free channels for non-HDA dai.

The ideal solution for me is something like this:

dai_new()
    // dai_config() no needed
        dai_params() // get dma channel
        dai_reset() // free dma channel for all gpdma/link dma.
    // dai_config(DMA_CHAN_INVALID) no needed
dai_free()

Copy link
Collaborator Author

@ranj063 ranj063 May 13, 2019

Choose a reason for hiding this comment

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

@keyonjie i'd like to set the channel in dma_params too but thats going to need some fundamental changes in the firmware in the way the pipelines are set up. Today the params are passed all the way from the host to the DAI. So dai_params is only invoked if host_params is.
For this we're going to have to split up our pipelines into FE and BE pipelines like we've been discussing off late.

But whats the disadvantage with the current way of using dai_config. Agreed there's a couple of extra IPC's sent just to set the channels but its only one once at at start and once at hw_free. And there's also a request to support multiple DAI configs for each DAI and the ability to switch the config at run-time. This change goes well with that flow I think

if (ret < 0)
return ret;

link = snd_hdac_ext_bus_get_link(bus, rtd->codec_dai->component->name);
if (!link)
return -EINVAL;

stream_tag = hdac_stream(link_dev)->stream_tag;
snd_hdac_ext_link_clear_stream_id(link, stream_tag);
snd_hdac_ext_stream_release(link_dev, HDAC_EXT_STREAM_TYPE_LINK);
link_dev->link_prepared = 0;

/* free the host DMA channel reserved by hostless streams */
hda_stream->host_reserved = 0;

return 0;
}
Expand All @@ -293,7 +385,6 @@ static const struct snd_soc_dai_ops hda_link_dai_ops = {
.hw_free = hda_link_hw_free,
.trigger = hda_link_pcm_trigger,
.prepare = hda_link_pcm_prepare,
.get_channel_map = hda_link_dma_get_channels,
};
#endif

Expand Down
Loading