Skip to content

PM: refine algorithm of dai link resuming #509

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
Jan 15, 2019
Merged

PM: refine algorithm of dai link resuming #509

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
129 changes: 68 additions & 61 deletions sound/soc/sof/topology.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1991,6 +1991,7 @@ static int sof_set_dai_config(struct snd_sof_dev *sdev, u32 size,
struct sof_ipc_dai_config *config)
{
struct snd_sof_dai *dai;
int found = 0;

list_for_each_entry(dai, &sdev->dai_list, list) {
if (!dai->name)
Expand All @@ -2000,9 +2001,21 @@ static int sof_set_dai_config(struct snd_sof_dev *sdev, u32 size,
dai->dai_config = kmemdup(config, size, GFP_KERNEL);
if (!dai->dai_config)
return -ENOMEM;

found = 1;
}
}

/*
* machine driver may define a dai link with playback and capture
* dai enabled, but the dai link in topology would support both, one
* or none of them. Here print a warning message to notify user
*/
if (!found) {
dev_warn(sdev->dev, "warning: failed to find dai for dai link %s",
link->name);

@plbossart plbossart Jan 14, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we really be returning 0 in that case?

@RanderWang RanderWang Jan 15, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, I will add a comment for it. I discussed with @ranj063 that dai may not be supported by topology although the dai link is defined in machine driver, so none of dai would be found. But we should print a warning to notify user: is this really you want?

@ranj063 ranj063 Jan 15, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@plbossart the rationale for returning a 0 is that even if we dont find a dai to set the config for, there will really be no negative consequences of having just a BE dai config without a pipeline that uses it.
Of course, logically it probably makes no sense. Thats why the warning here.

}

return 0;
}

Expand Down Expand Up @@ -2194,48 +2207,75 @@ static int sof_link_dmic_load(struct snd_soc_component *scomp, int index,
return ret;
}

/*
* for hda link, playback and capture are supported by different dai
* in FW. Here get the dai_index, set dma channel of each dai
* and send config to FW. In FW, each dai sets config by dai_index
*/
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)
int tx_slot,
int rx_slot)

@plbossart plbossart Jan 10, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this is very hard to review, you add changes to hda_process in this patch, and again changes in the next patch. I had to go directly to the code to figure out what the result is. Please don't do this, it's a waste of time for reviewers.

@RanderWang RanderWang Jan 11, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This patch is to simplify HDA processing, and another patch is to fix dai saving. I want to make reviewer understand it step by step, not all in one patch. I could merge them together.

