[keyonjie]

If we get XRUN IPC from firmware, let's deliver it to ALSA and ALSA
may be able to trigger stop->prepare->start later.

Signed-off-by: Keyon Jie yang.jie@linux.intel.com

[plbossart]

I didn't fully understand the intent of this patch, see comments below.

[plbossart]

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]

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]

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?

[plbossart]

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]

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]

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]

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

[keyonjie]

@plbossart PR updated with Kconfig item naming changed.

[plbossart]

Sorry, this patch is not aligned with my understanding of how the firmware and drivers should behave. I don't see the point of deferring the reaction on an x-run.

[plbossart]

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?

[plbossart]

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?

[plbossart]

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]

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.

[keyonjie]

@plbossart sorry, please hold on merging this, I might need more experiment on it before finalize it, and we need finalize the FW behavior before that also.

[keyonjie]

I have figured out fixes for driver recover FW initiated Xrun issue and verified they work fine.
Most fixes are come from FW side, driver side might need only IPC ABI bump, so closing this.