Re: [PATCH] ALSA: hda/hdmi: Add Intel silent stream support

[Takashi Iwai <tiwai@suse.de>]

On Wed, 24 Jun 2020 01:26:36 +0200,
Harsha Priya wrote:
> 
> External HDMI receivers have analog circuitry that needs to be powered-on
> when exiting standby, and a mechanism to detect PCM v. IEC61937 data.
> These two steps take time and up to 2-3 seconds of audio may be muted
> when starting playback.
> 
> Intel hardware can keep the link active with a 'silent stream', so that
> the receiver does not go through those two steps when valid audio is
> transmitted. This mechanism relies on an info packet and preventing the
> codec from going to D3, which will increase the platform static power
> consumption. The info packet assumes a basic 2ch stereo, and the silent
> stream is enabled when connecting a monitor. In case of format changes the
> detection of PCM v. IEC61937 needs to be re-run. In this case there is no
> way to avoid the 2-3s mute.
> 
> The silent stream is enabled with a Kconfig option, as well as a kernel
> parameter should there be a need to override the build time default.

I'm not sure whether the module option is the best interface.
An alternative is a mixer element that controls dynamically.  Then
it'll be per card unlike the module option.

And I think Kconfig is redundant.

> Silent stream is supported in Intel platforms Skylake and beyond.
> Tested HDMI plug-out plug-in from Intel Cometlake based Chromebook
> connected to few different monitors.

IMO, the feature enablement should be done only for those devices.
The current patch influences on all HDMI devices including AMD and
others that are irrelevant so far.

About the code changes:

>  /* update ELD and jack state via audio component */
>  static void sync_eld_via_acomp(struct hda_codec *codec,
>  			       struct hdmi_spec_per_pin *per_pin)
>  {
>  	struct hdmi_spec *spec = codec->spec;
>  	struct hdmi_eld *eld = &spec->temp_eld;
> +	bool monitor_prev, monitor_next;
>  
>  	mutex_lock(&per_pin->lock);
>  	eld->monitor_present = false;
> +	monitor_prev = per_pin->sink_eld.monitor_present;
>  	eld->eld_size = snd_hdac_acomp_get_eld(&codec->core, per_pin->pin_nid,
>  				      per_pin->dev_id, &eld->monitor_present,
>  				      eld->eld_buffer, ELD_MAX_SIZE);
>  	eld->eld_valid = (eld->eld_size > 0);
>  	update_eld(codec, per_pin, eld, 0);
> +	monitor_next = per_pin->sink_eld.monitor_present;
>  	mutex_unlock(&per_pin->lock);
> +
> +	/*
> +	 * Power-up will call hdmi_present_sense, so the PM calls
> +	 * have to be done without mutex held.
> +	 */
> +
> +	if (enable_silent_stream) {
> +		if (!monitor_prev && monitor_next) {

Isn't a valid ELD mandatory?  The monitor_present flag itself can be
set even for the monitor without audio support, IIRC.

> +			snd_hda_power_up_pm(codec);
> +			silent_stream_enable(codec, per_pin);
> +		} else if (monitor_prev && !monitor_next)
> +			snd_hda_power_down_pm(codec);

This power up/down sequence may lead to the unbalance if the
enable_silent_stream flag is flipped during operation.


thanks,

Takashi