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