Fixes for HDA-DMA driver #4820

ranj063 · 2021-09-28T19:30:45Z

Part 1 of HDA DMA fixes. Fixes the handling of FIFORDY bits and pause/release cleanups for host/link DMA

EDIT: part 2 is #4844

plbossart

Not fully clear on some simplifications - but the initial code is already confusing...

src/drivers/intel/hda/hda-dma.c

plbossart · 2021-09-28T19:36:01Z

src/drivers/intel/hda/hda-dma.c

@@ -407,8 +407,7 @@ static int hda_dma_enable_unlock(struct dma_chan_data *channel)
 	}

 	/* start link output transfer now */
-	if (channel->direction == DMA_DIR_MEM_TO_DEV &&
-	    !(hda_chan->state & HDA_STATE_RELEASE))
+	if (channel->direction == DMA_DIR_MEM_TO_DEV)


I wasn't able to follow that simplification.

the only thing release ever did was to start host DMA but we never pause/release the host DMA in the first place, so the preovious patch removed that. With that, release literally does nothing now

but we still set the HDA_STATE_RELEASE bit? I am officially confused!

that bit is used to do 2 things: 1. releae host DMA which will never happen because we do no release host DMA. 2. prevent incrementing the firmware pointer for release which actually results in an xrun. But it isnt a register BIT just a state bit we used for the above 2 things. So its no longer needed

so can we remove line 414 as well and even the definition of HDA_STATE_RELEASE?

but I have removed the defitnion of HDA_STATE_RELEASE, no? not sure what 414 is, probably clearing the state and I' have removed that as well

TBH, I would lump all the HDA DMA driver RELEASE removal into one patch.

plbossart · 2021-09-28T19:38:21Z

src/drivers/intel/hda/hda-dma.c

-	if (channel->direction == DMA_DIR_HMEM_TO_LMEM ||
-	    channel->direction == DMA_DIR_LMEM_TO_HMEM)
-		ret = hda_dma_host_start(channel);
-


Same, not able to follow this simplification - not sure why we did a host_start in a dma_release in the first place.

yeah, this change looks unrelated to RELEASE removal ? Line 570 also sets the RELEASE state, should we remove RELEASE completely ?

plbossart · 2021-09-28T19:40:09Z

src/drivers/intel/hda/hda-dma.c

 		tr_err(&hdma_tr, "hda_dma_link_check_xrun(): overrun detected");
 		dma_chan_reg_update_bits(chan, DGCS, DGCS_BOR, DGCS_BOR);
+		return -ENODATA;


doesn't this need additional changes in the caller as mentioned by @lyakh ?

I think his comment was for my host changes and I fixed that in my other PR. This one is correct isnt it?

I can't recall precisely the issue... Just wanted to check that this change is legit.

fredoh9 · 2021-09-28T22:07:24Z

SOFCI TEST

lgirdwood

Quite a lot of red on CI pause tests, this PR could be split to upstream the low risk updates first.

lgirdwood · 2021-09-29T10:06:17Z

src/drivers/intel/hda/hda-dma.c

-	if (channel->direction == DMA_DIR_HMEM_TO_LMEM ||
-	    channel->direction == DMA_DIR_LMEM_TO_HMEM)
-		ret = hda_dma_host_start(channel);
-


yeah, this change looks unrelated to RELEASE removal ? Line 570 also sets the RELEASE state, should we remove RELEASE completely ?

lgirdwood · 2021-09-29T10:07:44Z

src/drivers/intel/hda/hda-dma.c

@@ -407,8 +407,7 @@ static int hda_dma_enable_unlock(struct dma_chan_data *channel)
 	}

 	/* start link output transfer now */
-	if (channel->direction == DMA_DIR_MEM_TO_DEV &&
-	    !(hda_chan->state & HDA_STATE_RELEASE))
+	if (channel->direction == DMA_DIR_MEM_TO_DEV)


TBH, I would lump all the HDA DMA driver RELEASE removal into one patch.

lyakh · 2021-09-29T09:56:52Z

