Skip to content

[RFC] ASoC: SOF: set link dma channel in hda config #291

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 1 commit into from
Nov 29, 2018
Merged

[RFC] ASoC: SOF: set link dma channel in hda config #291

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 1 addition & 2 deletions include/sound/sof/dai-intel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,7 @@ struct sof_ipc_dai_ssp_params {

/* HDA Configuration Request - SOF_IPC_DAI_HDA_CONFIG */
struct sof_ipc_dai_hda_params {
struct sof_ipc_hdr hdr;
/* TODO */
uint32_t link_dma_ch;
} __packed;

/* DMIC Configuration Request - SOF_IPC_DAI_DMIC_CONFIG */
Expand Down
120 changes: 98 additions & 22 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 @@ -33,7 +33,59 @@ struct hda_pipe_params {
unsigned int link_bps;
};

/* TODO: add hda dai params in tplg, and configure this in topology parsing */
/* 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 the correct
* link dma channel, host allocates stream register and sends the
* corresponding link dma channel to FW to allocate link dma channel
*/
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)
{
Copy link
Member

@plbossart plbossart Nov 15, 2018

Choose a reason for hiding this comment

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

alignment is out of whack?

Copy link
Author

@RanderWang RanderWang Nov 16, 2018

Choose a reason for hiding this comment

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

yes, I will refine it later. I just finish this RFC before leaving office. Sorry, it is not perfect.

Copy link
Member

@lgirdwood lgirdwood Nov 21, 2018

Choose a reason for hiding this comment

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

Also better if the masks and slots can be embedded in a new structure, that way you are only passing a single ptr to a structure rather than 4 ptrs to ints.

Copy link
Author

@RanderWang RanderWang Nov 22, 2018

Choose a reason for hiding this comment

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

this is a ASoC interface and I just reuse it for hda.

struct hdac_bus *bus;
struct hdac_ext_stream *stream;
struct snd_pcm_substream substream = {0};
struct snd_sof_dev *sdev =
snd_soc_component_get_drvdata(dai->component);

bus = sof_to_bus(sdev);

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, "failed to find a free hda ext stream for playback");
return -EBUSY;
}

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

dev_dbg(bus->dev, "link dma channel %d for playback", *tx_slot);
}

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, "failed to find a free hda ext stream for capture");
return -EBUSY;
}

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

dev_dbg(bus->dev, "link dma channel %d for capture", *rx_slot);
}

return 0;
}

