Skip to content

ASoC: SOF: ipc4/Intel: Improve pcm delay reporting #4851

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
ujfalusi merged 16 commits into from
Mar 20, 2024
Merged

ASoC: SOF: ipc4/Intel: Improve pcm delay reporting #4851

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
cd2c934
ASoC: SOF: Add dsp_max_burst_size_in_ms member to snd_sof_pcm_stream
ujfalusi Mar 8, 2024
ac31adc
ASoC: SOF: ipc4-topology: Save the DMA maximum burst size for PCMs
ujfalusi Mar 8, 2024
61bab4d
ASoC: SOF: Intel: hda-pcm: Use dsp_max_burst_size_in_ms to place cons…
ujfalusi Mar 8, 2024
e993c4e
fixup! ASoC: SOF: Intel: hda: Implement get_stream_position (Linear L…
ujfalusi Feb 1, 2024
9aaf4e6
ASoC: SOF: Introduce a new callback pair to be used for PCM delay rep…
ujfalusi Feb 1, 2024
7827066
ASoC: SOF: Intel: Set the dai/host get frame/byte counter callbacks
ujfalusi Feb 1, 2024
69b5bc2
ASoC: SOF: ipc4-pcm: Use the snd_sof_pcm_get_dai_frame_counter() for …
ujfalusi Feb 1, 2024
f3ca746
ASoC: SOF: Intel: hda-common-ops: Do not set the get_stream_position …
ujfalusi Feb 1, 2024
a9b30e6
ASoC: SOF: Intel: Remove the get_stream_position callback
ujfalusi Feb 1, 2024
a757772
ASoC: SOF: ipc4-pcm: Move struct sof_ipc4_timestamp_info definition l…
ujfalusi Feb 2, 2024
be10808
ASoC: SOF: ipc4-pcm: Combine the SOF_IPC4_PIPE_PAUSED cases in pcm_tr…
ujfalusi Mar 7, 2024
42c2f6c
ASoC: SOF: ipc4-pcm: Invalidate the stream_start_offset in PAUSED state
ujfalusi Mar 7, 2024
f30d6e6
ASoC: SOF: sof-pcm: Add pointer callback to sof_ipc_pcm_ops
ujfalusi Mar 13, 2024
4eeae62
ASoC: SOF: ipc4-pcm: Correct the delay calculation
ujfalusi Feb 2, 2024
f44a0d4
ALSA: hda: Add pplcllpl/u members to hdac_ext_stream
ujfalusi Mar 14, 2024
a0fb868
ASoC: SOF: Intel: hda: Compensate LLP in case it is not reset
ujfalusi Mar 14, 2024
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
3 changes: 3 additions & 0 deletions include/sound/hdaudio_ext.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,9 @@ struct hdac_ext_stream {
u32 pphcldpl;
u32 pphcldpu;

u32 pplcllpl;
u32 pplcllpu;

bool decoupled:1;
bool link_locked:1;
bool link_prepared;
Expand Down
3 changes: 2 additions & 1 deletion sound/soc/sof/intel/hda-common-ops.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,8 @@ struct snd_sof_dsp_ops sof_hda_common_ops = {
.pcm_pointer = hda_dsp_pcm_pointer,
.pcm_ack = hda_dsp_pcm_ack,

.get_stream_position = hda_dsp_get_stream_hda_link_position,
.get_dai_frame_counter = hda_dsp_get_stream_llp,
.get_host_byte_counter = hda_dsp_get_stream_ldp,

/* firmware loading */
.load_firmware = snd_sof_load_firmware_raw,
Expand Down
11 changes: 11 additions & 0 deletions sound/soc/sof/intel/hda-dai-ops.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

#include <sound/pcm_params.h>
#include <sound/hdaudio_ext.h>
#include <sound/hda_register.h>
#include <sound/hda-mlink.h>
#include <sound/sof/ipc4/header.h>
#include <uapi/sound/sof/header.h>
Expand Down Expand Up @@ -349,6 +350,16 @@ static int hda_trigger(struct snd_sof_dev *sdev, struct snd_soc_dai *cpu_dai,
case SNDRV_PCM_TRIGGER_STOP:
case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
snd_hdac_ext_stream_clear(hext_stream);

/*
* Save the LLP registers in case the stream is
* restarting due PAUSE_RELEASE, or START without a pcm
* close/open since in this case the LLP register is not reset
* to 0 and the delay calculation will return with invalid
* results.
*/
hext_stream->pplcllpl = readl(hext_stream->pplc_addr + AZX_REG_PPLCLLPL);
hext_stream->pplcllpu = readl(hext_stream->pplc_addr + AZX_REG_PPLCLLPU);
break;
default:
dev_err(sdev->dev, "unknown trigger command %d\n", cmd);
Expand Down
29 changes: 29 additions & 0 deletions sound/soc/sof/intel/hda-pcm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,8 +263,37 @@ int hda_dsp_pcm_open(struct snd_sof_dev *sdev,
snd_pcm_hw_constraint_mask64(substream->runtime, SNDRV_PCM_HW_PARAM_FORMAT,
SNDRV_PCM_FMTBIT_S16 | SNDRV_PCM_FMTBIT_S32);

/*
* The dsp_max_burst_size_in_ms is the length of the maximum burst size
* of the host DMA in the ALSA buffer.
*
* On playback start the DMA will transfer dsp_max_burst_size_in_ms
* amount of data in one initial burst to fill up the host DMA buffer.
* Consequent DMA burst sizes are shorter and their length can vary.
* To make sure that userspace allocate large enough ALSA buffer we need
* to place a constraint on the buffer time.
*
* On capture the DMA will transfer 1ms chunks.
*
* Exact dsp_max_burst_size_in_ms constraint is racy, so set the
* constraint to a minimum of 2x dsp_max_burst_size_in_ms.
*/
if (spcm->stream[direction].dsp_max_burst_size_in_ms)
snd_pcm_hw_constraint_minmax(substream->runtime,
SNDRV_PCM_HW_PARAM_BUFFER_TIME,
spcm->stream[direction].dsp_max_burst_size_in_ms * USEC_PER_MSEC * 2,
UINT_MAX);
Copy link
Member

@plbossart plbossart Mar 15, 2024

Choose a reason for hiding this comment

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

By default

sps->dsp_buffer_time_ms = SOF_IPC4_MIN_DMA_BUFFER_SIZE;

so this forces the ALSA buffer to be a minimum of 4ms.

Also this is a bit ironic that the race happens for large buffers, IIRC the issue was an xrun with small periods with mplayer? If this is only for large buffers, could we enforce this constraint when

spcm->stream[substream->stream].dsp_buffer_time_ms > SOF_IPC4_MIN_DMA_BUFFER_SIZE;

Bear with me, reviewing on Friday afternoon...

Copy link
Collaborator Author

@ujfalusi ujfalusi Mar 18, 2024

Choose a reason for hiding this comment

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

The xrun happens with large buffers because application rarely if ever took small (< 4ms) buffers.

Copy link
Member

@plbossart plbossart Mar 18, 2024

Choose a reason for hiding this comment

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

so in practice could we not make this constraint conditional, e.g.

if ( spcm->stream[substream->stream].dsp_buffer_time_ms > SOF_IPC4_MIN_DMA_BUFFER_SIZE) {
    snd_pcm_hw_constraint_minmax(substream->runtime,
			SNDRV_PCM_HW_PARAM_BUFFER_TIME,
			spcm->stream[substream->stream].dsp_buffer_time_ms * USEC_PER_MSEC * 2,
			UINT_MAX);
}

Copy link
Collaborator Author

@ujfalusi ujfalusi Mar 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

so in practice could we not make this constraint conditional, e.g.

if ( spcm->stream[substream->stream].dsp_buffer_time_ms > SOF_IPC4_MIN_DMA_BUFFER_SIZE) {
    snd_pcm_hw_constraint_minmax(substream->runtime,
			SNDRV_PCM_HW_PARAM_BUFFER_TIME,
			spcm->stream[substream->stream].dsp_buffer_time_ms * USEC_PER_MSEC * 2,
			UINT_MAX);
}

No, the ALSA buffer must be constrained at all times, if the PCM uses the default 2ms buffer then the first DMA burst will copy 2ms data, the buffer should be double of that.

While on capture the ALSA buffer size should be at least 2ms, the DMA copies 1ms chunks from DSP to host, this I left out, but I can add.


/* binding pcm substream to hda stream */
substream->runtime->private_data = &dsp_stream->hstream;

/*
* Reset the llp cache values (they are used for LLP compensation in
* case the counter is not reset)
*/
dsp_stream->pplcllpl = 0;
dsp_stream->pplcllpu = 0;

return 0;
}
EXPORT_SYMBOL_NS(hda_dsp_pcm_open, SND_SOC_SOF_INTEL_HDA_COMMON);
Expand Down
57 changes: 52 additions & 5 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 @@ -1091,9 +1091,19 @@ snd_pcm_uframes_t hda_dsp_stream_get_position(struct hdac_stream *hstream,
}
EXPORT_SYMBOL_NS(hda_dsp_stream_get_position, SND_SOC_SOF_INTEL_HDA_COMMON);

u64 hda_dsp_get_stream_hda_link_position(struct snd_sof_dev *sdev,
struct snd_soc_component *component,
struct snd_pcm_substream *substream)
#define merge_u64(u32_u, u32_l) (((u64)(u32_u) << 32) | (u32_l))

/**
* hda_dsp_get_stream_llp - Retrieve the LLP (Linear Link Position) of the stream
* @sdev: SOF device
* @component: ASoC component
* @substream: PCM substream
*
* Returns the raw Linear Link Position value
*/
u64 hda_dsp_get_stream_llp(struct snd_sof_dev *sdev,
struct snd_soc_component *component,
struct snd_pcm_substream *substream)
{
struct hdac_stream *hstream = substream->runtime->private_data;
struct hdac_ext_stream *hext_stream = stream_to_hdac_ext_stream(hstream);
Expand All @@ -1112,6 +1122,43 @@ u64 hda_dsp_get_stream_hda_link_position(struct snd_sof_dev *sdev,
llp_l = readl(hext_stream->pplc_addr + AZX_REG_PPLCLLPL);
llp_u = readl(hext_stream->pplc_addr + AZX_REG_PPLCLLPU);

return ((u64)llp_u << 32) | llp_l;
/* Compensate the LLP counter with the saved offset */
if (hext_stream->pplcllpl || hext_stream->pplcllpu)
return merge_u64(llp_u, llp_l) -
merge_u64(hext_stream->pplcllpu, hext_stream->pplcllpl);

return merge_u64(llp_u, llp_l);
}
EXPORT_SYMBOL_NS(hda_dsp_get_stream_llp, SND_SOC_SOF_INTEL_HDA_COMMON);

/**
* hda_dsp_get_stream_ldp - Retrieve the LDP (Linear DMA Position) of the stream
* @sdev: SOF device
* @component: ASoC component
* @substream: PCM substream
*
* Returns the raw Linear Link Position value
*/
u64 hda_dsp_get_stream_ldp(struct snd_sof_dev *sdev,
struct snd_soc_component *component,
struct snd_pcm_substream *substream)
{
struct hdac_stream *hstream = substream->runtime->private_data;
struct hdac_ext_stream *hext_stream = stream_to_hdac_ext_stream(hstream);
u32 ldp_l, ldp_u;

/*
* The pphc_addr have been calculated during probe in
* hda_dsp_stream_init():
* pphc_addr = sdev->bar[HDA_DSP_PP_BAR] +
* SOF_HDA_PPHC_BASE +
* SOF_HDA_PPHC_INTERVAL * stream_index
*
* Use this pre-calculated address to avoid repeated re-calculation.
*/
ldp_l = readl(hext_stream->pphc_addr + AZX_REG_PPHCLDPL);
ldp_u = readl(hext_stream->pphc_addr + AZX_REG_PPHCLDPU);

return ((u64)ldp_u << 32) | ldp_l;
}
EXPORT_SYMBOL_NS(hda_dsp_get_stream_hda_link_position, SND_SOC_SOF_INTEL_HDA_COMMON);
EXPORT_SYMBOL_NS(hda_dsp_get_stream_ldp, SND_SOC_SOF_INTEL_HDA_COMMON);
9 changes: 6 additions & 3 deletions sound/soc/sof/intel/hda.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -666,9 +666,12 @@ bool hda_dsp_check_stream_irq(struct snd_sof_dev *sdev);

snd_pcm_uframes_t hda_dsp_stream_get_position(struct hdac_stream *hstream,
int direction, bool can_sleep);
u64 hda_dsp_get_stream_hda_link_position(struct snd_sof_dev *sdev,
struct snd_soc_component *component,
struct snd_pcm_substream *substream);
u64 hda_dsp_get_stream_llp(struct snd_sof_dev *sdev,
struct snd_soc_component *component,
struct snd_pcm_substream *substream);
u64 hda_dsp_get_stream_ldp(struct snd_sof_dev *sdev,
struct snd_soc_component *component,
struct snd_pcm_substream *substream);

struct hdac_ext_stream *
hda_dsp_stream_get(struct snd_sof_dev *sdev, int direction, u32 flags);
Expand Down
Loading