src/drivers/intel/hda/hda-dma.c

-	    (!config->cyclic && config->direction == DMA_DIR_LMEM_TO_HMEM))
+	/* set DGCS.FIFORDY for host output dma */
+	if ((config->cyclic && config->direction == DMA_DIR_HMEM_TO_LMEM) ||
+	    (config->cyclic && config->direction == DMA_DIR_LMEM_TO_HMEM))


assuming the removal of the negation is indeed intentional and a fix, apart from making it a separate patch as @plbossart is suggesting, please also then simplify the test as

if (config->cyclic && (config->direction == DMA_DIR_HMEM_TO_LMEM || config->direction == DMA_DIR_LMEM_TO_HMEM))

lyakh · 2021-09-29T10:09:37Z

src/drivers/intel/hda/hda-dma.c

@@ -407,8 +407,7 @@ static int hda_dma_enable_unlock(struct dma_chan_data *channel)
 	}

 	/* start link output transfer now */
-	if (channel->direction == DMA_DIR_MEM_TO_DEV &&
-	    !(hda_chan->state & HDA_STATE_RELEASE))
+	if (channel->direction == DMA_DIR_MEM_TO_DEV)


I'm probably missing something, I thought DMA_DIR_MEM_TO_DEV was only for link DMA playback?

ranj063 · 2021-09-29T15:23:44Z

@plbossart @lgirdwood I've dropped a couple of patches in this series. 1. to drop the release for link DMA. Even though this helps with the xrun, it doesnt completely avoid it. 2. return the right error in case of xrun. This is still TBD. We'll have to wait until I have a proper working solution for this.

src/drivers/intel/hda/hda-dma.c

lgirdwood · 2021-09-29T19:00:22Z

@wszypelt @kkarask looks like CI has stalled - can you unblock it ? Thanks !

marc-hb · 2021-09-29T19:45:32Z

https://sof-ci.01.org/sof-pr-viewer/#/build/PR4820/build7404529 is green but incomplete.

marc-hb · 2021-09-29T19:47:11Z

https://sof-ci.01.org/sofpr/PR4820/build10517/devicetest/ is green but missing all firmware logs because of some internal issue. Restarting it.

EDIT: still no firmware logs in https://sof-ci.01.org/sofpr/PR4820/build10520/devicetest/. Issue reported internally.

EDIT2 https://sof-ci.01.org/sofpr/PR4820/build10520/devicetest/?model=CML_RVP_SDW&testcase=check-playback-10sec has firmware logs but https://sof-ci.01.org/sofpr/PR4820/build10520/devicetest/?model=CML_RVP_SDW&testcase=check-sof-logger has none.

marc-hb · 2021-09-29T19:47:24Z

SOFCI TEST

keyonjie · 2021-09-30T02:04:50Z

src/drivers/intel/hda/hda-dma.c

-	/* set DGCS.FIFORDY for output dma */
-	if (config->direction == DMA_DIR_MEM_TO_DEV || config->direction == DMA_DIR_LMEM_TO_HMEM)
+	/* set DGCS.FIFORDY for host DMA for both playback and capture */
+	if (config->direction == DMA_DIR_HMEM_TO_LMEM || config->direction == DMA_DIR_LMEM_TO_HMEM)
 		dgcs |= DGCS_FIFORDY;

 	dma_chan_reg_write(channel, DGCS, dgcs);


Since we set/clear DGCS_FIFORDY explicitly in start/stop, do we need to preset it in _set_config() here?

this seems to directly contradict the stated purpose of f96878a

@lyakh I'm not sure what that specific commit achieved anyway. Before my change, we end up setting the FIFORDY bit for all DMAs unconditionally during START.

@ranj063 not sure GH made it clear enough what my comment was referring to. I meant the removal of cyclic support as per your commit comment "config->cyclic is not relevant for HDA DMAs." I'm not sure whether your comment is correct, but I just commented, that it contradicted the original commit, that introduced cyclic DMA support for HDA. Don't know whether that commit was right though.