static int hda_link_dma_params(struct hdac_ext_stream *stream,
struct hda_pipe_params *params)
{
Expand Down Expand Up @@ -78,12 +130,7 @@ static int hda_link_hw_params(struct snd_pcm_substream *substream,
struct hdac_ext_link *link;
int stream_tag;

link_dev = snd_hdac_ext_stream_assign(bus, substream,
HDAC_EXT_STREAM_TYPE_LINK);
if (!link_dev)
return -EBUSY;

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

link = snd_hdac_ext_bus_get_link(bus, codec_dai->component->name);
if (!link)
Expand Down Expand Up @@ -147,22 +194,50 @@ static int hda_link_pcm_trigger(struct snd_pcm_substream *substream,
static int hda_link_hw_free(struct snd_pcm_substream *substream,
struct snd_soc_dai *dai)
{
struct hdac_stream *hstream = substream->runtime->private_data;
struct hdac_bus *bus = hstream->bus;
struct snd_soc_pcm_runtime *rtd = snd_pcm_substream_chip(substream);
struct hdac_ext_stream *link_dev =
snd_soc_dai_get_dma_data(dai, substream);
const char *name;
unsigned int stream_tag;
struct hdac_bus *bus;
struct hdac_ext_link *link;

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

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

link_dev->link_prepared = 0;
struct snd_sof_dev *sdev;
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 = {0};

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);
name = rtd->codec_dai->component->name;
link = snd_hdac_ext_bus_get_link(bus, name);
if (!link)
return -EINVAL;

stream_tag = hdac_stream(link_dev)->stream_tag;
snd_hdac_ext_link_clear_stream_id(link, stream_tag);

link_dev->link_prepared = 0;
} else {
Copy link
Member

@plbossart plbossart Nov 27, 2018

Choose a reason for hiding this comment

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

in what cases is the 'else' selected? I really don't understand this code, sorry.

Copy link
Author

@RanderWang RanderWang Nov 28, 2018

Choose a reason for hiding this comment

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

else case: pcm is NULL, this the case for link dma resource free.

Copy link
Member

@plbossart plbossart Nov 28, 2018

Choose a reason for hiding this comment

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

let me re-ask the question: is hda_link_hw_free() called at different times with substream==NULL and substream != NULL conditions? if yes, can you explain the two cases?

Copy link
Author

@RanderWang RanderWang Nov 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

yes, two cases:
(1) it is called when audio stream is stopped, at this time you could get substream to do some work
(2) at dai_link_unload. I reuse this function with null substream.
It is very tricky to do like this. I also don't like it now. Now I want to do it in hda_dsp_free, but it is impossible do at there because there is no dai.

I am very struggled to find a interface to free logical stream. Actually, it is no need to free it and it would be free when sof is removed.

/* release all hda streams when dai link is unloaded */
sdev = snd_soc_component_get_drvdata(dai->component);
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);
}

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

return 0;
}
Expand All @@ -171,6 +246,7 @@ static const struct snd_soc_dai_ops hda_link_dai_ops = {
.hw_params = hda_link_hw_params,
.hw_free = hda_link_hw_free,
.trigger = hda_link_pcm_trigger,
.get_channel_map = hda_link_dma_get_channels,
};
#endif

Expand Down
159 changes: 149 additions & 10 deletions sound/soc/sof/topology.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2028,16 +2028,65 @@ static int sof_link_dmic_load(struct snd_soc_component *scomp, int index,
return ret;
}

static int sof_link_hda_process(struct snd_sof_dev *sdev,
struct snd_soc_dai_link *link,
struct sof_ipc_dai_config *config,
int slot,
int direction)
{
struct sof_ipc_reply reply;
u32 size = sizeof(*config);
struct snd_sof_dai *dai;
int found = 0;
int ret;

/* for hda link, playback and capture are supported by different
* dai in FW. Here get the dai_index of each dai and send config
* to FW. In FW, each dai sets config by dai_index
*/
list_for_each_entry(dai, &sdev->dai_list, list) {
if (!dai->name)
continue;

if (strcmp(link->name, dai->name) == 0 &&
dai->comp_dai.direction == direction) {
config->dai_index = dai->comp_dai.dai_index;
found = 1;
break;
}
}

if (!found) {
dev_err(sdev->dev, "failed to find dai %s", link->name);
return -EINVAL;
}

config->hda.link_dma_ch = slot;

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

return ret;
}

