* [PATCH v2] ALSA: hda/hdmi: Add Intel silent stream support
@ 2020-06-30 0:23 Harsha Priya
2020-06-30 11:58 ` Kai Vehmanen
2020-07-01 9:02 ` kernel test robot
0 siblings, 2 replies; 3+ messages in thread
From: Harsha Priya @ 2020-06-30 0:23 UTC (permalink / raw)
To: alsa-devel, tiwai; +Cc: kai.vehmanen, Harsha Priya, Emmanuel Jillela
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.
This approach is used based on the power_save capability as an example,
but in the future, it may be used with a kcontrol, depending on
UCM support for HDaudio legacy.
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.
Signed-off-by: Harsha Priya <harshapriya.n@intel.com>
Signed-off-by: Emmanuel Jillela <emmanuel.jillela@intel.com>
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
---
Change History:
v2:
- Kconfig symbol depends on SND_HDA_INTEL.
- Added valid eld check to along side of silent stream enable check,
under which silent stream is enabled.
- Error prints added for codec power up, power down functions.
- The silent stream is enabled with a Kconfig/module parameter
following the power_save capability as an example. But in the future,
it may be used with a kcontrol, depending on UCM support for HDaudio legacy.
v1:
- Initial version.
sound/pci/hda/Kconfig | 16 ++++++++++++++
sound/pci/hda/patch_hdmi.c | 45 ++++++++++++++++++++++++++++++++++++++
2 files changed, 61 insertions(+)
diff --git a/sound/pci/hda/Kconfig b/sound/pci/hda/Kconfig
index 7ba542e45a3d..1c4758d58fc3 100644
--- a/sound/pci/hda/Kconfig
+++ b/sound/pci/hda/Kconfig
@@ -232,4 +232,20 @@ config SND_HDA_POWER_SAVE_DEFAULT
endif
+config SND_HDA_INTEL_HDMI_SILENT_STREAM
+ bool "Enable Silent Stream always for HDMI"
+ depends on SND_HDA_INTEL
+ help
+ Intel hardware has a feature called 'silent stream', that
+ keeps external HDMI receiver's analog circuitry powered on
+ avoiding 2-3 sec silence during playback start. This mechanism
+ relies on an info packet and preventing the codec from going to
+ D3. (increasing the platform static power consumption when a
+ HDMI receiver is plugged-in). 2-3 sec silence at the playback
+ start is expected whenever there is format change. (default is
+ 2 channel format).
+ Say Y to enable Silent Stream feature.
+
+endif
+
endmenu
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index fbd7cc6026d8..68b81ea176c6 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -42,6 +42,11 @@ static bool enable_acomp = true;
module_param(enable_acomp, bool, 0444);
MODULE_PARM_DESC(enable_acomp, "Enable audio component binding (default=yes)");
+static bool enable_silent_stream =
+IS_ENABLED(CONFIG_SND_HDA_INTEL_HDMI_SILENT_STREAM);
+module_param(enable_silent_stream, bool, 0644);
+MODULE_PARM_DESC(enable_silent_stream, "Enable Silent Stream for HDMI devices");
+
struct hdmi_spec_per_cvt {
hda_nid_t cvt_nid;
int assigned;
@@ -1634,21 +1639,61 @@ static void hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
snd_hda_power_down_pm(codec);
}
+static void silent_stream_enable(struct hda_codec *codec,
+ struct hdmi_spec_per_pin *per_pin)
+{
+ codec_dbg(codec, "hdmi: enabling silent stream for NID %d\n",
+ per_pin->pin_nid);
+
+ mutex_lock(&per_pin->lock);
+ if (!per_pin->channels)
+ per_pin->channels = 2;
+ hdmi_setup_audio_infoframe(codec, per_pin, per_pin->non_pcm);
+ mutex_unlock(&per_pin->lock);
+}
+
/* 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 && eld->eld_valid) {
+ int pm_ret;
+
+ if (!monitor_prev && monitor_next && eld->eld_valid) {
+ pm_ret = snd_hda_power_up_pm(codec);
+ if (pm_ret < 0)
+ codec_err(codec,
+ "Monitor plugged-in, Failed to power up codec ret=[%d]\n",
+ pm_ret);
+ silent_stream_enable(codec, per_pin);
+ } else if (monitor_prev && !monitor_next && !eld->eld_valid) {
+ pm_ret = snd_hda_power_down_pm(codec);
+ if (pm_ret < 0)
+ codec_err(codec,
+ "Monitor plugged-out, Failed to power down codec ret=[%d]\n",
+ pm_ret);
+ }
+ }
}
static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
--
2.17.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] ALSA: hda/hdmi: Add Intel silent stream support
2020-06-30 0:23 [PATCH v2] ALSA: hda/hdmi: Add Intel silent stream support Harsha Priya
@ 2020-06-30 11:58 ` Kai Vehmanen
2020-07-01 9:02 ` kernel test robot
1 sibling, 0 replies; 3+ messages in thread
From: Kai Vehmanen @ 2020-06-30 11:58 UTC (permalink / raw)
To: Harsha Priya; +Cc: Takashi Iwai, kai.vehmanen, alsa-devel, Emmanuel Jillela
Hi,
hmm, I think a few review comments from Takashi were missed. See inline:
On Mon, 29 Jun 2020, Harsha Priya wrote:
> --- a/sound/pci/hda/Kconfig
> +++ b/sound/pci/hda/Kconfig
> @@ -232,4 +232,20 @@ config SND_HDA_POWER_SAVE_DEFAULT
>
> endif
>
> +config SND_HDA_INTEL_HDMI_SILENT_STREAM
> + bool "Enable Silent Stream always for HDMI"
> + depends on SND_HDA_INTEL
Takashi requested to limit this to Intel hardware only, and despite the
its name, SND_HDA_INTEL is not sufficient to do this.
I think best would be to take silent stream into use in
intel_hsw_common_init(). That indirection would also protect against
user-space modifying the module parameter value during execution (it is
only evaluated in intel_hsw_common_init()).
> + if (enable_silent_stream && eld->eld_valid) {
> + int pm_ret;
> +
> + if (!monitor_prev && monitor_next && eld->eld_valid) {
> + pm_ret = snd_hda_power_up_pm(codec);
> + if (pm_ret < 0)
> + codec_err(codec,
> + "Monitor plugged-in, Failed to power up codec ret=[%d]\n",
> + pm_ret);
> + silent_stream_enable(codec, per_pin);
> + } else if (monitor_prev && !monitor_next && !eld->eld_valid) {
> + pm_ret = snd_hda_power_down_pm(codec);
> + if (pm_ret < 0)
> + codec_err(codec,
> + "Monitor plugged-out, Failed to power down codec ret=[%d]\n",
> + pm_ret);
> + }
> + }
There a bug in above, outer "if" checks for "eld_valid" while inner
"else-if" checks for "!eld_valid" -> latter can never be reached.
Takashi, I was checking older code history on the acomp interface, and at
least on Intel platforms using acomp, it is sufficient to check for
eld->monitor_present. I.e. in
» 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);
... monitor_present will always be 0 if eld_size<0 (and eld_valid is
set to 0). An error monitor_present will not be set, but
sync_eld_via_acomp() initialized the value to 0 for this case.
The additional checks in the patch are thus harmless, but make the code a
bit suspicious looking. If the above did not hold, you could have a
sequence like
monitor_prev=0, monitor_next=1, eld_valid=1 -> power-up
monitor_prev=1, monitor_next=0, eld_valid=0 -> no op
monitor_prev=0, monitor_next=1, eld_valid=1 -> power-up again
.. so the refcounting would go out of sync. So I'd rather just track the
two variables.
Br, Kai
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] ALSA: hda/hdmi: Add Intel silent stream support
2020-06-30 0:23 [PATCH v2] ALSA: hda/hdmi: Add Intel silent stream support Harsha Priya
2020-06-30 11:58 ` Kai Vehmanen
@ 2020-07-01 9:02 ` kernel test robot
1 sibling, 0 replies; 3+ messages in thread
From: kernel test robot @ 2020-07-01 9:02 UTC (permalink / raw)
To: Harsha Priya, alsa-devel, tiwai
Cc: kai.vehmanen, Harsha Priya, kbuild-all, Emmanuel Jillela
Hi Harsha,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on sound/for-next]
[also build test ERROR on v5.8-rc3 next-20200701]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Harsha-Priya/ALSA-hda-hdmi-Add-Intel-silent-stream-support/20200630-082719
base: https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
config: ia64-randconfig-s032-20200701
compiler: ia64-linux-gcc (GCC) 9.3.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.2-3-gfa153962-dirty
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=ia64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> sound/pci/hda/Kconfig:258: unexpected 'if' within menu block
>> sound/pci/hda/Kconfig:260: unexpected 'menu' within if block
sound/Kconfig:102: 'if' in different file than 'if'
sound/pci/hda/Kconfig:2: location of the 'if'
sound/Kconfig:104: 'if' in different file than 'if'
sound/pci/hda/Kconfig:2: location of the 'if'
>> sound/Kconfig:106: unexpected 'if' within menu block
>> drivers/Kconfig:239: syntax error
drivers/Kconfig:238: invalid statement
make[2]: *** [scripts/kconfig/Makefile:71: oldconfig] Error 1
make[1]: *** [Makefile:606: oldconfig] Error 2
make: *** [Makefile:185: __sub-make] Error 2
make: Target 'oldconfig' not remade because of errors.
--
>> sound/pci/hda/Kconfig:258: unexpected 'if' within menu block
>> sound/pci/hda/Kconfig:260: unexpected 'menu' within if block
sound/Kconfig:102: 'if' in different file than 'if'
sound/pci/hda/Kconfig:2: location of the 'if'
sound/Kconfig:104: 'if' in different file than 'if'
sound/pci/hda/Kconfig:2: location of the 'if'
>> sound/Kconfig:106: unexpected 'if' within menu block
>> drivers/Kconfig:239: syntax error
drivers/Kconfig:238: invalid statement
make[2]: *** [scripts/kconfig/Makefile:71: olddefconfig] Error 1
make[1]: *** [Makefile:606: olddefconfig] Error 2
make: *** [Makefile:185: __sub-make] Error 2
make: Target 'olddefconfig' not remade because of errors.
vim +/if +258 sound/pci/hda/Kconfig
243
244 config SND_HDA_INTEL_HDMI_SILENT_STREAM
245 bool "Enable Silent Stream always for HDMI"
246 depends on SND_HDA_INTEL
247 help
248 Intel hardware has a feature called 'silent stream', that
249 keeps external HDMI receiver's analog circuitry powered on
250 avoiding 2-3 sec silence during playback start. This mechanism
251 relies on an info packet and preventing the codec from going to
252 D3. (increasing the platform static power consumption when a
253 HDMI receiver is plugged-in). 2-3 sec silence at the playback
254 start is expected whenever there is format change. (default is
255 2 channel format).
256 Say Y to enable Silent Stream feature.
257
> 258 endif
259
> 260 endmenu
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-07-01 9:03 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-30 0:23 [PATCH v2] ALSA: hda/hdmi: Add Intel silent stream support Harsha Priya
2020-06-30 11:58 ` Kai Vehmanen
2020-07-01 9:02 ` kernel test robot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).