Skip to content

ASoC:SOF:pm fix DAI config during resume #450

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
zhuyingjiang wants to merge 1 commit into from
Closed

ASoC:SOF:pm fix DAI config during resume #450

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
18 changes: 6 additions & 12 deletions sound/soc/sof/topology.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2161,6 +2161,12 @@ static int sof_link_hda_process(struct snd_sof_dev *sdev,

config->hda.link_dma_ch = slot;

/* Set the DAI config here since both DAI name and direction must
* be matched, this is important for playback and capture with the
* same DAI name.

@plbossart plbossart Dec 18, 2018

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.

I couldn't figure out what the comment means. You cannot match the DAI direction for playback and capture, the two parts of the sentence are logically opposite.

@zhuyingjiang zhuyingjiang Dec 19, 2018

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.

I mean if the playback and capture have the same DAI name, we need both check the name and the direction to find the right dai we need, and thus set the dai config.

*/
dai->dai_config = kmemdup(config, size, GFP_KERNEL);

@RanderWang RanderWang Dec 18, 2018 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

yes, could you add a comment for it ? I know you don't call sof_set_dai_config, but you need to add the comment "set config for the DAI with name and direction matched". Or you could add other comments to emphasize the job of sof_set_dai_config is done at here

@plbossart plbossart Dec 18, 2018

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.

Always check for memory alloc issues!

@zhuyingjiang zhuyingjiang Dec 19, 2018

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.

There is no memory leak here now, previous may cause memory leak. The problem is like:
suppose we have a dai list:
{
dai_name="Analog", direction="Playback";
dai_name="Analog", direction="Capture";
dai_name="Digital MIC", direction="Capture";
dai_name="Digital SSP", direction="Playback";
...
}
In the previous code(with the remove part), will do the kmemdup if the name matches, e.g "Analog", when we only set for dai with name "Analog" and direction "Playback", it will kmemdup two times the dai config as only check the name, "Analog Playback" and "Analog Capture" are all set to the dai config for "Analog Playback".
The next time when set for dai with name "Analog" and direction "Capture", still kmemdup two times the capture dai config for both "Analog Playback" and "Analog Capture".
From the tx message to firmware, we can see message for capture send two times, while no tx message for playback.
This is why the bug happens.

@plbossart plbossart Dec 19, 2018

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.

I talked about memory alloc and sorry mate but this can fail. you need to test all memory allocation. No discussion allowed on this topic.
Memory leaks happen when you don't free (which is a different problem).

Also I am not clear about your change. We have 2 routines which do something similar:

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

and

/* set config for all DAI's with name matching the link name */
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->dai_config = kmemdup(config, size, GFP_KERNEL);
			if (!dai->dai_config)
				return -ENOMEM;
		}
	}

	return 0;
}

It seems to me we have two routines doing the same thing and that if the direction needs to be used it could be factored in with a boolean flag. I don't like copy/paste code with differences, it's a sure way to write spaghetti code.

@zhuyingjiang zhuyingjiang Dec 19, 2018

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.

OK, got it. will add check if alloc fails.

@RanderWang RanderWang Dec 19, 2018 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@plbossart I discussed with @ranj063, one issue in our restore process is: each dai link is restored by each dai component, but there are two dai components(playback and capture) in one dai link. This makes one dai_link is restored two times not one for I2S. I will refine this algorithm.

@RanderWang RanderWang Dec 19, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

but for HDA, this algorithm is correct

@zhuyingjiang zhuyingjiang Dec 19, 2018

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.

Now only dai link with type SSP and DMIC call sof_set_dai_config and use dai name to find the dai.
After remove sof_set_dai_config in sof_link_hda_load, the HDA dai link find the dai with name and direction in sof_link_hda_process, so I think this is not duplicate routine.
The problem is: Is that only HDA will have playback/capture with the same dai name?
If yes, I think this is OK to keep this. If not, that means SSP and other still will have playback/capture with the same dai name, then probably, we need to change sof_set_dai_config to use name and direction to find the dai.
@RanderWang @ranj063 how do you think?

@RanderWang RanderWang Dec 19, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

yes, playback/capture in SSP have the same dai name


/* send message to DSP */
ret = sof_ipc_tx_message(sdev->ipc,
config->hdr.cmd, config, size, &reply,
Expand Down Expand Up @@ -2250,18 +2256,6 @@ static int sof_link_hda_load(struct snd_soc_component *scomp, int index,
}
}

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

@plbossart plbossart Dec 18, 2018

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.

I am lost as to why this is removed? doesn't that function do precisely what you wanted in the first place?

@zhuyingjiang zhuyingjiang Dec 19, 2018

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.

As state before, the HDA dai need match both name and direction, sof_set_dai_config is a common API which only check the name. So the dai config setting part is move into the sof_link_hda_process, where the name and direction both checked.

@plbossart plbossart Dec 19, 2018

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.

please look into extending it to cover direction then. No need for duplication of code.

@zhuyingjiang zhuyingjiang Dec 19, 2018

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.

OK, it is OK to add direction check in sof_set_dai_config, and it will also effect sof_link_ssp_load and sof_link_dmic_load.

@ranj063 ranj063 Dec 19, 2018 •
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.

@zhuyingjiang you could do some other checks in the function to check the type as well....if type is hda, check name and direction, else just name is enough. make sense?

@zhuyingjiang zhuyingjiang Dec 19, 2018

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.

@RanderWang what is your opinion? since you write the sof_set_dai_config, how to refine this is the better way?

@RanderWang RanderWang Dec 19, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

sof_set_dai_config is implemented by Ranjiani. I have discussed with Ranjiani, we need to refine pipeline restore. Now dai link is restored by dai component. This is wrong because there maybe two dai components, playback and capture, or one (playback or capture) in one dai link, and dai link would be restored two times for I2S.

I plan to discard all the sof_set_dai_configs and restoring code in pm, and make clear of dai components and dai links. dai links should be recorded and restored by dai_link_list, not by dai components

@zhuyingjiang do you want to do it? Or just close this PR and I will fix it.

@zhuyingjiang zhuyingjiang Dec 19, 2018

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.

@RanderWang OK, please go ahead refine your code, please also consider the two dai_index Pierre mentioned.

@RanderWang RanderWang Dec 19, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ok, I will consider my plan again. I don't want to make a aggressive change without great improvement

return ret;
}

Expand Down