Skip to content

[RFC] ASoC: SOF: add XRUN info delivering to ALSA #479

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

Closed
keyonjie wants to merge 2 commits into from
Closed

[RFC] ASoC: SOF: add XRUN info delivering to ALSA #479

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
83f62fb
ASoC: SOF: add XRUN info delivering to ALSA
keyonjie Dec 21, 2018
8faf094
ASoC: SOF: change kconfig naming to reflect the XRUN handling
keyonjie Dec 24, 2018
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
13 changes: 8 additions & 5 deletions sound/soc/sof/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,12 +99,15 @@ config SND_SOC_SOF_FORCE_NOCODEC_MODE
Say Y if you need this force nocodec mode option
If unsure select "N".

config SND_SOC_SOF_DEBUG_XRUN_STOP
bool "SOF stop on XRUN"
config SND_SOC_SOF_DEBUG_XRUN_REPORT
bool "SOF report FW XRUN to ALSA "
help
This option forces PCMs to stop on any XRUN event. This is useful to
preserve any trace data ond pipeline status prior to the XRUN.
Say Y if you are debugging SOF FW pipeline XRUNs.
This option report Firmware XRUN event to ALSA, and ALSA framework
will trigger PCM stop->prepare->start later on it.
If we don't report this XRUN event, ALSA will still observe XRUN
later when .pointer() callback has no update for some time, usually
that may lead to later and lower frequecy ALSA XRUN response.
Say Y if you are debugging SOF FW pipeline XRUNs handling.

@plbossart plbossart Dec 26, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I also don't get the intent here. Why just not always stop on an underrun? What are we trying to do with the deferred report?

@keyonjie keyonjie Dec 29, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I believe the the snd_pcm_stop_xrun() don't really stop the pipeline thoroughly for us, so the config item CONFIG_SND_SOC_SOF_DEBUG_XRUN_STOP here looks confusing.

What I am doing here is try to remove that and add a new config item for Xrun report.

If unsure select "N".

config SND_SOC_SOF_DEBUG_VERBOSE_IPC
Expand Down
5 changes: 2 additions & 3 deletions sound/soc/sof/ipc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -556,10 +556,9 @@ static void ipc_xrun(struct snd_sof_dev *sdev, u32 msg_id)
dev_dbg(sdev->dev, "posn XRUN: host %llx comp %d size %d\n",
posn.host_posn, posn.xrun_comp_id, posn.xrun_size);

#if defined(CONFIG_SND_SOC_SOF_DEBUG_XRUN_STOP)
/* stop PCM on XRUN - used for pipeline debug */
#if defined(CONFIG_SND_SOC_SOF_DEBUG_XRUN_REPORT)
/* save XRUN info and send to ALSA in .pointer later */
memcpy(&spcm->stream[direction].posn, &posn, sizeof(posn));
snd_pcm_stop_xrun(spcm->stream[direction].substream);

@plbossart plbossart Dec 21, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not sure I get what the meaning of this Kconfig is now, if we don't actually stop? Why wouldn't the Xrun info be passed in ALL cases?

@keyonjie keyonjie Dec 24, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I know your point here, Pierre. I planed to change the Kconfig in the coming PR(e.g. from CONFIG_SND_SOC_SOF_DEBUG_XRUN_STOP to CONFIG_SND_SOC_SOF_DEBUG_XRUN_REPORT).

So my intention here is:

  1. if SOF_DEBUG_XRUN_REPORT is unselected (by default), we don't report this Xrun reported from FW to ALSA, just report the normal position info. If the Xrun is not recovered successfully by FW itself, this Xrun will be accumulated and then ALSA will find actually Xrun and trigger stop->prepare->start at that time.
  2. if SOF_DEBUG_XRUN_REPORT is selected, we will trust Xrun reported from FW(we think/align on that FW report this to us and request driver to do something on that) to ALSA immediately, and ALSA will start triggering stop->prepare->start soon. According to when FW will report XRUN, it is implemented in and controlled by FW side totally.

@plbossart plbossart Dec 26, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry, I don't get any of the explanations. I thought we agreed that the firmware does not try to be smart and does not try to recover on its own?

@keyonjie keyonjie Dec 29, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I think we are not aligned on that yet, the smart recovery inside FW are still running on today's SOF FW.

#endif
}

Expand Down
7 changes: 7 additions & 0 deletions sound/soc/sof/pcm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,13 @@ static snd_pcm_uframes_t sof_pcm_pointer(struct snd_pcm_substream *substream)
if (rtd->dai_link->no_pcm)
return 0;

/* return Xrun to ALSA for further handling */
if (spcm->stream[substream->stream].posn.xrun_size) {
/* only report once for each XRUN IPC */
spcm->stream[substream->stream].posn.xrun_size = 0;
return SNDRV_PCM_POS_XRUN;

@plbossart plbossart Dec 21, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not sure why this is done here, before pcm_pointer callback? Shouldn't this be done once all the information is reported, i.e. below line 350 (with the return removed).

@keyonjie keyonjie Dec 24, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

We have only one return value(position) with .pointer() callback, I aimed to report/return SNDRV_PCM_POS_XRUN once XRUN happened, which will let ALSA trigger stop->prepare->start later.

@plbossart plbossart Dec 26, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

To be clear, I was talking about the second part of the code below

	/* return Xrun to ALSA for further handling */
	if (spcm->stream[substream->stream].posn.xrun_size) {
		/* only report once for each XRUN IPC */
		spcm->stream[substream->stream].posn.xrun_size = 0;
		return SNDRV_PCM_POS_XRUN;
	}

	/* if have dsp ops pointer callback, use that directly */
	if (sdev->ops->pcm_pointer)
		return sdev->ops->pcm_pointer(sdev, substream);

You return before calling pcm_pointer, is this desired/intentional or a bug?

}

/* if have dsp ops pointer callback, use that directly */
if (sdev->ops->pcm_pointer)
return sdev->ops->pcm_pointer(sdev, substream);
Expand Down