ASoC: SOF: PM: refine algorithm of dai link resuming by RanderWang · Pull Request #472 · thesofproject/linux

RanderWang · 2018-12-19T06:54:48Z

Dai link is restored by each dai component. This is wrong because
there maybe two dai components, playback and capture, or one
(playback or capture) in one dai link, so a dai link may be restored
two times, but only one dai link is needed.

Now restore dai link from index 0 to max index, each dai link is
restored only one time

Signed-off-by: Rander Wang rander.wang@linux.intel.com
Signed-off-by: Zhu Yingjiang yingjiang.zhu@linux.intel.com
Signed-off-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com

RanderWang · 2018-12-19T06:55:57Z

hi @zhuyingjiang , can you help to test it? I don't have the environment. Thanks!

RanderWang · 2018-12-19T08:13:31Z

I test it on APL and WHL, no problem is found.

plbossart · 2018-12-19T14:35:43Z

I have a conceptual problem with all this. From a hardware perspective, I see no reason why HDaudio should be different from SSP. We could if we wanted have both playback and capture supported with the same DAI name.

So I wonder if the code differences were added because of naming conventions used in skl_generic_hda where the playback and capture DAI links were separated, in a somewhat arbitrary manner.

In short, are we dealing with a problem liking to the hardware itself, or a software problem coming from the machine driver?

plbossart

I am starting to see what @RanderWang is suggesting, but this still needs some work. we should not make assumptions such as HDAudio is always in two dailinks or the config is stored in the playback component. Let's make this a bit more abstract and generic. Thanks!

plbossart · 2018-12-19T17:15:43Z

when would this fail? Is this a new test added for safety or is this a hidden feature?

@plbossart @RanderWang the intention seems to be correct here but the assumption that only the playback dai has the config is probably flawed.

During resume, we should avoid sending the dai config ipc twice for I2S. But this shouldnt be achieved by not setting the config for capture dai. We should instead check if the dai link has already been configured and skip it in that case.

@plbossart @RanderWang the intention seems to be correct here but the assumption that only the playback dai has the config is probably flawed.

During resume, we should avoid sending the dai config ipc twice for I2S. But this shouldnt be achieved by not setting the config for capture dai. We should instead check if the dai link has already been configured and skip it in that case.

yes, I also prefer

plbossart · 2018-12-19T17:18:34Z

I don't get this part. If a link has both .dpcm_playback and .dpcm_capture set, what happens here?

plbossart · 2018-12-19T17:20:57Z

what prevents you from pointing to the same config in the two parts? I don't get why we are making an assumption that if playback is present, then it's used as a source of information.

I have a conceptual problem with all this. From a hardware perspective, I see no reason why HDaudio should be different from SSP. We could if we wanted have both playback and capture supported with the same DAI name.

So I wonder if the code differences were added because of naming conventions used in skl_generic_hda where the playback and capture DAI links were separated, in a somewhat arbitrary manner.

In short, are we dealing with a problem liking to the hardware itself, or a software problem coming from the machine driver?

It is because of FW design. In FW, there are playback dais and capture dais for HDA and the number of playback dai is different with capture dai (HW caps: 9 playback, 7 capture on APL+). For SSP, playback and capture could be supported in one dai.

Yes, I also want to refine this in FW but now I don't have an idea how to redesign HDA dai in FW. And we also have to discuss with Marcin. Yesterday, I also spent long time to check this.
As you know, now our task is to do upstream. I don't know it is good for us to send patch after finishing upstream.

Yes, I also think about machine driver. Now playback and capture are set in one dai link. I check the legacy hda and find that it is correct that playback and capture are set in one dai link. I discussed with keyon and got a conclusion that FW should reorganize the dai of HDA, not separating playback and capture to different dai.

@RanderWang I'd like a solution where "it does not matter" what firmware and machine driver have used. we need to be able to work with all 4 combinations. If changing the assumptions breaks the implementation then it's not a good enough one.

plbossart · 2018-12-19T17:21:23Z

