Skip to content

Fixes for HDA-DMA driver #4820

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
lgirdwood merged 5 commits into from
Oct 5, 2021
Merged

Fixes for HDA-DMA driver #4820

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c86271a
drivers: Intel: hda-dma: remove check for config->cyclic
ranj063 Sep 28, 2021
a1158c8
drivers: Intel: hda-dma: FIFORDY bit is only valid for host DMA
ranj063 Sep 28, 2021
1973a66
drivers: Intel: hda-dma: remove unnecessary code in start/release
ranj063 Sep 28, 2021
10e25f7
lib: dma: make pause/release ops optional
ranj063 Sep 28, 2021
0e092f2
drivers: Intel: hda-dma: remove pause/release ops from hda_host_dma_ops
ranj063 Sep 28, 2021
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
38 changes: 15 additions & 23 deletions src/drivers/intel/hda/hda-dma.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,9 +348,8 @@ static int hda_dma_host_start(struct dma_chan_data *channel)
struct hda_chan_data *hda_chan = dma_chan_get_data(channel);
int ret = 0;

/* Force Host DMA to exit L1 only on start*/
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);
Copy link
Contributor

@keyonjie keyonjie Sep 30, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@ranj063 ranj063 Sep 30, 2021

Choose a reason for hiding this comment

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

@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


if (!hda_chan->irq_disabled)
return ret;
Expand Down Expand Up @@ -390,20 +389,21 @@ static int hda_dma_enable_unlock(struct dma_chan_data *channel)

hda_dma_get_dbg_vals(channel, HDA_DBG_PRE, HDA_DBG_BOTH);

/* enable the channel */
dma_chan_reg_update_bits(channel, DGCS, DGCS_GEN | DGCS_FIFORDY,
DGCS_GEN | DGCS_FIFORDY);

/* full buffer is copied at startup */
hda_chan = dma_chan_get_data(channel);
hda_chan->desc_avail = channel->desc_count;

/* enable the channel */
if (channel->direction == DMA_DIR_HMEM_TO_LMEM ||
channel->direction == DMA_DIR_LMEM_TO_HMEM) {
dma_chan_reg_update_bits(channel, DGCS, DGCS_GEN | DGCS_FIFORDY,
DGCS_FIFORDY | DGCS_GEN);
pm_runtime_get(PM_RUNTIME_HOST_DMA_L1, 0);
ret = hda_dma_host_start(channel);
if (ret < 0)
return ret;
} else {
dma_chan_reg_update_bits(channel, DGCS, DGCS_GEN, DGCS_GEN);
}

/* start link output transfer now */
Expand Down Expand Up @@ -565,16 +565,8 @@ static int hda_dma_release(struct dma_chan_data *channel)
tr_dbg(&hdma_tr, "hda-dmac: %d channel %d -> release",
channel->dma->plat_data.id, channel->index);

/*
* Prepare for the handling of release condition on the first work cb.
* This flag will be unset afterwards.
*/
hda_chan->state |= HDA_STATE_RELEASE;

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

Copy link
Member

@plbossart plbossart Sep 28, 2021

Choose a reason for hiding this comment

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

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

Copy link
Member

@lgirdwood lgirdwood Sep 29, 2021

Choose a reason for hiding this comment

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

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

irq_local_enable(flags);
return ret;
}
Expand Down Expand Up @@ -613,12 +605,15 @@ static int hda_dma_stop(struct dma_chan_data *channel)
tr_dbg(&hdma_tr, "hda-dmac: %d channel %d -> stop",
channel->dma->plat_data.id, channel->index);

/* disable the channel */
if (channel->direction == DMA_DIR_HMEM_TO_LMEM ||
channel->direction == DMA_DIR_LMEM_TO_HMEM)
channel->direction == DMA_DIR_LMEM_TO_HMEM) {
hda_dma_host_stop(channel);

/* disable the channel */
dma_chan_reg_update_bits(channel, DGCS, DGCS_GEN | DGCS_FIFORDY, 0);
dma_chan_reg_update_bits(channel, DGCS, DGCS_FIFORDY | DGCS_GEN, 0);
} else {
dma_chan_reg_update_bits(channel, DGCS, DGCS_GEN, 0);
}
channel->status = COMP_STATE_PREPARE;
hda_chan = dma_chan_get_data(channel);
hda_chan->state = 0;
Expand Down Expand Up @@ -756,9 +751,8 @@ static int hda_dma_set_config(struct dma_chan_data *channel,
config->src_width <= 2))
dgcs |= DGCS_SCS;

/* set DGCS.FIFORDY for output dma */
if ((config->cyclic && config->direction == DMA_DIR_MEM_TO_DEV) ||
(!config->cyclic && 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)
Copy link
Member

@plbossart plbossart Oct 4, 2021

Choose a reason for hiding this comment

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

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()

dgcs |= DGCS_FIFORDY;

dma_chan_reg_write(channel, DGCS, dgcs);
Copy link
Contributor

@keyonjie keyonjie Sep 30, 2021

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@lyakh lyakh Sep 30, 2021

Choose a reason for hiding this comment

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

this seems to directly contradict the stated purpose of f96878a

Copy link
Collaborator Author

@ranj063 ranj063 Sep 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@lyakh lyakh Oct 1, 2021

Choose a reason for hiding this comment

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

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

Copy link
Member

@lgirdwood lgirdwood Oct 4, 2021

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@ranj063 ranj063 Oct 4, 2021

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@lyakh lyakh Oct 5, 2021

Choose a reason for hiding this comment

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

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.

Expand Down Expand Up @@ -986,8 +980,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,
Copy link
Contributor

@keyonjie keyonjie Sep 30, 2021

Choose a reason for hiding this comment

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

"hda_dma_pause/release declared but not used"?

Copy link
Collaborator Author

@ranj063 ranj063 Sep 30, 2021

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@keyonjie keyonjie Sep 30, 2021

Choose a reason for hiding this comment

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

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().

.status = hda_dma_status,
.set_config = hda_dma_set_config,
.pm_context_restore = hda_dma_pm_context_restore,
Expand Down
10 changes: 6 additions & 4 deletions src/include/sof/lib/dma.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,16 +335,18 @@ static inline int dma_copy(struct dma_chan_data *channel, int bytes,

static inline int dma_pause(struct dma_chan_data *channel)
{
int ret = channel->dma->ops->pause(channel);
if (channel->dma->ops->pause)
return channel->dma->ops->pause(channel);

return ret;
return 0;
}

static inline int dma_release(struct dma_chan_data *channel)
{
int ret = channel->dma->ops->release(channel);
if (channel->dma->ops->release)
return channel->dma->ops->release(channel);

return ret;
return 0;
}

static inline int dma_status(struct dma_chan_data *channel,
Expand Down