Skip to content

ASoC: SOF: Intel: Make sure that link and host DMA is coupled after use #5525

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

Open
wants to merge 1 commit into
base: topic/sof-dev
Choose a base branch
from
Open

ASoC: SOF: Intel: Make sure that link and host DMA is coupled after use #5525

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
21 changes: 19 additions & 2 deletions sound/soc/sof/intel/hda-stream.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,9 +288,8 @@ static int _hda_dsp_stream_put(struct snd_sof_dev *sdev, int direction, int stre
{
const struct sof_intel_dsp_desc *chip_info = get_chip_info(sdev->pdata);
struct sof_intel_hda_dev *hda = sdev->pdata->hw_pdata;
struct hdac_ext_stream *hext_stream_match = NULL;
struct hdac_bus *bus = sof_to_bus(sdev);
struct sof_intel_hda_stream *hda_stream;
struct hdac_ext_stream *hext_stream;
struct hdac_ext_stream *link_stream;
struct hdac_stream *s;
bool dmi_l1_enable = true;
Expand All @@ -303,13 +302,17 @@ static int _hda_dsp_stream_put(struct snd_sof_dev *sdev, int direction, int stre
* that are DMI L1 incompatible.
*/
list_for_each_entry(s, &bus->stream_list, list) {
struct sof_intel_hda_stream *hda_stream;
struct hdac_ext_stream *hext_stream;

hext_stream = stream_to_hdac_ext_stream(s);
hda_stream = container_of(hext_stream, struct sof_intel_hda_stream, hext_stream);

if (!s->opened)
continue;

if (s->direction == direction && s->stream_tag == stream_tag) {
hext_stream_match = hext_stream;
s->opened = false;
found = true;
if (pair)
Expand All @@ -319,6 +322,20 @@ static int _hda_dsp_stream_put(struct snd_sof_dev *sdev, int direction, int stre
}
}

if (!sdev->dspless_mode_selected && !pair && hext_stream_match) {
/*
* Couple host and link DMA if link DMA is unused
* In case of channel pair the snd_hdac_ext_stream_release() will
* take care of coupling te HDA channel.
*/
if (!hext_stream_match->link_locked) {
u32 mask = BIT(hext_stream_match->hstream.index);

snd_sof_dsp_update_bits(sdev, HDA_DSP_PP_BAR,
SOF_HDA_REG_PP_PPCTL, mask, 0);
}
}

Copy link
Member

@plbossart plbossart Aug 28, 2025

Choose a reason for hiding this comment

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

what happens if there are two link DMAs for one host DMA, e.g. for a splitter on playback or merge on capture?

Copy link
Collaborator Author

@ujfalusi ujfalusi Aug 28, 2025

Choose a reason for hiding this comment

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

That I'm not sure, I cannot find a topology which has that config, but in that case I would assume that when we are at this point, both link DMAs have been released and we can couple them back.

I also think that in that case we would have reserved two host DMA channels, each using distinct stream_tag, but only use one of them on the host side, so we would couple back them eventually one-by-one?

Copy link
Member

@plbossart plbossart Sep 1, 2025

Choose a reason for hiding this comment

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

@bardliao in the case of amplifier aggregation, don't we have two link DMAs, one per PDI/port/link? IIRC we had to change the DMA handling because it didn't support amplifier aggregation with a single DMA transfer across two links.

Copy link
Collaborator

@ranj063 ranj063 Sep 9, 2025

Choose a reason for hiding this comment

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

@plbossart @ujfalusi I wonder what we missed but we do the coupling back in 2 places both in the host side (during hw_free)and the link side (in snd_hdac_ext_stream_release). If it is the case that during FE hw_free, the link is still locked, we should couple it back when the BE hw_free happens isnt it?

Copy link
Collaborator

@ranj063 ranj063 Sep 9, 2025

Choose a reason for hiding this comment

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

Maybe what we need to do is to make the coupling/decoupling match on both the BE and FE DAI to be done in the hw_params/hw_free instead of open/close for the FE and hw_params/hw_free for the BE DAI

Copy link
Collaborator Author

@ujfalusi ujfalusi Sep 9, 2025

Choose a reason for hiding this comment

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

We also need to handle the code loader DMA and BRA.
Just loading the driver in DSP mode and never doing audio activity, removing, switching to DSPless/Legacy mode will fail. Same with BRA.

I think our callback sequencing got changed over time and this got broken. Not a frequently used feature, but it hurts when it is not working as expected.

Copy link
Collaborator

@ranj063 ranj063 Sep 9, 2025

Choose a reason for hiding this comment

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

yeah but just doing it at the FE side during stream_put will not be enough to handle 1host DMA connected to 2 link DMAs like Pierre said. We need to do this at both ends whenever the last endpoint of the connected is released

Copy link
Collaborator

@ranj063 ranj063 Sep 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@ujfalusi this will definitely not suffice with aggregated amps with SDW.

Copy link
Collaborator Author

@ujfalusi ujfalusi Sep 26, 2025

Choose a reason for hiding this comment

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

Yes, I will revisit as soon as have time for it. Switching from DSP to DSPless/hda-legacy has been broken for a long time now, it is a developer feature and bugs only me ;)

Copy link
Collaborator Author

@ujfalusi ujfalusi Oct 29, 2025

Choose a reason for hiding this comment

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

Checked the code and the channel will be coupled if the link is released later or in case of multiple link DMA use.

We just missing this from the host side.

spin_unlock_irq(&bus->reg_lock);

/* Enable DMI L1 if permitted */
Expand Down
Loading