| Message ID | 1592954796-12449-1-git-send-email-harshapriya.n@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ALSA: hda/hdmi: Add Intel silent stream support | expand |
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-rc2 next-20200623]
[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/20200624-073425
base: https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
config: i386-tinyconfig
compiler: gcc-9 (Debian 9.3.0-13) 9.3.0
reproduce (this is a W=1 build):
make W=1 ARCH=i386 tinyconfig
make W=1 ARCH=i386
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.
--
>> 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[5]: *** [scripts/kconfig/Makefile:71: allnoconfig] Error 1
make[4]: *** [Makefile:606: allnoconfig] Error 2
make[3]: *** [Makefile:336: __build_one_by_one] Error 2
make[3]: Target 'allnoconfig' not remade because of errors.
make[3]: Target 'tiny.config' not remade because of errors.
make[2]: *** [scripts/kconfig/Makefile:109: tinyconfig] Error 2
make[1]: *** [Makefile:606: tinyconfig] Error 2
make: *** [Makefile:185: __sub-make] Error 2
make: Target 'tinyconfig' 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
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
2 nit-picks that I missed in previous versions of this patch, sorry: > diff --git a/sound/pci/hda/Kconfig b/sound/pci/hda/Kconfig > index 7ba542e45a3d..8804808410b3 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 nit-pick: should this be 'depends on SND_HDA_INTEL'? if not, this 'depends on SND_HDA' is redundant, already within an 'if SND_HDA' block > + 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 > + [...] > /* 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) { > + snd_hda_power_up_pm(codec); nit-pick: is there a need to test the return value? I see this in patch_hdmi.c ret = snd_hda_power_up_pm(codec); if (ret < 0 && pm_runtime_suspended(hda_codec_dev(codec))) goto out; > + silent_stream_enable(codec, per_pin); > + } else if (monitor_prev && !monitor_next) > + snd_hda_power_down_pm(codec); > + } > } > > static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll) >
> > 2 nit-picks that I missed in previous versions of this patch, sorry: > > > diff --git a/sound/pci/hda/Kconfig b/sound/pci/hda/Kconfig index > > 7ba542e45a3d..8804808410b3 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 > > nit-pick: should this be 'depends on SND_HDA_INTEL'? > if not, this 'depends on SND_HDA' is redundant, already within an 'if SND_HDA' > block ack. > > > + 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 > > + > > [...] > > > /* 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) { > > + snd_hda_power_up_pm(codec); > > nit-pick: is there a need to test the return value? I see this in patch_hdmi.c Since this parent function returns void, I could probably add a print stating the failure. > > ret = snd_hda_power_up_pm(codec); > if (ret < 0 && pm_runtime_suspended(hda_codec_dev(codec))) > goto out; > > > + silent_stream_enable(codec, per_pin); > > + } else if (monitor_prev && !monitor_next) > > + snd_hda_power_down_pm(codec); > > + } > > } > > > > static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, > > int repoll) > >
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
Dne 24. 06. 20 v 9:45 Takashi Iwai napsal(a): > 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. +1, kcontrol seems the appropriate way to control this. Jaroslav
>>> 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. > > +1, kcontrol seems the appropriate way to control this. It was my suggestion to use Kconfig+kernel parameter for simplicity/overrides. The kcontrol is a nice idea, but in practice we typically only have one card dealing with HDMI. It also doesn't have a UCM representation so would force the use of amixer and manual configs, or the UCM file would always set the mode.
On Wed, 24 Jun 2020 17:33:45 +0200, Pierre-Louis Bossart wrote: > > > >>> 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. > > > > +1, kcontrol seems the appropriate way to control this. > > It was my suggestion to use Kconfig+kernel parameter for > simplicity/overrides. > > The kcontrol is a nice idea, but in practice we typically only have > one card dealing with HDMI. Not really. There are systems with two HDMI outputs from both integrated and discrete GPUs. Most modern systems are only with hybrid graphics, though. > It also doesn't have a UCM representation > so would force the use of amixer and manual configs, or the UCM file > would always set the mode. But people usually use the distro kernels, so the situation is more or less equivalent; you'd have to adjust a module option manually if you want a different one from the default, and you'd have to be root to change it. So, rather the question is how we should provide the setup of such parameter. It's supposed to be a part of power management stuff that should be touched by either a smart PM tool or a manual override such as runtime PM setup? Or can it be seen as a more casual tuning? Takashi
On 6/24/20 11:43 AM, Takashi Iwai wrote: > On Wed, 24 Jun 2020 17:33:45 +0200, > Pierre-Louis Bossart wrote: >> >> >>>>> 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. >>> >>> +1, kcontrol seems the appropriate way to control this. >> >> It was my suggestion to use Kconfig+kernel parameter for >> simplicity/overrides. >> >> The kcontrol is a nice idea, but in practice we typically only have >> one card dealing with HDMI. > > Not really. There are systems with two HDMI outputs from both > integrated and discrete GPUs. Most modern systems are only with > hybrid graphics, though. Ok, maybe I am mistaken, in most of the HDMI issues we've seen only one HDMI source. But it's a good point that this is only supposed to be used for Intel whether it's a kernel parameter or a kcontrol shouldn't this be dependent on a PCI ID being detected and a SKYLAKE flag being set? it's my understanding that this applies from Skylake to TigerLake, not before. >> It also doesn't have a UCM representation >> so would force the use of amixer and manual configs, or the UCM file >> would always set the mode. > > But people usually use the distro kernels, so the situation is more or > less equivalent; you'd have to adjust a module option manually if you > want a different one from the default, and you'd have to be root to > change it. > > So, rather the question is how we should provide the setup of such > parameter. It's supposed to be a part of power management stuff that > should be touched by either a smart PM tool or a manual override such > as runtime PM setup? Or can it be seen as a more casual tuning? I am not aware of such tools. The only thing I know is that some of the HDaudio power settings are already controlled by kernel parameters, e.g. /etc/modprobe.d/audio_powersave.conf options snd_hda_intel power_save=1
Hey, On Wed, 24 Jun 2020, Takashi Iwai wrote: > So, rather the question is how we should provide the setup of such > parameter. It's supposed to be a part of power management stuff that > should be touched by either a smart PM tool or a manual override such > as runtime PM setup? Or can it be seen as a more casual tuning? this is fairly similar to "power_save" parameter (which has a default set via Kconfig). Enabling silent streaming comes at a power consumption cost as you will keep the audio controller powered up whenever a HDMI/DP receiver is connected (but you can get rid of the startup delay for audio that can be annoying for short audio clips like UI sounds). So depends mostly on the type of product. I'd expect system integrators who are used to modify "power_save", to also modify the default for silent_stream enable config. So in that sense using similar interface to expose the feature makes sense (so the people most likely to use the interface, are already familiar with it). Br, Kai
On Wed, 24 Jun 2020 19:05:14 +0200, Pierre-Louis Bossart wrote: > > > > On 6/24/20 11:43 AM, Takashi Iwai wrote: > > On Wed, 24 Jun 2020 17:33:45 +0200, > > Pierre-Louis Bossart wrote: > >> > >> > >>>>> 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. > >>> > >>> +1, kcontrol seems the appropriate way to control this. > >> > >> It was my suggestion to use Kconfig+kernel parameter for > >> simplicity/overrides. > >> > >> The kcontrol is a nice idea, but in practice we typically only have > >> one card dealing with HDMI. > > > > Not really. There are systems with two HDMI outputs from both > > integrated and discrete GPUs. Most modern systems are only with > > hybrid graphics, though. > > Ok, maybe I am mistaken, in most of the HDMI issues we've seen only > one HDMI source. > > But it's a good point that this is only supposed to be used for Intel > whether it's a kernel parameter or a kcontrol shouldn't this be > dependent on a PCI ID being detected and a SKYLAKE flag being set? > it's my understanding that this applies from Skylake to TigerLake, not > before. I guess we can check it from the codec ID? Change the probe function for Skylake+ codecs to patch_i915_skl_hdmi and co, and set the flag there. > >> It also doesn't have a UCM representation > >> so would force the use of amixer and manual configs, or the UCM file > >> would always set the mode. > > > > But people usually use the distro kernels, so the situation is more or > > less equivalent; you'd have to adjust a module option manually if you > > want a different one from the default, and you'd have to be root to > > change it. > > > > So, rather the question is how we should provide the setup of such > > parameter. It's supposed to be a part of power management stuff that > > should be touched by either a smart PM tool or a manual override such > > as runtime PM setup? Or can it be seen as a more casual tuning? > > I am not aware of such tools. The only thing I know is that some of > the HDaudio power settings are already controlled by kernel > parameters, e.g. > > /etc/modprobe.d/audio_powersave.conf > options snd_hda_intel power_save=1 Yes, it's been the primary knob for years to turn on/off the runtime PM for HD-audio and other legacy drivers. This was used by powertop or some other power-aware daemons and tools, to be toggled dynamically per the power cable state or such. And, how the silent stream feature should be seen? Should it be a system-wide root-only setup or adjustable per user? Would it be changed often? Such questions and answers will lead us to the right direction, I hope. Takashi
> On Wed, 24 Jun 2020 19:05:14 +0200, > Pierre-Louis Bossart wrote: > > > > > > > > On 6/24/20 11:43 AM, Takashi Iwai wrote: > > > On Wed, 24 Jun 2020 17:33:45 +0200, > > > Pierre-Louis Bossart wrote: > > >> > > >> > > >>>>> 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. > > >>> > > >>> +1, kcontrol seems the appropriate way to control this. > > >> > > >> It was my suggestion to use Kconfig+kernel parameter for > > >> simplicity/overrides. > > >> > > >> The kcontrol is a nice idea, but in practice we typically only have > > >> one card dealing with HDMI. > > > > > > Not really. There are systems with two HDMI outputs from both > > > integrated and discrete GPUs. Most modern systems are only with > > > hybrid graphics, though. > > > > Ok, maybe I am mistaken, in most of the HDMI issues we've seen only > > one HDMI source. > > > > But it's a good point that this is only supposed to be used for Intel > > whether it's a kernel parameter or a kcontrol shouldn't this be > > dependent on a PCI ID being detected and a SKYLAKE flag being set? > > it's my understanding that this applies from Skylake to TigerLake, not > > before. > > I guess we can check it from the codec ID? Change the probe function for > Skylake+ codecs to patch_i915_skl_hdmi and co, and set the flag there. > > > >> It also doesn't have a UCM representation so would force the use of > > >> amixer and manual configs, or the UCM file would always set the > > >> mode. > > > > > > But people usually use the distro kernels, so the situation is more > > > or less equivalent; you'd have to adjust a module option manually if > > > you want a different one from the default, and you'd have to be root > > > to change it. > > > > > > So, rather the question is how we should provide the setup of such > > > parameter. It's supposed to be a part of power management stuff > > > that should be touched by either a smart PM tool or a manual > > > override such as runtime PM setup? Or can it be seen as a more casual > tuning? > > > > I am not aware of such tools. The only thing I know is that some of > > the HDaudio power settings are already controlled by kernel > > parameters, e.g. > > > > /etc/modprobe.d/audio_powersave.conf > > options snd_hda_intel power_save=1 > > Yes, it's been the primary knob for years to turn on/off the runtime PM for HD- > audio and other legacy drivers. This was used by powertop or some other > power-aware daemons and tools, to be toggled dynamically per the power > cable state or such. > > And, how the silent stream feature should be seen? > Should it be a system-wide root-only setup or adjustable per user? > Would it be changed often? Such questions and answers will lead us to the > right direction, I hope. I think this feature should not be adjustable by the user during runtime because, a) It's based on the platform and OEM preference of having it (given it has power implications) b) Changing it on the runtime will cause the issue of unbalanced power up/down sequence like you mentioned > > > Takashi
+pulseaudio-discuss for information On Wed, 24 Jun 2020, at 1:33 PM, Takashi Iwai wrote: > On Wed, 24 Jun 2020 19:05:14 +0200, > Pierre-Louis Bossart wrote: > > > > > > > > On 6/24/20 11:43 AM, Takashi Iwai wrote: > > > On Wed, 24 Jun 2020 17:33:45 +0200, > > > Pierre-Louis Bossart wrote: > > >> > > >> > > >>>>> 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. > > >>> > > >>> +1, kcontrol seems the appropriate way to control this. > > >> > > >> It was my suggestion to use Kconfig+kernel parameter for > > >> simplicity/overrides. > > >> > > >> The kcontrol is a nice idea, but in practice we typically only have > > >> one card dealing with HDMI. > > > > > > Not really. There are systems with two HDMI outputs from both > > > integrated and discrete GPUs. Most modern systems are only with > > > hybrid graphics, though. > > > > Ok, maybe I am mistaken, in most of the HDMI issues we've seen only > > one HDMI source. > > > > But it's a good point that this is only supposed to be used for Intel > > whether it's a kernel parameter or a kcontrol shouldn't this be > > dependent on a PCI ID being detected and a SKYLAKE flag being set? > > it's my understanding that this applies from Skylake to TigerLake, not > > before. > > I guess we can check it from the codec ID? Change the probe function > for Skylake+ codecs to patch_i915_skl_hdmi and co, and set the flag > there. > > > >> It also doesn't have a UCM representation > > >> so would force the use of amixer and manual configs, or the UCM file > > >> would always set the mode. > > > > > > But people usually use the distro kernels, so the situation is more or > > > less equivalent; you'd have to adjust a module option manually if you > > > want a different one from the default, and you'd have to be root to > > > change it. > > > > > > So, rather the question is how we should provide the setup of such > > > parameter. It's supposed to be a part of power management stuff that > > > should be touched by either a smart PM tool or a manual override such > > > as runtime PM setup? Or can it be seen as a more casual tuning? > > > > I am not aware of such tools. The only thing I know is that some of > > the HDaudio power settings are already controlled by kernel > > parameters, e.g. > > > > /etc/modprobe.d/audio_powersave.conf > > options snd_hda_intel power_save=1 > > Yes, it's been the primary knob for years to turn on/off the runtime > PM for HD-audio and other legacy drivers. This was used by powertop > or some other power-aware daemons and tools, to be toggled dynamically > per the power cable state or such. > > And, how the silent stream feature should be seen? > Should it be a system-wide root-only setup or adjustable per user? > Would it be changed often? Such questions and answers will lead us to > the right direction, I hope. For audio, would UCM not be the appropriate point for a system integrator to decide how the audio device should be set up? This would allow for a choice based on the situation in which the device is actually being deployed without users having to muck around with module parameters -- maybe someone wants want this enabled for an HTPC setup, but not on a desktop connected to a monitor. -- Arun
On Wed, 24 Jun 2020 19:58:40 +0200, N, Harshapriya wrote: > > > On Wed, 24 Jun 2020 19:05:14 +0200, > > Pierre-Louis Bossart wrote: > > > > > > > > > > > > On 6/24/20 11:43 AM, Takashi Iwai wrote: > > > > On Wed, 24 Jun 2020 17:33:45 +0200, > > > > Pierre-Louis Bossart wrote: > > > >> > > > >> > > > >>>>> 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. > > > >>> > > > >>> +1, kcontrol seems the appropriate way to control this. > > > >> > > > >> It was my suggestion to use Kconfig+kernel parameter for > > > >> simplicity/overrides. > > > >> > > > >> The kcontrol is a nice idea, but in practice we typically only have > > > >> one card dealing with HDMI. > > > > > > > > Not really. There are systems with two HDMI outputs from both > > > > integrated and discrete GPUs. Most modern systems are only with > > > > hybrid graphics, though. > > > > > > Ok, maybe I am mistaken, in most of the HDMI issues we've seen only > > > one HDMI source. > > > > > > But it's a good point that this is only supposed to be used for Intel > > > whether it's a kernel parameter or a kcontrol shouldn't this be > > > dependent on a PCI ID being detected and a SKYLAKE flag being set? > > > it's my understanding that this applies from Skylake to TigerLake, not > > > before. > > > > I guess we can check it from the codec ID? Change the probe function for > > Skylake+ codecs to patch_i915_skl_hdmi and co, and set the flag there. > > > > > >> It also doesn't have a UCM representation so would force the use of > > > >> amixer and manual configs, or the UCM file would always set the > > > >> mode. > > > > > > > > But people usually use the distro kernels, so the situation is more > > > > or less equivalent; you'd have to adjust a module option manually if > > > > you want a different one from the default, and you'd have to be root > > > > to change it. > > > > > > > > So, rather the question is how we should provide the setup of such > > > > parameter. It's supposed to be a part of power management stuff > > > > that should be touched by either a smart PM tool or a manual > > > > override such as runtime PM setup? Or can it be seen as a more casual > > tuning? > > > > > > I am not aware of such tools. The only thing I know is that some of > > > the HDaudio power settings are already controlled by kernel > > > parameters, e.g. > > > > > > /etc/modprobe.d/audio_powersave.conf > > > options snd_hda_intel power_save=1 > > > > Yes, it's been the primary knob for years to turn on/off the runtime PM for HD- > > audio and other legacy drivers. This was used by powertop or some other > > power-aware daemons and tools, to be toggled dynamically per the power > > cable state or such. > > > > And, how the silent stream feature should be seen? > > Should it be a system-wide root-only setup or adjustable per user? > > Would it be changed often? Such questions and answers will lead us to the > > right direction, I hope. > I think this feature should not be adjustable by the user during runtime because, > a) It's based on the platform and OEM preference of having it (given it has power implications) Well, the argument isn't equivalent with runtime PM. The runtime PM is supposed to keep the almost same functionality, hence it's preferred to be enabled unless you hit a clear demerit (e.g. click noise or such). But this one is rather a trade-off between power-save vs avoiding the drop of first samples. The problem is that this feature blocks the whole runtime PM, even if you don't use HDMI audio. From that point, I guess many users prefer this off for those who don't use HDMI audio. > b) Changing it on the runtime will cause the issue of unbalanced power up/down sequence like you mentioned This should be no problem if we do code right :) thanks, Takashi
On Thu, 25 Jun 2020 02:18:58 +0200, Arun Raghavan wrote: > > +pulseaudio-discuss for information > > On Wed, 24 Jun 2020, at 1:33 PM, Takashi Iwai wrote: > > On Wed, 24 Jun 2020 19:05:14 +0200, > > Pierre-Louis Bossart wrote: > > > > > > > > > > > > On 6/24/20 11:43 AM, Takashi Iwai wrote: > > > > On Wed, 24 Jun 2020 17:33:45 +0200, > > > > Pierre-Louis Bossart wrote: > > > >> > > > >> > > > >>>>> 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. > > > >>> > > > >>> +1, kcontrol seems the appropriate way to control this. > > > >> > > > >> It was my suggestion to use Kconfig+kernel parameter for > > > >> simplicity/overrides. > > > >> > > > >> The kcontrol is a nice idea, but in practice we typically only have > > > >> one card dealing with HDMI. > > > > > > > > Not really. There are systems with two HDMI outputs from both > > > > integrated and discrete GPUs. Most modern systems are only with > > > > hybrid graphics, though. > > > > > > Ok, maybe I am mistaken, in most of the HDMI issues we've seen only > > > one HDMI source. > > > > > > But it's a good point that this is only supposed to be used for Intel > > > whether it's a kernel parameter or a kcontrol shouldn't this be > > > dependent on a PCI ID being detected and a SKYLAKE flag being set? > > > it's my understanding that this applies from Skylake to TigerLake, not > > > before. > > > > I guess we can check it from the codec ID? Change the probe function > > for Skylake+ codecs to patch_i915_skl_hdmi and co, and set the flag > > there. > > > > > >> It also doesn't have a UCM representation > > > >> so would force the use of amixer and manual configs, or the UCM file > > > >> would always set the mode. > > > > > > > > But people usually use the distro kernels, so the situation is more or > > > > less equivalent; you'd have to adjust a module option manually if you > > > > want a different one from the default, and you'd have to be root to > > > > change it. > > > > > > > > So, rather the question is how we should provide the setup of such > > > > parameter. It's supposed to be a part of power management stuff that > > > > should be touched by either a smart PM tool or a manual override such > > > > as runtime PM setup? Or can it be seen as a more casual tuning? > > > > > > I am not aware of such tools. The only thing I know is that some of > > > the HDaudio power settings are already controlled by kernel > > > parameters, e.g. > > > > > > /etc/modprobe.d/audio_powersave.conf > > > options snd_hda_intel power_save=1 > > > > Yes, it's been the primary knob for years to turn on/off the runtime > > PM for HD-audio and other legacy drivers. This was used by powertop > > or some other power-aware daemons and tools, to be toggled dynamically > > per the power cable state or such. > > > > And, how the silent stream feature should be seen? > > Should it be a system-wide root-only setup or adjustable per user? > > Would it be changed often? Such questions and answers will lead us to > > the right direction, I hope. > > For audio, would UCM not be the appropriate point for a system integrator to decide how the audio device should be set up? > > This would allow for a choice based on the situation in which the device is actually being deployed without users having to muck around with module parameters -- maybe someone wants want this enabled for an HTPC setup, but not on a desktop connected to a monitor. Right, that's my concern. Many users with HDMI monitor that is capable of audio don't use HDMI audio because they don't need it and/or the output sucks. For them, this feature is superfluous and harmful from the runtime PM POV. If it were provided via UCM, would it be yet another UCM profile like HDMI+silentstream? This can be confusing, too, I'm afraid. From the interface POV, as Kai suggested in another mail, the analogy to power_save option makes sense. OTOH, power_save is the knob that is better to be enabled (as long as it works), silent stream is the feature that is needed only when required. So it comes to the question which interface is easier to manage. thanks, Takashi
On Thu, 2020-06-25 at 09:03 +0200, Takashi Iwai wrote: > On Thu, 25 Jun 2020 02:18:58 +0200, > Arun Raghavan wrote: > > +pulseaudio-discuss for information > > > > On Wed, 24 Jun 2020, at 1:33 PM, Takashi Iwai wrote: > > > On Wed, 24 Jun 2020 19:05:14 +0200, > > > Pierre-Louis Bossart wrote: > > > > > > > > > > > > On 6/24/20 11:43 AM, Takashi Iwai wrote: > > > > > On Wed, 24 Jun 2020 17:33:45 +0200, > > > > > Pierre-Louis Bossart wrote: > > > > > > It also doesn't have a UCM representation > > > > > > so would force the use of amixer and manual configs, or the UCM file > > > > > > would always set the mode. > > > > > > > > > > But people usually use the distro kernels, so the situation is more or > > > > > less equivalent; you'd have to adjust a module option manually if you > > > > > want a different one from the default, and you'd have to be root to > > > > > change it. > > > > > > > > > > So, rather the question is how we should provide the setup of such > > > > > parameter. It's supposed to be a part of power management stuff that > > > > > should be touched by either a smart PM tool or a manual override such > > > > > as runtime PM setup? Or can it be seen as a more casual tuning? > > > > > > > > I am not aware of such tools. The only thing I know is that some of > > > > the HDaudio power settings are already controlled by kernel > > > > parameters, e.g. > > > > > > > > /etc/modprobe.d/audio_powersave.conf > > > > options snd_hda_intel power_save=1 > > > > > > Yes, it's been the primary knob for years to turn on/off the runtime > > > PM for HD-audio and other legacy drivers. This was used by powertop > > > or some other power-aware daemons and tools, to be toggled dynamically > > > per the power cable state or such. > > > > > > And, how the silent stream feature should be seen? > > > Should it be a system-wide root-only setup or adjustable per user? > > > Would it be changed often? Such questions and answers will lead us to > > > the right direction, I hope. > > > > For audio, would UCM not be the appropriate point for a system > > integrator to decide how the audio device should be set up? > > > > This would allow for a choice based on the situation in which the > > device is actually being deployed without users having to muck > > around with module parameters -- maybe someone wants want this > > enabled for an HTPC setup, but not on a desktop connected to a > > monitor. Is UCM really an appropriate place for deciding the setting? The default should be to disable the feature, and if that is done in UCM, how is the user expected to enable it when needed? I'm not aware of an easy way to tweak the UCM configuration (modifying distro-provided files is not good). I don't really get the talk about system integrators. This seems like an end-user setting to me. > Right, that's my concern. Many users with HDMI monitor that is > capable of audio don't use HDMI audio because they don't need it > and/or the output sucks. For them, this feature is superfluous and > harmful from the runtime PM POV. > > If it were provided via UCM, would it be yet another UCM profile like > HDMI+silentstream? This can be confusing, too, I'm afraid. > > From the interface POV, as Kai suggested in another mail, the > analogy to power_save option makes sense. OTOH, power_save is the > knob that is better to be enabled (as long as it works), silent stream > is the feature that is needed only when required. So it comes to the > question which interface is easier to manage. Having a separate UCM "profile" (do you mean a verb or a device?) seems overkill. I think there could be a device-specific variable, like SilentStreamControl, which would indicate that the device supports the silent stream feature. The variable would also point to the mixer control for enabling it. That said, I don't see much need for involving UCM at all. UCM becomes more relevant if we want PulseAudio to provide an API for controlling this feature, but until that happens, just having a mixer control that users can toggle seems sufficient to me. (I'm assuming that ALSA remembers mixer settings between boots. I'm not sure if that's the case, but I have the impression that the alsa-state thingy is universally enabled and implements this.)
>>>>> So, rather the question is how we should provide the setup of such >>>>> parameter. It's supposed to be a part of power management stuff that >>>>> should be touched by either a smart PM tool or a manual override such >>>>> as runtime PM setup? Or can it be seen as a more casual tuning? >>>> >>>> I am not aware of such tools. The only thing I know is that some of >>>> the HDaudio power settings are already controlled by kernel >>>> parameters, e.g. >>>> >>>> /etc/modprobe.d/audio_powersave.conf >>>> options snd_hda_intel power_save=1 >>> >>> Yes, it's been the primary knob for years to turn on/off the runtime >>> PM for HD-audio and other legacy drivers. This was used by powertop >>> or some other power-aware daemons and tools, to be toggled dynamically >>> per the power cable state or such. >>> >>> And, how the silent stream feature should be seen? >>> Should it be a system-wide root-only setup or adjustable per user? >>> Would it be changed often? Such questions and answers will lead us to >>> the right direction, I hope. >> >> For audio, would UCM not be the appropriate point for a system integrator to decide how the audio device should be set up? >> >> This would allow for a choice based on the situation in which the device is actually being deployed without users having to muck around with module parameters -- maybe someone wants want this enabled for an HTPC setup, but not on a desktop connected to a monitor. > > Right, that's my concern. Many users with HDMI monitor that is > capable of audio don't use HDMI audio because they don't need it > and/or the output sucks. For them, this feature is superfluous and > harmful from the runtime PM POV. > > If it were provided via UCM, would it be yet another UCM profile like > HDMI+silentstream? This can be confusing, too, I'm afraid. Unless I am mistaken, this silent stream would be applicable to the legacy HDaudio driver, as well as SOF. UCM is not used for the legacy HDaudio case, so that would close the door on UCM-based configurations, no? > From the interface POV, as Kai suggested in another mail, the > analogy to power_save option makes sense. OTOH, power_save is the > knob that is better to be enabled (as long as it works), silent stream > is the feature that is needed only when required. So it comes to the > question which interface is easier to manage.
Dne 25. 06. 20 v 16:46 Pierre-Louis Bossart napsal(a): > > > >>>>>> So, rather the question is how we should provide the setup of such >>>>>> parameter. It's supposed to be a part of power management stuff that >>>>>> should be touched by either a smart PM tool or a manual override such >>>>>> as runtime PM setup? Or can it be seen as a more casual tuning? >>>>> >>>>> I am not aware of such tools. The only thing I know is that some of >>>>> the HDaudio power settings are already controlled by kernel >>>>> parameters, e.g. >>>>> >>>>> /etc/modprobe.d/audio_powersave.conf >>>>> options snd_hda_intel power_save=1 >>>> >>>> Yes, it's been the primary knob for years to turn on/off the runtime >>>> PM for HD-audio and other legacy drivers. This was used by powertop >>>> or some other power-aware daemons and tools, to be toggled dynamically >>>> per the power cable state or such. >>>> >>>> And, how the silent stream feature should be seen? >>>> Should it be a system-wide root-only setup or adjustable per user? >>>> Would it be changed often? Such questions and answers will lead us to >>>> the right direction, I hope. >>> >>> For audio, would UCM not be the appropriate point for a system integrator to decide how the audio device should be set up? >>> >>> This would allow for a choice based on the situation in which the device is actually being deployed without users having to muck around with module parameters -- maybe someone wants want this enabled for an HTPC setup, but not on a desktop connected to a monitor. >> >> Right, that's my concern. Many users with HDMI monitor that is >> capable of audio don't use HDMI audio because they don't need it >> and/or the output sucks. For them, this feature is superfluous and >> harmful from the runtime PM POV. > >> If it were provided via UCM, would it be yet another UCM profile like >> HDMI+silentstream? This can be confusing, too, I'm afraid. > > Unless I am mistaken, this silent stream would be applicable to the > legacy HDaudio driver, as well as SOF. > > UCM is not used for the legacy HDaudio case, so that would close the > door on UCM-based configurations, no? UCM can be used for legacy HDA, too (and it is used for some legacy HDA models - dual codecs). It seems that it's better to describe the "abstract" device layout for users on the one place. My plan is to migrate to UCM completely at some day when the major issues are resolved. It may be a bit challenge for the legacy HDA driver and USB devices but doable. Jaroslav
On 6/25/20 10:30 AM, Jaroslav Kysela wrote: > Dne 25. 06. 20 v 16:46 Pierre-Louis Bossart napsal(a): >> >> >> >>>>>>> So, rather the question is how we should provide the setup of such >>>>>>> parameter. It's supposed to be a part of power management stuff >>>>>>> that >>>>>>> should be touched by either a smart PM tool or a manual override >>>>>>> such >>>>>>> as runtime PM setup? Or can it be seen as a more casual tuning? >>>>>> >>>>>> I am not aware of such tools. The only thing I know is that some of >>>>>> the HDaudio power settings are already controlled by kernel >>>>>> parameters, e.g. >>>>>> >>>>>> /etc/modprobe.d/audio_powersave.conf >>>>>> options snd_hda_intel power_save=1 >>>>> >>>>> Yes, it's been the primary knob for years to turn on/off the runtime >>>>> PM for HD-audio and other legacy drivers. This was used by powertop >>>>> or some other power-aware daemons and tools, to be toggled dynamically >>>>> per the power cable state or such. >>>>> >>>>> And, how the silent stream feature should be seen? >>>>> Should it be a system-wide root-only setup or adjustable per user? >>>>> Would it be changed often? Such questions and answers will lead us to >>>>> the right direction, I hope. >>>> >>>> For audio, would UCM not be the appropriate point for a system >>>> integrator to decide how the audio device should be set up? >>>> >>>> This would allow for a choice based on the situation in which the >>>> device is actually being deployed without users having to muck >>>> around with module parameters -- maybe someone wants want this >>>> enabled for an HTPC setup, but not on a desktop connected to a monitor. >>> >>> Right, that's my concern. Many users with HDMI monitor that is >>> capable of audio don't use HDMI audio because they don't need it >>> and/or the output sucks. For them, this feature is superfluous and >>> harmful from the runtime PM POV. > >>> If it were provided via UCM, would it be yet another UCM profile like >>> HDMI+silentstream? This can be confusing, too, I'm afraid. >> >> Unless I am mistaken, this silent stream would be applicable to the >> legacy HDaudio driver, as well as SOF. >> >> UCM is not used for the legacy HDaudio case, so that would close the >> door on UCM-based configurations, no? > > UCM can be used for legacy HDA, too (and it is used for some legacy HDA > models - dual codecs). It seems that it's better to describe the > "abstract" device layout for users on the one place. Did you mean represent the form-factor in UCM? Currently we use e.g. the same bytcr-rt5640 UCM file for tablets, notebooks and headless devices (mini desktops). UCM would need to get the information from somewhere else, I don't think it'd be practical to hard-code form-factors in UCM proper. > My plan is to migrate to UCM completely at some day when the major > issues are resolved. It may be a bit challenge for the legacy HDA driver > and USB devices but doable. For USB I don't know how this would work. USB report descriptors, if you wanted to represent all possible cases with UCM you'd need to build the union of all possible cases and make parts conditional.
diff --git a/sound/pci/hda/Kconfig b/sound/pci/hda/Kconfig index 7ba542e45a3d..8804808410b3 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 + 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..52bb81c952af 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 receivers"); + struct hdmi_spec_per_cvt { hda_nid_t cvt_nid; int assigned; @@ -1634,21 +1639,50 @@ 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) { + if (!monitor_prev && monitor_next) { + snd_hda_power_up_pm(codec); + silent_stream_enable(codec, per_pin); + } else if (monitor_prev && !monitor_next) + snd_hda_power_down_pm(codec); + } } static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)