[alsa-devel] [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 at 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);
> >
> >
>