[PATCH 0/4] A few more extensions to LPE audio driver

[PATCH 0/4] A few more extensions to LPE audio driver

[Takashi Iwai]

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(-)

-- 
2.11.1

[PATCH 1/4] ALSA: x86: Handle reset at prepare callback

[Takashi Iwai]

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_ */
-- 
2.11.1

[PATCH 2/4] ALSA: x86: Drop unused stream.running field

[Takashi Iwai]

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;
 };
 
 /*
-- 
2.11.1

[PATCH 3/4] ALSA: x86: Use runtime PM autosuspend

[Takashi Iwai]

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 = {
-- 
2.11.1

[PATCH 4/4] ALSA: x86: Enable keep-link feature

[Takashi Iwai]

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);
 
-- 
2.11.1

Re: [PATCH 4/4] ALSA: x86: Enable keep-link feature

[Pierre-Louis Bossart]

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);
>
>

Re: [PATCH 4/4] ALSA: x86: Enable keep-link feature

[Takashi Iwai]

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);
> >
> >
> 

Re: [PATCH 4/4] ALSA: x86: Enable keep-link feature

[Takashi Iwai]

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);
-- 
2.11.1

Re: [PATCH 0/4] A few more extensions to LPE audio driver

[Pierre-Louis Bossart]

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!

Re: [PATCH 0/4] A few more extensions to LPE audio driver

[Takashi Iwai]

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.
> 
> 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?

I guess so.  Could you try to remove ~/.config/pulse and see whether
it still happens?


> 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.

It's a oft-seen issue but interesting.  Though, I wonder whether PA
setup is proper when I looked at the below:

> 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

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.


> 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?

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.

> 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?

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.


> 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

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.


> 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.

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

Re: [PATCH 0/4] A few more extensions to LPE audio driver

[Ian W MORRISON]

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.
> >
> > 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?
>
> I guess so.  Could you try to remove ~/.config/pulse and see whether
> it still happens?
>
>
> > 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.
>
> It's a oft-seen issue but interesting.  Though, I wonder whether PA
> setup is proper when I looked at the below:
>
> > 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
>
> 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.
>
>
> > 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?
>
> 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.
>
> > 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?
>
> 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.
>
>
> > 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
>
> 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.
>
>
> > 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.
>
> 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
>