Maybe best to state in the comments the current feedback on setting DGCS.FIFORDY from KarL. This way everyone can understand.

@lgirdwood I have updated the commit message and added comments in the next patch for FIFORDY bit updates.

I think I was confused by the commit description phrase "config->cyclic is not relevant for HDA DMAs." It probably means "this one specific use of config->cyclic is irrelevant." If it really is trying to say, that the whole config->cyclic support in HDA DMA is irrelevant, then probably the whole commit f96878a should be reverted.

keyonjie · 2021-09-30T02:34:02Z

src/drivers/intel/hda/hda-dma.c

-	if (!(hda_chan->state & HDA_STATE_RELEASE))
-		pm_runtime_put(PM_RUNTIME_HOST_DMA_L1, 0);
+	/* Force Host DMA to exit L1 on start */
+	pm_runtime_put(PM_RUNTIME_HOST_DMA_L1, 0);


I think the original logic is that we don't need to force L1 exit for the pause/release iteration, while we need it for a _start, as there will be preloading for a hda_dma_start() while there isn't for the _release().

We do have dma_pause() and dma_release() calling in dai.c, even though there might not be trigger pause/release IPCs from the Linux host side, so I would suggest to keep this as it is in case it doesn't bring any issues like Xruns to us.

@keyonjie DAI uses link DMA and we have the ops for link DMA here. There is no need for host DMA pause/release when it is not used

keyonjie · 2021-09-30T02:36:22Z

src/drivers/intel/hda/hda-dma.c

@@ -979,8 +979,6 @@ const struct dma_ops hda_host_dma_ops = {
 	.start			= hda_dma_start,
 	.stop			= hda_dma_stop,
 	.copy			= hda_dma_host_copy,
-	.pause			= hda_dma_pause,
-	.release		= hda_dma_release,


"hda_dma_pause/release declared but not used"?

sorry dont get it? it is used in link DMA ops

sorry dont get it? it is used in link DMA ops

Oh, makes sense then, maybe better to rename them to hda_dma_link_pause/release().

lgirdwood · 2021-09-30T15:08:06Z

@ranj063 CI looks good around the pause states :) are all the opens resolved now ?

ranj063 · 2021-09-30T15:50:25Z

@ranj063 CI looks good around the pause states :) are all the opens resolved now ?

@lgirdwood I dont have any opens. As for @lyakh's comment, the check for FIFORDY bit in set_cnofig is inconsequential anyway, we were setting it unconditionally during START anyway. And I confirmed that we should set it for host DMA for playback and capture

marc-hb · 2021-09-30T17:24:36Z

SOFCI TEST

EDIT: re-running because the last run suffered from a CI issue, FW logs were missing, for instance https://sof-ci.01.org/sofpr/PR4820/build10520/devicetest/?model=TGLU_RVP_SDW&testcase=check-sof-logger has no FW logs.

config->cyclic is not relevant for HDA DMAs. So remove the check for it. We set config->cyclic to 1 for both playback and capture for all DAI's anyway. So the check today would never set FIFORDY for host output DMA during set_config. But nevertheless, during the start trigger, FIFORDY is set for all DMA's unconditionally. That is also incorrect and will be fixed in the following commit. For now, just remove the check for cyclic to prepare for FIFORDY bit cleanup. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

So, leave this bit untouched for link DMA and only set/clear it for the host DMA for both playback and capture. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Host DMA is never paused or released. Remove the code to start host DMA during release and the check for release in hda_dma_host_start(). Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Not all DMA's need a pause/release. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Host DMA is never paused/released. So, remove the pause and release ops from the host DMA ops. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

plbossart

I only have one comment, but that could be handled in a follow-up PR

plbossart · 2021-10-04T21:59:37Z

src/drivers/intel/hda/hda-dma.c

-	/* set DGCS.FIFORDY for output dma */
-	if (config->direction == DMA_DIR_MEM_TO_DEV || config->direction == DMA_DIR_LMEM_TO_HMEM)
+	/* set DGCS.FIFORDY for input/output host DMA only. It is not relevant for link DMA's */
+	if (config->direction == DMA_DIR_HMEM_TO_LMEM || config->direction == DMA_DIR_LMEM_TO_HMEM)


