[RanderWang]

Add direction check in sof_set_dai_config because dai config
is different between playback and capture on HDA platform now,
so dai config would be saved by name and direction

this fixs #442

[plbossart]

Thanks @RanderWang. This looks much better than the previous versions but can be improved further (also the commit message isn't very clear)

[plbossart]

so this is just to keep track of the fact that the DAI was configured already, right?

[RanderWang]

yes, I will add a comment for it

[ranj063]

@RanderWang i'm a bit confused by this logic because there are 3 types of dai's, each with indices ranging from 0 - max_of_each_type. But you use only one array of 0-max. So depending on which order the dai's were added, you could miss out on configuring some dai's. please correct me if i am wrong.

Also, we dont really have any issues because of sending the dai config twice for the same dai. Is it really important to avoid the second IPC if it does no harm?

[RanderWang]

@RanderWang i'm a bit confused by this logic because there are 3 types of dai's, each with indices ranging from 0 - max_of_each_type. But you use only one array of 0-max. So depending on which order the dai's were added, you could miss out on configuring some dai's. please correct me if i am wrong.

yes, there maybe 3 types of dais but not each indices ranging from 0 - max. All of them are in the range of 0 - max

Also, we dont really have any issues because of sending the dai config twice for the same dai. Is it really important to avoid the second IPC if it does no harm?

no, we have a issue. If one dai config is set two times, one DMA channel would be wasted in FW. please refer to thesofproject/sof#786

[ranj063]

@RanderWang thanks for confirming that this is required.

But about my first comment, so can different types of DAI's not have the same dai index?

[ranj063]

looking at sof-apl-nocodec.m4 , i feel like the assumption that the dai index for all DAI's is unique is wrong.

[RanderWang]

I check the file and don't find a same id. First we should make clear the usage of dai index. It is used in ASoC which doesn't care the type of dai. It just identifies dai by index.

[plbossart]

Meh. if the firmware can detect the configs are the same, why couldn't the driver do it as well?

[RanderWang]

@plbossart thanks for your comment. It make easier for me now. And the PR in FW is ready thesofproject/sof#786

[plbossart]

What does this FW PR have to do with configs being sent twice?

[RanderWang]

dma channel is not released when config is set again. This PR would check whether dma channel is allocated for a dai link config and avoid allocating dma channel again @keyonjie

[ranj063]

@RanderWang so are you suggesting we just send the config twice as the FW can handle it now?

[RanderWang]

update my PR.
(1) remove dai link check in kernel. FW could deal with this case
(2) check the status of dai config saving, and report error in one location to make it clear

[ranj063]

@RanderWang did you intend to proceed further if ret is < 0 here?

[RanderWang]

@ranj063 no, I will refine it

[ranj063]

@RanderWang the other thing Pierre suggested was to use a loop so you dont have the same code twice for playback and capture. You could try something like this:

for (dir = SNDRV_PCM_STREAM_PLAYBACK; dir <= SNDRV_PCM_STREAM_CAPTURE; dir++) {
flag = (dir == SNDRV_PCM_STREAM_PLAYBACK) ? link->dpcm_playback : link->dpcm_capture;
if (flag)
ret = sof_set_dai_config(sdev, size, link, config, dir);
}

[RanderWang]

do we need to use loop for two items? I don't like it

[plbossart]

I am even wondering why we can't deal with the direction inside of sof_set_dai_config since all the other arguments are identical. It'd simplify the code further.

[RanderWang]

yes, I will do it

[plbossart]

this PR changed directions multiple times, you are soon going to hit your quota of reviewer bandwidth.

[plbossart]

I am even wondering why we can't deal with the direction inside of sof_set_dai_config since all the other arguments are identical. It'd simplify the code further.

[RanderWang]

Update my PR, thanks!

[plbossart]

Thanks @RanderWang The first patch was fine and simple-enough (only minor improvement suggested), the second one isn't what I had in mind and makes things less readable.
So either you remove the second patch or you make it nicer, your call.

[plbossart]

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]

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.

