[alsa-devel] [PATCH 0/4] A few more extensions to LPE audio driver
Hi,
Linus decided to take another week for 4.11 merge window, so I could play a bit more, and here is the result :)
The first patch is the fix for possible GPU hang, and this has been for a while in my local branch and tested. This should be OK to merge soon. (It's also as same as what Ian tested in the last weekend.)
The next one is merely a cleanup patch, so it's fine, too. The third one is a transition to use the runtime PM autosuspend, and it's a preliminary for the last change.
The last one is the new and experimental one: it enables the keep-link feature. With this patch, the device is managed as default via autosuspend, and the link is opened (but silenced) for the pre-given time (2 seconds) after the PCM close. If the application restarts the stream quickly, the receiver is still warm and can resume gracefully.
All these patches are found in topic/intel-lpe-audio branch. (BTW, I cleaned up my branches: now for-next branch tracks the latest stable patches to be merged, while topic/intel-lpe-audio branch tracks the experimental patches. Beware that the latter may be rebased frequently.)
Takashi
===
Takashi Iwai (4): ALSA: x86: Handle reset at prepare callback ALSA: x86: Drop unused stream.running field ALSA: x86: Use runtime PM autosuspend ALSA: x86: Enable keep-link feature
sound/x86/intel_hdmi_audio.c | 110 ++++++++++++++++++++++++++++++++----------- sound/x86/intel_hdmi_audio.h | 2 +- 2 files changed, 84 insertions(+), 28 deletions(-)
Currently the driver handles some reset procedure at the trigger STOP and the underrun functions, where both are executed in the interrupt context. Especially the underrun function has a sync-loop to clear the UNDERRUN status bit, and this is supposed to be one of plausible causes of GPU hangup.
Since the job to be done in the interrupt handler should be minimum, we move the reset function out of trigger and underrun, and push it into the prepare (and hw_free) callbacks instead. Here a new flag, need_reset, is introduced to indicate the requirement of the reset procedure. This is for avoiding the multiple resets when PCM prepare is called sequentially.
Also in the UNDERRUN bit-clear sync loop, take a longer pause to be in the safer side. Taking a longer delay is no longer a problem now because we're running in the normal context.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/x86/intel_hdmi_audio.c | 39 +++++++++++++++++++++++++-------------- sound/x86/intel_hdmi_audio.h | 1 + 2 files changed, 26 insertions(+), 14 deletions(-)
diff --git a/sound/x86/intel_hdmi_audio.c b/sound/x86/intel_hdmi_audio.c index c0a080e5d1f4..015d57cd9725 100644 --- a/sound/x86/intel_hdmi_audio.c +++ b/sound/x86/intel_hdmi_audio.c @@ -29,6 +29,7 @@ #include <linux/interrupt.h> #include <linux/pm_runtime.h> #include <linux/dma-mapping.h> +#include <linux/delay.h> #include <asm/cacheflush.h> #include <sound/core.h> #include <sound/asoundef.h> @@ -976,8 +977,6 @@ static void had_process_buffer_done(struct snd_intelhad *intelhaddata) had_substream_put(intelhaddata); }
-#define MAX_CNT 0xFF - /* * The interrupt status 'sticky' bits might not be cleared by * setting '1' to that bit once... @@ -987,31 +986,37 @@ static void wait_clear_underrun_bit(struct snd_intelhad *intelhaddata) int i; u32 val;
- for (i = 0; i < MAX_CNT; i++) { + for (i = 0; i < 100; i++) { /* clear bit30, 31 AUD_HDMI_STATUS */ had_read_register(intelhaddata, AUD_HDMI_STATUS, &val); if (!(val & AUD_HDMI_STATUS_MASK_UNDERRUN)) return; + udelay(100); + cond_resched(); had_write_register(intelhaddata, AUD_HDMI_STATUS, val); } dev_err(intelhaddata->dev, "Unable to clear UNDERRUN bits\n"); }
-/* called from irq handler */ -static void had_process_buffer_underrun(struct snd_intelhad *intelhaddata) +/* Perform some reset procedure but only when need_reset is set; + * this is called from prepare or hw_free callbacks once after trigger STOP + * or underrun has been processed in order to settle down the h/w state. + */ +static void had_do_reset(struct snd_intelhad *intelhaddata) { - struct snd_pcm_substream *substream; + if (!intelhaddata->need_reset) + return;
- /* Handle Underrun interrupt within Audio Unit */ - had_write_register(intelhaddata, AUD_CONFIG, 0); - intelhaddata->aud_config.regval = 0; /* Reset buffer pointers */ had_reset_audio(intelhaddata); - wait_clear_underrun_bit(intelhaddata); + intelhaddata->need_reset = false; +}
- if (!intelhaddata->connected) - return; /* disconnected? - bail out */ +/* called from irq handler */ +static void had_process_buffer_underrun(struct snd_intelhad *intelhaddata) +{ + struct snd_pcm_substream *substream;
/* Report UNDERRUN error to above layers */ substream = had_substream_get(intelhaddata); @@ -1019,6 +1024,7 @@ static void had_process_buffer_underrun(struct snd_intelhad *intelhaddata) snd_pcm_stop_xrun(substream); had_substream_put(intelhaddata); } + intelhaddata->need_reset = true; }
/* @@ -1134,9 +1140,13 @@ static int had_pcm_hw_params(struct snd_pcm_substream *substream, */ static int had_pcm_hw_free(struct snd_pcm_substream *substream) { + struct snd_intelhad *intelhaddata; unsigned long addr; u32 pages;
+ intelhaddata = snd_pcm_substream_chip(substream); + had_do_reset(intelhaddata); + /* mark back the pages as cached/writeback region before the free */ if (substream->runtime->dma_area != NULL) { addr = (unsigned long) substream->runtime->dma_area; @@ -1188,8 +1198,7 @@ static int had_pcm_trigger(struct snd_pcm_substream *substream, int cmd) spin_unlock(&intelhaddata->had_spinlock); /* Disable Audio */ had_enable_audio(intelhaddata, false); - /* Reset buffer pointers */ - had_reset_audio(intelhaddata); + intelhaddata->need_reset = true; break;
default: @@ -1227,6 +1236,8 @@ static int had_pcm_prepare(struct snd_pcm_substream *substream) dev_dbg(intelhaddata->dev, "rate=%d\n", runtime->rate); dev_dbg(intelhaddata->dev, "channels=%d\n", runtime->channels);
+ had_do_reset(intelhaddata); + /* Get N value in KHz */ disp_samp_freq = intelhaddata->tmds_clock_speed;
diff --git a/sound/x86/intel_hdmi_audio.h b/sound/x86/intel_hdmi_audio.h index a96728a4e7bc..8b9e184fef44 100644 --- a/sound/x86/intel_hdmi_audio.h +++ b/sound/x86/intel_hdmi_audio.h @@ -130,6 +130,7 @@ struct snd_intelhad { union aud_cfg aud_config; /* AUD_CONFIG reg value cache */ struct work_struct hdmi_audio_wq; struct mutex mutex; /* for protecting chmap and eld */ + bool need_reset; };
#endif /* _INTEL_HDMI_AUDIO_ */
The pcm_stream_info.running field is only set in the PCM trigger callback but never referred, thus it can be safely removed.
Also, properly cover the spinlock in both the trigger START and STOP to protect had_enable_audio() calls.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/x86/intel_hdmi_audio.c | 11 ++--------- sound/x86/intel_hdmi_audio.h | 1 - 2 files changed, 2 insertions(+), 10 deletions(-)
diff --git a/sound/x86/intel_hdmi_audio.c b/sound/x86/intel_hdmi_audio.c index 015d57cd9725..9889cdf3ccf4 100644 --- a/sound/x86/intel_hdmi_audio.c +++ b/sound/x86/intel_hdmi_audio.c @@ -1168,6 +1168,7 @@ static int had_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
intelhaddata = snd_pcm_substream_chip(substream);
+ spin_lock(&intelhaddata->had_spinlock); switch (cmd) { case SNDRV_PCM_TRIGGER_START: case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: @@ -1180,8 +1181,6 @@ static int had_pcm_trigger(struct snd_pcm_substream *substream, int cmd) break; }
- intelhaddata->stream_info.running = true; - /* Enable Audio */ had_ack_irqs(intelhaddata); /* FIXME: do we need this? */ had_enable_audio(intelhaddata, true); @@ -1189,13 +1188,6 @@ static int had_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
case SNDRV_PCM_TRIGGER_STOP: case SNDRV_PCM_TRIGGER_PAUSE_PUSH: - case SNDRV_PCM_TRIGGER_SUSPEND: - spin_lock(&intelhaddata->had_spinlock); - - /* Stop reporting BUFFER_DONE/UNDERRUN to above layers */ - - intelhaddata->stream_info.running = false; - spin_unlock(&intelhaddata->had_spinlock); /* Disable Audio */ had_enable_audio(intelhaddata, false); intelhaddata->need_reset = true; @@ -1204,6 +1196,7 @@ static int had_pcm_trigger(struct snd_pcm_substream *substream, int cmd) default: retval = -EINVAL; } + spin_unlock(&intelhaddata->had_spinlock); return retval; }
diff --git a/sound/x86/intel_hdmi_audio.h b/sound/x86/intel_hdmi_audio.h index 8b9e184fef44..d6ba90fd011d 100644 --- a/sound/x86/intel_hdmi_audio.h +++ b/sound/x86/intel_hdmi_audio.h @@ -83,7 +83,6 @@ struct channel_map_table { struct pcm_stream_info { struct snd_pcm_substream *substream; int substream_refcount; - bool running; };
/*
This patch adds a few lines to the driver to use autosuspend for the runtime PM. It'll become useful with the combination of the keep-link feature.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/x86/intel_hdmi_audio.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-)
diff --git a/sound/x86/intel_hdmi_audio.c b/sound/x86/intel_hdmi_audio.c index 9889cdf3ccf4..95b07a260d54 100644 --- a/sound/x86/intel_hdmi_audio.c +++ b/sound/x86/intel_hdmi_audio.c @@ -1076,7 +1076,8 @@ static int had_pcm_open(struct snd_pcm_substream *substream)
return retval; error: - pm_runtime_put(intelhaddata->dev); + pm_runtime_mark_last_busy(intelhaddata->dev); + pm_runtime_put_autosuspend(intelhaddata->dev); return retval; }
@@ -1100,7 +1101,8 @@ static int had_pcm_close(struct snd_pcm_substream *substream) } spin_unlock_irq(&intelhaddata->had_spinlock);
- pm_runtime_put(intelhaddata->dev); + pm_runtime_mark_last_busy(intelhaddata->dev); + pm_runtime_put_autosuspend(intelhaddata->dev); return 0; }
@@ -1605,7 +1607,8 @@ static void had_audio_wq(struct work_struct *work) had_process_mode_change(ctx); } mutex_unlock(&ctx->mutex); - pm_runtime_put(ctx->dev); + pm_runtime_mark_last_busy(ctx->dev); + pm_runtime_put_autosuspend(ctx->dev); }
/* @@ -1637,10 +1640,17 @@ static int __maybe_unused hdmi_lpe_audio_suspend(struct device *dev) return err; }
+static int hdmi_lpe_audio_runtime_resume(struct device *dev) +{ + pm_runtime_mark_last_busy(dev); + return 0; +} + static int __maybe_unused hdmi_lpe_audio_resume(struct device *dev) { struct snd_intelhad *ctx = dev_get_drvdata(dev);
+ hdmi_lpe_audio_runtime_resume(dev); snd_power_change_state(ctx->card, SNDRV_CTL_POWER_D0); return 0; } @@ -1789,6 +1799,9 @@ static int hdmi_lpe_audio_probe(struct platform_device *pdev) pdata->notify_pending = false; spin_unlock_irq(&pdata->lpe_audio_slock);
+ pm_runtime_use_autosuspend(&pdev->dev); + pm_runtime_mark_last_busy(&pdev->dev); + pm_runtime_set_active(&pdev->dev); pm_runtime_enable(&pdev->dev);
@@ -1817,7 +1830,8 @@ static int hdmi_lpe_audio_remove(struct platform_device *pdev)
static const struct dev_pm_ops hdmi_lpe_audio_pm = { SET_SYSTEM_SLEEP_PM_OPS(hdmi_lpe_audio_suspend, hdmi_lpe_audio_resume) - SET_RUNTIME_PM_OPS(hdmi_lpe_audio_runtime_suspend, NULL, NULL) + SET_RUNTIME_PM_OPS(hdmi_lpe_audio_runtime_suspend, + hdmi_lpe_audio_runtime_resume, NULL) };
static struct platform_driver hdmi_lpe_audio_driver = {
This patch enables the "keep-link" feature experimentally. It's a feature where the device keeps the link and sending the silent output even after the PCM device is closed. Then the receiver will be resumed quickly once when a PCM is opened and a stream is sent again.
The stream link is turned off when the device goes to the auto suspend, and it's set to two seconds after the PCM close. This timeout value can be changed dynamically in a standard way via sysfs like other drivers. For example, to make it 10 seconds, run like:
echo 10000 > /sys/bus/platform/devices/hdmi-lpe-audio/power/autosuspend_delay_ms
This new keep-link feature itself is controlled via a new module option, keep_link. You can turn it on/off, again, via sysfs like:
echo 0 > /sys/module/snd_hdmi_lpe_audio/parameters/keep_link
As default, the feature is turned on.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/x86/intel_hdmi_audio.c | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+)
diff --git a/sound/x86/intel_hdmi_audio.c b/sound/x86/intel_hdmi_audio.c index 95b07a260d54..506cff306b5c 100644 --- a/sound/x86/intel_hdmi_audio.c +++ b/sound/x86/intel_hdmi_audio.c @@ -52,6 +52,10 @@ module_param_named(id, hdmi_card_id, charp, 0444); MODULE_PARM_DESC(id, "ID string for INTEL Intel HDMI Audio controller.");
+static bool keep_link = true; +module_param(keep_link, bool, 0644); +MODULE_PARM_DESC(keep_link, "Keep link on after the stream is closed."); + /* * ELD SA bits in the CEA Speaker Allocation data block */ @@ -217,8 +221,12 @@ static void had_write_register(struct snd_intelhad *ctx, u32 reg, u32 val) static void had_enable_audio(struct snd_intelhad *intelhaddata, bool enable) { + if (intelhaddata->aud_config.regx.aud_en == enable) + return; + /* update the cached value */ intelhaddata->aud_config.regx.aud_en = enable; + intelhaddata->aud_config.regx.underrun = keep_link; had_write_register(intelhaddata, AUD_CONFIG, intelhaddata->aud_config.regval); } @@ -901,6 +909,21 @@ static void had_init_ringbuf(struct snd_pcm_substream *substream, intelhaddata->bd_head = 0; /* reset at head again before starting */ }
+/* Set up the silent output after PCM close */ +static void had_keep_silent(struct snd_intelhad *intelhaddata) +{ + int i; + + if (!(keep_link && intelhaddata->connected && + intelhaddata->aud_config.regval)) + return; + + for (i = 0; i < HAD_NUM_OF_RING_BUFS; i++) + had_invalidate_bd(intelhaddata, i); + intelhaddata->need_reset = true; /* reset at next */ + had_enable_audio(intelhaddata, true); +} + /* process a bd, advance to the next */ static void had_advance_ringbuf(struct snd_pcm_substream *substream, struct snd_intelhad *intelhaddata) @@ -1007,6 +1030,9 @@ static void had_do_reset(struct snd_intelhad *intelhaddata) if (!intelhaddata->need_reset) return;
+ /* disable the silent output */ + had_enable_audio(intelhaddata, false); + /* Reset buffer pointers */ had_reset_audio(intelhaddata); wait_clear_underrun_bit(intelhaddata); @@ -1101,6 +1127,8 @@ static int had_pcm_close(struct snd_pcm_substream *substream) } spin_unlock_irq(&intelhaddata->had_spinlock);
+ had_keep_silent(intelhaddata); + pm_runtime_mark_last_busy(intelhaddata->dev); pm_runtime_put_autosuspend(intelhaddata->dev); return 0; @@ -1626,6 +1654,9 @@ static int hdmi_lpe_audio_runtime_suspend(struct device *dev) had_substream_put(ctx); }
+ /* disable the silent output */ + had_enable_audio(ctx, false); + return 0; }
@@ -1642,6 +1673,9 @@ static int __maybe_unused hdmi_lpe_audio_suspend(struct device *dev)
static int hdmi_lpe_audio_runtime_resume(struct device *dev) { + struct snd_intelhad *ctx = dev_get_drvdata(dev); + + had_keep_silent(ctx); pm_runtime_mark_last_busy(dev); return 0; } @@ -1799,6 +1833,10 @@ static int hdmi_lpe_audio_probe(struct platform_device *pdev) pdata->notify_pending = false; spin_unlock_irq(&pdata->lpe_audio_slock);
+ /* set a relatively long autosuspend delay (2 seconds) for making + * keep_link feature working reasonably + */ + pm_runtime_set_autosuspend_delay(&pdev->dev, 2000); pm_runtime_use_autosuspend(&pdev->dev); pm_runtime_mark_last_busy(&pdev->dev);
On 2/13/17 8:39 AM, Takashi Iwai wrote:
This patch enables the "keep-link" feature experimentally. It's a feature where the device keeps the link and sending the silent output even after the PCM device is closed. Then the receiver will be resumed quickly once when a PCM is opened and a stream is sent again.
The stream link is turned off when the device goes to the auto suspend, and it's set to two seconds after the PCM close. This timeout value can be changed dynamically in a standard way via sysfs like other drivers. For example, to make it 10 seconds, run like:
echo 10000 > /sys/bus/platform/devices/hdmi-lpe-audio/power/autosuspend_delay_ms
Keeping the link active has really a limited impact on power since the source provides power to the sink and will also drive the display. For people who rely on HDMI for system sounds/beeps/notifications it'd make more sense to make that value something like 15-30mn (i.e. aligned with display screen saver timeout), otherwise for every sound the receiver will have to spend 1-2 seconds figuring out if the data is PCM or compressed. PulseAudio has a 5s timeout on idle, 2s seems really low to me.
This new keep-link feature itself is controlled via a new module option, keep_link. You can turn it on/off, again, via sysfs like:
echo 0 > /sys/module/snd_hdmi_lpe_audio/parameters/keep_link
As default, the feature is turned on.
Signed-off-by: Takashi Iwai tiwai@suse.de
sound/x86/intel_hdmi_audio.c | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+)
diff --git a/sound/x86/intel_hdmi_audio.c b/sound/x86/intel_hdmi_audio.c index 95b07a260d54..506cff306b5c 100644 --- a/sound/x86/intel_hdmi_audio.c +++ b/sound/x86/intel_hdmi_audio.c @@ -52,6 +52,10 @@ module_param_named(id, hdmi_card_id, charp, 0444); MODULE_PARM_DESC(id, "ID string for INTEL Intel HDMI Audio controller.");
+static bool keep_link = true; +module_param(keep_link, bool, 0644); +MODULE_PARM_DESC(keep_link, "Keep link on after the stream is closed.");
/*
- ELD SA bits in the CEA Speaker Allocation data block
*/ @@ -217,8 +221,12 @@ static void had_write_register(struct snd_intelhad *ctx, u32 reg, u32 val) static void had_enable_audio(struct snd_intelhad *intelhaddata, bool enable) {
- if (intelhaddata->aud_config.regx.aud_en == enable)
return;- /* update the cached value */ intelhaddata->aud_config.regx.aud_en = enable;
- intelhaddata->aud_config.regx.underrun = keep_link; had_write_register(intelhaddata, AUD_CONFIG, intelhaddata->aud_config.regval);
} @@ -901,6 +909,21 @@ static void had_init_ringbuf(struct snd_pcm_substream *substream, intelhaddata->bd_head = 0; /* reset at head again before starting */ }
+/* Set up the silent output after PCM close */ +static void had_keep_silent(struct snd_intelhad *intelhaddata) +{
- int i;
- if (!(keep_link && intelhaddata->connected &&
intelhaddata->aud_config.regval))return;- for (i = 0; i < HAD_NUM_OF_RING_BUFS; i++)
had_invalidate_bd(intelhaddata, i);- intelhaddata->need_reset = true; /* reset at next */
- had_enable_audio(intelhaddata, true);
+}
/* process a bd, advance to the next */ static void had_advance_ringbuf(struct snd_pcm_substream *substream, struct snd_intelhad *intelhaddata) @@ -1007,6 +1030,9 @@ static void had_do_reset(struct snd_intelhad *intelhaddata) if (!intelhaddata->need_reset) return;
- /* disable the silent output */
- had_enable_audio(intelhaddata, false);
- /* Reset buffer pointers */ had_reset_audio(intelhaddata); wait_clear_underrun_bit(intelhaddata);
@@ -1101,6 +1127,8 @@ static int had_pcm_close(struct snd_pcm_substream *substream) } spin_unlock_irq(&intelhaddata->had_spinlock);
- had_keep_silent(intelhaddata);
- pm_runtime_mark_last_busy(intelhaddata->dev); pm_runtime_put_autosuspend(intelhaddata->dev); return 0;
@@ -1626,6 +1654,9 @@ static int hdmi_lpe_audio_runtime_suspend(struct device *dev) had_substream_put(ctx); }
- /* disable the silent output */
- had_enable_audio(ctx, false);
- return 0;
}
@@ -1642,6 +1673,9 @@ static int __maybe_unused hdmi_lpe_audio_suspend(struct device *dev)
static int hdmi_lpe_audio_runtime_resume(struct device *dev) {
- struct snd_intelhad *ctx = dev_get_drvdata(dev);
- had_keep_silent(ctx); pm_runtime_mark_last_busy(dev); return 0;
} @@ -1799,6 +1833,10 @@ static int hdmi_lpe_audio_probe(struct platform_device *pdev) pdata->notify_pending = false; spin_unlock_irq(&pdata->lpe_audio_slock);
- /* set a relatively long autosuspend delay (2 seconds) for making
* keep_link feature working reasonably*/- pm_runtime_set_autosuspend_delay(&pdev->dev, 2000); pm_runtime_use_autosuspend(&pdev->dev); pm_runtime_mark_last_busy(&pdev->dev);
On Mon, 13 Feb 2017 18:05:37 +0100, Pierre-Louis Bossart wrote:
On 2/13/17 8:39 AM, Takashi Iwai wrote:
This patch enables the "keep-link" feature experimentally. It's a feature where the device keeps the link and sending the silent output even after the PCM device is closed. Then the receiver will be resumed quickly once when a PCM is opened and a stream is sent again.
The stream link is turned off when the device goes to the auto suspend, and it's set to two seconds after the PCM close. This timeout value can be changed dynamically in a standard way via sysfs like other drivers. For example, to make it 10 seconds, run like:
echo 10000 > /sys/bus/platform/devices/hdmi-lpe-audio/power/autosuspend_delay_ms
Keeping the link active has really a limited impact on power since the source provides power to the sink and will also drive the display. For people who rely on HDMI for system sounds/beeps/notifications it'd make more sense to make that value something like 15-30mn (i.e. aligned with display screen saver timeout), otherwise for every sound the receiver will have to spend 1-2 seconds figuring out if the data is PCM or compressed. PulseAudio has a 5s timeout on idle, 2s seems really low to me.
I have no problem to extend the timeout -- or even to disable the autosuspend as default. The current time was deduced from the past experience: keeping the audio link on HSW and onward may cost very much in the i915 graphics side because it blocks the power well audio domain. That's why we had to enable HD-audio autosuspend for HDMI specifically; there was an explicit request from graphics people. But BYT/CHT has no power well domain, so this might not matter. (Correct me if my assumption is wrong.)
Basically it's trivial to "fix" it. We may leave the autosuspend timeout untouched (i.e. 0) and don't enable autosuspend as default but keep the power on as default. Two more lines cut. Good.
People who care about the power may adjust the standard sysfs entries accordingly.
thanks,
Takashi
This new keep-link feature itself is controlled via a new module option, keep_link. You can turn it on/off, again, via sysfs like:
echo 0 > /sys/module/snd_hdmi_lpe_audio/parameters/keep_link
As default, the feature is turned on.
Signed-off-by: Takashi Iwai tiwai@suse.de
sound/x86/intel_hdmi_audio.c | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+)
diff --git a/sound/x86/intel_hdmi_audio.c b/sound/x86/intel_hdmi_audio.c index 95b07a260d54..506cff306b5c 100644 --- a/sound/x86/intel_hdmi_audio.c +++ b/sound/x86/intel_hdmi_audio.c @@ -52,6 +52,10 @@ module_param_named(id, hdmi_card_id, charp, 0444); MODULE_PARM_DESC(id, "ID string for INTEL Intel HDMI Audio controller.");
+static bool keep_link = true; +module_param(keep_link, bool, 0644); +MODULE_PARM_DESC(keep_link, "Keep link on after the stream is closed.");
/*
- ELD SA bits in the CEA Speaker Allocation data block
*/ @@ -217,8 +221,12 @@ static void had_write_register(struct snd_intelhad *ctx, u32 reg, u32 val) static void had_enable_audio(struct snd_intelhad *intelhaddata, bool enable) {
- if (intelhaddata->aud_config.regx.aud_en == enable)
return;- /* update the cached value */ intelhaddata->aud_config.regx.aud_en = enable;
- intelhaddata->aud_config.regx.underrun = keep_link; had_write_register(intelhaddata, AUD_CONFIG, intelhaddata->aud_config.regval);
} @@ -901,6 +909,21 @@ static void had_init_ringbuf(struct snd_pcm_substream *substream, intelhaddata->bd_head = 0; /* reset at head again before starting */ }
+/* Set up the silent output after PCM close */ +static void had_keep_silent(struct snd_intelhad *intelhaddata) +{
- int i;
- if (!(keep_link && intelhaddata->connected &&
intelhaddata->aud_config.regval))return;- for (i = 0; i < HAD_NUM_OF_RING_BUFS; i++)
had_invalidate_bd(intelhaddata, i);- intelhaddata->need_reset = true; /* reset at next */
- had_enable_audio(intelhaddata, true);
+}
/* process a bd, advance to the next */ static void had_advance_ringbuf(struct snd_pcm_substream *substream, struct snd_intelhad *intelhaddata) @@ -1007,6 +1030,9 @@ static void had_do_reset(struct snd_intelhad *intelhaddata) if (!intelhaddata->need_reset) return;
- /* disable the silent output */
- had_enable_audio(intelhaddata, false);
- /* Reset buffer pointers */ had_reset_audio(intelhaddata); wait_clear_underrun_bit(intelhaddata);
@@ -1101,6 +1127,8 @@ static int had_pcm_close(struct snd_pcm_substream *substream) } spin_unlock_irq(&intelhaddata->had_spinlock);
- had_keep_silent(intelhaddata);
- pm_runtime_mark_last_busy(intelhaddata->dev); pm_runtime_put_autosuspend(intelhaddata->dev); return 0;
@@ -1626,6 +1654,9 @@ static int hdmi_lpe_audio_runtime_suspend(struct device *dev) had_substream_put(ctx); }
- /* disable the silent output */
- had_enable_audio(ctx, false);
- return 0;
}
@@ -1642,6 +1673,9 @@ static int __maybe_unused hdmi_lpe_audio_suspend(struct device *dev)
static int hdmi_lpe_audio_runtime_resume(struct device *dev) {
- struct snd_intelhad *ctx = dev_get_drvdata(dev);
- had_keep_silent(ctx); pm_runtime_mark_last_busy(dev); return 0;
} @@ -1799,6 +1833,10 @@ static int hdmi_lpe_audio_probe(struct platform_device *pdev) pdata->notify_pending = false; spin_unlock_irq(&pdata->lpe_audio_slock);
- /* set a relatively long autosuspend delay (2 seconds) for making
* keep_link feature working reasonably*/- pm_runtime_set_autosuspend_delay(&pdev->dev, 2000); pm_runtime_use_autosuspend(&pdev->dev); pm_runtime_mark_last_busy(&pdev->dev);
On Mon, 13 Feb 2017 21:05:31 +0100, Takashi Iwai wrote:
On Mon, 13 Feb 2017 18:05:37 +0100, Pierre-Louis Bossart wrote:
On 2/13/17 8:39 AM, Takashi Iwai wrote:
This patch enables the "keep-link" feature experimentally. It's a feature where the device keeps the link and sending the silent output even after the PCM device is closed. Then the receiver will be resumed quickly once when a PCM is opened and a stream is sent again.
The stream link is turned off when the device goes to the auto suspend, and it's set to two seconds after the PCM close. This timeout value can be changed dynamically in a standard way via sysfs like other drivers. For example, to make it 10 seconds, run like:
echo 10000 > /sys/bus/platform/devices/hdmi-lpe-audio/power/autosuspend_delay_ms
Keeping the link active has really a limited impact on power since the source provides power to the sink and will also drive the display. For people who rely on HDMI for system sounds/beeps/notifications it'd make more sense to make that value something like 15-30mn (i.e. aligned with display screen saver timeout), otherwise for every sound the receiver will have to spend 1-2 seconds figuring out if the data is PCM or compressed. PulseAudio has a 5s timeout on idle, 2s seems really low to me.
I have no problem to extend the timeout -- or even to disable the autosuspend as default. The current time was deduced from the past experience: keeping the audio link on HSW and onward may cost very much in the i915 graphics side because it blocks the power well audio domain. That's why we had to enable HD-audio autosuspend for HDMI specifically; there was an explicit request from graphics people. But BYT/CHT has no power well domain, so this might not matter. (Correct me if my assumption is wrong.)
Basically it's trivial to "fix" it. We may leave the autosuspend timeout untouched (i.e. 0) and don't enable autosuspend as default but keep the power on as default. Two more lines cut. Good.
People who care about the power may adjust the standard sysfs entries accordingly.
... and below is the revised patch. Will refresh the branch as well.
Takashi
-- 8< -- From: Takashi Iwai tiwai@suse.de Subject: [PATCH v2] ALSA: x86: Enable keep-link feature
This patch enables the "keep-link" feature experimentally. It's a feature where the device keeps the link and sending the silent output even after the PCM device is closed. Then the receiver will be resumed quickly once when a PCM is opened and a stream is sent again.
The stream link is turned off when the device goes to autosuspend or manual suspend.
With this commit, the runtime PM is not enabled any longer as default since there is little gain on BYT/CHT with this audio device PM. User who still wants the runtime PM may adjust the corresponding sysfs files (power/control and power/autosuspend_delay_ms) appropriately, of course.
This new keep-link feature itself is controlled via a new module option, keep_link. You can turn it on/off, again, via sysfs like:
echo 0 > /sys/module/snd_hdmi_lpe_audio/parameters/keep_link
As default, the feature is turned on.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/x86/intel_hdmi_audio.c | 40 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 38 insertions(+), 2 deletions(-)
diff --git a/sound/x86/intel_hdmi_audio.c b/sound/x86/intel_hdmi_audio.c index 95b07a260d54..0805c835bb8b 100644 --- a/sound/x86/intel_hdmi_audio.c +++ b/sound/x86/intel_hdmi_audio.c @@ -52,6 +52,10 @@ module_param_named(id, hdmi_card_id, charp, 0444); MODULE_PARM_DESC(id, "ID string for INTEL Intel HDMI Audio controller.");
+static bool keep_link = true; +module_param(keep_link, bool, 0644); +MODULE_PARM_DESC(keep_link, "Keep link on after the stream is closed."); + /* * ELD SA bits in the CEA Speaker Allocation data block */ @@ -217,8 +221,12 @@ static void had_write_register(struct snd_intelhad *ctx, u32 reg, u32 val) static void had_enable_audio(struct snd_intelhad *intelhaddata, bool enable) { + if (intelhaddata->aud_config.regx.aud_en == enable) + return; + /* update the cached value */ intelhaddata->aud_config.regx.aud_en = enable; + intelhaddata->aud_config.regx.underrun = keep_link; had_write_register(intelhaddata, AUD_CONFIG, intelhaddata->aud_config.regval); } @@ -901,6 +909,21 @@ static void had_init_ringbuf(struct snd_pcm_substream *substream, intelhaddata->bd_head = 0; /* reset at head again before starting */ }
+/* Set up the silent output after PCM close */ +static void had_keep_silent(struct snd_intelhad *intelhaddata) +{ + int i; + + if (!(keep_link && intelhaddata->connected && + intelhaddata->aud_config.regval)) + return; + + for (i = 0; i < HAD_NUM_OF_RING_BUFS; i++) + had_invalidate_bd(intelhaddata, i); + intelhaddata->need_reset = true; /* reset at next */ + had_enable_audio(intelhaddata, true); +} + /* process a bd, advance to the next */ static void had_advance_ringbuf(struct snd_pcm_substream *substream, struct snd_intelhad *intelhaddata) @@ -1007,6 +1030,9 @@ static void had_do_reset(struct snd_intelhad *intelhaddata) if (!intelhaddata->need_reset) return;
+ /* disable the silent output */ + had_enable_audio(intelhaddata, false); + /* Reset buffer pointers */ had_reset_audio(intelhaddata); wait_clear_underrun_bit(intelhaddata); @@ -1101,6 +1127,8 @@ static int had_pcm_close(struct snd_pcm_substream *substream) } spin_unlock_irq(&intelhaddata->had_spinlock);
+ had_keep_silent(intelhaddata); + pm_runtime_mark_last_busy(intelhaddata->dev); pm_runtime_put_autosuspend(intelhaddata->dev); return 0; @@ -1626,6 +1654,9 @@ static int hdmi_lpe_audio_runtime_suspend(struct device *dev) had_substream_put(ctx); }
+ /* disable the silent output */ + had_enable_audio(ctx, false); + return 0; }
@@ -1642,6 +1673,9 @@ static int __maybe_unused hdmi_lpe_audio_suspend(struct device *dev)
static int hdmi_lpe_audio_runtime_resume(struct device *dev) { + struct snd_intelhad *ctx = dev_get_drvdata(dev); + + had_keep_silent(ctx); pm_runtime_mark_last_busy(dev); return 0; } @@ -1799,11 +1833,13 @@ static int hdmi_lpe_audio_probe(struct platform_device *pdev) pdata->notify_pending = false; spin_unlock_irq(&pdata->lpe_audio_slock);
+ /* runtime PM isn't enabled as default, since it won't save much on + * BYT/CHT devices; user who want the runtime PM should adjust the + * power/ontrol and power/autosuspend_delay_ms sysfs entries instead + */ pm_runtime_use_autosuspend(&pdev->dev); pm_runtime_mark_last_busy(&pdev->dev); - pm_runtime_set_active(&pdev->dev); - pm_runtime_enable(&pdev->dev);
dev_dbg(&pdev->dev, "%s: handle pending notification\n", __func__); schedule_work(&ctx->hdmi_audio_wq);
On 02/13/2017 08:39 AM, Takashi Iwai wrote:
Hi,
Linus decided to take another week for 4.11 merge window, so I could play a bit more, and here is the result :)
The first patch is the fix for possible GPU hang, and this has been for a while in my local branch and tested. This should be OK to merge soon. (It's also as same as what Ian tested in the last weekend.)
The next one is merely a cleanup patch, so it's fine, too. The third one is a transition to use the runtime PM autosuspend, and it's a preliminary for the last change.
The last one is the new and experimental one: it enables the keep-link feature. With this patch, the device is managed as default via autosuspend, and the link is opened (but silenced) for the pre-given time (2 seconds) after the PCM close. If the application restarts the stream quickly, the receiver is still warm and can resume gracefully.
All these patches are found in topic/intel-lpe-audio branch. (BTW, I cleaned up my branches: now for-next branch tracks the latest stable patches to be merged, while topic/intel-lpe-audio branch tracks the experimental patches. Beware that the latter may be rebased frequently.)
I tried this topic/intel-lpe-audio branch on my Baytrail compute stick and CHT boards and I see a couple of problems while monkey testing.
1. PulseAudio fails to load the HDMI card on startup on the Compute Stick and only shows the dummy output, killing and restarting PulseAudio solves the problem. I had not seen this before and this doesn't happen on CHT devices, not sure if this a pulseaudio problem?
2. we had a set of errors in the past that are still present, i see them 100% of the time: E: [alsa-sink-HdmiLpeAudio] alsa-sink.c: ALSA woke us up to write new data to the device, but there was a ctually nothing to write! E: [alsa-sink-HdmiLpeAudio] alsa-sink.c: Most likely this is a bug in the ALSA driver 'snd_hdmi_lpe_audio '. Please report this issue to the ALSA developers. E: [alsa-sink-HdmiLpeAudio] alsa-sink.c: We were woken up with POLLOUT set -- however a subsequent snd_pc m_avail() returned 0 or another value < min_avail.
3. the third one is new, we have something wrong with the pointer management. This happened only once in my tests but still, not sure how it was introduced.
E: [alsa-sink-HdmiLpeAudio] alsa-util.c: snd_pcm_avail() returned a value that is exceptionally large: 13 6128 bytes (709 ms). E: [alsa-sink-HdmiLpeAudio] alsa-util.c: Most likely this is a bug in the ALSA driver 'snd_hdmi_lpe_audio '. Please report this issue to the ALSA developers. E: [alsa-sink-HdmiLpeAudio] alsa-util.c: snd_pcm_dump(): E: [alsa-sink-HdmiLpeAudio] alsa-util.c: Hardware PCM card 0 'Intel HDMI/DP LPE Audio' device 0 subdevice 0 E: [alsa-sink-HdmiLpeAudio] alsa-util.c: Its setup is: E: [alsa-sink-HdmiLpeAudio] alsa-util.c: stream : PLAYBACK E: [alsa-sink-HdmiLpeAudio] alsa-util.c: access : MMAP_INTERLEAVED E: [alsa-sink-HdmiLpeAudio] alsa-util.c: format : S16_LE E: [alsa-sink-HdmiLpeAudio] alsa-util.c: subformat : STD E: [alsa-sink-HdmiLpeAudio] alsa-util.c: channels : 2 E: [alsa-sink-HdmiLpeAudio] alsa-util.c: rate : 48000 E: [alsa-sink-HdmiLpeAudio] alsa-util.c: exact rate : 48000 (48000/1) E: [alsa-sink-HdmiLpeAudio] alsa-util.c: msbits : 16 E: [alsa-sink-HdmiLpeAudio] alsa-util.c: buffer_size : 3840 E: [alsa-sink-HdmiLpeAudio] alsa-util.c: period_size : 480 E: [alsa-sink-HdmiLpeAudio] alsa-util.c: period_time : 10000 E: [alsa-sink-HdmiLpeAudio] alsa-util.c: tstamp_mode : ENABLE E: [alsa-sink-HdmiLpeAudio] alsa-util.c: period_step : 1 E: [alsa-sink-HdmiLpeAudio] alsa-util.c: avail_min : 480 E: [alsa-sink-HdmiLpeAudio] alsa-util.c: period_event : 1 E: [alsa-sink-HdmiLpeAudio] alsa-util.c: start_threshold : -1 E: [alsa-sink-HdmiLpeAudio] alsa-util.c: stop_threshold : 8646911284551352320 E: [alsa-sink-HdmiLpeAudio] alsa-util.c: silence_threshold: 0 E: [alsa-sink-HdmiLpeAudio] alsa-util.c: silence_size : 0 E: [alsa-sink-HdmiLpeAudio] alsa-util.c: boundary : 8646911284551352320 E: [alsa-sink-HdmiLpeAudio] alsa-util.c: appl_ptr : 301456 E: [alsa-sink-HdmiLpeAudio] alsa-util.c: hw_ptr : 331680
Indeed this looks like a wrap-around?
4. the keep-link thing doesn't seem to work for me. Once PulseAudio suspends the output, if I try to play a short notification I lose the first second (as in hearing only 'left' in 'front left', as in with the first sound I play. I even increased the threshold to 10s and still no joy. Could there be a higher level suspend that turns the link off?
5. if I change the display resolution while playing a sound through hw:0, at the ALSA level the playback stops with all sorts of bad descriptor errors, which is fine. If I play through PulseAudio the card is unloaded when the resolution is changed and the sound keeps playing but to the dummy output. PulseAudio needs to be killed and restarted to play sound again over HDMI. Wondering if this is the same problem as for the first case, PulseAudio not able to digest a uevent when a card is created?
6. if I remove the HDMI output and reinsert it, PulseAudio again fails to detect. I have to kill pulseaudio and restart it. I also saw this log during plug/unplug W: [alsa-sink-HdmiLpeAudio] alsa-util.c: Could not recover from POLLERR|POLLNVAL|POLLHUP with snd_pcm_pre pare(): No such device
7. I can't get the multichannel sound to work, anything with more that 2ch is silent except for FR and FL. Playback keeps going but only 2 channels are playing. Not sure if this is my receiver sending bad ELD information or just that the hw_params are not limited to the capabilities of the display.
Overall it's pretty robust though compared to the legacy patches, except for the pulseaudio detection issue there was never a moment where I had to reboot or do something drastic to recover. Thanks for all your work Takashi!
On Mon, 13 Feb 2017 23:56:36 +0100, Pierre-Louis Bossart wrote:
On 02/13/2017 08:39 AM, Takashi Iwai wrote:
Hi,
Linus decided to take another week for 4.11 merge window, so I could play a bit more, and here is the result :)
The first patch is the fix for possible GPU hang, and this has been for a while in my local branch and tested. This should be OK to merge soon. (It's also as same as what Ian tested in the last weekend.)
The next one is merely a cleanup patch, so it's fine, too. The third one is a transition to use the runtime PM autosuspend, and it's a preliminary for the last change.
The last one is the new and experimental one: it enables the keep-link feature. With this patch, the device is managed as default via autosuspend, and the link is opened (but silenced) for the pre-given time (2 seconds) after the PCM close. If the application restarts the stream quickly, the receiver is still warm and can resume gracefully.
All these patches are found in topic/intel-lpe-audio branch. (BTW, I cleaned up my branches: now for-next branch tracks the latest stable patches to be merged, while topic/intel-lpe-audio branch tracks the experimental patches. Beware that the latter may be rebased frequently.)
I tried this topic/intel-lpe-audio branch on my Baytrail compute stick and CHT boards and I see a couple of problems while monkey testing.
- PulseAudio fails to load the HDMI card on startup on the Compute
Stick and only shows the dummy output, killing and restarting PulseAudio solves the problem. I had not seen this before and this doesn't happen on CHT devices, not sure if this a pulseaudio problem?
I guess so. Could you try to remove ~/.config/pulse and see whether it still happens?
- we had a set of errors in the past that are still present, i see
them 100% of the time: E: [alsa-sink-HdmiLpeAudio] alsa-sink.c: ALSA woke us up to write new data to the device, but there was a ctually nothing to write! E: [alsa-sink-HdmiLpeAudio] alsa-sink.c: Most likely this is a bug in the ALSA driver 'snd_hdmi_lpe_audio '. Please report this issue to the ALSA developers. E: [alsa-sink-HdmiLpeAudio] alsa-sink.c: We were woken up with POLLOUT set -- however a subsequent snd_pc m_avail() returned 0 or another value < min_avail.
It's a oft-seen issue but interesting. Though, I wonder whether PA setup is proper when I looked at the below:
- the third one is new, we have something wrong with the pointer
management. This happened only once in my tests but still, not sure how it was introduced.
E: [alsa-sink-HdmiLpeAudio] alsa-util.c: snd_pcm_avail() returned a value that is exceptionally large: 13 6128 bytes (709 ms). E: [alsa-sink-HdmiLpeAudio] alsa-util.c: Most likely this is a bug in the ALSA driver 'snd_hdmi_lpe_audio '. Please report this issue to the ALSA developers. E: [alsa-sink-HdmiLpeAudio] alsa-util.c: snd_pcm_dump(): E: [alsa-sink-HdmiLpeAudio] alsa-util.c: Hardware PCM card 0 'Intel HDMI/DP LPE Audio' device 0 subdevice 0 E: [alsa-sink-HdmiLpeAudio] alsa-util.c: Its setup is: E: [alsa-sink-HdmiLpeAudio] alsa-util.c: stream : PLAYBACK E: [alsa-sink-HdmiLpeAudio] alsa-util.c: access : MMAP_INTERLEAVED E: [alsa-sink-HdmiLpeAudio] alsa-util.c: format : S16_LE E: [alsa-sink-HdmiLpeAudio] alsa-util.c: subformat : STD E: [alsa-sink-HdmiLpeAudio] alsa-util.c: channels : 2 E: [alsa-sink-HdmiLpeAudio] alsa-util.c: rate : 48000 E: [alsa-sink-HdmiLpeAudio] alsa-util.c: exact rate : 48000 (48000/1) E: [alsa-sink-HdmiLpeAudio] alsa-util.c: msbits : 16 E: [alsa-sink-HdmiLpeAudio] alsa-util.c: buffer_size : 3840 E: [alsa-sink-HdmiLpeAudio] alsa-util.c: period_size : 480
See the values above.
E: [alsa-sink-HdmiLpeAudio] alsa-util.c: period_time : 10000 E: [alsa-sink-HdmiLpeAudio] alsa-util.c: tstamp_mode : ENABLE E: [alsa-sink-HdmiLpeAudio] alsa-util.c: period_step : 1 E: [alsa-sink-HdmiLpeAudio] alsa-util.c: avail_min : 480 E: [alsa-sink-HdmiLpeAudio] alsa-util.c: period_event : 1 E: [alsa-sink-HdmiLpeAudio] alsa-util.c: start_threshold : -1 E: [alsa-sink-HdmiLpeAudio] alsa-util.c: stop_threshold : 8646911284551352320 E: [alsa-sink-HdmiLpeAudio] alsa-util.c: silence_threshold: 0 E: [alsa-sink-HdmiLpeAudio] alsa-util.c: silence_size : 0 E: [alsa-sink-HdmiLpeAudio] alsa-util.c: boundary : 8646911284551352320 E: [alsa-sink-HdmiLpeAudio] alsa-util.c: appl_ptr : 301456 E: [alsa-sink-HdmiLpeAudio] alsa-util.c: hw_ptr : 331680
Indeed this looks like a wrap-around?
Why PA takes such a small buffer / period size (3840 / 480)? Do you have any special setup?
My test was with a bit old PA version, but it worked with a larger buffer, IIRC.
- the keep-link thing doesn't seem to work for me. Once PulseAudio
suspends the output, if I try to play a short notification I lose the first second (as in hearing only 'left' in 'front left', as in with the first sound I play. I even increased the threshold to 10s and still no joy. Could there be a higher level suspend that turns the link off?
OK, that's somewhat expected in a current patch. It keeps the link *after* the PCM device is closed, but it doesn't change the behavior so much *during* the PCM is being used by app. So PA keeps the device opened (maybe in PREPARED state?) after stopping the stream, and at this state, the link gets off because it's prepared for the playback trigger.
I don't know how we can avoid this: basically the silent output is a fake underrun. We need to set the invalid BDs to achieve it. Meanwhile, at PREPARED state, the buffer and the h/w must be set ready for streaming and just wait for the stream start toggle at TRIGGER.
Maybe there is some other way to achieve, but then I'd like to know the reference implementation before further hacking.
- if I change the display resolution while playing a sound through
hw:0, at the ALSA level the playback stops with all sorts of bad descriptor errors, which is fine. If I play through PulseAudio the card is unloaded when the resolution is changed and the sound keeps playing but to the dummy output. PulseAudio needs to be killed and restarted to play sound again over HDMI. Wondering if this is the same problem as for the first case, PulseAudio not able to digest a uevent when a card is created?
The problem is that it's no "card" plug. It's what I pointed in the past about PCM DISCONNECT state handling. The card itself isn't changed but only the PCM state changes. We could set PCM as DISCONNECTED, and then PA would treat like the card removal, and the device will be never reprobed unless the driver is really reprobed. It's like what you've seen. So, in the recent version, it never sets DISCONNECTED but it just stops the stream and rest returns -ENODEV error.
I'm not sure how PA react to this error case. Maybe -ENODEV is a bad choice. Need to ask Tanu about it.
- if I remove the HDMI output and reinsert it, PulseAudio again fails
to detect. I have to kill pulseaudio and restart it. I also saw this log during plug/unplug W: [alsa-sink-HdmiLpeAudio] alsa-util.c: Could not recover from POLLERR|POLLNVAL|POLLHUP with snd_pcm_pre pare(): No such device
A similar situation, but it's strange that it still returns -ENODEV. In the current code, -ENODEV should be returned only when the device is "connected", i.e. the mode is set.
But now looking at the code, I found a superfluous code at hotplug handler that should be removed:
-- 8< -- --- a/sound/x86/intel_hdmi_audio.c +++ b/sound/x86/intel_hdmi_audio.c @@ -1401,16 +1401,6 @@ static void had_process_hot_plug(struct snd_intelhad *intelhaddata) __func__, __LINE__); spin_unlock_irq(&intelhaddata->had_spinlock);
- /* Safety check */ - substream = had_substream_get(intelhaddata); - if (substream) { - dev_dbg(intelhaddata->dev, - "Force to stop the active stream by disconnection\n"); - /* Set runtime->state to hw_params done */ - snd_pcm_stop(substream, SNDRV_PCM_STATE_SETUP); - had_substream_put(intelhaddata); - } - had_build_channel_allocation_map(intelhaddata); }
-- 8< --
In anyway I'm going to check the behavior on my device, and reduce the superfluous code.
- I can't get the multichannel sound to work, anything with more that
2ch is silent except for FR and FL. Playback keeps going but only 2 channels are playing. Not sure if this is my receiver sending bad ELD information or just that the hw_params are not limited to the capabilities of the display.
Could you check ELD output? Now it's easily seen via ctl element. I have no surround receiver, so cannot check the capabilities, unfortunately.
About the multi-channel support, I haven't changed the code. For the multi-channels, the driver does: - Set AUD_CONFIG num_ch fields to channles-2 - Set AUD_CONFIG layout bit to 1 - Set IF2 chnl_cnt to channels-1 - Set IF2 chnl_alloc to CA value
Overall it's pretty robust though compared to the legacy patches, except for the pulseaudio detection issue there was never a moment where I had to reboot or do something drastic to recover. Thanks for all your work Takashi!
Thanks for your testing!
Takashi
In my testing I've seen the '*ERROR* Atomic update failure on pipe' message again on a CHT device whilst running a kernel build from the latest 'topic/intel-lpe-audio' branch:
[ 22.765836] intel_sst_acpi 808622A8:00: FW Version 01.0b.02.02 [ 32.280459] intel_sst_acpi 808622A8:00: FW Version 01.0b.02.02 [ 55.045261] intel_sst_acpi 808622A8:00: FW Version 01.0b.02.02 [ 83.529779] [drm:intel_pipe_update_end [i915]] *ERROR* Atomic update failure on pipe A (start=3853 end=3854) time 393 us, min 1073, max 1079, scanline start 1072, end 1099
However I cannot replicate it on demand.
On 14 February 2017 at 17:33, Takashi Iwai tiwai@suse.de wrote:
On Mon, 13 Feb 2017 23:56:36 +0100, Pierre-Louis Bossart wrote:
On 02/13/2017 08:39 AM, Takashi Iwai wrote:
Hi,
Linus decided to take another week for 4.11 merge window, so I could play a bit more, and here is the result :)
The first patch is the fix for possible GPU hang, and this has been for a while in my local branch and tested. This should be OK to merge soon. (It's also as same as what Ian tested in the last weekend.)
The next one is merely a cleanup patch, so it's fine, too. The third one is a transition to use the runtime PM autosuspend, and it's a preliminary for the last change.
The last one is the new and experimental one: it enables the keep-link feature. With this patch, the device is managed as default via autosuspend, and the link is opened (but silenced) for the pre-given time (2 seconds) after the PCM close. If the application restarts the stream quickly, the receiver is still warm and can resume gracefully.
All these patches are found in topic/intel-lpe-audio branch. (BTW, I cleaned up my branches: now for-next branch tracks the latest stable patches to be merged, while topic/intel-lpe-audio branch tracks the experimental patches. Beware that the latter may be rebased frequently.)
I tried this topic/intel-lpe-audio branch on my Baytrail compute stick and CHT boards and I see a couple of problems while monkey testing.
- PulseAudio fails to load the HDMI card on startup on the Compute
Stick and only shows the dummy output, killing and restarting PulseAudio solves the problem. I had not seen this before and this doesn't happen on CHT devices, not sure if this a pulseaudio problem?
I guess so. Could you try to remove ~/.config/pulse and see whether it still happens?
- we had a set of errors in the past that are still present, i see
them 100% of the time: E: [alsa-sink-HdmiLpeAudio] alsa-sink.c: ALSA woke us up to write new data to the device, but there was a ctually nothing to write! E: [alsa-sink-HdmiLpeAudio] alsa-sink.c: Most likely this is a bug in the ALSA driver 'snd_hdmi_lpe_audio '. Please report this issue to the ALSA developers. E: [alsa-sink-HdmiLpeAudio] alsa-sink.c: We were woken up with POLLOUT set -- however a subsequent snd_pc m_avail() returned 0 or another value < min_avail.
It's a oft-seen issue but interesting. Though, I wonder whether PA setup is proper when I looked at the below:
- the third one is new, we have something wrong with the pointer
management. This happened only once in my tests but still, not sure how it was introduced.
E: [alsa-sink-HdmiLpeAudio] alsa-util.c: snd_pcm_avail() returned a value that is exceptionally large: 13 6128 bytes (709 ms). E: [alsa-sink-HdmiLpeAudio] alsa-util.c: Most likely this is a bug in the ALSA driver 'snd_hdmi_lpe_audio '. Please report this issue to the ALSA developers. E: [alsa-sink-HdmiLpeAudio] alsa-util.c: snd_pcm_dump(): E: [alsa-sink-HdmiLpeAudio] alsa-util.c: Hardware PCM card 0 'Intel HDMI/DP LPE Audio' device 0 subdevice 0 E: [alsa-sink-HdmiLpeAudio] alsa-util.c: Its setup is: E: [alsa-sink-HdmiLpeAudio] alsa-util.c: stream : PLAYBACK E: [alsa-sink-HdmiLpeAudio] alsa-util.c: access :
MMAP_INTERLEAVED
E: [alsa-sink-HdmiLpeAudio] alsa-util.c: format : S16_LE E: [alsa-sink-HdmiLpeAudio] alsa-util.c: subformat : STD E: [alsa-sink-HdmiLpeAudio] alsa-util.c: channels : 2 E: [alsa-sink-HdmiLpeAudio] alsa-util.c: rate : 48000 E: [alsa-sink-HdmiLpeAudio] alsa-util.c: exact rate : 48000 (48000/1) E: [alsa-sink-HdmiLpeAudio] alsa-util.c: msbits : 16 E: [alsa-sink-HdmiLpeAudio] alsa-util.c: buffer_size : 3840 E: [alsa-sink-HdmiLpeAudio] alsa-util.c: period_size : 480
See the values above.
E: [alsa-sink-HdmiLpeAudio] alsa-util.c: period_time : 10000 E: [alsa-sink-HdmiLpeAudio] alsa-util.c: tstamp_mode : ENABLE E: [alsa-sink-HdmiLpeAudio] alsa-util.c: period_step : 1 E: [alsa-sink-HdmiLpeAudio] alsa-util.c: avail_min : 480 E: [alsa-sink-HdmiLpeAudio] alsa-util.c: period_event : 1 E: [alsa-sink-HdmiLpeAudio] alsa-util.c: start_threshold : -1 E: [alsa-sink-HdmiLpeAudio] alsa-util.c: stop_threshold : 8646911284551352320 E: [alsa-sink-HdmiLpeAudio] alsa-util.c: silence_threshold: 0 E: [alsa-sink-HdmiLpeAudio] alsa-util.c: silence_size : 0 E: [alsa-sink-HdmiLpeAudio] alsa-util.c: boundary : 8646911284551352320 E: [alsa-sink-HdmiLpeAudio] alsa-util.c: appl_ptr : 301456 E: [alsa-sink-HdmiLpeAudio] alsa-util.c: hw_ptr : 331680
Indeed this looks like a wrap-around?
Why PA takes such a small buffer / period size (3840 / 480)? Do you have any special setup?
My test was with a bit old PA version, but it worked with a larger buffer, IIRC.
- the keep-link thing doesn't seem to work for me. Once PulseAudio
suspends the output, if I try to play a short notification I lose the first second (as in hearing only 'left' in 'front left', as in with the first sound I play. I even increased the threshold to 10s and still no joy. Could there be a higher level suspend that turns the link off?
OK, that's somewhat expected in a current patch. It keeps the link *after* the PCM device is closed, but it doesn't change the behavior so much *during* the PCM is being used by app. So PA keeps the device opened (maybe in PREPARED state?) after stopping the stream, and at this state, the link gets off because it's prepared for the playback trigger.
I don't know how we can avoid this: basically the silent output is a fake underrun. We need to set the invalid BDs to achieve it. Meanwhile, at PREPARED state, the buffer and the h/w must be set ready for streaming and just wait for the stream start toggle at TRIGGER.
Maybe there is some other way to achieve, but then I'd like to know the reference implementation before further hacking.
- if I change the display resolution while playing a sound through
hw:0, at the ALSA level the playback stops with all sorts of bad descriptor errors, which is fine. If I play through PulseAudio the card is unloaded when the resolution is changed and the sound keeps playing but to the dummy output. PulseAudio needs to be killed and restarted to play sound again over HDMI. Wondering if this is the same problem as for the first case, PulseAudio not able to digest a uevent when a card is created?
The problem is that it's no "card" plug. It's what I pointed in the past about PCM DISCONNECT state handling. The card itself isn't changed but only the PCM state changes. We could set PCM as DISCONNECTED, and then PA would treat like the card removal, and the device will be never reprobed unless the driver is really reprobed. It's like what you've seen. So, in the recent version, it never sets DISCONNECTED but it just stops the stream and rest returns -ENODEV error.
I'm not sure how PA react to this error case. Maybe -ENODEV is a bad choice. Need to ask Tanu about it.
- if I remove the HDMI output and reinsert it, PulseAudio again fails
to detect. I have to kill pulseaudio and restart it. I also saw this log during plug/unplug W: [alsa-sink-HdmiLpeAudio] alsa-util.c: Could not recover from POLLERR|POLLNVAL|POLLHUP with snd_pcm_pre pare(): No such device
A similar situation, but it's strange that it still returns -ENODEV. In the current code, -ENODEV should be returned only when the device is "connected", i.e. the mode is set.
But now looking at the code, I found a superfluous code at hotplug handler that should be removed:
-- 8< -- --- a/sound/x86/intel_hdmi_audio.c +++ b/sound/x86/intel_hdmi_audio.c @@ -1401,16 +1401,6 @@ static void had_process_hot_plug(struct snd_intelhad *intelhaddata) __func__, __LINE__); spin_unlock_irq(&intelhaddata->had_spinlock);
/* Safety check */substream = had_substream_get(intelhaddata);if (substream) {dev_dbg(intelhaddata->dev,"Force to stop the active stream bydisconnection\n");
/* Set runtime->state to hw_params done */snd_pcm_stop(substream, SNDRV_PCM_STATE_SETUP);had_substream_put(intelhaddata);}had_build_channel_allocation_map(intelhaddata);}
-- 8< --
In anyway I'm going to check the behavior on my device, and reduce the superfluous code.
- I can't get the multichannel sound to work, anything with more that
2ch is silent except for FR and FL. Playback keeps going but only 2 channels are playing. Not sure if this is my receiver sending bad ELD information or just that the hw_params are not limited to the capabilities of the display.
Could you check ELD output? Now it's easily seen via ctl element. I have no surround receiver, so cannot check the capabilities, unfortunately.
About the multi-channel support, I haven't changed the code. For the multi-channels, the driver does:
- Set AUD_CONFIG num_ch fields to channles-2
- Set AUD_CONFIG layout bit to 1
- Set IF2 chnl_cnt to channels-1
- Set IF2 chnl_alloc to CA value
Overall it's pretty robust though compared to the legacy patches, except for the pulseaudio detection issue there was never a moment where I had to reboot or do something drastic to recover. Thanks for all your work Takashi!
Thanks for your testing!
Takashi
participants (3)
-
Ian W MORRISON -
Pierre-Louis Bossart -
Takashi Iwai