I would agree with @keyonjie that it's not clear why we need to keep setting this FIFORDY bit in hda_dma_set_config(), when it's already set in enable_dma() and cleared in stop_dma()

ranj063 requested review from dbaluta, lbetlej, lgirdwood, mmaka1 and plbossart as code owners September 28, 2021 19:30

plbossart reviewed Sep 28, 2021

View reviewed changes

marc-hb mentioned this pull request Sep 29, 2021

Re-enable trace filtering and implement DMA reset op #4793

Closed

lgirdwood reviewed Sep 29, 2021

View reviewed changes

lyakh requested changes Sep 29, 2021

View reviewed changes

ranj063 force-pushed the fix/hda_dma_v2 branch from 34dc488 to 6a57aa4 Compare September 29, 2021 15:21

ranj063 requested review from plbossart, lgirdwood and lyakh September 29, 2021 15:23

ranj063 changed the title ~~Fixes for HDA-DMA pause/release xrun~~ Fixes for HDA-DMA driver Sep 29, 2021

plbossart reviewed Sep 29, 2021

View reviewed changes

src/drivers/intel/hda/hda-dma.c Show resolved Hide resolved

ranj063 force-pushed the fix/hda_dma_v2 branch from 6a57aa4 to cea55e1 Compare September 29, 2021 15:39

keyonjie reviewed Sep 30, 2021

View reviewed changes

ranj063 added 4 commits October 4, 2021 12:36

drivers: Intel: hda-dma: FIFORDY bit is only valid for host DMA

a1158c8

So, leave this bit untouched for link DMA and only set/clear it for the host DMA for both playback and capture. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

drivers: Intel: hda-dma: remove unnecessary code in start/release

1973a66

Host DMA is never paused or released. Remove the code to start host DMA during release and the check for release in hda_dma_host_start(). Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

lib: dma: make pause/release ops optional

10e25f7

Not all DMA's need a pause/release. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

drivers: Intel: hda-dma: remove pause/release ops from hda_host_dma_ops

0e092f2

Host DMA is never paused/released. So, remove the pause and release ops from the host DMA ops. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

ranj063 force-pushed the fix/hda_dma_v2 branch from cea55e1 to 0e092f2 Compare October 4, 2021 19:48

ranj063 requested a review from plbossart October 4, 2021 19:48

plbossart approved these changes Oct 4, 2021

View reviewed changes

ranj063 mentioned this pull request Oct 5, 2021

Fix HDA DMA stop sequence #4844

Merged

lgirdwood approved these changes Oct 5, 2021

View reviewed changes

lgirdwood merged commit 379a455 into thesofproject:main Oct 5, 2021

ranj063 deleted the fix/hda_dma_v2 branch October 21, 2021 18:43

marc-hb mentioned this pull request Nov 2, 2021

[BUG] DMA trigger stop failed when suspend-resume during capturing on CML-SKU0983-SDW #4779

Closed

Fixes for HDA-DMA driver #4820

Fixes for HDA-DMA driver #4820

Conversation

ranj063 commented Sep 28, 2021 • edited by marc-hb Loading

plbossart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredoh9 commented Sep 28, 2021

lgirdwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ranj063 commented Sep 29, 2021

lgirdwood commented Sep 29, 2021

marc-hb commented Sep 29, 2021

marc-hb commented Sep 29, 2021 • edited Loading

marc-hb commented Sep 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ranj063 Sep 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgirdwood commented Sep 30, 2021

ranj063 commented Sep 30, 2021

marc-hb commented Sep 30, 2021 • edited Loading

plbossart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ranj063 commented Sep 28, 2021 •

edited by marc-hb

Loading

marc-hb commented Sep 29, 2021 •

edited

Loading

ranj063 Sep 30, 2021 •

edited

Loading

marc-hb commented Sep 30, 2021 •

edited

Loading