copy/paste, makes no sense for dmic, it's capture only.

sorry, I didn't realize that DMIC doesn't support capture.

plbossart · 2018-12-19T17:23:13Z

As mentioned above, you could do all this in the sof_set_dai_config, and you store the config in all components supported (playback, capture, playback and capture).

ranj063 · 2018-12-19T18:03:06Z

@plbossart @RanderWang the intention seems to be correct here but the assumption that only the playback dai has the config is probably flawed.

During resume, we should avoid sending the dai config ipc twice for I2S. But this shouldnt be achieved by not setting the config for capture dai. We should instead check if the dai link has already been configured and skip it in that case.

plbossart · 2018-12-19T18:43:18Z

@plbossart <https://github.com/plbossart> @RanderWang <https://github.com/RanderWang> the intention seems to be correct here but instead of assuming that only the playback dai has the config is probably flawed. During resume, we should avoid sending the dai config ipc twice for I2S. But this shouldnt be achieved by not setting the config for capture dai. We should instead check if the dai link has already been configured and skip it in that case

Agree 100%

RanderWang · 2018-12-20T02:58:06Z

@plbossart @RanderWang the intention seems to be correct here but the assumption that only the playback dai has the config is probably flawed.

During resume, we should avoid sending the dai config ipc twice for I2S. But this shouldnt be achieved by not setting the config for capture dai. We should instead check if the dai link has already been configured and skip it in that case.

@ranj063 your idea is good, but it is not easy to implement it. My idea is: sof_ipc_dai_config (dai link config) should not be stored in snd_sof_dai component , it should be stored in sof_ipc_dai_link( new structure). I want to introduce sof_ipc_dai_link to sof to save dai link config. what's your idea?

plbossart · 2018-12-20T04:11:01Z

@plbossart @RanderWang the intention seems to be correct here but the assumption that only the playback dai has the config is probably flawed.
During resume, we should avoid sending the dai config ipc twice for I2S. But this shouldnt be achieved by not setting the config for capture dai. We should instead check if the dai link has already been configured and skip it in that case.

@ranj063 your idea is good, but it is not easy to implement it. My idea is: sof_ipc_dai_config (dai link config) should not be stored in snd_sof_dai component , it should be stored in sof_ipc_dai_link( new structure). I want to introduce sof_ipc_dai_link to sof to save dai link config. what's your idea?

Just to be clear, you cannot add new structures and change the IPC at this point. No ABI changes unless there is a strong motivation to change the protocol (and implement a backwards-compatible solution in the kernel)

RanderWang · 2018-12-20T04:28:21Z

@plbossart @RanderWang the intention seems to be correct here but the assumption that only the playback dai has the config is probably flawed.
During resume, we should avoid sending the dai config ipc twice for I2S. But this shouldnt be achieved by not setting the config for capture dai. We should instead check if the dai link has already been configured and skip it in that case.

@ranj063 your idea is good, but it is not easy to implement it. My idea is: sof_ipc_dai_config (dai link config) should not be stored in snd_sof_dai component , it should be stored in sof_ipc_dai_link( new structure). I want to introduce sof_ipc_dai_link to sof to save dai link config. what's your idea?

Just to be clear, you cannot add new structures and change the IPC at this point. No ABI changes unless there is a strong motivation to change the protocol (and implement a backwards-compatible solution in the kernel)

@plbossart dai_link_list maybe better than sof_ipc_dai_link. It records dai_link like dai_list recording dai component. It is not used by IPC and it would be added in sof_dev like dai_link

RanderWang · 2018-12-20T04:35:28Z

@plbossart can I add new structure in sof_dev ?

Now I want to unify the HDA dai , SSP dai, and DMIC. Only one dai config is sent to FW for HDA dai.

The change is not aggressive in kernel instead in FW, but I may change
struct sof_ipc_dai_hda_params {
struct sof_ipc_hdr hdr;
uint32_t link_dma_ch;
} __packed;