[ranj063]

@RanderWang same here use SNDRV_PCM_STREAM_PLAYBACK instead of 0

[RanderWang]

yes, I will refine it

[ranj063]

and here

[RanderWang]

yes, I will refine it

[ranj063]

@RanderWang I think this change looks rather awkward. You really only use both playback and capture configs for SSP dai links and in that case both of them are the same.

[ranj063]

@RanderWang here's a suggestion to modify the patch. It is only compile tested. Please feel free to modify it and clean up as needed.
rander_patch.txt

[RanderWang]

update my PR, thanks.

[RanderWang]

@RanderWang here's a suggestion to modify the patch. It is only compile tested. Please feel free to modify it and clean up as needed.
rander_patch.txt

Thank you very much! I find another simple way to fix it.

[plbossart]

I try to be patient, polite and respectful, but I am afraid this is really still quite flaky . It's a mix of basic mistakes like not testing the error status and more fundamental ones with complicated and inefficient code. i've spend several hours this week on this...

[plbossart]

should there be a check that there was at least one change done? Currently this function only return 0 or -ENOMEM. If there was no match we still return zero, is this intentional?

[RanderWang]

ok, I will add result check.

[plbossart]

you need to test the status here?

[plbossart]

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]

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

[RanderWang]

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.

[plbossart]

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]

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.

[RanderWang]

Thanks for your patient. It is all caused by ugly design of HDA. It is a challenge to build beautiful code based on ugly base. Next let us refine our HDA code as SSP , then we don't have to struggle with divergence.

[RanderWang]

update PR, thanks!

[ranj063]

@RanderWang minor comment. shouldnt this be "failed to find dai" and not dai link

[ranj063]

@RanderWang This looks a lot cleaner but if tx_slot and rx_slot are both non-zero, you need to make sure both are set right? But here, you only check if one is found.

[RanderWang]

yes, both are set if non-zero. I get your idea

[plbossart]

On 1/11/19 2:11 AM, RanderWang wrote: update PR, thanks for review.

I will look at it if Ranjani/Keyon approve first, other things to do atm.

[ranj063]

@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]

@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 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]

@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]

@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]

@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]

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

[RanderWang]

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]

@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]

@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.

[keyonjie]

@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]

@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]

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

[RanderWang]

update my PR, after discussing with Ranjani and keyon, I do these change.
Machine driver may define dai link with playback and capture enabled, but the dai
link in topology would support both, one or none of them. So a warning would be reported
here if no dai found

Thanks for review!

[keyonjie]

LGTM.

[plbossart]

There is a very suspicious test that makes me think the code isn't quite right.
Also error handling on found=0 isn't clear (add a comment or fix) and couple of comment style issues.

[plbossart]

Should we really be returning 0 in that case?

[RanderWang]

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]

@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.

[plbossart]

ret is not set before testing it at each iteration, what are you trying to check here?
This looks like a mistake.

[RanderWang]

I am very sorry to have this error. I will remove it. I change my code according to reviewer's comment, but here I missed to delete it.

[plbossart]

fix block comment style, should be

/*
* machine
*/

[RanderWang]

yes, I will

[plbossart]

fix block comment style

[RanderWang]

yes, I will

[RanderWang]

update my PR
(1) fix style issue
(2) fix error checking issue
(3) add comment to explain found check

Test in on WHL. Thanks for review!

[plbossart]

ok let's merge. 97 comments is officially a record.

[wenqingfu]

if it's not 'fixs' in the header, #442 will be closed after the merge :)

[RanderWang]

if it's not 'fixs' in the header, #442 will be closed after the merge :)

copy & paste error......

[RanderWang]

“fixs #422”字样必须出现在submit PR的那个初始帖子或者commit message里, 否则也是无效的.
copy homework with error included

[wenqingfu]

🤦‍♂️