{
struct sof_ipc_reply reply;
u32 size = sizeof(*config);
struct snd_sof_dai *dai;
struct snd_sof_dai *sof_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)
list_for_each_entry(sof_dai, &sdev->dai_list, list) {
if (!sof_dai->name)
continue;

if (strcmp(link->name, dai->name) == 0 &&
dai->comp_dai.direction == direction) {
config->dai_index = dai->comp_dai.dai_index;
if (strcmp(link->name, sof_dai->name) == 0) {
if (sof_dai->comp_dai.direction ==
SNDRV_PCM_STREAM_PLAYBACK) {

@ranj063 ranj063 Jan 11, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@RanderWang hopefully this will be the last issue. Apart from checking the direction, you also need to check if tx_slot is non-zero to proceed here. Similarly for capture.

@RanderWang RanderWang Jan 14, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@ranj063 I have thought about this. There is no need here
(1) if there is a playback dai then tx_slot should be correct, please see sof_link_hda_load.
tx_slot would be queried if this dai link support playback. So I don't want to check it again.

(2) tx_slot could be zero and this means dma channel zero is used for hda.

@ranj063 ranj063 Jan 14, 2019 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@ranj063 I have thought about this. There is no need here
(1) if there is a playback dai then tx_slot should be correct, please see sof_link_hda_load.
tx_slot would be queried if this dai link support playback. So I don't want to check it again.

(2) tx_slot could be zero and this means dma channel zero is used for hda.

@RanderWang sorry what does it mean by "dma channel zero is used for hda"?
Also what about the capture case, if rx_slot is zero, you should be doing nothing but with this change you'd still send an ipc, isnt it?

@ranj063 ranj063 Jan 14, 2019 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@RanderWang i think I get your point. Basically if rx_slot is zero, the expectation is there we will not be able to find dai with the same dai_link with a capture direction. Is that your argument?

@RanderWang RanderWang Jan 14, 2019 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@ranj063 I just took playback for example. Capture is the same as playback. tx_slot and rx_slot
could be zero. They could be only sent if this dai link supports them

@RanderWang RanderWang Jan 14, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@ranj063 But I still don't get why we would remove playback and capture on SSP2 and still have just the dai_config for SSP2 BE dai link.

@ranj063 ranj063 Jan 14, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@RanderWang the point is you could have just playback or just capture on SSP2.

@RanderWang RanderWang Jan 14, 2019 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

you idea is: playback or capture are defined in machine driver, but our topology could support it or not. I discuss with Keyon, and we agree with you at this point. Our idea is: machine driver could define two dai, but topology should support at least one dai. So the error check in sof_set_dai_config is correct. It only check whether at least one dai exist. And in hda function, only check one of playback or capture exsit. The code should be the same as sof_set_dai_config. What is your idea? thanks!

@ranj063 ranj063 Jan 14, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@RanderWang @keyonjie I think there's no need to check even if one dai is set. The machine driver could define SSP2 but what if I do not want to use SSP2 at all for playback/capture?Logically, in that case i'd probably remove the dai_config for SSP2 but I could just leave it as well.

@RanderWang RanderWang Jan 14, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@ranj063 if you do not want to use SSP2 at all , why this dai link is set in topology file? If it is not in topology file, these function could not be called.

if (!link->dpcm_playback)
return -EINVAL;

config->hda.link_dma_ch = tx_slot;
} else {
if (!link->dpcm_capture)
return -EINVAL;

config->hda.link_dma_ch = rx_slot;
}

@plbossart plbossart Jan 9, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What I meant is try to pass link and config ONCE, then use it internablly in sof_set_dai_config.
Passing two arguments, once of which is NULL with additional tests is just horrible and makes the code more confusing.

@RanderWang RanderWang Jan 10, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

for HDA, two different configs need to be set by sof_set_dai_config.
If I just pass config ONCE, I need to add direction and call sof_set_dai_config two times.
The direction info is not included by link setting or config.

I will try to refine it without NULL.


config->dai_index = sof_dai->comp_dai.dai_index;
found = 1;
break;

/* save config in dai component */
sof_dai->dai_config = kmemdup(config, size, GFP_KERNEL);
if (!sof_dai->dai_config)
return -ENOMEM;

@plbossart plbossart Jan 10, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

you need to test the status here?

@plbossart plbossart Jan 10, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this look good at first sight, but then looking deeper the code does this

/* 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) {
			config->dai_index = dai->comp_dai.dai_index;

			if (dai->comp_dai.direction ==
			    SNDRV_PCM_STREAM_PLAYBACK)
				config->hda.link_dma_ch = tx_slot;
			else
				config->hda.link_dma_ch = rx_slot;

			ret = sof_set_dai_config(sdev, size, link,
						 config);
<<<<
static int sof_set_dai_config(struct snd_sof_dev *sdev, u32 size,
			      struct snd_soc_dai_link *link,
			      struct sof_ipc_dai_config *config)
{
	struct snd_sof_dai *dai;

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

		if (strcmp(link->name, dai->name) == 0 &&
		    dai->comp_dai.dai_index == config->dai_index) {
			dai->dai_config = kmemdup(config, size, GFP_KERNEL);
			if (!dai->dai_config)
				return -ENOMEM;
		}
	}

	return 0;
}
<<<<<

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

			if (ret < 0) {
				dev_err(sdev->dev, "error: failed to set DAI config for direction:%d of HDA dai %d\n",
					dai->comp_dai.direction,
					config->dai_index);

				return ret;
			}

			found = 1;
		}
	}

so you have two nested loops that look for the same data. Doesn't seem terribly efficient.

@ranj063 ranj063 Jan 10, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@plbossart that is a good catch. we should just kmemdup the config before sending the IPC.

@RanderWang RanderWang Jan 11, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

yes, I know this issue but I want to make HDA work as SSP so I call sof_set_dai_config.
Ok, I will refine it to avoid this call.

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

if (ret < 0) {
dev_err(sdev->dev, "error: failed to set DAI config for direction:%d of HDA dai %d\n",
sof_dai->comp_dai.direction,
config->dai_index);

return ret;
}
}
}

/*
* machine driver may define a dai link with playback and capture
* dai enabled, but the dai link in topology would support both, one
* or none of them. Here print a warning message to notify user
*/
if (!found) {
dev_err(sdev->dev, "error: failed to find dai %s in %s",
link->name, __func__);
return -EINVAL;
dev_warn(sdev->dev, "warning: failed to find dai for dai link %s",
link->name);
}

@keyonjie keyonjie Jan 14, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@RanderWang as we discussed, let's change this to be:

if ((link->dpcm_playback && playback_found) ||
    (link->dpcm_capture && capture_found)) {
        dev_info(...);//success
} else {
        dev_err();// error.
        return -EINVAL;
}

@ranj063 ranj063 Jan 14, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@RanderWang @keyonjie should it be a warning instead of error. My point is that nothing would be broken in this case. Its just that we will not have a pcm that uses the BE with this config in this case.

@RanderWang RanderWang Jan 14, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@ranj063 yes, I get your idea and I am refining it now

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

static int sof_link_hda_load(struct snd_soc_component *scomp, int index,
Expand All @@ -2248,7 +2288,6 @@ static int sof_link_hda_load(struct snd_soc_component *scomp, int index,
struct snd_soc_dai_link_component dai_component;
struct snd_soc_tplg_private *private = &cfg->priv;
struct snd_soc_dai *dai;

u32 size = sizeof(*config);
u32 tx_num = 0;
u32 tx_slot = 0;
Expand Down Expand Up @@ -2294,42 +2333,10 @@ static int sof_link_hda_load(struct snd_soc_component *scomp, int 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;
}
}

/* set config for all DAI's with name matching the link name */
ret = sof_set_dai_config(sdev, size, link, config);
if (ret < 0) {
if (link->dpcm_playback)
dev_err(sdev->dev, "error: failed to save DAI config for playback HDA%d\n",
config->dai_index);

if (link->dpcm_capture)
dev_err(sdev->dev, "error: failed to save DAI config for capture HDA%d\n",
config->dai_index);
}
ret = sof_link_hda_process(sdev, link, config, tx_slot, rx_slot);
if (ret < 0)
dev_err(sdev->dev, "error: failed to process hda dai link %s",
link->name);

return ret;
}
Expand Down