to
struct sof_ipc_dai_hda_params {
struct sof_ipc_hdr hdr;
uint16_t link_dma_ch[2];
} __packed;

Or in kernel code put dma channel of playback in low 16bit in link_dma_ch, dam channel of capture in high 16bit. Which one do you like ? Or It will be done in next release.

Thanks!

RanderWang · 2018-12-20T09:33:32Z

update PR. Thanks!

I want to change
struct sof_ipc_dai_hda_params {
struct sof_ipc_hdr hdr;
uint32_t link_dma_ch;
} __packed;

to
struct sof_ipc_dai_hda_params {
struct sof_ipc_hdr hdr;
uint16_t link_dma_ch[2];
} __packed;

but it is not fit for us at this time.

plbossart

Sorry, this PR is just massively complicated. This is supposed to be a 'refinement' and what it really is a fundamental rework of all the HDaudio handling
I also disagree with the statements in the commit message. ASoC does not prescribe that you have to use the same DAI and DAI link for playback and capture. In fact, it's not recommended when the capture and playback can operate independently with different clocks and settings. Intel only supports synchronous operations for the SSP, that's not a general rule.
So no sorry, too many assumptions, too many versions to review, I will not touch this before the holiday break. Please simplify and rework all this.

ranj063 · 2018-12-20T19:20:28Z

@RanderWang I agree with @plbossart this is too complicated for the problem we are trying to address.
We have 2 issues to deal with here. So lets break it into 2 parts:

Part 1: For SSP dai links, we should send only one ipc since we really only have 1 dai in the FW
Part 2: For HDA, we need to set the correct dai_index for the dai based on direction so we can resume normally.

For Part 1:
We cannot store the config for only the dai's for Playback and ignore the capture dai's and hope that everything works magically. Instead, we can use some kind of logic in the restore dai links code in pm.c to avoid sending the duplicate ipc for ssp.

For Part 2:
This applies only to HDA
The root cause is the fact that the config only has one dai_index stored for playback and capture when in fact these are twp separate dai's with different indices.
This one's a bit tricky to address but there are 2 ways to go about it:

we can ignore the config dai_index and just look up the dai_index just like we do during topology loading
The other thing which is probably the more elegant solution is to handle the sof_set_dai_config() properly. ie we only set the config for the dai which matches the link name and direction.

Let me know what you think about it.

RanderWang · 2018-12-21T03:03:00Z

@ranj063 for your Part1, I have done it in pm.c in which each dai link is only sent one time
for Part2 I prefer to solution 2.

sorry, it is too aggressive to make such big change, I will follow it later.

@plbossart "ASoC does not prescribe that you have to use the same DAI and DAI link for playback and capture" -- yes, this comment is absolutely right. For capture or playback, they could set in different dai link, and this is all dependent on machine driver. Now capture and playback are in the same dai link of analog in skl-generic-hda.

And I check the legacy hda driver, find that capture and playback are also in the same analog dai link. we can check a production PC with aplay -l. So it is correct for our machine driver to do this.

I also check and think about hda dai in FW. we only need to modify a few lines of code to support this. I think the most important change is to add enough comments.

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. Signed-off-by: Rander Wang <rander.wang@linux.intel.com>

Dai link is restored by each dai component. This is wrong because there maybe two dai components, playback and capture, or one (playback or capture) in one dai link, so a dai link may be restored two times, but only one dai link is needed. Now restore dai link from index 0 to max index, each dai link is restored only one time Signed-off-by: Rander Wang <rander.wang@linux.intel.com> Signed-off-by: Zhu Yingjiang <yingjiang.zhu@linux.intel.com> Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

plbossart · 2018-12-21T15:47:15Z

I also check and think about hda dai in FW. we only need to modify a few lines of code to support this. I think the most important change is to add enough comments.