static int sof_link_hda_load(struct snd_soc_component *scomp, int index,
struct snd_soc_dai_link *link,
struct snd_soc_tplg_link_config *cfg,
struct snd_soc_tplg_hw_config *hw_config,
struct sof_ipc_dai_config *config)
{
struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(scomp);
struct snd_soc_dai_link_component dai_component = {0};
struct snd_soc_tplg_private *private = &cfg->priv;
struct sof_ipc_reply reply;
struct snd_soc_dai *dai;

u32 size = sizeof(*config);
u32 tx_num = 0;
u32 tx_slot = 0;
u32 rx_num = 0;
u32 rx_slot = 0;
int ret;
Copy link
Member

@plbossart plbossart Nov 21, 2018

Choose a reason for hiding this comment

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

i like one variable per line, otherwise inits are hidden in plain sight...

Copy link
Author

@RanderWang RanderWang Nov 22, 2018

Choose a reason for hiding this comment

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

yes


/* init IPC */
Expand All @@ -2054,18 +2103,50 @@ static int sof_link_hda_load(struct snd_soc_component *scomp, int index,
return ret;
}

dev_dbg(sdev->dev, "tplg: config HDA%d fmt 0x%x\n",
config->dai_index, config->format);
dai_component.dai_name = link->cpu_dai_name;
dai = snd_soc_find_dai(&dai_component);
if (!dai) {
dev_err(sdev->dev, "failed to find dai %s", dai->name);
return -EINVAL;
}

/* send message to DSP */
ret = sof_ipc_tx_message(sdev->ipc,
config->hdr.cmd, config, size, &reply,
sizeof(reply));
if (link->dpcm_playback)
tx_num = 1;

if (ret < 0)
dev_err(sdev->dev, "error: failed to set DAI config for HDA%d\n",
if (link->dpcm_capture)
rx_num = 1;

ret = snd_soc_dai_get_channel_map(dai, &tx_num, &tx_slot,
&rx_num, &rx_slot);
if (ret < 0) {
dev_err(sdev->dev, "error: failed to get dma channel for HDA%d\n",
config->dai_index);

return ret;
}

/* for hda link, playback and capture are supported by different
* dai in FW. Here send dai config according to capability of dai.
*/
if (link->dpcm_playback) {
ret = sof_link_hda_process(sdev, link, config, tx_slot,
SNDRV_PCM_STREAM_PLAYBACK);
if (ret < 0) {
dev_err(sdev->dev, "error: failed to set DAI config for playback dai HDA%d\n",
config->dai_index);

return ret;
}
}

if (link->dpcm_capture) {
ret = sof_link_hda_process(sdev, link, config, rx_slot,
SNDRV_PCM_STREAM_CAPTURE);
if (ret < 0)
dev_err(sdev->dev, "error: failed to set DAI config for capture dai HDA%d\n",
config->dai_index);
}

return ret;
}

Expand Down Expand Up @@ -2152,10 +2233,68 @@ static int sof_link_load(struct snd_soc_component *scomp, int index,
return 0;
}

static int sof_link_hda_unload(struct snd_sof_dev *sdev,
struct snd_soc_dai_link *link)
{
struct snd_soc_dai_link_component dai_component = {0};
struct snd_soc_dai *dai;
int ret = 0;

dai_component.dai_name = link->cpu_dai_name;
dai = snd_soc_find_dai(&dai_component);
if (!dai) {
dev_err(sdev->dev, "failed to find dai %s", dai->name);
return -EINVAL;
}

if (dai->driver->ops->hw_free)
ret = dai->driver->ops->hw_free(NULL, dai);
if (ret < 0)
dev_err(sdev->dev, "error: failed to free hda resource for %s\n",
link->name);

return ret;
}

static int sof_link_unload(struct snd_soc_component *scomp,
struct snd_soc_dobj *dobj)
{
return 0;
struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(scomp);
struct snd_soc_dai_link *link =
container_of(dobj, struct snd_soc_dai_link, dobj);

struct snd_sof_dai *sof_dai = NULL;
int ret = 0;

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

if (strcmp(link->name, sof_dai->name) == 0)
break;
}

if (!sof_dai) {
dev_err(sdev->dev, "failed to find dai %s", link->name);
return -EINVAL;
}

switch (sof_dai->dai_config.type) {
case SOF_DAI_INTEL_SSP:
case SOF_DAI_INTEL_DMIC:
Copy link
Member

@plbossart plbossart Nov 27, 2018

Choose a reason for hiding this comment

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

/* fall through */ comment needed

Copy link
Author

@RanderWang RanderWang Nov 28, 2018

Choose a reason for hiding this comment

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

yes, I will add it

/* no resource needs to be released for SSP and DMIC */
break;
case SOF_DAI_INTEL_HDA:
ret = sof_link_hda_unload(sdev, link);
break;
default:
dev_err(sdev->dev, "error: invalid DAI type %d\n",
sof_dai->dai_config.type);
ret = -EINVAL;
break;
}

return ret;
}

/* bind PCM ID to host component ID */
Expand Down