Again the topology should be able to abstract differences in the modeling of the DAIs, i.e. it should "just work" whether the DAI supports playback and capture, or just capture or just playback. It'd be a mistake to only support DAIs which do playback and capture only. One case is for example SoundWire, where the interface can be configured at will. Please do not take the path of least resistance and a limited solution (dare I say hack?), otherwise this problem will come back at you in a few months

RanderWang · 2018-12-24T06:02:53Z

I also check and think about hda dai in FW. we only need to modify a few lines of code to support this. I think the most important change is to add enough comments.

Again the topology should be able to abstract differences in the modeling of the DAIs, i.e. it should "just work" whether the DAI supports playback and capture, or just capture or just playback. It'd be a mistake to only support DAIs which do playback and capture only. One case is for example SoundWire, where the interface can be configured at will. Please do not take the path of least resistance and a limited solution (dare I say hack?), otherwise this problem will come back at you in a few months

@plbossart Our our topology support all cases you mentioned. And my advise is more elegant than before. As you know, I can't make a good solution for the first time I work on HDA. let's talk the ugly status now.

First analog dai link is defined in machine driver
{
.name = "Analog Playback and Capture",
.id = 4,
...
.dpcm_playback = 1,
.dpcm_capture = 1,
...
},

So in topology, our dai should be defined with the same dai link index 0.
DAI_ADD(sof/pipe-dai-playback.m4, 1, HDA, 0, Analog Playback and Capture, ...)
DAI_ADD(sof/pipe-dai-capture.m4, 2, HDA, 0, Analog Playback and Capture, ...)

But now to be consistent with HDA dai by which I first considered only playback or capture is suppored, I changed the toplology to
DAI_ADD(sof/pipe-dai-playback.m4, 1, HDA, 0, Analog Playback and Capture, ...)
DAI_ADD(sof/pipe-dai-capture.m4, 2, HDA, 1, Analog Playback and Capture, ...)
And also in kernel, dai config is sent to FW two times for playback and capture.

This is a hack for FW to support hda dai. What I want to change is to make FW support playback and capture in the same hda dai and we never need to hack topology file and kernel driver to work with hda dai. Actually, we don't need to do much work to make this change, only a few lines of code and a comment that make clear the usage of hda dai.

ranj063 · 2018-12-24T07:57:19Z

I also check and think about hda dai in FW. we only need to modify a few lines of code to support this. I think the most important change is to add enough comments.

Again the topology should be able to abstract differences in the modeling of the DAIs, i.e. it should "just work" whether the DAI supports playback and capture, or just capture or just playback. It'd be a mistake to only support DAIs which do playback and capture only. One case is for example SoundWire, where the interface can be configured at will. Please do not take the path of least resistance and a limited solution (dare I say hack?), otherwise this problem will come back at you in a few months

@plbossart Our our topology support all cases you mentioned. And my advise is more elegant than before. As you know, I can't make a good solution for the first time I work on HDA. let's talk the ugly status now.

First analog dai link is defined in machine driver
{
.name = "Analog Playback and Capture",
.id = 4,
...
.dpcm_playback = 1,
.dpcm_capture = 1,
...
},

So in topology, our dai should be define with the same dai link index 0.
DAI_ADD(sof/pipe-dai-playback.m4, 1, HDA, 0, Analog Playback and Capture, ...)
DAI_ADD(sof/pipe-dai-capture.m4, 2, HDA, 0, Analog Playback and Capture, ...)

But now to be consistent with HDA dai by which I first considered only playback or capture is suppored, I changed the toplology to
DAI_ADD(sof/pipe-dai-playback.m4, 1, HDA, 0, Analog Playback and Capture, ...)
DAI_ADD(sof/pipe-dai-capture.m4, 2, HDA, 1, Analog Playback and Capture, ...)
And also in kernel, dai config is sent to FW two times for playback and capture.

This is a hack for FW to support hda dai. What I want to change is to make FW support playback and capture in the same hda dai and we never need to hack topology file and kernel driver to work with hda dai. Actually, we don't need to do much work to make this change, only a few lines of code and a comment that make clear the usage of hda dai.

@RanderWang that seems to be aligned with what @plbossart and I had discussed a few weeks ago when the HDA changes were made. Do you mind submitting the PR so we can see how this is done?

plbossart · 2018-12-26T18:25:19Z

Sorry, I can't figure out where this PR is going. Like I said multiple times, the driver should be able to work even when the capture and playback DAIs are separate. Fixing the firmware is just kicking the can down the road.

RanderWang · 2019-01-02T06:35:15Z

Sorry, I can't figure out where this PR is going. Like I said multiple times, the driver should be able to work even when the capture and playback DAIs are separate. Fixing the firmware is just kicking the can down the road.

Hi Pierre, I make clear about what I said a week ago.
(1) this PR just fix a PM bug and It doesn't change algorithm of HDA dai
(2) And the comments I gave for HDA dai is the plan I want to do.

I will focus on this PM bug and don't make a great change with HDA dai.

RanderWang · 2019-01-07T07:18:17Z

close it for it is hard to follow this PR after long time

RanderWang requested review from plbossart, ranj063 and zhuyingjiang December 19, 2018 06:54

RanderWang force-pushed the dai_link_restore branch 2 times, most recently from 9852aa7 to cc03dc4 Compare December 19, 2018 07:07

plbossart requested changes Dec 19, 2018

View reviewed changes

plbossart mentioned this pull request Dec 19, 2018

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

Closed

zhuyingjiang mentioned this pull request Dec 20, 2018

PM: the dai config message sent by hda_load and sof_restore_pipelines are not the same #442

Closed

RanderWang force-pushed the dai_link_restore branch 2 times, most recently from 3797a99 to ea30260 Compare December 20, 2018 09:00

RanderWang force-pushed the dai_link_restore branch 2 times, most recently from fb1d314 to fe5d1d4 Compare December 20, 2018 09:47

plbossart requested changes Dec 20, 2018

View reviewed changes

RanderWang force-pushed the dai_link_restore branch 2 times, most recently from 5915ba7 to 356a0ea Compare December 21, 2018 06:49

RanderWang added 2 commits December 21, 2018 14:49

ASoC: SOF: refine the algorithm of dai config setting

6d06701

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. Signed-off-by: Rander Wang <rander.wang@linux.intel.com>

RanderWang mentioned this pull request Jan 3, 2019

dma channel management fixes thesofproject/sof#786

Merged

mengdonglin added enhancement New feature or request upstream blocker Mandatory for upstream labels Jan 7, 2019

mengdonglin assigned RanderWang Jan 7, 2019

RanderWang closed this Jan 7, 2019

Conversation

RanderWang commented Dec 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RanderWang commented Dec 19, 2018

Uh oh!

RanderWang commented Dec 19, 2018

Uh oh!

plbossart commented Dec 19, 2018

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ranj063 commented Dec 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

plbossart commented Dec 19, 2018 via email

Uh oh!

RanderWang commented Dec 20, 2018

Uh oh!

plbossart commented Dec 20, 2018

Uh oh!

RanderWang commented Dec 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RanderWang commented Dec 20, 2018

Uh oh!

RanderWang commented Dec 20, 2018

Uh oh!

plbossart left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ranj063 commented Dec 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RanderWang commented Dec 21, 2018

Uh oh!

plbossart commented Dec 21, 2018

Uh oh!

RanderWang commented Dec 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ranj063 commented Dec 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

plbossart commented Dec 26, 2018

Uh oh!

RanderWang commented Jan 2, 2019

Uh oh!

RanderWang commented Jan 7, 2019

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

RanderWang commented Dec 19, 2018 •

edited

Loading

ranj063 commented Dec 19, 2018 •

edited

Loading

RanderWang commented Dec 20, 2018 •

edited

Loading

plbossart left a comment •

edited

Loading

ranj063 commented Dec 20, 2018 •

edited

Loading

RanderWang commented Dec 24, 2018 •

edited

Loading

ranj063 commented Dec 24, 2018 •

edited

Loading